Replace digid_eherkenning_oidc_generics with library code #1297
+1,515
−947
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sergei-maertens
force-pushed
the
chore/replace-digid-eh-oidc-generics
branch
2 times, most recently
from
a3f7e32
to
748d8b8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1297 +/- ##
===========================================
- Coverage 94.88% 94.86% -0.03%
===========================================
Files 1045 1045
Lines 38692 38691 -1
===========================================
- Hits 36714 36704 -10
- Misses 1978 1987 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
sergei-maertens
commented
Jul 6, 2024
There was a problem hiding this comment.
I suppose this will need some face-to-face discussion :)
sergei-maertens
commented
Jul 6, 2024
Comment on lines
290
to
299
| ADMIN_OIDC_USERNAME_CLAIM = config("ADMIN_OIDC_USERNAME_CLAIM", None) | ||
| ADMIN_OIDC_GROUPS_CLAIM = config("ADMIN_OIDC_GROUPS_CLAIM", None) |
There was a problem hiding this comment.
these need to be lists of strings rather than strings
sergei-maertens
force-pushed
the
chore/replace-digid-eh-oidc-generics
branch
3 times, most recently
from
f078ed1
to
924977d
Compare
sergei-maertens
commented
Jul 9, 2024
| @@ -87,8 +87,7 @@ | |||
| "django.contrib.auth.backends.ModelBackend", | |||
| "digid_eherkenning.mock.backends.DigiDBackend", | |||
| "eherkenning.mock.backends.eHerkenningBackend", | |||
| "digid_eherkenning_oidc_generics.backends.OIDCAuthenticationDigiDBackend", | |||
| "digid_eherkenning_oidc_generics.backends.OIDCAuthenticationEHerkenningBackend", | |||
| "open_inwoner.accounts.backends.DigiDEHerkenningOIDCBackend", | |||
There was a problem hiding this comment.
the setting duplication here bit my ass in development, spent about 20 minutes trying to figure out why my breakpoint wasn't hitting :(
sergei-maertens
force-pushed
the
chore/replace-digid-eh-oidc-generics
branch
from
686d522
to
c728bd4
Compare
sergei-maertens
changed the title
sergei-maertens
force-pushed
the
chore/replace-digid-eh-oidc-generics
branch
2 times, most recently
from
71e22ad
to
fd35173
Compare
sergei-maertens
commented
Aug 9, 2024
| @@ -16,6 +16,8 @@ | |||
| from .forms import GroupAdminForm | |||
| from .models import Action, Document, Invite, Message, User | |||
|
|
|||
| # XXX: register proxy models and unregister digid_eherkenning.oidc models | |||
Upgrade digid-eherkenning to a version with OIDC support. This requires a newer version of mozilla-django-oidc-db too.
The app_label is changed to avoid conflicts with the app_label defined in the 'digid_eherkenning.oidc' package. Migrations are added to rename any existing database tables and keep a consistent migration history/state. Special care is needed with regards to the django_migrations table - fixes here cannot be done through migrations, so the AppConfig.ready hook is used to bring these in a consistent state. Finally, the code that is replaced by the library is deleted to de-clutter, which includes uninstalling the models. The migrations are defined such that the new tables are created first and configuration can be copied over, which prevents data loss.
Some database fields have been renamed, which requires settings for the setup_configuration to be updated too. Future commits provide different authentication backends/views implementations, which require listing in their respective django settings too.
Using proxy models is the preferred approach to modify python behaviour as no database schema changes are usually required. This does require all code to call the proxy model rather than the concrete base model, or different outcomes for the same checks would cause problems. This includes the copy of the legacy data into the new models, which should be a smooth automatic migration.
The OIDC libraries support different OIDC-configurations/flow through the same views and backends, which is what this changeset achieves. The init views are exposed via separate URLs/endpoints, and bind the specific OIDC config flavour to be used to the endpoint. This information is recorded in the (session) state, and consulted again in the callback flow. All return flows (when returning from the OIDC Provider) go through the same callback view, which can route to specific handlers based on the configuration used. DigiD/eHerkenning is sent to a single authentication backend, which implements behaviours depending on the specific flavour used. The admin OIDC configuration is bound to the admin user backend, isolated from the DigiD/eHerkenning backend. Currently the callback URLs are still different urls for admin, digid and eherkenning, but there is no need for this - you can already use /oidc/callback/ instead (see the setting OIDC_CALLBACK_CLASS).
The configuration settings are tightly coupled to the django model fields (names and data types). The field names for the configuration have been changed compared to the vendored library, and the structure has been changed to be able to use claims with dots in their name. Unfortunately, this means that breaking changes in the infrastructure/ configuration setup are required because of the way this config is specified and used.
pi-sigma
force-pushed
the
chore/replace-digid-eh-oidc-generics
branch
from
fd35173
to
fb2e811
Compare
pi-sigma
force-pushed
the
chore/replace-digid-eh-oidc-generics
branch
from
fb2e811
to
7d70386
Compare
pi-sigma
reviewed
Oct 2, 2024
| Possible values string | ||
| Default value roles | ||
|
|
||
| Possible values list of strings |
There was a problem hiding this comment.
You can specify the values by adding the ClaimField to custom fields in the settings:
DJANGO_SETUP_CONFIG_CUSTOM_FIELDS = [
{
"field": "mozilla_django_oidc_db.fields.ClaimField",
"description": "list of strings representing a claim",
},
]
| Default value roles | ||
|
|
||
| Possible values list of strings | ||
| Default value [roles] |
There was a problem hiding this comment.
The code responsible for returning roles instead of [roles] as default value is this:
if isinstance(field, (JSONField, ArrayField)) and isinstance(default, Sequence):
try:
return ", ".join(str(item) for item in default)
except TypeError:
return str(default)
The values for env variables used to configure an ArrayField are comma-delimited strings, not lists (the comma-delimited string is transformed into a list via a call to config in the settings).
|
|
||
| def rename_app_label(): | ||
| with connection.cursor() as cursor: | ||
| # uses explicit query names so that we don't accidentally rwrite the |
There was a problem hiding this comment.
Suggested change
| # uses explicit query names so that we don't accidentally rwrite the | |
| # uses explicit query names so that we don't accidentally rewrite the |
| ( | ||
| "digid_eherkenning_oidc_generics_legacy", | ||
| "0003_alter_openidconnectdigidconfig_oidc_exempt_urls_and_more", | ||
| ), |
There was a problem hiding this comment.
Something's going wrong. After renaming of the tables Django attempts to run:
digid_eherkenning_oidc_generics_legacy.0004_alter_openidconnectdigidconfig_table_and_more
and complains:
psycopg2.errors.UndefinedTable: relation "digid_eherkenning_oidc_generics_legacy_openidconnectdigidconfig" does not exist
| Default value {'email': 'email', 'first_name': 'given_name', 'last_name': 'family_name'} | ||
|
|
||
| Description Mapping from user-model fields to OIDC claim paths | ||
| Possible values Mapping: {'some_key': ['Some value']} |
There was a problem hiding this comment.
The possible values are hard-coded for the different field types. Needs change in the library (or overwrite in the configuration step, but this would need to be done for every setting separately)
- Various models for digid-eherkenning-generics have the wrong
database tables associated due to renaming of the app label
before the migrations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trying this is as backwards compatible as possible, but there are some breaking changes. I can't find where to mention this in the documenation :/
Breaking changes
ADMIN_OIDC_CLAIM_MAPPINGmust now be a mapping of key -> list of strings. This makes it possible to use dots in claim names while still supporting nesting. Every element in the array is a level of nesting. Change is because ofClaimFieldsupport in mozilla-django-oidc-db 0.16+ADMIN_OIDC_USERNAME_CLAIMmust now be a list of strings. Same reason as above. I don't know how to fix this for the configuration via envvars, since the whole point of this feature is that no characters gain magical meaning. A comma could be used in a claim, so CSV splitting suffers from the same problem. Maybe it should just be ajson.loads(...)call on the envvar string?ADMIN_OIDC_GROUPS_CLAIMsame as forADMIN_OIDC_USERNAME_CLAIMOIDC_EXEMPT_URLSconfiguration variables, after checking in the office these don't appear to be used at all. The config option was removed in mozilla-django-oidc-db*_OIDC_ERROR_MESSAGE_MAPPINGconfiguration variables, in favour of hardcoded messages.Questions
is theerror_message_mappingreally necessary? Error code + prescribed error messages are specified by OIDC standard and Logius, no?OIDC_EXEMPT_URLSsetting for the non-standard URLs? Otherwise there is a session refresh redirect loop issue for DigiD/eHerkenning/admin via OIDC I suspect.