diff --git a/CHANGELOG.md b/CHANGELOG.md index 844ffbeea..e0bfd2339 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,19 @@ All notable changes to this project are documented in this file. The format is b across pages, and recover concurrent duplicate share grants without a 500 (#640). - Share UTC timestamp helpers across sharing, tenants, background tasks, API tokens and users (#640). +- Background tasks: serialize eager outbox dispatches so chained deliveries do not + overlap on shared in-memory SQLite connections or trip another handler's commit + guard during tenant teardown (#655). +- Api tokens: durably purge a deleted user's personal tokens across all tenants + on `UserDeleted`, preserving service keys and other users' tokens (#657). +- Users: cap refresh-token families at a DB-configurable 90 days, refusing refreshes + and revoking/pruning aged families while preserving reuse evidence for live families (#656). +- Users admin actions show translated server refusal details instead of generic errors; + unknown details retain the existing fallback (#653). +- Users: GET and PATCH `/api/users/me` now accept bearer access tokens from + `/api/users/auth/token` alongside browser sessions (#658). +- Users: redirect refused OAuth/OIDC browser sign-ins to login with translated English + and German messages; preserve JSON errors for API callers (#654). - Users: publish deletion, disabling and new role assignments transactionally through the durable outbox (inline cleanup without background tasks). Deletion purges tenant memberships, shares to the user and direct permission grants; tenants left without diff --git a/docs/modules/users.md b/docs/modules/users.md index 4f04a1645..8b12226c5 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -196,6 +196,7 @@ Everything else is DB-backed (initial values are pydantic defaults; edit under U | `verification_token_lifetime_seconds` | `604_800` (7 days) | | `bearer_token_lifetime_seconds` | `900` (15 min) | | `refresh_token_lifetime_seconds` | `2_592_000` (30 days) | +| `refresh_token_family_max_age_days` | `90` (≥ 1) — absolute age from the first token; requires a new login after expiry | | `cookie_name` | `"sm_auth"` | | `cookie_max_age_seconds` | `1_209_600` (14 days) | | `cookie_secure` | `True` (flipped to `False` in dev at startup) | diff --git a/modules/api_tokens/api_tokens/module.py b/modules/api_tokens/api_tokens/module.py index 32a09bbe6..581c8532f 100644 --- a/modules/api_tokens/api_tokens/module.py +++ b/modules/api_tokens/api_tokens/module.py @@ -38,10 +38,13 @@ class ApiTokensModule(ModuleBase): def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> None: from simple_module_core.tenant_events import TenantDeleted + from simple_module_core.user_events import UserDeleted from api_tokens.tenant_cleanup import purge_tenant_data + from api_tokens.user_cleanup import purge_user_tokens bus.subscribe_durable(TenantDeleted, purge_tenant_data) + bus.subscribe_durable(UserDeleted, purge_user_tokens) def register_settings(self, app: FastAPI) -> None: from api_tokens.services import ApiTokensServices diff --git a/modules/api_tokens/api_tokens/user_cleanup.py b/modules/api_tokens/api_tokens/user_cleanup.py new file mode 100644 index 000000000..82d696eb7 --- /dev/null +++ b/modules/api_tokens/api_tokens/user_cleanup.py @@ -0,0 +1,19 @@ +"""Durable cleanup of a deleted user's personal tokens.""" + +from simple_module_core.durable_events import DurableContext +from simple_module_core.user_events import UserDeleted +from simple_module_db.tenancy import all_tenants +from sqlmodel import col, delete + +from api_tokens.constants import TokenKind +from api_tokens.models import ApiToken + + +async def purge_user_tokens(event: UserDeleted, ctx: DurableContext) -> None: + with all_tenants(): + await ctx.db.execute( + delete(ApiToken).where( + col(ApiToken.kind) == TokenKind.PERSONAL, + col(ApiToken.owner_user_id) == event.user_id, + ) + ) diff --git a/modules/api_tokens/tests/test_api_tokens_user_cleanup.py b/modules/api_tokens/tests/test_api_tokens_user_cleanup.py new file mode 100644 index 000000000..89510147e --- /dev/null +++ b/modules/api_tokens/tests/test_api_tokens_user_cleanup.py @@ -0,0 +1,61 @@ +"""User deletion purges personal tokens across tenants transactionally (#657).""" + +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +from api_tokens.constants import TokenKind +from api_tokens.models import ApiToken +from api_tokens.module import ApiTokensModule +from simple_module_core.events import EventBus +from simple_module_core.user_events import UserDeleted +from simple_module_db import all_tenants, tenant_context +from simple_module_db.transaction import finalize_session, rollback_session +from sqlmodel import select + + +@pytest.mark.parametrize("rollback", [False, True]) +async def test_api_tokens_user_deleted_cleanup_is_scoped_idempotent_and_transactional( + db_session, rollback +): + deleted_id, other_id = uuid.uuid4(), uuid.uuid4() + kept_ids = set() + for tenant_id in ("first", "second"): + with tenant_context(tenant_id): + for kind, owner in ( + (TokenKind.PERSONAL, deleted_id), + (TokenKind.PERSONAL, other_id), + (TokenKind.SERVICE, deleted_id), + ): + token = ApiToken( + kind=kind, + owner_user_id=owner, + name="token", + prefix="test", + token_hash=uuid.uuid4().hex, + expires_at=datetime.now(UTC) + timedelta(days=1), + ) + db_session.add(token) + if kind == TokenKind.SERVICE or owner == other_id: + kept_ids.add(token.id) + await db_session.flush() + await finalize_session(db_session) + bus = EventBus() + # Workers register without an app; inline delivery uses the same durable subscription. + ApiTokensModule().register_event_handlers(bus) + with tenant_context("first"): + for _ in range(2): + await bus.publish_durable_or_inline(UserDeleted(deleted_id), db_session) + with all_tenants(): + remaining = (await db_session.execute(select(ApiToken.id))).scalars().all() + assert set(remaining) == kept_ids + if rollback: + await rollback_session(db_session) + else: + await finalize_session(db_session) + with all_tenants(): + remaining = (await db_session.execute(select(ApiToken.id))).scalars().all() + if rollback: + assert len(remaining) == 6 + else: + assert set(remaining) == kept_ids diff --git a/modules/background_tasks/background_tasks/outbox_transport.py b/modules/background_tasks/background_tasks/outbox_transport.py index aaecd5dde..c8392c1a3 100644 --- a/modules/background_tasks/background_tasks/outbox_transport.py +++ b/modules/background_tasks/background_tasks/outbox_transport.py @@ -132,6 +132,8 @@ def __init__( # Eager in-process deliveries still running (kept so they aren't # garbage-collected mid-flight, and so ``drain`` can await them). self._tasks: set[asyncio.Task[Any]] = set() + # Chained eager publishes must not share a StaticPool transaction (#655). + self._dispatch_lock = asyncio.Lock() def policy(self) -> OutboxPolicy: s = self._services.settings @@ -155,8 +157,9 @@ def schedule(self, db: AsyncSession | Session, ids: Sequence[uuid.UUID]) -> None id_strs = [str(i) for i in id_list] after_commit(db, [lambda: self._send(id_strs)]) - def _dispatch(self, ids: list[uuid.UUID]) -> Any: - return dispatch(self._db_state.session_factory, self._bus, self.policy(), ids=ids) + async def _dispatch(self, ids: list[uuid.UUID]) -> Any: + async with self._dispatch_lock: + return await dispatch(self._db_state.session_factory, self._bus, self.policy(), ids=ids) def _dispatch_soon(self, ids: list[uuid.UUID]) -> None: """Eager delivery from a session's ``after_commit`` event. diff --git a/modules/background_tasks/tests/test_bg_outbox_eager_serialization.py b/modules/background_tasks/tests/test_bg_outbox_eager_serialization.py new file mode 100644 index 000000000..b94dd6c2c --- /dev/null +++ b/modules/background_tasks/tests/test_bg_outbox_eager_serialization.py @@ -0,0 +1,55 @@ +"""Chained eager deliveries must not overlap on in-memory SQLite (#655).""" + +import asyncio +from dataclasses import dataclass + +from background_tasks.constants import OutboxStatus +from background_tasks.models import TaskExecution +from simple_module_core import DurableContext +from simple_module_core.events import Event + + +@dataclass +class EagerParent(Event): + pass + + +@dataclass +class EagerChild(Event): + pass + + +async def test_chained_eager_delivery_waits_for_parent_dispatch(app, outbox_rows, effect_names): + app.state.background_tasks.settings.task_always_eager = True + bus = app.state.sm.event_bus + active = False + + async def publish_child(event: EagerParent, ctx: DurableContext) -> None: + await bus.publish_durable(EagerChild(), ctx.db) + + async def effect(event: Event, ctx: DurableContext) -> None: + nonlocal active + assert not active, "eager dispatchers overlapped" + active = True + try: + await ctx.db.connection() + await asyncio.sleep(0.05) # intentional-sleep: child dispatcher claims while guarded + ctx.db.add(TaskExecution(task_name=f"effect:{type(event).__name__}")) + await ctx.db.flush() + finally: + active = False + + bus.subscribe_durable(EagerParent, publish_child, name="eager-publish-child") + bus.subscribe_durable(EagerParent, effect, name="eager-parent-effect") + bus.subscribe_durable(EagerChild, effect, name="eager-child-effect") + async with app.state.sm.db.session_factory() as db: + await bus.publish_durable(EagerParent(), db) + await db.commit() + + await bus.durable_transport.drain() + + rows = await outbox_rows() + assert len(rows) == 3 + assert all(row.status == OutboxStatus.DELIVERED for row in rows) + assert all(row.attempts == 1 for row in rows) + assert await effect_names() == ["effect:EagerChild", "effect:EagerParent"] diff --git a/modules/tenants/tests/test_tenants_teardown_integration.py b/modules/tenants/tests/test_tenants_teardown_integration.py index 8649df4b3..17c66de86 100644 --- a/modules/tenants/tests/test_tenants_teardown_integration.py +++ b/modules/tenants/tests/test_tenants_teardown_integration.py @@ -1,7 +1,9 @@ """A real tenant deletion delivers every installed owner's durable teardown.""" +from dataclasses import replace from datetime import UTC, datetime, timedelta +import pytest from api_tokens.models import ApiToken from audit_log.models import AuditEntry from background_tasks.outbox_claim import OutboxPolicy @@ -9,12 +11,21 @@ from feature_flags.models import FeatureFlagOverride from file_storage.backends.filesystem import FilesystemBackend from file_storage.models import StoredFile +from sharing.module import SharingModule from simple_module_db import all_tenants, tenant_context from simple_module_db.stored_settings import SCOPE_TENANT, Setting from sqlmodel import select from tenants.service import TenantService +@pytest.fixture(autouse=True, params=[False, True], ids=["default-order", "sharing-after-users"]) +def _sharing_load_order(request, monkeypatch): + if request.param: + monkeypatch.setattr( + SharingModule, "meta", replace(SharingModule.meta, optional_depends_on=["Users"]) + ) + + async def test_tenants_delete_removes_owned_data_but_retains_audit( app, tenant_client, client, tmp_path, monkeypatch ): diff --git a/modules/users/tests-js/users_Login.test.tsx b/modules/users/tests-js/users_Login.test.tsx index 910066275..b7fbb71e7 100644 --- a/modules/users/tests-js/users_Login.test.tsx +++ b/modules/users/tests-js/users_Login.test.tsx @@ -42,6 +42,22 @@ afterEach(() => { }); describe('users Login', () => { + it.each([ + ['account_not_verified', keys.users.login.oauth_account_not_verified], + ['signup_disabled', keys.users.login.oauth_signup_disabled], + ['signin_failed', keys.users.login.oauth_signin_failed], + ])('shows the translated OAuth message for %s', (code, key) => { + configureI18n({ locale: 'en', messages: { [key]: 'Sign-in was refused.' } }); + render(); + expect(screen.getByRole('alert')).toHaveTextContent('Sign-in was refused.'); + configureI18n({ locale: 'en', messages: {} }); + }); + + it('ignores unknown OAuth codes instead of displaying query text', () => { + render(); + expect(screen.queryByRole('alert')).toBeNull(); + }); + it('posts the form-encoded credentials and follows the server redirect on 204', async () => { const mock = stubFetch(() => new Response(null, { status: 204 })); render(); diff --git a/modules/users/tests-js/users_admin_error_details.test.tsx b/modules/users/tests-js/users_admin_error_details.test.tsx new file mode 100644 index 000000000..968d0e9f2 --- /dev/null +++ b/modules/users/tests-js/users_admin_error_details.test.tsx @@ -0,0 +1,112 @@ +import { configureI18n, t } from '@simple-module-py/i18n'; +import { act, renderHook, waitFor } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@inertiajs/react', () => ({ router: { reload: vi.fn(), on: vi.fn(() => vi.fn()) } })); +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +import { toast } from 'sonner'; +import { useUserRowActions } from '../users/admin/components/useUserRowActions'; +import de from '../users/locales/de.json'; +import en from '../users/locales/en.json'; +import { adminFailureMessage } from '../users/pages/failureMessage'; +import { useEditUserForm } from '../users/pages/Users/components/useEditUserForm'; +import { useUserActions } from '../users/pages/Users/components/useUserActions'; +import { json, stubFetch } from './users_fixtures'; + +afterEach(() => { + vi.unstubAllGlobals(); + vi.clearAllMocks(); +}); + +describe.each(['en', 'de'])('users admin errors (%s)', (locale) => { + function configure() { + const messages: Record = {}; + function flatten(value: object, prefix = 'users') { + for (const [key, child] of Object.entries(value)) { + if (typeof child === 'string') messages[`${prefix}.${key}`] = child; + else flatten(child, `${prefix}.${key}`); + } + } + flatten(locale === 'en' ? en : de); + configureI18n({ locale, messages }); + } + + const privileged = () => + locale === 'en' + ? 'You cannot manage a more privileged account.' + : 'Sie können kein Konto mit höheren Berechtigungen verwalten.'; + + it.each(['row', 'edit'])('translates the server refusal in the %s action toast', async (page) => { + configure(); + stubFetch(() => json(403, { detail: 'You cannot manage a more privileged account.' })); + const { result } = renderHook(() => { + const row = useUserRowActions(); + const edit = useUserActions('target', true, false, null); + return { row, edit }; + }); + await act(async () => { + if (page === 'row') await result.current.row.setActive('target', false); + else result.current.edit.disable(); + }); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith(privileged())); + expect(result.current.edit.isActive).toBe(true); + }); + + it.each([false, true])( + 'explains a refused role grant after details saved: %s', + async (detailsDirty) => { + configure(); + stubFetch((request) => + request.method === 'PATCH' + ? json(200, {}) + : json(403, { detail: 'You cannot assign roles you do not hold.' }), + ); + const user = { id: 'target', email: 'a@example.com', full_name: null, roles: [] }; + const { result } = renderHook(() => useEditUserForm(user)); + act(() => { + result.current.toggleRole('Admin'); + if (detailsDirty) result.current.setForm((prev) => ({ ...prev, fullName: 'New name' })); + }); + await act(async () => result.current.save()); + const reason = + locale === 'en' + ? 'You cannot assign roles you do not hold.' + : 'Sie können keine Rollen zuweisen, die Sie selbst nicht innehaben.'; + expect(result.current.error).toBe( + detailsDirty ? `${(locale === 'en' ? en : de).edit.error_partial} ${reason}` : reason, + ); + expect(result.current.dirtyCount).toBe(1); + }, + ); + + it.each(Object.entries(en.admin_errors))('translates the known refusal %s', (key, detail) => { + configure(); + expect(adminFailureMessage({ status: 400, detail }, 'fallback', 'network', t)).toBe( + (locale === 'en' ? en : de).admin_errors[key as keyof typeof en.admin_errors], + ); + }); + + it.each([null, '', 'constructor', 'unexpected server internals', [{ msg: 'invalid' }]])( + 'keeps the fallback for unknown detail %s', + async (detail) => { + configure(); + stubFetch(() => json(400, { detail })); + const { result } = renderHook(() => useUserRowActions()); + await act(async () => result.current.setActive('target', false)); + expect(toast.error).toHaveBeenCalledWith( + (locale === 'en' ? en : de).user_row.toast_status_failed, + ); + }, + ); + + it('keeps the network failure message', async () => { + configure(); + stubFetch(() => { + throw new TypeError('offline'); + }); + const { result } = renderHook(() => useUserRowActions()); + await act(async () => result.current.setActive('target', false)); + expect(toast.error).toHaveBeenCalledWith((locale === 'en' ? en : de).common.error_occurred); + }); +}); diff --git a/modules/users/tests/test_bearer_token_expiry.py b/modules/users/tests/test_bearer_token_expiry.py index d83fbe706..4a6f89e79 100644 --- a/modules/users/tests/test_bearer_token_expiry.py +++ b/modules/users/tests/test_bearer_token_expiry.py @@ -32,6 +32,30 @@ _CREDS = {"username": "admin@example.com", "password": "AdminPass1!"} +async def _cookie_strategy_user(app, client): + """Resolve the ``sm_auth`` cookie through the cookie strategy itself. + + ``/api/users/me`` now resolves the request principal (session or bearer, + #658), so it no longer exercises ``ExpiringDatabaseStrategy``; read the row + the way ``fastapi_users.current_user`` does instead. + """ + from users.backend import build_strategy + from users.db_adapter import PruningAccessTokenDatabase + from users.models import UserAccessToken + + async with app.state.sm.db.session_factory() as session: + + class _Manager: + def parse_id(self, value): + return value if isinstance(value, uuid.UUID) else uuid.UUID(str(value)) + + async def get(self, user_id): + return await session.get(User, user_id) + + strategy = build_strategy(PruningAccessTokenDatabase(session, UserAccessToken), 900) + return await strategy.read_token(client.cookies.get("sm_auth"), _Manager()) + + async def _tokens(app) -> list[UserAccessToken]: async with app.state.sm.db.session_factory() as session: return list((await session.execute(select(UserAccessToken))).scalars()) @@ -129,15 +153,14 @@ class TestAnExpiredRowStopsAuthenticating: async def test_the_cookie_strategy_path_refuses_it(self, users_app, anon_client): """`fastapi_users.current_user`, via ExpiringDatabaseStrategy. - ``/api/users/me`` is the probe because it depends on ``current_user``, - which is what reads the ``sm_auth`` row. The row stays well inside the - thirty-day read ceiling — only the deadline it was issued for has passed. + The row stays well inside the thirty-day read ceiling — only the + deadline it was issued for has passed. """ assert (await anon_client.post(_LOGIN, data=_CREDS)).status_code == 204 - assert (await anon_client.get(_ME)).status_code == 200 + assert await _cookie_strategy_user(users_app, anon_client) is not None await _expire_all(users_app) - assert (await anon_client.get(_ME)).status_code == 401 + assert await _cookie_strategy_user(users_app, anon_client) is None @pytest.mark.anyio async def test_the_provider_path_refuses_it(self, users_app, anon_client): diff --git a/modules/users/tests/test_remember_me.py b/modules/users/tests/test_remember_me.py index e681ca23c..4f23bbd2c 100644 --- a/modules/users/tests/test_remember_me.py +++ b/modules/users/tests/test_remember_me.py @@ -77,6 +77,30 @@ async def _age_access_tokens(app, *, days: int) -> None: await session.commit() +async def _cookie_strategy_user(app, client): + """Resolve the ``sm_auth`` cookie through the cookie strategy itself. + + ``/api/users/me`` now resolves the request principal (session or bearer, + #658), so it no longer exercises ``ExpiringDatabaseStrategy``; read the row + the way ``fastapi_users.current_user`` does instead. + """ + from users.backend import build_strategy + from users.db_adapter import PruningAccessTokenDatabase + from users.models import UserAccessToken + + async with app.state.sm.db.session_factory() as session: + + class _Manager: + def parse_id(self, value): + return value if isinstance(value, uuid.UUID) else uuid.UUID(str(value)) + + async def get(self, user_id): + return await session.get(User, user_id) + + strategy = build_strategy(PruningAccessTokenDatabase(session, UserAccessToken), 900) + return await strategy.read_token(client.cookies.get("sm_auth"), _Manager()) + + class TestRemembered: @pytest.mark.anyio async def test_the_auth_cookie_lasts_thirty_days(self, anon_client, users_db): @@ -104,14 +128,13 @@ async def test_a_token_inside_the_promised_window_is_still_read( know which sign-ins asked to be remembered — a row minted to last thirty days therefore has to be accepted for thirty, or the cookie outlives the credential it carries and the checkbox starts lying on - day fourteen. ``/api/users/me`` is the probe because it depends on - ``current_user``, which is the code path that reads the row. + day fourteen. """ await _make_user(users_db, "remember-token@example.com") await _login(anon_client, "remember-token@example.com", remember=True) await _age_access_tokens(users_app, days=29) - assert (await anon_client.get("/api/users/me")).status_code == 200 + assert await _cookie_strategy_user(users_app, anon_client) is not None @pytest.mark.anyio async def test_a_token_past_the_window_stops_being_read(self, anon_client, users_db, users_app): @@ -121,7 +144,7 @@ async def test_a_token_past_the_window_stops_being_read(self, anon_client, users await _login(anon_client, "stale-token@example.com", remember=True) await _age_access_tokens(users_app, days=31) - assert (await anon_client.get("/api/users/me")).status_code == 401 + assert await _cookie_strategy_user(users_app, anon_client) is None class TestTheWindowSurvivesLaterRequests: diff --git a/modules/users/tests/test_users_me_credentials.py b/modules/users/tests/test_users_me_credentials.py new file mode 100644 index 000000000..e22d38970 --- /dev/null +++ b/modules/users/tests/test_users_me_credentials.py @@ -0,0 +1,46 @@ +"""Self-profile routes accept local bearer tokens as well as browser cookies.""" + +import pytest +from _users_app_builders import user_row + + +@pytest.mark.parametrize("credential", ["bearer", "cookie"]) +async def test_users_me_reads_and_updates_profile(anon_client, alice, users_app, credential): + if credential == "bearer": + login = await anon_client.post( + "/api/users/auth/token", + json={"email": alice.email, "password": "UserPass1!"}, + ) + assert login.status_code == 200 + anon_client.cookies.clear() + headers = {"Authorization": f"Bearer {login.json()['access_token']}"} + else: + login = await anon_client.post( + "/api/users/auth/login", + data={"username": alice.email, "password": "UserPass1!"}, + ) + assert login.status_code == 204 + headers = {} + + profile = await anon_client.get("/api/users/me", headers=headers) + assert profile.status_code == 200 + assert profile.json()["id"] == str(alice.id) + assert profile.json()["email"] == alice.email + assert "hashed_password" not in profile.json() + + updated = await anon_client.patch( + "/api/users/me", headers=headers, json={"full_name": "Updated Profile"} + ) + assert updated.status_code == 200 + assert updated.json()["id"] == str(alice.id) + assert updated.json()["full_name"] == "Updated Profile" + assert (await user_row(users_app, alice.email)).full_name == "Updated Profile" + + +@pytest.mark.parametrize("method", ["GET", "PATCH"]) +@pytest.mark.parametrize("headers", [{}, {"Authorization": "Bearer invalid-token"}]) +async def test_users_me_requires_valid_credentials(anon_client, method, headers): + response = await anon_client.request( + method, "/api/users/me", headers=headers, json={"full_name": "Unauthorized"} + ) + assert response.status_code == 401 diff --git a/modules/users/tests/test_users_oauth_error_responses.py b/modules/users/tests/test_users_oauth_error_responses.py new file mode 100644 index 000000000..8422c851a --- /dev/null +++ b/modules/users/tests/test_users_oauth_error_responses.py @@ -0,0 +1,111 @@ +"""Exercise callback error responses without a database or provider network.""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import httpx +import pytest +from fastapi import APIRouter, FastAPI +from fastapi_users.exceptions import UserNotExists +from httpx_oauth.exceptions import GetProfileError +from starlette.middleware.sessions import SessionMiddleware +from users.deps import auth_backend, get_user_manager +from users.oauth.api import register_oauth_routes +from users.oauth.providers import OAuthProvider + + +@pytest.mark.anyio +@pytest.mark.parametrize( + ("failure", "detail", "public_code", "status"), + [ + ("unverified", "OAUTH_ACCOUNT_NOT_VERIFIED", "account_not_verified", 403), + ("signup", "OAUTH_SIGNUP_DISABLED", "signup_disabled", 403), + ("provider", "OAUTH_PROVIDER_ERROR", "signin_failed", 400), + ("profile", "OAUTH_PROVIDER_ERROR", "signin_failed", 400), + ("token", "OAUTH_PROVIDER_ERROR", "signin_failed", 400), + ("state", "OAUTH_INVALID_STATE", "signin_failed", 400), + ("missing_code", "OAUTH_MISSING_CODE", "signin_failed", 400), + ], +) +@pytest.mark.parametrize("accept", ["text/html", "application/json", "*/*", "xhr"]) +async def test_oauth_callback_error_response(failure, detail, public_code, status, accept): + app = FastAPI() + app.add_middleware(SessionMiddleware, secret_key="test-secret") + provider_client = AsyncMock() + provider_client.get_authorization_url.side_effect = lambda url, state: ( + f"https://idp/?state={state}" + ) + provider_client.get_access_token.return_value = {"access_token": "test-token"} + provider_client.get_profile.return_value = {"sub": "test-id", "email": "test@example.com"} + if failure == "token": + provider_client.get_access_token.side_effect = httpx.ConnectError("private details") + if failure == "profile": + provider_client.get_profile.side_effect = GetProfileError("private provider details") + app.state.users = SimpleNamespace( + oauth_clients={"oidc": OAuthProvider("oidc", "OIDC", provider_client)}, + settings=SimpleNamespace(allow_signup=False), + ) + manager = AsyncMock() + manager.get_by_oauth_account.side_effect = UserNotExists() + if failure == "unverified": + manager.get_by_email.return_value = SimpleNamespace(is_verified=False, invited_at=None) + else: + manager.get_by_email.side_effect = UserNotExists() + + async def manager_dep(): + return manager + + async def strategy_dep(): + return None + + app.dependency_overrides[get_user_manager] = manager_dep + app.dependency_overrides[auth_backend.get_strategy] = strategy_dep + router = APIRouter(prefix="/api/users") + register_oauth_routes(router) + app.include_router(router) + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://test" + ) as client: + login = await client.get("/api/users/auth/oidc/login") + state = httpx.URL(login.headers["location"]).params["state"] + params = {"state": "bad" if failure == "state" else state} + if failure == "provider": + params["error"] = "" + elif failure != "missing_code": + params["code"] = "test-code" + headers = {"accept": accept} + if accept == "xhr": + headers = {"accept": "text/html", "x-requested-with": "XMLHttpRequest"} + response = await client.get("/api/users/auth/oidc/callback", params=params, headers=headers) + if accept == "text/html": + assert response.status_code == 303 + assert response.headers["location"] == f"/users/login?oauth_error={public_code}" + else: + assert response.status_code == status + assert response.json() == {"detail": detail} + + +@pytest.mark.anyio +@pytest.mark.parametrize( + "code", ["account_not_verified", "signup_disabled", "signin_failed", ""}, + headers={"accept": "text/html" if browser else "application/json"}, + follow_redirects=False, + ) + if browser: + assert response.status_code == 303 + assert response.headers["location"] == "/users/login?oauth_error=signin_failed" + else: + assert response.status_code == 400 + assert response.json()["detail"] == "OAUTH_PROVIDER_ERROR" + + +@pytest.mark.anyio +@pytest.mark.parametrize( + "code", ["account_not_verified", "signup_disabled", "signin_failed", "