From 18435f75ffc71b099a302a0e91bce91ea331fdb3 Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Tue, 30 Jul 2024 16:42:19 -0500 Subject: [PATCH 1/5] Support transferable prop on SignIn --- packages/clerk-js/src/core/clerk.ts | 2 +- packages/clerk-js/src/core/constants.ts | 3 ++- .../clerk-js/src/ui/components/SignIn/SignIn.tsx | 1 + .../src/ui/components/SignIn/SignInStart.tsx | 10 ++++++++-- .../src/ui/contexts/ClerkUIComponentsContext.tsx | 2 ++ packages/types/src/clerk.ts | 12 ++++++++++++ 6 files changed, 26 insertions(+), 4 deletions(-) diff --git a/packages/clerk-js/src/core/clerk.ts b/packages/clerk-js/src/core/clerk.ts index 71ceb63e98e..b99d73b4ab4 100644 --- a/packages/clerk-js/src/core/clerk.ts +++ b/packages/clerk-js/src/core/clerk.ts @@ -1194,7 +1194,7 @@ export class Clerk implements ClerkInterface { return navigateToResetPassword(); } - const userNeedsToBeCreated = si.firstFactorVerificationStatus === 'transferable'; + const userNeedsToBeCreated = si.firstFactorVerificationStatus === 'transferable' && params.transferable; if (userNeedsToBeCreated) { const res = await signUp.create({ transfer: true }); diff --git a/packages/clerk-js/src/core/constants.ts b/packages/clerk-js/src/core/constants.ts index d87856fa4f9..82a8c3f3207 100644 --- a/packages/clerk-js/src/core/constants.ts +++ b/packages/clerk-js/src/core/constants.ts @@ -22,7 +22,8 @@ export const ERROR_CODES = { NOT_ALLOWED_ACCESS: 'not_allowed_access', SAML_USER_ATTRIBUTE_MISSING: 'saml_user_attribute_missing', USER_LOCKED: 'user_locked', -}; + EXTERNAL_ACCOUNT_NOT_FOUND: 'external_account_not_found', +} as const; export const SIGN_IN_INITIAL_VALUE_KEYS = ['email_address', 'phone_number', 'username']; export const SIGN_UP_INITIAL_VALUE_KEYS = ['email_address', 'phone_number', 'username', 'first_name', 'last_name']; diff --git a/packages/clerk-js/src/ui/components/SignIn/SignIn.tsx b/packages/clerk-js/src/ui/components/SignIn/SignIn.tsx index be5c48950f3..0f4287db471 100644 --- a/packages/clerk-js/src/ui/components/SignIn/SignIn.tsx +++ b/packages/clerk-js/src/ui/components/SignIn/SignIn.tsx @@ -47,6 +47,7 @@ function SignInRoutes(): JSX.Element { signInForceRedirectUrl={signInContext.afterSignInUrl} signUpForceRedirectUrl={signInContext.afterSignUpUrl} continueSignUpUrl={signInContext.signUpContinueUrl} + transferable={signInContext.transferable} firstFactorUrl={'../factor-one'} secondFactorUrl={'../factor-two'} resetPasswordUrl={'../reset-password'} diff --git a/packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx b/packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx index cdc4a70e962..754432c67bd 100644 --- a/packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx +++ b/packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx @@ -205,6 +205,11 @@ export function _SignInStart(): JSX.Element { useEffect(() => { async function handleOauthError() { + const defaultErrorHandler = () => { + // Error from server may be too much information for the end user, so set a generic error + card.setError('Unable to complete action at this time. If the problem persists please contact support.'); + }; + const error = signIn?.firstFactorVerification?.error; if (error) { switch (error.code) { @@ -214,12 +219,13 @@ export function _SignInStart(): JSX.Element { case ERROR_CODES.SAML_USER_ATTRIBUTE_MISSING: case ERROR_CODES.OAUTH_EMAIL_DOMAIN_RESERVED_BY_SAML: case ERROR_CODES.USER_LOCKED: + case ERROR_CODES.EXTERNAL_ACCOUNT_NOT_FOUND: card.setError(error); break; default: - // Error from server may be too much information for the end user, so set a generic error - card.setError('Unable to complete action at this time. If the problem persists please contact support.'); + defaultErrorHandler(); } + // TODO: This is a workaround in order to reset the sign in attempt // so that the oauth error does not persist on full page reloads. void (await signIn.create({})); diff --git a/packages/clerk-js/src/ui/contexts/ClerkUIComponentsContext.tsx b/packages/clerk-js/src/ui/contexts/ClerkUIComponentsContext.tsx index 72853b0758b..a685644f122 100644 --- a/packages/clerk-js/src/ui/contexts/ClerkUIComponentsContext.tsx +++ b/packages/clerk-js/src/ui/contexts/ClerkUIComponentsContext.tsx @@ -123,6 +123,7 @@ export type SignInContextType = SignInCtx & { authQueryString: string | null; afterSignUpUrl: string; afterSignInUrl: string; + transferable: boolean; }; export const useSignInContext = (): SignInContextType => { @@ -171,6 +172,7 @@ export const useSignInContext = (): SignInContextType => { return { ...ctx, + transferable: ctx.transferable ?? true, componentName, signUpUrl, signInUrl, diff --git a/packages/types/src/clerk.ts b/packages/types/src/clerk.ts index 62ce64ccb46..faf7a39717c 100644 --- a/packages/types/src/clerk.ts +++ b/packages/types/src/clerk.ts @@ -540,6 +540,12 @@ export type HandleOAuthCallbackParams = SignInForceRedirectUrl & * Full URL or path to navigate after requesting phone verification. */ verifyPhoneNumberUrl?: string | null; + /** + * Indicates whether or not sign in attempts are transferable to the sign up flow. + * Prevents opaque sign ups when a user attempts to sign in via OAuth with an email that doesn't exist. + * @default true + */ + transferable?: boolean; }; export type HandleSamlCallbackParams = HandleOAuthCallbackParams; @@ -728,6 +734,12 @@ export type SignInProps = RoutingOptions & { * Initial values that are used to prefill the sign in form. */ initialValues?: SignInInitialValues; + /** + * Indicates whether or not sign in attempts are transferable to the sign up flow. + * Prevents opaque sign ups when a user attempts to sign in via OAuth with an email that doesn't exist. + * @default true + */ + transferable?: boolean; } & SignUpForceRedirectUrl & SignUpFallbackRedirectUrl & LegacyRedirectProps & From 97b12b9d7133924ea9b77a3014318db66c58ea38 Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Tue, 30 Jul 2024 16:54:27 -0500 Subject: [PATCH 2/5] adds changeset --- .changeset/gold-emus-talk.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/gold-emus-talk.md diff --git a/.changeset/gold-emus-talk.md b/.changeset/gold-emus-talk.md new file mode 100644 index 00000000000..e801cc69ca5 --- /dev/null +++ b/.changeset/gold-emus-talk.md @@ -0,0 +1,7 @@ +--- +"@clerk/clerk-js": minor +"@clerk/types": minor +"@clerk/clerk-react": minor +--- + +Introduce `transferable` prop for `` to disable the automatic transfer of a sign in attempt to a sign up. From b054afb821e0d5072a334dc99d114c44c7330ab7 Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Tue, 6 Aug 2024 12:36:44 -0500 Subject: [PATCH 3/5] Add external_account_not_found to UnstableErrors type --- packages/types/src/clerk.ts | 21 ++++++++++----------- packages/types/src/localization.ts | 1 + 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/types/src/clerk.ts b/packages/types/src/clerk.ts index faf7a39717c..e53e3052c1a 100644 --- a/packages/types/src/clerk.ts +++ b/packages/types/src/clerk.ts @@ -500,7 +500,8 @@ export interface Clerk { handleUnauthenticated: () => Promise; } -export type HandleOAuthCallbackParams = SignInForceRedirectUrl & +export type HandleOAuthCallbackParams = TransferableOption & + SignInForceRedirectUrl & SignInFallbackRedirectUrl & SignUpForceRedirectUrl & SignUpFallbackRedirectUrl & @@ -540,12 +541,6 @@ export type HandleOAuthCallbackParams = SignInForceRedirectUrl & * Full URL or path to navigate after requesting phone verification. */ verifyPhoneNumberUrl?: string | null; - /** - * Indicates whether or not sign in attempts are transferable to the sign up flow. - * Prevents opaque sign ups when a user attempts to sign in via OAuth with an email that doesn't exist. - * @default true - */ - transferable?: boolean; }; export type HandleSamlCallbackParams = HandleOAuthCallbackParams; @@ -734,16 +729,20 @@ export type SignInProps = RoutingOptions & { * Initial values that are used to prefill the sign in form. */ initialValues?: SignInInitialValues; +} & TransferableOption & + SignUpForceRedirectUrl & + SignUpFallbackRedirectUrl & + LegacyRedirectProps & + AfterSignOutUrl; + +interface TransferableOption { /** * Indicates whether or not sign in attempts are transferable to the sign up flow. * Prevents opaque sign ups when a user attempts to sign in via OAuth with an email that doesn't exist. * @default true */ transferable?: boolean; -} & SignUpForceRedirectUrl & - SignUpFallbackRedirectUrl & - LegacyRedirectProps & - AfterSignOutUrl; +} export type SignInModalProps = WithoutRouting; diff --git a/packages/types/src/localization.ts b/packages/types/src/localization.ts index a1b2ae02666..712a68f7615 100644 --- a/packages/types/src/localization.ts +++ b/packages/types/src/localization.ts @@ -731,6 +731,7 @@ type _LocalizationResource = { type WithParamName = T & Partial>}`, LocalizationValue>>; type UnstableErrors = WithParamName<{ + external_account_not_found: LocalizationValue; identification_deletion_failed: LocalizationValue; phone_number_exists: LocalizationValue; form_identifier_not_found: LocalizationValue; From 7d4fc4176e51e72ca64ec293bd61632d70d5bff1 Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Tue, 6 Aug 2024 13:24:40 -0500 Subject: [PATCH 4/5] Adds a test case --- .../clerk-js/src/core/__tests__/clerk.test.ts | 86 ++++++++++++++++--- packages/clerk-js/src/core/clerk.ts | 6 +- 2 files changed, 78 insertions(+), 14 deletions(-) diff --git a/packages/clerk-js/src/core/__tests__/clerk.test.ts b/packages/clerk-js/src/core/__tests__/clerk.test.ts index 33971961675..5d2ef389795 100644 --- a/packages/clerk-js/src/core/__tests__/clerk.test.ts +++ b/packages/clerk-js/src/core/__tests__/clerk.test.ts @@ -183,7 +183,7 @@ describe('Clerk singleton', () => { await sut.setActive({ session: null }); await waitFor(() => { expect(mockSession.touch).not.toHaveBeenCalled(); - expect(evenBusSpy).toBeCalledWith('token:update', { token: null }); + expect(evenBusSpy).toHaveBeenCalledWith('token:update', { token: null }); }); }); @@ -207,7 +207,7 @@ describe('Clerk singleton', () => { await sut.setActive({ session: mockSession as any as ActiveSessionResource }); await waitFor(() => { expect(mockSession.touch).not.toHaveBeenCalled(); - expect(mockSession.getToken).toBeCalled(); + expect(mockSession.getToken).toHaveBeenCalled(); }); }); @@ -308,7 +308,7 @@ describe('Clerk singleton', () => { expect(executionOrder).toEqual(['session.touch', 'set cookie', 'before emit']); expect(mockSession2.touch).toHaveBeenCalled(); expect(mockSession2.getToken).toHaveBeenCalled(); - expect(beforeEmitMock).toBeCalledWith(mockSession2); + expect(beforeEmitMock).toHaveBeenCalledWith(mockSession2); expect(sut.session).toMatchObject(mockSession2); }); }); @@ -342,7 +342,7 @@ describe('Clerk singleton', () => { expect(mockSession.touch).toHaveBeenCalled(); expect(mockSession.getToken).toHaveBeenCalled(); expect((mockSession as any as ActiveSessionResource)?.lastActiveOrganizationId).toEqual('org-id'); - expect(beforeEmitMock).toBeCalledWith(mockSession); + expect(beforeEmitMock).toHaveBeenCalledWith(mockSession); expect(sut.session).toMatchObject(mockSession); }); }); @@ -370,8 +370,8 @@ describe('Clerk singleton', () => { expect(executionOrder).toEqual(['session.touch', 'before emit']); expect(mockSession.touch).toHaveBeenCalled(); expect((mockSession as any as ActiveSessionResource)?.lastActiveOrganizationId).toEqual('org-id'); - expect(mockSession.getToken).toBeCalled(); - expect(beforeEmitMock).toBeCalledWith(mockSession); + expect(mockSession.getToken).toHaveBeenCalled(); + expect(beforeEmitMock).toHaveBeenCalledWith(mockSession); expect(sut.session).toMatchObject(mockSession); }); }); @@ -581,7 +581,7 @@ describe('Clerk singleton', () => { const toUrl = 'http://test.host/'; await sut.navigate(toUrl); expect(mockHref).toHaveBeenCalledWith(toUrl); - expect(logSpy).not.toBeCalled(); + expect(logSpy).not.toHaveBeenCalled(); }); it('uses window location if a custom navigate is defined but destination has different origin', async () => { @@ -589,7 +589,7 @@ describe('Clerk singleton', () => { const toUrl = 'https://www.origindifferent.com/'; await sut.navigate(toUrl); expect(mockHref).toHaveBeenCalledWith(toUrl); - expect(logSpy).not.toBeCalled(); + expect(logSpy).not.toHaveBeenCalled(); }); it('wraps custom navigate method in a promise if provided and it sync', async () => { @@ -599,7 +599,7 @@ describe('Clerk singleton', () => { expect(res.then).toBeDefined(); expect(mockHref).not.toHaveBeenCalled(); expect(mockNavigate.mock.calls[0][0]).toBe('/path#hash'); - expect(logSpy).not.toBeCalled(); + expect(logSpy).not.toHaveBeenCalled(); }); it('logs navigation external navigation when routerDebug is enabled', async () => { @@ -608,8 +608,8 @@ describe('Clerk singleton', () => { await sut.navigate(toUrl); expect(mockHref).toHaveBeenCalledWith(toUrl); - expect(logSpy).toBeCalledTimes(1); - expect(logSpy).toBeCalledWith(`Clerk is navigating to: ${toUrl}`); + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith(`Clerk is navigating to: ${toUrl}`); }); it('logs navigation custom navigation when routerDebug is enabled', async () => { @@ -620,8 +620,8 @@ describe('Clerk singleton', () => { expect(mockHref).not.toHaveBeenCalled(); expect(mockNavigate.mock.calls[0][0]).toBe('/path#hash'); - expect(logSpy).toBeCalledTimes(1); - expect(logSpy).toBeCalledWith(`Clerk is navigating to: ${toUrl}`); + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith(`Clerk is navigating to: ${toUrl}`); }); }); @@ -690,6 +690,66 @@ describe('Clerk singleton', () => { }); }); + it('does not initiate the transfer flow when transferable: false is passed', async () => { + mockEnvironmentFetch.mockReturnValue( + Promise.resolve({ + authConfig: {}, + userSettings: mockUserSettings, + displayConfig: mockDisplayConfig, + isSingleSession: () => false, + isProduction: () => false, + isDevelopmentOrStaging: () => true, + onWindowLocationHost: () => false, + }), + ); + + mockClientFetch.mockReturnValue( + Promise.resolve({ + activeSessions: [], + signIn: new SignIn({ + status: 'needs_identifier', + first_factor_verification: { + status: 'transferable', + strategy: 'oauth_google', + external_verification_redirect_url: '', + error: { + code: 'external_account_not_found', + long_message: 'The External Account was not found.', + message: 'Invalid external account', + }, + }, + second_factor_verification: null, + identifier: '', + user_data: null, + created_session_id: null, + created_user_id: null, + } as any as SignInJSON), + signUp: new SignUp(null), + }), + ); + + const mockSetActive = jest.fn(); + const mockSignUpCreate = jest + .fn() + .mockReturnValue(Promise.resolve({ status: 'complete', createdSessionId: '123' })); + + const sut = new Clerk(productionPublishableKey); + await sut.load(mockedLoadOptions); + if (!sut.client) { + fail('we should always have a client'); + } + sut.client.signUp.create = mockSignUpCreate; + sut.setActive = mockSetActive; + + await sut.handleRedirectCallback({ transferable: false }); + + await waitFor(() => { + expect(mockSignUpCreate).not.toHaveBeenCalledWith({ transfer: true }); + expect(mockSetActive).not.toHaveBeenCalled(); + expect(mockNavigate).toHaveBeenCalledWith('/sign-in', undefined); + }); + }); + it('creates a new sign up and navigates to the continue sign-up path if the user was not found during sso signup and there are missing requirements', async () => { mockEnvironmentFetch.mockReturnValue( Promise.resolve({ diff --git a/packages/clerk-js/src/core/clerk.ts b/packages/clerk-js/src/core/clerk.ts index b99d73b4ab4..95e9f2ff84b 100644 --- a/packages/clerk-js/src/core/clerk.ts +++ b/packages/clerk-js/src/core/clerk.ts @@ -1194,9 +1194,13 @@ export class Clerk implements ClerkInterface { return navigateToResetPassword(); } - const userNeedsToBeCreated = si.firstFactorVerificationStatus === 'transferable' && params.transferable; + const userNeedsToBeCreated = si.firstFactorVerificationStatus === 'transferable'; if (userNeedsToBeCreated) { + if (params.transferable === false) { + return navigateToSignIn(); + } + const res = await signUp.create({ transfer: true }); switch (res.status) { case 'complete': From 36415526557bff2af00dba8984d8fd709ab17ece Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Tue, 6 Aug 2024 16:16:16 -0500 Subject: [PATCH 5/5] Update .changeset/gold-emus-talk.md --- .changeset/gold-emus-talk.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/gold-emus-talk.md b/.changeset/gold-emus-talk.md index e801cc69ca5..8bc4292897b 100644 --- a/.changeset/gold-emus-talk.md +++ b/.changeset/gold-emus-talk.md @@ -4,4 +4,4 @@ "@clerk/clerk-react": minor --- -Introduce `transferable` prop for `` to disable the automatic transfer of a sign in attempt to a sign up. +Introduce `transferable` prop for `` to disable the automatic transfer of a sign in attempt to a sign up attempt when attempting to sign in with a social provider when the account does not exist. Also adds a `transferable` option to `Clerk.handleRedirectCallback()` with the same functionality.