Support validated links coming from e.com by iwiznia · Pull Request #5387 · Expensify/App

iwiznia · 2021-09-21T13:53:12Z

Please review!

CC: @cead22, @tgolen, @marcaaron, @Luke9389

Details

Fixed Issues

https://github.com/Expensify/Expensify/issues/177739

Tests

See https://github.com/Expensify/Web-Expensify/pull/31977

QA Steps

Log into an account on NewDot Expensify, create a Workspace if you don't already have one using the + on the bottom right.
Log out of NewDot and then log into that same account on OldDot and go to Settings -> Policy and find your Workspace there. It should have a card icon like these:
Click on its name and verify it opens a new tab, automatically logs you in, and brings you to the workspace settings page:

Tested On

Screenshots

Included above

TomatoToaster · 2021-09-21T19:33:41Z

PR for adding workspace/new is ready here: https://github.com/Expensify/App/pull/5396/files. This could be its own PR and I could add testing steps for it.

Or I could merge it with this one which might help test that this PR works. I'm also trying to make sure urls where we do exitTo=workspace/<workspaceID>/card also works.

I'm going to work on the Web-Expensify part right now.

cead22 · 2021-09-21T23:08:50Z

            [SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE]: ROUTES.LOGIN_WITH_VALIDATE_CODE_2FA_NEW_WORKSPACE,
            [SCREENS.LOGIN_WITH_VALIDATE_CODE_WORKSPACE_CARD]: ROUTES.LOGIN_WITH_VALIDATE_CODE_WORKSPACE_CARD,
            [SCREENS.LOGIN_WITH_VALIDATE_CODE_2FA_WORKSPACE_CARD]: ROUTES.LOGIN_WITH_VALIDATE_CODE_2FA_WORKSPACE_CARD,
+            LogInWithShortLivedToken: ROUTES.LOGIN_WITH_SHORTLIVED_AUTHTOKEN,


Should we put LogInWithShortLivedToken in a const under SCREEN?

cead22 · 2021-09-21T23:12:12Z

+function signInWithShortLivedToken(accountID, email, shortLivedToken, encryptedAuthToken, exitTo) {
+    Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, loading: true});
+
+    createTemporaryLogin(shortLivedToken, encryptedAuthToken).then((response) => {


Is createTemporaryLogin missing the email param? Maybe it's not really needed in API.CreateLogin, but NAB we don't have to fix that in this PR

…sn't being set

marcaaron

Leaving a few initial comments.

marcaaron · 2021-09-21T23:49:43Z

        partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD,
        partnerUserID: credentials.autoGeneratedLogin,
        partnerUserSecret: credentials.autoGeneratedPassword,
+        authToken,


I am not really getting why the authToken is included when calling reauthenticate()? Maybe we could use a comment here or something. My previous understanding of this method is that we will use the credentials to get a new authToken not call it with the one already in storage or the last one we got when we called reauthenticate().

marcaaron · 2021-09-21T23:53:20Z

+function signInWithShortLivedToken(accountID, email, shortLivedToken) {
+    Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, loading: true});
+
+    createTemporaryLogin(shortLivedToken).then((response) => {


Is the fact that we are not passing an email to createTemporaryLogin going to create any sort of problem?

marcaaron · 2021-09-21T23:54:04Z

 * @param {String} email
+ * @return {Promise}
 */
 function createTemporaryLogin(authToken, encryptedAuthToken, email) {


If we're making encryptedAuthToken and email optional we should update the docs

marcaaron · 2021-09-22T00:00:33Z

+        const exitTo = decodeURIComponent(lodashGet(this.props.route.params, 'exitTo', ''));
+
+        // If the user is revisiting the component authenticated or they were already logged into the right account, we simply redirect them to the exitTo
+        if (this.props.session.authToken && email === this.props.session.email) {


NAB, it's good that we are checking if the email is the same. But it doesn't look like we are handling the case where it's not. We might not need to worry about this just yet, but feels like bad things would happen if you tried to sign in one user while still logged in as another.

marcaaron · 2021-09-22T00:06:59Z

+            email,
+        });
+        if (response.jsonCode === 200) {
+            getUserDetails();


Oh hmm why are we doing this? It should happen when we set the authToken no?

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

Lines 185 to 186 in 1901df3

User.getUserDetails();

User.getBetas();

cead22 · 2021-09-22T00:07:24Z

        });
 }

+function signInWithShortLivedToken(accountID, email, shortLivedToken) {


Don't we need to call authenticate with the short lived token before creating a login?

We're calling signInWithShortLivedToken(shortLivedToken) and not passing encrypted auth token, that means it'll be set to undefined in setSuccessfulSignInData right?

cead22 · 2021-09-22T00:08:01Z

+import Navigation from '../libs/Navigation/Navigation';
+
+const propTypes = {
+    /** The accountID and validateCode are passed via the URL */


Is this right or outdated?

Julesssss · 2021-09-22T11:09:34Z

    HOME: 'Home',
    LOADING: 'Loading',
    REPORT: 'Report',
+    LOG_IN_WITH_SHORT_LIVED_TOKEN: 'LogInWithShortLivedToken',


Can we align SHORTLIVED and SHORT_LIVED to help make this searchable?

Oh true. Yes, I will once I figure out which PR we should be using.

iwiznia · 2021-09-22T14:48:00Z

This PR was replaced by #5396

Support validated links coming from e.com

2b70ccf

iwiznia self-assigned this Sep 21, 2021

iwiznia added 2 commits September 21, 2021 19:06

Pass authToken and do not fail on non exiting credentials

d942fe6

Pass authToken in authenticate

3e45432

TomatoToaster self-assigned this Sep 21, 2021

Update linking from OldDot to incorporate route correctly

18afdf0

cead22 reviewed Sep 21, 2021

View reviewed changes

Use AuthScreen rerouting method and remove encryptedAuthToken which i…

85f20dc

…sn't being set

cead22 requested a review from marcaaron September 21, 2021 23:50

Amal Nazeem added 2 commits September 21, 2021 19:53

Use Screens Const

8ea5a2e

Incorporate Navigation.dismissModal to fix logic when closing a modal

579a6cb

TomatoToaster marked this pull request as ready for review September 22, 2021 00:01

TomatoToaster requested a review from a team as a code owner September 22, 2021 00:01

MelvinBot requested review from Julesssss and removed request for a team September 22, 2021 00:01

marcaaron reviewed Sep 22, 2021

View reviewed changes

cead22 reviewed Sep 22, 2021

View reviewed changes

TomatoToaster mentioned this pull request Sep 22, 2021

Reintroduce workspace new without a modal, just the url #5396

Merged

6 tasks

Add/fix doc for functions

31436b1

Julesssss reviewed Sep 22, 2021

View reviewed changes

reintroduce encrypted authToken

269697b

iwiznia closed this Sep 22, 2021

roryabraham deleted the ionatan_link_fromolddot branch June 8, 2023 01:25

Conversation

iwiznia commented Sep 21, 2021 • edited by TomatoToaster Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Uh oh!

TomatoToaster commented Sep 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwiznia commented Sep 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iwiznia commented Sep 21, 2021 •

edited by TomatoToaster

Loading