github/directus
1
1
Fork 0
mirror of https://github.com/directus/directus.git synced 2026-04-25 03:00:53 -04:00
Code Issues Packages Projects Releases Wiki Activity

Fix role update admin check (#21343)

Browse Source 
* Fix role update check for remaining admins

* Add changeset

* Fix default type for primary key on Alterations

* Update changeset

Co-authored-by: Daniel Biegler <DanielBiegler@users.noreply.github.com>

* updated type casting

* updated type casting

---------

Co-authored-by: Daniel Biegler <DanielBiegler@users.noreply.github.com>
Co-authored-by: Brainslug <tim@brainslug.nl>
This commit is contained in:
Pascal Jufer
2024-02-13 12:18:01 +01:00
committed by GitHub
parent 8ad86836d0
commit e373feafaa
4 changed files with 115 additions and 193 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/nine-garlics-begin.md Normal file
View File

@@ -0,0 +1,5 @@
---
'@directus/api': patch
---
Resolved error indicating inability to remove last admin user during user updates via roles

163
api/src/services/roles.test.ts
View File

@@ -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<string, any> = {
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<string, any> = {
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<string, any> = {
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<string, any> = {
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<string, any> = {
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<string, any> = {
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);
});
});
});
});

123
api/src/services/roles.ts
View File

@@ -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<void> {
private async checkForOtherAdminUsers(
key: PrimaryKey,
users: Alterations<User, 'id'> | (string | Partial<User>)[],
): Promise<void> {
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<User> & Pick<User, 'id'>)[] = [];
const usersUpdated: (Partial<User> & Pick<User, 'id'>)[] = [];
const usersCreated: Partial<User>[] = [];
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<User> & Pick<User, 'id'>);
} else {
usersAdded.push(user as Partial<User> & Pick<User, 'id'>);
}
} 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` });

17
api/src/types/items.ts
View File

@@ -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<string, any>;
export type PrimaryKey = string | number;
export type Alterations = {
create: {
[key: string]: any;
}[];
update: {
[key: string]: any;
}[];
delete: (number | string)[];
export type Alterations<T extends Item = Item, K extends keyof T | undefined = undefined> = {
create: Partial<T>[];
update: (K extends keyof T ? Partial<T> & Pick<T, K> : Partial<T>)[];
delete: (K extends keyof T ? T[K] : PrimaryKey)[];
};
export type MutationOptions = {