Fix #1422 - handle protected scopes better (#1424)

* Fix #1422 - handle protected scopes better

* Address PR comments

src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs

@@ -5,12 +5,11 @@
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Identity.Client.ApiConfig.Parameters;
+using Microsoft.Identity.Client.Cache.Items;
 using Microsoft.Identity.Client.Core;
-using Microsoft.Identity.Client.TelemetryCore.Internal.Events;
 using Microsoft.Identity.Client.OAuth2;
+using Microsoft.Identity.Client.TelemetryCore.Internal.Events;
 using Microsoft.Identity.Client.Utils;
-using System;
-using Microsoft.Identity.Client.Cache.Items;
 
 namespace Microsoft.Identity.Client.Internal.Requests
 {
@@ -39,7 +38,7 @@ internal override async Task<AuthenticationResult> ExecuteAsync(CancellationToke
                 if (cachedAccessTokenItem != null && !cachedAccessTokenItem.NeedsRefresh())
                 {
                     return new AuthenticationResult(
-                        cachedAccessTokenItem, 
+                        cachedAccessTokenItem,
                         null,
                         AuthenticationRequestParameters.AuthenticationScheme,
                         AuthenticationRequestParameters.RequestContext.CorrelationId);
@@ -60,9 +59,9 @@ internal override async Task<AuthenticationResult> ExecuteAsync(CancellationToke
                 {
                     logger.Info("Returning existing access token. It is not expired, but should be refreshed.");
                     return new AuthenticationResult(
-                        cachedAccessTokenItem, 
-                        null, 
-                        AuthenticationRequestParameters.AuthenticationScheme, 
+                        cachedAccessTokenItem,
+                        null,
+                        AuthenticationRequestParameters.AuthenticationScheme,
                         AuthenticationRequestParameters.RequestContext.CorrelationId);
                 }
 
@@ -83,6 +82,14 @@ protected override void EnrichTelemetryApiEvent(ApiEvent apiEvent)
             apiEvent.IsConfidentialClient = true;
         }
 
+        protected override SortedSet<string> GetDecoratedScope(SortedSet<string> inputScope)
+        {
+            // Client credentials should not add the reserved scopes
+            // "openid", "profile" and "offline_access" 
+            // because AT is on behalf of an app (no profile, no IDToken, no RT)
+            return new SortedSet<string>(inputScope);
+        }
+
         private Dictionary<string, string> GetBodyParameters()
         {
             var dict = new Dictionary<string, string>

src/client/Microsoft.Identity.Client/Internal/Requests/RequestBase.cs

@@ -87,24 +87,21 @@ private void LogRequestStarted(AuthenticationRequestParameters authenticationReq
             authenticationRequestParameters.RequestContext.Logger.InfoPii(messageWithPii, messageWithoutPii);
         }
 
-        protected SortedSet<string> GetDecoratedScope(SortedSet<string> inputScope)
+        protected virtual SortedSet<string> GetDecoratedScope(SortedSet<string> inputScope)
         {
-            SortedSet<string> set = new SortedSet<string>(inputScope.ToArray());
-            set.UnionWith(ScopeHelper.CreateSortedSetFromEnumerable(OAuth2Value.ReservedScopes));
+            // OAuth spec states that scopes are case sensitive, but 
+            // merge the reserved scopes in a case insensitive way, to 
+            // avoid sending things like "openid OpenId" (note that EVO is tollerant of this)
+            SortedSet<string> set = new SortedSet<string>(
+                inputScope.ToArray(),
+                StringComparer.OrdinalIgnoreCase);
+
+            set.UnionWith(OAuth2Value.ReservedScopes);
             return set;
         }
 
         protected void ValidateScopeInput(SortedSet<string> scopesToValidate)
         {
-            // Check if scope or additional scope contains client ID.
-            // TODO: instead of failing in the validation, could we simply just remove what the user sets and log that we did so instead?
-            if (scopesToValidate.Intersect(ScopeHelper.CreateSortedSetFromEnumerable(OAuth2Value.ReservedScopes)).Any())
-            {
-                throw new ArgumentException("MSAL always sends the scopes 'openid profile offline_access'. " +
-                                            "They cannot be suppressed as they are required for the " +
-                                            "library to function. Do not include any of these scopes in the scope parameter.");
-            }
-
             if (scopesToValidate.Contains(AuthenticationRequestParameters.ClientId))
             {
                 throw new ArgumentException("API does not accept client id as a user-provided scope");
@@ -279,7 +276,6 @@ internal async Task ResolveAuthorityEndpointsAsync()
                 AuthenticationRequestParameters.RequestContext).ConfigureAwait(false);
         }
 
-
         protected Task<MsalTokenResponse> SendTokenRequestAsync(
             IDictionary<string, string> additionalBodyParameters,
             CancellationToken cancellationToken)
@@ -290,7 +286,6 @@ protected Task<MsalTokenResponse> SendTokenRequestAsync(
                 cancellationToken);
         }
 
-
         protected async Task<MsalTokenResponse> SendTokenRequestAsync(
             string tokenEndpoint,
             IDictionary<string, string> additionalBodyParameters,

src/client/Microsoft.Identity.Client/OAuth2/OAuthConstants.cs

@@ -1,6 +1,8 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+using System.Collections.Generic;
+
 namespace Microsoft.Identity.Client.OAuth2
 {
     internal static class OAuth2Parameter
@@ -88,7 +90,8 @@ internal static class OAuth2Error
 
     internal static class OAuth2Value
     {
-        public static readonly string[] ReservedScopes = { ScopeOpenId, ScopeProfile, ScopeOfflineAccess };
+        public static readonly SortedSet<string> ReservedScopes = 
+            new SortedSet<string> { ScopeOpenId, ScopeProfile, ScopeOfflineAccess };
         public const string CodeChallengeMethodValue = "S256";
         public const string ScopeOpenId = "openid";
         public const string ScopeOfflineAccess = "offline_access";

tests/Microsoft.Identity.Test.Common/Core/Mocks/MockHttpManagerExtensions.cs

@@ -153,14 +153,17 @@ public static void AddMockHandlerContentNotFound(this MockHttpManager httpManage
                 });
         }
 
-        public static void AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(this MockHttpManager httpManager)
+        public static MockHttpMessageHandler AddMockHandlerSuccessfulClientCredentialTokenResponseMessage(this MockHttpManager httpManager)
         {
-            httpManager.AddMockHandler(
-                new MockHttpMessageHandler()
-                {
-                    ExpectedMethod = HttpMethod.Post,
-                    ResponseMessage = MockHelpers.CreateSuccessfulClientCredentialTokenResponseMessage()
-                });
+            var handler = new MockHttpMessageHandler()
+            {
+                ExpectedMethod = HttpMethod.Post,
+                ResponseMessage = MockHelpers.CreateSuccessfulClientCredentialTokenResponseMessage()
+            };
+
+            httpManager.AddMockHandler(handler);
+
+            return handler;
         }
 
         public static void AddFailingRequest(this MockHttpManager httpManager, Exception exceptionToThrow)

tests/Microsoft.Identity.Test.Common/Core/Mocks/MockHttpMessageHandler.cs

@@ -22,7 +22,6 @@ internal class MockHttpMessageHandler : HttpMessageHandler
         public string ExpectedUrl { get; set; }
         public IDictionary<string, string> ExpectedQueryParams { get; set; }
         public IDictionary<string, string> ExpectedPostData { get; set; }
-        public IDictionary<string, object> ExpectedPostDataObject { get; set; }
         public IDictionary<string, string> HttpTelemetryHeaders { get; set; }
         public HttpMethod ExpectedMethod { get; set; }
         public Exception ExceptionToThrow { get; set; }

tests/Microsoft.Identity.Test.Unit.net45/PublicApiTests/ConfidentialClientApplicationTests.cs

@@ -253,6 +253,34 @@ public async Task ClientCreds_MustFilterByTenantId_Async()
             }
         }
 
+        [TestMethod]
+        [WorkItem(1403)] // https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/1403
+        public async Task FooAsync()
+        {
+            using (var httpManager = new MockHttpManager())
+            {
+                httpManager.AddInstanceDiscoveryMockHandler();
+
+                var app = ConfidentialClientApplicationBuilder.Create(TestConstants.ClientId)
+                                                              .WithClientSecret(TestConstants.ClientSecret)
+                                                              .WithHttpManager(httpManager)
+                                                              .BuildConcrete();
+
+                httpManager.AddMockHandlerForTenantEndpointDiscovery(TestConstants.AuthorityUtidTenant);
+                var handler = httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage();
+                handler.ExpectedPostData = new Dictionary<string, string>()
+                {
+                    // Bug 1403: Do not add reserved scopes profile, offline_access and openid to Confidential Client request
+                    { "scope", TestConstants.s_scope.AsSingleString() } 
+                };
+
+                var result = await app.AcquireTokenForClient(TestConstants.s_scope.ToArray())
+                    .WithAuthority(TestConstants.AuthorityUtidTenant)
+                    .ExecuteAsync(CancellationToken.None)
+                    .ConfigureAwait(false);
+            }
+        }
+
         [TestMethod]
         public async Task ConfidentialClientUsingSecretTestAsync()
         {

tests/Microsoft.Identity.Test.Unit.net45/RequestsTests/IntegratedWindowsAuthUsernamePasswordTests.cs

@@ -641,11 +641,10 @@ public async Task ManagedUsernameSecureStringPasswordAcquireTokenTestAsync()
                     {
                         ExpectedMethod = HttpMethod.Post,
                         ResponseMessage = MockHelpers.CreateSuccessTokenResponseMessage(),
-                        ExpectedPostDataObject = new Dictionary<string, object>
+                        ExpectedPostData = new Dictionary<string, string>
                         {
                             {"grant_type", "password"},
                             {"username", TestConstants.s_user.Username},
-                            {"password", _secureString}
                         }
                     });
 
@@ -715,11 +714,11 @@ public async Task ManagedUsernameIncorrectPasswordAcquireTokenTestAsync()
                     {
                         ExpectedMethod = HttpMethod.Post,
                         ResponseMessage = MockHelpers.CreateInvalidGrantTokenResponseMessage(),
-                        ExpectedPostDataObject = new Dictionary<string, object>
+                        ExpectedPostData = new Dictionary<string, string>
                         {
                             {"grant_type", "password"},
                             {"username", TestConstants.s_user.Username},
-                            {"password", _secureString}
+                            {"password", "y"}
                         }
                     });
 

tests/devapps/NetCoreTestApp/Program.cs

@@ -23,7 +23,8 @@ public class Program
         private static readonly string s_clientIdForPublicApp = "1d18b3b0-251b-4714-a02a-9956cec86c2d";
 
         private static readonly string s_username = ""; // used for WIA and U/P, cannot be empty on .net core
-        private static readonly IEnumerable<string> s_scopes = new[] { "user.read" }; // used for WIA and U/P, can be empty
+        private static readonly IEnumerable<string> s_scopes = new[] {
+            "user.read", "openid" }; // used for WIA and U/P, can be empty
 
         private const string GraphAPIEndpoint = "https://graph.microsoft.com/v1.0/me";
 

tests/devapps/NetFxConsoleTestApp/Program.cs

@@ -23,15 +23,21 @@ public class Program
     {
         // This app has http://localhost redirect uri registered
         private static readonly string s_clientIdForPublicApp = "1d18b3b0-251b-4714-a02a-9956cec86c2d";
-        //private static readonly string s_clientIdForPublicApp = "655015be-5021-4afc-a683-a4223eb5d0e5";
+        private static readonly string s_clientIdForConfidentialApp =
+            Environment.GetEnvironmentVariable("LAB_APP_CLIENT_ID") ??
+            throw new ArgumentException("Please configure a client id");
+
+        private static readonly string s_confidentialClientSecret =
+            Environment.GetEnvironmentVariable("LAB_APP_CLIENT_SECRET") ??
+            throw new ArgumentException("Please configure a client secret");
 
         private static readonly string s_username = ""; // used for WIA and U/P, cannot be empty on .net core
-        private static readonly IEnumerable<string> s_scopes = new[] { "user.read" }; // used for WIA and U/P, can be empty
-        //private static readonly IEnumerable<string> s_scopes = new[] { "https://sshservice.azure.net/.default" }; // used for WIA and U/P, can be empty
+        private static readonly IEnumerable<string> s_scopes = new[] { "user.read", "Openid", "profile" }; // used for WIA and U/P, can be empty
 
         private const string GraphAPIEndpoint = "https://graph.microsoft.com/v1.0/me";
 
-        public static readonly string CacheFilePath = System.Reflection.Assembly.GetExecutingAssembly().Location + ".msalcache.json";
+        public static readonly string UserCacheFile = System.Reflection.Assembly.GetExecutingAssembly().Location + ".msalcache.user.json";
+        public static readonly string AppCacheFilePath = System.Reflection.Assembly.GetExecutingAssembly().Location + ".msalcache.app.json";
 
 
         private static readonly string[] s_tids = new[]  {
@@ -46,7 +52,8 @@ public static void Main(string[] args)
             Console.ResetColor();
             Console.BackgroundColor = ConsoleColor.Black;
             var pca = CreatePca();
-            RunConsoleAppLogicAsync(pca).Wait();
+            var cca = CreateCca();
+            RunConsoleAppLogicAsync(pca, cca).Wait();
         }
 
         private static string GetAuthority()
@@ -55,6 +62,18 @@ private static string GetAuthority()
             return $"https://login.microsoftonline.com/{tenant}";
         }
 
+        private static IConfidentialClientApplication CreateCca()
+        {
+            IConfidentialClientApplication cca = ConfidentialClientApplicationBuilder
+                .Create(s_clientIdForConfidentialApp)
+                .WithClientSecret(s_confidentialClientSecret)
+                .Build();
+
+            BindCache(cca.UserTokenCache, UserCacheFile);
+            //BindCache(cca.AppTokenCache, AppCacheFilePath);
+
+            return cca;
+        }
         private static IPublicClientApplication CreatePca()
         {
             IPublicClientApplication pca = PublicClientApplicationBuilder
@@ -64,18 +83,25 @@ private static IPublicClientApplication CreatePca()
                             .WithRedirectUri("http://localhost") // required for DefaultOsBrowser
                             .Build();
 
-            pca.UserTokenCache.SetBeforeAccess(notificationArgs =>
+            BindCache(pca.UserTokenCache, UserCacheFile);
+            return pca;
+        }
+
+        private static void BindCache(ITokenCache tokenCache, string file)
+        {
+            tokenCache.SetBeforeAccess(notificationArgs =>
             {
                 //Console.BackgroundColor = ConsoleColor.DarkYellow;
                 Console.WriteLine($"SetBeforeAccess invoked for {notificationArgs?.Account?.Username ?? "null"} ");
                 Debug.WriteLine($"SetBeforeAccess invoked for {notificationArgs?.Account?.Username ?? "null"} ");
                 Console.ResetColor();
 
-                notificationArgs.TokenCache.DeserializeMsalV3(File.Exists(CacheFilePath)
-                    ? File.ReadAllBytes(CacheFilePath)
+                notificationArgs.TokenCache.DeserializeMsalV3(File.Exists(file)
+                    ? File.ReadAllBytes(UserCacheFile)
                     : null);
             });
-            pca.UserTokenCache.SetAfterAccess(notificationArgs =>
+
+            tokenCache.SetAfterAccess(notificationArgs =>
             {
                 //Console.BackgroundColor = ConsoleColor.DarkYellow;
                 Console.WriteLine($"SetAfterAccess invoked for {notificationArgs?.Account?.Username ?? "null" } " +
@@ -88,13 +114,14 @@ private static IPublicClientApplication CreatePca()
                 if (notificationArgs.HasStateChanged)
                 {
                     // reflect changes in the persistent store
-                    File.WriteAllBytes(CacheFilePath, notificationArgs.TokenCache.SerializeMsalV3());
+                    File.WriteAllBytes(file, notificationArgs.TokenCache.SerializeMsalV3());
                 }
             });
-            return pca;
         }
 
-        private static async Task RunConsoleAppLogicAsync(IPublicClientApplication pca)
+        private static async Task RunConsoleAppLogicAsync(
+            IPublicClientApplication pca, 
+            IConfidentialClientApplication cca)
         {
             while (true)
             {
@@ -113,6 +140,7 @@ 5. Acquire Token Interactive via NetStandard lib
                         6. Acquire Token Silently
                         7. Acquire Token Silently - multiple requests in parallel
                         8. Acquire SSH Cert Interactive
+                        9. Client Credentials 
                         c. Clear cache
                         r. Rotate Tenant ID
                         e. Expire all ATs
@@ -148,7 +176,7 @@ x. Exit app
                             await FetchTokenAndCallGraphAsync(pca, authTask).ConfigureAwait(false);
 
                             break;
-                        case '4': 
+                        case '4':
 
                             authTask = pca.AcquireTokenInteractive(s_scopes)
                                 .WithPrompt(Prompt.Consent)
@@ -187,7 +215,7 @@ x. Exit app
                             break;
                         case '5': // Acquire Token Interactive via NetStandard lib
                             CancellationTokenSource cts2 = new CancellationTokenSource();
-                            var authenticator = new NetStandardAuthenticator(Log, CacheFilePath);
+                            var authenticator = new NetStandardAuthenticator(Log, UserCacheFile);
                             await FetchTokenAndCallGraphAsync(pca, authenticator.GetTokenInteractiveAsync(cts2.Token)).ConfigureAwait(false);
                             break;
                         case '8': // acquire SSH cert
@@ -217,6 +245,15 @@ x. Exit app
                             await FetchTokenAndCallGraphAsync(pca, authTask).ConfigureAwait(false);
 
                             break;
+                        case '9':
+
+                            authTask = cca.AcquireTokenForClient(
+                                new[] { "https://graph.microsoft.com/.default" }).
+                                ExecuteAsync();
+
+                            await FetchTokenAndCallGraphAsync(pca, authTask).ConfigureAwait(false);
+                            break;
+
                         case 'c':
                             var accounts2 = await pca.GetAccountsAsync().ConfigureAwait(false);
                             foreach (var acc in accounts2)
@@ -229,7 +266,8 @@ x. Exit app
 
                             s_currentTid = (s_currentTid + 1) % s_tids.Length;
                             pca = CreatePca();
-                            RunConsoleAppLogicAsync(pca).Wait();
+                            cca = CreateCca();
+                            RunConsoleAppLogicAsync(pca, cca).Wait();
                             break;
 
                         case 'e': // expire all ATs