cookie is the authority on if user is signed in #18594

Bjvanminnen · 2017-10-23T23:16:04Z

This hasn't been fully tested yet, but I wanted to get feedback on the approach first. I'm not certain how I feel about this - I like that there become a lot fewer ways of checking/setting sign in state, but don't like how it requires sticking data on window.

A recent change (https://github.com/code-dot-org/code-dot-org/pull/18299/files) tried to take advantage of the work we already do to cache user name for our sign in button in the upper right. It had some incorrect understandings, which are currently harmless, but only because that code wouldnt (currently) be called on any cached-scripts.

How that user cache actually works

In user_header.haml we create some DOM that has the name/id of the current_user set. On cached pages this will still be nil
We later execute some JS on that page that looks for a cookie, and if that cookie has a different name, it updates the DOM to have the correct name

What this PR does

Instead of trying to figure out sign in state in a bunch of different places, let the cookie be the canonical representation (with fallback to looking at the DOM).
As part of code-studio, look for the cookie, and then dispatch an action to our store where having the cookie implies we're signed in.
One challenge is that because this cookie is keyed off of rack_env, we need to let dashboard tell us where to find the cookie somehow. It might be that there's a better way to do this.

Bjvanminnen · 2017-10-23T23:16:35Z

apps/src/sites/studio/pages/code-studio.js

+  if (window.userNameCookieKey) {
+    const val = cookies.get(window.cookieKey);
+    if (val) {
+      store.dispatch(setUserSignedIn(true));


Should probably always dispatch setUserSignedIn, but with false if we don't have our cookie.

Bjvanminnen · 2017-10-23T23:17:24Z

shared/haml/user_header.haml

@@ -37,6 +37,9 @@
  // Use function closure to avoid cluttering global namespace.
  (function() {
    var cookieKey = '#{'_shortName' + (rack_env?(:production) ? '' : "_#{rack_env}")}';
+    // Because we use rack_env in our cookie key, the client has no way of knowing
+    // what key to look for. Stash it on window so that clients can access this.
+    window.userNameCookieKey = cookieKey;


Don't really like this, but not sure how I can do better. One option would be to instead pass rack_env and let client generate cookieKey from this

I don't mind this actually - and I think it's strictly better than duplicating the logic for constructing the key from the environment on the client. As a cross-cutting concern and (I think) strictly a constant within its rack_env I don't think this pollutes the global namespace too much.

Bjvanminnen · 2017-10-23T23:35:12Z

apps/src/sites/studio/pages/code-studio.js

+      // that are not cached, the server will set the user id in the DOM if you're
+      // signed in so check that as well.
+      const nameSpan = document.querySelector('.header_button.header_user.user_menu .user_name');
+      store.dispatch(setUserSignedIn(nameSpan && nameSpan.dataset.id));


I think the only way this happens is if the user deletes their cookies. In that case, we'll still fill in their user name correctly on non-cached scripts because the server set dataset.id. We should be as resilient as the header button.

I don't understand how this works. Does Rails have a way to identify the user's session that doesn't depend on cookies?

Good question. I only deleted the _shortName cookie. I think there's another session cookie that probably still has user info (and allows the server to reconstruct the _shortName cookie). That does mean we probably don't really need to care about this scenario since people are probably deleting all of their cookies (in which case server shouldnt know who you are) or none of them.

islemaster · 2017-10-23T23:47:02Z

This is relevant to my electron work - I'll read through it soon.

Bjvanminnen · 2017-10-24T16:17:01Z

apps/src/sites/studio/pages/code-studio.js

@@ -10,6 +10,9 @@ import $ from 'jquery';

 import { getStore } from '@cdo/apps/code-studio/redux';
 import { setRtlFromDOM } from '@cdo/apps/code-studio/isRtlRedux';
+import { setUserSignedIn } from '@cdo/apps/code-studio/progressRedux';


One other thing to note is that at this point signInState probably deserves its own reducer - progress doesn't really seem like the right place for it. I'm not going to make that change at this point though.

Bjvanminnen · 2017-10-24T17:22:42Z

apps/src/code-studio/progressRedux.js

+    const nameSpan = document.querySelector('.header_button.header_user.user_menu .user_name');
+    return !!(nameSpan && nameSpan.dataset.id);
+  }
+}


This definitely feels out of place in this file. I think it's okay for now, but in the future sign in state probably deserves its own reducer.

Bjvanminnen · 2017-10-24T17:23:40Z

@islemaster This is now ready for a real review. I did a little bit of refactoring, and also adding some tests. Thanks!

Bjvanminnen commented Oct 23, 2017

View reviewed changes

Bjvanminnen requested a review from islemaster October 23, 2017 23:36

islemaster self-assigned this Oct 23, 2017

Bjvanminnen commented Oct 24, 2017

View reviewed changes

Bjvanminnen force-pushed the userSigninState branch from c5eefec to a284b0e Compare October 24, 2017 17:21

Bjvanminnen changed the title ~~RFC: cookie is the authority on if user is signed in~~ cookie is the authority on if user is signed in Oct 24, 2017

Bjvanminnen added 3 commits October 24, 2017 10:22

cookie is the authority on if user is signed in

8fed840

handle case where no cookie better

833b065

refactor and tests

a284b0e

Bjvanminnen commented Oct 24, 2017

View reviewed changes

fix tests

d8e4ca0

islemaster approved these changes Oct 24, 2017

View reviewed changes

Bjvanminnen merged commit 0f63206 into staging Oct 24, 2017

Bjvanminnen deleted the userSigninState branch October 24, 2017 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cookie is the authority on if user is signed in #18594

cookie is the authority on if user is signed in #18594

Bjvanminnen commented Oct 23, 2017 •

edited

Bjvanminnen Oct 23, 2017

Bjvanminnen Oct 23, 2017

islemaster Oct 24, 2017

Bjvanminnen Oct 23, 2017

islemaster Oct 24, 2017

Bjvanminnen Oct 24, 2017

islemaster commented Oct 23, 2017

Bjvanminnen Oct 24, 2017

Bjvanminnen Oct 24, 2017

Bjvanminnen commented Oct 24, 2017

cookie is the authority on if user is signed in #18594

cookie is the authority on if user is signed in #18594

Conversation

Bjvanminnen commented Oct 23, 2017 • edited

How that user cache actually works

What this PR does

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

islemaster commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bjvanminnen commented Oct 24, 2017

Bjvanminnen commented Oct 23, 2017 •

edited