Skip to content

Commit

Permalink
feat: add phone number validation to user APIs
Browse files Browse the repository at this point in the history
  • Loading branch information
darcyYe committed May 17, 2024
1 parent 1b2359b commit af48649
Show file tree
Hide file tree
Showing 33 changed files with 277 additions and 139 deletions.
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
);
});
});
35 changes: 32 additions & 3 deletions packages/core/src/libraries/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { User, CreateUser, Scope, BindMfa, MfaVerification } from '@logto/s
import { MfaFactor, Users, UsersPasswordEncryptionMethod } from '@logto/schemas';
import { generateStandardShortId, generateStandardId } from '@logto/shared';
import type { Nullable } from '@silverhand/essentials';
import { deduplicate } from '@silverhand/essentials';
import { deduplicate, conditional } from '@silverhand/essentials';
import { argon2Verify, bcryptVerify, md5, sha1, sha256 } from 'hash-wasm';
import pRetry from 'p-retry';

Expand All @@ -14,6 +14,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
Expand Down Expand Up @@ -82,7 +83,7 @@ export const createUserLibrary = (queries: Queries) => {
hasUserWithId,
hasUserWithPhone,
findUsersByIds,
updateUserById,
updateUserById: updateUserByIdQuery,
findUserById,
},
usersRoles: { findUsersRolesByRoleId, findUsersRolesByUserId },
Expand All @@ -106,18 +107,45 @@ export const createUserLibrary = (queries: Queries) => {
{ retries, factor: 0 } // No need for exponential backoff
);

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

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L118 was not covered by tests
);

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

const insertUser = async (data: OmitAutoSetFields<CreateUser>, additionalRoleNames: string[]) => {
const roleNames = deduplicate([...EnvSet.values.userDefaultRoleNames, ...additionalRoleNames]);
const roles = await findRolesByRoleNames(roleNames);

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

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

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

View check run for this annotation

Codecov / codecov/patch

packages/core/src/libraries/user.ts#L134-L139

Added lines #L134 - L139 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 148 in packages/core/src/libraries/user.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/libraries/user.ts#L145-L148

Added lines #L145 - L148 were not covered by tests

if (roles.length > 0) {
const { insertUsersRoles } = createUsersRolesQueries(connection);
Expand Down Expand Up @@ -289,5 +317,6 @@ export const createUserLibrary = (queries: Queries) => {
addUserMfaVerification,
verifyUserPassword,
signOutUser,
updateUserById,
};
};
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 @@ -33,12 +33,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 @@ -64,8 +58,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 @@ -86,8 +79,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 @@ -22,14 +22,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 },
userSsoIdentities,
} = queries;
const {
Expand All @@ -39,6 +32,7 @@ export default function adminUserBasicsRoutes<T extends ManagementApiRouter>(
insertUser,
verifyUserPassword,
signOutUser,
updateUserById,
},
} = libraries;

Expand Down
41 changes: 22 additions & 19 deletions packages/core/src/routes/admin-user/mfa-verifications.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,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<CreateUser>): Promise<User> => ({
...mockUser,
...data,
})
),
deleteUserById: jest.fn(),
deleteUserIdentity: jest.fn(),
},
Expand All @@ -37,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'),
Expand All @@ -46,22 +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<User> => ({
...mockUser,
...removeUndefinedKeys(user), // No undefined values will be returned from database
})
),
} satisfies Partial<Libraries['users']>;
const mockLibraries = {
users: {
generateUserId: jest.fn(async () => 'fooId'),
insertUser: jest.fn(
async (user: CreateUser): Promise<User> => ({
...mockUser,
...removeUndefinedKeys(user), // No undefined values will be returned from database
})
),
updateUserById: jest.fn(
async (_, data: Partial<CreateUser>): Promise<User> => ({
...mockUser,
...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 @@ -32,12 +32,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 @@ -52,6 +46,12 @@ const usersLibraries = {
...user,
})
),
updateUserById: jest.fn(
async (_, data: Partial<CreateUser>): Promise<User> => ({
...mockUser,
...data,
})
),
} satisfies Partial<Libraries['users']>;

const mockGetLogtoConnectors = jest.fn(async () => mockLogtoConnectorList);
Expand All @@ -73,7 +73,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
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +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() };
const { generateUserId, insertUser } = userLibraries;
const userLibraries = {
generateUserId: jest.fn().mockResolvedValue('uid'),
updateUserById: jest.fn(),
insertUser: jest.fn(),
};
const { generateUserId, insertUser, updateUserById } = userLibraries;

const submitInteraction = await pickDefault(import('./submit-interaction.js'));
const now = Date.now();
Expand Down
Loading

0 comments on commit af48649

Please sign in to comment.