fix(client): generate fresh flow id and begin time on sign-out #4676
Conversation
|
I'm assuming the failing functional tests are to do with Vlad's comment in IRC, they should get fixed when you merge something else in @shane-tomlinson? |
| }); | ||
| // Navigate via the back-end in order to regenerate | ||
| // flow id and flow begin time for the next session. | ||
| this.window.location.href = '/signin'; |
shane-tomlinson
Jan 26, 2017
Member
👍 - I was just looking at how I do a similar thing in views/connect_another_device.js and realized a few things.
First, there should probably be an abstraction for the action of navigating via location.href instead via pushState. The reason is we want to ensure all metrics are flushed before redirecting and not all environments support the beforeunload handler, Safari on iOS is one example. With an abstraction, we could do something like:
navigateToUrl (url) {
return this.metrics.flush()
.then(() => this.window.location.href = url);
},
We have the this concept in external-links-mixin.js->_flushMetricsThenRedirect, maybe that could be surfaced as a public method, or maybe we could move that logic to router.js->navigate with a new option updateLocation: true or similar and have the external-links-mixin use that?
The second thing I noticed is that the connect another device page doesn't handle this either for the sign in button. Fortunately the sign in button is not displayed in Safari for iOS, so we should be good. You don't need to fix this, it's a note for me to open an issue about it.
The third thing I noticed is that we should probably propagate the current query parameters. If a user disconnects their current device when accessing /settings from within "Manage Account", then propagating the query params should allow the user to sign in again.
First, there should probably be an abstraction for the action of navigating via location.href instead via pushState. The reason is we want to ensure all metrics are flushed before redirecting and not all environments support the beforeunload handler, Safari on iOS is one example. With an abstraction, we could do something like:
navigateToUrl (url) {
return this.metrics.flush()
.then(() => this.window.location.href = url);
},We have the this concept in external-links-mixin.js->_flushMetricsThenRedirect, maybe that could be surfaced as a public method, or maybe we could move that logic to router.js->navigate with a new option updateLocation: true or similar and have the external-links-mixin use that?
The second thing I noticed is that the connect another device page doesn't handle this either for the sign in button. Fortunately the sign in button is not displayed in Safari for iOS, so we should be good. You don't need to fix this, it's a note for me to open an issue about it.
The third thing I noticed is that we should probably propagate the current query parameters. If a user disconnects their current device when accessing /settings from within "Manage Account", then propagating the query params should allow the user to sign in again.
philbooth
Jan 26, 2017
Author
Contributor
First, there should probably be an abstraction for the action of navigating via location.href instead via pushState. The reason is we want to ensure all metrics are flushed before redirecting and not all environments support the beforeunload handler...
Good point, I'll add something.
...we should probably propagate the current query parameters.
I opted not to do that because the existing code specifies clearQueryParams: true. Do you mean for the general case, and then override that in my usage, or do you mean my usage itself should propagate query params?
First, there should probably be an abstraction for the action of navigating via location.href instead via pushState. The reason is we want to ensure all metrics are flushed before redirecting and not all environments support the beforeunload handler...
Good point, I'll add something.
...we should probably propagate the current query parameters.
I opted not to do that because the existing code specifies clearQueryParams: true. Do you mean for the general case, and then override that in my usage, or do you mean my usage itself should propagate query params?
shane-tomlinson
Jan 26, 2017
Member
Do you mean for the general case, and then override that in my usage, or do you mean my usage itself should propagate query params?
Only in this case.
Do you mean for the general case, and then override that in my usage, or do you mean my usage itself should propagate query params?
Only in this case.
philbooth
Jan 26, 2017
Author
Contributor
Do you mean for the general case, and then override that in my usage, or do you mean my usage itself should propagate query params?
Only in this case.
A side-effect of this would be propagating the resume token. The resume token takes precedence over the data attributes for flow model initialisation. So if we propagate the resume token, it will cause duplicate flow events to be emitted, which is the problem I want to fix here.
I could make that resume-token-or-data-attribute check dependent on the current view, but that feels a bit sketchy. Any other suggestions?
Do you mean for the general case, and then override that in my usage, or do you mean my usage itself should propagate query params?
Only in this case.
A side-effect of this would be propagating the resume token. The resume token takes precedence over the data attributes for flow model initialisation. So if we propagate the resume token, it will cause duplicate flow events to be emitted, which is the problem I want to fix here.
I could make that resume-token-or-data-attribute check dependent on the current view, but that feels a bit sketchy. Any other suggestions?
|
I like this, much easier to follow. Considering this update, can clearSessionAndNavigateToSignIn be simplified some? The forms should automatically get cleared when calling |
philbooth
force-pushed the
phil/issue-3705-hard-refresh
branch
2 times, most recently
from
1757386
to
055b4fb
|
@shane-tomlinson, updated to flush metrics and propagate relier data to Note that I elected to not propagate the |
philbooth
force-pushed the
phil/issue-3705-hard-refresh
branch
2 times, most recently
from
e10784c
to
af25371
|
|
| @@ -762,6 +762,13 @@ define(function (require, exports, module) { | |||
| this._hasNavigated = true; | |||
| }, | |||
|
|
|||
| navigateViaServer (url) { | |||
shane-tomlinson
Jan 27, 2017
Member
Can you add a jsdoc here?
The method name, while describing exactly what it does, seems strange. serverNavigate seems equally strange. navigate(url, null, { server: true }) seems verbose.
What if it's something a bit more generic that can be used elsewhere, something like navigateToUrl. The name implies it's a hard redirect instead of via pushState. In particular, we could that to replace this bit of code in the external-links-mixin.
Can you add a jsdoc here?
The method name, while describing exactly what it does, seems strange. serverNavigate seems equally strange. navigate(url, null, { server: true }) seems verbose.
What if it's something a bit more generic that can be used elsewhere, something like navigateToUrl. The name implies it's a hard redirect instead of via pushState. In particular, we could that to replace this bit of code in the external-links-mixin.
philbooth
Jan 27, 2017
•
Author
Contributor
I did see the name navigateToUrl elsewhere and I'm not a fan. To me it fails to convey any meaningful difference to navigate. When viewed alongside each other, those two seem completely ambiguous I think.
I'm happy to drop navigateViaServer but I think the name(s) we use should reflect that the difference between the two methods is one makes a request to a server and the other occurs within the client.
Thinking about alternative possibilities now.
I did see the name navigateToUrl elsewhere and I'm not a fan. To me it fails to convey any meaningful difference to navigate. When viewed alongside each other, those two seem completely ambiguous I think.
I'm happy to drop navigateViaServer but I think the name(s) we use should reflect that the difference between the two methods is one makes a request to a server and the other occurs within the client.
Thinking about alternative possibilities now.
philbooth
Jan 27, 2017
Author
Contributor
How about request vs navigate? Does that better indicate that one of them is going to make an HTTP request?
How about request vs navigate? Does that better indicate that one of them is going to make an HTTP request?
shane-tomlinson
Jan 27, 2017
Member
I did see the name navigateToUrl elsewhere and I'm not a fan. To me it fails to convey any meaningful difference to navigate. When viewed alongside each other, those two seem completely ambiguous I think.
That method name isn't in the code currently, so must've been a comment.
How about request vs navigate? Does that better indicate that one of them is going to make an HTTP request?
Just request is too generic, it could mean any request and doesn't imply any side effects.
navigateAway, hardNavigate, leaveAppToUrl, redirectToUrl, maybe even setDocumentLocation?
I did see the name navigateToUrl elsewhere and I'm not a fan. To me it fails to convey any meaningful difference to navigate. When viewed alongside each other, those two seem completely ambiguous I think.
That method name isn't in the code currently, so must've been a comment.
How about request vs navigate? Does that better indicate that one of them is going to make an HTTP request?
Just request is too generic, it could mean any request and doesn't imply any side effects.
navigateAway, hardNavigate, leaveAppToUrl, redirectToUrl, maybe even setDocumentLocation?
philbooth
Jan 27, 2017
Author
Contributor
Of those, navigateAway and setDocumentLocation seem clearest to me. I'll go with navigateAway on the basis that setDocumentLocation is maybe too implementation-detaily.
Of those, navigateAway and setDocumentLocation seem clearest to me. I'll go with navigateAway on the basis that setDocumentLocation is maybe too implementation-detaily.
| // Unset uid otherwise it will henceforth be impossible | ||
| // to sign in to different accounts inside this tab. | ||
| this.relier.unset('uid'); | ||
| this.relier.unset('email'); |
shane-tomlinson
Jan 27, 2017
Member
Since email used to be cleared on the relier, we should probably not pass the email in the query parameters, otherwise the user's email is going to be displayed on the page, which might not be what the user expects.
Since email used to be cleared on the relier, we should probably not pass the email in the query parameters, otherwise the user's email is going to be displayed on the page, which might not be what the user expects.
philbooth
Jan 29, 2017
•
Author
Contributor
Thought it might be worth revisiting this comment in light of what you said further down:
...most users who sign in again after disconnecting the current device will have disconnected accidentally.
If that's the case, are we sure we don't want to propagate the email here? It seems like it might prevent some frustration for these users, if we think the majority who sign back in made a mistake.
Thought it might be worth revisiting this comment in light of what you said further down:
...most users who sign in again after disconnecting the current device will have disconnected accidentally.
If that's the case, are we sure we don't want to propagate the email here? It seems like it might prevent some frustration for these users, if we think the majority who sign back in made a mistake.
philbooth
Jan 30, 2017
Author
Contributor
If that's the case, are we sure we don't want to propagate the email here? It seems like it might prevent some frustration for these users, if we think the majority who sign back in made a mistake.
Ignore this, I misunderstood how the sign-in form behaves. I thought propagating the email was required for it to be set on the sign-in view, but that's working even after I took this out.
If that's the case, are we sure we don't want to propagate the email here? It seems like it might prevent some frustration for these users, if we think the majority who sign back in made a mistake.
Ignore this, I misunderstood how the sign-in form behaves. I thought propagating the email was required for it to be set on the sign-in view, but that's working even after I took this out.
| this.user.clearSignedInAccountUid(); | ||
| if (this._formPrefill) { |
shane-tomlinson
Jan 27, 2017
Member
Oh my, this is an awesome side-effect that I hadn't considered, formPrefill will be cleared automatically.
Oh my, this is an awesome side-effect that I hadn't considered, formPrefill will be cleared automatically.
| }, { | ||
| clearQueryParams: true | ||
| const queryString = Url.objToSearchString({ | ||
| context: this.relier.get('context'), |
shane-tomlinson
Jan 27, 2017
Member
Not including migration I understand, though I think we should continue to propagate the entrypoint and utm_ query params. One user was signed out, and the next signed in, the entrypoint is still the same, and we'll want to know where the new users came from when they complete the sign in.
As an aside, can you add a note about why only certain query parameters and not the query parameter list verbatim?
Not including migration I understand, though I think we should continue to propagate the entrypoint and utm_ query params. One user was signed out, and the next signed in, the entrypoint is still the same, and we'll want to know where the new users came from when they complete the sign in.
As an aside, can you add a note about why only certain query parameters and not the query parameter list verbatim?
philbooth
Jan 27, 2017
Author
Contributor
I think we should continue to propagate the entrypoint and utm_ query params... One user was signed out, and the next signed in, the entrypoint is still the same.
I strongly disagree with this but I realise my opinion probably doesn't qualify. I'll ask @davismtl later on to see what he thinks.
I think we should continue to propagate the
entrypointandutm_query params... One user was signed out, and the next signed in, the entrypoint is still the same.
I strongly disagree with this but I realise my opinion probably doesn't qualify. I'll ask @davismtl later on to see what he thinks.
shane-tomlinson
Jan 27, 2017
Member
Not including the utm_ and entrypoint is a change from current behavior as well.
Not including the utm_ and entrypoint is a change from current behavior as well.
philbooth
Jan 27, 2017
•
Author
Contributor
Well yes, but the current behaviour is wrong. Currently we emit duplicate flow events. This whole PR is a change from current behaviour.
Well yes, but the current behaviour is wrong. Currently we emit duplicate flow events. This whole PR is a change from current behaviour.
shane-tomlinson
Jan 27, 2017
Member
@davismtl and I had a 1:1 today and I asked.. He reckons most users who sign in again after disconnecting the current device will have disconnected accidentally. From that P.O.V., they should have the same entrypoint. He also reckons the counts are going to be really low so we really shouldn't stress whichever way.
@davismtl and I had a 1:1 today and I asked.. He reckons most users who sign in again after disconnecting the current device will have disconnected accidentally. From that P.O.V., they should have the same entrypoint. He also reckons the counts are going to be really low so we really shouldn't stress whichever way.
philbooth
Jan 27, 2017
Author
Contributor
the counts are going to be really low so we really shouldn't stress whichever way.
Cool, thanks.
the counts are going to be really low so we really shouldn't stress whichever way.
Cool, thanks.
| @@ -155,6 +159,13 @@ define(function (require, exports, module) { | |||
| return Backbone.Router.prototype.navigate.call(this, url, options); | |||
| }, | |||
|
|
|||
| navigateViaServer (url) { | |||
shane-tomlinson
Jan 27, 2017
Member
I realize many of the methods in this file lack jsdocs, can you add one? I'm trying to add them as I go in other PRs too.
And what do you think about the comment made here about changing the name to navigateToUrl?
I realize many of the methods in this file lack jsdocs, can you add one? I'm trying to add them as I go in other PRs too.
And what do you think about the comment made here about changing the name to navigateToUrl?
|
@shane-tomlinson I've made all your requested changes. |
|
Merging this on the basis that @shane-tomlinson's feedback is all implemented and he's off sick. Hope that's cool... |
The right thing to do. |
Fixes #3705. Fixes #3835 (indirectly).
After my protracted, abortive struggle in #4654, @shane-tomlinson generously pointed out that we could just do a hard navigate and force the server to regenerate
flowIdandflowBeginTimeon our behalf. I feel like such a fool.This change does that, touching 25 fewer files and 550-ish fewer lines than my previous attempt. I am enlightened.
@shane-tomlinson r?