fix: a role is not set here while the provider holds it
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN
This commit is contained in:
parent
cf2f42975e
commit
12453ba473
5 changed files with 73 additions and 4 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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())
|
||||
|
|
|
|||
Loading…
Reference in a new issue