Closed Bug 884681 Opened 8 years ago Closed 8 years ago

Username persists in session after logout

Categories

(Webmaker Graveyard :: webmaker.org, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kate, Assigned: mjschranz)

References

Details

Attachments

(2 files, 3 obsolete files)

After the user initiates a logout, req.session.email is null but req.session.username is still set to the user's username
Assignee: nobody → schranz.m
Attached file TWO PATCHES IN ONE (obsolete) —
https://github.com/mozilla/login.webmaker.org/pull/112
https://github.com/mozilla/node-webmaker-loginapi/pull/17

Double Patch!
Attachment #764719 - Flags: review?(pomax)
Attachment #764719 - Flags: review?(jon)
Attachment #764719 - Flags: review?(david.humphrey)
Comment on attachment 764719 [details]
TWO PATCHES IN ONE

Jon brought up doing this with express-persona.

Going to cancel these reviews.
Attachment #764719 - Flags: review?(pomax)
Attachment #764719 - Flags: review?(jon)
Attachment #764719 - Flags: review?(david.humphrey)
Duplicate of this bug: 884530
Attachment #764719 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Login Patch
Attachment #764779 - Attachment is obsolete: true
Attachment #764805 - Flags: review?(pomax)
Attachment #764805 - Flags: review?(jon)
Attachment #764805 - Flags: review?(david.humphrey)
Node Module Patch.
Attachment #764806 - Flags: review?(pomax)
Attachment #764806 - Flags: review?(jon)
Attachment #764806 - Flags: review?(david.humphrey)
login patch requires a rebase
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.
Attachment #764805 - Flags: review?(pomax) → review+
Comment on attachment 764806 [details] [review]
https://github.com/mozilla/node-webmaker-loginapi/pull/18

looks sane, with comments in the pull request
Attachment #764806 - Flags: review?(pomax) → review+
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.
Attachment #764805 - Flags: review+ → review?(pomax)
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.
Attachment #764806 - Flags: review?(jon)
Attachment #764806 - Flags: review?(david.humphrey)
Attachment #764806 - Flags: review-
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.
Attachment #764805 - Flags: review?(pomax)
Attachment #764805 - Flags: review?(jon)
Attachment #764805 - Flags: review?(david.humphrey)
Attachment #764805 - Flags: review-
Comment on attachment 764805 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/114

Getting a really weird travis fail here...
Attachment #764805 - Flags: review?(jon) → review-
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.
Attachment #764805 - Flags: review- → review?(jon)
Duplicate of this bug: 887467
Attachment #764806 - Flags: review+ → review?(pomax)
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.
Attachment #764805 - Flags: review?(jon) → review?(pomax)
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
Attachment #770916 - Flags: review?(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.
Blocks: 890294
Blocks: 890295
Blocks: 890297
Blocks: 890298
Attachment #770916 - Attachment is obsolete: true
Attachment #770916 - Flags: review?(pomax)
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
Both of the above patches have been landed. The follow up bugs are listed as bugs this blocks.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 890310
Attachment #764806 - Flags: review?(pomax)
Attachment #764806 - Flags: review?(jon)
Attachment #764805 - Flags: review?(pomax)
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.