diff --git a/docs/architecture.md b/docs/architecture.md index e88fcef..b3bad25 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -4,7 +4,7 @@ smorg is a keyboard-driven terminal dashboard: each connected integration is a tab, nothing is enabled by default, and the app is read-plus-safe-actions — it shows what's on your plate and opens things, it never writes to a service. -This document explains the load-bearing decisions. How to *add* an integration +This document explains the load-bearing decisions. How to _add_ an integration is covered in [CONTRIBUTING.md](../CONTRIBUTING.md). ## Three layers, two seams @@ -65,15 +65,14 @@ easiest way to misread this design: The current integrations occupy diagonal corners, which makes the two axes look like one: -| | MCP transport | REST transport | -| ---------------- | ------------- | -------------- | -| **OAuth** | Linear | — | -| **Pasted token** | — | GitHub | +| | MCP transport | REST transport | +| ---------------- | ------------- | ------------------------ | +| **OAuth** | Linear | Spotify, Google Calendar | +| **Pasted token** | — | GitHub | -The empty corners are circumstance, not design. OAuth + REST is where any -classic OAuth provider with no token alternative lands (Spotify is the -roadmap's first candidate); token + MCP would be an MCP server reached with a -static bearer token. +The empty corner is circumstance, not design. OAuth + REST is where a classic +OAuth provider with no token alternative lands; token + MCP would be an MCP +server reached with a static bearer token. ### The auth axis: OAuth where it is cheap, a pasted token where it is not @@ -90,6 +89,16 @@ carries the endpoints, and the user creates the OAuth app themselves and pastes its client id. That per-user setup step is worth it only when the provider issues no token a user could paste instead. +A provider that offers neither discovery nor a pasteable token, and whose own +console makes per-user app creation a chore, is declared as a +`BundledProvider`: smorg's maintainer registers the app once and the client id +and secret ship with the build, the pattern every desktop calendar client uses. +The secret is not treated as confidential, since an installed app cannot keep +one, and it never reaches output. Config records no client id for a bundled +tab; the id belongs to the build. Google Calendar is the first bundled +provider: Google rejects a secretless code exchange for a desktop client, +issues refresh tokens without extra parameters, and accepts any loopback port. + GitHub publishes no metadata document and registers no clients, so an OAuth tab there would need an app somebody registered by hand and a client id configured before the first login — a setup step per user, to arrive where one @@ -119,7 +128,7 @@ own source module, and why the allowlist is a feature: when a server changes its output, exactly one source breaks, its tab shows the failure, and every other tab keeps working. -Nothing versions MCP tool output, so every source treats response *shape* as +Nothing versions MCP tool output, so every source treats response _shape_ as untrusted alongside content: a field that should be an object may be a string, and that must degrade one tab (`Malformed`), never crash the app. The protocol revision we speak, and the upgrade path, live in [mcp-protocol.md](mcp-protocol.md). diff --git a/src/smorg/auth/login.py b/src/smorg/auth/login.py index 268c3f0..0bdfb39 100644 --- a/src/smorg/auth/login.py +++ b/src/smorg/auth/login.py @@ -100,12 +100,14 @@ def perform_login( try: redirect_uri = f"http://127.0.0.1:{server.server_port}/callback" metadata = oauth.resolve_metadata(client, method) + client_id = oauth.client_id_for(method, client_id) if client_id is None: provider = method.provider if isinstance(provider, oauth.StaticProvider): raise oauth.OAuthError( "this provider cannot register clients; connect with a client id" ) + assert isinstance(provider, oauth.DiscoveredProvider) client_id = oauth.register_client( client, metadata, provider, oauth.REGISTERED_REDIRECT_URI ) @@ -135,8 +137,15 @@ def perform_login( if "error" in received: raise oauth.OAuthError(f"authorization was refused: {sanitize_line(received['error'])}") + client_secret = oauth.client_secret_of(method) credentials = oauth.exchange_code( - client, metadata, client_id, received["code"], verifier, redirect_uri + client, + metadata, + client_id, + received["code"], + verifier, + redirect_uri, + client_secret=client_secret, ) return client_id, credentials finally: diff --git a/src/smorg/auth/oauth.py b/src/smorg/auth/oauth.py index 4b534b2..2f399f0 100644 --- a/src/smorg/auth/oauth.py +++ b/src/smorg/auth/oauth.py @@ -23,6 +23,7 @@ __all__ = [ "REGISTERED_REDIRECT_URI", "REGISTRATION_PORT", + "BundledProvider", "DiscoveredProvider", "OAuthError", "OAuthMethod", @@ -30,6 +31,8 @@ "StaticProvider", "build_authorize_url", "callback_port", + "client_id_for", + "client_secret_of", "discover", "exchange_code", "extra_scopes_warning", @@ -73,11 +76,20 @@ class StaticProvider: setup_hint: str +@dataclass(frozen=True) +class BundledProvider: + """smorg registered the OAuth app itself; the client id and secret ship with the build.""" + + metadata: ServerMetadata + client_id: str + client_secret: str + + @dataclass(frozen=True) class OAuthMethod: """Authorize in the browser against provider, requesting scopes.""" - provider: DiscoveredProvider | StaticProvider + provider: DiscoveredProvider | StaticProvider | BundledProvider scopes: tuple[str, ...] @@ -85,6 +97,20 @@ class OAuthError(Exception): """A registration, token, or discovery request failed. Never carries a token.""" +def client_secret_of(method: OAuthMethod) -> str | None: + if isinstance(method.provider, BundledProvider): + return method.provider.client_secret + return None + + +def client_id_for(method: OAuthMethod, recorded: str | None) -> str | None: + """The client id a login, refresh, or revocation uses: the build's own for a bundled provider, + otherwise whatever config recorded.""" + if isinstance(method.provider, BundledProvider): + return method.provider.client_id + return recorded + + def _json_object(response: httpx.Response, source: str) -> _JsonObject: """Decode a response body that must be a JSON object.""" try: @@ -130,7 +156,7 @@ def discover(client: httpx.Client, provider: DiscoveredProvider) -> ServerMetada def resolve_metadata(client: httpx.Client, method: OAuthMethod) -> ServerMetadata: - if isinstance(method.provider, StaticProvider): + if isinstance(method.provider, StaticProvider | BundledProvider): return method.provider.metadata return discover(client, method.provider) @@ -166,7 +192,7 @@ def register_client( def callback_port(method: OAuthMethod) -> int: # A hand-registered app pins its redirect URI, so a static provider binds the registered - # port exactly; discovered providers accept any loopback port (RFC 8252 §7.3). + # port exactly; discovered and bundled providers accept any loopback port (RFC 8252 §7.3). if isinstance(method.provider, StaticProvider): return REGISTRATION_PORT return 0 @@ -240,8 +266,13 @@ def _credentials_from_token_response( def _post_token( - client: httpx.Client, metadata: ServerMetadata, form: dict[str, str] + client: httpx.Client, + metadata: ServerMetadata, + form: dict[str, str], + client_secret: str | None = None, ) -> _JsonObject: + if client_secret is not None: + form = form | {"client_secret": client_secret} # Binds the issued token to the protected resource; omit it and the token # carries the wrong audience — rejected later at the API, not here. if metadata.resource: @@ -266,6 +297,7 @@ def exchange_code( code: str, verifier: str, redirect_uri: str, + client_secret: str | None = None, ) -> Credentials: payload = _post_token( client, @@ -277,6 +309,7 @@ def exchange_code( "client_id": client_id, "code_verifier": verifier, }, + client_secret, ) return _credentials_from_token_response(payload, fallback_refresh=None) @@ -286,6 +319,7 @@ def refresh_credentials( metadata: ServerMetadata, client_id: str, credentials: Credentials, + client_secret: str | None = None, ) -> Credentials: if credentials.refresh_token is None: raise OAuthError("no refresh token available; re-run smorg connect") @@ -297,6 +331,7 @@ def refresh_credentials( "refresh_token": credentials.refresh_token, "client_id": client_id, }, + client_secret, ) return _credentials_from_token_response(payload, fallback_refresh=credentials.refresh_token) diff --git a/src/smorg/auth/refresh.py b/src/smorg/auth/refresh.py index 2b9eb4a..070ebd1 100644 --- a/src/smorg/auth/refresh.py +++ b/src/smorg/auth/refresh.py @@ -66,6 +66,7 @@ def fresh_credentials( credentials = get_credentials(integration_id) if credentials is None or not _expiring(credentials): return credentials + client_id = oauth.client_id_for(method, client_id) if credentials.refresh_token is None or client_id is None: return credentials lock = _lock_for(integration_id) @@ -75,7 +76,10 @@ def fresh_credentials( return credentials try: metadata = oauth.resolve_metadata(http, method) - refreshed = oauth.refresh_credentials(http, metadata, client_id, credentials) + client_secret = oauth.client_secret_of(method) + refreshed = oauth.refresh_credentials( + http, metadata, client_id, credentials, client_secret=client_secret + ) except OAuthError as error: raise AuthExpired(f"token refresh failed ({error})") from error # A store failure here propagates as CredentialStoreError on purpose: diff --git a/src/smorg/cli.py b/src/smorg/cli.py index 4c14f34..051a59d 100644 --- a/src/smorg/cli.py +++ b/src/smorg/cli.py @@ -57,6 +57,16 @@ def on_authorize_url(url: str) -> None: ) +def _tab_config_for(integration_id: str, path: AuthPath, client_id: str) -> TabConfig: + """The config entry a connect records; a bundled client's id belongs to the build, not the + config.""" + if isinstance(path.method, oauth.OAuthMethod) and isinstance( + path.method.provider, oauth.BundledProvider + ): + return TabConfig(integration=integration_id, connection=path.id) + return TabConfig(integration=integration_id, client_id=client_id, connection=path.id) + + def _connect(integration_id: str) -> int: try: integration = get_integration(integration_id) @@ -106,7 +116,7 @@ def _connect(integration_id: str) -> int: print(str(error), file=sys.stderr) return 1 - tab_config = TabConfig(integration=integration_id, client_id=client_id, connection=path.id) + tab_config = _tab_config_for(integration_id, path, client_id) save_config(add_tab(config, tab_config)) print(f"connected {integration.manifest.display_name} (scope: {credentials.scope})") return 0 diff --git a/src/smorg/core/removal.py b/src/smorg/core/removal.py index d0490e0..7decde4 100644 --- a/src/smorg/core/removal.py +++ b/src/smorg/core/removal.py @@ -58,15 +58,16 @@ def remove_integration(integration_id: str) -> RemovalResult: get_integration(integration_id) revoked = False - if integration is not None and credentials is not None and tab is not None and tab.client_id: + if integration is not None and credentials is not None and tab is not None: try: path = integration.manifest.connection(tab.connection) except ValueError: path = None # a stale connection id must not block deletion - # Only try to revoke OAuth tokens: a pasted token has no provider to ask if path is not None and isinstance(path.method, oauth.OAuthMethod): - revoked = revoke_best_effort(path.method, tab.client_id, credentials) + client_id = oauth.client_id_for(path.method, tab.client_id) + if client_id is not None: + revoked = revoke_best_effort(path.method, client_id, credentials) # Credentials before config: dropping the tab first could strand credentials with nothing left # pointing at them. diff --git a/src/smorg/shell/menu/connect.py b/src/smorg/shell/menu/connect.py index c24bb1b..d8883ff 100644 --- a/src/smorg/shell/menu/connect.py +++ b/src/smorg/shell/menu/connect.py @@ -17,6 +17,7 @@ from smorg.auth.login import LoginCancelled, perform_login from smorg.auth.oauth import ( REGISTERED_REDIRECT_URI, + BundledProvider, OAuthError, OAuthMethod, StaticProvider, @@ -140,6 +141,13 @@ def connect_screen_for(integration: AddableIntegration, path: AuthPath) -> Manag return TokenModal(integration.integration_id, integration.display_name, path) if isinstance(path.method.provider, StaticProvider): return ClientIdModal(integration.integration_id, integration.display_name, path) + if isinstance(path.method.provider, BundledProvider): + return ConnectModal( + integration.integration_id, + integration.display_name, + path, + client_id=path.method.provider.client_id, + ) return ConnectModal(integration.integration_id, integration.display_name, path) @@ -376,9 +384,12 @@ def revoke_token() -> None: # nothing could revoke it later. revoke_best_effort(self.method, client_id, credentials) - tab_config = TabConfig( - integration=self.integration_id, client_id=client_id, connection=self.path.id - ) + if isinstance(self.method.provider, BundledProvider): + tab_config = TabConfig(integration=self.integration_id, connection=self.path.id) + else: + tab_config = TabConfig( + integration=self.integration_id, client_id=client_id, connection=self.path.id + ) warning = extra_scopes_warning( self.integration_id, self.display_name, self.method, credentials ) diff --git a/tests/auth/test_login.py b/tests/auth/test_login.py index bd90bf0..d1fa5c0 100644 --- a/tests/auth/test_login.py +++ b/tests/auth/test_login.py @@ -242,3 +242,41 @@ def test_a_static_login_without_a_client_id_raises(): port=0, timeout=10, ) + + +BUNDLED = oauth.OAuthMethod( + provider=oauth.BundledProvider( + metadata=oauth.ServerMetadata( + authorization_endpoint=METADATA["authorization_endpoint"], + token_endpoint=METADATA["token_endpoint"], + ), + client_id="client-bundled", + client_secret="secret-bundled", + ), + scopes=("read",), +) + + +def test_a_bundled_login_uses_the_bundled_id_posts_the_secret_and_never_registers(monkeypatch): + browser_sending(monkeypatch, "/callback?code=code-1&state={state}") + bodies = [] + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path == "/token": + bodies.append(urllib.parse.parse_qs(request.content.decode())) + return httpx.Response(200, json=TOKEN) + raise AssertionError(f"unexpected request to {request.url}") + + client_id, credentials = perform_login( + httpx.Client(transport=httpx.MockTransport(handler)), + BUNDLED, + None, + on_authorize_url=lambda url: None, + port=0, + timeout=10, + ) + + assert client_id == "client-bundled" + assert credentials.access_token == "at-1" + assert bodies[0]["client_id"] == ["client-bundled"] + assert bodies[0]["client_secret"] == ["secret-bundled"] diff --git a/tests/auth/test_oauth.py b/tests/auth/test_oauth.py index b97f912..4164472 100644 --- a/tests/auth/test_oauth.py +++ b/tests/auth/test_oauth.py @@ -9,6 +9,7 @@ from smorg.auth.oauth import ( REGISTRATION_PORT, + BundledProvider, DiscoveredProvider, OAuthError, OAuthMethod, @@ -16,6 +17,8 @@ StaticProvider, build_authorize_url, callback_port, + client_id_for, + client_secret_of, discover, exchange_code, extra_scopes_warning, @@ -348,3 +351,77 @@ def test_register_client_refuses_metadata_without_a_registration_endpoint(): def test_the_callback_port_is_pinned_only_for_a_static_provider(): assert callback_port(STATIC) == REGISTRATION_PORT assert callback_port(METHOD) == 0 + + +BUNDLED = OAuthMethod( + provider=BundledProvider( + metadata=ServerMetadata( + authorization_endpoint="https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint="https://oauth2.googleapis.com/token", + revocation_endpoint="https://oauth2.googleapis.com/revoke", + ), + client_id="bundled-id", + client_secret="bundled-secret", + ), + scopes=("https://www.googleapis.com/auth/calendar.readonly",), +) + + +def test_a_bundled_exchange_and_refresh_post_the_secret_and_a_static_one_does_not(metadata): + bodies = [] + + def handler(request): + bodies.append(parse_qs(request.content.decode())) + return httpx.Response(200, json={"access_token": "at-1", "expires_in": 3600}) + + bundled_metadata = resolve_metadata(client_returning(handler), BUNDLED) + exchange_code( + client_returning(handler), + bundled_metadata, + "bundled-id", + "code-1", + "v", + REDIRECT, + client_secret=client_secret_of(BUNDLED), + ) + old = Credentials(access_token="at-0", refresh_token="rt-0", expires_at=None, scope="") + refresh_credentials( + client_returning(handler), + bundled_metadata, + "bundled-id", + old, + client_secret=client_secret_of(BUNDLED), + ) + exchange_code(client_returning(handler), metadata, "client-abc", "code-1", "v", REDIRECT) + + assert bodies[0]["client_secret"] == ["bundled-secret"] + assert bodies[1]["client_secret"] == ["bundled-secret"] + assert "client_secret" not in bodies[2] + + +def test_client_id_for_prefers_the_bundled_id_over_a_recorded_one(): + assert client_id_for(BUNDLED, "recorded") == "bundled-id" + assert client_id_for(METHOD, "recorded") == "recorded" + assert client_id_for(METHOD, None) is None + + +def test_a_bundled_provider_uses_an_ephemeral_callback_port(): + assert callback_port(BUNDLED) == 0 + + +def test_a_bundled_error_never_contains_the_secret(): + def handler(request): + return httpx.Response(400, json={"error": "invalid_request"}) + + bundled_metadata = resolve_metadata(client_returning(handler), BUNDLED) + with pytest.raises(OAuthError) as excinfo: + exchange_code( + client_returning(handler), + bundled_metadata, + "bundled-id", + "code-1", + "v", + REDIRECT, + client_secret="bundled-secret", + ) + assert "bundled-secret" not in str(excinfo.value) diff --git a/tests/auth/test_refresh.py b/tests/auth/test_refresh.py index bb728c0..91d71f3 100644 --- a/tests/auth/test_refresh.py +++ b/tests/auth/test_refresh.py @@ -1,9 +1,11 @@ import threading from datetime import UTC, datetime, timedelta +from urllib.parse import parse_qs import httpx import pytest +from smorg.auth import oauth from smorg.auth.oauth import DiscoveredProvider, OAuthMethod from smorg.auth.refresh import EXPIRY_MARGIN, credentials_for, fresh_credentials from smorg.auth.store import Credentials, get_credentials, set_credentials @@ -209,3 +211,40 @@ def test_an_oauth_path_still_refreshes_through_the_same_resolver(): assert resolved is not None assert resolved.access_token == "access-new" assert hits != [] + + +BUNDLED = oauth.OAuthMethod( + provider=oauth.BundledProvider( + metadata=oauth.ServerMetadata( + authorization_endpoint="https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint="https://oauth2.googleapis.com/token", + ), + client_id="client-bundled", + client_secret="secret-bundled", + ), + scopes=("read",), +) + + +def test_a_bundled_refresh_uses_the_bundled_id_and_secret_even_with_no_recorded_id(): + expiring = Credentials( + access_token="at-old", + refresh_token="rt-1", + expires_at=datetime.now(UTC) + timedelta(seconds=10), + scope="read", + ) + set_credentials("gcal", expiring) + bodies = [] + + def handler(request): + bodies.append(parse_qs(request.content.decode())) + return httpx.Response(200, json={"access_token": "at-new", "expires_in": 3600}) + + refreshed = fresh_credentials( + "gcal", BUNDLED, None, httpx.Client(transport=httpx.MockTransport(handler)) + ) + + assert refreshed is not None + assert refreshed.access_token == "at-new" + assert bodies[0]["client_id"] == ["client-bundled"] + assert bodies[0]["client_secret"] == ["secret-bundled"] diff --git a/tests/core/test_removal.py b/tests/core/test_removal.py index 6dde6bc..2351aaf 100644 --- a/tests/core/test_removal.py +++ b/tests/core/test_removal.py @@ -1,4 +1,5 @@ -from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta +from types import SimpleNamespace import pytest @@ -82,6 +83,48 @@ def unreachable(client, provider): assert load_config().tabs == () +def test_a_bundled_tab_revokes_without_a_recorded_client_id(monkeypatch): + bundled = oauth.OAuthMethod( + provider=oauth.BundledProvider( + metadata=oauth.ServerMetadata( + authorization_endpoint="https://accounts.bundled.invalid/authorize", + token_endpoint="https://accounts.bundled.invalid/token", + revocation_endpoint="https://accounts.bundled.invalid/revoke", + ), + client_id="client-bundled", + client_secret="secret-bundled", + ), + scopes=("read",), + ) + from smorg.core.contract import AuthPath, Manifest + + manifest = Manifest( + id="bundled", + display_name="Bundled", + connections=(AuthPath(id="oauth", method=bundled),), + stale_after=timedelta(minutes=5), + actions=(), + ) + monkeypatch.setattr( + "smorg.core.removal.get_integration", + lambda integration_id: SimpleNamespace(manifest=manifest), + ) + revoked_with = [] + + def fake_revoke(method, client_id, credentials): + revoked_with.append(client_id) + return True + + monkeypatch.setattr("smorg.core.removal.revoke_best_effort", fake_revoke) + save_config(Config(tabs=(TabConfig(integration="bundled", connection="oauth"),))) + set_credentials("bundled", LIVE) + + result = remove_integration("bundled") + + assert result.revoked is True + assert revoked_with == ["client-bundled"] + + def test_stale_connection_id_skips_revocation_but_removal_completes(monkeypatch): save_config( Config(tabs=(TabConfig(integration="linear", client_id="client-abc", connection="nope"),)) diff --git a/tests/shell/test_menu.py b/tests/shell/test_menu.py index 2517a07..3b2ed33 100644 --- a/tests/shell/test_menu.py +++ b/tests/shell/test_menu.py @@ -12,6 +12,7 @@ from smorg.auth.login import LoginCancelled from smorg.auth.oauth import ( REGISTERED_REDIRECT_URI, + BundledProvider, DiscoveredProvider, OAuthMethod, ServerMetadata, @@ -893,6 +894,27 @@ def test_an_oauth_path_still_leads_to_the_browser_modal(): assert isinstance(connect_screen_for(widget, OAUTH_PATH), ConnectModal) +def test_a_bundled_path_skips_the_client_id_modal(): + bundled = OAuthMethod( + provider=BundledProvider( + metadata=ServerMetadata( + authorization_endpoint="https://accounts.bundled.invalid/authorize", + token_endpoint="https://accounts.bundled.invalid/token", + ), + client_id="client-bundled", + client_secret="secret-bundled", + ), + scopes=("read",), + ) + path = AuthPath(id="oauth", method=bundled) + integration = AddableIntegration("bundled", "Bundled", (path,)) + + screen = connect_screen_for(integration, path) + + assert isinstance(screen, ConnectModal) + assert screen.client_id == "client-bundled" + + @pytest.mark.asyncio async def test_the_token_modal_says_where_to_get_one_and_what_it_needs(registered): registered(fake_manifest("widget", connections=(TOKEN_PATH,))) diff --git a/tests/test_cli.py b/tests/test_cli.py index 8fa9b1f..855550d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4,7 +4,7 @@ import pytest -from smorg.auth.oauth import OAuthMethod, ServerMetadata, StaticProvider +from smorg.auth.oauth import BundledProvider, OAuthMethod, ServerMetadata, StaticProvider from smorg.auth.store import ( Credentials, CredentialStoreError, @@ -452,3 +452,47 @@ def fake_run_login(client, provider, client_id, **kwargs): assert "a client id is required" in capsys.readouterr().err assert logins == [] assert load_config().tabs == () + + +BUNDLED_MANIFEST = Manifest( + id="bundled", + display_name="Bundled", + connections=( + AuthPath( + id="oauth", + method=OAuthMethod( + provider=BundledProvider( + metadata=ServerMetadata( + authorization_endpoint="https://accounts.bundled.invalid/authorize", + token_endpoint="https://accounts.bundled.invalid/token", + ), + client_id="client-bundled", + client_secret="secret-bundled", + ), + scopes=("read",), + ), + ), + ), + stale_after=timedelta(minutes=5), + actions=(), +) + + +def test_connect_never_prompts_and_records_no_client_id_on_a_bundled_path(monkeypatch): + monkeypatch.setattr( + "smorg.cli.get_integration", + lambda integration_id: SimpleNamespace(manifest=BUNDLED_MANIFEST), + ) + + def fake_run_login(client, provider, client_id, **kwargs): + return ("client-bundled", LIVE) + + monkeypatch.setattr("smorg.cli.run_login", fake_run_login) + + def refuse_input(prompt=""): + raise AssertionError("a bundled connect must not prompt") + + monkeypatch.setattr("builtins.input", refuse_input) + + assert main(["connect", "bundled"]) == 0 + assert load_config().tabs == (TabConfig(integration="bundled", connection="oauth"),)