Skip to content

Remove credentials and session from Onyx on sign out #7397

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.

Sign up for GitHub

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

Merged
danieldoglas merged 1 commit into from
Jan 25, 2022
Merged

Remove credentials and session from Onyx on sign out #7397

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/libs/actions/Session/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ function signOut() {
})
.catch(error => Onyx.merge(ONYXKEYS.SESSION, {error: error.message}));
}
Onyx.set(ONYXKEYS.SESSION, null);
Onyx.set(ONYXKEYS.CREDENTIALS, null);
Timing.clearData();
redirectToSignIn();
Comment on lines +86 to 89

@kidroca kidroca Jan 28, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@luacmartins luacmartins Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@marcaaron marcaaron Feb 8, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@luacmartins luacmartins Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@luacmartins luacmartins Mar 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Screen Shot 2022-03-02 at 12 03 26 PM

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

@kidroca kidroca Mar 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@tgolen tgolen Mar 3, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. Click sign out
  2. Go to /signout which is just a special stand-alone page (everything else in the app would be unmounted at this point)
  3. Clear out Onyx
  4. 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.

@kidroca kidroca Mar 4, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@luacmartins luacmartins Mar 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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
timing_with_clear

Without changes
timing_no_clear

@kidroca kidroca Mar 4, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Log.info('Redirecting to Sign In because signOut() was called');
Expand Down