[jonpspri]

✨ Feature / Enhancement PR

🔗 Epic / Issue

Link to the epic or parent issue:
Closes #6966

---

🚀 Summary (1-2 sentences)

Adds GET /oauth/token/{gateway_id}, which returns the caller's own current OAuth access_token for a gateway (auto-refreshed if within a configurable threshold of expiring) without ever returning the refresh_token.

---

📏 Reviewability

• This PR has one clear purpose
• The linked issue is not labeled triage
• Unrelated bugs or improvements are tracked in separate issues/PRs
• Tests are included with the code they validate
• If AI-assisted, I understand and can explain the generated changes

---

🧪 Checks

List exact commands, screenshots, videos, logs, validation steps, or explain why evidence is not feasible.

• uv run ruff check passes on changed files
• uv run bandit passes on mcpgateway/routers/oauth_router.py
• uv run pytest tests/unit/mcpgateway/routers/test_oauth_router.py passes (222 tests)
• make lint / make test (full suite) not run in this environment — see individual commands above
• CHANGELOG updated (if user-facing) — not yet done, flagging for reviewer

---

📓 Notes (optional)

Design sketch or extra context.

• Disabled by default via OAUTH_TOKEN_RETRIEVAL_ENABLED (env, default false).
• Authorization requires gateways.token_read and the existing _enforce_gateway_access visibility check. The endpoint returns only the caller's stored token.
• Operators configure OAUTH_TOKEN_REFRESH_THRESHOLD_SECONDS from 0 to 3600 seconds (default 600). Callers cannot set the threshold.
• Response is {gateway_id, access_token, token_type, expires_at, scopes} — refresh_token is never included.
• Success responses set Cache-Control: no-store; issuance is logged (gateway id + user email) but the token value itself is never logged.

---

This PR replaces #6954, which was opened from a fork. The branch has been pushed to the IBM org repository to allow CI checks to run.

[madhu-mohan-jaishankar]

TokenScopingMiddleware only lets a permission-scoped API token through if the path has an entry in its (method, pattern, permission) table. /oauth/status has one. /oauth/token does not, so a token scoped to gateways.read gets a 403 before the handler runs.

[jonpspri]

Both review findings are addressed in cc29983.

1. Missing @require_permission (oauth_router.py). The handler now carries @require_permission(Permissions.GATEWAYS_READ), the same Layer 2 gate as GET /oauth/status/{gateway_id}. A deny-path test confirms a caller without gateways.read gets 403 before any gateway lookup.
2. Missing TokenScopingMiddleware entry (token_scoping.py). Added ("GET", ^/oauth/token(?:$|/), Permissions.GATEWAYS_READ) next to the /oauth/status entry. A token scoped to gateways.read now reaches the handler. Tokens with other permissions, and non-GET methods, stay default-denied. test_oauth_token_path_requires_gateways_read covers allow and deny cases.

Test changes: the handler tests now pass a dict user and keyword arguments, because @require_permission requires both. The unit tests in test_oauth_router.py and test_token_scoping.py pass. Ruff passes on the source files.

Not done: the CHANGELOG entry and the full pre-merge gate (make test, diff-cover, test-e2e).

[msureshkumar88]

The route is backend-only, off by default, and needs no migration. The Closes #6966 link is present. The token-scoping fix in cfd238736 resolves the earlier finding. Caller identity comes from the authenticated user, the query is keyed by that identity, and refresh_token is never returned. Those parts are sound.

Three items block merge. I list them first, then the remaining findings.

Blocking

blocking | mcpgateway/routers/oauth_router.py:1701 + mcpgateway/middleware/token_scoping.py:165 | The route returns a live upstream bearer token but requires only gateways.read. The viewer and platform_viewer roles hold gateways.read (bootstrap_db.py:476-515), and so does any API token scoped to it. A read-only role can lift the raw provider token and use it outside ContextForge, past RBAC, plugins and audit (CWE-200, CWE-522, CWE-863). | Add a dedicated permission (for example gateways.token_read). Map the route to it. Grant it to developer and team_admin only. Add deny tests for a viewer and for a gateways.read-only API token.
blocking | mcpgateway/routers/oauth_router.py:1769-1775 | The handler maps every None from get_user_token() to 404 "complete the OAuth flow". The DB backend returns None for no row, expired without refresh token, failed refresh, and any swallowed exception (db_backend.py:196-244). A DB or IdP outage tells the user to re-consent. A token still valid but inside threshold_seconds also returns None when the refresh fails. | Return a result with a state. 404 only for no row. 401/409 with a stable code for reauthorization needed. 502/503 for backend or IdP errors. Return the current token when refresh fails and the token has not expired.
blocking | tests/live_gateway/ | AGENTS.md requires a black-box test against a running gateway. All 12 new tests call the handler with mocks. None exercises the real RBAC decorator, TokenScopingMiddleware, SecurityHeadersMiddleware, or a stored token row. | Add a live test: flag off, no token, seeded token returns 200 without refresh_token, other user's token never returned, viewer and scoped token denied.

Functionally impacting

functionally-impacting | oauth_router.py:1752-1754 | No grant-type guard. client_credentials, password and token-exchange gateways hold no per-user token row. The caller gets 404 with a hint to run /oauth/authorize, which cannot work. | Return 400 unless grant_type is authorization_code. Add a test per grant type.
functionally-impacting | oauth_router.py:1708-1713 | threshold_seconds up to 3600 forces a refresh on every call when the provider token lifetime is 3600 seconds or less. There is no rate limit and no refresh lock. Concurrent refreshes can invalidate a rotating refresh token (CWE-770, CWE-362). | Lower the maximum. Add a per-user rate limit. Serialize refresh per gateway and user.
functionally-impacting | oauth_router.py:1776 | get_token_info() is a second backend call (a second Vault GET with retry on the Vault backend). It is unguarded. A raise gives a bare 500 after the token was refreshed. "or {}" hides a failure and returns expires_at null. | Return token and metadata from one storage call. Log and map failures.
functionally-impacting | oauth_router.py:1778-1779 | Token issuance appears only in an INFO log. | Call AuditTrailService.log_action() with its own session, a correlation_id, the user and the gateway. Never log the token.
functionally-impacting | CHANGELOG.md, docs/docs/manage/oauth.md, charts/mcp-stack/values.yaml | No CHANGELOG entry, no endpoint docs, no Helm value for OAUTH_TOKEN_RETRIEVAL_ENABLED. | Add all three. State the permission, the error contract and the exfiltration risk.

Suggestions and minor

suggestion | oauth_router.py:1778 | SecurityHeadersMiddleware already sets "no-store, private", Pragma and Expires on protected paths (security_headers.py:503-510). The handler header and the Response parameter are redundant. | Remove both. Assert the header in the live test.
suggestion | oauth_router.py:1762-1764 | Dead branch. _enforce_gateway_access() already raises 401 for an unknown email (oauth_router.py:589-591). | Delete the check and its test.
suggestion | oauth_router.py:1747-1750 | The 503 detail names the env var and confirms the route exists (CWE-209). | Return 404, or register the route only when the flag is true.
suggestion | oauth_router.py:1701 | The return type is a bare dict. | Add a Pydantic response model so OpenAPI documents it and a schema test enforces "no refresh_token".
suggestion | tests/unit/mcpgateway/routers/test_oauth_router.py | test_threshold_seconds_query_param_is_bounded reads route.dependant internals. | Call the route with TestClient at threshold_seconds=3601 and expect 422.
minor | oauth_router.py:1779 | The f-string log builds the message at every level. | Use lazy %s arguments.
minor | commits | The bandit-suppression commit stands alone. No commit references #6966. | Squash it into the first commit.

Process

Issue #6966 still carries the triage label. The maintainer asked whether OBO token exchange (RFC 8693) already covers this need. The thread has an answer but no decision. Get explicit acceptance on the issue before merge.

The full review, with CWE mapping and test-gap list, is in my local notes. I ran the 12 new tests locally and they pass. I did not run the full suite.

[cafalchio]

The route is backend-only, off by default, and needs no migration. The Closes #6966 link is present. The token-scoping fix in cfd238736 resolves the earlier finding. Caller identity comes from the authenticated user, the query is keyed by that identity, and refresh_token is never returned. Those parts are sound.

Three items block merge. I list them first, then the remaining findings.

Blocking

blocking | mcpgateway/routers/oauth_router.py:1701 + mcpgateway/middleware/token_scoping.py:165 | The route returns a live upstream bearer token but requires only gateways.read. The viewer and platform_viewer roles hold gateways.read (bootstrap_db.py:476-515), and so does any API token scoped to it. A read-only role can lift the raw provider token and use it outside ContextForge, past RBAC, plugins and audit (CWE-200, CWE-522, CWE-863). | Add a dedicated permission (for example gateways.token_read). Map the route to it. Grant it to developer and team_admin only. Add deny tests for a viewer and for a gateways.read-only API token.
blocking | mcpgateway/routers/oauth_router.py:1769-1775 | The handler maps every None from get_user_token() to 404 "complete the OAuth flow". The DB backend returns None for no row, expired without refresh token, failed refresh, and any swallowed exception (db_backend.py:196-244). A DB or IdP outage tells the user to re-consent. A token still valid but inside threshold_seconds also returns None when the refresh fails. | Return a result with a state. 404 only for no row. 401/409 with a stable code for reauthorization needed. 502/503 for backend or IdP errors. Return the current token when refresh fails and the token has not expired.
blocking | tests/live_gateway/ | AGENTS.md requires a black-box test against a running gateway. All 12 new tests call the handler with mocks. None exercises the real RBAC decorator, TokenScopingMiddleware, SecurityHeadersMiddleware, or a stored token row. | Add a live test: flag off, no token, seeded token returns 200 without refresh_token, other user's token never returned, viewer and scoped token denied.

Functionally impacting

functionally-impacting | oauth_router.py:1752-1754 | No grant-type guard. client_credentials, password and token-exchange gateways hold no per-user token row. The caller gets 404 with a hint to run /oauth/authorize, which cannot work. | Return 400 unless grant_type is authorization_code. Add a test per grant type.
functionally-impacting | oauth_router.py:1708-1713 | threshold_seconds up to 3600 forces a refresh on every call when the provider token lifetime is 3600 seconds or less. There is no rate limit and no refresh lock. Concurrent refreshes can invalidate a rotating refresh token (CWE-770, CWE-362). | Lower the maximum. Add a per-user rate limit. Serialize refresh per gateway and user.
functionally-impacting | oauth_router.py:1776 | get_token_info() is a second backend call (a second Vault GET with retry on the Vault backend). It is unguarded. A raise gives a bare 500 after the token was refreshed. "or {}" hides a failure and returns expires_at null. | Return token and metadata from one storage call. Log and map failures.
functionally-impacting | oauth_router.py:1778-1779 | Token issuance appears only in an INFO log. | Call AuditTrailService.log_action() with its own session, a correlation_id, the user and the gateway. Never log the token.
functionally-impacting | CHANGELOG.md, docs/docs/manage/oauth.md, charts/mcp-stack/values.yaml | No CHANGELOG entry, no endpoint docs, no Helm value for OAUTH_TOKEN_RETRIEVAL_ENABLED. | Add all three. State the permission, the error contract and the exfiltration risk.

Suggestions and minor

suggestion | oauth_router.py:1778 | SecurityHeadersMiddleware already sets "no-store, private", Pragma and Expires on protected paths (security_headers.py:503-510). The handler header and the Response parameter are redundant. | Remove both. Assert the header in the live test.
suggestion | oauth_router.py:1762-1764 | Dead branch. _enforce_gateway_access() already raises 401 for an unknown email (oauth_router.py:589-591). | Delete the check and its test.
suggestion | oauth_router.py:1747-1750 | The 503 detail names the env var and confirms the route exists (CWE-209). | Return 404, or register the route only when the flag is true.
suggestion | oauth_router.py:1701 | The return type is a bare dict. | Add a Pydantic response model so OpenAPI documents it and a schema test enforces "no refresh_token".
suggestion | tests/unit/mcpgateway/routers/test_oauth_router.py | test_threshold_seconds_query_param_is_bounded reads route.dependant internals. | Call the route with TestClient at threshold_seconds=3601 and expect 422.
minor | oauth_router.py:1779 | The f-string log builds the message at every level. | Use lazy %s arguments.
minor | commits | The bandit-suppression commit stands alone. No commit references #6966. | Squash it into the first commit.

Process

Issue #6966 still carries the triage label. The maintainer asked whether OBO token exchange (RFC 8693) already covers this need. The thread has an answer but no decision. Get explicit acceptance on the issue before merge.

The full review, with CWE mapping and test-gap list, is in my local notes. I ran the 12 new tests locally and they pass. I did not run the full suite.

Please do inline comment

[msureshkumar88]

The route is backend-only, off by default, and needs no migration. The Closes #6966 link is present. The token-scoping fix in cfd238736 resolves the earlier finding. Caller identity comes from the authenticated user, the query is keyed by that identity, and refresh_token is never returned. Those parts are sound.

Three items block merge. Each has an inline comment, except the live test, which has no file in the diff:

• oauth_router.py:1702: the route needs a dedicated permission, not gateways.read.
• oauth_router.py:1769: the handler maps every None from get_user_token() to 404.
• Missing live test: AGENTS.md requires a black-box test against a running gateway. All 12 new tests call the handler with mocks. None exercises the real RBAC decorator, TokenScopingMiddleware, SecurityHeadersMiddleware, or a stored token row. Add a test under tests/live_gateway/: flag off, no token, seeded token returns 200 without refresh_token, another user's token never returned, viewer and scoped token denied.

These items have no line in the diff:

• functionally-impacting | CHANGELOG.md, docs/docs/manage/oauth.md, charts/mcp-stack/values.yaml | No CHANGELOG entry, no endpoint docs, no Helm value for OAUTH_TOKEN_RETRIEVAL_ENABLED. Add all three. State the permission, the error contract and the exfiltration risk.
• minor | commits | The bandit-suppression commit stands alone. No commit references #6966. Squash it into the first commit.
• process | Issue #6966 still carries the triage label. The maintainer asked whether OBO token exchange (RFC 8693) already covers this need. The thread has an answer but no decision. Get explicit acceptance on the issue before merge.

I ran the 12 new tests locally and they pass. I did not run the full suite.

[msureshkumar88]

Thanks for the rework. Several earlier findings are now resolved:

• The handler maps failures to explicit states: 404 for no row, 409 for reauthorization, 502 for provider errors, 503 for storage errors. A still-valid token is returned when refresh fails. Both backends follow this.
• Token and metadata come from one get_user_token_details() call.
• The grant-type guard rejects non-authorization_code gateways.
• The caller-controlled threshold_seconds query parameter is gone. An operator setting replaces it.
• A live gateway test, a CHANGELOG entry and endpoint docs are present. Issue #6966 no longer carries triage.

Two blocking items remain. Each has an inline comment.

• oauth_router.py:1700: tokens.create does not restrict the route. The viewer and platform_viewer roles hold it.
• test_oauth_manager.py:1678: the PR makes 5 existing unit tests fail. They pass on main.

Verification on 98a2383e3:

• pytest tests/live_gateway/test_oauth_token_retrieval.py: 9 passed, 1 failed (test_viewer_and_wrong_team_are_denied[viewer] returns 200 with access-for-viewer).
• pytest on the changed unit test files: 5 failed in TestIssuerPinningOnAudienceLearning, all others passed.

Findings without a diff line:

functionally-impacting | charts/mcp-stack/values.yaml | No Helm value for OAUTH_TOKEN_RETRIEVAL_ENABLED or OAUTH_TOKEN_REFRESH_THRESHOLD_SECONDS. | Add both with the config.py defaults.
minor | PR description | The Notes section still describes the removed threshold_seconds query parameter and the gateways.read check. | Update it to the operator setting and the final permission.
minor | commits | The bandit-suppression commit stands alone. No commit references #6966. | Squash before merge.

Suggestions from the previous review that are still open: oauth_router.py:1745 (503 detail names the env var), the bare dict return type (add a response model), and the lazy %s log arguments at oauth_router.py:1784.

[msureshkumar88]

Thanks for the rework. Every item from my previous review is resolved on 47be13fd5:

• The route now requires the new gateways.token_read permission, here and in token_scoping.py. Only developer and team_admin receive it, so viewers can no longer retrieve the token.
• The test_oauth_manager.py hunk is reverted, and TestIssuerPinningOnAudienceLearning passes again.
• Each token issuance writes an audit entry through log_action(). The token value never reaches the log.
• The database backend serializes refresh per gateway and user, and re-reads the row before refreshing.
• report.md is removed. The Helm values, the PR Notes, the typo and the lazy log arguments are fixed.

Verification: the unit tests for the changed areas pass. A broader RBAC, OAuth and bootstrap run gives 3744 passed and 12 skipped.

One item remains before merge:

blocking | .secrets.baseline | "Run pre-commit hooks" fails because detect-secrets rewrites the baseline. The baseline carries chart entries that main does not have, and the exclude regex repeats ^.secrets.baseline$. | Restore the file from main, run make detect-secrets-scan, and commit the result.

Optional follow-ups: the 503 detail names the env var, the email check after _enforce_gateway_access() is unreachable, the handler Cache-Control header duplicates the middleware, and the route returns a bare dict. Squash on merge to tidy the commit history.

[madhu-mohan-jaishankar]

LGTM