diff --git a/app/api/routes_apikeys.py b/app/api/routes_apikeys.py index 551da51..4884f3e 100644 --- a/app/api/routes_apikeys.py +++ b/app/api/routes_apikeys.py @@ -9,7 +9,7 @@ from app.db.models import ApiKey, OrganizationMembership from app.db.session import get_db from app.rbac import get_current_user, require_org_permission_or_platform_admin -from app.schemas.apikeys import ApiKeyCreate, ApiKeyCreated, ApiKeyExchangeIn, ApiKeyExchangeOut, ApiKeyOut +from app.schemas.apikeys import ApiKeyCreate, ApiKeyCreated, ApiKeyExchangeIn, ApiKeyExchangeOut, ApiKeyOut, ApiKeyRename from app.services import apikey_service, org_service router = APIRouter(prefix="/orgs/{org_id}/api-keys", tags=["api-keys"]) @@ -42,7 +42,8 @@ def create_api_key( caller_permissions = org_service.permissions_for_membership(membership) try: api_key, full_key = apikey_service.create_api_key( - db, org_id, membership.user_id, body.name, body.scopes, caller_permissions + db, org_id, membership.user_id, body.name, body.scopes, caller_permissions, + expires_at=body.expires_at, ) except ValueError as e: raise HTTPException(400, str(e)) @@ -51,6 +52,7 @@ def create_api_key( name=api_key.name, key_prefix=api_key.key_prefix, scopes=api_key.scopes or [], + expires_at=api_key.expires_at, key=full_key, ) @@ -64,6 +66,24 @@ def list_api_keys( return [_key_out(k) for k in apikey_service.list_api_keys(db, org_id)] +@router.patch("/{key_id}", response_model=ApiKeyOut) +def rename_api_key( + org_id: int, + key_id: int, + body: ApiKeyRename, + db: Session = Depends(get_db), + membership: OrganizationMembership = Depends(require_org_permission_or_platform_admin(MANAGE_API_KEYS)), +): + key = apikey_service.get_api_key(db, org_id, key_id) + if not key: + raise HTTPException(404, "API key not found") + try: + apikey_service.rename_api_key(db, key, body.name, actor_user_id=membership.user_id) + except ValueError as e: + raise HTTPException(400, str(e)) + return _key_out(key) + + @router.delete("/{key_id}", status_code=204) def revoke_api_key( org_id: int, @@ -120,9 +140,31 @@ def exchange_api_key( # Org admins keep the org-wide /orgs/{org_id}/api-keys routes above. # --------------------------------------------------------------------------- -DEFAULT_SELF_SERVICE_SCOPES = ["dataset.read"] MAX_ACTIVE_KEYS_PER_USER = 10 +# M9 (API-key lifecycle, design doc's "Scopes" section): the public, +# developer-facing scope vocabulary self-service keys are issued and +# displayed in -- distinct from the internal dot-format IAM permission +# names (app/core/permission_names.py's registry) that org roles are +# actually granted and that create_api_key/exchange_api_key check +# against. Translating at this HTTP boundary only, rather than renaming +# dataset.read/usage.read themselves, keeps both of those exactly as they +# are everywhere else they're used -- role grants, the org-admin +# /orgs/{org_id}/api-keys router below, and every downstream consumer of +# an exchanged token's `permissions` claim (the gateway and policy engine +# still see "dataset.read"/"usage.read", unchanged). +PUBLIC_SCOPE_TO_PERMISSION = { + "literature:read": "dataset.read", + "usage:read": "usage.read", +} +PERMISSION_TO_PUBLIC_SCOPE = {v: k for k, v in PUBLIC_SCOPE_TO_PERMISSION.items()} + +DEFAULT_SELF_SERVICE_SCOPES = ["literature:read"] + + +def _to_public_scopes(internal_scopes: list[str]) -> list[str]: + return [PERMISSION_TO_PUBLIC_SCOPE.get(s, s) for s in internal_scopes] + def _self_service_membership( user: dict = Depends(get_current_user), @@ -158,12 +200,25 @@ def _own_keys(db: Session, membership: OrganizationMembership): ) +def _me_key_out(key: ApiKey) -> ApiKeyOut: + return ApiKeyOut( + id=key.id, + name=key.name, + key_prefix=key.key_prefix, + scopes=_to_public_scopes(key.scopes or []), + status=key.status, + created_at=key.created_at, + expires_at=key.expires_at, + last_used_at=key.last_used_at, + ) + + @me_router.get("", response_model=list[ApiKeyOut]) def list_my_api_keys( db: Session = Depends(get_db), membership: OrganizationMembership = Depends(_self_service_membership), ): - return [_key_out(k) for k in _own_keys(db, membership).all()] + return [_me_key_out(k) for k in _own_keys(db, membership).all()] @me_router.post("", response_model=ApiKeyCreated, status_code=201) @@ -175,11 +230,16 @@ def create_my_api_key( active = _own_keys(db, membership).filter(ApiKey.status == "active").count() if active >= MAX_ACTIVE_KEYS_PER_USER: raise HTTPException(409, f"At most {MAX_ACTIVE_KEYS_PER_USER} active keys; revoke one first") - scopes = body.scopes or DEFAULT_SELF_SERVICE_SCOPES + public_scopes = body.scopes or DEFAULT_SELF_SERVICE_SCOPES + unknown_scopes = set(public_scopes) - set(PUBLIC_SCOPE_TO_PERMISSION) + if unknown_scopes: + raise HTTPException(400, f"Unknown scope(s): {sorted(unknown_scopes)}") + internal_scopes = [PUBLIC_SCOPE_TO_PERMISSION[s] for s in public_scopes] try: api_key, full_key = apikey_service.create_api_key( - db, membership.organization_id, membership.user_id, body.name, scopes, + db, membership.organization_id, membership.user_id, body.name, internal_scopes, org_service.permissions_for_membership(membership), + expires_at=body.expires_at, ) except ValueError as e: raise HTTPException(400, str(e)) @@ -187,11 +247,29 @@ def create_my_api_key( id=api_key.id, name=api_key.name, key_prefix=api_key.key_prefix, - scopes=api_key.scopes or [], + scopes=_to_public_scopes(api_key.scopes or []), + expires_at=api_key.expires_at, key=full_key, ) +@me_router.patch("/{key_id}", response_model=ApiKeyOut) +def rename_my_api_key( + key_id: int, + body: ApiKeyRename, + db: Session = Depends(get_db), + membership: OrganizationMembership = Depends(_self_service_membership), +): + key = _own_keys(db, membership).filter(ApiKey.id == key_id).first() + if not key: + raise HTTPException(404, "API key not found") + try: + apikey_service.rename_api_key(db, key, body.name, actor_user_id=membership.user_id) + except ValueError as e: + raise HTTPException(400, str(e)) + return _me_key_out(key) + + @me_router.delete("/{key_id}", status_code=204) def revoke_my_api_key( key_id: int, diff --git a/app/schemas/apikeys.py b/app/schemas/apikeys.py index 9b8ded7..973c237 100644 --- a/app/schemas/apikeys.py +++ b/app/schemas/apikeys.py @@ -6,6 +6,11 @@ class ApiKeyCreate(BaseModel): name: str scopes: list[str] = [] + expires_at: datetime | None = None # M9: optional, self-service-settable; None = no expiry + + +class ApiKeyRename(BaseModel): + name: str class ApiKeyCreated(BaseModel): @@ -13,6 +18,7 @@ class ApiKeyCreated(BaseModel): name: str | None key_prefix: str scopes: list[str] + expires_at: datetime | None = None key: str # full plaintext key -- returned exactly once, at creation diff --git a/app/services/apikey_service.py b/app/services/apikey_service.py index 3170b48..42395a0 100644 --- a/app/services/apikey_service.py +++ b/app/services/apikey_service.py @@ -1,7 +1,7 @@ import hashlib import secrets import uuid -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone from sqlalchemy.orm import Session @@ -24,6 +24,19 @@ def _hash_key(full_key: str) -> str: return hashlib.sha256(full_key.encode()).hexdigest() +def _normalize_expiry(expires_at: datetime | None) -> datetime | None: + """Every other timestamp on this model (created_at, last_used_at, + revoked_at) is a naive UTC datetime.utcnow(), and verify_api_key's own + expiry check compares against one -- so a caller-supplied, possibly + tz-aware expires_at is converted to the same naive-UTC shape here, + once, rather than every comparison site needing to handle both.""" + if expires_at is None: + return None + if expires_at.tzinfo is not None: + expires_at = expires_at.astimezone(timezone.utc).replace(tzinfo=None) + return expires_at + + def create_api_key( db: Session, organization_id: int, @@ -31,6 +44,7 @@ def create_api_key( name: str, scopes: list[str], caller_permissions: set[str], + expires_at: datetime | None = None, ) -> tuple[ApiKey, str]: """Returns (ApiKey row, full plaintext key). The plaintext is never persisted -- only its sha256 hash is stored -- so this is the only @@ -45,6 +59,10 @@ def create_api_key( if invalid_scopes: raise ValueError(f"Cannot grant scopes you don't hold: {sorted(invalid_scopes)}") + expires_at = _normalize_expiry(expires_at) + if expires_at is not None and expires_at <= datetime.utcnow(): + raise ValueError("expires_at must be in the future") + full_key = _generate_key() api_key = ApiKey( organization_id=organization_id, @@ -55,6 +73,7 @@ def create_api_key( scopes=scopes, status="active", created_at=datetime.utcnow(), + expires_at=expires_at, ) db.add(api_key) db.flush() @@ -107,6 +126,29 @@ def revoke_api_key( return api_key +def rename_api_key( + db: Session, api_key: ApiKey, new_name: str, actor_user_id: int | None = None, +) -> ApiKey: + """Change a key's display name only -- scopes, status, and the key + material itself are untouched, so this never needs the + caller_permissions re-check create_api_key does.""" + if not new_name or not new_name.strip(): + raise ValueError("name must not be empty") + old_name = api_key.name + api_key.name = new_name + db.flush() + db.refresh(api_key) + audit_service.log_event( + db, AuditEventType.API_KEY_RENAMED, actor_user_id=actor_user_id, + organization_id=api_key.organization_id, resource_type="api_key", resource_id=api_key.id, + before_state={"name": old_name}, after_state={"name": api_key.name}, + metadata={"old_name": old_name, "new_name": api_key.name}, + commit=False, + ) + db.commit() + return api_key + + def verify_api_key(db: Session, full_key: str) -> ApiKey | None: """Look up an active, unexpired key by the hash of its full value. Used by exchange_api_key() below (POST /auth/api-keys/exchange).""" diff --git a/app/services/audit_service.py b/app/services/audit_service.py index d7500ac..8516921 100644 --- a/app/services/audit_service.py +++ b/app/services/audit_service.py @@ -39,6 +39,10 @@ class AuditEventType: USER_DISABLED = "user_disabled" API_KEY_CREATED = "api_key_created" API_KEY_REVOKED = "api_key_revoked" + # M9 (API-key lifecycle): renaming a key's display name only -- scopes, + # status, and the key material itself are unaffected and keep their own + # existing event types. + API_KEY_RENAMED = "api_key_renamed" OAUTH_CLIENT_CREATED = "oauth_client_created" OAUTH_CLIENT_REVOKED = "oauth_client_revoked" SSO_CONFIGURATION_CREATED = "sso_configuration_created" diff --git a/tests/test_apikeys.py b/tests/test_apikeys.py index 5e33776..633ff1d 100644 --- a/tests/test_apikeys.py +++ b/tests/test_apikeys.py @@ -7,6 +7,7 @@ Developer: Manish Kumar """ import uuid +from datetime import datetime, timedelta import pytest from sqlalchemy import create_engine @@ -116,6 +117,69 @@ def test_revoke_api_key(client, org): assert revoked["status"] == "revoked" +def test_create_api_key_with_future_expires_at(client, org): + """A caller-supplied expires_at in the future is stored and returned as-is.""" + future = (datetime.utcnow() + timedelta(days=30)).isoformat() + resp = client.post( + f"/orgs/{org['id']}/api-keys", + json={"name": "Expiring", "scopes": [], "expires_at": future}, + headers=org["owner_headers"], + ) + assert resp.status_code == 201 + assert resp.json()["expires_at"] is not None + + +def test_create_api_key_rejects_past_expires_at(client, org): + """expires_at in the past is rejected -- a key that's already expired the moment it's created + is never a legitimate request. + """ + past = (datetime.utcnow() - timedelta(days=1)).isoformat() + resp = client.post( + f"/orgs/{org['id']}/api-keys", + json={"name": "Already expired", "scopes": [], "expires_at": past}, + headers=org["owner_headers"], + ) + assert resp.status_code == 400 + + +def test_rename_api_key(client, org): + """PATCH renames a key and the new name is reflected in a subsequent listing.""" + create = client.post( + f"/orgs/{org['id']}/api-keys", json={"name": "Old name", "scopes": []}, headers=org["owner_headers"] + ) + key_id = create.json()["id"] + + resp = client.patch( + f"/orgs/{org['id']}/api-keys/{key_id}", json={"name": "New name"}, headers=org["owner_headers"] + ) + assert resp.status_code == 200 + assert resp.json()["name"] == "New name" + + listed = client.get(f"/orgs/{org['id']}/api-keys", headers=org["owner_headers"]) + assert next(k for k in listed.json() if k["id"] == key_id)["name"] == "New name" + + +def test_rename_api_key_rejects_empty_name(client, org): + """An empty/whitespace-only name is rejected rather than silently blanking the key's name.""" + create = client.post( + f"/orgs/{org['id']}/api-keys", json={"name": "Keep me", "scopes": []}, headers=org["owner_headers"] + ) + key_id = create.json()["id"] + + resp = client.patch( + f"/orgs/{org['id']}/api-keys/{key_id}", json={"name": " "}, headers=org["owner_headers"] + ) + assert resp.status_code == 400 + + +def test_rename_nonexistent_key_404(client, org): + """Renaming a key id that doesn't exist in this org returns 404.""" + resp = client.patch( + f"/orgs/{org['id']}/api-keys/999999", json={"name": "Nope"}, headers=org["owner_headers"] + ) + assert resp.status_code == 404 + + def test_missing_token_rejected(client, org): """Listing an organization's API keys without a bearer token is rejected with 401 or 403.""" resp = client.get(f"/orgs/{org['id']}/api-keys") diff --git a/tests/test_me_api_keys.py b/tests/test_me_api_keys.py index d0fb666..d542a5d 100644 --- a/tests/test_me_api_keys.py +++ b/tests/test_me_api_keys.py @@ -1,11 +1,15 @@ """Self-service API keys at /me/api-keys (Studio Developer page): a member creates keys in their session's organization with scopes limited to what -they hold (dataset.read by default), lists and revokes only their own keys, -is capped at MAX_ACTIVE_KEYS_PER_USER active keys, and cannot use an API -key's own token to manage keys. Sessions without an org or an active -membership are refused. +they hold, issued/displayed in the public literature:read/usage:read scope +vocabulary (literature:read by default -- translated to/from the internal +dataset.read/usage.read permission names at this HTTP boundary only, see +routes_apikeys.py's PUBLIC_SCOPE_TO_PERMISSION), lists and revokes only +their own keys, is capped at MAX_ACTIVE_KEYS_PER_USER active keys, and +cannot use an API key's own token to manage keys. Sessions without an org +or an active membership are refused. """ import uuid +from datetime import datetime, timedelta import pytest from sqlalchemy import create_engine @@ -53,23 +57,72 @@ def scientist(client): return {"org_id": org["id"], "headers": _hdr(_login(client, email)), "email": email} -def test_create_defaults_to_dataset_read_and_lists_own_keys(client, scientist): +def test_create_defaults_to_literature_read_and_lists_own_keys(client, scientist): resp = client.post("/me/api-keys", json={"name": "notebook"}, headers=scientist["headers"]) assert resp.status_code == 201 created = resp.json() - assert created["key"].startswith("omni_sk_") and created["scopes"] == ["dataset.read"] + assert created["key"].startswith("omni_sk_") and created["scopes"] == ["literature:read"] listed = client.get("/me/api-keys", headers=scientist["headers"]).json() assert [k["id"] for k in listed] == [created["id"]] + assert listed[0]["scopes"] == ["literature:read"] assert "key" not in listed[0] -def test_scopes_cannot_exceed_held_permissions(client, scientist): +def test_unknown_scope_name_is_rejected(client, scientist): + """A scope that isn't one of the public literature:read/usage:read names is rejected before it + ever reaches the caller_permissions check -- it's not a real public scope, known or not. + """ resp = client.post("/me/api-keys", json={"name": "x", "scopes": ["manage_billing_nope"]}, headers=scientist["headers"]) assert resp.status_code == 400 +def test_scopes_cannot_exceed_held_permissions(client, scientist): + """usage:read is a real public scope (-> usage.read), but the scientist role doesn't hold + usage.read, so requesting it is still rejected -- a known scope name is not an automatic grant. + """ + resp = client.post("/me/api-keys", json={"name": "x", "scopes": ["usage:read"]}, + headers=scientist["headers"]) + assert resp.status_code == 400 + + +def test_create_with_future_expires_at(client, scientist): + future = (datetime.utcnow() + timedelta(days=7)).isoformat() + resp = client.post("/me/api-keys", json={"name": "temp", "expires_at": future}, headers=scientist["headers"]) + assert resp.status_code == 201 + assert resp.json()["expires_at"] is not None + + +def test_create_rejects_past_expires_at(client, scientist): + past = (datetime.utcnow() - timedelta(days=1)).isoformat() + resp = client.post("/me/api-keys", json={"name": "temp", "expires_at": past}, headers=scientist["headers"]) + assert resp.status_code == 400 + + +def test_rename_own_key(client, scientist): + created = client.post("/me/api-keys", json={"name": "old"}, headers=scientist["headers"]).json() + + resp = client.patch(f"/me/api-keys/{created['id']}", json={"name": "new"}, headers=scientist["headers"]) + assert resp.status_code == 200 + assert resp.json()["name"] == "new" + + listed = client.get("/me/api-keys", headers=scientist["headers"]).json() + assert next(k for k in listed if k["id"] == created["id"])["name"] == "new" + + +def test_cannot_rename_another_members_key(client, scientist): + other = _register_login(client) + other_token = _login(client, other) + other_org = client.post("/orgs", json={"name": "Other2", "slug": f"other2-{uuid.uuid4().hex[:8]}"}, + headers=_hdr(other_token)).json() + assert other_org["id"] != scientist["org_id"] + key = client.post("/me/api-keys", json={"name": "mine"}, headers=scientist["headers"]).json() + other_headers = _hdr(_login(client, other)) + resp = client.patch(f"/me/api-keys/{key['id']}", json={"name": "stolen"}, headers=other_headers) + assert resp.status_code == 404 + + def test_revoke_own_key_publishes_and_is_idempotent(client, scientist): key = client.post("/me/api-keys", json={"name": "k"}, headers=scientist["headers"]).json() routes_apikeys.routes_auth._pub.publish.reset_mock() diff --git a/tests/test_route_authorization_coverage.py b/tests/test_route_authorization_coverage.py index 0846ac6..92d42d0 100644 --- a/tests/test_route_authorization_coverage.py +++ b/tests/test_route_authorization_coverage.py @@ -165,8 +165,14 @@ def test_org_scoped_route_inventory_matches_expected_count(): a third entry in the global-permission-exception set test_sso_mfa_policy_and_saml_override_routes_are_the_only_global_ permission_exception locks in below. + + 43 -> 44 as of M9 (API-key lifecycle): PATCH /orgs/{org_id}/api-keys/ + {key_id} was added (key rename), using + require_org_permission_or_platform_admin(MANAGE_API_KEYS) -- the same + dependency the existing POST/GET/DELETE routes at that same path + already use. """ - assert len(list(_org_scoped_routes())) == 43 + assert len(list(_org_scoped_routes())) == 44 def test_every_org_scoped_route_uses_the_platform_admin_aware_dependency():