More OIDC events, better data flow #442

Tratcher · 2015-09-09T21:20:39Z

Add new OIDC events:
#340 RedirectToIdentityProvider -> RedirectForAuthentication & RedirectForSignOut
#358 UserInformationReceived
#327 AuthenticationCompleted

Also, #372 make sure any mutable event state flows after the event.

@HaoK @brentschmaltz @tushargupta51 @PinpointTownes @Eilon @muratg

kevinchalet · 2015-09-09T21:55:16Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/Events/RedirectForSignOutContext.cs

+        /// Gets or sets the <see cref="OpenIdConnectMessage"/>.
+        /// </summary>
+        /// <exception cref="ArgumentNullException">if 'value' is null.</exception>
+        public OpenIdConnectMessage ProtocolMessage { get; [param: NotNull] set; }


Isn't NotNull discouraged for properties?

I still don't know why replacing the message was important for @brentschmaltz since it's already fully mutable.

Since these contexts are OIDC-specific, consider using more appropriate names like LogoutRequest.

NotNull removed.

I actually like the standard property name across all the OIDC events.

If this is an exact copy of the other Redirect context, can we not just have a single RedirectContext we use for both? I'm really not a huge fan of all of these context objects in general. Even if we have to add to the context in the future, I don't think sharing in this case is really going to haunt us will it?

Or if we want unique context classes per event, we should at least consider pulling the ProtocolMessage property that appears in all of them I think to a OpenIdBaseContext? That would at least make them empty child classes

kevinchalet · 2015-09-09T22:12:39Z

For the new events, I wonder if RedirectToAuthorizationEndpoint and RedirectToLogoutEndpoint wouldn't be a bit more expressive.

kevinchalet · 2015-09-09T22:17:52Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs

            idToken = tokenEndpointResponse.Message.IdToken;

-            var authorizationCodeRedeemedContext = await RunAuthorizationCodeRedeemedEventAsync(message, tokenEndpointResponse);
+            var authorizationCodeRedeemedContext = await RunAuthorizationCodeRedeemedEventAsync(message, code, tokenEndpointResponse);


You should consider exposing both authorization response and the token response when you can.

I don't follow. We're including the token response here. What authorization response are you looking for?

Wooops, wrong line. This remark was for the SecurityTokenReceived /SecurityTokenValidated notifications, where the token response is not added to the context.

This is a very annoying thing that currently prevents adding the access token retrieved from the token endpoint to the final ClaimsIdentity, since only the authorization response is exposed. (that's why I haven't updated this sample: https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Server/blob/vNext/samples/Mvc/Mvc.Client/Startup.cs#L101-L109)

SecurityTokenReceived only runs in the IdToken flow, where the token response is only available in the code flow. That said, yes, I can add the token response to the SecurityTokenValidated event.

Tratcher · 2015-09-10T16:26:29Z

@PinpointTownes It's actually called the EndSessionEndpoint. RedirectToEndSessionEndpoint? Seems ok.

kevinchalet · 2015-09-10T16:31:21Z

Yeah, but this name sounds weird (that's why I never use it). That said, if you want to use the official name, I'm fine.

Le 10 sept. 2015 à 18:26, Chris R notifications@github.com a écrit :

@PinpointTownes It's actually called the EndSessionEndpoint. RedirectToEndSessionEndpoint? Seems ok.

—
Reply to this email directly or view it on GitHub.

Tratcher · 2015-09-10T17:14:12Z

Updated.

Tratcher · 2015-09-11T18:50:51Z

Updated. @HaoK @PinpointTownes @brentschmaltz

kevinchalet · 2015-09-11T19:47:18Z

I'll test it in a real world app. Some renamed notifications look weird.

kevinchalet · 2015-09-13T12:08:01Z

There's a terrible bug with the authorization code flow. I'll investigate.

kevinchalet · 2015-09-13T13:18:39Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs

            // no need to validate signature when token is received using "code flow" as per spec [http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation].
            var validationParameters = Options.TokenValidationParameters.Clone();
            validationParameters.ValidateSignature = false;

-            ticket = ValidateToken(idToken, message, properties, validationParameters, out jwt);
+            ticket = ValidateToken(message.IdToken, message, properties, validationParameters, out jwt);


This line is bugged: message actually represents the authorization response, not the token response. And since we're in a path that only supports the code flow (and not the hybrid flow), message.IdToken is always null. You must instead extract the id_token from the token response.

error : [Microsoft.AspNet.Authentication.OpenIdConnect.OpenIdConnectAuthentica
tionMiddleware] OIDCH_0017: Exception occurred while processing message.
System.ArgumentNullException: La valeur ne peut pas être null.
Nom du paramètre : IDX10000: The parameter 'tokenstring' cannot be a 'null' or a
n empty string.
à Microsoft.IdentityModel.Logging.LogHelper.Throw(String message, Type except
ionType, EventLevel logLevel, Exception innerException)
à System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.CanReadToken(String
tokenString)
à Microsoft.AspNet.Authentication.OpenIdConnect.OpenIdConnectAuthenticationHa
ndler.ValidateToken(String idToken, OpenIdConnectMessage message, Authentication
Properties properties, TokenValidationParameters validationParameters, JwtSecuri
tyToken& jwt)

Good catch, fixed.

kevinchalet · 2015-09-13T18:59:53Z

On a related note, you should consider cleaning this code path: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs#L374-L392

Now that the OIDC middleware has appropriate anti-XSRF countermeasures (ValidateCorrelationId), the state parameter is mandatory and having a null value should be treated as an error before calling ValidateCorrelationId.

Tratcher · 2015-09-15T16:56:57Z

Updated.

kevinchalet · 2015-09-15T17:19:00Z

I briefly chatted with a friend about these notifications and we agreed they were terribly confusing. I suggest a much more aggressive renaming to match the standard names:

MessageReceived => AuthorizationResponseReceived.
IdTokenReceived/AuthorizationCodeRedeemed: replace them by TokenResponseReceived. When invoked after an authorization response (for what you name "id_token flows"), IdTokenReceived is basically useless as you already have MessageReceived.
IdTokenValidated => AuthorizationResponseValidated and TokenResponseValidated.

Tratcher · 2015-09-15T18:48:34Z

Hmm, there's something to that... lets see...

There's a catch with MessageReceived, you don't actually know if it's an OIDC message / Authorization Response yet, it could be any form post or get query. OIDC does not require a restricted callback path.

Your IdTokenReceived argument seems to apply to AuthorizationCodeReceived as well. I think these have some value over MessageReceived because you're now in a validated OIDC flow (error, state, correlation, etc.).

AuthorizationCodeRedeemed -> TokenResponseReceived makes sense.

IdTokenValidated -> AuthorizationResponseValidated vs TokenResponseValidated: There is an authorization response in both flows, but you only validate it in the implicit flow. Really the event is the big "Everything" validated at the end of the flow, almost like AuthenticationCompleted but it still has the details. How about just AuthenticationValidated?

On a related note comparing the two flows, it looks a little odd that code flow runs GetUserInformationAsync before Validated, but the implicit flow runs AuthorizationCodeReceived after Validated. Of the two, I think I'd move GetUserInformationAsync later.

Tratcher · 2015-09-15T18:50:22Z

Or maybe you could merge IdTokenReceived and AuthorizationCodeReceived into a single AuthorizationResponseReceived.

kevinchalet · 2015-09-15T19:19:07Z

There's a catch with MessageReceived, you don't actually know if it's an OIDC message / Authorization Response yet, it could be any form post or get query. OIDC does not require a restricted callback path.

Well, in its current form, MessageReceived is never called if the handler is not able to extract the OIDC message from the query string or the request form, which obviously limits the ability to retrieve the message from elsewhere: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectHandler.cs#L337-L340. I'm not sure that MessageReceived is really useful as-is. One evidence is that you were not able to support response_mode=query even with this hook.

At this point, yeah, we don't know yet if it's a "standard-compliant" authorization response, but on the other hand, you have no way to guarantee that. Not to mention that an OIDC message that contains an error or has no state parameter is still an authorization response, at least semantically speaking.

Your IdTokenReceived argument seems to apply to AuthorizationCodeReceived as well. I think these have some value over MessageReceived because you're now in a validated OIDC flow (error, state, correlation, etc.).

Do you have a concrete use case for these events?

IMHO, AuthorizationCodeReceived would be more useful if it was renamed to TokenRequestSending and invoked just before sending the token request (to allow modifying the token request before sending it on the wire).

Or maybe you could merge IdTokenReceived and AuthorizationCodeReceived into a single AuthorizationResponseReceived.

Yup 👍

IdTokenValidated -> AuthorizationResponseValidated vs TokenResponseValidated: There is an authorization response in both flows, but you only validate it in the implicit flow. Really the event is the big "Everything" validated at the end of the flow, almost like AuthenticationCompleted but it still has the details. How about just AuthenticationValidated?

AuthenticationValidated doesn't look too bad, but when implementing the hybrid, we'll need to validate both the authorization response and the token response (http://openid.net/specs/openid-connect-core-1_0.html#HybridAuthResponseValidation and http://openid.net/specs/openid-connect-core-1_0.html#HybridTokenResponseValidation), so I'm not sure it's worth adding a too generic event if we need to distinguish both steps.

On a related note comparing the two flows, it looks a little odd that code flow runs GetUserInformationAsync before Validated, but the implicit flow runs AuthorizationCodeReceived after Validated. Of the two, I think I'd move GetUserInformationAsync later.

Yeah, good idea 👍

Tratcher · 2015-09-15T22:31:01Z

The ODICMessage constructor doesn't do any message validation, it would work with almost any query or form post, so MessageReceived will fire for most requests. Listening to it is the only place developers can pre-process or filter incoming messages.

I realized that AuthorizationCodeReceived is called at different places in both flows. In code flow it's just before redeeming the code, but in implicit/hybrid it is your chance to redeem the code yourself. I'll have to think about how to rationalize that. They may need to be two different events.

Tratcher · 2015-09-15T22:31:15Z

Updated PR.

kevinchalet · 2015-09-15T22:38:41Z

The ODICMessage constructor doesn't do any message validation, it would work with almost any query or form post, so MessageReceived will fire for most requests. Listening to it is the only place developers can pre-process or filter incoming messages.

I never said it did (luckily, or I wouldn't use it so massively 😄). I just said that an OIDC message posted at client's redirect_uri endpoint is semantically an authorization response, even if it doesn't have the parameters you expect.

OIDC does not require a restricted callback path.

Talking about that, I wonder if this feature is still useful. IIRC, your main argument to keep it was to support "unsolicited assertions". With state now being mandatory, it's not possible anymore, since responses lacking a valid state - that can only be produced by the client app - will be rejected.

Tratcher · 2015-09-16T18:53:45Z

#455

Tratcher · 2015-09-16T19:24:44Z

I think the hybrid flow needs more work, but I'll call this good for now. @HaoK @brentschmaltz Can you take a quick pass?

#456

HaoK · 2015-09-16T19:50:37Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectHandler.cs

@@ -391,6 +387,26 @@ public OpenIdConnectHandler(HttpClient backchannel)
                    _configuration = await Options.ConfigurationManager.GetConfigurationAsync(Context.RequestAborted);
                }

+                Logger.LogDebug("Authorization response received.");
+                var authorizationResponseReceivedContext = new AuthorizationResponseReceivedContext(Context, Options)


nit: this is one really long local var name...just saying :)

HaoK · 2015-09-16T20:10:52Z

kevinchalet · 2015-09-16T20:40:44Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectHandler.cs

-                // if state is missing, just log it
-                if (string.IsNullOrEmpty(message.State))
+                // if any of the error fields are set, throw error null
+                if (!string.IsNullOrEmpty(message.Error))


Consider moving this block after the state check to reject requests missing a valid state since it must be returned by the server even on errored responses:

http://openid.net/specs/openid-connect-core-1_0.html#AuthError
https://tools.ietf.org/html/rfc6749#section-4.1.2.1

#358 Add a UserInformationReceived event. #327 Add AuthenticationCompleted event. #340 Split the Redirect event for Authentication and SignOut. Rename OnAuthorizationCodeRedeemed to OnTokenResponseReceived. Move IdTokenReceived to AuthorizationResponseReceived. Rename IdTokenValidated to AuthenticationValidated.

dnfclas added the cla-already-signed label Sep 9, 2015

kevinchalet reviewed Sep 9, 2015
View reviewed changes

kevinchalet reviewed Sep 13, 2015
View reviewed changes

Tratcher force-pushed the tratcher/events branch from 69d06c0 to 1f801d8 Compare September 15, 2015 16:56

HaoK reviewed Sep 16, 2015
View reviewed changes

kevinchalet reviewed Sep 16, 2015
View reviewed changes

Tratcher force-pushed the tratcher/events branch from 725088a to 1c0768f Compare September 16, 2015 21:34

Tratcher merged commit 1c0768f into dev Sep 16, 2015

Tratcher deleted the tratcher/events branch September 16, 2015 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More OIDC events, better data flow #442

More OIDC events, better data flow #442

Tratcher commented Sep 9, 2015

kevinchalet Sep 9, 2015

kevinchalet Sep 9, 2015

Tratcher Sep 9, 2015

HaoK Sep 10, 2015

HaoK Sep 10, 2015

kevinchalet commented Sep 9, 2015

kevinchalet Sep 9, 2015

Tratcher Sep 10, 2015

kevinchalet Sep 11, 2015

Tratcher Sep 11, 2015

Tratcher commented Sep 10, 2015

kevinchalet commented Sep 10, 2015

Tratcher commented Sep 10, 2015

Tratcher commented Sep 11, 2015

kevinchalet commented Sep 11, 2015

kevinchalet commented Sep 13, 2015

kevinchalet Sep 13, 2015

Tratcher Sep 15, 2015

kevinchalet commented Sep 13, 2015

Tratcher commented Sep 15, 2015

kevinchalet commented Sep 15, 2015

Tratcher commented Sep 15, 2015

Tratcher commented Sep 15, 2015

kevinchalet commented Sep 15, 2015

Tratcher commented Sep 15, 2015

Tratcher commented Sep 15, 2015

kevinchalet commented Sep 15, 2015

Tratcher commented Sep 16, 2015

Tratcher commented Sep 16, 2015

HaoK Sep 16, 2015

HaoK commented Sep 16, 2015

kevinchalet Sep 16, 2015

More OIDC events, better data flow #442

More OIDC events, better data flow #442

Conversation

Tratcher commented Sep 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Sep 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Sep 10, 2015

kevinchalet commented Sep 10, 2015

Tratcher commented Sep 10, 2015

Tratcher commented Sep 11, 2015

kevinchalet commented Sep 11, 2015

kevinchalet commented Sep 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Sep 13, 2015

Tratcher commented Sep 15, 2015

kevinchalet commented Sep 15, 2015

Tratcher commented Sep 15, 2015

Tratcher commented Sep 15, 2015

kevinchalet commented Sep 15, 2015

Tratcher commented Sep 15, 2015

Tratcher commented Sep 15, 2015

kevinchalet commented Sep 15, 2015

Tratcher commented Sep 16, 2015

Tratcher commented Sep 16, 2015

Choose a reason for hiding this comment

HaoK commented Sep 16, 2015

Choose a reason for hiding this comment