diff --git a/api/docs/adr/000-vertical-slice-architecture.md b/api/docs/adr/000-vertical-slice-architecture.md index 6ceeb6be..95fae513 100644 --- a/api/docs/adr/000-vertical-slice-architecture.md +++ b/api/docs/adr/000-vertical-slice-architecture.md @@ -83,7 +83,7 @@ damnit_api/ │ ├── serialization.py, preview.py │ └── gql.py # Strawberry types + Query/Subscription contributions │ -├── auth/ # OIDC flow, sessions, tokens, users, authz policy +├── auth/ # OIDC flow, sessions, tokens, users, authz policy; see ADR-011 ├── contextfile/ # context-file (REST) endpoints ├── mymdc/ # MyMdC port ├── appdb/ # application-DB engine/session/models; see ADR-010 diff --git a/api/docs/adr/011-authorisation-at-the-edge.md b/api/docs/adr/011-authorisation-at-the-edge.md new file mode 100644 index 00000000..ae3f8b4e --- /dev/null +++ b/api/docs/adr/011-authorisation-at-the-edge.md @@ -0,0 +1,42 @@ +--- +date: 2026-07-08 +--- + +# ADR-011 - Authorisation at the edge + +## Context and Problem Statement + +Proposal-membership authorisation was scattered and pointed the wrong way. The single predicate lived in `metadata/services.py` (`_check_user_allowed`), and the metadata services called it inline before each operation. The GraphQL permission classes reached into that same private function - an `auth`-to-`metadata` import which, together with `metadata` importing `auth` for the `User` type, formed a dependency cycle that [ADR-000](000-vertical-slice-architecture.md) forbids (`metadata -> auth`). + +Domain services performing authorisation also couples them to the request. A service can then only be called where a `User` is in scope, and the same check runs redundantly at the resolver and in the service. + +Separately, an unauthenticated GraphQL request produced a 500 rather than a 401. + +## Considered Options + +- One policy in `auth/`, enforced only at the transport edges. +- Keep the check inline in the domain services. +- A single global authorisation middleware. + +## Decision Outcome + +Chosen option: "one policy in `auth/`, enforced at the edges", because it gives the membership decision a single home in the slice that owns identity and leaves the domain services authorisation-free. + +`auth/policy.py` owns `require_proposal_member(user, proposal_number)`. Enforcement happens only at the edges: Strawberry permission classes for GraphQL fields, and a Litestar guard (`proposal_member_guard`) wired onto the proposal-scoped REST routers in the composition root. Local mode composes the guard out ([ADR-008](008-local-mode-composition.md)). The permission classes live in `shared/permissions.py` as transport adapters over the policy, so any slice's GraphQL contribution can attach them without importing `auth`. Domain services take plain parameters. + +Authentication failures now raise `UnauthenticatedError`, which the error handler renders as a 401. + +### Consequences + +- Good: domain services are reusable and context-agnostic; the `auth <-> metadata` cycle is broken. +- Good: authorisation has one auditable choke point per transport. +- Bad: two edge mechanisms - the GraphQL permission classes and the REST guard - must stay in step. +- Bad: the permission adapters put Strawberry types in `shared/`, which is otherwise framework-light. + +## Details + +Authentication (OIDC on server-side sessions) is unchanged; it is covered by the framework and session decisions in [ADR-006](006-litestar.md). This decision concerns authorisation and the authentication *edge*, the 401. + +The adapters live in `shared/permissions.py` rather than the `graphql/` transport package so that a slice depends only on `shared`, which is always an allowed direction, and never on the composition/transport package. The policy predicate stays in `auth/` because membership is identity: `auth -> proposals`/`metadata` is the one allowed cross-slice edge ([ADR-000](000-vertical-slice-architecture.md)), and `auth/policy.py` itself needs no `metadata` import. + +The 401 is a clean JSON body, not a redirect. A GraphQL request is an XHR call, so the single-page app performs the login redirect on a 401 rather than following a server redirect to the login page. diff --git a/api/docs/architecture.md b/api/docs/architecture.md index 4fd640cb..80f8336e 100644 --- a/api/docs/architecture.md +++ b/api/docs/architecture.md @@ -20,7 +20,7 @@ For more information, see [ADR-000](adr/000-vertical-slice-architecture.md). | --------------------------------- | --------------------------------------- | ------------------------------------------------------------------------------------------------------------------------- | ------- | ------------------------------------------------------------------------- | | `runs/` | Run/variable data - the core domain | Domain models, repository interface + implementations (see [ADR-005](adr/005-repository-pattern.md)), serialisation, preview extraction, its GraphQL types and resolvers | Partial | `runs/` (repository, models, sqlite + csv backends); resolvers still in `graphql/queries.py`/`subscriptions.py` | | `proposals/` | Proposal metadata and lookup | Proposal models, MyMdC-backed metadata services, path locator (see [ADR-004](adr/004-proposal-path-locator.md)) | Planned | `metadata/` | -| `auth/` | Authentication and authorisation | OAuth flow, sessions, token store, `User`, permission classes, the membership policy | Partial | Policy still in `metadata/services.py` | +| `auth/` | Authentication and authorisation | OAuth flow, sessions, token store, `User`, the membership policy (see [ADR-011](adr/011-authorisation-at-the-edge.md)) | Partial | Policy in `auth/policy.py`; permission adapters in `shared/permissions.py` | | `contextfile/` | Context-file viewing | File reading, watching, its routes | Done | As-is | | `graphql/` | GraphQL transport only | Schema assembly, context, directives, controller binding - no resolvers, no domain logic (see [ADR-007](adr/007-graphql-transport-only.md)) | Partial | Assembly still in `shared/gql.py`; resolvers still here | | `appdb/` | The app's own database (infrastructure) | Models, engine/session plumbing for `dw_api.sqlite` (see [ADR-010](adr/010-two-databases.md)) | Partial | `_db/` (Advanced Alchemy `SQLAlchemyPlugin`); `metadata/repository.py` | @@ -56,7 +56,7 @@ The key rules are: 2. **Composition root is the top:** it may import everything, but nothing is allowed to import it. - If importing a slice from the composition root forces a function-body import to avoid cycles, the type probably belongs in `core/`. 3. **Composition root reads settings:** everything else receives configuration as parameters (see [ADR-003](adr/003-injected-settings.md)). -4. **Authorisation applied at the edge:** routes and resolvers use dependencies and permission classes. +4. **Authorisation applied at the edge:** routes and resolvers use dependencies and permission classes (see [ADR-011](adr/011-authorisation-at-the-edge.md)). - This means that services should not apply authorisation rules themselves. 5. **No `if settings.is_local:` outside the composition root:** Local mode is selected by composition, not conditionals throughout the codebase (see [ADR-008](adr/008-local-mode-composition.md)). @@ -64,7 +64,6 @@ Note that these are currently only enforced by convention/review. Import linter/ !!! warning "Current issues" - - `auth` <--> `metadata` import cycle - `shared/gql.py`'s import-everything role - Function-body imports working around circular imports - Imports 'across' many modules and their files diff --git a/api/src/damnit_api/auth/models.py b/api/src/damnit_api/auth/models.py index de2976c8..47182a4d 100644 --- a/api/src/damnit_api/auth/models.py +++ b/api/src/damnit_api/auth/models.py @@ -8,6 +8,7 @@ from .. import get_logger from .._db.dependencies import DBSession from .._mymdc.dependencies import MyMdCClient +from ..shared.errors import UnauthenticatedError from ..shared.models import ProposalNumber logger = get_logger() @@ -47,7 +48,7 @@ def from_connection(cls, connection: ASGIConnection) -> Self: if settings.is_local: return DEV_USER # type: ignore[return-value] msg = "No user info in session" - raise ValueError(msg) + raise UnauthenticatedError(msg) return cls.model_validate(user_dict) diff --git a/api/src/damnit_api/auth/policy.py b/api/src/damnit_api/auth/policy.py new file mode 100644 index 00000000..95ffc21a --- /dev/null +++ b/api/src/damnit_api/auth/policy.py @@ -0,0 +1,67 @@ +"""Proposal-membership authorization policy (ADR-011). + +Authorization is enforced only at the edges: Strawberry permission classes +for GraphQL fields and a Litestar guard for REST routes, both delegating to +`require_proposal_member`. Domain services take plain parameters and perform +no authorization. +""" + +from typing import TYPE_CHECKING + +from .. import get_logger +from ..shared.errors import ForbiddenError +from ..shared.models import ProposalNumber +from . import models + +if TYPE_CHECKING: + from litestar.connection import ASGIConnection + from litestar.handlers.base import BaseRouteHandler + +logger = get_logger() + + +async def require_proposal_member( + user: "models.User", + proposal_number: ProposalNumber, +) -> None: + """Raise `ForbiddenError` unless `user` is a member of the proposal.""" + from ..shared.settings import settings + + if settings.is_local: + return + + if proposal_number not in user.proposals: + msg = ( + f"User not authorised for proposal {proposal_number}, or proposal does not " + "exist." + ) + await logger.ainfo("Forbidden", message=msg) + raise ForbiddenError(msg) + + +async def proposal_member_guard( + connection: "ASGIConnection", + _: "BaseRouteHandler", +) -> None: + """Litestar guard enforcing proposal membership on REST routes. + + Attached to the proposal-scoped routers in the composition root (ADR-008), + so the slices themselves stay authorization-free. Fails closed if a guarded + route carries no `proposal_number`. + """ + raw = connection.path_params.get("proposal_number") or connection.query_params.get( + "proposal_number" + ) + if raw is None: + # A guarded route without the parameter is a wiring bug; fail closed. + msg = "Route requires proposal membership but has no proposal_number" + raise ForbiddenError(msg) + proposal_number = ProposalNumber(int(raw)) + + app_state = connection.app.state.app_state + async with app_state.db_sessionmaker() as session: + user = await models.User.from_connection( + connection, app_state.mymdc_client, session + ) + + await require_proposal_member(user, proposal_number) diff --git a/api/src/damnit_api/graphql/queries.py b/api/src/damnit_api/graphql/queries.py index 67ff590b..045441a2 100644 --- a/api/src/damnit_api/graphql/queries.py +++ b/api/src/damnit_api/graphql/queries.py @@ -6,10 +6,10 @@ from strawberry.types.nodes import SelectedField from .. import get_logger -from ..auth.permissions import PROPOSAL_PERMISSIONS from ..metadata.services import _get_proposal_meta, _update_proposal_meta from ..runs.types import DamnitRun from ..shared.models import ProposalNumber +from ..shared.permissions import PROPOSAL_PERMISSIONS from .utils import DatabaseInput logger = get_logger() diff --git a/api/src/damnit_api/graphql/subscriptions.py b/api/src/damnit_api/graphql/subscriptions.py index 01d6cf82..8608f82e 100644 --- a/api/src/damnit_api/graphql/subscriptions.py +++ b/api/src/damnit_api/graphql/subscriptions.py @@ -7,9 +7,9 @@ from strawberry.types import Info from .. import get_logger -from ..auth.permissions import PROPOSAL_PERMISSIONS from ..runs.types import Timestamp from ..shared.errors import DataUnavailableError +from ..shared.permissions import PROPOSAL_PERMISSIONS from .publisher import proposal_channel from .utils import DatabaseInput diff --git a/api/src/damnit_api/main.py b/api/src/damnit_api/main.py index d7b83554..e8a1ed48 100644 --- a/api/src/damnit_api/main.py +++ b/api/src/damnit_api/main.py @@ -1,12 +1,13 @@ from contextlib import asynccontextmanager -from litestar import Litestar +from litestar import Litestar, Router from litestar.di import Provide from litestar.exceptions import HTTPException from . import contextfile, metadata from ._mymdc.dependencies import get_mymdc_client -from .auth.dependencies import get_oauth_user_info, get_user +from .auth.dependencies import get_oauth_user_info +from .auth.policy import proposal_member_guard # Known paths are redirected to the login page after a 401. KNOWN_PATHS = ["/graphql"] @@ -172,13 +173,23 @@ def _file_store(name: str) -> FileStore: auth_controller = auth.OAuthController stores = StoreRegistry(default_factory=_file_store) + # Proposal-membership authorization is enforced at the REST edge (ADR-011) + # by a single guard on the proposal-scoped routers; local mode composes it + # out (ADR-008), so the slices themselves stay authorization-free. + proposal_guards = [] if settings.is_local else [proposal_member_guard] + # ── GraphQL controller ──────────────────────────────────────────────────── gql_controller = get_gql_controller() + proposal_router = Router( + path="", + route_handlers=[metadata.router, contextfile.router], + guards=proposal_guards, + ) + return Litestar( route_handlers=[ - metadata.router, - contextfile.router, + proposal_router, auth_controller, gql_controller, ], @@ -191,7 +202,6 @@ def _file_store(name: str) -> FileStore: ), # The `session` dependency comes from the Advanced Alchemy plugin. "mymdc": Provide(get_mymdc_client, sync_to_thread=False), - "user": Provide(get_user), "oauth_user": Provide(get_oauth_user_info, sync_to_thread=False), "channels": Provide(get_channels, sync_to_thread=False), "run_update_publisher": Provide( diff --git a/api/src/damnit_api/metadata/gql.py b/api/src/damnit_api/metadata/gql.py index 43ab2e98..39436c39 100644 --- a/api/src/damnit_api/metadata/gql.py +++ b/api/src/damnit_api/metadata/gql.py @@ -8,10 +8,10 @@ import strawberry.experimental.pydantic as st_pydantic from .. import get_logger -from ..auth.permissions import IsAuthenticated from ..shared.models import ( ProposalNumber, # noqa: TC001 (Strawberry resolves at runtime) ) +from ..shared.permissions import IsAuthenticated from . import models, services if TYPE_CHECKING: diff --git a/api/src/damnit_api/metadata/routers.py b/api/src/damnit_api/metadata/routers.py index 28891ebd..1dbeb5fc 100644 --- a/api/src/damnit_api/metadata/routers.py +++ b/api/src/damnit_api/metadata/routers.py @@ -5,7 +5,6 @@ from sqlalchemy.ext.asyncio import AsyncSession from .._mymdc.dependencies import MyMdCClient -from ..auth.models import User from ..shared.models import ProposalNumber from . import services from .models import ProposalMeta @@ -14,11 +13,10 @@ async def get_proposal_meta( proposal_number: ProposalNumber, mymdc: MyMdCClient, - user: User, session: AsyncSession, ) -> ProposalMeta: """Dependency: resolve ProposalMeta from path/query parameter.""" - return await services.get_proposal_meta(mymdc, proposal_number, user, session) + return await services.get_proposal_meta(mymdc, proposal_number, session) @get("/proposal/{proposal_number:int}", sync_to_thread=False) diff --git a/api/src/damnit_api/metadata/services.py b/api/src/damnit_api/metadata/services.py index acbc017d..1ba7ac84 100644 --- a/api/src/damnit_api/metadata/services.py +++ b/api/src/damnit_api/metadata/services.py @@ -9,7 +9,6 @@ from anyio import Path as APath from .. import get_logger -from ..shared.errors import ForbiddenError from ..shared.models import ProposalNumber from .models import ProposalMeta, ProposalMetaBase from .repository import ProposalMetaRepository @@ -19,7 +18,6 @@ if TYPE_CHECKING: from .._db.dependencies import DBSession from .._mymdc.clients import MyMdCClient - from ..auth.dependencies import User from ..runs.repository import DamnitRepositoryRegistry @@ -162,30 +160,6 @@ async def _search_damnit_dir(path: Path) -> tuple[Path | None, list[Path]]: return None, searched_paths -async def _check_user_allowed( - proposal_number: ProposalNumber, - user: "User", -) -> None: - """Check if the user is allowed to access the given proposal number. - - Raises `ForbiddenError` if not allowed. - """ - from ..shared.settings import settings - - if settings.is_local: - return - - if proposal_number not in user.proposals: - msg = ( - f"User not authorised for proposal {proposal_number}, or proposal does not " - "exist." - ) - await logger.ainfo("Forbidden", message=msg) - raise ForbiddenError(msg) - - return - - async def _get_proposal_meta( client: "MyMdCClient", proposal_number: ProposalNumber, @@ -255,7 +229,6 @@ async def _get_proposal_meta_many( async def get_proposal_meta( client: "MyMdCClient", proposal_number: ProposalNumber, - user: "User", session: "DBSession", ) -> ProposalMeta: """Get proposal metadata by proposal number, using the repository and/or provided @@ -265,8 +238,6 @@ async def get_proposal_meta( if settings.is_local: return _local_proposal_meta(proposal_number) - await _check_user_allowed(proposal_number, user) - return await _get_proposal_meta(client, proposal_number, session) @@ -286,29 +257,20 @@ async def _update_proposal_meta( async def update_proposal_meta( client: "MyMdCClient", proposal_number: ProposalNumber, - user: "User", session: "DBSession", ) -> ProposalMeta: """Get proposal metadata by proposal number, using the repository and/or provided MyMdC Client.""" - - await _check_user_allowed(proposal_number, user) - return await _update_proposal_meta(client, proposal_number, session) async def update_proposal_meta_many( client: "MyMdCClient", proposal_numbers: list[ProposalNumber], - user: "User", session: "DBSession", ) -> list[ProposalMeta]: """Get proposal metadata by proposal number, using the repository and/or provided MyMdC Client.""" - - for proposal_number in proposal_numbers: - await _check_user_allowed(proposal_number, user) - results = [] for chunk in _chunks(proposal_numbers, n=10): new_fetched = await asyncio.gather( diff --git a/api/src/damnit_api/auth/permissions.py b/api/src/damnit_api/shared/permissions.py similarity index 77% rename from api/src/damnit_api/auth/permissions.py rename to api/src/damnit_api/shared/permissions.py index 8938ae46..14eff07f 100644 --- a/api/src/damnit_api/auth/permissions.py +++ b/api/src/damnit_api/shared/permissions.py @@ -1,12 +1,18 @@ -"""Strawberry permissions for GraphQL authorization.""" +"""Strawberry permissions: transport adapters over the auth policy (ADR-011). + +Lives in `shared/` so any slice's GraphQL contribution can attach them without +importing the `auth` slice (which the import-direction rules forbid). The +membership decision itself lives in `auth/policy.py`; these classes only adapt +it to the Strawberry field-permission protocol. +""" from strawberry.exceptions import StrawberryGraphQLError from strawberry.permission import BasePermission from strawberry.types import Info from .. import get_logger -from ..metadata.services import _check_user_allowed -from ..shared.errors import ForbiddenError +from ..auth.policy import require_proposal_member +from .errors import ForbiddenError logger = get_logger() @@ -46,7 +52,7 @@ async def has_permission(self, source, info: Info, **kwargs) -> bool: raise StrawberryGraphQLError(msg) from exc try: - await _check_user_allowed(proposal, user) + await require_proposal_member(user, proposal) return True except ForbiddenError: return False diff --git a/api/tests/graphql/conftest.py b/api/tests/graphql/conftest.py index 726bd026..c6bb64cf 100644 --- a/api/tests/graphql/conftest.py +++ b/api/tests/graphql/conftest.py @@ -60,12 +60,12 @@ def make_publisher(channels_plugin, repositories, **kwargs): def _patch_permissions(mocker, *, authenticated: bool, member: bool) -> None: mocker.patch( - "damnit_api.auth.permissions.IsAuthenticated.has_permission", + "damnit_api.shared.permissions.IsAuthenticated.has_permission", new_callable=mocker.AsyncMock, return_value=authenticated, ) mocker.patch( - "damnit_api.auth.permissions.IsProposalMember.has_permission", + "damnit_api.shared.permissions.IsProposalMember.has_permission", new_callable=mocker.AsyncMock, return_value=member, ) diff --git a/api/tests/graphql/test_permissions.py b/api/tests/graphql/test_permissions.py index c309dad4..3770d8f8 100644 --- a/api/tests/graphql/test_permissions.py +++ b/api/tests/graphql/test_permissions.py @@ -11,9 +11,9 @@ import pytest from strawberry.exceptions import StrawberryGraphQLError -from damnit_api.auth.permissions import IsAuthenticated, IsProposalMember from damnit_api.shared.errors import ForbiddenError from damnit_api.shared.models import ProposalNumber +from damnit_api.shared.permissions import IsAuthenticated, IsProposalMember def _info(context: Any) -> Any: @@ -72,7 +72,7 @@ async def test_is_proposal_member_none_proposal(): @pytest.mark.asyncio async def test_is_proposal_member_allowed(mocker): check = mocker.patch( - "damnit_api.auth.permissions._check_user_allowed", + "damnit_api.shared.permissions.require_proposal_member", new_callable=mocker.AsyncMock, ) ctx = _context(user="resolved-user") @@ -83,7 +83,7 @@ async def test_is_proposal_member_allowed(mocker): ) assert result is True ctx.get_user.assert_awaited_once() - check.assert_awaited_once_with(ProposalNumber(1234), "resolved-user") + check.assert_awaited_once_with("resolved-user", ProposalNumber(1234)) @pytest.mark.asyncio @@ -103,7 +103,7 @@ async def test_is_proposal_member_safe_upstream_error(): @pytest.mark.asyncio async def test_is_proposal_member_forbidden(mocker): mocker.patch( - "damnit_api.auth.permissions._check_user_allowed", + "damnit_api.shared.permissions.require_proposal_member", new_callable=mocker.AsyncMock, side_effect=ForbiddenError("nope"), ) diff --git a/api/tests/refactor/e2e/test_authz_parity.py b/api/tests/refactor/e2e/test_authz_parity.py index 432ca271..05c39b63 100644 --- a/api/tests/refactor/e2e/test_authz_parity.py +++ b/api/tests/refactor/e2e/test_authz_parity.py @@ -78,16 +78,11 @@ async def test_member_passes_authorization_unchanged(logged_in_client): async def test_graphql_query_without_session_rejected(e2e_client): - """An unauthenticated GraphQL request is rejected, not served. + """An unauthenticated GraphQL request is rejected with 401, not served. - The session lookup raises `ValueError` ("No user info in session") while - building the GraphQL context. Litestar's exception handling turns that into - a 500 response; under FastAPI the same error propagated unhandled through the - raw ASGI transport instead. - - !!! todo - - This should become a proper 401 error; update this test when it does. + The session lookup raises `UnauthenticatedError` ("No user info in + session") while building the GraphQL context; the DamnitWebError handler turns + that into a clean 401 the SPA can act on (redirect to login). """ response = await e2e_client.post("/graphql", json=runs_query(MEMBER_PROPOSAL)) - assert response.status_code == 500 + assert response.status_code == 401 diff --git a/api/tests/test_auth_policy.py b/api/tests/test_auth_policy.py new file mode 100644 index 00000000..11d6321b --- /dev/null +++ b/api/tests/test_auth_policy.py @@ -0,0 +1,109 @@ +"""Tests for the proposal-membership policy and its REST edge guard.""" + +from contextlib import asynccontextmanager +from types import SimpleNamespace +from typing import TYPE_CHECKING, cast + +import pytest +from litestar.connection import ASGIConnection + +from damnit_api.auth.models import User +from damnit_api.auth.policy import proposal_member_guard, require_proposal_member +from damnit_api.shared.errors import ForbiddenError, UnauthenticatedError +from damnit_api.shared.models import ProposalNumber + +if TYPE_CHECKING: + from litestar.handlers.base import BaseRouteHandler + +# Sync test-client files earlier in the collection order close the +# session-scoped loop; run on per-test loops instead. +pytestmark = pytest.mark.asyncio(loop_scope="function") + +# The guard's second argument is unused; a typed placeholder keeps the calls +# type-correct without constructing a real route handler. +_HANDLER = cast("BaseRouteHandler", None) + + +def _user(*proposals: int) -> User: + fake = SimpleNamespace(proposals=[ProposalNumber(p) for p in proposals]) + return cast("User", fake) + + +# ----------------------------------------------------------------------------- +# require_proposal_member + + +async def test_member_is_allowed(): + await require_proposal_member(_user(1234), ProposalNumber(1234)) + + +async def test_non_member_is_forbidden(): + with pytest.raises(ForbiddenError): + await require_proposal_member(_user(1111), ProposalNumber(1234)) + + +async def test_local_mode_bypasses_membership(mocker): + from damnit_api.shared import settings as settings_module + + mocker.patch.object(settings_module.settings, "damnit_path", "/data/p1234") + assert settings_module.settings.is_local + await require_proposal_member(_user(), ProposalNumber(1234)) + + +# ----------------------------------------------------------------------------- +# proposal_member_guard + + +def _connection( + *, + path_params: dict | None = None, + query_params: dict | None = None, +) -> ASGIConnection: + @asynccontextmanager + async def sessionmaker(): # noqa: RUF029 + yield object() + + app_state = SimpleNamespace(mymdc_client=object(), db_sessionmaker=sessionmaker) + fake = SimpleNamespace( + path_params=path_params or {}, + query_params=query_params or {}, + app=SimpleNamespace(state=SimpleNamespace(app_state=app_state)), + ) + return cast("ASGIConnection", fake) + + +async def test_guard_forbids_route_without_proposal_number(): + with pytest.raises(ForbiddenError): + await proposal_member_guard(_connection(), _HANDLER) + + +async def test_guard_rejects_unauthenticated_connection(mocker): + mocker.patch( + "damnit_api.auth.models.User.from_connection", + new_callable=mocker.AsyncMock, + side_effect=UnauthenticatedError("No user info in session"), + ) + conn = _connection(path_params={"proposal_number": 1234}) + with pytest.raises(UnauthenticatedError): + await proposal_member_guard(conn, _HANDLER) + + +async def test_guard_allows_member_via_path_param(mocker): + mocker.patch( + "damnit_api.auth.models.User.from_connection", + new_callable=mocker.AsyncMock, + return_value=_user(1234), + ) + conn = _connection(path_params={"proposal_number": 1234}) + await proposal_member_guard(conn, _HANDLER) + + +async def test_guard_forbids_non_member_via_query_param(mocker): + mocker.patch( + "damnit_api.auth.models.User.from_connection", + new_callable=mocker.AsyncMock, + return_value=_user(1111), + ) + conn = _connection(query_params={"proposal_number": "1234"}) + with pytest.raises(ForbiddenError): + await proposal_member_guard(conn, _HANDLER) diff --git a/api/zensical.toml b/api/zensical.toml index a8b28bd9..3a4d62f0 100644 --- a/api/zensical.toml +++ b/api/zensical.toml @@ -21,6 +21,8 @@ nav = [ "adr/007-graphql-transport-only.md", "adr/008-local-mode-composition.md", "adr/009-channels-subscriptions.md", + "adr/010-two-databases.md", + "adr/011-authorisation-at-the-edge.md", ]} ] }, { "Development" = [ diff --git a/frontend/packages/ui/src/app/pages/hero-page.tsx b/frontend/packages/ui/src/app/pages/hero-page.tsx index 720482f8..a3d307f1 100644 --- a/frontend/packages/ui/src/app/pages/hero-page.tsx +++ b/frontend/packages/ui/src/app/pages/hero-page.tsx @@ -1,7 +1,7 @@ import { useEffect } from 'react' import { Container, Title } from '@mantine/core' -import { history } from '#src/app/routes/history' +import { history } from '#src/lib/history' import classes from './hero-page.module.css' diff --git a/frontend/packages/ui/src/app/pages/logged-out-page.tsx b/frontend/packages/ui/src/app/pages/logged-out-page.tsx index 59e886d5..aa9a3cd2 100644 --- a/frontend/packages/ui/src/app/pages/logged-out-page.tsx +++ b/frontend/packages/ui/src/app/pages/logged-out-page.tsx @@ -1,7 +1,7 @@ import { Container, Text, Title } from '@mantine/core' import useUserInfo from '#src/features/auth/use-user-info' -import { history } from '#src/app/routes/history' +import { history } from '#src/lib/history' import MainNavButton from '#src/components/buttons/main-nav-button' import classes from './logged-out-page.module.css' diff --git a/frontend/packages/ui/src/app/pages/not-found-page.tsx b/frontend/packages/ui/src/app/pages/not-found-page.tsx index 26b5a45a..0f552347 100644 --- a/frontend/packages/ui/src/app/pages/not-found-page.tsx +++ b/frontend/packages/ui/src/app/pages/not-found-page.tsx @@ -1,6 +1,6 @@ import { Container, Title } from '@mantine/core' -import { history } from '#src/app/routes/history' +import { history } from '#src/lib/history' import MainNavButton from '#src/components/buttons/main-nav-button' import styles from './not-found-page.module.css' diff --git a/frontend/packages/ui/src/app/routes/private-route.tsx b/frontend/packages/ui/src/app/routes/private-route.tsx index 281eb7a9..c3861a45 100644 --- a/frontend/packages/ui/src/app/routes/private-route.tsx +++ b/frontend/packages/ui/src/app/routes/private-route.tsx @@ -2,8 +2,8 @@ import { type PropsWithChildren } from 'react' import { Navigate } from 'react-router' import useUserInfo from '#src/features/auth/use-user-info' +import { history } from '#src/lib/history' -import { history } from './history' function PrivateRoute({ children }: PropsWithChildren) { const { userInfo, isLoading, isError } = useUserInfo() diff --git a/frontend/packages/ui/src/graphql/apollo.ts b/frontend/packages/ui/src/graphql/apollo.ts index 03329907..13c6a947 100644 --- a/frontend/packages/ui/src/graphql/apollo.ts +++ b/frontend/packages/ui/src/graphql/apollo.ts @@ -11,6 +11,7 @@ import { type Operation, type FetchResult, } from '@apollo/client' +import { onError } from '@apollo/client/link/error' import { removeTypenameFromVariables, KEEP, @@ -21,6 +22,7 @@ import { getMainDefinition } from '@apollo/client/utilities' import { createClient } from 'graphql-ws' import { BASE_URL, WS_URL } from '#src/constants' +import { history } from '#src/lib/history' import { DEFERRED_TABLE_DATA_QUERY_NAME } from './operation-names' @@ -35,6 +37,25 @@ const retryLink = new RetryLink({ initial: 1000, max: 1000, }, + attempts: { + max: 5, + // Don't retry unauthenticated responses; surface them so `errorLink` + // can redirect to login instead of hammering the API. + retryIf: (error) => !!error && error.statusCode !== 401, + }, +}) + +// Redirect to the login route when the API reports the session is missing or +// invalid (401), e.g. an expired session mid-use. Route-level auth gating is +// handled separately by PrivateRoute (via /oauth/userinfo). +const errorLink = onError(({ networkError }) => { + if ( + networkError && + 'statusCode' in networkError && + networkError.statusCode === 401 + ) { + history.navigate('/login') + } }) const httpLink = new HttpLink({ uri: `${BASE_URL}graphql` }) @@ -133,5 +154,5 @@ export const cache = new InMemoryCache() export const client = new ApolloClient({ cache, - link: from([retryLink, removeTypenameLink, splitLink]), + link: from([errorLink, retryLink, removeTypenameLink, splitLink]), }) diff --git a/frontend/packages/ui/src/index.ts b/frontend/packages/ui/src/index.ts index 3b4097b5..af2d922b 100644 --- a/frontend/packages/ui/src/index.ts +++ b/frontend/packages/ui/src/index.ts @@ -35,7 +35,7 @@ export { default as LoginRoute } from './app/routes/login-route' export { default as LogoutRoute } from './app/routes/logout-route' export { default as PrivateRoute } from './app/routes/private-route' export { default as RootRoute } from './app/routes/root-route' -export { history } from './app/routes/history' +export { history } from './lib/history' // Data export { default as useProposal } from './data/use-proposal' diff --git a/frontend/packages/ui/src/app/routes/history.ts b/frontend/packages/ui/src/lib/history.ts similarity index 100% rename from frontend/packages/ui/src/app/routes/history.ts rename to frontend/packages/ui/src/lib/history.ts