Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adds navigation and styled wrapper for webview - Introduces Error handling start #30027

Closed
wants to merge 1 commit into from
Closed

Adds navigation and styled wrapper for webview - Introduces Error handling start #30027

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 49 additions & 20 deletions src/pages/signin/SAMLSignInPage/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
import React, {useCallback, useRef} from 'react';
import {View, ActivityIndicator, StyleSheet} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import PropTypes from 'prop-types';
import WebView from 'react-native-webview';
import HeaderWithBackButton from '../../../components/HeaderWithBackButton';
import ONYXKEYS from '../../../ONYXKEYS';
import CONFIG from '../../../CONFIG';
import WebView from 'react-native-webview';
import FullScreenLoadingIndicator from '../../../components/FullscreenLoadingIndicator';
import * as Session from '../../../libs/actions/Session';
import Navigation from '../../../libs/Navigation/Navigation';
import ROUTES from '../../../ROUTES';
import ScreenWrapper from '../../../components/ScreenWrapper';
import styles from '../../../styles/styles';
import themeColors from '../../../styles/themes/default';
import FullPageOfflineBlockingView from '../../../components/BlockingViews/FullPageOfflineBlockingView';

const propTypes = {
/** The credentials of the logged in person */
Expand All @@ -19,39 +26,61 @@
credentials: {},
};

const renderLoading = () => <FullScreenLoadingIndicator />;
// const renderLoading = () => <FullScreenLoadingIndicator />;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this if it's unused with the new logic


function SAMLSignInPage({credentials}) {
const webViewRef = useRef();
const samlLoginURL = `${CONFIG.EXPENSIFY.SAML_URL}?email=${credentials.login}&referer=${CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER}`;

/**
* Handles in-app navigation once we get a response back from Expensify
*
* @param {String} params.type
* @param {String} params.url
*/
* Handles in-app navigation once we get a response back from Expensify
*
* @param {String} params.type
* @param {String} params.url
*/
const handleNavigationStateChange = useCallback(
({type, url}) => {

Check failure on line 41 in src/pages/signin/SAMLSignInPage/index.native.js

View workflow job for this annotation

GitHub Actions / lint 

'type' is defined but never used
const searchParams = new URLSearchParams(new URL(url).search);
if (searchParams.has('shortLivedAuthToken')) {
const shortLivedAuthToken = searchParams.get('shortLivedAuthToken');
Session.signInWithShortLivedAuthToken(credentials.login, shortLivedAuthToken);
return;
Navigation.navigate(ROUTES.HOME);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the return; or Navigation.navigate(ROUTES.HOME) here actually. Session.signInWithShortLivedAuthToken should take care of any necessary redirects

}
if (searchParams.has('error')) {
// Run the Onyx action to set an error state on the sign in page
// Currently this is what's going to trigger because the backend isn't redirecting SAML correctly
Navigation.navigate(ROUTES.HOME);
}
},
[webViewRef],
[credentials.login],
);
return (
<WebView
ref={webViewRef}
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={renderLoading}
onNavigationStateChange={handleNavigationStateChange}
/>
<ScreenWrapper
shouldShowOfflineIndicator={false}
includeSafeAreaPaddingBottom={false}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to add a component display name and then pass it here

Suggested change
includeSafeAreaPaddingBottom={false}
includeSafeAreaPaddingBottom={false}
testID={SAMLSignInPage.displayName}

>
<HeaderWithBackButton
title="SAML Sign In"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can leave this blank I think it'd be cleaner

onBackButtonPress={() => Navigation.navigate(ROUTES.HOME)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we'd want to call Session.clearSignInData(); here. Otherwise, if the user has SAML required on their account they'll be dropped right back into the same flow

/>
<FullPageOfflineBlockingView>
<WebView
ref={webViewRef}
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={() => (
<View style={[StyleSheet.absoluteFillObject, styles.fullScreenLoading]}>
<ActivityIndicator
color={themeColors.spinner}
size="large"
/>
</View>
)}
onNavigationStateChange={handleNavigationStateChange}
/>
</FullPageOfflineBlockingView>
</ScreenWrapper>
);
}

Expand Down
Loading