✨ [#1902/1903] DigiD/eHerkenning via OIDC

[stevenbal]

tasks:

• https://taiga.maykinmedia.nl/project/open-inwoner/task/1902
• https://taiga.maykinmedia.nl/project/open-inwoner/task/1903

Issues:

• log-outgoing-requests fails on logout due to the URL being longer than 1000 chars (django.db.utils.DataError: value too long for type character varying(1000)). We should probably truncate chars past 1000 in the library? -> already fixed in [#29] change url field to textfield django-log-outgoing-requests#30 as Paul remarked
• do we want to use any data - other than BSN - from the payload we get back via OIDC? or do we leave this to the signal that fetches data from HaalCentraal?
• Multiple feature flags seems annoying, to enable OIDC eHerkenning you have to enable eHerkenning via siteconfig and via OIDC config (as override), not sure how we can improve this

[codecov-commenter]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (345147d) 92.81% compared to head (6a019ca) 92.91%.
Report is 10 commits behind head on develop.

Files	Patch %	Lines
src/digid_eherkenning_oidc_generics/views.py	89.79%	5 Missing ⚠️
src/open_inwoner/accounts/views/registration.py	78.57%	3 Missing ⚠️
src/digid_eherkenning_oidc_generics/backends.py	97.05%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #879      +/-   ##
===========================================
+ Coverage    92.81%   92.91%   +0.10%     
===========================================
  Files          802      815      +13     
  Lines        27516    28001     +485     
===========================================
+ Hits         25538    26017     +479     
- Misses        1978     1984       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alextreme]

Interesting approach, adding keycloak to the docker setup and as a (possible) CI step would offer a nice integrationtest for OIDC.

Ensure that you check that cancelling the loginflow also works as expected. This currently isn't working properly for Open Formulieren in combination with OIDC+DigiD, but showing a proper 'cancellation' message is a mandatory part of a DigiD audit.

log-outgoing-requests fails on logout due to the URL being longer than 1000 chars (django.db.utils.DataError: value too long for type character varying(1000)). We should probably truncate chars past 1000 in the library?

Please do (or at least create an issue for it). This sounds like a bit of a timebomb if you ran into it using OIDC, it is our own library but haven't seen this cause problems before.

do we want to use any data - other than BSN - from the payload we get back via OIDC? or do we leave this to the signal that fetches data from HaalCentraal?

We don't need anything other than the BSN if HaalCentraal BRP is connected, but I suspect that we can use the claim mapping if that's not the case? Either way the default claim mapping can be empty.

Multiple feature flags seems annoying, to enable OIDC eHerkenning you have to enable eHerkenning via siteconfig and via OIDC config (as override), not sure how we can improve this

I'm open to suggestions and further improvements. Maybe turn it the other way around, that we configure it via the siteconfig (DigiD: disabled, mock, Logius/SAML, OIDC) and that configuring this in turn toggles the SAML/OIDC config on? Maybe @pi-sigma has suggestions too.

[pi-sigma]

@stevenbal The url max length issue has been reported and (should be) fixed by converting the url field to a TextField: maykinmedia/django-log-outgoing-requests#29

[pi-sigma]

Let's try to resolve this question (discuss with @alextreme) so it doesn't remain here.

[pi-sigma]

@stevenbal The two-step approach to configuring eHerkenning with OIDC/SAML may not be ideal, but enabling all of it in siteconfig has its own issues. The most straightforward approach IMHO would be to keep the two-step approach, but document the OIDC/SAML config options in the eHerkenning help text in siteconfig. Perhaps even include a link to Koppelingen > eHerkenning/eIDAS-configuratie and Koppelingen > OpenID Connect configuration (btw: why is the latter English? I thought client-facing text should be Dutch by default. Not pressing as it will be translated anyway).

[stevenbal]

Ensure that you check that cancelling the loginflow also works as expected. This currently isn't working properly for Open Formulieren in combination with OIDC+DigiD, but showing a proper 'cancellation' message is a mandatory part of a DigiD audit.

@alextreme I'm not sure how to test this locally without actually connecting to a DigiD test/preprod instance (via Keycloak). Keycloak itself doesn't provide a cancel button that I could test with, should returning to the previous page also lead to this cancellation message?

[alextreme]

Ensure that you check that cancelling the loginflow also works as expected. This currently isn't working properly for Open Formulieren in combination with OIDC+DigiD, but showing a proper 'cancellation' message is a mandatory part of a DigiD audit.

@alextreme I'm not sure how to test this locally without actually connecting to a DigiD test/preprod instance (via Keycloak). Keycloak itself doesn't provide a cancel button that I could test with, should returning to the previous page also lead to this cancellation message?

In that case create a task to verify this after the next release on acc.

[stevenbal]

We don't need anything other than the BSN if HaalCentraal BRP is connected, but I suspect that we can use the claim mapping if that's not the case? Either way the default claim mapping can be empty.

The singletonmodels for DigD and eHerkenning OIDC don't have claim mappings (copied them from openforms), so currently all that is taken from the payload is BSN/KVK

[stevenbal]

@alextreme @pi-sigma with regards to the feature flags, I updated the helptext for eherkenning_enabled in SiteConfiguration and the enabled attributes on the OIDCConfig, to explain that enabling OIDC overrides the use of SAML

[alextreme]

We don't need anything other than the BSN if HaalCentraal BRP is connected, but I suspect that we can use the claim mapping if that's not the case? Either way the default claim mapping can be empty.

The singletonmodels for DigD and eHerkenning OIDC don't have claim mappings (copied them from openforms), so currently all that is taken from the payload is BSN/KVK

Ah you are correct. As long as there is a 'Claimnaam BSN' that can be modified, it normally should be bsn but I've noticed that different OIDC brokers pass along this value in a different attribute.

[stevenbal]

Ah you are correct. As long as there is a 'Claimnaam BSN' that can be modified, it normally should be bsn but I've noticed that different OIDC brokers pass along this value in a different attribute.

This indeed configurable via the admin:

and for eHerkenning