diff --git a/README.md b/README.md index 01b5168..3aa54e6 100644 --- a/README.md +++ b/README.md @@ -212,6 +212,28 @@ connections: scopes: {read: Read internal data} ``` +A `hosts` entry is an exact host name; nothing is matched by suffix. When one +host serves unrelated APIs side by side — `www.googleapis.com` carries Drive +and Calendar next to GCP's storage and compute — the entry can be scoped to a +path prefix, written `host/path/`, and requests must stay below that path. +A bare entry admits every path on its host, so declaring the same host bare +next to a scoped entry — or setting `base_url` on that host, which lists it +bare — is reported: the scoping would silently not happen. + +A connection's `oauth.scopes` go to every agent that declares it, so they stay +a read-only minimum. Scopes under `oauth.optional_scopes` are a menu: an agent +gets one only by selecting it in its own declaration, and the selection lands +in that agent's own authorization, so consenting to one agent's writes grants +nothing to any other. A scope outside the menu is refused by `gete validate`. + +```yaml +# agent.yaml +connections: + - freee # the defaults only + - id: google # the defaults plus a selection from the menu + scopes: [https://www.googleapis.com/auth/spreadsheets] +``` + `oauth.pkce: true` asks Gemini Enterprise to carry a code challenge through the flow. An authorization server that requires PKCE refuses the code exchange without one, and there is no other way to ask for it from a declaration. diff --git a/src/gete/catalog/connections/google.yaml b/src/gete/catalog/connections/google.yaml index b52ab82..87f3cd7 100644 --- a/src/gete/catalog/connections/google.yaml +++ b/src/gete/catalog/connections/google.yaml @@ -2,26 +2,56 @@ id: google display_name: Google Workspace docs: https://developers.google.com/workspace -# One host per API. Listing googleapis.com as a whole would let a Workspace -# authorization reach GCP APIs, and so would www.googleapis.com, which serves -# storage, compute, and oauth2 next to the Workspace APIs. +# The ceiling: every host a token may be sent to, whatever scopes it carries. +# One entry per API. Listing googleapis.com as a whole would let a Workspace +# authorization reach GCP APIs, and so would a bare www.googleapis.com, which +# serves storage, compute, and oauth2 next to Drive and Calendar. Those two +# are served nowhere else - their service names drive.googleapis.com and +# calendar-json.googleapis.com route no requests - so their entries scope +# www.googleapis.com to the APIs' own paths, Drive uploads included. hosts: - gmail.googleapis.com - - calendar-json.googleapis.com - sheets.googleapis.com - - drive.googleapis.com + - docs.googleapis.com + - slides.googleapis.com - people.googleapis.com + - www.googleapis.com/calendar/ + - www.googleapis.com/drive/ + - www.googleapis.com/upload/drive/ token_prefixes: - ya29. oauth: authorization_url: https://accounts.google.com/o/oauth2/v2/auth token_url: https://oauth2.googleapis.com/token - # Read-only. Asking for write access would let an agent send or approve - # on the user's behalf, and gete agents are not meant to. + # The floor: what every agent that declares the connection gets. Read-only + # and minimal, so a bare `connections: [google]` keeps meaning what it + # always meant. scopes: https://www.googleapis.com/auth/gmail.readonly: Read the subject, sender, and body of your mail https://www.googleapis.com/auth/calendar.readonly: Read your calendar events + # The menu: an agent gets one of these only by selecting it in its own + # declaration, and the selection lands in that agent's authorization alone. + # Write access lives here and never in the defaults, so no agent can write + # without saying so on its consent screen. + # + # drive.file reaches only the files the agent creates or the user opens + # with it; Drive-wide write is deliberately not offered. gmail.send is the + # heaviest entry - mail sent as the user is outward and cannot be recalled - + # and it grants sending alone: gmail.modify and mail.google.com are not + # offered, because rewriting or deleting the inbox is not needed to send. + optional_scopes: + https://www.googleapis.com/auth/spreadsheets.readonly: Read your spreadsheets + https://www.googleapis.com/auth/drive.readonly: Read your Drive files + https://www.googleapis.com/auth/documents.readonly: Read your documents + https://www.googleapis.com/auth/presentations.readonly: Read your presentations + https://www.googleapis.com/auth/contacts.readonly: Read your contacts + https://www.googleapis.com/auth/spreadsheets: Read and edit your spreadsheets + https://www.googleapis.com/auth/documents: Read and edit your documents + https://www.googleapis.com/auth/presentations: Read and edit your presentations + https://www.googleapis.com/auth/calendar.events: Read and edit events on your calendars + https://www.googleapis.com/auth/drive.file: Read and edit the Drive files this agent creates or opens + https://www.googleapis.com/auth/gmail.send: Send mail as you examples: accepts: diff --git a/src/gete/connection/checks.py b/src/gete/connection/checks.py index 4c9dea7..7886823 100644 --- a/src/gete/connection/checks.py +++ b/src/gete/connection/checks.py @@ -76,6 +76,23 @@ def connection_problems(connection: Connection, registry: Registry) -> list[str] problems.append( f"hosts: {host} is a whole platform domain, list the API host" ) + for entry in sorted(connection.hosts): + # A bare entry admits every path on its host, so a scoped entry for + # the same host never applies - it reads as a restriction it does not + # make. base_url puts its host on the list bare, so setting one on a + # scoped host silently widens the ceiling the same way. + host, slash, _ = entry.partition("/") + if not slash or host not in connection.hosts: + continue + if connection.base_url and urlsplit(connection.base_url).hostname == host: + problems.append( + f"hosts: {entry} never applies; base_url puts {host} on the " + "list bare, and a bare entry admits every path" + ) + else: + problems.append( + f"hosts: {entry} never applies; the bare {host} entry admits every path" + ) for other in registry.all(include_retired=True): if other.id == connection.id: continue @@ -85,6 +102,18 @@ def connection_problems(connection: Connection, registry: Registry) -> list[str] problems.append( f"token_prefixes: {prefix!r} overlaps {theirs!r} ({other.id})" ) + for scope in sorted(connection.oauth.optional_scopes): + if scope in connection.oauth.scopes: + problems.append( + f"oauth.optional_scopes: {scope} is already a default scope" + ) + if connection.oauth.optional_scopes and connection.oauth.authorization_query: + # The verbatim query is the whole authorization URL; a selection + # would be accepted and then never reach the consent screen. + problems.append( + "oauth.optional_scopes: the menu cannot be offered next to a " + "verbatim authorization_query, which fixes the scopes" + ) for token in connection.examples.accepts: if not connection.accepts_token(token): problems.append(f"examples.accepts: {token!r} is not accepted") @@ -99,8 +128,12 @@ def connection_problems(connection: Connection, registry: Registry) -> list[str] ) if not claims_google and connection.accepts_token(_GOOGLE_ACCESS_TOKEN_EXAMPLE): problems.append("a Google access token is accepted") - if connection.mcp_url is not None and not connection.needs_base_url: - mcp_host = urlsplit(connection.mcp_url).hostname - if mcp_host not in connection.hosts: - problems.append(f"mcp.url: host {mcp_host} is not in hosts") + # The MCP server is spoken to with the user's token, so its URL must sit + # where hosts lets the token go - path scoping included. + if ( + connection.mcp_url is not None + and not connection.needs_base_url + and not connection.allows(connection.mcp_url) + ): + problems.append(f"mcp.url: {connection.mcp_url} is not covered by hosts") return problems diff --git a/src/gete/connection/registry.py b/src/gete/connection/registry.py index 0a8dc54..f4a6be2 100644 --- a/src/gete/connection/registry.py +++ b/src/gete/connection/registry.py @@ -10,7 +10,7 @@ from collections.abc import Iterable, Iterator, Mapping from dataclasses import dataclass, field, replace from typing import Any -from urllib.parse import urlsplit +from urllib.parse import unquote, urlsplit from gete.catalog import catalog_connections from gete.errors import DeclarationError, RetiredConnection, UnknownConnection @@ -66,7 +66,10 @@ def looks_like_jwt(token: str) -> bool: class OAuth: """How users authorize the connection. - scopes maps a scope to the explanation shown on the consent screen. + scopes maps a scope to the explanation shown on the consent screen; every + agent that declares the connection gets them. optional_scopes is the menu + an agent may select from on top of that, in the same shape; none of them + reach a token unless the agent declares them. scope_parameter is the query parameter that carries the user scopes; Slack uses user_scope because scope means the app's own permissions there. authorization_query, when set, is used verbatim as the authorization URL's @@ -80,6 +83,7 @@ class OAuth: authorization_url: str token_url: str scopes: Mapping[str, str] + optional_scopes: Mapping[str, str] = field(default_factory=dict) scope_parameter: str = "scope" authorization_query: Mapping[str, str] | None = None pkce: bool = False @@ -92,6 +96,7 @@ def from_mapping( authorization_url=str(_rooted(data["authorization_url"], base_url)), token_url=str(_rooted(data["token_url"], base_url)), scopes=dict(data["scopes"]), + optional_scopes=dict(data.get("optional_scopes", {})), scope_parameter=data.get("scope_parameter", "scope"), pkce=bool(data.get("pkce", False)), authorization_query=( @@ -102,6 +107,26 @@ def from_mapping( ) +def _stays_below(path: str, prefix: str) -> bool: + """Whether the request path stays below the prefix however a server reads it. + + A dot segment or an encoded separator - percent-encoded any number of + times - is refused rather than resolved: resolving would have to guess + how many times the server decodes. + """ + for segment in path.split("/"): + decoded = unquote(segment) + while decoded != segment: + segment, decoded = decoded, unquote(decoded) + if segment in (".", "..") or "/" in segment or "\\" in segment: + return False + if not prefix.endswith("/"): + # The schema requires the trailing slash; a definition that dodged it + # must not widen the ceiling to /prefix-and-more. + prefix += "/" + return path.startswith("/" + prefix) + + @dataclass(frozen=True) class Examples: """Token shapes, not real tokens. The catalog checks accepts_token against them.""" @@ -225,21 +250,31 @@ def allows(self, url: str) -> bool: """Whether a token may be attached to a request for this URL. Only https, and only an exact host match. Prefix or suffix matching - would accept names such as slack.com.example.com. + would accept names such as slack.com.example.com. An entry written as + host/path/ admits only requests below that path: some platforms serve + unrelated APIs from one host, and the path is where they part. """ parsed = urlsplit(url) - return parsed.scheme == "https" and parsed.hostname in self.hosts + if parsed.scheme != "https" or parsed.hostname is None: + return False + for entry in self.hosts: + host, slash, prefix = entry.partition("/") + if parsed.hostname != host: + continue + if not slash or _stays_below(parsed.path, prefix): + return True + return False def allows_redirect(self, url: str) -> bool: """Whether a download may follow a redirect here; the token may not. - Exact hostname matching against the declared lists only: a named - host is no safer than an address literal, so nothing is accepted - for merely looking like a public name. + Exact matching against the declared lists only: a named host is no + safer than an address literal, so nothing is accepted for merely + looking like a public name. """ parsed = urlsplit(url) - return parsed.scheme == "https" and ( - parsed.hostname in self.hosts or parsed.hostname in self.redirect_hosts + return self.allows(url) or ( + parsed.scheme == "https" and parsed.hostname in self.redirect_hosts ) def reauthorization_message(self) -> str: diff --git a/src/gete/connections_listing.py b/src/gete/connections_listing.py index 7821e80..2142e47 100644 --- a/src/gete/connections_listing.py +++ b/src/gete/connections_listing.py @@ -43,6 +43,7 @@ def format_connection(connection: Connection) -> str: """ oauth = connection.oauth scopes = [f"{scope}: {text}" for scope, text in oauth.scopes.items()] + optional = [f"{scope}: {text}" for scope, text in oauth.optional_scopes.items()] fields: list[tuple[str, list[str]]] = [ # The same word the listing uses; the reason gets a line of its own. ("status", ["retired" if connection.retired else "available"]), @@ -63,6 +64,9 @@ def format_connection(connection: Connection) -> str: ("authorization", [oauth.authorization_url]), ("token url", [oauth.token_url]), ("scopes", scopes or [NONE_DECLARED]), + # The menu, because the OAuth client has to be prepared for every + # scope an agent may select, not only the defaults. + ("optional scopes", optional or [NONE_DECLARED]), ("client id", [connection.client_id_secret]), ("client secret", [connection.client_secret_secret]), ("redirect uri", [REDIRECT_URI]), diff --git a/src/gete/declaration.py b/src/gete/declaration.py index a54fe37..f3e9d64 100644 --- a/src/gete/declaration.py +++ b/src/gete/declaration.py @@ -69,7 +69,25 @@ def name(self) -> str: @property def connections(self) -> tuple[str, ...]: - return tuple(self.data.get("connections", ())) + """Connection ids, whichever way each entry is written. + + An entry is the id itself, or a mapping that also selects scopes from + the connection's menu; everything that only needs to know which + connections the agent holds reads the ids from here. + """ + return tuple( + entry if isinstance(entry, str) else str(entry["id"]) + for entry in self.data.get("connections", ()) + ) + + @property + def scope_selections(self) -> dict[str, tuple[str, ...]]: + """Selected optional scopes by id; ids that select nothing are absent.""" + return { + str(entry["id"]): tuple(entry["scopes"]) + for entry in self.data.get("connections", ()) + if not isinstance(entry, str) + } @property def tools(self) -> tuple[Mapping[str, Any], ...]: diff --git a/src/gete/register.py b/src/gete/register.py index 764c331..1e5d071 100644 --- a/src/gete/register.py +++ b/src/gete/register.py @@ -12,7 +12,7 @@ import base64 import json -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -47,10 +47,39 @@ def in_use_by_another(message: str) -> bool: return AUTHORIZATION_IN_USE in message -def authorization_uri(connection: Connection, client_id: str) -> str: - """Where the user is sent to consent, with the scopes the connection declares.""" +def requested_scopes(connection: Connection, selected: Sequence[str]) -> list[str]: + """The connection's default scopes plus the agent's selection, in that order. + + The selection must come from the connection's optional_scopes menu. The + menu is the reviewed ceiling, and register can be run without validate, so + a scope from outside it is refused here too rather than sent to the + consent screen. + """ + menu = connection.oauth.optional_scopes + unknown = [scope for scope in selected if scope not in menu] + if unknown: + raise DeclarationError( + f"connection {connection.id}: {', '.join(unknown)} not in " + "oauth.optional_scopes; an agent selects from the menu only" + ) + defaults = list(connection.oauth.scopes) + return defaults + [scope for scope in selected if scope not in defaults] + + +def authorization_uri( + connection: Connection, client_id: str, selected_scopes: Sequence[str] = () +) -> str: + """Where the user is sent to consent: the default scopes plus the selection.""" oauth = connection.oauth if oauth.authorization_query is not None: + if selected_scopes: + # The query is used as written; building the selection into it + # would second-guess the form known to work, and leaving it out + # would grant less than the agent declared without a word. + raise DeclarationError( + f"connection {connection.id} fixes its authorization_query; " + "a scope selection cannot reach the consent screen" + ) # The form known to work for this service; Gemini Enterprise adds # client_id and redirect_uri itself. params: dict[str, str] = dict(oauth.authorization_query) @@ -59,7 +88,9 @@ def authorization_uri(connection: Connection, client_id: str) -> str: "client_id": client_id, "redirect_uri": REDIRECT_URI, "response_type": "code", - oauth.scope_parameter: " ".join(oauth.scopes), + oauth.scope_parameter: " ".join( + requested_scopes(connection, selected_scopes) + ), # Without offline access there is no refresh token and reading # stops an hour after consent. "access_type": "offline", @@ -74,8 +105,14 @@ def authorization_body( connection: Connection, client_id: str, client_secret: str, + selected_scopes: Sequence[str] = (), ) -> dict[str, Any]: - """The Authorization resource, named per agent so two agents never share one.""" + """The Authorization resource, named per agent so two agents never share one. + + The scope selection rides in the authorization URI, so the selection is + per agent too: another agent on the same connection consents to its own + scopes and no more. + """ if connection.needs_base_url: # validate refuses this, but register can be run without it, and what # would be stored here is the link every user of the agent is sent to. @@ -84,7 +121,7 @@ def authorization_body( oauth2: dict[str, Any] = { "clientId": client_id, "clientSecret": client_secret, - "authorizationUri": authorization_uri(connection, client_id), + "authorizationUri": authorization_uri(connection, client_id, selected_scopes), "tokenUri": connection.oauth.token_url, } if connection.oauth.pkce: @@ -222,6 +259,19 @@ def _parent(self) -> str: return f"projects/{self._number}/locations/{self._ge_location}" def _register(self, agent: Agent, engine: str, summary: Summary) -> None: + seen: set[str] = set() + for connection_id in agent.connections: + if connection_id in seen: + # The schema cannot see that a string entry and a mapping + # entry name the same connection, and scope selections are + # keyed by id, so one entry's selection would stand for every + # duplicate silently. + raise DeclarationError( + f"connections: {connection_id} is declared twice; " + "only one entry's scope selection could reach the " + "consent screen" + ) + seen.add(connection_id) engines_url = ( f"https://{self._location}-aiplatform.googleapis.com/v1/projects/" f"{self._gcp_project}/locations/{self._location}/reasoningEngines" @@ -236,7 +286,10 @@ def _register(self, agent: Agent, engine: str, summary: Summary) -> None: ) authorizations = [ self._upsert_authorization( - agent, self._registry.get(connection_id), summary + agent, + self._registry.get(connection_id), + agent.scope_selections.get(connection_id, ()), + summary, ) for connection_id in agent.connections ] @@ -299,7 +352,11 @@ def _register(self, agent: Agent, engine: str, summary: Summary) -> None: summary.registered.append(agent.name) def _upsert_authorization( - self, agent: Agent, connection: Connection, summary: Summary + self, + agent: Agent, + connection: Connection, + selected_scopes: Sequence[str], + summary: Summary, ) -> str: body = authorization_body( self._parent, @@ -307,6 +364,7 @@ def _upsert_authorization( connection, self._secret(connection.client_id_secret), self._secret(connection.client_secret_secret), + selected_scopes, ) resource = str(body["name"]) identifier = resource.rsplit("/", 1)[-1] diff --git a/src/gete/schema/agent.json b/src/gete/schema/agent.json index c83fc88..36b987f 100644 --- a/src/gete/schema/agent.json +++ b/src/gete/schema/agent.json @@ -47,8 +47,36 @@ "type": "array", "uniqueItems": true, "items": { - "type": "string", - "pattern": "^[a-z][a-z0-9-]*$" + "oneOf": [ + { + "type": "string", + "pattern": "^[a-z][a-z0-9-]*$" + }, + { + "description": "A connection with scopes selected from its optional_scopes menu, on top of the defaults every agent gets. An entry that selects nothing is written as the plain string instead.", + "type": "object", + "additionalProperties": false, + "required": [ + "id", + "scopes" + ], + "properties": { + "id": { + "type": "string", + "pattern": "^[a-z][a-z0-9-]*$" + }, + "scopes": { + "type": "array", + "minItems": 1, + "uniqueItems": true, + "items": { + "type": "string", + "minLength": 1 + } + } + } + } + ] } }, "shared_credentials": { diff --git a/src/gete/schema/connection.json b/src/gete/schema/connection.json index a8f64e6..5647653 100644 --- a/src/gete/schema/connection.json +++ b/src/gete/schema/connection.json @@ -23,11 +23,18 @@ "$ref": "#/$defs/https_url" }, "hosts": { - "description": "Host names the user token may be sent to, matched exactly.", + "description": "Host names the user token may be sent to, matched exactly. An entry written as host/path/ admits only requests below that path, for hosts that serve unrelated APIs side by side.", "type": "array", "uniqueItems": true, "items": { - "$ref": "#/$defs/hostname" + "oneOf": [ + { + "$ref": "#/$defs/hostname" + }, + { + "$ref": "#/$defs/host_path_prefix" + } + ] } }, "base_url": { @@ -67,7 +74,15 @@ "$ref": "#/$defs/rooted_url" }, "scopes": { - "description": "Scope to the explanation shown on the consent screen.", + "description": "Scope to the explanation shown on the consent screen. Every agent that declares the connection gets these.", + "type": "object", + "additionalProperties": { + "type": "string", + "minLength": 1 + } + }, + "optional_scopes": { + "description": "Scope to the explanation shown on the consent screen. The menu an agent may select from; none of these reach a token unless the agent declares them.", "type": "object", "additionalProperties": { "type": "string", @@ -175,6 +190,10 @@ "hostname": { "type": "string", "pattern": "^(?=.{1,253}$)([a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?\\.)+[a-z]{2,63}$" + }, + "host_path_prefix": { + "type": "string", + "pattern": "^(?=[^/]{1,253}/)([a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?\\.)+[a-z]{2,63}(/(?!\\.\\.?/)[a-z0-9._~-]+)+/$" } } } diff --git a/src/gete/validate.py b/src/gete/validate.py index 85b4ba5..6ea9550 100644 --- a/src/gete/validate.py +++ b/src/gete/validate.py @@ -98,14 +98,36 @@ def _agent_problems( f"{MIN_SERVICE_ACCOUNT_ID_LENGTH} and {MAX_SERVICE_ACCOUNT_ID_LENGTH} " "characters; Terraform would be refused at apply time" ) + # known holds only the ids the registry resolved; the rules below look + # them up again. Duplicates are tracked apart so an unknown connection + # declared twice is still one duplicate, not two unknowns. known: set[str] = set() + declared: set[str] = set() for connection_id in agent.connections: + if connection_id in declared: + # uniqueItems cannot see that a string entry and a mapping entry + # name the same connection. + found.append(f"connections: {connection_id} is declared twice") + continue + declared.add(connection_id) try: connection = registry.get(connection_id) except GeteError as error: found.append(f"connections: {error}") continue known.add(connection_id) + menu = connection.oauth.optional_scopes + outside = [ + scope + for scope in agent.scope_selections.get(connection_id, ()) + if scope not in menu + ] + if outside: + offered = ", ".join(sorted(menu)) or "nothing" + found.append( + f"connections: {connection_id} does not offer " + f"{', '.join(outside)}; oauth.optional_scopes offers {offered}" + ) if connection.needs_base_url: # The definition left the root open because it moves with the # installation. Nothing it declares is an address until then: not diff --git a/tests/conformance/test_catalog.py b/tests/conformance/test_catalog.py index 532d8a1..4ee4e38 100644 --- a/tests/conformance/test_catalog.py +++ b/tests/conformance/test_catalog.py @@ -55,9 +55,84 @@ def test_google_hosts_are_specific_apis_not_the_whole_domain() -> None: """A Workspace authorization must not be usable against GCP APIs.""" hosts = CATALOG["google"]["hosts"] assert "googleapis.com" not in hosts - # www.googleapis.com serves storage, compute, and oauth2 as well. + # www.googleapis.com serves storage, compute, and oauth2 as well; it may + # appear only scoped to the path of an API that has no other home. assert "www.googleapis.com" not in hosts - assert all(host.endswith(".googleapis.com") for host in hosts) + for host in hosts: + name, _, path = host.partition("/") + assert name.endswith(".googleapis.com") + if name == "www.googleapis.com": + assert path, host + + +def test_google_reaches_drive_and_calendar_below_their_paths_only() -> None: + """Drive v3 and Calendar v3 are served from www.googleapis.com and nowhere + else. Their service names (drive.googleapis.com, calendar-json.googleapis.com) + route no requests and must not sit on the list looking like they do.""" + hosts = CATALOG["google"]["hosts"] + assert "www.googleapis.com/calendar/" in hosts + assert "www.googleapis.com/drive/" in hosts + assert "www.googleapis.com/upload/drive/" in hosts + assert "drive.googleapis.com" not in hosts + assert "calendar-json.googleapis.com" not in hosts + + +def test_google_scoped_entries_admit_the_apis_and_refuse_the_platform() -> None: + google = Registry.from_catalog().get("google") + assert google.allows("https://www.googleapis.com/calendar/v3/calendars/primary") + assert google.allows("https://www.googleapis.com/drive/v3/files") + assert google.allows("https://www.googleapis.com/upload/drive/v3/files") + assert not google.allows("https://www.googleapis.com/storage/v1/b/bucket") + assert not google.allows("https://www.googleapis.com/compute/v1/projects/p") + assert not google.allows("https://www.googleapis.com/oauth2/v4/token") + assert not google.allows("https://www.googleapis.com/drive/../storage/v1/b/b") + + +def test_google_defaults_stay_the_read_only_minimum() -> None: + """A bare `connections: [google]` grants what it always did, nothing more.""" + assert set(CATALOG["google"]["oauth"]["scopes"]) == { + "https://www.googleapis.com/auth/gmail.readonly", + "https://www.googleapis.com/auth/calendar.readonly", + } + + +def test_google_offers_workspace_reads_and_writes_on_the_menu() -> None: + """The whole menu, pinned: every entry is reviewed, and every entry is + served by a host in the ceiling.""" + assert set(CATALOG["google"]["oauth"]["optional_scopes"]) == { + "https://www.googleapis.com/auth/spreadsheets.readonly", + "https://www.googleapis.com/auth/drive.readonly", + "https://www.googleapis.com/auth/documents.readonly", + "https://www.googleapis.com/auth/presentations.readonly", + "https://www.googleapis.com/auth/contacts.readonly", + "https://www.googleapis.com/auth/spreadsheets", + "https://www.googleapis.com/auth/documents", + "https://www.googleapis.com/auth/presentations", + "https://www.googleapis.com/auth/calendar.events", + "https://www.googleapis.com/auth/drive.file", + "https://www.googleapis.com/auth/gmail.send", + } + + +def test_google_menu_has_no_blanket_drive_write_and_no_inbox_mutation() -> None: + """drive.file writes only what the agent created or opened, and sending + mail does not need the power to rewrite or delete the inbox.""" + menu = set(CATALOG["google"]["oauth"]["optional_scopes"]) + assert "https://www.googleapis.com/auth/drive" not in menu + assert "https://www.googleapis.com/auth/gmail.modify" not in menu + assert "https://mail.google.com/" not in menu + + +def test_google_sending_mail_says_so_on_the_consent_screen() -> None: + """Sending as the user is outward and irreversible; the text must not soften it.""" + menu = CATALOG["google"]["oauth"]["optional_scopes"] + assert "as you" in menu["https://www.googleapis.com/auth/gmail.send"] + + +def test_google_hosts_cover_the_docs_and_slides_apis() -> None: + hosts = CATALOG["google"]["hosts"] + assert "docs.googleapis.com" in hosts + assert "slides.googleapis.com" in hosts def test_catalog_files_are_read_with_dates_as_strings() -> None: diff --git a/tests/test_connection_registry.py b/tests/test_connection_registry.py index 6a629e8..fbad96f 100644 --- a/tests/test_connection_registry.py +++ b/tests/test_connection_registry.py @@ -85,6 +85,49 @@ def test_base_url_host_is_allowed_too() -> None: assert entry.allows("https://api.example.com/v1") +@pytest.mark.parametrize( + ("url", "allowed"), + [ + ("https://shared.example.com/api/v1/things", True), + ("https://shared.example.com/api/", True), + ("https://shared.example.com/api", False), + ("https://shared.example.com/apix/v1", False), + ("https://shared.example.com/other/v1", False), + ("https://shared.example.com/", False), + ("https://shared.example.com/api/../other/v1", False), + ("https://shared.example.com/api/%2e%2e/other/v1", False), + ("https://shared.example.com/api/%252e%252e/other/v1", False), + ("https://shared.example.com/api/..%2fother/v1", False), + ("https://shared.example.com/api/x%5c../other", False), + ("http://shared.example.com/api/v1", False), + ], +) +def test_a_path_scoped_host_allows_only_requests_below_the_path( + url: str, allowed: bool +) -> None: + """Some platforms serve unrelated APIs from one host; a host/path/ entry + admits one API without admitting the platform. A dot segment or an encoded + separator is refused rather than resolved, because resolving would have to + guess how many times the server decodes.""" + entry = connection(hosts=["shared.example.com/api/"]) + assert entry.allows(url) is allowed + + +def test_a_plain_hostname_entry_keeps_admitting_every_path() -> None: + entry = connection(hosts=["api.example.com", "shared.example.com/api/"]) + assert entry.allows("https://api.example.com/anything/at/all") + assert not entry.allows("https://shared.example.com/anything/at/all") + + +def test_a_redirect_may_go_below_a_path_scoped_host_but_not_beside_it() -> None: + entry = connection( + hosts=["shared.example.com/api/"], redirect_hosts=["cdn.example.com"] + ) + assert entry.allows_redirect("https://shared.example.com/api/download") + assert not entry.allows_redirect("https://shared.example.com/other/download") + assert entry.allows_redirect("https://cdn.example.com/download") + + def test_oauth_client_defaults_to_ge_oauth_id() -> None: entry = connection() assert entry.secret_prefix == "ge-oauth-example" @@ -96,6 +139,42 @@ def test_oauth_client_defaults_to_ge_oauth_id() -> None: ) +def test_optional_scopes_are_a_menu_next_to_the_defaults() -> None: + entry = connection( + oauth={**EXAMPLE["oauth"], "optional_scopes": {"write": "Change data"}} + ) + assert entry.oauth.scopes == {"read": "Read data"} + assert entry.oauth.optional_scopes == {"write": "Change data"} + + +def test_a_connection_without_a_menu_offers_nothing() -> None: + assert connection().oauth.optional_scopes == {} + + +def test_an_optional_scope_repeated_in_the_defaults_is_reported() -> None: + entry = connection( + oauth={**EXAMPLE["oauth"], "optional_scopes": {"read": "Read data"}} + ) + assert any( + "read" in problem for problem in connection_problems(entry, Registry([entry])) + ) + + +def test_a_menu_next_to_a_verbatim_authorization_query_is_reported() -> None: + """The query is used verbatim; no selection could ever reach the consent screen.""" + entry = connection( + oauth={ + **EXAMPLE["oauth"], + "authorization_query": {"response_type": "code"}, + "optional_scopes": {"write": "Change data"}, + } + ) + assert any( + "authorization_query" in problem + for problem in connection_problems(entry, Registry([entry])) + ) + + def test_catalog_entry_can_be_overridden_partially() -> None: registry = Registry.from_catalog( {"github": {"base_url": "https://api.github.example.com"}} @@ -177,6 +256,32 @@ def test_connection_without_hosts_is_reported() -> None: ) +def test_a_path_scoped_entry_next_to_the_bare_host_is_reported() -> None: + """The bare entry admits every path, so the scoped one reads as a + restriction it does not make.""" + entry = connection(hosts=["shared.example.com", "shared.example.com/api/"]) + assert any( + "shared.example.com/api/" in problem + for problem in connection_problems(entry, Registry([entry])) + ) + + +def test_a_base_url_on_the_scoped_host_is_reported_as_the_source() -> None: + """base_url puts its host on the list bare; a scoped entry cannot narrow it.""" + entry = connection( + hosts=["shared.example.com/api/"], base_url="https://shared.example.com" + ) + assert any( + "base_url" in problem + for problem in connection_problems(entry, Registry([entry])) + ) + + +def test_a_path_scoped_entry_without_the_bare_host_is_not_reported() -> None: + entry = connection(hosts=["api.example.com", "shared.example.com/api/"]) + assert connection_problems(entry, Registry([entry])) == [] + + def test_mcp_host_must_be_a_declared_host() -> None: entry = connection(mcp={"url": "https://mcp.example.com/mcp"}) assert any( @@ -190,6 +295,26 @@ def test_mcp_host_must_be_a_declared_host() -> None: assert connection_problems(good, Registry([good])) == [] +def test_mcp_url_below_a_path_scoped_host_is_a_declared_host() -> None: + entry = connection( + hosts=["shared.example.com/api/"], + mcp={"url": "https://shared.example.com/api/mcp"}, + ) + assert connection_problems(entry, Registry([entry])) == [] + + +def test_mcp_url_beside_the_scoped_path_is_reported() -> None: + """The MCP server is spoken to with the token; beside the path is off-limits.""" + entry = connection( + hosts=["shared.example.com/api/"], + mcp={"url": "https://shared.example.com/mcp"}, + ) + assert any( + "mcp.url" in problem + for problem in connection_problems(entry, Registry([entry])) + ) + + def test_examples_are_checked_against_accepts_token() -> None: entry = connection( token_prefixes=["ex_"], examples={"accepts": ["other_1"], "rejects": ["ex_1"]} diff --git a/tests/test_connections_command.py b/tests/test_connections_command.py index 86e455d..c0e66e7 100644 --- a/tests/test_connections_command.py +++ b/tests/test_connections_command.py @@ -6,8 +6,8 @@ from conftest import ProjectBuilder from gete.cli import main -from gete.connection import Registry -from gete.connections_listing import connections_table +from gete.connection import Connection, Registry +from gete.connections_listing import connections_table, format_connection def test_table_lists_every_connection_with_hosts_and_verification() -> None: @@ -40,6 +40,27 @@ def test_table_includes_private_connections_and_marks_their_source() -> None: assert rows["freee"]["source"] == "catalog" +def test_the_description_shows_the_menu_next_to_the_default_scopes() -> None: + """Preparing the OAuth client takes everything an agent may select, too.""" + entry = Connection.from_mapping( + { + "id": "example", + "display_name": "Example", + "hosts": ["api.example.com"], + "oauth": { + "authorization_url": "https://auth.example.com/authorize", + "token_url": "https://auth.example.com/token", + "scopes": {"read": "Read data"}, + "optional_scopes": {"write": "Change data"}, + }, + } + ) + output = format_connection(entry) + assert "read: Read data" in output + assert "write: Change data" in output + assert "optional scopes" in output + + def test_cli_prints_one_line_per_connection(project: ProjectBuilder) -> None: runner = CliRunner() with runner.isolated_filesystem(temp_dir=project.root): diff --git a/tests/test_declaration.py b/tests/test_declaration.py new file mode 100644 index 0000000..da457e1 --- /dev/null +++ b/tests/test_declaration.py @@ -0,0 +1,30 @@ +"""Typed access to agent.yaml: both spellings of a connection entry.""" + +from pathlib import Path + +from gete.declaration import Agent + +WRITE_SHEETS = "https://www.googleapis.com/auth/spreadsheets" + + +def agent_with(connections: list[object]) -> Agent: + return Agent( + directory=Path("agents/mail-triage"), + data={"name": "mail-triage", "connections": connections}, + ) + + +def test_connections_return_the_id_whichever_form_the_entry_uses() -> None: + agent = agent_with(["freee", {"id": "google", "scopes": [WRITE_SHEETS]}]) + assert agent.connections == ("freee", "google") + + +def test_scope_selections_come_only_from_entries_that_select() -> None: + agent = agent_with(["freee", {"id": "google", "scopes": [WRITE_SHEETS]}]) + assert agent.scope_selections == {"google": (WRITE_SHEETS,)} + + +def test_an_agent_without_connections_selects_nothing() -> None: + agent = Agent(directory=Path("agents/mail-triage"), data={"name": "mail-triage"}) + assert agent.connections == () + assert agent.scope_selections == {} diff --git a/tests/test_register.py b/tests/test_register.py index 92eed53..47886b4 100644 --- a/tests/test_register.py +++ b/tests/test_register.py @@ -88,6 +88,62 @@ def test_authorization_uri_copies_a_verbatim_query_when_declared() -> None: ) +MENU: dict[str, Any] = { + "id": "example", + "display_name": "Example", + "hosts": ["api.example.com"], + "token_prefixes": ["ex_"], + "oauth": { + "authorization_url": "https://auth.example.com/authorize", + "token_url": "https://auth.example.com/token", + "scopes": {"read": "Read data"}, + "optional_scopes": {"write": "Change data", "admin": "Administer data"}, + }, +} + + +def test_authorization_uri_appends_the_selection_after_the_defaults() -> None: + entry = Connection.from_mapping(MENU) + params = query(authorization_uri(entry, "c", ["write"])) + assert params["scope"] == ["read write"] + + +def test_without_a_selection_the_defaults_stand_alone() -> None: + entry = Connection.from_mapping(MENU) + params = query(authorization_uri(entry, "c")) + assert params["scope"] == ["read"] + + +def test_a_selection_outside_the_menu_is_refused() -> None: + """register may run without validate; the menu is the reviewed ceiling.""" + entry = Connection.from_mapping(MENU) + with pytest.raises(DeclarationError, match="optional_scopes"): + authorization_uri(entry, "c", ["repo"]) + + +def test_a_selection_next_to_a_verbatim_query_is_refused() -> None: + """The query is used as written, so the selection would silently go missing.""" + entry = Connection.from_mapping( + { + **MENU, + "oauth": { + **MENU["oauth"], + "authorization_query": {"response_type": "code"}, + }, + } + ) + with pytest.raises(DeclarationError, match="authorization_query"): + authorization_uri(entry, "c", ["write"]) + + +def test_authorization_body_carries_the_selection_into_the_uri() -> None: + body = authorization_body( + GE, "finance-agent", Connection.from_mapping(MENU), "id-1", "s", ["write"] + ) + params = query(body["serverSideOauth2"]["authorizationUri"]) + assert params["scope"] == ["read write"] + + def test_authorization_body_is_named_per_agent_and_carries_the_client() -> None: body = authorization_body( GE, "finance-agent", CATALOG.get("freee"), "id-1", "secret-1" @@ -274,6 +330,71 @@ def test_authorization_is_created_when_absent_and_a_notice_is_written( assert "
" in text +def test_the_registrar_builds_each_authorization_from_that_agents_selection( + project: ProjectBuilder, gcp: FakeGcp, tmp_path: Path +) -> None: + project.write_project( + { + "version": 1, + "project": "example-project", + "location": "us-central1", + "gemini_enterprise": {"project_number": NUMBER}, + "connections": {"example": {k: v for k, v in MENU.items() if k != "id"}}, + } + ) + project.write_agent( + "finance", + { + **FINANCE, + "connections": [{"id": "example", "scopes": ["write"]}], + }, + ) + for suffix, value in (("client-id", "id-1"), ("client-secret", "secret-1")): + gcp.route( + "GET", + f"{SECRETS}/ge-oauth-example-{suffix}/versions/latest:access", + secret(value), + ) + summary = register_project( + load_project(project.root / "gete.yaml"), gcp, tmp_path / "n.md" + ) + assert summary.failed == [] + posts = gcp.writes("POST") + assert posts[0][1] == {"authorizationId": "finance-example"} + params = query(posts[0][2]["serverSideOauth2"]["authorizationUri"]) + assert params["scope"] == ["read write"] + + +def test_the_registrar_refuses_a_connection_declared_in_both_forms( + project: ProjectBuilder, gcp: FakeGcp, tmp_path: Path +) -> None: + """The schema cannot see the two forms are one connection, register can run + without validate, and one entry's selection would stand for both silently.""" + project.write_project( + { + "version": 1, + "project": "example-project", + "location": "us-central1", + "gemini_enterprise": {"project_number": NUMBER}, + "connections": {"example": {k: v for k, v in MENU.items() if k != "id"}}, + } + ) + project.write_agent( + "finance", + { + **FINANCE, + "connections": ["example", {"id": "example", "scopes": ["write"]}], + }, + ) + summary = register_project( + load_project(project.root / "gete.yaml"), gcp, tmp_path / "n.md" + ) + assert summary.failed == ["finance"] + assert any("twice" in message for message in summary.messages) + assert gcp.writes("POST") == [] + assert gcp.writes("PATCH") == [] + + def test_existing_authorization_is_updated_not_recreated( project: ProjectBuilder, gcp: FakeGcp, tmp_path: Path ) -> None: diff --git a/tests/test_schema.py b/tests/test_schema.py index e4b2b20..412577e 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -157,6 +157,111 @@ def test_connections_must_be_unique() -> None: ) +def test_a_connection_entry_may_select_scopes() -> None: + validate_document( + "agent", + { + **AGENT, + "connections": [ + "freee", + { + "id": "google", + "scopes": ["https://www.googleapis.com/auth/spreadsheets"], + }, + ], + }, + source="agent.yaml", + ) + + +@pytest.mark.parametrize( + "entry", + [ + {"scopes": ["https://www.googleapis.com/auth/spreadsheets"]}, + {"id": "google", "scope": ["https://www.googleapis.com/auth/spreadsheets"]}, + {"id": "google", "scopes": []}, + {"id": "google", "scopes": ["a", "a"]}, + {"id": "Google", "scopes": ["a"]}, + ], +) +def test_a_connection_entry_needs_an_id_and_a_real_selection( + entry: dict[str, Any], +) -> None: + """An entry that selects nothing is written as the plain string instead.""" + with pytest.raises(DeclarationError, match="connections"): + validate_document( + "agent", {**AGENT, "connections": [entry]}, source="agent.yaml" + ) + + +def test_a_host_may_be_scoped_to_a_path_prefix() -> None: + """One host can serve unrelated APIs; the entry admits one API's paths.""" + validate_document( + "connection", + { + **CONNECTION, + "hosts": [ + "www.example.com/api/", + "www.example.com/up/api/", + "www.example.com/api.v3/", + ], + }, + source="connection.yaml", + ) + + +@pytest.mark.parametrize( + "host", + [ + "www.example.com/../", + "www.example.com/./", + "www.example.com/api/../", + "www.example.com/./api/", + ], +) +def test_a_path_scoped_host_may_not_contain_dot_segments(host: str) -> None: + """allows() refuses every request that carries a dot segment, so such an + entry could never match; refusing it here says so at declaration time.""" + with pytest.raises(DeclarationError, match="hosts"): + validate_document( + "connection", {**CONNECTION, "hosts": [host]}, source="connection.yaml" + ) + + +@pytest.mark.parametrize( + "host", + [ + "www.example.com/api", + "www.example.com/API/", + "www.example.com//", + "www.example.com/", + "Www.example.com/api/", + ], +) +def test_a_path_scoped_host_ends_in_a_slash_and_stays_lowercase(host: str) -> None: + """Without the trailing slash /api would also admit /api-and-more.""" + with pytest.raises(DeclarationError, match="hosts"): + validate_document( + "connection", {**CONNECTION, "hosts": [host]}, source="connection.yaml" + ) + + +def test_optional_scopes_map_a_scope_to_its_explanation() -> None: + oauth = {**CONNECTION["oauth"], "optional_scopes": {"write": "Change data"}} + validate_document( + "connection", {**CONNECTION, "oauth": oauth}, source="connection.yaml" + ) + + +def test_an_optional_scope_needs_a_non_empty_explanation() -> None: + """The explanation is what the consent screen shows; a blank one hides the grant.""" + oauth = {**CONNECTION["oauth"], "optional_scopes": {"write": ""}} + with pytest.raises(DeclarationError, match="optional_scopes"): + validate_document( + "connection", {**CONNECTION, "oauth": oauth}, source="connection.yaml" + ) + + @pytest.mark.parametrize( "tool", [ diff --git a/tests/test_validate.py b/tests/test_validate.py index 472f93e..d78c779 100644 --- a/tests/test_validate.py +++ b/tests/test_validate.py @@ -76,6 +76,87 @@ def test_duplicate_connection_is_reported(project: ProjectBuilder) -> None: assert any("connections" in p for p in problems(project)) +# INTERNAL_API with a menu an agent may select write access from. +MENU_API: dict[str, Any] = { + **INTERNAL_API, + "oauth": { + **INTERNAL_API["oauth"], + "optional_scopes": {"write": "Change internal data"}, + }, +} + + +def write_menu_api(project: ProjectBuilder) -> None: + project.write_project( + { + "version": 1, + "project": "example-project", + "location": "us-central1", + "connections": {"internal-api": MENU_API}, + } + ) + + +def test_a_selection_from_the_menu_passes(project: ProjectBuilder) -> None: + write_menu_api(project) + project.write_agent( + "mail-triage", + {"connections": [{"id": "internal-api", "scopes": ["write"]}]}, + ) + assert problems(project) == [] + + +def test_a_scope_outside_the_menu_is_reported_with_the_menu( + project: ProjectBuilder, +) -> None: + write_menu_api(project) + project.write_agent( + "mail-triage", + {"connections": [{"id": "internal-api", "scopes": ["admin"]}]}, + ) + found = problems(project) + assert any("admin" in p and "write" in p for p in found) + + +def test_a_selection_on_a_connection_without_a_menu_is_reported( + project: ProjectBuilder, +) -> None: + write_internal_api(project) + project.write_agent( + "mail-triage", + {"connections": [{"id": "internal-api", "scopes": ["write"]}]}, + ) + assert any("optional_scopes" in p for p in problems(project)) + + +def test_the_same_connection_in_both_forms_is_reported( + project: ProjectBuilder, +) -> None: + """uniqueItems cannot see that the string and the mapping name one connection.""" + write_menu_api(project) + project.write_agent( + "mail-triage", + { + "connections": [ + "internal-api", + {"id": "internal-api", "scopes": ["write"]}, + ] + }, + ) + assert any("twice" in p for p in problems(project)) + + +def test_a_duplicate_is_reported_even_when_the_connection_is_unknown( + project: ProjectBuilder, +) -> None: + """A typo repeated in both forms is one mistake, not two unknown connections.""" + project.write_agent( + "mail-triage", + {"connections": ["salesforce", {"id": "salesforce", "scopes": ["write"]}]}, + ) + assert any("twice" in p for p in problems(project)) + + def test_authorization_id_longer_than_63_is_reported(project: ProjectBuilder) -> None: """- becomes an authorization id, which is a DNS-style label.""" name = "a" * 58