0.3.0 hook up login + logout with zamboni (bug 970598) #24

Merged
merged 2 commits into from Apr 5, 2014

Projects

None yet

4 participants

@ngokevin
Member

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.

  • On initialization of the user module, look for the shared token if it was set from Zamboni (or from other commonplace sites).
  • On logout, send a ping to Zamboni to django.contrib.auth.logout everywhere.
@cvan
Member
cvan commented Feb 13, 2014

thanks for grabbing this one man

@ngokevin
Member
@mstriemer mstriemer and 1 other commented on an outdated diff Feb 26, 2014
src/media/js/login.js
- if (capabilities.persona) {
- console.log('Triggering Persona logout');
- navigator.id.logout();
- }
+ requests.post(urls.api.url('logout')).done(function() {
@mstriemer
mstriemer Feb 26, 2014 Member

What if the server doesn't care? Should this require being enabled? Should this be a DELETE request to a session endpoint instead of a POST to logout?

@cvan
cvan Feb 26, 2014 Member

I agree with @mstriemer. and why block on xhr to finish?

@mstriemer
mstriemer Feb 26, 2014 Member

Maybe we can have a logoutWith option which defaults to a resolved deferred.

login.logoutWith = function () {
    return request.post(urls.api.url('logout'));
};

// ...

}).on('click', '.logout', function(e) {
    login.logoutWith = login.logoutWith || deferred.Defer().resolve();
    login.logoutWith.then(function() {}, function() {});
});
@mstriemer
mstriemer Feb 26, 2014 Member

I think it makes sense to block until the server 200s as there could be some cleanup the server needs to perform. If logout is supposed to log you out everywhere then you'd expect that to be the case or be an error rather than staying logged in to the devhub on a public computer because the error was ignored.

@mstriemer mstriemer commented on an outdated diff Feb 26, 2014
src/media/js/login.js
- require('views').reload().done(function(){
- z.page.trigger('logged_out');
- signOutNotification();
- });
- } else {
- console.log('Reload on logout aborted by current view');
- }
+ if (!z.context.dont_reload_on_login) {
+ require('views').reload().done(function(){
+ z.page.trigger('logged_out');
+ signOutNotification();
+ });
+ } else {
+ console.log('Reload on logout aborted by current view');
+ }
+ });
});
@mstriemer
mstriemer Feb 26, 2014 Member

What happens if the server failed to log out the user? I'm thinking we display an error.

@cvan cvan commented on the diff Feb 26, 2014
src/media/js/routes_api.js.dist
@@ -7,5 +7,7 @@ define('routes_api', [], function() {
// "another_route": "/foo/bar/{0}/asdf"
// }
return {
+ 'login': '/api/v1/account/login/',
@cvan
cvan Feb 26, 2014 Member

why is this required?

@ngokevin
ngokevin Feb 26, 2014 Member

Because all Commonplace projects talk to Zamboni to login

@cvan cvan and 1 other commented on an outdated diff Feb 26, 2014
src/media/js/user.js
@@ -4,9 +4,9 @@ define('user',
var console = log('user');
- var token;
- var settings = {};
- var permissions = {};
+ var token = storage.getItem('user');
@cvan
cvan Feb 26, 2014 Member

what if we are not saving to localStorage?

@ngokevin
ngokevin Apr 2, 2014 Member

It's fine there's nothing in storage.getItem('user')

@mstriemer
Member

Is there some way we can pull out the login.js stuff and wire up some authentication hooks in commonplace? The zamboni changes concern me since it seems like we're trying to get two different authentication mechanisms to work and then have each fake that the other logged in or logged out.

If we had a shared login library we could use that in both places and ensure that we correctly login/logout.

@ngokevin
Member
ngokevin commented Mar 5, 2014

@mstriemer I had a go at making a shared login/logout library. I wasn't able to abstract too many things, only the setting/clearing of user token in localStorage (which is only a few lines of code). And even then, it requires a lot of shimming between Zamboni and Commonplace due to both having different dependencies available. At least now, the code works as is.

@diox diox and 1 other commented on an outdated diff Mar 5, 2014
src/media/js/user.js
@@ -4,9 +4,9 @@ define('user',
var console = log('user');
- var token;
- var settings = {};
- var permissions = {};
+ var token = storage.getItem('user');
+ var settings = JSON.parse(storage.getItem('settings')) || {};
+ var permissions = JSON.parse(storage.getItem('permissions')) || {};
@diox
diox Mar 5, 2014 Member

See also mozilla/fireplace@c064e6d and mozilla/fireplace@0f0f04d that add stuff to fireplace's user.js

@mstriemer
mstriemer Apr 4, 2014 Member

Pulling this data from storage happens again below.

@mstriemer mstriemer and 1 other commented on an outdated diff Mar 6, 2014
src/media/js/storage.js
@@ -17,6 +17,11 @@ define('storage', ['settings'], function(settings) {
};
}
+ // Expose storage version, to be used in Zamboni login.js.
+ prefix(ls.setItem, function() {
+ this['latestStorageVersion'] = settings.storage_version;
@mstriemer
mstriemer Mar 6, 2014 Member

This looks like a noop to me. Can you add a comment about how it works?

@ngokevin
ngokevin Mar 10, 2014 Member

Whoops, fixed

@ngokevin
Member
ngokevin commented Apr 2, 2014

r? this is 2 months going

@ngokevin ngokevin changed the title from 0.2.8 hook up login + logout with zamboni (bug 970598) to 0.3.0 hook up login + logout with zamboni (bug 970598) Apr 4, 2014
@ngokevin
Member
ngokevin commented Apr 4, 2014
@mstriemer
Member

Didn't get to test it locally but r+wc on the code.

@mstriemer mstriemer commented on an outdated diff Apr 4, 2014
src/media/js/storage.js
@@ -17,6 +17,14 @@ define('storage', ['settings'], function(settings) {
};
}
+ // Expose storage version (which is prefixed to every key).
+ // For instance, used in Zamboni login.js.
+ try {
+ ls.setItem('latestStorageVersion', settings.storage_version);
+ } catch(e) {
+ fakeStorage['latestStorageVersion'] = settings.storage_version;
@mstriemer
mstriemer Apr 4, 2014 Member

commonplace lint is upset with this, fakeStorage.latestStorageVersion = settings.storage_version; instead.

@ngokevin
Member
ngokevin commented Apr 5, 2014

thanks, i hope one day we can have one shared library, but commonplace js and zamboni js do not mix together trivially. this will be a mess to get out to commonplace projects, but here goes.

@ngokevin ngokevin merged commit af95808 into mozilla:master Apr 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment