Split CacheRefreshReason.ForceRefreshOrClaims into ForceRefresh and WithClaims #5188
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refines how cache refresh reasons are handled by splitting the previous combined value into distinct reasons for force refresh and claims requests. The changes impact the CacheRefreshReason enum, update the related request logic in multiple token acquisition flows, and adjust associated tests and telemetry recording accordingly.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Identity.Test.Unit/TelemetryTests/HttpTelemetryTests.cs | Updated telemetry assertions replacing ForceRefreshOrClaims with ForceRefresh |
| tests/Microsoft.Identity.Test.Unit/RequestsTests/LongRunningOnBehalfOfTests.cs | Adjusted OBO assertions and added new test for claims handling |
| src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs | Replaced combined cache refresh reason with distinct handling for ForceRefresh and WithClaims |
| src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs | Similar update to adjust cache refresh reason logic |
| tests/Microsoft.Identity.Test.Integration.netcore/Infrastructure/HttpTelemetryRecorder.cs | Upgraded parsing logic for the new cache refresh reason values |
| src/client/Microsoft.Identity.Client/Cache/CacheRefreshReason.cs | Updated the enum definitions to remove ForceRefreshOrClaims and add WithClaims and TokenRejected |
| src/client/Microsoft.Identity.Client/Internal/Requests/Silent/CacheSilentStrategy.cs | Updated cache refresh reason assignment in the silent token acquisition flow |
| src/client/Microsoft.Identity.Client/Internal/Requests/OnBehalfOfRequest.cs | Revised logic and logging to distinguish between ForceRefresh and WithClaims |
Comments suppressed due to low confidence (1)
src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs:39
- [nitpick] When both ForceRefresh and Claims are set, this logic will assign CacheRefreshReason.ForceRefresh. Consider clarifying or documenting the prioritization of these flags to ensure the intended behavior is explicit.
AuthenticationRequestParameters.RequestContext.ApiEvent.CacheInfo = _managedIdentityParameters.ForceRefresh ? CacheRefreshReason.ForceRefresh : CacheRefreshReason.WithClaims;
src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Identity.Test.Integration.netcore/Infrastructure/HttpTelemetryRecorder.cs
Show resolved
Hide resolved
|
The changes looks good to me. This change will result in change in all the queries that we have that looks at the cache refresh reason. Both the server side and client side. @bgavrilMS do you know if anyone else consumes this data? If they do then this might be breaking change for them. But we should update all the queries in the dashboards and the oneNote once this change goes in. |
src/client/Microsoft.Identity.Client/Cache/CacheRefreshReason.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| ForceRefreshOrClaims = 1, |
There was a problem hiding this comment.
question
Is removing this value a breaking change? If so, we may keep both ForceRefreshOrClaims = 1 and ForceRefresh = 1 (Csharp allows that), and use <summary>...</summary> to describe that ForceRefreshOrClaims remains only for backward compatibility purposes.
This way, it may also address @neha-bhargava 's call-out of compatibility concern.
Existing telemetry and dashboards will technically still work as-is; it is just that they won't see the new reasons until they are updated.
There was a problem hiding this comment.
Maybe leave value 1 as "not longer used" ? And use new values for everything else?
There was a problem hiding this comment.
@neha-bhargava ^^ can we do that ?
There was a problem hiding this comment.
updated per Bogdan's suggestion
There was a problem hiding this comment.
This does not seem right, we cannot change all the values. This will break all the server side queries. If you are adding new values, they get assigned new numbers in the enum.
src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Identity.Test.Unit/RequestsTests/LongRunningOnBehalfOfTests.cs
Show resolved
Hide resolved
Co-authored-by: Travis Walker <travis.walker@microsoft.com> Co-authored-by: Neha Bhargava <61847233+neha-bhargava@users.noreply.github.com>
made it |
I do not. Let's postpone this change and discuss it separately? |
| /// <summary> | ||
| /// Indicates that the resource (e.g., Microsoft Graph) has rejected the provided token. | ||
| /// </summary> | ||
| TokenRejectedByResource = 7 |
There was a problem hiding this comment.
rayluo can you please confirm the final name to be used?
suggestion (non-blocking)
From logical standpoint, I would still prefer to use TokenRejectedByCaller, because that is really it: the MSAL caller calls the WithAccessTokenSha256ToRefresh(...) for some reason; we hope it was caused by resource rejecting it, but we won't know for sure. (One real-world example is that another team at one point attempted proactively refreshing token more aggressively; even though we subsequently persuaded them not to, we can't rule out someone else would do it via WithAccessTokenSha256ToRefresh() in the future for other (possibly legit) reason.)
P.S.: Now that the ForceRefreshOrClaims split is on-hold, but perhaps we can at least add a TokenRejectedByCaller = 7 // Or 6, to unblock the WithAccessTokenSha256ToRefresh() PR.
let me come up with a mini spec for this and we can discuss different options of doing this. |
| /// When the token request goes to the identity provider because no cached access token exists | ||
| /// </summary> | ||
| NoCachedAccessToken = 2, | ||
| NoCachedAccessToken = 3, |
There was a problem hiding this comment.
@neha-bhargava is correct that this change does not look right. The original topic of that conversation was to keep an alias on the SDK implementation WITHOUT altering any old values. We shall never alter old values.
Please take this into consideration in the upcoming mini specs.
|
Closing this as we plan to revisit the design. |
Fixes #5186
Changes proposed in this request
This pull request includes several changes to the
CacheRefreshReasonenum and updates to the related logic and tests across multiple files. The changes focus on refining the reasons for cache refresh and improving the logging and handling of these reasons.Enum and Logic Updates:
src/client/Microsoft.Identity.Client/Cache/CacheRefreshReason.cs: UpdatedCacheRefreshReasonenum by removingForceRefreshOrClaimsand addingWithClaimsandTokenRejectedreasons. [1] [2]src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs: Updated cache refresh reason logic to distinguish betweenForceRefreshandWithClaims.src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs: Similar update to cache refresh reason logic.src/client/Microsoft.Identity.Client/Internal/Requests/OnBehalfOfRequest.cs: Updated to log specific cache refresh reasons.src/client/Microsoft.Identity.Client/Internal/Requests/Silent/CacheSilentStrategy.cs: Updated to log specific cache refresh reasons.Public API Updates:
src/client/Microsoft.Identity.Client/PublicApi/net462/PublicAPI.Shipped.txt: RemovedForceRefreshOrClaimsfrom the shipped API.src/client/Microsoft.Identity.Client/PublicApi/net462/PublicAPI.Unshipped.txt: AddedForceRefresh,WithClaims, andTokenRejectedto the unshipped API.PublicAPI.Shipped.txtandPublicAPI.Unshipped.txtfiles for different frameworks. [1] [2] [3] [4] [5]Test Updates:
tests/Microsoft.Identity.Test.Integration.netcore/Infrastructure/HttpTelemetryRecorder.cs: Updated to handle new cache refresh reasons.tests/Microsoft.Identity.Test.Unit/RequestsTests/LongRunningOnBehalfOfTests.cs: Added a new test forWithClaimsand updated existing tests to reflect the new enum values. [1] [2]tests/Microsoft.Identity.Test.Unit/TelemetryTests/HttpTelemetryTests.cs: Updated telemetry test to useForceRefreshinstead ofForceRefreshOrClaims.These changes collectively enhance the granularity and clarity of cache refresh reasons, improving both logging and functionality.
Testing
unit testing
Performance impact
none
Documentation