From 332f73bf0cee999080c48460cac15a8bf5ba2e3e Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 24 May 2024 17:24:14 +0200 Subject: [PATCH 01/25] :tada: Copy over digid-eherkenning-oidc-generics code from Open Forms --- digid_eherkenning/oidc/__init__.py | 65 ++ digid_eherkenning/oidc/admin.py | 103 +++ digid_eherkenning/oidc/apps.py | 9 + digid_eherkenning/oidc/forms.py | 38 + ...nitial_squashed_0007_auto_20221213_1347.py | 800 ++++++++++++++++++ .../migrations/0002_auto_20240207_1546.py | 52 ++ digid_eherkenning/oidc/migrations/__init__.py | 0 digid_eherkenning/oidc/models.py | 213 +++++ digid_eherkenning/oidc/tests/__init__.py | 0 .../oidc/tests/test_callback_endpoints.py | 45 + digid_eherkenning/oidc/utils.py | 29 + digid_eherkenning/oidc/views.py | 62 ++ setup.cfg | 2 + 13 files changed, 1418 insertions(+) create mode 100644 digid_eherkenning/oidc/__init__.py create mode 100644 digid_eherkenning/oidc/admin.py create mode 100644 digid_eherkenning/oidc/apps.py create mode 100644 digid_eherkenning/oidc/forms.py create mode 100644 digid_eherkenning/oidc/migrations/0001_initial_squashed_0007_auto_20221213_1347.py create mode 100644 digid_eherkenning/oidc/migrations/0002_auto_20240207_1546.py create mode 100644 digid_eherkenning/oidc/migrations/__init__.py create mode 100644 digid_eherkenning/oidc/models.py create mode 100644 digid_eherkenning/oidc/tests/__init__.py create mode 100644 digid_eherkenning/oidc/tests/test_callback_endpoints.py create mode 100644 digid_eherkenning/oidc/utils.py create mode 100644 digid_eherkenning/oidc/views.py diff --git a/digid_eherkenning/oidc/__init__.py b/digid_eherkenning/oidc/__init__.py new file mode 100644 index 0000000..69a897e --- /dev/null +++ b/digid_eherkenning/oidc/__init__.py @@ -0,0 +1,65 @@ +""" +DigiD-eHerkenning-OIDC-generics abstracts authenticating over OIDC. + +DigiD/eHerkenning are typically exposed via SAML bindings, but there exist identity +providers that abstract this and instead offer an OpenID Connect flow to log in with +DigiD and/or eHerkenning. This package facilitates integrating with such providers. + +The architecture and authentication flows are tricky in some places. Here's an attempt +to explain it. + +**Configuration** + +Each authentication means (DigiD, eHerkenning, mandate (machtigen) variants...) is +mapped to an OpenID client configuration, which roughly holds: + +- the OIDC endpoints to use/redirect users to +- the OAUTH2 client ID and secret to use, which indicate to the IdP which authentication + means they should send the user to +- which claims to look up/extract from the UserInfo endpoint/JWT + +These are stored in (subclasses of) the +:class:`~digid_herkenning.oidc.models.OpenIDConnectBaseConfig` model. + +**Authentication flow** + +When a user starts a login flow, they: + +1. Click the appriopriate button/link +2. A Django view processes this and looks up the relevant configuration +3. The view redirects the user to the identity provider (typically a different domain) +4. Authenticate with the IdP +5. The IdP redirects back to our application +6. Our callback view performs the OIDC exchange and extracts + stores the relevant user + information +7. Finally, the callback view looks up where the user needs to be redirected to and + sends them that way. + +Steps 2-3 are called the "init" phase in this package, while steps 6-7 are the +"callback" phase. + +**Init phase** + +The mozilla-django-oidc-db package provides the +:class:`~mozilla_django_oidc_db.views.OIDCInit` view class, for the init phase. It +ensures that the specified config class is persisted in the authentication state. + +This package provides concrete views bound to configuration classes: + +* :attr:`~digid_herkenning.oidc.views.digid_init` +* :attr:`~digid_herkenning.oidc.views.digid_machtigen_init` +* :attr:`~digid_herkenning.oidc.views.eherkenning_init` +* :attr:`~digid_herkenning.oidc.views.eherkenning_bewindvoering_init` + +**Callback phase** + +The callback phase validates the code and state, and loads which configuration class +needs to be used from the state. With this information, the authentication backends +from ``settings.AUTHENTICATION_BACKENDS`` are tried in order. Typically this will +use the backend shipped in mozilla-django-oidc-db, or a subclass of it. + +The OpenID connect flow exchanges the code for the an access token (and ID token), and +the user details are retrieved. You should provide a customized backend to determine +what needs to be done with this user information, e.g. create a django user or store +the information in the django session. +""" diff --git a/digid_eherkenning/oidc/admin.py b/digid_eherkenning/oidc/admin.py new file mode 100644 index 0000000..7188839 --- /dev/null +++ b/digid_eherkenning/oidc/admin.py @@ -0,0 +1,103 @@ +from collections.abc import Sequence +from copy import deepcopy + +from django.contrib import admin +from django.utils.translation import gettext_lazy as _ + +from solo.admin import SingletonModelAdmin + +from .forms import admin_modelform_factory +from .models import ( + OpenIDConnectDigiDMachtigenConfig, + OpenIDConnectEHerkenningBewindvoeringConfig, + OpenIDConnectEHerkenningConfig, + OpenIDConnectPublicConfig, +) + +# Using a dict because these retain ordering, and it makes things a bit more readable. +ATTRIBUTES_MAPPING_TITLE = _("Attributes to extract from claim") +COMMON_FIELDSETS = { + _("Activation"): { + "fields": ("enabled",), + }, + _("Common settings"): { + "fields": ( + "oidc_rp_client_id", + "oidc_rp_client_secret", + "oidc_rp_scopes_list", + "oidc_rp_sign_algo", + "oidc_rp_idp_sign_key", + ), + }, + ATTRIBUTES_MAPPING_TITLE: { + "fields": (), # populated from the factory function below + }, + _("Endpoints"): { + "fields": ( + "oidc_op_discovery_endpoint", + "oidc_op_jwks_endpoint", + "oidc_op_authorization_endpoint", + "oidc_op_token_endpoint", + "oidc_token_use_basic_auth", + "oidc_op_user_endpoint", + "oidc_op_logout_endpoint", + ), + }, + _("Keycloak specific settings"): { + "fields": ("oidc_keycloak_idp_hint",), + "classes": ["collapse in"], + }, + _("Advanced settings"): { + "fields": ( + "oidc_use_nonce", + "oidc_nonce_size", + "oidc_state_size", + "oidc_exempt_urls", + "userinfo_claims_source", + ), + "classes": ["collapse in"], + }, +} + + +def fieldsets_factory(claim_mapping_fields: Sequence[str]): + """ + Apply the shared fieldsets configuration with the model-specific overrides. + """ + _fieldsets = deepcopy(COMMON_FIELDSETS) + _fieldsets[ATTRIBUTES_MAPPING_TITLE]["fields"] += tuple(claim_mapping_fields) + return tuple((label, config) for label, config in _fieldsets.items()) + + +@admin.register(OpenIDConnectPublicConfig) +class OpenIDConnectConfigDigiDAdmin(SingletonModelAdmin): + form = admin_modelform_factory(OpenIDConnectPublicConfig) + fieldsets = fieldsets_factory(claim_mapping_fields=["identifier_claim_name"]) + + +@admin.register(OpenIDConnectEHerkenningConfig) +class OpenIDConnectConfigEHerkenningAdmin(SingletonModelAdmin): + form = admin_modelform_factory(OpenIDConnectEHerkenningConfig) + fieldsets = fieldsets_factory(claim_mapping_fields=["identifier_claim_name"]) + + +@admin.register(OpenIDConnectDigiDMachtigenConfig) +class OpenIDConnectConfigDigiDMachtigenAdmin(SingletonModelAdmin): + form = admin_modelform_factory(OpenIDConnectDigiDMachtigenConfig) + fieldsets = fieldsets_factory( + claim_mapping_fields=[ + "vertegenwoordigde_claim_name", + "gemachtigde_claim_name", + ] + ) + + +@admin.register(OpenIDConnectEHerkenningBewindvoeringConfig) +class OpenIDConnectConfigEHerkenningBewindvoeringAdmin(SingletonModelAdmin): + form = admin_modelform_factory(OpenIDConnectEHerkenningBewindvoeringConfig) + fieldsets = fieldsets_factory( + claim_mapping_fields=[ + "vertegenwoordigde_company_claim_name", + "gemachtigde_person_claim_name", + ] + ) diff --git a/digid_eherkenning/oidc/apps.py b/digid_eherkenning/oidc/apps.py new file mode 100644 index 0000000..9b3d369 --- /dev/null +++ b/digid_eherkenning/oidc/apps.py @@ -0,0 +1,9 @@ +from django.apps import AppConfig +from django.utils.translation import gettext_lazy as _ + + +class OIDCAppConfig(AppConfig): + name = "digid_eherkenning.oidc" + verbose_name = _("DigiD & eHerkenning via OpenID Connect") + # can't change this label because of existing migrations in Open Forms/Open Inwoner + label = "digid_eherkenning_oidc_generics" diff --git a/digid_eherkenning/oidc/forms.py b/digid_eherkenning/oidc/forms.py new file mode 100644 index 0000000..aeed63d --- /dev/null +++ b/digid_eherkenning/oidc/forms.py @@ -0,0 +1,38 @@ +from copy import deepcopy + +from django.forms import modelform_factory + +from mozilla_django_oidc_db.constants import OIDC_MAPPING +from mozilla_django_oidc_db.forms import OpenIDConnectConfigForm + +from .models import OpenIDConnectBaseConfig + + +def admin_modelform_factory(model: type[OpenIDConnectBaseConfig], *args, **kwargs): + """ + Factory function to generate a model form class for a given configuration model. + + The configuration model is expected to be a subclass of + :class:`~digid_eherkenning_oidc_generics.models.OpenIDConnectBaseConfig`. + + Additional args and kwargs are forwarded to django's + :func:`django.forms.modelform_factory`. + """ + kwargs.setdefault("form", OpenIDConnectConfigForm) + Form = modelform_factory(model, *args, **kwargs) + + assert issubclass( + Form, OpenIDConnectConfigForm + ), "The base form class must be a subclass of OpenIDConnectConfigForm." + + # update the mapping of discovery endpoint keys to model fields, since our base + # model adds the ``oidc_op_logout_endpoint`` field. + Form.oidc_mapping = { + **deepcopy(OIDC_MAPPING), + "oidc_op_logout_endpoint": "end_session_endpoint", + } + Form.required_endpoints = [ + *Form.required_endpoints, + "oidc_op_logout_endpoint", + ] + return Form diff --git a/digid_eherkenning/oidc/migrations/0001_initial_squashed_0007_auto_20221213_1347.py b/digid_eherkenning/oidc/migrations/0001_initial_squashed_0007_auto_20221213_1347.py new file mode 100644 index 0000000..d619500 --- /dev/null +++ b/digid_eherkenning/oidc/migrations/0001_initial_squashed_0007_auto_20221213_1347.py @@ -0,0 +1,800 @@ +# Generated by Django 3.2.20 on 2023-09-08 10:43 + +from django.db import migrations, models + +import digid_eherkenning_oidc_generics.models +import django_jsonform.models.fields +import mozilla_django_oidc_db.models + + +class Migration(migrations.Migration): + dependencies = [] + + operations = [ + migrations.CreateModel( + name="OpenIDConnectDigiDMachtigenConfig", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "enabled", + models.BooleanField( + default=False, + help_text="Indicates whether OpenID Connect for authentication/authorization is enabled", + verbose_name="enable", + ), + ), + ( + "oidc_rp_client_id", + models.CharField( + help_text="OpenID Connect client ID provided by the OIDC Provider", + max_length=1000, + verbose_name="OpenID Connect client ID", + ), + ), + ( + "oidc_rp_client_secret", + models.CharField( + help_text="OpenID Connect secret provided by the OIDC Provider", + max_length=1000, + verbose_name="OpenID Connect secret", + ), + ), + ( + "oidc_rp_sign_algo", + models.CharField( + default="HS256", + help_text="Algorithm the Identity Provider uses to sign ID tokens", + max_length=50, + verbose_name="OpenID sign algorithm", + ), + ), + ( + "oidc_op_discovery_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider discovery endpoint ending with a slash (`.well-known/...` will be added automatically). If this is provided, the remaining endpoints can be omitted, as they will be derived from this endpoint.", + max_length=1000, + verbose_name="Discovery endpoint", + ), + ), + ( + "oidc_op_jwks_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider JSON Web Key Set endpoint. Required if `RS256` is used as signing algorithm.", + max_length=1000, + verbose_name="JSON Web Key Set endpoint", + ), + ), + ( + "oidc_op_authorization_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider authorization endpoint", + max_length=1000, + verbose_name="Authorization endpoint", + ), + ), + ( + "oidc_op_token_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider token endpoint", + max_length=1000, + verbose_name="Token endpoint", + ), + ), + ( + "oidc_op_user_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider userinfo endpoint", + max_length=1000, + verbose_name="User endpoint", + ), + ), + ( + "oidc_rp_idp_sign_key", + models.CharField( + blank=True, + help_text="Key the Identity Provider uses to sign ID tokens in the case of an RSA sign algorithm. Should be the signing key in PEM or DER format.", + max_length=1000, + verbose_name="Sign key", + ), + ), + ( + "oidc_op_logout_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider logout endpoint", + max_length=1000, + verbose_name="Logout endpoint", + ), + ), + ( + "oidc_keycloak_idp_hint", + models.CharField( + blank=True, + help_text="Specific for Keycloak: parameter that indicates which identity provider should be used (therefore skipping the Keycloak login screen).", + max_length=1000, + verbose_name="Keycloak Identity Provider hint", + ), + ), + ( + "vertegenwoordigde_claim_name", + models.CharField( + default="aanvrager.bsn", + help_text="Name of the claim in which the BSN of the person being represented is stored", + max_length=50, + verbose_name="vertegenwoordigde claim name", + ), + ), + ( + "gemachtigde_claim_name", + models.CharField( + default="gemachtigde.bsn", + help_text="Name of the claim in which the BSN of the person representing someone else is stored", + max_length=50, + verbose_name="gemachtigde claim name", + ), + ), + ( + "oidc_rp_scopes_list", + django_jsonform.models.fields.ArrayField( + base_field=models.CharField( + max_length=50, verbose_name="OpenID Connect scope" + ), + blank=True, + default=digid_eherkenning_oidc_generics.models.get_default_scopes_bsn, + help_text="OpenID Connect scopes that are requested during login. These scopes are hardcoded and must be supported by the identity provider", + size=None, + verbose_name="OpenID Connect scopes", + ), + ), + ( + "oidc_exempt_urls", + django_jsonform.models.fields.ArrayField( + base_field=models.CharField( + max_length=1000, verbose_name="Exempt URL" + ), + blank=True, + default=list, + help_text="This is a list of absolute url paths, regular expressions for url paths, or Django view names. This plus the mozilla-django-oidc urls are exempted from the session renewal by the SessionRefresh middleware.", + size=None, + verbose_name="URLs exempt from session renewal", + ), + ), + ( + "oidc_nonce_size", + models.PositiveIntegerField( + default=32, + help_text="Sets the length of the random string used for OpenID Connect nonce verification", + verbose_name="Nonce size", + ), + ), + ( + "oidc_state_size", + models.PositiveIntegerField( + default=32, + help_text="Sets the length of the random string used for OpenID Connect state verification", + verbose_name="State size", + ), + ), + ( + "oidc_use_nonce", + models.BooleanField( + default=True, + help_text="Controls whether the OpenID Connect client uses nonce verification", + verbose_name="Use nonce", + ), + ), + ( + "userinfo_claims_source", + models.CharField( + choices=[ + ("userinfo_endpoint", "Userinfo endpoint"), + ("id_token", "ID token"), + ], + default="userinfo_endpoint", + help_text="Indicates the source from which the user information claims should be extracted.", + max_length=100, + verbose_name="user information claims extracted from", + ), + ), + ], + options={ + "verbose_name": "OpenID Connect configuration for DigiD Machtigen", + }, + bases=(models.Model,), + ), + migrations.CreateModel( + name="OpenIDConnectEHerkenningBewindvoeringConfig", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "enabled", + models.BooleanField( + default=False, + help_text="Indicates whether OpenID Connect for authentication/authorization is enabled", + verbose_name="enable", + ), + ), + ( + "oidc_rp_client_id", + models.CharField( + help_text="OpenID Connect client ID provided by the OIDC Provider", + max_length=1000, + verbose_name="OpenID Connect client ID", + ), + ), + ( + "oidc_rp_client_secret", + models.CharField( + help_text="OpenID Connect secret provided by the OIDC Provider", + max_length=1000, + verbose_name="OpenID Connect secret", + ), + ), + ( + "oidc_rp_sign_algo", + models.CharField( + default="HS256", + help_text="Algorithm the Identity Provider uses to sign ID tokens", + max_length=50, + verbose_name="OpenID sign algorithm", + ), + ), + ( + "oidc_op_discovery_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider discovery endpoint ending with a slash (`.well-known/...` will be added automatically). If this is provided, the remaining endpoints can be omitted, as they will be derived from this endpoint.", + max_length=1000, + verbose_name="Discovery endpoint", + ), + ), + ( + "oidc_op_jwks_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider JSON Web Key Set endpoint. Required if `RS256` is used as signing algorithm.", + max_length=1000, + verbose_name="JSON Web Key Set endpoint", + ), + ), + ( + "oidc_op_authorization_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider authorization endpoint", + max_length=1000, + verbose_name="Authorization endpoint", + ), + ), + ( + "oidc_op_token_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider token endpoint", + max_length=1000, + verbose_name="Token endpoint", + ), + ), + ( + "oidc_op_user_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider userinfo endpoint", + max_length=1000, + verbose_name="User endpoint", + ), + ), + ( + "oidc_rp_idp_sign_key", + models.CharField( + blank=True, + help_text="Key the Identity Provider uses to sign ID tokens in the case of an RSA sign algorithm. Should be the signing key in PEM or DER format.", + max_length=1000, + verbose_name="Sign key", + ), + ), + ( + "oidc_use_nonce", + models.BooleanField( + default=True, + help_text="Controls whether the OpenID Connect client uses nonce verification", + verbose_name="Use nonce", + ), + ), + ( + "oidc_nonce_size", + models.PositiveIntegerField( + default=32, + help_text="Sets the length of the random string used for OpenID Connect nonce verification", + verbose_name="Nonce size", + ), + ), + ( + "oidc_state_size", + models.PositiveIntegerField( + default=32, + help_text="Sets the length of the random string used for OpenID Connect state verification", + verbose_name="State size", + ), + ), + ( + "oidc_exempt_urls", + django_jsonform.models.fields.ArrayField( + base_field=models.CharField( + max_length=1000, verbose_name="Exempt URL" + ), + blank=True, + default=list, + help_text="This is a list of absolute url paths, regular expressions for url paths, or Django view names. This plus the mozilla-django-oidc urls are exempted from the session renewal by the SessionRefresh middleware.", + size=None, + verbose_name="URLs exempt from session renewal", + ), + ), + ( + "oidc_op_logout_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider logout endpoint", + max_length=1000, + verbose_name="Logout endpoint", + ), + ), + ( + "oidc_keycloak_idp_hint", + models.CharField( + blank=True, + help_text="Specific for Keycloak: parameter that indicates which identity provider should be used (therefore skipping the Keycloak login screen).", + max_length=1000, + verbose_name="Keycloak Identity Provider hint", + ), + ), + ( + "vertegenwoordigde_company_claim_name", + models.CharField( + default="aanvrager.kvk", + help_text="Name of the claim in which the KVK of the company being represented is stored", + max_length=50, + verbose_name="vertegenwoordigde company claim name", + ), + ), + ( + "gemachtigde_person_claim_name", + models.CharField( + default="gemachtigde.pseudoID", + help_text="Name of the claim in which the ID of the person representing a company is stored", + max_length=50, + verbose_name="gemachtigde person claim name", + ), + ), + ( + "oidc_rp_scopes_list", + django_jsonform.models.fields.ArrayField( + base_field=models.CharField( + max_length=50, verbose_name="OpenID Connect scope" + ), + blank=True, + default=digid_eherkenning_oidc_generics.models.get_default_scopes_bsn, + help_text="OpenID Connect scopes that are requested during login. These scopes are hardcoded and must be supported by the identity provider", + size=None, + verbose_name="OpenID Connect scopes", + ), + ), + ( + "userinfo_claims_source", + models.CharField( + choices=[ + ("userinfo_endpoint", "Userinfo endpoint"), + ("id_token", "ID token"), + ], + default="userinfo_endpoint", + help_text="Indicates the source from which the user information claims should be extracted.", + max_length=100, + verbose_name="user information claims extracted from", + ), + ), + ], + options={ + "verbose_name": "OpenID Connect configuration for eHerkenning Bewindvoering", + }, + bases=(models.Model,), + ), + migrations.CreateModel( + name="OpenIDConnectEHerkenningConfig", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "enabled", + models.BooleanField( + default=False, + help_text="Indicates whether OpenID Connect for authentication/authorization is enabled", + verbose_name="enable", + ), + ), + ( + "oidc_rp_client_id", + models.CharField( + help_text="OpenID Connect client ID provided by the OIDC Provider", + max_length=1000, + verbose_name="OpenID Connect client ID", + ), + ), + ( + "oidc_rp_client_secret", + models.CharField( + help_text="OpenID Connect secret provided by the OIDC Provider", + max_length=1000, + verbose_name="OpenID Connect secret", + ), + ), + ( + "oidc_rp_sign_algo", + models.CharField( + default="HS256", + help_text="Algorithm the Identity Provider uses to sign ID tokens", + max_length=50, + verbose_name="OpenID sign algorithm", + ), + ), + ( + "oidc_op_discovery_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider discovery endpoint ending with a slash (`.well-known/...` will be added automatically). If this is provided, the remaining endpoints can be omitted, as they will be derived from this endpoint.", + max_length=1000, + verbose_name="Discovery endpoint", + ), + ), + ( + "oidc_op_jwks_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider JSON Web Key Set endpoint. Required if `RS256` is used as signing algorithm.", + max_length=1000, + verbose_name="JSON Web Key Set endpoint", + ), + ), + ( + "oidc_op_authorization_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider authorization endpoint", + max_length=1000, + verbose_name="Authorization endpoint", + ), + ), + ( + "oidc_op_token_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider token endpoint", + max_length=1000, + verbose_name="Token endpoint", + ), + ), + ( + "oidc_op_user_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider userinfo endpoint", + max_length=1000, + verbose_name="User endpoint", + ), + ), + ( + "oidc_rp_idp_sign_key", + models.CharField( + blank=True, + help_text="Key the Identity Provider uses to sign ID tokens in the case of an RSA sign algorithm. Should be the signing key in PEM or DER format.", + max_length=1000, + verbose_name="Sign key", + ), + ), + ( + "oidc_op_logout_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider logout endpoint", + max_length=1000, + verbose_name="Logout endpoint", + ), + ), + ( + "oidc_keycloak_idp_hint", + models.CharField( + blank=True, + help_text="Specific for Keycloak: parameter that indicates which identity provider should be used (therefore skipping the Keycloak login screen).", + max_length=1000, + verbose_name="Keycloak Identity Provider hint", + ), + ), + ( + "identifier_claim_name", + models.CharField( + default="kvk", + help_text="The name of the claim in which the KVK of the user is stored", + max_length=100, + verbose_name="KVK claim name", + ), + ), + ( + "oidc_rp_scopes_list", + django_jsonform.models.fields.ArrayField( + base_field=models.CharField( + max_length=50, verbose_name="OpenID Connect scope" + ), + blank=True, + default=digid_eherkenning_oidc_generics.models.get_default_scopes_kvk, + help_text="OpenID Connect scopes that are requested during login. These scopes are hardcoded and must be supported by the identity provider", + size=None, + verbose_name="OpenID Connect scopes", + ), + ), + ( + "oidc_exempt_urls", + django_jsonform.models.fields.ArrayField( + base_field=models.CharField( + max_length=1000, verbose_name="Exempt URL" + ), + blank=True, + default=list, + help_text="This is a list of absolute url paths, regular expressions for url paths, or Django view names. This plus the mozilla-django-oidc urls are exempted from the session renewal by the SessionRefresh middleware.", + size=None, + verbose_name="URLs exempt from session renewal", + ), + ), + ( + "oidc_nonce_size", + models.PositiveIntegerField( + default=32, + help_text="Sets the length of the random string used for OpenID Connect nonce verification", + verbose_name="Nonce size", + ), + ), + ( + "oidc_state_size", + models.PositiveIntegerField( + default=32, + help_text="Sets the length of the random string used for OpenID Connect state verification", + verbose_name="State size", + ), + ), + ( + "oidc_use_nonce", + models.BooleanField( + default=True, + help_text="Controls whether the OpenID Connect client uses nonce verification", + verbose_name="Use nonce", + ), + ), + ( + "userinfo_claims_source", + models.CharField( + choices=[ + ("userinfo_endpoint", "Userinfo endpoint"), + ("id_token", "ID token"), + ], + default="userinfo_endpoint", + help_text="Indicates the source from which the user information claims should be extracted.", + max_length=100, + verbose_name="user information claims extracted from", + ), + ), + ], + options={ + "verbose_name": "OpenID Connect configuration for eHerkenning", + }, + bases=(models.Model,), + ), + migrations.CreateModel( + name="OpenIDConnectPublicConfig", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "enabled", + models.BooleanField( + default=False, + help_text="Indicates whether OpenID Connect for authentication/authorization is enabled", + verbose_name="enable", + ), + ), + ( + "oidc_rp_client_id", + models.CharField( + help_text="OpenID Connect client ID provided by the OIDC Provider", + max_length=1000, + verbose_name="OpenID Connect client ID", + ), + ), + ( + "oidc_rp_client_secret", + models.CharField( + help_text="OpenID Connect secret provided by the OIDC Provider", + max_length=1000, + verbose_name="OpenID Connect secret", + ), + ), + ( + "oidc_rp_sign_algo", + models.CharField( + default="HS256", + help_text="Algorithm the Identity Provider uses to sign ID tokens", + max_length=50, + verbose_name="OpenID sign algorithm", + ), + ), + ( + "oidc_op_discovery_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider discovery endpoint ending with a slash (`.well-known/...` will be added automatically). If this is provided, the remaining endpoints can be omitted, as they will be derived from this endpoint.", + max_length=1000, + verbose_name="Discovery endpoint", + ), + ), + ( + "oidc_op_jwks_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider JSON Web Key Set endpoint. Required if `RS256` is used as signing algorithm.", + max_length=1000, + verbose_name="JSON Web Key Set endpoint", + ), + ), + ( + "oidc_op_authorization_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider authorization endpoint", + max_length=1000, + verbose_name="Authorization endpoint", + ), + ), + ( + "oidc_op_token_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider token endpoint", + max_length=1000, + verbose_name="Token endpoint", + ), + ), + ( + "oidc_op_user_endpoint", + models.URLField( + help_text="URL of your OpenID Connect provider userinfo endpoint", + max_length=1000, + verbose_name="User endpoint", + ), + ), + ( + "oidc_rp_idp_sign_key", + models.CharField( + blank=True, + help_text="Key the Identity Provider uses to sign ID tokens in the case of an RSA sign algorithm. Should be the signing key in PEM or DER format.", + max_length=1000, + verbose_name="Sign key", + ), + ), + ( + "oidc_op_logout_endpoint", + models.URLField( + blank=True, + help_text="URL of your OpenID Connect provider logout endpoint", + max_length=1000, + verbose_name="Logout endpoint", + ), + ), + ( + "oidc_keycloak_idp_hint", + models.CharField( + blank=True, + help_text="Specific for Keycloak: parameter that indicates which identity provider should be used (therefore skipping the Keycloak login screen).", + max_length=1000, + verbose_name="Keycloak Identity Provider hint", + ), + ), + ( + "identifier_claim_name", + models.CharField( + default="bsn", + help_text="The name of the claim in which the BSN of the user is stored", + max_length=100, + verbose_name="BSN claim name", + ), + ), + ( + "oidc_rp_scopes_list", + django_jsonform.models.fields.ArrayField( + base_field=models.CharField( + max_length=50, verbose_name="OpenID Connect scope" + ), + blank=True, + default=digid_eherkenning_oidc_generics.models.get_default_scopes_bsn, + help_text="OpenID Connect scopes that are requested during login. These scopes are hardcoded and must be supported by the identity provider", + size=None, + verbose_name="OpenID Connect scopes", + ), + ), + ( + "oidc_exempt_urls", + django_jsonform.models.fields.ArrayField( + base_field=models.CharField( + max_length=1000, verbose_name="Exempt URL" + ), + blank=True, + default=list, + help_text="This is a list of absolute url paths, regular expressions for url paths, or Django view names. This plus the mozilla-django-oidc urls are exempted from the session renewal by the SessionRefresh middleware.", + size=None, + verbose_name="URLs exempt from session renewal", + ), + ), + ( + "oidc_nonce_size", + models.PositiveIntegerField( + default=32, + help_text="Sets the length of the random string used for OpenID Connect nonce verification", + verbose_name="Nonce size", + ), + ), + ( + "oidc_state_size", + models.PositiveIntegerField( + default=32, + help_text="Sets the length of the random string used for OpenID Connect state verification", + verbose_name="State size", + ), + ), + ( + "oidc_use_nonce", + models.BooleanField( + default=True, + help_text="Controls whether the OpenID Connect client uses nonce verification", + verbose_name="Use nonce", + ), + ), + ( + "userinfo_claims_source", + models.CharField( + choices=[ + ("userinfo_endpoint", "Userinfo endpoint"), + ("id_token", "ID token"), + ], + default="userinfo_endpoint", + help_text="Indicates the source from which the user information claims should be extracted.", + max_length=100, + verbose_name="user information claims extracted from", + ), + ), + ], + options={ + "verbose_name": "OpenID Connect configuration for DigiD", + }, + bases=(models.Model,), + ), + ] diff --git a/digid_eherkenning/oidc/migrations/0002_auto_20240207_1546.py b/digid_eherkenning/oidc/migrations/0002_auto_20240207_1546.py new file mode 100644 index 0000000..fa17bca --- /dev/null +++ b/digid_eherkenning/oidc/migrations/0002_auto_20240207_1546.py @@ -0,0 +1,52 @@ +# Generated by Django 3.2.24 on 2024-02-07 14:46 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ( + "digid_eherkenning_oidc_generics", + "0001_initial_squashed_0007_auto_20221213_1347", + ), + ] + + operations = [ + migrations.AddField( + model_name="openidconnectdigidmachtigenconfig", + name="oidc_token_use_basic_auth", + field=models.BooleanField( + default=False, + help_text="If enabled, the client ID and secret are sent in the HTTP Basic auth header when obtaining the access token. Otherwise, they are sent in the request body.", + verbose_name="Use Basic auth for token endpoint", + ), + ), + migrations.AddField( + model_name="openidconnecteherkenningbewindvoeringconfig", + name="oidc_token_use_basic_auth", + field=models.BooleanField( + default=False, + help_text="If enabled, the client ID and secret are sent in the HTTP Basic auth header when obtaining the access token. Otherwise, they are sent in the request body.", + verbose_name="Use Basic auth for token endpoint", + ), + ), + migrations.AddField( + model_name="openidconnecteherkenningconfig", + name="oidc_token_use_basic_auth", + field=models.BooleanField( + default=False, + help_text="If enabled, the client ID and secret are sent in the HTTP Basic auth header when obtaining the access token. Otherwise, they are sent in the request body.", + verbose_name="Use Basic auth for token endpoint", + ), + ), + migrations.AddField( + model_name="openidconnectpublicconfig", + name="oidc_token_use_basic_auth", + field=models.BooleanField( + default=False, + help_text="If enabled, the client ID and secret are sent in the HTTP Basic auth header when obtaining the access token. Otherwise, they are sent in the request body.", + verbose_name="Use Basic auth for token endpoint", + ), + ), + ] diff --git a/digid_eherkenning/oidc/migrations/__init__.py b/digid_eherkenning/oidc/migrations/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/digid_eherkenning/oidc/models.py b/digid_eherkenning/oidc/models.py new file mode 100644 index 0000000..adbf506 --- /dev/null +++ b/digid_eherkenning/oidc/models.py @@ -0,0 +1,213 @@ +from collections.abc import Collection +from typing import ClassVar + +from django.db import models +from django.utils.functional import classproperty +from django.utils.translation import gettext_lazy as _ + +from django_jsonform.models.fields import ArrayField +from mozilla_django_oidc_db.models import OpenIDConnectConfigBase +from mozilla_django_oidc_db.typing import ClaimPath + + +def get_default_scopes_bsn(): + """ + Returns the default scopes to request for OpenID Connect logins + """ + return ["openid", "bsn"] + + +def get_default_scopes_kvk(): + """ + Returns the default scopes to request for OpenID Connect logins + """ + return ["openid", "kvk"] + + +class OpenIDConnectBaseConfig(OpenIDConnectConfigBase): + """ + Configuration for DigiD authentication via OpenID connect + """ + + oidc_op_logout_endpoint = models.URLField( + _("Logout endpoint"), + max_length=1000, + help_text=_("URL of your OpenID Connect provider logout endpoint"), + blank=True, + ) + + # Keycloak specific config + oidc_keycloak_idp_hint = models.CharField( + _("Keycloak Identity Provider hint"), + max_length=1000, + help_text=_( + "Specific for Keycloak: parameter that indicates which identity provider " + "should be used (therefore skipping the Keycloak login screen)." + ), + blank=True, + ) + + class Meta: + verbose_name = _("OpenID Connect configuration") + abstract = True + + @classproperty + def oidcdb_check_idp_availability(cls) -> bool: + return True + + +class OpenIDConnectPublicConfig(OpenIDConnectBaseConfig): + """ + Configuration for DigiD authentication via OpenID connect + """ + + identifier_claim_name = models.CharField( + _("BSN claim name"), + max_length=100, + help_text=_("The name of the claim in which the BSN of the user is stored"), + default="bsn", + ) + oidc_rp_scopes_list = ArrayField( + verbose_name=_("OpenID Connect scopes"), + base_field=models.CharField(_("OpenID Connect scope"), max_length=50), + default=get_default_scopes_bsn, + blank=True, + help_text=_( + "OpenID Connect scopes that are requested during login. " + "These scopes are hardcoded and must be supported by the identity provider" + ), + ) + + custom_oidc_db_prefix: ClassVar[str] = "digid_oidc" + + class Meta: + verbose_name = _("OpenID Connect configuration for DigiD") + + @property + def oidcdb_username_claim(self) -> list[str]: + return [self.identifier_claim_name] + + +class OpenIDConnectDigiDMachtigenConfig(OpenIDConnectBaseConfig): + # TODO: support periods in claim keys + vertegenwoordigde_claim_name = models.CharField( + verbose_name=_("vertegenwoordigde claim name"), + default="aanvrager.bsn", + max_length=50, + help_text=_( + "Name of the claim in which the BSN of the person being represented is stored" + ), + ) + gemachtigde_claim_name = models.CharField( + verbose_name=_("gemachtigde claim name"), + default="gemachtigde.bsn", + max_length=50, + help_text=_( + "Name of the claim in which the BSN of the person representing someone else is stored" + ), + ) + oidc_rp_scopes_list = ArrayField( + verbose_name=_("OpenID Connect scopes"), + base_field=models.CharField(_("OpenID Connect scope"), max_length=50), + default=get_default_scopes_bsn, + blank=True, + help_text=_( + "OpenID Connect scopes that are requested during login. " + "These scopes are hardcoded and must be supported by the identity provider" + ), + ) + + custom_oidc_db_prefix: ClassVar[str] = "digid_machtigen_oidc" + + class Meta: + verbose_name = _("OpenID Connect configuration for DigiD Machtigen") + + @property + def digid_eherkenning_machtigen_claims(self) -> dict[str, ClaimPath]: + return { + "vertegenwoordigde": [self.vertegenwoordigde_claim_name], + "gemachtigde": [self.gemachtigde_claim_name], + } + + @property + def oidcdb_sensitive_claims(self) -> Collection[ClaimPath]: + return list(self.digid_eherkenning_machtigen_claims.values()) + + +class OpenIDConnectEHerkenningConfig(OpenIDConnectBaseConfig): + """ + Configuration for eHerkenning authentication via OpenID connect + """ + + identifier_claim_name = models.CharField( + _("KVK claim name"), + max_length=100, + help_text=_("The name of the claim in which the KVK of the user is stored"), + default="kvk", + ) + oidc_rp_scopes_list = ArrayField( + verbose_name=_("OpenID Connect scopes"), + base_field=models.CharField(_("OpenID Connect scope"), max_length=50), + default=get_default_scopes_kvk, + blank=True, + help_text=_( + "OpenID Connect scopes that are requested during login. " + "These scopes are hardcoded and must be supported by the identity provider" + ), + ) + + custom_oidc_db_prefix: ClassVar[str] = "eherkenning_oidc" + + class Meta: + verbose_name = _("OpenID Connect configuration for eHerkenning") + + @property + def oidcdb_username_claim(self) -> list[str]: + return [self.identifier_claim_name] + + +class OpenIDConnectEHerkenningBewindvoeringConfig(OpenIDConnectBaseConfig): + # TODO: support periods in claim keys + vertegenwoordigde_company_claim_name = models.CharField( + verbose_name=_("vertegenwoordigde company claim name"), + default="aanvrager.kvk", + max_length=50, + help_text=_( + "Name of the claim in which the KVK of the company being represented is stored" + ), + ) + gemachtigde_person_claim_name = models.CharField( + verbose_name=_("gemachtigde person claim name"), + default="gemachtigde.pseudoID", + max_length=50, + help_text=_( + "Name of the claim in which the ID of the person representing a company is stored" + ), + ) + oidc_rp_scopes_list = ArrayField( + verbose_name=_("OpenID Connect scopes"), + base_field=models.CharField(_("OpenID Connect scope"), max_length=50), + default=get_default_scopes_bsn, + blank=True, + help_text=_( + "OpenID Connect scopes that are requested during login. " + "These scopes are hardcoded and must be supported by the identity provider" + ), + ) + + custom_oidc_db_prefix: ClassVar[str] = "eherkenning_bewindvoering_oidc" + + class Meta: + verbose_name = _("OpenID Connect configuration for eHerkenning Bewindvoering") + + @property + def digid_eherkenning_machtigen_claims(self) -> dict[str, ClaimPath]: + # TODO: this nomenclature isn't entirely correct + return { + "vertegenwoordigde": [self.vertegenwoordigde_company_claim_name], + "gemachtigde": [self.gemachtigde_person_claim_name], + } + + @property + def oidcdb_sensitive_claims(self) -> Collection[ClaimPath]: + return list(self.digid_eherkenning_machtigen_claims.values()) diff --git a/digid_eherkenning/oidc/tests/__init__.py b/digid_eherkenning/oidc/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/digid_eherkenning/oidc/tests/test_callback_endpoints.py b/digid_eherkenning/oidc/tests/test_callback_endpoints.py new file mode 100644 index 0000000..da73bf3 --- /dev/null +++ b/digid_eherkenning/oidc/tests/test_callback_endpoints.py @@ -0,0 +1,45 @@ +from django.test import SimpleTestCase, override_settings +from django.urls import reverse + +from ..models import ( + OpenIDConnectDigiDMachtigenConfig, + OpenIDConnectEHerkenningBewindvoeringConfig, + OpenIDConnectEHerkenningConfig, + OpenIDConnectPublicConfig, +) + + +class CallbackEndpointTests(SimpleTestCase): + + @override_settings(USE_LEGACY_DIGID_EH_OIDC_ENDPOINTS=True) + def test_legacy_behaviour(self): + expected = ( + (OpenIDConnectPublicConfig, "/digid-oidc/callback/"), + (OpenIDConnectEHerkenningConfig, "/eherkenning-oidc/callback/"), + (OpenIDConnectDigiDMachtigenConfig, "/digid-machtigen-oidc/callback/"), + ( + OpenIDConnectEHerkenningBewindvoeringConfig, + "/eherkenning-bewindvoering-oidc/callback/", + ), + ) + + for config, expected_path in expected: + with self.subTest(config=config): + callback_path = reverse(config.oidc_authentication_callback_url) + + self.assertEqual(callback_path, expected_path) + + @override_settings(USE_LEGACY_DIGID_EH_OIDC_ENDPOINTS=False) + def test_new_behaviour(self): + expected = ( + OpenIDConnectPublicConfig, + OpenIDConnectEHerkenningConfig, + OpenIDConnectDigiDMachtigenConfig, + OpenIDConnectEHerkenningBewindvoeringConfig, + ) + + for config in expected: + with self.subTest(config=config): + callback_path = reverse(config.oidc_authentication_callback_url) + + self.assertEqual(callback_path, "/auth/oidc/callback/") diff --git a/digid_eherkenning/oidc/utils.py b/digid_eherkenning/oidc/utils.py new file mode 100644 index 0000000..84b29d4 --- /dev/null +++ b/digid_eherkenning/oidc/utils.py @@ -0,0 +1,29 @@ +import logging + +import requests + +from .models import OpenIDConnectBaseConfig + +logger = logging.getLogger(__name__) + + +def do_op_logout(config: OpenIDConnectBaseConfig, id_token: str) -> None: + """ + Perform the logout with the OpenID Provider. + + Standard: https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout + """ + logout_endpoint = config.oidc_op_logout_endpoint + if not logout_endpoint: + return + + response = requests.post(logout_endpoint, data={"id_token_hint": id_token}) + if not response.ok: + logger.warning( + "Failed to log out the user at the OpenID Provider. Status code: %s", + response.status_code, + extra={ + "response": response, + "status_code": response.status_code, + }, + ) diff --git a/digid_eherkenning/oidc/views.py b/digid_eherkenning/oidc/views.py new file mode 100644 index 0000000..ec9e4c3 --- /dev/null +++ b/digid_eherkenning/oidc/views.py @@ -0,0 +1,62 @@ +import logging + +from django.contrib.auth.models import AbstractBaseUser, AnonymousUser +from django.http import HttpResponseRedirect + +from mozilla_django_oidc_db.views import ( + OIDCAuthenticationCallbackView as BaseCallbackView, +) + +logger = logging.getLogger(__name__) + + +class OIDCAuthenticationCallbackView(BaseCallbackView): + """ + Check if the 'created user' from the authentication backend needs to be logged in. + + If we only want to perform the claim processing, then no real user is expected to + be returend from the authentication backend, and hence we also don't want to try + to log in in this dummy user (as in, set ``request.user`` to a django user + instance). + + Note that we deliberately don't perform these changes in :meth:`get` (anymore), + since we miss the upstream library changes/fixes when we make invasive changes. + Instead, the authentication backend receives all the necessary information and *is* + the right place to implement this logic. + """ + + expect_django_user: bool = True + """ + Set to ``True`` if a Django user is expected to be created by the backend. + + The OIDC backend is used just to obtain the claims via OIDC, but doesn't always need + to result in a real Django user record being created. + """ + + user: AbstractBaseUser | AnonymousUser # set on succesful auth/code exchange + + def login_success(self): + """ + Overridden to not actually log the user in, since setting the BSN/KVK/... in + the session variables is all that matters. + """ + assert self.user + + match (self.expect_django_user, self.user.pk): + case (False, pk) if pk is not None: + raise TypeError( + "A real Django user instance was returned from the authentication " + "backend. This is a configuration/programming mistake!" + ) + case (True, None): + raise TypeError( + "A fake Django user instance was returned from the authentication " + "backend. This is a configuration/programming mistake!" + ) + + # default behaviour which logs the user in *iff* we expect a real Django user + # to be returned, otherwise ignore all that. + if self.expect_django_user: + return super().login_success() + + return HttpResponseRedirect(self.success_url) diff --git a/setup.cfg b/setup.cfg index e8edf86..046a0b4 100644 --- a/setup.cfg +++ b/setup.cfg @@ -58,6 +58,8 @@ include = digid_eherkenning.* [options.extras_require] +oidc = + mozilla-django-oidc-db >= 0.17.0 tests = django-test-migrations pytest From 6f666c107cac7d2a3212a3cc05146eb7cfeb0ada Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 24 May 2024 17:49:07 +0200 Subject: [PATCH 02/25] :pencil: Document digid-eherkenning[oidc] --- digid_eherkenning/oidc/views.py | 17 ++++++++ docs/index.rst | 1 + docs/oidc.rst | 70 +++++++++++++++++++++++++++++++++ docs/quickstart.rst | 2 + 4 files changed, 90 insertions(+) create mode 100644 docs/oidc.rst diff --git a/digid_eherkenning/oidc/views.py b/digid_eherkenning/oidc/views.py index ec9e4c3..bb5705b 100644 --- a/digid_eherkenning/oidc/views.py +++ b/digid_eherkenning/oidc/views.py @@ -5,6 +5,15 @@ from mozilla_django_oidc_db.views import ( OIDCAuthenticationCallbackView as BaseCallbackView, + OIDCInit, + OIDInit, +) + +from .models import ( + OpenIDConnectDigiDMachtigenConfig, + OpenIDConnectEHerkenningBewindvoeringConfig, + OpenIDConnectEHerkenningConfig, + OpenIDConnectPublicConfig, ) logger = logging.getLogger(__name__) @@ -60,3 +69,11 @@ def login_success(self): return super().login_success() return HttpResponseRedirect(self.success_url) + + +didid_init = OIDCInit.as_view(config_class=OpenIDConnectPublicConfig) +eh_init = OIDCInit.as_view(config_class=OpenIDConnectEHerkenningConfig) +digid_machtigen_init = OIDCInit.as_view( + config_class=OpenIDConnectEHerkenningBewindvoeringConfig +) +eh_bewindvoering_init = OIDCInit.as_view(config_class=OpenIDConnectDigiDMachtigenConfig) diff --git a/docs/index.rst b/docs/index.rst index 261ece3..967dca0 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -20,6 +20,7 @@ Features :caption: Contents: quickstart + oidc configuration metadata cli diff --git a/docs/oidc.rst b/docs/oidc.rst new file mode 100644 index 0000000..965c2ec --- /dev/null +++ b/docs/oidc.rst @@ -0,0 +1,70 @@ +.. _oidc: + +====================================== +DigiD & eHerkenning via OpenID Connect +====================================== + +Some brokers expose a DigiD/eHerkenning identity provider via the OpenID Connect +protocol. The :mod:`digid_eherkenning.oidc` package provides support for this. + +The tooling for this builds on top of `mozilla-django-oidc-db`_, so those installation +instructions are also relevant. + +Installation +============ + +Install with the optional dependency group: + +.. code-block:: bash + + pip install django-digid-eherkenning[oidc] + +and add the necessary packages to ``INSTALLED_APPS``: + +.. code-block:: python + + INSTALLED_APPS = [ + ..., + "privates", + "simple_certmanager", + "django_jsonform", + "solo", + "mozilla_django_oidc", + "mozilla_django_oidc_db", + "digid_eherkenning", + "digid_eherkenning.oidc", + ..., + ] + +Make sure to follow mozilla-django-oidc-db's installation instructions too. + + +Authentication backend +====================== + +If you wish to create Django users for the users authenticating via OIDC, you need to +set up an authentication backend and add it to ``settings.AUTHENTICATION_BACKENDS``. + +We recommend to subclass :class:`mozilla_django_oidc_db.backends.OIDCAuthenticationBackend`, +and you can mix in some utilities from :mod:`digid_eherkenning.backends`. + +This package does not (yet) ship a default backend for user creation. + +Configuration +============= + +The OpenID Connect configuration is managed in the admin, where you control the Relying +Party aspects. + +Views +===== + +This package exposes initialization views, which you can hook up to your URL conf, or +incorporate in your own initialization flow views: + +* :attr:`digid_eherkenning.oidc.views.digid_init` +* :attr:`digid_eherkenning.oidc.views.eh_init` +* :attr:`digid_eherkenning.oidc.views.digid_machtigen_init` +* :attr:`digid_eherkenning.oidc.views.eh_bewindvoering_init` + +.. _mozilla-django-oidc-db: https://mozilla-django-oidc-db.readthedocs.io/en/latest/ diff --git a/docs/quickstart.rst b/docs/quickstart.rst index 0048117..8a5fe4a 100644 --- a/docs/quickstart.rst +++ b/docs/quickstart.rst @@ -18,6 +18,8 @@ Requirements Installation ------------ +See :ref:`oidc` for configuring the OIDC flavour. + Install with pip: .. code-block:: bash From 5c607f13bdca9be5f7816c1efe5c41d3e2f0fbe4 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 24 May 2024 17:50:28 +0200 Subject: [PATCH 03/25] :alien: GET logout is no longer allowed in modern Django --- tests/test_mock_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_mock_views.py b/tests/test_mock_views.py index d78e680..548b235 100644 --- a/tests/test_mock_views.py +++ b/tests/test_mock_views.py @@ -168,7 +168,7 @@ def test_logout(self): self.client.force_login(user) url = reverse("digid:logout") - response = self.client.get(url) + response = self.client.post(url) self.assertEqual(response.status_code, 302) self.assertFalse("_auth_user_id" in self.client.session) From 38af28c8aa16687e160676667f01e30364e8206c Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 24 May 2024 17:52:59 +0200 Subject: [PATCH 04/25] :fire: Drop obsoleted tests We only support a single endpoint going forward. --- digid_eherkenning/oidc/tests/__init__.py | 0 .../oidc/tests/test_callback_endpoints.py | 45 ------------------- 2 files changed, 45 deletions(-) delete mode 100644 digid_eherkenning/oidc/tests/__init__.py delete mode 100644 digid_eherkenning/oidc/tests/test_callback_endpoints.py diff --git a/digid_eherkenning/oidc/tests/__init__.py b/digid_eherkenning/oidc/tests/__init__.py deleted file mode 100644 index e69de29..0000000 diff --git a/digid_eherkenning/oidc/tests/test_callback_endpoints.py b/digid_eherkenning/oidc/tests/test_callback_endpoints.py deleted file mode 100644 index da73bf3..0000000 --- a/digid_eherkenning/oidc/tests/test_callback_endpoints.py +++ /dev/null @@ -1,45 +0,0 @@ -from django.test import SimpleTestCase, override_settings -from django.urls import reverse - -from ..models import ( - OpenIDConnectDigiDMachtigenConfig, - OpenIDConnectEHerkenningBewindvoeringConfig, - OpenIDConnectEHerkenningConfig, - OpenIDConnectPublicConfig, -) - - -class CallbackEndpointTests(SimpleTestCase): - - @override_settings(USE_LEGACY_DIGID_EH_OIDC_ENDPOINTS=True) - def test_legacy_behaviour(self): - expected = ( - (OpenIDConnectPublicConfig, "/digid-oidc/callback/"), - (OpenIDConnectEHerkenningConfig, "/eherkenning-oidc/callback/"), - (OpenIDConnectDigiDMachtigenConfig, "/digid-machtigen-oidc/callback/"), - ( - OpenIDConnectEHerkenningBewindvoeringConfig, - "/eherkenning-bewindvoering-oidc/callback/", - ), - ) - - for config, expected_path in expected: - with self.subTest(config=config): - callback_path = reverse(config.oidc_authentication_callback_url) - - self.assertEqual(callback_path, expected_path) - - @override_settings(USE_LEGACY_DIGID_EH_OIDC_ENDPOINTS=False) - def test_new_behaviour(self): - expected = ( - OpenIDConnectPublicConfig, - OpenIDConnectEHerkenningConfig, - OpenIDConnectDigiDMachtigenConfig, - OpenIDConnectEHerkenningBewindvoeringConfig, - ) - - for config in expected: - with self.subTest(config=config): - callback_path = reverse(config.oidc_authentication_callback_url) - - self.assertEqual(callback_path, "/auth/oidc/callback/") From 5a761af3b920b8701c5b8bcfd152029282c11f20 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 24 May 2024 17:56:34 +0200 Subject: [PATCH 05/25] :sparkles: Specify default_auto_field in backwards compatible manner --- digid_eherkenning/oidc/apps.py | 1 + tests/project/settings.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/digid_eherkenning/oidc/apps.py b/digid_eherkenning/oidc/apps.py index 9b3d369..41276ad 100644 --- a/digid_eherkenning/oidc/apps.py +++ b/digid_eherkenning/oidc/apps.py @@ -7,3 +7,4 @@ class OIDCAppConfig(AppConfig): verbose_name = _("DigiD & eHerkenning via OpenID Connect") # can't change this label because of existing migrations in Open Forms/Open Inwoner label = "digid_eherkenning_oidc_generics" + default_auto_field = "django.db.models.AutoField" diff --git a/tests/project/settings.py b/tests/project/settings.py index 326b635..9cce8c7 100644 --- a/tests/project/settings.py +++ b/tests/project/settings.py @@ -94,6 +94,8 @@ } } +DEFAULT_AUTO_FIELD = "django.db.models.AutoField" + # Password validation # https://docs.djangoproject.com/en/3.0/ref/settings/#auth-password-validators From a296a83ef03cdf25b8046e9f540e614186845329 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 24 May 2024 18:06:08 +0200 Subject: [PATCH 06/25] :card_file_box: Rename models for better representation of content --- digid_eherkenning/oidc/admin.py | 24 +++++++-------- ...nitial_squashed_0007_auto_20221213_1347.py | 11 +++---- ...onnectpublicconfig_digidconfig_and_more.py | 29 +++++++++++++++++++ digid_eherkenning/oidc/models.py | 8 ++--- digid_eherkenning/oidc/views.py | 19 +++++------- tests/project/settings.py | 11 +++++++ 6 files changed, 70 insertions(+), 32 deletions(-) create mode 100644 digid_eherkenning/oidc/migrations/0003_rename_openidconnectpublicconfig_digidconfig_and_more.py diff --git a/digid_eherkenning/oidc/admin.py b/digid_eherkenning/oidc/admin.py index 7188839..29fd611 100644 --- a/digid_eherkenning/oidc/admin.py +++ b/digid_eherkenning/oidc/admin.py @@ -8,10 +8,10 @@ from .forms import admin_modelform_factory from .models import ( - OpenIDConnectDigiDMachtigenConfig, - OpenIDConnectEHerkenningBewindvoeringConfig, - OpenIDConnectEHerkenningConfig, - OpenIDConnectPublicConfig, + DigiDConfig, + DigiDMachtigenConfig, + EHerkenningBewindvoeringConfig, + EHerkenningConfig, ) # Using a dict because these retain ordering, and it makes things a bit more readable. @@ -69,21 +69,21 @@ def fieldsets_factory(claim_mapping_fields: Sequence[str]): return tuple((label, config) for label, config in _fieldsets.items()) -@admin.register(OpenIDConnectPublicConfig) +@admin.register(DigiDConfig) class OpenIDConnectConfigDigiDAdmin(SingletonModelAdmin): - form = admin_modelform_factory(OpenIDConnectPublicConfig) + form = admin_modelform_factory(DigiDConfig) fieldsets = fieldsets_factory(claim_mapping_fields=["identifier_claim_name"]) -@admin.register(OpenIDConnectEHerkenningConfig) +@admin.register(EHerkenningConfig) class OpenIDConnectConfigEHerkenningAdmin(SingletonModelAdmin): - form = admin_modelform_factory(OpenIDConnectEHerkenningConfig) + form = admin_modelform_factory(EHerkenningConfig) fieldsets = fieldsets_factory(claim_mapping_fields=["identifier_claim_name"]) -@admin.register(OpenIDConnectDigiDMachtigenConfig) +@admin.register(DigiDMachtigenConfig) class OpenIDConnectConfigDigiDMachtigenAdmin(SingletonModelAdmin): - form = admin_modelform_factory(OpenIDConnectDigiDMachtigenConfig) + form = admin_modelform_factory(DigiDMachtigenConfig) fieldsets = fieldsets_factory( claim_mapping_fields=[ "vertegenwoordigde_claim_name", @@ -92,9 +92,9 @@ class OpenIDConnectConfigDigiDMachtigenAdmin(SingletonModelAdmin): ) -@admin.register(OpenIDConnectEHerkenningBewindvoeringConfig) +@admin.register(EHerkenningBewindvoeringConfig) class OpenIDConnectConfigEHerkenningBewindvoeringAdmin(SingletonModelAdmin): - form = admin_modelform_factory(OpenIDConnectEHerkenningBewindvoeringConfig) + form = admin_modelform_factory(EHerkenningBewindvoeringConfig) fieldsets = fieldsets_factory( claim_mapping_fields=[ "vertegenwoordigde_company_claim_name", diff --git a/digid_eherkenning/oidc/migrations/0001_initial_squashed_0007_auto_20221213_1347.py b/digid_eherkenning/oidc/migrations/0001_initial_squashed_0007_auto_20221213_1347.py index d619500..b78e5e7 100644 --- a/digid_eherkenning/oidc/migrations/0001_initial_squashed_0007_auto_20221213_1347.py +++ b/digid_eherkenning/oidc/migrations/0001_initial_squashed_0007_auto_20221213_1347.py @@ -2,10 +2,11 @@ from django.db import migrations, models -import digid_eherkenning_oidc_generics.models import django_jsonform.models.fields import mozilla_django_oidc_db.models +import digid_eherkenning.oidc.models + class Migration(migrations.Migration): dependencies = [] @@ -150,7 +151,7 @@ class Migration(migrations.Migration): max_length=50, verbose_name="OpenID Connect scope" ), blank=True, - default=digid_eherkenning_oidc_generics.models.get_default_scopes_bsn, + default=digid_eherkenning.oidc.models.get_default_scopes_bsn, help_text="OpenID Connect scopes that are requested during login. These scopes are hardcoded and must be supported by the identity provider", size=None, verbose_name="OpenID Connect scopes", @@ -388,7 +389,7 @@ class Migration(migrations.Migration): max_length=50, verbose_name="OpenID Connect scope" ), blank=True, - default=digid_eherkenning_oidc_generics.models.get_default_scopes_bsn, + default=digid_eherkenning.oidc.models.get_default_scopes_bsn, help_text="OpenID Connect scopes that are requested during login. These scopes are hardcoded and must be supported by the identity provider", size=None, verbose_name="OpenID Connect scopes", @@ -543,7 +544,7 @@ class Migration(migrations.Migration): max_length=50, verbose_name="OpenID Connect scope" ), blank=True, - default=digid_eherkenning_oidc_generics.models.get_default_scopes_kvk, + default=digid_eherkenning.oidc.models.get_default_scopes_kvk, help_text="OpenID Connect scopes that are requested during login. These scopes are hardcoded and must be supported by the identity provider", size=None, verbose_name="OpenID Connect scopes", @@ -735,7 +736,7 @@ class Migration(migrations.Migration): max_length=50, verbose_name="OpenID Connect scope" ), blank=True, - default=digid_eherkenning_oidc_generics.models.get_default_scopes_bsn, + default=digid_eherkenning.oidc.models.get_default_scopes_bsn, help_text="OpenID Connect scopes that are requested during login. These scopes are hardcoded and must be supported by the identity provider", size=None, verbose_name="OpenID Connect scopes", diff --git a/digid_eherkenning/oidc/migrations/0003_rename_openidconnectpublicconfig_digidconfig_and_more.py b/digid_eherkenning/oidc/migrations/0003_rename_openidconnectpublicconfig_digidconfig_and_more.py new file mode 100644 index 0000000..6cd8064 --- /dev/null +++ b/digid_eherkenning/oidc/migrations/0003_rename_openidconnectpublicconfig_digidconfig_and_more.py @@ -0,0 +1,29 @@ +# Generated by Django 4.2.13 on 2024-05-24 16:04 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("digid_eherkenning_oidc_generics", "0002_auto_20240207_1546"), + ] + + operations = [ + migrations.RenameModel( + old_name="OpenIDConnectPublicConfig", + new_name="DigiDConfig", + ), + migrations.RenameModel( + old_name="OpenIDConnectDigiDMachtigenConfig", + new_name="DigiDMachtigenConfig", + ), + migrations.RenameModel( + old_name="OpenIDConnectEHerkenningBewindvoeringConfig", + new_name="EHerkenningBewindvoeringConfig", + ), + migrations.RenameModel( + old_name="OpenIDConnectEHerkenningConfig", + new_name="EHerkenningConfig", + ), + ] diff --git a/digid_eherkenning/oidc/models.py b/digid_eherkenning/oidc/models.py index adbf506..d4326f4 100644 --- a/digid_eherkenning/oidc/models.py +++ b/digid_eherkenning/oidc/models.py @@ -56,7 +56,7 @@ def oidcdb_check_idp_availability(cls) -> bool: return True -class OpenIDConnectPublicConfig(OpenIDConnectBaseConfig): +class DigiDConfig(OpenIDConnectBaseConfig): """ Configuration for DigiD authentication via OpenID connect """ @@ -88,7 +88,7 @@ def oidcdb_username_claim(self) -> list[str]: return [self.identifier_claim_name] -class OpenIDConnectDigiDMachtigenConfig(OpenIDConnectBaseConfig): +class DigiDMachtigenConfig(OpenIDConnectBaseConfig): # TODO: support periods in claim keys vertegenwoordigde_claim_name = models.CharField( verbose_name=_("vertegenwoordigde claim name"), @@ -134,7 +134,7 @@ def oidcdb_sensitive_claims(self) -> Collection[ClaimPath]: return list(self.digid_eherkenning_machtigen_claims.values()) -class OpenIDConnectEHerkenningConfig(OpenIDConnectBaseConfig): +class EHerkenningConfig(OpenIDConnectBaseConfig): """ Configuration for eHerkenning authentication via OpenID connect """ @@ -166,7 +166,7 @@ def oidcdb_username_claim(self) -> list[str]: return [self.identifier_claim_name] -class OpenIDConnectEHerkenningBewindvoeringConfig(OpenIDConnectBaseConfig): +class EHerkenningBewindvoeringConfig(OpenIDConnectBaseConfig): # TODO: support periods in claim keys vertegenwoordigde_company_claim_name = models.CharField( verbose_name=_("vertegenwoordigde company claim name"), diff --git a/digid_eherkenning/oidc/views.py b/digid_eherkenning/oidc/views.py index bb5705b..bc084bc 100644 --- a/digid_eherkenning/oidc/views.py +++ b/digid_eherkenning/oidc/views.py @@ -6,14 +6,13 @@ from mozilla_django_oidc_db.views import ( OIDCAuthenticationCallbackView as BaseCallbackView, OIDCInit, - OIDInit, ) from .models import ( - OpenIDConnectDigiDMachtigenConfig, - OpenIDConnectEHerkenningBewindvoeringConfig, - OpenIDConnectEHerkenningConfig, - OpenIDConnectPublicConfig, + DigiDConfig, + DigiDMachtigenConfig, + EHerkenningBewindvoeringConfig, + EHerkenningConfig, ) logger = logging.getLogger(__name__) @@ -71,9 +70,7 @@ def login_success(self): return HttpResponseRedirect(self.success_url) -didid_init = OIDCInit.as_view(config_class=OpenIDConnectPublicConfig) -eh_init = OIDCInit.as_view(config_class=OpenIDConnectEHerkenningConfig) -digid_machtigen_init = OIDCInit.as_view( - config_class=OpenIDConnectEHerkenningBewindvoeringConfig -) -eh_bewindvoering_init = OIDCInit.as_view(config_class=OpenIDConnectDigiDMachtigenConfig) +didid_init = OIDCInit.as_view(config_class=DigiDConfig) +eh_init = OIDCInit.as_view(config_class=EHerkenningConfig) +digid_machtigen_init = OIDCInit.as_view(config_class=EHerkenningBewindvoeringConfig) +eh_bewindvoering_init = OIDCInit.as_view(config_class=DigiDMachtigenConfig) diff --git a/tests/project/settings.py b/tests/project/settings.py index 9cce8c7..7cfc878 100644 --- a/tests/project/settings.py +++ b/tests/project/settings.py @@ -42,9 +42,13 @@ "sessionprofile", "privates", "simple_certmanager", + "django_jsonform", "solo", + "mozilla_django_oidc", + "mozilla_django_oidc_db", # own apps "digid_eherkenning", + "digid_eherkenning.oidc", "tests.project", ] @@ -157,3 +161,10 @@ # DIGID_MOCK_IDP_LOGIN_URL = 'http://localhost:8008/digid/idp/inloggen/' # DIGID_MOCK_RETURN_URL = '/' # DIGID_MOCK_CANCEL_URL = '/' + +# These settings are evaluated at import-time of the urlconf in mozilla_django_oidc.urls. +# Changing them via @override_settings (or the pytest-django settings fixture) has no +# effect. +OIDC_AUTHENTICATE_CLASS = "mozilla_django_oidc_db.views.OIDCAuthenticationRequestView" +OIDC_CALLBACK_CLASS = "mozilla_django_oidc_db.views.OIDCCallbackView" +LOGIN_REDIRECT_URL = reverse_lazy("admin:index") From 0bedd5c093737047f4f42227084c25d110b127c7 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 11:03:55 +0200 Subject: [PATCH 07/25] :truck: Split models into separate modules Organizing by DigiD and eHerkenning, similarly to the SAML models. --- digid_eherkenning/oidc/forms.py | 2 +- digid_eherkenning/oidc/models.py | 213 ------------------- digid_eherkenning/oidc/models/__init__.py | 12 ++ digid_eherkenning/oidc/models/base.py | 31 +++ digid_eherkenning/oidc/models/digid.py | 83 ++++++++ digid_eherkenning/oidc/models/eherkenning.py | 88 ++++++++ 6 files changed, 215 insertions(+), 214 deletions(-) delete mode 100644 digid_eherkenning/oidc/models.py create mode 100644 digid_eherkenning/oidc/models/__init__.py create mode 100644 digid_eherkenning/oidc/models/base.py create mode 100644 digid_eherkenning/oidc/models/digid.py create mode 100644 digid_eherkenning/oidc/models/eherkenning.py diff --git a/digid_eherkenning/oidc/forms.py b/digid_eherkenning/oidc/forms.py index aeed63d..2aa32f1 100644 --- a/digid_eherkenning/oidc/forms.py +++ b/digid_eherkenning/oidc/forms.py @@ -5,7 +5,7 @@ from mozilla_django_oidc_db.constants import OIDC_MAPPING from mozilla_django_oidc_db.forms import OpenIDConnectConfigForm -from .models import OpenIDConnectBaseConfig +from .models.base import OpenIDConnectBaseConfig def admin_modelform_factory(model: type[OpenIDConnectBaseConfig], *args, **kwargs): diff --git a/digid_eherkenning/oidc/models.py b/digid_eherkenning/oidc/models.py deleted file mode 100644 index d4326f4..0000000 --- a/digid_eherkenning/oidc/models.py +++ /dev/null @@ -1,213 +0,0 @@ -from collections.abc import Collection -from typing import ClassVar - -from django.db import models -from django.utils.functional import classproperty -from django.utils.translation import gettext_lazy as _ - -from django_jsonform.models.fields import ArrayField -from mozilla_django_oidc_db.models import OpenIDConnectConfigBase -from mozilla_django_oidc_db.typing import ClaimPath - - -def get_default_scopes_bsn(): - """ - Returns the default scopes to request for OpenID Connect logins - """ - return ["openid", "bsn"] - - -def get_default_scopes_kvk(): - """ - Returns the default scopes to request for OpenID Connect logins - """ - return ["openid", "kvk"] - - -class OpenIDConnectBaseConfig(OpenIDConnectConfigBase): - """ - Configuration for DigiD authentication via OpenID connect - """ - - oidc_op_logout_endpoint = models.URLField( - _("Logout endpoint"), - max_length=1000, - help_text=_("URL of your OpenID Connect provider logout endpoint"), - blank=True, - ) - - # Keycloak specific config - oidc_keycloak_idp_hint = models.CharField( - _("Keycloak Identity Provider hint"), - max_length=1000, - help_text=_( - "Specific for Keycloak: parameter that indicates which identity provider " - "should be used (therefore skipping the Keycloak login screen)." - ), - blank=True, - ) - - class Meta: - verbose_name = _("OpenID Connect configuration") - abstract = True - - @classproperty - def oidcdb_check_idp_availability(cls) -> bool: - return True - - -class DigiDConfig(OpenIDConnectBaseConfig): - """ - Configuration for DigiD authentication via OpenID connect - """ - - identifier_claim_name = models.CharField( - _("BSN claim name"), - max_length=100, - help_text=_("The name of the claim in which the BSN of the user is stored"), - default="bsn", - ) - oidc_rp_scopes_list = ArrayField( - verbose_name=_("OpenID Connect scopes"), - base_field=models.CharField(_("OpenID Connect scope"), max_length=50), - default=get_default_scopes_bsn, - blank=True, - help_text=_( - "OpenID Connect scopes that are requested during login. " - "These scopes are hardcoded and must be supported by the identity provider" - ), - ) - - custom_oidc_db_prefix: ClassVar[str] = "digid_oidc" - - class Meta: - verbose_name = _("OpenID Connect configuration for DigiD") - - @property - def oidcdb_username_claim(self) -> list[str]: - return [self.identifier_claim_name] - - -class DigiDMachtigenConfig(OpenIDConnectBaseConfig): - # TODO: support periods in claim keys - vertegenwoordigde_claim_name = models.CharField( - verbose_name=_("vertegenwoordigde claim name"), - default="aanvrager.bsn", - max_length=50, - help_text=_( - "Name of the claim in which the BSN of the person being represented is stored" - ), - ) - gemachtigde_claim_name = models.CharField( - verbose_name=_("gemachtigde claim name"), - default="gemachtigde.bsn", - max_length=50, - help_text=_( - "Name of the claim in which the BSN of the person representing someone else is stored" - ), - ) - oidc_rp_scopes_list = ArrayField( - verbose_name=_("OpenID Connect scopes"), - base_field=models.CharField(_("OpenID Connect scope"), max_length=50), - default=get_default_scopes_bsn, - blank=True, - help_text=_( - "OpenID Connect scopes that are requested during login. " - "These scopes are hardcoded and must be supported by the identity provider" - ), - ) - - custom_oidc_db_prefix: ClassVar[str] = "digid_machtigen_oidc" - - class Meta: - verbose_name = _("OpenID Connect configuration for DigiD Machtigen") - - @property - def digid_eherkenning_machtigen_claims(self) -> dict[str, ClaimPath]: - return { - "vertegenwoordigde": [self.vertegenwoordigde_claim_name], - "gemachtigde": [self.gemachtigde_claim_name], - } - - @property - def oidcdb_sensitive_claims(self) -> Collection[ClaimPath]: - return list(self.digid_eherkenning_machtigen_claims.values()) - - -class EHerkenningConfig(OpenIDConnectBaseConfig): - """ - Configuration for eHerkenning authentication via OpenID connect - """ - - identifier_claim_name = models.CharField( - _("KVK claim name"), - max_length=100, - help_text=_("The name of the claim in which the KVK of the user is stored"), - default="kvk", - ) - oidc_rp_scopes_list = ArrayField( - verbose_name=_("OpenID Connect scopes"), - base_field=models.CharField(_("OpenID Connect scope"), max_length=50), - default=get_default_scopes_kvk, - blank=True, - help_text=_( - "OpenID Connect scopes that are requested during login. " - "These scopes are hardcoded and must be supported by the identity provider" - ), - ) - - custom_oidc_db_prefix: ClassVar[str] = "eherkenning_oidc" - - class Meta: - verbose_name = _("OpenID Connect configuration for eHerkenning") - - @property - def oidcdb_username_claim(self) -> list[str]: - return [self.identifier_claim_name] - - -class EHerkenningBewindvoeringConfig(OpenIDConnectBaseConfig): - # TODO: support periods in claim keys - vertegenwoordigde_company_claim_name = models.CharField( - verbose_name=_("vertegenwoordigde company claim name"), - default="aanvrager.kvk", - max_length=50, - help_text=_( - "Name of the claim in which the KVK of the company being represented is stored" - ), - ) - gemachtigde_person_claim_name = models.CharField( - verbose_name=_("gemachtigde person claim name"), - default="gemachtigde.pseudoID", - max_length=50, - help_text=_( - "Name of the claim in which the ID of the person representing a company is stored" - ), - ) - oidc_rp_scopes_list = ArrayField( - verbose_name=_("OpenID Connect scopes"), - base_field=models.CharField(_("OpenID Connect scope"), max_length=50), - default=get_default_scopes_bsn, - blank=True, - help_text=_( - "OpenID Connect scopes that are requested during login. " - "These scopes are hardcoded and must be supported by the identity provider" - ), - ) - - custom_oidc_db_prefix: ClassVar[str] = "eherkenning_bewindvoering_oidc" - - class Meta: - verbose_name = _("OpenID Connect configuration for eHerkenning Bewindvoering") - - @property - def digid_eherkenning_machtigen_claims(self) -> dict[str, ClaimPath]: - # TODO: this nomenclature isn't entirely correct - return { - "vertegenwoordigde": [self.vertegenwoordigde_company_claim_name], - "gemachtigde": [self.gemachtigde_person_claim_name], - } - - @property - def oidcdb_sensitive_claims(self) -> Collection[ClaimPath]: - return list(self.digid_eherkenning_machtigen_claims.values()) diff --git a/digid_eherkenning/oidc/models/__init__.py b/digid_eherkenning/oidc/models/__init__.py new file mode 100644 index 0000000..52f9b95 --- /dev/null +++ b/digid_eherkenning/oidc/models/__init__.py @@ -0,0 +1,12 @@ +from .base import get_default_scopes_bsn, get_default_scopes_kvk +from .digid import DigiDConfig, DigiDMachtigenConfig +from .eherkenning import EHerkenningBewindvoeringConfig, EHerkenningConfig + +__all__ = [ + "get_default_scopes_bsn", + "get_default_scopes_kvk", + "DigiDConfig", + "DigiDMachtigenConfig", + "EHerkenningConfig", + "EHerkenningBewindvoeringConfig", +] diff --git a/digid_eherkenning/oidc/models/base.py b/digid_eherkenning/oidc/models/base.py new file mode 100644 index 0000000..beef3cf --- /dev/null +++ b/digid_eherkenning/oidc/models/base.py @@ -0,0 +1,31 @@ +from django.utils.functional import classproperty +from django.utils.translation import gettext_lazy as _ + +from mozilla_django_oidc_db.models import OpenIDConnectConfigBase + + +def get_default_scopes_bsn(): + """ + Returns the default scopes to request for OpenID Connect logins + """ + return ["openid", "bsn"] + + +def get_default_scopes_kvk(): + """ + Returns the default scopes to request for OpenID Connect logins + """ + return ["openid", "kvk"] + + +class OpenIDConnectBaseConfig(OpenIDConnectConfigBase): + """ + Base configuration for DigiD/eHerkenning authentication via OpenID Connect. + """ + + class Meta: + abstract = True + + @classproperty + def oidcdb_check_idp_availability(cls) -> bool: + return True diff --git a/digid_eherkenning/oidc/models/digid.py b/digid_eherkenning/oidc/models/digid.py new file mode 100644 index 0000000..03b2091 --- /dev/null +++ b/digid_eherkenning/oidc/models/digid.py @@ -0,0 +1,83 @@ +from collections.abc import Collection + +from django.db import models +from django.utils.translation import gettext_lazy as _ + +from django_jsonform.models.fields import ArrayField +from mozilla_django_oidc_db.typing import ClaimPath + +from .base import OpenIDConnectBaseConfig, get_default_scopes_bsn + + +class DigiDConfig(OpenIDConnectBaseConfig): + """ + Configuration for DigiD authentication via OpenID connect + """ + + identifier_claim_name = models.CharField( + _("BSN claim name"), + max_length=100, + help_text=_("The name of the claim in which the BSN of the user is stored"), + default="bsn", + ) + oidc_rp_scopes_list = ArrayField( + verbose_name=_("OpenID Connect scopes"), + base_field=models.CharField(_("OpenID Connect scope"), max_length=50), + default=get_default_scopes_bsn, + blank=True, + help_text=_( + "OpenID Connect scopes that are requested during login. " + "These scopes are hardcoded and must be supported by the identity provider" + ), + ) + + class Meta: + verbose_name = _("OpenID Connect configuration for DigiD") + + @property + def oidcdb_username_claim(self) -> list[str]: + return [self.identifier_claim_name] + + +class DigiDMachtigenConfig(OpenIDConnectBaseConfig): + # TODO: support periods in claim keys + vertegenwoordigde_claim_name = models.CharField( + verbose_name=_("vertegenwoordigde claim name"), + default="aanvrager.bsn", + max_length=50, + help_text=_( + "Name of the claim in which the BSN of the person being represented is stored" + ), + ) + gemachtigde_claim_name = models.CharField( + verbose_name=_("gemachtigde claim name"), + default="gemachtigde.bsn", + max_length=50, + help_text=_( + "Name of the claim in which the BSN of the person representing someone else is stored" + ), + ) + oidc_rp_scopes_list = ArrayField( + verbose_name=_("OpenID Connect scopes"), + base_field=models.CharField(_("OpenID Connect scope"), max_length=50), + default=get_default_scopes_bsn, + blank=True, + help_text=_( + "OpenID Connect scopes that are requested during login. " + "These scopes are hardcoded and must be supported by the identity provider" + ), + ) + + class Meta: + verbose_name = _("OpenID Connect configuration for DigiD Machtigen") + + @property + def digid_eherkenning_machtigen_claims(self) -> dict[str, ClaimPath]: + return { + "vertegenwoordigde": [self.vertegenwoordigde_claim_name], + "gemachtigde": [self.gemachtigde_claim_name], + } + + @property + def oidcdb_sensitive_claims(self) -> Collection[ClaimPath]: + return list(self.digid_eherkenning_machtigen_claims.values()) diff --git a/digid_eherkenning/oidc/models/eherkenning.py b/digid_eherkenning/oidc/models/eherkenning.py new file mode 100644 index 0000000..bef6162 --- /dev/null +++ b/digid_eherkenning/oidc/models/eherkenning.py @@ -0,0 +1,88 @@ +from collections.abc import Collection + +from django.db import models +from django.utils.translation import gettext_lazy as _ + +from django_jsonform.models.fields import ArrayField +from mozilla_django_oidc_db.typing import ClaimPath + +from .base import ( + OpenIDConnectBaseConfig, + get_default_scopes_bsn, + get_default_scopes_kvk, +) + + +class EHerkenningConfig(OpenIDConnectBaseConfig): + """ + Configuration for eHerkenning authentication via OpenID connect + """ + + identifier_claim_name = models.CharField( + _("KVK claim name"), + max_length=100, + help_text=_("The name of the claim in which the KVK of the user is stored"), + default="kvk", + ) + oidc_rp_scopes_list = ArrayField( + verbose_name=_("OpenID Connect scopes"), + base_field=models.CharField(_("OpenID Connect scope"), max_length=50), + default=get_default_scopes_kvk, + blank=True, + help_text=_( + "OpenID Connect scopes that are requested during login. " + "These scopes are hardcoded and must be supported by the identity provider" + ), + ) + + class Meta: + verbose_name = _("OpenID Connect configuration for eHerkenning") + + @property + def oidcdb_username_claim(self) -> list[str]: + return [self.identifier_claim_name] + + +class EHerkenningBewindvoeringConfig(OpenIDConnectBaseConfig): + # TODO: support periods in claim keys + vertegenwoordigde_company_claim_name = models.CharField( + verbose_name=_("vertegenwoordigde company claim name"), + default="aanvrager.kvk", + max_length=50, + help_text=_( + "Name of the claim in which the KVK of the company being represented is stored" + ), + ) + gemachtigde_person_claim_name = models.CharField( + verbose_name=_("gemachtigde person claim name"), + default="gemachtigde.pseudoID", + max_length=50, + help_text=_( + "Name of the claim in which the ID of the person representing a company is stored" + ), + ) + oidc_rp_scopes_list = ArrayField( + verbose_name=_("OpenID Connect scopes"), + base_field=models.CharField(_("OpenID Connect scope"), max_length=50), + default=get_default_scopes_bsn, + blank=True, + help_text=_( + "OpenID Connect scopes that are requested during login. " + "These scopes are hardcoded and must be supported by the identity provider" + ), + ) + + class Meta: + verbose_name = _("OpenID Connect configuration for eHerkenning Bewindvoering") + + @property + def digid_eherkenning_machtigen_claims(self) -> dict[str, ClaimPath]: + # TODO: this nomenclature isn't entirely correct + return { + "vertegenwoordigde": [self.vertegenwoordigde_company_claim_name], + "gemachtigde": [self.gemachtigde_person_claim_name], + } + + @property + def oidcdb_sensitive_claims(self) -> Collection[ClaimPath]: + return list(self.digid_eherkenning_machtigen_claims.values()) From a914579f186dc3295eba75a88dcc064d7d30798d Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 12:05:36 +0200 Subject: [PATCH 08/25] :hammer: Parametrize settings for OIDC enabled/disabled This will allow us to make different test envs/builds to check the correct functioning of the library with and without OIDC. --- tests/project/settings.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/project/settings.py b/tests/project/settings.py index 7cfc878..5532ec4 100644 --- a/tests/project/settings.py +++ b/tests/project/settings.py @@ -31,6 +31,12 @@ # Application definition +_OIDC_APPS = [ + "django_jsonform", + "mozilla_django_oidc", + "mozilla_django_oidc_db", + "digid_eherkenning.oidc", +] INSTALLED_APPS = [ "django.contrib.admin", @@ -42,15 +48,14 @@ "sessionprofile", "privates", "simple_certmanager", - "django_jsonform", "solo", - "mozilla_django_oidc", - "mozilla_django_oidc_db", # own apps "digid_eherkenning", - "digid_eherkenning.oidc", "tests.project", ] +if os.environ.get("OIDC_ENABLED", False): + INSTALLED_APPS += _OIDC_APPS + MIDDLEWARE = [ "django.middleware.security.SecurityMiddleware", From 7f257ad25da80b0d20135aa65d3e9e0c30f33214 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 12:14:24 +0200 Subject: [PATCH 09/25] :construction_worker: Hook up oidc-enabled CI build --- .github/workflows/ci.yml | 24 ++++++++++++++++++++++-- setup.cfg | 1 - tests/oidc/__init__.py | 0 tests/oidc/test_oidc_integration.py | 2 ++ tests/project/settings.py | 22 +++++++++++++++++----- tox.ini | 17 ++++++++++++++++- 6 files changed, 57 insertions(+), 9 deletions(-) create mode 100644 tests/oidc/__init__.py create mode 100644 tests/oidc/test_oidc_integration.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0ff198d..cd9a9f8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,12 +12,23 @@ on: jobs: tests: - name: Run the test suite (Python ${{ matrix.python }}, Django ${{ matrix.django }}) + name: "Run the test suite (Python ${{ matrix.python }}, Django ${{ matrix.django }}, OIDC: ${{ matrix.oidc_enabled }})" runs-on: ubuntu-latest strategy: matrix: python: ['3.10', '3.11', '3.12'] django: ['4.2'] + oidc_enabled: ['no', 'yes'] + + services: + postgres: + image: postgres:15 + env: + POSTGRES_HOST_AUTH_METHOD: trust + ports: + - 5432:5432 + # needed because the postgres container does not provide a healthcheck + options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5 steps: - uses: actions/checkout@v4 @@ -34,10 +45,19 @@ jobs: run: pip install tox tox-gh-actions - name: Run tests - run: tox + run: | + extra_opts='' + if [ "$OIDC_ENABLED" != "yes" ]; then + extra_opts=' --ignore=tests/oidc ' + fi + + tox -- $extra_opts env: PYTHON_VERSION: ${{ matrix.python }} DJANGO: ${{ matrix.django }} + OIDC_ENABLED: ${{ matrix.oidc_enabled }} + PGUSER: postgres + PGHOST: localhost - name: Publish coverage report uses: codecov/codecov-action@v3 diff --git a/setup.cfg b/setup.cfg index 046a0b4..367c237 100644 --- a/setup.cfg +++ b/setup.cfg @@ -100,7 +100,6 @@ sections=FUTURE,STDLIB,DJANGO,THIRDPARTY,FIRSTPARTY,LOCALFOLDER [tool:pytest] testpaths = tests -python_classes = test_* DJANGO_SETTINGS_MODULE=tests.project.settings [pep8] diff --git a/tests/oidc/__init__.py b/tests/oidc/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/oidc/test_oidc_integration.py b/tests/oidc/test_oidc_integration.py new file mode 100644 index 0000000..03d2c72 --- /dev/null +++ b/tests/oidc/test_oidc_integration.py @@ -0,0 +1,2 @@ +def test_oidc_enabled(settings): + assert settings.OIDC_ENABLED is True diff --git a/tests/project/settings.py b/tests/project/settings.py index 5532ec4..3184aef 100644 --- a/tests/project/settings.py +++ b/tests/project/settings.py @@ -53,7 +53,8 @@ "digid_eherkenning", "tests.project", ] -if os.environ.get("OIDC_ENABLED", False): +OIDC_ENABLED = os.environ.get("OIDC_ENABLED", "no") == "yes" +if OIDC_ENABLED: INSTALLED_APPS += _OIDC_APPS @@ -97,10 +98,21 @@ # https://docs.djangoproject.com/en/3.0/ref/settings/#databases DATABASES = { - "default": { - "ENGINE": "django.db.backends.sqlite3", - "NAME": os.path.join(BASE_DIR, "db.sqlite3"), - } + "default": ( + { + "ENGINE": "django.db.backends.postgresql", + "NAME": os.getenv("PGDATABASE", "django_digid_eherkenning"), + "USER": os.getenv("PGUSER", "django_digid_eherkenning"), + "PASSWORD": os.getenv("PGPASSWORD", "django_digid_eherkenning"), + "HOST": os.getenv("DB_HOST", "localhost"), + "PORT": os.getenv("DB_PORT", 5432), + } + if OIDC_ENABLED + else { + "ENGINE": "django.db.backends.sqlite3", + "NAME": os.path.join(BASE_DIR, "db.sqlite3"), + } + ) } DEFAULT_AUTO_FIELD = "django.db.models.AutoField" diff --git a/tox.ini b/tox.ini index 17eca40..7e38986 100644 --- a/tox.ini +++ b/tox.ini @@ -1,6 +1,6 @@ [tox] envlist = - py{310,311,312}-django{42} + py{310,311,312}-django{42}-oidc{enabled,disabled} isort black docs @@ -15,15 +15,27 @@ python = [gh-actions:env] DJANGO = 4.2: django42 +OIDC_ENABLED = + yes: oidcenabled + no: oidcdisabled [testenv] setenv = PYTHONPATH={toxinidir} +passenv = + OIDC_ENABLED + PGUSER + PGDATABASE + PGPASSWORD + PGPORT + PGHOST extras = tests coverage + oidcenabled: oidc deps = django42: Django~=4.2 + oidcenabled: psycopg commands = py.test tests \ --cov --cov-report xml:reports/coverage-{envname}.xml \ @@ -44,8 +56,11 @@ basepython=python changedir=docs skipsdist=true extras = + oidc tests docs +deps = + psycopg commands= py.test check_sphinx.py -v \ --tb=auto \ From cbdae9db504d6e9f9c65ce3ce05e32399b4ba687 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 12:58:29 +0200 Subject: [PATCH 10/25] :construction_worker: Update to codecov action v4 --- .github/workflows/ci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cd9a9f8..65c98d9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,7 +60,9 @@ jobs: PGHOST: localhost - name: Publish coverage report - uses: codecov/codecov-action@v3 + uses: codecov/codecov-action@v4 + with: + token: ${{ secrets.CODECOV_TOKEN }} publish: name: Publish package to PyPI From fa355acfeb4337629c5b5fb9f86f51324c8de85d Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 13:10:45 +0200 Subject: [PATCH 11/25] :construction_worker: Track coverage with flags The OIDC subpackage is an optional feature that is tested separately in CI. --- .github/workflows/ci.yml | 1 + codecov.yml | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 codecov.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 65c98d9..e5a6b97 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,6 +63,7 @@ jobs: uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }} + flags: ${{ matrix.oidc_enabled == 'yes' && 'oidc' || 'base' }} publish: name: Publish package to PyPI diff --git a/codecov.yml b/codecov.yml new file mode 100644 index 0000000..1d7408f --- /dev/null +++ b/codecov.yml @@ -0,0 +1,18 @@ +flag_management: + default_rules: + carryforward: true + statuses: + - type: project + target: auto + threshold: 1% + - type: patch + target: 90% + individual_flags: # exceptions to the default rules above, stated flag by flag + - name: base + paths: + - digid_eherkenning + - '!digid_eherkenning/oidc/' + - name: oidc + paths: + - digid_eherkenning/oidc/ + carryforward: true From b8aec3091c564584a6a1b75d3956c2462a77abf1 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 14:12:51 +0200 Subject: [PATCH 12/25] :fire: do_op_logout is present in mozilla-django-oidc-db already --- digid_eherkenning/oidc/utils.py | 29 ----------------------------- 1 file changed, 29 deletions(-) delete mode 100644 digid_eherkenning/oidc/utils.py diff --git a/digid_eherkenning/oidc/utils.py b/digid_eherkenning/oidc/utils.py deleted file mode 100644 index 84b29d4..0000000 --- a/digid_eherkenning/oidc/utils.py +++ /dev/null @@ -1,29 +0,0 @@ -import logging - -import requests - -from .models import OpenIDConnectBaseConfig - -logger = logging.getLogger(__name__) - - -def do_op_logout(config: OpenIDConnectBaseConfig, id_token: str) -> None: - """ - Perform the logout with the OpenID Provider. - - Standard: https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout - """ - logout_endpoint = config.oidc_op_logout_endpoint - if not logout_endpoint: - return - - response = requests.post(logout_endpoint, data={"id_token_hint": id_token}) - if not response.ok: - logger.warning( - "Failed to log out the user at the OpenID Provider. Status code: %s", - response.status_code, - extra={ - "response": response, - "status_code": response.status_code, - }, - ) From 93aaa215e5ac4891a0af0ef032853bab61ad4b87 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 14:19:55 +0200 Subject: [PATCH 13/25] :wrench: Update coverage config * Enable branch coverage measuring * Hide 100% covered files in report * Skip migrations from coverage reports --- setup.cfg | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/setup.cfg b/setup.cfg index 367c237..d2a5858 100644 --- a/setup.cfg +++ b/setup.cfg @@ -110,3 +110,21 @@ exclude=migrations,static,media [flake8] max-line-length=88 exclude=env,.tox,doc + +[coverage:run] +branch = True +source = digid_eherkenning +omit = + # migrations run while django initializes the test db + */migrations/* + +[coverage:report] +skip_covered = True +exclude_also = + if (typing\.)?TYPE_CHECKING: + @(typing\.)?overload + class .*\(.*Protocol.*\): + @(abc\.)?abstractmethod + raise NotImplementedError + \.\.\. + pass From 187359e3416556c56d61ba7a540b1e11f3228997 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 14:23:16 +0200 Subject: [PATCH 14/25] :hammer: Auto-skip tests if OIDC is not enabled --- .github/workflows/ci.yml | 8 +------- tests/oidc/test_oidc_integration.py | 9 +++++++++ tox.ini | 1 + 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e5a6b97..4a33ff3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,13 +45,7 @@ jobs: run: pip install tox tox-gh-actions - name: Run tests - run: | - extra_opts='' - if [ "$OIDC_ENABLED" != "yes" ]; then - extra_opts=' --ignore=tests/oidc ' - fi - - tox -- $extra_opts + run: tox env: PYTHON_VERSION: ${{ matrix.python }} DJANGO: ${{ matrix.django }} diff --git a/tests/oidc/test_oidc_integration.py b/tests/oidc/test_oidc_integration.py index 03d2c72..5ca5cc3 100644 --- a/tests/oidc/test_oidc_integration.py +++ b/tests/oidc/test_oidc_integration.py @@ -1,2 +1,11 @@ +from django.conf import settings + +import pytest + +pytestmark = [ + pytest.mark.skipif(not settings.OIDC_ENABLED, reason="OIDC integration disabled"), +] + + def test_oidc_enabled(settings): assert settings.OIDC_ENABLED is True diff --git a/tox.ini b/tox.ini index 7e38986..074d309 100644 --- a/tox.ini +++ b/tox.ini @@ -22,6 +22,7 @@ OIDC_ENABLED = [testenv] setenv = PYTHONPATH={toxinidir} + oidcenabled: OIDC_ENABLED=yes passenv = OIDC_ENABLED PGUSER From 40674ede0cda64bc2e245b295290b8ec14844a86 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 14:54:37 +0200 Subject: [PATCH 15/25] :white_check_mark: Add tests for digid/eherkenning flows ... and immediately some bugs were found and fixed. --- digid_eherkenning/oidc/views.py | 6 +-- tests/conftest.py | 7 +++ tests/oidc/test_oidc_integration.py | 66 +++++++++++++++++++++++++++++ tests/project/urls.py | 6 +++ 4 files changed, 82 insertions(+), 3 deletions(-) diff --git a/digid_eherkenning/oidc/views.py b/digid_eherkenning/oidc/views.py index bc084bc..8c0fb76 100644 --- a/digid_eherkenning/oidc/views.py +++ b/digid_eherkenning/oidc/views.py @@ -70,7 +70,7 @@ def login_success(self): return HttpResponseRedirect(self.success_url) -didid_init = OIDCInit.as_view(config_class=DigiDConfig) +digid_init = OIDCInit.as_view(config_class=DigiDConfig) eh_init = OIDCInit.as_view(config_class=EHerkenningConfig) -digid_machtigen_init = OIDCInit.as_view(config_class=EHerkenningBewindvoeringConfig) -eh_bewindvoering_init = OIDCInit.as_view(config_class=DigiDMachtigenConfig) +digid_machtigen_init = OIDCInit.as_view(config_class=DigiDMachtigenConfig) +eh_bewindvoering_init = OIDCInit.as_view(config_class=EHerkenningBewindvoeringConfig) diff --git a/tests/conftest.py b/tests/conftest.py index df740f5..aa07c2f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,7 @@ from django.core.files import File import pytest +import responses from simple_certmanager.constants import CertificateTypes from simple_certmanager.models import Certificate @@ -135,3 +136,9 @@ def eherkenning_config(eherkenning_config_defaults): setattr(eherkenning_config_defaults, field, value) eherkenning_config_defaults.save() return eherkenning_config_defaults + + +@pytest.fixture +def mocked_responses(): + with responses.RequestsMock() as rsps: + yield rsps diff --git a/tests/oidc/test_oidc_integration.py b/tests/oidc/test_oidc_integration.py index 5ca5cc3..538b719 100644 --- a/tests/oidc/test_oidc_integration.py +++ b/tests/oidc/test_oidc_integration.py @@ -1,11 +1,77 @@ from django.conf import settings +from django.contrib.sessions.backends.db import SessionStore +from django.http import HttpRequest +from django.test import RequestFactory import pytest +from responses import RequestsMock + +from digid_eherkenning.oidc.models import ( + DigiDConfig, + DigiDMachtigenConfig, + EHerkenningBewindvoeringConfig, + EHerkenningConfig, +) +from digid_eherkenning.oidc.models.base import OpenIDConnectBaseConfig +from digid_eherkenning.oidc.views import ( + digid_init, + digid_machtigen_init, + eh_bewindvoering_init, + eh_init, +) pytestmark = [ pytest.mark.skipif(not settings.OIDC_ENABLED, reason="OIDC integration disabled"), ] +@pytest.fixture(autouse=True) +def disable_solo_cache(settings): + settings.SOLO_CACHE = None + + +@pytest.fixture +def auth_request(rf: RequestFactory): + request = rf.get("/authenticate/some-oidc", {"next": "/"}) + session = SessionStore() + session.save() + request.session = session + return request + + def test_oidc_enabled(settings): assert settings.OIDC_ENABLED is True + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "init_view,config_class", + ( + (digid_init, DigiDConfig), + (eh_init, EHerkenningConfig), + (digid_machtigen_init, DigiDMachtigenConfig), + (eh_bewindvoering_init, EHerkenningBewindvoeringConfig), + ), +) +def test_init_flow( + auth_request: HttpRequest, + mocked_responses: RequestsMock, + init_view, + config_class: type[OpenIDConnectBaseConfig], +): + _name = config_class._meta.model_name + config = config_class( + enabled=True, + oidc_rp_client_id=f"client-{_name}", + oidc_rp_client_secret=f"secret-{_name}", + oidc_op_authorization_endpoint=f"http://oidc.example.com/start-auth/{_name}", + ) + config.save() + mocked_responses.get(f"http://oidc.example.com/start-auth/{_name}", status=400) + + response = init_view(auth_request) + + assert response.status_code == 302 + assert response["Location"].startswith( + f"http://oidc.example.com/start-auth/{_name}" + ) diff --git a/tests/project/urls.py b/tests/project/urls.py index 6d96276..ea816a8 100644 --- a/tests/project/urls.py +++ b/tests/project/urls.py @@ -1,3 +1,4 @@ +from django.conf import settings from django.contrib import admin from django.urls import include, path @@ -5,3 +6,8 @@ path("admin/", admin.site.urls), path("", include("digid_eherkenning.urls")), ] + +if settings.OIDC_ENABLED: + urlpatterns += [ + path("oidc/", include("mozilla_django_oidc.urls")), + ] From f3e55d2c6195b24e8fc9141835054856afcc1bb3 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 15:31:39 +0200 Subject: [PATCH 16/25] :white_check_mark: Implement base backend and test coverage I don't see a good way to ensure 100% test coverage, especially with classes/models that are essentially intended to be subclassed further. --- digid_eherkenning/oidc/backends.py | 16 ++++ digid_eherkenning/oidc/models/base.py | 5 ++ setup.cfg | 5 ++ tests/oidc/test_oidc_integration.py | 111 +++++++++++++++++++++++++- tests/project/oidc_backends.py | 29 +++++++ 5 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 digid_eherkenning/oidc/backends.py create mode 100644 tests/project/oidc_backends.py diff --git a/digid_eherkenning/oidc/backends.py b/digid_eherkenning/oidc/backends.py new file mode 100644 index 0000000..e9ba725 --- /dev/null +++ b/digid_eherkenning/oidc/backends.py @@ -0,0 +1,16 @@ +from django.contrib.auth.models import AbstractUser + +from mozilla_django_oidc_db.backends import OIDCAuthenticationBackend +from mozilla_django_oidc_db.typing import JSONObject + +from .models.base import OpenIDConnectBaseConfig + + +class BaseBackend(OIDCAuthenticationBackend): + def _check_candidate_backend(self) -> bool: + suitable_model = issubclass(self.config_class, OpenIDConnectBaseConfig) + return suitable_model and super()._check_candidate_backend() + + def update_user(self, user: AbstractUser, claims: JSONObject): + # do nothing by default + return user diff --git a/digid_eherkenning/oidc/models/base.py b/digid_eherkenning/oidc/models/base.py index beef3cf..33e4cd0 100644 --- a/digid_eherkenning/oidc/models/base.py +++ b/digid_eherkenning/oidc/models/base.py @@ -29,3 +29,8 @@ class Meta: @classproperty def oidcdb_check_idp_availability(cls) -> bool: return True + + def get_callback_view(self): + from ..views import OIDCAuthenticationCallbackView + + return OIDCAuthenticationCallbackView.as_view() diff --git a/setup.cfg b/setup.cfg index d2a5858..b5837a1 100644 --- a/setup.cfg +++ b/setup.cfg @@ -46,6 +46,7 @@ tests_require = freezegun pytest pytest-django + pytest-mock responses freezegun tox @@ -64,6 +65,7 @@ tests = django-test-migrations pytest pytest-django + pytest-mock responses freezegun tox @@ -101,6 +103,9 @@ sections=FUTURE,STDLIB,DJANGO,THIRDPARTY,FIRSTPARTY,LOCALFOLDER [tool:pytest] testpaths = tests DJANGO_SETTINGS_MODULE=tests.project.settings +markers = + callback: additional configuration for the callback fixture + mock_backend_claims: claims to be returned by the mock backend fixture [pep8] max-line-length=88 diff --git a/tests/oidc/test_oidc_integration.py b/tests/oidc/test_oidc_integration.py index 538b719..b03bb53 100644 --- a/tests/oidc/test_oidc_integration.py +++ b/tests/oidc/test_oidc_integration.py @@ -1,9 +1,14 @@ +from urllib.parse import parse_qs, urlsplit + from django.conf import settings from django.contrib.sessions.backends.db import SessionStore from django.http import HttpRequest -from django.test import RequestFactory +from django.test import Client, RequestFactory +from django.urls import reverse import pytest +from mozilla_django_oidc_db.models import UserInformationClaimsSources +from mozilla_django_oidc_db.typing import JSONObject from responses import RequestsMock from digid_eherkenning.oidc.models import ( @@ -19,6 +24,7 @@ eh_bewindvoering_init, eh_init, ) +from tests.project.oidc_backends import MockBackend pytestmark = [ pytest.mark.skipif(not settings.OIDC_ENABLED, reason="OIDC integration disabled"), @@ -39,11 +45,65 @@ def auth_request(rf: RequestFactory): return request +@pytest.fixture +def mock_auth_backend(request, mocker): + marker = request.node.get_closest_marker("mock_backend_claims") + claims: JSONObject = marker.args[0] if marker else {"sub": "some_username"} + mock_backend = MockBackend(claims=claims) + backend_path = f"{MockBackend.__module__}.{MockBackend.__qualname__}" + mocker.patch( + "django.contrib.auth._get_backends", return_value=[(mock_backend, backend_path)] + ) + return mock_backend + + +@pytest.fixture +def callback( + request, auth_request: HttpRequest, rf: RequestFactory, client: Client, mocker +) -> tuple[HttpRequest, Client]: + """ + A django request primed by an OIDC auth request flow, ready for the callback flow. + """ + from mozilla_django_oidc_db.config import store_config + + mocker.patch( + "digid_eherkenning.oidc.views.OIDCInit.check_idp_availability", + return_value=None, + ) + + # set a default in case no marker is provided + init_view = None + + marker = request.node.get_closest_marker("callback") + if marker and (_init_view := marker.kwargs.get("init_view")): + init_view = _init_view + if init_view is None: + raise TypeError("You must provide the init_view marker to the callback fixture") + + response = init_view(auth_request) + redirect_url: str = response.url # type: ignore + assert redirect_url + state_key = parse_qs(urlsplit(redirect_url).query)["state"][0] + + callback_request = rf.get( + "/oidc/dummy-callback", + {"state": state_key, "code": "dummy-oidc-code"}, + ) + callback_request.session = auth_request.session + store_config(callback_request) + + session = client.session + for key, value in callback_request.session.items(): + session[key] = value + session.save() + + return callback_request, client + + def test_oidc_enabled(settings): assert settings.OIDC_ENABLED is True -@pytest.mark.django_db @pytest.mark.parametrize( "init_view,config_class", ( @@ -53,6 +113,7 @@ def test_oidc_enabled(settings): (eh_bewindvoering_init, EHerkenningBewindvoeringConfig), ), ) +@pytest.mark.django_db def test_init_flow( auth_request: HttpRequest, mocked_responses: RequestsMock, @@ -75,3 +136,49 @@ def test_init_flow( assert response["Location"].startswith( f"http://oidc.example.com/start-auth/{_name}" ) + + +@pytest.mark.mock_backend_claims({"bsn": "000000000"}) +@pytest.mark.callback(init_view=digid_init) +@pytest.mark.django_db +def test_digid_backend_and_callback_view( + settings, + callback: tuple[HttpRequest, Client], + mock_auth_backend, +): + config = DigiDConfig( + enabled=True, + oidc_op_authorization_endpoint="https://example.com", + userinfo_claims_source=UserInformationClaimsSources.id_token, + ) + config.save() + callback_url = reverse("oidc_authentication_callback") + callback_request, callback_client = callback + + callback_response = callback_client.get(callback_url, {**callback_request.GET}) + + assert callback_response.status_code == 302 + assert callback_response["Location"] == "/" + + +@pytest.mark.mock_backend_claims({"kvk": "12345678"}) +@pytest.mark.callback(init_view=eh_init) +@pytest.mark.django_db +def test_eh_backend_and_callback_view( + settings, + callback: tuple[HttpRequest, Client], + mock_auth_backend, +): + config = EHerkenningConfig( + enabled=True, + oidc_op_authorization_endpoint="https://example.com", + userinfo_claims_source=UserInformationClaimsSources.id_token, + ) + config.save() + callback_url = reverse("oidc_authentication_callback") + callback_request, callback_client = callback + + callback_response = callback_client.get(callback_url, {**callback_request.GET}) + + assert callback_response.status_code == 302 + assert callback_response["Location"] == "/" diff --git a/tests/project/oidc_backends.py b/tests/project/oidc_backends.py new file mode 100644 index 0000000..068e49c --- /dev/null +++ b/tests/project/oidc_backends.py @@ -0,0 +1,29 @@ +from mozilla_django_oidc_db.typing import JSONObject + +from digid_eherkenning.oidc.backends import BaseBackend + + +class MockBackend(BaseBackend): + """ + Auth backend that mocks the actual code -> token exchange and verification. + """ + + def __init__(self, claims: JSONObject): + super().__init__() + self._claims = claims + + def get_token(self, payload): + return { + "id_token": "-mock-id-token-", + "access_token": "-mock-access-token-", + } + + def verify_token(self, token: str, **kwargs) -> JSONObject: + return self._claims + + def _extract_username( + self, claims: JSONObject, *, raise_on_empty: bool = False + ) -> str: + username = super()._extract_username(claims, raise_on_empty=raise_on_empty) + prefix = self.config_class._meta.model_name + return f"{prefix}:{username}" From 623a7e6df917592ceb671ec97ddd76868b68f31c Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 15:37:12 +0200 Subject: [PATCH 17/25] :sparkles: Make callback view configurable without having to override models Subclassing models with proxies only to change a setting implemented on the base class is quite excessive, especially because it makes you override the admin registration. For callback view modifications, we can optionally provide a django setting. --- digid_eherkenning/oidc/models/base.py | 11 ++++++++--- digid_eherkenning/oidc/views.py | 2 ++ docs/oidc.rst | 3 +++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/digid_eherkenning/oidc/models/base.py b/digid_eherkenning/oidc/models/base.py index 33e4cd0..49d41ac 100644 --- a/digid_eherkenning/oidc/models/base.py +++ b/digid_eherkenning/oidc/models/base.py @@ -1,4 +1,6 @@ +from django.conf import settings from django.utils.functional import classproperty +from django.utils.module_loading import import_string from django.utils.translation import gettext_lazy as _ from mozilla_django_oidc_db.models import OpenIDConnectConfigBase @@ -31,6 +33,9 @@ def oidcdb_check_idp_availability(cls) -> bool: return True def get_callback_view(self): - from ..views import OIDCAuthenticationCallbackView - - return OIDCAuthenticationCallbackView.as_view() + configured_setting = getattr( + settings, + "DIGID_EHERKENNING_OIDC_CALLBACK_VIEW", + "digid_eherkenning.oidc.views.default_callback_view", + ) + return import_string(configured_setting) diff --git a/digid_eherkenning/oidc/views.py b/digid_eherkenning/oidc/views.py index 8c0fb76..fa9853c 100644 --- a/digid_eherkenning/oidc/views.py +++ b/digid_eherkenning/oidc/views.py @@ -74,3 +74,5 @@ def login_success(self): eh_init = OIDCInit.as_view(config_class=EHerkenningConfig) digid_machtigen_init = OIDCInit.as_view(config_class=DigiDMachtigenConfig) eh_bewindvoering_init = OIDCInit.as_view(config_class=EHerkenningBewindvoeringConfig) + +default_callback_view = OIDCAuthenticationCallbackView.as_view() diff --git a/docs/oidc.rst b/docs/oidc.rst index 965c2ec..f4c6d01 100644 --- a/docs/oidc.rst +++ b/docs/oidc.rst @@ -38,6 +38,9 @@ and add the necessary packages to ``INSTALLED_APPS``: Make sure to follow mozilla-django-oidc-db's installation instructions too. +Optionally you can point to an alternative callback view to use via the +``DIGID_EHERKENNING_OIDC_CALLBACK_VIEW`` setting, which defaults to +``"digid_eherkenning.oidc.views.default_callback_view"``. Authentication backend ====================== From 662de124ce1528a301803fc598bb7ed42610ebfa Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 28 May 2024 15:41:38 +0200 Subject: [PATCH 18/25] :construction_worker: Fix test collection --- .github/workflows/ci.yml | 3 ++- tests/oidc/test_oidc_integration.py | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4a33ff3..1b44ed5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,7 +45,8 @@ jobs: run: pip install tox tox-gh-actions - name: Run tests - run: tox + run: | + tox -- ${{ matrix.oidc_enabled != 'yes' && '--ignore tests/oidc' || '' }} env: PYTHON_VERSION: ${{ matrix.python }} DJANGO: ${{ matrix.django }} diff --git a/tests/oidc/test_oidc_integration.py b/tests/oidc/test_oidc_integration.py index b03bb53..c45f0e0 100644 --- a/tests/oidc/test_oidc_integration.py +++ b/tests/oidc/test_oidc_integration.py @@ -7,6 +7,7 @@ from django.urls import reverse import pytest +from mozilla_django_oidc_db.config import store_config from mozilla_django_oidc_db.models import UserInformationClaimsSources from mozilla_django_oidc_db.typing import JSONObject from responses import RequestsMock @@ -64,8 +65,6 @@ def callback( """ A django request primed by an OIDC auth request flow, ready for the callback flow. """ - from mozilla_django_oidc_db.config import store_config - mocker.patch( "digid_eherkenning.oidc.views.OIDCInit.check_idp_availability", return_value=None, From 0476eab8f1f67440f499415c0206de570fb0dcb7 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 3 Jun 2024 14:26:15 +0200 Subject: [PATCH 19/25] :sparkles: Export base config model So that downstream projects can import this too for type checks and issubclass checks. --- digid_eherkenning/oidc/models/__init__.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/digid_eherkenning/oidc/models/__init__.py b/digid_eherkenning/oidc/models/__init__.py index 52f9b95..e19c5fb 100644 --- a/digid_eherkenning/oidc/models/__init__.py +++ b/digid_eherkenning/oidc/models/__init__.py @@ -1,10 +1,15 @@ -from .base import get_default_scopes_bsn, get_default_scopes_kvk +from .base import ( + OpenIDConnectBaseConfig, + get_default_scopes_bsn, + get_default_scopes_kvk, +) from .digid import DigiDConfig, DigiDMachtigenConfig from .eherkenning import EHerkenningBewindvoeringConfig, EHerkenningConfig __all__ = [ "get_default_scopes_bsn", "get_default_scopes_kvk", + "OpenIDConnectBaseConfig", "DigiDConfig", "DigiDMachtigenConfig", "EHerkenningConfig", From 9012e74814f6ac6f24bec795ccc583b1ca5b0902 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 3 Jun 2024 14:58:45 +0200 Subject: [PATCH 20/25] :ok_hand: PR feedback --- digid_eherkenning/oidc/__init__.py | 2 +- digid_eherkenning/oidc/admin.py | 35 +++++++++++++++++- digid_eherkenning/oidc/forms.py | 38 -------------------- digid_eherkenning/oidc/models/digid.py | 2 +- digid_eherkenning/oidc/models/eherkenning.py | 2 +- digid_eherkenning/oidc/views.py | 2 +- 6 files changed, 38 insertions(+), 43 deletions(-) delete mode 100644 digid_eherkenning/oidc/forms.py diff --git a/digid_eherkenning/oidc/__init__.py b/digid_eherkenning/oidc/__init__.py index 69a897e..b59fe4f 100644 --- a/digid_eherkenning/oidc/__init__.py +++ b/digid_eherkenning/oidc/__init__.py @@ -58,7 +58,7 @@ from ``settings.AUTHENTICATION_BACKENDS`` are tried in order. Typically this will use the backend shipped in mozilla-django-oidc-db, or a subclass of it. -The OpenID connect flow exchanges the code for the an access token (and ID token), and +The OpenID connect flow exchanges the code for an access token (and ID token), and the user details are retrieved. You should provide a customized backend to determine what needs to be done with this user information, e.g. create a django user or store the information in the django session. diff --git a/digid_eherkenning/oidc/admin.py b/digid_eherkenning/oidc/admin.py index 29fd611..1d4a2f6 100644 --- a/digid_eherkenning/oidc/admin.py +++ b/digid_eherkenning/oidc/admin.py @@ -2,16 +2,19 @@ from copy import deepcopy from django.contrib import admin +from django.forms import modelform_factory from django.utils.translation import gettext_lazy as _ +from mozilla_django_oidc_db.constants import OIDC_MAPPING +from mozilla_django_oidc_db.forms import OpenIDConnectConfigForm from solo.admin import SingletonModelAdmin -from .forms import admin_modelform_factory from .models import ( DigiDConfig, DigiDMachtigenConfig, EHerkenningBewindvoeringConfig, EHerkenningConfig, + OpenIDConnectBaseConfig, ) # Using a dict because these retain ordering, and it makes things a bit more readable. @@ -60,6 +63,36 @@ } +def admin_modelform_factory(model: type[OpenIDConnectBaseConfig], *args, **kwargs): + """ + Factory function to generate a model form class for a given configuration model. + + The configuration model is expected to be a subclass of + :class:`~digid_eherkenning_oidc_generics.models.OpenIDConnectBaseConfig`. + + Additional args and kwargs are forwarded to django's + :func:`django.forms.modelform_factory`. + """ + kwargs.setdefault("form", OpenIDConnectConfigForm) + Form = modelform_factory(model, *args, **kwargs) + + assert issubclass( + Form, OpenIDConnectConfigForm + ), "The base form class must be a subclass of OpenIDConnectConfigForm." + + # update the mapping of discovery endpoint keys to model fields, since our base + # model adds the ``oidc_op_logout_endpoint`` field. + Form.oidc_mapping = { + **deepcopy(OIDC_MAPPING), + "oidc_op_logout_endpoint": "end_session_endpoint", + } + Form.required_endpoints = [ + *Form.required_endpoints, + "oidc_op_logout_endpoint", + ] + return Form + + def fieldsets_factory(claim_mapping_fields: Sequence[str]): """ Apply the shared fieldsets configuration with the model-specific overrides. diff --git a/digid_eherkenning/oidc/forms.py b/digid_eherkenning/oidc/forms.py deleted file mode 100644 index 2aa32f1..0000000 --- a/digid_eherkenning/oidc/forms.py +++ /dev/null @@ -1,38 +0,0 @@ -from copy import deepcopy - -from django.forms import modelform_factory - -from mozilla_django_oidc_db.constants import OIDC_MAPPING -from mozilla_django_oidc_db.forms import OpenIDConnectConfigForm - -from .models.base import OpenIDConnectBaseConfig - - -def admin_modelform_factory(model: type[OpenIDConnectBaseConfig], *args, **kwargs): - """ - Factory function to generate a model form class for a given configuration model. - - The configuration model is expected to be a subclass of - :class:`~digid_eherkenning_oidc_generics.models.OpenIDConnectBaseConfig`. - - Additional args and kwargs are forwarded to django's - :func:`django.forms.modelform_factory`. - """ - kwargs.setdefault("form", OpenIDConnectConfigForm) - Form = modelform_factory(model, *args, **kwargs) - - assert issubclass( - Form, OpenIDConnectConfigForm - ), "The base form class must be a subclass of OpenIDConnectConfigForm." - - # update the mapping of discovery endpoint keys to model fields, since our base - # model adds the ``oidc_op_logout_endpoint`` field. - Form.oidc_mapping = { - **deepcopy(OIDC_MAPPING), - "oidc_op_logout_endpoint": "end_session_endpoint", - } - Form.required_endpoints = [ - *Form.required_endpoints, - "oidc_op_logout_endpoint", - ] - return Form diff --git a/digid_eherkenning/oidc/models/digid.py b/digid_eherkenning/oidc/models/digid.py index 03b2091..e76b4e5 100644 --- a/digid_eherkenning/oidc/models/digid.py +++ b/digid_eherkenning/oidc/models/digid.py @@ -35,7 +35,7 @@ class Meta: verbose_name = _("OpenID Connect configuration for DigiD") @property - def oidcdb_username_claim(self) -> list[str]: + def oidcdb_username_claim(self) -> ClaimPath: return [self.identifier_claim_name] diff --git a/digid_eherkenning/oidc/models/eherkenning.py b/digid_eherkenning/oidc/models/eherkenning.py index bef6162..2bec814 100644 --- a/digid_eherkenning/oidc/models/eherkenning.py +++ b/digid_eherkenning/oidc/models/eherkenning.py @@ -39,7 +39,7 @@ class Meta: verbose_name = _("OpenID Connect configuration for eHerkenning") @property - def oidcdb_username_claim(self) -> list[str]: + def oidcdb_username_claim(self) -> ClaimPath: return [self.identifier_claim_name] diff --git a/digid_eherkenning/oidc/views.py b/digid_eherkenning/oidc/views.py index fa9853c..9b81ce7 100644 --- a/digid_eherkenning/oidc/views.py +++ b/digid_eherkenning/oidc/views.py @@ -24,7 +24,7 @@ class OIDCAuthenticationCallbackView(BaseCallbackView): If we only want to perform the claim processing, then no real user is expected to be returend from the authentication backend, and hence we also don't want to try - to log in in this dummy user (as in, set ``request.user`` to a django user + to log in this dummy user (as in, set ``request.user`` to a django user instance). Note that we deliberately don't perform these changes in :meth:`get` (anymore), From fed79255d8df8256f9daa7a6ad4dcb75e999c018 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 10 Jun 2024 17:58:19 +0200 Subject: [PATCH 21/25] :truck: Move fixtures into conftest of subpackage --- tests/oidc/conftest.py | 78 +++++++++++++++++++++++++++++ tests/oidc/test_oidc_integration.py | 75 +-------------------------- 2 files changed, 79 insertions(+), 74 deletions(-) create mode 100644 tests/oidc/conftest.py diff --git a/tests/oidc/conftest.py b/tests/oidc/conftest.py new file mode 100644 index 0000000..b0cc844 --- /dev/null +++ b/tests/oidc/conftest.py @@ -0,0 +1,78 @@ +from urllib.parse import parse_qs, urlsplit + +from django.contrib.sessions.backends.db import SessionStore +from django.http import HttpRequest +from django.test import Client, RequestFactory + +import pytest +from mozilla_django_oidc_db.config import store_config +from mozilla_django_oidc_db.typing import JSONObject + +from tests.project.oidc_backends import MockBackend + + +@pytest.fixture(autouse=True) +def disable_solo_cache(settings): + settings.SOLO_CACHE = None + + +@pytest.fixture +def auth_request(rf: RequestFactory): + request = rf.get("/authenticate/some-oidc", {"next": "/"}) + session = SessionStore() + session.save() + request.session = session + return request + + +@pytest.fixture +def mock_auth_backend(request, mocker): + marker = request.node.get_closest_marker("mock_backend_claims") + claims: JSONObject = marker.args[0] if marker else {"sub": "some_username"} + mock_backend = MockBackend(claims=claims) + backend_path = f"{MockBackend.__module__}.{MockBackend.__qualname__}" + mocker.patch( + "django.contrib.auth._get_backends", return_value=[(mock_backend, backend_path)] + ) + return mock_backend + + +@pytest.fixture +def callback( + request, auth_request: HttpRequest, rf: RequestFactory, client: Client, mocker +) -> tuple[HttpRequest, Client]: + """ + A django request primed by an OIDC auth request flow, ready for the callback flow. + """ + mocker.patch( + "digid_eherkenning.oidc.views.OIDCInit.check_idp_availability", + return_value=None, + ) + + # set a default in case no marker is provided + init_view = None + + marker = request.node.get_closest_marker("callback") + if marker and (_init_view := marker.kwargs.get("init_view")): + init_view = _init_view + if init_view is None: + raise TypeError("You must provide the init_view marker to the callback fixture") + + response = init_view(auth_request) + redirect_url: str = response.url # type: ignore + assert redirect_url + state_key = parse_qs(urlsplit(redirect_url).query)["state"][0] + + callback_request = rf.get( + "/oidc/dummy-callback", + {"state": state_key, "code": "dummy-oidc-code"}, + ) + callback_request.session = auth_request.session + store_config(callback_request) + + session = client.session + for key, value in callback_request.session.items(): + session[key] = value + session.save() + + return callback_request, client diff --git a/tests/oidc/test_oidc_integration.py b/tests/oidc/test_oidc_integration.py index c45f0e0..b162e01 100644 --- a/tests/oidc/test_oidc_integration.py +++ b/tests/oidc/test_oidc_integration.py @@ -1,15 +1,10 @@ -from urllib.parse import parse_qs, urlsplit - from django.conf import settings -from django.contrib.sessions.backends.db import SessionStore from django.http import HttpRequest -from django.test import Client, RequestFactory +from django.test import Client from django.urls import reverse import pytest -from mozilla_django_oidc_db.config import store_config from mozilla_django_oidc_db.models import UserInformationClaimsSources -from mozilla_django_oidc_db.typing import JSONObject from responses import RequestsMock from digid_eherkenning.oidc.models import ( @@ -25,80 +20,12 @@ eh_bewindvoering_init, eh_init, ) -from tests.project.oidc_backends import MockBackend pytestmark = [ pytest.mark.skipif(not settings.OIDC_ENABLED, reason="OIDC integration disabled"), ] -@pytest.fixture(autouse=True) -def disable_solo_cache(settings): - settings.SOLO_CACHE = None - - -@pytest.fixture -def auth_request(rf: RequestFactory): - request = rf.get("/authenticate/some-oidc", {"next": "/"}) - session = SessionStore() - session.save() - request.session = session - return request - - -@pytest.fixture -def mock_auth_backend(request, mocker): - marker = request.node.get_closest_marker("mock_backend_claims") - claims: JSONObject = marker.args[0] if marker else {"sub": "some_username"} - mock_backend = MockBackend(claims=claims) - backend_path = f"{MockBackend.__module__}.{MockBackend.__qualname__}" - mocker.patch( - "django.contrib.auth._get_backends", return_value=[(mock_backend, backend_path)] - ) - return mock_backend - - -@pytest.fixture -def callback( - request, auth_request: HttpRequest, rf: RequestFactory, client: Client, mocker -) -> tuple[HttpRequest, Client]: - """ - A django request primed by an OIDC auth request flow, ready for the callback flow. - """ - mocker.patch( - "digid_eherkenning.oidc.views.OIDCInit.check_idp_availability", - return_value=None, - ) - - # set a default in case no marker is provided - init_view = None - - marker = request.node.get_closest_marker("callback") - if marker and (_init_view := marker.kwargs.get("init_view")): - init_view = _init_view - if init_view is None: - raise TypeError("You must provide the init_view marker to the callback fixture") - - response = init_view(auth_request) - redirect_url: str = response.url # type: ignore - assert redirect_url - state_key = parse_qs(urlsplit(redirect_url).query)["state"][0] - - callback_request = rf.get( - "/oidc/dummy-callback", - {"state": state_key, "code": "dummy-oidc-code"}, - ) - callback_request.session = auth_request.session - store_config(callback_request) - - session = client.session - for key, value in callback_request.session.items(): - session[key] = value - session.save() - - return callback_request, client - - def test_oidc_enabled(settings): assert settings.OIDC_ENABLED is True From 905b609ec4c249829d3f169055901912e3cac3b1 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 10 Jun 2024 18:28:32 +0200 Subject: [PATCH 22/25] :white_check_mark: Add tests for the oidc-subpackage views Ensure that our sanity check in the base class works as expected for downstream projects wishing to override the callback behaviour on a per-config basis. --- setup.cfg | 2 +- tests/oidc/conftest.py | 12 +++- tests/oidc/test_oidc_integration.py | 6 +- tests/oidc/test_views.py | 97 +++++++++++++++++++++++++++++ tests/project/oidc_backends.py | 20 ++++++ 5 files changed, 129 insertions(+), 8 deletions(-) create mode 100644 tests/oidc/test_views.py diff --git a/setup.cfg b/setup.cfg index b5837a1..f23e6c3 100644 --- a/setup.cfg +++ b/setup.cfg @@ -105,7 +105,7 @@ testpaths = tests DJANGO_SETTINGS_MODULE=tests.project.settings markers = callback: additional configuration for the callback fixture - mock_backend_claims: claims to be returned by the mock backend fixture + mock_backend: class/claims to be returned by the mock backend fixture [pep8] max-line-length=88 diff --git a/tests/oidc/conftest.py b/tests/oidc/conftest.py index b0cc844..00030b0 100644 --- a/tests/oidc/conftest.py +++ b/tests/oidc/conftest.py @@ -27,9 +27,15 @@ def auth_request(rf: RequestFactory): @pytest.fixture def mock_auth_backend(request, mocker): - marker = request.node.get_closest_marker("mock_backend_claims") - claims: JSONObject = marker.args[0] if marker else {"sub": "some_username"} - mock_backend = MockBackend(claims=claims) + marker = request.node.get_closest_marker("mock_backend") + marker_kwargs = marker.kwargs if marker else {} + claims: JSONObject = ( + marker_kwargs["claims"] + if "claims" in marker_kwargs + else {"sub": "some_username"} + ) + BackendCls = marker_kwargs["cls"] if "cls" in marker_kwargs else MockBackend + mock_backend = BackendCls(claims=claims) backend_path = f"{MockBackend.__module__}.{MockBackend.__qualname__}" mocker.patch( "django.contrib.auth._get_backends", return_value=[(mock_backend, backend_path)] diff --git a/tests/oidc/test_oidc_integration.py b/tests/oidc/test_oidc_integration.py index b162e01..ae63382 100644 --- a/tests/oidc/test_oidc_integration.py +++ b/tests/oidc/test_oidc_integration.py @@ -64,11 +64,10 @@ def test_init_flow( ) -@pytest.mark.mock_backend_claims({"bsn": "000000000"}) +@pytest.mark.mock_backend(claims={"bsn": "000000000"}) @pytest.mark.callback(init_view=digid_init) @pytest.mark.django_db def test_digid_backend_and_callback_view( - settings, callback: tuple[HttpRequest, Client], mock_auth_backend, ): @@ -87,11 +86,10 @@ def test_digid_backend_and_callback_view( assert callback_response["Location"] == "/" -@pytest.mark.mock_backend_claims({"kvk": "12345678"}) +@pytest.mark.mock_backend(claims={"kvk": "12345678"}) @pytest.mark.callback(init_view=eh_init) @pytest.mark.django_db def test_eh_backend_and_callback_view( - settings, callback: tuple[HttpRequest, Client], mock_auth_backend, ): diff --git a/tests/oidc/test_views.py b/tests/oidc/test_views.py new file mode 100644 index 0000000..86c8cbc --- /dev/null +++ b/tests/oidc/test_views.py @@ -0,0 +1,97 @@ +from django.conf import settings +from django.http import HttpRequest +from django.test import Client +from django.urls import reverse + +import pytest +from mozilla_django_oidc_db.models import UserInformationClaimsSources +from mozilla_django_oidc_db.views import OIDCInit + +from digid_eherkenning.oidc.models import DigiDConfig +from digid_eherkenning.oidc.views import OIDCAuthenticationCallbackView +from tests.project.oidc_backends import ( + AnonymousDjangoUserBackend, + RealDjangoUserBackend, +) + +pytestmark = [ + pytest.mark.skipif(not settings.OIDC_ENABLED, reason="OIDC integration disabled"), +] + + +class Config1(DigiDConfig): + class Meta: + proxy = True + app_label = "project" + + def get_callback_view(self): + return Callback1.as_view() + + +class Callback1(OIDCAuthenticationCallbackView): + expect_django_user = False + + +@pytest.mark.mock_backend(claims={"bsn": "000000000"}, cls=RealDjangoUserBackend) +@pytest.mark.callback(init_view=OIDCInit.as_view(config_class=Config1)) +@pytest.mark.django_db +def test_backend_unexpectedly_returns_real_user( + callback: tuple[HttpRequest, Client], + mock_auth_backend, +): + config = DigiDConfig( + enabled=True, + oidc_op_authorization_endpoint="https://example.com", + userinfo_claims_source=UserInformationClaimsSources.id_token, + ) + config.save() + callback_url = reverse("oidc_authentication_callback") + callback_request, callback_client = callback + + with pytest.raises( + TypeError, + match=( + "A real Django user instance was returned from the authentication backend. " + "This is a configuration/programming mistake!" + ), + ): + callback_client.get(callback_url, {**callback_request.GET}) + + +class Config2(DigiDConfig): + class Meta: + proxy = True + app_label = "project" + + def get_callback_view(self): + return Callback2.as_view() + + +class Callback2(OIDCAuthenticationCallbackView): + expect_django_user = True + + +@pytest.mark.mock_backend(claims={"bsn": "000000000"}, cls=AnonymousDjangoUserBackend) +@pytest.mark.callback(init_view=OIDCInit.as_view(config_class=Config2)) +@pytest.mark.django_db +def test_backend_unexpectedly_returns_anonymous_user( + callback: tuple[HttpRequest, Client], + mock_auth_backend, +): + config = DigiDConfig( + enabled=True, + oidc_op_authorization_endpoint="https://example.com", + userinfo_claims_source=UserInformationClaimsSources.id_token, + ) + config.save() + callback_url = reverse("oidc_authentication_callback") + callback_request, callback_client = callback + + with pytest.raises( + TypeError, + match=( + "A fake Django user instance was returned from the authentication backend. " + "This is a configuration/programming mistake!" + ), + ): + callback_client.get(callback_url, {**callback_request.GET}) diff --git a/tests/project/oidc_backends.py b/tests/project/oidc_backends.py index 068e49c..a4c3011 100644 --- a/tests/project/oidc_backends.py +++ b/tests/project/oidc_backends.py @@ -1,6 +1,9 @@ +from django.contrib.auth.models import AnonymousUser + from mozilla_django_oidc_db.typing import JSONObject from digid_eherkenning.oidc.backends import BaseBackend +from tests.project.models import User class MockBackend(BaseBackend): @@ -27,3 +30,20 @@ def _extract_username( username = super()._extract_username(claims, raise_on_empty=raise_on_empty) prefix = self.config_class._meta.model_name return f"{prefix}:{username}" + + +class RealDjangoUserBackend(MockBackend): + """ + A backend that always returns a real Django user. + """ + + def get_or_create_user(self, access_token, id_token, payload): + user, _ = User.objects.get_or_create(username="admin") + return user + + +class AnonymousDjangoUserBackend(MockBackend): + def get_or_create_user(self, access_token, id_token, payload): + user = AnonymousUser() + user.is_active = True + return user From 200d9870b8a00fdf1320a70f0d5ac9e68c567de3 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 10 Jun 2024 18:34:47 +0200 Subject: [PATCH 23/25] :fire: Delete compatibility shim We don't support Django < 4.2 anymore. --- digid_eherkenning/compat.py | 8 ------ digid_eherkenning/views/digid.py | 3 +-- tests/oidc/test_views.py | 44 ++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 10 deletions(-) delete mode 100644 digid_eherkenning/compat.py diff --git a/digid_eherkenning/compat.py b/digid_eherkenning/compat.py deleted file mode 100644 index daf57ca..0000000 --- a/digid_eherkenning/compat.py +++ /dev/null @@ -1,8 +0,0 @@ -from django.views import View - - -def get_next_page(view: View) -> str: - try: - return view.get_default_redirect_url() # type: ignore - except AttributeError: - return view.get_next_page() # type: ignore diff --git a/digid_eherkenning/views/digid.py b/digid_eherkenning/views/digid.py index 499acac..a07e4ed 100644 --- a/digid_eherkenning/views/digid.py +++ b/digid_eherkenning/views/digid.py @@ -18,7 +18,6 @@ from onelogin.saml2.utils import OneLogin_Saml2_Error, OneLogin_Saml2_ValidationError from ..choices import DigiDAssuranceLevels, SectorType -from ..compat import get_next_page from ..forms import SAML2Form from ..saml2.digid import DigiDClient from ..utils import logout_user @@ -145,7 +144,7 @@ def dispatch(self, request, *args, **kwargs): raise PermissionDenied(_("You are not authenticated with Digid")) client = DigiDClient() - return_to = get_next_page(self) + return_to = self.get_default_redirect_url() logout_url = client.create_logout_request( request, return_to=return_to, name_id=name_id ) diff --git a/tests/oidc/test_views.py b/tests/oidc/test_views.py index 86c8cbc..a5a8996 100644 --- a/tests/oidc/test_views.py +++ b/tests/oidc/test_views.py @@ -95,3 +95,47 @@ def test_backend_unexpectedly_returns_anonymous_user( ), ): callback_client.get(callback_url, {**callback_request.GET}) + + +@pytest.mark.mock_backend(claims={"bsn": "000000000"}, cls=RealDjangoUserBackend) +@pytest.mark.callback(init_view=OIDCInit.as_view(config_class=Config2)) +@pytest.mark.django_db +def test_backend_returns_django_user( + callback: tuple[HttpRequest, Client], + mock_auth_backend, +): + config = DigiDConfig( + enabled=True, + oidc_op_authorization_endpoint="https://example.com", + userinfo_claims_source=UserInformationClaimsSources.id_token, + ) + config.save() + callback_url = reverse("oidc_authentication_callback") + callback_request, callback_client = callback + + response = callback_client.get(callback_url, {**callback_request.GET}) + + assert response.status_code == 302 + assert response["Location"] == "/" + + +@pytest.mark.mock_backend(claims={"bsn": "000000000"}, cls=AnonymousDjangoUserBackend) +@pytest.mark.callback(init_view=OIDCInit.as_view(config_class=Config1)) +@pytest.mark.django_db +def test_backend_returns_anonymous_user( + callback: tuple[HttpRequest, Client], + mock_auth_backend, +): + config = DigiDConfig( + enabled=True, + oidc_op_authorization_endpoint="https://example.com", + userinfo_claims_source=UserInformationClaimsSources.id_token, + ) + config.save() + callback_url = reverse("oidc_authentication_callback") + callback_request, callback_client = callback + + response = callback_client.get(callback_url, {**callback_request.GET}) + + assert response.status_code == 302 + assert response["Location"] == "/" From 78ac69edf25551ff05cbf53124467d35596b1374 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 11 Jun 2024 12:53:39 +0200 Subject: [PATCH 24/25] :ok_hand: Remove mapping/required field overrides The op_logout endpoint was provided in upstream, so the mapping in the modelform factory no longer requires patching. Additionally, the field is not necessarily required, so this requirement is dropped too. --- digid_eherkenning/oidc/admin.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/digid_eherkenning/oidc/admin.py b/digid_eherkenning/oidc/admin.py index 1d4a2f6..4fbd998 100644 --- a/digid_eherkenning/oidc/admin.py +++ b/digid_eherkenning/oidc/admin.py @@ -5,7 +5,6 @@ from django.forms import modelform_factory from django.utils.translation import gettext_lazy as _ -from mozilla_django_oidc_db.constants import OIDC_MAPPING from mozilla_django_oidc_db.forms import OpenIDConnectConfigForm from solo.admin import SingletonModelAdmin @@ -75,21 +74,9 @@ def admin_modelform_factory(model: type[OpenIDConnectBaseConfig], *args, **kwarg """ kwargs.setdefault("form", OpenIDConnectConfigForm) Form = modelform_factory(model, *args, **kwargs) - assert issubclass( Form, OpenIDConnectConfigForm ), "The base form class must be a subclass of OpenIDConnectConfigForm." - - # update the mapping of discovery endpoint keys to model fields, since our base - # model adds the ``oidc_op_logout_endpoint`` field. - Form.oidc_mapping = { - **deepcopy(OIDC_MAPPING), - "oidc_op_logout_endpoint": "end_session_endpoint", - } - Form.required_endpoints = [ - *Form.required_endpoints, - "oidc_op_logout_endpoint", - ] return Form From c254231685f0e0ea40ff7a652de20cde346f828c Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Tue, 11 Jun 2024 12:56:48 +0200 Subject: [PATCH 25/25] :ok_hand: Fix typo's and improve docstrings --- digid_eherkenning/oidc/models/base.py | 4 ++-- digid_eherkenning/oidc/views.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/digid_eherkenning/oidc/models/base.py b/digid_eherkenning/oidc/models/base.py index 49d41ac..ec9c3ad 100644 --- a/digid_eherkenning/oidc/models/base.py +++ b/digid_eherkenning/oidc/models/base.py @@ -8,14 +8,14 @@ def get_default_scopes_bsn(): """ - Returns the default scopes to request for OpenID Connect logins + Returns the default scopes to request for OpenID Connect logins for DigiD. """ return ["openid", "bsn"] def get_default_scopes_kvk(): """ - Returns the default scopes to request for OpenID Connect logins + Returns the default scopes to request for OpenID Connect logins for eHerkenning. """ return ["openid", "kvk"] diff --git a/digid_eherkenning/oidc/views.py b/digid_eherkenning/oidc/views.py index 9b81ce7..bcc206c 100644 --- a/digid_eherkenning/oidc/views.py +++ b/digid_eherkenning/oidc/views.py @@ -23,7 +23,7 @@ class OIDCAuthenticationCallbackView(BaseCallbackView): Check if the 'created user' from the authentication backend needs to be logged in. If we only want to perform the claim processing, then no real user is expected to - be returend from the authentication backend, and hence we also don't want to try + be returned from the authentication backend, and hence we also don't want to try to log in this dummy user (as in, set ``request.user`` to a django user instance).