From 68180e5d85730ddcc77d06176a1af300fe668afa Mon Sep 17 00:00:00 2001 From: David De Smet <2607383+idaviddesmet@users.noreply.github.com> Date: Fri, 11 Feb 2022 16:13:54 -0600 Subject: [PATCH 1/8] Allow custom cookie consent value --- .../CookiePolicy/src/CookiePolicyOptions.cs | 6 ++ .../CookiePolicy/src/PublicAPI.Shipped.txt | 2 + .../src/ResponseCookiesWrapper.cs | 13 ++- .../CookiePolicy/test/CookieConsentTests.cs | 90 +++++++++++++++++++ 4 files changed, 107 insertions(+), 4 deletions(-) diff --git a/src/Security/CookiePolicy/src/CookiePolicyOptions.cs b/src/Security/CookiePolicy/src/CookiePolicyOptions.cs index a02092a6964b..d6377c3e48e1 100644 --- a/src/Security/CookiePolicy/src/CookiePolicyOptions.cs +++ b/src/Security/CookiePolicy/src/CookiePolicyOptions.cs @@ -37,6 +37,12 @@ public class CookiePolicyOptions IsEssential = true, }; + /// + /// Gets or sets the consent value that is used to track if the user consented to the + /// cookie use policy. The default is yes. + /// + public string ConsentCookieValue { get; set; } = "yes"; + /// /// Checks if consent policies should be evaluated on this request. The default is false. /// diff --git a/src/Security/CookiePolicy/src/PublicAPI.Shipped.txt b/src/Security/CookiePolicy/src/PublicAPI.Shipped.txt index 4cd46d9bc9bd..5e8ca89484db 100644 --- a/src/Security/CookiePolicy/src/PublicAPI.Shipped.txt +++ b/src/Security/CookiePolicy/src/PublicAPI.Shipped.txt @@ -7,6 +7,8 @@ Microsoft.AspNetCore.Builder.CookiePolicyOptions.CheckConsentNeeded.get -> Syste Microsoft.AspNetCore.Builder.CookiePolicyOptions.CheckConsentNeeded.set -> void Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookie.get -> Microsoft.AspNetCore.Http.CookieBuilder! Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookie.set -> void +Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookieValue.get -> string! +Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookieValue.set -> void Microsoft.AspNetCore.Builder.CookiePolicyOptions.CookiePolicyOptions() -> void Microsoft.AspNetCore.Builder.CookiePolicyOptions.HttpOnly.get -> Microsoft.AspNetCore.CookiePolicy.HttpOnlyPolicy Microsoft.AspNetCore.Builder.CookiePolicyOptions.HttpOnly.set -> void diff --git a/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs b/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs index 11a74ffa85b7..5c8418d19fda 100644 --- a/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs +++ b/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs @@ -12,7 +12,7 @@ namespace Microsoft.AspNetCore.CookiePolicy; internal class ResponseCookiesWrapper : IResponseCookies, ITrackingConsentFeature { - private const string ConsentValue = "yes"; + private const string DefaultConsentValue = "yes"; private readonly ILogger _logger; private bool? _isConsentNeeded; private bool? _hasConsent; @@ -23,6 +23,11 @@ public ResponseCookiesWrapper(HttpContext context, CookiePolicyOptions options, Feature = feature; Options = options; _logger = logger; + + if (string.IsNullOrWhiteSpace(Options.ConsentCookieValue)) + { + Options.ConsentCookieValue = DefaultConsentValue; + } } private HttpContext Context { get; } @@ -55,7 +60,7 @@ public bool HasConsent if (!_hasConsent.HasValue) { var cookie = Context.Request.Cookies[Options.ConsentCookie.Name!]; - _hasConsent = string.Equals(cookie, ConsentValue, StringComparison.Ordinal); + _hasConsent = string.Equals(cookie, Options.ConsentCookieValue, StringComparison.Ordinal); _logger.HasConsent(_hasConsent.Value); } @@ -71,7 +76,7 @@ public void GrantConsent() { var cookieOptions = Options.ConsentCookie.Build(Context); // Note policy will be applied. We don't want to bypass policy because we want HttpOnly, Secure, etc. to apply. - Append(Options.ConsentCookie.Name!, ConsentValue, cookieOptions); + Append(Options.ConsentCookie.Name!, Options.ConsentCookieValue, cookieOptions); _logger.ConsentGranted(); } _hasConsent = true; @@ -93,7 +98,7 @@ public void WithdrawConsent() public string CreateConsentCookie() { var key = Options.ConsentCookie.Name; - var value = ConsentValue; + var value = Options.ConsentCookieValue; var options = Options.ConsentCookie.Build(Context); Debug.Assert(key != null); diff --git a/src/Security/CookiePolicy/test/CookieConsentTests.cs b/src/Security/CookiePolicy/test/CookieConsentTests.cs index 23144105605c..73c5a5b170c7 100644 --- a/src/Security/CookiePolicy/test/CookieConsentTests.cs +++ b/src/Security/CookiePolicy/test/CookieConsentTests.cs @@ -638,6 +638,96 @@ public async Task CreateConsentCookieAppliesPolicy() Assert.NotNull(manualCookie.Expires); // Expires may not exactly match to the second. } + [Fact] + public async Task CreateConsentCookieMatchesGrantConsentCookieWhenCookieValueIsCustom() + { + var httpContext = await RunTestAsync(options => + { + options.CheckConsentNeeded = context => true; + options.ConsentCookieValue = "true"; + }, + requestContext => { }, + context => + { + var feature = context.Features.Get(); + Assert.True(feature.IsConsentNeeded); + Assert.False(feature.HasConsent); + Assert.False(feature.CanTrack); + + feature.GrantConsent(); + + Assert.True(feature.IsConsentNeeded); + Assert.True(feature.HasConsent); + Assert.True(feature.CanTrack); + + var cookie = feature.CreateConsentCookie(); + context.Response.Headers["ManualCookie"] = cookie; + + return Task.CompletedTask; + }); + + var cookies = SetCookieHeaderValue.ParseList(httpContext.Response.Headers.SetCookie); + Assert.Equal(1, cookies.Count); + var consentCookie = cookies[0]; + Assert.Equal(".AspNet.Consent", consentCookie.Name); + Assert.Equal("true", consentCookie.Value); + Assert.Equal(Net.Http.Headers.SameSiteMode.Unspecified, consentCookie.SameSite); + Assert.NotNull(consentCookie.Expires); + + cookies = SetCookieHeaderValue.ParseList(httpContext.Response.Headers["ManualCookie"]); + Assert.Equal(1, cookies.Count); + var manualCookie = cookies[0]; + Assert.Equal(consentCookie.Name, manualCookie.Name); + Assert.Equal(consentCookie.Value, manualCookie.Value); + Assert.Equal(consentCookie.SameSite, manualCookie.SameSite); + Assert.NotNull(manualCookie.Expires); // Expires may not exactly match to the second. + } + + [Fact] + public async Task CreateConsentCookieMatchesGrantConsentCookieWhenCookieValueIsMissing() + { + var httpContext = await RunTestAsync(options => + { + options.CheckConsentNeeded = context => true; + options.ConsentCookieValue = ""; + }, + requestContext => { }, + context => + { + var feature = context.Features.Get(); + Assert.True(feature.IsConsentNeeded); + Assert.False(feature.HasConsent); + Assert.False(feature.CanTrack); + + feature.GrantConsent(); + + Assert.True(feature.IsConsentNeeded); + Assert.True(feature.HasConsent); + Assert.True(feature.CanTrack); + + var cookie = feature.CreateConsentCookie(); + context.Response.Headers["ManualCookie"] = cookie; + + return Task.CompletedTask; + }); + + var cookies = SetCookieHeaderValue.ParseList(httpContext.Response.Headers.SetCookie); + Assert.Equal(1, cookies.Count); + var consentCookie = cookies[0]; + Assert.Equal(".AspNet.Consent", consentCookie.Name); + Assert.Equal("yes", consentCookie.Value); + Assert.Equal(Net.Http.Headers.SameSiteMode.Unspecified, consentCookie.SameSite); + Assert.NotNull(consentCookie.Expires); + + cookies = SetCookieHeaderValue.ParseList(httpContext.Response.Headers["ManualCookie"]); + Assert.Equal(1, cookies.Count); + var manualCookie = cookies[0]; + Assert.Equal(consentCookie.Name, manualCookie.Name); + Assert.Equal(consentCookie.Value, manualCookie.Value); + Assert.Equal(consentCookie.SameSite, manualCookie.SameSite); + Assert.NotNull(manualCookie.Expires); // Expires may not exactly match to the second. + } + private async Task RunTestAsync(Action configureOptions, Action configureRequest, RequestDelegate handleRequest) { var host = new HostBuilder() From 21eb8808981083732fa7d586829dfb2d2154d835 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 14 Feb 2022 13:29:40 -0800 Subject: [PATCH 2/8] Fixup --- src/Security/CookiePolicy/src/CookiePolicyOptions.cs | 5 +++-- src/Security/CookiePolicy/src/PublicAPI.Shipped.txt | 2 -- src/Security/CookiePolicy/src/PublicAPI.Unshipped.txt | 2 ++ 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Security/CookiePolicy/src/CookiePolicyOptions.cs b/src/Security/CookiePolicy/src/CookiePolicyOptions.cs index d6377c3e48e1..6149eeb92987 100644 --- a/src/Security/CookiePolicy/src/CookiePolicyOptions.cs +++ b/src/Security/CookiePolicy/src/CookiePolicyOptions.cs @@ -38,9 +38,10 @@ public class CookiePolicyOptions }; /// - /// Gets or sets the consent value that is used to track if the user consented to the - /// cookie use policy. The default is yes. + /// Gets or sets the value for the cookie used to track if the user consented to the + /// cookie use policy. /// + /// Defaults to yes. public string ConsentCookieValue { get; set; } = "yes"; /// diff --git a/src/Security/CookiePolicy/src/PublicAPI.Shipped.txt b/src/Security/CookiePolicy/src/PublicAPI.Shipped.txt index 5e8ca89484db..4cd46d9bc9bd 100644 --- a/src/Security/CookiePolicy/src/PublicAPI.Shipped.txt +++ b/src/Security/CookiePolicy/src/PublicAPI.Shipped.txt @@ -7,8 +7,6 @@ Microsoft.AspNetCore.Builder.CookiePolicyOptions.CheckConsentNeeded.get -> Syste Microsoft.AspNetCore.Builder.CookiePolicyOptions.CheckConsentNeeded.set -> void Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookie.get -> Microsoft.AspNetCore.Http.CookieBuilder! Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookie.set -> void -Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookieValue.get -> string! -Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookieValue.set -> void Microsoft.AspNetCore.Builder.CookiePolicyOptions.CookiePolicyOptions() -> void Microsoft.AspNetCore.Builder.CookiePolicyOptions.HttpOnly.get -> Microsoft.AspNetCore.CookiePolicy.HttpOnlyPolicy Microsoft.AspNetCore.Builder.CookiePolicyOptions.HttpOnly.set -> void diff --git a/src/Security/CookiePolicy/src/PublicAPI.Unshipped.txt b/src/Security/CookiePolicy/src/PublicAPI.Unshipped.txt index 7dc5c58110bf..b37004541aaa 100644 --- a/src/Security/CookiePolicy/src/PublicAPI.Unshipped.txt +++ b/src/Security/CookiePolicy/src/PublicAPI.Unshipped.txt @@ -1 +1,3 @@ #nullable enable +Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookieValue.get -> string! +Microsoft.AspNetCore.Builder.CookiePolicyOptions.ConsentCookieValue.set -> void \ No newline at end of file From 1085ba39bb36ccabda30024d33c79731805175e4 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 14 Feb 2022 13:32:51 -0800 Subject: [PATCH 3/8] Fixup2 --- .../CookiePolicy/src/CookiePolicyOptions.cs | 15 ++++++++++++++- .../CookiePolicy/src/ResponseCookiesWrapper.cs | 5 ----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/Security/CookiePolicy/src/CookiePolicyOptions.cs b/src/Security/CookiePolicy/src/CookiePolicyOptions.cs index 6149eeb92987..9dd0d3fba185 100644 --- a/src/Security/CookiePolicy/src/CookiePolicyOptions.cs +++ b/src/Security/CookiePolicy/src/CookiePolicyOptions.cs @@ -11,6 +11,8 @@ namespace Microsoft.AspNetCore.Builder; /// public class CookiePolicyOptions { + private string _consentCookieValue = "yes"; + /// /// Affects the cookie's same site attribute. /// @@ -42,7 +44,18 @@ public class CookiePolicyOptions /// cookie use policy. /// /// Defaults to yes. - public string ConsentCookieValue { get; set; } = "yes"; + public string ConsentCookieValue + { + get => _consentCookieValue; + set + { + if (string.IsNullOrEmpty(value)) + { + throw new ArgumentException("Value cannot be null or empty string.", nameof(value)); + } + _consentCookieValue = value; + } + } /// /// Checks if consent policies should be evaluated on this request. The default is false. diff --git a/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs b/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs index 5c8418d19fda..0e2b934a349e 100644 --- a/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs +++ b/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs @@ -23,11 +23,6 @@ public ResponseCookiesWrapper(HttpContext context, CookiePolicyOptions options, Feature = feature; Options = options; _logger = logger; - - if (string.IsNullOrWhiteSpace(Options.ConsentCookieValue)) - { - Options.ConsentCookieValue = DefaultConsentValue; - } } private HttpContext Context { get; } From 81519d3048476f557d58b8a32f0b12733b254cc3 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 14 Feb 2022 13:33:16 -0800 Subject: [PATCH 4/8] Update src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs --- src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs b/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs index 0e2b934a349e..2de742c7b29a 100644 --- a/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs +++ b/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs @@ -12,7 +12,6 @@ namespace Microsoft.AspNetCore.CookiePolicy; internal class ResponseCookiesWrapper : IResponseCookies, ITrackingConsentFeature { - private const string DefaultConsentValue = "yes"; private readonly ILogger _logger; private bool? _isConsentNeeded; private bool? _hasConsent; From 37668a41fdaeb587ce68a914bbd460ab12aa3a36 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 14 Feb 2022 13:46:28 -0800 Subject: [PATCH 5/8] Remove test --- .../CookiePolicy/test/CookieConsentTests.cs | 45 ------------------- 1 file changed, 45 deletions(-) diff --git a/src/Security/CookiePolicy/test/CookieConsentTests.cs b/src/Security/CookiePolicy/test/CookieConsentTests.cs index 73c5a5b170c7..65861f365e50 100644 --- a/src/Security/CookiePolicy/test/CookieConsentTests.cs +++ b/src/Security/CookiePolicy/test/CookieConsentTests.cs @@ -683,51 +683,6 @@ public async Task CreateConsentCookieMatchesGrantConsentCookieWhenCookieValueIsC Assert.NotNull(manualCookie.Expires); // Expires may not exactly match to the second. } - [Fact] - public async Task CreateConsentCookieMatchesGrantConsentCookieWhenCookieValueIsMissing() - { - var httpContext = await RunTestAsync(options => - { - options.CheckConsentNeeded = context => true; - options.ConsentCookieValue = ""; - }, - requestContext => { }, - context => - { - var feature = context.Features.Get(); - Assert.True(feature.IsConsentNeeded); - Assert.False(feature.HasConsent); - Assert.False(feature.CanTrack); - - feature.GrantConsent(); - - Assert.True(feature.IsConsentNeeded); - Assert.True(feature.HasConsent); - Assert.True(feature.CanTrack); - - var cookie = feature.CreateConsentCookie(); - context.Response.Headers["ManualCookie"] = cookie; - - return Task.CompletedTask; - }); - - var cookies = SetCookieHeaderValue.ParseList(httpContext.Response.Headers.SetCookie); - Assert.Equal(1, cookies.Count); - var consentCookie = cookies[0]; - Assert.Equal(".AspNet.Consent", consentCookie.Name); - Assert.Equal("yes", consentCookie.Value); - Assert.Equal(Net.Http.Headers.SameSiteMode.Unspecified, consentCookie.SameSite); - Assert.NotNull(consentCookie.Expires); - - cookies = SetCookieHeaderValue.ParseList(httpContext.Response.Headers["ManualCookie"]); - Assert.Equal(1, cookies.Count); - var manualCookie = cookies[0]; - Assert.Equal(consentCookie.Name, manualCookie.Name); - Assert.Equal(consentCookie.Value, manualCookie.Value); - Assert.Equal(consentCookie.SameSite, manualCookie.SameSite); - Assert.NotNull(manualCookie.Expires); // Expires may not exactly match to the second. - } - private async Task RunTestAsync(Action configureOptions, Action configureRequest, RequestDelegate handleRequest) { var host = new HostBuilder() From 1eca79e2c0c419c660662a403541c5e41bb2b53f Mon Sep 17 00:00:00 2001 From: David De Smet <2607383+idaviddesmet@users.noreply.github.com> Date: Mon, 14 Feb 2022 16:10:10 -0600 Subject: [PATCH 6/8] Added UT for ConsentCookieValue when missing --- src/Security/CookiePolicy/test/CookieConsentTests.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/Security/CookiePolicy/test/CookieConsentTests.cs b/src/Security/CookiePolicy/test/CookieConsentTests.cs index 65861f365e50..13c05b4b9a9d 100644 --- a/src/Security/CookiePolicy/test/CookieConsentTests.cs +++ b/src/Security/CookiePolicy/test/CookieConsentTests.cs @@ -683,6 +683,17 @@ public async Task CreateConsentCookieMatchesGrantConsentCookieWhenCookieValueIsC Assert.NotNull(manualCookie.Expires); // Expires may not exactly match to the second. } + [Fact] + public void CreateCookiePolicyOptionsWithEmptyConsentCookieValueThrows() + { + var options = new CookiePolicyOptions(); + + var exceptionWhenEmpty = Assert.Throws(() => options.ConsentCookieValue = ""); + var exceptionWhenNull = Assert.Throws(() => options.ConsentCookieValue = null); + Assert.Equal("Value cannot be null or empty string. (Parameter 'value')", exceptionWhenEmpty.Message); + Assert.Equal("Value cannot be null or empty string. (Parameter 'value')", exceptionWhenNull.Message); + } + private async Task RunTestAsync(Action configureOptions, Action configureRequest, RequestDelegate handleRequest) { var host = new HostBuilder() From 5ca332311234fdbee5e49e6d4d0ea5f19b1223b8 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 14 Feb 2022 14:17:17 -0800 Subject: [PATCH 7/8] Fixup --- .../CookiePolicy/test/CookieConsentTests.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/Security/CookiePolicy/test/CookieConsentTests.cs b/src/Security/CookiePolicy/test/CookieConsentTests.cs index 13c05b4b9a9d..5acae0cecfe4 100644 --- a/src/Security/CookiePolicy/test/CookieConsentTests.cs +++ b/src/Security/CookiePolicy/test/CookieConsentTests.cs @@ -6,6 +6,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.TestHost; +using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Net.Http.Headers; @@ -683,15 +684,17 @@ public async Task CreateConsentCookieMatchesGrantConsentCookieWhenCookieValueIsC Assert.NotNull(manualCookie.Expires); // Expires may not exactly match to the second. } - [Fact] - public void CreateCookiePolicyOptionsWithEmptyConsentCookieValueThrows() + [Theory] + [InlineData(null)] + [InlineData("")] + public void CreateCookiePolicyOptionsWithEmptyConsentCookieValueThrows(string value) { var options = new CookiePolicyOptions(); - var exceptionWhenEmpty = Assert.Throws(() => options.ConsentCookieValue = ""); - var exceptionWhenNull = Assert.Throws(() => options.ConsentCookieValue = null); - Assert.Equal("Value cannot be null or empty string. (Parameter 'value')", exceptionWhenEmpty.Message); - Assert.Equal("Value cannot be null or empty string. (Parameter 'value')", exceptionWhenNull.Message); + ExceptionAssert.ThrowsArgument( + () => options.ConsentCookieValue = value, + "value", + "Value cannot be null or empty string."); } private async Task RunTestAsync(Action configureOptions, Action configureRequest, RequestDelegate handleRequest) From c600c27676f1ac29a73fc5985f7b810d0ad84763 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 14 Feb 2022 16:30:03 -0800 Subject: [PATCH 8/8] Update CookieConsentTests.cs --- src/Security/CookiePolicy/test/CookieConsentTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Security/CookiePolicy/test/CookieConsentTests.cs b/src/Security/CookiePolicy/test/CookieConsentTests.cs index 5acae0cecfe4..e217f18da910 100644 --- a/src/Security/CookiePolicy/test/CookieConsentTests.cs +++ b/src/Security/CookiePolicy/test/CookieConsentTests.cs @@ -691,7 +691,7 @@ public void CreateCookiePolicyOptionsWithEmptyConsentCookieValueThrows(string va { var options = new CookiePolicyOptions(); - ExceptionAssert.ThrowsArgument( + ExceptionAssert.ThrowsArgument( () => options.ConsentCookieValue = value, "value", "Value cannot be null or empty string.");