From e92601bb31765a2ff4a804dadf50aa1ec0360ffa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BCdiger=20Rolf?= Date: Mon, 19 Oct 2026 23:28:39 +0200 Subject: [PATCH] fix(auth): username fallback never adopts local or LTI-native accounts (0.2.2) With OIDC_MATCH_BY_USERNAME_FALLBACK an IdP login could adopt a local break-glass superuser (no subject) or an LTI-native account (lti:...) that shared the username. The fallback now only matches accounts with an existing non-LTI OIDC subject, and logs each adoption at WARNING. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 20 +++++++++ packages/django/basicbar-auth/README.md | 2 + .../basicbar-auth/basicbar_auth/oidc.py | 42 +++++++++++++++++-- .../basicbar_auth/tests/test_backend.py | 41 +++++++++++++++++- packages/django/basicbar-auth/pyproject.toml | 2 +- 5 files changed, 101 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 516b350..e9e0ab4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,26 @@ Changelog ist die Upgrade-Anleitung für die Tools. ## [Unreleased] +### basicbar-auth (→ wird `auth/v0.2.2`) + +**Username-Fallback übernimmt keine lokalen oder LTI-Konten mehr** (Security- +Review Abstimmbar). Mit `OIDC_MATCH_BY_USERNAME_FALLBACK=True` konnte ein +IdP-Login ein lokales Break-Glass-Konto (`createsuperuser`, kein Subject) +oder ein LTI-natives Konto (`lti::`) mit gleichem Usernamen +übernehmen — samt Superuser-Rechten. Der Fallback matcht jetzt nur Konten +mit vorhandenem, nicht-`lti:`-Subject (Subject-Drift nach Realm-Reimport +funktioniert unverändert). Jede Übernahme wird als WARNING mit Username sowie +altem und neuem Subject protokolliert. Kollidiert der Username mit einem +solchen Konto, verweigert `create_user` sauber mit `PermissionDenied` (Fehler- +Redirect statt 500, WARNING-Log); das bestehende Konto bleibt unberührt. Die Kollisionsprüfung auf dem +Create-Pfad ist case-insensitiv (`username__iexact`): eine IdP-Identität +„Frank“ wird abgewiesen, wenn lokal „frank“ existiert — bewusste +Verhaltensänderung, unabhängig vom Fallback-Flag. + +Migration: Version heben. Wer sich bewusst auf die Übernahme lokaler Konten +verlassen hat, muss diesen Account einmalig manuell mit dem OIDC-Subject +versehen. + ### @basicbar/ui (→ wird `ui/v0.9.1`) **`RichTextEditor` verwirft keine Eingaben mehr durch veraltete Echos.** Der diff --git a/packages/django/basicbar-auth/README.md b/packages/django/basicbar-auth/README.md index 1fc6c3f..1944707 100644 --- a/packages/django/basicbar-auth/README.md +++ b/packages/django/basicbar-auth/README.md @@ -86,6 +86,8 @@ standort-neutral — die Policy ist Konfiguration: eine neue Person das verwaiste Konto (samt Historie!) ihrer Vorgängerin erben. An Hochschulen üblich sind Mindest-Vakanzzeiten für Kennungen; fragt eure IdP-Betreiber. + Greift nur auf Konten mit vorhandenem OIDC-Subject; lokale Konten und + LTI-Konten werden nie übernommen. - **Löschfrist für inaktive Accounts** (Retention/Anonymisierung — die Umsetzung liegt derzeit im jeweiligen Tool, z. B. ausleihbar; das Paket liefert dafür `last_login`-Pflege und die `anonymized_at`-Konvention). diff --git a/packages/django/basicbar-auth/basicbar_auth/oidc.py b/packages/django/basicbar-auth/basicbar_auth/oidc.py index 1c1d95f..cdf0309 100644 --- a/packages/django/basicbar-auth/basicbar_auth/oidc.py +++ b/packages/django/basicbar-auth/basicbar_auth/oidc.py @@ -145,9 +145,28 @@ def filter_users_by_claims(self, claims): # trusted IdP, so fall back to it instead of crashing into the # unique-username constraint; update_user persists the new subject. username = claims.get(conf.get("OIDC_CLAIM_USERNAME")) - if username: - return self.UserModel.objects.filter(username__iexact=username) - return self.UserModel.objects.none() + if not username: + return self.UserModel.objects.none() + # Only accounts that already carry an OIDC subject qualify: local + # accounts (createsuperuser break-glass, no subject) and LTI-native + # accounts (``lti::``, see basicbar-lti) must never be + # adopted by an IdP login that merely shares the username. + candidates = ( + self.UserModel.objects.filter(username__iexact=username) + .exclude(subject__isnull=True) + .exclude(subject="") + .exclude(subject__startswith="lti:") + ) + # Evaluating once fills the queryset cache, so the caller reuses it. + matches = list(candidates) + if len(matches) == 1: + logger.warning( + "OIDC username fallback adopts user %r: subject %r -> %r", + matches[0].get_username(), + matches[0].subject, + subject, + ) + return candidates def create_user(self, claims): # Optional per-deployment cap on accounts. New identities are turned @@ -162,8 +181,23 @@ def create_user(self, claims): raise PermissionDenied( "The maximum number of user accounts has been reached." ) + username = claims.get(conf.get("OIDC_CLAIM_USERNAME")) or claims.get("sub") + # A local or LTI-native account with this username is not linked to + # this identity (the fallback refuses to adopt it): refuse cleanly + # instead of crashing into the unique-username constraint. + if self.UserModel.objects.filter(username__iexact=username).exists(): + logger.warning( + "Refusing OIDC account creation: username %r already exists " + "and is not linked to sub=%r.", + username, + claims.get("sub"), + ) + raise PermissionDenied( + "An account with this username already exists and is not " + "linked to this identity." + ) user = self.UserModel( - username=claims.get(conf.get("OIDC_CLAIM_USERNAME")) or claims.get("sub"), + username=username, email=claims.get(conf.get("OIDC_CLAIM_EMAIL"), ""), subject=claims.get("sub"), first_name=claims.get(conf.get("OIDC_CLAIM_FIRST_NAME"), ""), diff --git a/packages/django/basicbar-auth/basicbar_auth/tests/test_backend.py b/packages/django/basicbar-auth/basicbar_auth/tests/test_backend.py index 7dca7c3..354a63f 100644 --- a/packages/django/basicbar-auth/basicbar_auth/tests/test_backend.py +++ b/packages/django/basicbar-auth/basicbar_auth/tests/test_backend.py @@ -109,7 +109,8 @@ def test_opt_in_falls_back_to_username_and_adopts_subject(self): """A re-imported dev realm (or an IdP migration) re-issues subjects; the login must match the existing account instead of crashing into the unique-username constraint.""" - matched = self.backend.filter_users_by_claims(self.drifted) + with self.assertLogs("basicbar_auth.oidc", level="WARNING"): + matched = self.backend.filter_users_by_claims(self.drifted) self.assertEqual(list(matched), [self.user]) # update_user adopts the new subject, so the next login matches # directly again. @@ -117,6 +118,44 @@ def test_opt_in_falls_back_to_username_and_adopts_subject(self): self.assertEqual(updated.subject, "new-sub-456") self.assertEqual(self.backend.filter_users_by_claims(self.drifted).count(), 1) + @override_settings(OIDC_MATCH_BY_USERNAME_FALLBACK=True) + def test_fallback_never_adopts_a_local_account_without_subject(self): + """A break-glass account from ``createsuperuser`` has no OIDC subject; + an IdP login with the same username must not take it over.""" + for subject in (None, ""): + with self.subTest(subject=subject): + User.objects.all().delete() + User.objects.create(username="frank", subject=subject) + claims = {**CLAIMS, "sub": f"fresh-{subject!r}"} + self.assertEqual(self.backend.filter_users_by_claims(claims).count(), 0) + with self.assertRaises(PermissionDenied): + self.backend.create_user(claims) + self.assertEqual(User.objects.count(), 1) + + @override_settings(OIDC_MATCH_BY_USERNAME_FALLBACK=True) + def test_fallback_leaves_a_local_superuser_untouched(self): + User.objects.all().delete() + root = User.objects.create_superuser(username="frank", password="pw") + claims = {**CLAIMS, "sub": "fresh-sub"} + self.assertEqual(self.backend.filter_users_by_claims(claims).count(), 0) + # Nothing matched, so the OIDC flow takes the create path; the + # username clash is rejected instead of adopting the superuser. + with self.assertRaises(PermissionDenied): + self.backend.create_user(claims) + root.refresh_from_db() + self.assertTrue(root.is_superuser) + self.assertIsNone(root.subject) + + @override_settings(OIDC_MATCH_BY_USERNAME_FALLBACK=True) + def test_fallback_never_adopts_an_lti_native_account(self): + User.objects.all().delete() + User.objects.create(username="frank", subject="lti:3:abc") + claims = {**CLAIMS, "sub": "fresh-sub"} + self.assertEqual(self.backend.filter_users_by_claims(claims).count(), 0) + with self.assertLogs("basicbar_auth.oidc", level="WARNING"): + with self.assertRaises(PermissionDenied): + self.backend.create_user(claims) + @override_settings(OIDC_MATCH_BY_USERNAME_FALLBACK=True) def test_unknown_subject_and_username_matches_nothing(self): stranger = {"sub": "other", "preferred_username": "someone-else"} diff --git a/packages/django/basicbar-auth/pyproject.toml b/packages/django/basicbar-auth/pyproject.toml index 54edfad..dee1d8c 100644 --- a/packages/django/basicbar-auth/pyproject.toml +++ b/packages/django/basicbar-auth/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "basicbar-auth" -version = "0.2.1" +version = "0.2.2" description = "OIDC-Fundament der virtUOS -bar-Tools: Backend, Silent Login, Back-Channel-Logout, Discovery, AbstractBasicUser, optionale Account-Limits" readme = "README.md" requires-python = ">=3.12"