Incorporate community feedback from https://github.com/aspnet/Security/pull/112 #276

kevinchalet · 2015-05-24T17:34:07Z

This pull request fixes a few things that were not solved by @brentschmaltz's PR (#112).
Please note that it's still a WIP and that I'll probably add more commits.

/cc @HaoK @brentschmaltz

HaoK · 2015-05-24T17:40:56Z

Looks good so far...

HaoK · 2015-05-26T07:58:09Z

I actually like this suggestion from the PR you refer to (assuming its true)...

Rename OAuthBearerAuthenticationMiddleware => TokenAuthenticationMiddleware

HaoK · 2015-05-26T08:02:44Z

But I see lots of stuff that doesn't look general enough to call this TokenAuthenticationOptions in here though... with a bunch of OpenId stuff on there.

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuthBearer/OAuthBearerAuthenticationOptions.cs

kevinchalet · 2015-05-26T10:49:21Z

Renaming the OAuth2 bearer middleware was not part of the plan simply because there was no consensus concerning my suggestion 😄

IMHO, the best approach (as explained here: #112 (comment) and here: #112 (comment)) would probably consist in splitting the current OAuth2 bearer middleware into 2 middleware:

A basic TokenAuthenticationMiddleware, designed like the OAuth2 interactive middleware (with a public handler whose methods are protected and virtual and with generic notifications if you don't want to subclass it).
A "new" OpenIdConnectTokenAuthenticationMiddleware, subclassing TokenAuthenticationMiddleware and using the magic OIDC provider configuration discovery to validate tokens.

tushargupta51 · 2015-05-27T19:00:04Z

src/Microsoft.AspNet.Authentication.OAuthBearer/OAuthBearerAuthenticationOptions.cs

-                _securityTokenValidators = value;
-            }
-        }
+        public IList<ISecurityTokenValidator> SecurityTokenValidators { get; } = new List<ISecurityTokenValidator>();


Why there is no setter on this? Same comment for TVP.

Writable collection properties are generally considered as a bad practice: https://msdn.microsoft.com/fr-fr/library/ms182327(v=vs.140).aspx

I have seen that rule many times, i find it leads to some odd code. How will a user set the validators they want to use? It is a little more cumbersome. I think that rule was written before the new syntax like: new foo { x = this, y = that ...} existed.

Very easily, with Add: https://github.com/aspnet/Security/pull/276/files#diff-b15c3246438a45f3bed149e5f60b1e22R144

And BTW, the object initializer syntax cannot be used anymore, as we're now using delegates to set the options:

app.UseCookieAuthentication(options => { options.AutomaticAuthentication = true; });

tushargupta51 · 2015-05-27T23:53:24Z

I do not understand the reason for this pull request. Are there any issues opened that this PR is addressing?

kevinchalet · 2015-05-28T05:59:07Z

@tushargupta51 see @brentschmaltz's previous PR: #112

Tons of things have been omitted and this PR only aims at incorporating them 😄

brentschmaltz · 2015-05-28T16:58:40Z

@PinpointTownes specifically what issues are you addressing?

I see some formatting and style changes, some that break extensiblity, make the product more fragile.

For example, if you can set Options.StateDataFormat to null, then any consumer of that property must expect that it can be null.

Another example, setting timeout negative, what would be the result of that?

Siting the .Net guidelines as a reason for a change seems to imply that following those guidelines is required for code changes. I don't think that is true.

kevinchalet · 2015-05-28T17:21:07Z

@PinpointTownes specifically what issues are you addressing?

No particular issue, just formatting and style things you didn't include when you pushed your previous PR 😯

I see some formatting and style changes, some that break extensiblity, make the product more fragile.
For example, if you can set Options.StateDataFormat to null, then any consumer of that properties must expect that it can be null.

AGAIN, IT CANNOT BE NULL: a default format relying on the data protection block is automatically instantiated if you leave it to null.

Another example, setting timeout negative, what would be the result of that?
TimeSpan.FromMilliseconds(-1.0); is a valid value for HttpClient.Timeout. But I only removed the backing field to make it consistent with the OAuth2 client middleware, where @Tratcher only used an automatic property.

Siting the .Net guidelines as a reason for a change seems to imply that following those guidelines is required for code changes. I don't think that is true.

But rules are there for a reason, and you need a reason to break them 😄

kevinchalet · 2015-05-28T17:24:13Z

@brentschmaltz here's something I'd like to fix: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuthBearer/OAuthBearerAuthenticationHandler.cs#L178

The bearer middleware shouldn't throw by default, as it returns a 500 response instead of a correct challenge. And it's bad, bad, bad 😄

kevinchalet · 2015-05-28T17:32:29Z

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

        /// </summary>
-        public string Scope { get; set; }


Replaced by a list for consistency: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuth/OAuthAuthenticationOptions.cs#L92

kevinchalet · 2015-05-28T17:49:25Z

Other things I'd like to fix:

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs#L175 : if IssuerAddress is empty, where will the user agent be redirect to? We should fix that and forbid empty addresses and null configurations.
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs#L167 : this kind of null check is just awful and is the direct result of having both a Configuration property in the options and a ConfigurationManager. We should fix that and make it consistent.
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs#L150 : this option is basically pointless and exists nowhere else. I suggest removing it and always using the current URL when there's no explicit RedirectUri in the authentication properties (just like the other providers).
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs#L164 : storing the callback endpoint in the state parameter is pointless as it always refers to Options.RedirectUri, that never changes. It just makes the state - and the whole OIDC payload - unnecessary bigger. It also complicates the code flow (see @tushargupta51's PR and your recent remark).

brentschmaltz · 2015-05-29T15:52:02Z

Your points above:

IssuerAddress: A user can hook RedirectToIdentityProvider to set the IssuerAddress so the thought is we would block those users if we fault too early.
I agree it is not ideal, we need ConfigurationManager to obtain metadata automatically AND for users to be able to set metadata independently. We haven't revisited this yet. What do you propose?
3 / 4. The current way we capture the 'redirect_uri' is broken and needs to be changed. We need the exact 'redirect_uri' sent to the IdentityProvider for the ConfidentialClient 'code' -> 'access_token' flow. We are capturing too early, after RedirectToIdentityProvider seems better, as the user could have set the value. I don't think the flag will then be needed. My thoughts are the logic is similar to 'state' changes in Changes for State and test organization #233 where we capture after the notifications.

kevinchalet · 2015-05-29T19:07:18Z

IssuerAddress: A user can hook RedirectToIdentityProvider to set the IssuerAddress so the thought is we would block those users if we fault too early.

I agree it is not ideal, we need ConfigurationManager to obtain metadata automatically AND for users to be able to set metadata independently. We haven't revisited this yet. What do you propose?

I'm not saying that we shouldn't have these two properties (it's probably unavoidable BTW), but the way we're currently using them is just weird:

If you don't provide a metadata address (explicitly or implicitly using the authority) and don't set Options.Configuration, your app won't have any information concerning the issuer and will miserably fail during the challenge processing. Even enabling logging won't be helpful in this case, as it will simply log the generic (and cryptic) OIDCH_0036: Uri.IsWellFormedUriString(redirectUri, UriKind.Absolute) returned 'false', redirectUri is: {0}", (redirectUri ?? "null")) message.
Allowing developers to change the issuer address at runtime using notifications is not a bad thing, but it's definitely a "niche market". I'm totally convinced we should make AuthorizationEndpoint mandatory and fail very early if we can't find it in the static or dynamic configuration. You can always provide a dummy value and replace it at runtime using the RedirectToIdentityProvider notification. The same remark applies to every other OIDC parameter like redirect_uri, which is currently not required by the OIDC middleware, though it's declared as mandatory by the specifications.

3 / 4. The current way we capture the 'redirect_uri' is broken and needs to be changed. We need the exact 'redirect_uri' sent to the IdentityProvider for the ConfidentialClient 'code' -> 'access_token' flow.

Being able to "dynamically" change the redirect_uri has always been a weird thing for me. I still don't understand why we can do that and why Options.CallbackPath is not a mandatory property (BTW, the path check is wrong: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs#L244). What would be the advantage of that?

brentschmaltz · 2015-07-01T16:37:53Z

Any changes we take here should wait till #315 and #258 are merged.

kevinchalet · 2015-07-01T16:39:12Z

@brentschmaltz don't worry, I won't break your changes 😄

This PR needs to be rebased anyway.

kevinchalet · 2015-08-25T19:33:49Z

Rebased.

I limited this PR to the code style issues we discussed in @brentschmaltz's PR (#112) and removed the controversial changes. This PR also replaces Scope by a list of items (we could consider using a set instead) and removes the validators list, replaced by a single instance, as suggested by @brentschmaltz.

Tratcher · 2015-08-31T22:33:14Z

src/Microsoft.AspNet.Authentication.OAuthBearer/OAuthBearerAuthenticationOptions.cs


        /// <summary>
        /// The object provided by the application to process events raised by the bearer authentication middleware.
        /// The application may implement the interface fully, or it may create an instance of OAuthBearerAuthenticationProvider
        /// and assign delegates only to the events it wants to process.
        /// </summary>
-        public OAuthBearerAuthenticationNotifications Notifications { get; set; }
+        public OAuthBearerAuthenticationNotifications Notifications { get; [param: NotNull] set; } = new OAuthBearerAuthenticationNotifications();


FYI: we've been asked not to use NotNull in this context (more details coming soon). Let's revert back to setting these in the middleware constructor for now.

Do you want me to revert all the inline properties? This syntax makes the code much clearer IMHO.

You can keep those. We just don't want to put the non-null enforcement code here.

Okay. param: NotNull is currently used in a few places. Do you want me to remove them?

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Cookies/CookieAuthenticationOptions.cs#L41
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication/Notifications/RedirectToIdentityProviderNotification.cs#L26

Don't worry about the existing ones yet, just don't add more.

Tratcher · 2015-08-31T22:41:43Z

⌚ Almost ready.

Tratcher · 2015-08-31T23:20:37Z

src/Microsoft.AspNet.Authentication.OAuthBearer/OAuthBearerAuthenticationMiddleware.cs

@@ -38,16 +34,6 @@ public class OAuthBearerAuthenticationMiddleware : AuthenticationMiddleware<OAut
            ConfigureOptions<OAuthBearerAuthenticationOptions> configureOptions)
            : base(next, options, loggerFactory, encoder, configureOptions)
        {
-            if (Options.Notifications == null)
-            {
-                Options.Notifications = new OAuthBearerAuthenticationNotifications();


Can you revert this half too please?

Done. I also reverted the same change for OIDC and added a check for the OAuth2 generic middleware.

…idators list by a single validator

dnfclas added the cla-already-signed label May 24, 2015

tushargupta51 reviewed May 27, 2015
View reviewed changes

kevinchalet reviewed May 28, 2015
View reviewed changes

kevinchalet mentioned this pull request Aug 25, 2015

Consider modifying OpenIdConnectAuthenticationOptions to use a single ISecurityTokenValidator #410

Closed

Tratcher self-assigned this Aug 28, 2015

Tratcher added this to the 1.0.0-beta8 milestone Aug 28, 2015

Tratcher reviewed Aug 31, 2015
View reviewed changes

Use automatic properties, replace scope by a list and replace the val…

fa39144

…idators list by a single validator

Tratcher merged commit fa39144 into aspnet:dev Aug 31, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporate community feedback from https://github.com/aspnet/Security/pull/112 #276

Incorporate community feedback from https://github.com/aspnet/Security/pull/112 #276

kevinchalet commented May 24, 2015

HaoK commented May 24, 2015

HaoK commented May 26, 2015

HaoK commented May 26, 2015

kevinchalet commented May 26, 2015

tushargupta51 May 27, 2015

kevinchalet May 27, 2015

brentschmaltz May 27, 2015

kevinchalet May 28, 2015

tushargupta51 commented May 27, 2015

kevinchalet commented May 28, 2015

brentschmaltz commented May 28, 2015

kevinchalet commented May 28, 2015

kevinchalet commented May 28, 2015

kevinchalet May 28, 2015

kevinchalet commented May 28, 2015

brentschmaltz commented May 29, 2015

kevinchalet commented May 29, 2015

brentschmaltz commented Jul 1, 2015

kevinchalet commented Jul 1, 2015

kevinchalet commented Aug 25, 2015

Tratcher Aug 31, 2015

kevinchalet Aug 31, 2015

Tratcher Aug 31, 2015

kevinchalet Aug 31, 2015

Tratcher Aug 31, 2015

kevinchalet Aug 31, 2015

Tratcher commented Aug 31, 2015

Tratcher Aug 31, 2015

kevinchalet Aug 31, 2015

Incorporate community feedback from https://github.com/aspnet/Security/pull/112 #276

Incorporate community feedback from https://github.com/aspnet/Security/pull/112 #276

Conversation

kevinchalet commented May 24, 2015

HaoK commented May 24, 2015

HaoK commented May 26, 2015

HaoK commented May 26, 2015

kevinchalet commented May 26, 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

tushargupta51 commented May 27, 2015

kevinchalet commented May 28, 2015

brentschmaltz commented May 28, 2015

kevinchalet commented May 28, 2015

kevinchalet commented May 28, 2015

Choose a reason for hiding this comment

kevinchalet commented May 28, 2015

brentschmaltz commented May 29, 2015

kevinchalet commented May 29, 2015

brentschmaltz commented Jul 1, 2015

kevinchalet commented Jul 1, 2015

kevinchalet commented Aug 25, 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

Choose a reason for hiding this comment

Tratcher commented Aug 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment