Skip to content

Support SkipStatusCodePages on endpoints and authorized routes #38509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
captainsafia merged 3 commits into from
Nov 23, 2021
Merged

Support SkipStatusCodePages on endpoints and authorized routes #38509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
40b66e7
Support SkipStatusCodePages on endpoints and authorized routes
captainsafia Nov 18, 2021
6606f0f
Address feedback from API review
captainsafia Nov 22, 2021
b87b551
Fix sort of usings
captainsafia Nov 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Http/Http.Abstractions/src/Metadata/IStatusCodePagesMetadata.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Http.Metadata;

/// <summary>
/// Defines a contract used to specify metadata for skipping the StatusCodePage
/// middleware in <see cref="Endpoint.Metadata"/>.
Copy link
Copy Markdown
Contributor

@pranavkm pranavkm Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we drop the in Endpoint.Metadata? I don't know if it helps clarify things?

Copy link
Copy Markdown
Member

@halter73 halter73 Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the in Endpoint.Metadata. What's wrong with it? It could point the curious to API docs for endpoint routing.

/// </summary>
public interface ISkipStatusCodePagesMetadata
{
}
1 change: 1 addition & 0 deletions src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#nullable enable
*REMOVED*abstract Microsoft.AspNetCore.Http.HttpResponse.ContentType.get -> string!
abstract Microsoft.AspNetCore.Http.HttpResponse.ContentType.get -> string?
Microsoft.AspNetCore.Http.Metadata.ISkipStatusCodePagesMetadata
8 changes: 8 additions & 0 deletions src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Metadata;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.Diagnostics;
Expand Down Expand Up @@ -41,6 +42,13 @@ public async Task Invoke(HttpContext context)
{
var statusCodeFeature = new StatusCodePagesFeature();
context.Features.Set<IStatusCodePagesFeature>(statusCodeFeature);
var endpoint = context.GetEndpoint();
Copy link
Copy Markdown
Member

@Kahbazi Kahbazi Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having context.GetEndpoint(); before _next(context) implies that UseStatusCodePages should be after UseRouting which might not be the case everywhere, including the sample in the docs.

This could be after _next(context) and also only be called if statusCodeFeature.Enabled is true.

Copy link
Copy Markdown
Member

@Tratcher Tratcher Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair consideration, and we would want to make sure the doc was accurate.

Do we have examples of anything else that's endpoint aware but placed before UseRouting?

This ordering was recommended so that the attribute behavior could be applied early and then overridden by other middleware/code while processing the request. That's consistent with how the filter works today. If placed after _next, you wouldn't be able to tell if IStatusCodePagesFeature had the default value or if it was set by something in the app.

Copy link
Copy Markdown
Member

@Kahbazi Kahbazi Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tratcher Sorry, I'm confused a little bit! Are you saying that going forward StatusCodePagesMiddleware should always be after RoutingMiddleware?

Copy link
Copy Markdown
Member

@Tratcher Tratcher Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StatusCodePagesMiddleware can stay where it is and continue to work for existing apps, but if you want it to be route aware and use the SkipStatusCodePages attribute then it will need to be placed after routing.

Copy link
Copy Markdown
Member

@Kahbazi Kahbazi Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see! The implementation has been changed after I add the original comment. It's all clear now.

I assume if StatusCodePagesMiddleware with re-execute would be used after routing it will automatically add the routing, right?

aspnetcore/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesExtensions.cs

Lines 179 to 189 in 4656f15

if (app.Properties.TryGetValue(globalRouteBuilderKey, out var routeBuilder) && routeBuilder is not null)
{
return app.Use(next =>
{
RequestDelegate? newNext = null;
// start a new middleware pipeline
var builder = app.New();
// use the old routing pipeline if it exists so we preserve all the routes and matching logic
// ((IApplicationBuilder)WebApplication).New() does not copy globalRouteBuilderKey automatically like it does for all other properties.
builder.Properties[globalRouteBuilderKey] = routeBuilder;
builder.UseRouting();

What if StatusCodePagesFeature set the Enable property lazily? In this case it doesn't matter where StatusCodePagesMiddleware is as long as the feature is accessed after routing, right?

public class StatusCodePagesFeature : IStatusCodePagesFeature
{
    private readonly HttpContext httpContext;
    public StatusCodePagesFeature(HttpContext httpContext)
    {
        _httpContext = httpContext;
    }

    private bool? _enabled;
    public bool Enabled
    {
        get 
        {
            if (_enabled == null)
            {
                var endpoint = _httpContext.GetEndpoint();
                var skipStatusCodePageMetadata = endpoint?.Metadata.GetMetadata<ISkipStatusCodePagesMetadata>();
                if (skipStatusCodePageMetadata is not null)
                {
                    _enabled = false;
                }
                else
                {
                    _enabled = true;
                }
            }
            
            return _enabled;
        }
        set
        {
            _enabled = value;
        }
    }
}

Copy link
Copy Markdown
Member

@halter73 halter73 Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StatusCodePagesFeature is a public class, so if we were to change it, we'd have to make it continue to work as it does today when called with the empty constructor. I think it might be worthwhile to just use a new internal IStatusCodePagesFeature that lazily reads the endpoint.

The only thing I don't like is locking in the wrong value for Enabled if the property is read before UseRouting(). You could definitely argue it's no worse than today where it's locked in immediately when the feature is added, but that feels a little less surprising. I wonder if it'd be better to avoid negative caching unless the property is explicitly set, but maybe that's even more surprising.

var skipStatusCodePageMetadata = endpoint?.Metadata.GetMetadata<ISkipStatusCodePagesMetadata>();

if (skipStatusCodePageMetadata is not null)
{
statusCodeFeature.Enabled = false;
}

await _next(context);

Expand Down
1 change: 1 addition & 0 deletions src/Middleware/Diagnostics/test/UnitTests/Microsoft.AspNetCore.Diagnostics.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
<Reference Include="Microsoft.Extensions.DiagnosticAdapter" />
<Reference Include="Microsoft.Extensions.FileProviders.Embedded" />
<Reference Include="Microsoft.AspNetCore" />
<Reference Include="Microsoft.AspNetCore.Mvc.ViewFeatures" />
</ItemGroup>

</Project>
35 changes: 35 additions & 0 deletions src/Middleware/Diagnostics/test/UnitTests/StatusCodeMiddlewareTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.TestHost;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
Expand Down Expand Up @@ -283,4 +284,38 @@ public async Task Reexecute_WorksAfterUseRoutingWithGlobalRouteBuilder()
var content = await response.Content.ReadAsStringAsync();
Assert.Equal("errorPage", content);
}

[Fact]
public async Task SkipStatusCodePages_SupportsEndpoints()
Copy link
Copy Markdown
Contributor

@pranavkm pranavkm Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test that verifies we let people change the feature in the body of the endpoint?

endpoints.MapGet("/", [SkipStatusCodePages] (c) =>
{
         c.GetFeature<IStatusCodePagesFeature>().Enabled = false;
});

Copy link
Copy Markdown
Member

@halter73 halter73 Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except verify that you can set it to true! Although it really would be weird to reenable it after adding the attribute.

Copy link
Copy Markdown
Contributor Author

@captainsafia captainsafia Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fair! I missed seeing this before the auto-merge kicked it but I'll address in a follow-up.

{
var builder = WebApplication.CreateBuilder();
builder.WebHost.UseTestServer();
await using var app = builder.Build();

app.UseRouting();

app.UseStatusCodePages();

app.UseEndpoints(endpoints =>
{
endpoints.MapGet("/", [SkipStatusCodePages] (c) =>
{
c.Response.StatusCode = 404;
return Task.CompletedTask;
});
});

app.Run((context) =>
{
throw new InvalidOperationException("Invalid input provided.");
});

await app.StartAsync();

using var server = app.GetTestServer();
var client = server.CreateClient();
var response = await client.GetAsync("/");
var content = await response.Content.ReadAsStringAsync();
Assert.Empty(content);
}
}
3 changes: 2 additions & 1 deletion src/Mvc/Mvc.ViewFeatures/src/SkipStatusCodePagesAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using Microsoft.AspNetCore.Diagnostics;
using Microsoft.AspNetCore.Http.Metadata;
Comment thread
captainsafia marked this conversation as resolved.
using Microsoft.AspNetCore.Mvc.Filters;

namespace Microsoft.AspNetCore.Mvc;
Expand All @@ -11,7 +12,7 @@ namespace Microsoft.AspNetCore.Mvc;
/// A filter that prevents execution of the StatusCodePages middleware.
/// </summary>
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
public class SkipStatusCodePagesAttribute : Attribute, IResourceFilter
public class SkipStatusCodePagesAttribute : Attribute, IResourceFilter, ISkipStatusCodePagesMetadata
{
/// <inheritdoc />
public void OnResourceExecuted(ResourceExecutedContext context)
Expand Down