From 537bd040057063da83c785eecdd94e7738c46c5e Mon Sep 17 00:00:00 2001 From: Chris Carpita Date: Mon, 20 Sep 2021 21:07:18 -0400 Subject: [PATCH 1/2] fix(SignInPage): Ensure click handler and login popup occur in same tick The current SignIn implementation is broken on Safari iOS by default, due to the combination of useEffect and the extra asynchronous login check that occurs before the authentication routine that causes the popup (instantPopup: true). This change refactors the internals of SignIn page to avoid useEffect, which was arguably confusing for implementors, and also avoid overloading the use of the poorly scoped autoSignIn variable. Signed-off-by: Chris Carpita --- .changeset/hot-shirts-shake.md | 5 + .../src/layout/SignInPage/SignInPage.tsx | 91 ++++++++++--------- 2 files changed, 51 insertions(+), 45 deletions(-) create mode 100644 .changeset/hot-shirts-shake.md diff --git a/.changeset/hot-shirts-shake.md b/.changeset/hot-shirts-shake.md new file mode 100644 index 0000000000..68b133cfdf --- /dev/null +++ b/.changeset/hot-shirts-shake.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-components': minor +--- + +Fixed a popup-blocking bug affecting iOS Safari in SignInPage.tsx by ensuring that the popup occurs in the same tick as the tap/click diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index e0d4f6ed0d..84998b6ba6 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -15,6 +15,7 @@ */ import { + BackstageIdentity, configApiRef, SignInPageProps, useApi, @@ -91,62 +92,63 @@ export const SingleSignInPage = ({ const authApi = useApi(provider.apiRef); const configApi = useApi(configApiRef); - const [autoShowPopup, setAutoShowPopup] = useState(auto ?? false); - // Defaults to true so that an initial check for existing user session is made - const [retry, setRetry] = useState<{} | boolean | undefined>(undefined); const [error, setError] = useState(); + const [loginCount, setLoginCount] = useState(0); // The SignIn component takes some time to decide whether the user is logged-in or not. // showLoginPage is used to prevent a glitch-like experience where the sign-in page is // displayed for a split second when the user is already logged-in. const [showLoginPage, setShowLoginPage] = useState(false); - useEffect(() => { - const login = async () => { - try { - let identity; + type LoginOpts = { checkExisting?: boolean; showPopup?: boolean }; + const login = async ({ checkExisting, showPopup }: LoginOpts) => { + setLoginCount(prev => prev + 1); + try { + let identity: BackstageIdentity | undefined; + if (checkExisting) { // Do an initial check if any logged-in session exists identity = await authApi.getBackstageIdentity({ optional: true, }); - - // If no session exists, show the sign-in page - if (!identity && autoShowPopup) { - // Unless auto is set to true, this step should not happen. - // When user intentionally clicks the Sign In button, autoShowPopup is set to true - setShowLoginPage(true); - identity = await authApi.getBackstageIdentity({ - instantPopup: true, - }); - } - - if (!identity) { - setShowLoginPage(true); - return; - } - - const profile = await authApi.getProfile(); - onResult({ - userId: identity!.id, - profile: profile!, - getIdToken: () => { - return authApi - .getBackstageIdentity() - .then(i => i!.token ?? i!.idToken); - }, - signOut: async () => { - await authApi.signOut(); - }, - }); - } catch (err) { - // User closed the sign-in modal - setError(err); - setShowLoginPage(true); } - }; - login(); - }, [onResult, authApi, retry, autoShowPopup]); + // If no session exists, show the sign-in page + if (!identity && (showPopup || auto)) { + // Unless auto is set to true, this step should not happen. + // When user intentionally clicks the Sign In button, autoShowPopup is set to true + setShowLoginPage(true); + identity = await authApi.getBackstageIdentity({ + instantPopup: true, + }); + } + + if (!identity) { + setShowLoginPage(true); + return; + } + + const profile = await authApi.getProfile(); + onResult({ + userId: identity!.id, + profile: profile!, + getIdToken: () => { + return authApi + .getBackstageIdentity() + .then(i => i!.token ?? i!.idToken); + }, + signOut: async () => { + await authApi.signOut(); + }, + }); + } catch (err: any) { + // User closed the sign-in modal + setError(err); + setShowLoginPage(true); + } + }; + if (loginCount === 0) { + login({ checkExisting: true }); + } return showLoginPage ? ( @@ -168,8 +170,7 @@ export const SingleSignInPage = ({ color="primary" variant="outlined" onClick={() => { - setRetry({}); - setAutoShowPopup(true); + login({ showPopup: true }); }} > Sign In From 18d284b924f8bf24c0c364848170dba7bcfe2805 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Wed, 22 Sep 2021 15:43:33 +0200 Subject: [PATCH 2/2] fix lint warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- packages/core-components/src/layout/SignInPage/SignInPage.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index 84998b6ba6..d64a59158d 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -21,7 +21,7 @@ import { useApi, } from '@backstage/core-plugin-api'; import { Button, Grid, Typography } from '@material-ui/core'; -import React, { useEffect, useState } from 'react'; +import React, { useState } from 'react'; import { Progress } from '../../components/Progress'; import { Content } from '../Content/Content'; import { ContentHeader } from '../ContentHeader/ContentHeader';