[Identity] Fix identity cache issue - #49001
Kashif Khan (kashifkhan) wants to merge 5 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive cache-isolation change lacks the synchronous on-behalf-of implementation and coverage needed to verify parity.
Pull request overview
Fixes cross-user token leakage when multiple authorization-code or on-behalf-of credentials share an MSAL token cache.
Changes:
- Defers cache lookup until the credential establishes its account through an initial token exchange.
- Scopes subsequent access- and refresh-token lookups by
home_account_id. - Adds synchronous and asynchronous regression coverage for shared-cache scenarios.
File summaries
| File | Description |
|---|---|
sdk/identity/azure-identity/tests/test_obo_async.py |
Tests async OBO shared-cache account isolation. |
sdk/identity/azure-identity/tests/test_auth_code.py |
Tests sync authorization-code access/refresh-token isolation and fallback identity extraction. |
sdk/identity/azure-identity/tests/test_auth_code_async.py |
Tests async authorization-code shared-cache isolation. |
sdk/identity/azure-identity/azure/identity/aio/_credentials/on_behalf_of.py |
Prevents pre-exchange cache use and scopes later OBO cache lookups. |
sdk/identity/azure-identity/azure/identity/aio/_credentials/authorization_code.py |
Scopes async authorization-code cache lookups to the established account. |
sdk/identity/azure-identity/azure/identity/_internal/aad_client_base.py |
Tracks exchanged account identity and supports account-filtered cache searches. |
sdk/identity/azure-identity/azure/identity/_credentials/authorization_code.py |
Scopes sync authorization-code cache lookups to the established account. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Responses lacking both account-identifying fields still permit unscoped shared-cache lookups and potential cross-user token reuse.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sdk/identity/azure-identity/tests/test_auth_code.py:412
- This request only exercises the “authorization code is still present” branch, which bypasses the cache regardless of whether the ID-token fallback works. The test would still pass if
_get_home_account_idstopped readingsub; assert the establishedlast_home_account_id(or perform a subsequent account-bound cache lookup) so the advertised fallback is actually covered.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive authentication and shared-cache behavior warrants final human review despite focused regression coverage.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new API spec, a link to the pull request containing these API spec changes should be included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines