adk-python
adk-python copied to clipboard
fix: oauth refresh not triggered on token expiry
Link to Issue or Description of Change
Link to an existing issue:
- Closes: https://github.com/google/adk-python/issues/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. https://github.com/google/adk-python/blob/0094eea3cadf5fe2e960cc558e467dd2131de1b7/src/google/adk/auth/credential_manager.py#L208-L225
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.
https://github.com/google/adk-python/blob/0094eea3cadf5fe2e960cc558e467dd2131de1b7/src/google/adk/auth/credential_manager.py#L161-L167
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:
- [x] I have added or updated unit tests for my change.
- [x] 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
- [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.
- [x] I have manually tested my changes end-to-end.
- [x] Any dependent changes have been merged and published in downstream modules.
Summary of Changes
Hello @MarlzRana, I'm Gemini Code Assist[^1]! 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
exchangemethods within the authentication system to return a tuple containing both theAuthCredentialand a boolean flag indicating whether a token exchange (or refresh) actually occurred. This provides clearer state management for credential updates. - Improved Credential Management: The
CredentialManagernow 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
OAuth2CredentialExchangernow consistently returns(credential, False)in cases where an exchange is not needed (e.g., token already valid,authlibnot available, session creation fails,fetch_tokenfails, or unsupported grant type) and(credential, True)upon successful exchange. - Refactored Unit Tests: Corresponding unit tests for
OAuth2CredentialExchangerhave been updated to assert the new boolean return value and remove unnecessary@pytest.mark.asynciodecorators, leveragingpytest-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 :thumbsup: and :thumbsdown: 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.
[^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.
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!
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!