-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add iOS and Android view for SAML Login #29526
Merged
Merged
Changes from 32 commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
400156a
add native view for SAML SignInPage
NikkiWines ca0d065
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines 8991e7e
log in with shortlivedtoken when one is returned
NikkiWines ad6c373
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines 0168e72
include const
NikkiWines 9e4b7ae
style and clean up
NikkiWines 1866096
add SAMLLOadingINdicator
NikkiWines 89dc792
style
NikkiWines 526cf8f
prettier
NikkiWines c386ce4
include platform
NikkiWines ef12fe6
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines 5172f95
pull in changes from infinitered:cdanwards/saml-webview
NikkiWines 544ff27
fix imports
NikkiWines 7073d93
fix redirect flickering when pressing go back after choosing magic code
NikkiWines 4586fee
reset sign in data if going back from SSO provider page
NikkiWines 46a336f
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines 4dd5f55
style
NikkiWines 8ff4c64
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines 14818b7
add in error handling from App/pull/30027
NikkiWines ed66009
update imports
NikkiWines b247558
prettier
NikkiWines 380fa26
remove navigation after callback
NikkiWines dc9d918
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines eaa2438
style
NikkiWines 1c3ad08
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines b011750
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines efa391d
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines dab32d8
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines 769d880
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines fd12887
update error handing to set account error message
NikkiWines 6b5236d
Merge branch 'nikki-saml-newdot-ios' of https://github.com/Expensify/…
NikkiWines 79e89c2
return early when we've already initiated saml login
NikkiWines 6bbce78
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines 1a62ccc
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines 0ebfd25
showLoginForm when error is returned during SAML login
NikkiWines 4a020de
clear signin data after recieving error
NikkiWines 5c8519f
remove unused onyxkey import
NikkiWines 43d7439
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines 01c4029
Merge branch 'main' of https://github.com/Expensify/App into nikki-sa…
NikkiWines File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import React from 'react'; | ||
import {StyleSheet, View} from 'react-native'; | ||
import useLocalize from '@hooks/useLocalize'; | ||
import styles from '@styles/styles'; | ||
import themeColors from '@styles/themes/default'; | ||
import Icon from './Icon'; | ||
import * as Expensicons from './Icon/Expensicons'; | ||
import * as Illustrations from './Icon/Illustrations'; | ||
import Text from './Text'; | ||
|
||
function SAMLLoadingIndicator() { | ||
const {translate} = useLocalize(); | ||
return ( | ||
<View style={[StyleSheet.absoluteFillObject, styles.deeplinkWrapperContainer]}> | ||
<View style={styles.deeplinkWrapperMessage}> | ||
<View style={styles.mb2}> | ||
<Icon | ||
width={200} | ||
height={164} | ||
src={Illustrations.RocketBlue} | ||
/> | ||
</View> | ||
<Text style={[styles.textHeadline, styles.textXXLarge, styles.textAlignCenter]}>{translate('samlSignIn.launching')}</Text> | ||
<View style={[styles.mt2, styles.mh2, styles.fontSizeNormal, styles.textAlignCenter]}> | ||
<Text style={[styles.textAlignCenter]}>{translate('samlSignIn.oneMoment')}</Text> | ||
</View> | ||
</View> | ||
<View style={styles.deeplinkWrapperFooter}> | ||
<Icon | ||
width={154} | ||
height={34} | ||
fill={themeColors.success} | ||
src={Expensicons.ExpensifyWordmark} | ||
/> | ||
</View> | ||
</View> | ||
); | ||
} | ||
|
||
SAMLLoadingIndicator.displayName = 'SAMLLoadingIndicator'; | ||
|
||
export default SAMLLoadingIndicator; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
import PropTypes from 'prop-types'; | ||
import React, {useCallback, useState} from 'react'; | ||
import {withOnyx} from 'react-native-onyx'; | ||
import WebView from 'react-native-webview'; | ||
import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOfflineBlockingView'; | ||
import HeaderWithBackButton from '@components/HeaderWithBackButton'; | ||
import SAMLLoadingIndicator from '@components/SAMLLoadingIndicator'; | ||
import ScreenWrapper from '@components/ScreenWrapper'; | ||
import getPlatform from '@libs/getPlatform'; | ||
import Navigation from '@libs/Navigation/Navigation'; | ||
import * as Session from '@userActions/Session'; | ||
import CONFIG from '@src/CONFIG'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import ROUTES from '@src/ROUTES'; | ||
|
||
const propTypes = { | ||
/** The credentials of the logged in person */ | ||
credentials: PropTypes.shape({ | ||
/** The email/phone the user logged in with */ | ||
login: PropTypes.string, | ||
}), | ||
}; | ||
|
||
const defaultProps = { | ||
credentials: {}, | ||
}; | ||
|
||
function SAMLSignInPage({credentials}) { | ||
const samlLoginURL = `${CONFIG.EXPENSIFY.SAML_URL}?email=${credentials.login}&referer=${CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER}&platform=${getPlatform()}`; | ||
const [showNavigation, shouldShowNavigation] = useState(true); | ||
|
||
/** | ||
* Handles in-app navigation once we get a response back from Expensify | ||
* | ||
* @param {String} params.url | ||
*/ | ||
const handleNavigationStateChange = useCallback( | ||
({url}) => { | ||
// If we've gotten a callback then remove the option to navigate back to the sign in page | ||
if (url.includes('loginCallback')) { | ||
shouldShowNavigation(false); | ||
} | ||
|
||
const searchParams = new URLSearchParams(new URL(url).search); | ||
if (searchParams.has('shortLivedAuthToken')) { | ||
const shortLivedAuthToken = searchParams.get('shortLivedAuthToken'); | ||
Session.signInWithShortLivedAuthToken(credentials.login, shortLivedAuthToken); | ||
} | ||
|
||
// If the login attempt is unsuccessful, set the error message for the account and redirect to sign in page | ||
if (searchParams.has('error')) { | ||
Session.setAccountError(searchParams.get('error')); | ||
Navigation.navigate(ROUTES.HOME); | ||
} | ||
}, | ||
[credentials.login, shouldShowNavigation], | ||
); | ||
|
||
return ( | ||
<ScreenWrapper | ||
shouldShowOfflineIndicator={false} | ||
includeSafeAreaPaddingBottom={false} | ||
testID={SAMLSignInPage.displayName} | ||
> | ||
{showNavigation && ( | ||
<HeaderWithBackButton | ||
title="" | ||
onBackButtonPress={() => { | ||
Session.clearSignInData(); | ||
Navigation.navigate(ROUTES.HOME); | ||
}} | ||
/> | ||
)} | ||
<FullPageOfflineBlockingView> | ||
<WebView | ||
originWhitelist={['https://*']} | ||
source={{uri: samlLoginURL}} | ||
incognito // 'incognito' prop required for Android, issue here https://github.com/react-native-webview/react-native-webview/issues/1352 | ||
startInLoadingState | ||
renderLoading={() => <SAMLLoadingIndicator />} | ||
onNavigationStateChange={handleNavigationStateChange} | ||
/> | ||
</FullPageOfflineBlockingView> | ||
</ScreenWrapper> | ||
); | ||
} | ||
|
||
SAMLSignInPage.propTypes = propTypes; | ||
SAMLSignInPage.defaultProps = defaultProps; | ||
SAMLSignInPage.displayName = 'SAMLSignInPage'; | ||
|
||
export default withOnyx({ | ||
credentials: {key: ONYXKEYS.CREDENTIALS}, | ||
})(SAMLSignInPage); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
And this is the result :
CleanShot.2023-11-17.at.23.09.11.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
How does that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lied, I was able to reproduce it. Updated some logic and it should be working now 🤞 🍀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one small bug 🐛 adressing it now