Skip to content

Fix endpoints in StaticFileAuth sample #43352

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

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Aug 17, 2022

Fixes #43274

Two changes broke this sample:

Putting this fix in main (8) since there's no product or test impact.

@hylinux

@Tratcher Tratcher self-assigned this Aug 17, 2022
@ghost ghost added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Aug 17, 2022
@Tratcher Tratcher added this to the 8.0-alpha1 milestone Aug 17, 2022
@DamianEdwards
Copy link
Member

DamianEdwards commented Aug 30, 2022

Interesting. Do you think the way this sample implements auth for static files is still idiomatic? Should we consider pointing to something like https://github.com/DamianEdwards/AspNetCorePathAuthorization instead? Also, how does this sample compare to the approach outlined in the docs?

@Tratcher
Copy link
Member Author

The FallbackPolicy approach isn't granular, and the FileResult approach is too granular, you have to map out your whole file structure.

The path authorization approach is a reasonable compromise, are you planning to integrate that for 8?

@DamianEdwards
Copy link
Member

To be clear, you're saying the approaches in the docs are problematic (too broad, too granular)?

The path authorization approach via metadata-only endpoints is workable in .NET 7+. We could consider adding first-class support for such endpoints in .NET 8 (perhaps avoiding the per-request MatcherPolicy and instead modifying the route graph nodes themselves) which would allow efficient path-based rules for AuthZ and possibly output caching too. I'll log an issue to cover that.

@Tratcher
Copy link
Member Author

To be clear, you're saying the approaches in the docs are problematic (too broad, too granular)?

I would call them limited, or not dynamic. Authorizing everything is ok if that's what you want. Authorizing a single known file is ok too. But those samples don't allow any in-between. E.g. you can't restrict who as access to which files without hard coding every file.

I wouldn't want this sample to depend on yours until it was integrated in, it's too much to code people would need to copy.

@Tratcher
Copy link
Member Author

@DamianEdwards @halter73 Can you approve this fix reflecting the current reality and we'll revisit this sample after #43642?


var endpoint = new Endpoint(
c => throw new InvalidOperationException("Static file middleware should return file request."),
requestDelegate: null,
Copy link
Member

Choose a reason for hiding this comment

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

This led me to this:

// Return true because we only want to run if there is no endpoint delegate.
private static bool ValidateNoEndpointDelegate(HttpContext context) => context.GetEndpoint()?.RequestDelegate is null;

Gross! It's actually pretty clever though. Is this anywhere in our real docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not in the docs, that's Damien's 7.0 change that broke this sample.

@Tratcher Tratcher merged commit 5f91f1b into dotnet:main Sep 13, 2022
@Tratcher Tratcher deleted the tratcher/fileauth branch September 13, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

src/security/samples/StaticFilesAuth always not working
3 participants