From 12453ba473c59c939126d27ec054765e886fb061 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sun, 13 Sep 2026 14:04:16 +0200 Subject: [PATCH] fix: a role is not set here while the provider holds it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two endpoints still set a role locally — PUT /access/{id}/role and PUT /admin/users/{id}/role. With OIDC_ROLE_CLAIM configured, my sync applies the person's groups on every sign-in, so a role set in the app lasts until they next sign in and is then overwritten. That is the worst kind of setting: it appears to work, holds for an afternoon, and quietly undoes itself. Both now answer 409 naming where the switch actually is. With no mapping configured they behave exactly as before, which is what a site with no provider needs. The two existing tests now patch is_configured explicitly rather than inheriting whatever the container's .env happens to say — a test that reads differently on two machines is not a test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- backend/app/routers/access.py | 2 ++ backend/app/routers/admin.py | 3 ++- backend/app/services/sso_roles.py | 19 +++++++++++++++++++ backend/tests/test_access.py | 24 +++++++++++++++++++++--- backend/tests/test_sso_roles.py | 29 +++++++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 4 deletions(-) diff --git a/backend/app/routers/access.py b/backend/app/routers/access.py index 1df217c..61038dd 100644 --- a/backend/app/routers/access.py +++ b/backend/app/routers/access.py @@ -24,6 +24,7 @@ from sqlalchemy import func from sqlalchemy.orm import Session from app.database import get_db +from app.services import sso_roles from app.models.article import Article from app.models.category_grant import CategoryGrant from app.models.folder import QuestionFolder, QuestionFolderQuestion @@ -176,6 +177,7 @@ def set_role(user_id: int, data: RoleWrite, db: Session = Depends(get_db), purpose: a wildcard grant that silently meant everything would be the kind of permission nobody can audit. """ + sso_roles.refuse_local_role_change() if data.role not in ("user", "moderator"): raise HTTPException(400, "role must be user or moderator") user = _target_user(db, user_id) diff --git a/backend/app/routers/admin.py b/backend/app/routers/admin.py index 4bc2e79..f0d8d4a 100644 --- a/backend/app/routers/admin.py +++ b/backend/app/routers/admin.py @@ -13,7 +13,7 @@ from app.config import settings from app.database import get_db from app.models.user import User from app.models.ai_model_config import AIModelConfig -from app.services import ai_service, site_settings, tts_voices +from app.services import ai_service, site_settings, sso_roles, tts_voices from app.schemas.auth import UserResponse, UserUpdateRole, UserCreate from app.schemas.admin import AIModelConfigCreate, AIModelConfigResponse, AIModelConfigUpdate from app.utils.auth import require_admin, get_current_user, get_password_hash @@ -38,6 +38,7 @@ def update_user_role( db: Session = Depends(get_db), admin: User = Depends(require_admin), ): + sso_roles.refuse_local_role_change() if role_data.role not in ("admin", "moderator", "user"): raise HTTPException(status_code=400, detail="Role must be admin, moderator, or user") diff --git a/backend/app/services/sso_roles.py b/backend/app/services/sso_roles.py index 136f9c9..28b690e 100644 --- a/backend/app/services/sso_roles.py +++ b/backend/app/services/sso_roles.py @@ -46,6 +46,25 @@ def is_configured(settings) -> bool: or _values(getattr(settings, "OIDC_MODERATOR_GROUPS", "")))) +def refuse_local_role_change() -> None: + """Stop a role being set here when the provider is the one that decides. + + With group mapping configured, a role set in this app lasts until the + person next signs in and is then overwritten from their groups — a change + that appears to work, holds for an afternoon and quietly undoes itself. + Better to say where the switch actually is. + """ + from fastapi import HTTPException + + from app.config import settings + + if is_configured(settings): + raise HTTPException( + 409, + "Roles come from your identity provider's groups on this site. " + "Change the person's group there and it applies at their next sign-in.") + + def role_for(userinfo: dict, settings) -> str | None: """The role this person's groups earn, or None when mapping is off. diff --git a/backend/tests/test_access.py b/backend/tests/test_access.py index 92b72e5..0916d75 100644 --- a/backend/tests/test_access.py +++ b/backend/tests/test_access.py @@ -90,16 +90,34 @@ class AccessTests(unittest.TestCase): self.assertIn("already has everything", response.json()["detail"]) def test_everything_is_a_role_change_and_is_named_as_one(self): + # With no provider mapping configured, the role is set here. The + # patch is explicit because the container's own .env may have + # OIDC_ROLE_CLAIM set, and a test must not read differently on two + # machines. uid = self.peer().id - response = self.client.put(f"/access/{uid}/role", json={"role": "moderator"}) + with patch("app.services.sso_roles.is_configured", return_value=False): + response = self.client.put(f"/access/{uid}/role", json={"role": "moderator"}) self.assertEqual(response.status_code, 200, response.text) self.db.expire_all() self.assertEqual(self.db.get(type(self.peer()), uid).role, "moderator") self.assertTrue({r["id"]: r for r in self.client.get("/access/").json()["users"]}[uid]["everything"]) def test_you_cannot_change_your_own_access(self): - self.assertEqual( - self.client.put(f"/access/{self.bank.mod.id}/role", json={"role": "user"}).status_code, 400) + with patch("app.services.sso_roles.is_configured", return_value=False): + self.assertEqual( + self.client.put(f"/access/{self.bank.mod.id}/role", json={"role": "user"}).status_code, 400) + + def test_a_role_is_not_set_here_when_the_provider_holds_it(self): + """A local change would be overwritten at their next sign-in. + + Which is the worst kind of setting: it appears to work, holds for an + afternoon, and quietly undoes itself. The endpoint says where the + switch actually is instead. + """ + with patch("app.services.sso_roles.is_configured", return_value=True): + response = self.client.put(f"/access/{self.peer().id}/role", json={"role": "moderator"}) + self.assertEqual(response.status_code, 409) + self.assertIn("identity provider", response.json()["detail"]) def test_an_unknown_kind_is_refused_rather_than_ignored(self): uid = self.peer().id diff --git a/backend/tests/test_sso_roles.py b/backend/tests/test_sso_roles.py index 385db17..95a2634 100644 --- a/backend/tests/test_sso_roles.py +++ b/backend/tests/test_sso_roles.py @@ -6,6 +6,7 @@ import os os.environ.setdefault("DATABASE_URL", "sqlite:///:memory:") import unittest +import unittest.mock from types import SimpleNamespace from sqlalchemy import create_engine @@ -114,3 +115,31 @@ class SsoLoginShapeTests(unittest.TestCase): self.assertTrue(inspect.iscoroutinefunction(auth.sso_callback)) source = inspect.getsource(auth.sso_login) self.assertIn("await oauth.oidc.authorize_redirect", source) + + +class LocalRoleChangeTests(unittest.TestCase): + """Where the switch is, once the provider holds it. + + A role set in this app while group mapping is on lasts until the person + next signs in and is then overwritten from their groups — a change that + appears to work, holds for an afternoon and quietly undoes itself. The two + role endpoints say where the real switch is instead. + """ + + def test_refused_while_mapping_is_configured(self): + from fastapi import HTTPException + from types import SimpleNamespace + mapped = SimpleNamespace(OIDC_ROLE_CLAIM="groups", OIDC_ADMIN_GROUPS="bosses", + OIDC_MODERATOR_GROUPS="") + with unittest.mock.patch("app.config.settings", mapped): + with self.assertRaises(HTTPException) as caught: + sso_roles.refuse_local_role_change() + self.assertEqual(caught.exception.status_code, 409) + self.assertIn("identity provider", caught.exception.detail) + + def test_allowed_when_nothing_is_mapped(self): + from types import SimpleNamespace + unmapped = SimpleNamespace(OIDC_ROLE_CLAIM="", OIDC_ADMIN_GROUPS="", + OIDC_MODERATOR_GROUPS="") + with unittest.mock.patch("app.config.settings", unmapped): + self.assertIsNone(sso_roles.refuse_local_role_change())