-
Notifications
You must be signed in to change notification settings - Fork 0
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
🔧 [open-zaak/open-zaak#1629] Add generic base settings file #19
Conversation
6fa81c4
to
011c377
Compare
997a6d7
to
9e24806
Compare
eaa69f3
to
5a05d60
Compare
5a05d60
to
da4d2b6
Compare
if config("DISABLE_2FA", default=False): # pragma: no cover | ||
MAYKIN_2FA_ALLOW_MFA_BYPASS_BACKENDS = AUTHENTICATION_BACKENDS |
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.
Not 100% sure if we want this here, OF only has this in dev.py
, but that way you wouldn't be able to disable 2FA for docker containers locally for example.
@annashamray thoughts?
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.
We discussed it with Sjoerd for Objects API and he said that some test instances of Objects API and Objecttypes API don't use 2FA, therefore we added it into base.py
Since we don't have different docker images for test and prod environments, I think it would be nice to be able to disable it
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.
Alright, then I'll leave it in there 👍
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.
Could you add the list of env vars supported by this lib?
Maybe we could have RTD published in the future and link it in other API docs
open_api_framework/conf/base.py
Outdated
# Build paths inside the project, so further paths can be defined relative to | ||
# the code root. | ||
DJANGO_PROJECT_DIR = get_django_project_dir() | ||
BASE_DIR = os.path.abspath( |
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.
Should we move from os.path
to Pathlib
?
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.
Seems like a good idea 👍
"django_admin_index", | ||
"django.contrib.admin", | ||
# External applications. | ||
"axes", |
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.
Should the apps be ordered alphabetically within semantic groups? It would be neater, but we can live without it
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.
I could do this, but I'm not sure if order matters when it comes to these apps (two_factor
and maykin_2fa
for instance)
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.
Then let's leave it as it is
@annashamray I left this out of scope for this PR, I want to tackle this as part of open-zaak/open-zaak#1649 by generating documentation from our usage of |
* remove unused lines * replace os usage with pathlib
bb5b738
to
985ff2a
Compare
Related issue: open-zaak/open-zaak#1629
A first attempt at making a generic
base.py
settings file. I used the settings from Open Zaak as a template and implemented it first in Objecttypes: maykinmedia/objecttypes-api#102. Thisbase.py
will be extended as I add this to the other projects.