Update static files sample to use AuthorizationMiddleware #65
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,16 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.IO; | ||
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.AspNetCore.Authentication; | ||
| using Microsoft.AspNetCore.Authentication.Cookies; | ||
| using Microsoft.AspNetCore.Authorization; | ||
| using Microsoft.AspNetCore.Builder; | ||
| using Microsoft.AspNetCore.Hosting; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Http.Features; | ||
| using Microsoft.AspNetCore.Mvc; | ||
| using Microsoft.Extensions.Configuration; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
|
|
@@ -49,27 +53,36 @@ public void ConfigureServices(IServiceCollection services) | |
| { | ||
| return false; | ||
| } | ||
| if (context.Resource is Endpoint endpoint) | ||
| { | ||
| var userPath = Path.Combine(usersPath, userName); | ||
|
|
||
| var file = endpoint.Metadata.GetMetadata<IFileInfo>(); | ||
| if (file != null) | ||
| { | ||
| var path = Path.GetDirectoryName(file.PhysicalPath); | ||
| return string.Equals(path, basePath, StringComparison.OrdinalIgnoreCase) | ||
| || string.Equals(path, usersPath, StringComparison.OrdinalIgnoreCase) | ||
| || string.Equals(path, userPath, StringComparison.OrdinalIgnoreCase) | ||
| || path.StartsWith(userPath + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase); | ||
| } | ||
|
|
||
| var dir = endpoint.Metadata.GetMetadata<IDirectoryContents>(); | ||
| if (dir != null) | ||
| { | ||
| // https://github.com/aspnet/Home/issues/3073 | ||
| // This won't work right if the directory is empty | ||
| var path = Path.GetDirectoryName(dir.First().PhysicalPath); | ||
| return string.Equals(path, basePath, StringComparison.OrdinalIgnoreCase) | ||
| || string.Equals(path, usersPath, StringComparison.OrdinalIgnoreCase) | ||
| || string.Equals(path, userPath, StringComparison.OrdinalIgnoreCase) | ||
| || path.StartsWith(userPath + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase); | ||
| } | ||
|
|
||
| throw new InvalidOperationException($"Missing file system metadata."); | ||
| } | ||
|
|
||
| throw new InvalidOperationException($"Unknown resource type '{context.Resource.GetType()}'"); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
@@ -96,16 +109,27 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env, IAuthori | |
|
|
||
| var files = new PhysicalFileProvider(Path.Combine(env.ContentRootPath, "PrivateFiles")); | ||
|
|
||
| app.Use(async (context, next) => | ||
| { | ||
| // Set an endpoint for files inside PrivateFiles to run authorization | ||
| PathString remaining; | ||
| if (context.Request.Path.StartsWithSegments("/MapImperativeFiles", out remaining)) | ||
| { | ||
| SetFileEndpoint(context, files, remaining, "files"); | ||
| } | ||
| else if (context.Request.Path.StartsWithSegments("/MapAuthenticatedFiles", out remaining)) | ||
| { | ||
| SetFileEndpoint(context, files, remaining, null); | ||
| } | ||
|
|
||
| await next(); | ||
| }); | ||
|
|
||
| app.UseAuthorization(); | ||
|
|
||
| app.Map("/MapAuthenticatedFiles", branch => SetupFileServer(branch, files)); | ||
| app.Map("/MapImperativeFiles", branch => SetupFileServer(branch, files)); | ||
|
|
||
| app.UseMvc(routes => | ||
| { | ||
| routes.MapRoute( | ||
|
|
@@ -114,81 +138,61 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env, IAuthori | |
| }); | ||
| } | ||
|
|
||
| private void SetupFileServer(IApplicationBuilder builder, IFileProvider files) | ||
| { | ||
| builder.UseFileServer(new FileServerOptions() | ||
| { | ||
| EnableDirectoryBrowsing = true, | ||
| FileProvider = files | ||
| }); | ||
| } | ||
|
|
||
| private static void SetFileEndpoint(HttpContext context, PhysicalFileProvider files, PathString filePath, string policy) | ||
| { | ||
| var fileSystemInfo = GetFileSystemInfo(files, filePath); | ||
| if (fileSystemInfo != null) | ||
| { | ||
| var metadata = new List<object>(); | ||
| metadata.Add(fileSystemInfo); | ||
| metadata.Add(new AuthorizeAttribute(policy)); | ||
|
|
||
| var endpoint = new Endpoint( | ||
| c => Task.CompletedTask, | ||
|
There was a problem hiding this comment. null? Otherwise this breaks 404. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. Better yet, since we don't expect it to get called it should throw InvalidOperationException. |
||
| new EndpointMetadataCollection(metadata), | ||
|
There was a problem hiding this comment. Condense: |
||
| context.Request.Path); | ||
|
|
||
| context.Features.Set<IEndpointFeature>(new EndpointFeature(endpoint)); | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. Good suggestion! How would I ask because the implementation that routing sets implements multiple feature interfaces, e.g. There was a problem hiding this comment. I would not expect it to replace the feature implementation, only the endpoint. The features need to function independently. |
||
| } | ||
| } | ||
|
|
||
| private static object GetFileSystemInfo(PhysicalFileProvider files, string path) | ||
| { | ||
| var fileInfo = files.GetFileInfo(path); | ||
| if (fileInfo.Exists) | ||
| { | ||
| return fileInfo; | ||
| } | ||
| else | ||
| { | ||
| // https://github.com/aspnet/Home/issues/2537 | ||
| var dir = files.GetDirectoryContents(path); | ||
| if (dir.Exists) | ||
| { | ||
| return dir; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private class EndpointFeature : IEndpointFeature | ||
|
There was a problem hiding this comment. Callers shouldn't have to create this dummy type. See the comments about Get/SetEndpoint() extensions. |
||
| { | ||
| public Endpoint Endpoint { get; set; } | ||
|
|
||
| public EndpointFeature(Endpoint endpoint) | ||
| { | ||
| Endpoint = endpoint; | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This two stage process seems unnecessary.
Which could be consolidated to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like where aspnet/Routing#903 is going, but we're not there yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the overlapping file structure is not realistic, but the branching is.