React to SameSite addition to CookieOptions #141

JunTaoLuo · 2017-05-22T08:20:27Z

#127 We should be setting the default to strict?

rynowak · 2017-05-22T15:15:14Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenStore.cs

+            {
+                HttpOnly = true,
+                Domain = _options.CookieDomain,
+                SameSite = SameSiteEnforcementMode.Strict


I am not familiar with this option, Let's have a conversation with @blowdart - we need to understand what should be the default and if this should be configurable.

Spoke offline, I think this should remain as strict as we expect the cookie to be created by requests from the same site as subsequent requests. There's no need to make this configurable.

JunTaoLuo · 2017-05-31T22:24:08Z

Updated based on @blowdart's recommendation to make this option configurable. This is ready to merge.

rynowak · 2017-06-01T00:10:20Z

Sorry, what's the reason for making this configurable?

rynowak · 2017-06-01T01:31:49Z

If we're going to make this configurable we should attach an Action<CookieOptions> to AntiforgeryOptions and be done. Then users are empowered to do whatever they want and we don't end up duplicating all of CookieOptions on AntiforgeryOptions.

JunTaoLuo · 2017-06-01T21:55:35Z

src/Microsoft.AspNetCore.Antiforgery/AntiforgeryOptions.cs

        /// </summary>
-        public string CookieDomain { get; set; }
+        public Action<CookieOptions> ConfigureCookieOptions { get; set; }


This is now a breaking change that requires an announcement.

Get @Eilon to sign off on the breaking change, I don't get to approve these

Added him as a reviewer and will speak with him about the breaking change. Can you review the rest of the PR?

JunTaoLuo · 2017-06-01T21:56:54Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenStore.cs

@@ -80,6 +84,8 @@ public void SaveCookieToken(HttpContext httpContext, string token)
            }
            SetCookiePath(httpContext, options);

+            _options.ConfigureCookieOptions?.Invoke(options);


I call the new ConfigureCookieOptions last after everything else is set. If we don't want users to be able to override Secure or Path on the CookieOptions, we can move this up, though I don't see why we might want that.

Do we also want to configure the cookie path and secure in the delegate?

Personally I think allowing you to set both is fine. RequireSSL is also enforced on the validation side, so if you mess it up, you'll just get a 400.

We should double check with @blowdart

Consistency is good, but folks are already using the old way. I assume you don't want to remove the existing path, just add a way to set it here to?

Yea, not changing the existing behaviour. Calling the configure delegate here allows the user to overwrite what we set but as @rynowak mentioned, they will be doing so at their own risk.

rynowak · 2017-06-02T00:42:48Z

test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTokenStoreTest.cs

@@ -364,7 +364,7 @@ public void SaveCookieToken_NonNullAntiforgeryOptionsCookieDomain_UsesOptionsCoo
                .Returns("/index.html");
            var options = new AntiforgeryOptions();
            options.CookieName = _cookieName;
-            options.CookieDomain = expectedCookieDomain;
+            options.ConfigureCookieOptions = cookieOptions => cookieOptions.Domain = expectedCookieDomain;


Change the name of this test to reflect the change.

rynowak

from me, let's get @Eilon and @blowdart to approve of what's changing here

rynowak · 2017-06-02T20:57:12Z

src/Microsoft.AspNetCore.Antiforgery/AntiforgeryOptions.cs

@@ -54,12 +54,21 @@ public string CookieName
        public PathString? CookiePath { get; set; }


This should be obsolete too, it's redundant with ConfigureCookieOptions

Tratcher · 2017-06-02T20:57:38Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenStore.cs

+            var options = new CookieOptions
+            {
+                HttpOnly = true,
+                Domain = null,


still use the obsolete domain property

rynowak · 2017-06-02T20:57:50Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenStore.cs

@@ -69,9 +69,13 @@ public void SaveCookieToken(HttpContext httpContext, string token)
            Debug.Assert(httpContext != null);
            Debug.Assert(token != null);

-            var options = new CookieOptions();
-            options.HttpOnly = true;
-            options.Domain = _options.CookieDomain;


We still need to use _options.CookieDomain - it's not being removed.

JunTaoLuo · 2017-06-02T21:03:45Z

🆙📅 edit oops need to ignore obsolete errors

Tratcher · 2017-06-02T21:06:23Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenStore.cs

+                Domain = null,
+                SameSite = SameSiteMode.Strict
+            };
+
            // Note: don't use "newCookie.Secure = _options.RequireSSL;" since the default
            // value of newCookie.Secure is populated out of band.
            if (_options.RequireSsl)


This is misleading, remove it.

JunTaoLuo · 2017-06-02T21:18:38Z

As per discussion with @Tratcher, we will probably adopt this approach for other components that duplicate settings on CookieOptions, potentially updating things like Session. Will file an issue in HttpAbstractions to track. Filed aspnet/HttpAbstractions#853.

- allows configuration of CookieOptions such as SameSite without explicit duplication of the option on AntiforgeryOptions

JunTaoLuo requested review from blowdart and Tratcher May 22, 2017 08:20

dnfclas added the cla-already-signed label May 22, 2017

rynowak reviewed May 22, 2017

View reviewed changes

JunTaoLuo force-pushed the johluo/samesite branch from 7f3be63 to 752996d Compare May 26, 2017 01:13

JunTaoLuo force-pushed the johluo/samesite branch from 752996d to 93399a3 Compare June 1, 2017 21:54

JunTaoLuo commented Jun 1, 2017

View reviewed changes

JunTaoLuo requested a review from Eilon June 1, 2017 23:56

rynowak reviewed Jun 2, 2017

View reviewed changes

rynowak approved these changes Jun 2, 2017

View reviewed changes

rynowak reviewed Jun 2, 2017

View reviewed changes

Tratcher reviewed Jun 2, 2017

View reviewed changes

rynowak reviewed Jun 2, 2017

View reviewed changes

Tratcher reviewed Jun 2, 2017

View reviewed changes

rynowak approved these changes Jun 2, 2017

View reviewed changes

Tratcher approved these changes Jun 2, 2017

View reviewed changes

JunTaoLuo mentioned this pull request Jun 2, 2017

Reduce duplication when configuring cookie options aspnet/HttpAbstractions#853

Closed

Add configure delegate for CookieOptions

5870fce

- allows configuration of CookieOptions such as SameSite without explicit duplication of the option on AntiforgeryOptions

JunTaoLuo force-pushed the johluo/samesite branch from 2904563 to 5870fce Compare June 2, 2017 21:35

JunTaoLuo changed the base branch from dev to rel/2.0.0-preview2 June 2, 2017 21:35

JunTaoLuo merged commit 5870fce into rel/2.0.0-preview2 Jun 2, 2017

JunTaoLuo deleted the johluo/samesite branch June 2, 2017 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React to SameSite addition to CookieOptions #141

React to SameSite addition to CookieOptions #141

JunTaoLuo commented May 22, 2017

rynowak May 22, 2017

JunTaoLuo May 25, 2017

JunTaoLuo commented May 31, 2017

rynowak commented Jun 1, 2017

rynowak commented Jun 1, 2017

JunTaoLuo Jun 1, 2017

rynowak Jun 1, 2017

JunTaoLuo Jun 1, 2017

JunTaoLuo Jun 1, 2017

JunTaoLuo Jun 1, 2017

rynowak Jun 2, 2017

blowdart Jun 2, 2017

JunTaoLuo Jun 2, 2017

rynowak Jun 2, 2017

JunTaoLuo Jun 2, 2017

rynowak left a comment

rynowak Jun 2, 2017

Tratcher Jun 2, 2017

rynowak Jun 2, 2017

JunTaoLuo Jun 2, 2017

JunTaoLuo commented Jun 2, 2017 •

edited

Loading

Tratcher Jun 2, 2017

JunTaoLuo commented Jun 2, 2017 •

edited

Loading

		@@ -54,12 +54,21 @@ public string CookieName
		public PathString? CookiePath { get; set; }

React to SameSite addition to CookieOptions #141

React to SameSite addition to CookieOptions #141

Conversation

JunTaoLuo commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo commented May 31, 2017

rynowak commented Jun 1, 2017

rynowak commented Jun 1, 2017

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

Choose a reason for hiding this comment

rynowak left a 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

JunTaoLuo commented Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

JunTaoLuo commented Jun 2, 2017 • edited Loading

JunTaoLuo commented Jun 2, 2017 •

edited

Loading

JunTaoLuo commented Jun 2, 2017 •

edited

Loading