-
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
Support LOA claims and claim processing #72
Support LOA claims and claim processing #72
Conversation
The names of the base classes from the mozilla-django-oidc-db and digid_eherkenning.oidc packages were too confusing.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #72 +/- ##
==========================================
+ Coverage 90.41% 90.85% +0.43%
==========================================
Files 50 51 +1
Lines 1576 1651 +75
Branches 141 152 +11
==========================================
+ Hits 1425 1500 +75
Misses 110 110
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6b48d48
to
71fe4f8
Compare
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.
Looks good, just minor comments
loa = default | ||
loa_claim_missing = not default | ||
|
||
if loa_claim_missing: |
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'm not familiar with the types of response we get from IdPs, but is the reason we don't just do the following here:
if loa:
...
Because the loa
retrieved from the claims can be ""
and that is valid?
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.
Signicat appears to only return the BSN and no LOA information, Anoigo returns strings like "20"
but I'm being very defensive here since other providers may do even other things or intermediate keycloaks can rewrite claims...
Basically we check for us if a default is set to fall back: we consider the claim missing if:
- no claim to look up is configured and the default is empty
- the claim is configured, but not present and the default is empty
in other cases we can return a value:
- a (non-empty) default configured but no claim lookup configured -> use the default
- a claim lookup configured and the claim is found -> use the value, possibly it's post-processed with the value map. That should allow to map an empty string or
0
or whatever to a value that makes sense
* Added claim field to configure claim holding the value for LOA * Added field for LOA fallback value, populated from the respective choices (known identifiers from the standards).
Different brokers (Anoigo, Signicat) seem to return the LOA from the used authentication mean in a non-standard way, so our configuration needs to be able to translate their values into standardized values. Brokers that don't return a value at all can be configured to specify a default value to apply. Source values (from the broker) can be string or number, destination values are enum-based depending on whether it's DigiD or eHerkenning.
In non-strict (lax) mode, absent required claims don't lead to ValueError being raised so that the caller can handle this data appropriately. Required for some backwards-compatibility in Open Forms.
71fe4f8
to
0b17cfe
Compare
Turns out not all providers support returning LOA information in the claims for the authenticated user, so we can specify a default and apply a value mapping. Anoigo for example has a value
"20"
and I think this maps to DigiD middle assurance level?