hook up login + logout with commonplace via shared token (bug 970598) #1763

Merged
merged 1 commit into from Apr 5, 2014

Projects

None yet

4 participants

@ngokevin
Mozilla member

(Related to mozilla/commonplace#24): "Not sure if this is what we want, but this allows centralized login/logout so there is no disparity between who you are logged in as no matter which component of Marketplace you are on."

  • Set token in localStorage when logging in. Clear token from localStorage when logging out.
  • Have Marketplace login use accounts API rather than users.views:browserid_authenticate.
  • When logging in via accounts Login API view, also log into Django's auth.
  • Add a accounts Logout API view that logs out from Django's auth.
@cvan cvan and 1 other commented on an outdated diff Feb 13, 2014
media/js/devreg/login.js
@@ -1,5 +1,6 @@
define('login', ['notification'], function(notification) {
+ var STORAGE_VERSION = '0';
@cvan
cvan Feb 13, 2014

we're gonna hardcode this? I don't have a better idea though

@ngokevin
ngokevin Feb 19, 2014

Perhaps I could pass it in through Django settings into a data attribute?

@cvan
cvan Feb 26, 2014

ya think we can set in fireplace localSettings.latestStorageVersion?

@ngokevin
ngokevin Feb 26, 2014

Don't know if we can require Fireplace's settings file from Devhub's js?

@cvan cvan and 1 other commented on an outdated diff Feb 13, 2014
media/js/devreg/login.js
@@ -121,6 +126,22 @@ define('login', ['notification'], function(notification) {
});
}
+ function _prefix(storageKey) {
+ return STORAGE_VERSION + '::' + storageKey;
+ }
+
+ function setToken(data) {
@cvan
cvan Feb 13, 2014

where do you get the token?

@ngokevin
ngokevin Feb 13, 2014

accounts.views.LoginView response.

@cvan
cvan Feb 13, 2014

I mean where do we use the token? I don't see any localStorage.getItem

@cvan
Mozilla member

what about Reviewer Tools?

@ngokevin
Mozilla member

Reviewer Tools uses the same stuff as Developer Pages.

@robhudson robhudson and 1 other commented on an outdated diff Feb 13, 2014
mkt/account/views.py
@@ -154,6 +159,7 @@ def create_action(self, request, serializer):
request.groups = profile.groups.all()
# TODO: move this to the signal.
@robhudson
robhudson Feb 13, 2014

Does the TODO make sense? Move what to the signal? If you put your code there it looks like the comment is talking about your auth.login call.

@ngokevin
ngokevin Feb 19, 2014

I'll move the comment down

@mstriemer
Mozilla member

How did the old logout work? Is it still being used? What happens if the user hits the old django logout (I'm assuming this exists), are they still logged in to fireplace?

@cvan cvan commented on an outdated diff Feb 26, 2014
mkt/account/views.py
@@ -7,6 +7,8 @@
import basket
import commonware.log
+from django_browserid import get_audience
+from django.contrib import auth
@cvan
cvan Feb 26, 2014

this belongs below line 5

@cvan
Mozilla member

r? @diox

@ngokevin
Mozilla member

@mstriemer Currently if a user logs out of Django, they are still logged into fireplace. If a user logs out of Fireplace, they are still logged into Django.

@mstriemer mstriemer commented on an outdated diff Apr 4, 2014
media/js/devreg/login.js
@@ -121,6 +125,22 @@ define('login', ['notification'], function(notification) {
});
}
+ function _prefix(storageKey) {
+ return localStorage.getItem('latestStorageVersion') || '0' + '::' + storageKey;
@mstriemer
mstriemer Apr 4, 2014

I think you need some parens here.

'1' || '0' + '::' + 'user' // => "1"
@mstriemer mstriemer commented on the diff Apr 4, 2014
media/js/devreg/login.js
@@ -121,6 +125,22 @@ define('login', ['notification'], function(notification) {
});
}
+ function _prefix(storageKey) {
+ return localStorage.getItem('latestStorageVersion') || '0' + '::' + storageKey;
+ }
+
+ function setToken(data) {
+ localStorage.setItem(_prefix('user'), data.token);
+ localStorage.setItem(_prefix('settings'), JSON.stringify(data.settings));
+ localStorage.setItem(_prefix('permissions'), JSON.stringify(data.permissions));
+ }
+
+ function clearToken() {
+ localStorage.removeItem(_prefix('user'));
+ localStorage.removeItem(_prefix('settings'));
+ localStorage.removeItem(_prefix('permissions'));
@mstriemer
mstriemer Apr 4, 2014

This should probably clear apps too. I don't like that we have to do this in two places :(

@mstriemer
Mozilla member

Just tested this out, r+wc

@ngokevin
Mozilla member

Thanks, this might cause some noise, but it's a good time to push (right after tag) for a whole week of testing/updating commonplace projects.

@ngokevin ngokevin merged commit f33effe into mozilla:master Apr 5, 2014
@robhudson

This test is failing with a 403. If I pdb into the RestSharedSecretMiddleware the _user isn't there on request.GET. @ngokevin can you take a look and fix this test or code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment