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

ntdiary · 2023-03-16T15:12:08Z

Details

Fixed Issues

$ #15651
PROPOSAL: #15651 (comment)

Tests

Login with account A in the desktop app.
Login with account B in the web app.
Navigate to the user profile page in the web app.
Refresh the browser.
A prompt should appear asking to 'Open the app in Electron'.
Click on it and wait for the desktop app to open.
Verify the user profile page for account B is displayed in the desktop app.

Verify that no errors appear in the JS console

Offline tests

Login with account A in the desktop app.
Login with account B in the web app.
Navigate to the user profile page in the web app.
Refresh the browser.
A prompt should appear asking to 'Open the app in Electron'.
Turn off the network.
Click on 'Open the app in Electron' and wait for the desktop app to open.
Verify the login page is displayed in the desktop app.

QA Steps

Login with account A in the desktop app.
Login with account B in the web app.
Navigate to the user profile page in the web app.
Refresh the browser.
A prompt should appear asking to 'Open the app in Electron'.
Click on it and wait for the desktop app to open.
Verify the user profile page for account B is displayed in the desktop app.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

chrome.mp4

Mobile Web - Chrome

N/A

Mobile Web - Safari

N/A

Desktop

N/A

iOS

N/A

Android

N/A

…platform.

ntdiary · 2023-03-16T15:40:32Z

src/components/DeeplinkWrapper/index.website.js

+            return;
+        }
+        const exitTo = `${window.location.pathname}${window.location.search}${window.location.hash}`;
+        Authentication.getNewAuthToken().then((response) => {


We should just get the session in the Onyx or use Authenticate api to get a new session like currently?
If the request fails, the electron app will not be opened.

ntdiary · 2023-03-16T15:45:33Z

src/components/DeeplinkWrapper/index.website.js

        }
+        this.setState({deeplinkMatch: true});
+        const expensifyUrl = new URL(CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL);
+        if (!this.props.authenticated) {


Do we have some public links to open the desktop app?
And if so, do we need to clear the session information in the electron app when opening desktop app via these links?

@ntdiary I think if you direct user to /transition, LogOutPreviousUserPage is used. I haven't looked into this component closely, but I believe the session info for the previous user will be cleared, too.

@ntdiary We have a custom URL scheme. new-expensify://.

I think somewhere in the code, if the app is running on a desktop, the app is registering that URL schema with Mac OS, telling the OS that if OS detects user opening any URL that starts with new-expensify://, please let the desktop app handle what to do with the URL.

yeh, now when we click 'Open the app in Electron', it will always go to the /transition page first. 😄

ntdiary · 2023-03-16T15:48:28Z

Hi, @thesahindia, @hayata-suenaga , there may be a few points we need to confirm, I added the comments above first. 🙂

hayata-suenaga · 2023-03-16T16:58:53Z

@ntdiary I'm looking at your questions right now. I'll get back to you after I get insights into these.

hayata-suenaga · 2023-03-16T17:51:51Z

@ntdiary FYI the deep link is not working on production and staging right now. local NewDot webpage -> local desktop app seems to be working correctly.

ntdiary · 2023-03-17T15:36:41Z

I replaced autoToken with shortLivedAuthToken.

we need to get the credentials from backend, otherwise the authToken can't be refreshed after it expires.

In my tests, the shortLivedAuthToken is valid for 1 minute. This means that if we click the button after popup has been displayed for 1 minute, the token is invalid. For this case, I temporarily let the app jump to the home page instead of getting stuck on the /transition page.

cc @thesahindia, @hayata-suenaga

MelvinBot · 2023-03-17T16:56:14Z

@hayata-suenaga @thesahindia One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

ntdiary · 2023-03-20T08:35:44Z

@thesahindia @hayata-suenaga hi, please feel free to let me know if anything is not expected or needs to be improved. 🙂

hayata-suenaga · 2023-03-20T15:09:22Z

I'm looking at your comment now I have to be offline now and I'm OOO today, but if I can find a time tonight, I'll get back to you.

hayata-suenaga · 2023-03-21T04:17:40Z

@ntdiary thank you for checking on auth token lifespan.

I temporarily let the app jump to the home page instead of getting stuck on the /transition page.

Is it possible for us to intercept the event of clicking on Open Electron button and navigate the user back to the home page?

ntdiary · 2023-03-21T05:35:46Z

App/src/components/DeeplinkWrapper/index.website.js

Line 100 in 9fdc940

window.location.href = expensifyDeeplinkUrl;

@hayata-suenaga, I'm afraid not, this confirmation box is opened by the browser itself when we navigate to the deep link.
There doesn't seem to be an export api for javascript.

chrome source code: https://github.com/chromium/chromium/blob/676a4420834ca68ead67cb4e659db7da4fbbf63d/chrome/browser/ui/views/external_protocol_dialog.cc#L93-L94

hayata-suenaga · 2023-03-21T16:04:09Z

@ntdiary what do you think is the best way to deal with the possibility that the user's auth token might expire while waiting for them to click the "Open Electron App" button?

thesahindia · 2023-03-21T17:58:00Z

@ntdiary what do you think is the best way to deal with the possibility that the user's auth token might expire while waiting for them to click the "Open Electron App" button?

I think we can show a screen, telling the user that their auth token has expired.

ntdiary · 2023-03-22T07:05:32Z

How about showing this screen?

hayata-suenaga · 2023-03-22T15:42:32Z

@ntdiary does the user have to sign in again? not just refreshing the screen?

ntdiary · 2023-03-22T15:57:19Z

@ntdiary does the user have to sign in again? not just refreshing the screen?

The shortLivedAuthToken is generated when the web page is loaded. So the user needs to refresh the page on the web platform and then click "Open the app in Electron" again. 😅

thesahindia · 2023-03-22T15:59:02Z

How about showing this screen?

I think we should confirm this with @shawnborton.
@shawnborton, what do you think about the above screen for the below case?

User's auth token expires while waiting for them to click the "Open Electron App" button?

hayata-suenaga · 2023-03-22T16:10:39Z

I'll wait @shawnborton's response but shouldn't we display "Please refresh again" instead?

And @ntdiary how do you suggest we trigger the change of the screen when the token expires?

ntdiary · 2023-03-22T16:25:52Z

shouldn't we display "Please refresh again" instead?

@hayata-suenaga this screen is displayed in the desktop app. If the authToken has expired, we also can't get a new valid authToken by refreshing the page in the desktop app. 😂

hayata-suenaga

ohhh I completely misunderstood. yes it makes sense to prompt the user to log in again on the desktop side. let's wait for @shawnborton's review but otherwise LGTM!

ntdiary · 2023-03-29T15:43:37Z

Eh, sorry, I seem to have clicked something wrong, which caused the reviewers to change. 😅

src/components/DeeplinkWrapper/index.website.js

ntdiary · 2023-03-30T22:40:35Z

@hayata-suenaga hi, is there anything else I can do? 🙂

hayata-suenaga · 2023-03-31T11:49:45Z

@ntdiary I'm waiting for @thesahindia's review.

thesahindia

It looks good to me @hayata-suenaga

hayata-suenaga

LGTM!

src/libs/actions/Session/index.js

ntdiary · 2023-04-03T16:55:23Z

you mean oldPartnerUserID is always empty?

@hayata-suenaga, only in my test case. I'm not sure if it still useful in the original scenario. So maybe it's good to leave it as it is. (instead of removing it)😂

OSBotify · 2023-04-03T17:14:56Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-04-03T19:27:39Z

🚀 Deployed to staging by https://github.com/pecanoro in version: 1.2.94-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-04-05T02:28:26Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.94-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

tgolen

Hi, I'm sorry to be coming into this late, but someone asked me about some of the code implemented in this PR and I had many questions about it.

tgolen · 2023-04-13T13:34:23Z

src/components/DeeplinkWrapper/index.website.js

@@ -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;


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

App/src/components/DeeplinkWrapper/index.website.js

Line 39 in ae5ecb1

let focused = true;

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.

tgolen · 2023-04-13T13:35:50Z

src/components/DeeplinkWrapper/index.website.js

+            this.openRouteInDesktopApp(expensifyDeeplinkUrl);
+            return;
+        }
+        Authentication.getShortLivedAuthToken().then((shortLivedAuthToken) => {


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).

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

tgolen · 2023-04-13T13:36:34Z

src/components/DeeplinkWrapper/index.website.js

-        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(() => {


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.

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.

tgolen · 2023-04-13T13:37:43Z

src/components/DeeplinkWrapper/index.website.js

+                this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.INSTALLED});
+            } else {
+                this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.NOT_INSTALLED});


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.

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

tgolen · 2023-04-13T13:39:05Z

src/libs/Authentication.js

@@ -105,7 +105,17 @@ function reauthenticate(command = '') {
        });
 }

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


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).

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.

tgolen · 2023-04-13T13:40:37Z

src/libs/Authentication.js

+        if (response && response.shortLivedAuthToken) {
+            return Promise.resolve(response.shortLivedAuthToken);
+        }
+        return Promise.reject();


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).

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

tgolen · 2023-04-13T13:44:28Z

src/libs/actions/Session/index.js

@@ -239,6 +240,11 @@ function signInWithShortLivedAuthToken(email, authToken) {
                isLoading: true,
            },
        },
+        {
+            onyxMethod: CONST.ONYX.METHOD.MERGE,
+            key: ONYXKEYS.IS_TOKEN_VALID,


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?

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.

tgolen · 2023-04-13T13:46:48Z

src/libs/actions/Session/index.js

+        if (response) {
+            return;
+        }
+        Navigation.navigate();


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?

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.

hayata-suenaga · 2023-04-13T16:28:13Z

@tgolen thank you very much for the comments. we should have done more thorough review.

Some of the fixes suggested changed are being implemented in this PR

@ntdiary could you answer and answer @tgolen's questions above and are you also up for a followup PR for fixing issues?

ntdiary · 2023-04-14T08:32:25Z

@hayata-suenaga, of course, this is my responsibility.

@tgolen, thanks you very much for these comments and sorry for the confusion.
And I also re-checked the PR #16383 and this PR, then raised a new followup PR #17452, hope it can help us clean up the messy code and make the logic clearer.

tgolen · 2023-04-14T13:54:17Z

Great, thank you so much for addressing these! It's gonna make this code a lot cleaner. 👍

s77rt · 2023-04-18T05:46:31Z

I think this PR caused a regression #17391 (details here). This PR adds the transition prefix to all deeplinks and I think we should not do that because the transition prefix is only for OLDDOT -> NEWDOT and not WEB -> DESKTOP.

pass user session information when opening the electron app from web …

eb90123

…platform.

ntdiary commented Mar 16, 2023

View reviewed changes

use shortLivedAuthToken instead of authToken

21781a5

ntdiary added 2 commits March 17, 2023 23:44

clean up code

1b894bd

If we fail to get the shortLivedAuthToken, the pop-up won't open

5bf275b

ntdiary marked this pull request as ready for review March 17, 2023 16:56

ntdiary requested a review from a team as a code owner March 17, 2023 16:56

melvin-bot bot requested review from hayata-suenaga and thesahindia and removed request for a team March 17, 2023 16:56

hayata-suenaga requested a review from shawnborton March 22, 2023 16:32

hayata-suenaga previously approved these changes Mar 22, 2023

View reviewed changes

update scene description for SignInWithShortLivedAuthToken

3035261

ntdiary dismissed thesahindia’s stale review via 3035261 March 29, 2023 15:36

ntdiary requested review from hayata-suenaga and removed request for shawnborton March 29, 2023 15:40

pecanoro reviewed Mar 29, 2023

View reviewed changes

src/components/DeeplinkWrapper/index.website.js Show resolved Hide resolved

Add description for catch

42222cd

thesahindia approved these changes Mar 31, 2023

View reviewed changes

hayata-suenaga approved these changes Apr 3, 2023

View reviewed changes

src/libs/actions/Session/index.js Outdated Show resolved Hide resolved

pecanoro approved these changes Apr 3, 2023

View reviewed changes

pecanoro merged commit e4df2d2 into Expensify:main Apr 3, 2023

OSBotify mentioned this pull request Apr 3, 2023

Deploy Checklist: New Expensify 2023-04-03 #16880

Closed

21 tasks

Prince-Mendiratta mentioned this pull request Apr 4, 2023

[MacOS Electron Popup] allow deeplink popup for all routes on mac #16869

Merged

55 tasks

MelvinBot mentioned this pull request Apr 5, 2023

[HOLD for payment 2023-04-12] [$1000] Web - Clicking "open the app in expensify" opens in desktop with already logged in acc #15651

Closed

6 tasks

tgolen reviewed Apr 13, 2023

View reviewed changes

tgolen mentioned this pull request Apr 13, 2023

[HOLD] [WIP] Remove usage of Network.post() #17364

Closed

56 tasks

ntdiary mentioned this pull request Apr 17, 2023

clean up the deep link code #17452

Merged

56 tasks

s77rt mentioned this pull request Apr 18, 2023

[HOLD for payment 2023-07-26] [$2000] Desktop - App is show endless spinner when tap popup Open app in "New Expensify #17391

Closed

6 tasks

tgolen mentioned this pull request Apr 26, 2023

Clean up Deep Link Redirection code #17987

Closed

cead22 changed the title ~~pass user session information when opening the electron app from web platform~~ Pass user session information when opening the electron app from web platform May 25, 2023

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

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

Conversation

ntdiary commented Mar 16, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayata-suenaga Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntdiary commented Mar 16, 2023

hayata-suenaga commented Mar 16, 2023

hayata-suenaga commented Mar 16, 2023 • edited Loading

ntdiary commented Mar 17, 2023

MelvinBot commented Mar 17, 2023

ntdiary commented Mar 20, 2023

hayata-suenaga commented Mar 20, 2023 • edited Loading

hayata-suenaga commented Mar 21, 2023

ntdiary commented Mar 21, 2023

hayata-suenaga commented Mar 21, 2023

thesahindia commented Mar 21, 2023

ntdiary commented Mar 22, 2023

hayata-suenaga commented Mar 22, 2023

ntdiary commented Mar 22, 2023

thesahindia commented Mar 22, 2023

hayata-suenaga commented Mar 22, 2023

ntdiary commented Mar 22, 2023

hayata-suenaga left a comment

Choose a reason for hiding this comment

ntdiary commented Mar 29, 2023

ntdiary commented Mar 30, 2023

hayata-suenaga commented Mar 31, 2023

thesahindia left a comment

Choose a reason for hiding this comment

hayata-suenaga left a comment

Choose a reason for hiding this comment

ntdiary commented Apr 3, 2023

OSBotify commented Apr 3, 2023

OSBotify commented Apr 3, 2023

OSBotify commented Apr 5, 2023

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayata-suenaga commented Apr 13, 2023 • edited Loading

ntdiary commented Apr 14, 2023

tgolen commented Apr 14, 2023

s77rt commented Apr 18, 2023

ntdiary commented Mar 16, 2023 •

edited

Loading

hayata-suenaga Mar 16, 2023 •

edited

Loading

hayata-suenaga commented Mar 16, 2023 •

edited

Loading

hayata-suenaga commented Mar 20, 2023 •

edited

Loading

hayata-suenaga commented Apr 13, 2023 •

edited

Loading