From 94cee93a104f4f1e13e72a957d08f64e16d0d889 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 13:48:12 +0200 Subject: [PATCH 1/6] users: OAuth sign-in links or verifies an account only on a provider's email-verification proof (#622) OIDC requires email_verified=true, Google the People API metadata.verified of the primary email, GitHub a verified entry in /user/emails; Microsoft (Entra) email claims are never proof. Existing subject links keep working. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 5 + modules/users/tests/_users_oauth_support.py | 11 +- .../tests/test_users_oauth_email_claim.py | 127 ++++++++++++++++++ .../test_users_oauth_provider_identity.py | 65 +++++++++ modules/users/users/oauth/api.py | 14 +- modules/users/users/oauth/providers.py | 42 +++++- 6 files changed, 253 insertions(+), 11 deletions(-) create mode 100644 modules/users/tests/test_users_oauth_email_claim.py create mode 100644 modules/users/tests/test_users_oauth_provider_identity.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4152af0d7..79e72e71f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,11 @@ All notable changes to this project are documented in this file. The format is b session (#615). ### Security +- Users: require an OIDC UserInfo `email_verified: true` claim before linking by + email or marking a new account verified. Other OAuth providers no longer link + by email or automatically verify new accounts; existing subject links still + sign in (#622). **Behaviour change:** providers without email verification + proof cannot sign in to an existing account by matching its email alone. - DB: sessions bound to a SQLAlchemy `Connection` now inherit its engine's tenant policy and audit configuration, preserving strict tenant isolation and audit callbacks (#630). - Sign-in now rotates the session: users' `on_after_login` (password, OAuth, accept-invite) diff --git a/modules/users/tests/_users_oauth_support.py b/modules/users/tests/_users_oauth_support.py index d2e4f5638..333d132e4 100644 --- a/modules/users/tests/_users_oauth_support.py +++ b/modules/users/tests/_users_oauth_support.py @@ -25,12 +25,15 @@ async def get_access_token(self, code: str, redirect_uri: str) -> dict: async def get_id_email(self, access_token: str) -> tuple[str, str]: return self._account_id, self._account_email + async def get_profile(self, access_token: str) -> dict: + return {"sub": self._account_id, "email": self._account_email, "email_verified": True} + def install_fake_provider(app, account_id: str, account_email: str) -> None: from users.oauth import OAuthProvider - app.state.users.oauth_clients["microsoft"] = OAuthProvider( - "microsoft", "Microsoft", _FakeOAuthClient(account_id, account_email) + app.state.users.oauth_clients["oidc"] = OAuthProvider( + "oidc", "OIDC", _FakeOAuthClient(account_id, account_email) ) @@ -70,11 +73,11 @@ async def seed_password_user(app, email: str, **fields) -> uuid.UUID: async def oauth_callback(client: httpx.AsyncClient) -> httpx.Response: """Drive /login then /callback; return the callback response.""" - login = await client.get("/api/users/auth/microsoft/login", follow_redirects=False) + login = await client.get("/api/users/auth/oidc/login", follow_redirects=False) assert login.status_code == 302 state = parse_qs(urlparse(login.headers["location"]).query)["state"][0] return await client.get( - f"/api/users/auth/microsoft/callback?code=abc&state={state}", + f"/api/users/auth/oidc/callback?code=abc&state={state}", follow_redirects=False, ) diff --git a/modules/users/tests/test_users_oauth_email_claim.py b/modules/users/tests/test_users_oauth_email_claim.py new file mode 100644 index 000000000..678422985 --- /dev/null +++ b/modules/users/tests/test_users_oauth_email_claim.py @@ -0,0 +1,127 @@ +"""Only a proven provider email may link an existing account (GH #622).""" + +from urllib.parse import parse_qs, urlparse + +import pytest +from _users_oauth_support import find_user, seed_password_user, set_allow_signup +from sqlalchemy import select +from users.models import OAuthAccount +from users.oauth import OAuthProvider + +_EMAIL = "victim@example.com" + + +class _ClaimClient: + def __init__(self, claim): + self.profile = {"sub": "idp-attacker", "email": _EMAIL, **claim} + + async def get_authorization_url(self, redirect_uri, state): + return f"https://idp.example/authorize?state={state}" + + async def get_access_token(self, code, redirect_uri): + return {"access_token": "fake-token"} + + async def get_profile(self, token): + return self.profile + + async def get_id_email(self, token): + return self.profile["sub"], self.profile["email"] + + +async def _callback(app, client, claim, provider="oidc"): + app.state.users.oauth_clients[provider] = OAuthProvider(provider, provider, _ClaimClient(claim)) + login = await client.get(f"/api/users/auth/{provider}/login") + state = parse_qs(urlparse(login.headers["location"]).query)["state"][0] + return await client.get(f"/api/users/auth/{provider}/callback?code=abc&state={state}") + + +@pytest.mark.anyio +@pytest.mark.parametrize("allow_signup", [False, True]) +@pytest.mark.parametrize("claim", [{}, {"email_verified": False}, {"email_verified": "true"}]) +async def test_oidc_unproven_email_cannot_link_verified_user( + users_app, anon_client, claim, allow_signup +): + set_allow_signup(users_app, allow_signup) + user_id = await seed_password_user(users_app, _EMAIL, is_verified=True) + + response = await _callback(users_app, anon_client, claim) + + assert response.status_code == 400 + assert response.json()["detail"] == "OAUTH_USER_ALREADY_EXISTS" + assert users_app.state.users.settings.cookie_name not in response.cookies + async with users_app.state.sm.db.session_factory() as session: + assert list((await session.execute(select(OAuthAccount))).scalars()) == [] + assert (await find_user(users_app, _EMAIL)).id == user_id + + +@pytest.mark.anyio +@pytest.mark.parametrize( + "claim, verified", + [({}, False), ({"email_verified": False}, False), ({"email_verified": True}, True)], +) +async def test_oidc_signup_verification_follows_claim(users_app, anon_client, claim, verified): + set_allow_signup(users_app, True) + + assert (await _callback(users_app, anon_client, claim)).status_code == 303 + assert (await find_user(users_app, _EMAIL)).is_verified is verified + + +@pytest.mark.anyio +async def test_oidc_verified_email_can_link(users_app, anon_client): + user_id = await seed_password_user(users_app, _EMAIL, is_verified=True) + + assert (await _callback(users_app, anon_client, {"email_verified": True})).status_code == 303 + async with users_app.state.sm.db.session_factory() as session: + link = (await session.execute(select(OAuthAccount))).scalar_one() + assert link.user_id == user_id + + +@pytest.mark.anyio +async def test_oidc_existing_subject_can_sign_in_without_verified_email(users_app, anon_client): + set_allow_signup(users_app, True) + assert (await _callback(users_app, anon_client, {})).status_code == 303 + anon_client.cookies.clear() + set_allow_signup(users_app, False) + + assert (await _callback(users_app, anon_client, {})).status_code == 303 + + +@pytest.mark.anyio +# Google proof comes from People API metadata: test_users_oauth_provider_identity.py. +@pytest.mark.parametrize("provider", ["github", "microsoft"]) +async def test_provider_without_email_proof_cannot_link(users_app, anon_client, provider): + await seed_password_user(users_app, _EMAIL, is_verified=True) + + response = await _callback(users_app, anon_client, {}, provider) + + assert response.status_code == 400 + assert users_app.state.users.settings.cookie_name not in response.cookies + + +@pytest.mark.anyio +@pytest.mark.parametrize( + "claim, verified", + [ + ({}, False), + ({"email_verified": False}, False), + ({"email_verified": "true"}, False), + ({"email_verified": True}, True), + ], +) +async def test_oidc_identity_preserves_email_proof(claim, verified): + provider = OAuthProvider("oidc", "OIDC", _ClaimClient(claim)) + + assert await provider.get_identity("token") == ("idp-attacker", _EMAIL, verified) + + +@pytest.mark.anyio +# Google proof comes from People API metadata: test_users_oauth_provider_identity.py. +@pytest.mark.parametrize("provider", ["github", "microsoft"]) +async def test_non_oidc_identity_does_not_infer_email_proof(provider): + client = _ClaimClient({"email_verified": True}) + + assert await OAuthProvider(provider, provider, client).get_identity("token") == ( + "idp-attacker", + _EMAIL, + False, + ) diff --git a/modules/users/tests/test_users_oauth_provider_identity.py b/modules/users/tests/test_users_oauth_provider_identity.py new file mode 100644 index 000000000..755f24ca2 --- /dev/null +++ b/modules/users/tests/test_users_oauth_provider_identity.py @@ -0,0 +1,65 @@ +"""Per-provider email-verification proof for OAuth sign-in (GH #622).""" + +import pytest +from users.oauth import OAuthProvider + + +class _Google: + def __init__(self, verified): + self.profile = { + "resourceName": "people/1", + "emailAddresses": [ + {"value": "other@example.com", "metadata": {"primary": False, "verified": True}}, + {"value": "me@example.com", "metadata": {"primary": True, "verified": verified}}, + ], + } + + async def get_profile(self, token): + return self.profile + + +class _GitHub: + def __init__(self, emails, fail=False): + self.emails, self.fail = emails, fail + + async def get_id_email(self, token): + return "42", "me@example.com" + + async def get_emails(self, token): + if self.fail: + raise RuntimeError("scope missing") + return self.emails + + +class _Microsoft: + async def get_id_email(self, token): + return "ms-1", "me@example.com" + + +@pytest.mark.parametrize("verified", [True, False, None]) +async def test_google_uses_primary_email_verification(verified): + provider = OAuthProvider("google", "Google", _Google(verified)) + assert await provider.get_identity("t") == ("people/1", "me@example.com", verified is True) + + +@pytest.mark.parametrize( + "emails, expected", + [ + ([{"email": "me@example.com", "verified": True, "primary": True}], True), + ([{"email": "me@example.com", "verified": False, "primary": True}], False), + ([{"email": "other@example.com", "verified": True}], False), + ], +) +async def test_github_requires_the_chosen_email_to_be_verified(emails, expected): + provider = OAuthProvider("github", "GitHub", _GitHub(emails)) + assert await provider.get_identity("t") == ("42", "me@example.com", expected) + + +async def test_github_unreadable_emails_is_not_proof(): + provider = OAuthProvider("github", "GitHub", _GitHub([], fail=True)) + assert (await provider.get_identity("t"))[2] is False + + +async def test_microsoft_email_is_never_proof(): + provider = OAuthProvider("microsoft", "Microsoft", _Microsoft()) + assert await provider.get_identity("t") == ("ms-1", "me@example.com", False) diff --git a/modules/users/users/oauth/api.py b/modules/users/users/oauth/api.py index 41c90bd2b..6e9591c48 100644 --- a/modules/users/users/oauth/api.py +++ b/modules/users/users/oauth/api.py @@ -51,10 +51,10 @@ def _resolve_provider(request: Request, provider: str) -> OAuthProvider: async def _existing_account( user_manager: UserManager, provider: str, account_id: str, account_email: str ) -> tuple[User | None, bool]: - """The account ``oauth_callback`` would sign in, and whether it is already linked. + """Find an existing identity or email, and whether it is already linked. - Mirrors its two lookups: the linked OAuth account first, then — because - the callback passes ``associate_by_email=True`` — an account by email. + Mirrors the callback's two lookups: the linked OAuth account first, then + an account by email (which requires provider verification before linking). ``(None, False)`` means the callback would create a new account. """ try: @@ -122,7 +122,9 @@ async def callback( callback_url = str(request.url_for(_CALLBACK_ROUTE_NAME, provider=provider)) token = await provider_obj.client.get_access_token(code, callback_url) - account_id, account_email = await provider_obj.client.get_id_email(token["access_token"]) + account_id, account_email, email_verified = await provider_obj.get_identity( + token["access_token"] + ) if account_email is None: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="OAUTH_NO_EMAIL") @@ -155,8 +157,8 @@ async def callback( token.get("expires_at"), token.get("refresh_token"), request, - associate_by_email=True, - is_verified_by_default=True, + associate_by_email=email_verified, + is_verified_by_default=email_verified, ) except fu_exceptions.UserAlreadyExists: raise HTTPException( diff --git a/modules/users/users/oauth/providers.py b/modules/users/users/oauth/providers.py index e8bb3e470..cc80ba6ad 100644 --- a/modules/users/users/oauth/providers.py +++ b/modules/users/users/oauth/providers.py @@ -12,7 +12,7 @@ from __future__ import annotations import logging -from typing import TYPE_CHECKING, NamedTuple +from typing import TYPE_CHECKING, Any, NamedTuple if TYPE_CHECKING: from httpx_oauth.oauth2 import BaseOAuth2 @@ -29,6 +29,46 @@ class OAuthProvider(NamedTuple): display_name: str client: BaseOAuth2 + async def get_identity(self, token: str) -> tuple[str, str | None, bool]: + """Return subject, email, and proof that the provider verified that email. + + ``get_id_email`` drops verification metadata, so each provider reads it + from its own profile shape. Only a positive proof links an existing + account or marks a new one verified (#622). + """ + if self.name == "oidc": + profile = await self.client.get_profile(token) + return str(profile["sub"]), profile.get("email"), profile.get("email_verified") is True + if self.name == "google": + return _google_identity(await self.client.get_profile(token)) + account_id, email = await self.client.get_id_email(token) + if email is None: + return account_id, None, False + if self.name == "github": + return account_id, email, await _github_verified(self.client, token, email) + # Microsoft (Entra) email claims are tenant-controlled and unverified ("nOAuth"). + return account_id, email, False + + +def _google_identity(profile: dict[str, Any]) -> tuple[str, str | None, bool]: + """People API profile → (resourceName, primary email, its ``metadata.verified``).""" + primary = next( + (e for e in profile.get("emailAddresses", []) if e.get("metadata", {}).get("primary")), + None, + ) + if primary is None: + return str(profile["resourceName"]), None, False + verified = primary.get("metadata", {}).get("verified") is True + return str(profile["resourceName"]), primary.get("value"), verified + + +async def _github_verified(client: Any, token: str, email: str) -> bool: + try: + emails = await client.get_emails(token) + except Exception: + return False + return any(e.get("email") == email and e.get("verified") is True for e in emails) + def build_clients(settings: UsersSettings) -> list[OAuthProvider]: """Return one entry per provider that has both id and secret configured. From d9322a04dbfdc6d61bdf21c9b34d9ec6ed80cc0e Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 13:51:21 +0200 Subject: [PATCH 2/6] users: an unverified account (SSO-provisioned too) never absorbs a later verified OAuth login (#622) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../tests/test_users_oauth_email_claim.py | 17 ++++++++++++++ modules/users/users/oauth/api.py | 23 ++++++++----------- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/modules/users/tests/test_users_oauth_email_claim.py b/modules/users/tests/test_users_oauth_email_claim.py index 678422985..d93970f0e 100644 --- a/modules/users/tests/test_users_oauth_email_claim.py +++ b/modules/users/tests/test_users_oauth_email_claim.py @@ -125,3 +125,20 @@ async def test_non_oidc_identity_does_not_infer_email_proof(provider): _EMAIL, False, ) + + +@pytest.mark.anyio +async def test_unproven_sso_account_cannot_capture_a_later_verified_login(users_app, anon_client): + """Pre-hijack: an unproven provider creates the account, a verified one must not join it.""" + set_allow_signup(users_app, True) + assert (await _callback(users_app, anon_client, {}, "microsoft")).status_code == 303 + assert (await find_user(users_app, _EMAIL)).is_verified is False + anon_client.cookies.clear() + + response = await _callback(users_app, anon_client, {"email_verified": True}) + + assert response.status_code == 403 + assert response.json()["detail"] == "OAUTH_ACCOUNT_NOT_VERIFIED" + async with users_app.state.sm.db.session_factory() as session: + providers = {a.oauth_name for a in (await session.execute(select(OAuthAccount))).scalars()} + assert providers == {"microsoft"} diff --git a/modules/users/users/oauth/api.py b/modules/users/users/oauth/api.py index 6e9591c48..1eb0dd49d 100644 --- a/modules/users/users/oauth/api.py +++ b/modules/users/users/oauth/api.py @@ -67,21 +67,18 @@ async def _existing_account( return None, False -def _unproven_password_account(user: User) -> bool: - """A self-registered password account nobody has proven owns its email. +def _unproven_account(user: User) -> bool: + """An account nobody has proven owns its email. Linking an SSO identity to it by email would hand the provider's user an - account someone else may hold the password to — register the victim's - address first, wait for them to sign in with SSO, and share their account - (GH #547). External (SSO-provisioned) accounts have no password, and a - pending invite's password is random and unknown to anyone, so both link. + account someone else may already control: register the victim's address + with a password (GH #547), or sign in first through a provider that gives + no email proof so an unverified SSO account holds the attacker's link + (#622), then wait for the real owner to sign in with a verified email. A + pending invite links: its password is random and an admin chose the + address, so nobody could have pre-created it. """ - return ( - not user.is_verified - and not user.is_external - and user.hashed_password is not None - and user.invited_at is None - ) + return not user.is_verified and user.invited_at is None def register_oauth_routes(api_router: APIRouter) -> None: @@ -139,7 +136,7 @@ async def callback( raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="OAUTH_SIGNUP_DISABLED" ) - if existing is not None and not linked and _unproven_password_account(existing): + if existing is not None and not linked and _unproven_account(existing): raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="OAUTH_ACCOUNT_NOT_VERIFIED" ) From ea2c2d192e75e640816c8cd1bc3057762e63ac12 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 14:11:41 +0200 Subject: [PATCH 3/6] tenants: accepting an invitation requires a verified email, read fresh from the user directory (#623) Adds DirectoryUser.email_verified (None = provider doesn't track it); the users directory fills it from is_verified, Keycloak principals carry the token's email_verified claim as the fallback. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 6 +++ .../core/simple_module_core/user_directory.py | 3 ++ .../db/simple_module_db/user_directory_sql.py | 2 + modules/auth/auth/contracts/schemas.py | 6 +++ modules/keycloak/keycloak/provider.py | 1 + .../tests/test_keycloak_email_verification.py | 21 +++++++++ modules/tenants/tenants/endpoints/api.py | 21 ++++++++- modules/tenants/tenants/invitations.py | 9 ++-- .../test_tenants_invitation_verification.py | 45 +++++++++++++++++++ modules/users/users/directory.py | 15 ++++++- 10 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 modules/keycloak/tests/test_keycloak_email_verification.py create mode 100644 modules/tenants/tests/test_tenants_invitation_verification.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 79e72e71f..617536388 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,12 @@ All notable changes to this project are documented in this file. The format is b by email or automatically verify new accounts; existing subject links still sign in (#622). **Behaviour change:** providers without email verification proof cannot sign in to an existing account by matching its email alone. +- Tenants: require a verified principal email before accepting invitations, including + local accounts allowed to sign in without verification and Keycloak accounts (#623). + Verification is read fresh from the user directory (new `DirectoryUser.email_verified`; + `None` when the provider doesn't track it, in which case Keycloak's `email_verified` + token claim decides), so an account verified after signing in can accept without + re-signing in. - DB: sessions bound to a SQLAlchemy `Connection` now inherit its engine's tenant policy and audit configuration, preserving strict tenant isolation and audit callbacks (#630). - Sign-in now rotates the session: users' `on_after_login` (password, OAuth, accept-invite) diff --git a/framework/core/simple_module_core/user_directory.py b/framework/core/simple_module_core/user_directory.py index c6f20c224..2ddef91f0 100644 --- a/framework/core/simple_module_core/user_directory.py +++ b/framework/core/simple_module_core/user_directory.py @@ -45,6 +45,9 @@ class DirectoryUser: #: Role names, or ``None`` when the provider does not know them (Keycloak #: keeps roles in the IdP). Only filled when ``with_roles=True`` was asked. roles: tuple[str, ...] | None = None + #: Whether the account's email is proven, or ``None`` when the provider + #: does not track it (Keycloak: read the token's ``email_verified``). + email_verified: bool | None = None @property def label(self) -> str: diff --git a/framework/db/simple_module_db/user_directory_sql.py b/framework/db/simple_module_db/user_directory_sql.py index fe9f8efd0..3a244c1ac 100644 --- a/framework/db/simple_module_db/user_directory_sql.py +++ b/framework/db/simple_module_db/user_directory_sql.py @@ -26,6 +26,7 @@ def to_directory_user( *, roles: tuple[str, ...] | None = None, is_active: bool = True, + email_verified: bool | None = None, ) -> DirectoryUser: """Convert identity columns, preserving unknown versus empty roles. @@ -38,4 +39,5 @@ def to_directory_user( full_name=row.full_name, is_active=is_active, roles=roles, + email_verified=email_verified, ) diff --git a/modules/auth/auth/contracts/schemas.py b/modules/auth/auth/contracts/schemas.py index b28423440..148ad4153 100644 --- a/modules/auth/auth/contracts/schemas.py +++ b/modules/auth/auth/contracts/schemas.py @@ -58,6 +58,9 @@ class UserContext: # routes). ``None`` (a session, a service key) requires nothing. required_permission: str | None = None + # Providers must explicitly attest ownership of the principal's email. + email_verified: bool = False + @property def is_scoped(self) -> bool: """Whether a scope cap applies (any token-authenticated principal).""" @@ -83,6 +86,7 @@ def from_user( return cls( id=str(user.id), email=user.email, + email_verified=getattr(user, "is_verified", False) is True, name=user.full_name or user.email, roles=[r.name for r in user.roles], auth_kind=auth_kind, @@ -98,6 +102,7 @@ def to_session_dict(self) -> dict[str, Any]: return { "id": self.id, "email": self.email, + "email_verified": self.email_verified, "name": self.name, "roles": list(self.roles), "tenant_id": self.tenant_id, @@ -117,6 +122,7 @@ def from_session_dict(cls, payload: Any) -> UserContext | None: return cls( id=str(payload["id"]), email=str(payload["email"]), + email_verified=payload.get("email_verified") is True, name=str(payload["name"]), roles=list(payload.get("roles") or []), tenant_id=payload.get("tenant_id"), diff --git a/modules/keycloak/keycloak/provider.py b/modules/keycloak/keycloak/provider.py index c5b377ada..c830f293e 100644 --- a/modules/keycloak/keycloak/provider.py +++ b/modules/keycloak/keycloak/provider.py @@ -108,6 +108,7 @@ def _claims_to_user_context( return UserContext( id=cache_id, email=claims.get("email", ""), + email_verified=claims.get("email_verified") is True, name=(claims.get("preferred_username") or claims.get("name", "")), roles=self._without_tenant_roles(mapped), tenant_id=self._trusted_tenant(claims.get("tenant_id")), diff --git a/modules/keycloak/tests/test_keycloak_email_verification.py b/modules/keycloak/tests/test_keycloak_email_verification.py new file mode 100644 index 000000000..7034005db --- /dev/null +++ b/modules/keycloak/tests/test_keycloak_email_verification.py @@ -0,0 +1,21 @@ +"""Only an explicit verified-email claim can authorize invitation acceptance.""" + +import pytest +from auth.contracts.schemas import UserContext +from keycloak.provider import KeycloakAuthProvider + + +@pytest.mark.parametrize("claim", [True, False, None, "true", "false", 1]) +def test_email_verification_survives_keycloak_session(claim): + claims = {"email": "bob@corp.com"} + if claim is not None: + claims["email_verified"] = claim + ctx = KeycloakAuthProvider()._claims_to_user_context(claims, cache_id="bob") + assert ctx.email_verified is (claim is True) + restored = UserContext.from_session_dict(ctx.to_session_dict()) + assert restored.email_verified is (claim is True) + + +def test_legacy_session_does_not_assert_verified_email(): + ctx = UserContext.from_session_dict({"id": "bob", "email": "bob@corp.com", "name": "Bob"}) + assert ctx.email_verified is False diff --git a/modules/tenants/tenants/endpoints/api.py b/modules/tenants/tenants/endpoints/api.py index bee288ef1..a2dded0cf 100644 --- a/modules/tenants/tenants/endpoints/api.py +++ b/modules/tenants/tenants/endpoints/api.py @@ -9,8 +9,11 @@ from __future__ import annotations +from typing import Any + from fastapi import APIRouter, Depends, Request, Response from simple_module_core.permissions import grants +from simple_module_core.user_directory import get_user_directory from simple_module_hosting.permissions import RequiresPermission, resolved_permissions_for from simple_module_hosting.token_guard import Authenticated @@ -51,6 +54,17 @@ def _email_of(request: Request) -> str | None: return getattr(getattr(request.state, "user", None), "email", None) +async def _email_verified(request: Request, invitations: Any, user_id: str) -> bool: + """Read email proof fresh from the directory; the session's principal is a + sign-in-time snapshot. Providers that don't track it (Keycloak) fall back to + the principal's token claim (#623).""" + found = await get_user_directory(request.app).get_many(invitations.db, [user_id]) + known = found[user_id].email_verified if user_id in found else None + if known is not None: + return known + return getattr(getattr(request.state, "user", None), "email_verified", False) is True + + @router.get("/", response_model=list[MyTenantView], dependencies=[Depends(Authenticated())]) async def list_mine(user_id: UserIdDep, service: TenantServiceDep) -> list[MyTenantView]: rows = await service.list_for_user(user_id) @@ -176,7 +190,12 @@ async def accept_invitation( email = _email_of(request) if not email: raise TenantError("invitation_email_mismatch", status_code=403) - tenant = await invitations.accept(payload.get("token", ""), user_id=user_id, user_email=email) + tenant = await invitations.accept( + payload.get("token", ""), + user_id=user_id, + user_email=email, + email_verified=await _email_verified(request, invitations, user_id), + ) switch_active(request, tenant.id) membership = await invitations.tenants.get_membership(tenant.id, user_id) role = membership.role if membership else MembershipRole.MEMBER diff --git a/modules/tenants/tenants/invitations.py b/modules/tenants/tenants/invitations.py index 6da9e2339..d68e2c460 100644 --- a/modules/tenants/tenants/invitations.py +++ b/modules/tenants/tenants/invitations.py @@ -142,7 +142,9 @@ async def lookup( def is_expired(invitation: Invitation) -> bool: return _aware(invitation.expires_at) <= _now() - async def accept(self, token: str, *, user_id: str, user_email: str) -> Tenant: + async def accept( + self, token: str, *, user_id: str, user_email: str, email_verified: bool = False + ) -> Tenant: found = await self.lookup(token, for_update=True) if found is None: raise TenantError("invitation_not_found", status_code=404) @@ -153,9 +155,8 @@ async def accept(self, token: str, *, user_id: str, user_email: str) -> Tenant: raise TenantError("tenant_suspended", status_code=409) if self.is_expired(invitation): raise TenantError("invitation_expired", status_code=410) - # The link is a bearer secret; binding it to the invited address means - # a forwarded or leaked link cannot enrol a different account. - if user_email.strip().lower() != invitation.email: + # A leaked link cannot prove ownership of a self-asserted address. + if email_verified is not True or user_email.strip().lower() != invitation.email: raise TenantError("invitation_email_mismatch", status_code=403) invitation.accepted_at = _now() await self.tenants.add_member( diff --git a/modules/tenants/tests/test_tenants_invitation_verification.py b/modules/tenants/tests/test_tenants_invitation_verification.py new file mode 100644 index 000000000..df74613a6 --- /dev/null +++ b/modules/tenants/tests/test_tenants_invitation_verification.py @@ -0,0 +1,45 @@ +"""A leaked invitation must not prove ownership of an unverified address (#623).""" + +import uuid + +from users.models import User + + +async def test_unverified_account_cannot_accept_invitation(app, user_client): + app.state.users.settings.require_verification = False + async with ( + user_client("owner@corp.com") as (owner, _), + user_client("bob@corp.com") as ( + bob, + bob_id, + ), + ): + await owner.post("/api/tenants/", json={"name": "Corp"}) + issued = await owner.post( + "/api/tenants/current/invitations", + json={"email": "bob@corp.com", "role": "admin"}, + ) + assert issued.status_code == 201 + token = issued.json()["token"] + async with app.state.sm.db.session_factory() as db: + user = await db.get(User, uuid.UUID(bob_id)) + user.is_verified = False + await db.commit() + + rejected = await bob.post("/api/tenants/invitations/accept", json={"token": token}) + assert rejected.status_code == 403 + members = (await owner.get("/api/tenants/current/members")).json() + assert bob_id not in {member["user_id"] for member in members} + pending = (await owner.get("/api/tenants/current/invitations")).json() + assert issued.json()["id"] in {invitation["id"] for invitation in pending} + + # Rejection neither consumes the link nor grants the invited role. + # Verification is read fresh, so the SAME session can accept once the + # account is verified — no re-sign-in needed. + async with app.state.sm.db.session_factory() as db: + user = await db.get(User, uuid.UUID(bob_id)) + user.is_verified = True + await db.commit() + accepted = await bob.post("/api/tenants/invitations/accept", json={"token": token}) + assert accepted.status_code == 200 + assert accepted.json()["role"] == "admin" diff --git a/modules/users/users/directory.py b/modules/users/users/directory.py index 0a80604ff..2b15df5f1 100644 --- a/modules/users/users/directory.py +++ b/modules/users/users/directory.py @@ -27,7 +27,14 @@ # Columns only: loading ``User`` itself would also selectin-load its OAuth # accounts, a second query per lookup that nothing here reads. -_COLUMNS = (User.id, User.email, User.full_name, User.is_active, User.disabled_at) +_COLUMNS = ( + User.id, + User.email, + User.full_name, + User.is_active, + User.disabled_at, + User.is_verified, +) def _is_usable(row: Any) -> bool: @@ -55,7 +62,11 @@ async def get_many( user_roles = tuple(sorted(roles.get(row.id, ()))) if with_roles else None for key in parsed[row.id]: found[key] = to_directory_user( - row, key, roles=user_roles, is_active=_is_usable(row) + row, + key, + roles=user_roles, + is_active=_is_usable(row), + email_verified=bool(row.is_verified), ) return found From e11a13d578f3a2e81b8986c4bc20e370306d092d Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 14:20:36 +0200 Subject: [PATCH 4/6] users: role grants and account management never exceed the actor's authority; keep a last admin (#624) A delegate with users.manage grants only roles it holds and cannot manage admins, superusers or stronger accounts; an admin's scoped token manages ordinary accounts but never grants admin. Nobody disables or demotes themselves or the last active admin (serialized with FOR UPDATE on Postgres). Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 3 + .../test_users_admin_privilege_guards.py | 189 ++++++++++++++++++ .../test_users_admin_role_grant_rules.py | 34 ++++ modules/users/users/admin/queries.py | 5 +- modules/users/users/admin/safety.py | 93 +++++++++ modules/users/users/admin/service.py | 30 ++- modules/users/users/deps.py | 7 +- modules/users/users/errors.py | 12 +- modules/users/users/exceptions.py | 8 + modules/users/users/invites/bulk.py | 4 +- 10 files changed, 370 insertions(+), 15 deletions(-) create mode 100644 modules/users/tests/test_users_admin_privilege_guards.py create mode 100644 modules/users/tests/test_users_admin_role_grant_rules.py create mode 100644 modules/users/users/admin/safety.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 617536388..781c2861e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,9 @@ All notable changes to this project are documented in this file. The format is b session (#615). ### Security +- Users: prevent delegated `users.manage` callers from assigning roles they do not hold + or controlling more privileged accounts, including via reset links and invitations. + Block self-disable, self-demotion and removal of the last active administrator (#624). - Users: require an OIDC UserInfo `email_verified: true` claim before linking by email or marking a new account verified. Other OAuth providers no longer link by email or automatically verify new accounts; existing subject links still diff --git a/modules/users/tests/test_users_admin_privilege_guards.py b/modules/users/tests/test_users_admin_privilege_guards.py new file mode 100644 index 000000000..8b80aca5d --- /dev/null +++ b/modules/users/tests/test_users_admin_privilege_guards.py @@ -0,0 +1,189 @@ +"""Delegated user management must not grant access to stronger accounts (#624).""" + +import httpx +import pytest +from auth.contracts.schemas import UserContext +from fastapi import FastAPI +from simple_module_test import forge_session_cookie +from sqlalchemy import select +from test_api_admin import _make_user +from test_service_admin import _build_service +from users.admin.safety import check_role_assignment +from users.errors import install_exception_handlers +from users.exceptions import AdminAccountSafetyError, AdminPermissionError +from users.models import Role, User, UserRole + +pytestmark = pytest.mark.anyio + + +@pytest.mark.parametrize( + "error, status", [(AdminPermissionError, 403), (AdminAccountSafetyError, 400)] +) +async def test_admin_guard_http_status(error, status): + app = FastAPI() + install_exception_handlers(app) + + @app.post("/guarded") + async def guarded(): + raise error("Refused") + + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://testserver" + ) as client: + response = await client.post("/guarded") + assert response.status_code == status + assert response.json() == {"detail": "Refused"} + + +@pytest.mark.parametrize( + ("roles", "scopes", "wanted", "allowed"), + [ + (["admin"], None, ["admin", "user"], True), + (["manager", "user"], None, ["user"], True), + (["manager"], None, ["admin"], False), + (["manager"], None, ["powerful"], False), + (["admin"], ["users.manage"], ["admin"], False), + ], +) +def test_role_assignment_ceiling(roles, scopes, wanted, allowed): + actor = UserContext( + id="actor", email="actor@example.com", name="Actor", roles=roles, scopes=scopes + ) + if allowed: + check_role_assignment(actor, wanted) + else: + with pytest.raises(AdminPermissionError): + check_role_assignment(actor, wanted) + + +@pytest.fixture +async def delegated_client(users_app, users_db): + users_db.add(Role(name="manager")) + users_db.add(Role(name="powerful")) + await users_db.commit() + users_app.state.sm.permissions.map_role("manager", ["users.manage"]) + actor = await _make_user(users_db, "manager@example.com", role_names=["manager", "user"]) + cookie = forge_session_cookie( + str(users_app.state.sm.settings.secret_key), {"user_id": str(actor.id)} + ) + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=users_app), + base_url="http://testserver", + cookies={"session": cookie}, + ) as client: + yield client, actor.id + + +@pytest.mark.parametrize("role", ["admin", "powerful"]) +@pytest.mark.parametrize("operation", ["roles", "create", "invite", "bulk"]) +async def test_delegate_cannot_grant_unheld_role(delegated_client, users_db, operation, role): + client, actor_id = delegated_client + data = {"role_names": [role]} + if operation == "roles": + response = await client.put(f"/api/users/admin/{actor_id}/roles", json=data) + else: + data.update(email="escalated@example.com", password="SecurePass1!") + path = {"create": "", "invite": "/invite", "bulk": "/invite/bulk"}[operation] + if operation == "bulk": + data["emails"] = [data.pop("email")] + response = await client.post(f"/api/users/admin{path}", json=data) + assert response.status_code == 403 + assert ( + await users_db.execute(select(User).where(User.email == "escalated@example.com")) + ).scalar_one_or_none() is None + roles = ( + ( + await users_db.execute( + select(Role.name).join(UserRole).where(UserRole.user_id == actor_id) + ) + ) + .scalars() + .all() + ) + assert set(roles) == {"manager", "user"} + + +@pytest.mark.parametrize("kind", ["admin", "superuser", "powerful"]) +@pytest.mark.parametrize("operation", ["reset", "roles", "disable", "email", "delete"]) +async def test_delegate_cannot_control_privileged_target( + delegated_client, users_db, kind, operation +): + client, _ = delegated_client + target = await _make_user( + users_db, "protected@example.com", role_names=[] if kind == "superuser" else [kind] + ) + if kind == "superuser": + target.is_superuser = True + await users_db.commit() + url = f"/api/users/admin/{target.id}" + if operation == "reset": + response = await client.post(f"{url}/reset-password-link") + elif operation == "roles": + response = await client.put(f"{url}/roles", json={"role_names": ["user"]}) + elif operation == "disable": + response = await client.patch(f"{url}/disable") + elif operation == "email": + response = await client.patch(url, json={"email": "stolen@example.com"}) + else: + response = await client.delete(url) + assert response.status_code == 403 + await users_db.refresh(target) + assert target.email == "protected@example.com" + assert target.is_active is True + + +async def test_delegate_can_manage_ordinary_user(delegated_client, users_db): + client, _ = delegated_client + target = await _make_user(users_db, "ordinary@example.com", role_names=["user"]) + url = f"/api/users/admin/{target.id}" + response = await client.put(f"{url}/roles", json={"role_names": ["user"]}) + assert response.status_code == 200 + response = await client.post(f"{url}/reset-password-link") + assert response.status_code == 200 + assert "token=" in response.json()["link"] + response = await client.patch(f"{url}/disable") + assert response.status_code == 200 + + +@pytest.mark.parametrize("operation", ["disable", "roles"]) +async def test_admin_cannot_disable_or_demote_self(admin_client, users_db, operation): + admin = ( + await users_db.execute(select(User).where(User.email == "admin@example.com")) + ).scalar_one() + # Self-protection also applies when another administrator exists. + await _make_user(users_db, "other-admin@example.com", role_names=["admin"]) + url = f"/api/users/admin/{admin.id}/{operation}" + if operation == "roles": + response = await admin_client.put(url, json={"role_names": ["user"]}) + else: + response = await admin_client.patch(url) + assert response.status_code == 400 + await users_db.refresh(admin) + assert admin.is_active is True + assert ( + await users_db.execute(select(Role.name).join(UserRole).where(UserRole.user_id == admin.id)) + ).scalars().all() == ["admin"] + + +@pytest.mark.parametrize("operation", ["disable", "roles", "delete"]) +async def test_service_preserves_last_active_admin(users_app, users_db, operation): + admin = ( + await users_db.execute(select(User).where(User.email == "admin@example.com")) + ).scalar_one() + inactive = await _make_user(users_db, "inactive-admin@example.com", role_names=["admin"]) + inactive.is_active = False + await users_db.commit() + # The legacy superuser flag alone grants no framework admin permissions. + roleless = await _make_user(users_db, "roleless-superuser@example.com") + roleless.is_superuser = True + await users_db.commit() + service = _build_service(users_db, users_app) + with pytest.raises(AdminAccountSafetyError): + if operation == "roles": + await service.set_roles(admin.id, ["user"]) + elif operation == "delete": + await service.delete_user(admin.id) + else: + await service.disable(admin.id) + await users_db.refresh(admin) + assert admin.is_active is True diff --git a/modules/users/tests/test_users_admin_role_grant_rules.py b/modules/users/tests/test_users_admin_role_grant_rules.py new file mode 100644 index 000000000..2e18fb601 --- /dev/null +++ b/modules/users/tests/test_users_admin_role_grant_rules.py @@ -0,0 +1,34 @@ +"""Who may grant which roles (#624): full admin, an admin's scoped token, delegates.""" + +import pytest +from auth.contracts.schemas import UserContext +from users.admin.safety import check_role_assignment +from users.exceptions import AdminPermissionError + + +def _actor(roles, scopes=None): + return UserContext(id="a", email="a@x", name="a", roles=roles, scopes=scopes) + + +def test_full_admin_grants_admin(): + check_role_assignment(_actor(["admin"]), ["admin", "editor"]) + + +def test_admin_scoped_token_grants_ordinary_roles_but_not_admin(): + token = _actor(["admin"], scopes=["users.manage"]) + check_role_assignment(token, ["editor"]) + with pytest.raises(AdminPermissionError): + check_role_assignment(token, ["admin"]) + + +def test_delegate_grants_only_roles_it_holds(): + helpdesk = _actor(["helpdesk", "editor"]) + check_role_assignment(helpdesk, ["editor"]) + with pytest.raises(AdminPermissionError): + check_role_assignment(helpdesk, ["admin"]) + with pytest.raises(AdminPermissionError): + check_role_assignment(helpdesk, ["billing"]) + + +def test_trusted_in_process_caller_is_unrestricted(): + check_role_assignment(None, ["admin"]) diff --git a/modules/users/users/admin/queries.py b/modules/users/users/admin/queries.py index f4c1082d5..28e447041 100644 --- a/modules/users/users/admin/queries.py +++ b/modules/users/users/admin/queries.py @@ -116,7 +116,10 @@ async def list_roles(self) -> list[RoleListItem]: async def _get_user_with_roles(self, user_id: uuid.UUID) -> User | None: result = await self._db.execute( - select(User).where(col(User.id) == user_id).options(selectinload(User.roles)) # ty: ignore[invalid-argument-type] + select(User) + .where(col(User.id) == user_id) + .options(selectinload(User.roles)) # ty: ignore[invalid-argument-type] + .execution_options(populate_existing=True) ) return result.scalar_one_or_none() diff --git a/modules/users/users/admin/safety.py b/modules/users/users/admin/safety.py new file mode 100644 index 000000000..8fa9860f0 --- /dev/null +++ b/modules/users/users/admin/safety.py @@ -0,0 +1,93 @@ +"""Authorization and administrator availability for admin writes (#624).""" + +import uuid + +from auth.contracts.schemas import UserContext +from fastapi import FastAPI +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession +from sqlmodel import col + +from users.admin.queries import _UserServiceBase +from users.constants import ADMIN_ROLE_NAME +from users.exceptions import AdminAccountSafetyError, AdminPermissionError +from users.manager import UserManager +from users.models import Role, User, UserRole + + +def _is_admin(actor: UserContext) -> bool: + return ADMIN_ROLE_NAME in actor.roles + + +def check_role_assignment(actor: UserContext | None, role_names: list[str]) -> None: + """Refuse a grant wider than the actor's own authority. + + ``None`` is reserved for trusted in-process callers; HTTP always supplies an + actor. A full admin session grants anything; an admin's scoped token (PAT, + bearer) grants anything except ``admin`` itself; anyone else only roles they + hold. + """ + if actor is None or (_is_admin(actor) and not actor.is_scoped): + return + if _is_admin(actor): + if ADMIN_ROLE_NAME in role_names: + raise AdminPermissionError("You cannot assign roles you do not hold.") + return + if not set(role_names).issubset(actor.roles): + raise AdminPermissionError("You cannot assign roles you do not hold.") + + +class _UserAdminSafety(_UserServiceBase): + def __init__( + self, + db: AsyncSession, + user_manager: UserManager, + app: FastAPI | None = None, + *, + actor: UserContext | None = None, + ) -> None: + super().__init__(db, user_manager, app) + self._actor = actor + + async def _require_manageable_user(self, user_id: uuid.UUID) -> User: + user = await self._require_user(user_id) + actor = self._actor + if actor is None or (_is_admin(actor) and not actor.is_scoped): + return user + roles = {role.name for role in user.roles} + privileged = user.is_superuser or ADMIN_ROLE_NAME in roles + # An admin's scoped token manages ordinary accounts, never another admin. + stronger = not _is_admin(actor) and not roles.issubset(actor.roles) + if privileged or stronger: + raise AdminPermissionError("You cannot manage a more privileged account.") + return user + + async def _guard_admin_removal(self, user: User) -> None: + if self._actor is not None and str(user.id) == self._actor.id: + raise AdminAccountSafetyError("You cannot disable or demote your own account.") + if not user.is_active or user.disabled_at is not None: + return + if not user.is_superuser and ADMIN_ROLE_NAME not in {r.name for r in user.roles}: + return + + # Serialize destructive admin operations on Postgres before counting survivors. + await self._db.execute( + select(Role.id).where(Role.name == ADMIN_ROLE_NAME).with_for_update() + ) + admin_ids = ( + select(UserRole.user_id) + .join(Role, col(Role.id) == UserRole.role_id) + .where(Role.name == ADMIN_ROLE_NAME) + ) + remaining = await self._db.execute( + select(User.id) + .where( + col(User.id) != user.id, + col(User.is_active).is_(True), + col(User.disabled_at).is_(None), + col(User.id).in_(admin_ids), + ) + .limit(1) + ) + if remaining.scalar_one_or_none() is None: + raise AdminAccountSafetyError("You cannot remove the last active administrator.") diff --git a/modules/users/users/admin/service.py b/modules/users/users/admin/service.py index 484050c41..10c55d224 100644 --- a/modules/users/users/admin/service.py +++ b/modules/users/users/admin/service.py @@ -10,7 +10,8 @@ from sqlalchemy.orm import noload from sqlmodel import col -from users.admin.queries import _UserServiceBase +from users.admin.safety import _UserAdminSafety, check_role_assignment +from users.constants import ADMIN_ROLE_NAME from users.contracts.schemas import UserCreate from users.exceptions import ( EmailAlreadyExistsError, @@ -31,7 +32,7 @@ def _display_name(actor: User | None) -> str | None: return getattr(actor, "name", None) or None -class UserService(_UserServiceBase): +class UserService(_UserAdminSafety): async def create_user( self, email: str, @@ -47,6 +48,7 @@ async def create_user( check. ``is_verified=True`` means ``on_after_register`` does not fire a verification email (and with no request, no event is published here — the endpoint publishes ``UserCreated``).""" + check_role_assignment(self._actor, role_names) user_create = UserCreate( email=email, password=password, @@ -87,7 +89,7 @@ async def update_details( """Update a user's email + full name. Raises UserNotFoundError if the user is missing, EmailAlreadyExistsError if the new email is taken by another user.""" - user = await self._require_user(user_id) + user = await self._require_manageable_user(user_id) if email.lower() != user.email.lower(): clash = await self._db.execute( select(User.id).where( @@ -121,6 +123,9 @@ async def delete_user(self, user_id: uuid.UUID) -> None: mid-flush. ``session.delete(user)`` is what flags the session as written so ``get_db`` commits — a bulk ``DELETE`` of the user alone would be rolled back as a read-only request.""" + user = await self._require_manageable_user(user_id) + await self._guard_admin_removal(user) + self._db.expunge(user) result = await self._db.execute( select(User) .where(col(User.id) == user_id) @@ -149,6 +154,7 @@ async def invite( signed themselves up: the two rows are otherwise identical, and only this one has an outstanding email worth resending. """ + check_role_assignment(self._actor, role_names) password = secrets.token_urlsafe(32) user_create = UserCreate( email=email, @@ -196,7 +202,7 @@ async def resend_invite( self-signup would claim someone invited them, and re-enabling a disabled account to let its invite through would undo the disabling. """ - user = await self._require_user(user_id) + user = await self._require_manageable_user(user_id) if user.is_verified: raise NotPendingInviteError(user_id, "the invitation was already accepted") if not user.is_active: @@ -208,7 +214,8 @@ async def resend_invite( return user, self._manager.mint_invite_token(user, _display_name(invited_by)) async def disable(self, user_id: uuid.UUID) -> User: - user = await self._require_user(user_id) + user = await self._require_manageable_user(user_id) + await self._guard_admin_removal(user) user.disabled_at = datetime.now(UTC) user.is_active = False await self._revoke_sessions(user) @@ -224,7 +231,7 @@ async def disable(self, user_id: uuid.UUID) -> User: return user async def enable(self, user_id: uuid.UUID) -> User: - user = await self._require_user(user_id) + user = await self._require_manageable_user(user_id) user.disabled_at = None user.is_active = True await self._revoke_sessions(user) @@ -233,7 +240,7 @@ async def enable(self, user_id: uuid.UUID) -> User: async def unlock(self, user_id: uuid.UUID) -> User: """Lift a login lockout early. An ORM write, so the audit log records it.""" - user = await self._require_user(user_id) + user = await self._require_manageable_user(user_id) user.locked_until = None user.failed_login_count = 0 user.last_failed_login_at = None @@ -241,7 +248,7 @@ async def unlock(self, user_id: uuid.UUID) -> User: return user async def mark_verified(self, user_id: uuid.UUID) -> User: - user = await self._require_user(user_id) + user = await self._require_manageable_user(user_id) if not user.is_verified: user.is_verified = True await self._db.flush() @@ -254,7 +261,10 @@ async def set_roles( *, assigned_by: str | None = None, ) -> User: - user = await self._require_user(user_id) + user = await self._require_manageable_user(user_id) + check_role_assignment(self._actor, role_names) + if ADMIN_ROLE_NAME in {r.name for r in user.roles} and ADMIN_ROLE_NAME not in role_names: + await self._guard_admin_removal(user) # Delete all existing role assignments for this user await self._db.execute(delete(UserRole).where(col(UserRole.user_id) == user_id)) @@ -282,7 +292,7 @@ async def generate_reset_link(self, user_id: uuid.UUID, base_url: str) -> str: local password, so a reset link is meaningless (and would crash on the null-hash fingerprint). """ - user = await self._require_user(user_id) + user = await self._require_manageable_user(user_id) if user.is_external: raise ExternalUserNoPasswordError(user_id) diff --git a/modules/users/users/deps.py b/modules/users/users/deps.py index cec16fa49..e2e9a6e74 100644 --- a/modules/users/users/deps.py +++ b/modules/users/users/deps.py @@ -15,7 +15,7 @@ import uuid from typing import TYPE_CHECKING -from fastapi import Depends, Request +from fastapi import Depends, HTTPException, Request from fastapi_users import FastAPIUsers from simple_module_core.events import EventBus from simple_module_db.deps import get_db @@ -65,7 +65,10 @@ async def get_user_service( ) -> UserService: from users.admin.service import UserService - return UserService(db, user_manager, request.app) + actor = getattr(request.state, "user", None) + if actor is None: + raise HTTPException(status_code=401, detail="Not authenticated") + return UserService(db, user_manager, request.app, actor=actor) __all__ = [ diff --git a/modules/users/users/errors.py b/modules/users/users/errors.py index 8a00d595a..ee349472f 100644 --- a/modules/users/users/errors.py +++ b/modules/users/users/errors.py @@ -3,7 +3,15 @@ from fastapi import FastAPI, Request, status from fastapi.responses import JSONResponse -from users.exceptions import LoginLockedError +from users.exceptions import AdminAccountSafetyError, AdminPermissionError, LoginLockedError + + +async def _admin_permission(request: Request, exc: Exception) -> JSONResponse: + return JSONResponse({"detail": str(exc)}, status_code=status.HTTP_403_FORBIDDEN) + + +async def _admin_safety(request: Request, exc: Exception) -> JSONResponse: + return JSONResponse({"detail": str(exc)}, status_code=status.HTTP_400_BAD_REQUEST) async def _login_locked(request: Request, exc: Exception) -> JSONResponse: @@ -16,4 +24,6 @@ async def _login_locked(request: Request, exc: Exception) -> JSONResponse: def install_exception_handlers(app: FastAPI) -> None: + app.add_exception_handler(AdminPermissionError, _admin_permission) + app.add_exception_handler(AdminAccountSafetyError, _admin_safety) app.add_exception_handler(LoginLockedError, _login_locked) diff --git a/modules/users/users/exceptions.py b/modules/users/users/exceptions.py index ecdfa1d03..908ffcc3c 100644 --- a/modules/users/users/exceptions.py +++ b/modules/users/users/exceptions.py @@ -14,6 +14,14 @@ class UsersError(Exception): """Base for users business failures, independent of HTTP.""" +class AdminPermissionError(UsersError): + """Delegated management cannot grant or control a stronger account.""" + + +class AdminAccountSafetyError(UsersError): + """An operation would disable the actor or remove the last administrator.""" + + class UserNotFoundError(UsersError): """Raised when a user lookup by id/email returns nothing.""" diff --git a/modules/users/users/invites/bulk.py b/modules/users/users/invites/bulk.py index b3be12443..a273d70fa 100644 --- a/modules/users/users/invites/bulk.py +++ b/modules/users/users/invites/bulk.py @@ -17,6 +17,7 @@ from simple_module_db import independent_session from users.admin.links import admin_link_base +from users.admin.safety import check_role_assignment from users.admin.service import UserService from users.contracts.events import UserInvited from users.contracts.schemas import BulkInviteResponse, BulkInviteResult, UserBulkInvite @@ -87,6 +88,7 @@ async def admin_bulk_invite( ) -> BulkInviteResponse: """Invite every address in *data*, all sharing the same roles.""" invited_by = getattr(request.state, "user", None) + check_role_assignment(invited_by, data.role_names) invited_by_name = invited_by.name if invited_by else "Administrator" # Same question the page's ``mailer_delivers`` prop answers, asked once. @@ -125,7 +127,7 @@ async def admin_bulk_invite( mailer, request.app.state.users.settings, ) - service = UserService(checkpoint, manager, request.app) + service = UserService(checkpoint, manager, request.app, actor=invited_by) user, token = await service.invite( email, None, data.role_names, invited_by=invited_by ) From c9009d5d516534b3ed64ef6ad40894a77e8b7714 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 14:35:21 +0200 Subject: [PATCH 5/6] =?UTF-8?q?fix:=20address=20code=20review=20findings?= =?UTF-8?q?=20(round=201,=20pass=201)=20=E2=80=94=20#624=20compares=20effe?= =?UTF-8?q?ctive=20permissions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 36 ++++--- .../tests/demo/test_users_demo_credentials.py | 12 ++- .../test_users_admin_effective_permissions.py | 70 +++++++++++++ .../test_users_admin_privilege_guards.py | 24 +---- .../test_users_admin_role_grant_rules.py | 52 +++++++--- modules/users/users/admin/safety.py | 98 +++++++++++++++---- modules/users/users/admin/service.py | 8 +- modules/users/users/deps.py | 2 +- modules/users/users/invites/bulk.py | 8 +- 9 files changed, 229 insertions(+), 81 deletions(-) create mode 100644 modules/users/tests/test_users_admin_effective_permissions.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 781c2861e..db7f54d32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,20 +27,28 @@ All notable changes to this project are documented in this file. The format is b session (#615). ### Security -- Users: prevent delegated `users.manage` callers from assigning roles they do not hold - or controlling more privileged accounts, including via reset links and invitations. - Block self-disable, self-demotion and removal of the last active administrator (#624). -- Users: require an OIDC UserInfo `email_verified: true` claim before linking by - email or marking a new account verified. Other OAuth providers no longer link - by email or automatically verify new accounts; existing subject links still - sign in (#622). **Behaviour change:** providers without email verification - proof cannot sign in to an existing account by matching its email alone. -- Tenants: require a verified principal email before accepting invitations, including - local accounts allowed to sign in without verification and Keycloak accounts (#623). - Verification is read fresh from the user directory (new `DirectoryUser.email_verified`; - `None` when the provider doesn't track it, in which case Keycloak's `email_verified` - token claim decides), so an account verified after signing in can accept without - re-signing in. +- Users: account administration never exceeds the actor's own authority. Outside a full + admin session, a role can be granted and an account managed (edit, disable, reset link, + invite) only when its effective permissions (roles plus direct grants) are within the + actor's: a delegate holding `users.manage`, or an admin's scoped token, cannot grant + `admin`, cannot reach admin/superuser accounts, and cannot grant a role mapped to + permissions it lacks. Nobody can disable or demote themselves or the last active + administrator (#624). +- Users: OAuth sign-in links to an existing account by email, or marks a new account + verified, only with the provider's proof that the email is verified: OIDC + `email_verified: true`, Google's People API `metadata.verified` on the primary email, + a verified GitHub address from `/user/emails`; Microsoft (Entra) email claims are never + proof. An unverified account (SSO-provisioned ones included, pending invites excepted) + never absorbs a later OAuth login, closing an account pre-hijacking path (#622). + **Behaviour change:** a provider without proof cannot sign in to an existing account by + matching its email alone (`400 OAUTH_USER_ALREADY_EXISTS`); existing subject links still + sign in. +- Tenants: accepting an invitation requires a verified email (#623). Verification is read + fresh from the user directory (new `DirectoryUser.email_verified`; `None` when the + provider doesn't track it, in which case Keycloak's `email_verified` token claim decides), + so an account verified after signing in can accept without signing in again. + **Behaviour change:** with `users.require_verification` off, self-registered accounts must + verify their email (or be marked verified by an admin) before accepting an invitation. - DB: sessions bound to a SQLAlchemy `Connection` now inherit its engine's tenant policy and audit configuration, preserving strict tenant isolation and audit callbacks (#630). - Sign-in now rotates the session: users' `on_after_login` (password, OAuth, accept-invite) diff --git a/modules/users/tests/demo/test_users_demo_credentials.py b/modules/users/tests/demo/test_users_demo_credentials.py index 09c6c3c6a..9e5ecd274 100644 --- a/modules/users/tests/demo/test_users_demo_credentials.py +++ b/modules/users/tests/demo/test_users_demo_credentials.py @@ -7,6 +7,7 @@ from simple_module_test import forge_session_cookie from users.auth_local.token_api import _create_token_pair from users.demo import ensure_demo_users +from users.models import User @pytest.fixture @@ -37,11 +38,18 @@ async def test_demo_token_writes_follow_read_only_setting( demo_app.state.users.settings.demo_read_only = read_only headers = {"Authorization": f"Bearer {demo_credentials[credential]}"} # A successful read proves this credential actually resolves the demo identity. - target = demo_app.state.users.demo_user_ids[1] assert (await demo_client.get("/api/users/admin", headers=headers)).status_code == 200 + # A role-less account: the PAT's scopes (users.manage, api_tokens.use) cover it, so + # only the demo read-only gate decides. A target holding permissions beyond the + # token's scopes is refused regardless (#624). + async with demo_app.state.sm.db.session_factory() as db: + plain = User(email="plain@example.com", hashed_password=None, is_external=True) + db.add(plain) + await db.commit() + target = str(plain.id) response = await demo_client.patch( f"/api/users/admin/{target}", - json={"email": demo_app.state.users.settings.demo_user_email, "full_name": "Changed"}, + json={"email": "plain@example.com", "full_name": "Changed"}, headers=headers, ) assert response.status_code == (403 if read_only else 200), response.text diff --git a/modules/users/tests/test_users_admin_effective_permissions.py b/modules/users/tests/test_users_admin_effective_permissions.py new file mode 100644 index 000000000..f7766362c --- /dev/null +++ b/modules/users/tests/test_users_admin_effective_permissions.py @@ -0,0 +1,70 @@ +"""Delegated management compares effective permissions (#624): direct grants and token scopes.""" + +import httpx +import pytest +from api_tokens.constants import TokenKind +from api_tokens.service import ApiTokenService +from simple_module_test import forge_session_cookie +from sqlalchemy import select +from test_api_admin import _make_user +from users.models import Role, User + +pytestmark = pytest.mark.anyio + + +async def _client(app, headers=None, cookies=None): + return httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://testserver", + headers=headers, + cookies=cookies, + ) + + +async def test_delegate_cannot_manage_user_with_unheld_direct_grant(users_app, users_db): + users_db.add(Role(name="manager")) + await users_db.commit() + users_app.state.sm.permissions.map_role("manager", ["users.manage"]) + actor = await _make_user(users_db, "manager@example.com", role_names=["manager", "user"]) + target = await _make_user(users_db, "granted@example.com", role_names=["user"]) + ordinary = await _make_user(users_db, "plain@example.com", role_names=["user"]) + + async def direct_grants(request, user): + return {"files.delete"} if user.id == str(target.id) else set() + + users_app.state.sm.permissions.add_grant_source(direct_grants) + cookie = forge_session_cookie( + str(users_app.state.sm.settings.secret_key), {"user_id": str(actor.id)} + ) + async with await _client(users_app, cookies={"session": cookie}) as client: + denied = await client.patch(f"/api/users/admin/{target.id}/disable") + allowed = await client.patch(f"/api/users/admin/{ordinary.id}/disable") + assert denied.status_code == 403 + assert allowed.status_code == 200 + await users_db.refresh(target) + assert target.is_active is True + + +async def test_admin_pat_cannot_grant_role_beyond_its_scopes(users_app, users_db): + users_db.add(Role(name="deleter")) + await users_db.commit() + users_app.state.sm.permissions.map_role("deleter", ["files.delete"]) + admin = ( + await users_db.execute(select(User).where(User.email == "admin@example.com")) + ).scalar_one() + target = await _make_user(users_db, "target@example.com", role_names=[]) + _, pat = await ApiTokenService(users_db).create( + kind=TokenKind.PERSONAL, + name="scoped admin", + scopes=["users.manage", "api_tokens.use"], + expires_in_days=1, + max_lifetime_days=30, + owner_user_id=admin.id, + ) + await users_db.commit() + async with await _client(users_app, headers={"Authorization": f"Bearer {pat}"}) as client: + url = f"/api/users/admin/{target.id}/roles" + beyond = await client.put(url, json={"role_names": ["deleter"]}) + assert beyond.status_code == 403 + within = await client.put(url, json={"role_names": []}) + assert within.status_code == 200 diff --git a/modules/users/tests/test_users_admin_privilege_guards.py b/modules/users/tests/test_users_admin_privilege_guards.py index 8b80aca5d..ce51fedf0 100644 --- a/modules/users/tests/test_users_admin_privilege_guards.py +++ b/modules/users/tests/test_users_admin_privilege_guards.py @@ -2,13 +2,11 @@ import httpx import pytest -from auth.contracts.schemas import UserContext from fastapi import FastAPI from simple_module_test import forge_session_cookie from sqlalchemy import select from test_api_admin import _make_user from test_service_admin import _build_service -from users.admin.safety import check_role_assignment from users.errors import install_exception_handlers from users.exceptions import AdminAccountSafetyError, AdminPermissionError from users.models import Role, User, UserRole @@ -35,33 +33,13 @@ async def guarded(): assert response.json() == {"detail": "Refused"} -@pytest.mark.parametrize( - ("roles", "scopes", "wanted", "allowed"), - [ - (["admin"], None, ["admin", "user"], True), - (["manager", "user"], None, ["user"], True), - (["manager"], None, ["admin"], False), - (["manager"], None, ["powerful"], False), - (["admin"], ["users.manage"], ["admin"], False), - ], -) -def test_role_assignment_ceiling(roles, scopes, wanted, allowed): - actor = UserContext( - id="actor", email="actor@example.com", name="Actor", roles=roles, scopes=scopes - ) - if allowed: - check_role_assignment(actor, wanted) - else: - with pytest.raises(AdminPermissionError): - check_role_assignment(actor, wanted) - - @pytest.fixture async def delegated_client(users_app, users_db): users_db.add(Role(name="manager")) users_db.add(Role(name="powerful")) await users_db.commit() users_app.state.sm.permissions.map_role("manager", ["users.manage"]) + users_app.state.sm.permissions.map_role("powerful", ["files.delete"]) actor = await _make_user(users_db, "manager@example.com", role_names=["manager", "user"]) cookie = forge_session_cookie( str(users_app.state.sm.settings.secret_key), {"user_id": str(actor.id)} diff --git a/modules/users/tests/test_users_admin_role_grant_rules.py b/modules/users/tests/test_users_admin_role_grant_rules.py index 2e18fb601..750e0caee 100644 --- a/modules/users/tests/test_users_admin_role_grant_rules.py +++ b/modules/users/tests/test_users_admin_role_grant_rules.py @@ -1,34 +1,56 @@ -"""Who may grant which roles (#624): full admin, an admin's scoped token, delegates.""" +"""Who may grant which roles (#624): compared by effective permissions, not role names.""" import pytest from auth.contracts.schemas import UserContext +from simple_module_hosting.permissions import apply_scope_cap, resolve_permissions from users.admin.safety import check_role_assignment from users.exceptions import AdminPermissionError +ROLE_MAP = { + "admin": ["*"], + "helpdesk": ["users.manage"], + "editor": ["pages.edit"], + "billing": ["billing.manage"], + "files": ["files.delete"], + "empty": [], +} -def _actor(roles, scopes=None): - return UserContext(id="a", email="a@x", name="a", roles=roles, scopes=scopes) + +def _check(roles, wanted, scopes=None): + actor = UserContext(id="a", email="a@x", name="a", roles=roles, scopes=scopes) + held = apply_scope_cap(resolve_permissions(roles, ROLE_MAP), actor) + check_role_assignment(actor, wanted, held, ROLE_MAP) def test_full_admin_grants_admin(): - check_role_assignment(_actor(["admin"]), ["admin", "editor"]) + _check(["admin"], ["admin", "editor"]) -def test_admin_scoped_token_grants_ordinary_roles_but_not_admin(): - token = _actor(["admin"], scopes=["users.manage"]) - check_role_assignment(token, ["editor"]) +def test_admin_scoped_token_cannot_grant_admin(): with pytest.raises(AdminPermissionError): - check_role_assignment(token, ["admin"]) + _check(["admin"], ["admin"], scopes=["users.manage"]) -def test_delegate_grants_only_roles_it_holds(): - helpdesk = _actor(["helpdesk", "editor"]) - check_role_assignment(helpdesk, ["editor"]) - with pytest.raises(AdminPermissionError): - check_role_assignment(helpdesk, ["admin"]) +def test_admin_scoped_token_grants_only_roles_within_its_scopes(): + _check(["admin"], ["helpdesk", "empty"], scopes=["users.manage"]) with pytest.raises(AdminPermissionError): - check_role_assignment(helpdesk, ["billing"]) + _check(["admin"], ["files"], scopes=["users.manage"]) + + +def test_delegate_grants_roles_its_permissions_cover(): + _check(["helpdesk", "editor"], ["editor", "empty"]) + for wanted in (["admin"], ["billing"], ["files"]): + with pytest.raises(AdminPermissionError): + _check(["helpdesk", "editor"], wanted) + + +def test_delegate_holding_equivalent_permissions_via_another_role_may_grant(): + ROLE_MAP["twin"] = ["pages.edit"] + try: + _check(["editor"], ["twin"]) + finally: + del ROLE_MAP["twin"] def test_trusted_in_process_caller_is_unrestricted(): - check_role_assignment(None, ["admin"]) + check_role_assignment(None, ["admin"], set(), ROLE_MAP) diff --git a/modules/users/users/admin/safety.py b/modules/users/users/admin/safety.py index 8fa9860f0..e412152db 100644 --- a/modules/users/users/admin/safety.py +++ b/modules/users/users/admin/safety.py @@ -3,7 +3,15 @@ import uuid from auth.contracts.schemas import UserContext -from fastapi import FastAPI +from fastapi import FastAPI, Request +from simple_module_core.app_state import get_services +from simple_module_core.permissions import DEFAULT_ROLE_PERMISSIONS, grants +from simple_module_hosting.permissions import ( + apply_scope_cap, + ensure_resolved_permissions, + resolve_permissions, + resolve_principal_permissions, +) from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession from sqlmodel import col @@ -19,24 +27,46 @@ def _is_admin(actor: UserContext) -> bool: return ADMIN_ROLE_NAME in actor.roles -def check_role_assignment(actor: UserContext | None, role_names: list[str]) -> None: - """Refuse a grant wider than the actor's own authority. +def _is_full_admin(actor: UserContext | None) -> bool: + """``None`` is a trusted in-process caller; an unscoped admin holds everything.""" + return actor is None or (_is_admin(actor) and not actor.is_scoped) - ``None`` is reserved for trusted in-process callers; HTTP always supplies an - actor. A full admin session grants anything; an admin's scoped token (PAT, - bearer) grants anything except ``admin`` itself; anyone else only roles they - hold. + +def _covers(held: set[str], wanted: set[str]) -> bool: + return all(grants(held, permission) for permission in wanted) + + +def check_role_assignment( + actor: UserContext | None, + role_names: list[str], + actor_permissions: set[str], + role_map: dict[str, list[str]], +) -> None: + """Refuse a grant wider than the actor's effective permissions. + + ``actor_permissions`` is what the request's authorization resolved for the + actor (roles, direct grants, token-scope cap). Every requested role's + permissions must be covered; ``admin`` maps to the wildcard, so only a + principal holding the wildcard itself can grant it. A full admin session + (and ``None``, a trusted in-process caller) is unrestricted. """ - if actor is None or (_is_admin(actor) and not actor.is_scoped): - return - if _is_admin(actor): - if ADMIN_ROLE_NAME in role_names: - raise AdminPermissionError("You cannot assign roles you do not hold.") + if _is_full_admin(actor): return - if not set(role_names).issubset(actor.roles): + wanted = resolve_permissions(role_names, role_map) + if ADMIN_ROLE_NAME in role_names or not _covers(actor_permissions, wanted): raise AdminPermissionError("You cannot assign roles you do not hold.") +async def check_request_role_assignment(request: Request, role_names: list[str]) -> None: + """:func:`check_role_assignment` for the request's own principal.""" + actor = getattr(request.state, "user", None) + if _is_full_admin(actor): + return + registry = getattr(get_services(request.app), "permissions", None) + role_map = registry.role_map if registry is not None else DEFAULT_ROLE_PERMISSIONS + check_role_assignment(actor, role_names, await ensure_resolved_permissions(request), role_map) + + class _UserAdminSafety(_UserServiceBase): def __init__( self, @@ -45,20 +75,50 @@ def __init__( app: FastAPI | None = None, *, actor: UserContext | None = None, + request: Request | None = None, ) -> None: super().__init__(db, user_manager, app) self._actor = actor + self._request = request + + def _role_map(self) -> dict[str, list[str]]: + sm = get_services(self._app) if self._app is not None else None + registry = getattr(sm, "permissions", None) + return registry.role_map if registry is not None else DEFAULT_ROLE_PERMISSIONS + + async def _actor_permissions(self) -> set[str]: + actor = self._actor + if actor is None: + return set() + if self._request is not None: + return await ensure_resolved_permissions(self._request) + return apply_scope_cap(resolve_permissions(actor.roles, self._role_map()), actor) + + async def _target_permissions(self, user: User) -> set[str]: + """Roles plus direct grants, resolved the way a request as *user* would.""" + principal = UserContext.from_user(user) + if self._request is not None: + sm = get_services(self._request.app) + registry = getattr(sm, "permissions", None) + return await resolve_principal_permissions(self._request, principal, registry) + return resolve_permissions(principal.roles, self._role_map()) + + async def _check_roles(self, role_names: list[str]) -> None: + if _is_full_admin(self._actor): + return + check_role_assignment( + self._actor, role_names, await self._actor_permissions(), self._role_map() + ) async def _require_manageable_user(self, user_id: uuid.UUID) -> User: user = await self._require_user(user_id) - actor = self._actor - if actor is None or (_is_admin(actor) and not actor.is_scoped): + if _is_full_admin(self._actor): return user roles = {role.name for role in user.roles} - privileged = user.is_superuser or ADMIN_ROLE_NAME in roles - # An admin's scoped token manages ordinary accounts, never another admin. - stronger = not _is_admin(actor) and not roles.issubset(actor.roles) - if privileged or stronger: + if user.is_superuser or ADMIN_ROLE_NAME in roles: + raise AdminPermissionError("You cannot manage a more privileged account.") + held = await self._actor_permissions() + if not _covers(held, await self._target_permissions(user)): raise AdminPermissionError("You cannot manage a more privileged account.") return user diff --git a/modules/users/users/admin/service.py b/modules/users/users/admin/service.py index 10c55d224..0d5ecbb6b 100644 --- a/modules/users/users/admin/service.py +++ b/modules/users/users/admin/service.py @@ -10,7 +10,7 @@ from sqlalchemy.orm import noload from sqlmodel import col -from users.admin.safety import _UserAdminSafety, check_role_assignment +from users.admin.safety import _UserAdminSafety from users.constants import ADMIN_ROLE_NAME from users.contracts.schemas import UserCreate from users.exceptions import ( @@ -48,7 +48,7 @@ async def create_user( check. ``is_verified=True`` means ``on_after_register`` does not fire a verification email (and with no request, no event is published here — the endpoint publishes ``UserCreated``).""" - check_role_assignment(self._actor, role_names) + await self._check_roles(role_names) user_create = UserCreate( email=email, password=password, @@ -154,7 +154,7 @@ async def invite( signed themselves up: the two rows are otherwise identical, and only this one has an outstanding email worth resending. """ - check_role_assignment(self._actor, role_names) + await self._check_roles(role_names) password = secrets.token_urlsafe(32) user_create = UserCreate( email=email, @@ -262,7 +262,7 @@ async def set_roles( assigned_by: str | None = None, ) -> User: user = await self._require_manageable_user(user_id) - check_role_assignment(self._actor, role_names) + await self._check_roles(role_names) if ADMIN_ROLE_NAME in {r.name for r in user.roles} and ADMIN_ROLE_NAME not in role_names: await self._guard_admin_removal(user) diff --git a/modules/users/users/deps.py b/modules/users/users/deps.py index e2e9a6e74..9e8147c31 100644 --- a/modules/users/users/deps.py +++ b/modules/users/users/deps.py @@ -68,7 +68,7 @@ async def get_user_service( actor = getattr(request.state, "user", None) if actor is None: raise HTTPException(status_code=401, detail="Not authenticated") - return UserService(db, user_manager, request.app, actor=actor) + return UserService(db, user_manager, request.app, actor=actor, request=request) __all__ = [ diff --git a/modules/users/users/invites/bulk.py b/modules/users/users/invites/bulk.py index a273d70fa..5a2bd9df7 100644 --- a/modules/users/users/invites/bulk.py +++ b/modules/users/users/invites/bulk.py @@ -17,7 +17,7 @@ from simple_module_db import independent_session from users.admin.links import admin_link_base -from users.admin.safety import check_role_assignment +from users.admin.safety import check_request_role_assignment from users.admin.service import UserService from users.contracts.events import UserInvited from users.contracts.schemas import BulkInviteResponse, BulkInviteResult, UserBulkInvite @@ -88,7 +88,7 @@ async def admin_bulk_invite( ) -> BulkInviteResponse: """Invite every address in *data*, all sharing the same roles.""" invited_by = getattr(request.state, "user", None) - check_role_assignment(invited_by, data.role_names) + await check_request_role_assignment(request, data.role_names) invited_by_name = invited_by.name if invited_by else "Administrator" # Same question the page's ``mailer_delivers`` prop answers, asked once. @@ -127,7 +127,9 @@ async def admin_bulk_invite( mailer, request.app.state.users.settings, ) - service = UserService(checkpoint, manager, request.app, actor=invited_by) + service = UserService( + checkpoint, manager, request.app, actor=invited_by, request=request + ) user, token = await service.invite( email, None, data.role_names, invited_by=invited_by ) From 7d474fe099587c24cc0f3f85ef8f124ff7381e72 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 14:46:45 +0200 Subject: [PATCH 6/6] tenants: distinct invitation_email_unverified error for an unverified address (#623) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/tenants/tenants/hooks/useTenantErrors.ts | 1 + modules/tenants/tenants/invitations.py | 8 ++++++-- modules/tenants/tenants/locales/de.json | 1 + modules/tenants/tenants/locales/en.json | 1 + .../tenants/tests/test_tenants_invitation_verification.py | 1 + packages/i18n/src/generated-resources.ts | 1 + packages/i18n/src/keys.generated.ts | 1 + 7 files changed, 12 insertions(+), 2 deletions(-) diff --git a/modules/tenants/tenants/hooks/useTenantErrors.ts b/modules/tenants/tenants/hooks/useTenantErrors.ts index 4bff6e109..669598939 100644 --- a/modules/tenants/tenants/hooks/useTenantErrors.ts +++ b/modules/tenants/tenants/hooks/useTenantErrors.ts @@ -27,6 +27,7 @@ export function useTenantErrors() { invitation_expired: t(keys.tenants.errors.invitation_expired), invitation_used: t(keys.tenants.errors.invitation_used), invitation_email_mismatch: t(keys.tenants.errors.invitation_email_mismatch), + invitation_email_unverified: t(keys.tenants.errors.invitation_email_unverified), }; /** Translated copy for a failed call: its `{detail}` code (and `limit` for diff --git a/modules/tenants/tenants/invitations.py b/modules/tenants/tenants/invitations.py index d68e2c460..faaeb818e 100644 --- a/modules/tenants/tenants/invitations.py +++ b/modules/tenants/tenants/invitations.py @@ -155,9 +155,13 @@ async def accept( raise TenantError("tenant_suspended", status_code=409) if self.is_expired(invitation): raise TenantError("invitation_expired", status_code=410) - # A leaked link cannot prove ownership of a self-asserted address. - if email_verified is not True or user_email.strip().lower() != invitation.email: + # The link is a bearer secret; binding it to the invited address means + # a forwarded or leaked link cannot enrol a different account. + if user_email.strip().lower() != invitation.email: raise TenantError("invitation_email_mismatch", status_code=403) + # ...and a self-asserted, unverified address proves nothing (#623). + if email_verified is not True: + raise TenantError("invitation_email_unverified", status_code=403) invitation.accepted_at = _now() await self.tenants.add_member( tenant.id, diff --git a/modules/tenants/tenants/locales/de.json b/modules/tenants/tenants/locales/de.json index c669481b4..d85e5a1a8 100644 --- a/modules/tenants/tenants/locales/de.json +++ b/modules/tenants/tenants/locales/de.json @@ -137,6 +137,7 @@ "invitation_expired": "Diese Einladung ist abgelaufen.", "invitation_used": "Diese Einladung wurde bereits verwendet.", "invitation_email_mismatch": "Diese Einladung muss mit der eingeladenen E-Mail-Adresse angenommen werden.", + "invitation_email_unverified": "Bestätigen Sie Ihre E-Mail-Adresse, bevor Sie diese Einladung annehmen.", "generic": "Etwas ist schiefgelaufen. Bitte versuchen Sie es erneut.", "already_member": "Diese Person ist bereits Mitglied dieser Organisation.", "tenant_manager_required": "Nur ein Inhaber oder Administrator dieser Organisation kann dies tun.", diff --git a/modules/tenants/tenants/locales/en.json b/modules/tenants/tenants/locales/en.json index b17030f77..6e167e99b 100644 --- a/modules/tenants/tenants/locales/en.json +++ b/modules/tenants/tenants/locales/en.json @@ -137,6 +137,7 @@ "invitation_expired": "That invitation has expired.", "invitation_used": "That invitation has already been used.", "invitation_email_mismatch": "This invitation must be accepted by the invited email address.", + "invitation_email_unverified": "Verify your email address before accepting this invitation.", "generic": "Something went wrong. Please try again.", "already_member": "That person is already a member of this organisation.", "tenant_manager_required": "Only an owner or admin of this organisation can do that.", diff --git a/modules/tenants/tests/test_tenants_invitation_verification.py b/modules/tenants/tests/test_tenants_invitation_verification.py index df74613a6..8c1ee175e 100644 --- a/modules/tenants/tests/test_tenants_invitation_verification.py +++ b/modules/tenants/tests/test_tenants_invitation_verification.py @@ -28,6 +28,7 @@ async def test_unverified_account_cannot_accept_invitation(app, user_client): rejected = await bob.post("/api/tenants/invitations/accept", json={"token": token}) assert rejected.status_code == 403 + assert rejected.json()["detail"] == "invitation_email_unverified" members = (await owner.get("/api/tenants/current/members")).json() assert bob_id not in {member["user_id"] for member in members} pending = (await owner.get("/api/tenants/current/invitations")).json() diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 9d66d2e37..7383d4b81 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -912,6 +912,7 @@ export default { 'tenants.errors.already_member': '', 'tenants.errors.generic': '', 'tenants.errors.invitation_email_mismatch': '', + 'tenants.errors.invitation_email_unverified': '', 'tenants.errors.invitation_expired': '', 'tenants.errors.invitation_not_found': '', 'tenants.errors.invitation_used': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index 83610d294..b20ddb20c 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -1177,6 +1177,7 @@ export const keys = { already_member: 'tenants.errors.already_member', generic: 'tenants.errors.generic', invitation_email_mismatch: 'tenants.errors.invitation_email_mismatch', + invitation_email_unverified: 'tenants.errors.invitation_email_unverified', invitation_expired: 'tenants.errors.invitation_expired', invitation_not_found: 'tenants.errors.invitation_not_found', invitation_used: 'tenants.errors.invitation_used',