884681 - Username persists in session after logout

Status: RESOLVED FIXED

(Webmaker Graveyard :: webmaker.org, defect)

Description

[inactivate account for kate]

After the user initiates a logout, req.session.email is null but req.session.username is still set to the user's username

Comment 1

[Matthew Schranz [:mjschranz]]

Attachment: TWO PATCHES IN ONE

https://github.com/mozilla/login.webmaker.org/pull/112
https://github.com/mozilla/node-webmaker-loginapi/pull/17

Double Patch!

Comment 2

[Matthew Schranz [:mjschranz]]

Comment on attachment 764719 [details]
TWO PATCHES IN ONE

Jon brought up doing this with express-persona.

Going to cancel these reviews.

Comment 4

[Jon Buckley [:jbuck]]

Attachment: https://github.com/mozilla/node-webmaker-loginapi/pull/18

Comment 5

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/login.webmaker.org/pull/114

Login Patch

Comment 6

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/node-webmaker-loginapi/pull/18

Node Module Patch.

Comment 7

[Matthew Schranz [:mjschranz]]

Use https://github.com/mjschranz/popcorn.webmaker.org/tree/testPatch884681 as a test branch in Popcorn Maker.

Comment 8

[Pomax]

login patch requires a rebase

Comment 9

[Pomax]

Comment on attachment 764805 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/114

this seems pretty sane to me. We're starting to nest callbacks pretty deep now, though.

Comment 10

[Pomax]

Comment on attachment 764806 [details] [review]
https://github.com/mozilla/node-webmaker-loginapi/pull/18

looks sane, with comments in the pull request

Comment 11

[Matthew Schranz [:mjschranz]]

Comment on attachment 764805 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/114

Rebased up the login patch. Did review fixes for the node module. Would still like more eyes on this.

Comment 12

[Jon Buckley [:jbuck]]

Comment on attachment 764806 [details] [review]
https://github.com/mozilla/node-webmaker-loginapi/pull/18

This is an improvement, but we can go even further. Inside node-webmaker-loginapi:

module.exports = function(app, options) {
  var persona = require("express-persona");

  options.verifyResponse = function(err, req, res, email) {
    // your current personaLogin code
  };

  options.logoutResponse = function(err, req, res) {
    // your current personaLogout code
  };
  
  persona(app, options);
  
  return;
};

This way we could remove the need to require("express-persona") within our apps, and just use this module.

Comment 13

[Jon Buckley [:jbuck]]

Comment on attachment 764805 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/114

r- because I suspect you'll be able to remove all the GET /user/:email code when you change the node module patch.

Comment 14

[Matthew Schranz [:mjschranz]]

Comment on attachment 764805 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/114

Review fixes.

Comment 15

[Matthew Schranz [:mjschranz]]

Comment on attachment 764806 [details] [review]
https://github.com/mozilla/node-webmaker-loginapi/pull/18

Review Fixes.

Comment 16

[Jon Buckley [:jbuck]]

Comment on attachment 764806 [details] [review]
https://github.com/mozilla/node-webmaker-loginapi/pull/18

comments in PR

Comment 17

[Jon Buckley [:jbuck]]

Comment on attachment 764805 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/114

Getting a really weird travis fail here...

Comment 18

[Matthew Schranz [:mjschranz]]

Comment on attachment 764805 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/114

I don't see any fails on Travis, at least not after a quick update for work done on the module.

Comment 19

[Matthew Schranz [:mjschranz]]

Comment on attachment 764806 [details] [review]
https://github.com/mozilla/node-webmaker-loginapi/pull/18

Fixed review points.

Comment 21

[Matthew Schranz [:mjschranz]]

Comment on attachment 764805 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/114

Had to make updates, so this will need to be updated.

Comment 22

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/webmaker.org/pull/236/files

Here's it working with webmaker.org.

Obviously when the actual node module patch lands I'll have an appropriate version there in the package.json

Comment 23

[Pomax]

This works quite well! I'd like to r+, but there's a few attachments that probably don't need an r+ since they were testing-only bits. If you clear those, I'll r+ the remainder, and then we can see how many components need to update at the same time for pushing this to staging.

Comment 24

[[github robot]]

Commit pushed to master at https://github.com/mozilla/login.webmaker.org

https://github.com/mozilla/login.webmaker.org/commit/8d7ce8d58e55458a6a831b591bb8d4f893d71357
Bug 884681 - Adapt SSO-UX to account for express-persona adding username to session

Comment 25

[Matthew Schranz [:mjschranz]]

Both of the above patches have been landed. The follow up bugs are listed as bugs this blocks.