diff --git a/.changeset/nine-garlics-begin.md b/.changeset/nine-garlics-begin.md new file mode 100644 index 0000000000..b24448ab97 --- /dev/null +++ b/.changeset/nine-garlics-begin.md @@ -0,0 +1,5 @@ +--- +'@directus/api': patch +--- + +Resolved error indicating inability to remove last admin user during user updates via roles diff --git a/api/src/services/roles.test.ts b/api/src/services/roles.test.ts index 7bc3706a05..dfb6c0769c 100644 --- a/api/src/services/roles.test.ts +++ b/api/src/services/roles.test.ts @@ -122,6 +122,8 @@ describe('Integration Tests', () => { .select('select "id" from "directus_users" where "role" = ?') .responseOnce([{ id: userId1 }, { id: userId2 }]); + tracker.on.select('select count(*) as "count" from "directus_users"').responseOnce({ count: 1 }); + const result = await service.updateOne(adminRoleId, data); expect(result).toBe(adminRoleId); expect(superUpdateOne).toHaveBeenCalledOnce(); @@ -186,6 +188,10 @@ describe('Integration Tests', () => { tracker.on.select('select "admin_access" from "directus_roles"').responseOnce({ admin_access }); tracker.on.select('select "id" from "directus_users" where "role" = ?').responseOnce([{ id: userId1 }]); + tracker.on + .select('select count(*) as "count" from "directus_users" where "id" in') + .responseOnce({ count: 1 }); + const result = await service.updateOne(adminRoleId, data); expect(result).toBe(adminRoleId); expect(superUpdateOne).toHaveBeenCalledOnce(); @@ -202,6 +208,8 @@ describe('Integration Tests', () => { .select('select "id" from "directus_users" where "role" = ?') .responseOnce([{ id: userId1 }, { id: userId2 }]); + tracker.on.select('select count(*) as "count" from "directus_users"').responseOnce({ count: 1 }); + const result = await service.updateOne(adminRoleId, data); expect(result).toBe(adminRoleId); expect(superUpdateOne).toHaveBeenCalledOnce(); @@ -444,55 +452,6 @@ describe('Integration Tests', () => { expect(result).toBe(adminRoleId); expect(superUpdateOne).toHaveBeenCalledOnce(); }); - - it('having a removed last user that is not the last admin of system', async () => { - const data: Record = { - users: [], - }; - - tracker.on.select('select "admin_access" from "directus_roles"').responseOnce({ admin_access }); - tracker.on.select('select "id" from "directus_users" where "role" = ?').responseOnce([{ id: userId1 }]); - tracker.on.select('select count(*) as "count" from "directus_users"').responseOnce({ count: 1 }); - - const result = await service.updateOne(adminRoleId, data); - expect(result).toBe(adminRoleId); - expect(superUpdateOne).toHaveBeenCalledOnce(); - }); - - it('having a removed a last user that is the last admin of system', async () => { - const service = new RolesService({ - knex: db, - schema: testSchema, - accountability: { role: 'test', admin: false }, - }); - - const data: Record = { - users: [], - }; - - tracker.on.select('select "admin_access" from "directus_roles"').responseOnce({ admin_access }); - tracker.on.select('select "id" from "directus_users" where "role" = ?').responseOnce([{ id: userId1 }]); - tracker.on.select('select count(*) as "count" from "directus_users"').responseOnce({ count: 0 }); - - const promise = service.updateOne(adminRoleId, data); - - expect.assertions(5); // to ensure both assertions in the catch block are reached - - try { - await promise; - } catch (err: any) { - expect(err.message).toBe(`You don't have permission to access this.`); - expect(err).toBeInstanceOf(ForbiddenError); - } - - expect(superUpdateOne).toHaveBeenCalled(); - - expect(superUpdateOne.mock.lastCall![2].preMutationError.message).toBe( - `Can't process content. You can't remove the last admin user from the admin role.`, - ); - - expect(superUpdateOne.mock.lastCall![2].preMutationError).toBeInstanceOf(UnprocessableContentError); - }); }); describe('with an array of user objects', () => { @@ -562,55 +521,6 @@ describe('Integration Tests', () => { expect(result).toBe(adminRoleId); expect(superUpdateOne).toHaveBeenCalledOnce(); }); - - it('having a removed last user that is not the last admin of system', async () => { - const data: Record = { - users: [], - }; - - tracker.on.select('select "admin_access" from "directus_roles"').responseOnce({ admin_access }); - tracker.on.select('select "id" from "directus_users" where "role" = ?').responseOnce([{ id: userId1 }]); - tracker.on.select('select count(*) as "count" from "directus_users"').responseOnce({ count: 1 }); - - const result = await service.updateOne(adminRoleId, data); - expect(result).toBe(adminRoleId); - expect(superUpdateOne).toHaveBeenCalledOnce(); - }); - - it('having a removed a last user that is the last admin of system', async () => { - const service = new RolesService({ - knex: db, - schema: testSchema, - accountability: { role: 'test', admin: false }, - }); - - const data: Record = { - users: [], - }; - - tracker.on.select('select "admin_access" from "directus_roles"').responseOnce({ admin_access }); - tracker.on.select('select "id" from "directus_users" where "role" = ?').responseOnce([{ id: userId1 }]); - tracker.on.select('select count(*) as "count" from "directus_users"').responseOnce({ count: 0 }); - - const promise = service.updateOne(adminRoleId, data); - - expect.assertions(5); // to ensure both assertions in the catch block are reached - - try { - await promise; - } catch (err: any) { - expect(err.message).toBe(`You don't have permission to access this.`); - expect(err).toBeInstanceOf(ForbiddenError); - } - - expect(superUpdateOne).toHaveBeenCalled(); - - expect(superUpdateOne.mock.lastCall![2].preMutationError.message).toBe( - `Can't process content. You can't remove the last admin user from the admin role.`, - ); - - expect(superUpdateOne.mock.lastCall![2].preMutationError).toBeInstanceOf(UnprocessableContentError); - }); }); describe('with an alterations object', () => { @@ -710,63 +620,6 @@ describe('Integration Tests', () => { expect(result).toBe(adminRoleId); expect(superUpdateOne).toHaveBeenCalledOnce(); }); - - it('having a removed last user that is not the last admin of system', async () => { - const data: Record = { - users: { - create: [], - update: [], - delete: [userId1], - }, - }; - - tracker.on.select('select "admin_access" from "directus_roles"').responseOnce({ admin_access }); - tracker.on.select('select "id" from "directus_users" where "role" = ?').responseOnce([{ id: userId1 }]); - tracker.on.select('select count(*) as "count" from "directus_users"').responseOnce({ count: 1 }); - - const result = await service.updateOne(adminRoleId, data); - expect(result).toBe(adminRoleId); - expect(superUpdateOne).toHaveBeenCalledOnce(); - }); - - it('having a removed a last user that is the last admin of system', async () => { - const service = new RolesService({ - knex: db, - schema: testSchema, - accountability: { role: 'test', admin: false }, - }); - - const data: Record = { - users: { - create: [], - update: [], - delete: [userId1], - }, - }; - - tracker.on.select('select "admin_access" from "directus_roles"').responseOnce({ admin_access }); - tracker.on.select('select "id" from "directus_users" where "role" = ?').responseOnce([{ id: userId1 }]); - tracker.on.select('select count(*) as "count" from "directus_users"').responseOnce({ count: 0 }); - - const promise = service.updateOne(adminRoleId, data); - - expect.assertions(5); // to ensure both assertions in the catch block are reached - - try { - await promise; - } catch (err: any) { - expect(err.message).toBe(`You don't have permission to access this.`); - expect(err).toBeInstanceOf(ForbiddenError); - } - - expect(superUpdateOne).toHaveBeenCalled(); - - expect(superUpdateOne.mock.lastCall![2].preMutationError.message).toBe( - `Can't process content. You can't remove the last admin user from the admin role.`, - ); - - expect(superUpdateOne.mock.lastCall![2].preMutationError).toBeInstanceOf(UnprocessableContentError); - }); }); }); }); diff --git a/api/src/services/roles.ts b/api/src/services/roles.ts index f9d1963628..8fb16da119 100644 --- a/api/src/services/roles.ts +++ b/api/src/services/roles.ts @@ -1,6 +1,6 @@ -import type { Query } from '@directus/types'; import { ForbiddenError, UnprocessableContentError } from '@directus/errors'; -import type { AbstractServiceOptions, Alterations, Item, MutationOptions, PrimaryKey } from '../types/index.js'; +import type { Query, User } from '@directus/types'; +import type { AbstractServiceOptions, Alterations, MutationOptions, PrimaryKey } from '../types/index.js'; import { ItemsService } from './items.js'; import { PermissionsService } from './permissions.js'; import { PresetsService } from './presets.js'; @@ -20,51 +20,124 @@ export class RolesService extends ItemsService { .andWhere({ admin_access: true }) .first(); - const otherAdminRolesCount = +(otherAdminRoles?.count || 0); + const otherAdminRolesCount = Number(otherAdminRoles?.count ?? 0); if (otherAdminRolesCount === 0) { throw new UnprocessableContentError({ reason: `You can't delete the last admin role` }); } } - private async checkForOtherAdminUsers(key: PrimaryKey, users: Alterations | Item[]): Promise { + private async checkForOtherAdminUsers( + key: PrimaryKey, + users: Alterations | (string | Partial)[], + ): Promise { const role = await this.knex.select('admin_access').from('directus_roles').where('id', '=', key).first(); if (!role) throw new ForbiddenError(); - // The users that will now be in this new non-admin role - let userKeys: PrimaryKey[] = []; - - if (Array.isArray(users)) { - userKeys = users.map((user) => (typeof user === 'string' ? user : user['id'])).filter((id) => id); - } else { - userKeys = users.update.map((user) => user['id']).filter((id) => id); - } - - const usersThatWereInRoleBefore = (await this.knex.select('id').from('directus_users').where('role', '=', key)).map( + const usersBefore = (await this.knex.select('id').from('directus_users').where('role', '=', key)).map( (user) => user.id, ); - const usersThatAreRemoved = usersThatWereInRoleBefore.filter((id) => - Array.isArray(users) ? userKeys.includes(id) === false : users.delete.includes(id) === true, - ); + const usersAdded: (Partial & Pick)[] = []; + const usersUpdated: (Partial & Pick)[] = []; + const usersCreated: Partial[] = []; + const usersRemoved: string[] = []; - const usersThatAreAdded = Array.isArray(users) ? users : users.create; + if (Array.isArray(users)) { + const usersKept: string[] = []; - // If the role the users are moved to is an admin-role, and there's at least 1 (new) admin - // user, we don't have to check for other admin - // users - if ((role.admin_access === true || role.admin_access === 1) && usersThatAreAdded.length > 0) return; + for (const user of users) { + if (typeof user === 'string') { + if (usersBefore.includes(user)) { + usersKept.push(user); + } else { + usersAdded.push({ id: user }); + } + } else if (user.id) { + if (usersBefore.includes(user.id)) { + usersKept.push(user.id); + usersUpdated.push(user as Partial & Pick); + } else { + usersAdded.push(user as Partial & Pick); + } + } else { + usersCreated.push(user); + } + } + + usersRemoved.push(...usersBefore.filter((user) => !usersKept.includes(user))); + } else { + for (const user of users.update) { + if (usersBefore.includes(user['id'])) { + usersUpdated.push(user); + } else { + usersAdded.push(user); + } + } + + usersCreated.push(...users.create); + usersRemoved.push(...users.delete); + } + + if (role.admin_access === false || role.admin_access === 0) { + // Admin users might have moved in from other role, thus becoming non-admin + if (usersAdded.length > 0) { + const otherAdminUsers = await this.knex + .count('*', { as: 'count' }) + .from('directus_users') + .leftJoin('directus_roles', 'directus_users.role', 'directus_roles.id') + .whereNotIn('directus_users.id', usersAdded) + .andWhere({ 'directus_roles.admin_access': true, status: 'active' }) + .first(); + + const otherAdminUsersCount = Number(otherAdminUsers?.count ?? 0); + + if (otherAdminUsersCount === 0) { + throw new UnprocessableContentError({ reason: `You can't remove the last admin user from the admin role` }); + } + } + + return; + } + + // Only added or created new users + if (usersUpdated.length === 0 && usersRemoved.length === 0) return; + + // Active admin user(s) about to be created + if (usersCreated.some((user) => !('status' in user) || user.status === 'active')) return; + + const usersDeactivated = [...usersAdded, ...usersUpdated] + .filter((user) => 'status' in user && user.status !== 'active') + .map((user) => user.id); + + const usersAddedNonDeactivated = usersAdded + .filter((user) => !usersDeactivated.includes(user.id)) + .map((user) => user.id); + + // Active user(s) about to become admin + if (usersAddedNonDeactivated.length > 0) { + const userCount = await this.knex + .count('*', { as: 'count' }) + .from('directus_users') + .whereIn('id', usersAddedNonDeactivated) + .andWhere({ status: 'active' }) + .first(); + + if (Number(userCount?.count ?? 0) > 0) { + return; + } + } const otherAdminUsers = await this.knex .count('*', { as: 'count' }) .from('directus_users') - .whereNotIn('directus_users.id', [...userKeys, ...usersThatAreRemoved]) - .andWhere({ 'directus_roles.admin_access': true }) .leftJoin('directus_roles', 'directus_users.role', 'directus_roles.id') + .whereNotIn('directus_users.id', [...usersDeactivated, ...usersRemoved]) + .andWhere({ 'directus_roles.admin_access': true, status: 'active' }) .first(); - const otherAdminUsersCount = +(otherAdminUsers?.count || 0); + const otherAdminUsersCount = Number(otherAdminUsers?.count ?? 0); if (otherAdminUsersCount === 0) { throw new UnprocessableContentError({ reason: `You can't remove the last admin user from the admin role` }); diff --git a/api/src/types/items.ts b/api/src/types/items.ts index d72ee60a60..2495d10e9c 100644 --- a/api/src/types/items.ts +++ b/api/src/types/items.ts @@ -1,8 +1,3 @@ -/** - * I know this looks a little silly, but it allows us to explicitly differentiate between when we're - * expecting an item vs any other generic object. - */ - import type { DirectusError } from '@directus/errors'; import type { EventContext } from '@directus/types'; import type { MutationTracker } from '../services/items.js'; @@ -11,14 +6,10 @@ export type Item = Record; export type PrimaryKey = string | number; -export type Alterations = { - create: { - [key: string]: any; - }[]; - update: { - [key: string]: any; - }[]; - delete: (number | string)[]; +export type Alterations = { + create: Partial[]; + update: (K extends keyof T ? Partial & Pick : Partial)[]; + delete: (K extends keyof T ? T[K] : PrimaryKey)[]; }; export type MutationOptions = {