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())