From feb48708968f34604e027c1ae764fdf48cfd213f Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Thu, 23 Jun 2022 12:42:33 -0400 Subject: [PATCH 1/2] first cut at reworking how we add metadata to skip antiforgery checks --- samples/JS.Yarp/Startup.cs | 1 - .../BffYarpEndpointRouteBuilderExtensions.cs | 7 ++--- ...roxyEndpointConventionBuilderExtensions.cs | 24 ++++++++-------- src/Duende.Bff/BffMiddleware.cs | 23 +++------------ src/Duende.Bff/Endpoints/BffApiAttribute.cs | 21 +++----------- .../BffApiSkipAntiforgeryAttribute.cs | 14 ++++++++++ ...BffAuthorizationMiddlewareResultHandler.cs | 28 ++++++++----------- .../Endpoints/BffRemoteApiEndpointMetadata.cs | 7 +---- .../EndpointConventionBuilderExtensions.cs | 15 ++++++++-- src/Duende.Bff/Endpoints/IBffApiEndpoint.cs | 12 ++++++++ .../Endpoints/IBffApiSkipAntiforgry.cs | 11 ++++++++ test/Duende.Bff.Tests/TestHosts/BffHost.cs | 9 ++++-- 12 files changed, 89 insertions(+), 83 deletions(-) create mode 100644 src/Duende.Bff/Endpoints/BffApiSkipAntiforgeryAttribute.cs create mode 100644 src/Duende.Bff/Endpoints/IBffApiEndpoint.cs create mode 100644 src/Duende.Bff/Endpoints/IBffApiSkipAntiforgry.cs diff --git a/samples/JS.Yarp/Startup.cs b/samples/JS.Yarp/Startup.cs index 1d258ef..b083724 100644 --- a/samples/JS.Yarp/Startup.cs +++ b/samples/JS.Yarp/Startup.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Linq; using Duende.Bff; using Duende.Bff.Yarp; using Microsoft.AspNetCore.Builder; diff --git a/src/Duende.Bff.Yarp/BffYarpEndpointRouteBuilderExtensions.cs b/src/Duende.Bff.Yarp/BffYarpEndpointRouteBuilderExtensions.cs index 86a888d..d5c32a3 100644 --- a/src/Duende.Bff.Yarp/BffYarpEndpointRouteBuilderExtensions.cs +++ b/src/Duende.Bff.Yarp/BffYarpEndpointRouteBuilderExtensions.cs @@ -19,20 +19,17 @@ public static class BffYarpEndpointRouteBuilderExtensions /// /// /// - /// /// public static IEndpointConventionBuilder MapRemoteBffApiEndpoint( this IEndpointRouteBuilder endpoints, PathString localPath, - string apiAddress, - bool requireAntiForgeryCheck = true) + string apiAddress) { endpoints.CheckLicense(); return endpoints.Map( localPath.Add("/{**catch-all}").Value!, RemoteApiEndpoint.Map(localPath, apiAddress)) - .WithMetadata(new BffRemoteApiEndpointMetadata { RequireAntiForgeryHeader = requireAntiForgeryCheck }); - + .WithMetadata(new BffRemoteApiEndpointMetadata()); } } \ No newline at end of file diff --git a/src/Duende.Bff.Yarp/ReverseProxyEndpointConventionBuilderExtensions.cs b/src/Duende.Bff.Yarp/ReverseProxyEndpointConventionBuilderExtensions.cs index 6cca2f5..997e9d7 100644 --- a/src/Duende.Bff.Yarp/ReverseProxyEndpointConventionBuilderExtensions.cs +++ b/src/Duende.Bff.Yarp/ReverseProxyEndpointConventionBuilderExtensions.cs @@ -17,38 +17,36 @@ public static class ReverseProxyEndpointConventionBuilderExtensions /// /// /// - /// /// public static ReverseProxyConventionBuilder MapBffReverseProxy(this IEndpointRouteBuilder endpoints, - Action configureAction, - bool requireAntiforgeryCheck = true) + Action configureAction) { return endpoints.MapReverseProxy(configureAction) - .AsBffApiEndpoint(requireAntiforgeryCheck); + .AsBffApiEndpoint(); } /// /// Adds YARP with anti-forgery protection /// /// - /// /// - public static ReverseProxyConventionBuilder MapBffReverseProxy(this IEndpointRouteBuilder endpoints, - bool requireAntiforgeryCheck = true) + public static ReverseProxyConventionBuilder MapBffReverseProxy(this IEndpointRouteBuilder endpoints) { return endpoints.MapReverseProxy() - .AsBffApiEndpoint(requireAntiforgeryCheck); + .AsBffApiEndpoint(); } - + + // TODO: do we also need a SkipAntiforgery API? + // TODO: review the API comment below + /// /// Adds anti-forgery protection to YARP /// /// - /// /// - public static ReverseProxyConventionBuilder AsBffApiEndpoint(this ReverseProxyConventionBuilder builder, - bool requireAntiforgeryCheck = true) + public static ReverseProxyConventionBuilder AsBffApiEndpoint(this ReverseProxyConventionBuilder builder) { - return builder.WithMetadata(new BffApiAttribute(requireAntiforgeryCheck)); + return builder.WithMetadata(new BffApiAttribute()); } + } \ No newline at end of file diff --git a/src/Duende.Bff/BffMiddleware.cs b/src/Duende.Bff/BffMiddleware.cs index cb9d757..ca3789b 100644 --- a/src/Duende.Bff/BffMiddleware.cs +++ b/src/Duende.Bff/BffMiddleware.cs @@ -1,7 +1,6 @@ // Copyright (c) Duende Software. All rights reserved. // See LICENSE in the project root for license information. -using System.Linq; using System.Threading.Tasks; using Duende.Bff.Logging; using Microsoft.AspNetCore.Http; @@ -51,25 +50,11 @@ public async Task Invoke(HttpContext context) return; } - var localEndpointMetadata = endpoint.Metadata.GetOrderedMetadata(); - if (localEndpointMetadata.Any()) + var isBffEndpoint = endpoint.Metadata.GetMetadata() != null; + if (isBffEndpoint) { - var requireLocalAntiForgeryCheck = localEndpointMetadata.First().RequireAntiForgeryCheck; - if (requireLocalAntiForgeryCheck) - { - if (!context.CheckAntiForgeryHeader(_options)) - { - _logger.AntiForgeryValidationFailed(context.Request.Path); - - context.Response.StatusCode = 401; - return; - } - } - } - else - { - var remoteEndpoint = endpoint.Metadata.GetMetadata(); - if (remoteEndpoint is { RequireAntiForgeryHeader: true }) + var requireAntiForgeryCheck = endpoint.Metadata.GetMetadata() == null; + if (requireAntiForgeryCheck) { if (!context.CheckAntiForgeryHeader(_options)) { diff --git a/src/Duende.Bff/Endpoints/BffApiAttribute.cs b/src/Duende.Bff/Endpoints/BffApiAttribute.cs index 601d0cc..12bffa3 100644 --- a/src/Duende.Bff/Endpoints/BffApiAttribute.cs +++ b/src/Duende.Bff/Endpoints/BffApiAttribute.cs @@ -6,23 +6,10 @@ namespace Duende.Bff; /// -/// Decorates a controller as a local BFF API endpoint -/// This allows the BFF midleware to automatically add the antiforgery header checks as well as 302 to 401 conversion +/// Decorates a controller or action as a local BFF API endpoint +/// By default, this provides anti-forgery protection and response handling. /// [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)] -public class BffApiAttribute : Attribute +public class BffApiAttribute : Attribute, IBffApiEndpoint { - /// - /// specifies if anti-forgery check is required - /// - public bool RequireAntiForgeryCheck { get; } - - /// - /// ctor - /// - /// Specifies if the antiforgery header gets checked - public BffApiAttribute(bool requireAntiForgeryCheck = true) - { - RequireAntiForgeryCheck = requireAntiForgeryCheck; - } -} \ No newline at end of file +} diff --git a/src/Duende.Bff/Endpoints/BffApiSkipAntiforgeryAttribute.cs b/src/Duende.Bff/Endpoints/BffApiSkipAntiforgeryAttribute.cs new file mode 100644 index 0000000..104f673 --- /dev/null +++ b/src/Duende.Bff/Endpoints/BffApiSkipAntiforgeryAttribute.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 ignore the antiforgery header checks. +/// +[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)] +public class BffApiSkipAntiforgeryAttribute : Attribute, IBffApiSkipAntiforgry +{ +} \ No newline at end of file diff --git a/src/Duende.Bff/Endpoints/BffAuthorizationMiddlewareResultHandler.cs b/src/Duende.Bff/Endpoints/BffAuthorizationMiddlewareResultHandler.cs index 9c7ab6c..ae38eca 100644 --- a/src/Duende.Bff/Endpoints/BffAuthorizationMiddlewareResultHandler.cs +++ b/src/Duende.Bff/Endpoints/BffAuthorizationMiddlewareResultHandler.cs @@ -32,25 +32,21 @@ public Task HandleAsync(RequestDelegate next, HttpContext context, Authorization var endpoint = context.GetEndpoint(); if (endpoint == null) throw new ArgumentNullException(nameof(endpoint)); - - var remoteApi = endpoint.Metadata.GetMetadata(); - var localApi = endpoint.Metadata.GetMetadata(); - if (remoteApi == null && localApi == null) + var isBffEndpoint = endpoint.Metadata.GetMetadata() != null; + if (isBffEndpoint) { - return _handler.HandleAsync(next, context, policy, authorizeResult); - } - - if (authorizeResult.Challenged) - { - 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; + } } return _handler.HandleAsync(next, context, policy, authorizeResult); diff --git a/src/Duende.Bff/Endpoints/BffRemoteApiEndpointMetadata.cs b/src/Duende.Bff/Endpoints/BffRemoteApiEndpointMetadata.cs index 8d6a404..f1cc662 100644 --- a/src/Duende.Bff/Endpoints/BffRemoteApiEndpointMetadata.cs +++ b/src/Duende.Bff/Endpoints/BffRemoteApiEndpointMetadata.cs @@ -6,7 +6,7 @@ namespace Duende.Bff; /// /// Endpoint metadata for a remote BFF API endpoint /// -public class BffRemoteApiEndpointMetadata +public class BffRemoteApiEndpointMetadata : IBffApiEndpoint { /// /// Required token type (if any) @@ -18,11 +18,6 @@ public class BffRemoteApiEndpointMetadata /// public bool OptionalUserToken { get; set; } - /// - /// Require presence of anti-forgery header - /// - public bool RequireAntiForgeryHeader { get; set; } = true; - /// /// Maps to UserAccessTokenParameters and included if set /// diff --git a/src/Duende.Bff/Endpoints/EndpointConventionBuilderExtensions.cs b/src/Duende.Bff/Endpoints/EndpointConventionBuilderExtensions.cs index c455244..8cbc0f4 100644 --- a/src/Duende.Bff/Endpoints/EndpointConventionBuilderExtensions.cs +++ b/src/Duende.Bff/Endpoints/EndpointConventionBuilderExtensions.cs @@ -15,10 +15,19 @@ public static class EndpointConventionBuilderExtensions /// This metadata is used by the BFF middleware. /// /// - /// Specifies if the antiforgery header gets checked /// - public static IEndpointConventionBuilder AsBffApiEndpoint(this IEndpointConventionBuilder builder, bool requireAntiForgeryCheck = true) + public static IEndpointConventionBuilder AsBffApiEndpoint(this IEndpointConventionBuilder builder) { - return builder.WithMetadata(new BffApiAttribute(requireAntiForgeryCheck)); + return builder.WithMetadata(new BffApiAttribute()); + } + + /// + /// Adds marker that will cause the BFF framework to skip all antiforgery for this endpoint. + /// + /// + /// + public static IEndpointConventionBuilder SkipAntiforgery(this IEndpointConventionBuilder builder) + { + return builder.WithMetadata(new BffApiSkipAntiforgeryAttribute()); } } \ No newline at end of file diff --git a/src/Duende.Bff/Endpoints/IBffApiEndpoint.cs b/src/Duende.Bff/Endpoints/IBffApiEndpoint.cs new file mode 100644 index 0000000..463ad84 --- /dev/null +++ b/src/Duende.Bff/Endpoints/IBffApiEndpoint.cs @@ -0,0 +1,12 @@ +// Copyright (c) Duende Software. All rights reserved. +// See LICENSE in the project root for license information. + +namespace Duende.Bff; + +/// +/// Marks an endpoint as BFF API endpoint. +/// By default, this provides anti-forgery protection and response handling. +/// +public interface IBffApiEndpoint +{ +} diff --git a/src/Duende.Bff/Endpoints/IBffApiSkipAntiforgry.cs b/src/Duende.Bff/Endpoints/IBffApiSkipAntiforgry.cs new file mode 100644 index 0000000..92b49ef --- /dev/null +++ b/src/Duende.Bff/Endpoints/IBffApiSkipAntiforgry.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 ignore the antiforgery header checks. +/// +public interface IBffApiSkipAntiforgry +{ +} diff --git a/test/Duende.Bff.Tests/TestHosts/BffHost.cs b/test/Duende.Bff.Tests/TestHosts/BffHost.cs index 3e38519..3900f7c 100644 --- a/test/Duende.Bff.Tests/TestHosts/BffHost.cs +++ b/test/Duende.Bff.Tests/TestHosts/BffHost.cs @@ -205,7 +205,8 @@ private void Configure(IApplicationBuilder app) context.Response.ContentType = "application/json"; await context.Response.WriteAsync(JsonSerializer.Serialize(response)); }) - .AsBffApiEndpoint(requireAntiForgeryCheck: false); + .AsBffApiEndpoint() + .SkipAntiforgery(); endpoints.Map("/local_authz", async context => { @@ -271,7 +272,8 @@ private void Configure(IApplicationBuilder app) await context.Response.WriteAsync(JsonSerializer.Serialize(response)); }) .RequireAuthorization() - .AsBffApiEndpoint(requireAntiForgeryCheck: false); + .AsBffApiEndpoint() + .SkipAntiforgery(); endpoints.Map("/always_fail_authz_non_bff_endpoint", context => { return Task.CompletedTask; }) @@ -286,7 +288,8 @@ private void Configure(IApplicationBuilder app) .RequireAccessToken(); endpoints.MapRemoteBffApiEndpoint( - "/api_user_no_csrf", _apiHost.Url(), requireAntiForgeryCheck: false) + "/api_user_no_csrf", _apiHost.Url()) + .SkipAntiforgery() .RequireAccessToken(); endpoints.MapRemoteBffApiEndpoint( From ee3e9c1f299dc1711944da325af077c7e013d21b Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Fri, 24 Jun 2022 09:35:48 -0400 Subject: [PATCH 2/2] rename some tests for clarity --- test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs b/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs index 42edc94..2461904 100644 --- a/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs +++ b/test/Duende.Bff.Tests/Endpoints/LocalEndpointTests.cs @@ -34,7 +34,7 @@ public async Task calls_to_authorized_local_endpoint_should_succeed() } [Fact] - public async Task calls_to_authorized_local_endpoint_without_csrf_should_succeed() + public async Task calls_to_authorized_local_endpoint_without_csrf_should_succeed_without_antiforgery_header() { await BffHost.BffLoginAsync("alice"); @@ -61,7 +61,7 @@ public async Task unauthenticated_calls_to_authorized_local_endpoint_should_fail } [Fact] - public async Task calls_to_local_endpoint_should_require_csrf() + public async Task calls_to_local_endpoint_should_require_antiforgery_header() { var req = new HttpRequestMessage(HttpMethod.Get, BffHost.Url("/local_anon")); var response = await BffHost.BrowserClient.SendAsync(req); @@ -70,7 +70,7 @@ public async Task calls_to_local_endpoint_should_require_csrf() } [Fact] - public async Task calls_to_local_endpoint_without_antiforgery_should_not_require_csrf() + public async Task calls_to_local_endpoint_without_csrf_should_not_require_antiforgery_header() { var req = new HttpRequestMessage(HttpMethod.Get, BffHost.Url("/local_anon_no_csrf")); var response = await BffHost.BrowserClient.SendAsync(req);