[PAID, WAITING ON SITU FOR CHECKLIST] [$500] Account - Account is not simultaneously signed out on secondary device when closing account

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.42-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Sign in to main device and secondary device with the same account
2. Close account on main device
3. Navigate to secondary device

Expected Result:

• Account should be signed out on the secondary device simultaneously
• There should be 'Account closed successfully' message under email input field
• There shouldn't be console log error

Actual Result:

• Account is not signed out before user interacts with the account
• There is no 'Account closed successfully' message under email input field
• There is console log error

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6381147_1708026494674.Screen_Recording_2024-02-14_at_2.10.57_at_night.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011fa63892ed79f932
• Upwork Job ID: 1758227147283243008
• Last Price Increase: 2024-02-22
• Automatic offers:
• situchan | Reviewer | 0
• wildan-m | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011fa63892ed79f932

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan (External)

[melvin-bot]

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[brandonhenry]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When an account is closed on one device, the account does not simultaneously sign out on a secondary device. Additionally, there is no "Account closed successfully" message displayed under the email input field on the secondary device, and there is an error logged in the console.

What is the root cause of that problem?

The root cause appears to be a lack of real-time session management across multiple devices. The current system does not seem to notify or force logout on all active sessions once an account is closed. This might be due to the session tokens not being invalidated across the system or the secondary device not polling for session validity in real-time.

What changes do you think we should make in order to solve the problem?

• Implement a push notification system that informs all active sessions when an account is closed, prompting immediate sign out.
• Invalidate the session tokens at the server level once an account closure action is initiated, preventing further API calls from a closed account.
• Ensure that the secondary device listens for account status changes and responds accordingly by signing the user out and displaying the appropriate account closure message.
• Address the console log error by ensuring proper error handling and messaging across the application.

What alternative solutions did you explore? (Optional)

An alternative approach could involve periodically checking the validity of the session token against the server, but this could introduce unnecessary network requests and latency. Maybe do so but with a max retry count.

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[wildan-m]

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

1. There is console log error

When an account is closed, it will return this Onyx update:

    {
        "onyxMethod": "clear"
    },

It lacks a key property.

This code call includes no null check.

App/src/libs/actions/User.ts

Line 491 in 20af9e5

const reportActionsOnly = pushJSON.filter((update) => update.key.includes('reportActions_'));

2. Account is not signed out before user interacts with the account

This process is blocked by the first issue

3. There is no 'Account closed successfully' message under email input field

This behavior is blocked by the first issue

What changes do you think we should make in order to solve the problem?

Add a null check to the key property.
Change:

App/src/libs/actions/User.ts

Line 491 in 20af9e5

const reportActionsOnly = pushJSON.filter((update) => update.key.includes('reportActions_'));

to

    const reportActionsOnly = pushJSON.filter((update) => update.key?.includes('reportActions_'));

Why would adding a null check automatically resolve the second and third issues?

This is the updated onyx information from the server on the secondary device:

[
    {
        "onyxMethod": "clear"
    },
    {
        "key": "preferredLocale",
        "onyxMethod": "merge",
        "value": "en"
    },
    {
        "key": "closeAccount",
        "onyxMethod": "merge",
        "value": {
            "success": "Account closed successfully"
        }
    },
    {
        "key": "isCheckingPublicRoom",
        "onyxMethod": "set",
        "value": false
    }

"onyxMethod": "clear" will call Onyx.clear() and remove all onyx data, including session.authToken.

Clearing the authToken will trigger a re-render on the isAuthenticated props.

App/src/Expensify.js

Line 124 in 3e77445

const isAuthenticated = useMemo(() => Boolean(lodashGet(props.session, 'authToken', null)), [props.session]);

The isAuthenticated === false condition will redirect the user to the default public screens (Sign in page).

App/src/libs/Navigation/AppNavigator/index.tsx

Lines 24 to 30 in 3e77445

	if (authenticated) {
	const AuthScreens = require('./AuthScreens').default;
	// These are the protected screens and only accessible when an authToken is present
	return <AuthScreens />;
	}
	const PublicScreens = require('./PublicScreens').default;

In addition to redirecting, it will also trigger necessary cleaning tasks for sign-out.

App/src/libs/Navigation/AppNavigator/AuthScreens.tsx

Lines 253 to 258 in 3e77445

	return () => {
	unsubscribeShortcutsOverviewShortcut();
	unsubscribeSearchShortcut();
	unsubscribeChatShortcut();
	Session.cleanupSession();
	};

Why should we not call the signOut function on a secondary device?

It will cause an error as the logout API is cancelled, possibly due to a user's removed object that no longer exists.

Is there any difference between signing out from Onyx.clear and Session.signOut function?

Yes, cleanupSession is missing two cleanup tasks in comparison to Session.signOut. To resolve this, we can transfer these cleaning tasks from Session.signOut to cleanupSession.

Move:

App/src/libs/actions/Session/index.ts

Lines 100 to 103 in 3e77445

	clearCache().then(() => {
	Log.info('Cleared all cache data', true, {}, true);
	});
	Timing.clearData();

To

/**
 * Put any logic that needs to run when we are signed out here. This can be triggered when the current tab or another tab signs out.
 * - Cancels pending network calls - any lingering requests are discarded to prevent unwanted storage writes
 * - Clears all current params of the Home route - the login page URL should not contain any parameter
 */
function cleanupSession() {
    Pusher.disconnect();
    Timers.clearAll();
    Welcome.resetReadyCheck();
    PriorityMode.resetHasReadRequiredDataFromStorage();
    MainQueue.clear();
    HttpUtils.cancelPendingRequests();
    PersistedRequests.clear();
    NetworkConnection.clearReconnectionCallbacks();
    SessionUtils.resetDidUserLogInDuringSession();
    resetHomeRouteParams();
    clearCache().then(() => {
        Log.info('Cleared all cache data', true, {}, true);
    });
    Timing.clearData();
}

This way, cleanup tasks will be consistent for both methods.

Branch to test.

What alternative solutions did you explore? (Optional)

Alternative 1 -- add checkCloseAccount check

function playSoundForMessageType(pushJSON: OnyxServerUpdate[]) {
    if(checkCloseAccount(pushJSON)){
       return;
   }
.....

function checkCloseAccount(jsonData) {
    return _.some(jsonData, item => item.key === "closeAccount");
}

Alternative 2 -- Provide onyx update return with some key. e.g.

    {
        "key": "clear"
        "onyxMethod": "clear"
    },

Result

GMT20240219-125319_Clip_Wildan.M.s.Clip.02_19_2024.mp4

[wildan-m]

Proposal Updated

Add alternative solutions

[suneox]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Account - Account is not simultaneously signed out on secondary device when closing account

What is the root cause of that problem?

When the second device receives the event closeAccount from socket at this line we don't handle logout.

What changes do you think we should make in order to solve the problem?

We will check if the event include CLOSE_ACCOUNT_FORM and process to sign out

    PusherUtils.subscribeToMultiEvent(Pusher.TYPE.MULTIPLE_EVENT_TYPE.ONYX_API_UPDATE, (pushJSON: OnyxServerUpdate[]) => {
+       const onyxCloseAccount = pushJSON.find((update) => update.key === ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM);
+       if (onyxCloseAccount) {
+           Session.signOutAndRedirectToSignIn();
+           return Onyx.update([onyxCloseAccount]);
+       }
        playSoundForMessageType(pushJSON);

POC

• Account should be signed out on the secondary device simultaneously
• There should be 'Account closed successfully' message under email input field
• There shouldn't be console log error

Screen.Recording.2024-02-17.at.21.46.00.mov

What alternative solutions did you explore? (Optional)

[situchan]

Thanks for the proposals everyone.
@brandonhenry the root cause is not correct
@wildan-m you considered only 1 issue in RCA while 3 issues total
@suneox your proposal is almost close but I'm not sure it's the right place to trigger signout manually.

[wildan-m]

@situchan my proposal updated with result video. I feel the result cover all of three cases.

[situchan]

@wildan-m the root cause is still not updated

[wildan-m]

@situchan My Proposal updated.

It provides further clarification on the underlying issue, enhanced resolution, and the rationale behind avoiding calling signOut on the secondary device.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[jliexpensify]

Waiting on @situchan to re-review

[melvin-bot]

@jliexpensify, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[situchan]

reviewing updated proposal today

[situchan]

@wildan-m's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

[wildan-m]

@situchan @chiragsalian the PR is ready.

[chiragsalian]

When we close high traffic account, do we need to request HT account activation again when we log back in?

I'm not 100% sure but I'm pretty sure you would have to request it again once you reopen the account. But why are you testing with the HT account. Won't it be easier to test this issue with non HT accounts.

[situchan]

Maybe they wanna check this off

• I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).

I think it's fine as this bug is not related to HT account

[wildan-m]

@chiragsalian thank you for the confirmation. I have used my HT account for testing, could you assist me in re-assigning it to the HT account?

[email protected]

[wildan-m]

@chiragsalian, please ignore my earlier request. This form is automated to assign tasks promptly.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.48-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Correct simultaneous logout when the account is closed. #37481

If no regressions arise, payment will be issued on 2024-03-14. 🎊

For reference, here are some details about the assignees on this issue:

• @wildan-m requires payment automatic offer (Contributor)
• @situchan requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@situchan] The PR that introduced the bug has been identified. Link to the PR:
• [@situchan] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@situchan] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@situchan] Determine if we should create a regression test for this bug.
• [@situchan] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[wildan-m]

@jliexpensify @chiragsalian @situchan seems no regression here?

[jliexpensify]

Hi @wildan-m correct, I'll be paying now (no need to tag us!)

Bump @situchan to complete the checklist.

[jliexpensify]

Payment Summary

• C: @wildan-m $500
• C+: @situchan $500

Upwork job

[jliexpensify]

Paid and job closed, waiting on checklist form Situ.

[situchan]

Offending PR with comment: https://github.com/Expensify/App/pull/31055/files#r1525621084
As this was caught by QA team, we can skip regression test.