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: add phone number validation to user APIs #5882

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion packages/console/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
"jest-transformer-svg": "^2.0.0",
"just-kebab-case": "^4.2.0",
"ky": "^1.2.3",
"libphonenumber-js": "^1.10.51",
"libphonenumber-js": "^1.11.1",
"lint-staged": "^15.0.0",
"nanoid": "^5.0.1",
"overlayscrollbars": "^2.0.2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { emailRegEx, usernameRegEx } from '@logto/core-kit';
import type { User } from '@logto/schemas';
import { parsePhoneNumber } from '@logto/shared/universal';
import { conditionalString, trySafe } from '@silverhand/essentials';
import { parsePhoneNumberWithError } from 'libphonenumber-js';
import { parsePhoneNumberWithError } from 'libphonenumber-js/mobile';
import { useForm, useController } from 'react-hook-form';
import { toast } from 'react-hot-toast';
import { useTranslation } from 'react-i18next';
Expand Down
32 changes: 23 additions & 9 deletions packages/core/src/libraries/user.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,11 +186,15 @@ describe('verifyUserPassword()', () => {
};
it('migrates password to Argon2', async () => {
await verifyUserPassword(user, 'password');
expect(updateUserById).toHaveBeenCalledWith(user.id, {
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
passwordEncrypted: expect.stringContaining('argon2'),
passwordEncryptionMethod: UsersPasswordEncryptionMethod.Argon2i,
});
expect(updateUserById).toHaveBeenCalledWith(
user.id,
{
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
passwordEncrypted: expect.stringContaining('argon2'),
passwordEncryptionMethod: UsersPasswordEncryptionMethod.Argon2i,
},
undefined
);
});
});
});
Expand Down Expand Up @@ -220,17 +224,27 @@ describe('addUserMfaVerification()', () => {
beforeAll(() => {
jest.useFakeTimers();
jest.setSystemTime(new Date(createdAt));
jest.clearAllMocks();
});

afterAll(() => {
jest.useRealTimers();
});

it('update user with new mfa verification', async () => {
await addUserMfaVerification(mockUser.id, { type: MfaFactor.TOTP, secret: 'secret' });
expect(updateUserById).toHaveBeenCalledWith(mockUser.id, {
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
mfaVerifications: [{ type: MfaFactor.TOTP, key: 'secret', id: expect.anything(), createdAt }],
await addUserMfaVerification(mockUser.id, {
type: MfaFactor.TOTP,
secret: 'secret',
});
expect(updateUserById).toHaveBeenCalledWith(
mockUser.id,
{
mfaVerifications: [
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
{ type: MfaFactor.TOTP, key: 'secret', id: expect.anything(), createdAt },
],
},
undefined
);
});
});
43 changes: 39 additions & 4 deletions packages/core/src/libraries/user.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
/* eslint-disable max-lines */
import type { BindMfa, CreateUser, MfaVerification, Scope, User } from '@logto/schemas';
import { MfaFactor, RoleType, Users, UsersPasswordEncryptionMethod } from '@logto/schemas';
import { generateStandardId, generateStandardShortId } from '@logto/shared';
import { deduplicateByKey, type Nullable } from '@silverhand/essentials';
import { generateStandardShortId, generateStandardId } from '@logto/shared';
import type { Nullable } from '@silverhand/essentials';
import { deduplicateByKey, conditional } from '@silverhand/essentials';
import { argon2Verify, bcryptVerify, md5, sha1, sha256 } from 'hash-wasm';
import pRetry from 'p-retry';

Expand All @@ -14,6 +16,7 @@
import assertThat from '#src/utils/assert-that.js';
import { encryptPassword } from '#src/utils/password.js';
import type { OmitAutoSetFields } from '#src/utils/sql.js';
import { getValidPhoneNumber } from '#src/utils/user.js';

export const encryptUserPassword = async (
password: string
Expand Down Expand Up @@ -90,7 +93,7 @@
hasUserWithId,
hasUserWithPhone,
findUsersByIds,
updateUserById,
updateUserById: updateUserByIdQuery,
findUserById,
},
usersRoles: { findUsersRolesByRoleId, findUsersRolesByUserId },
Expand All @@ -115,6 +118,29 @@
{ retries, factor: 0 } // No need for exponential backoff
);

const updateUserById = async (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should put the validPhoneNumber validation logic at the API koaGuard level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering to make this phone number guard at API level. But since the DB insert and update operation could bypass the API-level check since some of them were used in scenarios other than in API handlers.
Make this change on the DB queries/libraries methods IMO should be the safest and the most easy-to-maintain way of doing this.

id: string,
set: Partial<OmitAutoSetFields<CreateUser>>,
jsonbMode?: 'replace' | 'merge'
) => {
const validPhoneNumber = conditional(
typeof set.primaryPhone === 'string' && getValidPhoneNumber(set.primaryPhone)
);

return updateUserByIdQuery(
id,
{
...set,
...conditional(
validPhoneNumber && {
primaryPhone: validPhoneNumber,
}

Check warning on line 137 in packages/core/src/libraries/user.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/libraries/user.ts#L136-L137

Added lines #L136 - L137 were not covered by tests
),
},
jsonbMode
);
};

const insertUser = async (
data: OmitAutoSetFields<CreateUser>,
additionalRoleNames: string[]
Expand All @@ -127,12 +153,19 @@

assertThat(parameterRoles.length === roleNames.length, 'role.default_role_missing');

const validPhoneNumber = conditional(
typeof data.primaryPhone === 'string' && getValidPhoneNumber(data.primaryPhone)
);

Check warning on line 159 in packages/core/src/libraries/user.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/libraries/user.ts#L156-L159

Added lines #L156 - L159 were not covered by tests
return pool.transaction(async (connection) => {
const insertUserQuery = buildInsertIntoWithPool(connection)(Users, {
returning: true,
});

const user = await insertUserQuery(data);
const user = await insertUserQuery({
...data,
...conditional(validPhoneNumber && { primaryPhone: validPhoneNumber }),
});

Check warning on line 168 in packages/core/src/libraries/user.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/libraries/user.ts#L165-L168

Added lines #L165 - L168 were not covered by tests
const roles = deduplicateByKey([...parameterRoles, ...defaultRoles], 'id');

if (roles.length > 0) {
Expand All @@ -145,7 +178,7 @@
const provisionOrganizations = async (): Promise<readonly string[]> => {
// Just-in-time organization provisioning
const userEmailDomain = data.primaryEmail?.split('@')[1];
// TODO: Remove this check when launching

Check warning on line 181 in packages/core/src/libraries/user.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/core/src/libraries/user.ts#L181

[no-warning-comments] Unexpected 'todo' comment: 'TODO: Remove this check when launching'.
if (EnvSet.values.isDevFeaturesEnabled && userEmailDomain) {
const organizationQueries = new OrganizationQueries(connection);
const organizationIds = await organizationQueries.emailDomains.getOrganizationIdsByDomain(
Expand Down Expand Up @@ -245,7 +278,7 @@
};

const addUserMfaVerification = async (userId: string, payload: BindMfa) => {
// TODO @sijie use jsonb array append

Check warning on line 281 in packages/core/src/libraries/user.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/core/src/libraries/user.ts#L281

[no-warning-comments] Unexpected 'todo' comment: 'TODO @sijie use jsonb array append'.
const { mfaVerifications } = await findUserById(userId);
await updateUserById(userId, {
mfaVerifications: [...mfaVerifications, converBindMfaToMfaVerification(payload)],
Expand Down Expand Up @@ -336,5 +369,7 @@
verifyUserPassword,
signOutUser,
findUserSsoIdentities,
updateUserById,
};
};
/* eslint-enable max-lines */
5 changes: 4 additions & 1 deletion packages/core/src/routes-me/social.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,12 @@ export default function socialRoutes<T extends AuthedMeRouter>(
) {
const {
queries: {
users: { findUserById, updateUserById, deleteUserIdentity, hasUserWithIdentity },
users: { findUserById, deleteUserIdentity, hasUserWithIdentity },
signInExperiences: { findDefaultSignInExperience },
},
libraries: {
users: { updateUserById },
},
connectors: { getLogtoConnectors, getLogtoConnectorById },
} = tenant;

Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/routes-me/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ export default function userRoutes<T extends AuthedMeRouter>(
) {
const {
queries: {
users: { findUserById, updateUserById },
users: { findUserById },
},
libraries: {
users: { checkIdentifierCollision, verifyUserPassword },
users: { checkIdentifierCollision, verifyUserPassword, updateUserById },
verificationStatuses: { createVerificationStatus, checkVerificationStatus },
},
} = tenant;
Expand Down
17 changes: 9 additions & 8 deletions packages/core/src/routes/admin-user/basics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,6 @@ const mockedQueries = {
hasUser: jest.fn(async () => mockHasUser()),
hasUserWithEmail: jest.fn(async () => mockHasUserWithEmail()),
hasUserWithPhone: jest.fn(async () => mockHasUserWithPhone()),
updateUserById: jest.fn(
async (_, data: Partial<CreateUser>): Promise<User> => ({
...mockUser,
...data,
})
),
deleteUserById: jest.fn(),
deleteUserIdentity: jest.fn(),
},
Expand All @@ -67,8 +61,7 @@ const mockHasUser = jest.fn(async () => false);
const mockHasUserWithEmail = jest.fn(async () => false);
const mockHasUserWithPhone = jest.fn(async () => false);

const { hasUser, findUserById, updateUserById, deleteUserIdentity, deleteUserById } =
mockedQueries.users;
const { hasUser, findUserById, deleteUserIdentity, deleteUserById } = mockedQueries.users;

const { encryptUserPassword } = await mockEsmWithActual('#src/libraries/user.js', () => ({
encryptUserPassword: jest.fn(() => ({
Expand All @@ -92,8 +85,16 @@ const usersLibraries = {
),
verifyUserPassword,
signOutUser,
updateUserById: jest.fn(
async (_, data: Partial<CreateUser>): Promise<User> => ({
...mockUser,
...data,
})
),
} satisfies Partial<Libraries['users']>;

const { updateUserById } = usersLibraries;

const adminUserRoutes = await pickDefault(import('./basics.js'));

describe('adminUserRoutes', () => {
Expand Down
10 changes: 2 additions & 8 deletions packages/core/src/routes/admin-user/basics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,7 @@ export default function adminUserBasicsRoutes<T extends ManagementApiRouter>(
) {
const [router, { queries, libraries }] = args;
const {
users: {
deleteUserById,
findUserById,
hasUser,
updateUserById,
hasUserWithEmail,
hasUserWithPhone,
},
users: { deleteUserById, findUserById, hasUser, hasUserWithEmail, hasUserWithPhone },
} = queries;
const {
users: {
Expand All @@ -40,6 +33,7 @@ export default function adminUserBasicsRoutes<T extends ManagementApiRouter>(
verifyUserPassword,
signOutUser,
findUserSsoIdentities,
updateUserById,
},
} = libraries;

Expand Down
45 changes: 24 additions & 21 deletions packages/core/src/routes/admin-user/mfa-verifications.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,6 @@ const mockedQueries = {
hasUser: jest.fn(async () => mockHasUser()),
hasUserWithEmail: jest.fn(async () => mockHasUserWithEmail()),
hasUserWithPhone: jest.fn(async () => mockHasUserWithPhone()),
updateUserById: jest.fn(
async (_, data: Partial<CreateUser>): Promise<User> => ({
...mockUser,
...data,
})
),
deleteUserById: jest.fn(),
deleteUserIdentity: jest.fn(),
},
Expand All @@ -38,7 +32,7 @@ const mockHasUser = jest.fn(async () => false);
const mockHasUserWithEmail = jest.fn(async () => false);
const mockHasUserWithPhone = jest.fn(async () => false);

const { findUserById, updateUserById } = mockedQueries.users;
const { findUserById } = mockedQueries.users;

await mockEsmWithActual('../interaction/utils/totp-validation.js', () => ({
generateTotpSecret: jest.fn().mockReturnValue('totp_secret'),
Expand All @@ -47,25 +41,34 @@ await mockEsmWithActual('../interaction/utils/backup-code-validation.js', () =>
generateBackupCodes: jest.fn().mockReturnValue(['code']),
}));

const usersLibraries = {
generateUserId: jest.fn(async () => 'fooId'),
insertUser: jest.fn(
async (user: CreateUser): Promise<InsertUserResult> => [
{
const mockLibraries = {
users: {
generateUserId: jest.fn(async () => 'fooId'),
insertUser: jest.fn(
async (user: CreateUser): Promise<InsertUserResult> => [
{
...mockUser,
...removeUndefinedKeys(user), // No undefined values will be returned from database
},
{ organizationIds: [] },
]
),
updateUserById: jest.fn(
async (_, data: Partial<CreateUser>): Promise<User> => ({
...mockUser,
...removeUndefinedKeys(user), // No undefined values will be returned from database
},
{ organizationIds: [] },
]
),
} satisfies Partial<Libraries['users']>;
...data,
})
),
addUserMfaVerification: jest.fn(),
},
} satisfies Partial2<Libraries>;

const { updateUserById } = mockLibraries.users;

const adminUserRoutes = await pickDefault(import('./mfa-verifications.js'));

describe('adminUserRoutes', () => {
const tenantContext = new MockTenant(undefined, mockedQueries, undefined, {
users: usersLibraries,
});
const tenantContext = new MockTenant(undefined, mockedQueries, undefined, mockLibraries);
const userRequest = createRequester({ authedRoutes: adminUserRoutes, tenantContext });

afterEach(() => {
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/routes/admin-user/mfa-verifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ export default function adminUserMfaVerificationsRoutes<T extends ManagementApiR
{
queries,
libraries: {
users: { addUserMfaVerification },
users: { addUserMfaVerification, updateUserById },
},
},
] = args;
const {
users: { findUserById, updateUserById },
users: { findUserById },
} = queries;

router.get(
Expand Down
15 changes: 8 additions & 7 deletions packages/core/src/routes/admin-user/social.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,6 @@ const mockedQueries = {
}
return mockUser;
}),
updateUserById: jest.fn(
async (_, data: Partial<CreateUser>): Promise<User> => ({
...mockUser,
...data,
})
),
hasUserWithIdentity: mockHasUserWithIdentity,
deleteUserById: jest.fn(),
deleteUserIdentity: jest.fn(),
Expand All @@ -57,6 +51,12 @@ const usersLibraries = {
{ organizationIds: [] },
]
),
updateUserById: jest.fn(
async (_, data: Partial<CreateUser>): Promise<User> => ({
...mockUser,
...data,
})
),
} satisfies Partial<Libraries['users']>;

const mockGetLogtoConnectors = jest.fn(async () => mockLogtoConnectorList);
Expand All @@ -78,7 +78,8 @@ const mockedConnectors = {
},
};

const { findUserById, updateUserById, deleteUserIdentity } = mockedQueries.users;
const { findUserById, deleteUserIdentity } = mockedQueries.users;
const { updateUserById } = usersLibraries;

const adminUserSocialRoutes = await pickDefault(import('./social.js'));

Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/routes/admin-user/social.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ export default function adminUserSocialRoutes<T extends ManagementApiRouter>(
) {
const {
queries: {
users: { findUserById, updateUserById, hasUserWithIdentity, deleteUserIdentity },
users: { findUserById, hasUserWithIdentity, deleteUserIdentity },
},
libraries: {
users: { updateUserById },
},
connectors: { getLogtoConnectorById },
} = tenant;
Expand Down
Loading
Loading