Repository navigation
fix(auth): handle an expired OAuth2 token that has no refresh token - #7729
Open
AkshayShah03 wants to merge 2 commits into
Open
AkshayShah03 wants to merge 2 commits into
AkshayShah03 wants to merge 2 commits into
Conversation
OAuth2CredentialRefresher called refresh_token(None), which raised AttributeError inside authlib, so every expired client-credentials token (and user tokens from providers that don't issue a refresh token) crashed the tool call. Return the original credential when there is no refresh token, and for the client credentials flow fetch a new token from the raw client credentials instead. Fixes google#7728
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
OAuth2CredentialRefresher.refreshcallsclient.refresh_token(..., refresh_token=None)when the expired token has no refresh token. authlib then raisesAttributeError: 'NoneType' object has no attribute 'get', which is not caught, so the tool call fails. This happens every time a client-credentials token expires (that grant has no refresh token), and for user tokens from providers that don't issue one.Solution:
OAuth2CredentialRefresher.refresh: when there is no refresh token, log a warning and return the original credential, the same as for a failed refresh (and as the docstring says).CredentialManager._refresh_credential: for the client credentials flow, when the stored token has expired and has no refresh token, run the exchange again on a copy of the raw client credentials to get a new token. If that exchange fails, the stored credential is returned and not re-saved. The sharedraw_auth_credentialis not mutated.Other flows keep the current behaviour after a refresh that can't happen: the stale token is returned, so the tool's 401 handling (for example
RestApiTool) asks the user to authorize again.Testing Plan
Unit Tests:
Added:
test_refresh_without_refresh_token_returns_original_and_logsintests/unittests/auth/refresher/test_oauth2_credential_refresher.py. It uses a real authlib session; onmainit fails with theAttributeError.test_get_auth_credential_refetches_expired_client_credentials_tokenintests/unittests/auth/test_credential_manager.py, for an OAuth2 client-credentials scheme and an OIDC scheme withclient_credentialsingrant_types_supported. A stored, expired client-credentials token is replaced by a newly fetched one and saved;refresh_token()is not called, and the shared raw credential is unchanged. Onmainboth cases fail becauserefresh_token()is called without a refresh token.test_get_auth_credential_refreshes_client_credentials_token_with_refresh_token: a client-credentials token that does have a refresh token is still refreshed (not re-fetched). This passes onmaintoo; it guards the existing path.The one failure,
integrations/vmaas/test_agent_engine_environment.py::test_reading_directory_raises, also fails onmainwithout this change.pyinkandisortreport no changes.Manual End-to-End (E2E) Tests:
Ran
CredentialManager.get_auth_credentialwith a stored expired token (no refresh token), withOAuth2Session.requestpatched to a fake token endpoint:main: both the authorization-code and client-credentials schemes raiseAttributeError: 'NoneType' object has no attribute 'get', with no HTTP request made.grant_type=client_credentialsrequest and returns the new token.The snippet in #7728 prints
AttributeErroronmainandreturned: expiredwith this change.Also ran
CredentialManager.get_auth_credentialagainst a real IdP: a local Keycloak 26.0.7 (realm with a confidential client and a user). Each stored token was marked expired, and the new token was checked with Keycloak's introspection endpoint. Keycloak's client-credentials grant returns norefresh_token, as expected.mainAttributeErrorclient_credentialsingrant_types_supported), expired, no refresh tokenAttributeErrorAttributeErrorinvalid_grant)Checklist