882655 - ! - Figure out an SSO/persona solution that doesn't get pop up blocked

Status: RESOLVED FIXED

(Webmaker Graveyard :: webmaker.org, defect)

Description

[Brett Gaylor [:brett]]

Whats the importance field above "blocker"?

Comment 1

[Ali Al Dallal [:alicoding]]

This might not be the best solution, but I was doing a quick research about what could be our solution here, and I found that "iframe sandbox" would at least help a bit to get this bypass.

It will at least tell the browser that it is okay to get this iframe popup even they have the option for blocked popup windows on.

It works for most modern browsers except few old one and including IE9 that it doesn't support.

IMO: I think having this feature at least would be better than having nothing at all.

Unless there will be a better solution for that :)

Comment 2

[David Humphrey (:humph)]

From discussions with jbuck on irc, the issue here is that we're not calling navigator.id.request() directly from a user action in the UI (i.e., we do it indirectly through postMessage into an iframe due to our navigator.idSSO wrapper), which means it isn't happening as a result of a trusted event, and therefore needs the user to allow it manually (popup blocking kicks in).

The way to fix this is for us to trigger navigator.id.request() directly in the UI callback initiated by the user's "Sign in" click.  Our current strategy looks like this:

User clicks -> navigator.idSSO.request() -> postMessage to iframe -> addEventListener("message") -> navigator.id.request()

Basically, we can't do that navigator.id.request() call on the far side of the postMessage--it has to happen before.  Jon suggests that the least bad solution would be to replace the sign-in/sign-up buttons with an iframe, that has the same appearance, and triggers request() directly/immediately.  What isn't yet clear is how to do this without dumping all our current work in idSSO.

Comment 3

[Pomax]

In principle we can have persona pop up its login window, get the email+assertion from that, and then postmessage that data to our iframe for login.webmaker.org validation, correct?

Comment 4

[David Humphrey (:humph)]

From discussions on vidyo with jbuck/Pomax, our plan is going to be trying to move the login/logout buttons into the same window (iframe) as we do the call to navigator.id.request(), such that we can wire direct event handlers for trusted events.

Pomax is going to give this a shot and see if he runs into any issues.

Comment 5

[Pomax]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/119

updates in thimble

Comment 6

[Pomax]

Attachment: https://github.com/mozilla/webmaker.org/pull/131

updates in webmaker

Comment 7

[Pomax]

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

updates in login

Comment 8

[Pomax]

NOTE: THIS CODE HAS NOT BEEN CLEANED UP YET

Comment 9

[Pomax]

NOTE 2: ONCE THIS LANDS, ALL CONSUMER APPS NEED THEIR SNIPPET UPDATED

Comment 10

[Kieran Sedgwick (:sedge)]

Working nicely, but needs some cleanup.

Comment 11

[Ali Al Dallal [:alicoding]]

I have tried on

Opera Version 12.15 on Mac
Chrome Version 27.0.1453.110 on Mac
Firefox Version 21.0 on Mac

Logged in from Thimble with popup blocked "ON" and it stills open up and I can login fine and crossed server 

The only thing needed now is to clean up :)

10+ for you :pomax

Comment 12

[Pomax]

Comment on attachment 762142 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/119

part one of three

Comment 13

[Pomax]

Comment on attachment 762143 [details] [review]
https://github.com/mozilla/webmaker.org/pull/131

part two of three

Comment 14

[Pomax]

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

part three of three

Comment 15

[David Humphrey (:humph)]

Comment on attachment 762142 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/119

One thing I want to make sure we do is styling of that iframe so we remove the border, etc. (i.e., so consumers can just use it without having to also style it).  Should we do that in css or js?  Should we add an id or class name to the iframe, or just pick it out of the DOM based on position in the nav?

r+ with that figured out.

Comment 16

[Pomax]

we're doing the styling - peek a gandour at the .css we're using in the webmaker.org pull request.

Comment 17

[David Humphrey (:humph)]

Comment on attachment 762143 [details] [review]
https://github.com/mozilla/webmaker.org/pull/131

A few comments in the PR, r+ with those.

Comment 18

[David Humphrey (:humph)]

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

Seems sane to me, but I'd like Kate to look at it (and other CSS changes).

Comment 19

[David Humphrey (:humph)]

General comments on this work:

* Amazing, thank you for getting it done so quickly and with such little fuss
* I want to see the patch for Popcorn Maker's use of this in here too
* I want to have a decision made about where and when the CSS fixes this needs are happening.  I'd suggest we do it in this bug, but I'm open to whatever gets it done today.
* I've asked Kate to look at CSS changes generally
* Let's figure out how to style the iframe for consumers so they don't have to set border to none, deal with sizing on iframe, etc.

Comment 20

[Pomax]

Comment on attachment 762143 [details] [review]
https://github.com/mozilla/webmaker.org/pull/131

refactored CSS needs a design pass, too.

Comment 21

[Pomax]

as for the iframe border, I'd suggest we simply take the iframe in the webmaker user bar. There's only one in there, but even if there were more, they all need their borders set to none.

Comment 22

[Pomax]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/55

I tried to add this to popcorn, too, but popcorn's... a bit of a mess =x

I got it to load, but the cross-site SSO doesn't seem to work (logging into popcorn doesn't log me into webmaker, I have to reload the browser and then it shows me as logged in). Who knows what's missing to make popcorn work too?

Comment 23

[David Humphrey (:humph)]

Comment on attachment 762324 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/55

I'd like to get Matt to weigh in, looks OK to me.

Comment 24

[Pomax]

Comment on attachment 762324 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/55

I've updated the popcorn patch to load include.js after the userbar has fully built. solves the problem, for me.

Comment 25

[Matthew Schranz [:mjschranz]]

Attachment: Additional work for Popcorn Patch

Additional work needs to be done for the patch to land in Popcorn Maker. Also, we need to figure out what's wrong with the /me and /account pages when clicked on in the bar.

Comment 26

[Pomax]

Point of login cookies taking time: On the very first time a user loads this, in the extremely unlikely event that they have webmaker and popcorn/thimble open in two tabs, the first login that sets a persona cookie will only set the cookie for the one site. Since the other tabs have no persona-controlled sessions yet, there will be no "magic switch" that logs them in simultaneously. However, any time after that, simultaneous login will happen just fine. To be honest, this sounds like a wontfix issue since users are unlikely to load up all the tools in different tabs and then log in (effectively testing our login process) the very first time they use webmaker.

Comment 27

[Matthew Schranz [:mjschranz]]

Comment on attachment 762324 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/55

R+ with my patch landed. We also fixed the issues being seen with /me and /account

Comment 28

[Pomax]

Comment on attachment 762142 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/119

just-to-be-sure r?

Comment 29

[Pomax]

Comment on attachment 762143 [details] [review]
https://github.com/mozilla/webmaker.org/pull/131

just-to-be-sure check

Comment 30

[Pomax]

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

just to be sure... v3

Comment 31

[Matthew Schranz [:mjschranz]]

Comment on attachment 762690 [details] [diff] [review]
Additional work for Popcorn Patch

This was merged into the Popcorn pull request.

Comment 32

[Matthew Schranz [:mjschranz]]

Comment on attachment 762142 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/119

R+, looks good locally for me.

Comment 33

[Matthew Schranz [:mjschranz]]

Comment on attachment 762143 [details] [review]
https://github.com/mozilla/webmaker.org/pull/131

R+ from me. Looked good locally.

Comment 34

[Matthew Schranz [:mjschranz]]

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

R+ again. Worked well locally for me.

Comment 35

[inactivate account for kate]

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

Could we do this instead? That way, we don't have to coordinate fonts etc. between the iframe and the user bar

https://github.com/Pomax/login.webmaker.org/pull/1

Also see https://github.com/Pomax/webmaker.org/pull/1

Comment 36

[Pomax]

both pull requests need a rebase before they're reviewable. login is probably good, webmaker will need an update, because it looks like it's based on a fairly "old" branch commit

Comment 37

[[github robot]]

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

https://github.com/mozilla/login.webmaker.org/commit/87f50aeafc92ad2bf4dc2cf5f93fee5b249ceb12
Merge pull request #102 from Pomax/bug882655

[#882655] SSO through direct iframe interaction

Comment 38

[[github robot]]

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

https://github.com/mozilla/thimble.webmaker.org/commit/d4863689b0db7db1b7b55b63c65533e351cd384c
Merge pull request #119 from Pomax/bug882655

[#882655] SSO through direct iframe interaction

Comment 39

[[github robot]]

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

https://github.com/mozilla/popcorn.webmaker.org/commit/be0db207eda95217858f2190f283df5eccda8a1f
Merge pull request #55 from Pomax/bug882655

[#882655] SSO through direct iframe interaction

Comment 40

[[github robot]]

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

https://github.com/mozilla/webmaker.org/commit/0f66942e6e3019a560b8db7d9175b8d4d9404d80
Merge pull request #131 from Pomax/bug882655

[#882655] SSO through direct iframe interaction

Comment 41

[Pomax]

All the things have landed. Builds running.

Comment 42

[Pomax]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/127

Comment 43

[Matthew Schranz [:mjschranz]]

Comment on attachment 762819 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/127

R+

Comment 44

[Pomax]

building new thimble for staging.

Comment 45

[Pomax]

landed. for reals.