diff --git a/.github/workflows/test-migrations.yml b/.github/workflows/test-migrations.yml index daa5824a47..dd9896c7c3 100644 --- a/.github/workflows/test-migrations.yml +++ b/.github/workflows/test-migrations.yml @@ -49,6 +49,12 @@ on: - 'tests/web/test_user_oauth_actor_ownership.py' - 'tests/shared/postgres_disposable.py' - 'tests/web/services/checkpoint_anchor_shared.py' + - 'tests/web/api/test_mcp_server_edit_lock_postgresql.py' + - 'src/xagent/web/api/mcp.py' + - 'src/xagent/web/api/custom_api.py' + - 'src/xagent/web/services/connector_team_scope.py' + - 'tests/web/api/test_custom_api_edit_lock_postgresql.py' + - 'tests/web/api/test_connector_hook_session_fault_postgresql.py' pull_request: branches: [main] # Required by the merge queue: without this the two required contexts below @@ -134,6 +140,12 @@ jobs: tests/web/test_user_oauth_actor_ownership.py tests/shared/postgres_disposable.py tests/web/services/checkpoint_anchor_shared.py + tests/web/api/test_mcp_server_edit_lock_postgresql.py + src/xagent/web/api/mcp.py + src/xagent/web/api/custom_api.py + src/xagent/web/services/connector_team_scope.py + tests/web/api/test_custom_api_edit_lock_postgresql.py + tests/web/api/test_connector_hook_session_fault_postgresql.py ) case "$EVENT_NAME" in @@ -437,6 +449,27 @@ jobs: env: XAGENT_TEST_POSTGRES_URL: postgresql://xagent:xagent@localhost:5432/xagent_test + - name: Test MCP server edit row lock (Postgres-only) + if: needs.detect-migration-changes.outputs.should-test == 'true' + run: | + pytest tests/web/api/test_mcp_server_edit_lock_postgresql.py -m postgresql -q + env: + XAGENT_TEST_POSTGRES_URL: postgresql://xagent:xagent@localhost:5432/xagent_test + + - name: Test Custom API edit row lock (Postgres-only) + if: needs.detect-migration-changes.outputs.should-test == 'true' + run: | + pytest tests/web/api/test_custom_api_edit_lock_postgresql.py -m postgresql -q + env: + XAGENT_TEST_POSTGRES_URL: postgresql://xagent:xagent@localhost:5432/xagent_test + + - name: Test connector hook session fault recovery (Postgres-only) + if: needs.detect-migration-changes.outputs.should-test == 'true' + run: | + pytest tests/web/api/test_connector_hook_session_fault_postgresql.py -m postgresql -q + env: + XAGENT_TEST_POSTGRES_URL: postgresql://xagent:xagent@localhost:5432/xagent_test + migrations-summary: name: Migrations Summary runs-on: ubuntu-latest diff --git a/src/xagent/web/api/custom_api.py b/src/xagent/web/api/custom_api.py index add0b86048..e714c72fc7 100644 --- a/src/xagent/web/api/custom_api.py +++ b/src/xagent/web/api/custom_api.py @@ -7,13 +7,14 @@ import logging from datetime import datetime -from typing import Any, Dict, List, Optional +from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, cast from fastapi import APIRouter, Depends, HTTPException, status from pydantic import BaseModel, Field from sqlalchemy.orm import Session from ...core.tools.adapters.vibe.connector_runtime import ( + ConnectorRuntimeError, validate_runtime_config_declaration, ) from ...core.utils.encryption import encrypt_value @@ -22,6 +23,10 @@ from ..models.database import get_db from ..models.user import User +if TYPE_CHECKING: + from ..services.connector_team_scope import ConnectorAccess + from .mcp import _TeamOwnedUserApi + logger = logging.getLogger(__name__) @@ -111,7 +116,7 @@ class Config: def _db_api_to_response( api: CustomApi, - user_api: UserCustomApi, + user_api: "UserCustomApi | _TeamOwnedUserApi", ) -> CustomApiResponse: """Convert database CustomApi to response model with masked env values.""" @@ -259,34 +264,124 @@ async def create_custom_api( return _db_api_to_response(new_api, user_api) -@custom_api_router.get("/{api_id}", response_model=CustomApiResponse) -async def get_custom_api( +def _http_from_connector_runtime(exc: ConnectorRuntimeError) -> HTTPException: + """One place that maps the connector seam's typed error onto this + module's HTTP answer. Four call sites need it (``get_custom_api``; + ``update_custom_api`` for the pre-lock resolution, the post-lock + re-check, and the rename hook), and this route has no function-wide + ``try`` the way ``update_mcp_server`` does, so each arm is local.""" + return HTTPException(status_code=exc.status_code, detail=exc.safe_message) + + +def _resolve_custom_api_for_request( + db: Session, + user_id: int, api_id: int, - current_user: User = Depends(get_current_user), - db: Session = Depends(get_db), -) -> CustomApiResponse: - """Get a specific Custom API by ID.""" + *, + skip_resolution_when: "Callable[[UserCustomApi], bool] | None" = None, +) -> "tuple[UserCustomApi | _TeamOwnedUserApi, CustomApi, ConnectorAccess | None]": + """Resolve the caller's association, the definition row, and the + caller's team access verdict, for ``GET``/``PUT /api/custom-apis/{id}``. + + Looks up the caller's own personal link row first, with the same query + both routes have always run. When that row exists and its ``custom_api`` + relationship resolves, the association and the definition row both come + from it and nothing else runs. When it does not -- no row, or a row + whose relationship is unexpectedly empty -- the definition row is + looked up on its own -- a team-owned API's shared row must still be + found even though this caller has no personal link to it -- and the + caller's team access verdict decides what happens next: + + - no working personal row and no team access (``access is None``) -> + 404, the same outcome every caller without an association has + always gotten. + - no working personal row but the caller's team links the API -> the + existing ``_TeamOwnedUserApi`` stand-in takes the association's + place, the same stand-in the aggregate connector list already + constructs for this case. + + ``skip_resolution_when`` lets a caller declare when its own working + personal row already decides the answer on its own, so resolving a + verdict would only add an unnecessary hook call: ``get_custom_api`` + passes a predicate that is always true, because it never reads the + verdict at all and a personal row -- owner or not -- already decides + what it returns; ``update_custom_api`` passes one that checks + ``can_edit``, because only an owner's ``can_edit=True`` decides the + edit answer on its own -- a non-owner's ``can_edit=False`` personal row + does not, since a granting team verdict can still widen it. Left + unset (the default), resolution is never skipped, which is what a + caller with no working personal row always needs -- the verdict is the + gate there and must stay fail-closed. + + Raises ``ConnectorRuntimeError`` when access resolution itself fails; + callers translate that into an ``HTTPException``. + """ + from ..services.connector_team_scope import resolve_one_connector_access_or_raise + from .mcp import _TeamOwnedUserApi user_api = ( db.query(UserCustomApi) .filter( UserCustomApi.custom_api_id == api_id, - UserCustomApi.user_id == current_user.id, + UserCustomApi.user_id == user_id, ) .first() ) + if user_api is not None and user_api.custom_api is not None: + api: Optional[CustomApi] = user_api.custom_api + else: + user_api = None + api = db.query(CustomApi).filter(CustomApi.id == api_id).first() - if not user_api or not user_api.custom_api: + already_decided = user_api is not None and ( + skip_resolution_when is not None and skip_resolution_when(user_api) + ) + + access: "ConnectorAccess | None" = None + if api is not None and not already_decided: + access = resolve_one_connector_access_or_raise( + db, int(user_id), ("custom_api", int(api.id)) + ) + + if user_api is None and access is None: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="Custom API not found", ) - return _db_api_to_response(user_api.custom_api, user_api) + resolved_user_api: "UserCustomApi | _TeamOwnedUserApi" = ( + user_api if user_api is not None else _TeamOwnedUserApi(int(user_id)) + ) + return resolved_user_api, cast(CustomApi, api), access + + +@custom_api_router.get("/{api_id}", response_model=CustomApiResponse) +def get_custom_api( + api_id: int, + current_user: User = Depends(get_current_user), + db: Session = Depends(get_db), +) -> CustomApiResponse: + """Get a specific Custom API by ID.""" + + try: + # This route never reads the verdict at all (see _db_api_to_response), + # so a working personal row -- owner or not -- always already + # decides everything this route returns; resolving one would only + # add an unnecessary hook call. + user_api, api, _team_access = _resolve_custom_api_for_request( + db, + int(current_user.id), + api_id, + skip_resolution_when=lambda _user_api: True, + ) + except ConnectorRuntimeError as exc: + raise _http_from_connector_runtime(exc) from exc + + return _db_api_to_response(api, user_api) @custom_api_router.put("/{api_id}", response_model=CustomApiResponse) -async def update_custom_api( +def update_custom_api( api_id: int, api_data: CustomApiUpdate, current_user: User = Depends(get_current_user), @@ -294,28 +389,110 @@ async def update_custom_api( ) -> CustomApiResponse: """Update an existing Custom API.""" - user_api = ( - db.query(UserCustomApi) - .filter( - UserCustomApi.custom_api_id == api_id, - UserCustomApi.user_id == current_user.id, + try: + # An owner's can_edit=True already decides the edit answer on its + # own (below), so resolving a verdict for that row would only add + # an unnecessary hook call; a non-owner's can_edit=False personal + # row does not decide it, since a granting team verdict can still + # widen it. + user_api, api, team_access = _resolve_custom_api_for_request( + db, + int(current_user.id), + api_id, + skip_resolution_when=lambda ua: bool(ua.can_edit), ) - .first() + except ConnectorRuntimeError as exc: + raise _http_from_connector_runtime(exc) from exc + + is_stand_in = not isinstance(user_api, UserCustomApi) + can_edit = bool(user_api.can_edit) or bool( + team_access is not None and team_access.can_edit ) + if not can_edit: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="You do not have permission to edit this Custom API", + ) - if not user_api or not user_api.custom_api: + # is_active lives on the personal association row; a caller with no + # personal row (the stand-in) has none to hold it, so a payload + # carrying it must be rejected outright -- writing it onto the + # stand-in would only set a shadowing instance attribute that + # persists nothing, and the response below would then read that + # shadow back and report a change that never happened. + if is_stand_in and api_data.is_active is not None: raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="Custom API not found", + status_code=status.HTTP_400_BAD_REQUEST, + detail="No personal connection exists to configure is_active for this API", ) - if not user_api.can_edit: + # A second, single-table lock on the definition row, taken before any + # field below reads or mutates it. The read above comes through the + # personal link row's relationship (or a bare lookup for a stand-in + # caller) and cannot itself lock just this table; this is a fresh + # statement, so a row deleted between the two still yields None here + # (handled as the same 404) rather than surfacing as an unrelated + # error out of the write path below. + locked_api = ( + db.query(CustomApi) + .filter(CustomApi.id == api_id) + .populate_existing() + .with_for_update() + .first() + ) + if locked_api is None: raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="You do not have permission to edit this Custom API", + status_code=status.HTTP_404_NOT_FOUND, detail="Custom API not found" + ) + api = locked_api + + # This re-check assumes READ COMMITTED, PostgreSQL's default, which this + # codebase sets no isolation_level on its engine to change: it needs a + # fresh snapshot to see a link the application revoked and committed + # after this request's pre-lock read. Under REPEATABLE READ or + # SERIALIZABLE the re-read reuses this transaction's original snapshot, + # sees the pre-lock answer again, and the recheck degrades to a + # no-op -- it would stop refusing, not start refusing wrongly. + # + # Same re-check as the MCP side's PUT, for the same reason: the verdict + # was resolved before this lock existed and the application that + # answers it can revoke the link at any moment. No personal-field + # exemption here, unlike MCP: this route's gate above refuses *every* + # payload without can_edit, including an is_active-only one, so the + # verdict is the authority for every write it admits. No platform-admin + # exemption either -- this route's gate has no admin bypass at all. + if team_access is not None and team_access.can_edit: + from ..services.connector_team_scope import ( + resolve_one_connector_access_or_raise, ) - api = user_api.custom_api + try: + rechecked = resolve_one_connector_access_or_raise( + db, int(current_user.id), ("custom_api", int(api_id)) + ) + except ConnectorRuntimeError as exc: + db.rollback() + raise _http_from_connector_runtime(exc) from exc + if rechecked is None or not rechecked.can_edit: + db.rollback() + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=( + "Your team's access to this Custom API changed while " + "this edit was in flight" + ), + ) + + # The row's declared type from here on is loosened for mypy's sake: the + # column-typed attributes below (name, description, env, ...) are all + # mutated directly by this route, exactly as before this gate existed. + mutable_api = cast(Any, api) + # Read only after the lock: rename_team_connector's "old" argument must + # be the name this transaction actually holds locked, not whatever the + # pre-lock read above saw -- a concurrent committed rename in between + # would otherwise make this stale, and the rewrite below would look for + # a name that no longer exists anywhere, leaving the previous renamer's + # selectors dangling with no error. old_name = str(api.name) # Check name uniqueness if name is changed @@ -326,23 +503,25 @@ async def update_custom_api( status_code=status.HTTP_400_BAD_REQUEST, detail=f"Custom API with name '{api_data.name}' already exists", ) - api.name = api_data.name + mutable_api.name = api_data.name # Update fields if api_data.description is not None: - api.description = api_data.description + mutable_api.description = api_data.description if api_data.url is not None: - api.url = api_data.url + mutable_api.url = api_data.url if api_data.method is not None: - api.method = api_data.method + mutable_api.method = api_data.method if api_data.headers is not None: - api.headers = api_data.headers + mutable_api.headers = api_data.headers if api_data.body is not None: - api.body = api_data.body + mutable_api.body = api_data.body # Process env variables if api_data.env is not None: - existing_env = api.env if isinstance(api.env, dict) else {} + existing_env: Dict[str, str] = ( + mutable_api.env if isinstance(api.env, dict) else {} + ) try: processed_env = _process_env_vars(api_data.env, existing_env) except ValueError as exc: @@ -350,7 +529,7 @@ async def update_custom_api( status_code=status.HTTP_400_BAD_REQUEST, detail=f"Invalid environment variables: {exc}", ) from exc - api.env = processed_env + mutable_api.env = processed_env fields_set = api_data.model_fields_set runtime_input_schema = ( @@ -374,7 +553,7 @@ async def update_custom_api( runtime_input_schema=runtime_input_schema, runtime_bindings=runtime_bindings, allow_delegated_authorization=allow_delegated_authorization, - static_headers=api.headers, + static_headers=mutable_api.headers, ) except ValueError as exc: raise HTTPException( @@ -382,26 +561,37 @@ async def update_custom_api( detail=f"Invalid runtime configuration: {exc}", ) from exc if "runtime_input_schema" in fields_set: - api.runtime_input_schema = runtime_input_schema + mutable_api.runtime_input_schema = runtime_input_schema if "runtime_bindings" in fields_set: - api.runtime_bindings = runtime_bindings + mutable_api.runtime_bindings = runtime_bindings if "allow_delegated_authorization" in fields_set: - api.allow_delegated_authorization = allow_delegated_authorization + mutable_api.allow_delegated_authorization = allow_delegated_authorization from ..services.connector_team_scope import rename_team_connector - rename_team_connector( - db, - int(current_user.id), - "custom_api", - int(api_id), - old_name, - str(api.name), - ) + # The same translation ``update_mcp_server`` gives this call + # (mcp.py:3853-3857): the seam raises its own typed error, and this + # route answers with the status that error declares rather than + # letting it reach the generic handler as a 500. This route has no + # function-wide ``try`` the way the MCP one does, so the arm is local; + # the mapping itself stays in ``_http_from_connector_runtime``, which + # is the module's one place that turns a seam error into an HTTP one. + try: + rename_team_connector( + db, + int(current_user.id), + "custom_api", + int(api_id), + old_name, + str(api.name), + ) + except ConnectorRuntimeError as exc: + db.rollback() + raise _http_from_connector_runtime(exc) from exc # Update UserCustomApi link if api_data.is_active is not None: - user_api.is_active = api_data.is_active # type: ignore[assignment] + user_api.is_active = api_data.is_active db.commit() db.refresh(api) @@ -410,7 +600,7 @@ async def update_custom_api( @custom_api_router.delete("/{api_id}", status_code=status.HTTP_204_NO_CONTENT) -async def delete_custom_api( +def delete_custom_api( api_id: int, current_user: User = Depends(get_current_user), db: Session = Depends(get_db), @@ -455,6 +645,43 @@ async def delete_custom_api( status_code=status.HTTP_403_FORBIDDEN, detail="Only a team admin can delete a team Custom API", ) + + # One global lock order over this pair of tables. ``update_custom_api`` + # locks the ``CustomApi`` definition row first and writes the + # ``UserCustomApi`` link row afterwards; both branches below delete the + # link row first and the definition row second, inside one transaction. + # Without this statement the two routes take the same two rows in + # opposite orders and a concurrent edit/delete pair can deadlock + # (PostgreSQL 40P01). Taken after every refusal above, so a request that + # is going to be refused never acquires the lock. ``populate_existing`` + # matches the PUT's own lock: the row this transaction holds is the one + # the deletion below acts on, not whatever the relationship read above + # happened to see. + # + # This statement orders THIS repository's two tables and nothing else. A + # connector team hook writes its own tables, which this lock does not + # cover, and the two routes reach it in opposite orders relative to this + # lock: the PUT takes the lock above and calls rename_team_connector + # afterwards, while this route calls delete_team_connector before taking + # the lock at all. So an installing application whose hooks lock a row of + # its own can still deadlock against a concurrent edit/delete pair on the + # same connector, and no ordering statement inside this repository can + # prevent that -- the hook side has to take its rows in an order + # compatible with this one, and only the application can arrange that. + locked_api = ( + db.query(CustomApi) + .filter(CustomApi.id == api_id) + .populate_existing() + .with_for_update() + .first() + ) + if locked_api is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Custom API not found", + ) + api = locked_api + if team_delete.team_owned: db.delete(user_api) db.flush([user_api]) diff --git a/src/xagent/web/api/mcp.py b/src/xagent/web/api/mcp.py index 1197390c6f..153ac62005 100644 --- a/src/xagent/web/api/mcp.py +++ b/src/xagent/web/api/mcp.py @@ -13,9 +13,20 @@ import secrets import shlex from collections.abc import Collection -from dataclasses import dataclass +from dataclasses import dataclass, replace from datetime import datetime, timedelta, timezone -from typing import Annotated, Any, Callable, Dict, List, Literal, Optional, Union, cast +from typing import ( + TYPE_CHECKING, + Annotated, + Any, + Callable, + Dict, + List, + Literal, + Optional, + Union, + cast, +) from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit import httpx @@ -27,6 +38,7 @@ from ...config import get_app_base_url, get_public_api_base_url, get_session_secret from ...core.tools.adapters.vibe.connector_runtime import ( + ConnectorRuntimeError, validate_runtime_config_declaration, ) from ...core.tools.core.mcp.data_config import MCPServerConfig @@ -77,6 +89,9 @@ list_scoped_user_oauth_accounts, ) +if TYPE_CHECKING: + from ..services.connector_team_scope import ConnectorAccess, ConnectorRef + logger = logging.getLogger(__name__) MCP_OAUTH_STATE_COOKIE = "xagent_mcp_oauth_state" @@ -1363,11 +1378,25 @@ def _check_mcp_permission( user_mcp: "UserMCPServer | _TeamOwnedUserMCP", is_admin: bool, require: str = "edit", + *, + team_access: "ConnectorAccess | None" = None, ) -> bool: """Whether the user may mutate shared MCP config. ``edit`` gates changes to the shared global config; ``delete`` gates removing the shared server. Admins bypass both. + + ``team_access`` is the caller's team access verdict for this connector + (``None`` when the caller's team does not link it, or when standalone + xagent has no access hook installed at all). It is a fallback only: an + owner's ``is_owner`` still wins the ``edit`` branch outright, with no + verdict consulted at all, and the ``delete`` branch does not read it -- + ``can_delete`` is not part of the verdict this seam reports. + + On the MCP routes the verdict reaching this parameter has already passed + through _team_access_for_shared_row, which withholds edit on a + platform-catalog row; this function itself applies no such test and + trusts what it is handed. """ if is_admin: return True @@ -1377,7 +1406,9 @@ def _check_mcp_permission( # non-owner. Checking is_owner too covers rows created before can_delete # was set (e.g. OAuth provisioning, migration-skipped is_owner rows). return is_owner or bool(getattr(user_mcp, "can_delete", False)) - return is_owner + if is_owner: + return True + return bool(team_access is not None and team_access.can_edit) # Owner-only global fields that are safe to compare (non-secret; secret values @@ -1435,8 +1466,15 @@ def _global_config_tampered(server_data: MCPServerUpdate, server: MCPServer) -> class _TeamOwnedUserMCP: """Stand-in for a missing UserMCPServer row: a team connector the user does - not personally own. Exposes the attributes the response builders read with - not-owned defaults (usable, but not editable/deletable).""" + not personally own. Its class attributes report the same not-owned + defaults a real, ownerless row would (``is_owner``, ``can_edit`` and + ``can_delete`` all ``False``) -- reading the attributes alone never + grants anything. The route-level gate (``_check_mcp_permission``) looks + past those defaults only on the ``edit`` branch, falling back to the + caller's own team access verdict when one links this connector. Nothing + reads past them on the ``delete`` branch: this stand-in grants no delete + right, and none of its attributes changes that. + """ is_owner = False can_edit = False @@ -1451,8 +1489,15 @@ def __init__(self, user_id: int) -> None: class _TeamOwnedUserApi: - """Stand-in for a missing UserCustomApi row (team-owned, not user-owned).""" + """Stand-in for a missing UserCustomApi row (team-owned, not user-owned). + + Same shape as ``_TeamOwnedUserMCP``: ``is_owner`` and ``can_edit`` both + report the not-owned default. ``update_custom_api`` (custom_api.py) + looks past ``can_edit`` for a caller whose own team access verdict + grants edit; there is no delete counterpart for Custom API at all. + """ + is_owner = False can_edit = False is_active = True is_default = False @@ -1461,6 +1506,117 @@ def __init__(self, user_id: int) -> None: self.user_id = int(user_id) +def _resolve_mcp_server_for_request( + db: Session, + user_id: int, + server_id: int, + *, + on_resolution_failure: Literal["raise", "degrade"] = "raise", +) -> "tuple[UserMCPServer | _TeamOwnedUserMCP, MCPServer, ConnectorAccess | None]": + """Resolve the caller's association, the definition row, and the + caller's team access verdict, for ``GET``/``PUT /api/mcp/servers/{id}``. + + Looks up the caller's own personal link row first, with the same + two-table join both routes have always run. When that row exists, the + association and the definition row both come from it and nothing else + runs. When it does not, the definition row is looked up on its own -- + a team-owned connector's shared row must still be found even though + this caller has no personal link to it -- and the caller's team access + verdict decides what happens next: + + - no personal row and no team access (``access is None``) -> 404, the + same outcome every caller without an association has always gotten. + - no personal row but the caller's team links the connector -> the + existing ``_TeamOwnedUserMCP`` stand-in takes the association's + place, the same stand-in the list endpoint's team-owned branch + already constructs. + + An owner's personal row already decides the edit answer on its own -- + ``_check_mcp_permission``'s edit branch returns ``True`` on ``is_owner`` + without ever consulting a verdict -- so resolving one for an owner would + only add an unnecessary hook call; this skips the call entirely for an + owner's row and returns ``access=None``. + + The verdict returned is the downgraded one -- see + _team_access_for_shared_row -- so both the gate and the reported field + below draw on the same object. + + ``on_resolution_failure`` decides what a hook failure means for this + call, and only the caller can know which: ``"raise"`` (the default) + lets ``ConnectorRuntimeError`` propagate to the caller's own + HTTPException translation, appropriate whenever this verdict is a + gate (``PUT`` -- the verdict decides whether the request is even + authorized). ``"degrade"`` reports ``can_edit_global=False`` instead + and lets the request succeed, appropriate only when this verdict is + pure decoration on a field the caller can already read regardless + (``GET`` -- the caller already has a personal row or their team + already cleared the gate above). Degrading without a personal row + would answer "does not exist" for a connector this call merely failed + to ask about, which is why the degrade branch below still raises when + ``user_mcp is None``: the verdict *is* the gate in that case, not a + decoration on top of one. + """ + from ..services.connector_team_scope import resolve_one_connector_access_or_raise + + result = ( + db.query(UserMCPServer, MCPServer) + .join(MCPServer, UserMCPServer.mcpserver_id == MCPServer.id) + .filter(UserMCPServer.user_id == user_id, MCPServer.id == server_id) + .first() + ) + if result is not None: + user_mcp: "UserMCPServer | _TeamOwnedUserMCP | None" = result[0] + server: Optional[MCPServer] = result[1] + else: + user_mcp = None + server = db.query(MCPServer).filter(MCPServer.id == server_id).first() + + already_decided = user_mcp is not None and bool( + getattr(user_mcp, "is_owner", False) + ) + + access: "ConnectorAccess | None" = None + if server is not None and not already_decided: + try: + access = resolve_one_connector_access_or_raise( + db, int(user_id), ("mcp", int(server.id)) + ) + except ConnectorRuntimeError: + # Degrade only when the caller's own personal row already got + # them past the gate. With no personal row the verdict *is* + # the gate, and degrading it to None would answer "does not + # exist" for a connector we merely failed to ask about. + if user_mcp is None or on_resolution_failure == "raise": + raise + logger.warning( + "Connector access resolution failed for MCP server %s " + "while reading it for user %s; reporting " + "can_edit_global=False", + int(server_id), + int(user_id), + ) + access = None + + if user_mcp is None and access is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, detail="MCP server not found" + ) + + if user_mcp is None: + user_mcp = _TeamOwnedUserMCP(int(user_id)) + + # Placed after the 404 above rather than before it. Today that ordering + # is belt-and-braces: this helper never turns a verdict into None, it only + # clears can_edit, so the 404 test above sees a non-None verdict either + # way. The ordering becomes load-bearing the moment the helper starts + # returning None for a row -- ahead of the 404, a catalog connector the + # caller's team genuinely links would read as "not found" to that member + # instead of as read-only. + access = _team_access_for_shared_row(db, cast(MCPServer, server), access) + + return user_mcp, cast(MCPServer, server), access + + def _db_server_to_response( server: MCPServer, user_mcp: UserMCPServer | _TeamOwnedUserMCP, @@ -1469,8 +1625,21 @@ def _db_server_to_response( app_id: Optional[str] = None, provider: Optional[str] = None, is_admin: bool = False, + team_access: "ConnectorAccess | None" = None, ) -> MCPServerResponse: - """Convert database MCPServer to response model.""" + """Convert database MCPServer to response model. + + ``team_access`` is the caller's team access verdict for this connector, + forwarded to ``_check_mcp_permission`` unchanged. ``None`` covers every + case where this function has nothing further to add: the caller's own + personal row already decided the answer so no hook was ever called for + it, a hook was called and genuinely answered "not linked", a hook call + was attempted and failed and the caller degraded rather than failing + the request, or no hook is installed in this deployment at all. Every + caller of this function decides for itself whether resolving a verdict + is worth a hook call before passing one in; this function never + resolves one on its own. + """ # Get status from manager if available config = server.to_config_dict() @@ -1501,7 +1670,9 @@ def _db_server_to_response( runtime_input_schema=server.runtime_input_schema, runtime_bindings=server.runtime_bindings, allow_delegated_authorization=bool(server.allow_delegated_authorization), - can_edit_global=_check_mcp_permission(user_mcp, is_admin, require="edit"), + can_edit_global=_check_mcp_permission( + user_mcp, is_admin, require="edit", team_access=team_access + ), transport_display=server.transport_display, created_at=_format_optional_datetime(server.created_at), updated_at=_format_optional_datetime(server.updated_at), @@ -1514,8 +1685,17 @@ def _db_server_to_response( def _custom_api_to_mcp_response( api: CustomApi, user_api: UserCustomApi | _TeamOwnedUserApi, + team_access: "ConnectorAccess | None" = None, ) -> MCPServerResponse: - """Project a Custom API into the aggregate connector response contract.""" + """Project a Custom API into the aggregate connector response contract. + + ``can_edit_global`` mirrors the exact predicate ``update_custom_api`` + (custom_api.py) gates its write on -- ``user_api.can_edit`` or a + granting team verdict -- duplicated here rather than shared, because + that route's gate lives in a different module this function cannot + reach into. Keep the two in lockstep: this field must never read + ``False`` for a connector whose ``PUT`` would return 2xx. + """ masked_env: dict[str, Any] = _mask_env(api.env) if isinstance(api.env, dict) else {} config: dict[str, Any] = {"env": masked_env} for field_name in ("url", "method", "headers", "body"): @@ -1536,7 +1716,8 @@ def _custom_api_to_mcp_response( runtime_input_schema=api.runtime_input_schema, runtime_bindings=api.runtime_bindings, allow_delegated_authorization=bool(api.allow_delegated_authorization), - can_edit_global=bool(user_api.can_edit), + can_edit_global=bool(user_api.can_edit) + or bool(team_access is not None and team_access.can_edit), transport_display="Custom API", created_at=_format_optional_datetime(api.created_at), updated_at=_format_optional_datetime(api.updated_at), @@ -1593,7 +1774,8 @@ def _catalog_app_keys(app: dict) -> list[str]: after the display name (_ensure_user_mcp_server). Single-sourced so every caller asking "which row is this app's" — the connected-state and shared-row lookups, the names a custom server may not take, the rows the connector - listing must not re-emit, and the rows that carry a platform key — cannot + listing must not re-emit, the rows that carry a platform key, and the rows + a team verdict may not grant edit on — cannot drift apart; one such drift is exactly what #1346 was. Normalized keys only. The raw id/name strings stay in use where a value @@ -1621,8 +1803,10 @@ def _server_catalog_keys(server: MCPServer) -> list[str]: against catalog *ids*, so one whose provider happens to equal some app's id is skipped even if it belongs to another app. Kept on purpose, because the catalog branch claims such a row by provider too (_is_oauth_server_for_app) - — a key this misses is a #1346 duplicate, while a key it over-matches only - moves a legacy row to the Remote tab, still editable via /api/mcp/servers. + — a key this misses is a #1346 duplicate, while a key it over-matches also + loses its team edit right through _team_access_for_shared_row; the row's + own owner is unaffected, since is_owner short-circuits that check in + _check_mcp_permission before any verdict is read. """ if _normalize_app_key(server.transport) != "oauth": return _app_lookup_keys(server.name) @@ -1642,6 +1826,86 @@ def _is_reserved_catalog_name(db: Session, name: object) -> bool: return any(key in _catalog_app_keys(app) for app in get_all_mcp_apps(db)) +def _catalog_reserved_keys(db: Session) -> "set[str]": + """Every normalized key the platform's app catalog claims, in one query. + + The same set ``list_mcp_apps`` builds inline as ``library_keys`` from the + ``library_apps`` list it already holds. Kept separate rather than shared + with that loop because that loop reuses a list it fetched for other + reasons, while the caller below needs the set on its own and only + sometimes. + """ + return {key for app in get_all_mcp_apps(db) for key in _catalog_app_keys(app)} + + +def _team_access_for_shared_row( + db: Session, + server: MCPServer, + access: "ConnectorAccess | None", + *, + reserved_keys: "set[str] | None" = None, +) -> "ConnectorAccess | None": + """The team access verdict as this repo's MCP routes may act on it. + + xagent provisions ONE shared ``MCPServer`` row per catalog app, and every + user who connects that app attaches to that same row; a key-based app's + row may additionally hold the administrator's platform fallback key in + ``env``. That row's configuration is the platform's, not any one team's, + so a verdict that grants edit on it is downgraded here rather than + trusted. Without this, an application answering ``can_edit=True`` for such + a ref would let one team rewrite ``command``/``args``/``url``/``env``/ + ``auth`` for every user of that app, including users in no team at all. + + Only ``can_edit`` is downgraded; ``team_owned`` is left as the application + answered it, so the connector stays visible and readable to the team and + the caller's stand-in resolution is unaffected. Dropping the verdict + entirely would not 404 such a row today -- the sole caller that can raise + a 404 applies this after that test, so the test sees the undowngraded + verdict either way. Keeping ``team_owned`` is what leaves the two + independent: neither this function's return shape nor that caller's + ordering has to hold for a connector the caller's team genuinely links to + stay reachable. + + The catalog test is ``_server_catalog_keys`` against the catalog's own + keys -- the same predicate ``list_mcp_apps`` uses to decide that a stored + row is some catalog app's shared row, so this module holds one definition + of "catalog-managed", not two. Two nearby functions are deliberately NOT + used for it: + + - ``_is_reserved_catalog_name`` answers a different question, "may a new + row take this name", and reads the name alone. A builtin-oauth catalog + row an administrator renamed still carries its ``app_id`` in ``auth`` + and is still the platform's row; that function no longer recognizes it, + and builtin-oauth is 21 of the 28 built-in catalog apps. + - ``_catalog_server_has_platform_key`` answers "catalog row that ALSO + carries the platform key", so every keyless and mcp_oauth row, and every + key-based row whose key each user supplies themselves, reads False there + while still being platform-owned configuration. + + DECLARED BOUNDARY -- a connector someone built themselves under a name a + catalog app later took. The catalog claims that name, so this function + treats such a row as catalog-managed and withholds the team edit. Its + creator keeps their own edit right in full: an owner's ``is_owner`` + decides the edit branch in ``_check_mcp_permission`` before any verdict is + read. What is withheld is only a TEAMMATE editing that connector on the + owner's behalf. Telling such a row apart from a real catalog row needs a + stored "who created this definition" fact the schema does not carry today; + until it does, this is the side the ambiguity is resolved on, on purpose. + + ``reserved_keys`` lets a caller resolving many rows in one request build + the key set once and pass it in. The test runs only for a verdict that + already grants edit -- the one case where it can change an answer -- so a + deployment with no access hook installed resolves ``None`` for every row + and issues no additional query at all. + """ + if access is None or not access.can_edit: + return access + keys = _catalog_reserved_keys(db) if reserved_keys is None else reserved_keys + if not keys.intersection(_server_catalog_keys(server)): + return access + return replace(access, can_edit=False) + + def _oauth_account_can_connect(oauth_account: object) -> bool: access_token = getattr(oauth_account, "access_token", None) if not access_token: @@ -2032,39 +2296,52 @@ def _local_mcp_can_authorize( def _local_mcp_can_configure( association: Union[UserMCPServer, UserCustomApi, None], + team_access: "ConnectorAccess | None" = None, ) -> bool: """Whether this viewer's configuration route would resolve for a local entry. One rule for both local branches: the four routes the picker's Configure - button reaches all take the same first gate -- a personal association row - for the calling user -- and answer 404 without one. ``GET``/``PUT - /api/mcp/servers/{id}`` (mcp.py) and ``GET``/``PUT /api/custom-apis/{id}`` - (custom_api.py) each query by ``user_id`` + connector id and raise 404 on - an empty result, which is why a team-owned connector reaching a member - through the visibility overlay alone (``association is None``) is not - configurable however visible or attachable it is. - - Deliberately reads nothing but the association's existence: + button reaches -- ``GET``/``PUT /api/mcp/servers/{id}`` (mcp.py) and + ``GET``/``PUT /api/custom-apis/{id}`` (custom_api.py) -- each resolve the + caller from the same two sources: a personal association row for the + calling user, or, when there is none, the caller's team access verdict + for the connector. Either source alone is enough to reach the route; + 404 only when both are absent. A team-owned connector reaching a member + through the visibility overlay alone (``association is None``) is + therefore configurable exactly when that member's own verdict links it + (``team_access is not None``), independent of whatever the visibility + overlay itself decided. + + Deliberately reads nothing else: - Not the connector's shape. Unlike ``can_attach``/``can_authorize``, no route this answers for treats the mcp_oauth shape differently. - Not ``is_active``. Neither route filters it, so a deactivated connector's owner can still open and save its form -- and withholding the button there would remove the only affordance that population has left. - - Not ``can_edit``. Existence alone is what the four routes' first gate - reads, and it is what this answers. Custom API's ``PUT`` has a second, - owner-side gate on ``can_edit`` (403), so this field's accuracy there - rests on a convention rather than an identity: the one production write - point sets ``can_edit=True`` (custom_api.py), and no other code path - creates the row. A future writer that leaves the column at its ``False`` - default would make this field claim an editable entry whose save is - refused -- add that gate here if that ever happens. + - Not ``can_edit``, and not the verdict's own ``can_edit`` field. A + verdict that links the connector but denies edit still resolves the + route -- the form opens, and a save attempt is refused owner-side, not + here. Existence of either source is what the four routes' first gate + reads, and it is what this answers. Both kinds' ``PUT`` now carry that + owner-side refusal (403): MCP's route refuses a stand-in whose verdict + denies edit outright, before the shared-config tamper check ever runs; + Custom API's route reads ``can_edit``/the verdict as its own gate. For + MCP, that refusal is a structural check against the same verdict this + function reads, so it never drifts from what this field reports. For + Custom API, this field's accuracy rests on a convention rather than an + identity: the one production write point sets ``can_edit=True`` + (custom_api.py), and no other code path creates the row. A future + writer that leaves the column at its ``False`` default would make this + field claim an editable entry whose save is refused -- add that gate + here if that ever happens. This is a UI hint, never a permission. Editing the shared configuration is additionally gated owner-side (``_check_mcp_permission(require="edit")`` - for MCP, ``can_edit`` for Custom API), and a forged value grants nothing. + for MCP, ``can_edit``/the verdict for Custom API), and a forged value + grants nothing. """ - return association is not None + return association is not None or team_access is not None @mcp_router.get("/apps", response_model=List[dict]) @@ -2268,6 +2545,7 @@ def list_mcp_apps( # they always did. from ..services.connector_team_scope import ( connector_visible_to_user, + resolve_connector_access_or_raise, visible_team_connector_ids, ) @@ -2315,6 +2593,71 @@ def list_mcp_apps( # fixable from the Tools page, unreachable from the picker. A team-shared # catalog connector loses its only picker entry the same way, which is # pre-existing for most apps and tracked in #1387. + # Custom APIs: same overlay as the MCP half above, moved up (out of + # its original position after the MCP loop) so both halves' team- + # owned rows are known before the single batched access call below. + user_custom_apis = ( + db.query(UserCustomApi, CustomApi) + .join(CustomApi, UserCustomApi.custom_api_id == CustomApi.id) + .filter(UserCustomApi.user_id == current_user.id) + .all() + ) + + # Same overlay as the MCP half above: a team-owned Custom API has no + # UserCustomApi row for the member, so it is carried as (api, None). + # The association is read for can_attach and can_configure below — a + # team-owned API is one the runtime overlays by id, exactly like the + # MCP half. + local_custom_apis: list[tuple[CustomApi, UserCustomApi | None]] = [ + (api, user_api) for user_api, api in user_custom_apis + ] + own_api_ids = {cast(int, api.id) for api, _ in local_custom_apis} + missing_api = [aid for aid in team_ids["custom_api"] if aid not in own_api_ids] + if missing_api: + local_custom_apis.extend( + (api, None) + for api in db.query(CustomApi) + .filter(CustomApi.id.in_(missing_api)) + .all() + ) + + # One batched call covering every stand-in row across both halves -- + # a personal row (user_mcp/user_api is not None) already answers + # can_configure on its own and needs no verdict at all. A resolution + # failure here degrades every stand-in row's can_configure to False + # rather than failing the whole listing -- the same per-row + # degradation this route has always offered, now paid for with one + # hook call instead of one per row. + access_refs: "set[ConnectorRef]" = { + ("mcp", cast(int, server.id)) + for server, user_mcp in local_mcps + if user_mcp is None + } | { + ("custom_api", cast(int, api.id)) + for api, user_api in local_custom_apis + if user_api is None + } + verdicts: "dict[ConnectorRef, ConnectorAccess]" = {} + if access_refs: + # Captured before the resolution call below: a failed hook can + # leave the shared session in a state where a lazy ORM attribute + # read triggers a query of its own, so the log line below reads + # a plain int gathered ahead of time rather than current_user.id + # off the row. + user_id_for_log = int(current_user.id) + try: + verdicts = resolve_connector_access_or_raise( + db, user_id_for_log, access_refs + ) + except ConnectorRuntimeError: + logger.warning( + "Connector access resolution failed while listing %s " + "local connectors for user %s; reporting " + "can_configure=False for those rows", + len(access_refs), + user_id_for_log, + ) + library_keys = {key for app in library_apps for key in _catalog_app_keys(app)} for server, user_mcp in local_mcps: if library_keys.intersection(_server_catalog_keys(server)): @@ -2331,6 +2674,18 @@ def list_mcp_apps( if category and category != "All": continue + # A personal row already answers can_configure on its own; only + # a team-owned row with none (user_mcp is None) needs a verdict, + # looked up from the batch answer computed once above. A ref + # missing from that answer -- because the caller's team does not + # link it, or because the whole batch call failed and was + # degraded -- reports can_configure=False for this row alone. + local_team_access: "ConnectorAccess | None" = ( + verdicts.get(("mcp", cast(int, server.id))) + if user_mcp is None + else None + ) + entry = { "id": server.name, "name": server.name, @@ -2362,7 +2717,7 @@ def list_mcp_apps( user_mcp, token_resolver_installed=token_resolver_installed, ), - "can_configure": _local_mcp_can_configure(user_mcp), + "can_configure": _local_mcp_can_configure(user_mcp, local_team_access), } # The picker dispatches its Connect button on auth_type, and custom # entries used to omit the field entirely — so an mcp_oauth server @@ -2392,32 +2747,8 @@ def list_mcp_apps( results.append(entry) - # Append Custom APIs - user_custom_apis = ( - db.query(UserCustomApi, CustomApi) - .join(CustomApi, UserCustomApi.custom_api_id == CustomApi.id) - .filter(UserCustomApi.user_id == current_user.id) - .all() - ) - - # Same overlay as the MCP half above: a team-owned Custom API has no - # UserCustomApi row for the member, so it is carried as (api, None). - # The association is read for can_attach and can_configure below — a - # team-owned API is one the runtime overlays by id, exactly like the - # MCP half. - local_custom_apis: list[tuple[CustomApi, UserCustomApi | None]] = [ - (api, user_api) for user_api, api in user_custom_apis - ] - own_api_ids = {cast(int, api.id) for api, _ in local_custom_apis} - missing_api = [aid for aid in team_ids["custom_api"] if aid not in own_api_ids] - if missing_api: - local_custom_apis.extend( - (api, None) - for api in db.query(CustomApi) - .filter(CustomApi.id.in_(missing_api)) - .all() - ) - + # Append Custom APIs (query and list assembled above, before the + # batched access call). for api, user_api in local_custom_apis: if search: search_lower = search.lower() @@ -2429,6 +2760,15 @@ def list_mcp_apps( if category and category != "All": continue + # Same batch lookup as the MCP loop above: only a stand-in row + # (user_api is None) needs a verdict, and a ref missing from the + # batch answer degrades this row's can_configure to False. + local_team_access = ( + verdicts.get(("custom_api", cast(int, api.id))) + if user_api is None + else None + ) + results.append( { "id": api.name, @@ -2457,7 +2797,9 @@ def list_mcp_apps( team_ids=team_ids["custom_api"], ), "can_authorize": False, - "can_configure": _local_mcp_can_configure(user_api), + "can_configure": _local_mcp_can_configure( + user_api, local_team_access + ), "runtime_input_schema": api.runtime_input_schema, "runtime_bindings": api.runtime_bindings, "allow_delegated_authorization": bool( @@ -2506,12 +2848,113 @@ def get_mcp_servers( if oauth.email and _oauth_account_can_connect(oauth) } + from ..services.connector_team_scope import ( + resolve_connector_access_or_raise, + visible_team_connector_ids, + ) + + # Every query this route needs is run up front, before the single + # batched access call below, so every row needing a verdict is known + # in one place. Order here does not affect the response: the four + # append loops further down (personal MCP, personal Custom API, + # stand-in MCP, stand-in Custom API) preserve the exact row order + # this route has always produced. + user_custom_apis = ( + db.query(UserCustomApi, CustomApi) + .join(CustomApi, UserCustomApi.custom_api_id == CustomApi.id) + .filter(UserCustomApi.user_id == effective_user_id) + .all() + ) + + # Team-owned connectors the user has no personal row for, so a team + # member sees the team's shared connectors in their own list. + team_ids = visible_team_connector_ids(db, effective_user_id) + + own_mcp_ids = {int(server.id) for _um, server in user_mcps} + missing_mcp = [sid for sid in team_ids["mcp"] if sid not in own_mcp_ids] + stand_in_mcp_servers = ( + db.query(MCPServer).filter(MCPServer.id.in_(missing_mcp)).all() + if missing_mcp + else [] + ) + + own_api_ids = {int(api.id) for _ua, api in user_custom_apis} + missing_api = [aid for aid in team_ids["custom_api"] if aid not in own_api_ids] + stand_in_apis = ( + db.query(CustomApi).filter(CustomApi.id.in_(missing_api)).all() + if missing_api + else [] + ) + + # One batched call for every row this listing needs a verdict for. + # An owner's reported right cannot change with a verdict (the edit + # branch returns True on is_owner alone), so only a non-owner + # personal row is worth asking about; a stand-in row holds no + # personal row at all and is unconditionally worth asking about. + access_refs: "set[ConnectorRef]" = ( + { + ("mcp", int(server.id)) + for user_mcp, server in user_mcps + if not bool(getattr(user_mcp, "is_owner", False)) + } + | { + ("custom_api", int(api.id)) + for user_api, api in user_custom_apis + if not bool(getattr(user_api, "is_owner", False)) + } + | {("mcp", int(server.id)) for server in stand_in_mcp_servers} + | {("custom_api", int(api.id)) for api in stand_in_apis} + ) + # A resolution failure here degrades every row that still needed a + # verdict to can_edit_global=False rather than failing the whole + # list: this call is a single batch, so it either succeeds for + # every row asked about or fails for all of them together -- there + # is no partial-failure mode to preserve at this granularity. A + # per-connector granularity still exists and is preserved: a + # verdict genuinely missing from a *successful* answer degrades + # only that one row, the same as before batching. + verdicts: "dict[ConnectorRef, ConnectorAccess]" = {} + if access_refs: + try: + verdicts = resolve_connector_access_or_raise( + db, effective_user_id, access_refs + ) + except ConnectorRuntimeError: + logger.warning( + "Connector access resolution failed while listing %s " + "connectors for user %s; reporting can_edit_global=False " + "for those rows", + len(access_refs), + effective_user_id, + ) + + # Built once per request, and only when some MCP verdict actually + # grants edit -- the downgrade below is its only reader. A listing + # with no granting verdict (every standalone deployment, and every + # team listing where nothing is editable) must cost exactly what it + # cost before this existed. + reserved_keys: "set[str] | None" = None + if any( + verdict.can_edit + for (kind, _connector_id), verdict in verdicts.items() + if kind == "mcp" + ): + reserved_keys = _catalog_reserved_keys(db) + is_admin = getattr(current_user, "is_admin", False) responses = [] for user_mcp, server in user_mcps: app_id, provider, connected_account = _enrich_oauth_server_info( db, server, oauth_emails ) + team_access = _team_access_for_shared_row( + db, + server, + None + if bool(getattr(user_mcp, "is_owner", False)) + else verdicts.get(("mcp", int(server.id))), + reserved_keys=reserved_keys, + ) responses.append( _db_server_to_response( server, @@ -2521,61 +2964,61 @@ def get_mcp_servers( app_id, provider, is_admin=is_admin, + team_access=team_access, ) ) - # Append Custom APIs - user_custom_apis = ( - db.query(UserCustomApi, CustomApi) - .join(CustomApi, UserCustomApi.custom_api_id == CustomApi.id) - .filter(UserCustomApi.user_id == effective_user_id) - .all() - ) - for user_api, api in user_custom_apis: - responses.append(_custom_api_to_mcp_response(api, user_api)) - - # Append team-owned connectors the user has no personal row for, so a - # team member sees the team's shared connectors in their own list. - from ..services.connector_team_scope import visible_team_connector_ids - - team_ids = visible_team_connector_ids(db, effective_user_id) + team_access = ( + None + if bool(getattr(user_api, "is_owner", False)) + else verdicts.get(("custom_api", int(api.id))) + ) + responses.append( + _custom_api_to_mcp_response(api, user_api, team_access=team_access) + ) - own_mcp_ids = {int(server.id) for _um, server in user_mcps} - missing_mcp = [sid for sid in team_ids["mcp"] if sid not in own_mcp_ids] - if missing_mcp: - for server in ( - db.query(MCPServer).filter(MCPServer.id.in_(missing_mcp)).all() - ): - app_id, provider, connected_account = _enrich_oauth_server_info( - db, server, oauth_emails - ) - responses.append( - _db_server_to_response( - server, - _TeamOwnedUserMCP(effective_user_id), - manager, - connected_account, - app_id, - provider, - is_admin=is_admin, - ) + for server in stand_in_mcp_servers: + app_id, provider, connected_account = _enrich_oauth_server_info( + db, server, oauth_emails + ) + team_access = _team_access_for_shared_row( + db, + server, + verdicts.get(("mcp", int(server.id))), + reserved_keys=reserved_keys, + ) + responses.append( + _db_server_to_response( + server, + _TeamOwnedUserMCP(effective_user_id), + manager, + connected_account, + app_id, + provider, + is_admin=is_admin, + team_access=team_access, ) + ) - own_api_ids = {int(api.id) for _ua, api in user_custom_apis} - missing_api = [aid for aid in team_ids["custom_api"] if aid not in own_api_ids] - if missing_api: - for api in db.query(CustomApi).filter(CustomApi.id.in_(missing_api)).all(): - responses.append( - _custom_api_to_mcp_response( - api, _TeamOwnedUserApi(effective_user_id) - ) + for api in stand_in_apis: + team_access = verdicts.get(("custom_api", int(api.id))) + responses.append( + _custom_api_to_mcp_response( + api, + _TeamOwnedUserApi(effective_user_id), + team_access=team_access, ) + ) return responses except HTTPException: raise + except ConnectorRuntimeError as exc: + raise HTTPException( + status_code=exc.status_code, detail=exc.safe_message + ) from exc except Exception as e: logger.error(f"Failed to list MCP servers: {e}") raise HTTPException( @@ -2595,21 +3038,15 @@ def get_mcp_server( manager = DatabaseMCPServerManager(db) user_id = current_user.id - # Check user has access to this server - result = ( - db.query(UserMCPServer, MCPServer) - .join(MCPServer, UserMCPServer.mcpserver_id == MCPServer.id) - .filter(UserMCPServer.user_id == user_id, MCPServer.id == server_id) - .first() + # Check user has access to this server: a personal row, or a team + # access verdict for a connector the caller has none for. The + # verdict is pure decoration on this read path -- degrade it to + # can_edit_global=False on a resolution failure rather than + # failing the whole read. + user_mcp, server, team_access = _resolve_mcp_server_for_request( + db, int(user_id), server_id, on_resolution_failure="degrade" ) - if not result: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, detail="MCP server not found" - ) - - user_mcp, server = result - # Actor credentials are not personal server connections. oauth_accounts = list_scoped_user_oauth_accounts( db, @@ -2634,10 +3071,15 @@ def get_mcp_server( app_id, provider, is_admin=getattr(current_user, "is_admin", False), + team_access=team_access, ) except HTTPException: raise + except ConnectorRuntimeError as exc: + raise HTTPException( + status_code=exc.status_code, detail=exc.safe_message + ) from exc except Exception as e: logger.error(f"Failed to get MCP server: {e}") raise HTTPException( @@ -3016,12 +3458,49 @@ def _apply_updates(a: Any) -> None: db.refresh(assoc) logger.info(f"User {current_user.id} connected MCP app '{server_name}'") + # assoc is a personal row this call just created or updated, always with + # is_owner=False (connecting never grants ownership) -- resolved so the + # response's can_edit_global comes from the same object the PUT gate + # would read. Every row this route returns is a catalog app's shared + # row, so the downgrade below makes that False -- the point is that it + # is False for the same reason the gate would refuse, not that it + # defaults to False. + # The association has already committed by this point, so a verdict + # failure here must not fail the request -- it only degrades + # can_edit_global to False, the value this route always reported before + # the verdict existed at all. + from ..services.connector_team_scope import resolve_one_connector_access_or_raise + + # Captured before the resolution call below: a failed hook can leave the + # shared session in a state where a lazy ORM attribute read triggers a + # query of its own, so the log line below reads plain ints gathered + # ahead of time rather than server.id/current_user.id off the row. + server_id_for_log = int(server.id) + user_id_for_log = int(current_user.id) + + team_access: "ConnectorAccess | None" = None + try: + team_access = resolve_one_connector_access_or_raise( + db, user_id_for_log, ("mcp", server_id_for_log) + ) + except ConnectorRuntimeError: + logger.warning( + "Connector access resolution failed for MCP server %s after " + "connecting it for user %s; reporting can_edit_global=False", + server_id_for_log, + user_id_for_log, + ) + # Every row this route returns is a catalog app's shared row, so this is + # what keeps its reported can_edit_global from advertising an edit the PUT + # gate would refuse. + team_access = _team_access_for_shared_row(db, server, team_access) return _db_server_to_response( server, assoc, manager, app_id=str(app_info["id"]), is_admin=getattr(current_user, "is_admin", False), + team_access=team_access, ) @@ -3198,6 +3677,9 @@ def create_mcp_server( db.refresh(user_mcp) logger.info(f"Created MCP server '{server_data.name}' for user {user_id}") + # No verdict to resolve: user_mcp was just constructed above with + # is_owner=True, so _check_mcp_permission's edit branch returns True + # on that alone -- a team access verdict could not change the value. return _db_server_to_response( server, user_mcp, manager, is_admin=getattr(current_user, "is_admin", False) ) @@ -3225,24 +3707,158 @@ def update_mcp_server( manager = DatabaseMCPServerManager(db) user_id = current_user.id - # Check user has access to this server - result = ( - db.query(UserMCPServer, MCPServer) - .join(MCPServer, UserMCPServer.mcpserver_id == MCPServer.id) - .filter(UserMCPServer.user_id == user_id, MCPServer.id == server_id) - .first() + # Check user has access to this server: a personal row, or a team + # access verdict for a connector the caller has none for. + user_mcp, server, team_access = _resolve_mcp_server_for_request( + db, int(user_id), server_id + ) + is_stand_in = isinstance(user_mcp, _TeamOwnedUserMCP) + can_edit_global = _check_mcp_permission( + user_mcp, + getattr(current_user, "is_admin", False), + require="edit", + team_access=team_access, ) - if not result: + # user_env and is_active both live on the personal association row; + # a caller with no personal row (the stand-in) has none to hold + # them, so a payload carrying either must be rejected outright -- + # silently dropping them would report a 200 for a write that never + # happened. This is independent of can_edit_global: even a team + # editor with edit rights on the shared config has no personal row + # of their own to store a per-user override or activation flag on. + if is_stand_in and ( + server_data.user_env is not None or server_data.is_active is not None + ): + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=( + "No personal connection exists to configure user_env or " + "is_active for this server" + ), + ) + + # A stand-in whose verdict denies edit has an empty writable field + # set: the guard above refuses the personal fields (there is no + # personal row to hold them), the tamper check below refuses every + # shared field it can compare, and the ones it deliberately cannot + # compare (secrets) are emptied out of the payload. Every payload + # this caller can send therefore either fails already or commits + # nothing -- and a 200 for a write that provably cannot change + # anything reports success for a request that had none. Ordered + # after the personal-field guard on purpose: "there is no personal + # connection to configure this on" is the more precise answer for + # that payload, and design matrix 43.1's `PUT personal only` cell + # for this population stays 400. + if is_stand_in and not can_edit_global: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="You do not have permission to edit this MCP server", + ) + + # A second, single-table lock on the definition row, taken before any + # tamper check or config build below reads or mutates it. The read + # above is a two-table join and cannot itself lock just this table; + # this is a fresh statement, so a row deleted between the two still + # yields None here (handled as the same 404) rather than surfacing + # as an unrelated error out of the write path below. + locked_server = ( + db.query(MCPServer) + .filter(MCPServer.id == server_id) + .populate_existing() + .with_for_update() + .first() + ) + if locked_server is None: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="MCP server not found" ) + server = locked_server + + # This re-resolve assumes READ COMMITTED, PostgreSQL's default, which + # this codebase sets no isolation_level on its engine to change: it + # needs a fresh snapshot to see a link the application revoked and + # committed after this request's pre-lock read. Under REPEATABLE READ + # or SERIALIZABLE the re-read reuses this transaction's original + # snapshot, sees the pre-lock answer again, and the recheck degrades + # to a no-op -- it would stop refusing, not start refusing wrongly. + # + # The verdict above was resolved before this lock existed, and the + # application that answers it can revoke the team's link at any + # moment -- it writes its own tables, which this lock does not + # cover. Re-resolve it here, while this transaction holds the + # definition row, and refuse if the answer no longer grants what + # the pre-lock answer granted. This narrows the window; it is not + # a fence, and cannot be one from inside this repository: the + # revoke path lives in the application that installs the hook, and + # a real fence needs both sides to take the same lock. The downgrade + # is re-applied here too, against the locked row: the name and auth + # it reads are mutable through this very route. + # + # Skipped for a payload that only touches this caller's own + # association row, and for a platform admin: neither writes on the + # verdict's authority (see _check_mcp_permission, which answers + # True on is_admin before it ever reads the verdict). + # + # Placed before any field below is read or mutated and before + # rename_team_connector runs, so a refusal here has nothing to + # undo -- zero side effects is structural, not something the + # rollback has to achieve. + # + # This set must stay in sync with every field this route writes + # onto the caller's personal association row (currently user_env + # and is_active, below): adding a third such field without adding + # it here would silently start subjecting a personal-only payload + # to the recheck too. + payload_is_personal_only = set(server_data.model_fields_set) <= { + "user_env", + "is_active", + } + if ( + team_access is not None + and team_access.can_edit + and not getattr(current_user, "is_admin", False) + and not payload_is_personal_only + ): + from ..services.connector_team_scope import ( + resolve_one_connector_access_or_raise, + ) - user_mcp, server = result + # Re-derived from the row this transaction holds locked, not from + # the pre-lock read: the name and auth the catalog test reads are + # both mutable through this very route. + rechecked = _team_access_for_shared_row( + db, + server, + resolve_one_connector_access_or_raise( + db, int(user_id), ("mcp", int(server_id)) + ), + ) + if rechecked is None or not rechecked.can_edit: + db.rollback() + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=( + "Your team's access to this MCP server changed while " + "this edit was in flight" + ), + ) + # The response below reports the verdict this write was + # actually authorized on, not the one resolved before the lock + # existed. Only rebound where the recheck ran: where it was + # skipped there is no fresher answer to report, and asking for + # one would cost a hook call on exactly the paths §59.2 of the + # design removed it from. + team_access = rechecked + + # Read only after the lock: rename_team_connector's "old" argument + # must be the name this transaction actually holds locked, not + # whatever was there at the pre-lock read above -- a concurrent + # committed rename in between would otherwise make this stale, and + # the rewrite below would then look for a name that no longer + # exists anywhere, leaving the previous renamer's selectors + # dangling with no error. old_name = str(server.name) - can_edit_global = _check_mcp_permission( - user_mcp, getattr(current_user, "is_admin", False), require="edit" - ) # Non-owners may not touch the shared global config (env, command, etc.); # they only get to set their own per-user env override below. Reject a @@ -3358,7 +3974,7 @@ def update_mcp_server( status_code=status.HTTP_400_BAD_REQUEST, detail=f"Invalid user environment variables: {exc}", ) from exc - user_mcp.env = encrypt_env_dict(merged_user_env) or None + cast(Any, user_mcp).env = encrypt_env_dict(merged_user_env) or None # Update user association if needed if server_data.is_active is not None: @@ -3372,15 +3988,26 @@ def update_mcp_server( db.commit() db.refresh(server) - db.refresh(user_mcp) + # The stand-in is not an ORM instance -- there is no row to refresh. + if not is_stand_in: + db.refresh(user_mcp) logger.info(f"Updated MCP server '{server.name}' for user {user_id}") return _db_server_to_response( - server, user_mcp, manager, is_admin=getattr(current_user, "is_admin", False) + server, + user_mcp, + manager, + is_admin=getattr(current_user, "is_admin", False), + team_access=team_access, ) except HTTPException: raise + except ConnectorRuntimeError as exc: + db.rollback() + raise HTTPException( + status_code=exc.status_code, detail=exc.safe_message + ) from exc except Exception as e: db.rollback() logger.error(f"Failed to update MCP server: {e}") @@ -3609,7 +4236,7 @@ async def delete_mcp_server( @mcp_router.post("/servers/{server_id}/toggle", response_model=MCPServerResponse) -async def toggle_mcp_server( +def toggle_mcp_server( server_id: int, current_user: User = Depends(get_current_user), db: Session = Depends(get_db), @@ -3644,8 +4271,45 @@ async def toggle_mcp_server( f"{status_text.capitalize()} MCP server '{server.name}' for user {user_id}" ) + # The gate above is unchanged (still 404s without a personal row, + # owner or not); only the reported field below draws on a team + # verdict -- now the downgraded one, so a catalog row's reported + # field cannot advertise an edit the PUT gate refuses. The toggle + # has already committed by the time this runs, so a verdict failure + # here must not fail the request -- it only degrades + # can_edit_global to False, the same answer this route reported + # before the verdict existed at all. + from ..services.connector_team_scope import ( + resolve_one_connector_access_or_raise, + ) + + # Captured before the resolution call below: a failed hook can leave + # the shared session in a state where a lazy ORM attribute read + # triggers a query of its own, so the log line below reads plain + # ints gathered ahead of time rather than server.id/user_id off the + # row. + server_id_for_log = int(server.id) + user_id_for_log = int(user_id) + + team_access: "ConnectorAccess | None" = None + try: + team_access = resolve_one_connector_access_or_raise( + db, user_id_for_log, ("mcp", server_id_for_log) + ) + except ConnectorRuntimeError: + logger.warning( + "Connector access resolution failed for MCP server %s after " + "toggling it for user %s; reporting can_edit_global=False", + server_id_for_log, + user_id_for_log, + ) + team_access = _team_access_for_shared_row(db, server, team_access) return _db_server_to_response( - server, user_mcp, manager, is_admin=getattr(current_user, "is_admin", False) + server, + user_mcp, + manager, + is_admin=getattr(current_user, "is_admin", False), + team_access=team_access, ) except HTTPException: diff --git a/src/xagent/web/services/connector_team_scope.py b/src/xagent/web/services/connector_team_scope.py index 61f22ed852..97ac04cf82 100644 --- a/src/xagent/web/services/connector_team_scope.py +++ b/src/xagent/web/services/connector_team_scope.py @@ -8,9 +8,10 @@ from __future__ import annotations import logging -from collections.abc import Callable, Collection +from collections.abc import Callable, Collection, Iterator +from contextlib import contextmanager from dataclasses import dataclass -from typing import TYPE_CHECKING, Any, Literal, Protocol +from typing import TYPE_CHECKING, Any, Literal, Protocol, TypeVar from sqlalchemy.sql.elements import ColumnElement @@ -42,6 +43,34 @@ class ConnectorDeleteDecision: ConnectorDeletedHook = Callable[[Any, int, ConnectorType, int], ConnectorDeleteDecision] + +@dataclass(frozen=True) +class ConnectorAccess: + """Whether the caller's team links a connector, and may edit it. + + A verdict that reaches a caller always carries ``team_owned=True``: + the only way to say "the caller's team does not link this connector" + is to leave its ref out of the hook's answer map entirely, not to + return a verdict with ``team_owned=False``. ``can_edit`` is otherwise + independent -- a team can link a connector without granting edit + rights to it, which is a legal answer on its own, not an intermediate + or partial state. Both fields are validated as exact bools on the way + in (see ``_validate_connector_access_answer``); the dataclass defaults + below stay ``False``/``False`` on purpose so that constructing a bare + ``ConnectorAccess()`` remains the shape the validator rejects, rather + than quietly becoming a legitimate "not linked" answer. + """ + + team_owned: bool = False + can_edit: bool = False + + +ConnectorRef = tuple[ConnectorType, int] + +ConnectorAccessHook = Callable[ + [Any, int, "Collection[ConnectorRef]"], "dict[ConnectorRef, ConnectorAccess]" +] + ConnectorVisibilityHook = Callable[[Any, int], dict[str, set[int]]] @@ -110,6 +139,7 @@ def __call__(self, db: Any, *, team_id: int) -> dict[str, set[int]]: ... _connector_renamed_hook: ConnectorRenamedHook | None = None _connector_visibility_hook: ConnectorVisibilityHook | None = None _team_connector_visibility_hook: TeamConnectorVisibilityHook | None = None +_connector_access_hook: ConnectorAccessHook | None = None def set_connector_team_hooks( @@ -118,6 +148,7 @@ def set_connector_team_hooks( renamed: ConnectorRenamedHook | None = None, visibility: ConnectorVisibilityHook | None = None, team_visibility: TeamConnectorVisibilityHook | None = None, + access: ConnectorAccessHook | None = None, ) -> None: """Install application-owned connector lifecycle hooks. @@ -129,17 +160,37 @@ def set_connector_team_hooks( global _connector_deleted_hook, _connector_renamed_hook global _connector_visibility_hook, _team_connector_visibility_hook + global _connector_access_hook _connector_deleted_hook = deleted _connector_renamed_hook = renamed _connector_visibility_hook = visibility _team_connector_visibility_hook = team_visibility + _connector_access_hook = access def visible_team_connector_ids(db: Any, user_id: int) -> dict[str, set[int]]: - """Team-shared connector ids visible to user; empty when no hook/standalone.""" + """Team-shared connector ids visible to user; empty when no hook/standalone. + + Answers list membership only. Direct-id reachability and edit + authority come from a different hook -- ``resolve_connector_access`` + -- and xagent enforces no relationship between the two answers: + they are separate module-level slots, installed separately, and + nothing cross-checks them. An installing application must derive + both from one and the same link query, because it is the only side + that can see its own link table; xagent cannot verify that and does + not try. + + When the two answers disagree, xagent answers each question from the + hook that owns it and does not reconcile them: a connector this hook + returns but the access hook omits appears in the listing and 404s on + direct id; a connector the access hook grants but this hook omits is + absent from the listing and still reachable and editable by id. + Neither is a defect in xagent -- both are what the installed answers + said. + """ if _connector_visibility_hook is None: return {"mcp": set(), "custom_api": set()} - return _connector_visibility_hook(db, int(user_id)) + return _call_connector_hook_gate(db, _connector_visibility_hook, db, int(user_id)) def _validate_team_connector_answer(answer: Any) -> dict[str, set[int]]: @@ -199,8 +250,13 @@ def team_connector_ids(db: Any, *, team_id: int | None) -> dict[str, set[int]]: """ if team_id is None or _team_connector_visibility_hook is None: return {"mcp": set(), "custom_api": set()} - answer = _team_connector_visibility_hook(db, team_id=int(team_id)) - return _validate_team_connector_answer(answer) + return _call_connector_hook_gate( + db, + _team_connector_visibility_hook, + db, + team_id=int(team_id), + validate=_validate_team_connector_answer, + ) def team_connector_hook_installed() -> bool: @@ -212,6 +268,236 @@ def team_connector_hook_installed() -> bool: return _team_connector_visibility_hook is not None +def _validate_connector_access_answer( + answer: Any, requested: "frozenset[ConnectorRef]" +) -> "dict[ConnectorRef, ConnectorAccess]": + """Validate the access hook's batch answer shape. + + An authorization input, not user-facing data: a malformed answer must + fail loudly, never be normalized, coerced, or defaulted to empty. The + hook answers a ``dict`` keyed on the connectors it was asked about; a + connector the caller's team does not link is expressed by leaving its + ref out of the answer entirely, never by a verdict with + ``team_owned=False`` -- unlike the team-visibility hook's + ``_validate_team_connector_answer`` above, where extra keys beyond the + two required ones are silently accepted because nothing ever reads + them, here the keys of the answer *are* the question: a verdict for a + ref that was never asked about means the hook answered a different + question than the one it was asked, and silently dropping it would + hide that the hook and the caller have gone out of sync. + + Each value must be a ``ConnectorAccess`` with ``team_owned`` exactly + ``True`` (an identity check, not a truthiness check, matching + ``knowledge_base_team_scope.py``'s ``element.team_owned is not True``) + and ``can_edit`` exactly ``True`` or ``False`` -- ``bool`` is a + subclass of ``int`` in Python, so a merely truthy value is never + accepted as a legitimate grant. + + Each key must be an exact ``(str, int)`` pair before it is even checked + for membership: ``bool``, ``float`` and ``Decimal`` all compare equal + to the ``int`` they alias (``True == 1``, ``1.0 == 1``, + ``Decimal("1") == 1``), and Python's ordinary tuple equality carries + that through to a key like ``("mcp", True)`` -- which would compare + equal to, and pass the membership check for, ``("mcp", 1)``. The keys + of this answer *are* the question (see above), so a key that only + resembles one of the refs asked about is not a legitimate answer to + the question, and must fail loudly here rather than being accepted as + the connector it merely aliases. + """ + if not isinstance(answer, dict): + raise ValueError( + "connector access hook returned a malformed answer: expected a " + f"dict, got {type(answer).__name__}" + ) + validated: "dict[ConnectorRef, ConnectorAccess]" = {} + for key, verdict in answer.items(): + if not isinstance(key, tuple) or len(key) != 2: + raise ValueError( + "connector access hook returned a malformed answer: key " + f"{key!r} is not a (connector_type, connector_id) pair" + ) + connector_type, connector_id = key + if not isinstance(connector_type, str): + raise ValueError( + "connector access hook returned a malformed answer: key " + f"{key!r} has a connector type that is not a str, got " + f"{type(connector_type).__name__}" + ) + if isinstance(connector_id, bool) or not isinstance(connector_id, int): + raise ValueError( + "connector access hook returned a malformed answer: key " + f"{key!r} has a connector id that is not an int (bool is a " + "subclass of int in Python and is never a legitimate " + f"connector id), got {connector_id!r}" + ) + if key not in requested: + raise ValueError( + "connector access hook returned a malformed answer: a " + f"verdict for {key!r}, which was not among the connectors " + "asked about" + ) + if not isinstance(verdict, ConnectorAccess): + raise ValueError( + "connector access hook returned a malformed answer: " + f"expected ConnectorAccess values, got " + f"{type(verdict).__name__} for {key!r}" + ) + if verdict.team_owned is not True: + raise ValueError( + "connector access hook returned a malformed answer for " + f"{key!r}: team_owned must be True -- a connector the " + "caller's team does not link is expressed by leaving it " + f"out of the answer, not by a verdict, got {verdict.team_owned!r}" + ) + if verdict.can_edit is not True and verdict.can_edit is not False: + raise ValueError( + "connector access hook returned a malformed answer for " + f"{key!r}: can_edit must be exactly True or False (bool is " + "a subclass of int in Python, and a truthy value is never " + f"a legitimate grant), got {verdict.can_edit!r}" + ) + validated[key] = verdict + return validated + + +def resolve_connector_access( + db: Any, user_id: int, refs: "Collection[ConnectorRef]" +) -> "dict[ConnectorRef, ConnectorAccess]": + """Whether the caller's team links each of ``refs``, and may edit it. + + Asks the installed access hook, if any, at most once per call + regardless of how many refs are passed -- batching is the point of + this signature, not an incidental property, because the seam's whole + reason to exist is to answer "what is this caller's team's + relationship to these connectors" without paying one hook call per + connector. Returns ``{}`` immediately, without calling the hook at + all, when no hook is installed or when ``refs`` is empty: an empty + request is never worth a call, and a standalone deployment with no + hook installed sees zero queries and zero behavior change. + + A ref missing from the returned map means "the caller's team does not + link this connector" -- the only way that fact is ever expressed (see + ``_validate_connector_access_answer``). The answer is shape-validated + before it reaches any caller. + + Answers direct-id reachability and edit authority only. List + membership comes from a different hook -- ``visible_team_connector_ids`` + -- and xagent enforces no relationship between the two answers: they + are separate module-level slots, installed separately, and nothing + cross-checks them. An installing application must derive both from one + and the same link query, because it is the only side that can see its + own link table; xagent cannot verify that and does not try. + + When the two answers disagree, xagent answers each question from the + hook that owns it and does not reconcile them: a connector this hook + grants but the visibility hook omits is absent from the listing and + still reachable and editable by id; a connector the visibility hook + returns but this hook omits appears in the listing and 404s on direct + id. Neither is a defect in xagent -- both are what the installed + answers said. + """ + requested = frozenset( + (connector_type, int(connector_id)) for connector_type, connector_id in refs + ) + if _connector_access_hook is None or not requested: + return {} + return _call_connector_hook_gate( + db, + _connector_access_hook, + db, + int(user_id), + requested, + validate=lambda answer: _validate_connector_access_answer(answer, requested), + ) + + +def _restore_session_after_hook_failure(db: Any) -> None: + """Roll back whatever a hook left on the shared session before its call + failed -- whether the hook raised, or answered with a shape this seam + rejected. + + Hooks are handed the endpoint's own live session (see + ``delete_team_connector``'s contract note). A hook whose own statement + failed leaves that transaction unusable on PostgreSQL, and an ORM + ``flush`` failure leaves it unusable on every backend -- so every + later statement in the request, including the ones a degradation path + needs to build its response, would be refused. Rolling back here, at + the one door every hook call and every answer check passes through, is + what keeps the degradation contract true; the roll back happens after + the route's own ``db.commit()`` on the post-commit decoration paths, so + it never discards durable work. + + A rollback that itself fails is logged and swallowed: this runs on an + already-failing path, the original failure is re-raised by the caller + either way, and there is no further recovery available. + """ + rollback = getattr(db, "rollback", None) + if rollback is None: + return + try: + rollback() + except Exception: + logger.warning( + "Rolling back after a failed connector hook failed", exc_info=True + ) + + +_HookResult = TypeVar("_HookResult") + + +def _call_connector_hook_gate( + db: Any, + hook: "Callable[..., _HookResult]", + *args: Any, + validate: "Callable[[Any], _HookResult] | None" = None, + **kwargs: Any, +) -> _HookResult: + """The one door every installed connector hook is called through, and + the one place its answer is checked. + + Hooks run on the endpoint's own live session (see + ``delete_team_connector``'s contract note). A hook whose own statement + failed leaves that transaction unusable on PostgreSQL, and a failed + ORM ``flush`` leaves it unusable on every backend -- so restoring the + session belongs to the invocation itself, not to whichever caller + happens to wrap it. Placing it here is what makes the property hold + for a hook slot added to this module later, without that slot's author + having to know about it: five slots exist today and only two of the + call paths used to be covered. + + ``validate``, when given, runs inside the same ``try`` because a hook + can poison the session *without* raising: run a statement that fails, + catch that itself, and answer with a shape this seam then rejects. The + rejection is this module's own exception rather than the hook's, so a + restore placed around the call alone would not fire for it -- the + session would stay unusable for everything the request does next. Two + of the five slots have an answer this seam validates; the other three + pass nothing, which says at the call site that this seam checks + nothing about those answers, rather than leaving that silent. + + The exception is re-raised unchanged, whichever of the two raised it; + this function decides nothing about how the failure is classified or + translated. That stays with the ``*_or_raise`` wrappers below, which + own the seam's typed-error contract. + + One shape stays uncovered, deliberately: a hook that poisons the + session, swallows its own failure, and still returns a well-formed + answer produces no exception at all -- neither here nor in a + validator -- so nothing triggers a restore. That was equally true + before this restore moved here; closing it would mean probing the + session's health after every hook call, which is a different design + than a failure path. + """ + try: + answer = hook(*args, **kwargs) + if validate is None: + return answer + return validate(answer) + except Exception: + _restore_session_after_hook_failure(db) + raise + + def resolve_team_connector_ids_or_raise( db: Any, *, team_id: int | None, log_subject: int | None ) -> dict[str, set[int]]: @@ -237,6 +523,15 @@ def resolve_team_connector_ids_or_raise( no identity guard of its own; production only reaches it through its guarded public wrapper). It is only ever formatted into the log message, never interpreted. + + The session restore that used to live on both failure arms here now + lives on ``_call_connector_hook_gate``, the single door every installed + hook is invoked through, and it covers both ways that call can fail: a + hook can leave a statement failed on the session and *then* raise its + own ``ConnectorRuntimeError``, and a hook can leave one failed, swallow + that itself, and answer with a shape this seam's own validator then + rejects. Neither is something the generic-exception arm below could + own, and both are restored before either arm sees the exception. """ try: return team_connector_ids(db, team_id=team_id) @@ -256,6 +551,114 @@ def resolve_team_connector_ids_or_raise( ) from exc +def resolve_connector_access_or_raise( + db: Any, user_id: int, refs: "Collection[ConnectorRef]" +) -> "dict[ConnectorRef, ConnectorAccess]": + """``resolve_connector_access(db, user_id, refs)``, with every + non-typed failure converted into the seam's one typed 503. + + A ``ConnectorRuntimeError`` -- whether raised by the hook itself or by + ``resolve_connector_access``'s own answer validation -- passes through + unchanged (same object, not re-wrapped). Any other exception is logged + at ``WARNING`` and converted into + ``ConnectorRuntimeError(ERROR_CONNECTOR_RUNTIME_UNAVAILABLE, "Connector + access is unavailable.", details={"reason": + "connector_access_resolution_failed"}, status_code=503)``. Unlike + ``resolve_team_connector_ids_or_raise``, there is no separate + ``log_subject`` parameter: ``user_id`` here already identifies the + caller directly, so it doubles as the value logged. The logged refs + are the plain ``(connector_type, id)`` tuples the caller passed in, + sorted for a stable log line -- never an ORM attribute read off a row, + which could itself fail if the session is left unusable by whatever + just failed. + + The session restore that used to live on both failure arms here now + lives on ``_call_connector_hook_gate``, the single door every installed + hook is invoked through, and it covers both ways that call can fail: a + hook can leave a statement failed on the session and *then* raise its + own ``ConnectorRuntimeError``, and a hook can leave one failed, swallow + that itself, and answer with a shape this seam's own validator then + rejects. Neither is something the generic-exception arm below could + own, and both are restored before either arm sees the exception. + """ + requested = frozenset( + (connector_type, int(connector_id)) for connector_type, connector_id in refs + ) + try: + return resolve_connector_access(db, user_id, requested) + except ConnectorRuntimeError: + raise + except Exception as exc: + logger.warning( + "Failed to resolve connector access for user %s across %s connectors: %s", + user_id, + len(requested), + sorted(requested), + exc_info=True, + ) + raise ConnectorRuntimeError( + ERROR_CONNECTOR_RUNTIME_UNAVAILABLE, + "Connector access is unavailable.", + details={"reason": "connector_access_resolution_failed"}, + status_code=503, + ) from exc + + +def resolve_one_connector_access_or_raise( + db: Any, user_id: int, ref: "ConnectorRef" +) -> "ConnectorAccess | None": + """Single-``ref`` convenience wrapper around + ``resolve_connector_access_or_raise``: wraps ``ref`` in a one-element + collection, calls the batch resolver, and unwraps the answer for that + ref. ``None`` means the same thing it means for any ref missing from a + batch answer -- not linked, or a legitimate answer the hook chose to + omit -- never a failure, which still raises ``ConnectorRuntimeError`` + same as the batch form. Exists so item GET/PUT call sites do not each + repeat the wrap-then-``.get(ref)`` shape by hand. + """ + return resolve_connector_access_or_raise(db, user_id, [ref]).get(ref) + + +@contextmanager +def snapshot_connector_team_hooks() -> Iterator[None]: + """Save every module-level hook slot, restore it on exit. + + Intended for tests: entering the block, replacing any slot (through + ``set_connector_team_hooks`` or a direct module-attribute monkeypatch), + and leaving restores every slot to the exact object it held before the + block, including a slot the block never touched. A slot added to this + module later must be added here too, or a snapshot taken before that + slot exists will silently fail to restore it -- covered by the + discovery-based coverage test in + tests/web/services/test_connector_team_scope.py, which enumerates every + module global ending in ``_hook`` and asserts this snapshot restores + each one by identity. Saving and restoring lives on the module because + the state being saved lives on the module: a test-side helper would + have to name and reach these globals from outside, and would go stale + the moment a slot is added here. + """ + global _connector_deleted_hook, _connector_renamed_hook + global _connector_visibility_hook, _team_connector_visibility_hook + global _connector_access_hook + saved = ( + _connector_deleted_hook, + _connector_renamed_hook, + _connector_visibility_hook, + _team_connector_visibility_hook, + _connector_access_hook, + ) + try: + yield + finally: + ( + _connector_deleted_hook, + _connector_renamed_hook, + _connector_visibility_hook, + _team_connector_visibility_hook, + _connector_access_hook, + ) = saved + + def connector_visible_to_user( *, association: "UserMCPServer | UserCustomApi | None", @@ -361,7 +764,9 @@ def delete_team_connector( if _connector_deleted_hook is None: return ConnectorDeleteDecision() - return _connector_deleted_hook(db, user_id, connector_type, connector_id) + return _call_connector_hook_gate( + db, _connector_deleted_hook, db, user_id, connector_type, connector_id + ) def rename_team_connector( @@ -375,6 +780,13 @@ def rename_team_connector( """Keep application-owned connector selectors aligned after a rename.""" if _connector_renamed_hook is not None and old_name != new_name: - _connector_renamed_hook( - db, user_id, connector_type, connector_id, old_name, new_name + _call_connector_hook_gate( + db, + _connector_renamed_hook, + db, + user_id, + connector_type, + connector_id, + old_name, + new_name, ) diff --git a/src/xagent/web/services/knowledge_base_team_scope.py b/src/xagent/web/services/knowledge_base_team_scope.py index ef3a50fc67..fe79c6092c 100644 --- a/src/xagent/web/services/knowledge_base_team_scope.py +++ b/src/xagent/web/services/knowledge_base_team_scope.py @@ -299,9 +299,12 @@ def snapshot_knowledge_base_team_hooks() -> Iterator[None]: slot added to this module later must be added here too, or a snapshot taken before that slot exists will silently fail to restore it. - The connector seam this module otherwise mirrors has no counterpart, and - that asymmetry is deliberate rather than a gap to close in either - direction. Its tests reset by calling the setter with no arguments, + The connector seam this module otherwise mirrors has its own equivalent, + ``connector_team_scope.snapshot_connector_team_hooks``, with the same + save-and-restore shape. The two primitives are independent: each saves + and restores only its own module's hook slots, and installing or + resetting one has no effect on the other. Tests that do not use either + primitive reset by calling the relevant setter with no arguments, which restores the *empty* state, not the state the test found. Every slot here is process-global, so a test that installs one and resets by clearing leaves any hook the process had installed before it gone, and diff --git a/tests/web/api/test_connector_hook_session_fault_postgresql.py b/tests/web/api/test_connector_hook_session_fault_postgresql.py new file mode 100644 index 0000000000..2ed6a3dbe6 --- /dev/null +++ b/tests/web/api/test_connector_hook_session_fault_postgresql.py @@ -0,0 +1,328 @@ +"""Real-PostgreSQL coverage for the connector access seam restoring a +shared session a hook left with a failed raw statement on it. + +``poison_by_raw_statement`` (see test_mcp_reported_edit_permission.py) only +actually poisons PostgreSQL: a failed raw statement aborts the surrounding +transaction there, so every later statement on the same connection is +refused until a rollback runs -- SQLite does not enforce that the same way, +so the SQLite-backed suite cannot prove this shape needs the fix. + +``test_the_seam_restores_the_session_after_a_raw_statement_failure`` is the +direct, independently mutation-sensitive proof: it calls +``resolve_connector_access_or_raise`` itself with a hook that runs the +poisoning statement, and asserts a fresh query on the same session succeeds +right after. Deleting the rollback call from +``_restore_session_after_hook_failure`` turns this test red on this file +specifically (confirmed by running it against a real server with that line +removed); it stays green on SQLite regardless, which is exactly why this +shape needs its own PostgreSQL-only proof. + +The four route-level tests below (toggle, connect, the apps listing, the +servers listing) are also run here for completeness -- they pin the +*correct* end-to-end behavior (2xx, durable writes) under this exact +failure shape on a real server. Whether each route *call itself* needs the +session restored is a separate question from whether its test does, and +the two no longer agree for all four: + +The route calls themselves are never independently mutation-sensitive for +this specific shape: each response builder happens to read the connector +row's attributes once *before* the hook ever runs (e.g. toggle_mcp_server's +own log line touches ``server.name``), which loads those attributes into +the ORM instance. Since ``poison_by_raw_statement`` aborts the underlying +transaction without SQLAlchemy's ORM-level "expire everything" cleanup +(unlike a failed flush -- see poison_by_orm_flush's docstring and +TestSessionRecoveryAfterHookFailure in the SQLite suite, which *is* +mutation-sensitive on both backends), no attribute on that already-loaded +row needs reloading afterward, so none of the four routes ever issues a +new statement on the poisoned connection while building its own response. + +The toggle and connect tests are independently mutation-sensitive anyway, +because each queries the database again *after* the route call returns, to +verify what actually landed (``refreshed``/``assoc`` below) -- and that +query runs directly on the same session the hook just poisoned, with no +rollback of the test's own in between. Removing the production restore +turns that query into the first statement that reaches the aborted +transaction, which PostgreSQL refuses. The apps-listing and servers-listing +tests stay non-sensitive: neither issues any further statement after the +route call, so there is nothing left in either test that could reach the +poisoned connection. The seam-level test above is what directly exercises +the poisoned connection regardless of any particular route's shape. + +``/api/mcp/servers`` (the sister listing to the apps listing above) now +has its own per-request degradation catch, added in this same revision, so +its route-level test below joins the other three rather than being +deferred -- see the sibling note in +test_mcp_reported_edit_permission.py's TestListMcpServersPerRowDegradation +for the SQLite-side proof of this same route using the ORM-flush failure +shape, which *is* mutation-sensitive there. + +Obtains its database through ``tests/shared/postgres_disposable.py`` +(``disposable_database_factory``), the same disposable-CREATE-DATABASE +helper the other ``*_postgresql.py`` suites in this repo use. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +import sqlalchemy as sa +from sqlalchemy.orm import sessionmaker + +from tests.shared.postgres_disposable import disposable_database_factory +from xagent.core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError +from xagent.web.models.database import Base +from xagent.web.models.mcp import MCPServer, UserMCPServer +from xagent.web.models.public_mcp import PublicMCPApp +from xagent.web.models.user import User +from xagent.web.services.connector_team_scope import ( + resolve_connector_access_or_raise, + set_connector_team_hooks, + snapshot_connector_team_hooks, +) + +pytestmark = pytest.mark.postgresql + + +def poison_by_raw_statement(db) -> None: + db.execute(sa.text("select * from no_such_table_at_all")) + + +@pytest.fixture() +def session_factory(): + with disposable_database_factory("xagent_connector_session_fault") as make_database: + engine = make_database("session_fault") + Base.metadata.create_all(bind=engine) + yield sessionmaker(autocommit=False, autoflush=False, bind=engine) + + +@pytest.fixture() +def seeded(session_factory): + """One owner, one owned MCP server (for toggle), one catalog app entry + (for connect), in their own committed rows.""" + with session_factory() as db: + owner = User(username="session-fault-owner", password_hash="x", is_admin=False) + db.add(owner) + db.flush() + server = MCPServer( + name="session-fault-target", + transport="stdio", + managed="external", + command="true", + ) + db.add(server) + db.flush() + db.add( + UserMCPServer( + user_id=int(owner.id), + mcpserver_id=int(server.id), + is_owner=True, + is_active=True, + ) + ) + db.add( + PublicMCPApp( + app_id="session-fault-catalog-app", + name="session-fault-catalog-app", + description="Session fault test app", + transport="stdio", + launch_config={ + "command": "npx", + "args": ["-y", "session-fault-catalog-app"], + }, + ) + ) + db.commit() + return int(owner.id), int(server.id) + + +def test_a_toggle_that_already_committed_still_returns_200_when_the_hook_poisons_the_session( + session_factory, seeded +) -> None: + import xagent.web.api.mcp as mcp_api + + owner_id, server_id = seeded + current_user = SimpleNamespace(id=owner_id, is_admin=False) + + def poisoning_access(db, user_id, refs): + poison_by_raw_statement(db) + return {} + + db = session_factory() + try: + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=poisoning_access) + response = mcp_api.toggle_mcp_server( + server_id, current_user=current_user, db=db + ) + assert response.can_edit_global is True + + # No rollback here on purpose: the seam's hook door already + # restored this session, and the query below is the statement that + # proves it -- on PostgreSQL a poisoned transaction refuses every + # later statement. Rolling back first would make this test pass + # with the production restore removed. + refreshed = ( + db.query(UserMCPServer) + .filter( + UserMCPServer.user_id == owner_id, + UserMCPServer.mcpserver_id == server_id, + ) + .one() + ) + assert refreshed.is_active is False + finally: + db.close() + + +def test_connecting_an_app_still_returns_200_when_the_hook_poisons_the_session( + session_factory, seeded +) -> None: + import xagent.web.api.mcp as mcp_api + + owner_id, _server_id = seeded + member = User(username="session-fault-member", password_hash="x", is_admin=False) + + def poisoning_access(db, user_id, refs): + poison_by_raw_statement(db) + return {} + + db = session_factory() + try: + db.add(member) + db.commit() + member_id = int(member.id) + current_user = SimpleNamespace(id=member_id, is_admin=False) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=poisoning_access) + response = mcp_api.connect_mcp_app( + "session-fault-catalog-app", + mcp_api.MCPAppConnectRequest(), + current_user=current_user, + db=db, + ) + # Connecting never grants ownership -- the same value this route + # always reported before any verdict existed. + assert response.can_edit_global is False + + # No rollback here on purpose -- see the same note in the toggle + # test above: the query below is the proof the seam's hook door + # restored the session, not just an incidental fresh read. + assoc = ( + db.query(UserMCPServer) + .join(MCPServer, UserMCPServer.mcpserver_id == MCPServer.id) + .filter( + UserMCPServer.user_id == member_id, + MCPServer.name == "session-fault-catalog-app", + ) + .one() + ) + # ``.one()`` raises when the row is missing, so its own success is + # the existence assertion. What this line adds is the route's own + # decision: connecting never grants ownership (mcp.py:3339-3341), + # and that decision survived the poisoned hook. + assert assoc.is_owner is False + finally: + db.close() + + +def test_the_apps_listing_still_returns_every_row_when_the_hook_poisons_the_session( + session_factory, seeded +) -> None: + import xagent.web.api.mcp as mcp_api + + owner_id, server_id = seeded + member = User(username="session-fault-apps-member", password_hash="x") + + def poisoning_access(db, user_id, refs): + poison_by_raw_statement(db) + return {} + + db = session_factory() + try: + db.add(member) + db.commit() + member_id = int(member.id) + current_user = SimpleNamespace(id=member_id, is_admin=False) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=poisoning_access, + visibility=lambda _db, _uid: { + "mcp": {server_id}, + "custom_api": set(), + }, + ) + entries = mcp_api.list_mcp_apps( + location="local", current_user=current_user, db=db + ) + + entry = next(e for e in entries if e["server_id"] == server_id) + assert entry["can_configure"] is False + finally: + db.close() + + +def test_the_servers_listing_still_returns_every_row_when_the_hook_poisons_the_session( + session_factory, seeded +) -> None: + import xagent.web.api.mcp as mcp_api + + owner_id, server_id = seeded + member = User(username="session-fault-servers-member", password_hash="x") + + def poisoning_access(db, user_id, refs): + poison_by_raw_statement(db) + return {} + + db = session_factory() + try: + db.add(member) + db.commit() + member_id = int(member.id) + current_user = SimpleNamespace(id=member_id, is_admin=False) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=poisoning_access, + visibility=lambda _db, _uid: { + "mcp": {server_id}, + "custom_api": set(), + }, + ) + entries = mcp_api.get_mcp_servers(current_user=current_user, db=db) + + entry = next(e for e in entries if e.id == server_id) + assert entry.can_edit_global is False + finally: + db.close() + + +def test_the_seam_restores_the_session_after_a_raw_statement_failure( + session_factory, seeded +) -> None: + """Direct proof at the seam itself, independent of any particular + route's attribute-loading order: a hook that runs a raw statement that + aborts the PostgreSQL transaction still leaves the session usable for + whatever the caller does next.""" + owner_id, server_id = seeded + + def poisoning_access(db, user_id, refs): + poison_by_raw_statement(db) + return {} + + db = session_factory() + try: + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=poisoning_access) + with pytest.raises(ConnectorRuntimeError) as excinfo: + resolve_connector_access_or_raise(db, owner_id, [("mcp", server_id)]) + assert excinfo.value.status_code == 503 + + # The session must be usable again immediately afterward -- not + # just after an explicit external rollback. + result = db.execute(sa.select(sa.literal(1))).scalar() + assert result == 1 + finally: + db.close() diff --git a/tests/web/api/test_custom_api.py b/tests/web/api/test_custom_api.py index ff59296eda..125b0fb8f9 100644 --- a/tests/web/api/test_custom_api.py +++ b/tests/web/api/test_custom_api.py @@ -1,10 +1,15 @@ +import ast +import importlib +import inspect from datetime import datetime +from types import SimpleNamespace from unittest.mock import MagicMock, call, patch import pytest from fastapi import HTTPException from pydantic import ValidationError -from sqlalchemy.orm import Session +from sqlalchemy import create_engine, event +from sqlalchemy.orm import Session, sessionmaker from xagent.web.api.custom_api import ( CustomApiCreate, @@ -18,8 +23,13 @@ update_custom_api, ) from xagent.web.models.custom_api import CustomApi, UserCustomApi +from xagent.web.models.database import Base from xagent.web.models.user import User -from xagent.web.services.connector_team_scope import ConnectorDeleteDecision +from xagent.web.services.connector_team_scope import ( + ConnectorDeleteDecision, + set_connector_team_hooks, + snapshot_connector_team_hooks, +) def test_custom_api_models_env_validation(): @@ -254,7 +264,7 @@ async def test_get_custom_api(): db.query().filter().first.return_value = mock_user_api - res = await get_custom_api(10, current_user=user, db=db) + res = get_custom_api(10, current_user=user, db=db) assert res.id == 10 assert res.name == "test_api" @@ -266,7 +276,7 @@ async def test_get_custom_api_not_found(): db.query().filter().first.return_value = None with pytest.raises(HTTPException) as exc_info: - await get_custom_api(99, current_user=user, db=db) + get_custom_api(99, current_user=user, db=db) assert exc_info.value.status_code == 404 @@ -296,6 +306,13 @@ async def test_update_custom_api(): # Return user api on first query # Return None for existing name check db.query().filter().first.side_effect = [mock_user_api, None] + # The row lock's own fresh query is a separate mock chain + # (.populate_existing().with_for_update() sits between .filter() and + # .first()), so it needs its own return value rather than sharing the + # side_effect list above. + db.query().filter().populate_existing().with_for_update().first.return_value = ( + mock_api + ) api_data = CustomApiUpdate( name="new_name", @@ -311,7 +328,7 @@ async def test_update_custom_api(): with patch( "xagent.web.api.custom_api.encrypt_value", side_effect=lambda x: f"enc_{x}" ): - await update_custom_api(10, api_data, current_user=user, db=db) + update_custom_api(10, api_data, current_user=user, db=db) assert mock_api.name == "new_name" assert mock_api.env == {"k1": "enc_old1", "k2": "enc_v2"} @@ -347,11 +364,16 @@ async def test_update_custom_api_env_replacement_deletes_only_the_omitted_secret custom_api=mock_api, ) db.query().filter().first.return_value = mock_user_api + # The row lock's own fresh query is a separate mock chain -- see the + # comment in test_update_custom_api. + db.query().filter().populate_existing().with_for_update().first.return_value = ( + mock_api + ) with patch( "xagent.web.api.custom_api.encrypt_value", side_effect=lambda x: f"enc_{x}" ): - await update_custom_api( + update_custom_api( 10, CustomApiUpdate(env={"TENANT": "********"}), current_user=user, @@ -381,9 +403,14 @@ async def test_update_custom_api_rejects_renamed_masked_secret(): custom_api=mock_api, ) db.query().filter().first.return_value = mock_user_api + # The row lock's own fresh query is a separate mock chain -- see the + # comment in test_update_custom_api. + db.query().filter().populate_existing().with_for_update().first.return_value = ( + mock_api + ) with pytest.raises(HTTPException) as exc_info: - await update_custom_api( + update_custom_api( 10, CustomApiUpdate(env={"RENAMED_TOKEN": "********"}), current_user=user, @@ -423,6 +450,11 @@ async def test_update_custom_api_explicit_null_clears_runtime_config(): custom_api=mock_api, ) db.query().filter().first.return_value = mock_user_api + # The row lock's own fresh query is a separate mock chain -- see the + # comment in test_update_custom_api. + db.query().filter().populate_existing().with_for_update().first.return_value = ( + mock_api + ) api_data = CustomApiUpdate( runtime_input_schema=None, @@ -430,7 +462,7 @@ async def test_update_custom_api_explicit_null_clears_runtime_config(): allow_delegated_authorization=False, ) - await update_custom_api(10, api_data, current_user=user, db=db) + update_custom_api(10, api_data, current_user=user, db=db) assert mock_api.runtime_input_schema is None assert mock_api.runtime_bindings is None @@ -449,8 +481,11 @@ async def test_delete_custom_api(): ) db.query().filter().first.return_value = mock_user_api + db.query().filter().populate_existing().with_for_update().first.return_value = ( + mock_api + ) - await delete_custom_api(10, current_user=user, db=db) + delete_custom_api(10, current_user=user, db=db) db.delete.assert_called_once_with(mock_api) db.commit.assert_called() @@ -465,6 +500,9 @@ async def test_delete_team_custom_api_flushes_only_current_user_link(): user_id=1, custom_api_id=10, can_delete=True, custom_api=mock_api ) db.query().filter().first.side_effect = [mock_user_api, None] + db.query().filter().populate_existing().with_for_update().first.return_value = ( + mock_api + ) decision = ConnectorDeleteDecision( team_owned=True, @@ -475,9 +513,292 @@ async def test_delete_team_custom_api_flushes_only_current_user_link(): "xagent.web.services.connector_team_scope.delete_team_connector", return_value=decision, ): - await delete_custom_api(10, current_user=user, db=db) + delete_custom_api(10, current_user=user, db=db) db.flush.assert_called_once_with([mock_user_api]) assert db.no_autoflush.__enter__.called assert db.delete.call_args_list == [call(mock_user_api), call(mock_api)] db.commit.assert_called_once() + + +def test_the_locking_routes_are_sync_defs_so_a_lock_wait_never_holds_the_event_loop(): + """Both routes below run a ``SELECT ... FOR UPDATE`` that can wait + indefinitely on a concurrent writer. FastAPI runs a coroutine route on + the event loop thread itself, so such a wait inside an ``async def`` + route stalls every other request the process is serving. Declaring them + as plain ``def`` puts them in the threadpool instead, which is what the + MCP side's own PUT already does.""" + import inspect + + from xagent.web.api import custom_api as custom_api_api + + assert not inspect.iscoroutinefunction(custom_api_api.update_custom_api) + assert not inspect.iscoroutinefunction(custom_api_api.delete_custom_api) + + +_SEAM_MODULES = ("xagent.web.api.custom_api", "xagent.web.api.mcp") + +# The one function that reaches the connector team seam and is still a +# coroutine, with the fact that makes it impossible to convert. Its own +# await -- one that is not the seam call itself -- is asserted below, so +# this entry cannot be claimed by a route whose coroutine is only the +# seam's doing. +_COROUTINE_EXEMPTIONS = {("xagent.web.api.mcp", "delete_mcp_server")} + +_SEAM_REACHING_FUNCTIONS = { + ("xagent.web.api.custom_api", "_resolve_custom_api_for_request"), + ("xagent.web.api.custom_api", "get_custom_api"), + ("xagent.web.api.custom_api", "update_custom_api"), + ("xagent.web.api.custom_api", "delete_custom_api"), + ("xagent.web.api.mcp", "_resolve_mcp_server_for_request"), + ("xagent.web.api.mcp", "_local_mcp_can_attach"), + ("xagent.web.api.mcp", "list_mcp_apps"), + ("xagent.web.api.mcp", "get_mcp_servers"), + ("xagent.web.api.mcp", "get_mcp_server"), + ("xagent.web.api.mcp", "connect_mcp_app"), + ("xagent.web.api.mcp", "update_mcp_server"), + ("xagent.web.api.mcp", "delete_mcp_server"), + ("xagent.web.api.mcp", "toggle_mcp_server"), +} + + +def _functions_reaching_the_connector_seam(module_name: str) -> dict[str, ast.AST]: + """Every top-level function in ``module_name`` that can reach an + installed connector team hook. + + Seeded on the functions that import ``connector_team_scope`` in their + own body -- which is how every call site in these two modules reaches + the seam -- then closed transitively over plain-name calls, because + two of the routes reach it only through a helper (``get_custom_api`` + through ``_resolve_custom_api_for_request``, ``get_mcp_server`` + through ``_resolve_mcp_server_for_request``). A seed-only check would + miss exactly the route this test exists for. + """ + module = importlib.import_module(module_name) + tree = ast.parse(inspect.getsource(module)) + functions = { + node.name: node + for node in tree.body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + } + reaching = { + name + for name, node in functions.items() + if any( + isinstance(child, ast.ImportFrom) + and child.module is not None + and child.module.endswith("connector_team_scope") + for child in ast.walk(node) + ) + } + changed = True + while changed: + changed = False + for name, node in functions.items(): + if name in reaching: + continue + called = { + child.func.id + for child in ast.walk(node) + if isinstance(child, ast.Call) and isinstance(child.func, ast.Name) + } + if called & reaching: + reaching.add(name) + changed = True + return {name: functions[name] for name in reaching} + + +def _seam_names_imported_by(node: ast.AST) -> set[str]: + """The names this function imports from ``connector_team_scope``. + + Read off the function's own body because that is how every call site in + these two modules reaches the seam -- the same fact + ``_functions_reaching_the_connector_seam`` above is seeded on. + """ + return { + alias.asname or alias.name + for child in ast.walk(node) + if isinstance(child, ast.ImportFrom) + and child.module is not None + and child.module.endswith("connector_team_scope") + for alias in child.names + } + + +def test_the_discovery_of_seam_reaching_functions_is_not_vacuous(): + """Pins the enumeration itself, so the assertion below cannot pass by + finding nothing.""" + found = { + (module_name, name) + for module_name in _SEAM_MODULES + for name in _functions_reaching_the_connector_seam(module_name) + } + assert found == _SEAM_REACHING_FUNCTIONS + + +def test_no_function_that_reaches_the_connector_seam_is_a_coroutine(): + """An installed connector team hook may be slow -- this repo's own + design assumes it does database-backed work. FastAPI runs a coroutine + route on the event loop thread itself, so a slow hook call inside an + ``async def`` stalls every other request the process is serving, not + just this one; a plain ``def`` goes to the threadpool instead, where a + slow call occupies one worker. + + Enumerated by reachability rather than by a hand-written list of + routes: the earlier fix for this same risk class swept siblings along + the "takes a row lock" axis and therefore missed two routes that call + a hook without taking one. + """ + offenders = [] + for module_name in _SEAM_MODULES: + for name, node in _functions_reaching_the_connector_seam(module_name).items(): + if not isinstance(node, ast.AsyncFunctionDef): + continue + if (module_name, name) in _COROUTINE_EXEMPTIONS: + # An exemption is only legitimate for a function that + # genuinely cannot be converted, so it must carry an await + # that is NOT the seam call itself. A function whose only + # await IS the seam call is a coroutine of the seam's own + # making -- convertible by making that call synchronous -- + # and "contains some await" would still wave it through. + seam_names = _seam_names_imported_by(node) + non_seam_awaits = [ + child + for child in ast.walk(node) + if isinstance(child, ast.Await) + and not ( + isinstance(child.value, ast.Call) + and isinstance(child.value.func, ast.Name) + and child.value.func.id in seam_names + ) + ] + assert non_seam_awaits, ( + f"{module_name}.{name} is exempted from this invariant, but " + "every await it has is a seam call -- the coroutine is the " + "seam's own doing, so make that call synchronous instead of " + "exempting the route" + ) + continue + offenders.append(f"{module_name}.{name}") + assert offenders == [], ( + "these functions can reach an installed connector team hook while " + f"running on the event loop thread: {offenders}" + ) + + +def _lock_order_session_factory(): + engine = create_engine("sqlite:///:memory:") + Base.metadata.create_all(engine) + return sessionmaker(autocommit=False, autoflush=False, bind=engine), engine + + +def _seed_owned_api_for_lock_order(session_factory, *, name: str) -> tuple[int, int]: + db = session_factory() + owner = User(username=f"user-{name}", password_hash="x", is_admin=False) + db.add(owner) + db.flush() + api = CustomApi(name=name, url="https://example.test/api", method="GET") + db.add(api) + db.flush() + db.add( + UserCustomApi( + user_id=owner.id, + custom_api_id=api.id, + is_owner=True, + can_edit=True, + can_delete=True, + is_active=True, + ) + ) + db.commit() + owner_id, api_id = int(owner.id), int(api.id) + db.close() + return owner_id, api_id + + +def _count_custom_apis_selects_before_first_delete(statements: list[str]) -> int: + """How many ``SELECT``s against ``custom_apis`` land before the first + ``DELETE`` of either table. + + The route's own not-found guard (``not user_api or not + user_api.custom_api``) always lazy-loads the ``custom_api`` relationship, + which is one such ``SELECT`` on its own -- with or without the lock + statement this test exists to pin. So *presence* of a ``custom_apis`` + ``SELECT`` before the delete is true either way and proves nothing; the + *count* is what distinguishes them -- one without the lock statement, + two with it, because ``populate_existing()`` forces the lock's query to + hit the database again rather than reuse the already-loaded row. + """ + count = 0 + for statement in statements: + upper = statement.strip().upper() + if upper.startswith("DELETE"): + break + if upper.startswith("SELECT") and "FROM CUSTOM_APIS" in upper: + count += 1 + return count + + +class TestDeleteLockOrderMatchesThePutsLockOrder: + """``update_custom_api`` locks the ``CustomApi`` definition row first and + writes the ``UserCustomApi`` link row afterwards. For the two routes to + share one global lock order, ``delete_custom_api`` must take the same + definition-row lock before it deletes the link row, in both of its + branches. + + SQLite silently drops ``FOR UPDATE`` (it is a no-op on this dialect), so + nothing here demonstrates that the lock actually blocks a second writer + -- that proof lives in test_custom_api_edit_lock_postgresql.py, against + a real server. What this proves instead is statement *order*, which is + dialect-independent and exercisable without one. + """ + + def _run(self, *, team_owned: bool) -> list[str]: + session_factory, engine = _lock_order_session_factory() + owner_id, api_id = _seed_owned_api_for_lock_order( + session_factory, + name="lock-order-team" if team_owned else "lock-order-cascade", + ) + db = session_factory() + current_user = SimpleNamespace(id=owner_id, is_admin=False) + + statements: list[str] = [] + + def record_query(conn, cursor, statement, parameters, context, executemany): + del conn, cursor, parameters, context, executemany + statements.append(statement) + + event.listen(engine, "before_cursor_execute", record_query) + try: + if team_owned: + + def deleted_hook(_db, _user_id, _connector_type, _connector_id): + return ConnectorDeleteDecision( + team_owned=True, authorized=True, delete_definition=True + ) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(deleted=deleted_hook) + delete_custom_api(api_id, current_user=current_user, db=db) + else: + delete_custom_api(api_id, current_user=current_user, db=db) + finally: + event.remove(engine, "before_cursor_execute", record_query) + db.close() + return statements + + def test_lock_order_team_owned_branch(self): + statements = self._run(team_owned=True) + assert _count_custom_apis_selects_before_first_delete(statements) == 2, ( + "expected the not-found guard's relationship load AND the new " + "lock statement's own SELECT against custom_apis, both before " + "the first DELETE" + ) + + def test_lock_order_cascade_branch(self): + statements = self._run(team_owned=False) + assert _count_custom_apis_selects_before_first_delete(statements) == 2, ( + "expected the not-found guard's relationship load AND the new " + "lock statement's own SELECT against custom_apis, both before " + "the first DELETE" + ) diff --git a/tests/web/api/test_custom_api_edit_lock_postgresql.py b/tests/web/api/test_custom_api_edit_lock_postgresql.py new file mode 100644 index 0000000000..acfe3eb787 --- /dev/null +++ b/tests/web/api/test_custom_api_edit_lock_postgresql.py @@ -0,0 +1,395 @@ +"""Real-PostgreSQL coverage for the row lock ``update_custom_api`` and +``delete_custom_api`` take on the ``CustomApi`` definition row before +propagating a rename or removing the link row, respectively. + +``FOR UPDATE`` is a no-op on SQLite -- every other suite in this repo runs +against SQLite, so nothing there can tell a genuine second-writer block +from a lock statement that silently does nothing. This file is the one +place that runs the real statement against a real server and proves it +actually blocks a second writer: two concurrent edits, an edit and a +concurrent delete both taking the same lock in the same order, and the +companion path where the row vanishes between the route's first read and +this lock. Mirrors test_mcp_server_edit_lock_postgresql.py's structure for +the MCP side of the edit lock; the MCP side's delete path takes no such +lock (see custom_api.py's own delete route for why the two kinds differ). + +Obtains its database through ``tests/shared/postgres_disposable.py`` +(``disposable_database_factory``), the same disposable-CREATE-DATABASE +helper the other ``*_postgresql.py`` suites in this repo use, rather than +opening a hand-rolled connection. That helper reads +``XAGENT_TEST_POSTGRES_URL`` and skips the whole module when it is unset. +""" + +from __future__ import annotations + +import threading +from concurrent.futures import ThreadPoolExecutor +from types import SimpleNamespace + +import pytest +import sqlalchemy as sa +from fastapi import HTTPException +from sqlalchemy.orm import sessionmaker + +from tests.shared.postgres_disposable import disposable_database_factory +from xagent.web.models.custom_api import CustomApi, UserCustomApi +from xagent.web.models.database import Base +from xagent.web.models.user import User +from xagent.web.services.connector_team_scope import ( + set_connector_team_hooks, + snapshot_connector_team_hooks, +) + +pytestmark = pytest.mark.postgresql + + +@pytest.fixture() +def session_factory(): + with disposable_database_factory("xagent_custom_api_edit_lock") as make_database: + engine = make_database("edit_lock") + Base.metadata.create_all(bind=engine) + yield sessionmaker(autocommit=False, autoflush=False, bind=engine) + + +@pytest.fixture() +def seeded(session_factory): + """One owner, one owned Custom API, in their own committed rows.""" + with session_factory() as db: + owner = User( + username="custom-api-edit-lock-owner", password_hash="x", is_admin=False + ) + db.add(owner) + db.flush() + api = CustomApi( + name="edit-lock-target", + url="https://example.com/api", + method="GET", + ) + db.add(api) + db.flush() + db.add( + UserCustomApi( + user_id=int(owner.id), + custom_api_id=int(api.id), + is_owner=True, + can_edit=True, + can_delete=True, + is_active=True, + ) + ) + db.commit() + return int(owner.id), int(api.id) + + +def test_a_second_editor_blocks_until_the_first_editors_transaction_finishes( + session_factory, seeded +) -> None: + """Two real connections, barrier-synchronised: the second call's own + lock statement must not return until the first call's transaction + commits or rolls back -- the actual behavior ``FOR UPDATE`` exists to + provide, and the one thing no SQLite-backed test can demonstrate. + """ + import xagent.web.api.custom_api as custom_api_api + from xagent.web.api.custom_api import CustomApiUpdate + + owner_id, api_id = seeded + current_user = SimpleNamespace(id=owner_id, is_admin=False) + + lock_acquired = threading.Event() + release_lock = threading.Event() + second_finished = threading.Event() + first_call_claimed = threading.Event() + first_call_lock = threading.Lock() + + real_validate = custom_api_api.validate_runtime_config_declaration + + def paced_validate(**kwargs): + # Both threads run through this same patched function once each + # gets past its own lock statement. Only the call that gets here + # *first* pauses: that is the first editor, holding its row lock + # open via this still-uncommitted transaction. A second call that + # reaches this point too (rather than staying blocked earlier, + # inside its own lock statement) is not made to wait a second + # time here -- pausing it too would prove nothing about the + # database lock, only about this Python-level barrier. + with first_call_lock: + is_first_call = not first_call_claimed.is_set() + first_call_claimed.set() + if is_first_call: + lock_acquired.set() + assert release_lock.wait(timeout=10), "the first editor was never released" + return real_validate(**kwargs) + + custom_api_api.validate_runtime_config_declaration = paced_validate + session_a = session_factory() + session_b = session_factory() + try: + + def run_first(): + return custom_api_api.update_custom_api( + api_id, + CustomApiUpdate(name="renamed-by-first-editor"), + current_user=current_user, + db=session_a, + ) + + def run_second(): + result = custom_api_api.update_custom_api( + api_id, + CustomApiUpdate(description="edited-by-second-editor"), + current_user=current_user, + db=session_b, + ) + second_finished.set() + return result + + with ThreadPoolExecutor(max_workers=2) as executor: + first = executor.submit(run_first) + assert lock_acquired.wait(timeout=5), ( + "the first editor never reached the lock" + ) + + second = executor.submit(run_second) + # The second call's own lock statement should still be blocked + # on the database at this point. If the lock were not real (or + # a no-op, as on SQLite), the second call would sail through + # almost immediately and this would flip to True. + assert not second_finished.wait(timeout=1.0), ( + "the second editor finished before the first one released " + "the row -- the lock did not actually block it" + ) + + release_lock.set() + first.result(timeout=10) + second.result(timeout=10) + + assert second_finished.is_set() + finally: + custom_api_api.validate_runtime_config_declaration = real_validate + session_a.close() + session_b.close() + + +def test_the_second_editors_rename_reports_the_first_editors_committed_name_as_old( + session_factory, seeded +) -> None: + """``rename_team_connector``'s ``old`` argument must be the name this + transaction's own lock actually holds once acquired, not whatever the + pre-lock read saw. + + Interleaving under test: the first editor renames the connector and + commits while the second editor is blocked on the lock. The second + editor then acquires the lock, refreshed to the first editor's + committed name, and renames again. If the second editor's ``old`` + argument were captured before its own lock instead, it would report + the connector's *original* name -- not the name every team agent's + selector was already rewritten to by the first editor's own call -- + and the second rewrite would search for a name nothing holds anymore, + leaving the first rewrite's result permanently dangling with no error. + """ + import xagent.web.api.custom_api as custom_api_api + from xagent.web.api.custom_api import CustomApiUpdate + + owner_id, api_id = seeded + current_user = SimpleNamespace(id=owner_id, is_admin=False) + + lock_acquired = threading.Event() + release_lock = threading.Event() + second_finished = threading.Event() + first_call_claimed = threading.Event() + first_call_lock = threading.Lock() + + renamed_calls: list[tuple[str, str]] = [] + renamed_calls_lock = threading.Lock() + + def spy_renamed_hook(_db, _user_id, _connector_type, _connector_id, old, new): + with renamed_calls_lock: + renamed_calls.append((old, new)) + + real_validate = custom_api_api.validate_runtime_config_declaration + + def paced_validate(**kwargs): + with first_call_lock: + is_first_call = not first_call_claimed.is_set() + first_call_claimed.set() + if is_first_call: + lock_acquired.set() + assert release_lock.wait(timeout=10), "the first editor was never released" + return real_validate(**kwargs) + + custom_api_api.validate_runtime_config_declaration = paced_validate + session_a = session_factory() + session_b = session_factory() + try: + with snapshot_connector_team_hooks(): + set_connector_team_hooks(renamed=spy_renamed_hook) + + def run_first(): + return custom_api_api.update_custom_api( + api_id, + CustomApiUpdate(name="renamed-by-first-editor"), + current_user=current_user, + db=session_a, + ) + + def run_second(): + result = custom_api_api.update_custom_api( + api_id, + CustomApiUpdate(name="renamed-by-second-editor"), + current_user=current_user, + db=session_b, + ) + second_finished.set() + return result + + with ThreadPoolExecutor(max_workers=2) as executor: + first = executor.submit(run_first) + assert lock_acquired.wait(timeout=5), ( + "the first editor never reached the lock" + ) + + second = executor.submit(run_second) + assert not second_finished.wait(timeout=1.0), ( + "the second editor finished before the first one released the row" + ) + + release_lock.set() + first.result(timeout=10) + second.result(timeout=10) + + assert renamed_calls == [ + ("edit-lock-target", "renamed-by-first-editor"), + ("renamed-by-first-editor", "renamed-by-second-editor"), + ] + finally: + custom_api_api.validate_runtime_config_declaration = real_validate + session_a.close() + session_b.close() + + +def test_a_row_that_vanishes_after_the_gate_but_before_the_lock_is_a_404_not_a_500( + session_factory, seeded +) -> None: + """The gate helper's own read can find the row and still lose a race to + a concurrent delete that commits before this route's own lock + statement runs. The lock statement must see that as an ordinary + "row not found" (``None``) and let the route's existing 404 handle + it, not surface as an unrelated 500 out of the write path below. + """ + import xagent.web.api.custom_api as custom_api_api + from xagent.web.api.custom_api import CustomApiUpdate + + owner_id, api_id = seeded + current_user = SimpleNamespace(id=owner_id, is_admin=False) + + real_resolve = custom_api_api._resolve_custom_api_for_request + + def resolve_then_delete_concurrently(db_, user_id, aid, **kwargs): + result = real_resolve(db_, user_id, aid, **kwargs) + # A concurrent delete that actually commits, from a separate + # connection, landing strictly between the gate helper's read + # above and the route's own lock statement below. + with session_factory() as other: + other.execute( + sa.delete(UserCustomApi).where(UserCustomApi.custom_api_id == aid) + ) + other.execute(sa.delete(CustomApi).where(CustomApi.id == aid)) + other.commit() + return result + + custom_api_api._resolve_custom_api_for_request = resolve_then_delete_concurrently + db = session_factory() + try: + with pytest.raises(HTTPException) as exc: + custom_api_api.update_custom_api( + api_id, + CustomApiUpdate(name="renamed-after-vanish"), + current_user=current_user, + db=db, + ) + assert exc.value.status_code == 404 + finally: + custom_api_api._resolve_custom_api_for_request = real_resolve + db.close() + + +def test_a_delete_blocks_until_a_concurrent_edits_transaction_finishes( + session_factory, seeded +) -> None: + """``delete_custom_api`` takes the same definition-row lock + ``update_custom_api`` does, in the same order (``CustomApi`` first), + precisely so that a concurrent edit/delete pair cannot deadlock + (PostgreSQL 40P01): the edit's transaction below must finish -- commit + or roll back -- before the delete's own lock statement can proceed, + the same block ``test_a_second_editor_blocks_until_the_first_editors_ + transaction_finishes`` above demonstrates between two edits. Before + delete_custom_api took this lock, its own child-row-first deletion + order (see custom_api.py) and the PUT's parent-row-first order let the + two routes take these same two rows in opposite orders. + """ + import xagent.web.api.custom_api as custom_api_api + from xagent.web.api.custom_api import CustomApiUpdate + + owner_id, api_id = seeded + current_user = SimpleNamespace(id=owner_id, is_admin=False) + + lock_acquired = threading.Event() + release_lock = threading.Event() + second_finished = threading.Event() + + real_validate = custom_api_api.validate_runtime_config_declaration + + def paced_validate(**kwargs): + # The editor's own lock statement runs earlier in the route, before + # this patched call -- by the time this pauses, the editor already + # holds the definition row lock in an uncommitted transaction. + lock_acquired.set() + assert release_lock.wait(timeout=10), "the editor was never released" + return real_validate(**kwargs) + + custom_api_api.validate_runtime_config_declaration = paced_validate + session_a = session_factory() + session_b = session_factory() + try: + + def run_edit(): + return custom_api_api.update_custom_api( + api_id, + CustomApiUpdate(is_active=False), + current_user=current_user, + db=session_a, + ) + + def run_delete(): + result = custom_api_api.delete_custom_api( + api_id, + current_user=current_user, + db=session_b, + ) + second_finished.set() + return result + + with ThreadPoolExecutor(max_workers=2) as executor: + editor = executor.submit(run_edit) + assert lock_acquired.wait(timeout=5), "the editor never reached the lock" + + deleter = executor.submit(run_delete) + # The delete's own lock statement should still be blocked on + # the database at this point. If the two routes took this pair + # of rows in opposite orders (or if either lock were a no-op, + # as on SQLite), the delete would sail through almost + # immediately and this would flip to True. + assert not second_finished.wait(timeout=1.0), ( + "the delete finished before the concurrent editor released " + "the row -- the lock did not actually block it" + ) + + release_lock.set() + editor.result(timeout=10) + deleter.result(timeout=10) + + assert second_finished.is_set() + finally: + custom_api_api.validate_runtime_config_declaration = real_validate + session_a.close() + session_b.close() diff --git a/tests/web/api/test_custom_api_team_connector_edit.py b/tests/web/api/test_custom_api_team_connector_edit.py new file mode 100644 index 0000000000..6c9bc56c25 --- /dev/null +++ b/tests/web/api/test_custom_api_team_connector_edit.py @@ -0,0 +1,653 @@ +"""The edit right on a team-linked Custom API: ``GET``/``PUT +/api/custom-apis/{api_id}`` resolve a caller with no personal row through +the connector access hook instead of 404ing outright, ``can_edit`` falls +back to that verdict for a caller with no personal row, an ``is_active`` +payload from such a caller rejects outright instead of writing a shadow +attribute the response then reads back, a raising hook surfaces as its +declared status rather than a 500, and the verdict is re-resolved once +more after the definition row's lock is taken, refusing the write if it +no longer grants what the pre-lock answer granted. + +Every test installs the access hook through ``snapshot_connector_team_hooks`` +so no hook state leaks between tests or into suites that run after this one. +""" + +from __future__ import annotations + +import pytest +from fastapi import HTTPException +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker + +from xagent.core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError +from xagent.web.api.custom_api import ( + CustomApiUpdate, + get_custom_api, + update_custom_api, +) +from xagent.web.models.custom_api import CustomApi, UserCustomApi +from xagent.web.models.database import Base +from xagent.web.models.user import User +from xagent.web.services.connector_team_scope import ( + ConnectorAccess, + set_connector_team_hooks, + snapshot_connector_team_hooks, +) + + +@pytest.fixture() +def db(): + engine = create_engine("sqlite:///:memory:") + Base.metadata.create_all(engine) + session_factory = sessionmaker(autocommit=False, autoflush=False, bind=engine) + session = session_factory() + try: + yield session + finally: + session.close() + + +def _make_user(db, user_id: int, *, is_admin: bool = False) -> User: + user = User( + id=user_id, username=f"user-{user_id}", password_hash="x", is_admin=is_admin + ) + db.add(user) + db.commit() + return user + + +def _make_owned_api(db, owner_id: int, *, name: str = "shared-api") -> CustomApi: + api = CustomApi(name=name, url="https://example.test/api", method="GET") + db.add(api) + db.flush() + db.add( + UserCustomApi( + user_id=owner_id, + custom_api_id=api.id, + is_owner=True, + can_edit=True, + can_delete=True, + is_active=True, + ) + ) + db.commit() + return api + + +async def _get(api_id, current_user, db): + return get_custom_api(api_id, current_user=current_user, db=db) + + +async def _put(api_id, payload, current_user, db): + return update_custom_api(api_id, payload, current_user=current_user, db=db) + + +def _sequenced_access_hook(*answers): + """An access hook that answers differently on successive calls, so a + test can make the second (post-lock) resolution disagree with the + first. ``None`` in the sequence means an empty answer -- the batch + contract's way of saying "the caller's team does not link this". An + entry that is an exception instance is raised instead of returned, so a + test can make the second resolution fail outright. The last entry + repeats for any further call. Records every call's ``refs`` on + ``.calls`` so a test can pin how many round trips the route pays.""" + calls: list[object] = [] + + def hook(db, user_id, refs): + calls.append(refs) + index = min(len(calls) - 1, len(answers) - 1) + answer = answers[index] + if isinstance(answer, BaseException): + raise answer + if answer is None: + return {} + return {ref: answer for ref in refs} + + hook.calls = calls + return hook + + +class TestGateHelperOnGetAndPut: + @pytest.mark.asyncio + async def test_get_404s_for_an_unrelated_user_with_no_link_and_no_team_access( + self, db + ): + owner = _make_user(db, 1) + stranger = _make_user(db, 2) + api = _make_owned_api(db, owner.id) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=lambda db, user_id, refs: {}) + with pytest.raises(HTTPException) as exc: + await _get(api.id, stranger, db) + assert exc.value.status_code == 404 + + @pytest.mark.asyncio + async def test_get_returns_the_stand_in_for_a_team_member_with_no_personal_row( + self, db + ): + owner = _make_user(db, 1) + member = _make_user(db, 2) + api = _make_owned_api(db, owner.id) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + response = await _get(api.id, member, db) + + assert response.id == api.id + assert response.user_id == member.id + + @pytest.mark.asyncio + async def test_get_owner_behaviour_is_unchanged_with_no_hook_installed(self, db): + owner = _make_user(db, 1) + api = _make_owned_api(db, owner.id) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks() + response = await _get(api.id, owner, db) + + assert response.id == api.id + assert response.user_id == owner.id + + +class TestPutWiringForATeamEditor: + @pytest.mark.asyncio + async def test_team_editor_edit_is_durable_and_creates_no_association_row(self, db): + owner = _make_user(db, 1) + editor = _make_user(db, 2) + api = _make_owned_api(db, owner.id) + api_id = api.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + response = await _put( + api_id, + CustomApiUpdate(description="edited by the team"), + editor, + db, + ) + + assert response.description == "edited by the team" + + # Durability, not staging -- a same-session query would still see + # an uncommitted UPDATE even if the route never committed. + db.rollback() + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.description == "edited by the team" + + # The edit did not fabricate a personal association for the team + # editor -- that would be a get-or-create write on an + # authorization path. + assert ( + db.query(UserCustomApi).filter(UserCustomApi.user_id == editor.id).first() + is None + ) + + @pytest.mark.asyncio + async def test_a_member_with_a_personal_row_edits_the_shared_config_durably( + self, db + ): + """The MCP twin of this test: a caller whose own personal row does + not grant edit, widened by a granting team verdict. ``can_edit=False`` + on the personal row is the point -- it is what keeps + ``_resolve_custom_api_for_request``'s ``skip_resolution_when=lambda + ua: bool(ua.can_edit)`` from short-circuiting before the verdict is + even resolved.""" + owner = _make_user(db, 1) + member = _make_user(db, 2) + api = _make_owned_api(db, owner.id, name="both-rows-custom-api") + api_id = api.id + db.add( + UserCustomApi( + user_id=member.id, + custom_api_id=api_id, + is_owner=False, + can_edit=False, + is_active=True, + ) + ) + db.commit() + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + response = await _put( + api_id, + CustomApiUpdate(description="widened-by-the-team"), + member, + db, + ) + + assert response.description == "widened-by-the-team" + + db.rollback() + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.description == "widened-by-the-team" + assert ( + db.query(UserCustomApi) + .filter( + UserCustomApi.user_id == member.id, + UserCustomApi.custom_api_id == api_id, + ) + .count() + == 1 + ) + + @pytest.mark.asyncio + async def test_view_only_team_member_cannot_tamper_the_shared_config(self, db): + owner = _make_user(db, 1) + member = _make_user(db, 2) + api = _make_owned_api(db, owner.id) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=False) + for ref in refs + } + ) + with pytest.raises(HTTPException) as exc: + await _put( + api.id, + CustomApiUpdate(description="should not land"), + member, + db, + ) + assert exc.value.status_code == 403 + + +@pytest.mark.asyncio +async def test_a_denying_verdict_stand_in_is_403_on_an_empty_payload_too(db): + """The MCP side needed a new guard for this (see + TestADenyingStandInIsRefusedRatherThanReportedSuccessful in + test_mcp_team_connector_edit.py) because its personal-field guard and + tamper check only fire for specific payload shapes. This route's own + gate (custom_api.py's ``can_edit`` check) has no such carve-out: it + requires the edit right for every payload, including an empty one, so + a stand-in whose verdict denies edit is already 403 here without any + new code. This test exists to pin that so it cannot be changed out + from under this route's contract unnoticed. + """ + owner = _make_user(db, 1) + member = _make_user(db, 2) + api = _make_owned_api(db, owner.id, name="denying-stand-in-target") + api_id = api.id + # Captured as plain values, not read off ``api`` after the call: ``api`` + # and the ``refreshed`` row below share the same identity-mapped Python + # object in this session, so comparing one against the other after the + # call would be comparing the object with itself and could never fail. + original_name = str(api.name) + original_description = str(api.description) if api.description is not None else None + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=False) for ref in refs + } + ) + with pytest.raises(HTTPException) as exc: + await _put(api_id, CustomApiUpdate(), member, db) + assert exc.value.status_code == 403 + + db.rollback() + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.name == original_name + assert refreshed.description == original_description + assert ( + db.query(UserCustomApi).filter(UserCustomApi.user_id == member.id).count() == 0 + ) + + +class TestIsActiveRejectionForAStandIn: + @pytest.mark.asyncio + async def test_is_active_from_a_caller_with_no_personal_row_is_400_not_a_silent_drop( + self, db + ): + owner = _make_user(db, 1) + editor = _make_user(db, 2) + api = _make_owned_api(db, owner.id, name="unchanged-name") + api_id = api.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + with pytest.raises(HTTPException) as exc: + await _put( + api_id, + CustomApiUpdate(is_active=False), + editor, + db, + ) + + # 1. the declared status. + assert exc.value.status_code == 400 + assert "personal connection" in str(exc.value.detail) + + # 2. nothing persisted -- the exception was raised before any + # commit, so a same-session rollback-then-requery must still show + # no personal association row for this caller. + db.rollback() + assert ( + db.query(UserCustomApi).filter(UserCustomApi.user_id == editor.id).first() + is None + ) + + # 3. the response body does not claim the change -- the call + # raised rather than returning, so no ``CustomApiResponse`` ever + # left the route carrying an ``is_active`` value nothing wrote. + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.name == "unchanged-name" + + +class TestTypedErrorArm: + """A raising hook still surfaces its declared status for a caller with + no working personal row -- the verdict is genuinely the gate for that + population and must stay fail-closed. An owner's row already decides + ``GET``'s answer (it never reads the verdict at all) and ``PUT``'s + (``can_edit`` is already ``True``), so neither ever calls the hook for + an owner's row; that population is pinned separately, below, in + ``TestOwnerIsImmuneToAHookFailure``.""" + + @pytest.mark.asyncio + async def test_get_surfaces_a_raising_hooks_declared_status(self, db): + owner = _make_user(db, 1) + member = _make_user(db, 2) + api = _make_owned_api(db, owner.id) + + def boom(*_a, **_k): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + with pytest.raises(HTTPException) as exc: + await _get(api.id, member, db) + + assert exc.value.status_code == 503 + + @pytest.mark.asyncio + async def test_put_surfaces_a_raising_hooks_declared_status_and_leaves_the_row_unchanged( + self, db + ): + owner = _make_user(db, 1) + member = _make_user(db, 2) + api = _make_owned_api(db, owner.id, name="pristine") + api_id = api.id + + def boom(*_a, **_k): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + with pytest.raises(HTTPException) as exc: + await _put( + api_id, + CustomApiUpdate(name="should-not-land"), + member, + db, + ) + + assert exc.value.status_code == 503 + + db.rollback() + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.name == "pristine" + + @pytest.mark.asyncio + async def test_put_passes_through_a_planted_connector_runtime_error_by_its_own_status( + self, db + ): + owner = _make_user(db, 1) + member = _make_user(db, 2) + api = _make_owned_api(db, owner.id) + + def boom(*_a, **_k): + raise ConnectorRuntimeError("planted", "planted failure", status_code=409) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + with pytest.raises(HTTPException) as exc: + await _put( + api.id, + CustomApiUpdate(description="irrelevant"), + member, + db, + ) + + assert exc.value.status_code == 409 + assert exc.value.detail == "planted failure" + + @pytest.mark.asyncio + async def test_a_raising_rename_hook_surfaces_its_declared_status_not_a_500( + self, db + ): + """The MCP side's PUT already translates this (mcp.py:3853-3857); + without the same arm here the seam's 503 reaches the client as a + generic 500, and the two connector kinds answer the same failure + differently.""" + owner = _make_user(db, 1) + api = _make_owned_api(db, owner.id, name="rename-hook-raises") + api_id = api.id + original_name = str(api.name) + + def boom(*_a, **_k): + raise ConnectorRuntimeError( + "connector_runtime_unavailable", + "Connector team scope is unavailable.", + status_code=503, + ) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(renamed=boom) + with pytest.raises(HTTPException) as exc: + update_custom_api( + api_id, + CustomApiUpdate(name="renamed-by-the-test"), + current_user=owner, + db=db, + ) + + assert exc.value.status_code == 503 + # Zero side effects: the rename that triggered the hook is rolled + # back with everything else this request had staged. + db.rollback() + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.name == original_name + + +class TestOwnerIsImmuneToAHookFailure: + """An owner's row already decides both routes' answers on its own -- + ``GET`` never reads the verdict at all, and ``PUT``'s ``can_edit`` is + already ``True`` -- so neither ever calls the hook for an owner's row. + A hook that would raise must therefore never surface: both routes + return their normal success status, unaffected by whatever the hook + would have done.""" + + @pytest.mark.asyncio + async def test_get_and_put_succeed_for_an_owner_even_though_the_hook_would_raise( + self, db + ): + owner = _make_user(db, 1) + api = _make_owned_api(db, owner.id, name="owner-immune") + api_id = api.id + + def boom(*_a, **_k): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + get_response = await _get(api_id, owner, db) + put_response = await _put( + api_id, + CustomApiUpdate(description="edited by the owner"), + owner, + db, + ) + + assert get_response.id == api_id + assert put_response.description == "edited by the owner" + + +class TestTheVerdictIsRevalidatedUnderTheDefinitionLock: + """The same re-check as the MCP side's PUT (see + TestTheVerdictIsRevalidatedUnderTheDefinitionLock in + test_mcp_team_connector_edit.py), for the same reason: the verdict + granting a stand-in edit access was resolved before this route's own + row lock existed, and the installing application can revoke the link + at any moment through its own tables, which this lock does not cover. + No personal-field exemption here: this route's gate requires can_edit + for every payload, including an is_active-only one, so the verdict is + the authority for everything this route admits. + """ + + async def _run(self, db, *, hook): + owner = _make_user(db, 1) + member = _make_user(db, 2) + api = _make_owned_api(db, owner.id, name="revalidated-under-lock") + api_id = api.id + # Captured as plain values before the call, not read off ``api`` + # afterwards: ``api`` and the requery below share the same + # identity-mapped Python object in this session, so comparing one + # against the other after the call would be comparing the object + # with itself and could never fail. + original_name = str(api.name) + original_description = ( + str(api.description) if api.description is not None else None + ) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + result = {} + try: + result["response"] = await _put( + api_id, + CustomApiUpdate(description="edited-while-in-flight"), + member, + db, + ) + except HTTPException as exc: + result["error"] = exc + return api, api_id, result, original_name, original_description + + @pytest.mark.asyncio + async def test_revoked_between_resolution_and_lock_is_refused(self, db): + hook = _sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True), None + ) + _api, api_id, result, original_name, original_description = await self._run( + db, hook=hook + ) + + assert result["error"].status_code == 403 + db.rollback() + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.name == original_name + assert refreshed.description == original_description + assert db.query(UserCustomApi).filter(UserCustomApi.user_id == 2).count() == 0 + + @pytest.mark.asyncio + async def test_downgraded_to_not_editable_between_resolution_and_lock_is_refused( + self, db + ): + hook = _sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True), + ConnectorAccess(team_owned=True, can_edit=False), + ) + _api, api_id, result, original_name, original_description = await self._run( + db, hook=hook + ) + + assert result["error"].status_code == 403 + db.rollback() + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.name == original_name + assert refreshed.description == original_description + assert db.query(UserCustomApi).filter(UserCustomApi.user_id == 2).count() == 0 + + @pytest.mark.asyncio + async def test_still_granted_on_recheck_commits_durably(self, db): + hook = _sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True), + ConnectorAccess(team_owned=True, can_edit=True), + ) + ( + _api, + api_id, + result, + _original_name, + _original_description, + ) = await self._run(db, hook=hook) + + assert "error" not in result + assert result["response"].description == "edited-while-in-flight" + + db.rollback() + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.description == "edited-while-in-flight" + + @pytest.mark.asyncio + async def test_recheck_that_raises_surfaces_the_hooks_own_status_with_zero_side_effects( + self, db + ): + hook = _sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True), + ValueError("hook exploded during recheck"), + ) + _api, api_id, result, original_name, original_description = await self._run( + db, hook=hook + ) + + assert result["error"].status_code == 503 + db.rollback() + refreshed = db.query(CustomApi).filter(CustomApi.id == api_id).one() + assert refreshed.name == original_name + assert refreshed.description == original_description + assert db.query(UserCustomApi).filter(UserCustomApi.user_id == 2).count() == 0 + + +class TestTheRecheckCostsExactlyOneExtraHookCall: + """The Custom API halves of cells i and j in the design's call-count + table -- MCP's own halves (cells e-h) live in + test_mcp_team_connector_edit.py.""" + + @pytest.mark.asyncio + async def test_a_granting_stand_in_editing_the_shared_config_pays_two_calls( + self, db + ): + owner = _make_user(db, 1) + member = _make_user(db, 2) + api = _make_owned_api(db, owner.id, name="cost-stand-in-shared") + api_id = api.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + await _put(api_id, CustomApiUpdate(description="shared-edit"), member, db) + + assert len(hook.calls) == 2 + + @pytest.mark.asyncio + async def test_an_owner_pays_zero_calls(self, db): + owner = _make_user(db, 1) + api = _make_owned_api(db, owner.id, name="cost-owner") + api_id = api.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + await _put(api_id, CustomApiUpdate(description="owner-edit"), owner, db) + + assert len(hook.calls) == 0 diff --git a/tests/web/api/test_mcp_reported_edit_permission.py b/tests/web/api/test_mcp_reported_edit_permission.py new file mode 100644 index 0000000000..615c724c7b --- /dev/null +++ b/tests/web/api/test_mcp_reported_edit_permission.py @@ -0,0 +1,1968 @@ +"""The reported ``can_edit_global``/``can_configure`` fields agree with what +the gates in earlier stages actually enforce, across every response-builder +call site and both connector kinds -- and the four MCP OAuth routes, the +rename call's scope, and every route's no-hook-installed shape are all +unchanged by threading that verdict through. + +Every test installs hooks (or explicitly installs none) through +``snapshot_connector_team_hooks`` so no hook state leaks between tests or +into suites that run after this one. +""" + +from __future__ import annotations + +import pytest +import sqlalchemy as sa +from fastapi import HTTPException +from sqlalchemy import create_engine, event +from sqlalchemy.orm import sessionmaker + +from xagent.core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError +from xagent.web.api.custom_api import ( + CustomApiUpdate, + delete_custom_api, + get_custom_api, + update_custom_api, +) +from xagent.web.api.mcp import ( + MCPAppConnectRequest, + MCPOAuthConnectRequest, + MCPOAuthDiscoverRequest, + MCPServerUpdate, + connect_mcp_app, + connect_mcp_oauth, + delete_mcp_oauth_grant, + delete_mcp_server, + discover_mcp_oauth, + get_mcp_oauth_status, + get_mcp_server, + get_mcp_servers, + list_mcp_apps, + toggle_mcp_server, + update_mcp_server, +) +from xagent.web.models.agent import Agent +from xagent.web.models.custom_api import CustomApi, UserCustomApi +from xagent.web.models.database import Base +from xagent.web.models.mcp import MCPServer, UserMCPServer +from xagent.web.models.public_mcp import PublicMCPApp +from xagent.web.models.user import User +from xagent.web.services.connector_team_scope import ( + ConnectorAccess, + set_connector_team_hooks, + snapshot_connector_team_hooks, +) + + +@pytest.fixture() +def db(): + engine = create_engine("sqlite:///:memory:") + Base.metadata.create_all(engine) + session_factory = sessionmaker(autocommit=False, autoflush=False, bind=engine) + session = session_factory() + try: + yield session + finally: + session.close() + + +def _make_user(db, user_id: int, *, is_admin: bool = False) -> User: + user = User( + id=user_id, username=f"user-{user_id}", password_hash="x", is_admin=is_admin + ) + db.add(user) + db.commit() + return user + + +def _make_owned_server(db, owner_id: int, *, name: str = "shared-server") -> MCPServer: + server = MCPServer(name=name, transport="stdio", managed="external", command="true") + db.add(server) + db.flush() + db.add( + UserMCPServer( + user_id=owner_id, + mcpserver_id=server.id, + is_owner=True, + is_active=True, + ) + ) + db.commit() + return server + + +def _make_owned_api(db, owner_id: int, *, name: str = "shared-api") -> CustomApi: + api = CustomApi(name=name, url="https://example.com/api", method="GET") + db.add(api) + db.flush() + db.add( + UserCustomApi( + user_id=owner_id, + custom_api_id=api.id, + is_owner=True, + can_edit=True, + can_delete=True, + is_active=True, + ) + ) + db.commit() + return api + + +def _fixed_answer_hook(access_answer): + """Build a batch access hook that answers every requested ref with the + same fixed verdict -- or, when ``access_answer`` is ``None``, answers + with an empty map, which is how "the caller's team does not link this" + is expressed under the batch contract.""" + + def _hook(db, user_id, refs): + if access_answer is None: + return {} + return {ref: access_answer for ref in refs} + + return _hook + + +class TestListEndpointAccessHookCallBudget: + """The list endpoint asks the access hook at most once per request, no + matter how many rows need a verdict -- pinned across two different + population sizes with a counting test double. Counting hook calls alone + would hide any SQL the endpoint's own queries issue on top of it, or + that the hook's own body issues, so a SQLAlchemy + ``before_cursor_execute`` listener additionally pins the *total* number + of SQL statements for two different row counts: if either grew with row + count, that would mean the endpoint reverted to a per-row hook call + after all.""" + + @pytest.mark.parametrize("num_rows", [2, 6], ids=["R=2", "R=6"]) + def test_the_list_asks_the_access_hook_exactly_once_no_matter_how_many_rows( + self, db, num_rows + ): + caller = _make_user(db, 100 + num_rows) + other_owner = _make_user(db, 200 + num_rows) + + # P = 2 personal rows the caller owns outright -- never worth a + # hook call. + owned = [ + _make_owned_server(db, caller.id, name=f"owned-{num_rows}-{i}") + for i in range(2) + ] + + # Q = num_rows personal rows the caller holds but does not own (a + # second link on a connector someone else owns). + shared_personal = [] + for i in range(num_rows): + server = _make_owned_server( + db, other_owner.id, name=f"shared-personal-{num_rows}-{i}" + ) + db.add( + UserMCPServer( + user_id=caller.id, + mcpserver_id=server.id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + shared_personal.append(server) + + # R = num_rows rows the caller has no personal row for at all, made + # visible through the separate visibility hook (not the access hook + # under test here). + stand_in = [ + _make_owned_server(db, other_owner.id, name=f"stand-in-{num_rows}-{i}") + for i in range(num_rows) + ] + + # Read every id the hooks below will need before the query listener + # attaches: the objects above were expired by their own setup + # commits (session default expire_on_commit=True), so reading .id + # for the first time inside the measured window would count as a + # query the *endpoint* issues, when it is really just this test's + # own setup catching up. caller.id specifically: get_mcp_servers + # reads current_user.id as its very first act. + _ = caller.id + owned_ids = {s.id for s in owned} + shared_personal_ids = {s.id for s in shared_personal} + stand_in_ids = {s.id for s in stand_in} + + calls: list[object] = [] + + def counting_access_hook(hook_db, user_id, refs): + calls.append(refs) + # A realistic hook resolves its own team-membership rows to + # answer the batch -- simulated here as three throwaway + # statements run once per call, regardless of how many refs + # were asked about. If the endpoint ever regressed to one hook + # call per row, the total statement count below would grow + # with num_rows; it must not. + for _ in range(3): + hook_db.execute(sa.select(sa.literal(1))) + return { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + + def visibility_hook(_db, _user_id): + return {"mcp": set(stand_in_ids), "custom_api": set()} + + queries: list[str] = [] + + def record_query(conn, cursor, statement, parameters, context, executemany): + del conn, cursor, parameters, context, executemany + queries.append(statement) + + engine = db.get_bind() + event.listen(engine, "before_cursor_execute", record_query) + try: + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=counting_access_hook, visibility=visibility_hook + ) + get_mcp_servers(current_user=caller, db=db) + finally: + event.remove(engine, "before_cursor_execute", record_query) + + assert len(calls) == 1 + requested_refs = calls[0] + assert set(requested_refs) == { + ("mcp", sid) for sid in shared_personal_ids | stand_in_ids + } + assert {rid for (_kind, rid) in requested_refs}.isdisjoint(owned_ids) + + # The hook-call count above cannot see the SQL the endpoint's own + # queries issue on top of it, or the hook's own three statements. + # Observed by running this exact population and reading the + # recorded statements, not derived from a formula -- but pinned as + # a constant on purpose: it must come out identical for num_rows=2 + # and num_rows=6, since every row within P, Q or R is served by one + # batched IN-clause query (or the single hook call), never a query + # or a hook call per row. Includes one additional catalog-keys + # SELECT that fires once per request, not once per row: every + # granting verdict this hook returns has to be checked against the + # platform catalog before it can be trusted as an edit grant, and + # that catalog is read once and shared across every row's check. + assert len(queries) == 8, queries + + +class TestAppsListEndpointAccessHookCallBudget: + """The sister endpoint's budget: ``/api/mcp/apps`` (``location=local``) + also asks the access hook at most once per request, covering both + connector kinds in the same call, independent of row count.""" + + @pytest.mark.parametrize("num_rows", [2, 6], ids=["R=2", "R=6"]) + def test_the_apps_listing_asks_the_access_hook_exactly_once_no_matter_how_many_rows( + self, db, num_rows + ): + owner = _make_user(db, 300 + num_rows) + member = _make_user(db, 400 + num_rows) + + # Personal rows the member owns outright -- a personal row already + # answers can_configure on its own, so these are never worth a + # hook call. + owned_mcp = [ + _make_owned_server(db, member.id, name=f"apps-owned-mcp-{num_rows}-{i}") + for i in range(2) + ] + owned_api = [ + _make_owned_api(db, member.id, name=f"apps-owned-api-{num_rows}-{i}") + for i in range(2) + ] + + # Stand-in rows across both kinds -- every one of these needs a + # verdict. + stand_in_mcp = [ + _make_owned_server(db, owner.id, name=f"apps-stand-in-mcp-{num_rows}-{i}") + for i in range(num_rows) + ] + stand_in_api = [ + _make_owned_api(db, owner.id, name=f"apps-stand-in-api-{num_rows}-{i}") + for i in range(num_rows) + ] + + _ = member.id + owned_mcp_ids = {s.id for s in owned_mcp} + owned_api_ids = {a.id for a in owned_api} + stand_in_mcp_ids = {s.id for s in stand_in_mcp} + stand_in_api_ids = {a.id for a in stand_in_api} + + calls: list[object] = [] + + def counting_access_hook(hook_db, user_id, refs): + calls.append(refs) + for _ in range(3): + hook_db.execute(sa.select(sa.literal(1))) + return { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + + def visibility_hook(_db, _user_id): + return {"mcp": set(stand_in_mcp_ids), "custom_api": set(stand_in_api_ids)} + + queries: list[str] = [] + + def record_query(conn, cursor, statement, parameters, context, executemany): + del conn, cursor, parameters, context, executemany + queries.append(statement) + + engine = db.get_bind() + event.listen(engine, "before_cursor_execute", record_query) + try: + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=counting_access_hook, visibility=visibility_hook + ) + list_mcp_apps(location="local", current_user=member, db=db) + finally: + event.remove(engine, "before_cursor_execute", record_query) + + assert len(calls) == 1 + requested_refs = calls[0] + assert set(requested_refs) == {("mcp", sid) for sid in stand_in_mcp_ids} | { + ("custom_api", aid) for aid in stand_in_api_ids + } + called_mcp_ids = {rid for (kind, rid) in requested_refs if kind == "mcp"} + called_api_ids = {rid for (kind, rid) in requested_refs if kind == "custom_api"} + assert called_mcp_ids.isdisjoint(owned_mcp_ids) + assert called_api_ids.isdisjoint(owned_api_ids) + + # Pinned as a constant for the same reason as the sibling test + # above: it must be identical for num_rows=2 and num_rows=6. + assert len(queries) == 10, queries + + +class TestDegradedListingQueryCostGrowsWithRowCount: + """The healthy half of this class's own name is already covered above + (the call budget classes pin a constant statement count for a healthy + hook). This class covers the other half: when the access hook fails, + ``_restore_session_after_hook_failure`` (connector_team_scope.py) calls + ``db.rollback()`` to recover the session the failed hook may have left + mid-statement. The same rollback, and so the same cost, applies when + the hook returns normally but its answer is rejected by the seam's + validator: the door restores the session for both. On SQLAlchemy + 2.0.48, that rollback expires every already-loaded object's every + mapped field, including primary keys -- + so the two listing loops below, each iterating a stand-in row per + connector, re-``SELECT`` that row one at a time on next access. Repo + issue #1711 independently confirmed this rollback behavior. This test + exists to pin that cost as a number CI will notice moving, not to + remove it: the recovery itself is required (a failed hook can leave a + statement failed on the shared session, and the next request on that + session needs it usable again), and there is no cheaper way to get + there available to this seam. + + Counts only ``SELECT`` statements (``q.lstrip().upper().startswith + ("SELECT")``) -- a different count than the two call-budget classes + above, which count every statement including the hook's own. The two + numbers are not meant to line up; this class exists to see the + per-row re-select specifically, and INSERT/UPDATE noise from a + healthy hook's own bookkeeping would only blur that. + + Population: ``num_rows`` stand-in MCP servers and ``num_rows`` + stand-in Custom APIs (owner-owned, visible to the caller only through + the visibility hook), with the caller holding zero personal + association rows of its own -- every row in both listings therefore + needs a verdict, so the degradation this class measures actually + fires for the whole listing, not just part of it. + """ + + # Measured directly against this PR's own code (2026-08-26, SQLite, + # SQLAlchemy 2.0.48): constant while healthy, BASE + 2*num_rows while + # failing. The "+2*num_rows" is one re-SELECT for the MCPServer/ + # CustomApi row and one for the UserMCPServer/UserCustomApi row per + # stand-in connector (both listings build one stand-in per row across + # both kinds; num_rows stand-ins per kind here, so 2*num_rows total + # re-selects). The extra "+1" on ``servers`` alone reflects that + # endpoint's own extra per-owner-lookup query the apps endpoint does + # not have; it does not grow with num_rows. + # + # ``HEALTHY["servers"]`` carries one further "+1" that ``BASE["servers"]`` + # does not: a healthy hook here always grants edit, so the servers + # listing's platform-catalog check reads the catalog once per request. + # The failing hook's verdicts map is empty, so that check never fires -- + # BASE stays the pre-catalog-check number on purpose. + HEALTHY = {"apps": 7, "servers": 6} + BASE = {"apps": 7, "servers": 5} + EXTRA = {"apps": 0, "servers": 1} + + def _run(self, db, *, endpoint, num_rows, failing): + owner = _make_user(db, 900 + num_rows * 10 + (1 if failing else 0)) + member = _make_user(db, 950 + num_rows * 10 + (1 if failing else 0)) + + stand_in_mcp = [ + _make_owned_server( + db, owner.id, name=f"cost-mcp-{endpoint}-{num_rows}-{failing}-{i}" + ) + for i in range(num_rows) + ] + stand_in_api = [ + _make_owned_api( + db, owner.id, name=f"cost-api-{endpoint}-{num_rows}-{failing}-{i}" + ) + for i in range(num_rows) + ] + # Warms member's attributes before the listener below is attached: + # every _make_user/_make_owned_* call above commits, which expires + # every already-loaded object under this session's default + # expire_on_commit. Without this access, the route's own first + # touch of current_user.id would trigger member's refresh SELECT + # after the listener is attached, inflating the count by one for a + # reason that has nothing to do with the degradation this class + # measures. + _ = member.id + mcp_ids = {s.id for s in stand_in_mcp} + api_ids = {a.id for a in stand_in_api} + + def failing_hook(hook_db, user_id, refs): + raise ValueError("hook exploded") + + def ok_hook(hook_db, user_id, refs): + return { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + + def visibility_hook(_db, _user_id): + return {"mcp": set(mcp_ids), "custom_api": set(api_ids)} + + queries: list[str] = [] + + def record_query(conn, cursor, statement, parameters, context, executemany): + del conn, cursor, parameters, context, executemany + queries.append(statement) + + engine = db.get_bind() + event.listen(engine, "before_cursor_execute", record_query) + try: + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=failing_hook if failing else ok_hook, + visibility=visibility_hook, + ) + if endpoint == "apps": + rows = list_mcp_apps(location="local", current_user=member, db=db) + else: + rows = get_mcp_servers(current_user=member, db=db) + finally: + event.remove(engine, "before_cursor_execute", record_query) + + assert len(rows) == 2 * num_rows + n_select = sum(1 for q in queries if q.lstrip().upper().startswith("SELECT")) + return n_select + + @pytest.mark.parametrize("failing", [False, True], ids=["healthy", "failing"]) + @pytest.mark.parametrize("endpoint", ["apps", "servers"]) + @pytest.mark.parametrize("num_rows", [2, 6], ids=["R=2", "R=6"]) + def test_select_count(self, db, endpoint, num_rows, failing): + n_select = self._run(db, endpoint=endpoint, num_rows=num_rows, failing=failing) + if failing: + expected = self.BASE[endpoint] + 2 * num_rows + self.EXTRA[endpoint] + assert n_select == expected, ( + f"expected {expected} SELECTs for a failing hook with " + f"num_rows={num_rows} on {endpoint} (base " + f"{self.BASE[endpoint]} + 2*{num_rows} row re-selects + " + f"{self.EXTRA[endpoint]} endpoint-specific extra), got " + f"{n_select}" + ) + else: + assert n_select == self.HEALTHY[endpoint], ( + f"expected a constant {self.HEALTHY[endpoint]} SELECTs for " + f"a healthy hook on {endpoint} regardless of num_rows, got " + f"{n_select}" + ) + + +class TestReportedEditPermissionConsistencyMcp: + """The response's can_edit_global must agree across every surface that + reports it, for the same (user, connector) -- for MCP connectors, across + the list, GET, PUT's response and toggle's response. + + One population is the exception: a stand-in whose verdict denies edit + no longer gets a PUT response to compare at all -- that payload's + writable field set is empty, so the route refuses it outright (see + TestADenyingStandInIsRefusedRatherThanReportedSuccessful in + test_mcp_team_connector_edit.py) rather than reporting a decorative + can_edit_global on a write that could never have landed.""" + + @pytest.mark.parametrize( + "population,access_answer,has_personal_row", + [ + ("owner", None, True), + ("personal_non_owner_no_team_link", None, True), + ( + # The PR's own central capability: a caller who already has + # a personal row that does not grant edit, widened by a + # granting team verdict. Every other population here either + # has no personal row (the stand-ins) or no verdict. + "personal_row_and_granting_verdict", + ConnectorAccess(team_owned=True, can_edit=True), + True, + ), + ( + "stand_in_granting_edit", + ConnectorAccess(team_owned=True, can_edit=True), + False, + ), + ( + "stand_in_denying_edit", + ConnectorAccess(team_owned=True, can_edit=False), + False, + ), + ( + # The admin bypass in _check_mcp_permission wins even over a + # verdict that itself denies edit -- this is the one + # population where the two connector kinds genuinely + # diverge (Custom API's own gate has no admin bypass at + # all), so it is pinned per kind, not by cross-kind equality. + "platform_admin", + ConnectorAccess(team_owned=True, can_edit=False), + False, + ), + ], + ) + async def test_can_edit_global_agrees_across_list_get_put_and_toggle( + self, db, population, access_answer, has_personal_row + ): + owner = _make_user(db, 10) + if population == "owner": + caller = owner + elif population == "platform_admin": + caller = _make_user(db, 12, is_admin=True) + else: + caller = _make_user(db, 11) + server = _make_owned_server(db, owner.id, name=f"consistency-mcp-{population}") + server_id = server.id + + if population in ( + "personal_non_owner_no_team_link", + "personal_row_and_granting_verdict", + ): + db.add( + UserMCPServer( + user_id=caller.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + + expected = population in ("owner", "platform_admin") or bool( + access_answer is not None and access_answer.can_edit + ) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=_fixed_answer_hook(access_answer), + visibility=lambda _db, _uid: {"mcp": {server_id}, "custom_api": set()}, + ) + + list_entries = get_mcp_servers(current_user=caller, db=db) + list_entry = next(r for r in list_entries if r.id == server_id) + + get_response = get_mcp_server(server_id, current_user=caller, db=db) + + # A denying stand-in's PUT no longer reaches a can_edit_global + # value to agree with: it is refused outright before this route + # builds a response at all (empty writable field set -- see + # TestADenyingStandInIsRefusedRatherThanReportedSuccessful in + # test_mcp_team_connector_edit.py). The other three surfaces + # below are unaffected by that guard and still agree on + # ``expected``. + put_response = None + if population == "stand_in_denying_edit": + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, MCPServerUpdate(), current_user=caller, db=db + ) + assert exc.value.status_code == 403 + else: + put_response = update_mcp_server( + server_id, MCPServerUpdate(), current_user=caller, db=db + ) + + toggle_response = None + if has_personal_row: + toggle_response = toggle_mcp_server( + server_id, current_user=caller, db=db + ) + + assert list_entry.can_edit_global == expected + assert get_response.can_edit_global == expected + if put_response is not None: + assert put_response.can_edit_global == expected + if toggle_response is not None: + assert toggle_response.can_edit_global == expected + + +class TestReportedEditPermissionConsistencyCustomApi: + """The same agreement, for the Custom API kind: ``_custom_api_to_mcp_response`` has no + ``_check_mcp_permission``-shaped gate to compare against and Custom + API's own ``GET``/``PUT`` response model carries no ``can_edit_global`` + field at all -- so the surface to agree with is not a second reported + field but ``update_custom_api``'s actual 2xx/403 outcome, exactly the + motivating case: the list must not report ``False`` for a connector + whose ``PUT`` now succeeds.""" + + @pytest.mark.parametrize( + "population,access_answer", + [ + ("owner", None), + ("personal_non_owner_no_team_link", None), + ( + "personal_row_and_granting_verdict", + ConnectorAccess(team_owned=True, can_edit=True), + ), + ( + "stand_in_granting_edit", + ConnectorAccess(team_owned=True, can_edit=True), + ), + ( + "stand_in_denying_edit", + ConnectorAccess(team_owned=True, can_edit=False), + ), + ( + # Unlike the MCP kind, update_custom_api's own gate has no + # admin bypass at all -- so a platform admin with no + # personal row and a denying verdict is refused just like + # any other caller, and the list must agree by reporting + # False, not by copying MCP's True. + "platform_admin", + ConnectorAccess(team_owned=True, can_edit=False), + ), + ], + ) + async def test_list_can_edit_global_agrees_with_whether_put_actually_succeeds( + self, db, population, access_answer + ): + owner = _make_user(db, 20) + if population == "owner": + caller = owner + elif population == "platform_admin": + caller = _make_user(db, 22, is_admin=True) + else: + caller = _make_user(db, 21) + api = _make_owned_api(db, owner.id, name=f"consistency-api-{population}") + api_id = api.id + + if population in ( + "personal_non_owner_no_team_link", + "personal_row_and_granting_verdict", + ): + db.add( + UserCustomApi( + user_id=caller.id, + custom_api_id=api_id, + is_owner=False, + can_edit=False, + is_active=True, + ) + ) + db.commit() + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=_fixed_answer_hook(access_answer), + visibility=lambda _db, _uid: {"mcp": set(), "custom_api": {api_id}}, + ) + + list_entries = get_mcp_servers(current_user=caller, db=db) + list_entry = next( + r + for r in list_entries + if r.id == api_id and r.transport == "custom_api" + ) + + try: + update_custom_api( + api_id, + CustomApiUpdate(description="edited by the consistency test"), + current_user=caller, + db=db, + ) + put_succeeded = True + except HTTPException as exc: + assert exc.status_code == 403 + put_succeeded = False + + assert list_entry.can_edit_global == put_succeeded + if population == "platform_admin": + # Pinned by value, not only by cross-surface agreement: a + # regression that adds an admin bypass to the list's formula + # alone would leave this False on one side and True on the + # other, which the equality assertion above already catches -- + # this makes the intended, current answer explicit too. + assert list_entry.can_edit_global is False + assert put_succeeded is False + + +class TestLocalCanConfigureWidening: + """``_local_mcp_can_configure`` answers True for a stand-in whose team + access verdict links the connector but denies edit -- visible and + reachable rather than invisible on ``association is None`` alone, for + both connector kinds.""" + + def test_mcp_stand_in_with_a_linked_but_not_editable_verdict_is_configurable( + self, db + ): + owner = _make_user(db, 30) + member = _make_user(db, 31) + server = _make_owned_server(db, owner.id, name="visible-not-editable-mcp") + server_id = server.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=_fixed_answer_hook( + ConnectorAccess(team_owned=True, can_edit=False) + ), + visibility=lambda _db, _uid: {"mcp": {server_id}, "custom_api": set()}, + ) + entries = list_mcp_apps(location="local", current_user=member, db=db) + entry = next(e for e in entries if e["server_id"] == server_id) + assert entry["can_configure"] is True + + # The actual route (fixed independently of this UI hint) already + # resolves for this population -- this proves the hint agrees. + response = get_mcp_server(server_id, current_user=member, db=db) + assert response.id == server_id + + async def test_custom_api_stand_in_with_a_linked_but_not_editable_verdict_is_configurable( + self, db + ): + owner = _make_user(db, 32) + member = _make_user(db, 33) + api = _make_owned_api(db, owner.id, name="visible-not-editable-api") + api_id = api.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=_fixed_answer_hook( + ConnectorAccess(team_owned=True, can_edit=False) + ), + visibility=lambda _db, _uid: {"mcp": set(), "custom_api": {api_id}}, + ) + entries = list_mcp_apps(location="local", current_user=member, db=db) + entry = next( + e + for e in entries + if e["server_id"] == api_id and e["transport"] == "custom_api" + ) + assert entry["can_configure"] is True + + response = get_custom_api(api_id, current_user=member, db=db) + assert response.id == api_id + + +class TestOAuthRoutesKeepTheirOwnGate: + """The four MCP OAuth routes keep the old personal-row-only helper and + still 404 a team member with no personal row, verdict or not.""" + + async def test_all_four_oauth_routes_404_a_team_member_with_no_personal_row( + self, db + ): + owner = _make_user(db, 40) + member = _make_user(db, 41) + server = _make_owned_server(db, owner.id, name="oauth-gate-untouched") + server_id = server.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=_fixed_answer_hook( + ConnectorAccess(team_owned=True, can_edit=True) + ) + ) + + with pytest.raises(HTTPException) as exc: + await discover_mcp_oauth( + server_id, MCPOAuthDiscoverRequest(), current_user=member, db=db + ) + assert exc.value.status_code == 404 + + with pytest.raises(HTTPException) as exc: + await connect_mcp_oauth( + server_id, + MCPOAuthConnectRequest(), + current_user=member, + db=db, + accept=None, + ) + assert exc.value.status_code == 404 + + with pytest.raises(HTTPException) as exc: + await get_mcp_oauth_status(server_id, current_user=member, db=db) + assert exc.value.status_code == 404 + + with pytest.raises(HTTPException) as exc: + await delete_mcp_oauth_grant(server_id, 1, current_user=member, db=db) + assert exc.value.status_code == 404 + + +class TestDenyingVerdictIsFalseEverywhere: + """A connector whose verdict denies edit reports can_edit_global False + in the list, in the response from GET, and in the response from PUT + alike. + + ``member`` holds a personal, non-owner association row here (population + D: personal row + team link + denying verdict), not a stand-in: a + stand-in whose verdict denies edit is now refused outright by PUT (see + TestADenyingStandInIsRefusedRatherThanReportedSuccessful in + test_mcp_team_connector_edit.py), so it can no longer reach a + successful PUT response to assert can_edit_global on. Population D + still can -- can_edit_global is False by the same route (no personal + can_edit, no granting verdict) on all three surfaces, and its PUT + succeeds because it is writing its own association row, not the + verdict-gated shared config. + """ + + async def test_a_denying_verdict_yields_false_in_the_list_get_and_put_response( + self, db + ): + owner = _make_user(db, 50) + member = _make_user(db, 51) + server = _make_owned_server(db, owner.id, name="denied-everywhere") + server_id = server.id + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=_fixed_answer_hook( + ConnectorAccess(team_owned=True, can_edit=False) + ), + visibility=lambda _db, _uid: {"mcp": {server_id}, "custom_api": set()}, + ) + list_entries = get_mcp_servers(current_user=member, db=db) + list_entry = next(r for r in list_entries if r.id == server_id) + get_response = get_mcp_server(server_id, current_user=member, db=db) + put_response = update_mcp_server( + server_id, MCPServerUpdate(), current_user=member, db=db + ) + + assert list_entry.can_edit_global is False + assert get_response.can_edit_global is False + assert put_response.can_edit_global is False + + +class TestRenameStaysScopedToItsOwnConnector: + """Renaming one connector must not reach outside the connector actually + being renamed.""" + + def test_renaming_one_connector_does_not_touch_an_outsiders_own_connector(self, db): + """A narrower, database-level regression guard, kept alongside the + selector oracle below because it pins a different failure mode: a + stray write to the wrong MCPServer row entirely. Passing this + alone does not prove the rename call is scoped correctly against + an outsider who links the *same* connector being renamed -- that + is what the second test in this class checks.""" + owner_a = _make_user(db, 60) + editor = _make_user(db, 61) + outsider = _make_user(db, 62) + + server_a = _make_owned_server(db, owner_a.id, name="rename-target") + server_b = _make_owned_server(db, outsider.id, name="outsiders-own-connector") + server_a_id, server_b_id = server_a.id, server_b.id + + renamed_calls: list[tuple[int, str, str]] = [] + + def spy_renamed_hook(_db, _user_id, _connector_type, connector_id, old, new): + renamed_calls.append((connector_id, old, new)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=_fixed_answer_hook( + ConnectorAccess(team_owned=True, can_edit=True) + ), + renamed=spy_renamed_hook, + ) + update_mcp_server( + server_a_id, + MCPServerUpdate(name="renamed-target"), + current_user=editor, + db=db, + ) + + assert renamed_calls == [(server_a_id, "rename-target", "renamed-target")] + + db.rollback() + outsiders_server = db.query(MCPServer).filter(MCPServer.id == server_b_id).one() + assert outsiders_server.name == "outsiders-own-connector" + + def test_renaming_a_connector_does_not_rewrite_an_outsiders_own_agent_selectors( + self, db + ): + """The rename call itself installs no selector fan-out of its own: + rewriting a stored name-based selector is entirely the installed + renamed-hook's job (not exercised here at all -- no ``renamed`` + hook is installed), never something the core rename call does on + its own reach. An outsider who also links the exact connector + being renamed, and whose own agent selects it by name in + ``tool_categories``, must see that selector completely untouched + by the call. Constructing that second association and reading + back ``tool_categories`` is the point: a test that only checks an + unrelated connector's own row (the test above) would stay green + even if this call directly rewrote every agent's selectors on its + own, because it never looks at an agent at all.""" + owner = _make_user(db, 63) + editor = _make_user(db, 64) + outsider = _make_user(db, 65) + + server = _make_owned_server(db, owner.id, name="rename-target-selected") + server_id = server.id + + # The second association: the outsider also personally links this + # exact connector, on a verdict that passes -- not the separate, + # unrelated connector the test above uses. + db.add( + UserMCPServer( + user_id=outsider.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + outsiders_agent = Agent( + user_id=outsider.id, + name="outsiders-agent", + tool_categories=["rename-target-selected"], + ) + db.add(outsiders_agent) + db.commit() + agent_id = outsiders_agent.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=_fixed_answer_hook( + ConnectorAccess(team_owned=True, can_edit=True) + ), + ) + update_mcp_server( + server_id, + MCPServerUpdate(name="renamed-target-selected"), + current_user=editor, + db=db, + ) + + db.rollback() + refreshed_agent = db.query(Agent).filter(Agent.id == agent_id).one() + assert refreshed_agent.tool_categories == ["rename-target-selected"] + + +class TestStandaloneParityWithNoHookInstalled: + """With no hook installed at all, every route touched by this work + behaves exactly as it did before any of it started -- across every one + of the design matrix's thirteen rows (design-v1.md section 32, I26), + for both populations standalone xagent can actually construct: A (the + connector's owner) and B (a caller with a personal, non-owner link + row -- legacy per-connector sharing that predates team editing). A + third population, a complete stranger with neither row nor link, + exists in the matrix's constructible-population footnote too and is + covered separately below. + + Two additional route legs this same work touched but that fall + outside the thirteen-row matrix -- ``/api/mcp/apps``'s + ``can_configure`` and ``connect_mcp_app``'s ``can_edit_global`` -- are + pinned for both populations at the end of this class, so this + module's own docstring claim ("every route touched by this work") is + backed by actual coverage rather than just asserted. + + Row numbering below matches the design doc's matrix exactly (13 rows): + 1/2 GET /servers list (presence, can_edit_global), 3 GET /servers/{id}, + 4 PUT changing a global field, 5 PUT resubmitting a global field's + current value unchanged, 6 PUT touching only a personal field, 7 PUT + touching both at once, 8 DELETE /servers/{id}, 9 POST .../toggle, + 10 GET /custom-apis/{id}, 11 PUT any editable Custom API field, + 12 PUT only Custom API's is_active, 13 DELETE /custom-apis/{id}. + """ + + @pytest.mark.parametrize( + "population", ["owner", "personal_non_owner"], ids=["A=owner", "B=personal"] + ) + async def test_the_matrix_rows_match_pre_change_behavior_with_no_hook( + self, db, population + ): + owner = _make_user(db, 700) + member = _make_user(db, 701) + caller = owner if population == "owner" else member + + server = _make_owned_server(db, owner.id, name=f"parity-mcp-{population}") + server_id = server.id + api = _make_owned_api(db, owner.id, name=f"parity-api-{population}") + api_id = api.id + + if population == "personal_non_owner": + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.add( + UserCustomApi( + user_id=member.id, + custom_api_id=api_id, + is_owner=False, + can_edit=False, + is_active=True, + ) + ) + db.commit() + + # Whether an *actual global-config change* (rows 4 and 7) is + # expected to succeed for this population -- owner always can, + # a personal-but-non-owner caller never can with no hook and thus + # no team verdict. + can_edit_global_config = population == "owner" + + with snapshot_connector_team_hooks(): + set_connector_team_hooks() # explicit reset: no hooks installed + + # Rows 1-2: GET /servers list -- presence and can_edit_global, + # for BOTH connector kinds. The aggregate listing projects + # Custom API rows through _custom_api_to_mcp_response, which + # this work also changed; asserting only the MCP row would + # leave that projection unpinned. Both rows are selected by + # (id, transport): the two kinds live in separate tables and + # their ids collide freely. + list_entries = get_mcp_servers(current_user=caller, db=db) + mcp_entry = next( + r + for r in list_entries + if r.id == server_id and r.transport != "custom_api" + ) + assert mcp_entry.can_edit_global is can_edit_global_config + api_list_entry = next( + r + for r in list_entries + if r.id == api_id and r.transport == "custom_api" + ) + assert api_list_entry.can_edit_global is can_edit_global_config + + # Row 3: GET /servers/{id}. + get_response = get_mcp_server(server_id, current_user=caller, db=db) + assert get_response.can_edit_global is can_edit_global_config + + # Row 4: PUT changing a global field (description). + if can_edit_global_config: + put_response = update_mcp_server( + server_id, + MCPServerUpdate(description="row4-changed"), + current_user=caller, + db=db, + ) + assert put_response.can_edit_global is True + current_description = "row4-changed" + else: + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(description="row4-attempted"), + current_user=caller, + db=db, + ) + assert exc.value.status_code == 403 + current_description = None # unchanged from creation (None) + + # Row 5: PUT resubmitting a global field's *current* value -- + # not an actual change, so it must succeed regardless of edit + # rights (the tamper check compares against the stored value). + row5_response = update_mcp_server( + server_id, + MCPServerUpdate(description=current_description), + current_user=caller, + db=db, + ) + assert row5_response.can_edit_global is can_edit_global_config + + # Row 6: PUT touching only a personal field (is_active) -- + # always allowed for a caller with a personal row, independent + # of global edit rights. + row6_response = update_mcp_server( + server_id, + MCPServerUpdate(is_active=False), + current_user=caller, + db=db, + ) + assert row6_response.can_edit_global is can_edit_global_config + assert row6_response.is_active is False + + # Row 7: PUT touching a global field and a personal field at + # the same time -- the global half decides the outcome. + if can_edit_global_config: + row7_response = update_mcp_server( + server_id, + MCPServerUpdate(description="row7-changed", is_active=True), + current_user=caller, + db=db, + ) + assert row7_response.can_edit_global is True + assert row7_response.is_active is True + else: + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(description="row7-attempted", is_active=True), + current_user=caller, + db=db, + ) + assert exc.value.status_code == 403 + + # Row 9: POST .../toggle -- gated on a personal row's mere + # existence, not on edit rights; both populations have one. + toggle_response = toggle_mcp_server(server_id, current_user=caller, db=db) + assert toggle_response.can_edit_global is can_edit_global_config + + # Row 10: GET /custom-apis/{id} -- never reads a verdict for a + # caller who already has a working personal row, of either + # population, so this always succeeds. + api_get_response = get_custom_api(api_id, current_user=caller, db=db) + assert api_get_response.id == api_id + + # Row 11: PUT any editable Custom API field -- gated on + # user_api.can_edit OR the team verdict; with no hook and no + # can_edit on a non-owner's row, this is 403 for population B. + if population == "owner": + api_put_response = update_custom_api( + api_id, + CustomApiUpdate(description="row11-changed"), + current_user=caller, + db=db, + ) + assert api_put_response.id == api_id + else: + with pytest.raises(HTTPException) as exc: + update_custom_api( + api_id, + CustomApiUpdate(description="row11-attempted"), + current_user=caller, + db=db, + ) + assert exc.value.status_code == 403 + + # Row 12: PUT only Custom API's is_active -- Custom API has no + # personal-field carve-out the way MCP's PUT does (row 6): + # the can_edit gate fires before the is_active-only check is + # ever reached, so this is 403 for population B too, not 200. + if population == "owner": + api_row12_response = update_custom_api( + api_id, + CustomApiUpdate(is_active=False), + current_user=caller, + db=db, + ) + assert api_row12_response.is_active is False + else: + with pytest.raises(HTTPException) as exc: + update_custom_api( + api_id, + CustomApiUpdate(is_active=False), + current_user=caller, + db=db, + ) + assert exc.value.status_code == 403 + + # Row 8: DELETE /servers/{id} -- last, since it consumes the + # row. Gated on is_owner OR can_delete; population B has + # neither. + if population == "owner": + await delete_mcp_server(server_id, current_user=caller, db=db) + else: + with pytest.raises(HTTPException) as exc: + await delete_mcp_server(server_id, current_user=caller, db=db) + assert exc.value.status_code == 403 + + # Row 13: DELETE /custom-apis/{id} -- last, same reasoning. + if population == "owner": + delete_custom_api(api_id, current_user=caller, db=db) + else: + with pytest.raises(HTTPException) as exc: + delete_custom_api(api_id, current_user=caller, db=db) + assert exc.value.status_code == 403 + + async def test_a_complete_stranger_still_gets_404_everywhere_with_no_hook(self, db): + """A caller with neither a personal row nor any team link is not one + of the matrix's two constructible populations (design-v1.md's J + column: standalone can only construct A and B), but the pre-change + 404 behavior for this case is worth keeping pinned too -- it is + what the matrix's population footnote is drawing the line against.""" + owner = _make_user(db, 702) + stranger = _make_user(db, 703) + server = _make_owned_server(db, owner.id, name="parity-stranger-mcp") + server_id = server.id + api = _make_owned_api(db, owner.id, name="parity-stranger-api") + api_id = api.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks() + + with pytest.raises(HTTPException) as exc: + get_mcp_server(server_id, current_user=stranger, db=db) + assert exc.value.status_code == 404 + + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(description="x"), + current_user=stranger, + db=db, + ) + assert exc.value.status_code == 404 + + with pytest.raises(HTTPException) as exc: + get_custom_api(api_id, current_user=stranger, db=db) + assert exc.value.status_code == 404 + + with pytest.raises(HTTPException) as exc: + update_custom_api( + api_id, + CustomApiUpdate(description="x"), + current_user=stranger, + db=db, + ) + assert exc.value.status_code == 404 + + @pytest.mark.parametrize( + "population", ["owner", "personal_non_owner"], ids=["A=owner", "B=personal"] + ) + async def test_the_apps_listing_can_configure_matches_pre_change_behavior( + self, db, population + ): + """Outside the thirteen-row matrix but touched by this same work: + ``/api/mcp/apps``'s ``can_configure`` reads only whether a personal + association row exists (or, absent one, a team verdict) -- both + constructible populations have a personal row, so both see True, + with no hook installed, for both connector kinds.""" + owner = _make_user(db, 704) + member = _make_user(db, 705) + caller = owner if population == "owner" else member + + server = _make_owned_server(db, owner.id, name=f"parity-apps-mcp-{population}") + server_id = server.id + api = _make_owned_api(db, owner.id, name=f"parity-apps-api-{population}") + api_id = api.id + + if population == "personal_non_owner": + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.add( + UserCustomApi( + user_id=member.id, + custom_api_id=api_id, + is_owner=False, + can_edit=False, + is_active=True, + ) + ) + db.commit() + + with snapshot_connector_team_hooks(): + set_connector_team_hooks() + entries = list_mcp_apps(location="local", current_user=caller, db=db) + + mcp_entry = next( + e + for e in entries + if e["server_id"] == server_id and e["transport"] != "custom_api" + ) + assert mcp_entry["can_configure"] is True + api_entry = next( + e + for e in entries + if e["server_id"] == api_id and e["transport"] == "custom_api" + ) + assert api_entry["can_configure"] is True + + @pytest.mark.parametrize( + "population", ["owner", "personal_non_owner"], ids=["A=owner", "B=personal"] + ) + async def test_connecting_an_app_can_edit_global_matches_pre_change_behavior( + self, db, population + ): + """Outside the thirteen-row matrix but touched by this same work: + connecting to a catalog app always creates a fresh, non-owning + association (``is_owner=False``), so ``can_edit_global`` is False + regardless of which population is doing the connecting -- pinned + for both, with no hook installed, so a future change that makes + this population-dependent would be caught.""" + owner = _make_user(db, 706) + member = _make_user(db, 707) + caller = owner if population == "owner" else member + _seed_catalog_app(db, f"parity-connect-app-{population}") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks() + response = connect_mcp_app( + f"parity-connect-app-{population}", + MCPAppConnectRequest(), + current_user=caller, + db=db, + ) + + assert response.can_edit_global is False + + +class TestListMcpAppsPerRowDegradation: + """``/api/mcp/apps``'s local-connector loop now resolves every stand-in + row's verdict, across both connector kinds, with one batched call -- + consolidated from the one-hook-call-per-row shape this route used to + have. A ref missing from an otherwise-successful answer still degrades + only that one row's ``can_configure`` to False, the same per-row + degradation this route has always offered -- now expressed by the + batch answer omitting a ref rather than a per-row hook call raising. A + hook that fails for the whole batch call degrades every row that + needed a verdict, but the response itself stays 200 with every row + present -- the failure never blanks the list.""" + + def test_an_answer_that_omits_one_connector_degrades_only_that_row(self, db): + owner = _make_user(db, 80) + member = _make_user(db, 81) + healthy_mcp = _make_owned_server(db, owner.id, name="healthy-connector") + omitted_mcp = _make_owned_server(db, owner.id, name="omitted-connector") + healthy_api = _make_owned_api(db, owner.id, name="healthy-api") + omitted_api = _make_owned_api(db, owner.id, name="omitted-api") + healthy_mcp_id, omitted_mcp_id = healthy_mcp.id, omitted_mcp.id + healthy_api_id, omitted_api_id = healthy_api.id, omitted_api.id + + def partial_access(_db, _user_id, refs): + # A legitimate "not linked" answer for the two omitted refs, + # not a failure -- distinct from the whole-batch failure the + # next test exercises. + omitted = {("mcp", omitted_mcp_id), ("custom_api", omitted_api_id)} + return { + ref: ConnectorAccess(team_owned=True, can_edit=True) + for ref in refs + if ref not in omitted + } + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=partial_access, + visibility=lambda _db, _uid: { + "mcp": {healthy_mcp_id, omitted_mcp_id}, + "custom_api": {healthy_api_id, omitted_api_id}, + }, + ) + entries = list_mcp_apps(location="local", current_user=member, db=db) + + healthy_mcp_entry = next(e for e in entries if e["server_id"] == healthy_mcp_id) + omitted_mcp_entry = next(e for e in entries if e["server_id"] == omitted_mcp_id) + healthy_api_entry = next( + e + for e in entries + if e["server_id"] == healthy_api_id and e["transport"] == "custom_api" + ) + omitted_api_entry = next( + e + for e in entries + if e["server_id"] == omitted_api_id and e["transport"] == "custom_api" + ) + assert healthy_mcp_entry["can_configure"] is True + assert omitted_mcp_entry["can_configure"] is False + assert healthy_api_entry["can_configure"] is True + assert omitted_api_entry["can_configure"] is False + + def test_a_failing_hook_does_not_blank_the_whole_apps_list(self, db): + owner = _make_user(db, 82) + member = _make_user(db, 83) + mcp_row = _make_owned_server(db, owner.id, name="stand-in-mcp") + api_row = _make_owned_api(db, owner.id, name="stand-in-api") + mcp_id, api_id = mcp_row.id, api_row.id + + def raising_access(_db, _user_id, _refs): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=raising_access, + visibility=lambda _db, _uid: { + "mcp": {mcp_id}, + "custom_api": {api_id}, + }, + ) + entries = list_mcp_apps(location="local", current_user=member, db=db) + + mcp_entry = next(e for e in entries if e["server_id"] == mcp_id) + api_entry = next( + e + for e in entries + if e["server_id"] == api_id and e["transport"] == "custom_api" + ) + assert mcp_entry["can_configure"] is False + assert api_entry["can_configure"] is False + + +def poison_by_raw_statement(db, *, colliding_user_id=None): + """Poison the session with a raw statement that fails outright. + + On PostgreSQL this aborts the surrounding transaction, so every later + statement on the same connection is refused until a rollback. On + SQLite, a failed Core-level statement like this one does not put the + ORM ``Session`` into a deactivated state the way a failed flush does + (see ``poison_by_orm_flush``) -- so this shape's recovery proof lives + in the PostgreSQL-only sibling suite + (test_connector_hook_session_fault_postgresql.py), not in the tests + that use this factory here. ``colliding_user_id`` is accepted and + ignored so both poison factories share one call signature. + """ + del colliding_user_id + db.execute(sa.text("select * from no_such_table_at_all")) + + +def poison_by_orm_flush(db, *, colliding_user_id): + """Poison the session by flushing a row that violates a real unique + constraint -- unlike ``poison_by_raw_statement``, this poisons the + ORM ``Session`` itself (not only the underlying DB transaction) on + every backend: SQLAlchemy marks the session's transaction inactive + after a failed flush, and any later operation on it raises + ``PendingRollbackError`` until a rollback runs. + """ + db.add(User(id=colliding_user_id, username="flush-poison-dup", password_hash="x")) + db.flush() + + +POISON_SHAPES = [poison_by_raw_statement, poison_by_orm_flush] +POISON_SHAPE_IDS = ["raw-statement", "orm-flush"] + + +def _seed_catalog_app(db, app_id: str = "session-fault-app") -> None: + db.add( + PublicMCPApp( + app_id=app_id, + name=app_id, + description="Session fault test app", + transport="stdio", + launch_config={"command": "npx", "args": ["-y", app_id]}, + ) + ) + db.commit() + + +class TestSessionRecoveryAfterHookFailure: + """A hook that leaves a failed statement on the shared session must not + turn a route that would otherwise succeed (or gracefully degrade) into + a 500 -- the seam's single hook-invocation door restores the session + before the failure ever reaches a caller to convert into a typed error + (see ``_call_connector_hook_gate`` in connector_team_scope.py). + + ``poison_by_raw_statement`` only actually poisons PostgreSQL (see its + docstring); it is still parametrized here so the SQLite half of this + file documents that shape's expected (correct, unaffected) behavior + too. The PostgreSQL-only proof that this shape needs the fix lives in + test_connector_hook_session_fault_postgresql.py. + """ + + @pytest.mark.parametrize("poison", POISON_SHAPES, ids=POISON_SHAPE_IDS) + def test_a_toggle_that_already_committed_still_returns_200_when_the_hook_poisons_the_session( + self, db, poison + ): + owner = _make_user(db, 90) + server = _make_owned_server(db, owner.id, name="toggle-poison-target") + server_id = server.id + owner_id = owner.id + + def poisoning_access(_db, _user_id, _refs): + poison(_db, colliding_user_id=owner_id) + return {} + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=poisoning_access) + response = toggle_mcp_server(server_id, current_user=owner, db=db) + + assert response.can_edit_global is True + + # No rollback here on purpose: the query below is the statement + # that proves the seam's hook door restored this session, not just + # an incidental fresh read (see the same note in + # test_connector_hook_session_fault_postgresql.py, where the + # orm-flush shape poisons on every backend the same way). + refreshed = ( + db.query(UserMCPServer) + .filter( + UserMCPServer.user_id == owner_id, + UserMCPServer.mcpserver_id == server_id, + ) + .one() + ) + # The connector was created active; toggling it once must have + # flipped it to inactive, and that flip must have durably + # committed (it happens before the hook is ever consulted) even + # though the hook poisoned the session afterward. + assert refreshed.is_active is False + + @pytest.mark.parametrize("poison", POISON_SHAPES, ids=POISON_SHAPE_IDS) + def test_connecting_an_app_still_returns_200_when_the_hook_poisons_the_session( + self, db, poison + ): + member = _make_user(db, 91) + member_id = member.id + _seed_catalog_app(db, "connect-poison-app") + + def poisoning_access(_db, _user_id, _refs): + poison(_db, colliding_user_id=member_id) + return {} + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=poisoning_access) + response = connect_mcp_app( + "connect-poison-app", + MCPAppConnectRequest(), + current_user=member, + db=db, + ) + + # Connecting never grants ownership (a fresh association is always + # is_owner=False), so with the hook degraded to no verdict at all, + # can_edit_global is False here -- the same value this route + # always reported before any verdict existed. + assert response.can_edit_global is False + + # No rollback here on purpose -- see the same note in the toggle + # test above. + assoc = ( + db.query(UserMCPServer) + .join(MCPServer, UserMCPServer.mcpserver_id == MCPServer.id) + .filter( + UserMCPServer.user_id == member_id, + MCPServer.name == "connect-poison-app", + ) + .one() + ) + # ``.one()`` already raises when the row is absent, so asserting it is + # not None asserts nothing. What this test is actually about is that + # the association survived the poisoned session with the shape connect + # writes: a non-owning, active personal link. + assert (assoc.is_owner, assoc.is_active) == (False, True) + + # test_the_servers_listing_still_returns_every_row_when_the_hook_poisons_the_session + # is not here: it lives in TestListMcpServersPerRowDegradation below, + # next to /api/mcp/servers's own per-request degradation catch -- + # that catch did not exist yet at the point this class was written, + # so the poison test could not have asserted a guarantee this route + # did not yet provide. + + def test_the_apps_listing_still_returns_every_row_when_the_hook_poisons_the_session( + self, db + ): + owner = _make_user(db, 94) + member = _make_user(db, 95) + member_id = member.id + server = _make_owned_server(db, owner.id, name="apps-list-poison-target") + server_id = server.id + + def poisoning_access(_db, _user_id, _refs): + poison_by_orm_flush(_db, colliding_user_id=member_id) + return {} + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=poisoning_access, + visibility=lambda _db, _uid: {"mcp": {server_id}, "custom_api": set()}, + ) + entries = list_mcp_apps(location="local", current_user=member, db=db) + + entry = next(e for e in entries if e["server_id"] == server_id) + assert entry["can_configure"] is False + + def test_a_typed_error_raised_by_the_hook_itself_also_restores_the_session( + self, db + ): + """The ``except ConnectorRuntimeError: raise`` arm must restore the + session too -- a hook can poison the session and *then* raise its + own typed error, not only a bare exception.""" + owner = _make_user(db, 96) + member = _make_user(db, 97) + member_id = member.id + server = _make_owned_server(db, owner.id, name="typed-error-poison-target") + server_id = server.id + + def poisoning_typed_hook(_db, _user_id, _refs): + try: + poison_by_orm_flush(_db, colliding_user_id=member_id) + except Exception: + pass + raise ConnectorRuntimeError("planted", "planted failure", status_code=409) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=poisoning_typed_hook, + visibility=lambda _db, _uid: {"mcp": {server_id}, "custom_api": set()}, + ) + with pytest.raises(HTTPException) as exc: + get_mcp_server(server_id, current_user=member, db=db) + assert exc.value.status_code == 409 + + # The session must be usable again immediately afterward -- + # not just after an explicit external rollback. + still_works = db.query(MCPServer).filter(MCPServer.id == server_id).first() + assert still_works is not None + + +class TestListMcpServersPerRowDegradation: + """``/api/mcp/servers``'s response loops resolve every row that still + needs a verdict -- a non-owner personal row, or a stand-in row with no + personal row at all -- with one batched call (see the shape built in + get_mcp_servers). A ref missing from an otherwise-successful answer + degrades only that one row's ``can_edit_global`` to False, the same + per-row degradation this route has always offered. A hook that fails + for the whole batch call degrades every row that needed a verdict, but + the response itself stays 200 with every row present -- the failure + never blanks the list. Mirrors ``TestListMcpAppsPerRowDegradation`` + above for the sister listing endpoint.""" + + def test_an_answer_that_omits_one_connector_degrades_only_that_row(self, db): + owner = _make_user(db, 84) + member = _make_user(db, 85) + healthy = _make_owned_server(db, owner.id, name="servers-healthy") + omitted = _make_owned_server(db, owner.id, name="servers-omitted") + healthy_id, omitted_id = healthy.id, omitted.id + + def partial_access(_db, _user_id, refs): + # A legitimate "not linked" answer for the omitted ref, not a + # failure -- distinct from the whole-batch failure the next + # test exercises. + skip = {("mcp", omitted_id)} + return { + ref: ConnectorAccess(team_owned=True, can_edit=True) + for ref in refs + if ref not in skip + } + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=partial_access, + visibility=lambda _db, _uid: { + "mcp": {healthy_id, omitted_id}, + "custom_api": set(), + }, + ) + entries = get_mcp_servers(current_user=member, db=db) + + healthy_entry = next(e for e in entries if e.id == healthy_id) + omitted_entry = next(e for e in entries if e.id == omitted_id) + assert healthy_entry.can_edit_global is True + assert omitted_entry.can_edit_global is False + + def test_a_failing_hook_does_not_blank_the_whole_servers_list(self, db): + owner = _make_user(db, 86) + member = _make_user(db, 87) + owned_by_member = _make_owned_server(db, member.id, name="servers-member-owned") + personal_non_owner = _make_owned_server( + db, owner.id, name="servers-personal-non-owner" + ) + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=personal_non_owner.id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + stand_in = _make_owned_server(db, owner.id, name="servers-stand-in") + + def raising_access(_db, _user_id, _refs): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=raising_access, + visibility=lambda _db, _uid: { + "mcp": {stand_in.id}, + "custom_api": set(), + }, + ) + entries = get_mcp_servers(current_user=member, db=db) + + assert {e.id for e in entries} == { + owned_by_member.id, + personal_non_owner.id, + stand_in.id, + } + owned_entry = next(e for e in entries if e.id == owned_by_member.id) + personal_entry = next(e for e in entries if e.id == personal_non_owner.id) + stand_in_entry = next(e for e in entries if e.id == stand_in.id) + # The owner's own row never needed a verdict at all -- the edit + # branch returns True on is_owner alone, so a failed batch call + # cannot touch it. + assert owned_entry.can_edit_global is True + assert personal_entry.can_edit_global is False + assert stand_in_entry.can_edit_global is False + + def test_the_servers_listing_still_returns_every_row_when_the_hook_poisons_the_session( + self, db + ): + """Sibling to the SQLite-side poison tests in + ``TestSessionRecoveryAfterHookFailure`` above -- deferred to this + class specifically because ``/api/mcp/servers`` had no per-request + degradation catch of its own until this same revision added one; + before that, a poisoned session on this route would have failed + the whole request regardless of any session-recovery fix.""" + owner = _make_user(db, 88) + member = _make_user(db, 89) + member_id = member.id + server = _make_owned_server(db, owner.id, name="servers-list-poison-target") + server_id = server.id + + def poisoning_access(_db, _user_id, _refs): + poison_by_orm_flush(_db, colliding_user_id=member_id) + return {} + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=poisoning_access, + visibility=lambda _db, _uid: { + "mcp": {server_id}, + "custom_api": set(), + }, + ) + entries = get_mcp_servers(current_user=member, db=db) + + entry = next(e for e in entries if e.id == server_id) + assert entry.can_edit_global is False + + +class TestSingleServerAccessResolutionFailure: + """A single MCP server's verdict plays two different roles depending on + the route: ``GET`` uses it as decoration on a row the caller can + already read (a personal row, or a team gate that already passed), so + a resolution failure there degrades ``can_edit_global`` to False and + the read still succeeds. ``PUT`` uses the same verdict as the gate + itself for a non-owner caller, so a resolution failure there must + still fail closed with a typed 503 -- never a silent 200 or a 404 that + would misreport "does not exist" for a connector the caller merely + could not be asked about.""" + + def test_reading_one_server_survives_a_failing_hook_when_a_personal_row_exists( + self, db + ): + owner = _make_user(db, 102) + member = _make_user(db, 103) + server = _make_owned_server(db, owner.id, name="read-degrade-target") + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server.id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + server_id = server.id + + def raising_access(_db, _user_id, _refs): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=raising_access) + response = get_mcp_server(server_id, current_user=member, db=db) + + assert response.can_edit_global is False + + def test_reading_one_server_still_fails_closed_without_a_personal_row(self, db): + owner = _make_user(db, 104) + member = _make_user(db, 105) + server = _make_owned_server(db, owner.id, name="read-gate-target") + server_id = server.id + + def raising_access(_db, _user_id, _refs): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=raising_access, + visibility=lambda _db, _uid: { + "mcp": {server_id}, + "custom_api": set(), + }, + ) + with pytest.raises(HTTPException) as exc: + get_mcp_server(server_id, current_user=member, db=db) + + # Must be 503 (typed, fail-closed) -- specifically not 404 + # (which would misreport "does not exist" for a connector the + # team's own visibility hook just said this caller can see) and + # not 200 (which would be the door itself failing open). + assert exc.value.status_code == 503 + + def test_updating_one_server_still_fails_closed_on_a_personal_only_payload( + self, db + ): + owner = _make_user(db, 106) + member = _make_user(db, 107) + server = _make_owned_server(db, owner.id, name="write-gate-target") + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server.id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + server_id = server.id + + def raising_access(_db, _user_id, _refs): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=raising_access) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(is_active=False), + current_user=member, + db=db, + ) + + # PUT never degrades, even for a payload that only touches the + # caller's own personal fields: the verdict is the gate that + # decides whether this caller may write at all. + assert exc.value.status_code == 503 + + +class TestAdminInspectingAnotherUsersListReportsPerKindSubject: + """``GET /api/mcp/servers?user_id=`` reports ``can_edit_global`` + from a different subject depending on connector kind, today: an MCP + row blends the *acting admin's own* bypass with the target's team + verdict (``_check_mcp_permission``'s ``is_admin`` short-circuit runs + before any verdict is even consulted), while a Custom API row reports + purely the *target's own* ``can_edit`` and team verdict, since Custom + API's write gate has no admin bypass at all + (``_custom_api_to_mcp_response`` never reads ``is_admin``). + + This pins the subject mix as it exists today -- it is not an + endorsement of it. "Whose capability should this field describe" is + an undecided product rule, tracked in xorbitsai/xagent#1703. This + test is a regression guard against either subject silently changing, + not a statement that the current split is correct. + """ + + async def test_admin_inspecting_another_users_list_reports_each_kind_from_its_own_subject( + self, db + ): + admin = _make_user(db, 800, is_admin=True) + target = _make_user(db, 801) + other_owner = _make_user(db, 802) + + server = _make_owned_server(db, other_owner.id, name="admin-subject-mcp") + server_id = server.id + db.add( + UserMCPServer( + user_id=target.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + api = _make_owned_api(db, target.id, name="admin-subject-api") + api_id = api.id + # A second Custom API the target can see but genuinely cannot + # edit -- a non-owning personal row, with the hook denying the + # target's own verdict on it too. Distinct from `api` above: + # `api`'s True could in principle come from an admin bypass this + # module does not have rather than from the target's own + # can_edit, and the two would be indistinguishable there (True or + # True is still True). This row is the one that actually proves + # the subject is the target and not the admin -- if a bypass on + # is_admin were ever added to Custom API's response builder, the + # admin's own True would leak into this row and flip it. + other_owner_api = _make_owned_api( + db, other_owner.id, name="admin-subject-denied-api" + ) + denied_api_id = other_owner_api.id + db.add( + UserCustomApi( + user_id=target.id, + custom_api_id=denied_api_id, + is_owner=False, + can_edit=False, + is_active=True, + ) + ) + db.commit() + + # Denies the target's own verdict on every ref -- but, deliberately, + # *grants* anyone else's, including the admin's own id. A correct + # list implementation always asks about the target being + # inspected, regardless of who is doing the viewing, so this + # granting branch should never be reached for this list call. A + # mutation that asked about the *viewer's* id instead of the + # target's would reach it and leak a wrong grant into a + # target-subject row -- this is what makes that class of bug + # visible rather than merely restating "the target is denied". + def access_hook_keyed_on_who_is_asked_about(_db, user_id, refs): + if user_id == target.id: + return { + ref: ConnectorAccess(team_owned=True, can_edit=False) + for ref in refs + } + return { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=access_hook_keyed_on_who_is_asked_about) + list_entries = get_mcp_servers(user_id=target.id, current_user=admin, db=db) + + mcp_entry = next(r for r in list_entries if r.id == server_id) + api_entry = next( + r for r in list_entries if r.id == api_id and r.transport == "custom_api" + ) + denied_api_entry = next( + r + for r in list_entries + if r.id == denied_api_id and r.transport == "custom_api" + ) + + # The MCP row's subject is the acting admin: True here, even + # though the target's own verdict (fetched for the target, not + # the admin) denies edit -- the admin bypass wins before any + # verdict is consulted. + assert mcp_entry.can_edit_global is True + + # The Custom API row's subject is the target: True because the + # target owns this API outright (can_edit=True on their own row), + # independent of the acting admin's identity or the denying + # verdict above -- if this test's admin were somehow the subject + # here too, this would need to be False (the verdict denies it). + assert api_entry.can_edit_global is True + + # This row is the one that actually distinguishes "target" from + # "admin" as the subject: the target's own verdict on it is + # denied and they do not own it, so it must be False despite the + # acting caller being an admin. An admin bypass leaking into + # Custom API's response builder would flip this to True. + assert denied_api_entry.can_edit_global is False + + # The list said the Custom API row is editable, but that value + # describes the target, not the caller -- acting as themselves, + # the admin has no personal row and no team link to this API at + # all, so a real write attempt 404s despite what the list just + # reported. A plain deny-everyone hook here (not the + # asker-dependent one above): this block is about the admin's own + # resolution outcome, not about which id a call asks about. + def access_hook_denies_everyone(_db, _user_id, _refs): + return {} + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=access_hook_denies_everyone) + with pytest.raises(HTTPException) as exc: + update_custom_api( + api_id, + CustomApiUpdate(description="admin-attempted-edit"), + current_user=admin, + db=db, + ) + assert exc.value.status_code == 404 diff --git a/tests/web/api/test_mcp_server_edit_lock_postgresql.py b/tests/web/api/test_mcp_server_edit_lock_postgresql.py new file mode 100644 index 0000000000..b2d1004b8d --- /dev/null +++ b/tests/web/api/test_mcp_server_edit_lock_postgresql.py @@ -0,0 +1,305 @@ +"""Real-PostgreSQL coverage for the row lock ``update_mcp_server`` takes on +the ``MCPServer`` definition row before building the new config. + +``FOR UPDATE`` is a no-op on SQLite -- every other suite in this repo runs +against SQLite, so nothing there can tell a genuine second-writer block +from a lock statement that silently does nothing. This file is the one +place that runs the real statement against a real server and proves it +actually blocks a second writer, plus the companion path where the row +vanishes between the route's first read and this lock. + +Obtains its database through ``tests/shared/postgres_disposable.py`` +(``disposable_database_factory``), the same disposable-CREATE-DATABASE +helper the other ``*_postgresql.py`` suites in this repo use, rather than +opening a hand-rolled connection. That helper reads +``XAGENT_TEST_POSTGRES_URL`` and skips the whole module when it is unset. +""" + +from __future__ import annotations + +import threading +from concurrent.futures import ThreadPoolExecutor +from types import SimpleNamespace + +import pytest +import sqlalchemy as sa +from fastapi import HTTPException +from sqlalchemy.orm import sessionmaker + +from tests.shared.postgres_disposable import disposable_database_factory +from xagent.web.models.database import Base +from xagent.web.models.mcp import MCPServer, UserMCPServer +from xagent.web.models.user import User +from xagent.web.services.connector_team_scope import ( + set_connector_team_hooks, + snapshot_connector_team_hooks, +) + +pytestmark = pytest.mark.postgresql + + +@pytest.fixture() +def session_factory(): + with disposable_database_factory("xagent_mcp_edit_lock") as make_database: + engine = make_database("edit_lock") + Base.metadata.create_all(bind=engine) + yield sessionmaker(autocommit=False, autoflush=False, bind=engine) + + +@pytest.fixture() +def seeded(session_factory): + """One owner, one owned MCP server, in their own committed rows.""" + with session_factory() as db: + owner = User(username="mcp-edit-lock-owner", password_hash="x", is_admin=False) + db.add(owner) + db.flush() + server = MCPServer( + name="edit-lock-target", + transport="stdio", + managed="external", + command="true", + ) + db.add(server) + db.flush() + db.add( + UserMCPServer( + user_id=int(owner.id), + mcpserver_id=int(server.id), + is_owner=True, + is_active=True, + ) + ) + db.commit() + return int(owner.id), int(server.id) + + +def test_a_second_editor_blocks_until_the_first_editors_transaction_finishes( + session_factory, seeded +) -> None: + """Two real connections, barrier-synchronised: the second call's own + lock statement must not return until the first call's transaction + commits or rolls back -- the actual behavior ``FOR UPDATE`` exists to + provide, and the one thing no SQLite-backed test can demonstrate. + """ + import xagent.web.api.mcp as mcp_api + from xagent.web.api.mcp import MCPServerUpdate + + owner_id, server_id = seeded + current_user = SimpleNamespace(id=owner_id, is_admin=False) + + lock_acquired = threading.Event() + release_lock = threading.Event() + second_finished = threading.Event() + first_call_claimed = threading.Event() + first_call_lock = threading.Lock() + + real_build_server_config = mcp_api._build_server_config + + def paced_build_server_config(update_data, server): + # Both threads run through this same patched function once each + # gets past its own lock statement. Only the call that gets here + # *first* pauses: that is the first editor, holding its row lock + # open via this still-uncommitted transaction. A second call that + # reaches this point too (rather than staying blocked earlier, + # inside its own lock statement) is not made to wait a second + # time here -- pausing it too would prove nothing about the + # database lock, only about this Python-level barrier. + with first_call_lock: + is_first_call = not first_call_claimed.is_set() + first_call_claimed.set() + if is_first_call: + lock_acquired.set() + assert release_lock.wait(timeout=10), "the first editor was never released" + return real_build_server_config(update_data, server) + + mcp_api._build_server_config = paced_build_server_config + session_a = session_factory() + session_b = session_factory() + try: + + def run_first(): + return mcp_api.update_mcp_server( + server_id, + MCPServerUpdate(name="renamed-by-first-editor"), + current_user=current_user, + db=session_a, + ) + + def run_second(): + result = mcp_api.update_mcp_server( + server_id, + MCPServerUpdate(description="edited-by-second-editor"), + current_user=current_user, + db=session_b, + ) + second_finished.set() + return result + + with ThreadPoolExecutor(max_workers=2) as executor: + first = executor.submit(run_first) + assert lock_acquired.wait(timeout=5), ( + "the first editor never reached the lock" + ) + + second = executor.submit(run_second) + # The second call's own lock statement should still be blocked + # on the database at this point. If the lock were not real (or + # a no-op, as on SQLite), the second call would sail through + # almost immediately and this would flip to True. + assert not second_finished.wait(timeout=1.0), ( + "the second editor finished before the first one released " + "the row -- the lock did not actually block it" + ) + + release_lock.set() + first.result(timeout=10) + second.result(timeout=10) + + assert second_finished.is_set() + finally: + mcp_api._build_server_config = real_build_server_config + session_a.close() + session_b.close() + + +def test_the_second_editors_rename_reports_the_first_editors_committed_name_as_old( + session_factory, seeded +) -> None: + """``rename_team_connector``'s ``old`` argument must be the name this + transaction's own lock actually holds once acquired, not whatever the + pre-lock read saw. + + Interleaving under test: the first editor renames the connector and + commits while the second editor is blocked on the lock. The second + editor then acquires the lock, refreshed to the first editor's + committed name, and renames again. If the second editor's ``old`` + argument were captured before its own lock instead, it would report + the connector's *original* name -- not the name every team agent's + selector was already rewritten to by the first editor's own call -- + and the second rewrite would search for a name nothing holds anymore, + leaving the first rewrite's result permanently dangling with no error. + """ + import xagent.web.api.mcp as mcp_api + from xagent.web.api.mcp import MCPServerUpdate + + owner_id, server_id = seeded + current_user = SimpleNamespace(id=owner_id, is_admin=False) + + lock_acquired = threading.Event() + release_lock = threading.Event() + second_finished = threading.Event() + first_call_claimed = threading.Event() + first_call_lock = threading.Lock() + + renamed_calls: list[tuple[str, str]] = [] + renamed_calls_lock = threading.Lock() + + def spy_renamed_hook(_db, _user_id, _connector_type, _connector_id, old, new): + with renamed_calls_lock: + renamed_calls.append((old, new)) + + real_build_server_config = mcp_api._build_server_config + + def paced_build_server_config(update_data, server): + with first_call_lock: + is_first_call = not first_call_claimed.is_set() + first_call_claimed.set() + if is_first_call: + lock_acquired.set() + assert release_lock.wait(timeout=10), "the first editor was never released" + return real_build_server_config(update_data, server) + + mcp_api._build_server_config = paced_build_server_config + session_a = session_factory() + session_b = session_factory() + try: + with snapshot_connector_team_hooks(): + set_connector_team_hooks(renamed=spy_renamed_hook) + + def run_first(): + return mcp_api.update_mcp_server( + server_id, + MCPServerUpdate(name="renamed-by-first-editor"), + current_user=current_user, + db=session_a, + ) + + def run_second(): + result = mcp_api.update_mcp_server( + server_id, + MCPServerUpdate(name="renamed-by-second-editor"), + current_user=current_user, + db=session_b, + ) + second_finished.set() + return result + + with ThreadPoolExecutor(max_workers=2) as executor: + first = executor.submit(run_first) + assert lock_acquired.wait(timeout=5), ( + "the first editor never reached the lock" + ) + + second = executor.submit(run_second) + assert not second_finished.wait(timeout=1.0), ( + "the second editor finished before the first one released the row" + ) + + release_lock.set() + first.result(timeout=10) + second.result(timeout=10) + + assert renamed_calls == [ + ("edit-lock-target", "renamed-by-first-editor"), + ("renamed-by-first-editor", "renamed-by-second-editor"), + ] + finally: + mcp_api._build_server_config = real_build_server_config + session_a.close() + session_b.close() + + +def test_a_row_that_vanishes_after_the_gate_but_before_the_lock_is_a_404_not_a_500( + session_factory, seeded +) -> None: + """The gate helper's own read can find the row and still lose a race to + a concurrent delete that commits before this route's own lock + statement runs. The lock statement must see that as an ordinary + "row not found" (``None``) and let the route's existing 404 handle + it, not surface as an unrelated 500 out of the write path below. + """ + import xagent.web.api.mcp as mcp_api + from xagent.web.api.mcp import MCPServerUpdate + + owner_id, server_id = seeded + current_user = SimpleNamespace(id=owner_id, is_admin=False) + + real_resolve = mcp_api._resolve_mcp_server_for_request + + def resolve_then_delete_concurrently(db_, user_id, sid): + result = real_resolve(db_, user_id, sid) + # A concurrent delete that actually commits, from a separate + # connection, landing strictly between the gate helper's read + # above and the route's own lock statement below. + with session_factory() as other: + other.execute( + sa.delete(UserMCPServer).where(UserMCPServer.mcpserver_id == sid) + ) + other.execute(sa.delete(MCPServer).where(MCPServer.id == sid)) + other.commit() + return result + + mcp_api._resolve_mcp_server_for_request = resolve_then_delete_concurrently + db = session_factory() + try: + with pytest.raises(HTTPException) as exc: + mcp_api.update_mcp_server( + server_id, + MCPServerUpdate(name="renamed-after-vanish"), + current_user=current_user, + db=db, + ) + assert exc.value.status_code == 404 + finally: + mcp_api._resolve_mcp_server_for_request = real_resolve + db.close() diff --git a/tests/web/api/test_mcp_team_connector_edit.py b/tests/web/api/test_mcp_team_connector_edit.py new file mode 100644 index 0000000000..9acdee9326 --- /dev/null +++ b/tests/web/api/test_mcp_team_connector_edit.py @@ -0,0 +1,1640 @@ +"""The edit right on a team-linked MCP connector: ``GET``/``PUT +/api/mcp/servers/{server_id}`` resolve a caller with no personal row +through the connector access hook instead of 404ing outright, the edit +branch of ``_check_mcp_permission`` falls back to that verdict, the two +per-user fields reject outright for a caller with no row to hold them, a +raising hook surfaces as its declared status rather than a 500, a +stand-in whose verdict denies edit is refused outright rather than +reported as an empty success, and the verdict is re-resolved once more +after the definition row's lock is taken, refusing the write if it no +longer grants what the pre-lock answer granted. + +Every test installs the access hook through +``snapshot_connector_team_hooks`` so no hook state leaks between tests or +into suites that run after this one. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest +from fastapi import HTTPException +from sqlalchemy import create_engine, event +from sqlalchemy.orm import sessionmaker + +from xagent.core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError +from xagent.web.api import mcp as mcp_module +from xagent.web.api.mcp import ( + MCPAppConnectRequest, + MCPServerUpdate, + _check_mcp_permission, + connect_mcp_app, + get_mcp_server, + get_mcp_servers, + toggle_mcp_server, + update_mcp_server, +) +from xagent.web.models.database import Base +from xagent.web.models.mcp import MCPServer, UserMCPServer +from xagent.web.models.public_mcp import PublicMCPApp +from xagent.web.models.user import User +from xagent.web.services.connector_team_scope import ( + ConnectorAccess, + set_connector_team_hooks, + snapshot_connector_team_hooks, +) + + +@pytest.fixture() +def db(): + engine = create_engine("sqlite:///:memory:") + Base.metadata.create_all(engine) + session_factory = sessionmaker(autocommit=False, autoflush=False, bind=engine) + session = session_factory() + try: + yield session + finally: + session.close() + + +def _make_user(db, user_id: int, *, is_admin: bool = False) -> User: + user = User( + id=user_id, username=f"user-{user_id}", password_hash="x", is_admin=is_admin + ) + db.add(user) + db.commit() + return user + + +def _make_owned_server(db, owner_id: int, *, name: str = "shared-server") -> MCPServer: + server = MCPServer(name=name, transport="stdio", managed="external", command="true") + db.add(server) + db.flush() + db.add( + UserMCPServer( + user_id=owner_id, + mcpserver_id=server.id, + is_owner=True, + is_active=True, + ) + ) + db.commit() + return server + + +def _sequenced_access_hook(*answers): + """An access hook that answers differently on successive calls, so a + test can make the second (post-lock) resolution disagree with the + first. ``None`` in the sequence means an empty answer -- the batch + contract's way of saying "the caller's team does not link this". An + entry that is an exception instance is raised instead of returned, so a + test can make the second resolution fail outright. The last entry + repeats for any further call. Records every call's ``refs`` on + ``.calls`` so a test can pin how many round trips the route pays.""" + calls: list[object] = [] + + def hook(db, user_id, refs): + calls.append(refs) + index = min(len(calls) - 1, len(answers) - 1) + answer = answers[index] + if isinstance(answer, BaseException): + raise answer + if answer is None: + return {} + return {ref: answer for ref in refs} + + hook.calls = calls + return hook + + +class TestCheckMcpPermissionTeamAccessFallback: + """New assertions only -- ``test_check_mcp_permission`` in + test_mcp_api.py is left untouched by design.""" + + def test_owner_wins_the_edit_branch_without_consulting_the_verdict(self): + from unittest.mock import MagicMock + + owner = MagicMock(is_owner=True, can_delete=False) + # A verdict that would deny edit rights on its own is still beaten + # by is_owner -- the verdict is a fallback, never an override. + denying_access = ConnectorAccess(team_owned=True, can_edit=False) + assert ( + _check_mcp_permission( + owner, is_admin=False, require="edit", team_access=denying_access + ) + is True + ) + + def test_non_owner_falls_back_to_a_granting_verdict(self): + from unittest.mock import MagicMock + + guest = MagicMock(is_owner=False, can_delete=False) + granting_access = ConnectorAccess(team_owned=True, can_edit=True) + assert ( + _check_mcp_permission( + guest, is_admin=False, require="edit", team_access=granting_access + ) + is True + ) + + def test_non_owner_stays_denied_by_a_linked_but_not_editable_verdict(self): + from unittest.mock import MagicMock + + guest = MagicMock(is_owner=False, can_delete=False) + linked_only = ConnectorAccess(team_owned=True, can_edit=False) + assert ( + _check_mcp_permission( + guest, is_admin=False, require="edit", team_access=linked_only + ) + is False + ) + + def test_missing_team_access_keyword_behaves_exactly_as_before(self): + from unittest.mock import MagicMock + + owner = MagicMock(is_owner=True, can_delete=False) + guest = MagicMock(is_owner=False, can_delete=False) + assert _check_mcp_permission(owner, is_admin=False, require="edit") is True + assert _check_mcp_permission(guest, is_admin=False, require="edit") is False + + def test_delete_branch_ignores_team_access_entirely(self): + """Delete stays exactly as it is today: a granting verdict changes + nothing on the ``delete`` branch, which reads only ``can_delete``.""" + from unittest.mock import MagicMock + + guest = MagicMock(is_owner=False, can_delete=False) + granting_access = ConnectorAccess(team_owned=True, can_edit=True) + assert ( + _check_mcp_permission( + guest, is_admin=False, require="delete", team_access=granting_access + ) + is False + ) + + +class TestGateHelperOnGetAndPut: + def test_get_404s_for_an_unrelated_user_with_no_link_and_no_team_access(self, db): + owner = _make_user(db, 1) + stranger = _make_user(db, 2) + server = _make_owned_server(db, owner.id) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=lambda db, user_id, refs: {}) + with pytest.raises(HTTPException) as exc: + get_mcp_server(server.id, current_user=stranger, db=db) + assert exc.value.status_code == 404 + + def test_get_returns_the_stand_in_for_a_team_member_with_no_personal_row(self, db): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + response = get_mcp_server(server.id, current_user=member, db=db) + + assert response.id == server.id + assert response.user_id == member.id + + def test_get_owner_behaviour_is_unchanged_with_no_hook_installed(self, db): + owner = _make_user(db, 1) + server = _make_owned_server(db, owner.id) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks() + response = get_mcp_server(server.id, current_user=owner, db=db) + + assert response.id == server.id + assert response.can_edit_global is True + + +class TestPutWiringForATeamEditor: + def test_team_editor_edit_is_durable_and_creates_no_association_row(self, db): + owner = _make_user(db, 1) + editor = _make_user(db, 2) + server = _make_owned_server(db, owner.id) + server_id = server.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + response = update_mcp_server( + server_id, + MCPServerUpdate(description="edited by the team"), + current_user=editor, + db=db, + ) + + assert response.description == "edited by the team" + + # I5: durability, not staging -- a same-session query would still + # see an uncommitted UPDATE even if the route never committed. + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.description == "edited by the team" + + # I6: the edit did not fabricate a personal association for the + # team editor -- that would be a get-or-create write on an + # authorization path. + assert ( + db.query(UserMCPServer).filter(UserMCPServer.user_id == editor.id).first() + is None + ) + + def test_a_member_with_a_personal_row_edits_the_shared_config_durably(self, db): + """Design invariants I5 and I6 for the population they were + written for and never got: a caller whose own personal row does + not grant edit, widened by a granting team verdict. The existing + coverage for both invariants uses the stand-in population, which + has no personal row at all.""" + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="both-rows-mcp") + server_id = server.id + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=_sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True) + ) + ) + response = update_mcp_server( + server_id, + MCPServerUpdate(description="widened-by-the-team"), + current_user=member, + db=db, + ) + + assert response.can_edit_global is True + + # I5: durability, not staging. + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.description == "widened-by-the-team" + # I6: the caller's one personal row, not a second one. + assert ( + db.query(UserMCPServer) + .filter( + UserMCPServer.user_id == member.id, + UserMCPServer.mcpserver_id == server_id, + ) + .count() + == 1 + ) + + def test_view_only_team_member_cannot_tamper_the_shared_config(self, db): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id) + server_id = server.id + # A personal, non-owner association row (population D), not a + # stand-in: a stand-in whose verdict denies edit is now refused + # outright before this route ever reaches the shared-config tamper + # check this test is pinning (see + # TestADenyingStandInIsRefusedRatherThanReportedSuccessful). D still + # has no can_edit of its own and no granting verdict, so it hits + # the same tamper-check 403 this test always meant to cover. + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=False) + for ref in refs + } + ) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(description="should not land"), + current_user=member, + db=db, + ) + assert exc.value.status_code == 403 + assert "shared configuration" in exc.value.detail + + def test_rename_propagates_to_team_agent_selectors(self, db, monkeypatch): + """I10, and the mutation check the design requires for it: deleting + the ``rename_team_connector`` call must turn this red.""" + owner = _make_user(db, 1) + editor = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="old-name") + server_id = server.id + + calls: list[tuple[str, str]] = [] + + def fake_renamed_hook(_db, _user_id, _connector_type, _connector_id, old, new): + calls.append((old, new)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + }, + renamed=fake_renamed_hook, + ) + update_mcp_server( + server_id, + MCPServerUpdate(name="new-name"), + current_user=editor, + db=db, + ) + + assert calls == [("old-name", "new-name")] + + +class TestUserEnvAndIsActiveRejectionForAStandIn: + def test_user_env_from_a_caller_with_no_personal_row_is_400_not_a_silent_drop( + self, db + ): + owner = _make_user(db, 1) + editor = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="unchanged-name") + server_id = server.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(user_env={"API_KEY": "x"}), + current_user=editor, + db=db, + ) + + assert exc.value.status_code == 400 + assert "personal connection" in str(exc.value.detail) + + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.name == "unchanged-name" + assert ( + db.query(UserMCPServer).filter(UserMCPServer.user_id == editor.id).first() + is None + ) + + def test_is_active_from_a_caller_with_no_personal_row_is_400_not_a_silent_drop( + self, db + ): + owner = _make_user(db, 1) + editor = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="still-unchanged") + server_id = server.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(is_active=False), + current_user=editor, + db=db, + ) + + assert exc.value.status_code == 400 + + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.name == "still-unchanged" + assert ( + db.query(UserMCPServer).filter(UserMCPServer.user_id == editor.id).first() + is None + ) + + +class TestTypedErrorArm: + """A raising hook still surfaces its declared status for a caller whose + own personal row does not already decide the answer -- the verdict is + genuinely the gate for that population, and must stay fail-closed. An + owner's row already decides the answer on its own, so a hook is never + called for it at all; that population is pinned separately, below, in + ``TestOwnerIsImmuneToAHookFailure``.""" + + def test_get_surfaces_a_raising_hooks_declared_status(self, db): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id) + + def boom(*_a, **_k): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + with pytest.raises(HTTPException) as exc: + get_mcp_server(server.id, current_user=member, db=db) + + assert exc.value.status_code == 503 + + def test_put_surfaces_a_raising_hooks_declared_status_and_leaves_the_row_unchanged( + self, db + ): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="pristine") + server_id = server.id + + def boom(*_a, **_k): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(name="should-not-land"), + current_user=member, + db=db, + ) + + assert exc.value.status_code == 503 + + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.name == "pristine" + + def test_put_passes_through_a_planted_connector_runtime_error_by_its_own_status( + self, db + ): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id) + + def boom(*_a, **_k): + raise ConnectorRuntimeError("planted", "planted failure", status_code=409) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server.id, + MCPServerUpdate(name="irrelevant"), + current_user=member, + db=db, + ) + + assert exc.value.status_code == 409 + assert exc.value.detail == "planted failure" + + +class TestOwnerIsImmuneToAHookFailure: + """An owner's row already decides the edit answer on its own -- the + edit branch returns True on ``is_owner`` without ever consulting a + verdict -- so ``GET``/``PUT`` never call the hook for an owner's row at + all. A hook that would raise must therefore never surface: both routes + return their normal success status, unaffected by whatever the hook + would have done.""" + + def test_get_and_put_succeed_for_an_owner_even_though_the_hook_would_raise( + self, db + ): + owner = _make_user(db, 1) + server = _make_owned_server(db, owner.id, name="owner-immune") + server_id = server.id + + def boom(*_a, **_k): + raise ValueError("hook exploded") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + get_response = get_mcp_server(server_id, current_user=owner, db=db) + put_response = update_mcp_server( + server_id, + MCPServerUpdate(description="edited by the owner"), + current_user=owner, + db=db, + ) + + assert get_response.can_edit_global is True + assert put_response.can_edit_global is True + assert put_response.description == "edited by the owner" + + +def _make_catalog_app(db, app_id: str) -> None: + db.add( + PublicMCPApp( + app_id=app_id, + name=app_id, + transport="stdio", + launch_config={"command": "true", "args": []}, + ) + ) + db.commit() + + +def _make_catalog_app_with_display_name( + db, app_id: str, display_name: str, *, transport: str = "stdio", launch_config=None +) -> None: + """A catalog app written the way the real registry writes one: the + display name is NOT the app_id. A test that seeds name == app_id would + let a name-only implementation pass for the wrong reason. + """ + db.add( + PublicMCPApp( + app_id=app_id, + name=display_name, + transport=transport, + launch_config=launch_config or {"command": "true", "args": []}, + ) + ) + db.commit() + + +def _make_catalog_server_row( + db, + *, + name: str, + transport: str = "stdio", + command: str | None = "true", + args: list | None = None, + url: str | None = None, + auth: dict | None = None, + env: dict | None = None, +) -> MCPServer: + """A shared server row shaped the way a catalog provisioning helper + would write it, constructed directly rather than through connect/OAuth + so a test can pick exactly which catalog shape it needs (api_key, + mcp_oauth, or a renamed builtin_oauth row).""" + server = MCPServer( + name=name, + transport=transport, + managed="external", + command=command, + args=args if args is not None else [], + url=url, + auth=auth, + env=env, + ) + db.add(server) + db.flush() + return server + + +class TestDecorationDegradesAfterTheWriteCommits: + """``toggle`` and ``connect`` both commit their write before resolving + the verdict, purely to decorate the response's ``can_edit_global`` -- + a hook failure there must degrade that field to False rather than fail + a request whose write already landed.""" + + async def test_toggle_degrades_and_keeps_its_effect_when_the_hook_raises( + self, db, monkeypatch + ): + # A non-owner personal row, not the owner's: an owner's + # can_edit_global cannot be moved by any verdict at all (is_owner + # wins outright), so only a non-owner's reported field actually + # depends on whether the verdict resolved or degraded. + owner = _make_user(db, 1) + editor = _make_user(db, 2) + server = _make_owned_server(db, owner.id) + server_id = server.id + db.add( + UserMCPServer( + user_id=editor.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + before = ( + db.query(UserMCPServer) + .filter( + UserMCPServer.user_id == editor.id, + UserMCPServer.mcpserver_id == server_id, + ) + .one() + .is_active + ) + + def boom(*_a, **_k): + raise ValueError("hook exploded") + + fake_logger = MagicMock() + monkeypatch.setattr(mcp_module, "logger", fake_logger) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + response = toggle_mcp_server(server_id, current_user=editor, db=db) + + assert response.can_edit_global is False + fake_logger.warning.assert_called_once() + + db.rollback() + refreshed = ( + db.query(UserMCPServer) + .filter( + UserMCPServer.user_id == editor.id, + UserMCPServer.mcpserver_id == server_id, + ) + .one() + ) + assert refreshed.is_active is (not before) + + def test_connect_degrades_and_keeps_its_effect_when_the_hook_raises( + self, db, monkeypatch + ): + user = _make_user(db, 1) + _make_catalog_app(db, "decorate-only-app") + + def boom(*_a, **_k): + raise ValueError("hook exploded") + + fake_logger = MagicMock() + monkeypatch.setattr(mcp_module, "logger", fake_logger) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + response = connect_mcp_app( + "decorate-only-app", + MCPAppConnectRequest(), + current_user=user, + db=db, + ) + + assert response.can_edit_global is False + fake_logger.warning.assert_called_once() + + db.rollback() + server = db.query(MCPServer).filter(MCPServer.name == "decorate-only-app").one() + assoc = ( + db.query(UserMCPServer) + .filter( + UserMCPServer.user_id == user.id, + UserMCPServer.mcpserver_id == server.id, + ) + .one() + ) + assert assoc.is_owner is False + + +class TestADenyingStandInIsRefusedRatherThanReportedSuccessful: + """A stand-in (no personal association row) whose verdict denies edit + has an empty writable field set on this route: the personal-field + guard refuses user_env/is_active (there is no personal row to hold + them), the tamper check refuses every shared field it can compare, and + the fields it cannot compare (secrets) are silently emptied out of the + payload rather than written. Every payload such a caller can send was + therefore already a no-op before this guard existed -- a 200 for it + reported success for a write that never happened. All three payload + shapes below are the ones that used to slip past the tamper check + specifically (an unset payload, a secret-only payload the tamper check + deliberately does not compare, and a payload that resubmits the + connector's current value) and confirm none of them can still commit + anything even with the new guard in place. + """ + + def _stand_in(self, db): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="denying-stand-in-target") + server_id = server.id + + def _run(payload): + # Captured as plain values, not read off ``server`` after the + # call: ``server`` and the ``refreshed`` row below share the + # same identity-mapped Python object in this session, so + # comparing one against the other after the call is comparing + # the object with itself and can never fail. + original_name = str(server.name) + original_description = ( + str(server.description) if server.description is not None else None + ) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=False) + for ref in refs + } + ) + with pytest.raises(HTTPException) as exc: + update_mcp_server(server_id, payload, current_user=member, db=db) + assert exc.value.status_code == 403 + + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.name == original_name + assert refreshed.description == original_description + assert ( + db.query(UserMCPServer) + .filter(UserMCPServer.user_id == member.id) + .count() + == 0 + ) + return server, refreshed + + return server_id, _run + + def test_an_empty_payload_is_refused(self, db): + _server_id, run = self._stand_in(db) + run(MCPServerUpdate()) + + def test_a_secrets_only_payload_the_tamper_check_never_compares_is_refused( + self, db + ): + _server_id, run = self._stand_in(db) + run(MCPServerUpdate(config={"env": {"K": "v"}})) + + def test_resubmitting_the_current_value_is_refused(self, db): + server_id, run = self._stand_in(db) + server = db.query(MCPServer).filter(MCPServer.id == server_id).one() + server.description = "the connector's current description" + db.commit() + run(MCPServerUpdate(description=server.description)) + + +class TestTheVerdictIsRevalidatedUnderTheDefinitionLock: + """The verdict that granted a stand-in edit access is resolved before + this route's own row lock exists. The installing application can + revoke the team's link to this connector at any moment in between -- + it writes its own tables, which this lock does not cover -- so the + route re-resolves the verdict once more after taking the lock, and + refuses (with zero side effects) if the answer no longer grants edit. + This narrows the window between resolving the verdict and committing + the write; it does not close it, since the caller's own definition-row + lock has nothing to say about a revoke the installing application makes + through its own tables. + """ + + def _run(self, db, *, hook): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="revalidated-under-lock") + server_id = server.id + # Captured as plain values before the call, not read off ``server`` + # afterwards: ``server`` and the requery below share the same + # identity-mapped Python object in this session, so comparing one + # against the other after the call would be comparing the object + # with itself and could never fail. + original_name = str(server.name) + original_description = ( + str(server.description) if server.description is not None else None + ) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + result = {} + try: + result["response"] = update_mcp_server( + server_id, + MCPServerUpdate(description="edited-while-in-flight"), + current_user=member, + db=db, + ) + except HTTPException as exc: + result["error"] = exc + return server, server_id, result, original_name, original_description + + def test_revoked_between_resolution_and_lock_is_refused(self, db): + hook = _sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True), None + ) + _server, server_id, result, original_name, original_description = self._run( + db, hook=hook + ) + + assert result["error"].status_code == 403 + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.name == original_name + assert refreshed.description == original_description + assert db.query(UserMCPServer).filter(UserMCPServer.user_id == 2).count() == 0 + + def test_downgraded_to_not_editable_between_resolution_and_lock_is_refused( + self, db + ): + hook = _sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True), + ConnectorAccess(team_owned=True, can_edit=False), + ) + _server, server_id, result, original_name, original_description = self._run( + db, hook=hook + ) + + assert result["error"].status_code == 403 + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.name == original_name + assert refreshed.description == original_description + assert db.query(UserMCPServer).filter(UserMCPServer.user_id == 2).count() == 0 + + def test_still_granted_on_recheck_commits_durably(self, db): + hook = _sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True), + ConnectorAccess(team_owned=True, can_edit=True), + ) + _server, server_id, result, _original_name, _original_description = self._run( + db, hook=hook + ) + + assert "error" not in result + assert result["response"].description == "edited-while-in-flight" + + # I5: durability, not staging. + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.description == "edited-while-in-flight" + + def test_a_verdict_that_changed_under_the_lock_is_the_one_reported(self, db): + """The pre-lock answer granted edit; the post-lock answer still + grants it but is a different object. The 200's can_edit_global + must come from the answer the write was authorized on, not from + the one resolved before the lock existed. + + This test alone cannot distinguish "reports the recheck" from + "reports the pre-lock answer": both objects grant edit, so either + one reported here yields the same True. What it pins is that the + recheck running does not accidentally break the response -- for + example by reassigning team_access in the refusal branch, or by + setting it to None. The mutation record for this test is kept in + the delivery report rather than asserted here, because the + distinguishing mutation (deleting the reassignment outright) + leaves this test green. + """ + hook = _sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True), + ConnectorAccess(team_owned=True, can_edit=True), + ) + _server, _server_id, result, _name, _description = self._run(db, hook=hook) + + assert "error" not in result + assert result["response"].can_edit_global is True + + def test_recheck_that_raises_surfaces_the_hooks_own_status_with_zero_side_effects( + self, db + ): + hook = _sequenced_access_hook( + ConnectorAccess(team_owned=True, can_edit=True), + ValueError("hook exploded during recheck"), + ) + _server, server_id, result, original_name, original_description = self._run( + db, hook=hook + ) + + assert result["error"].status_code == 503 + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.name == original_name + assert refreshed.description == original_description + assert db.query(UserMCPServer).filter(UserMCPServer.user_id == 2).count() == 0 + + +class TestTheRecheckCostsExactlyOneExtraHookCall: + """Which populations pay the recheck's extra hook round trip, and which + do not, spelled out as call counts. This is the executable form of the + trigger-condition table in the design: the recheck only runs when the + verdict is the caller's authority for a payload that actually needs it. + """ + + def test_a_granting_stand_in_editing_the_shared_config_pays_two_calls(self, db): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="cost-stand-in-shared") + server_id = server.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + update_mcp_server( + server_id, + MCPServerUpdate(description="shared-edit"), + current_user=member, + db=db, + ) + + assert len(hook.calls) == 2 + + def test_a_granting_stand_in_with_an_empty_payload_pays_one_call(self, db): + """An empty payload's ``model_fields_set`` is the empty set, which + is a subset of ``{"user_env", "is_active"}`` -- the personal-only + exemption, not the earlier personal-field 400 guard (that guard + only fires when ``user_env``/``is_active`` is actually present). + This is the payload shape that actually reaches the recheck's own + condition and exercises the exemption, unlike an is_active-only + payload, which never gets there at all for a stand-in (it 400s + first).""" + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="cost-stand-in-personal-only") + server_id = server.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + update_mcp_server(server_id, MCPServerUpdate(), current_user=member, db=db) + + assert len(hook.calls) == 1 + + def test_a_granting_stand_in_who_is_a_platform_admin_pays_one_call(self, db): + """A platform admin's write authority never comes from the verdict + in the first place: ``_check_mcp_permission`` answers True on + ``is_admin`` before it ever reads one. The recheck condition's + ``and not getattr(current_user, "is_admin", False)`` exists to skip + the recheck for exactly this population -- deleting that clause + from the condition must turn this red (2 calls instead of 1).""" + owner = _make_user(db, 1) + admin = _make_user(db, 2, is_admin=True) + server = _make_owned_server(db, owner.id, name="cost-stand-in-admin") + server_id = server.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + update_mcp_server( + server_id, + MCPServerUpdate(description="admin-edit"), + current_user=admin, + db=db, + ) + + assert len(hook.calls) == 1 + + def test_an_owner_pays_zero_calls(self, db): + owner = _make_user(db, 1) + server = _make_owned_server(db, owner.id, name="cost-owner") + server_id = server.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + update_mcp_server( + server_id, + MCPServerUpdate(description="owner-edit"), + current_user=owner, + db=db, + ) + + assert len(hook.calls) == 0 + + def test_a_denying_verdict_on_a_personal_row_pays_one_call(self, db): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="cost-personal-denied") + server_id = server.id + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=False)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + update_mcp_server( + server_id, + MCPServerUpdate(is_active=False), + current_user=member, + db=db, + ) + + assert len(hook.calls) == 1 + + def test_a_member_with_a_personal_row_pays_one_call_on_a_real_personal_field( + self, db + ): + """The personal-only exemption, exercised by a payload that + actually carries a personal field and by a caller the payload can + land on. The existing coverage is degenerate in two different + ways: the empty-payload case (above) never carries a field at all, + and the denying-verdict case (above) short-circuits one clause + earlier, on ``team_access.can_edit``, so neither reaches the + exemption with a real value.""" + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="both-rows-personal-only") + server_id = server.id + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + update_mcp_server( + server_id, + MCPServerUpdate(is_active=False), + current_user=member, + db=db, + ) + + assert len(hook.calls) == 1 + # The personal write the exemption exists to let through actually landed. + db.rollback() + refreshed = ( + db.query(UserMCPServer) + .filter( + UserMCPServer.user_id == member.id, + UserMCPServer.mcpserver_id == server_id, + ) + .one() + ) + assert refreshed.is_active is False + + +class TestCatalogRowsAreNeverTeamEditable: + """A team verdict that grants edit is downgraded to ``can_edit=False`` + whenever the row it names is some platform catalog app's shared row -- + across every kind of catalog row (api_key, mcp_oauth, a builtin_oauth + row an administrator renamed) and every route that produces or reports + a verdict (the GET/PUT gate, the list endpoint's two loops, connect, + and toggle). A self-built connector that happens to squat a catalog id + is deliberately NOT exempted from this: its creator keeps their own + edit right in full (``is_owner`` decides that outright), but a + teammate editing it on the owner's behalf is not. + """ + + def test_team_stand_in_cannot_rewrite_an_api_key_catalog_rows_command(self, db): + _make_catalog_app_with_display_name(db, "stripe", "Stripe") + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_catalog_server_row( + db, + name="stripe", + transport="stdio", + command="python", + args=["-m", "xagent.web.tools.mcp.stripe"], + ) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.commit() + server_id = server.id + original_command = server.command + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(config={"command": "evil", "args": []}), + current_user=member, + db=db, + ) + + assert exc.value.status_code == 403 + assert "You do not have permission to edit this MCP server" in exc.value.detail + # Exactly one hook call: the refusal comes from the downgrade + # applied when the verdict is first resolved, before any personal + # row exists to hold an edit right -- not from the post-lock + # recheck catching it a step later (that would be two calls). + assert len(hook.calls) == 1 + + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.command == original_command + + def test_team_stand_in_cannot_rewrite_an_mcp_oauth_catalog_rows_url(self, db): + _make_catalog_app_with_display_name( + db, + "notion", + "Notion", + transport="streamable_http", + launch_config={ + "url": "https://mcp.notion.com/mcp", + "auth": {"type": "mcp_oauth"}, + }, + ) + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_catalog_server_row( + db, + name="notion", + transport="streamable_http", + command=None, + url="https://mcp.notion.com/mcp", + auth={"type": "mcp_oauth"}, + ) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.commit() + server_id = server.id + original_url = server.url + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(config={"url": "https://evil.example/mcp"}), + current_user=member, + db=db, + ) + + assert exc.value.status_code == 403 + assert "You do not have permission to edit this MCP server" in exc.value.detail + assert len(hook.calls) == 1 + + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.url == original_url + + def test_team_stand_in_cannot_rewrite_an_api_key_catalog_row_with_no_platform_key( + self, db + ): + """Same shape as the ``stripe`` case above, except this row carries + no platform fallback key in ``env`` at all -- the one distinction + that matters if the downgrade were (wrongly) gated on + ``_catalog_server_has_platform_key`` instead of catalog membership: + that function reads False here, but the row is still the + platform's, not this team's, to hand out edit rights on.""" + _make_catalog_app_with_display_name( + db, + "acme-books", + "Acme Books", + transport="stdio", + launch_config={ + "command": "python", + "args": ["-m", "acme_books"], + "required_env": ["ACME_BOOKS_API_KEY"], + }, + ) + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_catalog_server_row( + db, + name="acme-books", + transport="stdio", + command="python", + args=["-m", "acme_books"], + env=None, + ) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.commit() + server_id = server.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(config={"command": "evil", "args": []}), + current_user=member, + db=db, + ) + + assert exc.value.status_code == 403 + + def test_a_self_built_row_with_no_name_collision_is_still_team_editable(self, db): + _make_catalog_app_with_display_name(db, "stripe", "Stripe") + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="my-custom-tool") + server_id = server.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + response = update_mcp_server( + server_id, + MCPServerUpdate(description="edited by the team"), + current_user=member, + db=db, + ) + + assert response.can_edit_global is True + + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.description == "edited by the team" + + def test_the_catalog_rows_owner_can_still_edit_it_themselves(self, db): + """A builtin_oauth connect writes ``is_owner=True`` on the + connecting user's association -- unlike the key-based/mcp_oauth + paths, which never do. The owner's edit right must not move: no + verdict is even consulted for it, so the hook installed here must + never be called at all. + + Uses the same stdio/api_key catalog shape as the tests above rather + than an actual oauth-transport row: ``update_mcp_server`` rebuilds + and revalidates the transport-specific config on every call + (including a description-only one), and ``MCPServerConfig`` does + not accept ``transport="oauth"`` at all -- a pre-existing + limitation of this route, unrelated to catalog membership. What + this test pins is the ownership bypass itself, which does not + depend on which catalog shape carries it. + """ + _make_catalog_app_with_display_name(db, "stripe", "Stripe") + owner = _make_user(db, 1) + server = _make_catalog_server_row( + db, + name="stripe", + transport="stdio", + command="python", + args=["-m", "xagent.web.tools.mcp.stripe"], + ) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.commit() + server_id = server.id + + def hook_must_not_be_called(*_a, **_k): + raise AssertionError("the access hook must not be called for an owner") + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook_must_not_be_called) + response = update_mcp_server( + server_id, + MCPServerUpdate(description="edited by its owner"), + current_user=owner, + db=db, + ) + + assert response.can_edit_global is True + + def test_get_on_a_catalog_row_still_reaches_it_but_reports_no_edit_right(self, db): + _make_catalog_app_with_display_name(db, "stripe", "Stripe") + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_catalog_server_row( + db, name="stripe", transport="stdio", command="python" + ) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.commit() + server_id = server.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + response = get_mcp_server(server_id, current_user=member, db=db) + + # What this pins is "reachable and readable, with no edit right": + # reaching this assertion at all means no 404 was raised. It does not + # distinguish clearing can_edit from dropping the verdict entirely -- + # past the 404 test above, those two are indistinguishable here. + assert response.can_edit_global is False + + def test_connecting_a_catalog_app_reports_no_edit_right_even_when_granted(self, db): + _make_catalog_app_with_display_name(db, "stripe", "Stripe") + user = _make_user(db, 1) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + response = connect_mcp_app( + "stripe", + MCPAppConnectRequest(), + current_user=user, + db=db, + ) + + assert response.can_edit_global is False + + def test_the_list_endpoints_stand_in_row_reports_no_edit_right(self, db): + _make_catalog_app_with_display_name( + db, + "notion", + "Notion", + transport="streamable_http", + launch_config={ + "url": "https://mcp.notion.com/mcp", + "auth": {"type": "mcp_oauth"}, + }, + ) + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_catalog_server_row( + db, + name="notion", + transport="streamable_http", + command=None, + url="https://mcp.notion.com/mcp", + auth={"type": "mcp_oauth"}, + ) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.commit() + server_id = server.id + + def visibility_hook(_db, _user_id): + return {"mcp": {server_id}, "custom_api": set()} + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + }, + visibility=visibility_hook, + ) + responses = get_mcp_servers(current_user=member, db=db) + + matches = [r for r in responses if r.id == server_id] + assert len(matches) == 1 + assert matches[0].can_edit_global is False + + def test_the_list_endpoints_personal_row_on_a_catalog_server_reports_no_edit_right( + self, db + ): + """Same downgrade as the stand-in case above, but for the other of + the list endpoint's two append loops: a caller who has their own + (non-owner) personal row on a catalog server, rather than no + personal row at all. Both loops call ``_team_access_for_shared_row`` + independently, so each needs its own test pinning it. + """ + _make_catalog_app_with_display_name(db, "stripe", "Stripe") + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_catalog_server_row( + db, name="stripe", transport="stdio", command="python" + ) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server.id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + server_id = server.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + responses = get_mcp_servers(current_user=member, db=db) + + matches = [r for r in responses if r.id == server_id] + assert len(matches) == 1 + assert matches[0].can_edit_global is False + + def test_toggle_on_a_catalog_row_reports_no_edit_right(self, db): + _make_catalog_app_with_display_name(db, "stripe", "Stripe") + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_catalog_server_row( + db, name="stripe", transport="stdio", command="python" + ) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server.id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + server_id = server.id + + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + access=lambda db, user_id, refs: { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + ) + response = toggle_mcp_server(server_id, current_user=member, db=db) + + assert response.can_edit_global is False + + def test_a_self_built_row_that_squats_a_catalog_id_is_not_team_editable_but_its_owner_still_edits_it( + self, db + ): + _make_catalog_app_with_display_name(db, "widget-sync", "Widget Sync") + creator = _make_user(db, 1) + teammate = _make_user(db, 2) + # Built directly, the way this test file builds every row -- not + # through connect/create, which would refuse this name outright + # (_is_reserved_catalog_name). This is the row create/rename block + # today, arriving here as if it predated the catalog app, or as if + # the reserved-name gate had a bug; the point of this test is what + # happens to a row in this shape once it exists, not how one could + # come to exist. + server = _make_catalog_server_row( + db, + name="widget-sync", + transport="stdio", + command="a-command-the-creator-chose", + ) + db.add( + UserMCPServer( + user_id=creator.id, + mcpserver_id=server.id, + is_owner=True, + is_active=True, + ) + ) + db.commit() + server_id = server.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + # (a) A teammate editing it on the owner's behalf is refused -- + # the catalog claims this name, and the row's own creation history + # is not something this schema records today. + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(description="edited by a teammate"), + current_user=teammate, + db=db, + ) + assert exc.value.status_code == 403 + + # (b) Its own creator is unaffected -- is_owner decides the edit + # branch outright, before any verdict (downgraded or not) is read. + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + response = update_mcp_server( + server_id, + MCPServerUpdate(description="edited by its creator"), + current_user=creator, + db=db, + ) + assert response.can_edit_global is True + + def test_team_stand_in_cannot_rewrite_a_renamed_builtin_oauth_catalog_row(self, db): + """A builtin_oauth row an administrator renamed away from the + catalog's display name still carries its ``app_id`` in ``auth`` -- + the one shape ``_is_reserved_catalog_name`` (name-only) would miss, + which is why that function must not be the downgrade's predicate. + """ + _make_catalog_app_with_display_name(db, "gmail", "Gmail") + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_catalog_server_row( + db, + name="team-mail-renamed", + transport="oauth", + command=None, + auth={"app_id": "gmail"}, + ) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.commit() + server_id = server.id + hook = _sequenced_access_hook(ConnectorAccess(team_owned=True, can_edit=True)) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=hook) + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(description="edited by a teammate"), + current_user=member, + db=db, + ) + + assert exc.value.status_code == 403 + assert "You do not have permission to edit this MCP server" in exc.value.detail + assert len(hook.calls) == 1 + + +class TestCatalogCheckQueryBudget: + """The per-request cost of the catalog downgrade: a deployment with no + granting verdict in a listing response pays nothing extra at all, and + one that does pays exactly one additional statement -- a single + catalog-keys SELECT shared across every row in the response -- not one + per row. Pinned across two population sizes, the same discipline + ``TestListEndpointAccessHookCallBudget`` in + test_mcp_reported_edit_permission.py already uses for the hook-call + count itself. + """ + + def _list_query_count(self, db, *, num_rows: int, grant_edit: bool) -> int: + suffix = f"{grant_edit}-{num_rows}" + owner = _make_user(db, 2000 + num_rows * 10 + (1 if grant_edit else 0)) + caller = _make_user(db, 2050 + num_rows * 10 + (1 if grant_edit else 0)) + stand_in = [ + _make_owned_server(db, owner.id, name=f"budget-{suffix}-{i}") + for i in range(num_rows) + ] + # Read before the query listener attaches, matching the sibling + # class's own discipline: these ids were expired by their own + # setup commits, and reading them for the first time inside the + # measured window would count as a query this test's setup causes, + # not one the endpoint itself issues. + _ = caller.id + stand_in_ids = {s.id for s in stand_in} + + def visibility_hook(_db, _user_id): + return {"mcp": set(stand_in_ids), "custom_api": set()} + + def access_hook(hook_db, user_id, refs): + del hook_db, user_id + return { + ref: ConnectorAccess(team_owned=True, can_edit=True) for ref in refs + } + + queries: list[str] = [] + + def record_query(conn, cursor, statement, parameters, context, executemany): + del conn, cursor, parameters, context, executemany + queries.append(statement) + + engine = db.get_bind() + event.listen(engine, "before_cursor_execute", record_query) + try: + with snapshot_connector_team_hooks(): + set_connector_team_hooks( + visibility=visibility_hook, + access=access_hook if grant_edit else None, + ) + get_mcp_servers(current_user=caller, db=db) + finally: + event.remove(engine, "before_cursor_execute", record_query) + return len(queries) + + def test_a_deployment_with_no_access_hook_pays_nothing_regardless_of_row_count( + self, db + ): + counts = { + n: self._list_query_count(db, num_rows=n, grant_edit=False) for n in (2, 6) + } + assert counts[2] == counts[6], counts + + def test_a_granting_access_hook_costs_exactly_one_more_query_regardless_of_row_count( + self, db + ): + without_hook = { + n: self._list_query_count(db, num_rows=n, grant_edit=False) for n in (2, 6) + } + with_hook = { + n: self._list_query_count(db, num_rows=n, grant_edit=True) for n in (2, 6) + } + assert with_hook[2] == with_hook[6], with_hook + assert with_hook[2] == without_hook[2] + 1, (with_hook, without_hook) diff --git a/tests/web/services/test_connector_team_scope.py b/tests/web/services/test_connector_team_scope.py index 2fc9a3f446..a2e896f00d 100644 --- a/tests/web/services/test_connector_team_scope.py +++ b/tests/web/services/test_connector_team_scope.py @@ -9,10 +9,12 @@ from __future__ import annotations from collections.abc import Iterator +from contextlib import contextmanager +from decimal import Decimal from types import SimpleNamespace import pytest -from sqlalchemy import create_engine +from sqlalchemy import create_engine, select from sqlalchemy.orm import Session, sessionmaker from sqlalchemy.pool import StaticPool @@ -33,11 +35,47 @@ # --------------------------------------------------------------------------- +@contextmanager +def _reset_hooks_scope() -> Iterator[None]: + # Snapshot-and-restore, not clear-everything: this module's own + # ``set_connector_team_hooks`` docstring says it clears every slot it + # is not given, so calling it bare to "reset" would drop whatever the + # process had installed before this file ran. ``snapshot_connector_team_hooks`` + # is what the newer suites in this repo use, and this file is the last + # one that did not. Pulled out of the fixture below so a test can + # exercise this scope directly (see + # ``test_the_reset_scope_restores_a_pre_installed_hook_rather_than_clearing_it``), + # since the fixture itself wraps the whole test body and cannot be + # asserted on from inside one. + with connector_team_scope.snapshot_connector_team_hooks(): + yield + agent_team_scope.set_agent_team_scope_hook(None) + + @pytest.fixture(autouse=True) def _reset_hooks() -> Iterator[None]: - yield - connector_team_scope.set_connector_team_hooks() - agent_team_scope.set_agent_team_scope_hook(None) + with _reset_hooks_scope(): + yield + + +def test_the_reset_scope_restores_a_pre_installed_hook_rather_than_clearing_it(): + """This file's autouse reset must restore what the process had, not + clear everything: a bare ``set_connector_team_hooks()`` drops any hook + installed before this file ran (its own docstring says so), which is + what the newer suites in this repo use ``snapshot_connector_team_hooks`` + to avoid. Asserted directly against the extracted scope rather than + from inside a fixture-wrapped test, since the fixture wraps the whole + test body and so cannot observe its own effect on itself.""" + # No manual cleanup needed here: this whole test body already runs + # inside the autouse fixture's own ``_reset_hooks_scope()``, which + # restores whatever was installed before this test to whatever it was + # before, once this test returns -- a bare ``set_connector_team_hooks()`` + # here would be exactly the clear-everything pattern this fix removes. + sentinel = lambda *_a, **_k: {} # noqa: E731 + connector_team_scope.set_connector_team_hooks(access=sentinel) + with _reset_hooks_scope(): + connector_team_scope.set_connector_team_hooks(access=lambda *_a, **_k: {}) + assert connector_team_scope._connector_access_hook is sentinel def test_team_connector_ids_empty_without_hook_installed(): @@ -52,10 +90,13 @@ def test_team_connector_hook_installed_reflects_presence(): connector_team_scope.set_connector_team_hooks( team_visibility=lambda db, *, team_id: {"mcp": set(), "custom_api": set()} ) - try: - assert connector_team_scope.team_connector_hook_installed() is True - finally: - connector_team_scope.set_connector_team_hooks() + assert connector_team_scope.team_connector_hook_installed() is True + # Load-bearing, not teardown: this line is what the assertion below is + # actually exercising -- that clearing the hook flips the reported + # presence back to False. The autouse fixture's own snapshot restore + # still runs after this test regardless, so nothing here is relied on + # for cleanup. + connector_team_scope.set_connector_team_hooks() assert connector_team_scope.team_connector_hook_installed() is False @@ -67,14 +108,11 @@ def _hook(db, *, team_id): return {"mcp": {1}, "custom_api": set()} connector_team_scope.set_connector_team_hooks(team_visibility=_hook) - try: - assert connector_team_scope.team_connector_ids(None, team_id=None) == { - "mcp": set(), - "custom_api": set(), - } - assert calls == [] - finally: - connector_team_scope.set_connector_team_hooks() + assert connector_team_scope.team_connector_ids(None, team_id=None) == { + "mcp": set(), + "custom_api": set(), + } + assert calls == [] def test_team_hook_invocation_contract(): @@ -86,16 +124,13 @@ def _record(db, *, team_id): return {"mcp": set(), "custom_api": set()} connector_team_scope.set_connector_team_hooks(team_visibility=_record) - try: - assert connector_team_scope.team_connector_ids(None, team_id=None) == { - "mcp": set(), - "custom_api": set(), - } - assert calls == [] - connector_team_scope.team_connector_ids(None, team_id=T1) - assert calls == [("kw", T1)] - finally: - connector_team_scope.set_connector_team_hooks() + assert connector_team_scope.team_connector_ids(None, team_id=None) == { + "mcp": set(), + "custom_api": set(), + } + assert calls == [] + connector_team_scope.team_connector_ids(None, team_id=T1) + assert calls == [("kw", T1)] def test_team_hook_positional_only_callable_raises(): @@ -107,11 +142,385 @@ def _positional_only(db, team_id, /): return {"mcp": set(), "custom_api": set()} connector_team_scope.set_connector_team_hooks(team_visibility=_positional_only) - try: - with pytest.raises(TypeError): - connector_team_scope.team_connector_ids(None, team_id=T1) - finally: - connector_team_scope.set_connector_team_hooks() + with pytest.raises(TypeError): + connector_team_scope.team_connector_ids(None, team_id=T1) + + +# --------------------------------------------------------------------------- +# ConnectorAccess and the access hook slot. +# --------------------------------------------------------------------------- + + +def test_connector_access_defaults_are_both_false(): + access = connector_team_scope.ConnectorAccess() + assert access.team_owned is False + assert access.can_edit is False + + +def test_resolve_connector_access_returns_an_empty_map_without_a_hook_installed(): + for refs in ([("mcp", 1), ("custom_api", 1), ("mcp", 999)], [("mcp", 1)]): + assert connector_team_scope.resolve_connector_access(None, 7, refs) == {} + + +def test_resolve_connector_access_asks_no_hook_when_no_ref_needs_one(): + """An installed hook is never called when there is nothing to ask about + -- an empty ``refs`` collection short-circuits before the hook, the + same way no hook installed does.""" + calls: list[object] = [] + + def _hook(db, user_id, refs): + calls.append(refs) + return {} + + connector_team_scope.set_connector_team_hooks(access=_hook) + assert connector_team_scope.resolve_connector_access(None, 7, []) == {} + assert calls == [] + + +def test_resolve_connector_access_calls_the_hook_once_with_the_requested_refs(): + calls = [] + + def _hook(db, user_id, refs): + calls.append((db, user_id, refs)) + return { + ("mcp", 11): connector_team_scope.ConnectorAccess( + team_owned=True, can_edit=True + ) + } + + connector_team_scope.set_connector_team_hooks(access=_hook) + result = connector_team_scope.resolve_connector_access(None, 7, [("mcp", 11)]) + assert result == { + ("mcp", 11): connector_team_scope.ConnectorAccess( + team_owned=True, can_edit=True + ) + } + assert len(calls) == 1 + called_db, called_user_id, called_refs = calls[0] + assert (called_db, called_user_id) == (None, 7) + assert called_refs == frozenset({("mcp", 11)}) + + +def test_resolve_connector_access_a_ref_missing_from_the_answer_means_not_linked(): + """Leaving a ref out of the answer is the only way to say "the caller's + team does not link this connector" -- distinct from a rejected + malformed verdict for that same ref.""" + connector_team_scope.set_connector_team_hooks(access=lambda *a: {}) + assert connector_team_scope.resolve_connector_access(None, 7, [("mcp", 11)]) == {} + + +# --------------------------------------------------------------------------- +# Validation of the access hook's answer shape at the boundary. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "malformed_answer", + [ + "dict-of-fields", + "connector-delete-decision", + "tuple", + "truthy-object-with-right-attrs", + "none", + "list", + ], +) +def test_resolve_connector_access_rejects_a_non_dict_answer(malformed_answer): + # Built inside the test body, not the parametrize list: a couple of + # these shapes are instances of types this module defines, and + # constructing them at collection time would make the whole file + # uncollectable while those types don't exist yet. + answer = { + "dict-of-fields": {"team_owned": True, "can_edit": True}, + "connector-delete-decision": connector_team_scope.ConnectorDeleteDecision( + team_owned=True, authorized=True + ), + "tuple": (True, True), + "truthy-object-with-right-attrs": SimpleNamespace( + team_owned=True, can_edit=True + ), + "none": None, + "list": [connector_team_scope.ConnectorAccess(team_owned=True, can_edit=True)], + }[malformed_answer] + + connector_team_scope.set_connector_team_hooks(access=lambda *a: answer) + with pytest.raises(ValueError): + connector_team_scope.resolve_connector_access(None, 7, [("mcp", 11)]) + + +def test_resolve_connector_access_rejects_a_verdict_for_a_connector_nobody_asked_about(): + """A verdict keyed on a ref outside the requested set means the hook + answered a different question than the one it was asked -- silently + dropping it would hide that the hook and the caller have gone out of + sync, so this must fail loudly instead.""" + connector_team_scope.set_connector_team_hooks( + access=lambda *a: { + ("mcp", 999): connector_team_scope.ConnectorAccess( + team_owned=True, can_edit=True + ) + } + ) + with pytest.raises(ValueError): + connector_team_scope.resolve_connector_access(None, 7, [("mcp", 11)]) + + +@pytest.mark.parametrize( + "connector_type,requested_id,alias_id", + [ + ("mcp", 1, True), + ("mcp", 1, 1.0), + ("mcp", 1, Decimal("1")), + ("custom_api", 2, 2.0), + ("custom_api", 1, True), + ], + ids=["mcp-bool", "mcp-float", "mcp-decimal", "custom-api-float", "custom-api-bool"], +) +def test_resolve_connector_access_rejects_a_key_whose_id_is_only_equal_to_an_int( + connector_type, requested_id, alias_id +): + """``True == 1``, ``1.0 == 1`` and ``Decimal("1") == 1`` in Python, so a + key carrying any of those in place of the requested connector id would + pass an ``in``-based membership check against ``requested`` -- and be + stored as a grant for the connector it merely aliases, not the one it + actually is. The exact-type check must reject it before membership is + ever checked.""" + connector_team_scope.set_connector_team_hooks( + access=lambda *a: { + (connector_type, alias_id): connector_team_scope.ConnectorAccess( + team_owned=True, can_edit=True + ) + } + ) + with pytest.raises(ValueError, match="not an int"): + connector_team_scope.resolve_connector_access( + None, 7, [(connector_type, requested_id)] + ) + + +def test_resolve_connector_access_rejects_a_key_that_is_not_a_tuple(): + connector_team_scope.set_connector_team_hooks( + access=lambda *a: { + "mcp": connector_team_scope.ConnectorAccess(team_owned=True, can_edit=True) + } + ) + with pytest.raises( + ValueError, match=r"not a \(connector_type, connector_id\) pair" + ): + connector_team_scope.resolve_connector_access(None, 7, [("mcp", 1)]) + + +def test_resolve_connector_access_rejects_a_key_of_the_wrong_length(): + connector_team_scope.set_connector_team_hooks( + access=lambda *a: { + ("mcp", 1, "x"): connector_team_scope.ConnectorAccess( + team_owned=True, can_edit=True + ) + } + ) + with pytest.raises( + ValueError, match=r"not a \(connector_type, connector_id\) pair" + ): + connector_team_scope.resolve_connector_access(None, 7, [("mcp", 1)]) + + +def test_resolve_connector_access_rejects_a_key_whose_connector_type_is_not_a_str(): + connector_team_scope.set_connector_team_hooks( + access=lambda *a: { + (1, 1): connector_team_scope.ConnectorAccess(team_owned=True, can_edit=True) + } + ) + with pytest.raises(ValueError, match="connector type that is not a str"): + connector_team_scope.resolve_connector_access(None, 7, [(1, 1)]) + + +@pytest.mark.parametrize( + "wrong_value", + ["dict", "duck-typed", "delete-decision", "none", "true"], +) +def test_resolve_connector_access_rejects_a_verdict_value_that_is_not_a_connector_access( + wrong_value, +): + """The key was asked about and the key's shape is fine -- what is + wrong is the value. A duck-typed object carrying ``team_owned=True`` + and ``can_edit=True`` would satisfy every attribute check below it, so + the type check is the only thing that stops a hook from answering with + something that merely resembles a verdict. Built in the body, not the + parametrize list, because two of these are instances of types this + module defines.""" + value = { + "dict": {"team_owned": True, "can_edit": True}, + "duck-typed": SimpleNamespace(team_owned=True, can_edit=True), + "delete-decision": connector_team_scope.ConnectorDeleteDecision( + team_owned=True, authorized=True + ), + "none": None, + "true": True, + }[wrong_value] + + connector_team_scope.set_connector_team_hooks( + access=lambda *_a: {("mcp", 11): value} + ) + with pytest.raises(ValueError, match="expected ConnectorAccess values"): + connector_team_scope.resolve_connector_access(None, 7, [("mcp", 11)]) + + +@pytest.mark.parametrize( + "bad_team_owned", + [False, "yes", 1], + ids=["false", "truthy-string", "truthy-int"], +) +def test_resolve_connector_access_rejects_a_team_owned_that_is_not_true( + bad_team_owned, +): + """``team_owned`` must be exactly ``True`` on every verdict that + reaches a caller -- "not linked" is expressed by leaving the ref out + of the answer, never by a verdict carrying a falsy or merely-truthy + ``team_owned``.""" + verdict = connector_team_scope.ConnectorAccess( + team_owned=bad_team_owned, can_edit=True + ) + connector_team_scope.set_connector_team_hooks( + access=lambda *a: {("mcp", 11): verdict} + ) + with pytest.raises(ValueError): + connector_team_scope.resolve_connector_access(None, 7, [("mcp", 11)]) + + +def test_resolve_connector_access_rejects_a_bare_connector_access_default(): + """``ConnectorAccess()`` -- the dataclass's own all-``False`` default -- + is rejected the same way: constructing a bare instance must never + become a legitimate "not linked" answer.""" + connector_team_scope.set_connector_team_hooks( + access=lambda *a: {("mcp", 11): connector_team_scope.ConnectorAccess()} + ) + with pytest.raises(ValueError): + connector_team_scope.resolve_connector_access(None, 7, [("mcp", 11)]) + + +@pytest.mark.parametrize( + "bad_can_edit", + ["false", 1, 0], + ids=["string", "truthy-int", "falsy-int"], +) +def test_resolve_connector_access_rejects_a_can_edit_that_is_not_exactly_bool( + bad_can_edit, +): + """``bool`` is a subclass of ``int`` in Python, so ``1``/``0`` would + pass a truthiness check -- this seam requires an exact ``True``/ + ``False`` instead, since a truthy value is never a legitimate grant.""" + verdict = connector_team_scope.ConnectorAccess( + team_owned=True, can_edit=bad_can_edit + ) + connector_team_scope.set_connector_team_hooks( + access=lambda *a: {("mcp", 11): verdict} + ) + with pytest.raises(ValueError): + connector_team_scope.resolve_connector_access(None, 7, [("mcp", 11)]) + + +def test_resolve_connector_access_accepts_linked_but_not_editable(): + """A linked-but-not-editable answer is legal on its own -- the seam does + not require can_edit to be True just because team_owned is.""" + answer = connector_team_scope.ConnectorAccess(team_owned=True, can_edit=False) + connector_team_scope.set_connector_team_hooks( + access=lambda *a: {("mcp", 11): answer} + ) + assert connector_team_scope.resolve_connector_access(None, 7, [("mcp", 11)]) == { + ("mcp", 11): answer + } + + +# --------------------------------------------------------------------------- +# The typed-failure wrapper. +# --------------------------------------------------------------------------- + + +def test_resolve_connector_access_or_raise_converts_value_error_to_503(): + def _hook(db, user_id, refs): + raise ValueError("hook returned garbage") + + connector_team_scope.set_connector_team_hooks(access=_hook) + with pytest.raises(ConnectorRuntimeError) as excinfo: + connector_team_scope.resolve_connector_access_or_raise(None, 7, [("mcp", 11)]) + assert excinfo.value.status_code == 503 + + +def test_resolve_connector_access_or_raise_passes_through_planted_error(): + planted = ConnectorRuntimeError( + "planted_code", "planted", details={"reason": "planted_reason"} + ) + + def _hook(db, user_id, refs): + raise planted + + connector_team_scope.set_connector_team_hooks(access=_hook) + with pytest.raises(ConnectorRuntimeError) as excinfo: + connector_team_scope.resolve_connector_access_or_raise(None, 7, [("mcp", 11)]) + assert excinfo.value is planted + + +def test_resolve_connector_access_or_raise_converts_malformed_answer_too(): + """The validator's ValueError for a malformed answer goes through the + same conversion as any other hook-side failure.""" + connector_team_scope.set_connector_team_hooks( + access=lambda *a: { + ("mcp", 11): connector_team_scope.ConnectorAccess( + team_owned=False, can_edit=True + ) + } + ) + with pytest.raises(ConnectorRuntimeError) as excinfo: + connector_team_scope.resolve_connector_access_or_raise(None, 7, [("mcp", 11)]) + assert excinfo.value.status_code == 503 + + +# --------------------------------------------------------------------------- +# snapshot_connector_team_hooks and its discovery-based coverage test. +# --------------------------------------------------------------------------- + + +def _connector_hook_slot_names() -> list[str]: + return [name for name in vars(connector_team_scope) if name.endswith("_hook")] + + +def test_connector_hook_slot_names_are_discoverable(): + # Sanity check the enumeration itself finds all five known slots, so + # the coverage test below is not vacuously true. + names = _connector_hook_slot_names() + assert names.count("_connector_deleted_hook") == 1 + assert names.count("_connector_renamed_hook") == 1 + assert names.count("_connector_visibility_hook") == 1 + assert names.count("_team_connector_visibility_hook") == 1 + assert names.count("_connector_access_hook") == 1 + assert len(names) == 5 + + +def test_snapshot_connector_team_hooks_restores_every_slot_by_identity(): + names = _connector_hook_slot_names() + originals = {name: getattr(connector_team_scope, name) for name in names} + + with connector_team_scope.snapshot_connector_team_hooks(): + for name in names: + setattr(connector_team_scope, name, lambda *a, **k: None) + for name in names: + assert getattr(connector_team_scope, name) is not originals[name] + + for name in names: + assert getattr(connector_team_scope, name) is originals[name] + + +def test_snapshot_connector_team_hooks_restores_on_exception(): + names = _connector_hook_slot_names() + originals = {name: getattr(connector_team_scope, name) for name in names} + + with pytest.raises(RuntimeError): + with connector_team_scope.snapshot_connector_team_hooks(): + for name in names: + setattr(connector_team_scope, name, lambda *a, **k: None) + raise RuntimeError("boom inside the block") + + for name in names: + assert getattr(connector_team_scope, name) is originals[name] # --------------------------------------------------------------------------- @@ -144,6 +553,144 @@ def _create_user(db: Session, username: str) -> User: return user +def _poisoning_hook_by_orm_flush(colliding_user_id: int): + """A hook that leaves a failed ORM flush on the shared session and then + raises. A failed flush marks the session's transaction inactive on + every backend, so any later statement raises ``PendingRollbackError`` + until something rolls back -- which is exactly what the seam's hook + door must do before the exception leaves the module.""" + + def hook(db, *_args, **_kwargs): + db.add( + User(id=colliding_user_id, username="flush-poison-dup", password_hash="x") + ) + db.flush() + + return hook + + +@pytest.mark.parametrize( + "slot,invoke", + [ + ( + "visibility", + lambda db: connector_team_scope.visible_team_connector_ids(db, 1), + ), + ( + "deleted", + lambda db: connector_team_scope.delete_team_connector(db, 1, "mcp", 1), + ), + ( + "renamed", + lambda db: connector_team_scope.rename_team_connector( + db, 1, "mcp", 1, "old", "new" + ), + ), + ], + ids=["visibility-hook", "deleted-hook", "renamed-hook"], +) +def test_every_hook_door_restores_the_session_when_the_hook_fails( + db_session, slot, invoke +): + """These three doors had no session restore before: only the two + ``*_or_raise`` wrappers had one, which covered the access hook and the + team-visibility hook and nothing else. The restore now lives on the + single invocation door, so every slot has it -- including a slot added + to this module later. The two slots this parametrization leaves out are + the two whose answers this seam validates; they are covered by the + sister test below, where the hook does not raise at all.""" + existing = _create_user(db_session, "already-here") + db_session.commit() + + with connector_team_scope.snapshot_connector_team_hooks(): + connector_team_scope.set_connector_team_hooks( + **{slot: _poisoning_hook_by_orm_flush(int(existing.id))} + ) + with pytest.raises(Exception): + invoke(db_session) + + # Without the restore this raises PendingRollbackError instead. + assert db_session.query(User).count() == 1 + + +def _swallowing_poisoning_hook_answering(colliding_user_id: int, answer: object): + """A hook that leaves a failed ORM flush on the shared session, + swallows that failure itself, and then answers with a shape the seam's + own validator rejects. + + The sister of ``_poisoning_hook_by_orm_flush`` above: there the hook + lets its failure propagate, so the door's ``except`` fires on the hook + call. Here nothing propagates out of the hook at all -- the door's + ``except`` fires on the validator's rejection instead, which is the + other half the restore has to cover. + """ + + def hook(db, *_args, **_kwargs): + try: + db.add( + User( + id=colliding_user_id, + username="swallowed-poison-dup", + password_hash="x", + ) + ) + db.flush() + except Exception: + pass + return answer + + return hook + + +@pytest.mark.parametrize( + "slot,answer,invoke", + [ + ( + "team_visibility", + {"mcp": "not-a-set", "custom_api": set()}, + lambda db: connector_team_scope.resolve_team_connector_ids_or_raise( + db, team_id=T1, log_subject=None + ), + ), + ( + "access", + {"not-a-ref": object()}, + lambda db: connector_team_scope.resolve_connector_access_or_raise( + db, 1, [("mcp", 11)] + ), + ), + ], + ids=["team-visibility-hook", "access-hook"], +) +def test_a_hook_that_swallows_its_failure_and_answers_malformed_restores_too( + db_session, slot, answer, invoke +): + """The two slots whose answers this seam validates are the two where + it can notice a hook that poisoned the shared session without ever + raising: the hook runs a statement that fails, catches that itself, + and returns an answer the validator then rejects. A hook can do the + same on the other three slots, where nothing checks the answer and so + nothing raises -- see the door's docstring on the shape that stays + uncovered. The rejection is the seam's own exception, not + the hook's, so the restore has to sit where it sees both -- inside the + door, around the validation as well as around the call.""" + existing = _create_user(db_session, "already-here") + db_session.commit() + + with connector_team_scope.snapshot_connector_team_hooks(): + connector_team_scope.set_connector_team_hooks( + **{slot: _swallowing_poisoning_hook_answering(int(existing.id), answer)} + ) + with pytest.raises(ConnectorRuntimeError) as excinfo: + invoke(db_session) + assert excinfo.value.status_code == 503 + + # No rollback of our own before this line: the query is the statement + # that proves the door restored the session, and its count proves the + # poisoning insert never landed. + assert db_session.query(User).count() == 1 + + def _create_mcp(db: Session, name: str, *, owner: User | None = None) -> MCPServer: server = MCPServer( name=name, @@ -263,22 +810,18 @@ async def test_scope_keys_on_agent_team_not_runner(db_session, seed, owner_team) team_id=_team, is_team_admin=False ) ) - try: - cfg = WebToolConfig( - db=db_session, - request=None, - user_id=int(seed.c.id), - connector_team_id=T1, - include_mcp_tools=True, - ) - configs = await cfg._load_mcp_server_configs() - assert {c["name"] for c in configs} == { - seed.active_own.name, - seed.team_s.name, - } - finally: - connector_team_scope.set_connector_team_hooks() - agent_team_scope.set_agent_team_scope_hook(None) + cfg = WebToolConfig( + db=db_session, + request=None, + user_id=int(seed.c.id), + connector_team_id=T1, + include_mcp_tools=True, + ) + configs = await cfg._load_mcp_server_configs() + assert {c["name"] for c in configs} == { + seed.active_own.name, + seed.team_s.name, + } # --------------------------------------------------------------------------- @@ -303,31 +846,28 @@ async def test_legacy_visibility_hook_alone_is_unchanged(db_session, seed): else {"mcp": set(), "custom_api": set()} ) ) - try: - assert connector_team_scope.team_connector_hook_installed() is False + assert connector_team_scope.team_connector_hook_installed() is False - # The tool loader consults no hook today and must not widen. - cfg = WebToolConfig( - db=db_session, - request=None, - user_id=int(seed.c.id), - connector_team_id=T1, - include_mcp_tools=True, - ) - configs = await cfg._load_mcp_server_configs() - assert {c["name"] for c in configs} == {seed.active_own.name} + # The tool loader consults no hook today and must not widen. + cfg = WebToolConfig( + db=db_session, + request=None, + user_id=int(seed.c.id), + connector_team_id=T1, + include_mcp_tools=True, + ) + configs = await cfg._load_mcp_server_configs() + assert {c["name"] for c in configs} == {seed.active_own.name} - # The runtime-connector loader keeps exactly today's answer via the - # fallback, for both connector kinds. - visible = _load_visible_runtime_connectors( - db_session, user_id=int(seed.c.id), agent_team_id=T1 - ) - mcp_ids = {r.connector_id for r in visible if r.connector_type == "mcp"} - capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} - assert mcp_ids == {int(seed.active_own.id), int(seed.team_s.id)} - assert capi_ids == {int(seed.capi_own.id), int(seed.a_capi.id)} - finally: - connector_team_scope.set_connector_team_hooks() + # The runtime-connector loader keeps exactly today's answer via the + # fallback, for both connector kinds. + visible = _load_visible_runtime_connectors( + db_session, user_id=int(seed.c.id), agent_team_id=T1 + ) + mcp_ids = {r.connector_id for r in visible if r.connector_type == "mcp"} + capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} + assert mcp_ids == {int(seed.active_own.id), int(seed.team_s.id)} + assert capi_ids == {int(seed.capi_own.id), int(seed.a_capi.id)} # --------------------------------------------------------------------------- @@ -338,14 +878,11 @@ async def test_legacy_visibility_hook_alone_is_unchanged(db_session, seed): def test_personal_agent_gets_no_team_custom_api(db_session, seed): connector_team_scope.set_connector_team_hooks(team_visibility=_team_hook(seed)) - try: - visible = _load_visible_runtime_connectors( - db_session, user_id=int(seed.c.id), agent_team_id=None - ) - capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} - assert capi_ids == {int(seed.capi_own.id)} - finally: - connector_team_scope.set_connector_team_hooks() + visible = _load_visible_runtime_connectors( + db_session, user_id=int(seed.c.id), agent_team_id=None + ) + capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} + assert capi_ids == {int(seed.capi_own.id)} # --------------------------------------------------------------------------- @@ -364,16 +901,13 @@ def test_installed_hook_returning_empty_does_not_fall_back(db_session, seed): ), team_visibility=lambda db, *, team_id: {"mcp": set(), "custom_api": set()}, ) - try: - visible = _load_visible_runtime_connectors( - db_session, user_id=int(seed.c.id), agent_team_id=T1 - ) - mcp_ids = {r.connector_id for r in visible if r.connector_type == "mcp"} - capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} - assert mcp_ids == {int(seed.active_own.id)} - assert capi_ids == {int(seed.capi_own.id)} - finally: - connector_team_scope.set_connector_team_hooks() + visible = _load_visible_runtime_connectors( + db_session, user_id=int(seed.c.id), agent_team_id=T1 + ) + mcp_ids = {r.connector_id for r in visible if r.connector_type == "mcp"} + capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} + assert mcp_ids == {int(seed.active_own.id)} + assert capi_ids == {int(seed.capi_own.id)} # --------------------------------------------------------------------------- @@ -401,19 +935,16 @@ def test_installed_hook_with_no_governing_agent_supersedes_legacy_overlay( ), team_visibility=_team_hook(seed), ) - try: - visible = _load_visible_runtime_connectors( - db_session, user_id=int(seed.c.id), agent_team_id=None - ) - mcp_ids = {r.connector_id for r in visible if r.connector_type == "mcp"} - capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} - # Personal-only on both connector kinds: seed.team_s / seed.a_capi - # (the legacy hook's answer) do NOT appear, even though the legacy - # hook alone would have granted them. - assert mcp_ids == {int(seed.active_own.id)} - assert capi_ids == {int(seed.capi_own.id)} - finally: - connector_team_scope.set_connector_team_hooks() + visible = _load_visible_runtime_connectors( + db_session, user_id=int(seed.c.id), agent_team_id=None + ) + mcp_ids = {r.connector_id for r in visible if r.connector_type == "mcp"} + capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} + # Personal-only on both connector kinds: seed.team_s / seed.a_capi + # (the legacy hook's answer) do NOT appear, even though the legacy + # hook alone would have granted them. + assert mcp_ids == {int(seed.active_own.id)} + assert capi_ids == {int(seed.capi_own.id)} # --------------------------------------------------------------------------- @@ -432,18 +963,15 @@ def test_installed_hook_with_no_governing_agent_supersedes_legacy_overlay( def test_new_hook_branch_unions_team_custom_api_too(db_session, seed): connector_team_scope.set_connector_team_hooks(team_visibility=_team_hook(seed)) - try: - visible = _load_visible_runtime_connectors( - db_session, user_id=int(seed.c.id), agent_team_id=T1 - ) - mcp_ids = {r.connector_id for r in visible if r.connector_type == "mcp"} - capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} - # T1's hook (see _team_hook above) grants both seed.team_s (mcp) and - # seed.a_capi (custom_api). Both grants union in now. - assert mcp_ids == {int(seed.active_own.id), int(seed.team_s.id)} - assert capi_ids == {int(seed.capi_own.id), int(seed.a_capi.id)} - finally: - connector_team_scope.set_connector_team_hooks() + visible = _load_visible_runtime_connectors( + db_session, user_id=int(seed.c.id), agent_team_id=T1 + ) + mcp_ids = {r.connector_id for r in visible if r.connector_type == "mcp"} + capi_ids = {r.connector_id for r in visible if r.connector_type == "custom_api"} + # T1's hook (see _team_hook above) grants both seed.team_s (mcp) and + # seed.a_capi (custom_api). Both grants union in now. + assert mcp_ids == {int(seed.active_own.id), int(seed.team_s.id)} + assert capi_ids == {int(seed.capi_own.id), int(seed.a_capi.id)} # --------------------------------------------------------------------------- @@ -498,11 +1026,8 @@ def test_team_connector_ids_raises_on_malformed_hook_answer(malformed_answer): connector_team_scope.set_connector_team_hooks( team_visibility=lambda db, *, team_id: malformed_answer ) - try: - with pytest.raises(ValueError): - connector_team_scope.team_connector_ids(None, team_id=T1) - finally: - connector_team_scope.set_connector_team_hooks() + with pytest.raises(ValueError): + connector_team_scope.team_connector_ids(None, team_id=T1) def test_team_connector_ids_accepts_and_ignores_extra_keys(): @@ -516,12 +1041,9 @@ def test_team_connector_ids_accepts_and_ignores_extra_keys(): "unexpected_extra_key": object(), } ) - try: - result = connector_team_scope.team_connector_ids(None, team_id=T1) - assert result["mcp"] == {1, 2} - assert result["custom_api"] == {3} - finally: - connector_team_scope.set_connector_team_hooks() + result = connector_team_scope.team_connector_ids(None, team_id=T1) + assert result["mcp"] == {1, 2} + assert result["custom_api"] == {3} @pytest.mark.asyncio @@ -533,21 +1055,18 @@ async def test_mcp_loader_seam_retypes_malformed_hook_answer(db_session, seed): connector_team_scope.set_connector_team_hooks( team_visibility=lambda db, *, team_id: {"mcp": "12", "custom_api": set()} ) - try: - cfg = WebToolConfig( - db=db_session, - request=None, - user_id=int(seed.c.id), - connector_team_id=T1, - include_mcp_tools=True, - ) - with pytest.raises(ConnectorRuntimeError) as excinfo: - await cfg._load_mcp_server_configs() - assert excinfo.value.status_code == 503 - assert excinfo.value.details["reason"] == "team_scope_resolution_failed" - assert isinstance(excinfo.value.__cause__, ValueError) - finally: - connector_team_scope.set_connector_team_hooks() + cfg = WebToolConfig( + db=db_session, + request=None, + user_id=int(seed.c.id), + connector_team_id=T1, + include_mcp_tools=True, + ) + with pytest.raises(ConnectorRuntimeError) as excinfo: + await cfg._load_mcp_server_configs() + assert excinfo.value.status_code == 503 + assert excinfo.value.details["reason"] == "team_scope_resolution_failed" + assert isinstance(excinfo.value.__cause__, ValueError) def test_runtime_view_seam_retypes_malformed_hook_answer(db_session, seed): @@ -566,19 +1085,16 @@ def test_runtime_view_seam_retypes_malformed_hook_answer(db_session, seed): connector_team_scope.set_connector_team_hooks( team_visibility=lambda db, *, team_id: {"mcp": "12", "custom_api": set()} ) - try: - with pytest.raises(ConnectorRuntimeError) as excinfo: - _load_custom_api_runtime_view_sync( - db_session, - task_id=str(task.id), - connector_runtime_turn_id=None, - user_id=int(seed.c.id), - agent_team_id=T1, - ) - assert excinfo.value.status_code == 503 - assert isinstance(excinfo.value.__cause__, ValueError) - finally: - connector_team_scope.set_connector_team_hooks() + with pytest.raises(ConnectorRuntimeError) as excinfo: + _load_custom_api_runtime_view_sync( + db_session, + task_id=str(task.id), + connector_runtime_turn_id=None, + user_id=int(seed.c.id), + agent_team_id=T1, + ) + assert excinfo.value.status_code == 503 + assert isinstance(excinfo.value.__cause__, ValueError) def test_resolve_or_raise_passes_a_typed_error_through_unchanged(): @@ -597,12 +1113,47 @@ def _raising_hook(db, *, team_id): raise planted connector_team_scope.set_connector_team_hooks(team_visibility=_raising_hook) - try: - with pytest.raises(ConnectorRuntimeError) as excinfo: - connector_team_scope.resolve_team_connector_ids_or_raise( - None, team_id=T1, log_subject="passthrough-probe" - ) - assert excinfo.value is planted - assert excinfo.value.details["reason"] == "planted_inner_reason" - finally: - connector_team_scope.set_connector_team_hooks() + with pytest.raises(ConnectorRuntimeError) as excinfo: + connector_team_scope.resolve_team_connector_ids_or_raise( + None, team_id=T1, log_subject="passthrough-probe" + ) + assert excinfo.value is planted + assert excinfo.value.details["reason"] == "planted_inner_reason" + + +# --------------------------------------------------------------------------- +# The team-visibility wrapper restores the shared session after a failed +# hook too -- the sister guarantee to resolve_connector_access_or_raise's, +# on the sister wrapper. +# --------------------------------------------------------------------------- + + +def test_the_team_scope_wrapper_also_restores_the_session(db_session): + """A hook that poisons the shared session via a failed ORM flush, then + lets that failure propagate, must not leave the session unusable for + whatever runs next in the same request.""" + poisoning_user_id = 900001 + db_session.add( + User(id=poisoning_user_id, username="team-scope-poison", password_hash="x") + ) + db_session.commit() + + def poisoning_team_visibility(db, *, team_id): + # A duplicate primary key -- a real ORM flush failure, not a + # simulated one -- propagates out of this hook uncaught. + db.add(User(id=poisoning_user_id, username="dup", password_hash="x")) + db.flush() + return {"mcp": set(), "custom_api": set()} # pragma: no cover - unreachable + + connector_team_scope.set_connector_team_hooks( + team_visibility=poisoning_team_visibility + ) + with pytest.raises(ConnectorRuntimeError) as excinfo: + connector_team_scope.resolve_team_connector_ids_or_raise( + db_session, team_id=T1, log_subject=None + ) + assert excinfo.value.status_code == 503 + + # The session must be usable again immediately afterward. + result = db_session.execute(select(1)).scalar() + assert result == 1 diff --git a/tests/web/test_team_sharing_hooks.py b/tests/web/test_team_sharing_hooks.py index 84d66dbf68..f089bef950 100644 --- a/tests/web/test_team_sharing_hooks.py +++ b/tests/web/test_team_sharing_hooks.py @@ -25,6 +25,7 @@ def test_agent_team_hooks_install_as_one_group(): def test_connector_team_hooks_delegate_and_reset(): deleted_calls = [] renamed_calls = [] + access_calls = [] connector_scope.set_connector_team_hooks( visibility=lambda db, user_id: {"mcp": {11}, "custom_api": {22}}, @@ -38,6 +39,13 @@ def test_connector_team_hooks_delegate_and_reset(): renamed=lambda db, user_id, kind, connector_id, old, new: renamed_calls.append( (db, user_id, kind, connector_id, old, new) ), + access=lambda db, user_id, refs: ( + access_calls.append((db, user_id, refs)) + or { + ref: connector_scope.ConnectorAccess(team_owned=True, can_edit=True) + for ref in refs + } + ), ) try: assert connector_scope.visible_team_connector_ids(None, 7) == { @@ -51,11 +59,17 @@ def test_connector_team_hooks_delegate_and_reset(): decision = connector_scope.delete_team_connector(None, 7, "mcp", 11) assert decision.team_owned and decision.authorized connector_scope.rename_team_connector(None, 7, "mcp", 11, "old", "new") + access = connector_scope.resolve_connector_access(None, 7, [("mcp", 11)]) + assert access == { + ("mcp", 11): connector_scope.ConnectorAccess(team_owned=True, can_edit=True) + } assert deleted_calls == [(None, 7, "mcp", 11)] assert renamed_calls == [(None, 7, "mcp", 11, "old", "new")] + assert access_calls == [(None, 7, frozenset({("mcp", 11)}))] finally: connector_scope.set_connector_team_hooks() assert connector_scope.team_connector_hook_installed() is False + assert connector_scope.resolve_connector_access(None, 7, [("mcp", 11)]) == {} def test_knowledge_base_team_hooks_delegate_with_none_session():