feat(`validate_locale_configuration`): discard and warn on configured but unsupported locale #7443

cfm · 2025-02-20T00:19:24Z

Status

Ready for review

Description of Changes

Closes #7442: If validate_locale_configuration() doesn't check the authoritative list of supported locales in the i18n.json file installed by securedrop-app-code, then a language for which support has been revoked won't be disabled until config.SUPPORTED_LOCALES is updated on the next securedrop-admin {sdconfig,install} run.

This isn't a bug; it was simply out of scope of #6557. Now #7442 motivates us to at least consider: Do we want an unsupported language to be disabled immediately on upgrade of securedrop-app-code, not just on the next securedrop-admin {sdconfig,install}? If yes, then this diff handles this case. If not, then we can just close this pull request.

Testing

Under make dev with:

--- a/securedrop/bin/dev-config
+++ b/securedrop/bin/dev-config
@@ -26,7 +26,7 @@ with open('securedrop/config.py', 'w') as f:
     text = env.get_template("securedrop/config.py.example").render(ctx)
     text += '\n'
     supported_locales = subprocess.check_output(['make', '--quiet', 'supported-locales']).decode().strip()
-    text += f'SUPPORTED_LOCALES = {supported_locales}\n'
+    text += f'SUPPORTED_LOCALES = {"de_DE", "ro", "en_US"}\n'
     f.write(text)
 
 with open('securedrop/rq_config.py', 'w') as f:
--- a/securedrop/i18n.json
+++ b/securedrop/i18n.json
@@ -63,10 +63,6 @@
       "name": "Portuguese, Portugal",
       "desktop": "pt_PT"
     },
-    "ro": {
-      "name": "Romanian",
-      "desktop": "ro"
-    },
     "ru": {
       "name": "Russian",
       "desktop": "ru"

Observe on startup:

[2025-02-19 23:51:46,846] WARNING in i18n: Configured locales {Locale('ro')} are not in the set of usable locales {Locale('de', territory='DE'), Locale('en', territory='US')}

Romanian is not offered in the locale switcher
/?l=ro defaults to en_US

Deployment

As above:

Do we want an unsupported language to be disabled immediately on upgrade of securedrop-app-code, not just on the next securedrop-admin {sdconfig,install}?
~~Do we want an error to be logged and an OSSEC alert to be sent in this case?~~
- Release QA should confirm that the warning (downgraded from error) does not trigger an OSSEC alert.

Checklist

If you made changes to the server application code:

Linting (make lint) and tests (make test) pass in the development container

NB. This is already covered at the implementation (as opposed to configuration) level by test_valid_but_unusable_locales().

If you made changes to `securedrop-admin`:

Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

Configuration tests pass

If you added or removed a file deployed with the application:

I have updated AppArmor rules to include the change

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

Choose one of the following:

I have opened a PR in the docs repo for these changes, or will do so later
I would appreciate help with the documentation
These changes do not require documentation

…ut unsupported locale If validate_locale_configuration() doesn't check the authoritative list of supported locales in the "i18n.json" file installed by securedrop-app-code, then a language for which support has been revoked won't be disabled until config.SUPPORTED_LOCALES is updated on the next "securedrop-admin {sdconfig,install}" run.

cfm · 2025-02-20T01:46:42Z

Noting for the record (from way back in 9c11950): Configured locales [...] are not in the set of usable locales [...] is probably a warning, not an error, but the decision then was that we did want this case to trigger an OSSEC alert.

cfm · 2025-02-20T02:13:36Z

Clarified out of band with @zenmonkeykstop: We can just close this pull request if we don't care about this disablement case—or defer to v2.13.

legoktm · 2025-02-20T15:40:50Z

IMO the ideal behavior would be:

any language we disable, is disabled on all SDs that have it explicitly enabled.
if you're using ./securedrop-admin sdconfig/install, an unsupported language is an error
the existing server configuration is not an OSSEC alert, and if/when Hindi and Romanian are supported again, those servers will automatically re-enable the language without any manual interaction needed.

Would that be reasonable?

Also, I'm not sure where in your plan it was, but can we also disable hi/ro before the rc is cut so that behavior can be QA'd at the same time?

securedrop/i18n.py

legoktm · 2025-02-20T15:42:17Z

securedrop/i18n.py

+        with open(I18N_CONF) as i18n_conf_file:
+            i18n_conf = json.load(i18n_conf_file)
+        supported = parse_locale_set(i18n_conf["supported_locales"].keys())
+    except FileNotFoundError:


In what case would the file not be found? If i18n.json is missing, that feels like a fatal error to me.

Clarified in ef98aa0. Let me know if this doesn't address your concern.

Sorry, but why would it not be available under test? Is something deleting it?

2b1214c has a green check. I'll turn it into a PR, but we don't need to get it into 2.12.

If I had to guess, if it was failing earlier possibly because the path was relative, but now that you changed it to an absolute path, it works?

nathandyer · 2025-02-20T16:11:44Z

I agree with @legoktm's ideal scenario above, and disabling a removed language on update rather than running ./securedrop-admin feels like the most straightforward / discoverable path. By which I mean, if I were an admin and the language suddenly reverted to a different language, I would be able to see there was an upgrade and find the release notes.

If we do decide to go this route, I think adding a short blurb in our docs saying "in the event that you are using a language that is no longer supported, your SecureDrop will disable it during the upgrade process for that release" (or similar).

cfm · 2025-02-21T05:42:08Z

Thanks both. Replying to @legoktm in #7443 (comment):

IMO the ideal behavior would be:

any language we disable, is disabled on all SDs that have it explicitly enabled.

if you're using ./securedrop-admin sdconfig/install, an unsupported language is an error

the existing server configuration is not an OSSEC alert

and if/when Hindi and Romanian are supported again, those servers will automatically re-enable the language without any manual interaction needed.

Would that be reasonable?

Very reasonable, and I've done some slicing and checklisting here to show what we have currently (checked) and as of this pull request (checked and in bold).

Changing from error (with OSSEC alert) to warning (no OSSEC alert) should be a one-line change I'll tack on tomorrow along with responding to your review feedback.

cfm · 2025-02-21T15:41:01Z

As of e50ead6:

any language we disable, is disabled on all SDs that have it explicitly enabled.

if you're using ./securedrop-admin sdconfig/install, an unsupported language is an error

the existing server configuration is not an OSSEC alert

and if/when Hindi and Romanian are supported again, those servers will automatically re-enable the language without any manual interaction needed.

…under test

…age from error to warning ---primarily to avoid triggering OSSEC alerts.

CR addressed via inline docs.

zenmonkeykstop

Change and test plan LGTM - this should likely have an additional test case but that can happen via a subsequent PR.

This operation should never fail because the i18n.json file is shipped with the package and always present. Possibly the rationale of needing it for tests no longer applies since the path is now absolute and not relative to the current working directory. See <#7443 (comment)>.

cfm added the i18n Anything related to translation or internationalization of SecureDrop label Feb 20, 2025

cfm added this to the SecureDrop 2.12.0 milestone Feb 20, 2025

cfm requested a review from a team as a code owner February 20, 2025 00:19

cfm requested review from legoktm and nathandyer February 20, 2025 00:20

zenmonkeykstop self-requested a review February 20, 2025 01:33

legoktm previously requested changes Feb 20, 2025

View reviewed changes

cfm force-pushed the 7442-revoke-lang branch from ac79d45 to bda09ec Compare February 21, 2025 15:39

cfm force-pushed the 7442-revoke-lang branch from bda09ec to 7bbc6ad Compare February 21, 2025 15:42

cfm requested a review from legoktm February 21, 2025 15:42

cfm and others added 2 commits February 21, 2025 10:49

fix(validate_locale_configuration): "i18n.json" may not be available …

ef98aa0

…under test

chore(validate_locale_configuration): downgrade "missing locale" mess…

e50ead6

…age from error to warning ---primarily to avoid triggering OSSEC alerts.

cfm force-pushed the 7442-revoke-lang branch from 7bbc6ad to e50ead6 Compare February 21, 2025 15:49

cfm assigned zenmonkeykstop Feb 24, 2025

cfm mentioned this pull request Feb 24, 2025

chore: revoke support for Hindi (hi) #7446

Merged

zenmonkeykstop approved these changes Feb 24, 2025

View reviewed changes

zenmonkeykstop added this pull request to the merge queue Feb 24, 2025

Merged via the queue into develop with commit 5c1e2fa Feb 24, 2025
44 checks passed

This was referenced Feb 27, 2025

fix: allow Apache to read i18n.json #7451

Merged

i18n.json is used in app startup but not included in apache2 apparmor config. #7450

Closed

legoktm mentioned this pull request Mar 4, 2025

Remove guard around opening i18n.json #7458

Open

2 tasks

nathandyer mentioned this pull request Mar 4, 2025

Release SecureDrop 2.12.0 #7444

Open

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(`validate_locale_configuration`): discard and warn on configured but unsupported locale #7443

feat(`validate_locale_configuration`): discard and warn on configured but unsupported locale #7443

cfm commented Feb 20, 2025 •

edited

Loading

cfm commented Feb 20, 2025

cfm commented Feb 20, 2025

legoktm commented Feb 20, 2025

legoktm Feb 20, 2025

cfm Feb 21, 2025 •

edited

Loading

legoktm Mar 3, 2025

legoktm Mar 4, 2025

nathandyer commented Feb 20, 2025 •

edited

Loading

cfm commented Feb 21, 2025 •

edited

Loading

cfm commented Feb 21, 2025 •

edited

Loading

zenmonkeykstop left a comment

feat(validate_locale_configuration): discard and warn on configured but unsupported locale #7443

feat(validate_locale_configuration): discard and warn on configured but unsupported locale #7443

Conversation

cfm commented Feb 20, 2025 • edited Loading

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the server application code:

If you made changes to securedrop-admin:

If you made changes to the system configuration:

If you added or removed a file deployed with the application:

If you made non-trivial code changes:

cfm commented Feb 20, 2025

cfm commented Feb 20, 2025

legoktm commented Feb 20, 2025

legoktm Feb 20, 2025

Choose a reason for hiding this comment

cfm Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

legoktm Mar 3, 2025

Choose a reason for hiding this comment

legoktm Mar 4, 2025

Choose a reason for hiding this comment

nathandyer commented Feb 20, 2025 • edited Loading

cfm commented Feb 21, 2025 • edited Loading

cfm commented Feb 21, 2025 • edited Loading

zenmonkeykstop left a comment

Choose a reason for hiding this comment

feat(`validate_locale_configuration`): discard and warn on configured but unsupported locale #7443

feat(`validate_locale_configuration`): discard and warn on configured but unsupported locale #7443

cfm commented Feb 20, 2025 •

edited

Loading

If you made changes to `securedrop-admin`:

cfm Feb 21, 2025 •

edited

Loading

nathandyer commented Feb 20, 2025 •

edited

Loading

cfm commented Feb 21, 2025 •

edited

Loading

cfm commented Feb 21, 2025 •

edited

Loading