From 11061e412e81dcb3fb1b8cd63971d771306265cc Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 30 May 2019 13:27:09 -0700 Subject: [PATCH] EndpointMiddleware does not invoke endpoint (#10650) * EndpointMiddleware does not invoke endpoint Fixes: #10649 The bug is that the endpoint delegate is not invoked when SuppressCheckForUnhandledSecurityMetadata is set to true. This option is provided so that can you suppress the security checks done by routing, but right now what it does is suppress the entire middleware. We had tests for the supression cases, but they didn't actually validate that the middleware did any work, just that we don't throw. Fixed that. * harden tests --- src/Http/Routing/src/EndpointMiddleware.cs | 25 ++- .../test/UnitTests/EndpointMiddlewareTest.cs | 172 +++++++++++++----- 2 files changed, 134 insertions(+), 63 deletions(-) diff --git a/src/Http/Routing/src/EndpointMiddleware.cs b/src/Http/Routing/src/EndpointMiddleware.cs index 97e56a04e7..b481c1bdeb 100644 --- a/src/Http/Routing/src/EndpointMiddleware.cs +++ b/src/Http/Routing/src/EndpointMiddleware.cs @@ -36,22 +36,19 @@ namespace Microsoft.AspNetCore.Routing var endpoint = httpContext.Features.Get()?.Endpoint; if (endpoint?.RequestDelegate != null) { - if (_routeOptions.SuppressCheckForUnhandledSecurityMetadata) + if (!_routeOptions.SuppressCheckForUnhandledSecurityMetadata) { - // User opted out of this check. - return Task.CompletedTask; - } + if (endpoint.Metadata.GetMetadata() != null && + !httpContext.Items.ContainsKey(AuthorizationMiddlewareInvokedKey)) + { + ThrowMissingAuthMiddlewareException(endpoint); + } - if (endpoint.Metadata.GetMetadata() != null && - !httpContext.Items.ContainsKey(AuthorizationMiddlewareInvokedKey)) - { - ThrowMissingAuthMiddlewareException(endpoint); - } - - if (endpoint.Metadata.GetMetadata() != null && - !httpContext.Items.ContainsKey(CorsMiddlewareInvokedKey)) - { - ThrowMissingCorsMiddlewareException(endpoint); + if (endpoint.Metadata.GetMetadata() != null && + !httpContext.Items.ContainsKey(CorsMiddlewareInvokedKey)) + { + ThrowMissingCorsMiddlewareException(endpoint); + } } Log.ExecutingEndpoint(_logger, endpoint); diff --git a/src/Http/Routing/test/UnitTests/EndpointMiddlewareTest.cs b/src/Http/Routing/test/UnitTests/EndpointMiddlewareTest.cs index dc7dc3d87c..1467425222 100644 --- a/src/Http/Routing/test/UnitTests/EndpointMiddlewareTest.cs +++ b/src/Http/Routing/test/UnitTests/EndpointMiddlewareTest.cs @@ -25,8 +25,10 @@ namespace Microsoft.AspNetCore.Routing var httpContext = new DefaultHttpContext(); httpContext.RequestServices = new ServiceProvider(); + var calledNext = false; RequestDelegate next = (c) => { + calledNext = true; return Task.CompletedTask; }; @@ -35,7 +37,8 @@ namespace Microsoft.AspNetCore.Routing // Act await middleware.Invoke(httpContext); - // Assert - does not throw + // Assert + Assert.True(calledNext); } [Fact] @@ -46,37 +49,10 @@ namespace Microsoft.AspNetCore.Routing httpContext.RequestServices = new ServiceProvider(); httpContext.SetEndpoint(null); + var calledNext = false; RequestDelegate next = (c) => { - return Task.CompletedTask; - }; - - var middleware = new EndpointMiddleware(NullLogger.Instance, next, RouteOptions); - - // Act - await middleware.Invoke(httpContext); - - // Assert - does not throw - } - - [Fact] - public async Task Invoke_WithEndpoint_InvokesDelegate() - { - // Arrange - var httpContext = new DefaultHttpContext(); - httpContext.RequestServices = new ServiceProvider(); - - var invoked = false; - RequestDelegate endpointFunc = (c) => - { - invoked = true; - return Task.CompletedTask; - }; - - httpContext.SetEndpoint(new Endpoint(endpointFunc, EndpointMetadataCollection.Empty, "Test")); - - RequestDelegate next = (c) => - { + calledNext = true; return Task.CompletedTask; }; @@ -86,7 +62,37 @@ namespace Microsoft.AspNetCore.Routing await middleware.Invoke(httpContext); // Assert - Assert.True(invoked); + Assert.True(calledNext); + } + + [Fact] + public async Task Invoke_WithEndpoint_InvokesDelegate() + { + // Arrange + var httpContext = new DefaultHttpContext(); + httpContext.RequestServices = new ServiceProvider(); + + var calledEndpoint = false; + RequestDelegate endpointFunc = (c) => + { + calledEndpoint = true; + return Task.CompletedTask; + }; + + httpContext.SetEndpoint(new Endpoint(endpointFunc, EndpointMetadataCollection.Empty, "Test")); + + RequestDelegate next = (c) => + { + throw new InvalidTimeZoneException("Should not be called"); + }; + + var middleware = new EndpointMiddleware(NullLogger.Instance, next, RouteOptions); + + // Act + await middleware.Invoke(httpContext); + + // Assert + Assert.True(calledEndpoint); } [Fact] @@ -101,9 +107,14 @@ namespace Microsoft.AspNetCore.Routing RequestServices = new ServiceProvider() }; - httpContext.SetEndpoint(new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(Mock.Of()), "Test")); + RequestDelegate throwIfCalled = (c) => + { + throw new InvalidTimeZoneException("Should not be called"); + }; - var middleware = new EndpointMiddleware(NullLogger.Instance, _ => Task.CompletedTask, RouteOptions); + httpContext.SetEndpoint(new Endpoint(throwIfCalled, new EndpointMetadataCollection(Mock.Of()), "Test")); + + var middleware = new EndpointMiddleware(NullLogger.Instance, throwIfCalled, RouteOptions); // Act & Assert var ex = await Assert.ThrowsAsync(() => middleware.Invoke(httpContext)); @@ -121,16 +132,29 @@ namespace Microsoft.AspNetCore.Routing RequestServices = new ServiceProvider() }; - httpContext.SetEndpoint(new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(Mock.Of()), "Test")); + var calledEndpoint = false; + RequestDelegate endpointFunc = (c) => + { + calledEndpoint = true; + return Task.CompletedTask; + }; + + httpContext.SetEndpoint(new Endpoint(endpointFunc, new EndpointMetadataCollection(Mock.Of()), "Test")); httpContext.Items[EndpointMiddleware.AuthorizationMiddlewareInvokedKey] = true; - var middleware = new EndpointMiddleware(NullLogger.Instance, _ => Task.CompletedTask, RouteOptions); + RequestDelegate next = (c) => + { + throw new InvalidTimeZoneException("Should not be called"); + }; - // Act & Assert + var middleware = new EndpointMiddleware(NullLogger.Instance, next, RouteOptions); + + // Act await middleware.Invoke(httpContext); - // If we got this far, we can sound the everything's OK alarm. + // Assert + Assert.True(calledEndpoint); } [Fact] @@ -142,13 +166,29 @@ namespace Microsoft.AspNetCore.Routing RequestServices = new ServiceProvider() }; - httpContext.SetEndpoint(new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(Mock.Of()), "Test")); + var calledEndpoint = false; + RequestDelegate endpointFunc = (c) => + { + calledEndpoint = true; + return Task.CompletedTask; + }; + + httpContext.SetEndpoint(new Endpoint(endpointFunc, new EndpointMetadataCollection(Mock.Of()), "Test")); var routeOptions = Options.Create(new RouteOptions { SuppressCheckForUnhandledSecurityMetadata = true }); - var middleware = new EndpointMiddleware(NullLogger.Instance, _ => Task.CompletedTask, routeOptions); - // Act & Assert + RequestDelegate next = (c) => + { + throw new InvalidTimeZoneException("Should not be called"); + }; + + var middleware = new EndpointMiddleware(NullLogger.Instance, next, routeOptions); + + // Act await middleware.Invoke(httpContext); + + // Assert + Assert.True(calledEndpoint); } [Fact] @@ -163,9 +203,14 @@ namespace Microsoft.AspNetCore.Routing RequestServices = new ServiceProvider() }; - httpContext.SetEndpoint(new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(Mock.Of()), "Test")); + RequestDelegate throwIfCalled = (c) => + { + throw new InvalidTimeZoneException("Should not be called"); + }; - var middleware = new EndpointMiddleware(NullLogger.Instance, _ => Task.CompletedTask, RouteOptions); + httpContext.SetEndpoint(new Endpoint(throwIfCalled, new EndpointMetadataCollection(Mock.Of()), "Test")); + + var middleware = new EndpointMiddleware(NullLogger.Instance, throwIfCalled, RouteOptions); // Act & Assert var ex = await Assert.ThrowsAsync(() => middleware.Invoke(httpContext)); @@ -183,16 +228,29 @@ namespace Microsoft.AspNetCore.Routing RequestServices = new ServiceProvider() }; - httpContext.SetEndpoint(new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(Mock.Of()), "Test")); + var calledEndpoint = false; + RequestDelegate endpointFunc = (c) => + { + calledEndpoint = true; + return Task.CompletedTask; + }; + + httpContext.SetEndpoint(new Endpoint(endpointFunc, new EndpointMetadataCollection(Mock.Of()), "Test")); httpContext.Items[EndpointMiddleware.CorsMiddlewareInvokedKey] = true; - var middleware = new EndpointMiddleware(NullLogger.Instance, _ => Task.CompletedTask, RouteOptions); + RequestDelegate next = (c) => + { + throw new InvalidTimeZoneException("Should not be called"); + }; - // Act & Assert + var middleware = new EndpointMiddleware(NullLogger.Instance, next, RouteOptions); + + // Act await middleware.Invoke(httpContext); - // If we got this far, we can sound the everything's OK alarm. + // Assert + Assert.True(calledEndpoint); } [Fact] @@ -204,13 +262,29 @@ namespace Microsoft.AspNetCore.Routing RequestServices = new ServiceProvider() }; - httpContext.SetEndpoint(new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(Mock.Of()), "Test")); + var calledEndpoint = false; + RequestDelegate endpointFunc = (c) => + { + calledEndpoint = true; + return Task.CompletedTask; + }; + + httpContext.SetEndpoint(new Endpoint(endpointFunc, new EndpointMetadataCollection(Mock.Of()), "Test")); var routeOptions = Options.Create(new RouteOptions { SuppressCheckForUnhandledSecurityMetadata = true }); - var middleware = new EndpointMiddleware(NullLogger.Instance, _ => Task.CompletedTask, routeOptions); - // Act & Assert + RequestDelegate next = (c) => + { + throw new InvalidTimeZoneException("Should not be called"); + }; + + var middleware = new EndpointMiddleware(NullLogger.Instance, next, routeOptions); + + // Act await middleware.Invoke(httpContext); + + // Assert + Assert.True(calledEndpoint); } private class ServiceProvider : IServiceProvider