Add AuthenticationProperties to HandleRequestResult and RemoteFailureContext #1299
Conversation
|
@Tratcher, |
|
More for @anitchanana than for me to review. |
Tratcher
force-pushed
the
tratcher/authfailed
branch
from
4ed0b5e to
5d7be8a
Compare
| @@ -475,6 +475,12 @@ protected virtual Task<bool> HandleSignOutCallbackAsync() | |||
| authorizationResponse = messageReceivedContext.ProtocolMessage; | |||
| properties = messageReceivedContext.Properties; | |||
|
|
|||
| // if any of the error fields are set, throw error null | |||
| if (!string.IsNullOrEmpty(authorizationResponse.Error)) | |||
There was a problem hiding this comment.
I was trying to make the order of operations consistent between OAuth and OIDC.
There was a problem hiding this comment.
IMHO, that would be better to fix the OAuth2 handler to validate the state/XSRF cookie before handling the error.
There was a problem hiding this comment.
Fair. I was worried about the state being dropped for some error responses and thus hiding the error response with a correlation failure. Not sure how often that happens in practice.
Tratcher
force-pushed
the
tratcher/authfailed
branch
3 times, most recently
from
46af5bc to
88e91f6
Compare
|
bump |
| if (!string.IsNullOrEmpty(authorizationResponse.State)) | ||
| { | ||
| properties = Options.StateDataFormat.Unprotect(authorizationResponse.State); | ||
|
|
There was a problem hiding this comment.
Nit: private helper method for this Unprotect/decode clear state block that's repeated?
| @@ -11,6 +11,7 @@ namespace Microsoft.AspNetCore.Authentication | |||
| /// </summary> | |||
| public class RemoteFailureContext : HandleRequestContext<RemoteAuthenticationOptions> | |||
| { | |||
| [Obsolete] | |||
There was a problem hiding this comment.
Should this have some text to go along with the warning?
There was a problem hiding this comment.
shrug we don't really expect anyone to use these directly.
There was a problem hiding this comment.
Why bother with obsolete at all, its not incorrect to use this ctor is it?
There was a problem hiding this comment.
It's obsolete as the first step towards getting rid of it, it's not used anymore.
Tratcher
force-pushed
the
tratcher/authfailed
branch
2 times, most recently
from
c0889db to
5fbc418
Compare
|
Updated |
Tratcher
force-pushed
the
tratcher/authfailed
branch
from
5fbc418 to
144ee21
Compare
#1188 Requires aspnet/HttpAbstractions#889
User state for the login is flown via auth properties, but these aren't available in the OnRemoteFailure event. To add them we need to flow them via the AuthResult.
If this works out then we may use it to flow additional data for #1178. The difference is that the data in #1178 isn't a consistent data structure across handlers.
/cc: @brockallen