Fixes #253: Check/decide the behavior of VerifyUserHasAnyAcceptedScop…

…e() (#259) * Fixes #253: - Checks that the HttpContext is not null, otherwise throws an ArgumentNullException - If there is no authenticated user, the HTTP response status code is set to 401 - If there is an authenticated user, the HTTP response code is set to 403 and a message tells which scopes to acquire Updated the tests with the new behavior. Updated the sample to surface a nice execption to the developer. * Update src/Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs Co-authored-by: Marsh Macy <[email protected]> * removing an un-needed space * Update src/Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs Co-authored-by: jennyf19 <[email protected]> Co-authored-by: Marsh Macy <[email protected]> Co-authored-by: jennyf19 <[email protected]>
AzureAD · Jun 26, 2020 · 99d5542 · 99d5542
1 parent 35d83f6
commit 99d5542
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 38 deletions.
diff --git a/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml b/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml
diff --git a/src/Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs b/src/Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs
@@ -19,34 +19,47 @@ public static class ScopesRequiredHttpContextExtensions
         /// <summary>
         /// When applied to an <see cref="HttpContext"/>, verifies that the user authenticated in the
         /// web API has any of the accepted scopes.
+        /// If there is no authenticated user, the reponse is a 401 (Unauthenticated).
         /// If the authenticated user does not have any of these <paramref name="acceptedScopes"/>, the
-        /// method throws an HTTP Unauthorized with the message telling which scopes are expected in the token.
+        /// method updates the HTTP response providing a status code 403 (Forbidden)
+        /// and writes to the response body a message telling which scopes are expected in the token.
         /// </summary>
         /// <param name="context">HttpContext (from the controller).</param>
         /// <param name="acceptedScopes">Scopes accepted by this web API.</param>
-        /// <exception cref="HttpRequestException"> with a <see cref="HttpResponse.StatusCode"/> set to
-        /// <see cref="HttpStatusCode.Unauthorized"/>.
-        /// </exception>
+        /// <remarks>When the scopes don't match, the response is a 403 (Forbidden),
+        /// because the user is authenticated (hence not 401), but not authorized.</remarks>
         public static void VerifyUserHasAnyAcceptedScope(this HttpContext context, params string[] acceptedScopes)
         {
             if (acceptedScopes == null)
             {
                 throw new ArgumentNullException(nameof(acceptedScopes));
             }
 
-            Claim scopeClaim = context?.User?.FindFirst(ClaimConstants.Scope);
-
-            // Fallback to scp claim name
-            if (scopeClaim == null)
+            if (context == null)
             {
-                scopeClaim = context?.User?.FindFirst(ClaimConstants.Scp);
+                throw new ArgumentNullException(nameof(context));
             }
-
-            if (scopeClaim == null || !scopeClaim.Value.Split(' ').Intersect(acceptedScopes).Any())
+            else if (context.User == null || context.User.Claims == null || !context.User.Claims.Any())
             {
                 context.Response.StatusCode = (int)HttpStatusCode.Unauthorized;
-                string message = $"The 'scope' claim does not contain scopes '{string.Join(",", acceptedScopes)}' or was not found";
-                throw new HttpRequestException(message);
+            }
+            else
+            {
+                // Attempt with Scp claim
+                Claim? scopeClaim = context.User.FindFirst(ClaimConstants.Scp);
+
+                // Fallback to Scope claim name
+                if (scopeClaim == null)
+                {
+                    scopeClaim = context?.User?.FindFirst(ClaimConstants.Scope);
+                }
+
+                if (scopeClaim == null || !scopeClaim.Value.Split(' ').Intersect(acceptedScopes).Any())
+                {
+                    context.Response.StatusCode = (int)HttpStatusCode.Forbidden;
+                    string message = $"The 'scope' or 'scp' claim does not contain scopes '{string.Join(",", acceptedScopes)}' or was not found";
+                    context.Response.WriteAsync(message);
+                }
             }
         }
     }

diff --git a/tests/Microsoft.Identity.Web.Test.Common/TestHelpers/HttpContextUtilities.cs b/tests/Microsoft.Identity.Web.Test.Common/TestHelpers/HttpContextUtilities.cs
@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+using System.IO;
 using System.Security.Claims;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
@@ -21,7 +22,10 @@ public static HttpContext CreateHttpContext()
             featureCollection.Set<IHttpResponseFeature>(new HttpResponseFeature());
             featureCollection.Set<IHttpRequestFeature>(new HttpRequestFeature());
 
-            return contextFactory.Create(featureCollection);
+            HttpContext httpContext = contextFactory.Create(featureCollection);
+            httpContext.Response.Body = new MemoryStream();
+            httpContext.Response.Body.Seek(0, SeekOrigin.Begin);
+            return httpContext;
         }
 
         public static HttpContext CreateHttpContext(string[] userScopes)
@@ -37,4 +41,4 @@ public static HttpContext CreateHttpContext(string[] userScopes)
             return httpContext;
         }
     }
-}
+}
diff --git a/tests/Microsoft.Identity.Web.Test/Resource/ScopesRequiredHttpContextExtensionsTests.cs b/tests/Microsoft.Identity.Web.Test/Resource/ScopesRequiredHttpContextExtensionsTests.cs
@@ -18,7 +18,7 @@ public void VerifyUserHasAnyAcceptedScope_NullParameters_ThrowsException()
         {
             HttpContext httpContext = null;
 
-            Assert.Throws<NullReferenceException>(() => httpContext.VerifyUserHasAnyAcceptedScope(string.Empty));
+            Assert.Throws<ArgumentNullException>(() => httpContext.VerifyUserHasAnyAcceptedScope(string.Empty));
 
             httpContext = HttpContextUtilities.CreateHttpContext();
 
@@ -29,35 +29,44 @@ public void VerifyUserHasAnyAcceptedScope_NullParameters_ThrowsException()
         public void VerifyUserHasAnyAcceptedScope_NoClaims_ThrowsException()
         {
             var acceptedScopes = new[] { "acceptedScope1", "acceptedScope2" };
-            var expectedErrorMessage = $"The 'scope' claim does not contain scopes '{string.Join(",", acceptedScopes)}' or was not found";
             var expectedStatusCode = (int)HttpStatusCode.Unauthorized;
 
             var httpContext = HttpContextUtilities.CreateHttpContext();
+            httpContext.VerifyUserHasAnyAcceptedScope(acceptedScopes);
 
-            var exception = Assert.Throws<HttpRequestException>(() => httpContext.VerifyUserHasAnyAcceptedScope(acceptedScopes));
-            Assert.Equal(expectedStatusCode, httpContext.Response.StatusCode);
-            Assert.Equal(expectedErrorMessage, exception.Message);
+            HttpResponse response = httpContext.Response;
+            Assert.Equal(expectedStatusCode, response.StatusCode);
         }
 
         [Fact]
         public void VerifyUserHasAnyAcceptedScope_NoAcceptedScopes_ThrowsException()
         {
             var acceptedScopes = new[] { "acceptedScope1", "acceptedScope2" };
             var actualScopes = new[] { "acceptedScope3", "acceptedScope4" };
-            var expectedErrorMessage = $"The 'scope' claim does not contain scopes '{string.Join(",", acceptedScopes)}' or was not found";
-            var expectedStatusCode = (int)HttpStatusCode.Unauthorized;
+            var expectedErrorMessage = $"The 'scope' or 'scp' claim does not contain scopes '{string.Join(",", acceptedScopes)}' or was not found";
+            var expectedStatusCode = (int)HttpStatusCode.Forbidden;
 
             var httpContext = HttpContextUtilities.CreateHttpContext(actualScopes);
+            httpContext.VerifyUserHasAnyAcceptedScope(acceptedScopes);
 
-            var exception = Assert.Throws<HttpRequestException>(() => httpContext.VerifyUserHasAnyAcceptedScope(acceptedScopes));
-            Assert.Equal(expectedStatusCode, httpContext.Response.StatusCode);
-            Assert.Equal(expectedErrorMessage, exception.Message);
+            HttpResponse response = httpContext.Response;
+            Assert.Equal(expectedStatusCode, response.StatusCode);
+            Assert.Equal(expectedErrorMessage, GetBody(response));
 
             httpContext = HttpContextUtilities.CreateHttpContext(new[] { "acceptedScope3", "acceptedScope4" });
+            httpContext.VerifyUserHasAnyAcceptedScope(acceptedScopes);
+            response = httpContext.Response;
+            Assert.Equal(expectedStatusCode, response.StatusCode);
+            Assert.Equal(expectedErrorMessage, GetBody(response));
+        }
 
-            exception = Assert.Throws<HttpRequestException>(() => httpContext.VerifyUserHasAnyAcceptedScope(acceptedScopes));
-            Assert.Equal(expectedStatusCode, httpContext.Response.StatusCode);
-            Assert.Equal(expectedErrorMessage, exception.Message);
+        private static string GetBody(HttpResponse response)
+        {
+            byte[] buffer = new byte[response.Body.Length];
+            response.Body.Seek(0, System.IO.SeekOrigin.Begin);
+            response.Body.Read(buffer, 0, buffer.Length);
+            string body = System.Text.Encoding.Default.GetString(buffer);
+            return body;
         }
 
         [Fact]

diff --git a/tests/WebAppCallsWebApiCallsGraph/Client/Services/TodoListService.cs b/tests/WebAppCallsWebApiCallsGraph/Client/Services/TodoListService.cs
@@ -105,15 +105,14 @@ public async Task<IEnumerable<Todo>> GetAsync()
         {
             await PrepareAuthenticatedClient();
             var response = await _httpClient.GetAsync($"{ _TodoListBaseAddress}/api/todolist");
+            var content = await response.Content.ReadAsStringAsync();
+
             if (response.StatusCode == HttpStatusCode.OK)
             {
-                var content = await response.Content.ReadAsStringAsync();
                 IEnumerable<Todo> todolist = JsonSerializer.Deserialize<IEnumerable<Todo>>(content, _jsonOptions);
-
                 return todolist;
             }
-
-            throw new HttpRequestException($"Invalid status code in the HttpResponseMessage: {response.StatusCode}.");
+            throw new HttpRequestException($"Invalid status code in the HttpResponseMessage: {response.StatusCode}. Cause: {content}");
         }
 
         private async Task PrepareAuthenticatedClient()