Skip to content
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

add an 'auto-logout toggle' (take 2) #447

Closed
wants to merge 1 commit into from

Conversation

twrichards
Copy link
Contributor

@twrichards twrichards commented Jun 6, 2024

This reattempts #439, which was reverted in #445 because it doesn't work in FireFox [and Chrome in a few months] owing to stricter policy w.r.t. SameSite attribute of cookies and AWS cookies need to be present when hitting logout. This time we achieve the same end result with window.open( ) which is displayed for 0.75s before being closed and redirected (which should allow enough time for the logout to complete)...
janus_auto_logout_popup

…e [for AWS Console links] open `https://signin.aws.amazon.com/logout` for 0.75s (to allow logout to complete) before redirecting to the AWS console for desired/clicked account
setTimeout(() => {
popup.close();
location.href = targetHref;
}, 750);
Copy link
Contributor

@adamnfish adamnfish Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can't detect a successful logout request (which as we've discussed, really ought not to be possible) then there's an inherent trade-off between inconvenience and reliability. 750ms is aaaages so it should be fine, but we know it isn't always going to be. We're delaying the page by enough to be annoying, without it being enough to be reliable.

This is on top of the other issues with the approach. For example, I usually open links in a new tab and this feature won't work in that case. We're building something that doesn't really fit with how web UIs work, which means it is sure to have surprising behaviour from time to time. This is why I went down a quick rabbit hole on this feature a few years back, and we ended up deciding to wait and see if AWS offer support / people needed it enough.

But this PR suggests people do want to the feature! It's always a delight to get a contribution from someone that wants to improve the tool for themselves and their colleagues. 750ms is way less than doing it manually, I can see how this will be useful to people. so if this is a feature people want we should go ahead with it.

A word of caution though, thesee UI papercuts all add up. UIs are the main way people interact with our tools and unreliable UIs add to the sense that our tooling is old, poorly maintained, or no longer fit for purpose. We should aspire to build UIs that are reliable. They should be accessble, responsive (to interaction and browser size), elegantly handle edge cases, give good error feedback, and make impossible states impossible. By far the easiest way to achieve this is to build standards-compliant UIs that lean into the browser platform - not reinventing browser features or (in this case) trying to work around them.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this the outline of what is in tension here!

I do agree with the sentiment of being able to achieve super-slick UIs, especially ones we use continuously through the day. Wasting time on substandard UIs ultimately means I use them less or it just makes my day feel like mud.

But - the current situation for some engineers is muddier than mud1 in regards to the current logout workflow - and this alleviates some of the faff. Could this be v0.1 of the feature, get feedback on it e.g. How often it fails, do people find it useful, do people even use it etc etc. If people like it - it could indicate needing to iterate on it. It might even prompt someone who then gets frustrating with this trade-off to try find a better way? If it's not really used, or deemed not easy enough to use - it might not worth the overhead and clutter - and we can remove it.

If we do choose to try it - I also wonder if we could be a little more optimistic and go for < 500ms? 750ms is aaaaages, or at least a noticeable amount. The failure in that instance then s you get the old logout flow 🤷.

Footnotes

  1. I'll drop the analogy here

};

$("a[href*='/console?permissionId=']").each(function(_, el){
el.onclick = (clickEvent) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On @adamnfish's point of opening in a new tab (untested), are we at risk of a race condition? i.e.

  • it opens a new tab
  • logs into account-a
  • popup opens in background
  • logs me out of my newly created session in account-a

Granted this would only be the case if I hadn't been signed into another account (beginning of the day) but it feels like this would have to work well for both situations?

Copy link
Contributor

github-actions bot commented Jul 8, 2024

This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days

@github-actions github-actions bot added the Stale label Jul 8, 2024
Copy link
Contributor

This PR was closed because it has been stalled for 3 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants