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

Fix session state issues #483

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix session state issues #483

wants to merge 4 commits into from

Conversation

emlun
Copy link
Member

@emlun emlun commented Dec 3, 2024

Fixes part of #474: log out whenever api and keystore are not both logged in. This fixes the issue with sessionStorage attributes from the api module remaining after the idle logout enforced by the keystore.

emlun added 3 commits December 3, 2024 18:14
The cause of this data corruption is: after successfully logging in and
unlocking the keystore, the `useEffect` in `LocalStorageKeystore` calls
`setCachedUsers` to update the cached users (to copy the `prfKeys` from the
logged-in user's `privateData` to the matching cached user). But when there are
two tabs open simultaneously, that code runs simultaneously in both tabs, and
the `globalUserHandleB64u` is different between the two tabs. The user1 tab has
`globalUserHandleB64u` set to the user handle of user1, and the user2 tab has
`globalUserHandleB64u` set to the user handle of user2. So the result is that
both cached users get updated with user2's `prfKeys`, which then causes the user
to be logged into user2 even if they press the cache button labeled "Log in as
user1".
@emlun emlun changed the title Session issues Fix session state issues Dec 3, 2024
@emlun
Copy link
Member Author

emlun commented Dec 3, 2024

@gkatrakazas I've force-pushed over this branch with a new fix which seems to work more reliably and not log the user out immediately on signup. Please give this another go!

Copy link
Member

@gkatrakazas gkatrakazas left a comment

Choose a reason for hiding this comment

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

Great work! Everything seems to function perfectly in most cases. However, I noticed one case: when a user logs in or signs up in one tab, causing another user tabs to log out, the SessionContext logout function is not triggered but on userInactivity works as expected.

@emlun
Copy link
Member Author

emlun commented Dec 6, 2024

Ah, right. That was intentional because the logout function in SessionContext clears both sessionStorage and localStorage, so if user1 triggers logout because user2 logged in, then the logout call from user1 will immediately log user2 out right after login. But you're right that it makes sense to also clear the api session when the keystore closes due to a different user logging in. Commit 95b8bed fixes this.

@gkatrakazas
Copy link
Member

The primary issue addressed in my PR (#488) was the multiple event listeners being attached to a single tab and the repeated session cleanup processes. The goal was to streamline the cleanup process to ensure it executes efficiently and consistently for every logout or session termination. Feel free to evaluate if this improves the overall cleanup process.

@emlun
Copy link
Member Author

emlun commented Dec 6, 2024

Heh, interesting, I had also started on a similar refactorization I hadn't pushed 🙂 but with a (mostly) different goal than you had in #488. I've pushed this too as PR #489, please have a look! Maybe there's some useful overlap between these PRs.

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

Successfully merging this pull request may close these issues.

2 participants