From 6cc51d098b9ff824881e5362aeda9e205e03e601 Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Thu, 16 May 2024 17:07:05 +0800 Subject: [PATCH] feat: add phone number validation to user APIs --- packages/console/package.json | 2 +- .../pages/UserDetails/UserSettings/index.tsx | 2 +- packages/core/src/libraries/user.test.ts | 32 +++++++++++---- packages/core/src/libraries/user.ts | 38 +++++++++++++++-- packages/core/src/routes-me/social.ts | 5 ++- packages/core/src/routes-me/user.ts | 4 +- .../core/src/routes/admin-user/basics.test.ts | 17 ++++---- packages/core/src/routes/admin-user/basics.ts | 10 +---- .../admin-user/mfa-verifications.test.ts | 41 +++++++++---------- .../routes/admin-user/mfa-verifications.ts | 4 +- .../core/src/routes/admin-user/social.test.ts | 15 +++---- packages/core/src/routes/admin-user/social.ts | 5 ++- .../actions/submit-interaction.mfa.test.ts | 6 +-- .../actions/submit-interaction.test.ts | 6 +-- .../interaction/actions/submit-interaction.ts | 9 ++-- .../core/src/routes/interaction/mfa.test.ts | 5 +++ packages/core/src/routes/interaction/mfa.ts | 4 +- .../interaction/utils/single-sign-on.test.ts | 2 +- .../interaction/utils/single-sign-on.ts | 17 ++++---- .../mfa-payload-verification.test.ts | 18 +++++--- .../verifications/mfa-payload-verification.ts | 4 +- .../mfa-verification.backup-code.test.ts | 18 +++++--- .../verifications/mfa-verification.test.ts | 16 +++++--- packages/core/src/utils/user.ts | 26 ++++++++++++ packages/experience/package.json | 3 +- packages/experience/src/utils/country-code.ts | 21 +++------- packages/experience/src/utils/form.ts | 7 ++-- .../src/tests/api/admin-user.search.test.ts | 6 +-- .../src/tests/api/admin-user.test.ts | 8 ++++ .../src/tests/api/role.user.test.ts | 10 ++++- packages/shared/package.json | 2 +- packages/shared/src/utils/phone.ts | 30 ++++++++++++-- pnpm-lock.yaml | 21 ++++++---- 33 files changed, 273 insertions(+), 141 deletions(-) diff --git a/packages/console/package.json b/packages/console/package.json index c36f033d1..9ef637d97 100644 --- a/packages/console/package.json +++ b/packages/console/package.json @@ -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", diff --git a/packages/console/src/pages/UserDetails/UserSettings/index.tsx b/packages/console/src/pages/UserDetails/UserSettings/index.tsx index bc08dbead..21f4d39ef 100644 --- a/packages/console/src/pages/UserDetails/UserSettings/index.tsx +++ b/packages/console/src/pages/UserDetails/UserSettings/index.tsx @@ -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'; diff --git a/packages/core/src/libraries/user.test.ts b/packages/core/src/libraries/user.test.ts index fcd18cbfa..d62b5c3ee 100644 --- a/packages/core/src/libraries/user.test.ts +++ b/packages/core/src/libraries/user.test.ts @@ -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 + ); }); }); }); @@ -220,6 +224,7 @@ describe('addUserMfaVerification()', () => { beforeAll(() => { jest.useFakeTimers(); jest.setSystemTime(new Date(createdAt)); + jest.clearAllMocks(); }); afterAll(() => { @@ -227,10 +232,19 @@ describe('addUserMfaVerification()', () => { }); 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 + ); }); }); diff --git a/packages/core/src/libraries/user.ts b/packages/core/src/libraries/user.ts index 043ec64ea..781ef6af6 100644 --- a/packages/core/src/libraries/user.ts +++ b/packages/core/src/libraries/user.ts @@ -1,7 +1,8 @@ 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'; @@ -14,6 +15,7 @@ import type Queries from '#src/tenants/Queries.js'; 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 @@ -90,7 +92,7 @@ export const createUserLibrary = (queries: Queries) => { hasUserWithId, hasUserWithPhone, findUsersByIds, - updateUserById, + updateUserById: updateUserByIdQuery, findUserById, }, usersRoles: { findUsersRolesByRoleId, findUsersRolesByUserId }, @@ -115,6 +117,24 @@ export const createUserLibrary = (queries: Queries) => { { retries, factor: 0 } // No need for exponential backoff ); + const updateUserById = async ( + id: string, + set: Partial>, + jsonbMode?: 'replace' | 'merge' + ) => { + const validPhoneNumber = conditional( + 'primaryPhone' in set && + typeof set.primaryPhone === 'string' && + getValidPhoneNumber(set.primaryPhone) + ); + + return updateUserByIdQuery( + id, + { ...set, ...conditional(validPhoneNumber && { primaryPhone: validPhoneNumber }) }, + jsonbMode + ); + }; + const insertUser = async ( data: OmitAutoSetFields, additionalRoleNames: string[] @@ -127,12 +147,21 @@ export const createUserLibrary = (queries: Queries) => { assertThat(parameterRoles.length === roleNames.length, 'role.default_role_missing'); + const validPhoneNumber = conditional( + 'primaryPhone' in data && + typeof data.primaryPhone === 'string' && + getValidPhoneNumber(data.primaryPhone) + ); + 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 }), + }); const roles = deduplicateByKey([...parameterRoles, ...defaultRoles], 'id'); if (roles.length > 0) { @@ -336,5 +365,6 @@ export const createUserLibrary = (queries: Queries) => { verifyUserPassword, signOutUser, findUserSsoIdentities, + updateUserById, }; }; diff --git a/packages/core/src/routes-me/social.ts b/packages/core/src/routes-me/social.ts index 6da3f6d9d..d280651b0 100644 --- a/packages/core/src/routes-me/social.ts +++ b/packages/core/src/routes-me/social.ts @@ -22,9 +22,12 @@ export default function socialRoutes( ) { const { queries: { - users: { findUserById, updateUserById, deleteUserIdentity, hasUserWithIdentity }, + users: { findUserById, deleteUserIdentity, hasUserWithIdentity }, signInExperiences: { findDefaultSignInExperience }, }, + libraries: { + users: { updateUserById }, + }, connectors: { getLogtoConnectors, getLogtoConnectorById }, } = tenant; diff --git a/packages/core/src/routes-me/user.ts b/packages/core/src/routes-me/user.ts index c1e3faf9c..0ec0cd406 100644 --- a/packages/core/src/routes-me/user.ts +++ b/packages/core/src/routes-me/user.ts @@ -17,10 +17,10 @@ export default function userRoutes( ) { const { queries: { - users: { findUserById, updateUserById }, + users: { findUserById }, }, libraries: { - users: { checkIdentifierCollision, verifyUserPassword }, + users: { checkIdentifierCollision, verifyUserPassword, updateUserById }, verificationStatuses: { createVerificationStatus, checkVerificationStatus }, }, } = tenant; diff --git a/packages/core/src/routes/admin-user/basics.test.ts b/packages/core/src/routes/admin-user/basics.test.ts index 4de3d6e5a..5cf4f6a65 100644 --- a/packages/core/src/routes/admin-user/basics.test.ts +++ b/packages/core/src/routes/admin-user/basics.test.ts @@ -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): Promise => ({ - ...mockUser, - ...data, - }) - ), deleteUserById: jest.fn(), deleteUserIdentity: jest.fn(), }, @@ -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(() => ({ @@ -92,8 +85,16 @@ const usersLibraries = { ), verifyUserPassword, signOutUser, + updateUserById: jest.fn( + async (_, data: Partial): Promise => ({ + ...mockUser, + ...data, + }) + ), } satisfies Partial; +const { updateUserById } = usersLibraries; + const adminUserRoutes = await pickDefault(import('./basics.js')); describe('adminUserRoutes', () => { diff --git a/packages/core/src/routes/admin-user/basics.ts b/packages/core/src/routes/admin-user/basics.ts index 330c47ff1..d3699bc70 100644 --- a/packages/core/src/routes/admin-user/basics.ts +++ b/packages/core/src/routes/admin-user/basics.ts @@ -23,14 +23,7 @@ export default function adminUserBasicsRoutes( ) { const [router, { queries, libraries }] = args; const { - users: { - deleteUserById, - findUserById, - hasUser, - updateUserById, - hasUserWithEmail, - hasUserWithPhone, - }, + users: { deleteUserById, findUserById, hasUser, hasUserWithEmail, hasUserWithPhone }, } = queries; const { users: { @@ -40,6 +33,7 @@ export default function adminUserBasicsRoutes( verifyUserPassword, signOutUser, findUserSsoIdentities, + updateUserById, }, } = libraries; diff --git a/packages/core/src/routes/admin-user/mfa-verifications.test.ts b/packages/core/src/routes/admin-user/mfa-verifications.test.ts index 84c85a2f6..3439504dd 100644 --- a/packages/core/src/routes/admin-user/mfa-verifications.test.ts +++ b/packages/core/src/routes/admin-user/mfa-verifications.test.ts @@ -8,7 +8,6 @@ import { mockUserTotpMfaVerification, mockUserWithMfaVerifications, } from '#src/__mocks__/index.js'; -import { type InsertUserResult } from '#src/libraries/user.js'; import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import { MockTenant, type Partial2 } from '#src/test-utils/tenant.js'; @@ -23,12 +22,6 @@ const mockedQueries = { hasUser: jest.fn(async () => mockHasUser()), hasUserWithEmail: jest.fn(async () => mockHasUserWithEmail()), hasUserWithPhone: jest.fn(async () => mockHasUserWithPhone()), - updateUserById: jest.fn( - async (_, data: Partial): Promise => ({ - ...mockUser, - ...data, - }) - ), deleteUserById: jest.fn(), deleteUserIdentity: jest.fn(), }, @@ -38,7 +31,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'), @@ -47,25 +40,31 @@ 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 => [ - { +const mockLibraries = { + users: { + generateUserId: jest.fn(async () => 'fooId'), + insertUser: jest.fn( + async (user: CreateUser): Promise => ({ ...mockUser, ...removeUndefinedKeys(user), // No undefined values will be returned from database - }, - { organizationIds: [] }, - ] - ), -} satisfies Partial; + }) + ), + updateUserById: jest.fn( + async (_, data: Partial): Promise => ({ + ...mockUser, + ...data, + }) + ), + addUserMfaVerification: jest.fn(), + }, +} satisfies Partial2; + +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(() => { diff --git a/packages/core/src/routes/admin-user/mfa-verifications.ts b/packages/core/src/routes/admin-user/mfa-verifications.ts index bbcd5e0af..ed00c44b6 100644 --- a/packages/core/src/routes/admin-user/mfa-verifications.ts +++ b/packages/core/src/routes/admin-user/mfa-verifications.ts @@ -21,12 +21,12 @@ export default function adminUserMfaVerificationsRoutes): Promise => ({ - ...mockUser, - ...data, - }) - ), hasUserWithIdentity: mockHasUserWithIdentity, deleteUserById: jest.fn(), deleteUserIdentity: jest.fn(), @@ -57,6 +51,12 @@ const usersLibraries = { { organizationIds: [] }, ] ), + updateUserById: jest.fn( + async (_, data: Partial): Promise => ({ + ...mockUser, + ...data, + }) + ), } satisfies Partial; const mockGetLogtoConnectors = jest.fn(async () => mockLogtoConnectorList); @@ -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')); diff --git a/packages/core/src/routes/admin-user/social.ts b/packages/core/src/routes/admin-user/social.ts index 888d05130..340353ada 100644 --- a/packages/core/src/routes/admin-user/social.ts +++ b/packages/core/src/routes/admin-user/social.ts @@ -20,7 +20,10 @@ export default function adminUserSocialRoutes( ) { const { queries: { - users: { findUserById, updateUserById, hasUserWithIdentity, deleteUserIdentity }, + users: { findUserById, hasUserWithIdentity, deleteUserIdentity }, + }, + libraries: { + users: { updateUserById }, }, connectors: { getLogtoConnectorById }, } = tenant; diff --git a/packages/core/src/routes/interaction/actions/submit-interaction.mfa.test.ts b/packages/core/src/routes/interaction/actions/submit-interaction.mfa.test.ts index c02d179e4..f7e4544e1 100644 --- a/packages/core/src/routes/interaction/actions/submit-interaction.mfa.test.ts +++ b/packages/core/src/routes/interaction/actions/submit-interaction.mfa.test.ts @@ -44,19 +44,19 @@ const userQueries = { identities: { google: { userId: 'googleId', details: {} } }, mfaVerifications: [], }), - updateUserById: jest.fn(), hasActiveUsers: jest.fn().mockResolvedValue(true), hasUserWithEmail: jest.fn().mockResolvedValue(false), hasUserWithPhone: jest.fn().mockResolvedValue(false), }; -const { hasActiveUsers, updateUserById } = userQueries; +const { hasActiveUsers } = userQueries; const userLibraries = { generateUserId: jest.fn().mockResolvedValue('uid'), insertUser: jest.fn().mockResolvedValue([{}, { organizationIds: [] }]), + updateUserById: jest.fn(), }; -const { generateUserId, insertUser } = userLibraries; +const { generateUserId, insertUser, updateUserById } = userLibraries; const submitInteraction = await pickDefault(import('./submit-interaction.js')); const now = Date.now(); diff --git a/packages/core/src/routes/interaction/actions/submit-interaction.test.ts b/packages/core/src/routes/interaction/actions/submit-interaction.test.ts index 90f6a384f..62dd3264b 100644 --- a/packages/core/src/routes/interaction/actions/submit-interaction.test.ts +++ b/packages/core/src/routes/interaction/actions/submit-interaction.test.ts @@ -53,21 +53,21 @@ const userQueries = { identities: { google: { userId: 'googleId', details: {} } }, mfaVerifications: [], }), - updateUserById: jest.fn(async (id: string, user: Partial) => user as User), hasActiveUsers: jest.fn().mockResolvedValue(true), hasUserWithEmail: jest.fn().mockResolvedValue(false), hasUserWithPhone: jest.fn().mockResolvedValue(false), }; -const { hasActiveUsers, updateUserById, hasUserWithEmail, hasUserWithPhone } = userQueries; +const { hasActiveUsers, hasUserWithEmail, hasUserWithPhone } = userQueries; const userLibraries = { generateUserId: jest.fn().mockResolvedValue('uid'), insertUser: jest.fn( async (user: CreateUser): Promise => [user as User, { organizationIds: [] }] ), + updateUserById: jest.fn(async (id: string, user: Partial) => user as User), }; -const { generateUserId, insertUser } = userLibraries; +const { generateUserId, insertUser, updateUserById } = userLibraries; const submitInteraction = await pickDefault(import('./submit-interaction.js')); const now = Date.now(); diff --git a/packages/core/src/routes/interaction/actions/submit-interaction.ts b/packages/core/src/routes/interaction/actions/submit-interaction.ts index 36f428f42..a44d84de8 100644 --- a/packages/core/src/routes/interaction/actions/submit-interaction.ts +++ b/packages/core/src/routes/interaction/actions/submit-interaction.ts @@ -213,8 +213,9 @@ async function handleSubmitSignIn( tenantContext: TenantContext, log?: LogEntry ) { - const { provider, queries } = tenantContext; - const { findUserById, updateUserById } = queries.users; + const { provider, queries, libraries } = tenantContext; + const { findUserById } = queries.users; + const { updateUserById } = libraries.users; const { accountId } = interaction; log?.append({ userId: accountId }); @@ -262,8 +263,8 @@ export default async function submitInteraction( tenantContext: TenantContext, log?: LogEntry ) { - const { provider, queries } = tenantContext; - const { updateUserById } = queries.users; + const { provider, libraries } = tenantContext; + const { updateUserById } = libraries.users; const { event, profile } = interaction; if (event === InteractionEvent.Register) { diff --git a/packages/core/src/routes/interaction/mfa.test.ts b/packages/core/src/routes/interaction/mfa.test.ts index 5a0f93c5c..e6ef69b86 100644 --- a/packages/core/src/routes/interaction/mfa.test.ts +++ b/packages/core/src/routes/interaction/mfa.test.ts @@ -76,6 +76,11 @@ const tenantContext = new MockTenant( }, users: { findUserById: jest.fn().mockResolvedValue(mockUserWithMfaVerifications), + }, + }, + undefined, + { + users: { updateUserById, }, } diff --git a/packages/core/src/routes/interaction/mfa.ts b/packages/core/src/routes/interaction/mfa.ts index 3e08d7813..ec91d3563 100644 --- a/packages/core/src/routes/interaction/mfa.ts +++ b/packages/core/src/routes/interaction/mfa.ts @@ -30,7 +30,7 @@ export default function mfaRoutes( router: Router>>, tenant: TenantContext ) { - const { provider, queries } = tenant; + const { provider, queries, libraries } = tenant; // Set New MFA router.post( @@ -131,7 +131,7 @@ export default function mfaRoutes( // Update last used time const user = await queries.users.findUserById(accountId); - await queries.users.updateUserById(accountId, { + await libraries.users.updateUserById(accountId, { mfaVerifications: user.mfaVerifications.map((mfa) => { if (mfa.id !== verifiedMfa.id) { return mfa; diff --git a/packages/core/src/routes/interaction/utils/single-sign-on.test.ts b/packages/core/src/routes/interaction/utils/single-sign-on.test.ts index eca0d1ad7..2fdadb30a 100644 --- a/packages/core/src/routes/interaction/utils/single-sign-on.test.ts +++ b/packages/core/src/routes/interaction/utils/single-sign-on.test.ts @@ -80,7 +80,6 @@ describe('Single sign on util methods tests', () => { insert: insertUserSsoIdentityMock, }, users: { - updateUserById: updateUserMock, findUserByEmail: findUserByEmailMock, }, }, @@ -89,6 +88,7 @@ describe('Single sign on util methods tests', () => { users: { insertUser: insertUserMock, generateUserId: generateUserIdMock, + updateUserById: updateUserMock, }, ssoConnectors: { getAvailableSsoConnectors: getAvailableSsoConnectorsMock, diff --git a/packages/core/src/routes/interaction/utils/single-sign-on.ts b/packages/core/src/routes/interaction/utils/single-sign-on.ts index e67514a3b..2080d256c 100644 --- a/packages/core/src/routes/interaction/utils/single-sign-on.ts +++ b/packages/core/src/routes/interaction/utils/single-sign-on.ts @@ -14,6 +14,7 @@ import RequestError from '#src/errors/RequestError/index.js'; import { type WithLogContext } from '#src/middleware/koa-audit-log.js'; import { type WithInteractionDetailsContext } from '#src/routes/interaction/middleware/koa-interaction-details.js'; import { ssoConnectorFactories, type SingleSignOnConnectorSession } from '#src/sso/index.js'; +import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import type TenantContext from '#src/tenants/TenantContext.js'; import assertThat from '#src/utils/assert-that.js'; @@ -139,7 +140,7 @@ export const handleSsoAuthentication = async ( ssoAuthentication: SsoAuthenticationResult ): Promise => { const { createLog } = ctx; - const { provider, queries } = tenant; + const { provider, queries, libraries } = tenant; const { userSsoIdentities: userSsoIdentitiesQueries, users: usersQueries } = queries; const { issuer, userInfo } = ssoAuthentication; @@ -151,7 +152,7 @@ export const handleSsoAuthentication = async ( // SignIn if (userSsoIdentity) { - return signInWithSsoAuthentication(ctx, queries, { + return signInWithSsoAuthentication(ctx, queries, libraries, { connectorData, userSsoIdentity, ssoAuthentication, @@ -162,7 +163,7 @@ export const handleSsoAuthentication = async ( // SignIn and link with existing user account with a same email if (user) { - return signInAndLinkWithSsoAuthentication(ctx, queries, { + return signInAndLinkWithSsoAuthentication(ctx, queries, libraries, { connectorData, user, ssoAuthentication, @@ -180,7 +181,8 @@ export const handleSsoAuthentication = async ( const signInWithSsoAuthentication = async ( ctx: WithLogContext, - { userSsoIdentities: userSsoIdentitiesQueries, users: usersQueries }: Queries, + { userSsoIdentities: userSsoIdentitiesQueries }: Queries, + { users: usersLibraries }: Libraries, { connectorData: { id: connectorId, syncProfile }, userSsoIdentity: { id, userId }, @@ -211,7 +213,7 @@ const signInWithSsoAuthentication = async ( } : undefined; - await usersQueries.updateUserById(userId, { + await usersLibraries.updateUserById(userId, { ...syncingProfile, lastSignInAt: Date.now(), }); @@ -234,7 +236,8 @@ const signInWithSsoAuthentication = async ( const signInAndLinkWithSsoAuthentication = async ( ctx: WithLogContext, - { userSsoIdentities: userSsoIdentitiesQueries, users: usersQueries }: Queries, + { userSsoIdentities: userSsoIdentitiesQueries }: Queries, + { users: usersLibraries }: Libraries, { connectorData: { id: connectorId, syncProfile }, user: { id: userId }, @@ -269,7 +272,7 @@ const signInAndLinkWithSsoAuthentication = async ( } : undefined; - await usersQueries.updateUserById(userId, { + await usersLibraries.updateUserById(userId, { ...syncingProfile, lastSignInAt: Date.now(), }); diff --git a/packages/core/src/routes/interaction/verifications/mfa-payload-verification.test.ts b/packages/core/src/routes/interaction/verifications/mfa-payload-verification.test.ts index c3a6d5b82..59dad687e 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-payload-verification.test.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-payload-verification.test.ts @@ -24,12 +24,20 @@ const { mockEsm } = createMockUtils(jest); const findUserById = jest.fn(); const updateUserById = jest.fn(); -const tenantContext = new MockTenant(undefined, { - users: { - findUserById, - updateUserById, +const tenantContext = new MockTenant( + undefined, + { + users: { + findUserById, + }, }, -}); + undefined, + { + users: { + updateUserById, + }, + } +); const { validateTotpToken } = mockEsm('../utils/totp-validation.js', () => ({ validateTotpToken: jest.fn().mockReturnValue(true), diff --git a/packages/core/src/routes/interaction/verifications/mfa-payload-verification.ts b/packages/core/src/routes/interaction/verifications/mfa-payload-verification.ts index 4fb697516..afa9fb451 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-payload-verification.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-payload-verification.ts @@ -230,7 +230,7 @@ export async function verifyMfaPayloadVerification( if (newCounter !== undefined) { // Update the authenticator's counter in the DB to the newest count in the authentication - await tenant.queries.users.updateUserById(accountId, { + await tenant.libraries.users.updateUserById(accountId, { mfaVerifications: user.mfaVerifications.map((mfa) => { if (mfa.type !== MfaFactor.WebAuthn || mfa.id !== result.id) { return mfa; @@ -250,7 +250,7 @@ export async function verifyMfaPayloadVerification( const { id, type } = await verifyBackupCode(user.mfaVerifications, verifyMfaPayload); // Mark the backup code as used - await tenant.queries.users.updateUserById(accountId, { + await tenant.libraries.users.updateUserById(accountId, { mfaVerifications: user.mfaVerifications.map((mfa) => { if (mfa.id !== id || mfa.type !== MfaFactor.BackupCode) { return mfa; diff --git a/packages/core/src/routes/interaction/verifications/mfa-verification.backup-code.test.ts b/packages/core/src/routes/interaction/verifications/mfa-verification.backup-code.test.ts index c9014b907..f3d48c418 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-verification.backup-code.test.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-verification.backup-code.test.ts @@ -28,12 +28,20 @@ const { mockEsmWithActual } = createMockUtils(jest); const findUserById = jest.fn(); const updateUserById = jest.fn(); -const tenantContext = new MockTenant(undefined, { - users: { - findUserById, - updateUserById, +const tenantContext = new MockTenant( + undefined, + { + users: { + findUserById, + }, }, -}); + undefined, + { + users: { + updateUserById, + }, + } +); const mockBackupCodes = ['foo']; await mockEsmWithActual('../utils/backup-code-validation.js', () => ({ diff --git a/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts b/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts index e561293aa..42efc3a36 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts @@ -28,12 +28,18 @@ const { mockEsmWithActual } = createMockUtils(jest); const findUserById = jest.fn(); const updateUserById = jest.fn(); -const tenantContext = new MockTenant(undefined, { - users: { - findUserById, - updateUserById, +const tenantContext = new MockTenant( + undefined, + { + users: { + findUserById, + }, }, -}); + undefined, + { + users: { updateUserById }, + } +); const mockBackupCodes = ['foo']; await mockEsmWithActual('../utils/backup-code-validation.js', () => ({ diff --git a/packages/core/src/utils/user.ts b/packages/core/src/utils/user.ts index d7d5df661..026b73af6 100644 --- a/packages/core/src/utils/user.ts +++ b/packages/core/src/utils/user.ts @@ -1,4 +1,8 @@ import { MfaFactor, type User, type UserMfaVerificationResponse } from '@logto/schemas'; +import { parseE164PhoneNumberWithError, ParseError } from '@logto/shared/universal'; +import { tryThat } from '@silverhand/essentials'; + +import RequestError from '#src/errors/RequestError/index.js'; export const transpileUserMfaVerifications = ( mfaVerifications: User['mfaVerifications'] @@ -21,3 +25,25 @@ export const transpileUserMfaVerifications = ( return { id, createdAt, type }; }); }; + +export const getValidPhoneNumber = (phone: string) => + tryThat( + () => { + if (!phone) { + return phone; + } + + const phoneNumber = parseE164PhoneNumberWithError(phone); + if (!phoneNumber.isValid()) { + throw new RequestError({ code: 'user.invalid_phone', status: 422 }); + } + + return phoneNumber.number.slice(1); + }, + (error: unknown) => { + if (error instanceof ParseError) { + throw new RequestError({ code: 'user.invalid_phone', status: 422 }, error); + } + throw error; + } + ); diff --git a/packages/experience/package.json b/packages/experience/package.json index 98a87dd2d..dc5eaef83 100644 --- a/packages/experience/package.json +++ b/packages/experience/package.json @@ -27,6 +27,7 @@ "@logto/phrases": "workspace:^1.11.0", "@logto/phrases-experience": "workspace:^1.6.1", "@logto/schemas": "workspace:^1.17.0", + "@logto/shared": "workspace:^3.1.0", "@parcel/compressor-brotli": "2.9.3", "@parcel/compressor-gzip": "2.9.3", "@parcel/core": "2.9.3", @@ -67,7 +68,7 @@ "jest-transformer-svg": "^2.0.0", "js-base64": "^3.7.5", "ky": "^1.2.3", - "libphonenumber-js": "^1.10.51", + "libphonenumber-js": "^1.11.1", "lint-staged": "^15.0.0", "parcel": "2.9.3", "parcel-resolver-ignore": "^2.1.3", diff --git a/packages/experience/src/utils/country-code.ts b/packages/experience/src/utils/country-code.ts index 294c1b24c..1b031495a 100644 --- a/packages/experience/src/utils/country-code.ts +++ b/packages/experience/src/utils/country-code.ts @@ -1,10 +1,7 @@ +import { parseE164PhoneNumberWithError } from '@logto/shared/universal'; import i18next from 'i18next'; -import type { CountryCode, CountryCallingCode, E164Number } from 'libphonenumber-js/mobile'; -import { - getCountries, - getCountryCallingCode, - parsePhoneNumberWithError, -} from 'libphonenumber-js/mobile'; +import type { CountryCode, CountryCallingCode } from 'libphonenumber-js/mobile'; +import { getCountries, getCountryCallingCode } from 'libphonenumber-js/mobile'; export const fallbackCountryCode = 'US'; @@ -86,17 +83,9 @@ export const getCountryList = (): CountryMetaData[] => { ]; }; -export const parseE164Number = (value: string): E164Number | '' => { - if (!value || value.startsWith('+')) { - return value; - } - - return `+${value}`; -}; - export const formatPhoneNumberWithCountryCallingCode = (number: string) => { try { - const phoneNumber = parsePhoneNumberWithError(parseE164Number(number)); + const phoneNumber = parseE164PhoneNumberWithError(number); return `+${phoneNumber.countryCallingCode} ${phoneNumber.nationalNumber}`; } catch { @@ -106,7 +95,7 @@ export const formatPhoneNumberWithCountryCallingCode = (number: string) => { export const parsePhoneNumber = (value: string) => { try { - const phoneNumber = parsePhoneNumberWithError(parseE164Number(value)); + const phoneNumber = parseE164PhoneNumberWithError(value); return { countryCallingCode: phoneNumber.countryCallingCode, diff --git a/packages/experience/src/utils/form.ts b/packages/experience/src/utils/form.ts index fa65dad98..312c1a6d1 100644 --- a/packages/experience/src/utils/form.ts +++ b/packages/experience/src/utils/form.ts @@ -1,12 +1,13 @@ import { usernameRegEx, emailRegEx } from '@logto/core-kit'; import { SignInIdentifier } from '@logto/schemas'; +import { parseE164PhoneNumberWithError } from '@logto/shared/universal'; import i18next from 'i18next'; import type { TFuncKey } from 'i18next'; -import { parsePhoneNumberWithError, ParseError } from 'libphonenumber-js/mobile'; +import { ParseError } from 'libphonenumber-js/mobile'; import type { ErrorType } from '@/components/ErrorMessage'; import type { IdentifierInputType } from '@/components/InputFields/SmartInputField'; -import { parseE164Number, parsePhoneNumber } from '@/utils/country-code'; +import { parsePhoneNumber } from '@/utils/country-code'; const { t } = i18next; @@ -32,7 +33,7 @@ export const validateEmail = (email: string): ErrorType | undefined => { export const validatePhone = (value: string): ErrorType | undefined => { try { - const phoneNumber = parsePhoneNumberWithError(parseE164Number(value)); + const phoneNumber = parseE164PhoneNumberWithError(value); if (!phoneNumber.isValid()) { return 'invalid_phone'; diff --git a/packages/integration-tests/src/tests/api/admin-user.search.test.ts b/packages/integration-tests/src/tests/api/admin-user.search.test.ts index 23b5f6f75..f760da515 100644 --- a/packages/integration-tests/src/tests/api/admin-user.search.test.ts +++ b/packages/integration-tests/src/tests/api/admin-user.search.test.ts @@ -35,7 +35,6 @@ describe('admin console user search params', () => { 'jerry swift jr jr', ]; const emailSuffix = ['@gmail.com', '@foo.bar', '@geek.best']; - const phonePrefix = ['101', '102', '202']; // eslint-disable-next-line @silverhand/fp/no-mutation users = await Promise.all( @@ -47,8 +46,7 @@ describe('admin console user search params', () => { .map((segment) => segment[0]!.toUpperCase() + segment.slice(1)) .join(' '); const primaryEmail = username + emailSuffix[index % emailSuffix.length]!; - const primaryPhone = - phonePrefix[index % phonePrefix.length]! + index.toString().padStart(5, '0'); + const primaryPhone = '1310805' + index.toString().padStart(4, '0'); return createUserByAdmin({ username: prefix + username, primaryEmail, primaryPhone, name }); }) @@ -74,7 +72,7 @@ describe('admin console user search params', () => { }); it('should search primaryPhone', async () => { - const { headers, json } = await getUsers([['search', '%0000%']]); + const { headers, json } = await getUsers([['search', '%000%']]); expect(headers.get('total-number')).toEqual('10'); expect( diff --git a/packages/integration-tests/src/tests/api/admin-user.test.ts b/packages/integration-tests/src/tests/api/admin-user.test.ts index 5c4a60c4a..018950404 100644 --- a/packages/integration-tests/src/tests/api/admin-user.test.ts +++ b/packages/integration-tests/src/tests/api/admin-user.test.ts @@ -155,6 +155,14 @@ describe('admin console user management', () => { }); }); + it('should respond 422 when update user with invalid phone', async () => { + const user = await createUserByAdmin(); + await expectRejects(updateUser(user.id, { primaryPhone: '13110000000' }), { + code: 'user.invalid_phone', + status: 422, + }); + }); + it('should fail when update userinfo with conflict identifiers', async () => { const [username, primaryEmail, primaryPhone] = [ generateUsername(), diff --git a/packages/integration-tests/src/tests/api/role.user.test.ts b/packages/integration-tests/src/tests/api/role.user.test.ts index a046839d3..fc97360bb 100644 --- a/packages/integration-tests/src/tests/api/role.user.test.ts +++ b/packages/integration-tests/src/tests/api/role.user.test.ts @@ -11,6 +11,7 @@ import { } from '#src/api/role.js'; import { expectRejects } from '#src/helpers/index.js'; import { generateNewUserProfile } from '#src/helpers/user.js'; +import { generatePhone } from '#src/utils.js'; describe('roles users', () => { it('should get role users successfully and can get roles correctly (specifying exclude user)', async () => { @@ -38,7 +39,14 @@ describe('roles users', () => { name: 'user001', primaryEmail: 'user001@logto.io', }); - const user2 = await createUser({ name: 'user002', primaryPhone: '123456789' }); + + // Can not create user with invalid phone number. + await expectRejects(createUser({ name: 'user002', primaryPhone: '123456789' }), { + code: 'user.invalid_phone', + status: 422, + }); + const user2 = await createUser({ name: 'user002', primaryPhone: generatePhone() }); + const user3 = await createUser({ username: 'username3', primaryEmail: 'user3@logto.io' }); await assignUsersToRole([user1.id, user2.id, user3.id], role.id); diff --git a/packages/shared/package.json b/packages/shared/package.json index 5f8035307..53c06b47e 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -62,7 +62,7 @@ "@silverhand/essentials": "^2.9.1", "chalk": "^5.0.0", "find-up": "^7.0.0", - "libphonenumber-js": "^1.9.49", + "libphonenumber-js": "^1.11.1", "nanoid": "^5.0.1" } } diff --git a/packages/shared/src/utils/phone.ts b/packages/shared/src/utils/phone.ts index befbab1a7..c55e5ff25 100644 --- a/packages/shared/src/utils/phone.ts +++ b/packages/shared/src/utils/phone.ts @@ -1,4 +1,27 @@ -import { parsePhoneNumberWithError } from 'libphonenumber-js'; +import { parsePhoneNumberWithError } from 'libphonenumber-js/mobile'; +import type { E164Number } from 'libphonenumber-js/mobile'; + +export { ParseError } from 'libphonenumber-js/mobile'; + +function validateE164Number(value: string): asserts value is E164Number { + if (value && !value.startsWith('+')) { + throw new TypeError(`Invalid E164Number: ${value}`); + } +} + +const parseE164Number = (value: string): E164Number | '' => { + // If typeof `value` is string and `!value` is true, then `value` is an empty string. + if (!value) { + return ''; + } + + const result = value.startsWith('+') ? value : `+${value}`; + validateE164Number(result); + return result; +}; + +export const parseE164PhoneNumberWithError = (value: string) => + parsePhoneNumberWithError(parseE164Number(value)); /** * Parse phone number to number string. @@ -6,7 +29,7 @@ import { parsePhoneNumberWithError } from 'libphonenumber-js'; */ export const parsePhoneNumber = (phone: string) => { try { - return parsePhoneNumberWithError(phone).number.slice(1); + return parseE164PhoneNumberWithError(phone).number.slice(1); } catch { console.error(`Invalid phone number: ${phone}`); return phone; @@ -19,8 +42,7 @@ export const parsePhoneNumber = (phone: string) => { */ export const formatToInternationalPhoneNumber = (phone: string) => { try { - const phoneNumber = phone.startsWith('+') ? phone : `+${phone}`; - return parsePhoneNumberWithError(phoneNumber).formatInternational(); + return parseE164PhoneNumberWithError(phone).formatInternational(); } catch { console.error(`Invalid phone number: ${phone}`); return phone; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 515209c67..ffccf2b09 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -3032,8 +3032,8 @@ importers: specifier: ^1.2.3 version: 1.2.3 libphonenumber-js: - specifier: ^1.10.51 - version: 1.10.51 + specifier: ^1.11.1 + version: 1.11.1 lint-staged: specifier: ^15.0.0 version: 15.0.2 @@ -3563,6 +3563,9 @@ importers: '@logto/schemas': specifier: workspace:^1.17.0 version: link:../schemas + '@logto/shared': + specifier: workspace:^3.1.0 + version: link:../shared '@parcel/compressor-brotli': specifier: 2.9.3 version: 2.9.3(@parcel/core@2.9.3) @@ -3684,8 +3687,8 @@ importers: specifier: ^1.2.3 version: 1.2.3 libphonenumber-js: - specifier: ^1.10.51 - version: 1.10.51 + specifier: ^1.11.1 + version: 1.11.1 lint-staged: specifier: ^15.0.0 version: 15.0.2 @@ -4012,8 +4015,8 @@ importers: specifier: ^7.0.0 version: 7.0.0 libphonenumber-js: - specifier: ^1.9.49 - version: 1.10.51 + specifier: ^1.11.1 + version: 1.11.1 nanoid: specifier: ^5.0.1 version: 5.0.1 @@ -9897,8 +9900,8 @@ packages: resolution: {integrity: sha512-+bT2uH4E5LGE7h/n3evcS/sQlJXCpIp6ym8OWJ5eV6+67Dsql/LaaT7qJBAt2rzfoa/5QBGBhxDix1dMt2kQKQ==} engines: {node: '>= 0.8.0'} - libphonenumber-js@1.10.51: - resolution: {integrity: sha512-vY2I+rQwrDQzoPds0JeTEpeWzbUJgqoV0O4v31PauHBb/e+1KCXKylHcDnBMgJZ9fH9mErsEbROJY3Z3JtqEmg==} + libphonenumber-js@1.11.1: + resolution: {integrity: sha512-Wze1LPwcnzvcKGcRHFGFECTaLzxOtujwpf924difr5zniyYv1C2PiW0419qDR7m8lKDxsImu5mwxFuXhXpjmvw==} lightningcss-darwin-arm64@1.16.1: resolution: {integrity: sha512-/J898YSAiGVqdybHdIF3Ao0Hbh2vyVVj5YNm3NznVzTSvkOi3qQCAtO97sfmNz+bSRHXga7ZPLm+89PpOM5gAg==} @@ -20790,7 +20793,7 @@ snapshots: prelude-ls: 1.2.1 type-check: 0.4.0 - libphonenumber-js@1.10.51: {} + libphonenumber-js@1.11.1: {} lightningcss-darwin-arm64@1.16.1: optional: true