-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: validate SDConfig.SUPPORTED_LANGUAGES
for usable locales
#6406
Conversation
SDConfig.SUPPORTED_LANGUAGES
for *usable* localesSDConfig.SUPPORTED_LANGUAGES
for usable locales
Codecov Report
@@ Coverage Diff @@
## develop #6406 +/- ##
===========================================
- Coverage 84.02% 83.83% -0.19%
===========================================
Files 61 62 +1
Lines 4312 4331 +19
Branches 524 525 +1
===========================================
+ Hits 3623 3631 +8
- Misses 565 575 +10
- Partials 124 125 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
84ac53d
to
925fefb
Compare
Still walking through the test plan (more feedback to come shortly...), but when I switched my locale to de_DE, deleted it and restarted the dev container, I got a stack trace of:
Resetting my Tor Browser identity/session fixed it so I don't think it's a problem in practice for production installs, but flagging it as it seems to reveal another place not using/checking USABLE_LOCALES first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dev test plan passed, minus the one stacktrace I posted. I didn't go onto the staging part yet, will do that in the next round of review.
Thanks, @legoktm! I'll try to turn this around this afternoon (Pacific) in the hope of getting it under the feature-freeze wire for v2.4.0. |
A locale is considered usable if it is both (a) available in the filesystem and (b) configured by the administrator in SDConfig.SUPPORTED_LANGUAGES. Once we've determined which configured locales are actually usable, we: 1. warn if a configured locale is not available; 2. fall back to the hard-coded FALLBACK_LOCALE ("en_US") if SDConfig.DEFAULT_LOCALE is not usable; and 3. error out if neither the default nor the fallback locale is usable.
We can't iterate directly over USABLE_LOCALES: Set(Locale) because Locales aren't sortable. Instead, we parse config.SUPPORTED_LOCALES in order and skip those that aren't in USABLE_LOCALES.
… to disambiguate SUPPORTED_LOCALES thread: #6406 (comment)
34eaea2
to
bc5e366
Compare
… to disambiguate SUPPORTED_LOCALES thread: #6406 (comment)
bc5e366
to
9621639
Compare
This is a safe assumption except in the corner case where (e.g.): 1. A user begins a session and selects language L. 2. The Application Server upgrades to a version of securedrop-app-code that removes support for language L. 3. The user resumes their session, which still has language L selected. But, since this case has come up in testing[1], best to check for it. [1]: #6406 (comment)
As of 9c11950, get_locale() handled user/agent-provided locale preferences and deferred to resolve_fallback_locale() if none could be satisfied. Here we build a single list of locales, beginning with user/agent preferences and ending with the server-side {DEFAULT_FALLBACK}_LOCALEs, and call babel.core.negotiate_locale() once to negotiate one (if possible).
Thanks for catching this failure-mode. I'm not able to reproduce it, but I've added what I think is the necessary defense-in-depth check in eb2a042. If you're still able to reproduce with this change, and think it worth addressing, could you specify a more-detailed reproduction in terms of the "delete This change prompted the further refactoring proposed in eb2a042, to consolidate the locale-negotiation logic in |
Using your latest set of changes I can no longer reproduce it, so I assume it's fixed. Going through the rest of the test plan now... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on both dev and staging, LGTM!
After freedomofpress/securedrop#6557, in lieu of a Venn diagram. I regret that I did not manage to use identical terminology between freedomofpress/securedrop#6406 and freedomofpress/securedrop#6557.
After freedomofpress/securedrop#6557, in lieu of a Venn diagram. I regret that I did not manage to use identical terminology between freedomofpress/securedrop#6406 and freedomofpress/securedrop#6557.
Status
Ready for review
Description of Changes
Closes #6366 by determining that a locale is usable if it is both (a) available in the filesystem and (b) configured by the administrator in
SDConfig.SUPPORTED_LANGUAGES
. Once we've determined which configured locales are actually usable, we:warn if a configured locale is not available;
fall back to the hard-coded
FALLBACK_LOCALE
(en_US
) ifSDConfig.DEFAULT_LOCALE
is not usable; anderror out if neither the default nor the fallback locale is usable.
Testing
Routine testing
In the development environment:
zh_{Hans,Hant}
are each (separately) listed, selectable, and loadable.Configured language loses support
In the development environment:
First:
Observe the error:
[2022-04-19 22:14:24,290] ERROR in i18n: Configured locales {Locale('de', territory='DE')} are not in the set of usable locales {Locale('es', territory='ES'), Locale('is'), Locale('pt', territory='BR'), Locale('zh', script='Hans'), Locale('ro'), Locale('it', territory='IT'), Locale('en', territory='US'), Locale('ca'), Locale('nl'), Locale('fr', territory='FR'), Locale('el'), Locale('sv'), Locale('ar'), Locale('cs'), Locale('ru'), Locale('hi'), Locale('nb', territory='NO'), Locale('tr'), Locale('zh', script='Hant'), Locale('sk')}
In the Source Interface, observe that Deutsch is no longer available in the language menu.
Default language loses support
In the development environment:
Apply the patch:
Then:
Observe the error:
[2022-04-19 22:14:24,290] ERROR in i18n: Configured locales {Locale('de', territory='DE')} are not in the set of usable locales {Locale('es', territory='ES'), Locale('is'), Locale('pt', territory='BR'), Locale('zh', script='Hans'), Locale('ro'), Locale('it', territory='IT'), Locale('en', territory='US'), Locale('ca'), Locale('nl'), Locale('fr', territory='FR'), Locale('el'), Locale('sv'), Locale('ar'), Locale('cs'), Locale('ru'), Locale('hi'), Locale('nb', territory='NO'), Locale('tr'), Locale('zh', script='Hant'), Locale('sk')}
In the Source Interface, observe that Deutsch is no longer available in the language menu.
No usable fallback locale
In the development environment:
Apply the patch:
Then:
Observe the crash:
OSSEC alerts
In the staging environment:
On the Monitor Server,
tail -f /var/ossec/logs/alerts/alerts.log
.On the Application Server:
On the Monitor Server, observe an alert like:
Deployment
This is a disruptive change for an instance X if:
DEFAULT_LANGUAGE
.This is a breaking change for X if, in addition:
en_US
has been removed from X'sSUPPORTED_LANGUAGES
.A report of instances'
supported_languages
from the Directory should tell us whether any instances will need proactive outreach about either of these scenarios. Otherwise, we should make sure to warn about them in the release notes.Checklist
If you made changes to the server application code:
make lint
) and tests (make test
) pass in the development containerIf you made non-trivial code changes:
Choose one of the following: