Use POST for SAML login #52658
Use POST for SAML login #52658
Conversation
|
Appears to be failing on an unrelated tyepscript check error - but should be ready for review |
melvin-bot
bot
requested review from
MariaHCD
and removed request for
a team
| } | ||
| return response.json() as Promise<Response>; | ||
| }) | ||
| .then((response) => response); |
There was a problem hiding this comment.
Do we need this second .then()? Can we return the json in the previous .then() 🤔
There was a problem hiding this comment.
Yeah, at least from my testing we need both - I also followed the convention from here
| return fetch(CONFIG.EXPENSIFY.SAML_URL, { | ||
| method: CONST.NETWORK.METHOD.POST, | ||
| body, | ||
| credentials: 'omit', |
There was a problem hiding this comment.
Just curious - why are we setting credentials:omit here?
There was a problem hiding this comment.
Following the convention used here - I can test without it but figured it was safest to go with logic we already knew worked.
| } | ||
|
|
||
| const body = new FormData(); | ||
| body.append('email', credentials?.login ?? ''); |
There was a problem hiding this comment.
What happens if email is set as an empty string?
There was a problem hiding this comment.
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.
| Navigation.navigate(ROUTES.HOME); | ||
|
|
||
| Navigation.isNavigationReady().then(() => { | ||
| // We must call goBack() to remove the /transition route from history |
There was a problem hiding this comment.
Just curious - where is this transition route set? Is it only from OldDot -> NewDot?
There was a problem hiding this comment.
Yeah OldDot -> NewDot - here
Co-authored-by: Maria D'Costa <[email protected]>
…ikki-saml-use-post
|
Off hold now that the corresponding Web-E PR is on prod! |
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
*(Internal) *
Success Flow
Use Single Sign OnoptionError Flow
try{here:throw new ExpException('An error occurred while loggin in, please try again.', 403);Use Single Sign OnoptionAn 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 inACS (Consumer) URL ValidatorandACS (Consumer) URLLastly, for Android mWeb, if you use
127.0.0.1:8082/for your newDot URL, you'll need to update the usage ofURL_TO_NEWDOThere to use that URL instead.(External)
Use Single Sign OnoptionOffline tests
N/A SAML is not supported offline
QA Steps
Log into New Expensify with the email [email protected]
Choose
Use Single Sign OnoptionConfirm 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
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.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