fix(auth): strip redirect_uri from credential_key
Merge https://github.com/google/adk-python/pull/5692
### Link to Issue or Description of Change
**1. Link to an existing issue (if applicable):**
- Closes: #5691
This change adds `auth_credential.oauth2.redirect_uri = None` to the OAuth2 strip block at the three call sites where credential hashing happens. `redirect_uri` is deployment configuration (which callback URL the auth server should redirect to), not part of the credential identity, so it should be excluded from the hash just as access_token, refresh_token, expires_at, and the other transient OAuth2 fields already are. Without this change, a credential minted under one deployment URL cannot be retrieved when the deployment moves to another.
### Testing Plan
**Unit Tests:**
- [x] I have added or updated unit tests for my change.
- [x] All unit tests pass locally.
Three new tests, one per affected method. Each constructs two `AuthCredential` instances that differ only in `redirect_uri` and asserts the computed key is identical:
- `tests/unittests/tools/openapi_tool/openapi_spec_parser/test_tool_auth_handler.py::test_credential_key_is_stable_across_redirect_uri`
- `tests/unittests/tools/openapi_tool/openapi_spec_parser/test_tool_auth_handler.py::test_legacy_credential_key_is_stable_across_redirect_uri`
- `tests/unittests/auth/test_auth_config.py::test_credential_key_is_stable_across_redirect_uri`
```
$ pytest tests/unittests/tools/openapi_tool/openapi_spec_parser/test_tool_auth_handler.py tests/unittests/auth/test_auth_config.py
======================== 14 passed, 8 warnings in 2.90s ========================
$ pytest tests/unittests/
================ 5740 passed, 2340 warnings in 86.64s (0:01:26) ================
```
**Manual End-to-End (E2E) Tests:**
A self-contained Runner-based reproduction is at https://github.com/doughayden/adk-issue-examples/tree/main/05-redirect_uri_in_credential_hash. The agent definition (`agent.py`) wires up an `OpenAPIToolset` against a local OAuth2 test server, configured with one redirect_uri value. `main.py` constructs an `InMemoryRunner`, seeds a real (non-expired) OAuth2 credential into session state under a different redirect_uri's hash, and runs the agent. The `--apply-fix` flag monkey-patches the proposed fix to demonstrate the resolution end-to-end.
Without the fix:
```
🌤️ WeatherAssistant Agent — redirect_uri-in-hash repro
============================================================
Proposed fix applied: False
STORED_REDIRECT_URI: http://localhost:8080/callback
CURRENT_REDIRECT_URI: http://localhost:8081/callback
Hash keys produced by ToolContextCredentialStore.get_credential_key:
STORED → oauth2_55f666541ad22e39_oauth2_8ba0457897522d9d_existing_exchanged_credential
CURRENT → oauth2_55f666541ad22e39_oauth2_ae16199243c358df_existing_exchanged_credential
❌ Keys differ — credentials minted under STORED are not retrievable.
👤 User: What's the weather in San Francisco?
🌤️ Weather Assistant event stream:
[function_call] get_weather by WeatherAssistant
[auth_event] adk_request_credential by WeatherAssistant
[function_response] get_weather by WeatherAssistant
[text] WeatherAssistant: 'It seems I need your authorization to access weather data. Could you please g...'
Event counts:
function_calls: 1
auth_events: 1
function_responses: 1
text_events: 1
✅ Bug reproduced: agent emitted 1 adk_request_credential event(s) despite a valid seeded credential being present in state.
```
With the fix:
```
🌤️ WeatherAssistant Agent — redirect_uri-in-hash repro
============================================================
Proposed fix applied: True
STORED_REDIRECT_URI: http://localhost:8080/callback
CURRENT_REDIRECT_URI: http://localhost:8081/callback
Hash keys produced by ToolContextCredentialStore.get_credential_key:
STORED → oauth2_55f666541ad22e39_oauth2_c2ad46dffd26cd87_existing_exchanged_credential
CURRENT → oauth2_55f666541ad22e39_oauth2_c2ad46dffd26cd87_existing_exchanged_credential
✅ Keys match — fix is taking effect at the hash level.
👤 User: What's the weather in San Francisco?
🌤️ Weather Assistant event stream:
[function_call] get_weather by WeatherAssistant
[function_response] get_weather by WeatherAssistant
[text] WeatherAssistant: 'The weather in San Francisco is Clear with a temperature of 30 degrees Celsiu...'
Event counts:
function_calls: 1
auth_events: 0
function_responses: 1
text_events: 1
✅ Fix verified: tool call succeeded against the seeded credential without an adk_request_credential prompt.
```
### Checklist
- [x] I have read the [CONTRIBUTING.md](https://github.com/google/adk-python/blob/main/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.
- [ ] Any dependent changes have been merged and published in downstream modules.
### Additional context
**Scope:**
The same strip block exists at three call sites and has the same gap at all three. This PR patches all three. Patching only the tool-level pair (`tool_auth_handler.py`) and leaving the framework-level path (`auth_tool.py:AuthConfig.get_credential_key`) would leave the bug reachable for any consumer that does not work around #5327 with `get_auth_config = lambda: None`. Patching only `AuthConfig.get_credential_key` and leaving the tool-level pair would leave the bug reachable on the standard tool-level credential lookup path.
**Upgrade note:**
The credential_key shape changes with this fix: `redirect_uri` is no longer included in the hash. OAuth credentials cached in existing session state under the pre-fix key shape become unreachable under the new key. Users should expect a one-time re-auth prompt on the first run after upgrading. Subsequent runs use the new key normally.
**Related:**
- #5327 (preemptive toolset auth)
- #5328 (refresh request scope)
- #5329 (refreshed credential persistence, fixed in 218ea76e)
- #5637 (tool-level auth termination)
Co-authored-by: George Weale <gweale@google.com>
COPYBARA_INTEGRATE_REVIEW=https://github.com/google/adk-python/pull/5692 from doughayden:fix/credential-key-strip-redirect-uri 3be2a85c75ebca865467499de8bf29af3e735c31
PiperOrigin-RevId: 941217122
This commit is contained in:
doughayden
committed by
Copybara-Service
Copybara-Service
parent
14a24f2bee
commit
ffa1843951
@@ -121,14 +121,16 @@ class AuthConfig(BaseModelWithConfig):
|
||||
auth_credential.model_extra.clear()
|
||||
if auth_credential and auth_credential.oauth2:
|
||||
auth_credential = auth_credential.model_copy(deep=True)
|
||||
auth_credential.oauth2.auth_uri = None
|
||||
auth_credential.oauth2.state = None
|
||||
auth_credential.oauth2.auth_response_uri = None
|
||||
auth_credential.oauth2.auth_code = None
|
||||
auth_credential.oauth2.access_token = None
|
||||
auth_credential.oauth2.refresh_token = None
|
||||
auth_credential.oauth2.expires_at = None
|
||||
auth_credential.oauth2.expires_in = None
|
||||
if auth_credential.oauth2:
|
||||
auth_credential.oauth2.auth_uri = None
|
||||
auth_credential.oauth2.state = None
|
||||
auth_credential.oauth2.auth_response_uri = None
|
||||
auth_credential.oauth2.auth_code = None
|
||||
auth_credential.oauth2.access_token = None
|
||||
auth_credential.oauth2.refresh_token = None
|
||||
auth_credential.oauth2.expires_at = None
|
||||
auth_credential.oauth2.expires_in = None
|
||||
auth_credential.oauth2.redirect_uri = None
|
||||
credential_name = (
|
||||
f"{auth_credential.auth_type.value}_{_stable_model_digest(auth_credential)}"
|
||||
if auth_credential
|
||||
|
||||
@@ -62,14 +62,16 @@ class ToolContextCredentialStore:
|
||||
) -> str:
|
||||
if auth_credential and auth_credential.oauth2:
|
||||
auth_credential = auth_credential.model_copy(deep=True)
|
||||
auth_credential.oauth2.auth_uri = None
|
||||
auth_credential.oauth2.state = None
|
||||
auth_credential.oauth2.auth_response_uri = None
|
||||
auth_credential.oauth2.auth_code = None
|
||||
auth_credential.oauth2.access_token = None
|
||||
auth_credential.oauth2.refresh_token = None
|
||||
auth_credential.oauth2.expires_at = None
|
||||
auth_credential.oauth2.expires_in = None
|
||||
if auth_credential.oauth2:
|
||||
auth_credential.oauth2.auth_uri = None
|
||||
auth_credential.oauth2.state = None
|
||||
auth_credential.oauth2.auth_response_uri = None
|
||||
auth_credential.oauth2.auth_code = None
|
||||
auth_credential.oauth2.access_token = None
|
||||
auth_credential.oauth2.refresh_token = None
|
||||
auth_credential.oauth2.expires_at = None
|
||||
auth_credential.oauth2.expires_in = None
|
||||
auth_credential.oauth2.redirect_uri = None
|
||||
scheme_name = (
|
||||
f"{auth_scheme.type_.name}_{self._legacy_stable_digest(auth_scheme.model_dump_json())}"
|
||||
if auth_scheme
|
||||
@@ -91,14 +93,16 @@ class ToolContextCredentialStore:
|
||||
|
||||
if auth_credential and auth_credential.oauth2:
|
||||
auth_credential = auth_credential.model_copy(deep=True)
|
||||
auth_credential.oauth2.auth_uri = None
|
||||
auth_credential.oauth2.state = None
|
||||
auth_credential.oauth2.auth_response_uri = None
|
||||
auth_credential.oauth2.auth_code = None
|
||||
auth_credential.oauth2.access_token = None
|
||||
auth_credential.oauth2.refresh_token = None
|
||||
auth_credential.oauth2.expires_at = None
|
||||
auth_credential.oauth2.expires_in = None
|
||||
if auth_credential.oauth2:
|
||||
auth_credential.oauth2.auth_uri = None
|
||||
auth_credential.oauth2.state = None
|
||||
auth_credential.oauth2.auth_response_uri = None
|
||||
auth_credential.oauth2.auth_code = None
|
||||
auth_credential.oauth2.access_token = None
|
||||
auth_credential.oauth2.refresh_token = None
|
||||
auth_credential.oauth2.expires_at = None
|
||||
auth_credential.oauth2.expires_in = None
|
||||
auth_credential.oauth2.redirect_uri = None
|
||||
scheme_name = (
|
||||
f"{auth_scheme.type_.name}_{_stable_model_digest(auth_scheme)}"
|
||||
if auth_scheme
|
||||
|
||||
@@ -176,3 +176,41 @@ def test_credential_key_with_custom_auth_scheme():
|
||||
key = custom_config.credential_key
|
||||
assert key.startswith("adk_mock_custom_type_")
|
||||
assert len(key) > len("adk_mock_custom_type_")
|
||||
|
||||
|
||||
def test_credential_key_is_stable_across_redirect_uri(oauth2_auth_scheme):
|
||||
"""AuthConfig.credential_key should be invariant under redirect_uri changes.
|
||||
|
||||
redirect_uri is deployment configuration (which callback URL the auth
|
||||
server should redirect to), not part of the credential identity. Two
|
||||
AuthConfig instances built from credentials that share the same client_id,
|
||||
client_secret, and scopes but differ only in redirect_uri should produce
|
||||
the same credential_key.
|
||||
"""
|
||||
credential_local = AuthCredential(
|
||||
auth_type=AuthCredentialTypes.OAUTH2,
|
||||
oauth2=OAuth2Auth(
|
||||
client_id="client",
|
||||
client_secret="secret",
|
||||
redirect_uri="http://localhost:8001/oauth2callback",
|
||||
),
|
||||
)
|
||||
credential_deployed = AuthCredential(
|
||||
auth_type=AuthCredentialTypes.OAUTH2,
|
||||
oauth2=OAuth2Auth(
|
||||
client_id="client",
|
||||
client_secret="secret",
|
||||
redirect_uri="https://deployed.example.com/oauth2callback",
|
||||
),
|
||||
)
|
||||
|
||||
config_local = AuthConfig(
|
||||
auth_scheme=oauth2_auth_scheme,
|
||||
raw_auth_credential=credential_local,
|
||||
)
|
||||
config_deployed = AuthConfig(
|
||||
auth_scheme=oauth2_auth_scheme,
|
||||
raw_auth_credential=credential_deployed,
|
||||
)
|
||||
|
||||
assert config_local.credential_key == config_deployed.credential_key
|
||||
|
||||
@@ -353,3 +353,65 @@ async def test_refreshed_credential_is_persisted_to_store(
|
||||
assert persisted is not None
|
||||
assert persisted.oauth2.access_token == 'new_access_token'
|
||||
assert persisted.oauth2.refresh_token == 'new_refresh_token'
|
||||
|
||||
|
||||
def test_credential_key_is_stable_across_redirect_uri():
|
||||
"""get_credential_key should be invariant under redirect_uri changes.
|
||||
|
||||
redirect_uri is deployment configuration (which callback URL the auth
|
||||
server should redirect to), not part of the credential identity. Two
|
||||
AuthCredential instances that share the same client_id, client_secret,
|
||||
and scopes but differ only in redirect_uri should produce the same key.
|
||||
"""
|
||||
scheme, _ = get_mock_openid_scheme_credential()
|
||||
credential_local = AuthCredential(
|
||||
auth_type=AuthCredentialTypes.OAUTH2,
|
||||
oauth2=OAuth2Auth(
|
||||
client_id='client',
|
||||
client_secret='secret',
|
||||
redirect_uri='http://localhost:8001/oauth2callback',
|
||||
),
|
||||
)
|
||||
credential_deployed = AuthCredential(
|
||||
auth_type=AuthCredentialTypes.OAUTH2,
|
||||
oauth2=OAuth2Auth(
|
||||
client_id='client',
|
||||
client_secret='secret',
|
||||
redirect_uri='https://deployed.example.com/oauth2callback',
|
||||
),
|
||||
)
|
||||
store = ToolContextCredentialStore(tool_context=create_mock_tool_context())
|
||||
|
||||
assert store.get_credential_key(
|
||||
scheme, credential_local
|
||||
) == store.get_credential_key(scheme, credential_deployed)
|
||||
|
||||
|
||||
def test_legacy_credential_key_is_stable_across_redirect_uri():
|
||||
"""_get_legacy_credential_key should be invariant under redirect_uri changes.
|
||||
|
||||
The same redirect_uri-strip behavior must apply to the legacy key path so
|
||||
that already-stored credentials remain findable after the fix.
|
||||
"""
|
||||
scheme, _ = get_mock_openid_scheme_credential()
|
||||
credential_local = AuthCredential(
|
||||
auth_type=AuthCredentialTypes.OAUTH2,
|
||||
oauth2=OAuth2Auth(
|
||||
client_id='client',
|
||||
client_secret='secret',
|
||||
redirect_uri='http://localhost:8001/oauth2callback',
|
||||
),
|
||||
)
|
||||
credential_deployed = AuthCredential(
|
||||
auth_type=AuthCredentialTypes.OAUTH2,
|
||||
oauth2=OAuth2Auth(
|
||||
client_id='client',
|
||||
client_secret='secret',
|
||||
redirect_uri='https://deployed.example.com/oauth2callback',
|
||||
),
|
||||
)
|
||||
store = ToolContextCredentialStore(tool_context=create_mock_tool_context())
|
||||
|
||||
assert store._get_legacy_credential_key(
|
||||
scheme, credential_local
|
||||
) == store._get_legacy_credential_key(scheme, credential_deployed)
|
||||
|
||||
Reference in New Issue
Block a user