pr comments

GladwinJohnson · GladwinJohnson · commit fcec87b00c88 · 2025-03-12T17:27:53.000-07:00
diff --git a/src/client/Microsoft.Identity.Client/ApiConfig/Parameters/AcquireTokenForClientParameters.cs b/src/client/Microsoft.Identity.Client/ApiConfig/Parameters/AcquireTokenForClientParameters.cs
@@ -26,6 +26,7 @@ public void LogParameters(ILoggerAdapter logger)
                 builder.AppendLine("=== AcquireTokenForClientParameters ===");
                 builder.AppendLine("SendX5C: " + SendX5C);
                 builder.AppendLine("ForceRefresh: " + ForceRefresh);
+                builder.AppendLine($"AccessTokenHashToRefresh: {!string.IsNullOrEmpty(AccessTokenHashToRefresh)}");
                 logger.Info(builder.ToString());
             }
         }
diff --git a/src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs b/src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs
@@ -202,6 +202,10 @@ private async Task<AuthenticationResult> SendTokenRequestToAppTokenProviderAsync
             return authResult;
         }
 
+        /// <summary>
+        /// Checks if the token should be used from the cache.
+        /// </summary>
+        /// <returns></returns>
         private async Task<MsalAccessTokenCacheItem> GetCachedAccessTokenAsync()
         {
             // Fetch the cache item (could be null if none found).
@@ -219,6 +223,11 @@ private async Task<MsalAccessTokenCacheItem> GetCachedAccessTokenAsync()
             return cacheItem;
         }
 
+        /// <summary>
+        /// Checks if the token should be used from the cache.
+        /// </summary>
+        /// <param name="cacheItem"></param>
+        /// <returns></returns>
         private bool ShouldUseCachedToken(MsalAccessTokenCacheItem cacheItem)
         {
             // 1) No cached item 
@@ -239,18 +248,32 @@ private bool ShouldUseCachedToken(MsalAccessTokenCacheItem cacheItem)
             return true;
         }
 
+        /// <summary>
+        /// Checks if the token hash matches the hash provided in AccessTokenHashToRefresh.
+        /// </summary>
+        /// <param name="tokenSecret"></param>
+        /// <param name="accessTokenHashToRefresh"></param>
+        /// <returns></returns>
         private bool IsMatchingTokenHash(string tokenSecret, string accessTokenHashToRefresh)
         {
             string cachedTokenHash = _cryptoManager.CreateSha256Hash(tokenSecret);
             return string.Equals(cachedTokenHash, accessTokenHashToRefresh, StringComparison.Ordinal);
         }
 
+        /// <summary>
+        /// Marks the request as a cache hit and increments the cache hit count.
+        /// </summary>
         private void MarkAccessTokenAsCacheHit()
         {
             AuthenticationRequestParameters.RequestContext.ApiEvent.IsAccessTokenCacheHit = true;
             Metrics.IncrementTotalAccessTokensFromCache();
         }
 
+        /// <summary>
+        /// returns the cached access token item 
+        /// </summary>
+        /// <param name="cachedAccessTokenItem"></param>
+        /// <returns></returns>
         private AuthenticationResult CreateAuthenticationResultFromCache(MsalAccessTokenCacheItem cachedAccessTokenItem)
         {
             AuthenticationResult authResult = new AuthenticationResult(
@@ -266,6 +289,11 @@ private AuthenticationResult CreateAuthenticationResultFromCache(MsalAccessToken
             return authResult;
         }
 
+        /// <summary>
+        /// Gets overriden scopes for client credentials flow
+        /// </summary>
+        /// <param name="inputScopes"></param>
+        /// <returns></returns>
         protected override SortedSet<string> GetOverriddenScopes(ISet<string> inputScopes)
         {
             // Client credentials should not add the reserved scopes
@@ -274,6 +302,10 @@ protected override SortedSet<string> GetOverriddenScopes(ISet<string> inputScope
             return new SortedSet<string>(inputScopes);
         }
 
+        /// <summary>
+        /// Gets the body parameters for the client credentials flow
+        /// </summary>
+        /// <returns></returns>
         private Dictionary<string, string> GetBodyParameters()
         {
             var dict = new Dictionary<string, string>
@@ -285,6 +317,11 @@ private Dictionary<string, string> GetBodyParameters()
             return dict;
         }
 
+        /// <summary>
+        /// Gets the CCS header for the client credentials flow
+        /// </summary>
+        /// <param name="additionalBodyParameters"></param>
+        /// <returns></returns>
         protected override KeyValuePair<string, string>? GetCcsHeader(IDictionary<string, string> additionalBodyParameters)
         {
             return null;
diff --git a/tests/Microsoft.Identity.Test.Unit/PublicApiTests/ConfidentialClientApplicationTests.cs b/tests/Microsoft.Identity.Test.Unit/PublicApiTests/ConfidentialClientApplicationTests.cs
@@ -2157,6 +2157,8 @@ public void ConfidentialClient_WithInvalidAuthority_ThrowsArgumentException()
         [TestMethod]
         public async Task WithAccessTokenSha256ToRefresh_MatchingHash_GetsTokenFromIdp_Async()
         {
+            const string accessToken = "access-token";
+
             // Arrange
             using (var httpManager = new MockHttpManager())
             {
@@ -2170,19 +2172,19 @@ public async Task WithAccessTokenSha256ToRefresh_MatchingHash_GetsTokenFromIdp_A
                     .BuildConcrete();
 
                 // 1) First network call: populates the cache with "access-token"
-                httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(token: "access-token");
+                httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(token: accessToken);
                 AuthenticationResult initialResult = await app.AcquireTokenForClient(TestConstants.s_scope).ExecuteAsync().ConfigureAwait(false);
 
-                Assert.AreEqual("access-token", initialResult.AccessToken);
+                Assert.AreEqual(accessToken, initialResult.AccessToken);
                 Assert.AreEqual(TokenSource.IdentityProvider, initialResult.AuthenticationResultMetadata.TokenSource);
 
                 // 2) Second call: re-check the cache. Should see the same token from cache
                 AuthenticationResult secondResult = await app.AcquireTokenForClient(TestConstants.s_scope).ExecuteAsync().ConfigureAwait(false);
-                Assert.AreEqual("access-token", secondResult.AccessToken);
+                Assert.AreEqual(accessToken, secondResult.AccessToken);
                 Assert.AreEqual(TokenSource.Cache, secondResult.AuthenticationResultMetadata.TokenSource);
 
                 // 3) Now specify the same token's hash as "bad" => expect a new token from IdP
-                string tokenHash = ComputeSHA256("access-token");
+                string tokenHash = ComputeSHA256(accessToken);
 
                 // Add another network response to simulate fetching a new token
                 httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(token: "new-access-token");
@@ -2263,6 +2265,9 @@ public async Task ForceRefreshAndAccessTokenHash_ThrowsException_Async()
         [TestMethod]
         public async Task AcquireTokenForClient_WithClaims_And_MatchingHash_SkipsCache_Async()
         {
+            const string oldToken = "old-token";
+            const string freshToken = "fresh-token";
+
             using (var httpManager = new MockHttpManager())
             {
                 httpManager.AddInstanceDiscoveryMockHandler();
@@ -2275,16 +2280,16 @@ public async Task AcquireTokenForClient_WithClaims_And_MatchingHash_SkipsCache_A
                     .WithExperimentalFeatures(true)
                     .BuildConcrete();
 
-                httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(token: "old-token");
+                httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(token: oldToken);
                 AuthenticationResult firstResult = await app.AcquireTokenForClient(TestConstants.s_scope).ExecuteAsync().ConfigureAwait(false);
 
-                Assert.AreEqual("old-token", firstResult.AccessToken);
+                Assert.AreEqual(oldToken, firstResult.AccessToken);
 
                 // 2) We do matching hash => a new token is returned
-                string tokenHash = ComputeSHA256("old-token");
+                string tokenHash = ComputeSHA256(oldToken);
 
                 // Add second network response for the new token
-                httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(token: "fresh-token");
+                httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(token: freshToken);
 
                 // Act
                 AuthenticationResult result = await app.AcquireTokenForClient(TestConstants.s_scope)
@@ -2294,14 +2299,16 @@ public async Task AcquireTokenForClient_WithClaims_And_MatchingHash_SkipsCache_A
                     .ConfigureAwait(false);
 
                 // Assert => new token from the IDP
-                Assert.AreEqual("fresh-token", result.AccessToken);
+                Assert.AreEqual(freshToken, result.AccessToken);
                 Assert.AreEqual(TokenSource.IdentityProvider, result.AuthenticationResultMetadata.TokenSource);
             }
         }
 
         [TestMethod]
         public async Task AcquireTokenForClient_WithClaims_And_MismatchedHash_UsesCache_Async()
         {
+            const string cacheToken = "cache-token";
+
             using (var httpManager = new MockHttpManager())
             {
                 httpManager.AddInstanceDiscoveryMockHandler();
@@ -2315,13 +2322,13 @@ public async Task AcquireTokenForClient_WithClaims_And_MismatchedHash_UsesCache_
                     .BuildConcrete();
 
                 // First network call: populates the cache with "cache-token"
-                httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(token: "cache-token");
+                httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(token: cacheToken);
 
                 var initialResult = await app.AcquireTokenForClient(TestConstants.s_scope)
                     .ExecuteAsync()
                     .ConfigureAwait(false);
 
-                Assert.AreEqual("cache-token", initialResult.AccessToken);
+                Assert.AreEqual(cacheToken, initialResult.AccessToken);
                 Assert.AreEqual(TokenSource.IdentityProvider, initialResult.AuthenticationResultMetadata.TokenSource);
 
                 // 2) We'll do a mismatched hash => expect to keep using the cached token
@@ -2335,7 +2342,7 @@ public async Task AcquireTokenForClient_WithClaims_And_MismatchedHash_UsesCache_
                     .ConfigureAwait(false);
 
                 // Assert => we keep using the cached token
-                Assert.AreEqual("cache-token", result.AccessToken,
+                Assert.AreEqual(cacheToken, result.AccessToken,
                     "We reuse the cache if the hash does not match the 'bad' token’s hash.");
                 Assert.AreEqual(TokenSource.Cache, result.AuthenticationResultMetadata.TokenSource);
             }

Original file line number	Diff line number	Diff line change
`@@ -26,6 +26,7 @@ public void LogParameters(ILoggerAdapter logger)`
`26`	`26`	`builder.AppendLine("=== AcquireTokenForClientParameters ===");`
`27`	`27`	`builder.AppendLine("SendX5C: " + SendX5C);`
`28`	`28`	`builder.AppendLine("ForceRefresh: " + ForceRefresh);`
	`29`	`+ builder.AppendLine($"AccessTokenHashToRefresh: {!string.IsNullOrEmpty(AccessTokenHashToRefresh)}");`
`29`	`30`	`logger.Info(builder.ToString());`
`30`	`31`	`}`
`31`	`32`	`}`
Original file line number	Diff line number	Diff line change
`@@ -202,6 +202,10 @@ private async Task<AuthenticationResult> SendTokenRequestToAppTokenProviderAsync`
`202`	`202`	`return authResult;`
`203`	`203`	`}`
`204`	`204`
	`205`	`+ /// <summary>`
	`206`	`+ /// Checks if the token should be used from the cache.`
	`207`	`+ /// </summary>`
	`208`	`+ /// <returns></returns>`
`205`	`209`	`private async Task<MsalAccessTokenCacheItem> GetCachedAccessTokenAsync()`
`206`	`210`	`{`
`207`	`211`	`// Fetch the cache item (could be null if none found).`
`@@ -219,6 +223,11 @@ private async Task<MsalAccessTokenCacheItem> GetCachedAccessTokenAsync()`
`219`	`223`	`return cacheItem;`
`220`	`224`	`}`
`221`	`225`
	`226`	`+ /// <summary>`
	`227`	`+ /// Checks if the token should be used from the cache.`
	`228`	`+ /// </summary>`
	`229`	`+ /// <param name="cacheItem"></param>`
	`230`	`+ /// <returns></returns>`
`222`	`231`	`private bool ShouldUseCachedToken(MsalAccessTokenCacheItem cacheItem)`
`223`	`232`	`{`
`224`	`233`	`// 1) No cached item`
`@@ -239,18 +248,32 @@ private bool ShouldUseCachedToken(MsalAccessTokenCacheItem cacheItem)`
`239`	`248`	`return true;`
`240`	`249`	`}`
`241`	`250`
	`251`	`+ /// <summary>`
	`252`	`+ /// Checks if the token hash matches the hash provided in AccessTokenHashToRefresh.`
	`253`	`+ /// </summary>`
	`254`	`+ /// <param name="tokenSecret"></param>`
	`255`	`+ /// <param name="accessTokenHashToRefresh"></param>`
	`256`	`+ /// <returns></returns>`
`242`	`257`	`private bool IsMatchingTokenHash(string tokenSecret, string accessTokenHashToRefresh)`
`243`	`258`	`{`
`244`	`259`	`string cachedTokenHash = _cryptoManager.CreateSha256Hash(tokenSecret);`
`245`	`260`	`return string.Equals(cachedTokenHash, accessTokenHashToRefresh, StringComparison.Ordinal);`
`246`	`261`	`}`
`247`	`262`
	`263`	`+ /// <summary>`
	`264`	`+ /// Marks the request as a cache hit and increments the cache hit count.`
	`265`	`+ /// </summary>`
`248`	`266`	`private void MarkAccessTokenAsCacheHit()`
`249`	`267`	`{`
`250`	`268`	`AuthenticationRequestParameters.RequestContext.ApiEvent.IsAccessTokenCacheHit = true;`
`251`	`269`	`Metrics.IncrementTotalAccessTokensFromCache();`
`252`	`270`	`}`
`253`	`271`
	`272`	`+ /// <summary>`
	`273`	`+ /// returns the cached access token item`
	`274`	`+ /// </summary>`
	`275`	`+ /// <param name="cachedAccessTokenItem"></param>`
	`276`	`+ /// <returns></returns>`
`254`	`277`	`private AuthenticationResult CreateAuthenticationResultFromCache(MsalAccessTokenCacheItem cachedAccessTokenItem)`
`255`	`278`	`{`
`256`	`279`	`AuthenticationResult authResult = new AuthenticationResult(`
`@@ -266,6 +289,11 @@ private AuthenticationResult CreateAuthenticationResultFromCache(MsalAccessToken`
`266`	`289`	`return authResult;`
`267`	`290`	`}`
`268`	`291`
	`292`	`+ /// <summary>`
	`293`	`+ /// Gets overriden scopes for client credentials flow`
	`294`	`+ /// </summary>`
	`295`	`+ /// <param name="inputScopes"></param>`
	`296`	`+ /// <returns></returns>`
`269`	`297`	`protected override SortedSet<string> GetOverriddenScopes(ISet<string> inputScopes)`
`270`	`298`	`{`
`271`	`299`	`// Client credentials should not add the reserved scopes`
`@@ -274,6 +302,10 @@ protected override SortedSet<string> GetOverriddenScopes(ISet<string> inputScope`
`274`	`302`	`return new SortedSet<string>(inputScopes);`
`275`	`303`	`}`
`276`	`304`
	`305`	`+ /// <summary>`
	`306`	`+ /// Gets the body parameters for the client credentials flow`
	`307`	`+ /// </summary>`
	`308`	`+ /// <returns></returns>`
`277`	`309`	`private Dictionary<string, string> GetBodyParameters()`
`278`	`310`	`{`
`279`	`311`	`var dict = new Dictionary<string, string>`
`@@ -285,6 +317,11 @@ private Dictionary<string, string> GetBodyParameters()`
`285`	`317`	`return dict;`
`286`	`318`	`}`
`287`	`319`
	`320`	`+ /// <summary>`
	`321`	`+ /// Gets the CCS header for the client credentials flow`
	`322`	`+ /// </summary>`
	`323`	`+ /// <param name="additionalBodyParameters"></param>`
	`324`	`+ /// <returns></returns>`
`288`	`325`	`protected override KeyValuePair<string, string>? GetCcsHeader(IDictionary<string, string> additionalBodyParameters)`
`289`	`326`	`{`
`290`	`327`	`return null;`