Remove DEV_MODE and USE_FAKE_FXA_AUTH, replace with PROD_MODE and FXA_CONFIG #23049
Conversation
KevinMind
requested review from
a team and
diox
and removed request for
a team
KevinMind
force-pushed
the
fake-fxa-auth
branch
4 times, most recently
from
0928b7f to
2731dee
Compare
…_CONFIG This commit introduces several changes to simplify and improve the authentication and environment configuration: - Replaced `DEV_MODE` with `PROD_MODE` derived from version JSON instead of environment variable - Removed `USE_FAKE_FXA_AUTH` setting replaced with `FXA_CONFIG` to control fake authentication - Simplified conditional logic around development and production environments to rely on stable build time variables
KevinMind
force-pushed
the
fake-fxa-auth
branch
from
2731dee to
22fbf75
Compare
src/olympia/amo/utils.py
Outdated
| """Return whether or not to use a fake FxA server for authentication. | ||
| Should always return False in production""" | ||
| return settings.DEV_MODE and settings.USE_FAKE_FXA_AUTH | ||
| return config.get('client_id') == '.' |
There was a problem hiding this comment.
The main benefit of the explicit settings checks was to avoid accidentally having fake FxA enabled on dev/stage/prod if an environnement variable wasn't set properly (something we can't easily see/test/control ourselves until after deployment).
We should at least keep something like USE_FAKE_FXA_AUTH, change it ALLOW_FAKE_FXA_AUTH and use it here on top of the check on client_id being . to keep the same guarantee.
There was a problem hiding this comment.
I was thinking about a Django system check that raises if those values are not set in production. Wdyt?
There was a problem hiding this comment.
But how would that check pass on local envs then ? You'd end up needing a similar mechanism to special case local envs ?
There was a problem hiding this comment.
I was actually thinking we could have a django check that ensures those values are not '.' in production. Would that also solve the issue? Production pods would just fail if they start without proper auth configured.
There was a problem hiding this comment.
I found a potential compromise. we can re-use the existing ENV setting to check if we are running on a "local" environment or not and only allow fxa if both ENV == 'local' and FXA_CONFIG is as expected.
I've had bad experiences previously trying to add "custom" flags in settings. I think we should aim for a narrow waist and have a setting that tells us the information we actually care about "are you in prod or not" in a way that is clear and doesn't have any explicit meaning about how that information should be used.
Otherwise we end up having many of these bespoke flags that all should agree with each other but then become impossible to coordinate correctly.
…lidated runtime environment variable ENV
KevinMind
force-pushed
the
fake-fxa-auth
branch
from
b1c777a to
26f73ed
Compare
KevinMind
force-pushed
the
fake-fxa-auth
branch
from
0f299b6 to
701f278
Compare
src/olympia/urls.py
Outdated
| def serve_static_files(request, path, **kwargs): | ||
| if settings.TARGET == 'production': | ||
| return serve_static(request, path, document_root=settings.STATIC_ROOT, **kwargs) | ||
| else: | ||
| return static_serve(request, path, insecure=True, show_indexes=True, **kwargs) | ||
|
|
||
|
|
||
| def serve_javascript_catalog(request, locale, **kwargs): | ||
| with translation.override(locale): | ||
| return JavaScriptCatalog.as_view()(request, locale, **kwargs) |
There was a problem hiding this comment.
I don't think we want these to be available on "real" environment - so why not use the same trick as for FxA and only make them available if ENV == 'local' ? Don't need to move the code around, can just replace the existing condition
There was a problem hiding this comment.
I tried that, but got failing tests. My thought was, it's not actually a problem to serve these paths as they effectively cannot be reached in production. and even if they were, that means CDN and NGINX could not serve the file... so wouldn't serving it here actually be an improvement?
If you feel strongly I'll add a gate but it would have to be that we register the URLS and then 404 if env !== local.
When testing this with the gate at the register level the "override_settings" is applied too late so it doesn't behave as expected unless you pass the environment variable when executing the test. That felt like overkill...
Co-authored-by: Mathieu Pillard <[email protected]>
|
Pausing this for now. Other higher priorities. |
Fixes: mozilla/addons#15331
Description
This commit introduces several changes to simplify and improve the authentication and environment configuration:
DEV_MODEwithPROD_MODEderived from version JSON instead of environment variableUSE_FAKE_FXA_AUTHsetting replaced withFXA_CONFIGto control fake authenticationContext
This patch simplifies the configuration of "prod" versus "dev" mode by relying on build time parameters that reliably inform what kind of server to run. Environment variables can be overwritten and have no direct connection to the build, but get_version_json can tell you exactly waht kind of image you are running.
Additionally, sometimes you want to run in prod mode, but don't want to setup a real authentication server. Now you can do that because we use a more granular check for fake FXA based on the presence of valid fxa credentials at runtime.
Testing
Dev mode
Run in dev mode
App should run in dev mode with debug toolbar and non compiled assets
Prod mode
Run in prod mode
Expect compiled assets, no debug toolbar (unless DEBUG=True is explicitly set)
and finally, fake fxa should still be used by default.
Real auth
To run with real auth you must set the environment variables
and run in either dev or prod mode. Try both.
Checklist
#ISSUENUMat the top of your PR to an existing open issue in the mozilla/addons repository.