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

iOS / Android - After sign out of an account redirects user to resend link page #4595

Closed
kavimuru opened this issue Aug 12, 2021 · 13 comments
Closed
Assignees
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2 Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Aug 12, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Launch the app
  2. Sign in with any account
  3. Go to Setting page and click sign out

Expected Result:

After sign out user redirected to Sign in page

Actual Result:

After sign out user redirected to Resend link page
NOTE: Tapping the "resend link" CTA in iOS crashes the app and does nothing in Android.

Workaround:

Unknown

Platform:

  • iOS
  • Android

Version Number:
1.0.85-0 (Android)
1.0.84-5 (iOS)
Logs: https://stackoverflow.com/c/expensify/questions/4856
**Notes/Photos/Videos:
Issue occurs with both gmail and expenifail accounts

Bug5189768_Screen_Recording_20210811-210830_Expensifycash.mp4
GTLJ3693.MP4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

@kavimuru kavimuru added the DeployBlockerCash This issue or pull request should block deployment label Aug 12, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@MelvinBot
Copy link

Triggered auto assignment to @iwiznia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@kavimuru kavimuru changed the title Android - After sign out of an account redirects user to resend link page iOS / Android - After sign out of an account redirects user to resend link page Aug 12, 2021
@iwiznia
Copy link
Contributor

iwiznia commented Aug 12, 2021

@kidroca is investigating. We suspect #4509 caused this and if so we will probably revert.

Context https://expensify.slack.com/archives/C01GTK53T8Q/p1628776131244300

@iwiznia
Copy link
Contributor

iwiznia commented Aug 12, 2021

Revert PR sent

@kidroca
Copy link
Contributor

kidroca commented Aug 12, 2021

I think I've found the real issue

  1. The entire storage is cleared on signOut

    Onyx.clear().then(() => {

  2. We have this object defined as the default account data

    [ONYXKEYS.ACCOUNT]: CONST.DEFAULT_ACCOUNT_DATA,

DEFAULT_ACCOUNT_DATA: {error: '', success: '', loading: false},

  1. When Onyx storage is cleared - it's repopulated with the defaults
    here: https://github.com/Expensify/react-native-onyx/blob/073760394ed8d83304beabf227f2c2fb75b3f04f/lib/Onyx.js#L634
    and here: https://github.com/Expensify/react-native-onyx/blob/073760394ed8d83304beabf227f2c2fb75b3f04f/lib/Onyx.js#L606
  • the function initializeWithDefaultKeyStates merges default values with existing data in storage
  • since storage was cleared account is the exact object shared in step 2) above
  1. At the SignInPage
    const showLoginForm = !this.props.credentials.login;
    const validAccount = this.props.account.accountExists
    && this.props.account.validated
    && !this.props.account.forgotPassword;

For some reason we still have props.credentials.login here
while validAccount is undefined (aka falsy)

And ultimately we have true here

const showResendValidationLinkForm = this.props.credentials.login && !validAccount;


I'm investigating why we have props.credentials.login even after we've cleared storage and cache

@kidroca
Copy link
Contributor

kidroca commented Aug 12, 2021

Ok, this seems to be what's going on:

function redirectToSignIn

Onyx.set(ONYXKEYS.SESSION, {authToken: null})
.then(() => {
Onyx.clear().then(() => {
if (preferredLocale) {
Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);

When we're redirected to sign in we're first setting {authToken: null}
This triggers a change that mounts the PublicScreens and the SiginIn screen
But because we wait for set to complete and only then call Onyx.clear there's a split second where a few connections (withOnyx(SignIn)) are initiated before Onxy.clear runs. One of them is for credentials

  • Onyx.set informs subscribers optimistically before the actual write to storage complets

With the new LRU cache from the PR we've just reverted the credentials information is still in cache
Onyx.connect picks it up just before cache is cleared by Onyx.clear
And even though Onyx.clear does keyChanged(key, null); nearly at the same time
The sendDataToConnection happens last
Since it does a setState on the instance - it overwrites and adds credentials in the component state

With the old connection based cache strategy, we don't have credentials in cache, because there's a brief time where everyone is disconnected from that key. And so the bug does not happen

This looks like to be a problem with Onyx and cache, and might happen with any cache strategy due to how connect works
But it does look like something too rare to worry ATM or have as a priority.
It's seems to be isolated to cases where we're waiting for Onyx.set to complete, and then only if we do something drastic related to storage, like clearing it

@kidroca
Copy link
Contributor

kidroca commented Aug 12, 2021

As for the bug
It's fixed by rewriting this logic:

// We must set the authToken to null so we can navigate to "signin" it's not possible to navigate to the route as
// it only exists when the authToken is null.
Onyx.set(ONYXKEYS.SESSION, {authToken: null})
.then(() => {
Onyx.clear().then(() => {
if (preferredLocale) {
Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);
}
if (errorMessage) {
Onyx.set(ONYXKEYS.SESSION, {error: errorMessage});
}
if (activeClients && activeClients.length > 0) {
Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, activeClients);
}
});
});

Like so:

    // When we clear storage we clear the auth token as well, and are redirected to Login
    Onyx.clear().then(() => {
        if (preferredLocale) {
            Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);
        }
        if (errorMessage) {
            Onyx.set(ONYXKEYS.SESSION, {error: errorMessage});
        }
        if (activeClients && activeClients.length > 0) {
            Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, activeClients);
        }
    });

Or if you worry that something else might preserve the authToken if not explicitly removed

 Onyx.clear()
         .then(() => Onyx.set(ONYXKEYS.SESSION, {authToken: null}))
         .then(() => ...);

@iwiznia
The bug mentioned on slack, where you briefly see the password input, does not happen with the above proposal

Android.Emulator.-.Pixel_2_API_29_5554.2021-08-12.21-07-17.mp4

It looks like it was caused by the same but the calls happened in different order

@iwiznia
Copy link
Contributor

iwiznia commented Aug 12, 2021

Nice, thanks for the investigation and fix!

@iwiznia
Copy link
Contributor

iwiznia commented Aug 12, 2021

hmmm curious, why did the bug only happen in android?

@francoisl francoisl added the Reviewing Has a PR in review label Aug 12, 2021
@kidroca
Copy link
Contributor

kidroca commented Aug 12, 2021

I thought it happened on both iOS and Android ?
The cause is storage timing, there would be no problem if:

  • the Onyx.set happens very fast (or is not called at all)
  • the connections that follow happen slow (from disk instead of cache)
    • when there's no data in cache (for the bug triggering connection) (LRU cache expires as well) data will be read from disk

@isagoico
Copy link

Retest on Android was a pass 🎉 Closing the issue.

@Beamanator
Copy link
Contributor

Beamanator commented Sep 10, 2021

@kidroca Is it possible your PR caused this issue? #4971 See investigation by @parasharrajat here: #4971 (comment)

@kidroca
Copy link
Contributor

kidroca commented Sep 10, 2021

Yes,
Looks like I shouldn't have removed the setting of Auth Token to null
Though the comment mislead me: https://github.com/Expensify/App/pull/4614/files#diff-d27dcfad2dc98cae7cb0f5b5103c607af01dd3ea7a5a234673f5259c5bd158d1L48

I think the proposed solution is ok, and we just need something to trigger the storage event by in other tabs setting a key

AFAIK Onyx.clear should trigger the storage event, not sure why it doesn't...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

8 participants