From a1f6009cc5f7f1c4ef6fd3e02f3216386a609075 Mon Sep 17 00:00:00 2001 From: simeng-li Date: Thu, 1 Aug 2024 17:17:51 +0800 Subject: [PATCH] refactor(core): make password optional in NewPasswordIdentity (#6377) refactor(core): make password optional in NewPasswordIdentity verification make password optioanl in NewPasswordIdentity verification --- .../classes/experience-interaction.ts | 2 - .../sign-in-experience-validator.test.ts | 200 ++++++------------ .../libraries/sign-in-experience-validator.ts | 24 +-- .../new-password-identity-verification.ts | 11 +- .../new-password-identity-verification.ts | 7 +- .../src/client/experience/index.ts | 4 +- .../verification-code.test.ts | 68 +++++- ...new-password-identity-verification.test.ts | 21 ++ 8 files changed, 170 insertions(+), 167 deletions(-) diff --git a/packages/core/src/routes/experience/classes/experience-interaction.ts b/packages/core/src/routes/experience/classes/experience-interaction.ts index bd55f296a..1af2e005c 100644 --- a/packages/core/src/routes/experience/classes/experience-interaction.ts +++ b/packages/core/src/routes/experience/classes/experience-interaction.ts @@ -483,8 +483,6 @@ export default class ExperienceInteraction { const newProfile = await getNewUserProfileFromVerificationRecord(verificationRecord); await this.profile.profileValidator.guardProfileUniquenessAcrossUsers(newProfile); - await this.signInExperienceValidator.guardMandatoryPasswordOnRegister(verificationRecord); - const user = await this.provisionLibrary.createUser(newProfile); this.userId = user.id; diff --git a/packages/core/src/routes/experience/classes/libraries/sign-in-experience-validator.test.ts b/packages/core/src/routes/experience/classes/libraries/sign-in-experience-validator.test.ts index 9ee07d4e4..e3971e829 100644 --- a/packages/core/src/routes/experience/classes/libraries/sign-in-experience-validator.test.ts +++ b/packages/core/src/routes/experience/classes/libraries/sign-in-experience-validator.test.ts @@ -1,4 +1,5 @@ /* eslint-disable max-lines */ +import { type LogtoErrorCode } from '@logto/phrases'; import { InteractionEvent, type SignInExperience, @@ -339,7 +340,11 @@ describe('SignInExperienceValidator', () => { string, { signInExperience: SignInExperience; - cases: Array<{ verificationRecord: VerificationRecord; accepted: boolean }>; + cases: Array<{ + verificationRecord: VerificationRecord; + accepted: boolean; + errorCode?: LogtoErrorCode; + }>; } > = Object.freeze({ 'only username is enabled for sign-up': { @@ -364,7 +369,7 @@ describe('SignInExperienceValidator', () => { ...mockSignInExperience, signUp: { identifiers: [SignInIdentifier.Email], - password: true, + password: false, verify: true, }, }, @@ -377,6 +382,10 @@ describe('SignInExperienceValidator', () => { verificationRecord: verificationCodeVerificationRecords[SignInIdentifier.Phone], accepted: false, }, + { + verificationRecord: emailNewPasswordIdentityVerificationRecord, + accepted: false, + }, ], }, 'email and phone are enabled for sign-up': { @@ -384,7 +393,7 @@ describe('SignInExperienceValidator', () => { ...mockSignInExperience, signUp: { identifiers: [SignInIdentifier.Email, SignInIdentifier.Phone], - password: true, + password: false, verify: true, }, }, @@ -399,6 +408,27 @@ describe('SignInExperienceValidator', () => { }, ], }, + 'email are enabled for sign-up but password is required': { + signInExperience: { + ...mockSignInExperience, + signUp: { + identifiers: [SignInIdentifier.Email], + password: true, + verify: true, + }, + }, + cases: [ + { + verificationRecord: verificationCodeVerificationRecords[SignInIdentifier.Email], + accepted: false, + errorCode: 'user.password_required_in_profile', + }, + { + verificationRecord: emailNewPasswordIdentityVerificationRecord, + accepted: true, + }, + ], + }, 'enterprise sso enabled': { signInExperience: { ...mockSignInExperience, @@ -428,30 +458,36 @@ describe('SignInExperienceValidator', () => { describe.each(Object.keys(registerVerificationTestCases))(`%s`, (testCase) => { const { signInExperience, cases } = registerVerificationTestCases[testCase]!; - it.each(cases)('guard verification record %p', async ({ verificationRecord, accepted }) => { - signInExperiences.findDefaultSignInExperience.mockResolvedValueOnce(signInExperience); + it.each(cases)( + 'guard verification record %p', + async ({ verificationRecord, accepted, errorCode }) => { + signInExperiences.findDefaultSignInExperience.mockResolvedValueOnce(signInExperience); - const signInExperienceSettings = new SignInExperienceValidator( - mockTenant.libraries, - mockTenant.queries - ); + const signInExperienceSettings = new SignInExperienceValidator( + mockTenant.libraries, + mockTenant.queries + ); - await (accepted - ? expect( - signInExperienceSettings.verifyIdentificationMethod( - InteractionEvent.Register, - verificationRecord - ) - ).resolves.not.toThrow() - : expect( - signInExperienceSettings.verifyIdentificationMethod( - InteractionEvent.Register, - verificationRecord - ) - ).rejects.toMatchError( - new RequestError({ code: 'user.sign_up_method_not_enabled', status: 422 }) - )); - }); + await (accepted + ? expect( + signInExperienceSettings.verifyIdentificationMethod( + InteractionEvent.Register, + verificationRecord + ) + ).resolves.not.toThrow() + : expect( + signInExperienceSettings.verifyIdentificationMethod( + InteractionEvent.Register, + verificationRecord + ) + ).rejects.toMatchError( + new RequestError({ + code: errorCode ?? 'user.sign_up_method_not_enabled', + status: 422, + }) + )); + } + ); }); }); @@ -518,119 +554,5 @@ describe('SignInExperienceValidator', () => { ).rejects.toMatchError(expectError); }); }); - - describe('guardMandatoryPasswordOnRegister', () => { - const testCases: Record< - string, - { - signInExperience: SignInExperience; - cases: Array<{ verificationRecord: VerificationRecord; accepted: boolean }>; - } - > = Object.freeze({ - 'should throw error for CodeVerification Records if password is required': { - signInExperience: { - ...mockSignInExperience, - signUp: { - identifiers: [SignInIdentifier.Email], - password: true, - verify: true, - }, - }, - cases: [ - { - verificationRecord: verificationCodeVerificationRecords[SignInIdentifier.Email], - accepted: false, - }, - { - verificationRecord: verificationCodeVerificationRecords[SignInIdentifier.Phone], - accepted: false, - }, - ], - }, - 'should not throw error for CodeVerification Records if password is not required': { - signInExperience: { - ...mockSignInExperience, - signUp: { - identifiers: [SignInIdentifier.Email], - password: false, - verify: true, - }, - }, - cases: [ - { - verificationRecord: verificationCodeVerificationRecords[SignInIdentifier.Email], - accepted: true, - }, - { - verificationRecord: verificationCodeVerificationRecords[SignInIdentifier.Phone], - accepted: true, - }, - ], - }, - 'should not throw error for NewPasswordIdentity verification record': { - signInExperience: { - ...mockSignInExperience, - signUp: { - identifiers: [SignInIdentifier.Username], - password: true, - verify: true, - }, - }, - cases: [ - { - verificationRecord: newPasswordIdentityVerificationRecord, - accepted: true, - }, - { - verificationRecord: emailNewPasswordIdentityVerificationRecord, - accepted: true, - }, - ], - }, - 'should not throw error for Social and SSO verification records': { - signInExperience: { - ...mockSignInExperience, - signUp: { - identifiers: [SignInIdentifier.Email], - password: true, - verify: true, - }, - }, - cases: [ - { - verificationRecord: socialVerificationRecord, - accepted: true, - }, - { - verificationRecord: enterpriseSsoVerificationRecords, - accepted: true, - }, - ], - }, - }); - - describe.each(Object.keys(testCases))(`%s`, (testCase) => { - const { signInExperience, cases } = testCases[testCase]!; - - it.each(cases)('guard verification record %p', async ({ verificationRecord, accepted }) => { - signInExperiences.findDefaultSignInExperience.mockResolvedValueOnce(signInExperience); - - const signInExperienceSettings = new SignInExperienceValidator( - mockTenant.libraries, - mockTenant.queries - ); - - await (accepted - ? expect( - signInExperienceSettings.guardMandatoryPasswordOnRegister(verificationRecord) - ).resolves.not.toThrow() - : expect( - signInExperienceSettings.guardMandatoryPasswordOnRegister(verificationRecord) - ).rejects.toMatchError( - new RequestError({ code: 'user.password_required_in_profile', status: 422 }) - )); - }); - }); - }); }); /* eslint-enable max-lines */ diff --git a/packages/core/src/routes/experience/classes/libraries/sign-in-experience-validator.ts b/packages/core/src/routes/experience/classes/libraries/sign-in-experience-validator.ts index 52fb96660..8db74727f 100644 --- a/packages/core/src/routes/experience/classes/libraries/sign-in-experience-validator.ts +++ b/packages/core/src/routes/experience/classes/libraries/sign-in-experience-validator.ts @@ -162,25 +162,6 @@ export class SignInExperienceValidator { return mandatoryUserProfile; } - /** - * If password is enabled in the sign-up settings, - * guard the verification record contains password (NewPasswordIdentity). - * - * - Password is not required for social and SSO verification records. - */ - public async guardMandatoryPasswordOnRegister({ type }: VerificationRecord) { - const { signUp } = await this.getSignInExperienceData(); - - if ( - signUp.password && - [VerificationType.EmailVerificationCode, VerificationType.PhoneVerificationCode].includes( - type - ) - ) { - throw new RequestError({ code: 'user.password_required_in_profile', status: 422 }); - } - } - /** * Guard the verification records contains email identifier with SSO enabled * @@ -283,6 +264,11 @@ export class SignInExperienceValidator { signUp.identifiers.includes(type) && signUp.verify, new RequestError({ code: 'user.sign_up_method_not_enabled', status: 422 }) ); + + assertThat( + !signUp.password, + new RequestError({ code: 'user.password_required_in_profile', status: 422 }) + ); break; } case VerificationType.Social: { diff --git a/packages/core/src/routes/experience/classes/verifications/new-password-identity-verification.ts b/packages/core/src/routes/experience/classes/verifications/new-password-identity-verification.ts index 09b803b0e..771829033 100644 --- a/packages/core/src/routes/experience/classes/verifications/new-password-identity-verification.ts +++ b/packages/core/src/routes/experience/classes/verifications/new-password-identity-verification.ts @@ -93,12 +93,21 @@ export class NewPasswordIdentityVerification * * - Check if the identifier is unique across users * - Validate the password against the password policy + * + * @throws {RequestError} with status 422 if the identifier is in use by another user + * @throws {RequestError} with status 422 if the password is not provided + * @throws {RequestError} with status 422 if the password does not meet the password policy */ - async verify(password: string) { + async verify(password?: string) { const { identifier } = this; const identifierProfile = interactionIdentifierToUserProfile(identifier); await this.profileValidator.guardProfileUniquenessAcrossUsers(identifierProfile); + assertThat( + password, + new RequestError({ code: 'user.password_required_in_profile', status: 422 }) + ); + const passwordPolicy = await this.signInExperienceValidator.getPasswordPolicy(); const passwordValidator = new PasswordValidator(passwordPolicy); await passwordValidator.validatePassword(password, identifierProfile); diff --git a/packages/core/src/routes/experience/verification-routes/new-password-identity-verification.ts b/packages/core/src/routes/experience/verification-routes/new-password-identity-verification.ts index 9004e8c9e..aa4be2564 100644 --- a/packages/core/src/routes/experience/verification-routes/new-password-identity-verification.ts +++ b/packages/core/src/routes/experience/verification-routes/new-password-identity-verification.ts @@ -1,4 +1,4 @@ -import { passwordVerificationPayloadGuard, VerificationType } from '@logto/schemas'; +import { interactionIdentifierGuard, VerificationType } from '@logto/schemas'; import { Action } from '@logto/schemas/lib/types/log/interaction.js'; import type Router from 'koa-router'; import { z } from 'zod'; @@ -17,7 +17,10 @@ export default function newPasswordIdentityVerificationRoutes< router.post( `${experienceRoutes.verification}/new-password-identity`, koaGuard({ - body: passwordVerificationPayloadGuard, + body: z.object({ + identifier: interactionIdentifierGuard, + password: z.string().optional(), + }), status: [200, 400, 422], response: z.object({ verificationId: z.string(), diff --git a/packages/integration-tests/src/client/experience/index.ts b/packages/integration-tests/src/client/experience/index.ts index 905579323..546d023c7 100644 --- a/packages/integration-tests/src/client/experience/index.ts +++ b/packages/integration-tests/src/client/experience/index.ts @@ -195,7 +195,9 @@ export class ExperienceClient extends MockClient { .json<{ verificationId: string }>(); } - public async createNewPasswordIdentityVerification(payload: PasswordVerificationPayload) { + public async createNewPasswordIdentityVerification( + payload: Pick & { password?: string } + ) { return api .post(`${experienceRoutes.verification}/new-password-identity`, { headers: { cookie: this.interactionCookie }, diff --git a/packages/integration-tests/src/tests/api/experience-api/register-interaction/verification-code.test.ts b/packages/integration-tests/src/tests/api/experience-api/register-interaction/verification-code.test.ts index 3191fcfba..0df478cd5 100644 --- a/packages/integration-tests/src/tests/api/experience-api/register-interaction/verification-code.test.ts +++ b/packages/integration-tests/src/tests/api/experience-api/register-interaction/verification-code.test.ts @@ -52,13 +52,13 @@ devFeatureTest.describe('Register interaction with verification code happy path' await Promise.all([setEmailConnector(), setSmsConnector()]); await enableAllVerificationCodeSignInMethods({ identifiers: [SignInIdentifier.Email, SignInIdentifier.Phone], - password: true, + password: false, verify: true, }); }); it.each(verificationIdentifierType)( - 'Should fail to sign-up with existing %p identifier and directly sign-in instead ', + 'Should fail to sign-up with existing %p identifier and directly sign-in instead', async (identifierType) => { const { userProfile, user } = await generateNewUser({ [identifiersTypeToUserProfile[identifierType]]: true, @@ -110,7 +110,7 @@ devFeatureTest.describe('Register interaction with verification code happy path' ); }); - describe('fulfill password', () => { + describe('password enabled', () => { beforeAll(async () => { await enableAllVerificationCodeSignInMethods({ identifiers: [SignInIdentifier.Email, SignInIdentifier.Phone], @@ -163,6 +163,68 @@ devFeatureTest.describe('Register interaction with verification code happy path' } ); + it.each(verificationIdentifierType)( + 'Should fail to sign-up with existing %p identifier and directly sign-in instead', + async (identifierType) => { + const { userProfile, user } = await generateNewUser({ + [identifiersTypeToUserProfile[identifierType]]: true, + password: true, + }); + + const identifier: VerificationCodeIdentifier = { + type: identifierType, + value: userProfile[identifiersTypeToUserProfile[identifierType]]!, + }; + + const client = await initExperienceClient(InteractionEvent.Register); + + const { verificationId, code } = await successfullySendVerificationCode(client, { + identifier, + interactionEvent: InteractionEvent.Register, + }); + + await successfullyVerifyVerificationCode(client, { + identifier, + verificationId, + code, + }); + + await expectRejects( + client.identifyUser({ + verificationId, + }), + { + code: `user.password_required_in_profile`, + status: 422, + } + ); + + await expectRejects( + client.createNewPasswordIdentityVerification({ + identifier, + }), + { + code: `user.${identifierType}_already_in_use`, + status: 422, + } + ); + + await client.updateInteractionEvent({ + interactionEvent: InteractionEvent.SignIn, + }); + + await client.identifyUser({ + verificationId, + }); + + const { redirectTo } = await client.submitInteraction(); + await processSession(client, redirectTo); + await logoutClient(client); + + await deleteUser(user.id); + } + ); + it.each(verificationIdentifierType)( 'Should register with verification code using %p and fulfill the password successfully', async (identifier) => { diff --git a/packages/integration-tests/src/tests/api/experience-api/verifications/new-password-identity-verification.test.ts b/packages/integration-tests/src/tests/api/experience-api/verifications/new-password-identity-verification.test.ts index 8c1df1359..fa0df227e 100644 --- a/packages/integration-tests/src/tests/api/experience-api/verifications/new-password-identity-verification.test.ts +++ b/packages/integration-tests/src/tests/api/experience-api/verifications/new-password-identity-verification.test.ts @@ -119,6 +119,27 @@ devFeatureTest.describe('password verifications', () => { [username, 'userInfo'], ]; + it('should throw error if password is not provided', async () => { + const { primaryEmail } = generateNewUserProfile({ + primaryEmail: true, + }); + + const client = await initExperienceClient(); + + await expectRejects( + client.createNewPasswordIdentityVerification({ + identifier: { + type: SignInIdentifier.Email, + value: primaryEmail, + }, + }), + { + code: 'user.password_required_in_profile', + status: 422, + } + ); + }); + it.each(invalidPasswords)('should reject invalid password %p', async (password) => { const client = await initExperienceClient();