From 306e9542bbff9bd0ef551b0235f9f5a54e3707f2 Mon Sep 17 00:00:00 2001 From: Gao Sun Date: Wed, 10 Apr 2024 16:41:38 +0800 Subject: [PATCH 1/3] refactor(experience): fall back to sign-in page for edge cases --- .../pages/SocialSignInWebCallback/index.tsx | 2 +- .../use-single-sign-on-listener.ts | 14 +- .../use-social-sign-in-listener.ts | 16 +- .../tests/experience/direct-sign-in.test.ts | 25 ++- .../tests/experience/social-sign-in.test.ts | 163 ++++++++++++++++++ .../src/ui-helpers/expect-experience.ts | 5 +- .../src/ui-helpers/expect-page.ts | 12 +- packages/integration-tests/src/utils.ts | 3 + 8 files changed, 227 insertions(+), 13 deletions(-) create mode 100644 packages/integration-tests/src/tests/experience/social-sign-in.test.ts diff --git a/packages/experience/src/pages/SocialSignInWebCallback/index.tsx b/packages/experience/src/pages/SocialSignInWebCallback/index.tsx index d684d9f1ef0..29a11949f5f 100644 --- a/packages/experience/src/pages/SocialSignInWebCallback/index.tsx +++ b/packages/experience/src/pages/SocialSignInWebCallback/index.tsx @@ -21,7 +21,7 @@ const SocialSignInWebCallback = () => { } // Connector not found, return sign in page - return ; + return ; }; export default SocialSignInWebCallback; diff --git a/packages/experience/src/pages/SocialSignInWebCallback/use-single-sign-on-listener.ts b/packages/experience/src/pages/SocialSignInWebCallback/use-single-sign-on-listener.ts index 1e35b785c4f..00f7b88f87d 100644 --- a/packages/experience/src/pages/SocialSignInWebCallback/use-single-sign-on-listener.ts +++ b/packages/experience/src/pages/SocialSignInWebCallback/use-single-sign-on-listener.ts @@ -1,7 +1,7 @@ -import { SignInMode } from '@logto/schemas'; +import { SignInMode, experience } from '@logto/schemas'; import { useCallback, useEffect, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { useSearchParams } from 'react-router-dom'; +import { useNavigate, useSearchParams } from 'react-router-dom'; import { singleSignOnAuthorization, singleSignOnRegistration } from '@/apis/single-sign-on'; import useApi from '@/hooks/use-api'; @@ -16,11 +16,13 @@ const useSingleSignOnRegister = () => { const handleError = useErrorHandler(); const request = useApi(singleSignOnRegistration); const { termsValidation } = useTerms(); + const navigate = useNavigate(); return useCallback( async (connectorId: string) => { // Agree to terms and conditions first before proceeding if (!(await termsValidation())) { + navigate('/' + experience.routes.signIn); return; } @@ -36,7 +38,7 @@ const useSingleSignOnRegister = () => { window.location.replace(result.redirectTo); } }, - [handleError, request, termsValidation] + [handleError, navigate, request, termsValidation] ); }; @@ -59,6 +61,7 @@ const useSingleSignOnListener = (connectorId: string) => { const { signInMode } = useSieMethods(); const handleError = useErrorHandler(); + const navigate = useNavigate(); const singleSignOnAuthorizationRequest = useApi(singleSignOnAuthorization); const registerSingleSignOnIdentity = useSingleSignOnRegister(); @@ -78,7 +81,7 @@ const useSingleSignOnListener = (connectorId: string) => { // Should not let user register new social account under sign-in only mode if (signInMode === SignInMode.SignIn) { setToast(error.message); - + navigate('/' + experience.routes.signIn); return; } @@ -94,6 +97,7 @@ const useSingleSignOnListener = (connectorId: string) => { }, [ handleError, + navigate, registerSingleSignOnIdentity, setToast, signInMode, @@ -117,6 +121,7 @@ const useSingleSignOnListener = (connectorId: string) => { // Validate the state parameter if (!state || !stateValidation(state, connectorId)) { setToast(t('error.invalid_connector_auth')); + navigate('/' + experience.routes.signIn); return; } @@ -124,6 +129,7 @@ const useSingleSignOnListener = (connectorId: string) => { }, [ connectorId, isConsumed, + navigate, searchParameters, setSearchParameters, setToast, diff --git a/packages/experience/src/pages/SocialSignInWebCallback/use-social-sign-in-listener.ts b/packages/experience/src/pages/SocialSignInWebCallback/use-social-sign-in-listener.ts index e9ca9378b11..240d5c2a9be 100644 --- a/packages/experience/src/pages/SocialSignInWebCallback/use-social-sign-in-listener.ts +++ b/packages/experience/src/pages/SocialSignInWebCallback/use-social-sign-in-listener.ts @@ -1,5 +1,5 @@ import type { RequestErrorBody } from '@logto/schemas'; -import { SignInMode } from '@logto/schemas'; +import { SignInMode, experience } from '@logto/schemas'; import { useCallback, useEffect, useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { useNavigate, useSearchParams } from 'react-router-dom'; @@ -63,12 +63,13 @@ const useSocialSignInListener = (connectorId: string) => { // Should not let user register new social account under sign-in only mode if (signInMode === SignInMode.SignIn) { setToast(error.message); - + navigate('/' + experience.routes.signIn); return; } // Agree to terms and conditions first before proceeding if (!(await termsValidation())) { + navigate('/' + experience.routes.signIn); return; } @@ -76,7 +77,14 @@ const useSocialSignInListener = (connectorId: string) => { }, ...preSignInErrorHandler, }), - [preSignInErrorHandler, signInMode, termsValidation, accountNotExistErrorHandler, setToast] + [ + preSignInErrorHandler, + signInMode, + termsValidation, + accountNotExistErrorHandler, + setToast, + navigate, + ] ); const signInWithSocialHandler = useCallback( @@ -119,6 +127,7 @@ const useSocialSignInListener = (connectorId: string) => { if (!state || !stateValidation(state, connectorId)) { setToast(t('error.invalid_connector_auth')); + navigate('/' + experience.routes.signIn); return; } @@ -126,6 +135,7 @@ const useSocialSignInListener = (connectorId: string) => { }, [ connectorId, isConsumed, + navigate, searchParameters, setSearchParameters, setToast, diff --git a/packages/integration-tests/src/tests/experience/direct-sign-in.test.ts b/packages/integration-tests/src/tests/experience/direct-sign-in.test.ts index aa19dfff175..4bd3633c208 100644 --- a/packages/integration-tests/src/tests/experience/direct-sign-in.test.ts +++ b/packages/integration-tests/src/tests/experience/direct-sign-in.test.ts @@ -2,6 +2,7 @@ import crypto from 'node:crypto'; import { ConnectorType } from '@logto/connector-kit'; import { SignInIdentifier, SsoProviderName } from '@logto/schemas'; +import { appendPath } from '@silverhand/essentials'; import { mockSocialConnectorTarget } from '#src/__mocks__/connectors-mock.js'; import { updateSignInExperience } from '#src/api/sign-in-experience.js'; @@ -9,6 +10,7 @@ import { createSsoConnector } from '#src/api/sso-connector.js'; import { demoAppUrl, logtoUrl } from '#src/constants.js'; import { clearConnectorsByTypes, setSocialConnector } from '#src/helpers/connector.js'; import ExpectExperience from '#src/ui-helpers/expect-experience.js'; +import { dcls, dmodal } from '#src/utils.js'; const randomString = () => crypto.randomBytes(8).toString('hex'); @@ -40,6 +42,8 @@ describe('direct sign-in', () => { // eslint-disable-next-line @silverhand/fp/no-mutation context.ssoConnectorId = ssoConnector.id; await updateSignInExperience({ + termsOfUseUrl: 'https://example.com/terms', + privacyPolicyUrl: 'https://example.com/privacy', signUp: { identifiers: [], password: true, verify: false }, signIn: { methods: [ @@ -57,12 +61,26 @@ describe('direct sign-in', () => { }); it('should be landed to the social identity provider directly', async () => { + const socialUserId = 'foo_' + randomString(); const experience = new ExpectExperience(await browser.newPage()); const url = new URL(demoAppUrl); url.searchParams.set('direct_sign_in', `social:${mockSocialConnectorTarget}`); await experience.page.goto(url.href); - await experience.toProcessSocialSignIn({ socialUserId: 'foo', clickButton: false }); + await experience.toProcessSocialSignIn({ + socialUserId, + clickButton: false, + }); + + // Redirected back to the social callback page + experience.toMatchUrl(new RegExp(appendPath(new URL(logtoUrl), 'callback/social/.*').href)); + + // Should have popped up the terms of use and privacy policy dialog + await experience.toMatchElement([dmodal(), dcls('content')].join(' '), { + text: /terms of use/i, + }); + await experience.toClickButton(/agree/i); + experience.toMatchUrl(demoAppUrl); await experience.toClick('div[role=button]', /sign out/i); await experience.page.close(); @@ -71,11 +89,12 @@ describe('direct sign-in', () => { it('should be landed to the sso identity provider directly', async () => { const experience = new ExpectExperience(await browser.newPage()); const url = new URL(demoAppUrl); + const socialUserId = 'foo_' + randomString(); url.searchParams.set('direct_sign_in', `sso:${context.ssoConnectorId!}`); await experience.page.goto(url.href); await experience.toProcessSocialSignIn({ - socialUserId: 'foo', + socialUserId, clickButton: false, authUrl: ssoOidcIssuer + '/auth', }); @@ -84,7 +103,7 @@ describe('direct sign-in', () => { // with the code and user ID in the query string. const callbackUrl = new URL(experience.page.url()); expect(callbackUrl.searchParams.get('code')).toBe('mock-code'); - expect(callbackUrl.searchParams.get('userId')).toBe('foo'); + expect(callbackUrl.searchParams.get('userId')).toBe(socialUserId); expect(new URL(callbackUrl.pathname, callbackUrl.origin).href).toBe(demoAppUrl.href); await experience.page.close(); diff --git a/packages/integration-tests/src/tests/experience/social-sign-in.test.ts b/packages/integration-tests/src/tests/experience/social-sign-in.test.ts new file mode 100644 index 00000000000..9f89563b2db --- /dev/null +++ b/packages/integration-tests/src/tests/experience/social-sign-in.test.ts @@ -0,0 +1,163 @@ +import crypto from 'node:crypto'; + +import { ConnectorType } from '@logto/connector-kit'; +import { SignInIdentifier, SignInMode, SsoProviderName } from '@logto/schemas'; +import { appendPath } from '@silverhand/essentials'; + +import { updateSignInExperience } from '#src/api/sign-in-experience.js'; +import { createSsoConnector } from '#src/api/sso-connector.js'; +import { demoAppUrl, logtoUrl } from '#src/constants.js'; +import { + clearConnectorsByTypes, + setEmailConnector, + setSocialConnector, +} from '#src/helpers/connector.js'; +import ExpectExperience from '#src/ui-helpers/expect-experience.js'; +import { dcls, dmodal, generateEmail } from '#src/utils.js'; + +const randomString = () => crypto.randomBytes(8).toString('hex'); + +/** + * NOTE: This test suite assumes test cases will run sequentially (which is Jest default). + * Parallel execution will lead to errors. + */ +// Tip: See https://github.com/argos-ci/jest-puppeteer/blob/main/packages/expect-puppeteer/README.md +// for convenient expect methods +describe('social sign-in (with email identifier)', () => { + const context = new (class Context { + ssoConnectorId?: string; + })(); + const ssoOidcIssuer = `${logtoUrl}/oidc`; + // eslint-disable-next-line @silverhand/fp/no-let + let experience: ExpectExperience; + const socialUserId = 'foo_' + randomString(); + + beforeAll(async () => { + await clearConnectorsByTypes([ConnectorType.Social, ConnectorType.Email, ConnectorType.Sms]); + await setEmailConnector(); + await setSocialConnector(); + const ssoConnector = await createSsoConnector({ + providerName: SsoProviderName.OIDC, + connectorName: 'test-oidc-' + randomString(), + domains: [`foo${randomString()}.com`], + config: { + clientId: 'foo', + clientSecret: 'bar', + issuer: ssoOidcIssuer, + }, + }); + // eslint-disable-next-line @silverhand/fp/no-mutation + context.ssoConnectorId = ssoConnector.id; + await updateSignInExperience({ + termsOfUseUrl: 'https://example.com/terms', + privacyPolicyUrl: 'https://example.com/privacy', + signUp: { identifiers: [SignInIdentifier.Email], password: true, verify: true }, + signIn: { + methods: [ + { + identifier: SignInIdentifier.Username, + password: true, + verificationCode: false, + isPasswordPrimary: true, + }, + { + identifier: SignInIdentifier.Email, + password: true, + verificationCode: false, + isPasswordPrimary: true, + }, + ], + }, + signInMode: SignInMode.SignIn, + singleSignOnEnabled: true, + socialSignInConnectorTargets: ['mock-social'], + }); + }); + + it('should be able to start the social sign-in flow', async () => { + // eslint-disable-next-line @silverhand/fp/no-mutation + experience = new ExpectExperience(await browser.newPage()); + await experience.startWith(demoAppUrl, 'sign-in'); + await experience.toProcessSocialSignIn({ + socialUserId, + }); + + // Registration disabled, should be redirected back to the sign-in page + await experience.waitForToast('not been registered yet'); + experience.toBeAt('sign-in'); + }); + + it('should be able to cancel (disagree) the terms of use', async () => { + await updateSignInExperience({ signInMode: SignInMode.SignInAndRegister }); + await experience.toProcessSocialSignIn({ + socialUserId, + }); + // Redirected back to the social callback page + experience.toMatchUrl(new RegExp(appendPath(new URL(logtoUrl), 'callback/social/.*').href)); + + // Should have popped up the terms of use and privacy policy dialog + await experience.toMatchElement([dmodal(), dcls('content')].join(' '), { + text: /terms of use/i, + }); + await experience.toClickButton(/cancel/i); + experience.toBeAt('sign-in'); + }); + + it('should be able to start the social sign-in flow again if state mismatch', async () => { + await experience.toProcessSocialSignIn({ + socialUserId, + state: '', // Overriding the state to cause a mismatch + }); + await experience.waitForToast(/invalid/); + experience.toBeAt('sign-in'); + }); + + it('should be able to start the social sign-in flow again', async () => { + await experience.toProcessSocialSignIn({ + socialUserId, + }); + + // Redirected back to the social callback page + experience.toMatchUrl(new RegExp(appendPath(new URL(logtoUrl), 'callback/social/.*').href)); + + // Should have popped up the terms of use and privacy policy dialog + await experience.toMatchElement([dmodal(), dcls('content')].join(' '), { + text: /terms of use/i, + }); + await experience.toClickButton(/agree/i); + }); + + it('should be able to verify the required email address', async () => { + await experience.toFillInput('identifier', generateEmail(), { submit: true }); + await experience.toCompleteVerification('continue', 'Email'); + + await experience.verifyThenEnd(); + }); + + it('should directly sign in for existing users without pop-up terms of use', async () => { + // eslint-disable-next-line @silverhand/fp/no-mutation + experience = new ExpectExperience(await browser.newPage()); + await experience.startWith(demoAppUrl, 'sign-in'); + await experience.toProcessSocialSignIn({ + socialUserId, + }); + await experience.verifyThenEnd(); + }); + + it('should directly sign in for new users when terms of use has not been set', async () => { + await updateSignInExperience({ + termsOfUseUrl: '', + privacyPolicyUrl: '', + }); + + // eslint-disable-next-line @silverhand/fp/no-mutation + experience = new ExpectExperience(await browser.newPage()); + await experience.startWith(demoAppUrl, 'sign-in'); + await experience.toProcessSocialSignIn({ + socialUserId: 'bar_' + randomString(), + }); + await experience.toFillInput('identifier', generateEmail(), { submit: true }); + await experience.toCompleteVerification('continue', 'Email'); + await experience.verifyThenEnd(); + }); +}); diff --git a/packages/integration-tests/src/ui-helpers/expect-experience.ts b/packages/integration-tests/src/ui-helpers/expect-experience.ts index ba541016717..c6588b4bda8 100644 --- a/packages/integration-tests/src/ui-helpers/expect-experience.ts +++ b/packages/integration-tests/src/ui-helpers/expect-experience.ts @@ -249,6 +249,7 @@ export default class ExpectExperience extends ExpectPage { socialPhone, clickButton = true, authUrl = mockSocialAuthPageUrl, + state: stateOverride, }: { socialUserId: string; socialEmail?: string; @@ -257,6 +258,8 @@ export default class ExpectExperience extends ExpectPage { clickButton?: boolean; /** The URL to wait for the social auth page. */ authUrl?: string; + /** The state parameter to override. */ + state?: string; }) { const authPageRequestListener = this.page.waitForRequest((request) => request.url().startsWith(authUrl) @@ -270,7 +273,7 @@ export default class ExpectExperience extends ExpectPage { const { searchParams: authSearchParams } = new URL(result.url()); const redirectUri = authSearchParams.get('redirect_uri') ?? ''; - const state = authSearchParams.get('state') ?? ''; + const state = stateOverride ?? authSearchParams.get('state') ?? ''; // Mock social redirects const callbackUrl = new URL(redirectUri); diff --git a/packages/integration-tests/src/ui-helpers/expect-page.ts b/packages/integration-tests/src/ui-helpers/expect-page.ts index 2460f871327..94f82794c31 100644 --- a/packages/integration-tests/src/ui-helpers/expect-page.ts +++ b/packages/integration-tests/src/ui-helpers/expect-page.ts @@ -53,7 +53,7 @@ export default class ExpectPage { * @param shouldNavigate Whether the click should trigger a navigation. Defaults to `true`. * @see {@link ExpectPage.toClick} */ - async toClickButton(text: string, shouldNavigate = true) { + async toClickButton(text: string | RegExp, shouldNavigate = true) { return this.toClick('button', text, shouldNavigate); } @@ -136,6 +136,16 @@ export default class ExpectPage { return expect(this.page).toMatchElement('*[role=alert]', { text }); } + /** + * Expect the page to match an element with the given selector and text. + * + * @alias `expect(this.page).toMatchElement()` + * @see {@link jest.Matchers.toMatchElement} + */ + async toMatchElement(...args: Parameters['toMatchElement']>) { + return expect(this.page).toMatchElement(...args); + } + /** * Expect the page's URL to match the given URL. * diff --git a/packages/integration-tests/src/utils.ts b/packages/integration-tests/src/utils.ts index 259aede9111..d1e3e000158 100644 --- a/packages/integration-tests/src/utils.ts +++ b/packages/integration-tests/src/utils.ts @@ -113,6 +113,9 @@ export const cls = (className: C) => `[class*=_${className}]` */ export const dcls = (className: C) => `div${cls(className)}` as const; +/** Build the string for a CSS selector that matches a `
` element with `aria-modal=true`. */ +export const dmodal = () => `div[aria-modal=true]` as const; + /** * Generate a random test name that starts with `test_` and followed by 4 random characters. * From 04d71802a929716ad71a76f1ab35aff41fe6e09c Mon Sep 17 00:00:00 2001 From: Gao Sun Date: Wed, 10 Apr 2024 17:06:02 +0800 Subject: [PATCH 2/3] refactor: add unit tests --- .../SocialSignInWebCallback/index.test.tsx | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/experience/src/pages/SocialSignInWebCallback/index.test.tsx b/packages/experience/src/pages/SocialSignInWebCallback/index.test.tsx index e31463c9d7e..8f31efccae8 100644 --- a/packages/experience/src/pages/SocialSignInWebCallback/index.test.tsx +++ b/packages/experience/src/pages/SocialSignInWebCallback/index.test.tsx @@ -1,5 +1,5 @@ import { waitFor } from '@testing-library/react'; -import { Route, Routes, useSearchParams } from 'react-router-dom'; +import { Navigate, Route, Routes, useSearchParams } from 'react-router-dom'; import renderWithPageContext from '@/__mocks__/RenderWithPageContext'; import SettingsProvider from '@/__mocks__/RenderWithPageContext/SettingsProvider'; @@ -28,11 +28,33 @@ jest.mock('@/apis/single-sign-on', () => ({ jest.mock('react-router-dom', () => ({ ...jest.requireActual('react-router-dom'), useSearchParams: jest.fn(), + Navigate: jest.fn(() => null), })); const mockUseSearchParameters = useSearchParams as jest.Mock; +const mockNavigate = Navigate as jest.Mock; describe('SocialCallbackPage with code', () => { + describe('fallback', () => { + it('should redirect to /sign-in if connectorId is not found', async () => { + mockUseSearchParameters.mockReturnValue([new URLSearchParams('code=foo'), jest.fn()]); + + renderWithPageContext( + + + } /> + + , + { initialEntries: ['/sign-in/social/invalid'] } + ); + + await waitFor(() => { + expect(signInWithSocial).not.toBeCalled(); + expect(mockNavigate.mock.calls[0][0].to).toBe('/sign-in'); + }); + }); + }); + describe('signIn with social', () => { const connectorId = socialConnectors[0]!.id; const state = generateState(); From 39b4ffc5781ecd8c72ee9cb4332a46e1968ad4b7 Mon Sep 17 00:00:00 2001 From: Gao Sun Date: Wed, 10 Apr 2024 19:22:22 +0800 Subject: [PATCH 3/3] refactor: fix tests --- packages/integration-tests/src/ui-helpers/index.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/integration-tests/src/ui-helpers/index.ts b/packages/integration-tests/src/ui-helpers/index.ts index 82a1b879bc3..e2337658ddd 100644 --- a/packages/integration-tests/src/ui-helpers/index.ts +++ b/packages/integration-tests/src/ui-helpers/index.ts @@ -160,6 +160,8 @@ export const setupUsernameAndEmailExperience = async (passwordPolicy?: PartialPa await clearConnectorsByTypes([ConnectorType.Email, ConnectorType.Sms]); await setEmailConnector(); await updateSignInExperience({ + termsOfUseUrl: '', + privacyPolicyUrl: '', signInMode: SignInMode.SignInAndRegister, signUp: { identifiers: [SignInIdentifier.Username],