fix: oauth refresh not triggered on token expiry

[MarlzRana]

Link to Issue or Description of Change

Link to an existing issue:

• Closes: OAuth Refresh Not Triggered On Token Expiry #3761

Problem:
The method below does not return the credential and whether the credential was exchanged. It returns the credential and whether an exchanger was found. Two different things, with the latter always true if one was able to exchange to get a credential initially, as an exchanger existed for that credential type.

adk-python/src/google/adk/auth/credential_manager.py

Lines 208 to 225 in 0094eea

	async def _exchange_credential(
	self, credential: AuthCredential
	) -> tuple[AuthCredential, bool]:
	"""Exchange credential if needed and return the credential and whether it was exchanged."""
	exchanger = self._exchanger_registry.get_exchanger(credential.auth_type)
	if not exchanger:
	return credential, False
	if isinstance(exchanger, ServiceAccountCredentialExchanger):
	exchanged_credential = exchanger.exchange_credential(
	self._auth_config.auth_scheme, credential
	)
	else:
	exchanged_credential = await exchanger.exchange(
	credential, self._auth_config.auth_scheme
	)
	return exchanged_credential, True

Which means in the aforementioned (modal) case, that in the below code was_exchanged is always true, and we never hit the code to refresh the credential if needed - which means credential refresh never happens.

adk-python/src/google/adk/auth/credential_manager.py

Lines 161 to 167 in 0094eea

	# Step 6: Exchange credential if needed (e.g., service account to access token)
	credential, was_exchanged = await self._exchange_credential(credential)
	# Step 7: Refresh credential if expired
	was_refreshed = False
	if not was_exchanged:
	credential, was_refreshed = await self._refresh_credential(credential)

Solution:
Let's get the credential exchangers to indicate whether they performed token exchange and pass that back correctly in _exchange_credential in the credential manager, so that was_exchanged is correctly set to False (and not always True) when we already have a token in the credential, and hence, we do check if the credential needs refreshing, and refresh if necessary (instead of always skipping over this logic when was_exchanged was always True prior to this PR).

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

Manual End-to-End (E2E) Tests:

Tested this locally with the ADK web and a long-running session that surpassed the duration of the token validity, and observed token refresh occur, and no longer obtained a 401.

Checklist

• I have read the CONTRIBUTING.md document.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have added tests that prove my fix is effective or that my feature works.
• New and existing unit tests pass locally with my changes.
• I have manually tested my changes end-to-end.
• Any dependent changes have been merged and published in downstream modules.

[gemini-code-assist]

Summary of Changes

Hello @MarlzRana, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses an issue where OAuth token refreshes were not being correctly triggered upon token expiry. The solution involves a significant refactoring of the credential exchange mechanism to provide more explicit feedback on whether a token exchange actually occurred. By modifying the exchange methods to return a boolean flag alongside the credential, the system can now accurately determine if a refresh was successful or necessary, leading to more robust and reliable authentication flows.

Highlights

• Enhanced OAuth Token Exchange Logic: The core change involves modifying the exchange methods within the authentication system to return a tuple containing both the AuthCredential and a boolean flag indicating whether a token exchange (or refresh) actually occurred. This provides clearer state management for credential updates.
• Improved Credential Management: The CredentialManager now explicitly checks this boolean flag to determine if a credential was successfully exchanged, which is crucial for correctly handling token expiry and refresh scenarios, ensuring that an OAuth refresh is properly triggered when needed.
• Updated OAuth2 Exchanger Behavior: The OAuth2CredentialExchanger now consistently returns (credential, False) in cases where an exchange is not needed (e.g., token already valid, authlib not available, session creation fails, fetch_token fails, or unsupported grant type) and (credential, True) upon successful exchange.
• Refactored Unit Tests: Corresponding unit tests for OAuth2CredentialExchanger have been updated to assert the new boolean return value and remove unnecessary @pytest.mark.asyncio decorators, leveraging pytest-asyncio's auto-detection for async test functions.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[adk-bot]

Response from ADK Triaging Agent

Hello @MarlzRana, thank you for creating this PR!

Could you please fill out the Testing Plan section in your PR description? This information will help reviewers to review your PR more efficiently. Thanks!

[gemini-code-assist]

Code Review

This pull request addresses a bug where OAuth tokens were not being refreshed upon expiry. The fix involves changing the exchange method in credential exchangers to return a tuple containing the credential and a boolean indicating whether an exchange actually occurred. This information is then used in CredentialManager to decide whether to attempt a token refresh.

The changes are logical and correctly fix the identified issue. The refactoring of the exchange method signature is consistently applied across BaseCredentialExchanger, OAuth2CredentialExchanger, and the corresponding tests.

I have provided a couple of comments with suggestions for improving code clarity and maintainability.

[gemini-code-assist]

For better readability, it's clearer to unpack the tuple returned by exchanger.exchange. This makes the intent of the code more explicit.

    credential, _ = await exchanger.exchange(
        self.auth_config.exchanged_auth_credential, self.auth_config.auth_scheme
    )
    return credential

[MarlzRana]

Agreed - addressed in 2dae391.

[gemini-code-assist]

While this change correctly handles the different interfaces of credential exchangers, the isinstance check points to a larger design issue. ServiceAccountCredentialExchanger does not conform to the BaseCredentialExchanger interface (it's not async, has a different method name exchange_credential, and a different return contract is being created for it here).

For better long-term maintainability, consider refactoring ServiceAccountCredentialExchanger to properly implement the BaseCredentialExchanger interface. This would remove the need for this special handling and make the credential exchange logic more uniform.

[MarlzRana]

For better long-term maintainability, consider refactoring ServiceAccountCredentialExchanger to properly implement the BaseCredentialExchanger interface. This would remove the need for this special handling and make the credential exchange logic more uniform.

Agreed. There are currently two BaseCredentialExchanger's (A) (B), B should be deprecated, and as part of that process ServiceAccountCredentialExchanger should inherit from A instead of B, and be moved to src/google/adk/auth/exchanger/service_account_exchanger.py instead of being in its current location (src/google/adk/tools/openapi_tool/auth/credential_exchangers/service_account_exchanger.py).

[adk-bot]

Response from ADK Triaging Agent

Hello @MarlzRana, thank you for creating this PR!

Could you please fill out the Testing Plan section in your PR description? This information will help reviewers to review your PR more efficiently. Thanks!

[ryanaiagent]

Hi @MarlzRana ,Thank you for your contribution! We appreciate you taking the time to submit this pull request. Your PR has been received by the team and is currently under review. We will provide feedback as soon as we have an update to share.

[ryanaiagent]

Hi @ankursharmas , can you please review this.

[xuanyang15]

Thank you @ryanaiagent for the triaging! I have started the review process since #3761 is assigned to me.