-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Use POST for SAML login #52658
base: main
Are you sure you want to change the base?
Use POST for SAML login #52658
Changes from 31 commits
f5ed242
67148f6
8125593
abea27c
6e71e1a
193e333
5354911
488e69e
2326cd0
1caea73
e81cd71
9d63639
455fb47
3f678b6
4d57937
72d735d
fc12c62
d60395c
103f0b6
cc16359
01dd6ff
9312a29
8f1cb9f
2d614f7
d04d6db
75aa238
ccc87c4
c366857
50f47d1
bd30dd4
4ce67c8
9dc8399
73e8164
fa294e1
fb2acc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,16 @@ | ||
import React, {useCallback, useState} from 'react'; | ||
import React, {useCallback, useEffect, useRef, useState} from 'react'; | ||
import {useOnyx} from 'react-native-onyx'; | ||
import WebView from 'react-native-webview'; | ||
import type {WebViewNativeEvent} from 'react-native-webview/lib/WebViewTypes'; | ||
import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOfflineBlockingView'; | ||
import HeaderWithBackButton from '@components/HeaderWithBackButton'; | ||
import SAMLLoadingIndicator from '@components/SAMLLoadingIndicator'; | ||
import ScreenWrapper from '@components/ScreenWrapper'; | ||
import useLocalize from '@hooks/useLocalize'; | ||
import getPlatform from '@libs/getPlatform'; | ||
import getUAForWebView from '@libs/getUAForWebView'; | ||
import Log from '@libs/Log'; | ||
import {handleSAMLLoginError, postSAMLLogin} from '@libs/LoginUtils'; | ||
import Navigation from '@libs/Navigation/Navigation'; | ||
import * as Session from '@userActions/Session'; | ||
import CONFIG from '@src/CONFIG'; | ||
|
@@ -17,15 +20,45 @@ import ROUTES from '@src/ROUTES'; | |
function SAMLSignInPage() { | ||
const [account] = useOnyx(ONYXKEYS.ACCOUNT); | ||
const [credentials] = useOnyx(ONYXKEYS.CREDENTIALS); | ||
const samlLoginURL = `${CONFIG.EXPENSIFY.SAML_URL}?email=${credentials?.login}&referer=${CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER}&platform=${getPlatform()}`; | ||
const [showNavigation, shouldShowNavigation] = useState(true); | ||
const [SAMLUrl, setSAMLUrl] = useState(''); | ||
const webViewRef = useRef<WebView>(null); | ||
const {translate} = useLocalize(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't get the use of this ref. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
useEffect(() => { | ||
// If we don't have a valid login to pass here, direct the user back to a clean sign in state to try again | ||
if (!credentials?.login) { | ||
handleSAMLLoginError(translate('common.error.email'), true); | ||
return; | ||
} | ||
|
||
// If we've already gotten a url back to log into the user's Identity Provider (IdP), then don't re-fetch it | ||
if (SAMLUrl) { | ||
return; | ||
} | ||
|
||
const body = new FormData(); | ||
body.append('email', credentials.login); | ||
body.append('referer', CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER); | ||
body.append('platform', getPlatform()); | ||
postSAMLLogin(body) | ||
.then((response) => { | ||
if (!response || !response.url) { | ||
handleSAMLLoginError(translate('common.error.login'), false); | ||
return; | ||
} | ||
setSAMLUrl(response.url); | ||
}) | ||
.catch((error: Error) => { | ||
handleSAMLLoginError(error.message ?? translate('common.error.login'), false); | ||
}); | ||
}, [credentials?.login, SAMLUrl, translate]); | ||
|
||
/** | ||
* Handles in-app navigation once we get a response back from Expensify | ||
*/ | ||
const handleNavigationStateChange = useCallback( | ||
({url}: WebViewNativeEvent) => { | ||
Log.info('SAMLSignInPage - Handling SAML navigation change'); | ||
// If we've gotten a callback then remove the option to navigate back to the sign-in page | ||
if (url.includes('loginCallback')) { | ||
shouldShowNavigation(false); | ||
|
@@ -42,7 +75,12 @@ function SAMLSignInPage() { | |
if (searchParams.has('error')) { | ||
Session.clearSignInData(); | ||
Session.setAccountError(searchParams.get('error') ?? ''); | ||
Navigation.navigate(ROUTES.HOME); | ||
|
||
Navigation.isNavigationReady().then(() => { | ||
// We must call goBack() to remove the /transition route from history | ||
Navigation.goBack(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious - where is this transition route set? Is it only from OldDot -> NewDot? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah OldDot -> NewDot - here |
||
Navigation.navigate(ROUTES.HOME); | ||
}); | ||
} | ||
}, | ||
[credentials?.login, shouldShowNavigation, account?.isLoading], | ||
|
@@ -66,14 +104,18 @@ function SAMLSignInPage() { | |
/> | ||
)} | ||
<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} | ||
/> | ||
{!!SAMLUrl && ( | ||
<WebView | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we show the loader until we have the url? |
||
ref={webViewRef} | ||
originWhitelist={['https://*']} | ||
source={{uri: SAMLUrl}} | ||
userAgent={getUAForWebView()} | ||
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> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,42 @@ | ||
import React, {useEffect} from 'react'; | ||
import {withOnyx} from 'react-native-onyx'; | ||
import {useOnyx} from 'react-native-onyx'; | ||
import SAMLLoadingIndicator from '@components/SAMLLoadingIndicator'; | ||
import useLocalize from '@hooks/useLocalize'; | ||
import {handleSAMLLoginError, postSAMLLogin} from '@libs/LoginUtils'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. better to import this as |
||
import CONFIG from '@src/CONFIG'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import type {SAMLSignInPageOnyxProps, SAMLSignInPageProps} from './types'; | ||
|
||
function SAMLSignInPage({credentials}: SAMLSignInPageProps) { | ||
function SAMLSignInPage() { | ||
const {translate} = useLocalize(); | ||
const [credentials] = useOnyx(ONYXKEYS.CREDENTIALS); | ||
|
||
useEffect(() => { | ||
window.location.replace(`${CONFIG.EXPENSIFY.SAML_URL}?email=${credentials?.login}&referer=${CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER}`); | ||
}, [credentials?.login]); | ||
// If we don't have a valid login to pass here, direct the user back to a clean sign in state to try again | ||
if (!credentials?.login) { | ||
handleSAMLLoginError(translate('common.error.email'), true); | ||
return; | ||
} | ||
|
||
const body = new FormData(); | ||
body.append('email', credentials.login); | ||
body.append('referer', CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER); | ||
|
||
postSAMLLogin(body) | ||
.then((response) => { | ||
if (!response || !response.url) { | ||
handleSAMLLoginError(translate('common.error.login'), false); | ||
return; | ||
} | ||
window.location.replace(response.url); | ||
}) | ||
.catch((error: Error) => { | ||
handleSAMLLoginError(error.message ?? translate('common.error.login'), false); | ||
}); | ||
}, [credentials?.login, translate]); | ||
|
||
return <SAMLLoadingIndicator />; | ||
} | ||
|
||
SAMLSignInPage.displayName = 'SAMLSignInPage'; | ||
|
||
export default withOnyx<SAMLSignInPageProps, SAMLSignInPageOnyxProps>({ | ||
account: {key: ONYXKEYS.ACCOUNT}, | ||
credentials: {key: ONYXKEYS.CREDENTIALS}, | ||
})(SAMLSignInPage); | ||
export default SAMLSignInPage; |
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.
Just curious - why are we setting
credentials:omit
here?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.
Following the convention used here - I can test without it but figured it was safest to go with logic we already knew worked.