Fix ConfidentialClient's AcquireTokenSilent and AcquireTokenOnBehalfOf claims logic (#43513)

christothes · web-flow · commit ae13ec206b7e · 2024-04-19T14:11:04.000-05:00
diff --git a/sdk/identity/Azure.Identity/CHANGELOG.md b/sdk/identity/Azure.Identity/CHANGELOG.md
@@ -1,9 +1,9 @@
 # Release History
 
-## 1.12.0-beta.1 (2024-04-17)
+## 1.11.2 (2024-04-19)
 
-### Other Changes
-- An experimental overload `Authenticate` method on `InteractiveBrowserCredential` now supports the experimental `PopTokenRequestContext` parameter.
+### Bugs Fixed
+- Fixed an issue which caused claims to be incorrectly added to confidential client credentials such as `DeviceCodeCredential` [#43468](https://github.com/Azure/azure-sdk-for-net/issues/43468)
 
 ## 1.11.1 (2024-04-16)
 
diff --git a/sdk/identity/Azure.Identity/api/Azure.Identity.netstandard2.0.cs b/sdk/identity/Azure.Identity/api/Azure.Identity.netstandard2.0.cs
@@ -261,23 +261,19 @@ public static partial class IdentityModelFactory
         public static Azure.Identity.AuthenticationRecord AuthenticationRecord(string username, string authority, string homeAccountId, string tenantId, string clientId) { throw null; }
         public static Azure.Identity.DeviceCodeInfo DeviceCodeInfo(string userCode, string deviceCode, System.Uri verificationUri, System.DateTimeOffset expiresOn, string message, string clientId, System.Collections.Generic.IReadOnlyCollection<string> scopes) { throw null; }
     }
-    public partial class InteractiveBrowserCredential : Azure.Core.TokenCredential, Azure.Core.ISupportsProofOfPossession
+    public partial class InteractiveBrowserCredential : Azure.Core.TokenCredential
     {
         public InteractiveBrowserCredential() { }
         public InteractiveBrowserCredential(Azure.Identity.InteractiveBrowserCredentialOptions options) { }
         [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
         public InteractiveBrowserCredential(string clientId) { }
         [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
         public InteractiveBrowserCredential(string tenantId, string clientId, Azure.Identity.TokenCredentialOptions options = null) { }
-        public virtual Azure.Identity.AuthenticationRecord Authenticate(Azure.Core.PopTokenRequestContext requestContext, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
         public virtual Azure.Identity.AuthenticationRecord Authenticate(Azure.Core.TokenRequestContext requestContext, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
         public virtual Azure.Identity.AuthenticationRecord Authenticate(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
-        public virtual System.Threading.Tasks.Task<Azure.Identity.AuthenticationRecord> AuthenticateAsync(Azure.Core.PopTokenRequestContext requestContext, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
         public virtual System.Threading.Tasks.Task<Azure.Identity.AuthenticationRecord> AuthenticateAsync(Azure.Core.TokenRequestContext requestContext, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
         public virtual System.Threading.Tasks.Task<Azure.Identity.AuthenticationRecord> AuthenticateAsync(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
-        public Azure.Core.AccessToken GetToken(Azure.Core.PopTokenRequestContext requestContext, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
         public override Azure.Core.AccessToken GetToken(Azure.Core.TokenRequestContext requestContext, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
-        public System.Threading.Tasks.ValueTask<Azure.Core.AccessToken> GetTokenAsync(Azure.Core.PopTokenRequestContext requestContext, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
         public override System.Threading.Tasks.ValueTask<Azure.Core.AccessToken> GetTokenAsync(Azure.Core.TokenRequestContext requestContext, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
     }
     public partial class InteractiveBrowserCredentialOptions : Azure.Identity.TokenCredentialOptions
diff --git a/sdk/identity/Azure.Identity/src/Azure.Identity.csproj b/sdk/identity/Azure.Identity/src/Azure.Identity.csproj
@@ -2,7 +2,7 @@
   <PropertyGroup>
     <Description>This is the implementation of the Azure SDK Client Library for Azure Identity</Description>
     <AssemblyTitle>Microsoft Azure.Identity Component</AssemblyTitle>
-    <Version>1.12.0-beta.1</Version>
+    <Version>1.11.2</Version>
     <!--The ApiCompatVersion is managed automatically and should not generally be modified manually.-->
     <ApiCompatVersion>1.11.1</ApiCompatVersion>
     <PackageTags>Microsoft Azure Identity;$(PackageCommonTags)</PackageTags>
diff --git a/sdk/identity/Azure.Identity/src/Credentials/AuthorizationCodeCredential.cs b/sdk/identity/Azure.Identity/src/Credentials/AuthorizationCodeCredential.cs
@@ -181,7 +181,15 @@ private async ValueTask<AccessToken> GetTokenImplAsync(bool async, TokenRequestC
         private async Task<AccessToken> AcquireTokenWithCode(bool async, TokenRequestContext requestContext, AccessToken token, string tenantId, CancellationToken cancellationToken)
         {
             AuthenticationResult result = await Client
-                                    .AcquireTokenByAuthorizationCodeAsync(requestContext.Scopes, _authCode, tenantId, _redirectUri, requestContext.Claims, requestContext.IsCaeEnabled, async, cancellationToken)
+                                    .AcquireTokenByAuthorizationCodeAsync(
+                                        scopes: requestContext.Scopes,
+                                        code: _authCode,
+                                        tenantId: tenantId,
+                                        redirectUri: _redirectUri,
+                                        claims: requestContext.Claims,
+                                        enableCae: requestContext.IsCaeEnabled,
+                                        async,
+                                        cancellationToken)
                                     .ConfigureAwait(false);
             _record = new AuthenticationRecord(result, _clientId);
             token = new AccessToken(result.AccessToken, result.ExpiresOn);
diff --git a/sdk/identity/Azure.Identity/src/MsalConfidentialClient.cs b/sdk/identity/Azure.Identity/src/MsalConfidentialClient.cs
@@ -220,7 +220,7 @@ public virtual async ValueTask<AuthenticationResult> AcquireTokenSilentCoreAsync
                 };
                 builder.WithTenantIdFromAuthority(uriBuilder.Uri);
             }
-            if (string.IsNullOrEmpty(claims))
+            if (!string.IsNullOrEmpty(claims))
             {
                 builder.WithClaims(claims);
             }
@@ -239,7 +239,7 @@ public virtual async ValueTask<AuthenticationResult> AcquireTokenByAuthorization
             bool async,
             CancellationToken cancellationToken)
         {
-            var result = await AcquireTokenByAuthorizationCodeCoreAsync(scopes, code, tenantId, redirectUri, claims, enableCae, async, cancellationToken).ConfigureAwait(false);
+            var result = await AcquireTokenByAuthorizationCodeCoreAsync(scopes: scopes, code: code, tenantId: tenantId, redirectUri: redirectUri, claims: claims, enableCae: enableCae, async, cancellationToken).ConfigureAwait(false);
             LogAccountDetails(result);
             return result;
         }
@@ -248,8 +248,8 @@ public virtual async ValueTask<AuthenticationResult> AcquireTokenByAuthorization
             string[] scopes,
             string code,
             string tenantId,
-            string claims,
             string redirectUri,
+            string claims,
             bool enableCae,
             bool async,
             CancellationToken cancellationToken)
@@ -312,6 +312,10 @@ public virtual async ValueTask<AuthenticationResult> AcquireTokenOnBehalfOfCoreA
                 };
                 builder.WithTenantIdFromAuthority(uriBuilder.Uri);
             }
+            if (!string.IsNullOrEmpty(claims))
+            {
+                builder.WithClaims(claims);
+            }
             return await builder
                 .ExecuteAsync(async, cancellationToken)
                 .ConfigureAwait(false);
diff --git a/sdk/identity/Azure.Identity/tests/AuthorizationCodeCredentialTests.cs b/sdk/identity/Azure.Identity/tests/AuthorizationCodeCredentialTests.cs
@@ -33,6 +33,7 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
                 DisableInstanceDiscovery = config.DisableInstanceDiscovery,
                 AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,
                 IsUnsafeSupportLoggingEnabled = config.IsUnsafeSupportLoggingEnabled,
+                RedirectUri = config.RedirectUri,
             };
             if (config.Transport != null)
             {
diff --git a/sdk/identity/Azure.Identity/tests/Azure.Identity.Tests.csproj b/sdk/identity/Azure.Identity/tests/Azure.Identity.Tests.csproj
@@ -16,9 +16,9 @@
     <PackageReference Include="Azure.Storage.Blobs" />
   </ItemGroup>
 <!-- Remove before shipping GA -->
-  <PropertyGroup>
+  <!-- <PropertyGroup>
         <DefineConstants>PREVIEW_FEATURE_FLAG</DefineConstants>
-  </PropertyGroup>
+  </PropertyGroup> -->
 <!-- End remove before shipping GA -->
   <ItemGroup>
     <ProjectReference Include="$(AzureCoreTestFramework)" />
diff --git a/sdk/identity/Azure.Identity/tests/CredentialTestBase.cs b/sdk/identity/Azure.Identity/tests/CredentialTestBase.cs
@@ -355,6 +355,92 @@ public async Task EnableCae()
             Assert.True(observedNoCae);
         }
 
+        [Test]
+        public async Task ClaimsSetCorrectlyOnRequest()
+        {
+            // Configure the transport
+            var token = Guid.NewGuid().ToString();
+            var idToken = CredentialTestHelpers.CreateMsalIdToken(Guid.NewGuid().ToString(), "userName", TenantId);
+            bool calledDiscoveryEndpoint = false;
+            bool isPubClient = false;
+            const string Claims = "myClaims";
+
+            var mockTransport = new MockTransport(req =>
+            {
+                calledDiscoveryEndpoint |= req.Uri.Path.Contains("discovery/instance");
+
+                MockResponse response = new(200);
+                if (req.Uri.Path.EndsWith("/devicecode"))
+                {
+                    response = CredentialTestHelpers.CreateMockMsalDeviceCodeResponse();
+                }
+                else if (req.Uri.Path.Contains("/userrealm/"))
+                {
+                    response.SetContent(UserrealmResponse);
+                }
+                else
+                {
+                    if (isPubClient || typeof(TCredOptions) == typeof(AuthorizationCodeCredentialOptions) || typeof(TCredOptions) == typeof(OnBehalfOfCredentialOptions))
+                    {
+                        response = CredentialTestHelpers.CreateMockMsalTokenResponse(200, token, TenantId, ExpectedUsername, ObjectId);
+                    }
+                    else
+                    {
+                        response.SetContent($"{{\"token_type\": \"Bearer\",\"expires_in\": 9999,\"ext_expires_in\": 9999,\"access_token\": \"{token}\" }}");
+                    }
+                    if (req.Content != null)
+                    {
+                        var stream = new MemoryStream();
+                        req.Content.WriteTo(stream, default);
+                        var content = new BinaryData(stream.ToArray()).ToString();
+                        var queryString = Uri.UnescapeDataString(content)
+                            .Split('&')
+                            .Select(q => q.Split('='))
+                            .ToDictionary(kvp => kvp[0], kvp => kvp[1]);
+                        bool containsClaims = queryString.TryGetValue("claims", out var claimsJson);
+
+                        if (req.ClientRequestId == "NoClaims")
+                        {
+                            Assert.False(containsClaims, "(NoClaims) Claims should not be present. Claims=" + claimsJson);
+                        }
+                        if (req.ClientRequestId == "WithClaims")
+                        {
+                            Assert.True(containsClaims, "(WithClaims) Claims should be present");
+                            Assert.AreEqual(Claims, claimsJson, "(WithClaims) Claims should match");
+                        }
+                    }
+                }
+
+                return response;
+            });
+
+            var config = new CommonCredentialTestConfig()
+            {
+                Transport = mockTransport,
+                TenantId = TenantId,
+                RedirectUri = new Uri("http://localhost:8400/")
+            };
+            var credential = GetTokenCredential(config);
+            if (!CredentialTestHelpers.IsMsalCredential(credential))
+            {
+                Assert.Ignore("EnableCAE tests do not apply to the non-MSAL credentials.");
+            }
+            isPubClient = CredentialTestHelpers.IsCredentialTypePubClient(credential);
+
+            using (HttpPipeline.CreateClientRequestIdScope("NoClaims"))
+            {
+                // First call to populate the account record for confidential client creds
+                await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default), default);
+                var actualToken = await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Alternate), default);
+                Assert.AreEqual(token, actualToken.Token);
+            }
+            using (HttpPipeline.CreateClientRequestIdScope("WithClaims"))
+            {
+                var actualToken = await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Alternate2, claims: Claims), default);
+                Assert.AreEqual(token, actualToken.Token);
+            }
+        }
+
         [Test]
         public async Task TokenRequestContextClaimsPassedToMSAL()
         {
@@ -624,6 +710,7 @@ public class CommonCredentialTestConfig : TokenCredentialOptions, ISupportsAddit
             public TokenRequestContext RequestContext { get; set; }
             public string TenantId { get; set; }
             public IList<string> AdditionallyAllowedTenants { get; set; } = new List<string>();
+            public Uri RedirectUri { get; set; }
             internal TenantIdResolverBase TestTentantIdResolver { get; set; }
             internal MockMsalConfidentialClient MockConfidentialMsalClient { get; set; }
             internal MockMsalPublicClient MockPublicMsalClient { get; set; }
diff --git a/sdk/identity/Azure.Identity/tests/Mock/MockScopes.cs b/sdk/identity/Azure.Identity/tests/Mock/MockScopes.cs
@@ -19,6 +19,7 @@ private MockScopes(string[] scopes)
         public static MockScopes Default = new MockScopes(new string[] { "https://default.mock.auth.scope/.default" });
 
         public static MockScopes Alternate = new MockScopes(new string[] { "https://alternate.mock.auth.scope/.default" });
+        public static MockScopes Alternate2 = new MockScopes(new string[] { "https://alternate2.mock.auth.scope/.default" });
 
         public override string ToString()
         {

Original file line number	Diff line number	Diff line change
`@@ -33,6 +33,7 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co`
`33`	`33`	`DisableInstanceDiscovery = config.DisableInstanceDiscovery,`
`34`	`34`	`AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,`
`35`	`35`	`IsUnsafeSupportLoggingEnabled = config.IsUnsafeSupportLoggingEnabled,`
	`36`	`+ RedirectUri = config.RedirectUri,`
`36`	`37`	`};`
`37`	`38`	`if (config.Transport != null)`
`38`	`39`	`{`
Original file line number	Diff line number	Diff line change
`@@ -19,6 +19,7 @@ private MockScopes(string[] scopes)`
`19`	`19`	`public static MockScopes Default = new MockScopes(new string[] { "https://default.mock.auth.scope/.default" });`
`20`	`20`
`21`	`21`	`public static MockScopes Alternate = new MockScopes(new string[] { "https://alternate.mock.auth.scope/.default" });`
	`22`	`+ public static MockScopes Alternate2 = new MockScopes(new string[] { "https://alternate2.mock.auth.scope/.default" });`
`22`	`23`
`23`	`24`	`public override string ToString()`
`24`	`25`	`{`