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

[CORL-1048] Cookie Deprecation #2944

Merged
merged 10 commits into from
May 13, 2020
Merged

[CORL-1048] Cookie Deprecation #2944

merged 10 commits into from
May 13, 2020

Conversation

wyattjoh
Copy link
Collaborator

What does this PR do?

New third party cookie policies created by Safari in their recent 13.1 release have been causing issues for organizations using Coral with our non-SSO based (email/password) login methods.

This is closely related to the issue described privacycg/storage-access#2 (comment). Our hopes is that the Storage Access API takes these issues into account during the development to provide guidelines for first-party SaaS social integrations while maintaining the security boundary of the iframe.

Other comment platforms render directly into the DOM of the top level frame which provide inferior security boundaries when compared to the isolation granted by the iframe.

This PR removes cookies entirely as they are transparent to the JS and therefore causes issues described in the below scenario. This PR instead tries to save access token details to localStorage whenever possible to ensure admin logins persist after login.

Problem Scenario

  1. The user first arrives at organization.com, where Coral is embedded from organization.coral.coralproject.net.
  2. The user scrolls to the comment widget, clicks "Sign In" to open a popup, thereby granting temporary storage access.
  3. The user signs in with valid credentials, and we set a cookie containing the access token.
  4. We then send the access token back through postMessage to the iframe, and the user can proceed logged in.
  5. The user refreshes or navigates on the top frame (on organization.com), and scrolls back to the comment section.

Because the user navigated, the storage access is removed, and the iframe no longer has storage access, and therefore does not show that the user is signed in. The issue we're experiencing now is when the user again clicks "Sign In":

  1. A popup opens for login.
  2. The popup (now granted storage access), sends a request to validate the current user session (where we now have the cookie attached).
  3. The response says we're logged in via cookie, and the popup sends a message to the iframe regarding the valid credentials.

Because we don't have access to the access token after the refresh (because of the HttpOnly flag on the cookie), we can't send it back to the iframe after the second popup. This means that the user is stuck, the iframe does not have permission to access or send cookies along with the request, and we don't want to expose the cookie to the JS.

What changes to the GraphQL/Database Schema does this PR introduce?

None.

How do I test this PR?

Try to use in Safari where the Coral domain differs from the embedder domain.

@wyattjoh wyattjoh added the bug label Apr 24, 2020
@wyattjoh wyattjoh added this to the v6.2.0 milestone Apr 24, 2020
@wyattjoh wyattjoh requested review from tessalt and cvle April 24, 2020 21:37
@wyattjoh wyattjoh temporarily deployed to pure-lowlands-73130 April 24, 2020 21:56 Inactive
@wyattjoh wyattjoh temporarily deployed to pure-lowlands-73130 April 27, 2020 16:32 Inactive
@wyattjoh wyattjoh temporarily deployed to pure-lowlands-73130 April 27, 2020 19:15 Inactive
Copy link
Contributor

@tessalt tessalt left a comment

Choose a reason for hiding this comment

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

works!

Copy link
Member

@cvle cvle left a comment

Choose a reason for hiding this comment

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

Putting a merge block for now, needs discussion

Copy link
Member

@cvle cvle left a comment

Choose a reason for hiding this comment

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

Actionlist from discussion with @wyattjoh :

  • Restore Relay Storage as source of truth
  • Remove Auth, JWTManager, cleanupCallbacks and simplify into functions (or a class if it really needs to^^)
  • clear session storage during clearSession
  • No need to deal with about-to-expire-tokens for now
  • Remove automatic pause/resume in createManagedSubscriptionClient

@cvle cvle removed the don't merge label May 7, 2020
@wyattjoh wyattjoh requested a review from cvle May 7, 2020 23:46
src/core/client/framework/lib/auth/auth.ts Outdated Show resolved Hide resolved
// Return the parsed access token.
return parseAccessToken(accessToken);
} catch (err) {
if (process.env.NODE_ENV !== "production") {
Copy link
Member

Choose a reason for hiding this comment

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

Just a question: What harm is there to output these errors in production?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From a users perspective, it's not likely that this would be used to debug issues. If we're concerned about tracking these errors, we should consider these types of guards as indicators to include error reporters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants