feat: return per-caller OAuth token status from /oauth/status - #6620
feat: return per-caller OAuth token status from /oauth/status#6620marekdano wants to merge 9 commits into
Conversation
e789c19 to
7aaca1d
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Nice work wiring TokenStorageService.get_token_info into the status endpoints — the scope match to #6459 is tight (per-caller status derived server-side, existing fields preserved, batch endpoint instead of N requests per card), and the parametrized tests for valid/near_expiry/expired/missing plus the "not shared across users" test are exactly the right things to lock down. No alembic migration needed here and none was added, which is correct since this only exposes an existing field.
A few things worth addressing before merge, all confined to the new code in oauth_router.py:
1. Batch endpoint N+1 queries (get_oauth_status_batch, ~line 1480-1487)
The loop calls get_oauth_status() once per id, so a 100-id batch does ~100 separate SELECT on Gateway plus ~100 separate token lookups — roughly 200-300 round trips for what the issue asks to be "batched per page load, no N+1 queries." Worth hoisting a single select(Gateway).where(Gateway.id.in_(deduped_ids)) and reusing one TokenStorageService instance across the loop rather than constructing/query-ing per id. (Note: this should stay a sequential loop, not asyncio.gather — the default database backend's get_token_info has no actual await inside it, and the shared synchronous Session isn't safe to use concurrently across coroutines.)
2. Batch endpoint swallows 5xx the same as 404/403 (~line 1483-1486)
get_oauth_status converts any unexpected exception into HTTPException(500, ...) (line 1437-1439), and the batch handler's except HTTPException: continue treats that identically to "not found" or "not accessible" — silently dropping the id with no log line. Could you split this so 403/404 are omitted as intended, but a 5xx gets logged (and maybe surfaced) rather than disappearing? Otherwise a backend fault during a batch call is indistinguishable from "gateway doesn't exist."
3. DB lookup failure reads as "not authorized" (db_backend.py:303-305 → _get_caller_token_status, line 1358-1359)
DatabaseTokenBackend.get_token_info catches its own exceptions and returns None on failure, and _get_caller_token_status maps None to {"status": "missing", "authorized": False}. Pre-existing behavior in db_backend.py, but this PR is what first exposes it through a user-facing field that's meant to drive UI state — during a DB hiccup, a user who has authorized would see "missing" and be told to re-authorize. Worth at least a distinct status (or a log line) so a transient failure isn't indistinguishable from "never authorized."
Smaller, non-blocking:
- The
if not gateway_ids: raise HTTPException(400, ...)branch (line 1473) looks unreachable via a real HTTP request — a fully-omittedgateway_idsquery param triggers FastAPI's own422before the handler runs, and in production withshould_expose_error_details()false that comes back as a generic message anyway. The unit test covers the function called directly, not the route, so this path isn't actually exercised end-to-end. - Consider one
TestClient-based test for the batch route (the file already has this pattern elsewhere) to catch exactly this kind of param-parsing mismatch. docs/docs/manage/oauth-troubleshooting.md:78still describes/oauth/status/{gateway_id}as returning only "OAuth configuration status" — could use a follow-up mention ofuser_token_statusand the new batch route.
Happy to take another pass once these land.
9805c71 to
07563ee
Compare
ba3c8fd to
b4ce256
Compare
vishu-bh
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments, PR looks good to merge once conflict is resolved
msureshkumar88
left a comment
There was a problem hiding this comment.
Nice work wiring TokenStorageService.get_token_info into the status endpoints — the per-caller isolation and the shared _build_oauth_status_payload helper keeping the two routes from drifting are both solid, and skipping an alembic migration is the right call here since OAuthToken already carries the per-user state.
Two things I think are worth another pass before merge:
1. The batch endpoint's N+1 fix doesn't hold for non-admin/narrowed-admin callers
get_oauth_status_batch does one bulk SELECT ... WHERE Gateway.id.in_(...), but the per-id _enforce_gateway_access call still goes through token_scoping_middleware._check_resource_team_ownership, which re-fetches each Gateway row from the DB by id for any caller that isn't a fully un-narrowed admin. I verified this with a quick test using realistic uuid4().hex gateway ids (the actual Gateway.id default) instead of "gateway123":
| ids | db.execute calls |
|---|---|
| 1 real hex id | 2 |
| 2 real hex ids | 3 |
So it's still effectively N+1 for the common case. The existing test_get_oauth_status_batch_single_gateway_query only passes because "gateway123" doesn't match _RESOURCE_PATTERNS' [a-f0-9\-]+ regex, so the ownership recheck never fires — a real hex id would fail that same assertion.
Issue #6459 calls out "Batched per page load, no N+1 queries" as an explicit requirement, so I think this needs a real fix rather than a follow-up: since the batch loop already has the loaded Gateway object, could _enforce_gateway_access (or the ownership check it delegates to) accept a preloaded gateway and skip the re-SELECT when one's supplied? Worth also swapping the test fixture id for a real hex string so this regresses loudly next time.
2. VaultTokenBackend.get_token_info doesn't get the same lookup-failure fix as the DB backend
This PR changes DatabaseTokenBackend.get_token_info to re-raise on failure instead of swallowing to None, specifically so a transient DB error is distinguishable from "never authorized" once it surfaces through _get_caller_token_status. VaultTokenBackend.get_token_info still catches VaultConnectionError/VaultAuthError and returns None, so on OAUTH_TOKEN_BACKEND=vault deployments a transient Vault outage would still report every caller as status: missing, authorized: false — the exact ambiguity this PR is fixing for the DB backend. Issue #6459 mentions get_token_info being implemented in both backends, so I think this should land in the same PR rather than as a gap.
Smaller/non-blocking notes, happy to see these follow up separately if preferred:
- Worth a batch test that exercises
_enforce_gateway_access's team/private-visibility branch (all current fixtures usevisibility="public"), and one that checks batch/single payload parity for the same gateway_id + caller. - No batch coverage for a non-
authorization_codegrant type (confirminguser_token_statusstays omitted). - Per AGENTS.md, PRs exercisable through a live gateway should include a black-box test under
tests/live_gateway/— currently there's none for either endpoint. - The PR body doesn't reference #6459 anywhere, so merging won't auto-close the tracking issue (branch name does, but GitHub doesn't use that for auto-close).
bc1e1f9 to
b85b3ad
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Nice work here — this cleanly wires up logic that was already built and tested (get_token_info) rather than reinventing it, keeps the change backend-only as scoped, and the batch endpoint's fail-closed/omit-on-error design for inaccessible gateways is the right call. The preloaded_gateway fix to _check_resource_team_ownership is also a nice catch — it quietly fixes a redundant SELECT on all four existing OAuth endpoints, not just the new batch one. Test coverage on the router/middleware side is thorough (per-user isolation, N+1 regression with real hex ids, private-visibility owner/non-owner cases, batch edge cases).
A few things worth addressing before merge, plus some lower-priority suggestions.
Suggested before merge
No live/black-box test for either status endpoint. All coverage for user_token_status and the new batch endpoint is unit-level against mocked Session/TokenStorageService objects — nothing in tests/live_gateway, tests/integration, or the load tests exercises them against a running gateway. The PR description's manual-test section (create gateway, seed a token via DatabaseTokenBackend, curl the endpoint, verify per-user isolation) is already exactly this scenario — would it make sense to turn that into an automated tests/live_gateway test rather than leaving it as copy-paste curl instructions?
Worth considering (non-blocking)
Batch endpoint's per-gateway loop is fully sequential, and the two backends have different costs here:
DatabaseTokenBackend.get_token_infoissues a synchronous SELECT per gateway id —asyncio.gatherwouldn't help (sync SQLAlchemy doesn't yield), but a single batched query (WHERE gateway_id IN (:ids) AND app_user_email = :email) would collapse N queries to 1.VaultTokenBackend.get_token_infoopens a newhttpx.AsyncClientper call (no connection reuse) and doesn't consult the existing_token_cache, so at the 100-id batch cap this is up to 100 uncached, sequential Vault round trips per catalog page load. Worth a look for anyone deploying with the Vault backend.
Two except Exception handlers in the batch loop log only str(exc), dropping the traceback (oauth_router.py, the access-check and payload-build handlers). Since these are the paths that would catch something unexpected rather than the anticipated Vault/DB errors, logger.exception(...) there would make a future incident easier to root-cause, with no change to the response.
Minor
Neither status endpoint declares a response_model (other routers in this codebase, e.g. tokens.py, do), so the user_token_status shape isn't visible in the generated OpenAPI schema. Given a few UI issues are blocked on this as a contract, might be worth a typed model in a follow-up — not something this PR needs to carry.
The get_token_info swallow-to-None → log-and-reraise change in base.py/db_backend.py/vault_backend.py is a broader behavior change than the issue asked for, though I traced it and it's safe: the only caller (_get_caller_token_status) catches it and maps back to the same "missing" response either way, and it doesn't break any of the existing tests for those backends. Just flagging it since it changes a shared method's contract for future callers, not just this one.
320815a to
9629a89
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Thanks for wiring this up — get_token_info sitting unused was exactly the kind of gap that's easy to miss, and the batch endpoint design (dedupe, cap, silent omission of inaccessible ids) matches what a catalog grid actually needs. The preloaded_gateway reuse in _check_resource_team_ownership and the accompanying mismatch-fallback test are a nice touch. Test coverage on the router side is thorough — the 4-way status parametrize, cross-user isolation test, and the db.execute.call_count == 1 regression guard all land well. Good to see a live black-box test included too.
A few things worth resolving before merge:
Blocking
-
Batch endpoint is still N+1 on the token lookup. The
Gatewayfetch is batched (gateways_by_id = {...}via oneIN (...)query), but_get_caller_token_statusis still called once per gateway id inside the loop, each issuing its ownSELECTagainstoauth_tokens(or a Vault round trip). The issue text calls this out explicitly: "Batched per page load, no N+1 queries." Would a bulk lookup work here — e.g. aget_token_info_bulk(gateway_ids, app_user_email)onAbstractTokenBackendwith a default loop implementation, overridden inDatabaseTokenBackendwith a singleWHERE gateway_id IN (...) AND app_user_email = :email? That would close the gap for the default backend without touching Vault's per-item nature. -
The
None→raisechange in the backends doesn't reach the caller.db_backend.get_token_infoandvault_backend.get_token_infonow raise instead of returningNoneon failure, with docstrings explaining this lets callers "distinguish never-authorized from lookup-failed." But the only caller (_get_caller_token_statusinoauth_router.py) catches the exception and returns{"status": "missing", "authorized": False}— collapsing straight back to the same value the change was meant to avoid. Two ways to close this: either surface a distinct"status": "unknown"in the router (so a Vault outage doesn't look like "never authorized" to the UI — which matters, since a UI acting on that would prompt a fresh OAuth flow with an IdP), or drop the backend/base contract change and keep just the router-level log line. Either is fine, just want the two ends to agree. -
No bound on a slow/unresponsive backend in the batch loop. With
OAUTH_TOKEN_BACKEND=vault, each of up to 100 ids in a batch call goes through_vault_request's 3-attempt retry with exponential backoff (up to ~33s per id on a hard failure), sequentially, inside a single request. Worth adding a timeout around the batch as a whole, or a circuit-breaker so one backend failure doesn't retry 100 times?
Suggestions
- Consider adding
@require_permission("gateways.read")to both status endpoints (matching the existing/vault/authorize/{id}pattern) and a_PERMISSION_PATTERNSentry for/oauth/status— right now a scoped API token (as opposed to a session token) would get a 403 fromTokenScopingMiddleware's default-deny, since the path isn't in the permission map. If the React catalog ends up using scoped tokens, the batch endpoint would be unreachable for it. - The
if not gateway_ids: raise HTTPException(400)branch looks unreachable over HTTP —gateway_idshas nodefault=, so FastAPI returns 422 before the handler runs when the param is omitted. Worth giving itdefault=[](making the 400 path real) or dropping the check and its test. _get_caller_token_status's except-block useslogger.error(..., str(e))(no traceback), while the batch loop useslogger.exceptionfor the same kind of failure — worth aligning onlogger.exceptionthere too.- Live test suite could use a deny-path case (a
privategateway owned by someone else → 403 / omitted from batch) alongside the existing per-user token isolation test, plus teardown for thesecond_userfixture (currently left in the DB after the run).
Minor
oauth-troubleshooting.mddocuments the new routes but not theuser_token_statusshape, the four status values, the 300s near-expiry threshold, or the 100-id cap / silent-omission behavior for inaccessible ids — might be worth a short addition for API consumers.- Could you add
Closes #6459to the PR description? The body currently references the issues this unblocks but not the one it resolves.
Happy to take another pass once these land — the shape of the change is right, just want the batch endpoint to actually deliver on the no-N+1 requirement from the issue.
9629a89 to
8437ce2
Compare
Blocking findings fixed
Suggestions addressed
Minor
TestsAdded/updated unit tests across |
msureshkumar88
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up on the batching, backend-error handling, documentation, and live coverage. One blocking authorization gap remains.
Blocking — preserve Layer-2 RBAC on the OAuth status routes
GET /oauth/status/{gateway_id} and GET /oauth/status authenticate the caller and run gateway visibility/ownership checks, but neither is decorated with @require_permission(Permissions.GATEWAYS_READ). The new _PERMISSION_PATTERNS entry is necessary for scoped API tokens, but it is only Layer 1: an empty token scope intentionally passes through to route-level RBAC, and these handlers do not perform that check.
This lets an authenticated principal without gateways.read receive OAuth configuration and its caller-scoped token state for a visible gateway, contrary to the project’s two-layer authorization invariant (CWE-863). The current live isolation test is a useful regression setup: on a clean stack its second user has teams=[] and no role, yet receives 200 for the public gateway.
Could we add @require_permission(Permissions.GATEWAYS_READ) to both status routes, retain the token-scope mapping, and add deny-path coverage for a caller with no gateways.read permission? The batch route’s expected response for a caller lacking that permission should be explicitly asserted as well.
Layer 1 token scoping and the per-gateway ownership check alone let an
authenticated caller without gateways.read view OAuth configuration and
their own token state for a visible gateway. Add
@require_permission(Permissions.GATEWAYS_READ) to GET /oauth/status and
GET /oauth/status/{gateway_id}, update existing unit tests to call the
now RBAC-decorated handlers with keyword arguments and a real dict for
current_user (required by the decorator), and add explicit deny-path
coverage for both routes.
Addresses review feedback on PR #6620 (CWE-863).
Signed-off-by: Marek Dano <mk.dano@gmail.com>
Signed-off-by: Marek Dano <mk.dano@gmail.com>
…ity) Batch /oauth/status now issues one gateway SELECT and reuses a single TokenStorageService across all ids instead of one per id. Batch access checks distinguish expected 403/404 omissions from 5xx/unexpected failures, which are now logged instead of silently dropped. Database token lookups re-raise on failure instead of collapsing to the same "missing" result as a token that was never issued, so a transient DB error is distinguishable in logs from "never authorized". Signed-off-by: Marek Dano <mk.dano@gmail.com>
Signed-off-by: Marek Dano <mk.dano@gmail.com>
…error visibility) Reuse the already-loaded gateway in the per-id ownership recheck so the batch endpoint no longer re-SELECTs each Gateway row for non-admin and narrowed-admin callers. Also make VaultTokenBackend.get_token_info re-raise on connection/auth failure instead of swallowing to None, matching DatabaseTokenBackend, so a Vault outage isn't indistinguishable from "never authorized" through the status endpoint. Signed-off-by: Marek Dano <mk.dano@gmail.com>
…tch loop Automates the PR's manual-test scenario (create oauth gateway, seed a token via DatabaseTokenBackend, curl the status endpoint, verify per-user isolation) as tests/live_gateway/mcp/test_oauth_status_live.py, satisfying AGENTS.md's requirement that behavior exercisable through a live gateway carry a black-box test. Also switches the two generic except-Exception handlers in the batch loop to logger.exception so an unexpected failure keeps its traceback in the logs. Signed-off-by: Marek Dano <mk.dano@gmail.com>
…ve test detect-secrets flagged the localhost:5433 compose default DB URL in test_oauth_status_live.py as a Basic Auth Credential. It's the well-known local-only "postgres:mysecretpassword" default already hardcoded in docker-compose.yml for the testing stack, not a real secret. Signed-off-by: Marek Dano <mk.dano@gmail.com>
… unknown status, batch timeout) Wires get_token_info_bulk() into AbstractTokenBackend/TokenStorageService, with a single-query override in DatabaseTokenBackend, so the batch endpoint issues one token lookup for the whole request instead of one per gateway id. Adds a distinct "unknown" user_token_status value (vs "missing") for a failed lookup, and bounds the batch's bulk lookup with a timeout so a slow/unresponsive backend can't hold the request open indefinitely - pending ids report "unknown" rather than the request hanging. Adds a gateways.read middleware permission-pattern entry for /oauth/status* so scoped API tokens aren't default-denied, gives gateway_ids a real default so the empty-batch 400 path is reachable over HTTP, aligns the single-endpoint failure log with the batch loop's logger.exception, and adds a live-test deny-path case plus second_user teardown. Signed-off-by: Marek Dano <mk.dano@gmail.com>
Layer 1 token scoping and the per-gateway ownership check alone let an
authenticated caller without gateways.read view OAuth configuration and
their own token state for a visible gateway. Add
@require_permission(Permissions.GATEWAYS_READ) to GET /oauth/status and
GET /oauth/status/{gateway_id}, update existing unit tests to call the
now RBAC-decorated handlers with keyword arguments and a real dict for
current_user (required by the decorator), and add explicit deny-path
coverage for both routes.
Addresses review feedback on PR #6620 (CWE-863).
Signed-off-by: Marek Dano <mk.dano@gmail.com>
9692af0 to
80e29af
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Thanks for adding the Layer-2 gateways.read checks to both OAuth status routes — that closes the authorization gap identified in the previous review, and the new unit deny-path assertions cover the decorator itself.
One blocking inconsistency remains in the live black-box coverage:
Blocking — update the live-test user for the new RBAC requirement
second_user is inserted directly as an EmailUser without any UserRole, and its JWT uses teams=[]. Under the real permission path this user has no gateways.read permission, so the new decorator returns 403 before the handlers reach the per-user isolation or private-gateway filtering logic.
That conflicts with these current expectations:
test_per_user_isolationexpects 200 andstatus: missing.test_private_gateway_denies_non_ownerexpects 403, but now exercises missing RBAC permission rather than private ownership.test_batch_endpoint_omits_private_gateway_for_non_ownerexpects 200 with omission, but receives 403 at the route-level gate.
Could the fixture grant this user an appropriate role containing gateways.read for the isolation and visibility scenarios, while retaining separate no-permission coverage that explicitly expects 403 for both routes? The PR body's manual per-user-isolation steps should be updated similarly, since they also create a role-less user and currently expect status: missing.
I verified the behavior with the real PermissionService against an isolated database: a directly inserted user has zero roles and check_permission(..., Permissions.GATEWAYS_READ, token_teams=[], check_any_team=True) returns false.
…st users) Grant the live black-box test's second_user a global platform_viewer role assignment (gateways.read) so the per-user isolation and private-gateway tests exercise ownership/visibility logic instead of being turned away by the gateways.read decorator added in the previous round. Add a separate role-less no_permission_user fixture with explicit 403 coverage on both the single and batch OAuth status routes, so the no-permission deny path stays tested independently. Signed-off-by: Marek Dano <mk.dano@gmail.com>
|
Verification: I started |
Summary
Wired
TokenStorageService.get_token_info(already implemented, previously unused) intomcpgateway/routers/oauth_router.py.GET /oauth/status/{gateway_id}For
authorization_codegateways, now includes auser_token_statusfield derived from the authenticated caller's own identity (never a client-supplied user param):client_id,scopes,authorization_url, etc.) are unchanged for backward compatibility.GET /oauth/status?gateway_ids=a&gateway_ids=b&...(new)Batch equivalent so a grid of cards issues one request instead of N.
Tests
All 216 tests in
test_oauth_router.pypass.make ruff interrogate pylintis clean (100% docstring coverage, 10.00/10 pylint).Scope
Backend-only, no UI. Diff is scoped to 2 files (~91 insertions in the router, ~100 lines of tests). Per the issue, this unblocks
#5592, #3876, #3850, and the catalog card states in #5967.
Manual Test
Prereqs:
make devrunning, venv activated, secrets patched (make init-secrets-patch-env).1. Get a bearer token for a real user
The server enforces strict "user must exist in DB".
admin@example.comis the bootstrapped platform admin. Don'tsource .envdirectly - it contains JSON/regex values that break bash parsing. Pull just the one variable you need:2. Create a test gateway directly in the DB
POST /gatewaysactively probes the URL and rolls back if it's not a real MCP server, so insert the row directly instead:Copy the printed id:
3. Check status before authorizing — expect
missing4. Seed a token to simulate a completed OAuth flow
Re-check status — expect
valid:Try the other states by overriding
EXPIRES_IN:5. Batch endpoint — a real id plus a nonexistent one
Only
$GW_IDshould appear in the response —nonexistent-idis silently omitted rather than failing the whole batch.6. Per-user isolation — a second user must see
missingother@example.comshould see"status": "missing"even thoughadmin@example.comhas a valid token on the same gateway.Cleanup