Skip to content
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.

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

[API Review] Support SkipStatusCodePages on endpoints and authorized routes #38573

Closed
captainsafia opened this issue Nov 22, 2021 · 7 comments
Closed
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks Docs This issue tracks updating documentation
Milestone

Comments

@captainsafia
Copy link
Member

Background and Motivation

#10317 identifies an issue where the behavior of the AuthorizationMiddleware circumvents the filter logic in SkipStatusCodeAttribute and results in the StatusCodePageMiddleware being executed when it shouldn't and masking the underlying 401 from the AuthorizationMiddleware.

This PR attempts to resolve this issue by removing the dependency on the IFilter execution order from the SkipStatusCodeAttribute by introducing an ISkipStatusCodesMetadata interface.

Proposed API

namespace Microsoft.AspNetCore.Http.Metadata;

public interface ISkipStatusCodePagesMetadata
{
    bool Enabled { get; }
}
- public class SkipStatusCodePagesAttribute : Attribute, IResourceFilter
+ public class SkipStatusCodePagesAttribute : Attribute, IResourceFilter, ISkipStatusCodePagesMetadata
{
+  public bool Enabled { get; }
}

Usage Examples

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

PR located at #38509

@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-web-frameworks labels Nov 22, 2021
@ghost
Copy link

ghost commented Nov 22, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@pranavkm
Copy link
Contributor

API review:

Approved. We can remove the Enabled property and make this a vanilla

namespace Microsoft.AspNetCore.Http.Metadata;

+ public interface ISkipStatusCodePagesMetadata
+ {
+ }

namespace Microsoft.AspNetCore.Mvc;

- public class SkipStatusCodePagesAttribute : Attribute, IResourceFilter
+ public class SkipStatusCodePagesAttribute : Attribute, IResourceFilter, ISkipStatusCodePagesMetadata
{
}

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Nov 22, 2021
@Kahbazi
Copy link
Member

Kahbazi commented Nov 22, 2021

@pranavkm I'm curios, why remove the Enable property and go against the routing guideline.

DO make it possible to override metadata

The best way to follow these guidelines is to avoid defining marker metadata:

  • Don't just look for the presence of a metadata type.
  • Define a property on the metadata and check the property.

@pranavkm
Copy link
Contributor

@Kahbazi we haven't seen any asks from users to want to toggle the feature which is in when we think a Enabled property would make sense. Our plan was to fix this with a narrow API that replicates the behavior of the MVC filter by turning it in to metadata.

@rafikiassumani-msft rafikiassumani-msft added this to the 7.0-preview1 milestone Nov 22, 2021
@captainsafia
Copy link
Member Author

Closing as completed.

@Kahbazi
Copy link
Member

Kahbazi commented Jan 10, 2022

@captainsafia Just curious. Is there any follow up issue for #38509 (comment)?

@halter73
Copy link
Member

@Kahbazi Here's a follow up issue: #39472

@BrennanConroy BrennanConroy added the Docs This issue tracks updating documentation label Feb 3, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks Docs This issue tracks updating documentation
Projects
None yet
Development

No branches or pull requests

6 participants