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

stevenbal · 2024-02-12T15:00:00Z

issue: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2101

Fixes two issues:

If an OIDC user logs in for the first time, they are now actually authenticated (previously they had to log in twice)
If OIDC is enabled for admins, the is_staff attribute is set for those users (according to the OIDC config) and they are redirected to the admin interface

codecov-commenter · 2024-02-15T12:22:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0f25981) 94.84% compared to head (8a68f4f) 94.85%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1021      +/-   ##
===========================================
+ Coverage    94.84%   94.85%   +0.01%     
===========================================
  Files          880      880              
  Lines        30725    30805      +80     
===========================================
+ Hits         29141    29221      +80     
  Misses        1584     1584

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

Bartvaderkin

I put some notes, although it is mostly the same feedback about filter/first being sub-optimal for tests, and showing intent.

Bartvaderkin · 2024-02-21T10:00:06Z

src/open_inwoner/accounts/tests/test_oidc_views.py

+        self.assertTrue(User.objects.filter(oidc_id="some_username").exists())
+        self.assertEqual(user.oidc_id, "some_username")
+
+        db_user = User.objects.filter(oidc_id="some_username").first()


In general tests are stronger if instead of .filter()/.first() (or .exists()) for single records to just use a .get() so it will also communicate and test the record must exists and cannot be multiple.

Also the test itself seems weird? I don't quite get what it tries to prove: isn't user the same record as db_user?

You're right, the tests were a bit messy already and I was adding stuff around that 😅.

I cleaned it up, the test is supposed to verify that an existing admin user (that is not yet OIDC), is updated if a user with the same email logs in with OIDC

Bartvaderkin · 2024-02-21T10:02:57Z

src/open_inwoner/accounts/tests/test_oidc_views.py

+        user.refresh_from_db()
+
+        self.assertTrue(User.objects.filter(oidc_id="some_username").exists())
+        self.assertEqual(user.oidc_id, "some_username")
+
+        db_user = User.objects.filter(oidc_id="some_username").first()
+
+        self.assertEqual(db_user.id, user.id)
+        self.assertEqual(db_user.login_type, LoginTypeChoices.oidc)
+        self.assertEqual(db_user.is_staff, False)


Same as above, not quite clear what is tested here.

Bartvaderkin · 2024-02-21T10:03:32Z

src/open_inwoner/accounts/tests/test_oidc_views.py

+        user.refresh_from_db()
+
        self.assertTrue(User.objects.filter(oidc_id="some_username").exists())
        self.assertEqual(user.oidc_id, "some_username")

        db_user = User.objects.filter(oidc_id="some_username").first()

        self.assertEqual(db_user.id, user.id)
        self.assertEqual(db_user.login_type, LoginTypeChoices.oidc)
+        self.assertEqual(db_user.is_staff, False)


Same as above.

Bartvaderkin · 2024-02-21T10:06:07Z

src/open_inwoner/accounts/tests/test_oidc_views.py

+            callback_response, reverse("admin:index"), fetch_redirect_response=True
+        )
+
+        new_user = User.objects.filter(email="[email protected]").first()


Here it is more clear as new_user and the test title work together.

Nevertheless the filter()/first() is a bit floppy and should just be a .get() (note there is even a assertIsNotNone(new_user) there to work around .first() ☹️

Bartvaderkin · 2024-02-21T10:07:03Z

src/open_inwoner/accounts/tests/test_oidc_views.py

+        new_user = User.objects.filter(email="[email protected]").first()
+
+        self.assertIsNotNone(new_user)


Bartvaderkin · 2024-02-21T10:09:21Z

src/open_inwoner/accounts/tests/test_oidc_views.py

@@ -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),


I've seen this patch+return_value often without the id=1 not sure if it matters.

It does appear to be necessary in some cases, when the user groups are updated:

default_groups = self.config.default_groups.all() ... ValueError: "<OpenIDConnectConfig: OpenID Connect configuration>" needs to have a value for field "id" before this many-to-many relationship can be used.

I'll check in which cases I can remove it

Don't worry too much, it was just an observation. The error makes sense if something in the test needs a saved instance, like we have some ForeignKey's to solo objects so we can use inline-admins.

Bartvaderkin · 2024-02-21T10:11:58Z

src/open_inwoner/accounts/tests/test_oidc_views.py

+        user.refresh_from_db()
+
+        self.assertTrue(User.objects.filter(oidc_id="some_username").exists())
+        self.assertEqual(user.oidc_id, "some_username")


The filter/exists line tests the same thing as the user.refresh_from_db() + self.assertEqual(user.oidc_id, "some_username")

issue: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2101

issue: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2101 previously, if a user logged in with credentials for which there was no User in django yet, the User object would be created but the user would not actually be authenticated (and thus have to log in a second time)

ensure OIDC users: * are redirected to the admin if OIDC is enabled for admins * get is_staff is true when the User instance is created

Bartvaderkin

Cool. I left a small note but it is not blocking.

src/open_inwoner/accounts/tests/test_oidc_views.py

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

stevenbal marked this pull request as draft February 12, 2024 15:00

stevenbal force-pushed the fix/2101-oidc-admin-issues branch 4 times, most recently from d6433bc to 65afe3d Compare February 15, 2024 11:00

stevenbal marked this pull request as ready for review February 15, 2024 11:01

stevenbal requested review from alextreme and Bartvaderkin February 15, 2024 11:04

stevenbal force-pushed the fix/2101-oidc-admin-issues branch from 65afe3d to 85e3e92 Compare February 15, 2024 11:50

stevenbal force-pushed the fix/2101-oidc-admin-issues branch 3 times, most recently from 11f9df2 to 0efc77e Compare February 19, 2024 11:03

Bartvaderkin suggested changes Feb 21, 2024

View reviewed changes

stevenbal added 5 commits February 22, 2024 10:32

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

d207099

issue: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2101

💄 [#2101] Fix OIDC admin login styling

e7222ca

🐛 [#2101] Properly implement OIDC for admin users

d426fdd

ensure OIDC users: * are redirected to the admin if OIDC is enabled for admins * get is_staff is true when the User instance is created

✅ [#2101] Add/fix tests for OIDC for admins/regular users

f5cee21

stevenbal force-pushed the fix/2101-oidc-admin-issues branch from 0efc77e to 4282281 Compare February 22, 2024 10:03

stevenbal requested a review from Bartvaderkin February 22, 2024 10:08

Bartvaderkin approved these changes Feb 22, 2024

View reviewed changes

src/open_inwoner/accounts/tests/test_oidc_views.py Outdated Show resolved Hide resolved

👌 [#2101] PR feedback

b388419

stevenbal force-pushed the fix/2101-oidc-admin-issues branch from 4282281 to b388419 Compare February 23, 2024 09:03

stevenbal merged commit 96c3676 into develop Feb 23, 2024
15 checks passed

stevenbal deleted the fix/2101-oidc-admin-issues branch February 23, 2024 09:44

stevenbal added a commit that referenced this pull request Feb 23, 2024

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

7c2318a

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stevenbal commented Feb 12, 2024 •

edited

Loading

codecov-commenter commented Feb 15, 2024 •

edited

Loading

Bartvaderkin left a comment

Bartvaderkin Feb 21, 2024 •

edited

Loading

stevenbal Feb 22, 2024

Bartvaderkin Feb 21, 2024

Bartvaderkin Feb 21, 2024

Bartvaderkin Feb 21, 2024 •

edited

Loading

Bartvaderkin Feb 21, 2024

Bartvaderkin Feb 21, 2024

stevenbal Feb 22, 2024 •

edited

Loading

Bartvaderkin Feb 22, 2024

Bartvaderkin Feb 21, 2024

Bartvaderkin left a comment

		new_user = User.objects.filter(email="[email protected]").first()

		self.assertIsNotNone(new_user)

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

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

Conversation

stevenbal commented Feb 12, 2024 • edited Loading

codecov-commenter commented Feb 15, 2024 • edited Loading

Codecov Report

Bartvaderkin left a comment

Choose a reason for hiding this comment

Bartvaderkin Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

stevenbal Feb 22, 2024

Choose a reason for hiding this comment

Bartvaderkin Feb 21, 2024

Choose a reason for hiding this comment

Bartvaderkin Feb 21, 2024

Choose a reason for hiding this comment

Bartvaderkin Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Bartvaderkin Feb 21, 2024

Choose a reason for hiding this comment

Bartvaderkin Feb 21, 2024

Choose a reason for hiding this comment

stevenbal Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Bartvaderkin Feb 22, 2024

Choose a reason for hiding this comment

Bartvaderkin Feb 21, 2024

Choose a reason for hiding this comment

Bartvaderkin left a comment

Choose a reason for hiding this comment

stevenbal commented Feb 12, 2024 •

edited

Loading

codecov-commenter commented Feb 15, 2024 •

edited

Loading

Bartvaderkin Feb 21, 2024 •

edited

Loading

Bartvaderkin Feb 21, 2024 •

edited

Loading

stevenbal Feb 22, 2024 •

edited

Loading