Skip to content

Fix: request user consent for authorization-code OpenAPI tools - #862

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/tool-auth-handler-authorization-code-consent
Open

Fix: request user consent for authorization-code OpenAPI tools#862
AmaadMartin wants to merge 1 commit into
mainfrom
fix/tool-auth-handler-authorization-code-consent

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:
    Problem: A RestApiTool built from an oauth2 authorizationCode scheme can never obtain user consent. ToolAuthHandler.prepareAuthCredentials() falls back to the configured {clientId, clientSecret}, which is truthy, so it never calls context.requestCredential() and never returns pending. It hands the token-less credential to the exchanger instead, which throws CredentialExchangeError: clientId, clientSecret, and either authCode or authResponseUri are required for authorization code exchange. on the very first call. The static-credential fallback from Feat: Port VertexAiExampleStore example provider from adk-python #536 is correct for apiKey, http, serviceAccount and clientCredentials, but wrong for the grants that mint a token against a user's consent.

Solution: A new module-level predicate requiresUserSignIn() gates that fallback. An OAuth2/OIDC credential with no accessToken, on a scheme whose grant type is authorizationCode, is ignored as a source of authorization, so the handler asks the client to sign in and returns {state: 'pending'}. The second leg is unchanged: context.getAuthResponse() still wins, and the exchange-and-cache path runs as before.

Behaviour source: _external_exchange_required in adk-python's src/google/adk/tools/openapi_tool/openapi_spec_parser/tool_auth_handler.py.

Deliberate parity deviation: the Python predicate keys off the credential alone, so clientCredentials also routes through a credential request there. This guard adds a second condition, the scheme's grant type. A two-legged clientCredentials exchange needs no human, and core/test/tools/openapi_tool/tool_auth_handler_test.ts already pins that. This is process-internal control flow, not a wire contract, so the local behaviour wins.

Deliberate fail-open: the guard is an allow-list of one grant, so every other scheme keeps today's behaviour rather than starting to return pending. That covers determineGrantType() === undefined (a bare openIdConnect scheme, or flows: {}) and the implicit and password grants. Those last two do need a human, but OAuth2CredentialExchanger implements neither, and AuthHandler.generateAuthUri() always builds a response_type=code URI against the authorization endpoint, falling back to the token endpoint when a flow has no authorizationUrl. Asking the user to sign in there would replace a stateless failure with an interactive dead end, and would cache the useless credential for the rest of the session.

Collision check: gh pr list on the fork shows four overlapping open PRs, none of which lands this change: #772 (refreshes a stored credential, same file, different region), #774 (widens the authScheme parameter type), #845 (rebuilds this test file on real Contexts), #773 (determineGrantType scheme guards). This PR branches from main.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

Seven tests were added to core/test/tools/openapi_tool/tool_auth_handler_test.ts. No existing test was changed. They build a real Context, so requestCredential mints the sign-in URI through the production AuthHandler. New code has 100% line and branch coverage; the file reports 96.55% branch and 88.11% line because of two pre-existing untested spots, fromToolContext and the || 'default' fallback in getCredentialKey.

Mutation testing, each run against the added tests:

Mutation Test that failed Message
Restore credential = authResponseCredential ?? this.authCredential asks the client to sign in for the authorization-code grant expected 'done' to be 'pending'
Compare the grant type against CLIENT_CREDENTIALS instead exchanges a client-credentials credential without asking the user, and the pre-existing caches a static credential that did require an exchange expected 'pending' to be 'done'
Delete the accessToken early return uses a configured OAuth2 credential that already carries an access token expected 'pending' to be 'done'
Delete the authType early return uses a configured bearer credential on an authorization-code scheme Auth Scheme oauth2 requires oauth2 in authCredential.

Commands run on the pushed commit:

npx vitest run --project unit:core \
  core/test/tools/openapi_tool/tool_auth_handler_test.ts \
  core/test/tools/openapi_tool/rest_api_tool_test.ts \
  core/test/auth/oauth2/oauth2_credential_exchanger_test.ts \
  core/test/auth/auth_schemes_test.ts \
  core/test/auth/auth_handler_test.ts     # 89 passed
npm run build -w core                     # ok
npx eslint <the two touched files>        # clean
npx prettier --check <the two touched files>  # clean
tsc --noEmit                              # 292 errors before and after, 6 in the touched
                                          # test file before and after; none added

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Build an OpenAPIToolset from a spec whose security scheme is oauth2 with an authorizationCode flow, pass authCredential: {authType: OAUTH2, oauth2: {clientId, clientSecret}}, and call tool.runAsync() once with a Context that has a functionCallId. No live provider is needed. I ran this script against the built package, with no mocks:

Before the fix:

CredentialExchangeError: clientId, clientSecret, and either authCode or authResponseUri
are required for authorization code exchange.

After the fix:

tool result: {"pending":true,"message":"Needs your authorization to access your data."}
auth uri: https://accounts.example.com/o/oauth2/auth?client_id=client-id&redirect_uri=
&response_type=code&scope=...&state=...&access_type=offline&prompt=consent

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

A RestApiTool configured with an OAuth2 authorizationCode scheme and a
{clientId, clientSecret} credential never reached
Context.requestCredential(): the configured credential is truthy, so the
handler passed it straight to the exchanger, which threw because no
authorization code was present. Gate the fallback on the grant type, so a
user-interactive grant with no access token asks the client to sign in and
returns {state: 'pending'}. Two-legged clientCredentials, apiKey, http and
serviceAccount credentials keep exchanging as before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant