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 redundant event listeners and session cleanup #488

Merged
merged 9 commits into from
Jan 23, 2025

Conversation

gkatrakazas
Copy link
Member

@gkatrakazas gkatrakazas commented Dec 6, 2024

This PR propose fixes for issues related to session management and logout synchronization across tabs ( pr #483 ), ensuring a consistent and optimized user experience.

Session Storage Cleanup

  • Ensures session storage is properly cleaned in all logout scenarios
  • Optimizes the event listener system:
    • Improved the conditions for sending events by split the uEffect which calling the closeTabLocal in order to send event only once in any case.
    • Refined how events are received and handled, reducing unnecessary overhead.

Implementation Notes

  • Added console logs at key points to facilitate easier review and debugging during testing. These logs will be removed before the merge.

emlun and others added 4 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".
@gkatrakazas gkatrakazas requested a review from emlun December 6, 2024 15:43
@emlun
Copy link
Member

emlun commented Dec 6, 2024

I think most of commit 6fb4f5b is now duplicated by commit 95b8bed in #483, right?

It's also not clear to me why the changeIsLoggedIn thing is necessary. This seems like adding a lot of extra moving part for something that feels like it should be much simpler to solve.

@gkatrakazas
Copy link
Member Author

gkatrakazas commented Dec 6, 2024

Let's set aside the freshlogin for now.

So let me clarify better the issues I was trying to address.


Issue with Event Listeners

When using addEventListener(..., { once: true }), the behavior can lead to unintended consequences. Specifically, if a tab listens to an event once, it will only listen again after a full page reload. Here's a scenario to illustrate the problem:

  1. Open tab_1 and sign up as test1.
  2. Open tab_2 and sign up as test2. This causes tab_1 to log out and clean its session (as expected).

If we repeat steps 1 and 2 without reloading the tabs, tab_2 will no longer listen to the clean session event. This behavior forced us to remove the { once: true } configuration and instead follow this logic:

useEffect(() => {
  // Add event listener
  keystoreEvents.addEventListener('eventName', eventHandler);

  // Cleanup event listener to prevent duplicates
  return () => {
    keystoreEvents.removeEventListener('eventName', eventHandler);
  };
}, []);

Preventing Circular Event Calls

Another issue arose when calling close() on logout. The close() function triggers:

eventTarget.dispatchEvent(new CustomEvent(KeystoreEvent.Close));

This, in turn, calls close() again, creating a loop. While the { once: true } option prevented this issue, removing it introduced the risk of circular calls, which needed to be carefully handled.

Multiple Event Dispatch Issue

Lastly, we addressed an edge case in LocalStorageKeystore.ts.

When a user clicks logout in one tab, the closeTabLocal() function is triggered in other tabs. However, this could be called more that one time, causing the event listener to send duplicate messages. We needed to ensure that each logout triggers the event exactly once, even if multiple tabs are open.
That was the reason i split the useEffect into three.

Now regarding the freshLogin the issue, when a user redirect to us with params, we store to 'state on privateRoute and we pass as state to Login page and redirect him back to previus. The problem here is when the user logout and login again the state keep it and redirect him again to the url with params, the only way to clean up that, is on logout to redirect to Login wiouth state. I am trying to find the best way to do that.

@emlun
Copy link
Member

emlun commented Jan 17, 2025

(See also reply in #489 (comment))

Specifically, if a tab listens to an event once, it will only listen again after a full page reload.

Hm, ok. I thought that it wouldn't actually behave like that because the useEffect would re-register the listener on each call of the hook whenever state changes. I thought I tested that too. But I suppose that doesn't always work, then.

@gkatrakazas gkatrakazas force-pushed the session-issues-logout branch from b02e313 to 6c94f01 Compare January 22, 2025 11:42
@gkatrakazas gkatrakazas changed the base branch from session-issues to master January 22, 2025 11:44
@gkatrakazas gkatrakazas force-pushed the session-issues-logout branch from 541b21d to b86e169 Compare January 22, 2025 14:54
@gkatrakazas gkatrakazas merged commit 3d7a82f into master Jan 23, 2025
4 checks passed
@gkatrakazas gkatrakazas linked an issue Jan 23, 2025 that may be closed by this pull request
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.

Improve Logout Behavior and Session Management
3 participants