diff --git a/backend/druks/agents.py b/backend/druks/agents.py index 4cedcfef..6b8edf2e 100644 --- a/backend/druks/agents.py +++ b/backend/druks/agents.py @@ -31,6 +31,7 @@ from druks.sandbox.client import provisioning_key, sandbox_client from druks.sandbox.models import SandboxIdentity, SecretRef from druks.sandbox.templates import get_template_id +from druks.secrets.enums import SecretKind from druks.settings import load_settings from druks.usage.models import UsageScrape from druks.workflows import _in_step, current_workflow @@ -172,6 +173,10 @@ def __post_init__(self) -> None: f"{declared} goes to a sandbox, so {secret.service.__name__} must declare " "`host`: the one host the secret may be sent to" ) + if secret.service.secret_kind != SecretKind.STATIC: + raise TypeError( + f"{declared} belongs to an App key, which issues tokens; no sandbox holds it" + ) if self.id: # an explicit id means a standalone agent — it registers itself now agents.register(self) diff --git a/backend/druks/chat/service.py b/backend/druks/chat/service.py index 2cd7ecff..660743f7 100644 --- a/backend/druks/chat/service.py +++ b/backend/druks/chat/service.py @@ -14,7 +14,7 @@ from druks.accounts.enums import AccountKind from druks.apps.loader import get_app -from druks.apps.registry import channels +from druks.apps.registry import channels, services from druks.durable.engine import step_session from druks.durable.models import Run from druks.files.constants import MAX_UPLOAD_BYTES @@ -39,6 +39,8 @@ from druks.sandbox.layout import get_remote_home, get_work_root from druks.sandbox.models import SandboxIdentity, SecretRef from druks.sandbox.templates import get_template_id +from druks.secrets.datastructures import Audience +from druks.secrets.models import VaultSecret from druks.services.exceptions import ServiceNotConnectedError from druks.workspaces import Workspace @@ -107,7 +109,7 @@ async def get_sandbox( *, config: AgentConfig, allowed_tools: AllowedTools, - mcp_secret_refs: list[SecretRef], + secret_refs: list[SecretRef], ) -> tuple[Host, SandboxIdentity]: """The account's sandbox for a new turn: a live sandbox that holds the Chat agent's current secrets, or a new one.""" @@ -115,7 +117,7 @@ async def get_sandbox( token = await get_druks_account_token(session, account_id, allowed_tools, name=CHAT_KEY_NAME) refs = [ *config.secret_refs, - *mcp_secret_refs, + *secret_refs, SecretRef( name=get_bearer_token_env_var(server.name).lower(), secret_id=token.id, @@ -208,19 +210,32 @@ async def deliver_pending(session: AsyncSession, conversation: Conversation) -> f"Chat runs on {adapters}. The Bot's settings select " f"{config.harness_class.name}. Set its harness to one of them." ) - # A bot serves outside people, so only the operator reaches the enabled MCP servers. - mcp_servers, mcp_secret_refs = (), [] + # A bot serves outside people, so only the operator reaches the enabled MCP + # servers and holds their own sign-ins. + mcp_servers, secret_refs = (), [] if conversation.account.kind == AccountKind.OPERATOR: # A server the operator has not connected must not stop the chat. - mcp_servers, mcp_secret_refs = await Workspace.get_all_mcp_servers( + mcp_servers, secret_refs = await Workspace.get_all_mcp_servers( session, None, conversation.account_id, skip_unauthenticated=True ) + for service in services.all(): + if service.host and service.token_endpoint: + sign_ins = await VaultSecret.list_account_connections( + session, Audience.service(service.slug), conversation.account_id + ) + # A sandbox has one variable per service, so it holds one sign-in. + secret_refs += [ + SecretRef( + name=service.slug, secret_id=sign_in.id, host=service.host + ) + for sign_in in sign_ins[:1] + ] host, identity = await get_sandbox( session, conversation.account_id, config=config, allowed_tools=tools, - mcp_secret_refs=mcp_secret_refs, + secret_refs=secret_refs, ) try: bridge = Bridge(host) diff --git a/backend/druks/contrib/software_factory/workflows.py b/backend/druks/contrib/software_factory/workflows.py index fc4477dc..f18a4ed4 100644 --- a/backend/druks/contrib/software_factory/workflows.py +++ b/backend/druks/contrib/software_factory/workflows.py @@ -484,9 +484,10 @@ async def get_secrets(cls, subject: Any) -> list[SandboxSecret]: actor = await get_review_actor() return [ SandboxSecret( - name=Github.secret_name, + name=actor.service.slug, secret_id=(await actor.service.get()).id, resource=cls.get_repo(subject), + host=actor.service.host, ) ] diff --git a/backend/druks/core/services.py b/backend/druks/core/services.py index 96a87ede..3756c857 100644 --- a/backend/druks/core/services.py +++ b/backend/druks/core/services.py @@ -26,8 +26,8 @@ class Github(Service): person's sign-in through it links their GitHub account to their Druks account.""" secret_kind = SecretKind.APP_KEY - # The Drukbox catalog name a box holds this identity's token under. - secret_name = "github" + # Drukbox knows this host: a token for it reaches git and gh. + host = "github.com" description = ( "The GitHub App druks acts as. Create it from here, or paste an existing " "App's credentials from the GitHub developer settings page." @@ -105,6 +105,11 @@ async def issue_token(cls, resource: str) -> tuple[str, datetime]: """The installation token for the repo, and the expiry GitHub gave it.""" return await (await cls.get_client()).token_for_repo(resource) + @classmethod + def is_grant_revoked(cls, status: int, tokens: dict[str, Any]) -> bool: + # GitHub answers a dead refresh token with a 200. + return tokens.get("error") == "bad_refresh_token" + @classmethod async def get_identity(cls, access_token: str) -> dict[str, Any]: """The person behind a user token, keyed the way ``Account.lookup`` finds them.""" diff --git a/backend/druks/sandbox/datastructures.py b/backend/druks/sandbox/datastructures.py index 710348e0..4cd3c8bf 100644 --- a/backend/druks/sandbox/datastructures.py +++ b/backend/druks/sandbox/datastructures.py @@ -162,7 +162,8 @@ class SandboxSecret: """A secret a workspace's box holds as a placeholder. ``secret_id`` names the vault row the issuer answers from and ``resource`` what its token is for. A ``host`` makes it a custom entry: the proxy swaps the placeholder in the - request header at that host, and the box reads it from ``name.upper()``.""" + request header at that host, and the box reads it from ``name.upper()``. + Drukbox knows ``github.com``: that entry is its GitHub service.""" name: str secret_id: str diff --git a/backend/druks/sandbox/models.py b/backend/druks/sandbox/models.py index 6bf73f6f..8a5a8e61 100644 --- a/backend/druks/sandbox/models.py +++ b/backend/druks/sandbox/models.py @@ -46,7 +46,8 @@ class SecretRef(Base): # What the token is for: the repo. Empty for a subscription. resource: Mapped[str] = mapped_column(default="") # The host the box's placeholder is swapped at, for a custom entry such - # as an MCP server. Empty for a Drukbox catalog entry. + # as an MCP server, or ``github.com`` for Drukbox's GitHub service. Empty + # for a Drukbox catalog entry. host: Mapped[str] = mapped_column(default="") identity: Mapped["SandboxIdentity"] = relationship(back_populates="secret_refs", lazy="raise") diff --git a/backend/druks/services/base.py b/backend/druks/services/base.py index 62247472..a273c29e 100644 --- a/backend/druks/services/base.py +++ b/backend/druks/services/base.py @@ -16,7 +16,7 @@ from druks.secrets.models import VaultSecret from .exceptions import OauthExchangeError, ServiceConnectError, ServiceNotConnectedError -from .oauth import OauthClient, fetch_identity +from .oauth import OauthClient, fetch_identity, is_grant_revoked # GoogleCalendar -> google_calendar, HTTPServer -> http_server. _CAMEL_BOUNDARY = re.compile(r"(?<=[a-z0-9])(?=[A-Z])|(?<=[A-Z])(?=[A-Z][a-z])") @@ -317,6 +317,12 @@ def read_grant(cls, tokens: dict[str, Any]) -> dict[str, Any]: "scopes": tokens.get("scope", "").split(), } + @classmethod + def is_grant_revoked(cls, status: int, tokens: dict[str, Any]) -> bool: + """Whether the token endpoint's answer to a refresh says the provider revoked + the grant. Override for a provider that reports it otherwise than RFC 6749.""" + return is_grant_revoked(status, tokens) + @classmethod async def get_oauth_client(cls) -> OauthClient: """The connected identity as a configured ``OauthClient``, keyed by @@ -333,6 +339,7 @@ async def get_oauth_client(cls) -> OauthClient: client_secret=connected.secrets["client_secret"], basic_auth=cls.basic_auth, extra_authorize_params=cls.extra_authorize_params, + is_grant_revoked=cls.is_grant_revoked, ) @classmethod diff --git a/backend/druks/services/oauth.py b/backend/druks/services/oauth.py index c6ce4813..a7106e99 100644 --- a/backend/druks/services/oauth.py +++ b/backend/druks/services/oauth.py @@ -3,6 +3,7 @@ import hashlib import json import secrets +from collections.abc import Callable from contextlib import AsyncExitStack from datetime import UTC, datetime, timedelta from typing import Any, cast @@ -32,6 +33,12 @@ def _http() -> httpx.AsyncClient: return httpx.AsyncClient(timeout=30.0, follow_redirects=True) +def is_grant_revoked(status: int, tokens: dict[str, Any]) -> bool: + """Whether a token endpoint's answer says the provider revoked the grant, as RFC + 6749 reports it. A service overrides its own method for a provider that differs.""" + return status != 200 and tokens.get("error") == "invalid_grant" + + async def _post_token( http: httpx.AsyncClient, token_endpoint: str, @@ -84,6 +91,7 @@ class OauthClient: example RFC 8707's ``resource``. ``extra_authorize_params`` go into every consent query, for example Google's ``access_type=offline`` and ``prompt=consent``. Each ``begin_connect`` asks for its own scopes. + ``is_grant_revoked`` reads the token endpoint's answer to a refresh. """ def __init__( @@ -99,6 +107,7 @@ def __init__( extra_authorize_params: dict[str, str] | None = None, mint_wait_interval_seconds: float = OAUTH_MINT_WAIT_INTERVAL_SECONDS, mint_wait_attempts: int = OAUTH_MINT_WAIT_ATTEMPTS, + is_grant_revoked: Callable[[int, dict[str, Any]], bool] = is_grant_revoked, ) -> None: self.provider = provider self.authorization_endpoint = authorization_endpoint @@ -110,6 +119,7 @@ def __init__( self.extra_authorize_params = dict(extra_authorize_params or {}) self.mint_wait_interval_seconds = mint_wait_interval_seconds self.mint_wait_attempts = mint_wait_attempts + self.is_grant_revoked = is_grant_revoked async def begin_connect( self, @@ -241,30 +251,33 @@ async def get_access_token( ) except httpx.HTTPError as error: raise OauthRefreshError(self.provider, str(error)) from error - if response.status_code != 200: - if "invalid_grant" in response.text: - # The provider withdrew the grant; presenting it again can never - # succeed. The revoke commits on its own: the caller's step - # session rolls back when this error propagates. - async with get_session(session.bind) as own: - revoked = await own.get(VaultSecret, connection.id) - await self.disconnect(revoked, reason="invalid_grant") - await own.commit() + try: + tokens = response.json() + except ValueError: + tokens = None + if not isinstance(tokens, dict): + if response.status_code == 200: raise OauthRefreshError( - self.provider, - "the provider revoked the grant; sign in again to restore the connection", + self.provider, "the token endpoint returned malformed JSON" ) + tokens = {} + if self.is_grant_revoked(response.status_code, tokens): + # Presenting the grant again can never succeed. The revoke commits on + # its own: the caller's step session rolls back when this error propagates. + async with get_session(session.bind) as own: + revoked = await own.get(VaultSecret, connection.id) + await self.disconnect(revoked, reason="invalid_grant") + await own.commit() + raise OauthRefreshError( + self.provider, + "the provider revoked the grant; sign in again to restore the connection", + ) + if response.status_code != 200: await redis.delete(token_key) raise OauthRefreshError( self.provider, f"HTTP {response.status_code} from the token endpoint" ) - try: - tokens = response.json() - except ValueError as error: - raise OauthRefreshError( - self.provider, "the token endpoint returned malformed JSON" - ) from error - if not isinstance(tokens, dict) or not tokens.get("access_token"): + if not tokens.get("access_token"): raise OauthRefreshError( self.provider, "the token endpoint returned no access token" ) diff --git a/backend/druks/workspaces.py b/backend/druks/workspaces.py index ceabefe9..9cca3da8 100644 --- a/backend/druks/workspaces.py +++ b/backend/druks/workspaces.py @@ -278,9 +278,10 @@ async def get_secrets(cls, subject: Any) -> list[SandboxSecret]: # issuer reads. A service that is not connected fails here, before the box. return [ SandboxSecret( - name=cls.github.secret_name, + name=cls.github.slug, secret_id=(await cls.github.get()).id, resource=cls.get_repo(subject), + host=cls.github.host, ) ] diff --git a/backend/tests/druks-field_notes/tests/test_workflows.py b/backend/tests/druks-field_notes/tests/test_workflows.py index 191719fe..88db21b0 100644 --- a/backend/tests/druks-field_notes/tests/test_workflows.py +++ b/backend/tests/druks-field_notes/tests/test_workflows.py @@ -48,4 +48,6 @@ async def test_survey_workspace_clones_the_subject_repo(druks_db): "github", identity={"app_id": "1", "slug": "druks-operator"}, secrets={"private_key": "pem"} ) [secret] = await workflow.workspace_class.get_secrets(await workflow.subject) - assert secret == SandboxSecret(name="github", secret_id=row.id, resource="acme/widgets") + assert secret == SandboxSecret( + name="github", secret_id=row.id, resource="acme/widgets", host="github.com" + ) diff --git a/backend/tests/software_factory/test_build_workspace.py b/backend/tests/software_factory/test_build_workspace.py index e6b31202..7c5f3dd6 100644 --- a/backend/tests/software_factory/test_build_workspace.py +++ b/backend/tests/software_factory/test_build_workspace.py @@ -85,7 +85,9 @@ async def test_build_workspace_declares_its_github_mcp_as_the_review_actor(druks assert github.bearer_token_env_var == get_bearer_token_env_var(GITHUB_MCP_NAME) assert ref.key == ("mcp_github_token", reviewer.id, "o/main", "api.githubcopilot.com") [clone] = await BuildWorkspace.get_secrets(subject) - assert clone == SandboxSecret(name="github", secret_id=operator.id, resource="o/main") + assert clone == SandboxSecret( + name="github", secret_id=operator.id, resource="o/main", host="github.com" + ) async def test_get_workspace_kwargs_carries_the_build_fields(): @@ -157,7 +159,9 @@ async def test_review_mcp_and_gh_use_the_review_actor(druks_db): assert github.bearer_token_env_var == get_bearer_token_env_var(GITHUB_MCP_NAME) assert ref.key == ("mcp_github_token", reviewer.id, "o/app", "api.githubcopilot.com") [clone] = await ReviewWorkspace.get_secrets(subject) - assert clone == SandboxSecret(name="github", secret_id=reviewer.id, resource="o/app") + assert clone == SandboxSecret( + name="github_reviewer", secret_id=reviewer.id, resource="o/app", host="github.com" + ) class _IdentitySandbox: diff --git a/backend/tests/software_factory/test_review.py b/backend/tests/software_factory/test_review.py index d44e3f54..7e2e46ec 100644 --- a/backend/tests/software_factory/test_review.py +++ b/backend/tests/software_factory/test_review.py @@ -199,7 +199,7 @@ async def test_an_unconnected_reviewer_borrows_the_operator_in_comment_mode(druk def test_the_reviewer_is_an_optional_service_the_app_declares(): assert (GithubReviewer.slug, GithubReviewer.required) == ("github_reviewer", False) - assert GithubReviewer.secret_name == "github" + assert GithubReviewer.host == "github.com" fields = GithubReviewer.Settings.model_fields assert set(fields) == {"app_id", "private_key"} assert field_kind(fields["private_key"]) == "secret" diff --git a/backend/tests/test_chat.py b/backend/tests/test_chat.py index 0bed93fd..8fcf7a4a 100644 --- a/backend/tests/test_chat.py +++ b/backend/tests/test_chat.py @@ -206,14 +206,14 @@ async def attach(*, host_id): conversation.account_id, config=config, allowed_tools=Toolkit.ALL, - mcp_secret_refs=[], + secret_refs=[], ) second_host, _identity = await service.get_sandbox( druks_db, second.account_id, config=config, allowed_tools=Toolkit.ALL, - mcp_secret_refs=[], + secret_refs=[], ) login = await get_druks_account_token(druks_db, conversation.account_id, (), name="login") moved = SimpleNamespace( @@ -224,7 +224,7 @@ async def attach(*, host_id): second.account_id, config=moved, allowed_tools=Toolkit.ALL, - mcp_secret_refs=[], + secret_refs=[], ) assert first_host.id == second_host.id @@ -485,7 +485,7 @@ async def download(**values): @pytest.mark.parametrize( "kind, expected", [(AccountKind.OPERATOR, [LINEAR]), (AccountKind.BOT, [])] ) -async def test_only_the_operator_reaches_the_connected_mcp_servers( +async def test_only_the_operator_reaches_the_connected_mcp_servers_and_holds_their_sign_in( druks_db, conversation, sandbox, monkeypatch, kind, expected ): await McpServer.create( @@ -495,6 +495,13 @@ async def test_only_the_operator_reaches_the_connected_mcp_servers( secret_headers={"Authorization": "Bearer lin_secret"}, ) await McpServer.create(druks_db, name="sentry", url="https://mcp.sentry.dev/mcp", is_oauth=True) + sign_in = await VaultSecret.connect( + druks_db, + Audience.service("github"), + account_id=conversation.account_id, + refresh_token="ghr_one", + scopes=[], + ) conversation.account.kind = kind starts = [] @@ -512,8 +519,11 @@ async def follow_turn(session, conversation, message, bridge): await service.deliver_pending(druks_db, conversation) - [refs] = [call.kwargs["mcp_secret_refs"] for call in service.get_sandbox.await_args_list] - assert len(refs) == len(expected) + [refs] = [call.kwargs["secret_refs"] for call in service.get_sandbox.await_args_list] + assert [ref.name for ref in refs] == ["mcp_linear_header_0", "github"] * len(expected) + assert [ref.key for ref in refs if ref.name == "github"] == [ + ("github", sign_in.id, "", "github.com") + ] * len(expected) assert starts[0]["mcpServers"] == expected @@ -939,7 +949,7 @@ async def test_stop_during_startup_keeps_the_sandbox_and_sends_only_the_next_mes sandbox_requests = 0 prompts = [] - async def get_sandbox(session, account_id, *, config, allowed_tools, mcp_secret_refs): + async def get_sandbox(session, account_id, *, config, allowed_tools, secret_refs): nonlocal sandbox_requests sandbox_requests += 1 await session.commit() diff --git a/backend/tests/test_oauth_client.py b/backend/tests/test_oauth_client.py index 89c5f6b1..d6577acf 100644 --- a/backend/tests/test_oauth_client.py +++ b/backend/tests/test_oauth_client.py @@ -5,6 +5,7 @@ import httpx import pytest +from druks.core.services import Github from druks.db import db_session from druks.redis import get_client from druks.secrets.datastructures import Audience @@ -186,6 +187,24 @@ async def record(name, **kwargs): assert not published +async def test_a_service_reads_its_own_revoked_answer(token_endpoint, monkeypatch): + token_endpoint.response = {"error": "bad_refresh_token", "error_description": "expired"} + client = _client(is_grant_revoked=Github.is_grant_revoked) + published = [] + + async def record(name, **kwargs): + published.append(name) + + monkeypatch.setattr("druks.services.oauth.publish", record) + connection = await _connection() + + with pytest.raises(OauthRefreshError, match="revoked the grant"): + await client.get_access_token(db_session(), connection=connection) + + assert not await VaultSecret.reload(db_session(), connection.id) + assert published == ["oauth.disconnected"] + + async def test_get_refresh_uses_basic_auth(token_endpoint): connection = await _connection() diff --git a/backend/tests/test_services.py b/backend/tests/test_services.py index 81dd3895..1f76709d 100644 --- a/backend/tests/test_services.py +++ b/backend/tests/test_services.py @@ -566,6 +566,8 @@ class Settings(BaseModel): Acme.host = "api.acme.test" with pytest.raises(TypeError, match="Acme.fields.base_url is not a secret field"): Agent(contract=AgentOutput, secrets=(Acme.fields.base_url,)) + with pytest.raises(TypeError, match="Github.fields.private_key belongs to an App key"): + Agent(contract=AgentOutput, secrets=(Github.fields.private_key,)) async def test_oauth_service_declarations_fail_loudly(declared_services): diff --git a/backend/tests/test_workspaces.py b/backend/tests/test_workspaces.py index 8620f93e..2654bf06 100644 --- a/backend/tests/test_workspaces.py +++ b/backend/tests/test_workspaces.py @@ -80,7 +80,9 @@ async def test_repo_workspace_names_its_github_secret_and_repo_before_the_box_ex [secret] = await RepoWorkspace.get_secrets(subject) - assert secret == SandboxSecret(name="github", secret_id=row.id, resource="acme/widgets") + assert secret == SandboxSecret( + name="github", secret_id=row.id, resource="acme/widgets", host="github.com" + ) assert await Workspace.get_secrets(subject) == [] diff --git a/docs/chat.md b/docs/chat.md index 1a33b9cc..afca5fac 100644 --- a/docs/chat.md +++ b/docs/chat.md @@ -108,6 +108,14 @@ leaves out a server that cannot authenticate for you, such as an OAuth server that you have not connected. A Bot's agent reaches only the Druks server. When you enable, disable, or connect a server, the next turn replaces the sandbox. +An operator's sandbox also holds their own sign-in at each service that names a +host, so the command line acts as that person. With a GitHub sign-in, git and +`gh` act as you: your name is on the issues and comments, and the token permits +only what both you and the App can do. Druks refreshes the token before it +expires. Without a sign-in the sandbox has no access to that service: see +[GitHub](#github). A Bot's sandbox never holds a sign-in. +A build still acts as the App: see [GitHub](configuration.md#github). + The agent can change Druks through those tools. Chat has no permission dialog or proposal mode. Claude runs in bypass mode and cannot call `AskUserQuestion`. Codex runs in full-access mode for an operator. For a Bot it runs in read-only @@ -369,8 +377,8 @@ while the GitHub card is connected: see [GitHub](configuration.md#github). Druks knows you by your GitHub account. Select **Connect GitHub** on the GitHub pane. GitHub asks you to authorize the App, and Druks saves your GitHub sign-in -under your account, like a Gmail connection. Other apps can use that grant. -**Disconnect** revokes it. +under your account, like a Gmail connection. Other apps can use that grant, and +your Chat sandbox acts on GitHub with it. **Disconnect** revokes it. When you tag the App before you connect, the App answers once in the thread: connect GitHub in Druks, then tag it again. It holds nothing. diff --git a/docs/writing-an-app.md b/docs/writing-an-app.md index eec72719..48b1809b 100644 --- a/docs/writing-an-app.md +++ b/docs/writing-an-app.md @@ -569,6 +569,11 @@ that creates the sandbox. Give the agents of such a workflow the same `secrets`. For a credential that depends on the subject, such as one account's connection, override [`get_secrets(subject)`](#customize-the-workspace) on the workspace. +A person's own sign-in needs no list. If a service has OAuth endpoints and a +`host`, the Chat sandbox of each operator holds that operator's sign-in under +the service slug, the way it holds a listed field. An operator with no sign-in +gets no variable. + Do not ask the framework to infer domain side effects from agent prose. The prompt or a subsequent explicit step owns those actions. @@ -762,8 +767,9 @@ The secret names the vault row the issuer answers from. With a `host`, it is a custom entry. The sandbox holds a placeholder in `BILLING_TOKEN`, the name in upper case. The secrets proxy puts the value in a request header only for that host. A header row supplies its own header. Any other row goes out as -`Authorization: Bearer `. Without a `host`, the name is a Drukbox -catalog entry such as `github`, and Drukbox sets its variable and hosts. +`Authorization: Bearer `. An entry for `github.com` is Drukbox's GitHub +service under any name: `GH_TOKEN`, with git and `gh` set up. Without a `host`, +the name is a Drukbox catalog entry, and Drukbox sets its variable and hosts. `resource` tells the issuer what the token is for, such as a repo. Druks reads the secrets before the sandbox exists, so read them from the subject alone.