From ba42701dec198b267005ed996595b3664dd90c24 Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Wed, 21 Sep 2022 11:56:00 -0400 Subject: [PATCH 1/5] add a skip response handling metadata flag --- .../EndpointConventionBuilderExtensions.cs | 12 +- .../BffApiSkipResponseHandlingAttribute.cs | 14 ++ ...BffAuthorizationMiddlewareResultHandler.cs | 20 ++- .../IBffApiSkipResponseHandling.cs | 11 ++ .../Endpoints/LocalEndpointTests.cs | 25 ++- test/Duende.Bff.Tests/TestHosts/BffHost.cs | 158 +++++++++++++++--- 6 files changed, 205 insertions(+), 35 deletions(-) create mode 100644 src/Duende.Bff/EndpointProcessing/BffApiSkipResponseHandlingAttribute.cs create mode 100644 src/Duende.Bff/EndpointProcessing/IBffApiSkipResponseHandling.cs diff --git a/src/Duende.Bff/Configuration/EndpointConventionBuilderExtensions.cs b/src/Duende.Bff/Configuration/EndpointConventionBuilderExtensions.cs index 8cbc0f47..a0141d4e 100644 --- a/src/Duende.Bff/Configuration/EndpointConventionBuilderExtensions.cs +++ b/src/Duende.Bff/Configuration/EndpointConventionBuilderExtensions.cs @@ -20,7 +20,7 @@ public static IEndpointConventionBuilder AsBffApiEndpoint(this IEndpointConventi { return builder.WithMetadata(new BffApiAttribute()); } - + /// /// Adds marker that will cause the BFF framework to skip all antiforgery for this endpoint. /// @@ -30,4 +30,14 @@ public static IEndpointConventionBuilder SkipAntiforgery(this IEndpointConventio { return builder.WithMetadata(new BffApiSkipAntiforgeryAttribute()); } + + /// + /// Adds marker that will cause the BFF framework will not override the HTTP response status code. + /// + /// + /// + public static IEndpointConventionBuilder SkipResponseHandling(this IEndpointConventionBuilder builder) + { + return builder.WithMetadata(new BffApiSkipResponseHandlingAttribute()); + } } \ No newline at end of file diff --git a/src/Duende.Bff/EndpointProcessing/BffApiSkipResponseHandlingAttribute.cs b/src/Duende.Bff/EndpointProcessing/BffApiSkipResponseHandlingAttribute.cs new file mode 100644 index 00000000..20fe5d10 --- /dev/null +++ b/src/Duende.Bff/EndpointProcessing/BffApiSkipResponseHandlingAttribute.cs @@ -0,0 +1,14 @@ +// Copyright (c) Duende Software. All rights reserved. +// See LICENSE in the project root for license information. + +using System; + +namespace Duende.Bff; + +/// +/// This attribute indicates that the BFF midleware will not override the HTTP response status code. +/// +[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)] +public class BffApiSkipResponseHandlingAttribute : Attribute, IBffApiSkipResponseHandling +{ +} \ No newline at end of file diff --git a/src/Duende.Bff/EndpointProcessing/BffAuthorizationMiddlewareResultHandler.cs b/src/Duende.Bff/EndpointProcessing/BffAuthorizationMiddlewareResultHandler.cs index b4ebfa98..91dad6a0 100644 --- a/src/Duende.Bff/EndpointProcessing/BffAuthorizationMiddlewareResultHandler.cs +++ b/src/Duende.Bff/EndpointProcessing/BffAuthorizationMiddlewareResultHandler.cs @@ -36,16 +36,20 @@ public Task HandleAsync(RequestDelegate next, HttpContext context, Authorization var isBffEndpoint = endpoint.Metadata.GetMetadata() != null; if (isBffEndpoint) { - if (authorizeResult.Challenged) + var requireResponseHandling = endpoint.Metadata.GetMetadata() == null; + if (requireResponseHandling) { - context.Response.StatusCode = (int)HttpStatusCode.Unauthorized; - return Task.CompletedTask; - } + if (authorizeResult.Challenged) + { + context.Response.StatusCode = (int)HttpStatusCode.Unauthorized; + return Task.CompletedTask; + } - if (authorizeResult.Forbidden) - { - context.Response.StatusCode = (int)HttpStatusCode.Forbidden; - return Task.CompletedTask; + if (authorizeResult.Forbidden) + { + context.Response.StatusCode = (int)HttpStatusCode.Forbidden; + return Task.CompletedTask; + } } } } diff --git a/src/Duende.Bff/EndpointProcessing/IBffApiSkipResponseHandling.cs b/src/Duende.Bff/EndpointProcessing/IBffApiSkipResponseHandling.cs new file mode 100644 index 00000000..7682cd53 --- /dev/null +++ b/src/Duende.Bff/EndpointProcessing/IBffApiSkipResponseHandling.cs @@ -0,0 +1,11 @@ +// Copyright (c) Duende Software. All rights reserved. +// See LICENSE in the project root for license information. + +namespace Duende.Bff; + +/// +/// Indicates that the BFF midleware will not override the HTTP response status code. +/// +public interface IBffApiSkipResponseHandling +{ +} diff --git a/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs b/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs index 32c528fb..c364be1e 100644 --- a/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs +++ b/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs @@ -149,11 +149,11 @@ public async Task forbidden_api_call_should_return_403() response.StatusCode.Should().Be(HttpStatusCode.Forbidden); } - [Fact] - public async Task response_status_401_should_return_401() + [Fact(Skip = "need to restore manual response handling")] + public async Task challenge_response_should_return_401() { await BffHost.BffLoginAsync("alice"); - BffHost.LocalApiStatusCodeToReturn = 401; + BffHost.LocalApiResponseStatus = BffHost.ResponseStatus.Challenge; var req = new HttpRequestMessage(HttpMethod.Get, BffHost.Url("/local_authz")); req.Headers.Add("x-csrf", "1"); @@ -162,11 +162,11 @@ public async Task response_status_401_should_return_401() response.StatusCode.Should().Be(HttpStatusCode.Unauthorized); } - [Fact] - public async Task response_status_403_should_return_403() + [Fact(Skip = "need to restore manual response handling")] + public async Task forbid_response_should_return_403() { await BffHost.BffLoginAsync("alice"); - BffHost.LocalApiStatusCodeToReturn = 403; + BffHost.LocalApiResponseStatus = BffHost.ResponseStatus.Forbid; var req = new HttpRequestMessage(HttpMethod.Get, BffHost.Url("/local_authz")); req.Headers.Add("x-csrf", "1"); @@ -175,6 +175,19 @@ public async Task response_status_403_should_return_403() response.StatusCode.Should().Be(HttpStatusCode.Forbidden); } + [Fact] + public async Task challenge_response_when_response_handling_skipped_should_trigger_redirect_for_login() + { + await BffHost.BffLoginAsync("alice"); + BffHost.LocalApiResponseStatus = BffHost.ResponseStatus.Challenge; + + var req = new HttpRequestMessage(HttpMethod.Get, BffHost.Url("/local_anon_no_csrf_no_response_handling")); + var response = await BffHost.BrowserClient.SendAsync(req); + + response.StatusCode.Should().Be(HttpStatusCode.Redirect); + } + + [Fact] public async Task fallback_policy_should_not_fail() { diff --git a/test/Duende.Bff.Tests/TestHosts/BffHost.cs b/test/Duende.Bff.Tests/TestHosts/BffHost.cs index a5e4da4f..64d58982 100644 --- a/test/Duende.Bff.Tests/TestHosts/BffHost.cs +++ b/test/Duende.Bff.Tests/TestHosts/BffHost.cs @@ -16,12 +16,17 @@ using System.Threading.Tasks; using Duende.Bff.Yarp; using Microsoft.AspNetCore.HttpOverrides; +using Microsoft.AspNetCore.Authentication; namespace Duende.Bff.Tests.TestHosts { public class BffHost : GenericHost { - public int? LocalApiStatusCodeToReturn { get; set; } + public enum ResponseStatus + { + Ok, Challenge, Forbid + } + public ResponseStatus LocalApiResponseStatus { get; set; } = ResponseStatus.Ok; private readonly IdentityServerHost _identityServerHost; private readonly ApiHost _apiHost; @@ -48,7 +53,8 @@ private void ConfigureServices(IServiceCollection services) services.AddRouting(); services.AddAuthorization(); - var bff = services.AddBff(options => { + var bff = services.AddBff(options => + { BffOptions = options; }); @@ -158,14 +164,28 @@ private void Configure(IApplicationBuilder app) RequestHeaders = requestHeaders }; - context.Response.StatusCode = LocalApiStatusCodeToReturn ?? 200; - LocalApiStatusCodeToReturn = null; + if (LocalApiResponseStatus == ResponseStatus.Ok) + { + context.Response.StatusCode = 200; - context.Response.ContentType = "application/json"; - await context.Response.WriteAsync(JsonSerializer.Serialize(response)); + context.Response.ContentType = "application/json"; + await context.Response.WriteAsync(JsonSerializer.Serialize(response)); + } + else if (LocalApiResponseStatus == ResponseStatus.Challenge) + { + await context.ChallengeAsync(); + } + else if (LocalApiResponseStatus == ResponseStatus.Forbid) + { + await context.ForbidAsync(); + } + else + { + throw new Exception("Invalid LocalApiResponseStatus"); + } }) .AsBffApiEndpoint(); - + endpoints.Map("/local_anon_no_csrf", async context => { // capture body if present @@ -197,15 +217,85 @@ private void Configure(IApplicationBuilder app) RequestHeaders = requestHeaders }; - context.Response.StatusCode = LocalApiStatusCodeToReturn ?? 200; - LocalApiStatusCodeToReturn = null; + if (LocalApiResponseStatus == ResponseStatus.Ok) + { + context.Response.StatusCode = 200; - context.Response.ContentType = "application/json"; - await context.Response.WriteAsync(JsonSerializer.Serialize(response)); + context.Response.ContentType = "application/json"; + await context.Response.WriteAsync(JsonSerializer.Serialize(response)); + } + else if (LocalApiResponseStatus == ResponseStatus.Challenge) + { + await context.ChallengeAsync(); + } + else if (LocalApiResponseStatus == ResponseStatus.Forbid) + { + await context.ForbidAsync(); + } + else + { + throw new Exception("Invalid LocalApiResponseStatus"); + } }) .AsBffApiEndpoint() .SkipAntiforgery(); + endpoints.Map("/local_anon_no_csrf_no_response_handling", async context => + { + // capture body if present + var body = default(string); + if (context.Request.HasJsonContentType()) + { + using (var sr = new StreamReader(context.Request.Body)) + { + body = await sr.ReadToEndAsync(); + } + } + + // capture request headers + var requestHeaders = new Dictionary>(); + foreach (var header in context.Request.Headers) + { + var values = new List(header.Value.Select(v => v)); + requestHeaders.Add(header.Key, values); + } + + var response = new ApiResponse( + context.Request.Method, + context.Request.Path.Value, + context.User.FindFirst(("sub"))?.Value, + context.User.FindFirst(("client_id"))?.Value, + context.User.Claims.Select(x => new ClaimRecord(x.Type, x.Value)).ToArray()) + { + Body = body, + RequestHeaders = requestHeaders + }; + + if (LocalApiResponseStatus == ResponseStatus.Ok) + { + context.Response.StatusCode = 200; + + context.Response.ContentType = "application/json"; + await context.Response.WriteAsync(JsonSerializer.Serialize(response)); + } + else if (LocalApiResponseStatus == ResponseStatus.Challenge) + { + await context.ChallengeAsync(); + } + else if (LocalApiResponseStatus == ResponseStatus.Forbid) + { + await context.ForbidAsync(); + } + else + { + throw new Exception("Invalid LocalApiResponseStatus"); + } + }) + .AsBffApiEndpoint() + .SkipAntiforgery() + .SkipResponseHandling(); + + endpoints.Map("/local_authz", async context => { var sub = context.User.FindFirst(("sub"))?.Value; @@ -230,11 +320,25 @@ private void Configure(IApplicationBuilder app) Body = body }; - context.Response.StatusCode = LocalApiStatusCodeToReturn ?? 200; - LocalApiStatusCodeToReturn = null; + if (LocalApiResponseStatus == ResponseStatus.Ok) + { + context.Response.StatusCode = 200; - context.Response.ContentType = "application/json"; - await context.Response.WriteAsync(JsonSerializer.Serialize(response)); + context.Response.ContentType = "application/json"; + await context.Response.WriteAsync(JsonSerializer.Serialize(response)); + } + else if (LocalApiResponseStatus == ResponseStatus.Challenge) + { + await context.ChallengeAsync(); + } + else if (LocalApiResponseStatus == ResponseStatus.Forbid) + { + await context.ForbidAsync(); + } + else + { + throw new Exception("Invalid LocalApiResponseStatus"); + } }) .RequireAuthorization() .AsBffApiEndpoint(); @@ -263,11 +367,25 @@ private void Configure(IApplicationBuilder app) Body = body }; - context.Response.StatusCode = LocalApiStatusCodeToReturn ?? 200; - LocalApiStatusCodeToReturn = null; + if (LocalApiResponseStatus == ResponseStatus.Ok) + { + context.Response.StatusCode = 200; - context.Response.ContentType = "application/json"; - await context.Response.WriteAsync(JsonSerializer.Serialize(response)); + context.Response.ContentType = "application/json"; + await context.Response.WriteAsync(JsonSerializer.Serialize(response)); + } + else if (LocalApiResponseStatus == ResponseStatus.Challenge) + { + await context.ChallengeAsync(); + } + else if (LocalApiResponseStatus == ResponseStatus.Forbid) + { + await context.ForbidAsync(); + } + else + { + throw new Exception("Invalid LocalApiResponseStatus"); + } }) .RequireAuthorization() .AsBffApiEndpoint() @@ -306,7 +424,7 @@ private void Configure(IApplicationBuilder app) "/api_anon_only", _apiHost.Url()); endpoints.Map( - "/not_bff_endpoint", + "/not_bff_endpoint", RemoteApiEndpoint.Map("/not_bff_endpoint", _apiHost.Url())); }); From a667a8bedb5038a97d293e6f135fb221fc0c556c Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Wed, 21 Sep 2022 12:29:11 -0400 Subject: [PATCH 2/5] reintroduce response handling logic in BFF middleware --- .../EndpointProcessing/BffMiddleware.cs | 22 +++++++++++++++++++ .../Endpoints/LocalEndpointTests.cs | 4 ++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/Duende.Bff/EndpointProcessing/BffMiddleware.cs b/src/Duende.Bff/EndpointProcessing/BffMiddleware.cs index 38c4ec91..fedf7f4d 100644 --- a/src/Duende.Bff/EndpointProcessing/BffMiddleware.cs +++ b/src/Duende.Bff/EndpointProcessing/BffMiddleware.cs @@ -67,6 +67,28 @@ public async Task Invoke(HttpContext context) } } + context.Response.OnStarting(() => + { + // outbound: we assume that an API will never return a 302 + // if a 302 is returned, that must be the challenge to the OIDC provider + // we convert this to a 401 + if (isBffEndpoint) + { + if (context.Response.StatusCode == 302) + { + var requireResponseHandling = endpoint.Metadata.GetMetadata() == null; + if (requireResponseHandling) + { + context.Response.StatusCode = 401; + context.Response.Headers.Remove("Location"); + context.Response.Headers.Remove("Set-Cookie"); + } + } + } + + return Task.CompletedTask; + }); + await _next(context); } } \ No newline at end of file diff --git a/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs b/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs index c364be1e..8abac858 100644 --- a/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs +++ b/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs @@ -149,7 +149,7 @@ public async Task forbidden_api_call_should_return_403() response.StatusCode.Should().Be(HttpStatusCode.Forbidden); } - [Fact(Skip = "need to restore manual response handling")] + [Fact] public async Task challenge_response_should_return_401() { await BffHost.BffLoginAsync("alice"); @@ -162,7 +162,7 @@ public async Task challenge_response_should_return_401() response.StatusCode.Should().Be(HttpStatusCode.Unauthorized); } - [Fact(Skip = "need to restore manual response handling")] + [Fact(Skip = "need to handle Forbid when cookie handler triggers redirect to access denied page")] public async Task forbid_response_should_return_403() { await BffHost.BffLoginAsync("alice"); From 2e4a0858670e4a03b9850308fd8066ce8d1cefc5 Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Wed, 21 Sep 2022 12:56:12 -0400 Subject: [PATCH 3/5] add authentication service decorator to handle response processing for challenge/forbid calls --- .../BffServiceCollectionExtensions.cs | 6 ++ src/Duende.Bff/Configuration/Decorator.cs | 90 +++++++++++++++++++ .../BffAuthenticationService.cs | 90 +++++++++++++++++++ .../EndpointProcessing/BffMiddleware.cs | 22 ----- .../Endpoints/LocalEndpointTests.cs | 2 +- 5 files changed, 187 insertions(+), 23 deletions(-) create mode 100644 src/Duende.Bff/Configuration/Decorator.cs create mode 100644 src/Duende.Bff/EndpointProcessing/BffAuthenticationService.cs diff --git a/src/Duende.Bff/Configuration/BffServiceCollectionExtensions.cs b/src/Duende.Bff/Configuration/BffServiceCollectionExtensions.cs index 6676a649..d64b8476 100644 --- a/src/Duende.Bff/Configuration/BffServiceCollectionExtensions.cs +++ b/src/Duende.Bff/Configuration/BffServiceCollectionExtensions.cs @@ -9,6 +9,8 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.Extensions.Options; using Microsoft.AspNetCore.Authentication.OpenIdConnect; +using System.Linq; +using Microsoft.AspNetCore.Authentication; namespace Microsoft.AspNetCore.Builder; @@ -55,6 +57,10 @@ public static BffBuilder AddBff(this IServiceCollection services, Action(); + // wrap ASP.NET Core + services.AddAuthentication(); + services.AddTransientDecorator(); + return new BffBuilder(services); } } \ No newline at end of file diff --git a/src/Duende.Bff/Configuration/Decorator.cs b/src/Duende.Bff/Configuration/Decorator.cs new file mode 100644 index 00000000..77239cd1 --- /dev/null +++ b/src/Duende.Bff/Configuration/Decorator.cs @@ -0,0 +1,90 @@ +// Copyright (c) Duende Software. All rights reserved. +// See LICENSE in the project root for license information. + +using Microsoft.Extensions.DependencyInjection; +using System; +using System.Linq; + +namespace Microsoft.AspNetCore.Builder; + +/// +/// Extension methods for the BFF DI services +/// +static class DecoratorServiceCollectionExtensions +{ + internal static void AddTransientDecorator(this IServiceCollection services) + where TService : class + where TImplementation : class, TService + { + services.AddDecorator(); + services.AddTransient(); + } + + internal static void AddDecorator(this IServiceCollection services) + { + var registration = services.LastOrDefault(x => x.ServiceType == typeof(TService)); + if (registration == null) + { + throw new InvalidOperationException("Service type: " + typeof(TService).Name + " not registered."); + } + if (services.Any(x => x.ServiceType == typeof(Decorator))) + { + throw new InvalidOperationException("Decorator already registered for type: " + typeof(TService).Name + "."); + } + + services.Remove(registration); + + if (registration.ImplementationInstance != null) + { + var type = registration.ImplementationInstance.GetType(); + var innerType = typeof(Decorator<,>).MakeGenericType(typeof(TService), type); + services.Add(new ServiceDescriptor(typeof(Decorator), innerType, ServiceLifetime.Transient)); + services.Add(new ServiceDescriptor(type, registration.ImplementationInstance)); + } + else if (registration.ImplementationFactory != null) + { + services.Add(new ServiceDescriptor(typeof(Decorator), provider => + { + return new DisposableDecorator((TService)registration.ImplementationFactory(provider)); + }, registration.Lifetime)); + } + else + { + var type = registration.ImplementationType!; + var innerType = typeof(Decorator<,>).MakeGenericType(typeof(TService), type); + services.Add(new ServiceDescriptor(typeof(Decorator), innerType, ServiceLifetime.Transient)); + services.Add(new ServiceDescriptor(type, type, registration.Lifetime)); + } + } + +} + +internal class Decorator +{ + public TService Instance { get; set; } + + public Decorator(TService instance) + { + Instance = instance; + } +} + +internal class Decorator : Decorator + where TImpl : class, TService +{ + public Decorator(TImpl instance) : base(instance) + { + } +} + +internal class DisposableDecorator : Decorator, IDisposable +{ + public DisposableDecorator(TService instance) : base(instance) + { + } + + public void Dispose() + { + (Instance as IDisposable)?.Dispose(); + } +} \ No newline at end of file diff --git a/src/Duende.Bff/EndpointProcessing/BffAuthenticationService.cs b/src/Duende.Bff/EndpointProcessing/BffAuthenticationService.cs new file mode 100644 index 00000000..2fa36f33 --- /dev/null +++ b/src/Duende.Bff/EndpointProcessing/BffAuthenticationService.cs @@ -0,0 +1,90 @@ +// Copyright (c) Duende Software. All rights reserved. +// See LICENSE in the project root for license information. + +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Http; +using System.Security.Claims; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.AspNetCore.Builder; + +namespace Duende.Bff; + +// this decorates the real authentication service to detect when +// Challenge of Forbid is being called for a BFF API endpoint +internal class BffAuthenticationService : IAuthenticationService +{ + private readonly IAuthenticationService _inner; + private readonly ILogger _logger; + + public BffAuthenticationService( + Decorator decorator, + ILogger logger) + { + _inner = decorator.Instance; + _logger = logger; + } + + public Task SignInAsync(HttpContext context, string? scheme, ClaimsPrincipal principal, AuthenticationProperties? properties) + { + return _inner.SignInAsync(context, scheme, principal, properties); + } + + public Task SignOutAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) + { + return _inner.SignOutAsync(context, scheme, properties); + } + + public Task AuthenticateAsync(HttpContext context, string? scheme) + { + return _inner.AuthenticateAsync(context, scheme); + } + + public async Task ChallengeAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) + { + await _inner.ChallengeAsync(context, scheme, properties); + + var endpoint = context.GetEndpoint(); + if (endpoint != null) + { + if (context.Response.StatusCode == 302) + { + var isBffEndpoint = endpoint.Metadata.GetMetadata() != null; + if (isBffEndpoint) + { + var requireResponseHandling = endpoint.Metadata.GetMetadata() == null; + if (requireResponseHandling) + { + context.Response.StatusCode = 401; + context.Response.Headers.Remove("Location"); + context.Response.Headers.Remove("Set-Cookie"); + } + } + } + } + } + + public async Task ForbidAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) + { + await _inner.ForbidAsync(context, scheme, properties); + + var endpoint = context.GetEndpoint(); + if (endpoint != null) + { + if (context.Response.StatusCode == 302) + { + var isBffEndpoint = endpoint.Metadata.GetMetadata() != null; + if (isBffEndpoint) + { + var requireResponseHandling = endpoint.Metadata.GetMetadata() == null; + if (requireResponseHandling) + { + context.Response.StatusCode = 403; + context.Response.Headers.Remove("Location"); + context.Response.Headers.Remove("Set-Cookie"); + } + } + } + } + } +} \ No newline at end of file diff --git a/src/Duende.Bff/EndpointProcessing/BffMiddleware.cs b/src/Duende.Bff/EndpointProcessing/BffMiddleware.cs index fedf7f4d..38c4ec91 100644 --- a/src/Duende.Bff/EndpointProcessing/BffMiddleware.cs +++ b/src/Duende.Bff/EndpointProcessing/BffMiddleware.cs @@ -67,28 +67,6 @@ public async Task Invoke(HttpContext context) } } - context.Response.OnStarting(() => - { - // outbound: we assume that an API will never return a 302 - // if a 302 is returned, that must be the challenge to the OIDC provider - // we convert this to a 401 - if (isBffEndpoint) - { - if (context.Response.StatusCode == 302) - { - var requireResponseHandling = endpoint.Metadata.GetMetadata() == null; - if (requireResponseHandling) - { - context.Response.StatusCode = 401; - context.Response.Headers.Remove("Location"); - context.Response.Headers.Remove("Set-Cookie"); - } - } - } - - return Task.CompletedTask; - }); - await _next(context); } } \ No newline at end of file diff --git a/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs b/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs index 8abac858..23cebb7a 100644 --- a/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs +++ b/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs @@ -162,7 +162,7 @@ public async Task challenge_response_should_return_401() response.StatusCode.Should().Be(HttpStatusCode.Unauthorized); } - [Fact(Skip = "need to handle Forbid when cookie handler triggers redirect to access denied page")] + [Fact] public async Task forbid_response_should_return_403() { await BffHost.BffLoginAsync("alice"); From fdfefa8d1d2cdba7260c871f1e821faddf555200 Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Wed, 21 Sep 2022 12:57:59 -0400 Subject: [PATCH 4/5] remove IAuthorizationMiddlewareResultHandler since it doesn't handle all calls to Challenge/Forbid --- .../BffServiceCollectionExtensions.cs | 2 - ...BffAuthorizationMiddlewareResultHandler.cs | 60 ------------------- 2 files changed, 62 deletions(-) delete mode 100644 src/Duende.Bff/EndpointProcessing/BffAuthorizationMiddlewareResultHandler.cs diff --git a/src/Duende.Bff/Configuration/BffServiceCollectionExtensions.cs b/src/Duende.Bff/Configuration/BffServiceCollectionExtensions.cs index d64b8476..32f8b255 100644 --- a/src/Duende.Bff/Configuration/BffServiceCollectionExtensions.cs +++ b/src/Duende.Bff/Configuration/BffServiceCollectionExtensions.cs @@ -55,8 +55,6 @@ public static BffBuilder AddBff(this IServiceCollection services, Action, PostConfigureOidcOptionsForSilentLogin>(); - services.AddTransient(); - // wrap ASP.NET Core services.AddAuthentication(); services.AddTransientDecorator(); diff --git a/src/Duende.Bff/EndpointProcessing/BffAuthorizationMiddlewareResultHandler.cs b/src/Duende.Bff/EndpointProcessing/BffAuthorizationMiddlewareResultHandler.cs deleted file mode 100644 index 91dad6a0..00000000 --- a/src/Duende.Bff/EndpointProcessing/BffAuthorizationMiddlewareResultHandler.cs +++ /dev/null @@ -1,60 +0,0 @@ -// Copyright (c) Duende Software. All rights reserved. -// See LICENSE in the project root for license information. - -using System; -using System.Net; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Authorization.Policy; -using Microsoft.AspNetCore.Http; - -namespace Duende.Bff -{ - /// - /// Converts Challenge/Forbid to Ajax friendly status codes for BFF API endpoints - /// - public class BffAuthorizationMiddlewareResultHandler : IAuthorizationMiddlewareResultHandler - { - private readonly AuthorizationMiddlewareResultHandler _handler; - - /// - /// ctor - /// - public BffAuthorizationMiddlewareResultHandler() - { - _handler = new AuthorizationMiddlewareResultHandler(); - } - - /// - public Task HandleAsync(RequestDelegate next, HttpContext context, AuthorizationPolicy policy, - PolicyAuthorizationResult authorizeResult) - { - var endpoint = context.GetEndpoint(); - - if (endpoint != null) - { - var isBffEndpoint = endpoint.Metadata.GetMetadata() != null; - if (isBffEndpoint) - { - var requireResponseHandling = endpoint.Metadata.GetMetadata() == null; - if (requireResponseHandling) - { - if (authorizeResult.Challenged) - { - context.Response.StatusCode = (int)HttpStatusCode.Unauthorized; - return Task.CompletedTask; - } - - if (authorizeResult.Forbidden) - { - context.Response.StatusCode = (int)HttpStatusCode.Forbidden; - return Task.CompletedTask; - } - } - } - } - - return _handler.HandleAsync(next, context, policy, authorizeResult); - } - } -} \ No newline at end of file From b486b71db7041c1dfcbd02369fb0aec802de02de Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Fri, 23 Sep 2022 10:46:54 -0400 Subject: [PATCH 5/5] emit logs on response handling --- src/Duende.Bff/EndpointProcessing/BffAuthenticationService.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Duende.Bff/EndpointProcessing/BffAuthenticationService.cs b/src/Duende.Bff/EndpointProcessing/BffAuthenticationService.cs index 2fa36f33..68867c0e 100644 --- a/src/Duende.Bff/EndpointProcessing/BffAuthenticationService.cs +++ b/src/Duende.Bff/EndpointProcessing/BffAuthenticationService.cs @@ -55,6 +55,8 @@ public async Task ChallengeAsync(HttpContext context, string? scheme, Authentica var requireResponseHandling = endpoint.Metadata.GetMetadata() == null; if (requireResponseHandling) { + _logger.LogDebug("Challenge was called for a BFF API endpoint, BFF response handing changing status code to 401."); + context.Response.StatusCode = 401; context.Response.Headers.Remove("Location"); context.Response.Headers.Remove("Set-Cookie"); @@ -79,6 +81,8 @@ public async Task ForbidAsync(HttpContext context, string? scheme, Authenticatio var requireResponseHandling = endpoint.Metadata.GetMetadata() == null; if (requireResponseHandling) { + _logger.LogDebug("Forbid was called for a BFF API endpoint, BFF response handing changing status code to 403."); + context.Response.StatusCode = 403; context.Response.Headers.Remove("Location"); context.Response.Headers.Remove("Set-Cookie");