Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Same site cookie fix #261

Merged
merged 8 commits into from
Jan 20, 2020
Merged

Same site cookie fix #261

merged 8 commits into from
Jan 20, 2020

Conversation

TiagoBrenck
Copy link
Contributor

Purpose

In order to work with the "sameSite = none" compatibility problem, a method was added on Microsoft.Identity.Web using the logic suggested in this Microsoft doc.

I am opening this PR to get a feedback of the implementation used. If in agreement, I will apply the changes in all samples

What was done?

  • Updated to Core 3.1, so we can support the new SameSiteMode.Unspecified value (Core 3.0 workaround didnt work and a Github issue has been opened)
  • Created a static method HandleSameSiteCookieCompatibility (name is opened for suggestions) that injects the MS Doc suggested code in the CookiePolicyOptions
  • Added an option to extend the "check user-agent against list" so developers are free to pass their own implementation as parameter.
  • The default "check user-agent against list" method was extracted from MS Docs

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

}

// Method taken from https://devblogs.microsoft.com/aspnet/upcoming-samesite-cookie-changes-in-asp-net-and-asp-net-core/
public static bool DisallowsSameSiteNone(string userAgent)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this list covers all the necessary user agents. If there are more browsers, then maybe pulling this list from a config file would be more dynamic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list was suggested by Barry `s team, and he added this comment in his post:

Under testing with the Azure Active Directory team we have found the following checks work for all the common user agents that Azure Active Directory sees that don’t understand the new value.

But it is definitely not a bullet proof code.

For the list in the config file approach, I am challenged about how to represent a nested check in a list. For example, in the current one there is this nested logic:

userAgent.Contains("Macintosh; Intel Mac OS X 10_14") && 
        userAgent.Contains("Version/") && userAgent.Contains("Safari")

What I understood from this check, is to cover strings like Macintosh; Intel Mac OS X 10_14 <anything here> Version/ <anything here> Safari. How could we represent this in a config file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we'd release Microsoft.Identity.Web as a NuGet package we can quickly update it if a new browser is affected.
I'd think that we would not want to oblige every application developer to have to change a config file. @pmaytak (rather upgrade the NuGet package)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok Jean-Marc. Worst case scenario, the new method HandleSameSiteCookieCompatibility has an overload that accepts a custom validator method.

Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @TiagoBrenck
Given that @pmaytak has validated the fix with Chromium 60, I'd go ahead and apply to the other folders, and then the other samples.

@TiagoBrenck TiagoBrenck removed the do not merge PR not ready to merge yet label Jan 20, 2020
@jmprieur jmprieur self-requested a review January 20, 2020 22:30
Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @TiagoBrenck
the only outstanding issue was you also need to update the .NET Core sdk version of Microsoft.Identity.Web.Test, which I've done

@TiagoBrenck TiagoBrenck merged commit f25cdbd into master Jan 20, 2020
@jmprieur jmprieur deleted the tibre/sameSiteCookieFix branch February 10, 2020 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants