Update static files sample to use AuthorizationMiddleware

[JamesNK]

re: aspnet/Security#1894 (comment)

S P I C Y

[JamesNK]

Note: ReturnUrl is messed up because of aspnet/Security#1730 😢

[Tratcher]

Luckily aspnet/Security#1730 is getting fixed in aspnet/Security#1887

[HaoK]

I don't really like creating a stand alone AuthorizationOptions which is detached from the normal AuthorizationOptions that someone might have configured.

What if this was branch.UseAuthorization("AuthenticatedFilesPolicy") which has the middleware automatically combine the policy specified before doing Authorize, but doesn't use a decoupled options instance.

So each instance of the middleware sorta behaves like an Authorize attribute, where you can add additional requirements.

[JamesNK]

🆙 📅

I refactored the sample to rely more on setting an endpoint with metadata and less on branching and having different AuthZ policies on different branches.

There is a single point of authorization and the sample longer needs a UseAuthorization(options) overload or RequiredPolicy.

I think this is better way of doing it.

[HaoK]

Yeah this looks much better!

[Tratcher]

Condense: new EndpointMetadataCollection(fileSystemInfo, new AuthorizeAttribute(policy)),

[Tratcher]

null? Otherwise this breaks 404.
It's a bit awkward to use Endpoints for auth without using them to actually handle the request.

[JamesNK]

It doesn't break 404 because an endpoint is only being created for a file, and files are always being return by UseFileSystem middleware.

The endpoint request delegate is never being called, but if it was then returning a null would NRE when it is awaited.

[Tratcher]

It should never be awaited. If it matches a file then this would never get run. If it doesn't match a file then calling this would be wrong.
https://github.com/aspnet/HttpAbstractions/pull/1059/files#diff-3376870948b08e8c913217b7dab1bdcbR86

[Tratcher]

Better yet, since we don't expect it to get called it should throw InvalidOperationException.

[Tratcher]

Callers shouldn't have to create this dummy type. See the comments about Get/SetEndpoint() extensions.

[Tratcher]

This warrants a SetEndpoint extension method that creates the dummy feature for you internally. We do this for other features already. And while you're at it add the matching GetEndpoint extension.

[JamesNK]

Good suggestion! How would SetEndpoint work if there is already an IEndpointFeature set? Would it update the Endpoint property and leave the same feature, or would it always set a new feature with endpoint.

I ask because the implementation that routing sets implements multiple feature interfaces, e.g. IEndpointFeature and IRouteValuesFeature and IRouterFeature (for legacy).

[Tratcher]

I would not expect it to replace the feature implementation, only the endpoint. The features need to function independently.

[Tratcher]

This two stage process seems unnecessary.

app.Map("/MapImperativeFiles", branch => {
  branch.Use((context, next) => { SetFileEndpoint(context, files, "files"); return next(); })
  branch.UseAuthorization();
  SetupFileServer(branch, files);
});
app.Map("/MapAuthenticatedFiles", branch => {
  branch.Use((context, next) => { SetFileEndpoint(context, files, null); return next(); })
  branch.UseAuthorization();
  SetupFileServer(branch, files);
});

Which could be consolidated to:

void MapFileAuth(IApplicationBuilder app, string path, IFileProvider files, string policy)
{
app.Map(path, branch => {
  branch.Use((context, next) => { SetFileEndpoint(context, files, policy); return next(); })
  branch.UseAuthorization();
  SetupFileServer(branch, files);
});
}

MapFileAuth(app, "/MapImperativeFiles", files, "files");
MapFileAuth(app, "/MapAuthenticatedFiles", files, null);

[JamesNK]

Hmmmm, I prefer the current way it is working. There is a single point of authorization for all requests.

Do you not the process of calculating and setting an endpoint for a file? After I updated this sample I raised an issue about making this more of a first class citizen here - https://github.com/aspnet/Routing/issues/903

I'd like it to look something like:

// For files sitting in wwwroot
app.UseStaticFiles();

app.UseEndpointRouting(routes =>
{
    // MVC
    routes.MapController(name: "default", template: "{controller=Home}/{action=Index}/{id?}");

    // Static file endpoints for files in non-standard locations
    routes.MapStaticFiles("/MapImperativeFiles").RequireAuthorization("files");
    routes.MapStaticFiles("/MapAuthenticatedFiles").RequireAuthorization();
});

app.UseAuthorization();

I don't think this sample would be able to become quite this clean. The branching, and different URLs pointed at the same files makes things messy. Have you considered not having branching in this sample? I think it would make it more real world if there was a set of directories that required auth, and another set that required auth from a specific person.

// @rynowak

[Tratcher]

Having a single UseAuthorization per branch makes sense, but don't artificially bend the pipeline back on itself just to reduce the calls to UseAuthorization. You're effectively duplicating the Map code before and after UseAuthorization when it's a lot cleaner not to.

[Tratcher]

I do like where aspnet/Routing#903 is going, but we're not there yet.

[Tratcher]

I agree the overlapping file structure is not realistic, but the branching is.

[natemcmaster]

This repo is going to be archived. AuthSamples are now part of aspnet/AspNetCore.

cref dotnet/aspnetcore#4088