867218 - Configure cookieSession() on login.wm.o to use the "webmaker.org" shared session

Status: RESOLVED INVALID

(Webmaker Graveyard :: Login, defect)

Description

[Kieran Sedgwick (:sedge)]

Login.wm.o must attach authentication data to a shared sessionCookie for "webmaker.org".  This requires the app to modify its own sessionCookie to attach data to the shared one.

Comment 1

[Kieran Sedgwick (:sedge)]

Attachment: Pull request for this patch

https://bugzilla.mozilla.org/show_bug.cgi?id=867218

Comment 2

[Kieran Sedgwick (:sedge)]

Comment on attachment 744293 [details]
Pull request for this patch

>https://github.com/mozilla/login.webmaker.org/pull/40

Comment 3

[David Humphrey (:humph)]

Process point: someone to review the code changes, and maybe, if you think it necessary, another person to review the architecture change(s) if any.  In Mozilla that's usually known as a Review and Super Review.  There are times where a change crosses a bunch of code/expertise boundaries, and getting specific people to review various bits of what you've done makes sense.  I'd suggest that you can usually get away with 1 or 2 reviews, 3 in rare situations.  You'll never need 5 reviewers for a patch, ime.

That said, I'm loving watching you dive into hard problems, put up patch after patch, and not get frustrated or lose your edge.  Great work.

Comment 4

[David Humphrey (:humph)]

Comment on attachment 744293 [details]
Pull request for this patch

Clearing some reviewers to keep this moving.

Comment 5

[Pomax]

r-, your branch isn't based on master so will need a rebase first.

Comment 6

[Ross Bruniges [:boozeniges]]

Confused - I see that the pull request is closed, can't see if it got openned up somewhere else?

Comment 7

[Ross Bruniges [:boozeniges]]

Thanks for adding in a well described integration guide - it made no sense when reading the PR but reading it as an actual file it's fantastic.

Other than a couple of points I made (which may well be invalid or unnecessary) I'll happily r+ this once it's all properly rebased.

Comment 8

[Kieran Sedgwick (:sedge)]

Attachment: Pull request for this patch

https://github.com/mozilla/login.webmaker.org/pull/42

Comment 9

[Ross Bruniges [:boozeniges]]

Comment on attachment 744622 [details] [review]
Pull request for this patch

I think there are a couple of comments on the ticket that at the moment warrant a fail pending further discussion.

Would like to re-iterate the work on the readme - that's a very good integration guide.

Comment 10

[Pomax]

Comment on attachment 744622 [details] [review]
Pull request for this patch

this pull request got closed.

Comment 11

[Kieran Sedgwick (:sedge)]

Attachment: Pull request for this patch

https://github.com/mozilla/login.webmaker.org/pull/46

Comment 12

[Kieran Sedgwick (:sedge)]

Attachment: PR for thimble implementation

https://github.com/mozilla/thimble.webmaker.org/pull/48

Comment 13

[Kieran Sedgwick (:sedge)]

At this point we're attempting to fully understand why the session-sharing between apps isn't stable.  Apps can assume we WILL get it there, and the other teams can assume "req.session.auth"'s existence to confirm a logged in user.  

Req.session.auth will contain an attribute called "_id", containing the user's email for further API calls to the login server if needed.

Comment 14

[Pomax]

Comment on attachment 745918 [details] [review]
PR for thimble implementation

closing, and will add a new attachement with a our operational changes surgically injected into master, for added win.

Comment 15

[Pomax]

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

Comment 16

[Pomax]

One thing that's still missing is an updated README. I shall make this update happen, too.

Comment 17

[Pomax]

readme updated.

Comment 18

[Ross Bruniges [:boozeniges]]

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

r+ from an understanding and smell test review.

Hoping that additional reviews can clear up any "this doesn't actually work" reviews.

Comment 19

[Pomax]

note: unhappy with the use of both parser_secret and session_secret, filed https://github.com/visionmedia/express/issues/1607 to find out what the correct way of doing things is here, since the docs are not sufficient to determine that.

Comment 20

[Scott [:thecount] Downe]

I'm having a hard time figuring out what this patch is doing, and how to test it.

I'll take a deeper look, but any more info would be appreciated.

Comment 21

[David Humphrey (:humph)]

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

This doesn't need so many reviewers.

Comment 22

[Pomax]

perhaps not, but thecount has so far been the only one to signal actual "I tried to use it and I ran into these problems" issues, so his review on IRC has been pretty invaluable.

Comment 23

[Pomax]

also note that in order to test this, you will also need a consumer app, such as the SSO-enabled branch of thimble: https://github.com/pomax/thimble.webmaker.org/tree/bug866815

Comment 24

[Pomax]

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

switched review to humph so it doesn't block

Comment 25

[Pomax]

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

updated the pull request based on the comments.

Comment 26

[Jon Buckley]

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

r+ with nits, use SESSION_SECRET for both cookieParser and cookieSession, rather than separate secrets.

Comment 27

[Pomax]

replaced parser_secret with session_secret and updated the pull request.

Comment 28

[[github robot]]

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

https://github.com/mozilla/login.webmaker.org/commit/a50d164deb7107969d53d98cac6d0cbf31975e73
Merge pull request #47 from Pomax/bug867218

cross-wm-tool cookie management

Comment 29

[Pomax]

merged. follow-up bugs filed, closing this one.

Comment 30

[Pomax]

we're not quite there yet. Deploying to heroku still had some issues, so we need to ensure that works before we can permanently close.

Comment 31

[Jon Buckley]

Going to back out Pomax's changes

Comment 32

[[github robot]]

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

https://github.com/mozilla/login.webmaker.org/commit/c31162a9cfd0fcde65c615de6e0a889fb1dab520
Revert bug 867218 - Use shared sessions for SSO

This reverts commit a50d164deb7107969d53d98cac6d0cbf31975e73, reversing
changes made to 6f395225ad42ab007a15357be5a180c987e657e2.

Comment 33

[Jon Buckley]

Based on bug 867223 we're not doing it this way.