✨ [#2263] Setup configuration steps for authentication #1137
Conversation
stevenbal
force-pushed
the
feature/2263-setup-config-auth
branch
5 times, most recently
from
4c5e48a
to
c0ff93f
Compare
stevenbal
force-pushed
the
feature/2263-setup-config-auth
branch
2 times, most recently
from
10a4e55
to
94b94b6
Compare
stevenbal
force-pushed
the
feature/2263-setup-config-auth
branch
4 times, most recently
from
2b73dc5
to
9058681
Compare
stevenbal
changed the title
| for setting in self.all_settings: | ||
| value = getattr(settings, setting, None) | ||
| if value is not None: | ||
| model_field_name = setting.split("DIGID_OIDC_")[1].lower() |
There was a problem hiding this comment.
I rely on the name being prefixed by DIGID_OIDC_ and the rest of the variable name being identical to the name of the model field. Some fields on the config start with oidc_, that's why some settings contain OIDC_ twice (e.g. DIGID_OIDC_OIDC_RP_CLIENT_ID).
stevenbal
force-pushed
the
feature/2263-setup-config-auth
branch
2 times, most recently
from
510b9b3
to
35b40ac
Compare
stevenbal
force-pushed
the
feature/2263-setup-config-auth
branch
from
35b40ac
to
58da371
Compare
| required_settings = [ | ||
| "DIGID_OIDC_OIDC_RP_CLIENT_ID", | ||
| "DIGID_OIDC_OIDC_RP_CLIENT_SECRET", | ||
| # NOTE these are part of the model, but not actually part of the admin form |
There was a problem hiding this comment.
We have this in some of the other config setup steps too. Perhaps we should decide if we want to expose these variables (then un-comment) or not (then delete the commented out code).
There was a problem hiding this comment.
I added them to the form and removed the comments
| f"Something went wrong while saving configuration: {form.errors}" | ||
| ) | ||
|
|
||
| form.save() |
There was a problem hiding this comment.
In other places, form.save() is wrapped in try/except, here it it is not.
There was a problem hiding this comment.
The SAML models can throw ValidationErrors from methods called in .save() (https://github.com/maykinmedia/django-digid-eherkenning/blob/master/digid_eherkenning/models/base.py#L215), but the OIDC form/models don't do this (validation happens during form.is_valid instead)
There was a problem hiding this comment.
It looks like this mixes regular settings with one-time configuration settings? Can we somehow split that in the code? Or are these always applied?
Also I'd really recommend to look into a solution that would add the description (and other info to generate the documentation) in the same place because it's going to be awful to manage multiple massive lists like this accurately.
You mean With regards to the description/documentation, #1150 is probably the PR to tackle this in? |
https://taiga.maykinmedia.nl/project/open-inwoner/task/2263 also remove deprecated DIGID settings from base.py
* add missing model fields to DigiD/eHerkenning OIDC admin form * split up envvar into DIGID_ENABLED and DIGID_CONFIG_ENABLE * use DIGID_ENABLED for both SAML and OIDC versions
stevenbal
force-pushed
the
feature/2263-setup-config-auth
branch
from
58da371
to
2f1547e
Compare
…ests ✅ [#2324] Add test for setup_configuration command
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1137 +/- ##
===========================================
+ Coverage 95.05% 95.15% +0.09%
===========================================
Files 950 959 +9
Lines 33790 34753 +963
===========================================
+ Hits 32119 33068 +949
- Misses 1671 1685 +14 ☔ View full report in Codecov by Sentry. |
task: https://taiga.maykinmedia.nl/project/open-inwoner/task/2263
Implements setup configuration for:
I also removed the old DigiD SAML settings, because they have been deprecated: https://github.com/maykinmedia/django-digid-eherkenning/blob/master/CHANGELOG.rst#050-2022-10-31