Make unauthenticated request.
All unauthenticated requests should fail on ".RequireAuthenticatedUser()" requirement and not execute further if "InvokeHandlersAfterFailure" is set to "false"
Every unauthenticated request is executing "RequireAssertion" followed by ".RequireAuthenticatedUser()" requirement when "InvokeHandlersAfterFailure" is set to "false".
aspnetcore/src/Security/Authorization/Core/src/DenyAnonymousAuthorizationRequirement.cs
Lines 20 to 31
8b30d86
@blowdart thanks for a quick reply.
It would be great if you could point me to a documentation where it states that, or make this behavior more obvious.
Based on your statement, if it has to stay as it is,
what would be your recommended approach for custom policies implementation that expect user to be authenticated?
The only way I see it for now is to either build custom "DenyAnonymousAuthorizationRequirement" that will fail authorization or write if (context.User.Identity.IsAuthenticated)
statement in every policy which is cumbersome to my mind.
I'm afraid the cumbersome route is probably best, because remember policy can not only be called via attributes, but via code, and requirements can also be evaluated via code, and that route can be done anywhere, so you can't rely on an authenticated user being filled at that point, only that user isn't null (by default you get an anonymous user if no authn has taken place).
@blowdart I do agree with you.
In that case I would suggest you to revisit the implementation of "AuthorizationOptions.InvokeHandlersAfterFailure" since it does not guarantee what it states and also provide some additional comments to "DenyAnonymousAuthorizationRequirement" that it also does not fail on unauthenticated requests and every custom policy should always perform such checks on their own, since to me it is the big opening after a long time of using it 😀. Did a questionnaire to my colleges and as you can guess - it is no F way obvious to them neither.
@blowdart is correct, so generally its not recommended to call Fail as its possible other handlers can satisfy an authorization requirement, for example, imagine there's an AdminHandler that succeeds all requirements for a specific user no matter what. If any handler called fail, that would block that type of scenario. So the misconception here is that there's a handler that fails an authorization request if the user is not authenticated. That's not what DenyAnonymentAuthorizationRequirement implies, in our design, its more of a Require IsAuthenticated requirement. Perhaps that would have been a better name, but hopefully that explanation makes sense
@HaoK I would totally agree with you in that use cases you provided, but here is the thing - I have an option “ AuthorizationOptions.InvokeHandlersAfterFailure” which I expect that would not invoke handlers further but it is just not working as designed because DenyAnonymousAuthorizationRequirement has unobvious behavior.
I would expose some opt-in properties that would change behavior of requirements and make the whole concept more straightforward.
Right now there is no any way from out of the box to make a combination of multiple requirements where responsibilities are separated, since I need to perform authentication check in every requirement. Does it sound to you as a good and straightforward experience?
The option is doing exactly what it says, it does not invoke handlers after Fail has been called. DenyAnonymousAuthorizationRequirement doesn't call Fail. If you wish to have a handler that fails explicitly, you can always implement your own FailIfNotAuthenticatedHandler and register that to do what you seek. But that's not what we do out of the box.
Needs: Author Feedback
The author of this issue needs to respond in order for us to continue investigating this issue.
label
Aug 26, 2021
This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.
See our Issue Management Policies for more information.
@blowdart, yes I would suggest to add additional explanation to .RequireAuthenticatedUser()
saying that it is not failing the authorization on unauthenticated request, so there should be additional authentication check in every authorization requirement.
Also, make more clear in comment how AuthorizationOptions.InvokeHandlersAfterFailure
behaves, add comment that it stops invocation only if handler explicitly called .Fail()
and add reference to DenyAnonymousAuthorizationRequirement
But you decide whether it makes sense or not.
Personally, I would extend DenyAnonymousAuthorizationRequirement
with optional parameter which fails requirement once specified. That would improve overall experience and not cause any breaking changes and no need in additional comments since it would be self explanatory
Needs: Attention 👋
This issue needs the attention of a contributor, typically because the OP has provided an update.
and removed
Needs: Author Feedback
The author of this issue needs to respond in order for us to continue investigating this issue.
Status: No Recent Activity
labels
Sep 3, 2021
@blowdart we can tweak https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.authorization.authorizationoptions.invokehandlersafterfailure?view=aspnetcore-5.0 to be specific, "Determines whether authentication handlers should be invoked after a failure." where "failure" can be changed to "AuthorizationHandlerContext.Fail" is called, but there's conceptually no other way for a failure to happen, but this might make it slightly clearer
Needs: Attention 👋
This issue needs the attention of a contributor, typically because the OP has provided an update.
label
Sep 7, 2021
✔️ Resolution: Answered
Resolved because the question asked by the original author has been answered.
label
Sep 14, 2021
✔️ Resolution: Answered
Resolved because the question asked by the original author has been answered.
Status: Resolved