Permalink
Browse files

Bug #2307: don't expire existing sessions when adding a secondary add…

…ress

If a persona.org account is initially created with a "primary"
address (meaning an address served by a participating IdP, so
persona.org is given an assertion from that IdP as proof of ownership),
the new account will not have a password associated with it. If you then
add a "secondary" address (meaning an address *not* served by a
participating IdP, requiring an email challenge to prove ownership), you
will have to set up a password when you add the secondary. The
establishment of this password should *not* invalidate any sessions that
were set up earlier.

In Bug #2307, this manifested as the first browser (in which the
add-secondary-email operation was started, so it had the old session and
was waiting for the operation to complete, polling
/wsapi/email_addition_status all the while) receiving a "400
Unauthorized" error when the email challenge link was opened in a second
browser (which thus got a new session).

The test for this effect lives in tests/primary-then-secondary-test.js,
which need the same 2-second delay as password-update-test.js (to make
sure that the modified lastPasswordReset time was actually different
than the previous value, so the session really would be expired).
  • Loading branch information...
1 parent cc6c775 commit 5f5d8e5389a3a0b4d1cc2bedd3901c71fe496ee0 @warner warner committed Aug 15, 2012
Showing with 41 additions and 2 deletions.
  1. +1 −1 lib/db/json.js
  2. +1 −1 lib/db/mysql.js
  3. +39 −0 tests/primary-then-secondary-test.js
View
2 lib/db/json.js
@@ -310,7 +310,7 @@ exports.completeConfirmEmail = function(secret, cb) {
exports.emailToUID(o.email, function(err, uid) {
if(err) return cb(err, o.email, o.existing_user);
- exports.updatePassword(uid, hash, true, function(err) {
+ exports.updatePassword(uid, hash, false, function(err) {
cb(err || null, o.email, o.existing_user);
});
});
View
2 lib/db/mysql.js
@@ -397,7 +397,7 @@ exports.completeConfirmEmail = function(secret, cb) {
// we're adding or reverifying an email address to an existing user account. add appropriate
// entries into email table.
if (o.passwd) {
- exports.updatePassword(o.existing_user, o.passwd, true, function(err) {
+ exports.updatePassword(o.existing_user, o.passwd, false, function(err) {
if (err) return cb('could not set user\'s password');
addEmailToUser(o.existing_user, o.email, 'secondary', cb);
});
View
39 tests/primary-then-secondary-test.js
@@ -49,6 +49,7 @@ suite.addBatch({
}
});
+var the_assertion;
// now let's generate an assertion using this user
suite.addBatch({
"generating an assertion": {
@@ -60,6 +61,7 @@ suite.addBatch({
},
"and logging in with the assertion succeeds": {
topic: function(err, assertion) {
+ the_assertion = assertion;
wsapi.post('/wsapi/auth_with_assertion', {
assertion: assertion,
ephemeral: true
@@ -85,6 +87,32 @@ suite.addBatch({
}
});
+// this second session, logged in with just the primary, should *not* be
+// invalidated by the addition of a secondary address (and consequent
+// establishment of a password)
+var context2 = {};
+suite.addBatch({
+ "establishing a second session": {
+ topic: function() {
+ wsapi.post('/wsapi/auth_with_assertion', {
+ assertion: the_assertion,
+ ephemeral: true
+ }, context2).call(this);
+ },
+ "works as expected": function(err, r) {
+ assert.strictEqual(JSON.parse(r.body).success, true);
+ },
+ "after waiting for lastPasswordReset's now() to increment": {
+ topic: function() {
+ // see password-update-test.js for an explanation of this delay
+ setTimeout(this.callback, 2000);
+ },
+ "we've waited long enough": function() {}
+ }
+ }
+});
+
+
var token;
// now we have a new account. let's add a secondary to it
suite.addBatch({
@@ -238,6 +266,17 @@ suite.addBatch({
}
});
+// and the second session should still be valid
+suite.addBatch({
+ "second session is still valid": {
+ topic: wsapi.post('/wsapi/prolong_session', {}, context2),
+ "works as expected": function(err, r) {
+ assert.strictEqual(r.code, 200);
+ assert.strictEqual(r.body, "OK");
+ }
+ }
+});
+
// shut the server down and cleanup
start_stop.addShutdownBatches(suite);

0 comments on commit 5f5d8e5

Please sign in to comment.