AuthZ 2.0 - [Breaking] AuthorizeResult/Failure #1193
Conversation
|
@HaoK, |
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.AspNetCore.Authentication.Abstractions" Version="$(AspNetCoreVersion)" /> |
There was a problem hiding this comment.
AuthZ currently doesn't depend on Http or AuthN, if we are ok taking these dependencies in the main AuthZ package, we don't need to split out the authorize helper logic
| /// <summary> | ||
| /// Encapsulates a failure result of <see cref="IAuthorizationService.AuthorizeAsync(ClaimsPrincipal, object, IEnumerable{IAuthorizationRequirement})"/>. | ||
| /// </summary> | ||
| public class AuthorizeFailure |
There was a problem hiding this comment.
All these verb + Result classes sound a bit weird. Why not AuthorizationFailure?
There was a problem hiding this comment.
I started with that, but we went with AuthenticateResult (there that was meant to signify that it was a result for the Authenticate method). I could go either way here since I've had it both ways in this code already :)
| /// <summary> | ||
| /// If true, means the callee should challenge and try again. | ||
| /// </summary> | ||
| public bool Challenged { get; private set; } |
There was a problem hiding this comment.
Challenged => RequiresAuthentication?
|
Updated the PolicyEvaluator to have the new logic where when authorization fails, forbid is returned if any schemes were successfully authenticated. |
| } | ||
| } | ||
|
|
||
| if (newPrincipal == null) |
There was a problem hiding this comment.
Doh yeah, looks like I am missing tests for the Authenticate method too :)
|
| return AuthenticateResult.None(); | ||
| } | ||
| } | ||
| return AuthenticateResult.None(); |
There was a problem hiding this comment.
Review: we will never trigger forbidden if authentication schemes were not set on the policy, since authZ isn't doing the authentication in that case, its just relying on what's in context.User.
There was a problem hiding this comment.
We could explicitly check the result of AuthenticateAsync with the default scheme to determine if we want to return success/none instead to control the behavior of forbid/challenge if authorization fails later.
|
@Tratcher updated to react to the breaking auth changes in HttpAbstractions |
@blowdart @ajcvickers @Tratcher
Needed for: #901
First step needed to surface why authorization fails, returning a result which can contain failure information instead of just a bool.
Also added IAuthorizationHandlerProvider, and reworked IAuthorizationEvaluator to work with the new API.
Fixes: #1176