Replace configure method on TwitterOptions and OpenIdConnectOptions with CookieBuilder #1284

natemcmaster · 2017-06-29T23:43:19Z

Uses the new CookieBuilder API added here: aspnet/HttpAbstractions#882

Part 1- look for a separate PR to remove duplication from CookieAuthenticationOptions.

dnfclas · 2017-06-29T23:43:23Z

@natemcmaster,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

HaoK · 2017-06-29T23:51:14Z

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

-                            };
-
-                            Options.ConfigureNonceCookie?.Invoke(Context, cookieOptions);
+                                cookieOptions.Path = OriginalPathBase + Options.CallbackPath;


Maybe just do this instead? cookieOptions.Path = cookieOptions.Path ?? OriginalPathBase + Options.CallbackPath;

There's also a subtle bug here, since even if Path == null, the Builder could set the cookie.Path, which would get blasted away

Yes, that's why this deliberately checks if (Options.NonceCookie.Path == null) first.

Yeah so what if they have CustomCookieBuilder which sets cookieOptions.Path = "/mypath" in Build(), this code will still stomp on the path. The check should be checking the real cookieOptions.Path, not the builder property

Hmm. This is problematic. The default ctor for CookieOptions always sets Path to "/". If someone implements a custom cookie builder, it will be hard for us to detect if they indend to overwrite the default value or not.

var builder = new CookieBuilder(); // builder.Path= null var options = builder.Build(context) // options.Path= "/" // builder.Path== null means we can assume the user does not indent to change // the default path, therefore we set it to OriginalPathBase + Options.CallbackPath

var builder = new MyCustomCookieBuilder(); // builder.Path= null var options = builder.Build(context) // options.Path= "/mypath" // builder.Path== null means ????

Just check for options.Path == "/" and builder.Path== null?

We are talking about Path not name right?

Or we can just check for options.Path == "/" since we can't really assume anything based on the property alone.

if (cookieOptions.Path == "/") { cookieOptions.Path = OriginalPathBase + Options.CallbackPath; }

I figured out a way to do that. I override the build method to detect the value of Path before. Should allow user to override.

Tratcher · 2017-06-30T02:23:08Z

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

-                Expires = Clock.UtcNow.Add(Options.ProtocolValidator.NonceLifetime)
-            };
+            var cookieOptions = Options.NonceCookie.Build(Context);
+            cookieOptions.Expires = Clock.UtcNow.Add(Options.ProtocolValidator.NonceLifetime);


This isn't honoring the Expiration from CookieBuilder. Only set it if not null?

Any way to resolve Options.ProtocolValidator.NonceLifetime in the builder? Then you could incorporate it in a way that could be overridden.

Yup, just add a Property in a OpenIdConnectCookieBuilder for it

Tratcher · 2017-06-30T02:27:41Z

src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs

@@ -50,7 +49,8 @@ public TwitterHandler(IOptionsSnapshot<TwitterOptions> options, ILoggerFactory l
        {
            AuthenticationProperties properties = null;
            var query = Request.Query;
-            var protectedRequestToken = Request.Cookies[StateCookie];
+            var cookieName = Options.StateCookie.Name ?? StateCookieName;


Yeah, this gets simpler if we prevent Name from being null.

Tratcher · 2017-06-30T02:29:23Z

Add support for the correlation cookie, or file a followup bug since this isn't currently configurable.

Security/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs

Line 190 in ff9f145

var cookieOptions = new CookieOptions

HaoK · 2017-06-30T03:00:10Z

Should just make the change in this PR for correlation cookie

natemcmaster · 2017-06-30T18:33:49Z

Rebased and added the correlation cookie change to this PR.

HaoK · 2017-06-30T19:12:59Z

src/Microsoft.AspNetCore.Authentication/Internal/RequestPathCookieBuilder.cs

+            var path = Path;
+            if (path == null)
+            {
+                var originalPathBase = context.Features.Get<IAuthenticationFeature>()?.OriginalPathBase ?? context.Request.PathBase;


Should we consider having the handlers set a OriginalPathBase property on the CookieBuilder instead of having the builder pull it from the context?

HaoK · 2017-06-30T19:16:01Z

src/Microsoft.AspNetCore.Authentication/Internal/RequestPathBaseCookieBuilder.cs

+            var path = Path;
+            if (path == null)
+            {
+                var originalPathBase = context.Features.Get<IAuthenticationFeature>()?.OriginalPathBase ?? context.Request.PathBase;


Commenting again in the right version: Should we make this a property on RequestPathBaseCookieBuilder that the handlers set instead of making the builder pull it out?

Possibly racy. The builder is shared across multiple contexts.

natemcmaster · 2017-07-05T16:38:53Z

🔔

HaoK · 2017-07-05T16:51:57Z

src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs

            {
-                Logger.CorrelationPropertyNotFound(CorrelationPrefix);
+                Logger.CorrelationPropertyNotFound(RemoteAuthenticationOptions.CorrelationPrefix);


Shouldn't this be something like Options.CorrelationCookie.Name now?

Yup. Updated.

…Connect options with CookieBuilder

Tratcher · 2017-07-05T17:49:33Z

src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs


-            Response.Cookies.Delete(StateCookie, cookieOptions);
+            Response.Cookies.Delete(Options.StateCookie.Name, cookieOptions);


I'm trying to think if a user would Build their options any differently if they knew they were building a delete cookie.

natemcmaster requested review from Tratcher and HaoK June 29, 2017 23:43

dnfclas added the cla-already-signed label Jun 29, 2017

HaoK reviewed Jun 29, 2017

View reviewed changes

Tratcher reviewed Jun 30, 2017

View reviewed changes

natemcmaster force-pushed the namc/replace-cookie-builder branch from 2cbf29b to 0a740b2 Compare June 30, 2017 18:33

HaoK reviewed Jun 30, 2017

View reviewed changes

This was referenced Jun 30, 2017

Add CookieBuilder to CookieAuthenticationOptions and obsolete the duplicated properties #1285

Merged

Unifying API for configuring cookie settings aspnet/Announcements#257

Open

natemcmaster force-pushed the namc/replace-cookie-builder branch from 0403f05 to d7cc4bb Compare June 30, 2017 22:15

HaoK reviewed Jul 5, 2017

View reviewed changes

HaoK approved these changes Jul 5, 2017

View reviewed changes

Replace configure method on Twitter, RemoteAuthentication, and OpenId…

968237d

…Connect options with CookieBuilder

natemcmaster force-pushed the namc/replace-cookie-builder branch from a565a24 to 968237d Compare July 5, 2017 16:58

natemcmaster merged commit 968237d into dev Jul 5, 2017

natemcmaster deleted the namc/replace-cookie-builder branch July 5, 2017 17:09

natemcmaster mentioned this pull request Jul 5, 2017

Reduce duplication when configuring cookie options aspnet/HttpAbstractions#853

Closed

Tratcher reviewed Jul 5, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace configure method on TwitterOptions and OpenIdConnectOptions with CookieBuilder #1284

Replace configure method on TwitterOptions and OpenIdConnectOptions with CookieBuilder #1284

natemcmaster commented Jun 29, 2017

dnfclas commented Jun 29, 2017

HaoK Jun 29, 2017

natemcmaster Jun 29, 2017

HaoK Jun 29, 2017

natemcmaster Jun 30, 2017 •

edited

Loading

HaoK Jun 30, 2017 •

edited

Loading

HaoK Jun 30, 2017

HaoK Jun 30, 2017

natemcmaster Jun 30, 2017

Tratcher Jun 30, 2017

HaoK Jun 30, 2017

natemcmaster Jun 30, 2017

Tratcher Jun 30, 2017

natemcmaster Jun 30, 2017

Tratcher commented Jun 30, 2017

HaoK commented Jun 30, 2017

natemcmaster commented Jun 30, 2017

HaoK Jun 30, 2017

HaoK Jun 30, 2017

natemcmaster Jun 30, 2017

natemcmaster commented Jul 5, 2017

HaoK Jul 5, 2017

natemcmaster Jul 5, 2017

Tratcher Jul 5, 2017


		Response.Cookies.Delete(StateCookie, cookieOptions);
		Response.Cookies.Delete(Options.StateCookie.Name, cookieOptions);

Replace configure method on TwitterOptions and OpenIdConnectOptions with CookieBuilder #1284

Replace configure method on TwitterOptions and OpenIdConnectOptions with CookieBuilder #1284

Conversation

natemcmaster commented Jun 29, 2017

dnfclas commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

HaoK Jun 30, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Jun 30, 2017

HaoK commented Jun 30, 2017

natemcmaster commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster Jun 30, 2017 •

edited

Loading

HaoK Jun 30, 2017 •

edited

Loading