From 61f4e7fd2d0bbb12c9ee9beab0c2b44d78e9ffc2 Mon Sep 17 00:00:00 2001 From: wangsijie Date: Wed, 4 Jan 2023 15:39:27 +0800 Subject: [PATCH] feat: add users_roles table and remove role_names (#2525) --- .../console/src/components/UserName/index.tsx | 4 +- .../UserDetails/components/UserSettings.tsx | 3 +- .../console/src/pages/UserDetails/types.ts | 1 - .../console/src/pages/UserDetails/utils.ts | 3 +- packages/core/src/__mocks__/user.ts | 8 +- packages/core/src/libraries/user.ts | 26 ++++++- packages/core/src/oidc/scope.ts | 4 +- packages/core/src/queries/roles.test.ts | 42 +++++++++- packages/core/src/queries/roles.ts | 19 ++++- packages/core/src/queries/user.test.ts | 22 +----- packages/core/src/queries/user.ts | 69 ++++++++++++---- packages/core/src/queries/users-roles.ts | 38 +++++++++ packages/core/src/routes/admin-user.test.ts | 78 +++++++++++-------- packages/core/src/routes/admin-user.ts | 67 +++++++++++----- .../src/tests/api/admin-user.test.ts | 1 - .../alterations/next-1672815959-user-roles.ts | 75 ++++++++++++++++++ packages/schemas/src/seeds/roles.ts | 4 +- packages/schemas/src/types/user.ts | 6 +- packages/schemas/tables/users.sql | 1 - packages/schemas/tables/usersroles.sql | 5 ++ 20 files changed, 369 insertions(+), 107 deletions(-) create mode 100644 packages/core/src/queries/users-roles.ts create mode 100644 packages/schemas/alterations/next-1672815959-user-roles.ts create mode 100644 packages/schemas/tables/usersroles.sql diff --git a/packages/console/src/components/UserName/index.tsx b/packages/console/src/components/UserName/index.tsx index 7135a7fa6..1993d8bc5 100644 --- a/packages/console/src/components/UserName/index.tsx +++ b/packages/console/src/components/UserName/index.tsx @@ -1,4 +1,4 @@ -import type { User } from '@logto/schemas'; +import type { UserWithRoleNames } from '@logto/schemas'; import { UserRole } from '@logto/schemas'; import { useTranslation } from 'react-i18next'; import { Link } from 'react-router-dom'; @@ -14,7 +14,7 @@ type Props = { }; const UserName = ({ userId, isLink = false }: Props) => { - const { data, error } = useSWR(`/api/users/${userId}`); + const { data, error } = useSWR(`/api/users/${userId}`); const { t } = useTranslation(undefined, { keyPrefix: 'admin_console' }); const isLoading = !data && !error; diff --git a/packages/console/src/pages/UserDetails/components/UserSettings.tsx b/packages/console/src/pages/UserDetails/components/UserSettings.tsx index 539851267..26b680fd4 100644 --- a/packages/console/src/pages/UserDetails/components/UserSettings.tsx +++ b/packages/console/src/pages/UserDetails/components/UserSettings.tsx @@ -52,7 +52,7 @@ const UserSettings = ({ userData, userFormData, isDeleted, onUserUpdated }: Prop return; } - const { customData: inputCustomData, name, avatar, roleNames } = formData; + const { customData: inputCustomData, name, avatar } = formData; const parseResult = safeParseJson(inputCustomData); @@ -73,7 +73,6 @@ const UserSettings = ({ userData, userFormData, isDeleted, onUserUpdated }: Prop const payload: Partial = { name, avatar, - roleNames, customData: guardResult.data, }; diff --git a/packages/console/src/pages/UserDetails/types.ts b/packages/console/src/pages/UserDetails/types.ts index 188b3d80a..ee55b2126 100644 --- a/packages/console/src/pages/UserDetails/types.ts +++ b/packages/console/src/pages/UserDetails/types.ts @@ -4,6 +4,5 @@ export type UserDetailsForm = { username: string; name: string; avatar: string; - roleNames: string[]; customData: string; }; diff --git a/packages/console/src/pages/UserDetails/utils.ts b/packages/console/src/pages/UserDetails/utils.ts index d24bc4e83..99403b78f 100644 --- a/packages/console/src/pages/UserDetails/utils.ts +++ b/packages/console/src/pages/UserDetails/utils.ts @@ -4,7 +4,7 @@ import type { UserDetailsForm } from './types'; export const userDetailsParser = { toLocalForm: (data: User): UserDetailsForm => { - const { primaryEmail, primaryPhone, username, name, avatar, roleNames, customData } = data; + const { primaryEmail, primaryPhone, username, name, avatar, customData } = data; return { primaryEmail: primaryEmail ?? '', @@ -12,7 +12,6 @@ export const userDetailsParser = { username: username ?? '', name: name ?? '', avatar: avatar ?? '', - roleNames, customData: JSON.stringify(customData, null, 2), }; }, diff --git a/packages/core/src/__mocks__/user.ts b/packages/core/src/__mocks__/user.ts index e5362f04b..5396cf43d 100644 --- a/packages/core/src/__mocks__/user.ts +++ b/packages/core/src/__mocks__/user.ts @@ -1,8 +1,8 @@ -import type { User } from '@logto/schemas'; +import type { UserWithRoleNames } from '@logto/schemas'; import { userInfoSelectFields, UsersPasswordEncryptionMethod } from '@logto/schemas'; import { pick } from '@silverhand/essentials'; -export const mockUser: User = { +export const mockUser: UserWithRoleNames = { id: 'foo', username: 'foo', primaryEmail: 'foo@logto.io', @@ -25,7 +25,7 @@ export const mockUser: User = { export const mockUserResponse = pick(mockUser, ...userInfoSelectFields); export const mockPasswordEncrypted = 'a1b2c3'; -export const mockUserWithPassword: User = { +export const mockUserWithPassword: UserWithRoleNames = { id: 'id', username: 'username', primaryEmail: 'foo@logto.io', @@ -45,7 +45,7 @@ export const mockUserWithPassword: User = { isSuspended: false, }; -export const mockUserList: User[] = [ +export const mockUserList: UserWithRoleNames[] = [ { id: '1', username: 'foo1', diff --git a/packages/core/src/libraries/user.ts b/packages/core/src/libraries/user.ts index 9ef49041a..86d9a7482 100644 --- a/packages/core/src/libraries/user.ts +++ b/packages/core/src/libraries/user.ts @@ -1,6 +1,7 @@ import { buildIdGenerator } from '@logto/core-kit'; import type { User, CreateUser } from '@logto/schemas'; import { Users, UsersPasswordEncryptionMethod } from '@logto/schemas'; +import type { OmitAutoSetFields } from '@logto/shared'; import type { Nullable } from '@silverhand/essentials'; import { deduplicate } from '@silverhand/essentials'; import { argon2Verify } from 'hash-wasm'; @@ -9,8 +10,9 @@ import pRetry from 'p-retry'; import { buildInsertInto } from '#src/database/insert-into.js'; import envSet from '#src/env-set/index.js'; import RequestError from '#src/errors/RequestError/index.js'; -import { findRolesByRoleNames, insertRoles } from '#src/queries/roles.js'; +import { findRolesByRoleNames, insertRoles, findRoleByRoleName } from '#src/queries/roles.js'; import { hasUser, hasUserWithEmail, hasUserWithId, hasUserWithPhone } from '#src/queries/user.js'; +import { insertUsersRoles } from '#src/queries/users-roles.js'; import assertThat from '#src/utils/assert-that.js'; import { encryptPassword } from '#src/utils/password.js'; @@ -66,7 +68,10 @@ const insertUserQuery = buildInsertInto(Users, { // Temp solution since Hasura requires a role to proceed authn. // The source of default roles should be guarded and moved to database once we implement RBAC. -export const insertUser: typeof insertUserQuery = async ({ roleNames, ...rest }) => { +export const insertUser = async ({ + roleNames, + ...rest +}: OmitAutoSetFields & { roleNames?: string[] }) => { const computedRoleNames = deduplicate( (roleNames ?? []).concat(envSet.values.userDefaultRoleNames) ); @@ -88,7 +93,22 @@ export const insertUser: typeof insertUserQuery = async ({ roleNames, ...rest }) } } - return insertUserQuery({ roleNames: computedRoleNames, ...rest }); + const user = await insertUserQuery(rest); + + await Promise.all([ + computedRoleNames.map(async (roleName) => { + const role = await findRoleByRoleName(roleName); + + if (!role) { + // Not expected to happen, just inserted above, so is 500 + throw new Error(`Can not find role: ${roleName}`); + } + + await insertUsersRoles([{ userId: user.id, roleId: role.id }]); + }), + ]); + + return user; }; export const checkIdentifierCollision = async ( diff --git a/packages/core/src/oidc/scope.ts b/packages/core/src/oidc/scope.ts index bb30565cb..fe96e0121 100644 --- a/packages/core/src/oidc/scope.ts +++ b/packages/core/src/oidc/scope.ts @@ -1,10 +1,10 @@ import type { UserClaim } from '@logto/core-kit'; import { idTokenClaims, userinfoClaims, UserScope } from '@logto/core-kit'; -import type { User } from '@logto/schemas'; +import type { UserWithRoleNames } from '@logto/schemas'; import type { Nullable } from '@silverhand/essentials'; import type { ClaimsParameterMember } from 'oidc-provider'; -export const claimToUserKey: Readonly> = Object.freeze({ +export const claimToUserKey: Readonly> = Object.freeze({ name: 'name', picture: 'avatar', username: 'username', diff --git a/packages/core/src/queries/roles.test.ts b/packages/core/src/queries/roles.test.ts index f33497298..485b598b9 100644 --- a/packages/core/src/queries/roles.test.ts +++ b/packages/core/src/queries/roles.test.ts @@ -7,7 +7,12 @@ import envSet from '#src/env-set/index.js'; import type { QueryType } from '#src/utils/test-utils.js'; import { expectSqlAssert } from '#src/utils/test-utils.js'; -import { findAllRoles, findRolesByRoleNames } from './roles.js'; +import { + findAllRoles, + findRoleByRoleName, + findRolesByRoleIds, + findRolesByRoleNames, +} from './roles.js'; const { jest } = import.meta; @@ -40,6 +45,41 @@ describe('roles query', () => { await expect(findAllRoles()).resolves.toEqual([mockRole]); }); + it('findRolesByRoleIds', async () => { + const roleIds = [mockRole.id]; + const expectSql = sql` + select ${sql.join(Object.values(fields), sql`, `)} + from ${table} + where ${fields.id} in (${sql.join(roleIds, sql`, `)}) + `; + + mockQuery.mockImplementationOnce(async (sql, values) => { + expectSqlAssert(sql, expectSql.sql); + expect(values).toEqual([roleIds.join(', ')]); + + return createMockQueryResult([mockRole]); + }); + + await expect(findRolesByRoleIds(roleIds)).resolves.toEqual([mockRole]); + }); + + it('findRoleByRoleName', async () => { + const expectSql = sql` + select ${sql.join(Object.values(fields), sql`, `)} + from ${table} + where ${fields.name} = ${mockRole.name} + `; + + mockQuery.mockImplementationOnce(async (sql, values) => { + expectSqlAssert(sql, expectSql.sql); + expect(values).toEqual([mockRole.name]); + + return createMockQueryResult([mockRole]); + }); + + await expect(findRoleByRoleName(mockRole.name)).resolves.toEqual(mockRole); + }); + it('findRolesByRoleNames', async () => { const roleNames = ['foo']; diff --git a/packages/core/src/queries/roles.ts b/packages/core/src/queries/roles.ts index d989bde4a..2071b6501 100644 --- a/packages/core/src/queries/roles.ts +++ b/packages/core/src/queries/roles.ts @@ -12,6 +12,14 @@ export const findAllRoles = async () => select ${sql.join(Object.values(fields), sql`, `)} from ${table} `); +export const findRolesByRoleIds = async (roleIds: string[]) => + roleIds.length > 0 + ? envSet.pool.any(sql` + select ${sql.join(Object.values(fields), sql`, `)} + from ${table} + where ${fields.id} in (${sql.join(roleIds, sql`, `)}) + `) + : []; export const findRolesByRoleNames = async (roleNames: string[]) => envSet.pool.any(sql` @@ -20,11 +28,18 @@ export const findRolesByRoleNames = async (roleNames: string[]) => where ${fields.name} in (${sql.join(roleNames, sql`, `)}) `); +export const findRoleByRoleName = async (roleName: string) => + envSet.pool.maybeOne(sql` + select ${sql.join(Object.values(fields), sql`, `)} + from ${table} + where ${fields.name} = ${roleName} + `); + export const insertRoles = async (roles: Role[]) => envSet.pool.query(sql` - insert into ${table} (${fields.name}, ${fields.description}) values + insert into ${table} (${fields.id}, ${fields.name}, ${fields.description}) values ${sql.join( - roles.map(({ name, description }) => sql`(${name}, ${description})`), + roles.map(({ id, name, description }) => sql`(${id}, ${name}, ${description})`), sql`, ` )} `); diff --git a/packages/core/src/queries/user.test.ts b/packages/core/src/queries/user.test.ts index 5d9b884fc..b7035964d 100644 --- a/packages/core/src/queries/user.test.ts +++ b/packages/core/src/queries/user.test.ts @@ -1,4 +1,4 @@ -import { Users } from '@logto/schemas'; +import { Roles, Users, UsersRoles } from '@logto/schemas'; import { convertToIdentifiers } from '@logto/shared'; import { createMockPool, createMockQueryResult, sql } from 'slonik'; @@ -12,7 +12,6 @@ import { findUserByUsername, findUserByEmail, findUserByPhone, - findUserById, findUserByIdentity, hasUser, hasUserWithId, @@ -38,6 +37,8 @@ jest.spyOn(envSet, 'pool', 'get').mockReturnValue( describe('user query', () => { const { table, fields } = convertToIdentifiers(Users); + const { fields: rolesFields, table: rolesTable } = convertToIdentifiers(Roles); + const { fields: usersRolesFields, table: usersRolesTable } = convertToIdentifiers(UsersRoles); const dbvalue = { ...mockUser, roleNames: JSON.stringify(mockUser.roleNames), @@ -99,23 +100,6 @@ describe('user query', () => { await expect(findUserByPhone(mockUser.primaryPhone!)).resolves.toEqual(dbvalue); }); - it('findUserById', async () => { - const expectSql = sql` - select ${sql.join(Object.values(fields), sql`,`)} - from ${table} - where ${fields.id}=$1 - `; - - mockQuery.mockImplementationOnce(async (sql, values) => { - expectSqlAssert(sql, expectSql.sql); - expect(values).toEqual([mockUser.id]); - - return createMockQueryResult([dbvalue]); - }); - - await expect(findUserById(mockUser.id)).resolves.toEqual(dbvalue); - }); - it('findUserByIdentity', async () => { const target = 'github'; diff --git a/packages/core/src/queries/user.ts b/packages/core/src/queries/user.ts index 0d04b7484..484b751fe 100644 --- a/packages/core/src/queries/user.ts +++ b/packages/core/src/queries/user.ts @@ -1,5 +1,5 @@ -import type { User, CreateUser } from '@logto/schemas'; -import { SearchJointMode, Users, UserRole } from '@logto/schemas'; +import type { User, CreateUser, UserWithRoleNames } from '@logto/schemas'; +import { SearchJointMode, Users } from '@logto/schemas'; import type { OmitAutoSetFields } from '@logto/shared'; import { conditionalSql, convertToIdentifiers } from '@logto/shared'; import { sql } from 'slonik'; @@ -10,6 +10,9 @@ import { DeletionError } from '#src/errors/SlonikError/index.js'; import type { Search } from '#src/utils/search.js'; import { buildConditionsFromSearch } from '#src/utils/search.js'; +import { findRoleByRoleName, findRolesByRoleIds } from './roles.js'; +import { findUsersRolesByRoleId, findUsersRolesByUserId } from './users-roles.js'; + const { table, fields } = convertToIdentifiers(Users); export const findUserByUsername = async (username: string) => @@ -33,12 +36,22 @@ export const findUserByPhone = async (phone: string) => where ${fields.primaryPhone}=${phone} `); -export const findUserById = async (id: string) => - envSet.pool.one(sql` +export const findUserById = async (id: string): Promise => { + const user = await envSet.pool.one(sql` select ${sql.join(Object.values(fields), sql`,`)} from ${table} where ${fields.id}=${id} `); + const userRoles = await findUsersRolesByUserId(user.id); + + const roles = + userRoles.length > 0 ? await findRolesByRoleIds(userRoles.map(({ roleId }) => roleId)) : []; + + return { + ...user, + roleNames: roles.map(({ name }) => name), + }; +}; export const findUserByIdentity = async (target: string, userId: string) => envSet.pool.maybeOne( @@ -89,7 +102,7 @@ export const hasUserWithIdentity = async (target: string, userId: string) => ` ); -const buildUserConditions = (search: Search, hideAdminUser: boolean) => { +const buildUserConditions = (search: Search, excludeUserIds: string[]) => { const hasSearch = search.matches.length > 0; const searchFields = [ Users.fields.id, @@ -99,10 +112,11 @@ const buildUserConditions = (search: Search, hideAdminUser: boolean) => { Users.fields.name, ]; - if (hideAdminUser) { - // Cannot use \`= any()\` here since we didn't find the Slonik way to do so. Consider replacing Slonik. + if (excludeUserIds.length > 0) { + // FIXME @sijie temp solution to filter out admin users, + // It is too complex to use join return sql` - where not ${fields.roleNames} @> ${sql.jsonb([UserRole.Admin])} + where ${fields.id} not in (${sql.join(excludeUserIds, sql`, `)}) ${conditionalSql( hasSearch, () => sql`and (${buildConditionsFromSearch(search, searchFields)})` @@ -118,29 +132,42 @@ const buildUserConditions = (search: Search, hideAdminUser: boolean) => { export const defaultUserSearch = { matches: [], isCaseSensitive: false, joint: SearchJointMode.Or }; -export const countUsers = async (search: Search = defaultUserSearch, hideAdminUser = false) => +export const countUsers = async ( + search: Search = defaultUserSearch, + excludeUserIds: string[] = [] +) => envSet.pool.one<{ count: number }>(sql` select count(*) from ${table} - ${buildUserConditions(search, hideAdminUser)} + ${buildUserConditions(search, excludeUserIds)} `); export const findUsers = async ( limit: number, offset: number, search: Search, - hideAdminUser: boolean + excludeUserIds: string[] = [] ) => envSet.pool.any( sql` - select ${sql.join(Object.values(fields), sql`,`)} + select ${sql.join( + Object.values(fields).map((field) => sql`${table}.${field}`), + sql`,` + )} from ${table} - ${buildUserConditions(search, hideAdminUser)} + ${buildUserConditions(search, excludeUserIds)} limit ${limit} offset ${offset} ` ); +export const findUsersByIds = async (userIds: string[]) => + envSet.pool.any(sql` + select ${sql.join(Object.values(fields), sql`, `)} + from ${table} + where ${fields.id} in (${sql.join(userIds, sql`, `)}) + `); + const updateUser = buildUpdateWhere(Users, true); export const updateUserById = async ( @@ -186,3 +213,19 @@ export const getDailyNewUserCountsByTimeInterval = async ( and ${fields.createdAt} <= to_timestamp(${endTimeInclusive}::double precision / 1000) group by date(${fields.createdAt}) `); + +export const findUsersByRoleName = async (roleName: string) => { + const role = await findRoleByRoleName(roleName); + + if (!role) { + return []; + } + + const usersRoles = await findUsersRolesByRoleId(role.id); + + if (usersRoles.length === 0) { + return []; + } + + return findUsersByIds(usersRoles.map(({ userId }) => userId)); +}; diff --git a/packages/core/src/queries/users-roles.ts b/packages/core/src/queries/users-roles.ts new file mode 100644 index 000000000..c5f8677f9 --- /dev/null +++ b/packages/core/src/queries/users-roles.ts @@ -0,0 +1,38 @@ +import type { UsersRole } from '@logto/schemas'; +import { UsersRoles } from '@logto/schemas'; +import { convertToIdentifiers } from '@logto/shared'; +import { sql } from 'slonik'; + +import envSet from '#src/env-set/index.js'; + +const { table, fields } = convertToIdentifiers(UsersRoles); + +export const findUsersRolesByUserId = async (userId: string) => + envSet.pool.any(sql` + select ${sql.join(Object.values(fields), sql`,`)} + from ${table} + where ${fields.userId}=${userId} + `); + +export const findUsersRolesByRoleId = async (roleId: string) => + envSet.pool.any(sql` + select ${sql.join(Object.values(fields), sql`,`)} + from ${table} + where ${fields.roleId}=${roleId} + `); + +export const insertUsersRoles = async (usersRoles: UsersRole[]) => + envSet.pool.query(sql` + insert into ${table} (${fields.userId}, ${fields.roleId}) values + ${sql.join( + usersRoles.map(({ userId, roleId }) => sql`(${userId}, ${roleId})`), + sql`, ` + )} + `); + +export const deleteUsersRolesByUserIdAndRoleId = async (userId: string, roleId: string) => { + await envSet.pool.query(sql` + delete from ${table} + where ${fields.userId} = ${userId} and ${fields.roleId} = ${roleId} + `); +}; diff --git a/packages/core/src/routes/admin-user.test.ts b/packages/core/src/routes/admin-user.test.ts index c1aabb9eb..daecea810 100644 --- a/packages/core/src/routes/admin-user.test.ts +++ b/packages/core/src/routes/admin-user.test.ts @@ -171,14 +171,17 @@ describe('adminUserRoutes', () => { .send({ username, name, avatar, primaryEmail, primaryPhone }); expect(response.status).toEqual(200); - expect(response.body).toEqual({ - ...mockUserResponse, - primaryEmail, - primaryPhone, - username, - name, - avatar, - }); + expect(updateUserById).toHaveBeenCalledWith( + 'foo', + { + primaryEmail, + primaryPhone, + username, + name, + avatar, + }, + expect.anything() + ); }); it('PATCH /users/:userId should allow empty string for clearable fields', async () => { @@ -186,12 +189,15 @@ describe('adminUserRoutes', () => { .patch('/users/foo') .send({ name: '', avatar: '', primaryEmail: '' }); expect(response.status).toEqual(200); - expect(response.body).toEqual({ - ...mockUserResponse, - name: '', - avatar: '', - primaryEmail: '', - }); + expect(updateUserById).toHaveBeenCalledWith( + 'foo', + { + name: '', + avatar: '', + primaryEmail: '', + }, + expect.anything() + ); }); it('PATCH /users/:userId should allow null values for clearable fields', async () => { @@ -199,12 +205,15 @@ describe('adminUserRoutes', () => { .patch('/users/foo') .send({ name: null, username: null, primaryPhone: null }); expect(response.status).toEqual(200); - expect(response.body).toEqual({ - ...mockUserResponse, - name: null, - username: null, - primaryPhone: null, - }); + expect(updateUserById).toHaveBeenCalledWith( + 'foo', + { + name: null, + username: null, + primaryPhone: null, + }, + expect.anything() + ); }); it('PATCH /users/:userId should allow partial update', async () => { @@ -212,18 +221,24 @@ describe('adminUserRoutes', () => { const updateNameResponse = await userRequest.patch('/users/foo').send({ name }); expect(updateNameResponse.status).toEqual(200); - expect(updateNameResponse.body).toEqual({ - ...mockUserResponse, - name, - }); + expect(updateUserById).toHaveBeenCalledWith( + 'foo', + { + name, + }, + expect.anything() + ); const avatar = 'https://www.michael.png'; const updateAvatarResponse = await userRequest.patch('/users/foo').send({ avatar }); expect(updateAvatarResponse.status).toEqual(200); - expect(updateAvatarResponse.body).toEqual({ - ...mockUserResponse, - avatar, - }); + expect(updateUserById).toHaveBeenCalledWith( + 'foo', + { + avatar, + }, + expect.anything() + ); }); it('PATCH /users/:userId should throw when avatar URL is invalid', async () => { @@ -289,7 +304,7 @@ describe('adminUserRoutes', () => { ] ); await expect( - userRequest.patch('/users/foo').send({ roleNames: ['admin'] }) + userRequest.patch('/users/foo').send({ roleNames: ['superadmin'] }) ).resolves.toHaveProperty('status', 400); expect(findUserById).toHaveBeenCalledTimes(1); expect(updateUserById).not.toHaveBeenCalled(); @@ -300,10 +315,7 @@ describe('adminUserRoutes', () => { const response = await userRequest.patch('/users/foo').send({ roleNames }); expect(response.status).toEqual(200); - expect(response.body).toEqual({ - ...mockUserResponse, - roleNames, - }); + expect(response.body).toEqual(mockUserResponse); }); it('PATCH /users/:userId/password', async () => { diff --git a/packages/core/src/routes/admin-user.ts b/packages/core/src/routes/admin-user.ts index 7853d98c8..a09a54b36 100644 --- a/packages/core/src/routes/admin-user.ts +++ b/packages/core/src/routes/admin-user.ts @@ -26,7 +26,9 @@ import { updateUserById, hasUserWithEmail, hasUserWithPhone, + findUsersByRoleName, } from '#src/queries/user.js'; +import { deleteUsersRolesByUserIdAndRoleId, insertUsersRoles } from '#src/queries/users-roles.js'; import assertThat from '#src/utils/assert-that.js'; import { parseSearchParamsForSearch } from '#src/utils/search.js'; @@ -42,9 +44,12 @@ export default function adminUserRoutes(router: T) { const search = parseSearchParamsForSearch(searchParams); const hideAdminUser = isTrue(searchParams.get('hideAdminUser')); + const adminUsers = hideAdminUser ? await findUsersByRoleName(UserRole.Admin) : []; + const excludeUserIds = adminUsers.map(({ id }) => id); + const [{ count }, users] = await Promise.all([ - countUsers(search, hideAdminUser), - findUsers(limit, offset, search, hideAdminUser), + countUsers(search, excludeUserIds), + findUsers(limit, offset, search, excludeUserIds), ]); ctx.pagination.totalCount = count; @@ -194,31 +199,57 @@ export default function adminUserRoutes(router: T) { body, } = ctx.guard; - await findUserById(userId); + const user = await findUserById(userId); await checkIdentifierCollision(body, userId); + const { roleNames, ...userUpdates } = body; + // Temp solution to validate the existence of input roleNames - if (body.roleNames?.length) { - const { roleNames } = body; + if (roleNames) { const roles = await findRolesByRoleNames(roleNames); - if (roles.length !== roleNames.length) { - const resourcesNotFound = roleNames.filter( - (roleName) => !roles.some(({ name }) => roleName === name) + // Insert new roles + const newRoles = roleNames.filter((roleName) => !user.roleNames.includes(roleName)); + + if (newRoles.length > 0) { + await insertUsersRoles( + newRoles.map((roleName) => { + const role = roles.find(({ name }) => name === roleName); + + if (!role) { + throw new RequestError({ + status: 400, + code: 'user.invalid_role_names', + data: { + roleNames: roleName, + }, + }); + } + + return { + userId: user.id, + roleId: role.id, + }; + }) ); - throw new RequestError({ - status: 400, - code: 'user.invalid_role_names', - data: { - roleNames: resourcesNotFound.join(','), - }, - }); } + + // Remove old roles + const oldRoles = user.roleNames.filter((roleName) => !roleNames.includes(roleName)); + + await Promise.all( + oldRoles.map(async (roleName) => { + const role = roles.find(({ name }) => name === roleName); + + if (role) { + await deleteUsersRolesByUserIdAndRoleId(user.id, role.id); + } + }) + ); } - const user = await updateUserById(userId, body, 'replace'); - - ctx.body = pick(user, ...userInfoSelectFields); + const updatedUser = await updateUserById(userId, userUpdates, 'replace'); + ctx.body = pick(updatedUser, ...userInfoSelectFields); return next(); } 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 bf99e4ffb..1d1e9aab5 100644 --- a/packages/integration-tests/src/tests/api/admin-user.test.ts +++ b/packages/integration-tests/src/tests/api/admin-user.test.ts @@ -42,7 +42,6 @@ describe('admin console user management', () => { customData: { level: 1, }, - roleNames: ['admin'], }; const updatedUser = await updateUser(user.id, newUserData); diff --git a/packages/schemas/alterations/next-1672815959-user-roles.ts b/packages/schemas/alterations/next-1672815959-user-roles.ts new file mode 100644 index 000000000..70b46db0a --- /dev/null +++ b/packages/schemas/alterations/next-1672815959-user-roles.ts @@ -0,0 +1,75 @@ +import { sql } from 'slonik'; + +import type { AlterationScript } from '../lib/types/alteration.js'; + +const alteration: AlterationScript = { + up: async (pool) => { + await pool.query(sql` + create table users_roles ( + user_id varchar(21) not null references users (id) on update cascade on delete cascade, + role_id varchar(21) not null references roles (id) on update cascade on delete cascade, + primary key (user_id, role_id) + ); + `); + const users = await pool.any<{ id: string; roleNames: string[] }>(sql` + select * from users where jsonb_array_length(role_names) > 0 + `); + const roles = await pool.any<{ id: string; name: string }>(sql` + select * from roles + `); + + for (const user of users) { + for (const roleName of user.roleNames) { + if (!roleName) { + continue; + } + + const role = roles.find(({ name }) => name === roleName); + + if (!role) { + throw new Error(`Unable to find role: ${roleName}`); + } + + // eslint-disable-next-line no-await-in-loop + await pool.query(sql` + insert into users_roles (user_id, role_id) values (${user.id}, ${role.id}) + `); + } + } + + await pool.query(sql` + alter table users drop column role_names + `); + }, + down: async (pool) => { + await pool.query(sql` + alter table users add column role_names jsonb not null default '[]'::jsonb + `); + + const relations = await pool.any<{ userId: string; roleId: string }>(sql` + select * from users_roles + `); + const roles = await pool.any<{ id: string; name: string }>(sql` + select * from roles + `); + + for (const relation of relations) { + const role = roles.find(({ id }) => id === relation.roleId); + + if (!role) { + continue; + } + + // eslint-disable-next-line no-await-in-loop + await pool.query(sql` + update users set role_names = role_names || '[${role.name}]'::jsonb where id = ${relation.userId} + `); + } + + await pool.query(sql` + drop table users_roles; + `); + }, +}; + +export default alteration; diff --git a/packages/schemas/src/seeds/roles.ts b/packages/schemas/src/seeds/roles.ts index fc79930cd..569cb0dec 100644 --- a/packages/schemas/src/seeds/roles.ts +++ b/packages/schemas/src/seeds/roles.ts @@ -1,11 +1,13 @@ import type { CreateRole } from '../db-entries/index.js'; import { UserRole } from '../types/index.js'; +export const adminConsoleAdminRoleId = 'ac-admin-id'; + /** * Default Admin Role for Admin Console. */ export const defaultRole: Readonly = { - id: 'ac-admin-id', + id: adminConsoleAdminRoleId, name: UserRole.Admin, description: 'Admin role for Logto.', }; diff --git a/packages/schemas/src/types/user.ts b/packages/schemas/src/types/user.ts index 678a3305b..0e76124f9 100644 --- a/packages/schemas/src/types/user.ts +++ b/packages/schemas/src/types/user.ts @@ -1,4 +1,4 @@ -import type { CreateUser } from '../db-entries/index.js'; +import type { CreateUser, User } from '../db-entries/index.js'; export const userInfoSelectFields = Object.freeze([ 'id', @@ -7,7 +7,6 @@ export const userInfoSelectFields = Object.freeze([ 'primaryPhone', 'name', 'avatar', - 'roleNames', 'customData', 'identities', 'lastSignInAt', @@ -26,3 +25,6 @@ export type UserProfileResponse = UserInfo & { hasPasswordSet: boolean }; export enum UserRole { Admin = 'admin', } + +// FIXME @sijie remove this after RBAC is completed. +export type UserWithRoleNames = User & { roleNames: string[] }; diff --git a/packages/schemas/tables/users.sql b/packages/schemas/tables/users.sql index 3d988a435..0d01e28e0 100644 --- a/packages/schemas/tables/users.sql +++ b/packages/schemas/tables/users.sql @@ -10,7 +10,6 @@ create table users ( name varchar(128), avatar varchar(2048), application_id varchar(21), - role_names jsonb /* @use RoleNames */ not null default '[]'::jsonb, identities jsonb /* @use Identities */ not null default '{}'::jsonb, custom_data jsonb /* @use ArbitraryObject */ not null default '{}'::jsonb, is_suspended boolean not null default false, diff --git a/packages/schemas/tables/usersroles.sql b/packages/schemas/tables/usersroles.sql new file mode 100644 index 000000000..7cb88d9cb --- /dev/null +++ b/packages/schemas/tables/usersroles.sql @@ -0,0 +1,5 @@ +create table users_roles ( + user_id varchar(21) not null references users (id) on update cascade on delete cascade, + role_id varchar(21) not null references roles (id) on update cascade on delete cascade, + primary key (user_id, role_id) +);