From 477872d14996b3be78eb60c2eeffe7cc60b7a3fe Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 17 May 2021 23:00:32 +0200 Subject: [PATCH 1/5] auth: Fix SingleSingInPage to not ask for login on every page refresh 1. Disable instantPopup when requesting Backstage identity. If it is set to true, existing user sessions are discarded - from core-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts 2. The auto prop should be true by default, to trigger a log-in attempt when the SingleSignInPage component is loaded Signed-off-by: Himanshu Mishra --- .../core/src/layout/SignInPage/SignInPage.tsx | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/packages/core/src/layout/SignInPage/SignInPage.tsx b/packages/core/src/layout/SignInPage/SignInPage.tsx index daefe40c73..46d91a5375 100644 --- a/packages/core/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core/src/layout/SignInPage/SignInPage.tsx @@ -14,18 +14,18 @@ * limitations under the License. */ +import { configApiRef, SignInPageProps, useApi } from '@backstage/core-api'; +import { Button, Grid, Typography } from '@material-ui/core'; import React, { useEffect, useState } from 'react'; -import { Page } from '../Page'; -import { Header } from '../Header'; +import { Progress } from '../../components/Progress'; import { Content } from '../Content/Content'; import { ContentHeader } from '../ContentHeader/ContentHeader'; -import { Grid, Button, Typography } from '@material-ui/core'; -import { SignInPageProps, useApi, configApiRef } from '@backstage/core-api'; -import { useSignInProviders, getSignInProviders } from './providers'; -import { IdentityProviders, SignInProviderConfig } from './types'; -import { Progress } from '../../components/Progress'; -import { GridItem, useStyles } from './styles'; +import { Header } from '../Header'; import { InfoCard } from '../InfoCard'; +import { Page } from '../Page'; +import { getSignInProviders, useSignInProviders } from './providers'; +import { GridItem, useStyles } from './styles'; +import { IdentityProviders, SignInProviderConfig } from './types'; type MultiSignInPageProps = SignInPageProps & { providers: IdentityProviders; @@ -81,7 +81,7 @@ export const MultiSignInPage = ({ export const SingleSignInPage = ({ onResult, provider, - auto, + auto = true, }: SingleSignInPageProps) => { const classes = useStyles(); const authApi = useApi(provider.apiRef); @@ -92,9 +92,7 @@ export const SingleSignInPage = ({ useEffect(() => { const login = async () => { - const identity = await authApi.getBackstageIdentity({ - instantPopup: true, - }); + const identity = await authApi.getBackstageIdentity(); const profile = await authApi.getProfile(); onResult({ From 5da6a561d1e77f9b115100810f6e373fc60a2273 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 17 May 2021 23:10:47 +0200 Subject: [PATCH 2/5] chore: Add changeset for SingleSignInPage bug fix Signed-off-by: Himanshu Mishra --- .changeset/moody-beds-chew.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/moody-beds-chew.md diff --git a/.changeset/moody-beds-chew.md b/.changeset/moody-beds-chew.md new file mode 100644 index 0000000000..72962eea59 --- /dev/null +++ b/.changeset/moody-beds-chew.md @@ -0,0 +1,5 @@ +--- +'@backstage/core': patch +--- + +Fix a bug where users are asked to log-in on every page refresh. This is specific to people with only one sign-in provider in their SignInPage component. From a4dd21dd251fae3da0249cef4f8470c462d32cf9 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 18 May 2021 11:24:05 +0200 Subject: [PATCH 3/5] SignInPage: prevent a small glitch on page load when user is logged in Signed-off-by: Himanshu Mishra --- .../core/src/layout/SignInPage/SignInPage.tsx | 44 ++++++++++++------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/packages/core/src/layout/SignInPage/SignInPage.tsx b/packages/core/src/layout/SignInPage/SignInPage.tsx index 46d91a5375..74a53b6b45 100644 --- a/packages/core/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core/src/layout/SignInPage/SignInPage.tsx @@ -90,29 +90,43 @@ export const SingleSignInPage = ({ const [retry, setRetry] = useState<{} | boolean | undefined>(auto); const [error, setError] = useState(); + // 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 () => { - const identity = await authApi.getBackstageIdentity(); + try { + const identity = await authApi.getBackstageIdentity(); - const profile = await authApi.getProfile(); - onResult({ - userId: identity!.id, - profile: profile!, - getIdToken: () => { - return authApi.getBackstageIdentity().then(i => i!.idToken); - }, - signOut: async () => { - await authApi.signOut(); - }, - }); + const profile = await authApi.getProfile(); + onResult({ + userId: identity!.id, + profile: profile!, + getIdToken: () => { + return authApi.getBackstageIdentity().then(i => i!.idToken); + }, + signOut: async () => { + await authApi.signOut(); + }, + }); + + // Don't show if login is successful + setShowLoginPage(false); + } catch (err) { + // User closed the sign-in modal + setError(err); + setShowLoginPage(true); + } }; if (retry) { - login().catch(setError); + login(); } }, [onResult, authApi, retry]); - return ( + return showLoginPage ? (
@@ -148,7 +162,7 @@ export const SingleSignInPage = ({ - ); + ) : null; }; export const SignInPage = (props: Props) => { From 045cc0207624d33ea74a63226baadb8f4c9e5c70 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 18 May 2021 16:15:07 +0200 Subject: [PATCH 4/5] 1. use optional: true when fetching backstageidentity which doesn't prompt sign-up if no session is found 2. use Progress bar when the logic is happening 3. remove redundant showLoginPage Signed-off-by: Himanshu Mishra --- .../core/src/layout/SignInPage/SignInPage.tsx | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/core/src/layout/SignInPage/SignInPage.tsx b/packages/core/src/layout/SignInPage/SignInPage.tsx index 74a53b6b45..92041f9a0f 100644 --- a/packages/core/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core/src/layout/SignInPage/SignInPage.tsx @@ -98,7 +98,18 @@ export const SingleSignInPage = ({ useEffect(() => { const login = async () => { try { - const identity = await authApi.getBackstageIdentity(); + let identity; + // 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) { + identity = await authApi.getBackstageIdentity({ + instantPopup: true, + }); + } const profile = await authApi.getProfile(); onResult({ @@ -111,9 +122,6 @@ export const SingleSignInPage = ({ await authApi.signOut(); }, }); - - // Don't show if login is successful - setShowLoginPage(false); } catch (err) { // User closed the sign-in modal setError(err); @@ -162,7 +170,9 @@ export const SingleSignInPage = ({ - ) : null; + ) : ( + + ); }; export const SignInPage = (props: Props) => { From 9e793d3f8dea8274582359a4fab01080a1d5731e Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Thu, 20 May 2021 11:00:41 +0200 Subject: [PATCH 5/5] auth(signinpage): handle case when auto is set to false A check for existing user session should still be made. Signed-off-by: Himanshu Mishra --- .../core/src/layout/SignInPage/SignInPage.tsx | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/core/src/layout/SignInPage/SignInPage.tsx b/packages/core/src/layout/SignInPage/SignInPage.tsx index 92041f9a0f..6a8dcf454e 100644 --- a/packages/core/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core/src/layout/SignInPage/SignInPage.tsx @@ -81,13 +81,15 @@ export const MultiSignInPage = ({ export const SingleSignInPage = ({ onResult, provider, - auto = true, + auto, }: SingleSignInPageProps) => { const classes = useStyles(); const authApi = useApi(provider.apiRef); const configApi = useApi(configApiRef); - const [retry, setRetry] = useState<{} | boolean | undefined>(auto); + 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>(true); const [error, setError] = useState(); // The SignIn component takes some time to decide whether the user is logged-in or not. @@ -105,12 +107,19 @@ export const SingleSignInPage = ({ }); // If no session exists, show the sign-in page - if (!identity) { + 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 identity = await authApi.getBackstageIdentity({ instantPopup: true, }); } + if (!identity) { + setShowLoginPage(true); + return; + } + const profile = await authApi.getProfile(); onResult({ userId: identity!.id, @@ -132,7 +141,7 @@ export const SingleSignInPage = ({ if (retry) { login(); } - }, [onResult, authApi, retry]); + }, [onResult, authApi, retry, autoShowPopup]); return showLoginPage ? ( @@ -153,7 +162,10 @@ export const SingleSignInPage = ({