Skip to content

Commit

Permalink
More spiking
Browse files Browse the repository at this point in the history
  • Loading branch information
@stenington
stenington committed Feb 19, 2013
1 parent f36be3d commit a46cee7
Show file tree
Hide file tree
Showing 9 changed files with 114 additions and 86 deletions.
1 change: 1 addition & 0 deletions app.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ app.get('/', backpack.manage);
app.get('/backpack', backpack.manage)
app.get('/backpack/login', backpack.login);
app.get('/backpack/signout', backpack.signout);
app.post('/backpack/signout', backpack.signout);
app.post('/backpack/badge', backpack.userBadgeUpload);
app.post('/backpack/authenticate', backpack.authenticate);
app.get('/stats', backpack.stats);
Expand Down
8 changes: 7 additions & 1 deletion controllers/backpack.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ var Group = require('../models/group');
*/

exports.login = function login(request, response) {
if (request.user)
return response.redirect('/', 303);

// request.flash returns an array. Pass on the whole thing to the view and
// decide there if we want to display all of them or just the first one.
response.render('login.html', {
Expand Down Expand Up @@ -85,7 +88,10 @@ exports.authenticate = function authenticate(request, response) {

exports.signout = function signout(request, response) {
request.session = {};
response.redirect('/backpack/login', 303);
if (request.xhr)
response.json(200);
else
response.redirect('/backpack/login', 303);
};

/**
Expand Down
3 changes: 2 additions & 1 deletion middleware.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,10 @@ exports.csrf = function (options) {
var value = options.value || defaultValue;
var list = options.whitelist;
return function (req, res, next) {
var token = res.locals.csrfToken = req.session._csrf || (req.session._csrf = utils.uid(24));

This comment has been minimized.

Sign in to view

Copy link
@toolness

toolness Feb 19, 2013

If we're adding csrfToken as a global context variable for templates, we should probably remove it as an explicitly passed-in variable from the controllers that pass it in, right?

This comment has been minimized.

Sign in to view

Copy link
@stenington

stenington Feb 21, 2013

Author Owner

Yep.

if (whitelisted(list, req.url)) return next();

var token = req.session._csrf || (req.session._csrf = utils.uid(24));
if ('GET' == req.method || 'HEAD' == req.method) return next();
var val = value(req);
if (val != token) {
Expand Down
4 changes: 2 additions & 2 deletions static/js/backpack.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ if(!nunjucks.env) {
}(/*end setup*/)


!!function appInitialize (){
!!function appInitialize ($){

This comment has been minimized.

Sign in to view

Copy link
@toolness

toolness Feb 19, 2013

If we remove jQuery as a shim dependency we can probably undo this too.

var global = {
dragging: false
Expand Down Expand Up @@ -568,4 +568,4 @@ _.each(existingBadges, Badge.fromElement);
_.each(existingGroups, Group.fromElement);

//end app scope
}();
}(window.jQuery);
29 changes: 19 additions & 10 deletions static/js/browserid-ajax.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,37 +4,46 @@ define(["jquery", "backbone-events"], function($, BackboneEvents) {
return function BrowserIDAjax(options) {
var id = options.id || window.navigator.id;
var network = options.network || $;

var self = {
email: options.email || null,
email: options.email,
csrfToken: options.csrfToken,
id: id,
login: function() {
self.id.get(function(assertion) {
if (assertion)
post(options.verifyURL, {assertion: assertion}, 'login');
else
self.trigger("login:error");
});
self.id.request();
},
logout: function() {
post(options.logoutURL, {}, 'logout');
self.id.logout();
}
};

self.id.watch({
loggedInUser: self.email,
onlogin: function(assertion) {
if (assertion)
post(options.verifyURL, {assertion: assertion}, 'login');
else
self.trigger("login:error");
},
onlogout: function() {
post(options.logoutURL, {}, 'logout');
}
});

BackboneEvents.mixin(self);

function post(url, data, eventName) {
network.ajax({
type: 'POST',
url: url,
headers: {"X-CSRFToken": self.csrfToken},
headers: {"x-csrf-token": self.csrfToken},
dataType: 'json',
data: data,
error: function() {
console.log(arguments);
self.trigger(eventName + ":error");
},
success: function(data) {
self.csrfToken = data.csrfToken;
self.email = data.email;
self.trigger(eventName, self);
}
Expand Down
5 changes: 4 additions & 1 deletion static/js/require-config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ var require = {
baseUrl: "js",
shim: {
'jquery': {
exports: 'jQuery'
exports: 'jQuery',
init: function() {
this.jQuery.noConflict();

This comment has been minimized.

Sign in to view

Copy link
@toolness

toolness Feb 19, 2013

Hmm, we might want to simply remove jQuery as a shim for now--because the rest of the backpack code uses it, and the rest of the backpack code doesn't yet use requirejs, we don't want any weird race conditions preventing the non-requirejs code from accessing jQuery. Doing this basically means removing the shim, and removing jQuery as an explicit dependency from all requirejs modules that declare it (i.e., the modules will simply use the global reference to jQuery/$). Later on, if/when we've converted the rest of the backpack to use requirejs, we can bring jQuery back in as an explicit dependency (at that point we'll also need to make sure tabzilla isn't competing for jQuery loading, as per mozilla/friendlycode@dfbd4c2).

This comment has been minimized.

Sign in to view

Copy link
@stenington

stenington Feb 21, 2013

Author Owner

Okay, makes sense to me.
}
}
},
paths: {
Expand Down
59 changes: 39 additions & 20 deletions static/test/backpack/test-browserid-ajax.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,17 @@
defineTests(["browserid-ajax"], function(BrowserIDAjax) {
module("browserid-ajax");

function FakeNavigatorID() {
function FakeNavigatorID(assertion) {
return {
_options: null,
get: function(cb) {
this._options = {onlogin: cb};
watch: function(opts) {
this._options = opts;
},
request: function(opts) {
this._options.onlogin(assertion);
},
logout: function() {
this._options.onlogout();
}
};
}
Expand All @@ -20,11 +26,33 @@ defineTests(["browserid-ajax"], function(BrowserIDAjax) {
};
}

test("email sentinel values preserved", function() {
var id = FakeNavigatorID();

var browserid = BrowserIDAjax({
email: null, /* not logged in */
id: id
});
strictEqual(id._options.loggedInUser, null);

browserid = BrowserIDAjax({
email: undefined, /* login status unknown */
id: id
});
strictEqual(id._options.loggedInUser, undefined);

browserid = BrowserIDAjax({
email: 'foo@bar.org', /* foo@bar.org logged in */
id: id
});
equal(id._options.loggedInUser, 'foo@bar.org');
});

test("login error on failed verify works", function() {
var loginErrorEvents = 0;
var browserid = BrowserIDAjax({
email: '',
id: FakeNavigatorID(),
email: null,
id: FakeNavigatorID('fake assertion for foo@bar.org'),
verifyURL: '/verify',
logoutURL: '/logout',
csrfToken: 'fake csrf token',
Expand All @@ -37,26 +65,25 @@ defineTests(["browserid-ajax"], function(BrowserIDAjax) {

equal(loginErrorEvents, 0);
browserid.login();
browserid.id._options.onlogin('fake assertion for foo@bar.org');
equal(loginErrorEvents, 1);
});

test("verification works", function() {
var loginEvents = 0;
var loginErrorEvents = 0;
var browserid = BrowserIDAjax({
email: '',
id: FakeNavigatorID(),
email: null,
id: FakeNavigatorID('fake assertion for foo@bar.org'),
verifyURL: '/verify',
logoutURL: '/logout',
csrfToken: 'fake csrf token',
network: FakeNetwork({
'POST /verify': function(options) {
equal(options.data.assertion, 'fake assertion for foo@bar.org');
equal(options.headers['X-CSRFToken'], 'fake csrf token');
equal(options.headers['x-csrf-token'], 'fake csrf token');
equal(options.dataType, 'json');
options.success({
csrfToken: 'new fake csrf token',
status: 'ok',
email: 'foo@bar.org',
});
}
Expand All @@ -65,17 +92,11 @@ defineTests(["browserid-ajax"], function(BrowserIDAjax) {
.on('login:error', function() { loginErrorEvents++; });

equal(browserid.email, null);
equal(browserid.csrfToken, 'fake csrf token');

This comment has been minimized.

Sign in to view

Copy link
@toolness

toolness Feb 19, 2013

Hmm, I'm not sure why these assertions are being removed... Can you explain?

This comment has been minimized.

Sign in to view

Copy link
@stenington

stenington Feb 20, 2013

Author Owner

In webmaker-nav it seemed like the server would respond with a new csrfToken each time, which we don't have in the backpack so testing that browserid.csrfToken changes isn't necessary. (See browserid-ajax.js line 47.)

The "null assertions..." tests don't make sense because in the Observer API you never end up with null assertions as far as I can tell. I think it signaled a cancelled login in the Callback API, but now there's an oncancel you can pass to navigator.id.request() or something.

browserid.login();
browserid.id._options.onlogin(null);
equal(loginEvents, 0, "null assertions don't trigger login events");
equal(loginErrorEvents, 1, "null assertions do trigger login:error evts");
browserid.id._options.onlogin('fake assertion for foo@bar.org');

equal(loginEvents, 1);
equal(browserid.email, 'foo@bar.org');
equal(browserid.csrfToken, 'new fake csrf token');
});

test("logout works", function() {
Expand All @@ -88,23 +109,21 @@ defineTests(["browserid-ajax"], function(BrowserIDAjax) {
csrfToken: 'fake csrf token',
network: FakeNetwork({
'POST /logout': function(options) {
equal(options.headers['X-CSRFToken'], 'fake csrf token');
equal(options.headers['x-csrf-token'], 'fake csrf token');
equal(options.dataType, 'json');
options.success({
csrfToken: 'another new fake csrf token',
status: 'ok',
email: null,
});
}
})
}).on('logout', function() { logoutEvents++; });

equal(browserid.email, 'foo@barf.org');
equal(browserid.csrfToken, 'fake csrf token');

browserid.logout();

equal(logoutEvents, 1);
equal(browserid.email, null);
equal(browserid.csrfToken, 'another new fake csrf token');
});
});
41 changes: 38 additions & 3 deletions views/layout.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,14 @@ <h3><a class="brand" href="/">
Help: {% if tooltips %}Off{% else %}On{% endif %}
</a></li>
</ul>
{% if user %}
<ul class="nav pull-right">
{% if user %}
<li class="user">{{user.attributes.email}}</li>
<li><a href="/backpack/signout">Sign Out</a></li>
<li><a class="js-browserid-logout" href="#">Sign Out</a></li>
{% else %}
<li><a class="js-browserid-link" href="#">Sign In</a></li>
{% endif %}
</ul>
{% endif %}
</div>
</div>
</div>
Expand Down Expand Up @@ -87,6 +89,39 @@ <h2>Legal</h2>
</div>

<script src="//www.mozilla.org/tabzilla/media/js/tabzilla.js"></script>
<script src="/js/require-config.js"></script>
<script src="/js/require.min.js"></script>
<script type="text/javascript">
require(['browserid-ajax'], function(BrowserIDAjax){
var browserid = BrowserIDAjax({
{% if user %}
email: "{{ user.attributes.email }}",
{% else %}
email: null,
{% endif %}
verifyURL: '/backpack/authenticate',
logoutURL: '/backpack/signout',
csrfToken: '{{ csrfToken }}'
}).on('login:error', function() {
console.log('error');

This comment has been minimized.

Sign in to view

Copy link
@toolness

toolness Feb 19, 2013

I wonder if it'd be useful to ping the server when this happens, so we can check server logs and at least detect how often this is being called in the wild.

This comment has been minimized.

Sign in to view

Copy link
@stenington

stenington Feb 21, 2013

Author Owner

Yeah, that's probably useful. Also, displaying an error to the user of course.
}).on('login', function() {
console.log('woot');
window.location.reload();
}).on('logout', function() {
console.log('logout');
window.location.reload();
}).on('logout:error', function() {
console.log('wat');
});

$('.js-browserid-link').bind('click', function(){
browserid.login();
});
$('.js-browserid-logout').bind('click', function(){
browserid.logout();
});
});
</script>
{% block scripts %}{% endblock %}

</body>
Expand Down
50 changes: 2 additions & 48 deletions views/login.html
View file
Original file line number Diff line number Diff line change
@@ -1,55 +1,9 @@
{% extends 'layout.html' %}
{% block body %}
<h1>Welcome</h1>
<h2>Use the green button below to <a href="#" class="js-browserid-link">sign in.</a><br/> Don&rsquo;t worry if you don&rsquo;t have an account, that&rsquo;ll get taken care of.</h2>

<form class="signin js-browserid-form" method="POST" action="/backpack/authenticate">
<input class="js-browserid-input" name="assertion" type="hidden"></input>
<input name="_csrf" type="hidden" value="{{ csrfToken }}"></input>
</form>
<h2>Use the blue button below to <a href="#" class="js-browserid-link">sign in.</a><br/> Don&rsquo;t worry if you don&rsquo;t have an account, that&rsquo;ll get taken care of.</h2>

<div style="padding-top: 10px">
<a class="js-browserid-link" href="#"><img src="https://browserid.org/i/sign_in_green.png"/></a>
<a class="js-browserid-link" href="#"><img src="https://developer.mozilla.org/files/3957/email_sign_in_blue.png"/></a>
</div>

<script src="/js/require-config.js"></script>
<script src="/js/require.min.js"></script>
<script type="text/javascript">
require(['browserid-ajax', 'jquery'], function(BrowserIDAjax, $){
var browserid = BrowserIDAjax({
email: '',
verifyURL: '/backpack/authenticate',
logoutURL: '/backpack/signout',
csrfToken: '{{ csrfToken }}'
}).on('login:error', function() {
console.log('error');
}).on('login', function() {
console.log('woot');
window.location = '/';
});

$('.js-browserid-link').bind('click', function(){ browserid.login(); });
});
/*!!function loginHandler () {
//begin login handler
function launchBrowserId(callback) {
return function() { navigator.id.get(callback, {
siteName: 'Open Badge Backpack',
termsOfService: '/tou.html',
privacyPolicy: '/privacy.html',
returnTo: '/'
}); }
}
function handleResponse(assertion) {
if (!assertion) return false;
$('.js-browserid-input').val(assertion);
$('.js-browserid-form').trigger('submit');
}
$('.js-browserid-link').bind('click', launchBrowserId(handleResponse));
//begin login handler scope
}();
*/
</script>
{% endblock %}

1 comment on commit a46cee7

@toolness
Copy link

Choose a reason for hiding this comment

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

Also, you should make sure you add a - phantomjs static/test/phantom-qunit.js http://localhost:8888/test/backpack/ entry to your .travis.yml so the new tests are CI'd.

Please sign in to comment.