Add iOS and Android view for SAML Login #29526
Conversation
Deploying with
|
| Latest commit: |
8991e7e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://61bf3dc3.helpdot.pages.dev |
| Branch Preview URL: | https://nikki-saml-newdot-ios.helpdot.pages.dev |
NikkiWines
changed the title
|
@NikkiWines looking great! I was thinking about it, and I'm not sure that we need to navigate after authentication. The navigation stacks will be swapped out |
Oh yeah, good point - let's remove that option once they're authenticated. Though, I'd say that's more of a polish than a real blocker for this one
Oh oops! I definitely meant to include this in the logic I pulled from your PR. Let me add it now |
|
(oops) |
src/pages/signin/SignInPage.js
Outdated
| } else if (hasInitiatedSAMLLogin) { | ||
| // Return early because we're already navigating to a different page | ||
| return; |
There was a problem hiding this comment.
@NikkiWines I think this is not required. With this code, on login failure, the App lands on an empty screen.
CleanShot.2023-11-17.at.23.11.13.mp4
I tried to fix it with this code. We need to verify whether the user has initiated the SAML login process and, if an error has occurred, display the login form to handle the error presentation.
diff --git a/src/pages/signin/SignInPage.js b/src/pages/signin/SignInPage.js
index ce70e548df..2b88e2d346 100644
--- a/src/pages/signin/SignInPage.js
+++ b/src/pages/signin/SignInPage.js
@@ -97,6 +97,7 @@ function getRenderOptions({hasLogin, hasValidateCode, account, isPrimaryLogin, i
const isSAMLEnabled = Boolean(account.isSAMLEnabled);
const isSAMLRequired = Boolean(account.isSAMLRequired);
const hasEmailDeliveryFailure = Boolean(account.hasEmailDeliveryFailure);
+ const hasLoginError = !_.isEmpty(account.errors);
// True if the user has SAML required and we haven't already initiated SAML for their account
const shouldInitiateSAMLLogin = hasAccount && hasLogin && isSAMLRequired && !hasInitiatedSAMLLogin && account.isLoading;
@@ -109,7 +110,7 @@ function getRenderOptions({hasLogin, hasValidateCode, account, isPrimaryLogin, i
Session.clearSignInData();
}
- const shouldShowLoginForm = isClientTheLeader && !hasLogin && !hasValidateCode;
+ const shouldShowLoginForm = isClientTheLeader && ((!hasLogin && !hasValidateCode) || hasInitiatedSAMLLogin && hasLoginError);
const shouldShowEmailDeliveryFailurePage = hasLogin && hasEmailDeliveryFailure && !shouldShowChooseSSOOrMagicCode && !shouldInitiateSAMLLogin;
const isUnvalidatedSecondaryLogin = hasLogin && !isPrimaryLogin && !account.validated && !hasEmailDeliveryFailure;
const shouldShowValidateCodeForm =
@@ -192,9 +193,6 @@ function SignInPage({credentials, account, isInModal, activeClients, preferredLo
if (shouldInitiateSAMLLogin) {
setHasInitiatedSAMLLogin(true);
Navigation.isNavigationReady().then(() => Navigation.navigate(ROUTES.SAML_SIGN_IN));
- } else if (hasInitiatedSAMLLogin) {
- // Return early because we're already navigating to a different page
- return;
}
let welcomeHeader = '';And this is the result :
CleanShot.2023-11-17.at.23.09.11.mp4
There was a problem hiding this comment.
Hmm, I'm not getting that blank page while testing @fedirjh - are you experiencing that only on mobile or also on web? Also if we remove the hasInitiatedSAMLLogin logic we'll get the [warn] SignInPage in unexpected state! - "" error when the user signs in using the SAML required flow.
What about something like:
-function getRenderOptions({hasLogin, hasValidateCode, account, isPrimaryLogin, isUsingMagicCode, hasInitiatedSAMLLogin, isClientTheLeader}) {
+function getRenderOptions({hasLogin, hasValidateCode, account, isPrimaryLogin, isUsingMagicCode, hasInitiatedSAMLLogin, isClientTheLeader, hasAccountError}) {
const hasAccount = !_.isEmpty(account);
const isSAMLEnabled = Boolean(account.isSAMLEnabled);
const isSAMLRequired = Boolean(account.isSAMLRequired);
@@ -109,7 +109,7 @@ function getRenderOptions({hasLogin, hasValidateCode, account, isPrimaryLogin, i
Session.clearSignInData();
}
- const shouldShowLoginForm = isClientTheLeader && !hasLogin && !hasValidateCode;
+ const shouldShowLoginForm = hasAccountError || isClientTheLeader && !hasLogin && !hasValidateCode;
const shouldShowEmailDeliveryFailurePage = hasLogin && hasEmailDeliveryFailure && !shouldShowChooseSSOOrMagicCode && !shouldInitiateSAMLLogin;
const isUnvalidatedSecondaryLogin = hasLogin && !isPrimaryLogin && !account.validated && !hasEmailDeliveryFailure;
const shouldShowValidateCodeForm =
@@ -148,6 +148,7 @@ function SignInPage({credentials, account, isInModal, activeClients, preferredLo
const [hasInitiatedSAMLLogin, setHasInitiatedSAMLLogin] = useState(false);
const isClientTheLeader = activeClients && ActiveClientManager.isClientTheLeader();
+ const hasAccountError = !_.isEmpty(account.errors);
useEffect(() => Performance.measureTTI(), []);
useEffect(() => {
@@ -187,12 +188,13 @@ function SignInPage({credentials, account, isInModal, activeClients, preferredLo
isUsingMagicCode,
hasInitiatedSAMLLogin,
isClientTheLeader,
+ hasAccountError,
});
if (shouldInitiateSAMLLogin) {
setHasInitiatedSAMLLogin(true);
Navigation.isNavigationReady().then(() => Navigation.navigate(ROUTES.SAML_SIGN_IN));
- } else if (hasInitiatedSAMLLogin) {
+ } else if (hasInitiatedSAMLLogin && !hasAccountError) {
// Return early because we're already navigating to a different page
return;
}There was a problem hiding this comment.
are you experiencing that only on mobile or also on web?
@NikkiWines I have not encountered this problem on the web. On the web, it redirects me to the production app, indicating that the app state is not preserved. Instead, a new, fresh state is constructed with the error data.
On mobile, however, the situation is different. The app state is preserved, and a new component (webView) is pushed to the screen. When the web-view is closed, the app restores the old login view along with its state. Therefore, we need to implement failure catch-up logic in that view.
Also if we remove the
hasInitiatedSAMLLoginlogic we'll get the[warn] SignInPage in unexpected state! - ""error when the user signs in using the SAML required flow.
Aha I see. So, that logic will handle the sign-in success case. It makes sense; we need to address both success and failure cases within the same flow.
What about something like:
I tried this solution but it didn't work. It seems account.errors is rested at some point during the flow.
There was a problem hiding this comment.
Also if we remove the
hasInitiatedSAMLLoginlogic we'll get the[warn] SignInPage in unexpected state! - ""error when the user signs in using the SAML required flow.
@NikkiWines I guess this error comes from this line , I think we need to update that block to address the failure case:
App/src/pages/signin/SignInPage.js
Lines 239 to 241 in 79e89c2
There was a problem hiding this comment.
Sorry for the delay here - had some simulator issues and then was out of office for the US holiday weekend. I'm unable to reproduce the behavior you're seeing with the blank screen, but I do see an issue where the user gets redirected to the oldDot error page instead. That's because of a re-routing issue on the backend, which I'll push a PR up shortly to fix.
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-28.at.11.37.28.mp4
I'd rather get this PR out now so that the success flow works on production, and then do a follow up PR to more accurately handle failed sign-in cases. Given that, I can revert my commit that added the following logic:
} else if (hasInitiatedSAMLLogin) {
// Return early because we're already navigating to a different page
return;
How does that sound?
There was a problem hiding this comment.
That looks good to me. Let's address failed sign-in cases carefully in a follow-up PR.
There was a problem hiding this comment.
I lied, I was able to reproduce it. Updated some logic and it should be working now 🤞 🍀
There was a problem hiding this comment.
Found one small bug 🐛 adressing it now
|
@MonilBhavsar tagged you in another PR 😅 but it's super short. Other than that I think this should be ready for a re-review. TY to you both for all the reviewing 🙇 |
|
Looks good to me. It seems all bugs are fixed: Success LoginCleanShot.2023-11-29.at.01.49.56.mp4Failed LoginCleanShot.2023-11-29.at.01.33.23.mp4 |
|
@arosiclair 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] |
|
Edit: Ooh wait nvm, looks like you don't have to! |
|
Merging main now should fix Jest tests |
|
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by https://github.com/yuwenmemon in version: 1.4.7-4 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 1.4.8-0 🚀
|
|
Just tested this on staging iOS and it worked great. Found one small bug where the keyboard stays up on the |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.8-3 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.8-3 🚀
|
Details
Adds SAML sign in flow for iOS and Android platforms. Tied to backend changes from here
Fixed Issues
$ #28217
PROPOSAL: N/A
Tests
(Internal - ios and android native app only)
Go Backoption and confirm you're dropped back on the sign-in page with your email pre-filledContinueUse Magic Codeoption and confirm you get a magic code sent to your emailGo Backand confirm you're back on the sign-in page with your email pre-filledContinueUse Single Sign Onoption<in the top leftContinueUse Single Sign Onagain(External - ios and android native app only)
Go Backoption and confirm you're dropped back on the sign-in page with your email pre-filledContinueUse Magic Codeoption and confirm you get a magic code sent to your emailGo Backand confirm you're back on the sign-in page with your email pre-filledContinueUse Single Sign Onoption<in the top leftContinueUse Single Sign OnagainOffline tests
QA Steps
Same as test steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
SAML Enabled
SAML Required
Android: mWeb Chrome
N/A
iOS: Native
SAML Enabled
Simulator.Screen.Recording.-.iPhone.14.-.2023-10-30.at.16.25.29.mp4
SAML Required
Simulator.Screen.Recording.-.iPhone.14.-.2023-10-30.at.17.01.10.mp4
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
N/A
MacOS: Desktop
N/A