Make AuthorizeFilter constructable #5274
Conversation
| /// Initializes a new instance of <see cref="AuthorizeFilter"/>. | ||
| /// </summary> | ||
| /// <param name="authorizeData">The <see cref="IAuthorizeData"/> to combine into an <see cref="IAuthorizeData"/>.</param> | ||
| public AuthorizeFilter(IEnumerable<IAuthorizeData> authorizeData) |
There was a problem hiding this comment.
should we add one more constructor which takes in a policy name for convenience? (probably other properties like roles and active authentication schemes too)
There was a problem hiding this comment.
Not super sure we need that. The motive for this bug wasn't to make it simplify the ctor but more about making it possible to add AuthFilter as a global filter.
There was a problem hiding this comment.
It would be super nice to have the policy name as a ctor. That will be the most common way to use this as a global filter.
pranavkm
force-pushed
the
prkrishn/5254
branch
from
19c1130 to
69d87b7
Compare
| @@ -61,18 +61,18 @@ public void OnProvidersExecuting(ApplicationModelProviderContext context) | |||
| } | |||
| } | |||
|
|
|||
| private AuthorizeFilter GetFilter(IEnumerable<IAuthorizeData> authData) | |||
| public static AuthorizeFilter GetFilter(IAuthorizationPolicyProvider policyProvider, IEnumerable<IAuthorizeData> authData) | |||
There was a problem hiding this comment.
Ideally this would be a static factory method on AuthorizeFilter, but this API should go away eventually, so lets keep it where it is.
There was a problem hiding this comment.
I thought as much. It's kinda awful.
|
⌚ for a ctor that takes a policy name |
| .BuildServiceProvider(); | ||
|
|
||
| // Act | ||
| var result = factory.CreateInstance(serviceProvider); |
There was a problem hiding this comment.
could you make one more call and the result should be same
There was a problem hiding this comment.
What would be the same?
There was a problem hiding this comment.
I mean the filter instance on one more call to CreateInstance should create the same instance of the filter (I ask this because the other tests here short circuit in the if condition and do not go through DI)
There was a problem hiding this comment.
I don't think CreateInstance has to return the same instance every time. The filter provider should cache the value for you as long as the filter is reusable.
|
Updated |
|
|
1 similar comment
|
|
Fixes #5253