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.

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

Jump to bottom

Cleanup Pusher subscriptions when signing out in another tab #5555

Merged
merged 9 commits into from
Sep 28, 2021
Merged

Cleanup Pusher subscriptions when signing out in another tab #5555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7bef47b
Move cleanup logic to callback
marcaaron Sep 21, 2021
3a55a94
remove newline
marcaaron Sep 21, 2021
e89394d
add SignoutManager to manage cross tab communication
marcaaron Sep 28, 2021
99890c7
move session cleanup tasks to AuthScreens
marcaaron Sep 28, 2021
db5a0b3
remove broadcast channel. not necessary
marcaaron Sep 28, 2021
27f5d05
fix conflicts
marcaaron Sep 28, 2021
7da6e74
Add BroadcastChannel as we cant use localStorage
marcaaron Sep 28, 2021
f3e3f82
add SignoutManager + remove bcast channel again
marcaaron Sep 28, 2021
ccef3d9
remove return promise
marcaaron Sep 28, 2021
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
3 changes: 3 additions & 0 deletions src/ONYXKEYS.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,7 @@ export default {

// Stores Workspace ID that will be tied to reimbursement account during setup
REIMBURSEMENT_ACCOUNT_WORKSPACE_ID: 'reimbursementAccountWorkspaceID',

// Notifies all tabs that they should sign out and clear storage.
SHOULD_SIGN_OUT: 'shouldSignOut',
};
3 changes: 2 additions & 1 deletion src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ import CardOverlay from '../../../components/CardOverlay';
import defaultScreenOptions from './defaultScreenOptions';
import * as API from '../../API';
import {setLocale} from '../../actions/App';
import {cleanupSession} from '../../actions/Session';
import WorkspaceNew from '../../../pages/workspace/WorkspaceNew';

Onyx.connect({
Expand Down Expand Up @@ -249,7 +250,7 @@ class AuthScreens extends React.Component {
if (this.unsubscribeGroupShortcut) {
this.unsubscribeGroupShortcut();
}
NetworkConnection.stopListeningForReconnect();
cleanupSession();
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: I was wondering why do we do this here instead of in clearStorageAndRedirect. I feels like the clean up logic is spread and it could be more centralized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, mainly because we are setting up the subscriptions in AuthScreens componentDidMount() here:

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

Lines 157 to 159 in ccef3d9

componentDidMount() {
NetworkConnection.listenForReconnect();
PusherConnectionManager.init();

so I thought it made more sense to tear them down in componentWillUnmount().

We definitely could do it in clearStorageAndRedirect() but I think making the "cleanup" stuff a side effect of losing the authToken is OK because we set up a lot of logic in componentDidMount() that could also be triggered by the presence of the authToken.

Basically the assumption is that AuthScreens mounted = we have authToken and AuthScreen unmount = we lost the authToken.

But yeah I wouldn't mind cleaning this up eventually or doing something differently if it becomes complicated enough to where people are thoroughly confused about how it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, mainly because we are setting up the subscriptions in AuthScreens componentDidMount() here:

Ohh that makes sense, I only looked at the code in componentWillUnmount

But yeah I wouldn't mind cleaning this up eventually or doing something differently if it becomes complicated enough to where people are thoroughly confused about how it works.

Agreed, I think it is good for now like it is, there is a good enough reason to keep it there.

clearInterval(this.interval);
this.interval = null;
hasLoadedPolicies = false;
Expand Down
38 changes: 38 additions & 0 deletions src/libs/SignoutManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import Onyx from 'react-native-onyx';
import ONYXKEYS from '../ONYXKEYS';

let signoutCallback = () => {};
let errorMessage = '';
let shouldSignOut = false;
Onyx.connect({
key: ONYXKEYS.SHOULD_SIGN_OUT,
callback: (val) => {
if (!shouldSignOut && val) {
signoutCallback(errorMessage);
errorMessage = '';
Onyx.set(ONYXKEYS.SHOULD_SIGN_OUT, false);
}

shouldSignOut = val;
},
});

/**
* @param {Function} callback
*/
function registerSignoutCallback(callback) {
signoutCallback = callback;
}

/**
* @param {String} message
*/
function signOut(message) {
errorMessage = message;
Onyx.set(ONYXKEYS.SHOULD_SIGN_OUT, true);
}

export default {
signOut,
registerSignoutCallback,
};
19 changes: 18 additions & 1 deletion src/libs/actions/Session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@ import Navigation from '../Navigation/Navigation';
import ROUTES from '../../ROUTES';
import {translateLocal} from '../translate';
import * as Network from '../Network';
import UnreadIndicatorUpdater from '../UnreadIndicatorUpdater';
import Timers from '../Timers';
import * as Pusher from '../Pusher/pusher';
import NetworkConnection from '../NetworkConnection';
import {getUserDetails} from './User';


let credentials = {};
Onyx.connect({
key: ONYXKEYS.CREDENTIALS,
Expand Down Expand Up @@ -339,6 +342,19 @@ function continueSessionFromECom(accountID, validateCode, twoFactorAuthCode) {
});
}

/**
* 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.
*/
function cleanupSession() {
// We got signed out in this tab or another so clean up any subscriptions and timers
NetworkConnection.stopListeningForReconnect();
UnreadIndicatorUpdater.stopListeningForReportChanges();
PushNotification.deregister();
PushNotification.clearNotifications();
Pusher.disconnect();
Timers.clearAll();
}

export {
continueSessionFromECom,
fetchAccountDetails,
Expand All @@ -349,4 +365,5 @@ export {
reopenAccount,
resendValidationLink,
resetPassword,
cleanupSession,
};
41 changes: 16 additions & 25 deletions src/libs/actions/SignInRedirect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import Onyx from 'react-native-onyx';
import SignoutManager from '../SignoutManager';
import ONYXKEYS from '../../ONYXKEYS';
import * as Pusher from '../Pusher/pusher';
import UnreadIndicatorUpdater from '../UnreadIndicatorUpdater';
import PushNotification from '../Notification/PushNotification';
import Timers from '../Timers';

let currentActiveClients;
Onyx.connect({
Expand All @@ -20,18 +17,9 @@ Onyx.connect({
});

/**
* Clears the Onyx store and redirects to the sign in page.
* Normally this method would live in Session.js, but that would cause a circular dependency with Network.js.
*
* @param {String} [errorMessage] error message to be displayed on the sign in page
* @param {String} errorMessage
*/
function redirectToSignIn(errorMessage) {
UnreadIndicatorUpdater.stopListeningForReportChanges();
PushNotification.deregister();
PushNotification.clearNotifications();
Pusher.disconnect();
Timers.clearAll();

function clearStorageAndRedirect(errorMessage) {
const activeClients = currentActiveClients;
const preferredLocale = currentPreferredLocale;

Expand All @@ -45,18 +33,21 @@ function redirectToSignIn(errorMessage) {
Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, activeClients);
}

const session = {
// We must set the authToken to null so that signOut action is triggered across other clients
authToken: null,
};

if (errorMessage) {
session.error = errorMessage;
}

// `Onyx.clear` reinitialize the Onyx instance with initial values so use `Onyx.merge` instead of `Onyx.set`.
Onyx.merge(ONYXKEYS.SESSION, session);
Onyx.merge(ONYXKEYS.SESSION, {error: errorMessage});
});
}

SignoutManager.registerSignoutCallback(clearStorageAndRedirect);

/**
* Clears the Onyx store and redirects to the sign in page.
* Normally this method would live in Session.js, but that would cause a circular dependency with Network.js.
*
* @param {String} [errorMessage] error message to be displayed on the sign in page
*/
function redirectToSignIn(errorMessage) {
SignoutManager.signOut(errorMessage);
}

export default redirectToSignIn;