Report permissions on project writes; throttle API-key last_used_at

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
 

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

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,21 +323,25 @@ 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.
 
     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

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
 

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)

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