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

Pass user session information when opening the electron app from web platform #16043

Merged
merged 14 commits into from
Apr 3, 2023
Merged
3 changes: 3 additions & 0 deletions src/ONYXKEYS.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,4 +193,7 @@ export default {

// Is app in beta version
IS_BETA: 'isBeta',

// Whether the auth token is valid
IS_TOKEN_VALID: 'isTokenValid',
};
67 changes: 51 additions & 16 deletions src/components/DeeplinkWrapper/index.website.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,31 @@ import CONST from '../../CONST';
import CONFIG from '../../CONFIG';
import * as Browser from '../../libs/Browser';
import ONYXKEYS from '../../ONYXKEYS';
import * as Authentication from '../../libs/Authentication';

const propTypes = {
/** Children to render. */
children: PropTypes.node.isRequired,

/** Beta features list */
betas: PropTypes.arrayOf(PropTypes.string),

/** Session info for the currently logged-in user. */
session: PropTypes.shape({

/** Currently logged-in user email */
email: PropTypes.string,

/** Currently logged-in user authToken */
authToken: PropTypes.string,
}),
};
const defaultProps = {
betas: [],
session: {
email: '',
authToken: '',
},
};

class DeeplinkWrapper extends PureComponent {
Expand All @@ -29,27 +44,18 @@ class DeeplinkWrapper extends PureComponent {
appInstallationCheckStatus: (this.isMacOSWeb() && CONFIG.ENVIRONMENT !== CONST.ENVIRONMENT.DEV)
? CONST.DESKTOP_DEEPLINK_APP_STATE.CHECKING : CONST.DESKTOP_DEEPLINK_APP_STATE.NOT_INSTALLED,
};
this.focused = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a code comment to explain what this is used for, or update the name of the property to this.isBrowserWindowFocused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


It comes from the original focus variable. I think we can safely remove it now, after the redirection screen is removed, I think we don't need it anymore.

}

componentDidMount() {
if (!this.isMacOSWeb() || CONFIG.ENVIRONMENT === CONST.ENVIRONMENT.DEV) {
return;
}

let focused = true;

window.addEventListener('blur', () => {
focused = false;
this.focused = false;
});

setTimeout(() => {
if (!focused) {
this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.INSTALLED});
} else {
this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.NOT_INSTALLED});
}
}, 500);

// check if pathname matches with deeplink routes
const matchedRoute = _.find(deeplinkRoutes, (route) => {
if (route.isDisabled && route.isDisabled(this.props.betas)) {
Expand All @@ -59,14 +65,42 @@ class DeeplinkWrapper extends PureComponent {
return routeRegex.test(window.location.pathname);
});

if (matchedRoute) {
this.openRouteInDesktopApp();
if (!matchedRoute) {
this.updateAppInstallationCheckStatus();
return;
}
const expensifyUrl = new URL(CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL);
const params = new URLSearchParams();
params.set('exitTo', `${window.location.pathname}${window.location.search}${window.location.hash}`);
if (!this.props.session.authToken) {
const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}/transition?${params.toString()}`;
this.openRouteInDesktopApp(expensifyDeeplinkUrl);
return;
}
Authentication.getShortLivedAuthToken().then((shortLivedAuthToken) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Promise chains should be EXPLICITLY avoided in code. We did a ton of work to remove all the API chaining that we should not see these reintroduced back into the code (and if Authentication would have been an action file as opposed to a lib file, ESLint would have thrown an error).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out. It has been removed.

params.set('email', this.props.session.email);
params.set('shortLivedAuthToken', `${shortLivedAuthToken}`);
const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}/transition?${params.toString()}`;
this.openRouteInDesktopApp(expensifyDeeplinkUrl);
}).catch(() => {
// If the request is successful, we call the updateAppInstallationCheckStatus before the prompt pops up.
// If not, we only need to make sure that the state will be updated.
this.updateAppInstallationCheckStatus();
});
}

openRouteInDesktopApp() {
const expensifyUrl = new URL(CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL);
const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}${window.location.pathname}${window.location.search}${window.location.hash}`;
updateAppInstallationCheckStatus() {
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there a setTimeout() here? I see this is moving some of the original code, but we should NEVER EVER have a setTimeout() in the code without a thorough code comment explaining it's necessity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I guess it was originally intended to check the focus with a delay. But it's not needed now since the redirection screen has been removed.

if (!this.focused) {
this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.INSTALLED});
} else {
this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.NOT_INSTALLED});
Comment on lines +95 to +97
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears to me that these values aren't used anywhere? The only thing I see looking at this value is in the render method and it's only looking for the value of CHECKING.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's not needed now since the redirection screen has been removed, and we can replace appInstallationCheckStatus with isShortLivedAuthTokenLoading, the latter is clearer.

}
}, 500);
}

openRouteInDesktopApp(expensifyDeeplinkUrl) {
this.updateAppInstallationCheckStatus();

// This check is necessary for Safari, otherwise, if the user
// does NOT have the Expensify desktop app installed, it's gonna
Expand Down Expand Up @@ -115,4 +149,5 @@ DeeplinkWrapper.propTypes = propTypes;
DeeplinkWrapper.defaultProps = defaultProps;
export default withOnyx({
betas: {key: ONYXKEYS.BETAS},
session: {key: ONYXKEYS.SESSION},
})(DeeplinkWrapper);
5 changes: 5 additions & 0 deletions src/languages/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@ export default {
updateApp: 'Update app',
updatePrompt: 'A new version of this app is available.\nUpdate now or restart the app at a later time to download the latest changes.',
},
deeplinkWrapper: {
launching: 'Launching Expensify',
expired: 'Your session has expired.',
signIn: 'Please sign in again.',
},
validateCodeModal: {
successfulSignInTitle: 'Abracadabra,\nyou are signed in!',
successfulSignInDescription: 'Head back to your original tab to continue.',
Expand Down
5 changes: 5 additions & 0 deletions src/languages/es.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,11 @@ export default {
updateApp: 'Actualizar app',
updatePrompt: 'Existe una nueva versión de esta aplicación.\nActualiza ahora or reinicia la aplicación más tarde para recibir la última versión.',
},
deeplinkWrapper: {
launching: 'Cargando Expensify',
expired: 'Tu sesión ha expirado.',
signIn: 'Por favor, inicia sesión de nuevo.',
},
validateCodeModal: {
successfulSignInTitle: 'Abracadabra,\n¡sesión iniciada!',
successfulSignInDescription: 'Vuelve a la pestaña original para continuar.',
Expand Down
10 changes: 10 additions & 0 deletions src/libs/Authentication.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,17 @@ function reauthenticate(command = '') {
});
}

function getShortLivedAuthToken() {
return Network.post('OpenOldDotLink', {shouldRetry: false}).then((response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is Network.post() used here? This is considered a deprecated method (and sorry if it was never marked as such, but we've been trying to remove this for the past year).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also used to think that using the onyx to retrieve short live token would be a little weird, but based on your discussions, this should no longer be an issue. And if we can create the BeginDeepLinkRedirect new command, then we can also no longer call makeRequestWithSideEffects.

if (response && response.shortLivedAuthToken) {
return Promise.resolve(response.shortLivedAuthToken);
}
return Promise.reject();
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this failure mean? It looks like it sets a state property in the deeplink component that either indicates that the desktop app is or isn't installed (yet, the state property is never checked for that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's no longer needed either. We can remove it after we replace the appInstallationCheckStatus with isShortLivedAuthTokenLoading.

});
}

export {
reauthenticate,
Authenticate,
getShortLivedAuthToken,
};
29 changes: 26 additions & 3 deletions src/libs/actions/Session/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import * as API from '../../API';
import * as NetworkStore from '../../Network/NetworkStore';
import * as Report from '../Report';
import DateUtils from '../../DateUtils';
import Navigation from '../../Navigation/Navigation';

let credentials = {};
Onyx.connect({
Expand Down Expand Up @@ -239,6 +240,11 @@ function signInWithShortLivedAuthToken(email, authToken) {
isLoading: true,
},
},
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.IS_TOKEN_VALID,
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it's only used for showing a loading indicator in LogInWithShortLivedAuthTokenPage and has nothing to do with a token being valid or invalid. Please change the name to IS_TOKEN_LOADING. However, this is already ONYXKEYS.ACCOUNT.isLoading so why is there a need for both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw ONYXKEYS.ACCOUNT.isLoading being used in the login form, not sure if it can be used in other scenarios.
And replace IS_TOKEN_VALID with ONYXKEYS.ACCOUNT.isLoading in the new followup PR.

value: true,
},
];

const successData = [
Expand All @@ -259,12 +265,29 @@ function signInWithShortLivedAuthToken(email, authToken) {
isLoading: false,
},
},
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.IS_TOKEN_VALID,
value: false,
},
];

// If the user is transitioning to newDot from a different account on oldDot the credentials may be tied to
// the old account. If so, should not pass the auto-generated login as it is not tied to the account being signed in
// If the user is signing in with a different account from the current app, should not pass the auto-generated login as it may be tied to the old account.
// scene 1: the user is transitioning to newDot from a different account on oldDot.
// scene 2: the user is transitioning to desktop app from a different account on web app.
const oldPartnerUserID = credentials.login === email ? credentials.autoGeneratedLogin : '';
API.write('SignInWithShortLivedAuthToken', {authToken, oldPartnerUserID}, {optimisticData, successData, failureData});
// eslint-disable-next-line rulesdir/no-api-side-effects-method
API.makeRequestWithSideEffects(
'SignInWithShortLivedAuthToken',
{authToken, oldPartnerUserID},
{optimisticData, successData, failureData},
null,
).then((response) => {
if (response) {
return;
}
Navigation.navigate();
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this might have something to do with one of the above scenarios, but it is not clear what. Can you please add a code comment to explain why this is here and why there is no way that this can be done optimistically before the response comes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, if a request fails due to a sudden disconnection, we need to redirect the user to the login page instead of leaving them stuck on the transition page. I tried using network.isOffline to handle this situation in the new PR and left a todo comment for review.

});
}

/**
Expand Down
78 changes: 75 additions & 3 deletions src/pages/LogInWithShortLivedAuthTokenPage.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,21 @@
import React, {Component} from 'react';
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import {View} from 'react-native';
import Text from '../components/Text';
import * as Session from '../libs/actions/Session';
import FullScreenLoadingIndicator from '../components/FullscreenLoadingIndicator';
import Navigation from '../libs/Navigation/Navigation';
import styles from '../styles/styles';
import colors from '../styles/colors';
import Icon from '../components/Icon';
import * as Expensicons from '../components/Icon/Expensicons';
import * as Illustrations from '../components/Icon/Illustrations';
import withLocalize, {withLocalizePropTypes} from '../components/withLocalize';
import compose from '../libs/compose';
import TextLink from '../components/TextLink';
import ONYXKEYS from '../ONYXKEYS';

const propTypes = {
/** The parameters needed to authenticate with a short lived token are in the URL */
Expand All @@ -19,6 +32,15 @@ const propTypes = {
email: PropTypes.string,
}),
}).isRequired,

...withLocalizePropTypes,

/** Whether the short lived auth token is valid */
isTokenValid: PropTypes.bool,
};

const defaultProps = {
isTokenValid: true,
};

class LogInWithShortLivedAuthTokenPage extends Component {
Expand All @@ -27,14 +49,64 @@ class LogInWithShortLivedAuthTokenPage extends Component {

// We have to check for both shortLivedAuthToken and shortLivedToken, as the old mobile app uses shortLivedToken, and is not being actively updated.
const shortLivedAuthToken = lodashGet(this.props, 'route.params.shortLivedAuthToken', '') || lodashGet(this.props, 'route.params.shortLivedToken', '');
Session.signInWithShortLivedAuthToken(email, shortLivedAuthToken);
if (shortLivedAuthToken) {
Session.signInWithShortLivedAuthToken(email, shortLivedAuthToken);
return;
}
const exitTo = lodashGet(this.props, 'route.params.exitTo', '');
if (exitTo) {
Navigation.isNavigationReady().then(() => {
Navigation.navigate(exitTo);
});
}
}

render() {
return <FullScreenLoadingIndicator />;
if (this.props.isTokenValid) {
return <FullScreenLoadingIndicator />;
}
return (
<View style={styles.deeplinkWrapperContainer}>
<View style={styles.deeplinkWrapperMessage}>
<View style={styles.mb2}>
<Icon
width={200}
height={164}
src={Illustrations.RocketBlue}
/>
</View>
<Text style={[styles.textHeadline, styles.textXXLarge]}>
{this.props.translate('deeplinkWrapper.launching')}
</Text>
<View style={styles.mt2}>
<Text style={[styles.fontSizeNormal, styles.textAlignCenter]}>
{this.props.translate('deeplinkWrapper.expired')}
{' '}
<TextLink onPress={() => Navigation.navigate()}>
{this.props.translate('deeplinkWrapper.signIn')}
</TextLink>
</Text>
</View>
</View>
<View style={styles.deeplinkWrapperFooter}>
<Icon
width={154}
height={34}
fill={colors.green}
src={Expensicons.ExpensifyWordmark}
/>
</View>
</View>
);
}
}

LogInWithShortLivedAuthTokenPage.propTypes = propTypes;
LogInWithShortLivedAuthTokenPage.defaultProps = defaultProps;

export default LogInWithShortLivedAuthTokenPage;
export default compose(
withLocalize,
withOnyx({
isTokenValid: {key: ONYXKEYS.IS_TOKEN_VALID},
}),
)(LogInWithShortLivedAuthTokenPage);