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

RESOLVED FIXED

Status

--
blocker
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: brett, Assigned: pomax)

Tracking

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Whats the importance field above "blocker"?
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 :)
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.
(Assignee)

Comment 3

5 years ago
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?
(Assignee)

Updated

5 years ago
Assignee: nobody → pomax
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 8

5 years ago
NOTE: THIS CODE HAS NOT BEEN CLEANED UP YET
(Assignee)

Comment 9

5 years ago
NOTE 2: ONCE THIS LANDS, ALL CONSUMER APPS NEED THEIR SNIPPET UPDATED
Working nicely, but needs some cleanup.
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
(Assignee)

Comment 12

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

part one of three
Attachment #762142 - Flags: review?(david.humphrey)
(Assignee)

Comment 13

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

part two of three
Attachment #762143 - Flags: review?(david.humphrey)
(Assignee)

Comment 14

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

part three of three
Attachment #762144 - Flags: review?(david.humphrey)
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.
Attachment #762142 - Flags: review?(david.humphrey) → review+
(Assignee)

Comment 16

5 years ago
we're doing the styling - peek a gandour at the .css we're using in the webmaker.org pull request.
Comment on attachment 762143 [details] [review]
https://github.com/mozilla/webmaker.org/pull/131

A few comments in the PR, r+ with those.
Attachment #762143 - Flags: review?(david.humphrey) → review+
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).
Attachment #762144 - Flags: review?(kate)
Attachment #762144 - Flags: review?(david.humphrey)
Attachment #762144 - Flags: review+
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.
(Assignee)

Comment 20

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

refactored CSS needs a design pass, too.
Attachment #762143 - Flags: review+ → review?(kate)
(Assignee)

Comment 21

5 years ago
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.
(Assignee)

Comment 22

5 years ago
Created attachment 762324 [details] [review]
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?
Attachment #762324 - Flags: review?(schranz.m)
Attachment #762324 - Flags: review?(jon)
Attachment #762324 - Flags: review?(david.humphrey)
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.
Attachment #762324 - Flags: review?(david.humphrey) → review+
(Assignee)

Comment 24

5 years ago
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.
Created attachment 762690 [details] [diff] [review]
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.
Attachment #762690 - Flags: review?(jon)
Attachment #762690 - Flags: review?(david.humphrey)
Attachment #762324 - Flags: review?(schranz.m) → review-
(Assignee)

Comment 26

5 years ago
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 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
Attachment #762324 - Flags: review- → review+
(Assignee)

Comment 28

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

just-to-be-sure r?
Attachment #762142 - Flags: review+ → review?(schranz.m)
(Assignee)

Comment 29

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

just-to-be-sure check
Attachment #762143 - Flags: review?(schranz.m)
(Assignee)

Comment 30

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

just to be sure... v3
Attachment #762144 - Flags: review+ → review?(schranz.m)
Comment on attachment 762690 [details] [diff] [review]
Additional work for Popcorn Patch

This was merged into the Popcorn pull request.
Attachment #762690 - Attachment is obsolete: true
Attachment #762690 - Flags: review?(jon)
Attachment #762690 - Flags: review?(david.humphrey)
Comment on attachment 762142 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/119

R+, looks good locally for me.
Attachment #762142 - Flags: review?(schranz.m) → review+
Comment on attachment 762143 [details] [review]
https://github.com/mozilla/webmaker.org/pull/131

R+ from me. Looked good locally.
Attachment #762143 - Flags: review?(schranz.m) → review+
Comment on attachment 762144 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/102

R+ again. Worked well locally for me.
Attachment #762144 - Flags: review?(schranz.m) → review+
(Assignee)

Updated

5 years ago
Attachment #762324 - Flags: review?(jon)
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
(Assignee)

Comment 36

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 883236
Attachment #762143 - Flags: review?(kate) review?(kate) → review+ review+
Attachment #762144 - Flags: review?(kate) → review+

Comment 37

5 years ago
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

5 years ago
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

5 years ago
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

5 years ago
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
(Assignee)

Comment 41

5 years ago
All the things have landed. Builds running.
(Assignee)

Comment 44

5 years ago
building new thimble for staging.
(Assignee)

Comment 45

5 years ago
landed. for reals.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Duplicate of this bug: 881096
Attachment mime type: text/plain text/plain text/plain text/plain text/plain → text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.