diff --git a/docs/superpowers/specs/2026-06-23-external-sso-users-design.md b/docs/superpowers/specs/2026-06-23-external-sso-users-design.md new file mode 100644 index 00000000..68a45c5d --- /dev/null +++ b/docs/superpowers/specs/2026-06-23-external-sso-users-design.md @@ -0,0 +1,139 @@ +# External (SSO) users in the `users` module + +**Date:** 2026-06-23 +**Status:** Approved (design) — pending implementation plan +**Branch:** `feat/users-external-sso` (independent of PR #192) + +## Problem + +When someone signs in through an external IdP (Microsoft/Entra, Google, GitHub, +generic OIDC), the `users` module already provisions a real local `users_user` +row just-in-time (via fastapi-users `oauth_callback`, see +`modules/users/users/oauth/api.py`), links it to a `users_oauth_account`, and +lets admins assign roles to it through the normal admin path +(`PUT /api/users/admin/{id}/roles`). Two things are missing for the desired UX: + +1. OAuth-provisioned users get a **random unknown password hash**, not a real + "no password" — and `users_user.hashed_password` is **not nullable**. +2. Nothing **marks them as external**, so the admin UI treats them like local + password accounts (e.g. offers a password reset that makes no sense). + +The standalone `oidc` module (PR #192) is a *different* model — stateless bearer +-token validation with its own cache table and no local users — and is **out of +scope here**; it stays as-is for that use case. + +## Goal + +An external user: +- is **created locally** on first SSO login (existing behaviour), +- has a **genuinely null password** (cannot log in with a password, cannot be + sent a password reset), +- is **clearly marked external** in the admin UI, and +- can be **assigned roles exactly like a normal user** (existing behaviour). + +## Non-goals (YAGNI) + +- Converting external ↔ local (e.g. an admin setting an initial password to + "graduate" an SSO user to a password account). +- Per-provider external policy (all IdP-provisioned users are uniformly + external). +- Any change to the standalone `oidc` module / PR #192. +- A default role on creation — external users start with **no roles**; admins + assign them. + +## Design + +### A. Data model + migration + +`modules/users/users/models/user.py`: +- `hashed_password: str` → **`hashed_password: str | None`** (nullable). +- Add **`is_external: bool`** (NOT NULL, `server_default` false, default False). + +One autogenerated Alembic migration under `host/migrations/versions/`: +- make `users_user.hashed_password` nullable, +- add `users_user.is_external` with `server_default="false"` so existing rows + backfill to non-external; passwords are untouched. + +(SQLite note: the project runs both Postgres and SQLite; Alembic batch +operations already used elsewhere handle SQLite column alters.) + +### B. Provisioning — OAuth creates an external user + +The OAuth callback path (`modules/users/users/oauth/api.py` → +`UserManager.oauth_callback`) must, **when it creates a new user**, set +`hashed_password = None` and `is_external = True`. + +- Implementation hook: override `oauth_callback` in + `modules/users/users/manager.py` (`UserManager`) — call `super().oauth_callback(...)` + but provision new users with a null password + `is_external=True`. The exact + mechanism (override `oauth_callback` vs. detect-new-then-patch in the + `oauth/api.py` wrapper) is finalised in the implementation plan; the contract + is: **only newly-created OAuth users become external.** +- **Existing-email linking is unchanged** (`associate_by_email=True`): when an + IdP login matches an existing *password* user by (verified) email, the OAuth + account is linked to that row; the user keeps their password and stays + **non-external**. + +### C. Credential guards (null-password safety net) + +A null `hashed_password` must never reach a hashing/verification call. Guard all +password-credential paths so external users are rejected with a clear message +(e.g. *"This account signs in with SSO."*): + +- **Password login** (`modules/users/users/auth_local/api.py`, fastapi-users + `authenticate`): override `UserManager.authenticate` so a user with + `hashed_password is None` fails as invalid credentials, preserving the + timing-safe dummy-hash behaviour. +- **Forgot / reset password** (public fastapi-users routers wired in + `modules/users/users/module.py:146`) and **admin reset-link** + (`modules/users/users/admin/api.py:235` → `UserManager.generate_reset_password_token`, + `manager.py:133`, which calls `password_helper.hash(user.hashed_password)` and + would crash on `None`): reject external users **before** the hash call. +- Any self-service set/change-password route: same rejection. (Exact route list + enumerated in the implementation plan.) + +### D. Admin UI marking + +- Add `is_external` to `UserListItem` (`modules/users/users/contracts/schemas.py:71`) + and the user-detail schema, and to the admin queries that build them + (`modules/users/users/admin/queries.py` / `admin/service.py`). +- `/users/admin` list + detail (`modules/users/users/pages/Users/Index.tsx`, + `pages/Users/components/DetailsCard.tsx`, `components/DangerZone.tsx`): show an + **"External · SSO"** badge and **hide/disable** the "set password" / "send + reset link" actions for external users. + +### E. Roles — no change + +Role assignment is the existing `PUT /api/users/admin/{id}/roles` +(`admin/api.py:198`). External users start with no roles; admins assign exactly +as for password users. + +### F. Auth provider / login — no change + +`UsersAuthProvider.resolve_user` (`modules/users/users/provider.py`) resolves the +principal from `session["user_id"]` → DB user (roles eager-loaded), honouring +`is_active` / `disabled_at`. External users resolve identically. + +## Testing + +- OAuth-created user → `is_external is True`, `hashed_password is None`. +- Password login for an external user → rejected (clear message), no crash. +- Forgot-password and admin reset-link for an external user → rejected, no + `hash(None)` crash. +- Admin **can assign roles** to an external user via the normal endpoint. +- Existing **password user** logging in via OAuth → account linked, password + retained, `is_external` stays False. +- Admin list/detail payloads include `is_external`; UI shows the badge and hides + password actions (component test). +- Migration: existing rows backfill `is_external=False`; `hashed_password` + becomes nullable without data loss. + +## Risks / edge cases + +- **fastapi-users assumes a non-null `hashed_password`** in `authenticate` and + reset-token generation — both are explicitly guarded above; tests cover the + null path. +- **Email-based linking trust**: `associate_by_email=True` links by IdP email. + Acceptable for Entra (verified emails); a provider that emits unverified + emails could allow linking to an existing account. Out of scope to change + here, but noted. diff --git a/framework/core/tests/test_module_base.py b/framework/core/tests/test_module_base.py index a5f0b672..33164a13 100644 --- a/framework/core/tests/test_module_base.py +++ b/framework/core/tests/test_module_base.py @@ -35,7 +35,7 @@ async def test_custom_fields(self): async def test_frozen(self): meta = ModuleMeta(name="Frozen") with pytest.raises(AttributeError): - meta.name = "Changed" # type: ignore[misc] # ty: ignore[invalid-assignment] + meta.name = "Changed" # type: ignore[misc] class DummyModule(ModuleBase): diff --git a/framework/core/tests/test_services.py b/framework/core/tests/test_services.py index 86cd6228..b5fa8ffc 100644 --- a/framework/core/tests/test_services.py +++ b/framework/core/tests/test_services.py @@ -11,13 +11,13 @@ async def test_services_is_frozen(self) -> None: """Mutation after construction must raise — singletons don't change at runtime.""" s = _make_services() with pytest.raises((AttributeError, TypeError)): - s.settings = None # type: ignore[misc,assignment] # ty: ignore[invalid-assignment] + s.settings = None # type: ignore[misc,assignment] async def test_services_has_slots(self) -> None: """Slotted dataclass prevents silent attribute additions (the original bloat pattern).""" s = _make_services() with pytest.raises((AttributeError, TypeError)): - s.rogue_new_attribute = 42 # type: ignore[attr-defined] # ty: ignore[invalid-assignment] + s.rogue_new_attribute = 42 # type: ignore[attr-defined] async def test_services_round_trip_field_access(self) -> None: """Every declared field must be readable after construction.""" diff --git a/host/migrations/versions/92965b00f105_users_external_sso_nullable_password_is_.py b/host/migrations/versions/92965b00f105_users_external_sso_nullable_password_is_.py new file mode 100644 index 00000000..91cecff5 --- /dev/null +++ b/host/migrations/versions/92965b00f105_users_external_sso_nullable_password_is_.py @@ -0,0 +1,62 @@ +"""users external sso: nullable password + is_external + +Revision ID: 92965b00f105 +Revises: 873ca2015033 +Create Date: 2026-06-23 16:08:20.901942 +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "92965b00f105" +down_revision: str | None = "873ca2015033" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +# External (SSO) users have no local password, so ``hashed_password`` becomes +# nullable and a marker column ``is_external`` is added. SQLite cannot ALTER a +# column's nullability in place, so the change goes through ``batch_alter_table`` +# (a table rebuild on SQLite, a direct ALTER on Postgres). The rebuild can't +# reflect the expression-based ``lower(email)`` index, so it's dropped and +# recreated explicitly on SQLite. + + +def upgrade() -> None: + bind = op.get_bind() + with op.batch_alter_table("users_user") as batch_op: + batch_op.add_column( + sa.Column( + "is_external", + sa.Boolean(), + server_default=sa.false(), + nullable=False, + ) + ) + batch_op.alter_column( + "hashed_password", + existing_type=sa.String(length=1024), + nullable=True, + ) + if bind.dialect.name == "sqlite": + op.execute("DROP INDEX IF EXISTS ix_users_user_email_lower") + op.create_index("ix_users_user_email_lower", "users_user", [sa.text("lower(email)")]) + + +def downgrade() -> None: + # Restoring ``hashed_password NOT NULL`` will fail if any external (SSO) + # user exists at downgrade time — they carry a NULL password by design. + # Such rows must be deleted or given a password before downgrading. + bind = op.get_bind() + with op.batch_alter_table("users_user") as batch_op: + batch_op.alter_column( + "hashed_password", + existing_type=sa.String(length=1024), + nullable=False, + ) + batch_op.drop_column("is_external") + if bind.dialect.name == "sqlite": + op.execute("DROP INDEX IF EXISTS ix_users_user_email_lower") + op.create_index("ix_users_user_email_lower", "users_user", [sa.text("lower(email)")]) diff --git a/modules/keycloak/keycloak/models.py b/modules/keycloak/keycloak/models.py index 0183b42e..e11efc63 100644 --- a/modules/keycloak/keycloak/models.py +++ b/modules/keycloak/keycloak/models.py @@ -11,7 +11,7 @@ Base = create_module_base("keycloak") -class KeycloakUserCache(Base, table=True): +class KeycloakUserCache(Base, table=True): # ty: ignore[unsupported-base] __tablename__ = "keycloak_user_cache" id: uuid_mod.UUID = Field(default_factory=uuid_mod.uuid4, primary_key=True) diff --git a/modules/users/tests/test_external_users.py b/modules/users/tests/test_external_users.py new file mode 100644 index 00000000..4fd16a37 --- /dev/null +++ b/modules/users/tests/test_external_users.py @@ -0,0 +1,228 @@ +"""External (SSO) user behaviour: null password, marking, and guards. + +Covers the feature added on top of the OAuth/OIDC login flow: +- a *new* OAuth login provisions an external user (null password, marked), +- an OAuth login that *links* to an existing password account leaves it local, +- external users cannot password-login or be sent a reset link, +- admins can still assign roles to external users like any other user, +- the admin list surfaces ``is_external``. +""" + +from __future__ import annotations + +import uuid +from urllib.parse import parse_qs, urlparse + +import pytest +from fastapi_users.password import PasswordHelper +from sqlalchemy import select +from users.models import Role, User + +_pw = PasswordHelper() + + +# --------------------------------------------------------------------------- +# Fake OAuth provider (no network) to drive the /login + /callback flow +# --------------------------------------------------------------------------- + + +class _FakeOAuthClient: + def __init__(self, account_id: str, account_email: str) -> None: + self._account_id = account_id + self._account_email = account_email + + async def get_authorization_url(self, redirect_uri: str, state: str) -> str: + return f"https://idp.example/authorize?state={state}" + + async def get_access_token(self, code: str, redirect_uri: str) -> dict: + return {"access_token": "fake-token", "expires_at": None, "refresh_token": None} + + async def get_id_email(self, access_token: str) -> tuple[str, str]: + return self._account_id, self._account_email + + +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) + ) + + +async def _run_oauth_login(client) -> int: + """Drive /login then /callback; return the callback status code.""" + login = await client.get("/api/users/auth/microsoft/login", follow_redirects=False) + assert login.status_code == 302 + state = parse_qs(urlparse(login.headers["location"]).query)["state"][0] + cb = await client.get( + f"/api/users/auth/microsoft/callback?code=abc&state={state}", + follow_redirects=False, + ) + return cb.status_code + + +async def _get_user_by_email(db, email: str) -> User: + return (await db.execute(select(User).where(User.email == email))).scalar_one() + + +# --------------------------------------------------------------------------- +# Provisioning +# --------------------------------------------------------------------------- + + +@pytest.mark.anyio +async def test_new_oauth_login_creates_external_user(users_app, anon_client, users_db): + _install_fake_provider(users_app, "ms-oid-1", "sso-new@example.com") + + assert await _run_oauth_login(anon_client) == 303 + + user = await _get_user_by_email(users_db, "sso-new@example.com") + assert user.is_external is True + assert user.hashed_password is None + assert user.is_verified is True + + +@pytest.mark.anyio +async def test_oauth_login_links_existing_password_user_stays_local( + users_app, anon_client, users_db +): + existing = User( + id=uuid.uuid4(), + email="local@example.com", + hashed_password=_pw.hash("SecurePass1!"), + is_active=True, + is_verified=True, + ) + users_db.add(existing) + await users_db.commit() + + _install_fake_provider(users_app, "ms-oid-2", "local@example.com") + assert await _run_oauth_login(anon_client) == 303 + + user = await _get_user_by_email(users_db, "local@example.com") + assert user.is_external is False + assert user.hashed_password is not None # password retained + + +# --------------------------------------------------------------------------- +# Credential guards +# --------------------------------------------------------------------------- + + +async def _seed_external_user(db, email: str = "ext@example.com") -> User: + user = User( + id=uuid.uuid4(), + email=email, + hashed_password=None, + is_active=True, + is_verified=True, + is_external=True, + ) + db.add(user) + await db.commit() + return user + + +@pytest.mark.anyio +async def test_external_user_cannot_password_login(users_app, anon_client, users_db): + await _seed_external_user(users_db, "ext-login@example.com") + + resp = await anon_client.post( + "/api/users/auth/login", + data={"username": "ext-login@example.com", "password": "anything-at-all"}, + ) + assert resp.status_code == 400 + assert resp.json()["detail"] == "LOGIN_BAD_CREDENTIALS" + + +@pytest.mark.anyio +async def test_external_user_cannot_bearer_token_login(users_app, anon_client, users_db): + """The bearer-token login path must reject external users cleanly (401), + not 500 on a None-hash verify, and without a timing/error enumeration leak.""" + await _seed_external_user(users_db, "ext-token@example.com") + + resp = await anon_client.post( + "/api/users/auth/token", + json={"email": "ext-token@example.com", "password": "anything-at-all"}, + ) + assert resp.status_code == 401 + assert resp.json()["detail"] == "Invalid credentials" + + +@pytest.mark.anyio +async def test_admin_reset_link_rejected_for_external_user(admin_client, users_db): + user = await _seed_external_user(users_db, "ext-reset@example.com") + + resp = await admin_client.post(f"/api/users/admin/{user.id}/reset-password-link") + assert resp.status_code == 409 + + +@pytest.mark.anyio +async def test_forgot_password_is_noop_for_external_user(users_app): + from users.db_adapter import UserDatabaseWithRoles + from users.manager import UserManager + from users.models import OAuthAccount + + async with users_app.state.sm.db.session_factory() as session: + user_db = UserDatabaseWithRoles(session, User, OAuthAccount) + manager = UserManager(user_db, users_app.state.users.mailer, users_app.state.users.settings) + + sent: list[str] = [] + + async def _spy(user, token, request=None): + sent.append(user.email) + + manager.on_after_forgot_password = _spy + + external = User( + id=uuid.uuid4(), + email="ext-forgot@example.com", + hashed_password=None, + is_active=True, + is_verified=True, + is_external=True, + ) + local = User( + id=uuid.uuid4(), + email="local-forgot@example.com", + hashed_password=_pw.hash("SecurePass1!"), + is_active=True, + is_verified=True, + ) + + await manager.forgot_password(external) + assert sent == [] # SSO account: no reset email + + await manager.forgot_password(local) + assert sent == ["local-forgot@example.com"] # control: local account still works + + +# --------------------------------------------------------------------------- +# Roles + admin visibility (external users are managed like normal users) +# --------------------------------------------------------------------------- + + +@pytest.mark.anyio +async def test_admin_can_assign_role_to_external_user(admin_client, users_db): + user = await _seed_external_user(users_db, "ext-role@example.com") + users_db.add(Role(id=uuid.uuid4(), name="member", description="Member")) + await users_db.commit() + + resp = await admin_client.put( + f"/api/users/admin/{user.id}/roles", + json={"role_names": ["member"]}, + ) + assert resp.status_code == 200 + assert "member" in resp.json()["roles"] + + +@pytest.mark.anyio +async def test_admin_list_exposes_is_external(admin_client, users_db): + await _seed_external_user(users_db, "ext-list@example.com") + + resp = await admin_client.get("/api/users/admin?per_page=100") + assert resp.status_code == 200 + rows = {r["email"]: r for r in resp.json()} + assert rows["ext-list@example.com"]["is_external"] is True + # The seeded admin is a local account. + assert rows["admin@example.com"]["is_external"] is False diff --git a/modules/users/users/admin/api.py b/modules/users/users/admin/api.py index 42daaa59..2a8e2b14 100644 --- a/modules/users/users/admin/api.py +++ b/modules/users/users/admin/api.py @@ -28,7 +28,11 @@ UserListItem, ) from users.deps import get_event_bus, get_mailer, get_user_service -from users.exceptions import EmailAlreadyExistsError, UserNotFoundError +from users.exceptions import ( + EmailAlreadyExistsError, + ExternalUserNoPasswordError, + UserNotFoundError, +) admin_router = APIRouter( prefix="/admin", @@ -243,4 +247,9 @@ async def admin_reset_password_link( link = await service.generate_reset_link(user_id, base_url) except UserNotFoundError: raise HTTPException(status_code=404, detail="User not found") from None + except ExternalUserNoPasswordError: + raise HTTPException( + status_code=409, + detail="External (SSO) users have no password to reset", + ) from None return PasswordResetLink(link=link) diff --git a/modules/users/users/admin/components/UserRow.tsx b/modules/users/users/admin/components/UserRow.tsx index 940c3ac7..c74ae03d 100644 --- a/modules/users/users/admin/components/UserRow.tsx +++ b/modules/users/users/admin/components/UserRow.tsx @@ -10,6 +10,7 @@ export interface UserListItem { full_name: string | null; is_active: boolean; is_verified: boolean; + is_external: boolean; last_login_at: string | null; created_at: string | null; roles: string[]; @@ -63,7 +64,14 @@ export function UserRow({ user }: { user: UserListItem }) { {user.roles.length > 0 ? user.roles.join(', ') : '—'} - +
+ + {user.is_external && ( + + SSO + + )} +
{user.last_login_at ? new Date(user.last_login_at).toLocaleDateString() : '—'} diff --git a/modules/users/users/admin/queries.py b/modules/users/users/admin/queries.py index e21af660..a39a05d0 100644 --- a/modules/users/users/admin/queries.py +++ b/modules/users/users/admin/queries.py @@ -40,6 +40,7 @@ def to_list_item(self, user: User) -> UserListItem: full_name=user.full_name, is_active=user.is_active, is_verified=user.is_verified, + is_external=user.is_external, disabled_at=user.disabled_at, last_login_at=user.last_login_at, created_at=user.created_at, @@ -103,6 +104,7 @@ async def list_users( User.full_name, User.is_active, User.is_verified, + User.is_external, User.disabled_at, User.last_login_at, User.created_at, diff --git a/modules/users/users/admin/service.py b/modules/users/users/admin/service.py index 1cb27a59..32978cd2 100644 --- a/modules/users/users/admin/service.py +++ b/modules/users/users/admin/service.py @@ -11,7 +11,11 @@ from users.admin.queries import _UserServiceBase from users.contracts.schemas import UserCreate -from users.exceptions import EmailAlreadyExistsError, UserNotFoundError +from users.exceptions import ( + EmailAlreadyExistsError, + ExternalUserNoPasswordError, + UserNotFoundError, +) from users.models import OAuthAccount, RefreshToken, User, UserAccessToken, UserRole @@ -205,8 +209,15 @@ async def set_roles( return user async def generate_reset_link(self, user_id: uuid.UUID, base_url: str) -> str: - """Build an admin-copyable password-reset URL. No email side-effect.""" + """Build an admin-copyable password-reset URL. No email side-effect. + + Raises ``ExternalUserNoPasswordError`` for SSO users — they have no + local password, so a reset link is meaningless (and would crash on the + null-hash fingerprint). + """ user = await self._require_user(user_id) + if user.is_external: + raise ExternalUserNoPasswordError(user_id) token = await self._manager.generate_reset_password_token(user) return f"{base_url.rstrip('/')}/users/reset-password?token={token}" diff --git a/modules/users/users/auth_local/token_api.py b/modules/users/users/auth_local/token_api.py index 3b333edf..f69ba243 100644 --- a/modules/users/users/auth_local/token_api.py +++ b/modules/users/users/auth_local/token_api.py @@ -62,7 +62,17 @@ async def token_login( stmt = select(User).where(User.email == body.email) user = (await db.execute(stmt)).scalar_one_or_none() - if user is None or not user.is_active or user.disabled_at is not None: + if ( + user is None + or not user.is_active + or user.disabled_at is not None + # External (SSO) users have ``hashed_password is None`` — there's no + # local password to verify. Treat them like a missing user: verifying + # against a None hash would raise (500) and the instant failure would + # leak that the account is SSO-only. The session login is guarded the + # same way in ``UserManager.authenticate``. + or user.hashed_password is None + ): # Constant-time: run bcrypt on a dummy hash to prevent timing-based # email enumeration (existing user + wrong password takes ~50ms for # bcrypt; missing user would be instant without this). diff --git a/modules/users/users/backend.py b/modules/users/users/backend.py index 66059cf5..76f7be29 100644 --- a/modules/users/users/backend.py +++ b/modules/users/users/backend.py @@ -82,4 +82,4 @@ def reconfigure_cookie_transport( transport.cookie_name = settings.cookie_name transport.cookie_max_age = settings.cookie_max_age_seconds transport.cookie_secure = settings.cookie_secure - transport.cookie_samesite = settings.cookie_samesite # type: ignore[assignment] # ty: ignore[invalid-assignment] + transport.cookie_samesite = settings.cookie_samesite # type: ignore[assignment] diff --git a/modules/users/users/constants.py b/modules/users/users/constants.py index 6da5b379..9469d1b7 100644 --- a/modules/users/users/constants.py +++ b/modules/users/users/constants.py @@ -20,6 +20,12 @@ # Session keys SESSION_USER_ID_KEY = "user_id" +# request.state flag set by the OAuth callback before find-or-create so the +# manager's on_after_register hook can mark *newly provisioned* OAuth users as +# external (null password). on_after_register fires only for new users, so the +# flag is ignored when an OAuth login merely links to an existing account. +OAUTH_REGISTRATION_REQUEST_FLAG = "users_oauth_registration" + # Admin list-endpoint allowed filter/sort values ALLOWED_STATUS = frozenset({"active", "disabled"}) ALLOWED_VERIFIED = frozenset({"yes", "no"}) diff --git a/modules/users/users/contracts/schemas.py b/modules/users/users/contracts/schemas.py index 6052254f..31e9d27c 100644 --- a/modules/users/users/contracts/schemas.py +++ b/modules/users/users/contracts/schemas.py @@ -25,6 +25,7 @@ class UserRead(CreateUpdateDictModel, SQLModel): is_active: bool = True is_superuser: bool = False is_verified: bool = False + is_external: bool = False full_name: str | None = None tenant_id: str | None = None disabled_at: datetime | None = None @@ -74,6 +75,7 @@ class UserListItem(SQLModel): full_name: str | None = None is_active: bool is_verified: bool + is_external: bool = False disabled_at: datetime | None = None last_login_at: datetime | None = None created_at: datetime | None = None diff --git a/modules/users/users/exceptions.py b/modules/users/users/exceptions.py index e45e94ee..316386fa 100644 --- a/modules/users/users/exceptions.py +++ b/modules/users/users/exceptions.py @@ -24,3 +24,15 @@ class EmailAlreadyExistsError(Exception): def __init__(self, email: str) -> None: super().__init__(f"Email {email} already in use") self.email = email + + +class ExternalUserNoPasswordError(Exception): + """Raised when a password-credential action targets an external (SSO) user. + + External users have no local password, so password reset / set has no + meaning for them. Endpoints translate this into a 4xx. + """ + + def __init__(self, user_id: uuid.UUID) -> None: + super().__init__(f"User {user_id} is external (SSO) and has no password") + self.user_id = user_id diff --git a/modules/users/users/manager.py b/modules/users/users/manager.py index e09737c6..4de82509 100644 --- a/modules/users/users/manager.py +++ b/modules/users/users/manager.py @@ -11,9 +11,10 @@ from fastapi_users import BaseUserManager, UUIDIDMixin, exceptions from fastapi_users.jwt import generate_jwt -from users.constants import SESSION_USER_ID_KEY +from users.constants import OAUTH_REGISTRATION_REQUEST_FLAG, SESSION_USER_ID_KEY from users.contracts.events import UserRegistered from users.db_adapter import UserDatabaseWithRoles, get_user_db +from users.exceptions import ExternalUserNoPasswordError from users.mailer import Mailer from users.models import User @@ -51,9 +52,57 @@ async def validate_password(self, password: str, user) -> None: if password.isdigit(): raise exceptions.InvalidPasswordException(reason="Password cannot be all numbers") + # ── Auth (password-credential guards) ──────────────────── + + async def authenticate(self, credentials): + """Authenticate by email + password. + + Mirrors ``BaseUserManager.authenticate`` but rejects **external (SSO) + users**: they have ``hashed_password is None``, so there is no local + password to verify. We still run a dummy hash to keep timing uniform + with the user-not-found and wrong-password paths. + """ + try: + user = await self.get_by_email(credentials.username) + except exceptions.UserNotExists: + self.password_helper.hash(credentials.password) + return None + + if user.hashed_password is None: + self.password_helper.hash(credentials.password) + return None + + verified, updated_password_hash = self.password_helper.verify_and_update( + credentials.password, user.hashed_password + ) + if not verified: + return None + if updated_password_hash is not None: + await self.user_db.update(user, {"hashed_password": updated_password_hash}) + return user + + async def forgot_password(self, user: User, request: Request | None = None) -> None: + """Skip password reset for external (SSO) users — they have no password. + + Returning silently (rather than raising) preserves the public + forgot-password endpoint's anti-enumeration behaviour: it always + responds the same regardless of whether the account can reset. + """ + if user.is_external or user.hashed_password is None: + return + await super().forgot_password(user, request) + # ── Lifecycle hooks ────────────────────────────────────── async def on_after_register(self, user: User, request: Request | None = None) -> None: + # A user provisioned via OAuth (flag set by the OAuth callback before + # find-or-create) is external: drop the random password fastapi-users + # assigned and mark the account SSO-only. Fires only for *new* users — + # OAuth logins that link to an existing account don't reach here. + if request is not None and getattr(request.state, OAUTH_REGISTRATION_REQUEST_FLAG, False): + user.hashed_password = None + user.is_external = True + await self.user_db.update(user, {"hashed_password": None, "is_external": True}) await self._publish_user_registered(user, request) if not user.is_verified: # Kicks off on_after_request_verify, which sends the email @@ -130,6 +179,9 @@ async def generate_reset_password_token(self, user: User) -> str: default rounds) so we offload it to a worker thread — otherwise a single admin action would stall the event loop for other requests. """ + if user.is_external or user.hashed_password is None: + # No local password to fingerprint — hashing None would crash. + raise ExternalUserNoPasswordError(user.id) fingerprint = await asyncio.to_thread(self.password_helper.hash, user.hashed_password) token_data = { "sub": str(user.id), diff --git a/modules/users/users/models/user.py b/modules/users/users/models/user.py index 0d813f18..ff9da602 100644 --- a/modules/users/users/models/user.py +++ b/modules/users/users/models/user.py @@ -16,7 +16,7 @@ from fastapi_users_db_sqlalchemy.generics import GUID from simple_module_db.mixins import AuditMixin -from sqlalchemy import DateTime, Index, text +from sqlalchemy import DateTime, Index, false, text from sqlmodel import Field, Relationship from users.models._base import Base @@ -40,10 +40,20 @@ class User(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] primary_key=True, ) email: str = Field(max_length=320, unique=True, index=True) - hashed_password: str = Field(max_length=1024) + # Nullable: external (SSO) users have NO local password. fastapi-users sets + # this for password accounts; the OAuth path leaves it ``None`` (see + # ``UserManager.on_after_register``). + hashed_password: str | None = Field(default=None, max_length=1024) is_active: bool = Field(default=True) is_superuser: bool = Field(default=False) is_verified: bool = Field(default=False) + # True for users provisioned via an external IdP (Microsoft/Entra, Google, + # GitHub, generic OIDC). They authenticate only through SSO; password login + # and password reset are refused. Roles are still assigned locally. + is_external: bool = Field( + default=False, + sa_column_kwargs={"server_default": false()}, + ) full_name: str | None = Field(default=None, max_length=255) tenant_id: str | None = Field(default=None, max_length=50, index=True) diff --git a/modules/users/users/oauth/api.py b/modules/users/users/oauth/api.py index 88b141fc..a86cf3b5 100644 --- a/modules/users/users/oauth/api.py +++ b/modules/users/users/oauth/api.py @@ -24,6 +24,7 @@ from fastapi_users import exceptions as fu_exceptions from starlette.responses import RedirectResponse +from users.constants import OAUTH_REGISTRATION_REQUEST_FLAG from users.deps import auth_backend, get_user_manager if TYPE_CHECKING: @@ -86,6 +87,10 @@ async def callback( if account_email is None: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="OAUTH_NO_EMAIL") + # Mark this request so the manager's on_after_register provisions a + # *new* OAuth user as external (null password). Ignored when the login + # links to an existing account (on_after_register won't fire). + setattr(request.state, OAUTH_REGISTRATION_REQUEST_FLAG, True) try: user = await user_manager.oauth_callback( provider, diff --git a/modules/users/users/pages/Users/Edit.tsx b/modules/users/users/pages/Users/Edit.tsx index 01df14cd..675ca969 100644 --- a/modules/users/users/pages/Users/Edit.tsx +++ b/modules/users/users/pages/Users/Edit.tsx @@ -18,6 +18,7 @@ interface UserListItem { full_name: string | null; is_active: boolean; is_verified: boolean; + is_external: boolean; disabled_at: string | null; last_login_at: string | null; created_at: string | null; @@ -142,6 +143,24 @@ function Edit() { Metadata
+
Sign-in
+
+ {user.is_external ? ( + + External · SSO + + ) : ( + + Local · password + + )} +
Created
{fmt(user.created_at)}
Last login
@@ -183,6 +202,7 @@ function Edit() { void; onEnable: () => void; @@ -27,6 +28,7 @@ interface Props { export function AccountStatusCard({ email, isActive, + isExternal, savingStatus, onDisable, onEnable, @@ -47,6 +49,11 @@ export function AccountStatusCard({ > {isActive ? 'active' : 'disabled'} + {isExternal && ( + + External · SSO + + )}
{isActive ? ( @@ -74,28 +81,36 @@ export function AccountStatusCard({ {savingStatus ? 'Saving…' : 'Enable account'} )} - - - - - - - Generate reset link for {email}? - - A one-time password-reset URL will be copied to your clipboard. Any previously - issued reset link for this user will be invalidated. - - - - Cancel - Generate - - - + {!isExternal && ( + + + + + + + Generate reset link for {email}? + + A one-time password-reset URL will be copied to your clipboard. Any previously + issued reset link for this user will be invalidated. + + + + Cancel + Generate + + + + )}
+ {isExternal && ( +

+ This account signs in through an external identity provider (SSO) and has no password, + so there's no reset link to generate. +

+ )} );