-
Notifications
You must be signed in to change notification settings - Fork 586
Changes for State and test organization #233
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,8 @@ namespace Microsoft.AspNet.Authentication.OpenIdConnect | |
| /// </summary> | ||
| public class OpenIdConnectAuthenticationNotifications | ||
| { | ||
| Func<RedirectToIdentityProviderNotification<OpenIdConnectMessage, OpenIdConnectAuthenticationOptions>, Task> _redirectToIdentityProvider; | ||
|
|
||
| /// <summary> | ||
| /// Creates a new set of notifications. Each notification has a default no-op behavior unless otherwise documented. | ||
| /// </summary> | ||
|
|
@@ -44,7 +46,19 @@ public OpenIdConnectAuthenticationNotifications() | |
| /// <summary> | ||
| /// Invoked to manipulate redirects to the identity provider for SignIn, SignOut, or Challenge. | ||
| /// </summary> | ||
| public Func<RedirectToIdentityProviderNotification<OpenIdConnectMessage, OpenIdConnectAuthenticationOptions>, Task> RedirectToIdentityProvider { get; set; } | ||
| public Func<RedirectToIdentityProviderNotification<OpenIdConnectMessage, OpenIdConnectAuthenticationOptions>, Task> RedirectToIdentityProvider | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So why is this Func special compared to all of the other Func setters that allow setting to null? I'm not arguing necessarily that they should be nullable, I'm just pointing out that they should all behave the same
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing special with this Func. I stopped here to see if folks would like to see this pattern for all the Func's.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory we could use [NotNull] for these instead. It would be less work for us, and it would generate the same output as a compile step.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 for [NotNull]
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can work in this case, but for the notifications themselves, since the Ticket and Message can be set, we need some stronger validation. I noticed in FB and Goodle MW and Handlers we don't check for null in public methods.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All public methods need to check for null. If we have some that don't, then that's a bug.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then we need to take action as you will find a lot of such code. |
||
| { | ||
| get { return _redirectToIdentityProvider; } | ||
| set | ||
| { | ||
| if (value == null) | ||
| { | ||
| throw new ArgumentNullException("value"); | ||
| } | ||
|
|
||
| _redirectToIdentityProvider = value; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Invoked with the security token that has been extracted from the protocol message. | ||
|
|
@@ -55,6 +69,5 @@ public OpenIdConnectAuthenticationNotifications() | |
| /// Invoked after the security token has passed validation and a ClaimsIdentity has been generated. | ||
| /// </summary> | ||
| public Func<SecurityTokenValidatedNotification<OpenIdConnectMessage, OpenIdConnectAuthenticationOptions>, Task> SecurityTokenValidated { get; set; } | ||
|
|
||
| } | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,64 @@ | ||
| // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using Microsoft.AspNet.Http; | ||
| using Microsoft.AspNet.Http.Authentication; | ||
| using Microsoft.Framework.Internal; | ||
|
|
||
| namespace Microsoft.AspNet.Authentication.Notifications | ||
| { | ||
| /// <summary> | ||
| /// When a user configures the <see cref="AuthenticationMiddleware{TOptions}"/> to be notified prior to redirecting to an IdentityProvider | ||
| /// and instance of <see cref="RedirectFromIdentityProviderNotification{TMessage, TOptions, TMessage, AuthenticationProperties}"/> is passed to 'RedirectToIdentityProvider". | ||
| /// </summary> | ||
| /// <typeparam name="TMessage">protocol specific message.</typeparam> | ||
| /// <typeparam name="TOptions">protocol specific options.</typeparam> | ||
| public class RedirectToIdentityProviderNotification<TMessage, TOptions> : BaseNotification<TOptions> | ||
| { | ||
| public RedirectToIdentityProviderNotification(HttpContext context, TOptions options) : base(context, options) | ||
| TMessage _message; | ||
| AuthenticationProperties _properties; | ||
|
|
||
| public RedirectToIdentityProviderNotification([NotNull] HttpContext context, [NotNull] TOptions options, [NotNull] TMessage protocolMessage, [NotNull] AuthenticationProperties properties ) : base(context, options) | ||
| { | ||
| ProtocolMessage = protocolMessage; | ||
| AuthenticationProperties = properties; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the <see cref="{TMessage}"/>. | ||
| /// </summary> | ||
| /// <exception cref="ArgumentNullException">if 'value' is null.</exception> | ||
| public TMessage ProtocolMessage | ||
| { | ||
| get { return _message; } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the public set really necessary on Message/AuthProperties? can these not be private and only settable via the ctor? If the sets are only needed for unit tests, internal setter seems better than than adding null checks.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allows users to read a message and modify it. If you think about it, this allows some very powerful extensibility. |
||
| set | ||
| { | ||
| if (value == null) | ||
| { | ||
| throw new ArgumentNullException("value"); | ||
| } | ||
|
|
||
| _message = value; | ||
| } | ||
| } | ||
|
|
||
| public TMessage ProtocolMessage { get; set; } | ||
| /// <summary> | ||
| /// Gets or sets the <see cref="AuthenticationProperties"/>. | ||
| /// </summary> | ||
| /// <exception cref="ArgumentNullException">if 'value' is null.</exception> | ||
| public AuthenticationProperties AuthenticationProperties | ||
| { | ||
| get { return _properties; } | ||
| set | ||
| { | ||
| if (value == null) | ||
| { | ||
| throw new ArgumentNullException("value"); | ||
| } | ||
|
|
||
| _properties = value; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might have been discussed by why is
UrlEncoder.UrlEncodenot used to encode state data in this change ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suhasj When the message is created by OpenIdConnectMessage.CreateAuthenticationRequestUrl formatting will take place. If we encode here, it will be duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to parse the state back out for them on the other end.