diff --git a/packages/core/src/routes/experience/classes/experience-interaction.ts b/packages/core/src/routes/experience/classes/experience-interaction.ts index 79b5219da..bd55f296a 100644 --- a/packages/core/src/routes/experience/classes/experience-interaction.ts +++ b/packages/core/src/routes/experience/classes/experience-interaction.ts @@ -1,6 +1,12 @@ /* eslint-disable max-lines */ import { type ToZodObject } from '@logto/connector-kit'; -import { InteractionEvent, type User } from '@logto/schemas'; +import { + InteractionEvent, + SignInIdentifier, + VerificationType, + type InteractionIdentifier, + type User, +} from '@logto/schemas'; import { conditional } from '@silverhand/essentials'; import { z } from 'zod'; @@ -28,6 +34,7 @@ import { SignInExperienceValidator } from './libraries/sign-in-experience-valida import { Mfa, mfaDataGuard, userMfaDataKey, type MfaData } from './mfa.js'; import { Profile } from './profile.js'; import { toUserSocialIdentityData } from './utils.js'; +import { identifierCodeVerificationTypeMap } from './verifications/code-verification.js'; import { buildVerificationRecord, verificationRecordDataGuard, @@ -456,13 +463,28 @@ export default class ExperienceInteraction { /** * Create a new user using the verification record. * - * @throws {RequestError} with 400 if the verification record is invalid for creating a new user or not verified + * @throws {RequestError} with 422 if a new password identity verification is provided, but identifier (email/phone) is not verified + * @throws {RequestError} with 400 if the verification record can not be used for creating a new user or not verified * @throws {RequestError} with 422 if the profile data is not unique across users + * @throws {RequestError} with 422 if the password is required for the sign-up settings but only email/phone verification record is provided */ private async createNewUser(verificationRecord: VerificationRecord) { + if (verificationRecord.type === VerificationType.NewPasswordIdentity) { + const { identifier } = verificationRecord; + assertThat( + this.isIdentifierVerified(identifier), + new RequestError( + { code: 'session.identifier_not_verified', status: 422 }, + { identifier: identifier.value } + ) + ); + } + 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; @@ -500,6 +522,20 @@ export default class ExperienceInteraction { return this.verificationRecordsArray.find((record) => record.id === verificationId); } + private isIdentifierVerified(identifier: InteractionIdentifier) { + const { type, value } = identifier; + + if (type === SignInIdentifier.Username) { + return true; + } + + const verificationRecord = this.verificationRecords.get( + identifierCodeVerificationTypeMap[type] + ); + + return verificationRecord?.identifier.value === value && verificationRecord.isVerified; + } + /** * Clean up the interaction storage. */ 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 44b3839bc..9ee07d4e4 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 @@ -42,6 +42,15 @@ const newPasswordIdentityVerificationRecord = NewPasswordIdentityVerification.cr } ); +const emailNewPasswordIdentityVerificationRecord = NewPasswordIdentityVerification.create( + mockTenant.libraries, + mockTenant.queries, + { + type: SignInIdentifier.Email, + value: `foo@${emailDomain}`, + } +); + const passwordVerificationRecords = Object.fromEntries( Object.values(SignInIdentifier).map((identifier) => [ identifier, @@ -509,5 +518,119 @@ 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 ca4ab9e2b..52fb96660 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,6 +162,25 @@ 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 * 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 c3f8ccd48..09b803b0e 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 @@ -45,8 +45,7 @@ export const newPasswordIdentityVerificationRecordDataGuard = z.object({ * * @remarks This verification record can only be used for new user registration. * By default this verification record allows all types of identifiers, username, email, and phone. - * But in our current product design, only username + password registration is supported. The identifier type - * will be guarded at the API level. + * For email and phone identifiers, a `CodeVerification` record is required. */ export class NewPasswordIdentityVerification implements VerificationRecord 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 113748f8e..9004e8c9e 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 { newPasswordIdentityVerificationPayloadGuard, VerificationType } from '@logto/schemas'; +import { passwordVerificationPayloadGuard, 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,7 @@ export default function newPasswordIdentityVerificationRoutes< router.post( `${experienceRoutes.verification}/new-password-identity`, koaGuard({ - body: newPasswordIdentityVerificationPayloadGuard, + body: passwordVerificationPayloadGuard, 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 426e1b5aa..905579323 100644 --- a/packages/integration-tests/src/client/experience/index.ts +++ b/packages/integration-tests/src/client/experience/index.ts @@ -3,7 +3,6 @@ import { type IdentificationApiPayload, type InteractionEvent, type MfaFactor, - type NewPasswordIdentityVerificationPayload, type PasswordVerificationPayload, type UpdateProfileApiPayload, type VerificationCodeIdentifier, @@ -196,9 +195,7 @@ export class ExperienceClient extends MockClient { .json<{ verificationId: string }>(); } - public async createNewPasswordIdentityVerification( - payload: NewPasswordIdentityVerificationPayload - ) { + public async createNewPasswordIdentityVerification(payload: PasswordVerificationPayload) { return api .post(`${experienceRoutes.verification}/new-password-identity`, { headers: { cookie: this.interactionCookie }, diff --git a/packages/integration-tests/src/helpers/experience/index.ts b/packages/integration-tests/src/helpers/experience/index.ts index 303d9de26..c3db52436 100644 --- a/packages/integration-tests/src/helpers/experience/index.ts +++ b/packages/integration-tests/src/helpers/experience/index.ts @@ -109,28 +109,29 @@ export const registerNewUserWithVerificationCode = async ( interactionEvent: InteractionEvent.Register, }); - const verifiedVerificationId = await successfullyVerifyVerificationCode(client, { + await successfullyVerifyVerificationCode(client, { identifier, verificationId, code, }); - await client.identifyUser({ - verificationId: verifiedVerificationId, - }); - if (options?.fulfillPassword) { - await expectRejects(client.submitInteraction(), { - code: 'user.missing_profile', + await expectRejects(client.identifyUser({ verificationId }), { + code: 'user.password_required_in_profile', status: 422, }); const password = generatePassword(); - await client.updateProfile({ - type: 'password', - value: password, - }); + const { verificationId: newPasswordIdentityVerificationId } = + await client.createNewPasswordIdentityVerification({ + identifier, + password, + }); + + await client.identifyUser({ verificationId: newPasswordIdentityVerificationId }); + } else { + await client.identifyUser({ verificationId }); } const { redirectTo } = await client.submitInteraction(); 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 9aee1dde4..3191fcfba 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 @@ -15,7 +15,7 @@ import { import { expectRejects } from '#src/helpers/index.js'; import { enableAllVerificationCodeSignInMethods } from '#src/helpers/sign-in-experience.js'; import { generateNewUser } from '#src/helpers/user.js'; -import { devFeatureTest, generateEmail, generatePhone } from '#src/utils.js'; +import { devFeatureTest, generateEmail, generatePassword, generatePhone } from '#src/utils.js'; const verificationIdentifierType: readonly [SignInIdentifier.Email, SignInIdentifier.Phone] = Object.freeze([SignInIdentifier.Email, SignInIdentifier.Phone]); @@ -47,56 +47,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, + describe('sign-up with existing identifier', () => { + beforeAll(async () => { + await Promise.all([setEmailConnector(), setSmsConnector()]); + await enableAllVerificationCodeSignInMethods({ + identifiers: [SignInIdentifier.Email, SignInIdentifier.Phone], + password: true, + verify: true, }); + }); - const identifier: VerificationCodeIdentifier = { - type: identifierType, - value: userProfile[identifiersTypeToUserProfile[identifierType]]!, - }; + 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 client = await initExperienceClient(InteractionEvent.Register); + const identifier: VerificationCodeIdentifier = { + type: identifierType, + value: userProfile[identifiersTypeToUserProfile[identifierType]]!, + }; - const { verificationId, code } = await successfullySendVerificationCode(client, { - identifier, - interactionEvent: InteractionEvent.Register, - }); + const client = await initExperienceClient(InteractionEvent.Register); - await successfullyVerifyVerificationCode(client, { - identifier, - verificationId, - code, - }); + const { verificationId, code } = await successfullySendVerificationCode(client, { + identifier, + interactionEvent: InteractionEvent.Register, + }); - await expectRejects( - client.identifyUser({ + await successfullyVerifyVerificationCode(client, { + identifier, verificationId, - }), - { - code: `user.${identifierType}_already_in_use`, - status: 422, - } - ); + code, + }); - await client.updateInteractionEvent({ - interactionEvent: InteractionEvent.SignIn, - }); + await expectRejects( + client.identifyUser({ + verificationId, + }), + { + code: `user.${identifierType}_already_in_use`, + status: 422, + } + ); - await client.identifyUser({ - verificationId, - }); + await client.updateInteractionEvent({ + interactionEvent: InteractionEvent.SignIn, + }); - const { redirectTo } = await client.submitInteraction(); - await processSession(client, redirectTo); - await logoutClient(client); + await client.identifyUser({ + verificationId, + }); - await deleteUser(user.id); - } - ); + const { redirectTo } = await client.submitInteraction(); + await processSession(client, redirectTo); + await logoutClient(client); + + await deleteUser(user.id); + } + ); + }); describe('fulfill password', () => { beforeAll(async () => { @@ -107,6 +119,50 @@ devFeatureTest.describe('Register interaction with verification code happy path' }); }); + it.each(verificationIdentifierType)( + 'Should throw identifier not verified error when trying to fulfill password without verifying %p identifier', + async (identifier) => { + const client = await initExperienceClient(InteractionEvent.Register); + const interactionIdentifier = { + type: identifier, + value: identifier === SignInIdentifier.Email ? generateEmail() : generatePhone(), + }; + + const { verificationId } = await client.createNewPasswordIdentityVerification({ + identifier: interactionIdentifier, + password: generatePassword(), + }); + + await expectRejects(client.identifyUser({ verificationId }), { + code: 'session.identifier_not_verified', + status: 422, + }); + + const { verificationId: codeVerificationId, code } = await successfullySendVerificationCode( + client, + { + identifier: interactionIdentifier, + interactionEvent: InteractionEvent.Register, + } + ); + + await successfullyVerifyVerificationCode(client, { + identifier: interactionIdentifier, + verificationId: codeVerificationId, + code, + }); + + await client.identifyUser({ verificationId }); + + const { redirectTo } = await client.submitInteraction(); + + const userId = await processSession(client, redirectTo); + await logoutClient(client); + + await deleteUser(userId); + } + ); + 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 48187b208..8c1df1359 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 @@ -3,22 +3,12 @@ import { SignInIdentifier } from '@logto/schemas'; import { updateSignInExperience } from '#src/api/sign-in-experience.js'; import { initExperienceClient } from '#src/helpers/client.js'; import { expectRejects } from '#src/helpers/index.js'; -import { generateNewUser } from '#src/helpers/user.js'; +import { generateNewUserProfile, UserApiTest } from '#src/helpers/user.js'; import { devFeatureTest, randomString } from '#src/utils.js'; -const invalidIdentifiers = Object.freeze([ - { - type: SignInIdentifier.Email, - value: 'email', - }, - { - type: SignInIdentifier.Phone, - value: 'phone', - }, -]); - devFeatureTest.describe('password verifications', () => { const username = 'test_' + randomString(); + const userApi = new UserApiTest(); beforeAll(async () => { await updateSignInExperience({ @@ -35,6 +25,10 @@ devFeatureTest.describe('password verifications', () => { }); }); + afterEach(async () => { + await userApi.cleanUp(); + }); + afterAll(async () => { await updateSignInExperience({ // Need to reset password policy to default value otherwise it will affect other tests. @@ -42,44 +36,77 @@ devFeatureTest.describe('password verifications', () => { }); }); - it.each(invalidIdentifiers)( - 'should fail to verify with password using %p', - async (identifier) => { + describe('invalid identifier check', () => { + it('should throw error if username is registered', async () => { + const { username, password } = generateNewUserProfile({ username: true, password: true }); + await userApi.create({ username, password }); + const client = await initExperienceClient(); await expectRejects( client.createNewPasswordIdentityVerification({ - // @ts-expect-error - identifier, - password: 'password', + identifier: { + type: SignInIdentifier.Username, + value: username, + }, + password, }), { - code: 'guard.invalid_input', - status: 400, + code: 'user.username_already_in_use', + status: 422, } ); - } - ); + }); - it('should throw error if username is registered', async () => { - const { userProfile } = await generateNewUser({ username: true, password: true }); - const { username } = userProfile; + it('should throw error if email is registered', async () => { + const { primaryEmail, password } = generateNewUserProfile({ + primaryEmail: true, + password: true, + }); - const client = await initExperienceClient(); + await userApi.create({ primaryEmail, password }); - await expectRejects( - client.createNewPasswordIdentityVerification({ - identifier: { - type: SignInIdentifier.Username, - value: username, - }, - password: 'password', - }), - { - code: 'user.username_already_in_use', - status: 422, - } - ); + const client = await initExperienceClient(); + + await expectRejects( + client.createNewPasswordIdentityVerification({ + identifier: { + type: SignInIdentifier.Email, + value: primaryEmail, + }, + password, + }), + { + code: 'user.email_already_in_use', + status: 422, + } + ); + }); + + it('should throw error if phone is registered', async () => { + const { primaryPhone, password } = generateNewUserProfile({ + primaryPhone: true, + password: true, + }); + + await userApi.create({ primaryPhone, password }); + + const client = await initExperienceClient(); + + await expectRejects( + client.createNewPasswordIdentityVerification({ + identifier: { + type: SignInIdentifier.Phone, + value: primaryPhone, + }, + password, + }), + { + code: 'user.phone_already_in_use', + status: 422, + } + ); + }); }); describe('password policy check', () => { diff --git a/packages/phrases/src/locales/en/errors/session.ts b/packages/phrases/src/locales/en/errors/session.ts index f4c8943cc..61511ffb7 100644 --- a/packages/phrases/src/locales/en/errors/session.ts +++ b/packages/phrases/src/locales/en/errors/session.ts @@ -27,6 +27,8 @@ const session = { not_supported_for_forgot_password: 'This operation is not supported for forgot password.', identity_conflict: 'Identity mismatch detected. Please initiate a new session to proceed with a different identity.', + identifier_not_verified: + 'The provided identifier {{identifier}} has not been verified. Please create a verification record for this identifier and complete the verification process.', mfa: { require_mfa_verification: 'Mfa verification is required to sign in.', mfa_sign_in_only: 'Mfa is only available for sign-in interaction.', diff --git a/packages/schemas/src/types/interactions.ts b/packages/schemas/src/types/interactions.ts index 84b99faca..da97eb8ab 100644 --- a/packages/schemas/src/types/interactions.ts +++ b/packages/schemas/src/types/interactions.ts @@ -1,4 +1,3 @@ -/* eslint-disable max-lines */ import { emailRegEx, phoneRegEx, usernameRegEx } from '@logto/core-kit'; import { z } from 'zod'; @@ -121,26 +120,6 @@ export const backupCodeVerificationVerifyPayloadGuard = z.object({ code: z.string().min(1), }) satisfies ToZodObject; -/** - * Payload type for `POST /api/experience/verification/new-password-identity`. - * @remarks Currently we only support username identifier for new password identity registration. - * For email and phone new identity registration, a `CodeVerification` record is required. - */ -export type NewPasswordIdentityVerificationPayload = { - identifier: { - type: SignInIdentifier.Username; - value: string; - }; - password: string; -}; -export const newPasswordIdentityVerificationPayloadGuard = z.object({ - identifier: z.object({ - type: z.literal(SignInIdentifier.Username), - value: z.string(), - }), - password: z.string().min(1), -}) satisfies ToZodObject; - /** Payload type for `POST /api/experience/identification`. */ export type IdentificationApiPayload = { /** The ID of the verification record that is used to identify the user. */ @@ -432,4 +411,3 @@ export const verifyMfaResultGuard = z.object({ }); export type VerifyMfaResult = z.infer; -/* eslint-enable max-lines */