diff --git a/docs/content/Extensions/personal-access-tokens.mdx b/docs/content/Extensions/personal-access-tokens.mdx index 909b9ef6..47d3624d 100644 --- a/docs/content/Extensions/personal-access-tokens.mdx +++ b/docs/content/Extensions/personal-access-tokens.mdx @@ -111,7 +111,7 @@ A `write` scope includes the matching `read` scope. | `teams:read` | View teams, members and resource shares | | `chat:run` | Ask agents and search sources (`/api/answer`, `/stream`, `/api/search`); used for benchmarking | -`agents:keys` is separate from `agents:write` on purpose: a deployment token that updates agents does not need to be able to read or rotate the secrets other systems use to call them. +`agents:keys` is separate from `agents:write` on purpose. Creating, publishing or adopting an agent mints its API key; a token without `agents:keys` gets the key back masked (`1234...90ab`), because an agent key keeps working after the token that saw it is revoked. Put differently: a deployment token that updates agents does not need to be able to read or rotate the secrets other systems use to call them. Some parts of the API are never available to a token, whatever its scopes: token management, the admin API, team management, sign-in flows, device pairing, and the interactive OAuth handshakes used by connectors and MCP servers. A token also never carries the `admin` role, even when its owner is an admin. @@ -135,12 +135,23 @@ For a restricted family the token: - can read, update and delete only the listed resources, and listings show only those; - **cannot create** new resources of that family, since a new resource would be outside the list; - cannot attach a resource outside the list to something else, for example set an agent's source to a source the token may not use; -- is refused (`403`, `"error": "resource_not_allowed"`) wherever DocsGPT cannot prove the request stays inside the list. Schedules, conversations and analytics are closed to agent-restricted tokens for that reason, the message replay endpoints (`/api/messages//tail` and `/api/messages//events`) are closed to every restricted token, and `/api/sources/paginated` is closed to source-restricted tokens (use `/api/sources`). +- is refused (`403`, `"error": "resource_not_allowed"`) wherever DocsGPT cannot prove the request stays inside the list: + - **Conversations, analytics and message replay** (`/api/messages//tail`, `/api/messages//events`) are closed to every restricted token. They span all agents and contain cited source text and tool output. + - **Schedules** run an agent with a free-form instruction and store the output. A token restricted to agents can list and create schedules for its agents; every other schedule route, and schedules altogether for tokens restricted on another family, are closed. + - **Workflow writes** are closed to tokens restricted on sources, tools or prompts, because a workflow graph names those inside its nodes. Such a token also cannot attach a workflow to an agent unless it is restricted on workflows too, in which case only the listed workflows can be attached. + - `/api/sources/paginated` is closed to source-restricted tokens (use `/api/sources`). + +A restriction covers what the token *asks for*, not what an allowed resource already contains: an agent on the list runs with its own sources, prompt and tools even when the token is also restricted on those families. List an agent only if you are happy for the token to use everything that agent uses. Restrictions and chat (`chat:run`): -- A token restricted to specific **agents** must pass `agent_id` in the request body, and it must be one of the listed agents. An agent `api_key` in the body is refused. -- A token restricted to specific **sources** (but not agents) may chat against those sources with `active_docs`. It cannot run agents, because an agent brings its own sources. Restrict the token to agents instead to allow that. +- A token restricted to specific **agents** must pass exactly one `agent_id` in the request body, and it must be one of the listed agents. An agent `api_key` or an inline workflow in the body is refused. +- A token restricted to specific **sources** only may chat against those sources with `active_docs`. It cannot run agents, because an agent brings its own sources. Restrict the token to agents instead to allow that. +- A token restricted on **prompts** or **workflows** must also be restricted to agents to chat. +- A token restricted on **tools** cannot use chat at all, and a tools restriction cannot be combined with `chat:run` when the token is created. Chat executes tools (an agent's own, or your default tools when there is no agent) and those cannot be held to a list. +- A `conversation_id` must belong to the agent being run (or to no agent, for agent-less chat). Otherwise the server would continue, append to, or resume pending tool calls of another agent's conversation. + +`agents:write` and import: applying an agent definition can create the prompt and tools it references and rewrite the agent's workflow, all under `agents:write` alone. It does not need `prompts:write`, `tools:write` or `workflows:write`, so treat `agents:write` as able to create those through an import. Restrictions and `agents apply`: a token restricted to specific agents can apply a definition only when it updates one of those agents. A token restricted on sources, prompts, tools or workflows cannot import agents at all, because an import resolves those references by name and may create them. @@ -151,6 +162,7 @@ Restrictions and `agents apply`: a token restricted to specific agents can apply - Revoking a token in **Settings → Access Tokens** takes effect on the next request. - Admins can list a user's tokens with `GET /api/admin/users//tokens` and revoke any token with `DELETE /api/admin/tokens/`. The admin **revoke sessions** action also revokes all of that user's tokens. - Tokens of a deactivated user (through the admin API or SCIM) stop working immediately and work again if the user is reactivated. +- `GET /api/user/tokens` reports a token past its expiry as `"status": "expired"`. - Token creation and revocation are recorded in the authentication audit log (`pat_created`, `pat_revoked`), visible to admins. An expired token's name can be reused: creating a token with that name retires the expired one. diff --git a/docsgpt/api/admin/routes.py b/docsgpt/api/admin/routes.py index 0bad4772..3d178904 100644 --- a/docsgpt/api/admin/routes.py +++ b/docsgpt/api/admin/routes.py @@ -250,9 +250,18 @@ class AdminUserSessionsResource(Resource): ok = denylist.deny_user(user_id) with db_session() as conn: # A forced logout that left API credentials alive would not be one. - tokens_revoked = PersonalAccessTokensRepository(conn).revoke_all_for_user( + revoked_token_ids = PersonalAccessTokensRepository(conn).revoke_all_for_user( user_id, reason="admin_sessions_revoked" ) + # One pat_revoked event per token, like every other revocation path. + for token_id in revoked_token_ids: + AuthEventsRepository(conn).insert( + user_id, + "pat_revoked", + ip=request.remote_addr, + user_agent=request.headers.get("User-Agent"), + metadata={"token_id": token_id, "by": _actor(), "via": "admin_sessions_revoked"}, + ) AuthEventsRepository(conn).insert( user_id, "admin_sessions_revoked", @@ -262,7 +271,7 @@ class AdminUserSessionsResource(Resource): "by": _actor(), "via": "admin_api", "persisted": ok, - "personal_access_tokens_revoked": tokens_revoked, + "personal_access_tokens_revoked": len(revoked_token_ids), }, ) return make_response(jsonify({"success": True, "revoked": ok}), 200) diff --git a/docsgpt/api/pat/routes.py b/docsgpt/api/pat/routes.py index bb3ea8c5..7c2ecc36 100644 --- a/docsgpt/api/pat/routes.py +++ b/docsgpt/api/pat/routes.py @@ -9,6 +9,7 @@ token can neither mint a replacement nor widen itself. from __future__ import annotations import uuid +from datetime import datetime, timezone from flask import jsonify, make_response, request from flask_restx import Namespace, Resource @@ -50,21 +51,35 @@ def _session_user_id(): def _valid_uuid(value: str) -> bool: + """Canonical form only: ``uuid.UUID`` also accepts ``urn:uuid:…`` and braces, which Postgres does not.""" try: - uuid.UUID(str(value)) + return str(uuid.UUID(str(value))) == str(value).lower() except (ValueError, AttributeError, TypeError): return False - return True + + +def _is_expired(expires_at) -> bool: + if not expires_at: + return False + try: + moment = expires_at if isinstance(expires_at, datetime) else datetime.fromisoformat(str(expires_at)) + except ValueError: + return False + if moment.tzinfo is None: + moment = moment.replace(tzinfo=timezone.utc) + return moment <= datetime.now(timezone.utc) def serialize_token(row: dict) -> dict: + # The row keeps status 'active' until someone revokes it; report what is true for a caller. + status = "expired" if row["status"] == "active" and _is_expired(row.get("expires_at")) else row["status"] return { "id": str(row["id"]), "name": row["name"], "token_prefix": row["token_prefix"], "scopes": list(row.get("scopes") or []), "resource_filter": row.get("resource_filter") or {}, - "status": row["status"], + "status": status, "expires_at": row.get("expires_at"), "last_used_at": row.get("last_used_at"), "last_used_ip": row.get("last_used_ip"), @@ -135,6 +150,8 @@ class PersonalAccessTokens(Resource): try: with db_session() as conn: repo = PersonalAccessTokensRepository(conn) + # Serialise this user's creates so concurrent requests cannot both pass the cap check. + repo.lock_user(user_id) if repo.count_active(user_id) >= settings.PAT_MAX_PER_USER: return _error( f"Token limit reached ({settings.PAT_MAX_PER_USER}); revoke one first", 409 diff --git a/docsgpt/api/pat/rules.py b/docsgpt/api/pat/rules.py index 88f1df76..784f764c 100644 --- a/docsgpt/api/pat/rules.py +++ b/docsgpt/api/pat/rules.py @@ -44,7 +44,7 @@ class Rule: open: bool = False in_route: bool = False blocked_by: tuple[str, ...] = () - check: Optional[Callable[[Any, dict], Optional[str]]] = None + check: Optional[Callable[[Any, dict, Optional[str]], Optional[str]]] = None def _rule(scope: Optional[str] = None, *ids: Locator, any_of: tuple[str, ...] = (), **kwargs) -> Rule: @@ -61,6 +61,9 @@ def _rule(scope: Optional[str] = None, *ids: Locator, any_of: tuple[str, ...] = MESSAGE_REPLAY_SCOPES = ("conversations:read", "chat:run") _ALL_FAMILIES = ("agents", "sources", "prompts", "tools", "workflows") +_WORKFLOW_CONTENT_FAMILIES = ("sources", "tools", "prompts") +# A route reached through an agent id can prove the agent; nothing else about it. +_NON_AGENT_FAMILIES = ("sources", "prompts", "tools", "workflows") # Ids of other families that agent create/update accept in their JSON-or-form body. _AGENT_BODY_REFS = ( @@ -72,15 +75,41 @@ _AGENT_BODY_REFS = ( ) -def _chat_check(request, resource_filter: dict) -> Optional[str]: +def _conversation_agent_id(conversation_id: str, user_id: Optional[str]) -> tuple[bool, str]: + """``(found, agent_id)`` for a conversation the user can reach; ``agent_id`` is "" when it has none.""" + from docsgpt.storage.db.repositories.conversations import ConversationsRepository + from docsgpt.storage.db.session import db_readonly + + if not user_id: + return False, "" + try: + with db_readonly() as conn: + row = ConversationsRepository(conn).get_any(str(conversation_id), user_id) + except Exception: + return False, "" + if not row: + return False, "" + return True, str(row.get("agent_id") or "") + + +def _chat_check(request, resource_filter: dict, user_id: Optional[str]) -> Optional[str]: """Keep a restricted token's chat traffic inside its allowlists. An agent brings its own sources, prompt and tools, which this table cannot - see, so a token restricted on any family must name an allowed agent (or, - when only sources are restricted, chat against allowed sources directly). - An agent ``api_key`` in the body would swap in an arbitrary agent. + see, so a restricted token must name an allowed agent. The one exception is + a token restricted on sources only, which may chat against allowed sources + directly. Everything that could swap in another agent or another set of + resources is refused: an agent ``api_key``, an inline workflow, and a + ``conversation_id`` that belongs to a different agent (the server would + otherwise continue, append to, or resume tool calls of that conversation). + + Chat executes tools: an agent's own, or the user's defaults when there is + no agent. Neither can be held to a tools allowlist from here, so a token + restricted on tools cannot chat at all. """ body = _json_body(request) + if "tools" in resource_filter: + return "A token restricted to specific tools cannot use chat endpoints" if body.get("api_key"): return "A restricted token cannot chat with an agent API key; pass agent_id" if body.get("workflow"): @@ -88,11 +117,33 @@ def _chat_check(request, resource_filter: dict) -> Optional[str]: return "A restricted token cannot run an inline workflow" agent_ids = _as_ids(body.get("agent_id")) if "agents" in resource_filter: - if not agent_ids: + if len(agent_ids) != 1: return "This token is restricted to specific agents; pass agent_id" - return None # the agent id itself is verified through ``refs`` - if agent_ids: - return "This token is restricted to specific resources and cannot run arbitrary agents" + # the agent id itself is verified through ``refs`` + elif set(resource_filter) - {"sources"}: + return "Restrict this token to specific agents to use chat endpoints" + elif agent_ids: + return "This token is restricted to specific sources and cannot run agents" + conversation_id = body.get("conversation_id") + if conversation_id: + found, conversation_agent = _conversation_agent_id(conversation_id, user_id) + expected = agent_ids[0] if agent_ids else "" + if not found or _canonical(conversation_agent) != _canonical(expected): + return "This conversation does not belong to the agent this token may use" + return None + + +def _agent_body_check(request, resource_filter: dict, user_id: Optional[str]) -> Optional[str]: + """A workflow pulls in its own sources, tools and prompts, which a reference check cannot see. + + A token restricted on any of those may attach a workflow to an agent only + when it is also restricted on workflows, so the workflow is one its owner + chose (``refs`` then verifies the id). + """ + if "workflows" in resource_filter or not set(resource_filter) & {"sources", "tools", "prompts"}: + return None + if _read(request, (BODY, "workflow")): + return "A token restricted to specific sources, tools or prompts cannot attach a workflow to an agent" return None @@ -124,9 +175,9 @@ RULES: dict[tuple[str, str], Rule] = { ("/api/guardrails/summary", "GET"): _rule("agents:read", (QUERY, "agent_id")), ("/api/agents/folders/", "GET"): _rule("agents:read", open=True), ("/api/agents/folders/", "GET"): _rule("agents:read"), - ("/api/create_agent", "POST"): _rule("agents:write", refs=_AGENT_BODY_REFS), + ("/api/create_agent", "POST"): _rule("agents:write", refs=_AGENT_BODY_REFS, check=_agent_body_check), ("/api/update_agent/", "PUT"): _rule( - "agents:write", (VIEW, "agent_id"), refs=_AGENT_BODY_REFS + "agents:write", (VIEW, "agent_id"), refs=_AGENT_BODY_REFS, check=_agent_body_check ), ("/api/delete_agent", "DELETE"): _rule("agents:write", (QUERY, "id")), ("/api/adopt_agent", "POST"): _rule("agents:write"), @@ -142,22 +193,25 @@ RULES: dict[tuple[str, str], Rule] = { ("/api/agents/folders/bulk_move", "POST"): _rule("agents:write", (JSON, "agent_ids")), ("/api/regenerate_agent_key/", "POST"): _rule("agents:keys", (VIEW, "agent_id")), ("/api/agent_webhook", "GET"): _rule("agents:keys", (QUERY, "id")), - # Schedules hang off an agent. + # Schedules hang off an agent, and a schedule runs that agent with a free-form + # instruction and stores the output. The agent id proves the agent and nothing + # else, so tokens restricted on any other family are kept out; routes that + # carry only a schedule id prove nothing and are closed to every restricted token. ("/api/agents//schedules", "GET"): _rule( - "schedules:read", refs=(("agents", (VIEW, "agent_id")),) + "schedules:read", refs=(("agents", (VIEW, "agent_id")),), blocked_by=_NON_AGENT_FAMILIES ), ("/api/agents//schedules", "POST"): _rule( - "schedules:write", refs=(("agents", (VIEW, "agent_id")),), blocked_by=("tools",) + "schedules:write", refs=(("agents", (VIEW, "agent_id")),), blocked_by=_NON_AGENT_FAMILIES ), - ("/api/schedules/", "GET"): _rule("schedules:read", blocked_by=("agents",)), - ("/api/schedules//runs", "GET"): _rule("schedules:read", blocked_by=("agents",)), + ("/api/schedules/", "GET"): _rule("schedules:read", blocked_by=_ALL_FAMILIES), + ("/api/schedules//runs", "GET"): _rule("schedules:read", blocked_by=_ALL_FAMILIES), ("/api/schedules//runs/", "GET"): _rule( - "schedules:read", blocked_by=("agents",) + "schedules:read", blocked_by=_ALL_FAMILIES ), - ("/api/schedules/", "PUT"): _rule("schedules:write", blocked_by=("agents", "tools")), - ("/api/schedules/", "PATCH"): _rule("schedules:write", blocked_by=("agents", "tools")), - ("/api/schedules/", "DELETE"): _rule("schedules:write", blocked_by=("agents",)), - ("/api/schedules//run", "POST"): _rule("schedules:write", blocked_by=("agents",)), + ("/api/schedules/", "PUT"): _rule("schedules:write", blocked_by=_ALL_FAMILIES), + ("/api/schedules/", "PATCH"): _rule("schedules:write", blocked_by=_ALL_FAMILIES), + ("/api/schedules/", "DELETE"): _rule("schedules:write", blocked_by=_ALL_FAMILIES), + ("/api/schedules//run", "POST"): _rule("schedules:write", blocked_by=_ALL_FAMILIES), # Sources ("/api/sources", "GET"): _rule("sources:read", listing=True), # Counted and paged in SQL, so it cannot be narrowed here; restricted tokens use /api/sources. @@ -217,28 +271,32 @@ RULES: dict[tuple[str, str], Rule] = { ("/api/user/models/test", "POST"): _rule("models:write"), ("/api/user/models//test", "POST"): _rule("models:write"), # Workflows - ("/api/workflows", "POST"): _rule("workflows:write"), + # A workflow graph names sources, tools and prompts inside its nodes, out of reach of ``refs``. + ("/api/workflows", "POST"): _rule("workflows:write", blocked_by=_WORKFLOW_CONTENT_FAMILIES), ("/api/workflows/", "GET"): _rule("workflows:read", (VIEW, "workflow_id")), - ("/api/workflows/", "PUT"): _rule("workflows:write", (VIEW, "workflow_id")), + ("/api/workflows/", "PUT"): _rule( + "workflows:write", (VIEW, "workflow_id"), blocked_by=_WORKFLOW_CONTENT_FAMILIES + ), ("/api/workflows/", "DELETE"): _rule("workflows:write", (VIEW, "workflow_id")), - # Conversations and analytics span every agent, so an agent-restricted token is kept out. - ("/api/get_conversations", "GET"): _rule("conversations:read", blocked_by=("agents",)), - ("/api/search_conversations", "GET"): _rule("conversations:read", blocked_by=("agents",)), - ("/api/get_single_conversation", "GET"): _rule("conversations:read", blocked_by=("agents",)), + # Conversations and analytics span every agent and carry cited source text and tool + # output, so they are closed to every restricted token. + ("/api/get_conversations", "GET"): _rule("conversations:read", blocked_by=_ALL_FAMILIES), + ("/api/search_conversations", "GET"): _rule("conversations:read", blocked_by=_ALL_FAMILIES), + ("/api/get_single_conversation", "GET"): _rule("conversations:read", blocked_by=_ALL_FAMILIES), # A message cannot be tied to an allowlist from here, so any restricted token is kept out. ("/api/messages//tail", "GET"): _rule( any_of=MESSAGE_REPLAY_SCOPES, family=None, blocked_by=_ALL_FAMILIES ), - ("/api/delete_conversation", "POST"): _rule("conversations:write", blocked_by=("agents",)), - ("/api/delete_all_conversations", "GET"): _rule("conversations:write", blocked_by=("agents",)), - ("/api/update_conversation_name", "POST"): _rule("conversations:write", blocked_by=("agents",)), - ("/api/feedback", "POST"): _rule("conversations:write", blocked_by=("agents",)), - ("/api/get_message_analytics", "POST"): _rule("analytics:read", blocked_by=("agents",)), - ("/api/get_token_analytics", "POST"): _rule("analytics:read", blocked_by=("agents",)), - ("/api/get_feedback_analytics", "POST"): _rule("analytics:read", blocked_by=("agents",)), - ("/api/get_tool_analytics", "POST"): _rule("analytics:read", blocked_by=("agents",)), - ("/api/get_schedule_analytics", "POST"): _rule("analytics:read", blocked_by=("agents",)), - ("/api/get_user_logs", "POST"): _rule("analytics:read", blocked_by=("agents",)), + ("/api/delete_conversation", "POST"): _rule("conversations:write", blocked_by=_ALL_FAMILIES), + ("/api/delete_all_conversations", "GET"): _rule("conversations:write", blocked_by=_ALL_FAMILIES), + ("/api/update_conversation_name", "POST"): _rule("conversations:write", blocked_by=_ALL_FAMILIES), + ("/api/feedback", "POST"): _rule("conversations:write", blocked_by=_ALL_FAMILIES), + ("/api/get_message_analytics", "POST"): _rule("analytics:read", blocked_by=_ALL_FAMILIES), + ("/api/get_token_analytics", "POST"): _rule("analytics:read", blocked_by=_ALL_FAMILIES), + ("/api/get_feedback_analytics", "POST"): _rule("analytics:read", blocked_by=_ALL_FAMILIES), + ("/api/get_tool_analytics", "POST"): _rule("analytics:read", blocked_by=_ALL_FAMILIES), + ("/api/get_schedule_analytics", "POST"): _rule("analytics:read", blocked_by=_ALL_FAMILIES), + ("/api/get_user_logs", "POST"): _rule("analytics:read", blocked_by=_ALL_FAMILIES), # Teams (read only) ("/api/teams", "GET"): _rule("teams:read"), ("/api/teams/", "GET"): _rule("teams:read"), @@ -395,18 +453,18 @@ def authorize(request, decoded_token: dict) -> Optional[tuple[dict, int]]: resource_filter = decoded_token.get("resource_filter") or {} if not resource_filter: return None - reason = _check_resources(request, rule, resource_filter) + reason = _check_resources(request, rule, resource_filter, decoded_token.get("sub")) if reason is None: return None return ({"success": False, "error": "resource_not_allowed", "message": reason}, 403) -def _check_resources(request, rule: Rule, resource_filter: dict) -> Optional[str]: +def _check_resources(request, rule: Rule, resource_filter: dict, user_id: Optional[str] = None) -> Optional[str]: for family in rule.blocked_by: if family in resource_filter: return f"This endpoint is not available to a token restricted to specific {family}" if rule.check is not None: - reason = rule.check(request, resource_filter) + reason = rule.check(request, resource_filter, user_id) if reason: return reason for family, locator in rule.refs: @@ -459,3 +517,19 @@ def _is_uuid(value: str) -> bool: except (ValueError, AttributeError, TypeError): return False return True + + +def may_see_agent_keys(request) -> bool: + """False for a token without ``agents:keys``: it must not receive a plaintext agent API key. + + Create, first publish and adopt all mint a key and used to return it, which + handed a deploy token a secret that outlives the token's own revocation. + """ + decoded = getattr(request, "decoded_token", None) + if not is_pat(decoded): + return True + return "agents:keys" in (decoded.get("scopes") or []) + + +def mask_agent_key(key: Optional[str]) -> str: + return f"{key[:4]}...{key[-4:]}" if key else "" diff --git a/docsgpt/api/pat/tokens.py b/docsgpt/api/pat/tokens.py index fa4cb9d0..6e4978bd 100644 --- a/docsgpt/api/pat/tokens.py +++ b/docsgpt/api/pat/tokens.py @@ -121,6 +121,9 @@ def normalize_resource_filter(raw: Any, scopes: list[str]) -> dict[str, list[str # chat:run acts on agents and sources, so both may be restricted alongside it. if "chat" in families: families.update({"agents", "sources"}) + if "tools" in raw and "chat:run" in scopes: + # Chat executes tools (an agent's own, or the user's defaults), which cannot be held to an allowlist. + raise ValueError("resource_filter.tools cannot be combined with the chat:run scope") out: dict[str, list[str]] = {} for family, ids in raw.items(): if family not in FILTERABLE_FAMILIES: diff --git a/docsgpt/api/user/agents/routes.py b/docsgpt/api/user/agents/routes.py index 757dd933..6a0ec2b0 100644 --- a/docsgpt/api/user/agents/routes.py +++ b/docsgpt/api/user/agents/routes.py @@ -9,7 +9,7 @@ from flask_restx import fields, Namespace, Resource from pydantic import ValidationError as PydanticValidationError from docsgpt.api import api -from docsgpt.api.pat.rules import filter_listing +from docsgpt.api.pat.rules import filter_listing, mask_agent_key, may_see_agent_keys from docsgpt.guardrails.config import AgentConfig from docsgpt.api.user.base import ( copy_agent_image_for_user, @@ -763,7 +763,9 @@ class CreateAgent(Resource): except Exception as err: current_app.logger.error(f"Error creating agent: {err}", exc_info=True) return make_response(jsonify({"success": False}), 400) - return make_response(jsonify({"id": new_id, "key": key}), 201) + # A token without agents:keys never receives the plaintext agent key. + visible_key = key if may_see_agent_keys(request) else mask_agent_key(key) + return make_response(jsonify({"id": new_id, "key": visible_key}), 201) @agents_ns.route("/update_agent/") @@ -1307,7 +1309,11 @@ class UpdateAgent(Resource): "message": "Agent updated successfully", } if newly_generated_key: - response_data["key"] = newly_generated_key + response_data["key"] = ( + newly_generated_key + if may_see_agent_keys(request) + else mask_agent_key(newly_generated_key) + ) return make_response(jsonify(response_data), 200) @@ -1811,7 +1817,9 @@ class AdoptAgent(Resource): ) response_agent = _format_agent_output(new_agent, include_key_masked=False) - response_agent["key"] = new_key + response_agent["key"] = ( + new_key if may_see_agent_keys(request) else mask_agent_key(new_key) + ) return make_response( jsonify({"success": True, "agent": response_agent}), 200 ) diff --git a/docsgpt/storage/db/repositories/personal_access_tokens.py b/docsgpt/storage/db/repositories/personal_access_tokens.py index efbe250b..12bd80cc 100644 --- a/docsgpt/storage/db/repositories/personal_access_tokens.py +++ b/docsgpt/storage/db/repositories/personal_access_tokens.py @@ -76,6 +76,13 @@ class PersonalAccessTokensRepository: result = self._conn.execute(text(sql), {"user_id": user_id}) return [row_to_dict(r) for r in result.fetchall()] + def lock_user(self, user_id: str) -> None: + """Hold a per-user advisory lock until the transaction ends.""" + self._conn.execute( + text("SELECT pg_advisory_xact_lock(hashtextextended(:key, 0))"), + {"key": f"personal_access_tokens:{user_id}"}, + ) + def count_active(self, user_id: str) -> int: """Live tokens only: revoked and expired rows don't count against the per-user cap.""" return self._conn.execute( @@ -161,13 +168,14 @@ class PersonalAccessTokensRepository: params["user_id"] = user_id return self._conn.execute(text(sql), params).rowcount > 0 - def revoke_all_for_user(self, user_id: str, *, reason: str = "admin_revoked") -> int: + def revoke_all_for_user(self, user_id: str, *, reason: str = "admin_revoked") -> list[str]: + """Revoke every live token of a user. Returns the revoked token ids (for the audit trail).""" result = self._conn.execute( text( "UPDATE personal_access_tokens " "SET status = 'revoked', revoked_at = now(), revoke_reason = :reason " - "WHERE user_id = :user_id AND status = 'active'" + "WHERE user_id = :user_id AND status = 'active' RETURNING id" ), {"user_id": user_id, "reason": reason}, ) - return result.rowcount + return [str(row[0]) for row in result.fetchall()] diff --git a/frontend/src/settings/PersonalAccessTokens.tsx b/frontend/src/settings/PersonalAccessTokens.tsx index 7439e567..f417200c 100644 --- a/frontend/src/settings/PersonalAccessTokens.tsx +++ b/frontend/src/settings/PersonalAccessTokens.tsx @@ -4,6 +4,7 @@ import { useTranslation } from 'react-i18next'; import { useSelector } from 'react-redux'; import patService, { + AccessTokenApiError, AccessTokenPolicy, AccessTokenScope, CreateAccessTokenResponse, @@ -134,6 +135,13 @@ export default function PersonalAccessTokens() { setTokens((prev) => prev.filter((item) => item.id !== target.id)); setError(null); } catch (err) { + if (err instanceof AccessTokenApiError && err.status === 404) { + // Already revoked elsewhere (another tab, an admin): it is gone, so + // drop the stale row instead of reporting a failure. + setTokens((prev) => prev.filter((item) => item.id !== target.id)); + setError(null); + return; + } console.error('Failed to revoke access token:', err); setError( (err instanceof Error && err.message) || diff --git a/frontend/src/settings/accessTokenUtils.test.ts b/frontend/src/settings/accessTokenUtils.test.ts index c820fc8d..a9df9ed5 100644 --- a/frontend/src/settings/accessTokenUtils.test.ts +++ b/frontend/src/settings/accessTokenUtils.test.ts @@ -294,3 +294,14 @@ describe('NO_ESCAPE', () => { expect(NO_ESCAPE).toEqual({ interpolation: { escapeValue: false } }); }); }); + +describe('eligibleFilterFamilies with chat:run', () => { + const all = ['agents', 'sources', 'prompts', 'tools', 'workflows']; + it('never offers a tools restriction next to chat:run', () => { + expect(eligibleFilterFamilies(['tools:write', 'chat:run'], all)).toEqual([ + 'agents', + 'sources', + ]); + expect(eligibleFilterFamilies(['tools:write'], all)).toEqual(['tools']); + }); +}); diff --git a/frontend/src/settings/accessTokenUtils.ts b/frontend/src/settings/accessTokenUtils.ts index 9e7e56c3..ddfc4094 100644 --- a/frontend/src/settings/accessTokenUtils.ts +++ b/frontend/src/settings/accessTokenUtils.ts @@ -75,7 +75,11 @@ export function eligibleFilterFamilies( filterableFamilies: string[], ): string[] { const families = new Set(selected.map(scopeFamily)); - if (families.has('chat')) CHAT_FAMILIES.forEach((f) => families.add(f)); + const chat = families.has('chat'); + if (chat) CHAT_FAMILIES.forEach((f) => families.add(f)); + // Chat executes tools, which cannot be held to an allowlist, so the server + // rejects a tools restriction on a token that also has chat:run. + if (chat) families.delete('tools'); return filterableFamilies.filter((family) => families.has(family)); } diff --git a/tests/api/test_admin_dashboard.py b/tests/api/test_admin_dashboard.py index 50f7dfe0..59c67e4c 100644 --- a/tests/api/test_admin_dashboard.py +++ b/tests/api/test_admin_dashboard.py @@ -178,13 +178,20 @@ class TestUserLifecycle: def test_force_logout(self, client): events = Mock() - with _admin(AuthEventsRepository=Mock(return_value=events)), patch( - "docsgpt.api.admin.routes.denylist" - ) as dl: + tokens = Mock() + tokens.revoke_all_for_user.return_value = ["t1", "t2"] + with _admin( + AuthEventsRepository=Mock(return_value=events), + PersonalAccessTokensRepository=Mock(return_value=tokens), + ), patch("docsgpt.api.admin.routes.denylist") as dl: dl.deny_user.return_value = True resp = client.post("/api/admin/users/bob/revoke-sessions") assert resp.status_code == 200 dl.deny_user.assert_called_once_with("bob") + # A forced logout also revokes the user's API tokens, one audit event each. + tokens.revoke_all_for_user.assert_called_once_with("bob", reason="admin_sessions_revoked") + recorded = [call.args[1] for call in events.insert.call_args_list] + assert recorded == ["pat_revoked", "pat_revoked", "admin_sessions_revoked"] def test_user_detail(self, client): users = Mock() diff --git a/tests/api/test_pat_routes.py b/tests/api/test_pat_routes.py index f7592cf4..cf21c8c5 100644 --- a/tests/api/test_pat_routes.py +++ b/tests/api/test_pat_routes.py @@ -166,6 +166,16 @@ class TestCreate: assert "dgpt_pat_" not in json.dumps(metadata) +class TestExpiredStatus: + def test_expired_token_is_reported_as_expired_not_active(self, client, db): + _create(client) + with _session(): + assert json.loads(client.get("/api/user/tokens").data)["tokens"][0]["status"] == "active" + db.execute(text("UPDATE personal_access_tokens SET expires_at = now() - interval '1 day'")) + with _session(): + assert json.loads(client.get("/api/user/tokens").data)["tokens"][0]["status"] == "expired" + + class TestList: def test_includes_scope_catalog_and_policy(self, client, db): with _session(): @@ -219,10 +229,16 @@ class TestRevoke: with _session(sub="bob"): assert client.delete(f"/api/user/tokens/{token_id}").status_code == 404 - def test_unknown_and_malformed_ids(self, client, db): + @pytest.mark.parametrize( + "token_id", + [AGENT_A, "not-a-uuid", f"urn:uuid:{AGENT_A}", "{" + AGENT_A + "}", AGENT_A.replace("-", "")], + ) + def test_unknown_and_malformed_ids(self, client, db, token_id): + # uuid.UUID() accepts urn:/braced/bare-hex spellings that Postgres rejects; none may reach the cast. with _session(): - assert client.delete(f"/api/user/tokens/{AGENT_A}").status_code == 404 - assert client.delete("/api/user/tokens/not-a-uuid").status_code == 404 + assert client.delete(f"/api/user/tokens/{token_id}").status_code == 404 + with _session(sub="root", roles=("admin", "user")): + assert client.delete(f"/api/admin/tokens/{token_id}").status_code == 404 class TestAdmin: @@ -256,3 +272,8 @@ class TestAdmin: assert client.post("/api/admin/users/alice/revoke-sessions").status_code == 200 headers = {"Authorization": f"Bearer {created['token']}"} assert client.get("/api/user/me", headers=headers).status_code == 401 + events = db.execute( + text("SELECT metadata FROM auth_events WHERE user_id = 'alice' AND event = 'pat_revoked'") + ).all() + assert [e[0]["token_id"] for e in events] == [created["personal_access_token"]["id"]] + assert events[0][0]["via"] == "admin_sessions_revoked" diff --git a/tests/api/test_pat_rules.py b/tests/api/test_pat_rules.py index 4bdc7070..69e532a3 100644 --- a/tests/api/test_pat_rules.py +++ b/tests/api/test_pat_rules.py @@ -246,6 +246,102 @@ class TestResourceRestrictions: assert _denied(_call(client, "GET", "/api/sources/paginated", claims)) == "resource_not_allowed" +WORKFLOW_A = "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee" +TOOL_A = "ffffffff-ffff-ffff-ffff-ffffffffffff" + + +@pytest.mark.unit +class TestRelationshipsBeyondIds: + """Rows whose content or parent the table cannot see are closed to restricted tokens.""" + + @pytest.mark.parametrize("family", ["sources", "tools", "prompts"]) + def test_workflow_writes_are_closed_to_tokens_restricted_on_what_a_graph_can_name(self, client, family): + claims = _claims(["workflows:write"], {family: [SOURCE_A]}) + assert _denied(_call(client, "POST", "/api/workflows", claims, json={})) == "resource_not_allowed" + assert ( + _denied(_call(client, "PUT", f"/api/workflows/{WORKFLOW_A}", claims, json={})) + == "resource_not_allowed" + ) + assert _denied(_call(client, "GET", f"/api/workflows/{WORKFLOW_A}", claims)) is None + + def test_workflow_restricted_token_can_still_edit_its_workflows(self, client): + claims = _claims(["workflows:write"], {"workflows": [WORKFLOW_A]}) + assert _denied(_call(client, "PUT", f"/api/workflows/{WORKFLOW_A}", claims, json={})) is None + + @pytest.mark.parametrize("family", ["sources", "tools", "prompts"]) + def test_agent_cannot_be_pointed_at_a_workflow_by_a_token_restricted_on_its_contents(self, client, family): + claims = _claims(["agents:write"], {family: [SOURCE_A]}) + for body in ({"workflow": WORKFLOW_A}, {"workflow": {"id": WORKFLOW_A}}): + response = _call(client, "PUT", f"/api/update_agent/{AGENT_A}", claims, json=body) + assert _denied(response) == "resource_not_allowed" + form = _call(client, "PUT", f"/api/update_agent/{AGENT_A}", claims, data={"workflow": WORKFLOW_A}) + assert _denied(form) == "resource_not_allowed" + assert _denied(_call(client, "PUT", f"/api/update_agent/{AGENT_A}", claims, json={"name": "x"})) is None + + def test_workflow_allowlist_lets_the_agent_use_those_workflows_only(self, client): + claims = _claims(["agents:write"], {"sources": [SOURCE_A], "workflows": [WORKFLOW_A]}) + ok = _call(client, "PUT", f"/api/update_agent/{AGENT_A}", claims, json={"workflow": WORKFLOW_A}) + bad = _call(client, "PUT", f"/api/update_agent/{AGENT_A}", claims, json={"workflow": SOURCE_B}) + assert _denied(ok) is None + assert _denied(bad) == "resource_not_allowed" + + @pytest.mark.parametrize("family", ["sources", "prompts", "tools", "workflows"]) + def test_schedules_are_closed_to_tokens_restricted_on_anything_but_agents(self, client, family): + claims = _claims(["schedules:write"], {family: [SOURCE_A]}) + for method, path in ( + ("GET", f"/api/agents/{AGENT_A}/schedules"), + ("POST", f"/api/agents/{AGENT_A}/schedules"), + ("GET", "/api/schedules/s1"), + ("POST", "/api/schedules/s1/run"), + ("GET", "/api/schedules/s1/runs"), + ): + assert _denied(_call(client, method, path, claims, json={})) == "resource_not_allowed", path + + @pytest.mark.parametrize("family", ["agents", "sources", "prompts", "tools", "workflows"]) + def test_conversations_and_analytics_are_closed_to_every_restricted_token(self, client, family): + claims = _claims(["conversations:write", "analytics:read"], {family: [SOURCE_A]}) + for method, path in ( + ("GET", "/api/get_conversations"), + ("GET", "/api/get_single_conversation?id=c1"), + ("GET", "/api/search_conversations?q=x"), + ("POST", "/api/delete_conversation"), + ("POST", "/api/feedback"), + ("POST", "/api/get_message_analytics"), + ("POST", "/api/get_user_logs"), + ): + assert _denied(_call(client, method, path, claims, json={})) == "resource_not_allowed", path + + +@pytest.mark.unit +class TestAgentKeyVisibility: + def _request(self, flask_app, claims): + from flask import request + + ctx = flask_app.test_request_context("/") + ctx.push() + request.decoded_token = claims + return ctx, request + + def test_sessions_and_tokens_with_the_keys_scope_see_the_key(self, flask_app): + for claims in ({"sub": "alice"}, _claims(["agents:write", "agents:keys"])): + ctx, request = self._request(flask_app, claims) + try: + assert rules.may_see_agent_keys(request) is True + finally: + ctx.pop() + + def test_token_without_the_keys_scope_does_not(self, flask_app): + ctx, request = self._request(flask_app, _claims(["agents:write"])) + try: + assert rules.may_see_agent_keys(request) is False + finally: + ctx.pop() + + def test_mask(self): + assert rules.mask_agent_key("12345678-aaaa-bbbb-cccc-1234567890ab") == "1234...90ab" + assert rules.mask_agent_key("") == "" and rules.mask_agent_key(None) == "" + + @pytest.mark.unit class TestChatRestrictions: def _chat(self, client, claims, body): @@ -274,6 +370,52 @@ class TestChatRestrictions: assert self._chat(client, claims, {"question": "hi", "active_docs": [SOURCE_A, SOURCE_B]}) == "resource_not_allowed" assert self._chat(client, claims, {"question": "hi", "agent_id": AGENT_A}) == "resource_not_allowed" + @pytest.mark.parametrize("extra", [{}, {"agents": [AGENT_A]}]) + def test_tools_restricted_token_cannot_chat_at_all(self, client, extra): + claims = _claims(["chat:run"], {"tools": [TOOL_A], **extra}) + assert self._chat(client, claims, {"question": "hi"}) == "resource_not_allowed" + assert self._chat(client, claims, {"question": "hi", "agent_id": AGENT_A}) == "resource_not_allowed" + + @pytest.mark.parametrize("family", ["prompts", "workflows"]) + def test_agentless_chat_needs_an_agent_restriction_unless_only_sources_are_restricted(self, client, family): + claims = _claims(["chat:run"], {family: [SOURCE_A]}) + assert self._chat(client, claims, {"question": "hi"}) == "resource_not_allowed" + assert self._chat(client, claims, {"question": "hi", "agent_id": AGENT_A}) == "resource_not_allowed" + + def test_conversation_must_belong_to_the_agent_being_run(self, client): + claims = _claims(["chat:run"], {"agents": [AGENT_A]}) + body = {"question": "hi", "agent_id": AGENT_A, "conversation_id": "c1"} + with patch.object(rules, "_conversation_agent_id", return_value=(True, AGENT_A.upper())) as lookup: + assert self._chat(client, claims, body) is None + lookup.assert_called_once_with("c1", "alice") + for result in ((True, AGENT_B), (True, ""), (False, "")): + with patch.object(rules, "_conversation_agent_id", return_value=result): + assert self._chat(client, claims, body) == "resource_not_allowed", result + + def test_conversation_resume_with_tool_actions_is_held_to_the_same_rule(self, client): + claims = _claims(["chat:run"], {"agents": [AGENT_A]}) + body = {"agent_id": AGENT_A, "conversation_id": "c1", "tool_actions": [{"call_id": "x"}]} + with patch.object(rules, "_conversation_agent_id", return_value=(True, AGENT_B)): + assert self._chat(client, claims, body) == "resource_not_allowed" + + def test_agentless_token_cannot_continue_an_agent_conversation(self, client): + claims = _claims(["chat:run"], {"sources": [SOURCE_A]}) + body = {"question": "hi", "active_docs": SOURCE_A, "conversation_id": "c1"} + with patch.object(rules, "_conversation_agent_id", return_value=(True, AGENT_B)): + assert self._chat(client, claims, body) == "resource_not_allowed" + with patch.object(rules, "_conversation_agent_id", return_value=(True, "")): + assert self._chat(client, claims, body) is None + + def test_unrestricted_token_never_pays_for_the_conversation_lookup(self, client): + with patch.object(rules, "_conversation_agent_id") as lookup: + self._chat(client, _claims(["chat:run"]), {"question": "hi", "conversation_id": "c1"}) + lookup.assert_not_called() + + def test_conversation_lookup_fails_closed(self): + with patch("docsgpt.storage.db.session.db_readonly", side_effect=RuntimeError("db down")): + assert rules._conversation_agent_id("c1", "alice") == (False, "") + assert rules._conversation_agent_id("c1", None) == (False, "") + def test_retrieval_test_honours_the_source_allowlist(self, client): claims = _claims(["chat:run"], {"sources": [SOURCE_A]}) ok = _call(client, "POST", f"/api/sources/{SOURCE_A}/search", claims, json={"query": "q"}) diff --git a/tests/api/test_pat_tokens.py b/tests/api/test_pat_tokens.py index 1e38d0ce..24f6a5bb 100644 --- a/tests/api/test_pat_tokens.py +++ b/tests/api/test_pat_tokens.py @@ -113,6 +113,11 @@ class TestResourceFilter: ) assert set(out) == {"agents", "sources"} + def test_tools_restriction_cannot_be_combined_with_chat(self): + with pytest.raises(ValueError, match="chat:run"): + tokens.normalize_resource_filter({"tools": [self.UUID_A]}, ["tools:read", "chat:run"]) + assert tokens.normalize_resource_filter({"tools": [self.UUID_A]}, ["tools:read"]) + @pytest.mark.parametrize( "raw,scopes", [ diff --git a/tests/storage/db/repositories/test_personal_access_tokens.py b/tests/storage/db/repositories/test_personal_access_tokens.py index 8a6848a3..0f4732f0 100644 --- a/tests/storage/db/repositories/test_personal_access_tokens.py +++ b/tests/storage/db/repositories/test_personal_access_tokens.py @@ -150,7 +150,7 @@ class TestRevoke: _create(repo, name="a", token_hash="h1") _create(repo, name="b", token_hash="h2") _create(repo, user_id="u2", token_hash="h3") - assert repo.revoke_all_for_user("u1") == 2 + assert len(repo.revoke_all_for_user("u1")) == 2 assert repo.list_for_user("u1") == [] assert len(repo.list_for_user("u2")) == 1