diff --git a/api/auth/deps.py b/api/auth/deps.py index 286322dd..49ca8f21 100644 --- a/api/auth/deps.py +++ b/api/auth/deps.py @@ -1,7 +1,7 @@ """ Authentication dependencies for protecting endpoints """ -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone from typing import Annotated import uuid @@ -15,6 +15,12 @@ from api.auth.models import User, APIKey from api.auth.services import ensure_timezone_aware +# How stale api_keys.last_used_at may get before a request refreshes it. The +# column answers "is this key still in use", which is judged in days, so +# minute precision buys nothing -- and writing it on every request made each +# API-key call a database write and commit, however read-only the call itself. +API_KEY_TOUCH_INTERVAL = timedelta(minutes=5) + # OAuth2 scheme for token extraction oauth2_scheme = OAuth2PasswordBearer(tokenUrl="/api/v1/auth/login") @@ -53,10 +59,13 @@ def authenticate_api_key(session: Session, token: str) -> User: if user is None: raise credentials_exception - # Update last_used_at - api_key.last_used_at = datetime.now(timezone.utc) - session.add(api_key) - session.commit() + # Refresh last_used_at at most once per API_KEY_TOUCH_INTERVAL. + now = datetime.now(timezone.utc) + last_used = ensure_timezone_aware(api_key.last_used_at) + if last_used is None or now - last_used >= API_KEY_TOUCH_INTERVAL: + api_key.last_used_at = now + session.add(api_key) + session.commit() return user diff --git a/api/project/models.py b/api/project/models.py index 9eae6c77..2f288738 100755 --- a/api/project/models.py +++ b/api/project/models.py @@ -110,14 +110,14 @@ class ProjectPublic(SQLModel): # for the same project_id. This response must therefore never go into a # shared or CDN cache. # - # Populated on the detail route only, and left None by the list routes, the - # same way sequencing_runs is. Resolving it costs a membership lookup per + # Populated on the detail, update and patch routes, and left None by the + # list routes, the same way sequencing_runs is. Resolving it costs a membership lookup per # project, which on a page of projects is an N+1 for something no list view # needs -- there are no per-row actions to gate. # # None is not the same as []. None means the field was not populated -- the - # list, create, update and patch routes all return this model without - # resolving it; [] means this caller genuinely holds nothing here. + # list and create routes return this model without resolving it; [] means + # this caller genuinely holds nothing here. # # None once also covered an anonymous caller. It no longer can: the detail # route requires a caller and then project:read, so a request that reaches diff --git a/api/project/routes.py b/api/project/routes.py index 5ffd8474..51e2dcd7 100755 --- a/api/project/routes.py +++ b/api/project/routes.py @@ -22,7 +22,9 @@ require_project_permission, ) from api.rbac.permissions import Permission +from api.rbac.resolver import AuthzContext from api.project.models import ( + Project, ProjectCreate, ProjectUpdate, ProjectPublic, @@ -287,6 +289,26 @@ def get_project_by_project_id( return result +def _with_permissions( + result: ProjectPublic, project: Project, authz: AuthzContext +) -> ProjectPublic: + """ + Report the caller's permissions on a project they have just written. + + Without this a UI that saves a project gets `permissions: null` back and has + to re-read the detail route to keep its controls right -- and a save can + change the answer, because `restricted` is one of the fields it writes. + + So restriction is read from the result, not from the project as loaded + before the write. A non-member administrator who locks a project has, in + the same request, stopped being able to act on it, and the response says so. + """ + result.permissions = authz.effective_project_permissions( + project.id, restricted=result.restricted + ) + return result + + @router.put( "/{project_id}", status_code=status.HTTP_200_OK, @@ -301,7 +323,8 @@ def update_project( session: SessionDep, opensearch_client: OpenSearchDep, project: ProjectDep, - update_request: ProjectUpdate + update_request: ProjectUpdate, + authz: AuthzDep, ) -> ProjectPublic: """ Full replacement update of a project. @@ -309,13 +332,16 @@ def update_project( Attributes provided here **replace** all existing attributes. To merge/upsert attributes without removing unmentioned ones, use ``PATCH /{project_id}`` instead. + + Carries `permissions`, as the detail route does; see _with_permissions. """ - return services.update_project( + result = services.update_project( session=session, opensearch_client=opensearch_client, project_id=project.project_id, update_request=update_request, ) + return _with_permissions(result, project, authz) @router.patch( @@ -333,6 +359,7 @@ def patch_project( opensearch_client: OpenSearchDep, project: ProjectDep, update_request: ProjectUpdate, + authz: AuthzDep, ) -> ProjectPublic: """ Partially update a project using merge/upsert semantics. @@ -341,13 +368,16 @@ def patch_project( from the request. Each supplied attribute is upserted: existing keys are updated, new keys are inserted, and unmentioned keys are left untouched. An empty attributes list is a no-op. + + Carries `permissions`, as the detail route does; see _with_permissions. """ - return services.patch_project( + result = services.patch_project( session=session, opensearch_client=opensearch_client, project_id=project.project_id, update_request=update_request, ) + return _with_permissions(result, project, authz) ############################################################################### # "Samples" Endpoints /api/v1/projects/{project_id}/samples diff --git a/docs/RBAC.md b/docs/RBAC.md index 4a00bd01..16e0cb64 100644 --- a/docs/RBAC.md +++ b/docs/RBAC.md @@ -1054,7 +1054,7 @@ Both take `role:read` rather than `user:read`: the latter is the directory-searc `None` and `[]` are different facts, and the distinction is worth keeping straight: -- **`None` means the field was not populated.** Only the detail route resolves it. The list, create, update and patch routes all return `ProjectPublic` without it, the same way they leave `sequencing_runs` unset — resolving it costs a membership lookup per project, which on a page of projects is an N+1 for something no list view needs, since there are no per-row controls to gate. One consequence for a UI: after a successful `PUT`/`PATCH` the field comes back `None`, so re-read the detail route rather than treating the response as a refreshed permission set. +- **`None` means the field was not populated.** The detail, `PUT` and `PATCH` routes resolve it. The list and create routes return `ProjectPublic` without it, the same way they leave `sequencing_runs` unset — resolving it costs a membership lookup per project, which on a page of projects is an N+1 for something no list view needs, since there are no per-row controls to gate. A `PUT`/`PATCH` response is a refreshed permission set, and it reflects the write: restriction is read from the saved project, so locking a project you are not a member of comes back with only the view permissions. - **`[]` means a real caller was evaluated and holds nothing here.** `None` once also covered an anonymous request, and the field was built on `OptionalAuthzDep` for that reason. It no longer can: every feature router requires a caller, and the detail route additionally requires `project:read`, so a request that reaches the handler always has somebody to report on. The optional dependency is kept rather than folded into `load_authz`, because collapsing it is a change to the route and not to the helper — its docstring records the same thing. @@ -1677,7 +1677,7 @@ Consequences for the rest of this design: MCP needs no service account, so `serv Two notes that will otherwise cost someone an afternoon, for whichever machine identities do end up existing: - A service-account user must be created with **both** `is_active` and `is_verified` true, or its API key fails on every route using `get_current_active_user`. -- `authenticate_api_key` writes `last_used_at` and **commits on every request** (`api/auth/deps.py:57`). That is write amplification independent of RBAC and deserves its own ticket — throttle to once per N minutes. +- ~~`authenticate_api_key` writes `last_used_at` and **commits on every request**~~. Throttled: the column is refreshed at most once per `API_KEY_TOUCH_INTERVAL` (5 minutes) per key, which is far finer than the days-scale idle judgements it feeds. ### Frontend diff --git a/tests/api/test_api_keys.py b/tests/api/test_api_keys.py index 83621756..efb76764 100644 --- a/tests/api/test_api_keys.py +++ b/tests/api/test_api_keys.py @@ -8,8 +8,10 @@ from fastapi.testclient import TestClient from sqlmodel import Session +from api.auth.deps import API_KEY_TOUCH_INTERVAL, authenticate_api_key from api.auth.models import User, APIKey -from core.security import hash_password +from api.auth.services import ensure_timezone_aware +from core.security import hash_api_key, hash_password @pytest.fixture(name="test_user") @@ -198,6 +200,40 @@ def test_last_used_at_updated( api_key = session.get(APIKey, uuid.UUID(key_id)) assert api_key.last_used_at is not None + def test_last_used_at_is_not_rewritten_within_the_interval( + self, test_user, session: Session + ): + """A recent last_used_at is left alone, so a busy key is not a write per call.""" + raw_key, recent = self._key_used_at( + session, test_user, datetime.now(timezone.utc) - timedelta(minutes=1) + ) + authenticate_api_key(session, raw_key) + session.refresh(recent) + assert ensure_timezone_aware(recent.last_used_at) < ( + datetime.now(timezone.utc) - timedelta(seconds=30) + ) + + def test_last_used_at_is_refreshed_once_stale(self, test_user, session: Session): + stale_at = datetime.now(timezone.utc) - API_KEY_TOUCH_INTERVAL - timedelta(minutes=1) + raw_key, stale = self._key_used_at(session, test_user, stale_at) + authenticate_api_key(session, raw_key) + session.refresh(stale) + assert ensure_timezone_aware(stale.last_used_at) > stale_at + timedelta(minutes=1) + + @staticmethod + def _key_used_at(session: Session, user, last_used_at): + raw_key = "ngs360_" + uuid.uuid4().hex + api_key = APIKey( + user_id=user.id, + name="Throttled Key", + key_prefix=raw_key[:12], + hashed_key=hash_api_key(raw_key), + last_used_at=last_used_at, + ) + session.add(api_key) + session.commit() + return raw_key, api_key + def test_revoked_key_returns_401(self, unauthenticated_client: TestClient, test_user): """Revoking a key then using it for auth returns 401.""" jwt_token = _login(unauthenticated_client, test_user.email) diff --git a/tests/api/test_rbac_admin.py b/tests/api/test_rbac_admin.py index 28ad4ab1..82cef603 100644 --- a/tests/api/test_rbac_admin.py +++ b/tests/api/test_rbac_admin.py @@ -393,14 +393,46 @@ def test_an_anonymous_caller_is_refused_outright( any more: every feature router requires a caller, so an anonymous request is 401 at the authentication layer before the project:read guard is even reached, and never gets a body to carry either answer. - The field stays optional regardless, because the create, update and - patch routes return ProjectPublic without ever populating it. + The field stays optional regardless, because the list and create + routes return ProjectPublic without ever populating it. """ response = unauthenticated_client.get( f"/api/v1/projects/{test_project.project_id}" ) assert response.status_code == 401 + @pytest.mark.parametrize("method", ["put", "patch"]) + def test_a_write_reports_it_too(self, method, client, test_project): + """ + PUT and PATCH answer with the same field, so a UI that saves a project + does not have to re-read it to keep its controls right. + """ + body = getattr(client, method)( + f"/api/v1/projects/{test_project.project_id}", json={"name": "Renamed"} + ).json() + assert body["name"] == "Renamed" + assert "sample:create" in body["permissions"] + + @pytest.mark.parametrize("method", ["put", "patch"]) + def test_a_write_that_restricts_reports_the_restricted_answer( + self, method, superuser_client, session, test_project + ): + """ + Restriction is read from the saved project, not the one loaded before + the write. A non-member administrator who locks a project can no + longer act on it, and the response must not offer them the controls. + """ + assert session.exec( + select(ProjectMember).where(ProjectMember.project_id == test_project.id) + ).first() is None + + body = getattr(superuser_client, method)( + f"/api/v1/projects/{test_project.project_id}", json={"restricted": True} + ).json() + assert body["restricted"] is True + assert body["permissions"] == ["project:manage_members", "project:read", + "sample:read"] + def test_the_list_route_does_not_populate_it(self, superuser_client, test_project): """ Left None on the list, like sequencing_runs: resolving it per row is an