Remove AccessToken::is_expired() #2611
Conversation
github-actions
bot
added
Azure.Core
github-project-automation
bot
moved this to Untriaged
in Azure Identity SDK Improvements
chlowell
requested review from
heaths,
RickWinter and
LarryOsterman
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR removes the misleading AccessToken::is_expired() method and replaces it with a more robust token refresh mechanism while also adding test coverage and fixing an error in the proactive token refresh logic.
- Removed AccessToken::is_expired() and updated the token checking logic in cache and bearer token policy.
- Implemented a new helper (should_refresh) to determine token refresh timing.
- Enhanced error handling during token refresh and updated tests accordingly.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sdk/identity/azure_identity/src/credentials/cache.rs | Replaced usage of is_expired() with should_refresh and updated test expiration. |
| sdk/core/azure_core/src/http/policies/bearer_token_policy.rs | Refactored token refresh logic and improved error handling during refresh. |
| sdk/core/azure_core/src/credentials.rs | Removed the is_expired() method as part of the deprecation. |
| sdk/core/azure_core/CHANGELOG.md | Documented the breaking change and bug fix. |
| ### Bugs Fixed | ||
|
|
||
| - `BearerTokenCredentialPolicy` returns an error when a proactive token refresh attempt fails |
There was a problem hiding this comment.
Especially if this was a customer-filed bug, we reference the bug number such that it renders as (#{num}). If not a customer-filed issue, no need.
| Some(token) if should_refresh(&token.expires_on) => { | ||
| // token is expired or within its refresh window. Upgrade the lock and | ||
| // acquire a new token, provided another thread hasn't already done so | ||
| let expires_on = token.expires_on; | ||
| drop(access_token); | ||
| let mut access_token = self.access_token.write().await; |
There was a problem hiding this comment.
We probably don't need to take a write lock this soon - only when we write. Is there any disadvantage to a couple of threads potentially authenticating but only the last writer wins? IIRC, this is how creds in the .NET SDK work, and generally advised if a guarded operation is non-destructive and, in this case, wouldn't get us throttled (I doubt that for auth - not within reason, anyway).
There was a problem hiding this comment.
Throttling is actually a real danger because IMDS allows only 5 requests per second. We may need to add backoff between proactive refresh attempts to reduce the risk further; a highly concurrent app could hammer IMDS during an outage and get throttled.
| use typespec_client_core::http::{policies::TransportPolicy, Method, TransportOptions}; | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| struct MockCredential { |
There was a problem hiding this comment.
Just FYI: we actually have one in azure_core_test, though maybe this one is purpose-built for this task.
...because it's unnecessary and its name is misleading. This PR also adds test coverage and fixes a bug: the policy currently returns errors from proactive refresh attempts even when it has a valid cached token. (There's no value in doing that because the client request could otherwise succeed.)