Integrate AuthorizeView with actual authorization #10487
Conversation
|
|
||
| namespace Microsoft.AspNetCore.Blazor.Services | ||
| { | ||
| internal class WebAssemblyConsoleLogger<T> : ILogger<T> |
There was a problem hiding this comment.
This is intended as a minimal way of meeting the requirements for the built-in default IAuthorizationService, without bringing in the full-fledged Microsoft.AspNetCore.Extensions.Logging.
The full logging package adds about 100KB to the app size and a couple extra assembly downloads, so if we can have a bare-bones one without it that's preferable. People who want to use the full thing (e.g., to control logging levels, have multiple categories, etc.) can still reference it and add it to their DI config.
There was a problem hiding this comment.
The full logging package adds about 100KB to the app size and a couple extra assembly downloads, so if we can have a bare-bones one without it that's preferable. People who want to use the full thing (e.g., to control logging levels, have multiple categories, etc.) can still reference it and add it to their DI config.
To me this seems like a great thing to chase down in the future when we have a TFM for web assembly. Right now if we wanted to offer a thinner profile of these features for Blazor we have no real way to do so.
| @@ -102,6 +104,10 @@ private void CreateServiceProvider() | |||
| }; | |||
| }); | |||
|
|
|||
| // Needed for authorization | |||
| services.AddOptions(); | |||
| services.TryAdd(ServiceDescriptor.Singleton(typeof(ILogger<>), typeof(WebAssemblyConsoleLogger<>))); | |||
There was a problem hiding this comment.
I think you'll also have to support ILoggerFactory. You're going to find code in our stack that expects that, and it should be easy to add.
There was a problem hiding this comment.
I know it is fairly easy to add, but we don't have any code paths that hit it currently, and I'm trying to be minimal with this. We're always free to add more default services later.
If you have reasons to think it's going to be problematic if we don't, let me know, otherwise I'll leave this as-is.
There was a problem hiding this comment.
I can't really articulate a specific scenario. This just feels lazy - because we know what we would do if people complain. We could prevent them from having to log and issue and wait for us to fix it by just doing a little thing now.
There was a problem hiding this comment.
Yeah, we want to have a similar set of services in both environments, otherwise when I try to use a component that does logging, then I end up having to register it myself, making the experience different in the two environments.
There was a problem hiding this comment.
OK, I've added a super-minimal ILoggerFactory.
Yeah, we want to have a similar set of services in both environments
I'd be cautious about thinking about it like that. The environments have very different capabilities, goals, and characteristics. Most of the server-side services are for server-side application-server concerns, so a large proportion of them won't be present on WebAssembly by design.
| var policy = await AuthorizationPolicy.CombineAsync( | ||
| AuthorizationPolicyProvider, selfAsAuthorizeData); | ||
| var result = await AuthorizationService.AuthorizeAsync(user, Resource, policy); | ||
| return result.Succeeded; |
There was a problem hiding this comment.
This is the same as what DefaultHubDispatcher does, which looks more or less equivalent in terms of its requirements and its position in the stack.
There was a problem hiding this comment.
So just to confirm I'm reading this correctly, selfAsAuthorizeData is total set of authorize attributes for this view that you want to use for the authZ check.
There was a problem hiding this comment.
Yes (except this info doesn’t come from attributes in this case- that’s a separate feature on the Blazor router).
| @@ -10,6 +10,7 @@ | |||
| </PropertyGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <Reference Include="Microsoft.AspNetCore.Authorization" /> | |||
There was a problem hiding this comment.
I don't have a better idea right now, but are we really OK with baking this in at the lowest level. We're saying all components present and future have access to the ASP.NET Authorization library
There was a problem hiding this comment.
I'm thinking about things like a React native experience, and I think the point is that you'd still want access to things like AuthorizeView in that environment. So I don't really feel like it's a problem I guess. ....
There was a problem hiding this comment.
Yep, I agree with both the uneasiness and the conclusion that this is the best outcome currently.
Considering that all the Components/Blazor packages have AspNetCore in their name, it doesn't really change anything to reference Microsoft.AspNetCore.Authorization as compared with if we had Microsoft.AspNetCore.Components.Authorization.
There was a problem hiding this comment.
Yeah, this makes me a bit sad on the inside. I'm wondering if we could use some of the infrastructure for single file publish coming in 3.0 to separate the core of the platform from the baked-in components that we give you into two assemblies and simply ILMerge them at publish time.
from me. it would be good if 
|
TODO Manual ref-assembly code for the new params on |
SteveSandersonMS
force-pushed
the
stevesa/blazor-authorization
branch
from
f14a3f0 to
f63cdce
Compare
ghost
deleted the
No description provided.