Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core,schemas): add mandatory password guard on register #6368

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -28,6 +34,7 @@
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,
Expand Down Expand Up @@ -456,13 +463,28 @@
/**
* 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 }
)
);
}

Check warning on line 481 in packages/core/src/routes/experience/classes/experience-interaction.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/routes/experience/classes/experience-interaction.ts#L473-L481

Added lines #L473 - L481 were not covered by tests

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;
Expand Down Expand Up @@ -500,6 +522,20 @@
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;
}

Check warning on line 537 in packages/core/src/routes/experience/classes/experience-interaction.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/routes/experience/classes/experience-interaction.ts#L526-L537

Added lines #L526 - L537 were not covered by tests

/**
* Clean up the interaction storage.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 */
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<VerificationType.NewPasswordIdentity>
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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(),
Expand Down
5 changes: 1 addition & 4 deletions packages/integration-tests/src/client/experience/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {
type IdentificationApiPayload,
type InteractionEvent,
type MfaFactor,
type NewPasswordIdentityVerificationPayload,
type PasswordVerificationPayload,
type UpdateProfileApiPayload,
type VerificationCodeIdentifier,
Expand Down Expand Up @@ -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 },
Expand Down
23 changes: 12 additions & 11 deletions packages/integration-tests/src/helpers/experience/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading
Loading