feat(client): notify other tabs of successful sign in/out #3144
Conversation
| 'views/base', | ||
| 'views/form', | ||
| 'stache!templates/sign_in', |
philbooth
Oct 7, 2015
Author
Contributor
I did this under the impression that we were trying to move towards alphabetical order everywhere; this list was a bit of a mess and it wasn't clear where the correct insertion point for the new dependency ('views/mixins/inter-tab-channel-mixin') was otherwise. Hope that's okay.
I did this under the impression that we were trying to move towards alphabetical order everywhere; this list was a bit of a mess and it wasn't clear where the correct insertion point for the new dependency ('views/mixins/inter-tab-channel-mixin') was otherwise. Hope that's okay.
shane-tomlinson
Oct 9, 2015
Member
👍
philbooth
force-pushed the
phil/issue-3024
branch
2 times, most recently
from
6ce5bd3
to
4aca072
|
Is this ready for review @philbooth? |
|
@shane-tomlinson It is! |
| this, options); | ||
|
|
||
| this.unsetCapability('interTabSignIn'); |
vladikoff
Oct 9, 2015
Contributor
should this override this in the same way as it is done here instead of using unset: https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/models/auth_brokers/fx-fennec-v1.js#L27 ? Also why is OAuth not using interTabSignIn, could we add a comment?
should this override this in the same way as it is done here instead of using unset: https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/models/auth_brokers/fx-fennec-v1.js#L27 ? Also why is OAuth not using interTabSignIn, could we add a comment?
| self.navigate('signin', { | ||
| success: t('Signed out successfully') | ||
| }); | ||
| }; |
philbooth
Oct 10, 2015
Author
Contributor
Do you mean move the body of the function out or move the assignment itself out?
Do you mean move the body of the function out or move the assignment itself out?
philbooth
force-pushed the
phil/issue-3024
branch
2 times, most recently
from
99de93a
to
c3bc19c
|
Have pushed fixes for your comments @vladikoff, thanks for the feedback. I assumed the second one objected to the function body rather than the assignment, but let me know if I misunderstood. |
| DEFAULT_XHR_TIMEOUT_MS: 2500 | ||
| DEFAULT_XHR_TIMEOUT_MS: 2500, | ||
|
|
||
| SIGNIN_SUCCESS: 'signin.success', |
shane-tomlinson
Oct 12, 2015
Member
Since these are both messages can you add a prefix or suffix saying so? I wonder if there is a better place for these, closer to where they are used so some local context exists. I sometimes wonder if constants.js was a mistake.
Since these are both messages can you add a prefix or suffix saying so? I wonder if there is a better place for these, closer to where they are used so some local context exists. I sometimes wonder if constants.js was a mistake.
| assert.isTrue(broker.hasCapability('interTabSignIn')); | ||
| }); | ||
|
|
||
| it('returns `true` for `emailVerificationMarketingSnippet` by default', function () { |
shane-tomlinson
Oct 12, 2015
Member
Thanks, I completely forgot to add these.
Thanks, I completely forgot to add these.
| defaultCapabilities: _.extend({}, proto.defaultCapabilities, { | ||
| // Disable inter-tab sign-in for OAuth due to the potential for unintended | ||
| // consequences from redirecting to a relier URL more than once. | ||
| interTabSignIn: false |
shane-tomlinson
Oct 12, 2015
Member
Just to understand, interTabSignIn only affects the sign in message, all environments support the signout message by default?
Just to understand, interTabSignIn only affects the sign in message, all environments support the signout message by default?
philbooth
Oct 12, 2015
Author
Contributor
Yep.
Yep.
| function interTabSignInHandler (event) { | ||
| if (this.broker.hasCapability('interTabSignIn')) { | ||
| this.user.setSignedInAccount(event.data) | ||
| .then(this.navigate.bind(this, 'settings')); |
shane-tomlinson
Oct 12, 2015
Member
settings seems reasonable here, though if all brokers redirect to the /settings page, the capability check might not be necessary. In the comment here, you mention that OAuth users should not redirect to the relier as the reason for the capability. If the user is always redirected to settings, they are never redirected to the relier AFAICT.
settings seems reasonable here, though if all brokers redirect to the /settings page, the capability check might not be necessary. In the comment here, you mention that OAuth users should not redirect to the relier as the reason for the capability. If the user is always redirected to settings, they are never redirected to the relier AFAICT.
philbooth
Oct 12, 2015
Author
Contributor
Wouldn't the OAuth redirect URL be the more logical place to redirect them to though? An OAuth user may not know or care what Firefox Accounts is, so finding a background tab has been redirected to our settings page might seem weird to them.
Wouldn't the OAuth redirect URL be the more logical place to redirect them to though? An OAuth user may not know or care what Firefox Accounts is, so finding a background tab has been redirected to our settings page might seem weird to them.
| @@ -310,4 +325,11 @@ function (Cocktail, p, BaseView, FormView, SignInTemplate, Session, | |||
| ); | |||
|
|
|||
| return View; | |||
|
|
|||
| function interTabSignInHandler (event) { | |||
shane-tomlinson
Oct 12, 2015
Member
Do you think it makes sense to take this functionality, add it to a mixin, and hook it up to /signup and /reset_password too? I'm unsure about how the /confirm_* screens should act.
Do you think it makes sense to take this functionality, add it to a mixin, and hook it up to /signup and /reset_password too? I'm unsure about how the /confirm_* screens should act.
shane-tomlinson
Oct 12, 2015
Member
Another edge case is tab 1 is at /signin, in tab 2, the user signs up & verifies, tab 1 just sits there. Am I too bothered? Not really.
Another edge case is tab 1 is at /signin, in tab 2, the user signs up & verifies, tab 1 just sits there. Am I too bothered? Not really.
philbooth
force-pushed the
phil/issue-3024
branch
3 times, most recently
from
f58bdcf
to
8dcec33
|
@philbooth - I just came across PR #2994 that sends an |
|
FWIW, I'm not sure if I would have used |
philbooth
force-pushed the
phil/issue-3024
branch
3 times, most recently
from
caaaf14
to
dccec19
|
@philbooth - you'll need to rebase this! |
Yep, I'm still working on it though, not ready to be reviewed yet! :) |
Fixes #3024, redirecting to sign-in/settings across multiple tabs.
Deliberately does not address OAuth sign-in because of potential unintended consequences from redirecting to a relier URL more than once. Maybe that is something that can be opted in to per-relier in the future. There's also no attempt to cover successful sign-up here either.
Because
InterTabChannelMixinis now a dependency of the sign-in and settings views, there was a little bit of collateral damage (force_auth::initialize, the router tests) where the new dependency also needed to be met.The change makes use of the broker capabilities, which weren't comprehensively covered by the unit tests before. So I added a bunch of assertions for their default capabilities as part of this change, since some of the new tests assumed that the capabilities would be correct.
There is no new functional test for OAuth sign-in, because I couldn't work out how to reliably test that a thing doesn't happen with Selenium. If I simply
findById('fxa-signin-header'), it succeeds immediately instead of waiting for navigation. I couldsleep(), but I've been bitten by those before; if you have better suggestions, I'll be happy to implement them. The OAuth behaviour is covered in the unit tests though, by stubbinghasCapabilityto returnfalseand asserting that the sign-in activity does not occur.