Merge pull request #1021 from maykinmedia/fix/2101-oidc-admin-issues

🔧 [#2101] Add OIDC admin config to admin index fixture

.github/workflows/ci.yml

@@ -56,13 +56,19 @@ jobs:
           setup-node: 'yes'
           npm-ci-flags: '--legacy-peer-deps'
 
-      # We temporarily disable parallel test runs due to a Django bug:
-      # https://code.djangoproject.com/ticket/32114
-      # https://github.com/django/django/pull/17650
       - name: Run tests
         run: |
           python src/manage.py collectstatic --noinput --link
+          coverage run \
           coverage run src/manage.py test src --exclude-tag=e2e
+            --concurrency=multiprocessing \
+            --parallel-mode \
+            src/manage.py test src \
+            --parallel \
+            --exclude-tag=e2e \
+            --exclude-tag=elastic
+          coverage run src/manage.py test src --tag=elastic --exclude-tag=e2e
+          coverage combine
 
         env:
           DJANGO_SETTINGS_MODULE: open_inwoner.conf.ci

src/open_inwoner/accounts/backends.py

@@ -4,7 +4,7 @@
 from django.contrib.auth import get_user_model
 from django.contrib.auth.backends import ModelBackend
 from django.contrib.auth.hashers import check_password
-from django.urls import reverse_lazy
+from django.urls import reverse, reverse_lazy
 
 from axes.backends import AxesBackend
 from mozilla_django_oidc_db.backends import OIDCAuthenticationBackend
@@ -79,6 +79,10 @@ def authenticate(self, request, *args, **kwargs):
         if request and request.path != self.callback_path:
             return
 
+        config = SiteConfiguration.get_solo()
+        if request and config.openid_enabled_for_admin:
+            request.session["oidc_login_next"] = reverse("admin:index")
+
         return super().authenticate(request, *args, **kwargs)
 
     def create_user(self, claims):
@@ -106,6 +110,8 @@ def create_user(self, claims):
             # TODO verify we want unusable_password
             existing_user.set_unusable_password()
             existing_user.save()
+            # Ensure `make_user_staff` is used
+            self.update_user(existing_user, claims)
             return existing_user
         else:
             logger.debug("Creating OIDC user: %s", unique_id)
@@ -116,9 +122,13 @@ def create_user(self, claims):
                 "login_type": LoginTypeChoices.oidc,
             }
             user = self.UserModel.objects.create_user(**kwargs)
+            # Ensure `make_user_staff` is used
             self.update_user(user, claims)
             # TODO verify we want unusable_password
             user.set_unusable_password()
+            # Saving after using `set_unusable_password` is required, otherwise the user
+            # will not actually be authenticated, see: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2101
+            user.save()
 
             return user
 

src/open_inwoner/accounts/tests/test_oidc_views.py

@@ -19,6 +19,8 @@
     GENERIC_DIGID_ERROR_MSG,
     GENERIC_EHERKENNING_ERROR_MSG,
 )
+from open_inwoner.configurations.choices import OpenIDDisplayChoices
+from open_inwoner.configurations.models import SiteConfiguration
 from open_inwoner.kvk.branches import KVK_BRANCH_SESSION_VARIABLE
 
 from ..choices import LoginTypeChoices
@@ -34,10 +36,15 @@ class OIDCFlowTests(TestCase):
     @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_token")
     @patch(
         "mozilla_django_oidc_db.mixins.OpenIDConnectConfig.get_solo",
-        return_value=OpenIDConnectConfig(id=1, enabled=True),
+        return_value=OpenIDConnectConfig(id=1, enabled=True, make_users_staff=True),
+    )
+    @patch(
+        "open_inwoner.configurations.models.SiteConfiguration.get_solo",
+        return_value=SiteConfiguration(openid_display=OpenIDDisplayChoices.admin),
     )
-    def test_existing_email_updates_user(
+    def test_existing_email_updates_admin_user(
         self,
+        mock_config_get_solo,
         mock_get_solo,
         mock_get_token,
         mock_verify_token,
@@ -62,18 +69,126 @@ def test_existing_email_updates_user(
             callback_url, {"code": "mock", "state": "mock"}
         )
 
+        self.assertRedirects(
+            callback_response, reverse("admin:index"), fetch_redirect_response=True
+        )
+
+        self.assertEqual(User.objects.count(), 1)
+
         user.refresh_from_db()
 
+        self.assertEqual(user.oidc_id, "some_username")
+        self.assertEqual(user.login_type, LoginTypeChoices.oidc)
+        self.assertEqual(user.is_staff, True)
+
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_userinfo")
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.store_tokens")
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.verify_token")
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_token")
+    @patch(
+        "mozilla_django_oidc_db.mixins.OpenIDConnectConfig.get_solo",
+        return_value=OpenIDConnectConfig(id=1, enabled=True, make_users_staff=False),
+    )
+    @patch(
+        "open_inwoner.configurations.models.SiteConfiguration.get_solo",
+        return_value=SiteConfiguration(openid_display=OpenIDDisplayChoices.regular),
+    )
+    def test_existing_email_updates_regular_user(
+        self,
+        mock_config_get_solo,
+        mock_get_solo,
+        mock_get_token,
+        mock_verify_token,
+        mock_store_tokens,
+        mock_get_userinfo,
+    ):
+        # set up a user with a colliding email address
+        # sub is the oidc_id field in our db
+        mock_get_userinfo.return_value = {
+            "email": "[email protected]",
+            "sub": "some_username",
+        }
+        user = UserFactory.create(email="[email protected]")
+        self.assertEqual(user.oidc_id, "")
+        session = self.client.session
+        session["oidc_states"] = {"mock": {"nonce": "nonce"}}
+        session.save()
+        callback_url = reverse("oidc_authentication_callback")
+
+        # enter the login flow
+        callback_response = self.client.get(
+            callback_url, {"code": "mock", "state": "mock"}
+        )
+
         self.assertRedirects(
-            callback_response, reverse("pages-root"), fetch_redirect_response=False
+            callback_response, reverse("pages-root"), fetch_redirect_response=True
         )
-        self.assertTrue(User.objects.filter(oidc_id="some_username").exists())
+
+        self.assertEqual(User.objects.count(), 1)
+
+        user.refresh_from_db()
+
         self.assertEqual(user.oidc_id, "some_username")
+        self.assertEqual(user.login_type, LoginTypeChoices.oidc)
+        self.assertEqual(user.is_staff, False)
 
-        db_user = User.objects.filter(oidc_id="some_username").first()
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_userinfo")
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.store_tokens")
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.verify_token")
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_token")
+    @patch(
+        "mozilla_django_oidc_db.mixins.OpenIDConnectConfig.get_solo",
+        return_value=OpenIDConnectConfig(
+            id=1,
+            enabled=True,
+            make_users_staff=False,
+            claim_mapping={"first_name": "first_name"},
+        ),
+    )
+    @patch(
+        "open_inwoner.configurations.models.SiteConfiguration.get_solo",
+        return_value=SiteConfiguration(openid_display=OpenIDDisplayChoices.regular),
+    )
+    def test_existing_oidc_id_updates_regular_user(
+        self,
+        mock_config_get_solo,
+        mock_get_solo,
+        mock_get_token,
+        mock_verify_token,
+        mock_store_tokens,
+        mock_get_userinfo,
+    ):
+        # set up a user with a colliding email address
+        # sub is the oidc_id field in our db
+        mock_get_userinfo.return_value = {
+            "email": "[email protected]",
+            "sub": "some_username",
+            "first_name": "bar",
+        }
+        user = UserFactory.create(
+            oidc_id="some_username", first_name="Foo", login_type=LoginTypeChoices.oidc
+        )
+        session = self.client.session
+        session["oidc_states"] = {"mock": {"nonce": "nonce"}}
+        session.save()
+        callback_url = reverse("oidc_authentication_callback")
 
-        self.assertEqual(db_user.id, user.id)
-        self.assertEqual(db_user.login_type, LoginTypeChoices.oidc)
+        # enter the login flow
+        callback_response = self.client.get(
+            callback_url, {"code": "mock", "state": "mock"}
+        )
+
+        self.assertRedirects(
+            callback_response, reverse("pages-root"), fetch_redirect_response=True
+        )
+
+        self.assertEqual(User.objects.count(), 1)
+
+        user.refresh_from_db()
+
+        self.assertEqual(user.oidc_id, "some_username")
+        self.assertEqual(user.first_name, "bar")
+        self.assertEqual(user.is_staff, False)
 
     @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_userinfo")
     @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.store_tokens")
@@ -83,8 +198,13 @@ def test_existing_email_updates_user(
         "mozilla_django_oidc_db.mixins.OpenIDConnectConfig.get_solo",
         return_value=OpenIDConnectConfig(id=1, enabled=True),
     )
+    @patch(
+        "open_inwoner.configurations.models.SiteConfiguration.get_solo",
+        return_value=SiteConfiguration(openid_display=OpenIDDisplayChoices.regular),
+    )
     def test_existing_case_sensitive_email_updates_user(
         self,
+        mock_config_get_solo,
         mock_get_solo,
         mock_get_token,
         mock_verify_token,
@@ -123,17 +243,23 @@ def test_existing_case_sensitive_email_updates_user(
 
         self.assertEqual(db_user.id, user.id)
         self.assertEqual(db_user.login_type, LoginTypeChoices.oidc)
+        self.assertEqual(db_user.is_staff, False)
 
     @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_userinfo")
     @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.store_tokens")
     @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.verify_token")
     @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_token")
     @patch(
         "mozilla_django_oidc_db.mixins.OpenIDConnectConfig.get_solo",
-        return_value=OpenIDConnectConfig(id=1, enabled=True),
+        return_value=OpenIDConnectConfig(id=1, enabled=True, make_users_staff=True),
     )
-    def test_new_user_is_created_when_new_email(
+    @patch(
+        "open_inwoner.configurations.models.SiteConfiguration.get_solo",
+        return_value=SiteConfiguration(openid_display=OpenIDDisplayChoices.admin),
+    )
+    def test_new_admin_user_is_created_when_new_email(
         self,
+        mock_config_get_solo,
         mock_get_solo,
         mock_get_token,
         mock_verify_token,
@@ -159,13 +285,61 @@ def test_new_user_is_created_when_new_email(
         )
 
         self.assertRedirects(
-            callback_response, reverse("pages-root"), fetch_redirect_response=False
+            callback_response, reverse("admin:index"), fetch_redirect_response=True
+        )
+
+        new_user = User.objects.get(email="[email protected]")
+
+        self.assertEqual(new_user.oidc_id, "some_username")
+        self.assertEqual(new_user.login_type, LoginTypeChoices.oidc)
+        self.assertEqual(new_user.is_staff, True)
+
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_userinfo")
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.store_tokens")
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.verify_token")
+    @patch("mozilla_django_oidc_db.backends.OIDCAuthenticationBackend.get_token")
+    @patch(
+        "mozilla_django_oidc_db.mixins.OpenIDConnectConfig.get_solo",
+        return_value=OpenIDConnectConfig(id=1, enabled=True, make_users_staff=False),
+    )
+    @patch(
+        "open_inwoner.configurations.models.SiteConfiguration.get_solo",
+        return_value=SiteConfiguration(openid_display=OpenIDDisplayChoices.regular),
+    )
+    def test_new_regular_user_is_created_when_new_email(
+        self,
+        mock_config_get_solo,
+        mock_get_solo,
+        mock_get_token,
+        mock_verify_token,
+        mock_store_tokens,
+        mock_get_userinfo,
+    ):
+        # set up a user with a non existing email address
+        mock_get_userinfo.return_value = {
+            "email": "[email protected]",
+            "sub": "some_username",
+        }
+        UserFactory.create(email="[email protected]")
+        session = self.client.session
+        session["oidc_states"] = {"mock": {"nonce": "nonce"}}
+        session.save()
+        callback_url = reverse("oidc_authentication_callback")
+
+        # enter the login flow
+        callback_response = self.client.get(
+            callback_url, {"code": "mock", "state": "mock"}
         )
-        new_user = User.objects.filter(email="[email protected]")
 
-        self.assertTrue(new_user.exists())
-        self.assertEqual(new_user.get().oidc_id, "some_username")
-        self.assertEqual(new_user.get().login_type, LoginTypeChoices.oidc)
+        self.assertRedirects(
+            callback_response, reverse("pages-root"), fetch_redirect_response=True
+        )
+
+        new_user = User.objects.get(email="[email protected]")
+
+        self.assertEqual(new_user.oidc_id, "some_username")
+        self.assertEqual(new_user.login_type, LoginTypeChoices.oidc)
+        self.assertEqual(new_user.is_staff, False)
 
     def test_error_page_direct_access_forbidden(self):
         error_url = reverse("admin-oidc-error")
@@ -182,8 +356,13 @@ def test_error_page_direct_access_forbidden(self):
         "mozilla_django_oidc_db.mixins.OpenIDConnectConfig.get_solo",
         return_value=OpenIDConnectConfig(id=1, enabled=True),
     )
+    @patch(
+        "open_inwoner.configurations.models.SiteConfiguration.get_solo",
+        return_value=SiteConfiguration(openid_display=OpenIDDisplayChoices.regular),
+    )
     def test_error_first_cleared_after_succesful_login(
         self,
+        mock_config_get_solo,
         mock_get_solo,
         mock_get_token,
         mock_verify_token,

src/open_inwoner/conf/fixtures/django-admin-index.json

@@ -26,6 +26,10 @@
                 "accounts",
                 "user"
             ],
+            [
+                "mozilla_django_oidc_db",
+                "openidconnectconfig"
+            ],
             [
                 "userfeed",
                 "feeditemdata"

src/open_inwoner/configurations/tests/test_oidc.py

@@ -41,7 +41,10 @@ def test_admin_only_enabled(self):
 
         oidc_login_option = response.pyquery.find(".admin-login-option")
 
-        self.assertEqual(oidc_login_option.text(), _("Login with organization account"))
+        self.assertEqual(
+            oidc_login_option.text(),
+            "{} {}".format(_("or"), _("Login with organization account")),
+        )
 
     def test_admin_only_disabled(self):
         """Assert that the OIDC login option is only displayed for regular users"""

src/open_inwoner/scss/admin/_app_overrides.scss

@@ -104,3 +104,10 @@ $djai-border-width: 8px;
     }
   }
 }
+
+/* Extra login links in admin login screen */
+.admin-login-option {
+  text-align: center;
+  clear: both;
+  padding-top: 1em;
+}

src/open_inwoner/templates/admin/login.html

@@ -0,0 +1,17 @@
+{% extends "admin/login.html" %}
+{% load solo_tags i18n %}
+
+
+{% block content %}
+{{ block.super }}
+
+{% get_solo 'mozilla_django_oidc_db.OpenIDConnectConfig' as oidc_config %}
+{% get_solo 'configurations.SiteConfiguration' as site_config %}
+{% if oidc_config.enabled and site_config.openid_enabled_for_admin %}
+<div class="admin-login-option">{% trans "or" %}</div>
+<div class="admin-login-option">
+    <a href="{% url 'oidc_authentication_init' %}">{% trans "Login with organization account" %}</a>
+</div>
+{% endif %}
+
+{% endblock %}