From 99d55422cce0e1fd8f8d2bb2f2f6fef72b35fedf Mon Sep 17 00:00:00 2001 From: Jean-Marc Prieur Date: Fri, 26 Jun 2020 17:24:59 +0200 Subject: [PATCH] Fixes #253: Check/decide the behavior of VerifyUserHasAnyAcceptedScope() (#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 * removing an un-needed space * Update src/Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs Co-authored-by: jennyf19 Co-authored-by: Marsh Macy Co-authored-by: jennyf19 --- .../Microsoft.Identity.Web.xml | 13 ++++--- .../ScopesRequiredHttpContextExtensions.cs | 39 ++++++++++++------- .../TestHelpers/HttpContextUtilities.cs | 8 +++- ...copesRequiredHttpContextExtensionsTests.cs | 35 ++++++++++------- .../Client/Services/TodoListService.cs | 7 ++-- 5 files changed, 64 insertions(+), 38 deletions(-) diff --git a/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml b/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml index ea88c397c..98f14a175 100644 --- a/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml +++ b/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml @@ -1210,14 +1210,15 @@ When applied to an , 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 , 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 Forbidden (403) + and writes to the response body a message telling which scopes are expected in the token. HttpContext (from the controller). Scopes accepted by this web API. - with a set to - . - + When the scopes don't match the response is a 403 (Forbidden), + because the user is authenticated (hence not 401), but not authorized. @@ -1508,14 +1509,14 @@ Initializes a new instance of the class. By default, the sliding expiration is set for 14 days. - + Gets or sets the value of the duration after which the cache entry will expire unless it's used This is the duration the tokens are kept in memory cache. In production, a higher value, up-to 90 days is recommended. - The SlidingExpiration value. + The AbsoluteExpirationRelativeToNow value. diff --git a/src/Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs b/src/Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs index 8c27cbf3c..b24dde2fa 100644 --- a/src/Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs +++ b/src/Microsoft.Identity.Web/Resource/ScopesRequiredHttpContextExtensions.cs @@ -19,14 +19,15 @@ public static class ScopesRequiredHttpContextExtensions /// /// When applied to an , 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 , 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. /// /// HttpContext (from the controller). /// Scopes accepted by this web API. - /// with a set to - /// . - /// + /// When the scopes don't match, the response is a 403 (Forbidden), + /// because the user is authenticated (hence not 401), but not authorized. public static void VerifyUserHasAnyAcceptedScope(this HttpContext context, params string[] acceptedScopes) { if (acceptedScopes == null) @@ -34,19 +35,31 @@ public static void VerifyUserHasAnyAcceptedScope(this HttpContext context, param 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 index b06b394f5..e49f666f2 100644 --- 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(new HttpResponseFeature()); featureCollection.Set(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; } } -} \ No newline at end of file +} diff --git a/tests/Microsoft.Identity.Web.Test/Resource/ScopesRequiredHttpContextExtensionsTests.cs b/tests/Microsoft.Identity.Web.Test/Resource/ScopesRequiredHttpContextExtensionsTests.cs index b0ea3c294..aa440adab 100644 --- 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(() => httpContext.VerifyUserHasAnyAcceptedScope(string.Empty)); + Assert.Throws(() => httpContext.VerifyUserHasAnyAcceptedScope(string.Empty)); httpContext = HttpContextUtilities.CreateHttpContext(); @@ -29,14 +29,13 @@ 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(() => httpContext.VerifyUserHasAnyAcceptedScope(acceptedScopes)); - Assert.Equal(expectedStatusCode, httpContext.Response.StatusCode); - Assert.Equal(expectedErrorMessage, exception.Message); + HttpResponse response = httpContext.Response; + Assert.Equal(expectedStatusCode, response.StatusCode); } [Fact] @@ -44,20 +43,30 @@ 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(() => 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(() => 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 index cceb6950c..207bf2c68 100644 --- a/tests/WebAppCallsWebApiCallsGraph/Client/Services/TodoListService.cs +++ b/tests/WebAppCallsWebApiCallsGraph/Client/Services/TodoListService.cs @@ -105,15 +105,14 @@ public async Task> 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 todolist = JsonSerializer.Deserialize>(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()