Remove credentials and session from Onyx on sign out #7397

luacmartins · 2022-01-25T20:54:14Z

Details

Removes session and credentials from local storage after sign out.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/193459

Tests

Login
Sign out
Refresh the page and verify that you still see the login page.

Verify that no errors appear in the JS console

QA Steps

Steps above.

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

web.mov

Mobile Web

Desktop

desktop.mov

iOS

Android

github-actions · 2022-01-25T20:56:56Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

danieldoglas

LGTM

…ignOut Remove credentials and session from Onyx on sign out (cherry picked from commit ba15491)

OSBotify · 2022-01-25T23:13:31Z

🚀 Cherry-picked to staging by @danieldoglas in version: 1.1.32-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

kidroca · 2022-01-28T20:53:52Z

src/libs/actions/Session/index.js

+    Onyx.set(ONYXKEYS.SESSION, null);
+    Onyx.set(ONYXKEYS.CREDENTIALS, null);
    Timing.clearData();
    redirectToSignIn();


Hey why is this necessary redirectToSignIn is clearing the whole storage anyway?

For some reason redirectToSignIn was failing to clear the storage and discard the authToken. This PR solved an issue where after being redirected, the user could refresh the page and would be logged in again.

For some reason redirectToSignIn was failing to clear the storage and discard the authToken. This PR solved an issue where after being redirected, the user could refresh the page and would be logged in again.

What was the reason?

Not sure yet. I'll investigate it as part of https://github.com/Expensify/Expensify/issues/194430

Update - it seems that Onyx.clear is slow. In my tests, it took about 2.3s for Onyx.clear to be executed. In the meantime, we could refresh the page and "sign in" again since an authToken is still stored.

I took a quick look at the implementation of Onyx.clear, but I'm not sure how to improve its performance. Any suggestions? @kidroca @marcaaron

Does it mean that if I have component A that is subscribed to three keys, then when clear() is called, component A re-renders three times?

yes and no, depending on what you consider a render
yes - it would trigger render method 3 times and also trigger any children renders.
no - the screen might not re-render at all, or only do it once

setState is called per key and each time triggers a re-render

subscriber.withOnyxInstance.setState({ [subscriber.statePropertyName]: data, });

Another possible alternative is something like adding a clearWithoutTriggeringSubscribers() type method.

This would not make any subscribers aware of the change and isAuthenticated would not transition to false

This would not make any subscribers aware of the change and isAuthenticated would not transition to false

Yep, you're right.

OK, so another thought. We know that when we are signing out, we want everything to get unmounted. What if we unmount everything before calling clear()? Practically, that could be something like:

Click sign out

Go to /signout which is just a special stand-alone page (everything else in the app would be unmounted at this point)

Clear out Onyx

Redirect to sign in

Maybe that's still too much of a hack. Again, I'd rather fix the performance in the first place, but I did want to see what other alternatives we could think of.

The best alternative (besides making clear or Onyx faster) is already applied

Onyx.set(ONYXKEYS.SESSION, null); Onyx.set(ONYXKEYS.CREDENTIALS, null);

Removes authToken unmounting 95% of App and takes us to the SignIn page
Then Onyx.clear clears the rest of the stored values

@luacmartins Onyx.clear happens faster with these changes and doesn't take 2.3s, right?

Hmm either something changed or my computer is faster today, as both cases took about 1.2-1.3s. The difference in timing is pretty small, so it's hard to say that clear happens faster because of those changes. I used the same account for both tests, cleared browsing data and did a fresh login in each scenario.

With changes

Without changes

It seems JS blocking time is 100ms
Maybe in the past it was 1000 for some reason, or it was never the cause for the delay...

It might depend on the amount of data stored in index DB
Or if reads / writes were happening while clearing for some reason

remove credentials and session from Onyx on signOut

f272bca

luacmartins self-assigned this Jan 25, 2022

luacmartins added the CP Staging label Jan 25, 2022

luacmartins marked this pull request as ready for review January 25, 2022 21:00

luacmartins requested a review from a team as a code owner January 25, 2022 21:00

MelvinBot requested review from Gonals and removed request for a team January 25, 2022 21:01

danieldoglas approved these changes Jan 25, 2022

View reviewed changes

danieldoglas merged commit ba15491 into main Jan 25, 2022

danieldoglas deleted the cmartins-deleteCredentialsOnSignOut branch January 25, 2022 22:03

OSBotify pushed a commit that referenced this pull request Jan 25, 2022

Merge pull request #7397 from Expensify/cmartins-deleteCredentialsOnS…

710e869

…ignOut Remove credentials and session from Onyx on sign out (cherry picked from commit ba15491)

This was referenced Jan 25, 2022

🍒 Cherry pick PR #7397 to staging 🍒 #7402

Merged

Deploy Checklist: New Expensify 2022-01-25 #7395

Closed

thienlnam mentioned this pull request Jan 26, 2022

[$4000] Android - Infinite loading spinner appears when sign in to the app leaving idle for 10 min after sign out #7424

Closed

kidroca reviewed Jan 28, 2022

View reviewed changes

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

fedirjh mentioned this pull request Nov 17, 2022

[$2000] BUG: unread indicator count is not reset after logout reported by @parasharrajat #11671

Closed

fedirjh mentioned this pull request Jan 3, 2023

[Performance] Onyx.clear() takes a long time and can leave stale data in storage #13884

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove credentials and session from Onyx on sign out #7397

Remove credentials and session from Onyx on sign out #7397

luacmartins commented Jan 25, 2022

github-actions bot commented Jan 25, 2022

danieldoglas left a comment

OSBotify commented Jan 25, 2022

kidroca Jan 28, 2022

luacmartins Jan 28, 2022

marcaaron Feb 8, 2022

luacmartins Feb 8, 2022

luacmartins Mar 2, 2022 •

edited

Loading

kidroca Mar 3, 2022 •

edited

Loading

tgolen Mar 3, 2022

kidroca Mar 4, 2022

luacmartins Mar 4, 2022

kidroca Mar 4, 2022

Remove credentials and session from Onyx on sign out #7397

Remove credentials and session from Onyx on sign out #7397

Conversation

luacmartins commented Jan 25, 2022

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Jan 25, 2022

danieldoglas left a comment

Choose a reason for hiding this comment

OSBotify commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

kidroca Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins Mar 2, 2022 •

edited

Loading

kidroca Mar 3, 2022 •

edited

Loading