diff --git a/src/xagent/web/api/mcp.py b/src/xagent/web/api/mcp.py index dfb325ab35..d9e6f18507 100644 --- a/src/xagent/web/api/mcp.py +++ b/src/xagent/web/api/mcp.py @@ -14,10 +14,21 @@ 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 enum import Enum -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 from uuid import UUID @@ -31,6 +42,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 @@ -83,6 +95,9 @@ normalize_user_oauth_resource_owner_key, ) +if TYPE_CHECKING: + from ..services.connector_team_scope import ConnectorAccess + logger = logging.getLogger(__name__) MCP_OAUTH_STATE_COOKIE = "xagent_mcp_oauth_state" @@ -1929,11 +1944,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 @@ -1943,7 +1972,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 @@ -2001,8 +2032,25 @@ 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_active`` and ``is_default`` are not read off any row -- there is + none -- and report a fixed placeholder instead: this user has no personal + activation state or default choice for a connector they never connected, + so there is no real value to report. A response built off this stand-in + (``update_mcp_server``'s post-lock cascade can construct one for a + caller whose personal row was deleted while it waited for the definition + lock, when the re-resolved verdict still authorizes the edit) reports + ``is_active=True`` and ``is_default=False`` for such a caller regardless + of what, if anything, their connection looked like before it was + removed. ``__slots__`` declares only ``user_id`` as a real per-instance attribute. Every other name below is a class attribute, not a slot, so assigning to @@ -2030,6 +2078,12 @@ def __init__(self, user_id: int) -> None: class _TeamOwnedUserApi: """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, so reading the attributes alone never + grants anything. This module only ever reads them to build a response; + the Custom API routes that act on them live in their own module and + resolve their own rows. + Same reasoning as ``_TeamOwnedUserMCP`` above: ``__slots__`` leaves ``user_id`` as the only attribute an instance can hold, so a write to ``can_edit``, ``is_active`` or ``is_default`` raises ``AttributeError`` @@ -2038,6 +2092,7 @@ class _TeamOwnedUserApi: __slots__ = ("user_id",) + is_owner = False can_edit = False is_active = True is_default = False @@ -2046,6 +2101,122 @@ 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. It is the CALLER's population, not the caller's HTTP method, that + settles it. ``"degrade"`` reports ``can_edit_global=False`` and lets the + request proceed; both routes pass it, because on both of them a caller + who reached this point holding a personal row was admitted by that row + and not by the verdict -- an owner and a platform administrator each + decide the edit branch before a verdict is read at all, and a non-owner + member writing only their own association fields writes nothing the + verdict governs. Refusing them because an optional integration is down + would make an outage of that integration the answer to a request whose + authority never came from it. + + That choice never reaches the population the verdict IS the gate for. + The degrade branch below still raises when ``user_mcp is None``, + unconditionally: with no personal row, degrading the verdict to ``None`` + would answer "does not exist" for a connector this call merely failed to + ask about. ``"raise"`` stays the default so a future call site that has + not made this decision fails closed rather than degrading by accident. + """ + 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, user_id, ("mcp", int(server.id)) + ) + except ConnectorRuntimeError as exc: + # 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 (%s) for MCP server %s " + "while resolving it for user %s; reporting " + "can_edit_global=False", + exc, + server_id, + 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, @@ -2054,8 +2225,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() @@ -2086,7 +2270,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), @@ -2181,7 +2367,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 @@ -2209,8 +2396,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) @@ -2230,6 +2419,139 @@ 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 callers of this function need 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 _definition_row_has_an_owner(db: Session, server: MCPServer) -> bool: + """Whether any user's association row claims ``is_owner`` on this row. + + Two provisioning paths write ``is_owner=True``: ``create_mcp_server`` + for a connector a user built themselves, and the builtin-OAuth connect in + ``auth.py`` for the row it provisions. Three call sites write + ``is_owner=False`` instead -- the two in this module and the + builtin-OAuth visibility path in ``mcp_apps.py`` -- and the catalog + provisioning helpers create the shared row with no association at all -- + which is why ``_reject_user_owned_catalog_squat`` can refuse to adopt an + owned row as a catalog row. So "no owner" is the shape of a + platform-provisioned row, and of a row whose creator's account has since + been deleted (association rows cascade with the user), and of nothing a + team built. + + Same query as ``_reject_user_owned_catalog_squat`` asks on the connect + paths, deliberately written out a second time rather than shared: that + function is on the connect path, which this change does not otherwise + touch and which carries no regression coverage of its own. + """ + return ( + db.query(UserMCPServer) + .filter( + UserMCPServer.mcpserver_id == server.id, + UserMCPServer.is_owner.is_(True), + ) + .first() + is not None + ) + + +def _team_access_for_shared_row( + db: Session, + server: MCPServer, + access: "ConnectorAccess | 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 the largest auth kind in the built-in catalog. + - ``_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. + + A second, independent reason to withhold the edit: the row has no owner + at all. A catalog app's shared row is provisioned without any association + (``_reject_user_owned_catalog_squat``'s docstring states this), and both + catalog-connect paths write ``is_owner=False``, so a platform row stays + ownerless for its whole life -- while every connector a user built + themselves has an ``is_owner=True`` row from the moment it is created. + The catalog-key test above cannot see a platform row an administrator + renamed away from its key, because for a non-``oauth`` row that key IS + the current name and this same route can change it; the ownership test + does not depend on any field this route can write. The two are + complementary, not redundant: a renamed builtin-OAuth row is still + matched by its ``auth.app_id`` and does carry an owner, while a renamed + key-based, keyless or remote-MCP-OAuth row is caught only by the + ownership test. + + What this deliberately also withholds: a connector whose creator's + account was deleted. Association rows cascade with the user, so such a + row becomes ownerless and stops being team-editable. That is the + conservative direction -- a wrong answer here refuses an edit rather than + granting one -- and it is the side the ambiguity is resolved on until the + schema carries a durable "this row came from the catalog" fact. + + Both tests run only for a verdict that already grants edit -- the one + case where either can change an answer -- so a deployment with no access + hook installed resolves ``None`` for every row and issues no additional + query at all. Ordering matters for the same reason: the ownership lookup + sits after the catalog test, so a row the catalog already claims costs + nothing extra. + """ + if access is None or not access.can_edit: + return access + if _catalog_reserved_keys(db).intersection(_server_catalog_keys(server)): + return replace(access, can_edit=False) + if not _definition_row_has_an_owner(db, server): + return replace(access, can_edit=False) + return access + + def _oauth_account_can_connect(oauth_account: object) -> bool: access_token = getattr(oauth_account, "access_token", None) if not access_token: @@ -3183,21 +3505,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, @@ -3222,10 +3538,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( @@ -3956,6 +4277,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) ) @@ -3983,23 +4307,80 @@ 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() - ) - - if not result: + # Check user has access to this server: a personal row, or a team + # access verdict for a connector the caller has none for. + # + # A caller who already holds a personal row got past the gate on that + # row, not on the verdict: an owner and a platform administrator each + # decide the edit branch without one being read at all, and a plain + # non-owner member writing only their own fields writes nothing the + # verdict governs. For all of them a resolution failure degrades to + # can_edit_global=False and the write proceeds. A caller with NO + # personal row is the population the verdict is the gate for, and the + # helper keeps failing closed for it regardless of what is passed + # here. + user_mcp, server, team_access = _resolve_mcp_server_for_request( + db, int(user_id), server_id, on_resolution_failure="degrade" + ) + 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, + ) + + # 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. + # + # This is the gate-side half of the guard; a caller who still has a + # personal row here can lose it during the wait for the definition + # lock, so a second copy runs again below, after that wait, on the + # row a fresh read then finds. + # + # Presence, not value: ``MCPServerUpdate`` accepts an explicit null + # for both fields and Pydantic records it in ``model_fields_set``, so + # ``{"user_env": null}`` carries the field and must be refused the + # same as any other value would be. Testing the value instead let + # that payload through to a 200 that stored nothing -- and, mixed + # with a shared field, to a 200 that applied the shared half while + # silently dropping the personal one. This is the same test + # ``writes_definition_row`` below makes, and the same one + # ``custom_api.py``'s own stand-in guard makes. + if is_stand_in and ( + "user_env" in server_data.model_fields_set + or "is_active" in server_data.model_fields_set + ): raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, detail="MCP server not found" + status_code=status.HTTP_400_BAD_REQUEST, + detail=( + "No personal connection exists to configure user_env or " + "is_active for this server" + ), ) - user_mcp, server = result - can_edit_global = _check_mcp_permission( - user_mcp, getattr(current_user, "is_admin", False), require="edit" - ) + # 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: for a payload carrying + # only ``user_env``/``is_active``, "there is no personal connection + # to configure this on" is the more precise answer than "you may not + # edit this server", so that payload keeps its 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", + ) # Which row a request writes decides which row it locks. Seven of the # nine fields of ``MCPServerUpdate`` target the shared ``MCPServer`` @@ -4034,16 +4415,25 @@ def update_mcp_server( # One flag, three decisions, and they are the same decision on # purpose: whether to take the row lock (the ``with_for_update`` # below), whether to re-derive the caller's write authority after - # that wait (the block right after the lock), and whether to - # rebuild, validate and write the definition row (the block - # further down). Moving a field into the exclusion set above - # therefore also drops that payload's post-lock re-authorization -- - # the hazard ``custom_api.py``'s equivalent comment warns about -- - # and drops its runtime-config validation with it. Change that set - # only with all three in view; + # that wait (the block right after the lock, which re-reads the + # caller's link row, the caller's admin flag and the caller's team + # access verdict), and whether to rebuild, validate and write the + # definition row (the block further down). Moving a field into the + # exclusion set above therefore also drops that payload's post-lock + # re-authorization -- the hazard ``custom_api.py``'s equivalent + # comment warns about -- and drops its runtime-config validation with + # it. Change that set only with all three in view; # ``tests/web/api/test_mcp_update_lock_partition.py`` fails on a # field added to ``MCPServerUpdate`` without that decision. # + # The exclusion set names exactly the two fields that write the + # caller's own association row and nothing shared, so a payload it + # excludes has no shared write for any of the three to protect. That + # makes ``writes_definition_row`` this route's only spelling of "the + # payload touches something shared": no other guard here re-tests the + # payload's field set, and a second such test would only drift from + # this one. + # # This set is also exactly the set of payloads the block below # rebuilds and writes the definition row for: the whole rebuild -- # building ``update_data``, validating it, and writing every @@ -4120,44 +4510,53 @@ def update_mcp_server( server = current_server if writes_definition_row: - # The join gate above ran before the lock statement, and the - # lock statement waits. Both inputs to ``can_edit_global`` -- - # that the caller still has a ``UserMCPServer`` link to this - # server (link ownership), and whether the caller is a platform - # admin -- were read from that pre-wait state: the gate's join - # for the link, ``current_user`` (built once by the auth - # dependency before this route even started) for admin status. - # A supported admin user deletion removes association rows and - # leaves every definition row standing; a platform admin's own - # admin flag can itself be revoked by another admin; an MCP - # disconnect removes the caller's own link while another user's - # link keeps the definition alive. Any of these can commit - # inside the wait. The request would then write and commit the - # shared definition row on a revoked authority, and fail only - # afterwards, in ``db.refresh(user_mcp)`` below -- which runs - # after the commit, so the generic handler's rollback cannot - # take the shared write back and the caller sees a 500 over a - # durable change. + # The gate above ran before the lock statement, and the lock + # statement waits. All three inputs to ``can_edit_global`` -- + # whether the caller still has a ``UserMCPServer`` link to this + # server (link ownership), whether the caller is a platform + # admin, and the caller's team access verdict for this + # connector -- were read from that pre-wait state: the gate's + # join for the link, ``current_user`` (built once by the auth + # dependency before this route even started) for admin status, + # and the gate's own hook call for the verdict. A supported + # admin user deletion removes association rows and leaves every + # definition row standing; a platform admin's own admin flag can + # itself be revoked by another admin; an MCP disconnect removes + # the caller's own link while another user's link keeps the + # definition alive; and the application that answers the verdict + # can revoke the team's link at any moment, writing its own + # tables, which this lock does not cover. Any of these can + # commit inside the wait. The request would then write and + # commit the shared definition row on a revoked authority, and + # fail only afterwards, in ``db.refresh(user_mcp)`` below -- + # which runs after the commit, so the generic handler's rollback + # cannot take the shared write back and the caller sees a 500 + # over a durable change. # # ``populate_existing()`` on the definition query above - # refreshes that statement's row and nothing else, so both - # inputs need their own fresh single-table reads here -- the - # gate above is a two-table join and cannot address either - # table alone. The link read below replaces ``user_mcp``; the - # admin read further below replaces the value passed into - # ``_check_mcp_permission``. Together they re-derive - # ``can_edit_global`` for the rest of the route: the per-user - # env write, the activation write, the refresh and the response - # all read ``user_mcp``, and the owner-only guard below reads - # ``can_edit_global``. + # refreshes that statement's row and nothing else, so all three + # inputs need their own fresh reads here -- the gate above is a + # two-table join and cannot address either table alone. The link + # read below replaces ``user_mcp``; the admin read after it + # replaces the value passed into ``_check_mcp_permission``; the + # verdict re-resolve after that replaces ``team_access``. + # Together they re-derive ``can_edit_global`` for the rest of + # the route: the per-user env write, the activation write, the + # refresh and the response all read ``user_mcp``, and the + # owner-only guard below reads ``can_edit_global``. # - # A gone link is this route's existing 404, matching the gate. - # A link that is still there but no longer owns the server is - # not an error by itself here -- the gate does not refuse a - # non-owner either -- so it is answered exactly as the gate - # would have answered it: the owner-only guard below rejects a - # payload that changes the shared configuration and drops one - # that does not. + # This whole block is skipped for a payload that writes nothing + # but the caller's own association row, because none of these + # re-reads can change what such a payload is allowed to do. + # + # A gone link no longer 404s by itself here: what a caller with + # no personal row may still do is the re-resolved verdict's + # answer, asked by the cascade below. A link that is still there + # but no longer owns the server is not an error either -- the + # gate does not refuse a non-owner -- so both cases are answered + # by that same cascade, exactly as the gate would have answered + # them: the owner-only guard further down rejects a payload that + # changes the shared configuration and drops one that does not. current_user_mcp = ( db.query(UserMCPServer) .filter( @@ -4167,18 +4566,26 @@ def update_mcp_server( .populate_existing() .first() ) - if current_user_mcp is None: - # Same reasoning as the definition-row 404 above: this read - # ran inside the transaction that held the lock, so an - # explicit rollback keeps this 404 from leaving that - # transaction open for the outer ``except HTTPException`` - # handler to skip past. - db.rollback() - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="MCP server not found", - ) - user_mcp = current_user_mcp + # ``user_mcp`` is re-pointed in both directions, because the lock + # wait can move a caller either way: one who reached the gate on + # their team's verdict may have acquired a personal row, and one + # who reached it on a personal row may have had it deleted. + # + # Nothing is refused here. Whether a caller with no personal row + # may still write is the verdict's answer, and it is asked below; + # refusing first would 404 a team editor whose team does authorise + # this edit. What must not survive this point is the gate's ORM + # object: continuing to hold a row another session has deleted + # raises ``StaleDataError`` at commit for a payload that writes it, + # and ``ObjectDeletedError`` while building the response for one + # that does not -- and the second of those fails *after* the + # definition-row write has already committed. + if current_user_mcp is not None: + user_mcp = current_user_mcp + is_stand_in = False + else: + user_mcp = _TeamOwnedUserMCP(int(user_id)) + is_stand_in = True # ``current_user.is_admin`` is the value the auth dependency's # own read fixed before this route's wait for the lock, same as # ``user_mcp``'s pre-lock read above -- and admin status is @@ -4202,10 +4609,85 @@ def update_mcp_server( status_code=status.HTTP_404_NOT_FOUND, detail="Requesting user account no longer exists", ) + is_admin_now = bool(current_admin_user.is_admin) + + # Without a post-lock re-ask, a caller whose personal row vanished + # during the wait has only the gate's verdict to fall back on. + # None means the gate never found one either, which is the gate's + # own 404 condition -- answer it the same way it would have. + if is_stand_in and team_access is None: + db.rollback() + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="MCP server not found", + ) + + # The lock-side counterpart of the guard above the lock: that one + # runs before the wait and cannot see a personal row deleted + # during it. Same wording on purpose -- it is the same refusal, + # discovered later. + # + # Ordered after the authorization branches above, which mirrors + # the gate's own order: the gate answers 404 for a caller with + # neither a personal row nor a verdict (in + # ``_resolve_mcp_server_for_request``) before it ever reaches its + # own personal-field 400. + # + # Same presence test as the guard above, for the same reason. + if is_stand_in and ( + "user_env" in server_data.model_fields_set + or "is_active" in server_data.model_fields_set + ): + db.rollback() + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=( + "No personal connection exists to configure user_env or " + "is_active for this server" + ), + ) + can_edit_global = _check_mcp_permission( - user_mcp, bool(current_admin_user.is_admin), require="edit" + user_mcp, + is_admin_now, + require="edit", + team_access=team_access, ) + # The lock-side counterpart of the stand-in refusal above the + # lock. That one runs before the wait and cannot see a personal + # row deleted during it: a caller admitted on a real non-owner + # row, holding a verdict that links the connector but denies the + # edit, reaches this point as a stand-in whose re-derived answer + # is still "no". Without this, a payload the tamper check cannot + # compare -- an unchanged one, or one carrying only secrets -- + # would go on to rebuild and commit the definition row, which + # normalizes any shared column still holding a legacy NULL, and + # answer 200. + # + # 404 rather than the 403 the gate answers: this caller holds no + # row on this connector any more, and "not found" discloses less + # about a connector they no longer have any link to. It is also + # the answer ``custom_api.py``'s own post-lock cascade gives the + # same state, and the answer the follow-up's re-resolution gives + # it once that lands, so the three agree rather than drifting. + # + # Ordered after the personal-field guard above, mirroring the + # gate's own order: for a payload carrying only ``user_env`` or + # ``is_active``, "there is no personal connection to configure + # this on" is the more precise answer. + # + # Placed before the name read below and before the tamper check, + # the rebuild, the rename hook and the commit, so the refusal has + # nothing to undo; the rollback ends the transaction and releases + # the definition row's lock. + if is_stand_in and not can_edit_global: + db.rollback() + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="MCP server not found", + ) + # Read from the fresh definition-row read above, not the pre-lock # read further up: rename_team_connector's "old" argument must be # the name that read actually returned. On the path that writes @@ -4336,7 +4818,13 @@ 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 + # ``Any`` rather than ``UserMCPServer``: what mypy rejects is + # assigning through a ``Column[...]``-typed attribute, so a + # concrete cast fails here with "expression has type + # ``dict[Any, Any] | None``, variable has type ``Column[Any]``". + # Four sibling call sites in this module take the same escape + # hatch for the same reason. + 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: @@ -4359,15 +4847,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}") diff --git a/src/xagent/web/services/connector_team_scope.py b/src/xagent/web/services/connector_team_scope.py index 5150e5d51f..7a74a96bb5 100644 --- a/src/xagent/web/services/connector_team_scope.py +++ b/src/xagent/web/services/connector_team_scope.py @@ -49,10 +49,11 @@ | ``custom_api.update_custom_api`` | the ``custom_apis`` definition row, ``FOR UPDATE``, on the payloads that write that row | no | ``True`` | | ``custom_api._recheck_team_access_under_definition_lock`` | the ``custom_apis`` definition row, ``FOR UPDATE``, taken by ``update_custom_api`` before this call | no | ``True`` | | ``custom_api.delete_custom_api`` | the ``custom_apis`` definition row, ``FOR UPDATE`` | no | ``True`` | +| ``custom_api._resolve_custom_api_for_request`` | nothing -- this resolution runs before either of its two routes takes any lock | no | ``False`` | | ``mcp.update_mcp_server`` | the ``mcp_servers`` definition row, ``FOR UPDATE ... KEY SHARE``, on the payloads that write that row | no | ``True`` | +| ``mcp._resolve_mcp_server_for_request`` | nothing; this is the gate both ``GET`` and ``PUT`` run before any lock exists | no | ``False`` | | ``mcp._teardown_mcp_app_server_locally`` | three row locks: ``public_mcp_apps``, ``mcp_servers``, ``user_mcpservers`` | no, within this function -- see the note below | ``True`` | | ``mcp.delete_mcp_server`` | two row locks: ``mcp_servers`` and ``user_mcpservers``, taken by ``_lock_active_mcp_oauth_lifecycle`` before this call | no | ``True`` | -| ``custom_api._resolve_custom_api_for_request`` | nothing -- this resolution runs before either of its two routes takes any lock | no | ``False`` | ``mcp._teardown_mcp_app_server_locally`` is a helper, not a route: it has no route decorator, and its only caller in this repository outside tests is the diff --git a/tests/web/api/test_connector_seam_off_event_loop.py b/tests/web/api/test_connector_seam_off_event_loop.py index 40c249a7f9..5e8acd7cc5 100644 --- a/tests/web/api/test_connector_seam_off_event_loop.py +++ b/tests/web/api/test_connector_seam_off_event_loop.py @@ -60,6 +60,7 @@ "xagent.web.api.custom_api.update_custom_api", "xagent.web.api.custom_api.delete_custom_api", "xagent.web.api.mcp._local_mcp_can_attach", + "xagent.web.api.mcp._resolve_mcp_server_for_request", # The coroutine that owns app-scoped teardown, ``teardown_mcp_app_server``, # is absent on purpose: it hands this helper to ``asyncio.to_thread`` # instead of calling it, so the seam runs in a worker thread and the @@ -70,6 +71,7 @@ # the offender list. "xagent.web.api.mcp._teardown_mcp_app_server_locally", "xagent.web.api.mcp.delete_mcp_server", + "xagent.web.api.mcp.get_mcp_server", "xagent.web.api.mcp.get_mcp_servers", "xagent.web.api.mcp.list_mcp_apps", "xagent.web.api.mcp.update_mcp_server", @@ -205,7 +207,9 @@ def _functions_reaching_the_connector_seam() -> dict[str, ast.AST]: module's top level -- then closed transitively over plain-name calls to another function in the same module already in the reaching set, so that a route reaching the seam only through a local helper is enumerated as - well. A method defined in a class body is kept, keyed as + well: on the MCP side, ``get_mcp_server`` reaches the seam only through + ``_resolve_mcp_server_for_request``, and a seed-only check would miss it. + A method defined in a class body is kept, keyed as ``module.Class.method``, rather than dropped, since these modules already have ``async def`` methods. """ 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..6835516660 --- /dev/null +++ b/tests/web/api/test_mcp_reported_edit_permission.py @@ -0,0 +1,450 @@ +"""The single-server ``GET``/``PUT`` gate surfaces a raising hook's declared +status rather than a 500, and restores the session afterward. A ``GET`` +whose verdict resolution fails degrades ``can_edit_global`` to False rather +than failing the read; a ``PUT`` fails closed with a typed 503 only for the +caller the verdict is the gate for -- one with no personal association row; +a caller who already holds one degrades the same way ``GET`` does. + +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 +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.mcp import ( + MCPOAuthConnectRequest, + MCPOAuthDiscoverRequest, + MCPServerUpdate, + connect_mcp_oauth, + delete_mcp_oauth_grant, + discover_mcp_oauth, + get_mcp_oauth_status, + get_mcp_server, + update_mcp_server, +) +from xagent.web.models.agent import Agent +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 ( + 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 _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 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 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, the routes this work touches behave + exactly as they did before any of it started, 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, is covered + separately below, against the single-server routes. + """ + + def test_a_complete_stranger_still_gets_404_with_no_hook(self, db): + """A caller with neither a personal row nor any team link cannot be + constructed in the matrix above, which only builds callers that do + have a personal row. The pre-change 404 for that caller is worth + pinning on its own.""" + 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 + + 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 + + +def poison_by_orm_flush(db, *, colliding_user_id): + """Poison the session by flushing a row that violates a real unique + constraint -- 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_orm_flush] +POISON_SHAPE_IDS = ["orm-flush"] + + +class TestSessionRecoveryAfterHookFailure: + """A hook that leaves a failed statement on the shared session must not + turn a route that would otherwise succeed 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). + """ + + 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 TestSingleServerAccessResolutionFailure: + """A single MCP server's verdict plays two different roles depending on + the caller's population, on both ``GET`` and ``PUT``: for a caller who + already holds a personal row -- an owner, a platform administrator, or a + non-owner member writing only their own association fields -- the + verdict is decoration on something the route can already answer without + it, so a resolution failure there degrades ``can_edit_global`` to False + and the request still succeeds. For a caller with no personal row the + verdict *is* the gate, 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_degrades_for_a_caller_who_holds_a_personal_row( + self, db + ): + owner = _make_user(db, 106) + member = _make_user(db, 107) + member_id = member.id + 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) + response = update_mcp_server( + server_id, + MCPServerUpdate(is_active=False), + current_user=member, + db=db, + ) + + # This caller was admitted by their own association row, not by the + # verdict: they are a non-owner writing only a field that lives on + # that row, so no verdict governs the write. An outage of the + # optional team lookup therefore degrades the reported edit right + # and lets the write land, rather than becoming the answer. + assert response.can_edit_global is False + + db.commit() + stored = ( + db.query(UserMCPServer) + .filter( + UserMCPServer.user_id == member_id, + UserMCPServer.mcpserver_id == server_id, + ) + .one() + ) + assert stored.is_active is False 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..879b4e2c0a --- /dev/null +++ b/tests/web/api/test_mcp_team_connector_edit.py @@ -0,0 +1,1648 @@ +"""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 a personal row that vanishes while this +request waits for the definition row's lock is answered with the gate's +own 404 -- an admin with neither a personal row nor a verdict included, +though an admin who does hold a verdict is a separate case this module +does not cover. + +Every test that installs the access hook does so 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 ast +import importlib +import inspect + +import pytest +import sqlalchemy as sa +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.mcp import ( + MCPServerUpdate, + _check_mcp_permission, + get_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: + """The team access verdict as a fallback on the ``edit`` branch. + + Covers only the verdict-aware behavior; the owner/admin/delete + behavior this function has always had is covered by + ``test_check_mcp_permission`` in test_mcp_api.py.""" + + 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" + + # Durability, not staging: the rollback below is what makes this a + # real check -- 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" + + # 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): + """Durability and no-fabricated-row, for the population the other + tests in this class do not cover: a caller who does have a personal + row, but one that grants no edit, widened by a granting team + verdict. Every other test here 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 + + # Durability, not staging. + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.description == "widened-by-the-team" + # 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): + """A rename by a team editor must rewrite the team's agent + selectors, exactly as an owner's rename does. Mutation check: + deleting the ``rename_team_connector`` call turns 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 TestAnExplicitNullPersonalFieldIsRefused: + """``MCPServerUpdate`` accepts an explicit ``null`` for ``user_env`` and + ``is_active`` the same as it accepts any other value, and Pydantic + records that in ``model_fields_set`` -- so ``{"user_env": null}`` carries + the field and must be refused the same way any other value is, on both + sides of the definition-row lock. Testing the value instead of presence + let such a payload through to a 200 that stored nothing, and -- mixed + with a shared field -- to a 200 that applied the shared half while + silently dropping the personal one. + """ + + @pytest.mark.parametrize("field", ["user_env", "is_active"]) + def test_an_explicit_null_personal_field_is_refused(self, db, field): + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="explicit-null-pre-lock") + server_id = server.id + payload = MCPServerUpdate.model_validate({field: None}) + + 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, payload, current_user=member, db=db) + + assert exc.value.status_code == 400 + assert exc.value.detail == ( + "No personal connection exists to configure user_env or " + "is_active for this server" + ) + + @pytest.mark.parametrize("field", ["user_env", "is_active"]) + def test_an_explicit_null_personal_field_is_refused_after_the_lock_too( + self, db, field + ): + owner = _make_user(db, 1) + member = _make_user(db, 2) + member_id = member.id + server = _make_owned_server(db, owner.id, name="explicit-null-post-lock") + server.description = "original" + server_id = server.id + db.add( + UserMCPServer( + user_id=member_id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + payload = MCPServerUpdate.model_validate( + {field: None, "description": "should-not-land"} + ) + + real_query = db.query + fired = False + + def query_and_delete_on_first_recheck(*entities, **kwargs): + nonlocal fired + if entities == (UserMCPServer,) and not fired: + fired = True + db.execute( + sa.delete(UserMCPServer).where( + UserMCPServer.user_id == member_id, + UserMCPServer.mcpserver_id == server_id, + ) + ) + db.commit() + return real_query(*entities, **kwargs) + + db.query = query_and_delete_on_first_recheck + try: + 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, payload, current_user=member, db=db) + finally: + db.query = real_query + + assert fired + assert exc.value.status_code == 400 + assert exc.value.detail == ( + "No personal connection exists to configure user_env or " + "is_active for this server" + ) + + db.rollback() + refreshed = real_query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.description == "original" + + def test_a_denied_stand_in_with_an_explicit_null_still_gets_the_personal_field_400( + self, db + ): + """The lock-side personal-field 400 fires before the lock-side + denied-stand-in 404 does, even for a caller whose re-derived verdict + denies edit outright. Both guards would refuse this request, so only + running the mutation that swaps their order (moving the 404 ahead of + the 400) can tell whether the ordering the docstring above the 404 + guard promises is real rather than untested.""" + owner = _make_user(db, 1) + member = _make_user(db, 2) + member_id = member.id + server = _make_owned_server(db, owner.id, name="explicit-null-denied-post-lock") + server.description = "original" + server_id = server.id + db.add( + UserMCPServer( + user_id=member_id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + payload = MCPServerUpdate.model_validate( + {"is_active": None, "description": "should-not-land"} + ) + + real_query = db.query + fired = False + + def query_and_delete_on_first_recheck(*entities, **kwargs): + nonlocal fired + if entities == (UserMCPServer,) and not fired: + fired = True + db.execute( + sa.delete(UserMCPServer).where( + UserMCPServer.user_id == member_id, + UserMCPServer.mcpserver_id == server_id, + ) + ) + db.commit() + return real_query(*entities, **kwargs) + + db.query = query_and_delete_on_first_recheck + try: + 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) + finally: + db.query = real_query + + assert fired + assert exc.value.status_code == 400 + assert exc.value.detail == ( + "No personal connection exists to configure user_env or " + "is_active for this server" + ) + + db.rollback() + refreshed = real_query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.description == "original" + + +class TestARowThatVanishesUnderTheLockIsTheGatesOwn404: + """A caller whose personal association row is deleted while this request + waits for the definition-row lock is answered the same way a caller who + never had one is: 404, nothing written. A platform admin is not exempt -- + the gate 404s an admin with neither a personal row nor a verdict, and this + answers the same population the same way once the row is gone. + + The deletion fires on this route's first single-entity ``UserMCPServer`` + query, which is the re-read the lock is taken for: the gate's own read + joins that table to ``MCPServer``, so a bare ``(UserMCPServer,)`` can only + be the post-lock re-read. + """ + + @pytest.mark.parametrize( + "is_admin", [False, True], ids=["member", "platform-admin"] + ) + def test_a_personal_row_deleted_during_the_lock_wait_is_the_gates_own_404( + self, db, is_admin + ): + caller = _make_user(db, 2, is_admin=is_admin) + server = MCPServer( + name="shared-server", transport="stdio", managed="external", command="true" + ) + db.add(server) + db.commit() + server_id = server.id + db.add( + UserMCPServer( + user_id=caller.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + + real_query = db.query + fired = False + + def query_and_delete_on_first_recheck(*entities, **kwargs): + nonlocal fired + if entities == (UserMCPServer,) and not fired: + fired = True + db.execute( + sa.delete(UserMCPServer).where( + UserMCPServer.user_id == caller.id, + UserMCPServer.mcpserver_id == server_id, + ) + ) + db.commit() + return real_query(*entities, **kwargs) + + db.query = query_and_delete_on_first_recheck + try: + with pytest.raises(HTTPException) as exc: + update_mcp_server( + server_id, + MCPServerUpdate(name="renamed"), + current_user=caller, + db=db, + ) + finally: + db.query = real_query + + assert fired + assert exc.value.status_code == 404 + assert exc.value.detail == "MCP server not found" + db.rollback() + refreshed = real_query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.name == "shared-server" + + +class TestADeniedStandInIsRefusedAfterTheLock: + """The lock-side counterpart of + ``TestADenyingStandInIsRefusedRatherThanReportedSuccessful``: a caller + admitted on a real non-owner association row, holding a verdict that + links the connector but denies edit, whose row is then deleted while + this request waits for the definition-row lock. That caller reaches the + lock as a stand-in whose re-derived answer is still "no" -- refused + before the name is read, before the tamper check, and before anything + is rebuilt or committed, the same way the gate would have refused a + stand-in with a denying verdict from the start. + """ + + @pytest.mark.parametrize( + "make_payload", + [ + lambda server: MCPServerUpdate(config={"env": {"K": "v"}}), + lambda server: MCPServerUpdate(description=server.description), + ], + ids=["secrets-only-payload", "resubmit-current-value-payload"], + ) + def test_a_denied_stand_in_after_the_lock_is_refused_before_anything_is_written( + self, db, make_payload + ): + owner = _make_user(db, 1) + member = _make_user(db, 2) + member_id = member.id + server = _make_owned_server(db, owner.id, name="denied-stand-in-after-the-lock") + server.description = "the connector's current description" + server_id = server.id + db.add( + UserMCPServer( + user_id=member_id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + payload = make_payload(server) + + real_query = db.query + fired = False + + def query_and_delete_on_first_recheck(*entities, **kwargs): + nonlocal fired + if entities == (UserMCPServer,) and not fired: + fired = True + db.execute( + sa.delete(UserMCPServer).where( + UserMCPServer.user_id == member_id, + UserMCPServer.mcpserver_id == server_id, + ) + ) + db.commit() + return real_query(*entities, **kwargs) + + db.query = query_and_delete_on_first_recheck + try: + 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) + finally: + db.query = real_query + + assert fired + assert exc.value.status_code == 404 + assert exc.value.detail == "MCP server not found" + + db.rollback() + refreshed = real_query(MCPServer).filter(MCPServer.id == server_id).one() + # A legacy NULL, still unnormalized: the rebuild that would turn it + # into ``[]`` never ran. + assert refreshed.concurrent_tools 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 TestAnAlreadyAssociatedCallersEditSurvivesAHookOutage: + """A caller who already holds a personal association row was admitted + onto this route by that row, not by the team verdict -- an owner and a + platform administrator each decide the edit branch before a verdict is + ever read, and a non-owner member writing only their own association + fields writes nothing the verdict governs. For all of them a hook + failure degrades the reported edit right rather than answering the + whole request with the hook's own outage; see + ``TestTypedErrorArm`` above for the population the verdict genuinely + gates, which still fails closed. + """ + + def test_an_admin_with_a_non_owner_row_still_writes_when_the_hook_fails(self, db): + owner = _make_user(db, 1) + admin = _make_user(db, 2, is_admin=True) + server = _make_owned_server(db, owner.id, name="admin-writes-through-outage") + server_id = server.id + db.add( + UserMCPServer( + user_id=admin.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + + def boom(*_a, **_k): + raise ConnectorRuntimeError("planted", "planted failure", status_code=503) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + response = update_mcp_server( + server_id, + MCPServerUpdate(description="admin edits the shared row"), + current_user=admin, + db=db, + ) + + assert response.description == "admin edits the shared row" + + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert refreshed.description == "admin edits the shared row" + + def test_a_member_setting_only_their_own_user_env_still_writes_when_the_hook_fails( + self, db + ): + owner = _make_user(db, 1) + member = _make_user(db, 2) + member_id = member.id + server = _make_owned_server(db, owner.id, name="member-writes-through-outage") + server_id = server.id + db.add( + UserMCPServer( + user_id=member.id, + mcpserver_id=server_id, + is_owner=False, + is_active=True, + ) + ) + db.commit() + + def boom(*_a, **_k): + raise ConnectorRuntimeError("planted", "planted failure", status_code=503) + + with snapshot_connector_team_hooks(): + set_connector_team_hooks(access=boom) + response = update_mcp_server( + server_id, + MCPServerUpdate(user_env={"API_KEY": "widened-by-a-member"}), + current_user=member, + db=db, + ) + + assert response.can_edit_global is False + + db.commit() + stored = ( + db.query(UserMCPServer) + .filter( + UserMCPServer.user_id == member_id, + UserMCPServer.mcpserver_id == server_id, + ) + .one() + ) + assert stored.env is not None + + +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 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 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) reached through the GET/PUT gate. 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. + + ``api-key-row-with-no-platform-key``: same shape as + ``api-key-row-with-platform-key``, 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. + + ``renamed-builtin-oauth-row``: 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. + """ + + @pytest.mark.parametrize( + "catalog_app, catalog_row, payload, unchanged_field", + [ + ( + lambda db: _make_catalog_app_with_display_name(db, "stripe", "Stripe"), + lambda db: _make_catalog_server_row( + db, + name="stripe", + transport="stdio", + command="python", + args=["-m", "xagent.web.tools.mcp.stripe"], + ), + MCPServerUpdate(config={"command": "evil", "args": []}), + "command", + ), + ( + lambda 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"}, + }, + ), + lambda db: _make_catalog_server_row( + db, + name="notion", + transport="streamable_http", + command=None, + url="https://mcp.notion.com/mcp", + auth={"type": "mcp_oauth"}, + ), + MCPServerUpdate(config={"url": "https://evil.example/mcp"}), + "url", + ), + ( + lambda db: _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"], + }, + ), + lambda db: _make_catalog_server_row( + db, + name="acme-books", + transport="stdio", + command="python", + args=["-m", "acme_books"], + env=None, + ), + MCPServerUpdate(config={"command": "evil", "args": []}), + None, + ), + ( + lambda db: _make_catalog_app_with_display_name(db, "gmail", "Gmail"), + lambda db: _make_catalog_server_row( + db, + name="team-mail-renamed", + transport="oauth", + command=None, + auth={"app_id": "gmail"}, + ), + MCPServerUpdate(description="edited by a teammate"), + None, + ), + ], + ids=[ + "api-key-row-with-platform-key", + "mcp-oauth-row", + "api-key-row-with-no-platform-key", + "renamed-builtin-oauth-row", + ], + ) + def test_team_stand_in_cannot_rewrite_a_catalog_row( + self, db, catalog_app, catalog_row, payload, unchanged_field + ): + catalog_app(db) + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = catalog_row(db) + db.add( + UserMCPServer( + user_id=owner.id, mcpserver_id=server.id, is_owner=True, is_active=True + ) + ) + db.commit() + server_id = server.id + original_value = getattr(server, unchanged_field) if unchanged_field else None + 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, + payload, + 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 + + if unchanged_field is not None: + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert getattr(refreshed, unchanged_field) == original_value + + 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_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 + + +class TestOwnershipWithholdsTheTeamEditRightFromAnOwnerlessRow: + """A team verdict that grants edit is also downgraded when the + definition row it names has no ``is_owner=True`` association at all -- + independent of, and in addition to, the catalog-key test above. The + catalog-key test alone cannot see a non-``oauth`` row an administrator + renamed away from its key, because for that row the key IS the current + name and this same route can change it. The ownership test does not + depend on any field this route can write, so it still catches such a + row after the rename. + """ + + @pytest.mark.parametrize( + "catalog_app, catalog_row, tamper_payload, unchanged_field", + [ + ( + lambda db: _make_catalog_app_with_display_name( + db, "billing-api", "Billing API" + ), + lambda db: _make_catalog_server_row( + db, + name="billing-api", + transport="stdio", + command="python", + args=["-m", "xagent.web.tools.mcp.billing_api"], + ), + MCPServerUpdate(config={"command": "evil", "args": []}), + "command", + ), + ( + lambda db: _make_catalog_app_with_display_name( + db, "browser-tool", "Browser Tool" + ), + lambda db: _make_catalog_server_row( + db, + name="browser-tool", + transport="stdio", + command="npx", + args=["-y", "@browser/tool"], + ), + MCPServerUpdate(config={"command": "evil", "args": []}), + "command", + ), + ( + lambda db: _make_catalog_app_with_display_name( + db, + "docs-oauth", + "Docs OAuth", + transport="streamable_http", + launch_config={ + "url": "https://mcp.docs.example/mcp", + "auth": {"type": "mcp_oauth"}, + }, + ), + lambda db: _make_catalog_server_row( + db, + name="docs-oauth", + transport="streamable_http", + command=None, + url="https://mcp.docs.example/mcp", + auth={"type": "mcp_oauth"}, + ), + MCPServerUpdate(config={"url": "https://evil.example/mcp"}), + "url", + ), + ], + ids=["api_key", "keyless", "mcp_oauth"], + ) + def test_a_renamed_catalog_row_with_no_owner_is_not_team_editable( + self, db, catalog_app, catalog_row, tamper_payload, unchanged_field + ): + catalog_app(db) + admin = _make_user(db, 1, is_admin=True) + member = _make_user(db, 2) + server = catalog_row(db) + server_id = server.id + # Production shape: the administrator who connected this catalog row + # holds an ordinary non-owner association -- connect never marks one + # is_owner=True, so this row has an association but no owner. + db.add( + UserMCPServer( + user_id=admin.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=True) for ref in refs + } + ) + update_mcp_server( + server_id, + MCPServerUpdate(name="renamed-away-from-the-catalog-key"), + current_user=admin, + db=db, + ) + + original_value = getattr( + db.query(MCPServer).filter(MCPServer.id == server_id).one(), + unchanged_field, + ) + with pytest.raises(HTTPException) as exc: + update_mcp_server(server_id, tamper_payload, 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 + + db.rollback() + refreshed = db.query(MCPServer).filter(MCPServer.id == server_id).one() + assert getattr(refreshed, unchanged_field) == original_value + + def test_a_row_whose_owner_is_gone_is_not_team_editable(self, db): + """A definition row with no owner and no catalog-key collision + either -- the shape left behind once a connector's creator account + has been deleted, since association rows cascade with the user. + This is the conservative direction stated in the docstring: a + wrong answer here refuses the edit rather than granting one.""" + member = _make_user(db, 2) + server = MCPServer( + name="orphaned-connector", + transport="stdio", + managed="external", + command="true", + ) + db.add(server) + 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 + } + ) + 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 "You do not have permission to edit this MCP server" in exc.value.detail + + def test_an_owned_row_still_gets_the_team_edit_right(self, db): + """Reverse anchor, guarding against an over-broad fix: a row that + does have an owner -- not a catalog row -- must keep its team edit + right. Written so a change that downgrades every row + unconditionally, not only ownerless ones, cannot pass by refusing + everything.""" + owner = _make_user(db, 1) + member = _make_user(db, 2) + server = _make_owned_server(db, owner.id, name="owned-row-still-editable") + 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=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" + + +_SEAM_MODULE = "xagent.web.api.mcp" + +# The arms that answer a failed access verdict with a warning instead of +# re-raising it. Pinned as a count so the enumeration below cannot pass by +# finding nothing, and so a second arm has to come here before it can skip the +# invariant. +_DEGRADING_CONNECTOR_RUNTIME_HANDLERS = 1 + + +def _connector_runtime_handlers_that_log() -> list[ast.ExceptHandler]: + """Every ``except ConnectorRuntimeError`` arm in this module that answers + the failure with a warning rather than re-raising it.""" + module = importlib.import_module(_SEAM_MODULE) + tree = ast.parse(inspect.getsource(module)) + handlers = [] + for node in ast.walk(tree): + if not isinstance(node, ast.ExceptHandler): + continue + if not ( + isinstance(node.type, ast.Name) and node.type.id == "ConnectorRuntimeError" + ): + continue + if any( + isinstance(child, ast.Call) + and isinstance(child.func, ast.Attribute) + and child.func.attr == "warning" + and isinstance(child.func.value, ast.Name) + and child.func.value.id == "logger" + for child in ast.walk(node) + ): + handlers.append(node) + return handlers + + +def test_the_degrading_handler_enumeration_is_not_vacuous(): + """Pins the enumeration itself, so the assertion below cannot pass by + finding nothing.""" + assert ( + len(_connector_runtime_handlers_that_log()) + == _DEGRADING_CONNECTOR_RUNTIME_HANDLERS + ) + + +def test_every_degrading_handler_logs_the_failure_it_degraded_on(): + """An arm that answers a failed verdict with a warning leaves the response + at 200, so that warning is the only record of why the caller lost a + reported edit right. It has to name the failure, which means formatting + the caught ``ConnectorRuntimeError`` -- whose ``str`` is + ``": "`` -- into the line. + + Pinned in the source rather than only per route: the behavioural pin in + ``test_mcp_reported_edit_permission.py`` can exercise one arm per test, + and an arm added later would inherit neither that pin nor this reasoning. + """ + offenders = [] + for handler in _connector_runtime_handlers_that_log(): + for call in [ + child + for child in ast.walk(handler) + if isinstance(child, ast.Call) + and isinstance(child.func, ast.Attribute) + and child.func.attr == "warning" + ]: + carries_the_exception = handler.name is not None and any( + isinstance(arg, ast.Name) and arg.id == handler.name + for arg in call.args + ) + if not carries_the_exception: + offenders.append(f"line {call.lineno}") + assert offenders == [], ( + "these degrade arms log a warning that never formats the caught " + f"ConnectorRuntimeError, so the failure has no identity: {offenders}" + ) diff --git a/tests/web/services/test_connector_team_scope.py b/tests/web/services/test_connector_team_scope.py index 014edab2a4..ac559c2f14 100644 --- a/tests/web/services/test_connector_team_scope.py +++ b/tests/web/services/test_connector_team_scope.py @@ -2049,7 +2049,9 @@ def test_the_access_slot_lets_the_boundary_error_through_its_wrapper(db_session) because the only ``access`` call site that holds a lock lives in ``custom_api.py``, and that path is covered by ``tests/web/api/test_custom_api_team_connector_edit.py`` instead. The - seam's own transient-outage error gets folded into + MCP gateway's ``access`` call site (``mcp._resolve_mcp_server_for_request``) + does not hold a lock: it runs before either ``GET`` or ``PUT`` takes + one. The seam's own transient-outage error gets folded into ``ConnectorRuntimeError`` by the surrounding ``except Exception``; this one must not -- a permanent defect in the installing application's code is a different failure than an outage, and folding it in would