diff --git a/changelog.d/tsk-4bw2ew-signin-broker-cap.md b/changelog.d/tsk-4bw2ew-signin-broker-cap.md new file mode 100644 index 000000000..fd407ebd2 --- /dev/null +++ b/changelog.d/tsk-4bw2ew-signin-broker-cap.md @@ -0,0 +1,2 @@ +### Fixed +- Sign-in broker now allows only one open request per agent per kind. A second `POST /api/auth-broker/requests` for the same agent and kind while the first is pending returns `409` with detail "a sign-in request is already open for this agent" and raises no notification. diff --git a/docs/agent-coordination.md b/docs/agent-coordination.md index 68c1374c1..0c1738d48 100644 --- a/docs/agent-coordination.md +++ b/docs/agent-coordination.md @@ -2355,14 +2355,16 @@ is not usable inside a container until that follow-up lands. Store: `{"kind": "claude-subscription", "url": "...", "prompt": "..."}`. The url must be `https` on an exact allowlisted host (`claude.ai`, `console.anthropic.com`; no subdomain, userinfo or non-443 port), else `422`: - a card the user taps must never open a site the agent chose. An agent may have - at most 3 open (non-completed, non-expired) requests at once; the 4th returns - `429` with detail ``too many open sign-in requests``. Returns - `{id, kind, status, host, expires_in_seconds}` and raises an `auth_broker` - notification to the agent's owner. An agent the registry cannot attribute (a - bound local-token agent with no registry row) gets its card sent to each - admin instead; the card is never a broadcast (`user_id` null). With no admin - to receive it, create is `409`. +a card the user taps must never open a site the agent chose. An agent may have + at most one pending or code-ready (non-expired) request per kind at once; a + second request for the same kind returns `409` with detail + ``a sign-in request is already open for this agent``. Returns + `{id, kind, status, host, expires_in_seconds}` and raises an `auth_broker` + notification to the agent's owner. An agent the registry cannot attribute (a + bound local-token agent with no registry row) gets its card sent to each + admin instead; the card is never a broadcast (`user_id` null). With no admin +to receive it, create is `409`. In addition, an agent may hold at most 3 pending or code-ready (non-expired) requests across all statuses including code_read; + the next one returns `429` with detail ``too many open sign-in requests``. 2. The desktop (bell, toast, Notifications app) renders that notification as a sign-in card: the agent's name, the host, an **Open** button (system browser; Firefox on taOSmobile) and a code field. The card re-checks the same host diff --git a/tests/test_auth_broker.py b/tests/test_auth_broker.py index 74b4322bd..a4c493b53 100644 --- a/tests/test_auth_broker.py +++ b/tests/test_auth_broker.py @@ -71,10 +71,15 @@ async def test_create_refuses_url_off_allowlist(client, app, url): @pytest.mark.asyncio async def test_create_accepts_allowlisted_hosts(client, app): - for url in (SIGNIN_URL, "https://console.anthropic.com/oauth/authorize?x=1"): - resp = await _create(client, app, url=url) - assert resp.status_code == 200, resp.text - assert resp.json()["id"] + # First request succeeds + resp1 = await _create(client, app, url=SIGNIN_URL) + assert resp1.status_code == 200, resp1.text + assert resp1.json()["id"] + + # Second request with different allowlisted host but same kind, different agent is accepted + resp2 = await _create(client, app, "other-agent", url="https://console.anthropic.com/oauth/authorize?x=1") + assert resp2.status_code == 200, resp2.text + assert resp2.json()["id"] @pytest.mark.asyncio @@ -376,31 +381,33 @@ async def test_secret_grant_preserves_existing_agents(client, app, monkeypatch, @pytest.mark.asyncio async def test_create_request_cap_per_agent(client, app): - """An agent can have at most MAX_OPEN_PER_AGENT open requests.""" + """An agent can have at most one open request per kind.""" from tinyagentos.auth_broker import MAX_OPEN_PER_AGENT hdr = _bearer(app, "haiku-agent") other_hdr = _bearer(app, "other-agent") - # Create MAX_OPEN_PER_AGENT requests for haiku-agent - for _ in range(MAX_OPEN_PER_AGENT): - resp = await _create(client, app, "haiku-agent") - assert resp.status_code == 200, resp.text + # First request succeeds + resp = await _create(client, app, "haiku-agent") + assert resp.status_code == 200, resp.text - # The (MAX_OPEN_PER_AGENT + 1)th request should be rejected with 429 + # Second request for same agent and kind is rejected with 409 resp = await _create(client, app, "haiku-agent") - assert resp.status_code == 429, resp.text - assert "too many open sign-in requests" in resp.text + assert resp.status_code == 409, resp.text + assert "a sign-in request is already open for this agent" in resp.text - # No 4th notification was added for haiku-agent + # Only one notification for haiku-agent rows = await app.state.notifications.list(limit=50) haiku_notifs = [r for r in rows if r.get("source") == "auth_broker" and r["data"]["agent"] == "haiku-agent"] - assert len(haiku_notifs) == MAX_OPEN_PER_AGENT + assert len(haiku_notifs) == 1 # A different agent can still create resp = await _create(client, app, "other-agent") assert resp.status_code == 200, resp.text + # The store-level MAX_OPEN_PER_AGENT cap still applies as a fallback + # (tested separately in test_store_create_enforces_cap_under_lock) + @pytest.mark.asyncio async def test_expired_code_ready_cleared_on_create(client, app, monkeypatch): @@ -436,6 +443,35 @@ async def test_expired_code_ready_cleared_on_create(client, app, monkeypatch): assert new_rec["status"] == "pending" +@pytest.mark.asyncio +async def test_store_create_refuses_second_open_same_kind(tmp_data_dir): + """AuthBrokerStore.create refuses a second open request for the same requester and kind. + + This test demonstrates the race condition that was fixed by making the one-open-request-per-kind + check atomic inside AuthBrokerStore.create(). + """ + from tinyagentos.auth_broker import KIND_CLAUDE_SUBSCRIPTION, AuthBrokerStore, OpenRequestExists + + store = AuthBrokerStore(tmp_data_dir / "auth_broker_test.db") + await store.init() + try: + url = "https://claude.ai/oauth/authorize?code=true&client_id=abc&state=xyz" + + # First create succeeds + rec1 = await store.create( + requester="agent-y", kind=KIND_CLAUDE_SUBSCRIPTION, url=url + ) + assert rec1["status"] == "pending" + + # Second create for same requester and kind should raise OpenRequestExists + with pytest.raises(OpenRequestExists): + await store.create( + requester="agent-y", kind=KIND_CLAUDE_SUBSCRIPTION, url=url + ) + finally: + await store.close() + + @pytest.mark.asyncio async def test_store_create_enforces_cap_under_lock(tmp_data_dir): """AuthBrokerStore.create enforces MAX_OPEN_PER_AGENT inside its lock. @@ -452,11 +488,14 @@ async def test_store_create_enforces_cap_under_lock(tmp_data_dir): url = "https://claude.ai/oauth/authorize?code=true&client_id=abc&state=xyz" # Create MAX_OPEN_PER_AGENT requests for "agent-x" - for _ in range(MAX_OPEN_PER_AGENT): + for i in range(MAX_OPEN_PER_AGENT): rec = await store.create( requester="agent-x", kind=KIND_CLAUDE_SUBSCRIPTION, url=url ) assert rec["status"] == "pending" + # Move this request to code_read so the next create can succeed + await store.set_code(rec["id"], "c") + await store.take_code(rec["id"]) # The (MAX_OPEN_PER_AGENT + 1)th create for "agent-x" must raise with pytest.raises(PendingCapExceeded) as exc_info: @@ -490,6 +529,9 @@ async def test_cap_reject_leaves_no_open_transaction(app): requester="cap-test-agent", kind=KIND_CLAUDE_SUBSCRIPTION, url=url ) assert rec["status"] == "pending" + # Move this request to code_read so the next create can succeed + await store.set_code(rec["id"], "c") + await store.take_code(rec["id"]) # The next create must raise PendingCapExceeded with pytest.raises(PendingCapExceeded): @@ -515,6 +557,9 @@ async def test_rollback_exception_does_not_mask_pending_capexceeded(app): requester="rollback-test-agent", kind=KIND_CLAUDE_SUBSCRIPTION, url=url ) assert rec["status"] == "pending" + # Move this request to code_read so the next create can succeed + await store.set_code(rec["id"], "c") + await store.take_code(rec["id"]) # Monkeypatch rollback to raise an exception async def failing_rollback(): @@ -530,3 +575,74 @@ async def failing_rollback(): assert exc_info.value.key == "rollback-test-agent" assert exc_info.value.cap == MAX_OPEN_PER_AGENT assert exc_info.value.pending == MAX_OPEN_PER_AGENT + + + +@pytest.mark.asyncio +async def test_create_second_request_same_agent_same_kind_returns_409(client, app): + """A second create by the same agent for the same kind while the first is pending + returns 409 and adds no second auth_broker notification.""" + hdr = _bearer(app, "haiku-agent") + + # First create succeeds + resp1 = await _create(client, app, "haiku-agent") + assert resp1.status_code == 200, resp1.text + rid1 = resp1.json()["id"] + + # Second create for same agent and kind should be 409 + resp2 = await _create(client, app, "haiku-agent") + assert resp2.status_code == 409, resp2.text + assert "a sign-in request is already open for this agent" in resp2.text + + # Only one notification should exist + rows = await app.state.notifications.list(limit=50) + hits = [r for r in rows if r.get("source") == "auth_broker" and r["data"]["agent"] == "haiku-agent"] + assert len(hits) == 1, f"Expected 1 notification, got {len(hits)}" + + +@pytest.mark.asyncio +async def test_create_after_expiry_succeeds(client, app, monkeypatch): + """After the first request expires, a new create for the same agent and kind succeeds.""" + import tinyagentos.auth_broker as broker + + t0 = 1_800_000_000.0 + monkeypatch.setattr(broker, "_now", lambda: t0) + hdr = _bearer(app, "haiku-agent") + + # Create first request + resp1 = await _create(client, app, "haiku-agent") + assert resp1.status_code == 200, resp1.text + rid1 = resp1.json()["id"] + + # Advance time past expiry + monkeypatch.setattr(broker, "_now", lambda: t0 + broker.EXPIRY_SECONDS + 1) + + # New create should succeed + resp2 = await _create(client, app, "haiku-agent") + assert resp2.status_code == 200, resp2.text + rid2 = resp2.json()["id"] + assert rid2 != rid1 + + # Two notifications should exist (one for each request) + rows = await app.state.notifications.list(limit=50) + hits = [r for r in rows if r.get("source") == "auth_broker" and r["data"]["agent"] == "haiku-agent"] + assert len(hits) == 2, f"Expected 2 notifications, got {len(hits)}" + + +@pytest.mark.asyncio +async def test_different_agent_not_blocked(client, app): + """A different agent can create a request even when another agent has one open.""" + # First agent creates a request + resp1 = await _create(client, app, "haiku-agent") + assert resp1.status_code == 200, resp1.text + + # Different agent creates a request - should succeed + resp2 = await _create(client, app, "other-agent") + assert resp2.status_code == 200, resp2.text + + # Both should have notifications + rows = await app.state.notifications.list(limit=50) + haiku_hits = [r for r in rows if r.get("source") == "auth_broker" and r["data"]["agent"] == "haiku-agent"] + other_hits = [r for r in rows if r.get("source") == "auth_broker" and r["data"]["agent"] == "other-agent"] + assert len(haiku_hits) == 1 + assert len(other_hits) == 1 diff --git a/tinyagentos/auth_broker.py b/tinyagentos/auth_broker.py index 214a5b4c5..3f3b01795 100644 --- a/tinyagentos/auth_broker.py +++ b/tinyagentos/auth_broker.py @@ -25,6 +25,8 @@ import aiosqlite from tinyagentos.base_store import BaseStore, PendingCapExceeded +class OpenRequestExists(Exception): + """Raised when a create operation would exceed the one-open-request-per-kind limit.""" logger = logging.getLogger(__name__) @@ -126,6 +128,28 @@ async def count_open(self, requester: str) -> int: count += 1 return count + async def open_request(self, requester: str, kind: str) -> dict | None: + """Return the newest open request for *requester* and *kind*. + + An open request has status 'pending' or 'code_ready' and is not expired. + Returns None if no such request exists. + """ + self._require_db() + now = _now() + async with self._db.execute( + "SELECT * FROM auth_broker_requests" + " WHERE requester = ? AND kind = ? AND status IN ('pending', 'code_ready')" + " ORDER BY created_at DESC LIMIT 1", + (requester, kind), + ) as cur: + row = await cur.fetchone() + if row is None: + return None + rec = _row(row) + if self.expired(rec, now): + return None + return rec + async def create(self, *, requester: str, kind: str, url: str, prompt: str = "") -> dict: self._require_db() if kind not in KINDS: @@ -149,6 +173,11 @@ async def create(self, *, requester: str, kind: str, url: str, prompt: str = "") " WHERE status = 'code_ready' AND created_at < ?", (now - EXPIRY_SECONDS,), ) + # Check for an existing open request for this requester and kind + # This must be inside the lock to prevent the race condition where + # two concurrent creates both pass this check and both insert + if await self.open_request(requester, kind) is not None: + raise OpenRequestExists(requester) open_count = 0 async with self._db.execute( "SELECT * FROM auth_broker_requests WHERE requester = ? AND status != 'completed'", diff --git a/tinyagentos/routes/auth_broker.py b/tinyagentos/routes/auth_broker.py index 84dbf99d9..d3409c627 100644 --- a/tinyagentos/routes/auth_broker.py +++ b/tinyagentos/routes/auth_broker.py @@ -22,7 +22,9 @@ from pydantic import BaseModel, Field from tinyagentos.agent_token_auth import check_agent_identity -from tinyagentos.auth_broker import EXPIRY_SECONDS, AuthBrokerStore, signin_host +from tinyagentos.auth_broker import ( + EXPIRY_SECONDS, AuthBrokerStore, OpenRequestExists, signin_host +) from tinyagentos.auth_context import ( current_user, require_agent_owner_or_admin, @@ -105,6 +107,8 @@ async def create_request(request: Request, body: CreateBody): status_code=422, detail="url must be https on an allowed sign-in host: claude.ai, console.anthropic.com", ) + # Check for an existing open request for this agent and kind. + store = _store(request) # The card goes to the agent's owner. An agent the registry cannot attribute # (a bound local-token agent with no registry row) goes to the admins, who # are the only users allowed to answer it. Never user_id=None: that is a @@ -117,7 +121,7 @@ async def create_request(request: Request, body: CreateBody): if not recipients: raise HTTPException(status_code=409, detail="no user to receive the sign-in card") try: - rec = await _store(request).create( + rec = await store.create( requester=requester, kind=body.kind, url=body.url, prompt=body.prompt.strip() ) except ValueError as exc: @@ -127,6 +131,11 @@ async def create_request(request: Request, body: CreateBody): status_code=429, detail="too many open sign-in requests", ) + except OpenRequestExists: + raise HTTPException( + status_code=409, + detail="a sign-in request is already open for this agent", + ) notifs = getattr(request.app.state, "notifications", None) if notifs is not None: