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

Web: Fix local storage clearing #27256

Merged
merged 2 commits into from Jun 2, 2023
Merged

Web: Fix local storage clearing #27256

merged 2 commits into from Jun 2, 2023

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Jun 2, 2023

fixes #27254

fixes a bug where upon logout, the local storage was not getting cleared as expected.

i'm not sure if i am explaining this right, but the previous code, as we were deleting keys, the next index value did not necessarily meant it was the next key. if the previous key was deleted, then the next index value may refer to next next key (or out of bounds). so depending on the order of keys, it was a delete or miss, which is why users were having inconsistent issues with login/logouts

the incident was: when a user logged in, the user got logged right back out, and it was because the local storage key LAST_ACTIVE: 'grv_teleport_last_active' was not getting deleted, so it kept the old expired value.

this bug was introduced (backported) starting from 12.4.0

when i wrote the test, i tested it against the old code first, which failed

i was also able to replicate the issue where my local storage wasn't getting cleared

re @zac about

// See if there is inactive date already set in local storage.
, i think we do need this check to handle a case where user just closes tabs (but not the browser) which does not clear the storage, so user can step away, and someone else could go load the website and resume the user's session. granted i only tested this with idle time set to 1min...

@zmb3
Copy link
Collaborator

zmb3 commented Jun 2, 2023

Shouldn't we also update last active on login as an extra measure to prevent this?

Can you also link to the PR that introduced the issue?

@kimlisa
Copy link
Contributor Author

kimlisa commented Jun 2, 2023

Shouldn't we also update last active on login as an extra measure to prevent this?

Can you also link to the PR that introduced the issue?

i did think about it, but didn't think it was really necessary to reset the LAST_ACTIVE on login, b/c if clearing the storage on logout works correctly, there should never be a stagnant LAST_ACTIVE on login. If for some reason there is one (perhaps user logged out on the bugged version, which may not have deleted the LAST_ACTIVE and then use the patched version?), user will at most re-login once.

here are the cases where we auto log the user out (which should clear local storage):

  • inactivity
  • access request expired
  • invalid session
  • expired token

if you still think it's a nice to have, i'll add it

Comment on lines 32 to 36
const keys = [];
for (let i = 0; i < window.localStorage.length; i++) {
const key = window.localStorage.key(i);

keys.push(window.localStorage.key(i));
}
keys.forEach(key => {
if (!KEEP_LOCALSTORAGE_KEYS_ON_LOGOUT.includes(key)) {
window.localStorage.removeItem(key);
}
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but we can clean this up a bit by just using Object.keys

Object.keys(window.localStorage).forEach

@kimlisa kimlisa force-pushed the lisa/local-storage-bug-fix branch from 4a5c60c to 53b316c Compare June 2, 2023 17:40
@kimlisa kimlisa enabled auto-merge June 2, 2023 17:41
@kimlisa kimlisa added this pull request to the merge queue Jun 2, 2023
Merged via the queue into master with commit cc148f2 Jun 2, 2023
20 checks passed
@kimlisa kimlisa deleted the lisa/local-storage-bug-fix branch June 2, 2023 18:10
@public-teleport-github-review-bot

@kimlisa See the table below for backport results.

Branch Result
branch/v12 Create PR
branch/v13 Create PR

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

Successfully merging this pull request may close these issues.

Web UI immediately logs user out
5 participants