Use POST for SAML login #52658

NikkiWines · 2024-11-16T02:30:04Z

Explanation of Change

Updates the SAML login flow to use a POST request instead of opening a user-modifiable URL

Dependant on https://github.com/Expensify/Web-Expensify/pull/44462

Fixed Issues

$ Tied to https://github.com/Expensify/Expensify/issues/424196
PROPOSAL: N/A

Tests

Verify that no errors appear in the JS console

(Internal)
Success Flow

Follow steps here to get set up with SAML locally (it's so much simpler now!)
Configure your account to have SAML enabled but not required in Expensify Classic
Enter the email for your account in New Expensify
Choose Use Single Sign On option
Confirm you briefly see an interstitial page about launching your SSO provider's login portal
Enter your SSO credentials
Confirm you're redirected back to New Expensify and are logged into your account

Error Flow

In Web-Expensify, add the following line right below the try{ here: throw new ExpException('An error occurred while loggin in, please try again.', 403);
Enter the email for your account in New Expensify
Choose Use Single Sign On option
Confirm you briefly see an interstitial page
Confirm you're dropped back on the sign in page and see the error An error occurred while loggin in, please try again.

Notes for dev testing:

For the newDot redirect to work locally you may need to hardcode the following value to true.

For mobile testing, you will need to change the url in OneLogin's Expensidev application to your ngrok url instead of https:://www.expensify.com.dev/ so that it loads correctly on the simulators. You can do that by loggin into OneLogin (using carlos@ or dbondy@ creds in 1Pass), going to Administration > Left Hamburger Menu > Applications > Applications > Expensidev Connection > Configuration and updating the urls in ACS (Consumer) URL Validator and ACS (Consumer) URL

Lastly, for Android mWeb, if you use 127.0.0.1:8082/ for your newDot URL, you'll need to update the usage of URL_TO_NEWDOT here to use that URL instead.

(External)

Create an account with a validated domain on Expensify Classic
Configure your account to have SAML enabled but not required in Expensify Classic (use an iDP of your choice, we tend to test with OneLogin)
Enter the email for your account in New Expensify
Confirm you see the option to choose to sign in with SAML or with a magic code
Choose Use Single Sign On option
Confirm you briefly see an interstitial page about launching your SSO provider's login portal
Enter your SSO credentials
Confirm you're redirected back to New Expensify and are logged into your account

Offline tests

N/A SAML is not supported offline

QA Steps

Verify that no errors appear in the JS console

Log into New Expensify with the email [email protected]
Choose Use Single Sign On option
Confirm you briefly see an interstitial page about launching your SSO provider's login portal
Enter the SSO credentials for [email protected] (in 1password under QA OneLogin)
Confirm you're redirected back to New Expensify and are logged into your account
- Note: You will be dropped back into new.expensify.com, not staging.new.expensify.com, this is known and expected because of how the SAML metadata is configured in Expensify Classic

PR Author Checklist

Screenshots/Videos

Android: Native

Error:

Screen.Recording.2024-11-28.at.18.19.42.mov

Success:

android.success.mov

Android: mWeb Chrome

Error:

mweb.android.failure.mov

Success:

android.mweb.success.mov

iOS: Native

Error:

ios.error.mp4

Success:

ios.success.mp4

iOS: mWeb Safari

Error:

ios.mweb.error.mp4

Success:

ios.mweb.success.mp4

MacOS: Chrome / Safari

Error:

web.error.mov

Success:

web.success.mov

MacOS: Desktop

Error:

desktop.error.mov

Success:

desktop.success.mov

NikkiWines · 2024-11-28T22:06:59Z

Appears to be failing on an unrelated tyepscript check error - but should be ready for review

Error: src/pages/workspace/upgrade/UpgradeIntro.tsx(91,48): error TS2339: Property 'PLAN_TYPES_AND_PRICING_HELP_URL' does not exist on type '{ readonly HEIC_SIGNATURES: readonly ["6674797068656963", "6674797068656978", "6674797068657631", "667479706d696631"]; readonly RECENT_WAYPOINTS_NUMBER: 20; readonly DEFAULT_DB_NAME: "OnyxDB"; ... 491 more ...; readonly HYBRID_APP: { ...; }; }'.
Error: Process completed with exit code 2.

melvin-bot · 2024-11-28T22:08:03Z

@MariaHCD Please 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]

MariaHCD

Mostly have some questions / clarifications :)

MariaHCD · 2024-11-29T09:43:49Z

src/libs/LoginUtils.ts

+            }
+            return response.json() as Promise<Response>;
+        })
+        .then((response) => response);


Do we need this second .then()? Can we return the json in the previous .then() 🤔

Yeah, at least from my testing we need both - I also followed the convention from here

What's the convention in this case? I don't see .then((response) => response); in the linked code, and was also wondering about this because this line looks like no-op

looks like this is actually fine without - removing!

src/pages/signin/SAMLSignInPage/index.native.tsx

MariaHCD · 2024-11-29T09:57:29Z

src/libs/LoginUtils.ts

+    return fetch(CONFIG.EXPENSIFY.SAML_URL, {
+        method: CONST.NETWORK.METHOD.POST,
+        body,
+        credentials: 'omit',


Just curious - why are we setting credentials:omit here?

Following the convention used here - I can test without it but figured it was safest to go with logic we already knew worked.

MariaHCD · 2024-11-29T10:11:39Z

src/pages/signin/SAMLSignInPage/index.native.tsx

+        }
+
+        const body = new FormData();
+        body.append('email', credentials?.login ?? '');


What happens if email is set as an empty string?

I think rn the iDP page fails to load so they get stuck on the loading screen, i'll update the logic to return early if there's no login to pass.

MariaHCD · 2024-11-29T10:12:42Z

src/pages/signin/SAMLSignInPage/index.native.tsx

-                Navigation.navigate(ROUTES.HOME);
+
+                Navigation.isNavigationReady().then(() => {
+                    // We must call goBack() to remove the /transition route from history


Just curious - where is this transition route set? Is it only from OldDot -> NewDot?

Yeah OldDot -> NewDot - here

src/pages/signin/SAMLSignInPage/index.native.tsx

src/pages/signin/SAMLSignInPage/index.tsx

Co-authored-by: Maria D'Costa <[email protected]>

…ikki-saml-use-post

NikkiWines · 2024-12-20T00:56:17Z

updated once more 🙇

cead22

LGTM! Can you ask a C+ to complete the reviewer checklist with tests in all platform 🙏 ?

parasharrajat · 2024-12-20T22:23:01Z

There are a bunch of test steps here. Which test suites do I have to follow @NikkiWines?

NikkiWines · 2024-12-20T22:24:57Z

@parasharrajat just the steps under (External) but for each platform!

parasharrajat · 2024-12-20T22:57:00Z

In the process of adding a verified domain to my test account.

parasharrajat · 2024-12-20T22:58:47Z

src/pages/signin/SAMLSignInPage/index.tsx

 import SAMLLoadingIndicator from '@components/SAMLLoadingIndicator';
+import useLocalize from '@hooks/useLocalize';
+import {handleSAMLLoginError, postSAMLLogin} from '@libs/LoginUtils';


better to import this as import * as LoginUtils format.

parasharrajat · 2024-12-20T23:02:36Z

src/pages/signin/SAMLSignInPage/index.native.tsx

-                    renderLoading={() => <SAMLLoadingIndicator />}
-                    onNavigationStateChange={handleNavigationStateChange}
-                />
+                {!!SAMLUrl && (


Should we show the loader until we have the url?

parasharrajat · 2024-12-20T23:03:41Z

src/pages/signin/SAMLSignInPage/index.native.tsx

    const [showNavigation, shouldShowNavigation] = useState(true);
+    const [SAMLUrl, setSAMLUrl] = useState('');
+    const webViewRef = useRef<WebView>(null);


I didn't get the use of this ref.

I wasn't sure about it either but I saw the same usage for multiple other WebViews (see: ConnectToQuickbookOnlineFlow, ConnectToXeroFlow, WalletStatementModal) and figured it was wise to follow that existing convention

parasharrajat · 2024-12-22T14:08:26Z

OK, I see this now. What does that mean?

NikkiWines · 2025-01-06T16:28:03Z

@parasharrajat sorry for the delay, I was OOO for the EOY holidays. Just to confirm, have you done the following:

Set up an account that has SAML enabled or required on Expensify Classic (by creating an account with a validated domain and enabling/requiring SAML on that domain with an SSO provider of your choice)
Attempted to log in via NewExpensify as that account or as a member of that domain

The error you're seeing appears to be due to your email not passing some validation checks. If you want to share the email you're using and try again I can pull the logs for your account and see if I can help figure out what's going wrong.

parasharrajat · 2025-01-06T16:30:49Z

I am using the admin account to login which has the domain. Email is contact@netvade[DOT]com

NikkiWines · 2025-01-06T17:26:44Z

From the internal logs it looks like the issue is that the nameID returned from your SSO provider (google-oauth2|101006148289929418908) is different than the domain provided (netvade.com) and so we try to use the nameID as the domain and then fail to correctly parse an email from it. That logic is here (internal link)

cc: @cead22 you might know a bit about this as it looks like it was modified fairly recently in your PR here

2024-12-22 13:08:35 916	[email protected]	NameID in the SAML response is on a different domain than the passed one ~~ nameID: 'google-oauth2|101006148289929418908' domain: 'netvade.com'
2024-12-22 13:08:35 916	[email protected]	User_Store - Invalid email ~~ email: '+@'
2024-12-22 13:08:35 916	[email protected]	Expensify\Authentication\SAML2\Utils - Error Expensify\Libs\Error\ExpError - E2C65E07-FA96-4D79-9209-BE4D400192BD ~~ exceptionJsonCode: '402' exceptionUserMessage: 'The given email is not valid.' exceptionMessage: 'The given email is not valid.'

parasharrajat · 2025-01-07T08:52:00Z

Let me analyze this.

parasharrajat · 2025-01-07T17:32:39Z

I believe I am doing something wrong. I used Auth0 to setup SAML but I created the account from my gmail email.

NikkiWines · 2025-01-08T15:40:47Z

Hmm, yeah - you'll definitely need parity between the email used for the account and the email used for Auth0/SAML

NikkiWines · 2025-01-08T16:45:46Z

@parasharrajat you might try OneLogin's 30 day free trial to test this

parasharrajat · 2025-01-09T19:45:53Z

Setting up OneLogin and I will have this test in 8 hours due to traveling.

parasharrajat · 2025-01-10T12:31:04Z

Ok, I was able to login and I am redirected to OldDot afterwards. @NikkiWines
I also need to disable CORS to be able to proceed.

10.01.2025_18.03.31_REC.mp4

NikkiWines · 2025-01-10T17:44:05Z

Ah, yes, this is because when doing the SAML routing via dev we don't persist some of the cookies that we use to determine whether to route the user to New Expensify vs. Expensify Classic. I experience the same thing locally unless I hardcode those values for testing purposes.

I can confirm that your account is going through the post flow though (via the logs), and the re-routing logic in the callback is actually unchanged as part of migrating this to use post, so we should be all good here.

2025-01-10 12:33:33 872	[email protected]	User is using SAML post flow ~~ email: '[email protected]' requestMethod: 'POST' httpReferrer: 'https://dev.new.expensify.com:8082/' referrer: 'ecash' appVersion: ''

cc:: @cead22 what do you think?

parasharrajat · 2025-01-10T18:22:42Z

Let me test this on all platforms in that case.

parasharrajat · 2025-01-10T21:25:21Z

Screenshots

🔲 iOS / native

11.01.2025_02.54.01_REC.mp4

🔲 iOS / Safari

CORS error

🔲 MacOS / Desktop

11.01.2025_02.42.10_REC.mp4

🔲 MacOS / Chrome

401987740-33c990b7-972c-4a13-a465-fa60d7f928ec.mp4

🔲 Android / Chrome

CORS error

🔲 Android / native

11.01.2025_02.52.02_REC.mp4

parasharrajat · 2025-01-10T21:26:14Z

@NikkiWines I noticed that even on native dev apps the redirection does not happen to app, after login, user is redirected to oldDot website in the opened webview instead of going back to app. Is this expected?

NikkiWines added 2 commits November 15, 2024 21:28

use fetch instead of simply window.location

f5ed242

update to use withOnyx, updated props

67148f6

NikkiWines self-assigned this Nov 16, 2024

NikkiWines added 13 commits November 20, 2024 22:50

Merge branch 'main' of github.com:Expensify/App into nikki-saml-use-post

8125593

Merge branch 'main' of github.com:Expensify/App into nikki-saml-use-post

abea27c

minor style

6e71e1a

introduce post for SAML native

193e333

style

5354911

dry up saml url logic

488e69e

style

2326cd0

Merge branch 'main' of github.com:Expensify/App into nikki-saml-use-post

1caea73

Merge branch 'main' of github.com:Expensify/App into nikki-saml-use-post

e81cd71

update SAML native logic to handle errors

9d63639

fix error handling for signing in with short lived authtoken

455fb47

minor style

3f678b6

Merge branch 'main' of github.com:Expensify/App into nikki-saml-use-post

4d57937

NikkiWines changed the title ~~Use POST for SAML login~~ [HOLD] Use POST for SAML login Nov 28, 2024

NikkiWines marked this pull request as ready for review November 28, 2024 22:07

NikkiWines requested a review from a team as a code owner November 28, 2024 22:07

melvin-bot bot requested review from MariaHCD and removed request for a team November 28, 2024 22:08

MariaHCD reviewed Nov 29, 2024

View reviewed changes

MariaHCD requested a review from cead22 November 29, 2024 10:28

NikkiWines and others added 5 commits November 29, 2024 10:43

Add clarification

72d735d

Co-authored-by: Maria D'Costa <[email protected]>

return early with clean session if no login

fc12c62

prettier

d60395c

Merge branch 'nikki-saml-use-post' of github.com:Expensify/App into n…

103f0b6

…ikki-saml-use-post

add copy

cc16359

NikkiWines added 2 commits December 19, 2024 18:41

dry handle error functionality and rename function

c366857

remove noop

50f47d1

NikkiWines added 2 commits December 19, 2024 19:58

fix imports

bd30dd4

add back navigation import

4ce67c8

cead22 previously approved these changes Dec 20, 2024

View reviewed changes

NikkiWines requested a review from parasharrajat December 20, 2024 22:21

parasharrajat suggested changes Dec 20, 2024

View reviewed changes

fix import and show loading screen

9dc8399

NikkiWines dismissed cead22’s stale review via 9dc8399 December 21, 2024 01:47

fix double negation

73e8164

Merge branch 'main' of github.com:Expensify/App into nikki-saml-use-post

fa294e1

Merge branch 'main' of github.com:Expensify/App into nikki-saml-use-post

fb2acc8

Use POST for SAML login #52658

Are you sure you want to change the base?

Use POST for SAML login #52658

Conversation

NikkiWines commented Nov 16, 2024 • edited Loading

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

NikkiWines commented Nov 28, 2024

melvin-bot bot commented Nov 28, 2024

MariaHCD 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

NikkiWines commented Dec 20, 2024

cead22 left a comment

Choose a reason for hiding this comment

parasharrajat commented Dec 20, 2024

NikkiWines commented Dec 20, 2024 • edited Loading

parasharrajat commented Dec 20, 2024

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

parasharrajat commented Dec 22, 2024

NikkiWines commented Jan 6, 2025

parasharrajat commented Jan 6, 2025

NikkiWines commented Jan 6, 2025 • edited Loading

parasharrajat commented Jan 7, 2025

parasharrajat commented Jan 7, 2025

NikkiWines commented Jan 8, 2025

NikkiWines commented Jan 8, 2025

parasharrajat commented Jan 9, 2025 • edited Loading

parasharrajat commented Jan 10, 2025 • edited Loading

NikkiWines commented Jan 10, 2025 • edited Loading

parasharrajat commented Jan 10, 2025

parasharrajat commented Jan 10, 2025

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

parasharrajat commented Jan 10, 2025

NikkiWines commented Nov 16, 2024 •

edited

Loading

NikkiWines commented Dec 20, 2024 •

edited

Loading

NikkiWines commented Jan 6, 2025 •

edited

Loading

parasharrajat commented Jan 9, 2025 •

edited

Loading

parasharrajat commented Jan 10, 2025 •

edited

Loading

NikkiWines commented Jan 10, 2025 •

edited

Loading