-
Notifications
You must be signed in to change notification settings - Fork 21
NOJIRA sign out of all synchronized tabs #523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ function TimeoutDialog($module, $sessionActivityService) { | |||
| const cleanupFunctions = []; | ||||
| let currentTimer; | ||||
| const sessionActivityService = $sessionActivityService; | ||||
| let signedOut = false; | ||||
|
|
||||
| function init() { | ||||
| const validate = ValidateInput; | ||||
|
|
@@ -61,6 +62,9 @@ function TimeoutDialog($module, $sessionActivityService) { | |||
| synchroniseTabs: validate.boolean( | ||||
| lookupData('data-synchronise-tabs') || false, | ||||
| ), | ||||
| synchroniseTabsSignout: validate.boolean( | ||||
| lookupData('data-synchronise-tabs-signout') || false, | ||||
| ), | ||||
| hideSignOutButton: validate.boolean( | ||||
| lookupData('data-hide-sign-out-button') || false, | ||||
| ), | ||||
|
|
@@ -82,6 +86,12 @@ function TimeoutDialog($module, $sessionActivityService) { | |||
| const listenForSessionActivityAndResetDialogTimer = () => { | ||||
| if (settings.synchroniseTabs) { | ||||
| sessionActivityService.onActivity((event) => { | ||||
| if (event.signedOut) { | ||||
| if (!signedOut && settings.synchroniseTabsSignout) { | ||||
| signOut(); | ||||
| return; | ||||
| } | ||||
| } | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. apologies! Taken me longer than I thought to come back, I am still a bit unsure about scenario where someone signs out after updating to a version of hmrc-frontend with this - it seems because of the reuse of session activity channel - and because events in that channel aren't discriminated in any way - like at the moment any message to it for someone on current version would be presumed to be session activity (that should keep session alive) so if message I'm not actually sure how default params work for js functions - if passing undefined here will cause the default to be used, or if it will cause it to be undefined
it might be I guess that it won't break anything but will cause background tabs timeout timers to reset - where they might get reset anyway, if the foreground tab which was explicitly signed out lands on a page with timeout dialog on it and some session activity from new session is broadcast then one other thing I notice with this, background tabs will rebroadcast the sign out event - so if you have 4 tabs, and you sign out in 1, so there's the chance for some kind of sign out loop or oddness like if someone signs out in one window, they get redirected immediately to sign out endpoint and may end up after that on a page with timeout dialog on it again - if that was case and then there is the chance that sign out events broadcast from background pages would hit pages from the new session and could theoretically create some kind of sign out loop I guess the bits that are making stuff difficult:
this is kind of hitting on the past stuff we had with this I think where it feels really complicated and like there are quite a lot of risks in terms of architecture of frontend services in HMRC
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. timeout dialog code just seems to be quite a precarious jenga tower to make changes too - it might MIGHT be easier to fork it and modify it for your purposes - than try to change it upstream - and then for the future maybe it's the kind of thing that would be better reimplemented with with the bits making stuff difficult considered upfront as part of the design -where we kind of patched in this session awareness stuff |
||||
| const timeOfActivity = event.timestamp; | ||||
| cleanup(); | ||||
| setupDialogTimer(timeOfActivity); | ||||
|
|
@@ -285,6 +295,8 @@ function TimeoutDialog($module, $sessionActivityService) { | |||
| const getDateNow = () => Date.now(); | ||||
|
|
||||
| const signOut = () => { | ||||
| signedOut = true; | ||||
| sessionActivityService.logSignedOut(); | ||||
| RedirectHelper.redirectToUrl(settings.signOutUrl); | ||||
| }; | ||||
|
|
||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of hmrc services share a domain tax.service.gov.uk so if we implemented this change there would be some services with the new js and some on the old - I think the way that sessionActivityService is implemented at the moment - broadcasting signOut event through the same channel would look make it look like session activity to other tabs because it doesn't discriminate type of event at the moment - not sure what that would result in (though somewhat might be same as current - in that once user has signed out, and they hit another page of a tax service the session activity might be broadcast from that new page - though lack of timestamp that it expects from session activity might cause an error maybe)
some other thoughts
I think I'll need to think about it a bit more thoroughly - it makes sense to me - but might need to be extra defensive in how it's implemented / make it so you need to opt-in somehow such that we don't turn it on by default for stuff on tax.service.gov.uk so we didn't break some unknowingly delicately balanced thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. Happy to add another option
synchroniseTabsSignoutdata-synchronise-tabs-signoutWould that work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup that would make it easier to add for us I think - when we first implemented the syncing - it was just to stop a background tab being able to take out the session - and we flagged up there were still gaps / not great behaviours (like if data was entered in form on background tab, without this the data could sit in there filled in indefinitely- and as you say have it seem like session was still active) but we weren't confident we wouldn't pull wrong jenga piece and cause a live incident of some kind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed an updated version.