fix(core): Better input validation for the changeRole endpoint (#8189)

also refactored the code to
1. stop passing around `scope === 'global'`, since this code can be used
only for changing globalRole.
2. leak less details when input validation fails.

## Review / Merge checklist
- [x] PR title and summary are descriptive
- [x] Tests included
This commit is contained in:
कारतोफ्फेलस्क्रिप्ट™ 2024-01-03 09:33:35 +01:00 committed by GitHub
parent 11cda41214
commit cfe9525dd4
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 102 additions and 160 deletions

View file

@ -4,7 +4,7 @@ import type { WorkflowEntity } from '@db/entities/WorkflowEntity';
import type { CredentialsEntity } from '@db/entities/CredentialsEntity';
import type { TagEntity } from '@db/entities/TagEntity';
import type { User } from '@db/entities/User';
import type { UserUpdatePayload } from '@/requests';
import type { UserRoleChangePayload, UserUpdatePayload } from '@/requests';
import { BadRequestError } from './errors/response-errors/bad-request.error';
/**
@ -15,7 +15,13 @@ export function getSessionId(req: express.Request): string | undefined {
}
export async function validateEntity(
entity: WorkflowEntity | CredentialsEntity | TagEntity | User | UserUpdatePayload,
entity:
| WorkflowEntity
| CredentialsEntity
| TagEntity
| User
| UserUpdatePayload
| UserRoleChangePayload,
): Promise<void> {
const errors = await validate(entity);

View file

@ -2,13 +2,27 @@ import { In } from 'typeorm';
import { User } from '@db/entities/User';
import { SharedCredentials } from '@db/entities/SharedCredentials';
import { SharedWorkflow } from '@db/entities/SharedWorkflow';
import { RequireGlobalScope, Authorized, Delete, Get, RestController, Patch } from '@/decorators';
import { ListQuery, UserRequest, UserSettingsUpdatePayload } from '@/requests';
import {
RequireGlobalScope,
Authorized,
Delete,
Get,
RestController,
Patch,
Licensed,
} from '@/decorators';
import {
ListQuery,
UserRequest,
UserRoleChangePayload,
UserSettingsUpdatePayload,
} from '@/requests';
import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner';
import type { PublicUser, ITelemetryUserDeletionData } from '@/Interfaces';
import { AuthIdentity } from '@db/entities/AuthIdentity';
import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository';
import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository';
import { UserRepository } from '@db/repositories/user.repository';
import { plainToInstance } from 'class-transformer';
import { RoleService } from '@/services/role.service';
import { UserService } from '@/services/user.service';
@ -17,10 +31,9 @@ import { Logger } from '@/Logger';
import { UnauthorizedError } from '@/errors/response-errors/unauthorized.error';
import { NotFoundError } from '@/errors/response-errors/not-found.error';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { License } from '@/License';
import { ExternalHooks } from '@/ExternalHooks';
import { InternalHooks } from '@/InternalHooks';
import { UserRepository } from '@/databases/repositories/user.repository';
import { validateEntity } from '@/GenericHelpers';
@Authorized()
@RestController('/users')
@ -31,22 +44,17 @@ export class UsersController {
private readonly internalHooks: InternalHooks,
private readonly sharedCredentialsRepository: SharedCredentialsRepository,
private readonly sharedWorkflowRepository: SharedWorkflowRepository,
private readonly userRepository: UserRepository,
private readonly activeWorkflowRunner: ActiveWorkflowRunner,
private readonly roleService: RoleService,
private readonly userService: UserService,
private readonly license: License,
private readonly userRepository: UserRepository,
) {}
static ERROR_MESSAGES = {
CHANGE_ROLE: {
MISSING_NEW_ROLE_KEY: 'Expected `newRole` to exist',
MISSING_NEW_ROLE_VALUE: 'Expected `newRole` to have `name` and `scope`',
NO_USER: 'Target user not found',
NO_ADMIN_ON_OWNER: 'Admin cannot change role on global owner',
NO_OWNER_ON_OWNER: 'Owner cannot change role on global owner',
NO_USER_TO_OWNER: 'Cannot promote user to global owner',
NO_ADMIN_IF_UNLICENSED: 'Admin role is not available without a license',
},
} as const;
@ -298,74 +306,38 @@ export class UsersController {
@Patch('/:id/role')
@RequireGlobalScope('user:changeRole')
async changeRole(req: UserRequest.ChangeRole) {
const {
MISSING_NEW_ROLE_KEY,
MISSING_NEW_ROLE_VALUE,
NO_ADMIN_ON_OWNER,
NO_USER_TO_OWNER,
NO_USER,
NO_OWNER_ON_OWNER,
NO_ADMIN_IF_UNLICENSED,
} = UsersController.ERROR_MESSAGES.CHANGE_ROLE;
@Licensed('feat:advancedPermissions')
async changeGlobalRole(req: UserRequest.ChangeRole) {
const { NO_ADMIN_ON_OWNER, NO_USER, NO_OWNER_ON_OWNER } =
UsersController.ERROR_MESSAGES.CHANGE_ROLE;
const { newRole } = req.body;
if (!newRole) {
throw new BadRequestError(MISSING_NEW_ROLE_KEY);
}
if (!newRole.name || !newRole.scope) {
throw new BadRequestError(MISSING_NEW_ROLE_VALUE);
}
if (newRole.scope === 'global' && newRole.name === 'owner') {
throw new UnauthorizedError(NO_USER_TO_OWNER);
}
const payload = plainToInstance(UserRoleChangePayload, req.body);
await validateEntity(payload);
const targetUser = await this.userRepository.findOne({
where: { id: req.params.id },
relations: ['globalRole'],
});
if (targetUser === null) {
throw new NotFoundError(NO_USER);
}
if (
newRole.scope === 'global' &&
newRole.name === 'admin' &&
!this.license.isAdvancedPermissionsLicensed()
) {
throw new UnauthorizedError(NO_ADMIN_IF_UNLICENSED);
}
if (
req.user.globalRole.scope === 'global' &&
req.user.globalRole.name === 'admin' &&
targetUser.globalRole.scope === 'global' &&
targetUser.globalRole.name === 'owner'
) {
if (req.user.globalRole.name === 'admin' && targetUser.globalRole.name === 'owner') {
throw new UnauthorizedError(NO_ADMIN_ON_OWNER);
}
if (
req.user.globalRole.scope === 'global' &&
req.user.globalRole.name === 'owner' &&
targetUser.globalRole.scope === 'global' &&
targetUser.globalRole.name === 'owner'
) {
if (req.user.globalRole.name === 'owner' && targetUser.globalRole.name === 'owner') {
throw new UnauthorizedError(NO_OWNER_ON_OWNER);
}
const roleToSet = await this.roleService.findCached(newRole.scope, newRole.name);
const roleToSet = await this.roleService.findCached('global', payload.newRoleName);
await this.userService.update(targetUser.id, { globalRole: roleToSet });
await this.userService.update(targetUser.id, { globalRoleId: roleToSet.id });
void this.internalHooks.onUserRoleChange({
user: req.user,
target_user_id: targetUser.id,
target_user_new_role: [newRole.scope, newRole.name].join(' '),
target_user_new_role: ['global', payload.newRoleName].join(' '),
public_api: false,
});

View file

@ -16,7 +16,7 @@ import type {
IWorkflowSettings,
} from 'n8n-workflow';
import { IsBoolean, IsEmail, IsOptional, IsString, Length } from 'class-validator';
import { IsBoolean, IsEmail, IsIn, IsOptional, IsString, Length } from 'class-validator';
import { NoXss } from '@db/utils/customValidators';
import type {
PublicUser,
@ -25,7 +25,7 @@ import type {
SecretsProvider,
SecretsProviderState,
} from '@/Interfaces';
import type { Role, RoleNames, RoleScopes } from '@db/entities/Role';
import type { Role, RoleNames } from '@db/entities/Role';
import type { User } from '@db/entities/User';
import type { UserManagementMailer } from '@/UserManagement/email';
import type { Variables } from '@db/entities/Variables';
@ -47,6 +47,7 @@ export class UserUpdatePayload implements Pick<User, 'email' | 'firstName' | 'la
@Length(1, 32, { message: 'Last name must be $constraint1 to $constraint2 characters long.' })
lastName: string;
}
export class UserSettingsUpdatePayload {
@IsBoolean({ message: 'userActivated should be a boolean' })
@IsOptional()
@ -57,6 +58,11 @@ export class UserSettingsUpdatePayload {
allowSSOManualLogin?: boolean;
}
export class UserRoleChangePayload {
@IsIn(['member', 'admin'])
newRoleName: Exclude<RoleNames, 'user' | 'editor' | 'owner'>;
}
export type AuthlessRequest<
RouteParams = {},
ResponseBody = {},
@ -332,12 +338,7 @@ export declare namespace UserRequest {
{ transferId?: string; includeRole: boolean }
>;
export type ChangeRole = AuthenticatedRequest<
{ id: string },
{},
{ newRole?: { scope?: RoleScopes; name?: RoleNames } },
{}
>;
export type ChangeRole = AuthenticatedRequest<{ id: string }, {}, UserRoleChangePayload, {}>;
export type Get = AuthenticatedRequest<
{ id: string; email: string; identifier: string },

View file

@ -361,15 +361,8 @@ describe('PATCH /users/:id/role', () => {
let memberAgent: SuperAgentTest;
let authlessAgent: SuperAgentTest;
const {
MISSING_NEW_ROLE_KEY,
MISSING_NEW_ROLE_VALUE,
NO_ADMIN_ON_OWNER,
NO_USER_TO_OWNER,
NO_USER,
NO_OWNER_ON_OWNER,
NO_ADMIN_IF_UNLICENSED,
} = UsersController.ERROR_MESSAGES.CHANGE_ROLE;
const { NO_ADMIN_ON_OWNER, NO_USER, NO_OWNER_ON_OWNER } =
UsersController.ERROR_MESSAGES.CHANGE_ROLE;
const UNAUTHORIZED = 'Unauthorized';
@ -393,17 +386,31 @@ describe('PATCH /users/:id/role', () => {
describe('unauthenticated user', () => {
test('should receive 401', async () => {
const response = await authlessAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(401);
});
});
describe('Invalid payload should return 400 when newRoleName', () => {
test.each([
['is missing', {}],
['is `owner`', { newRoleName: 'owner' }],
['is an array', { newRoleName: ['owner'] }],
])('%s', async (_, payload) => {
const response = await adminAgent.patch(`/users/${member.id}/role`).send(payload);
expect(response.statusCode).toBe(400);
expect(response.body.message).toBe(
'newRoleName must be one of the following values: member, admin',
);
});
});
describe('member', () => {
test('should fail to demote owner to member', async () => {
const response = await memberAgent.patch(`/users/${owner.id}/role`).send({
newRole: { scope: 'global', name: 'member' },
newRoleName: 'member',
});
expect(response.statusCode).toBe(403);
@ -412,7 +419,7 @@ describe('PATCH /users/:id/role', () => {
test('should fail to demote owner to admin', async () => {
const response = await memberAgent.patch(`/users/${owner.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(403);
@ -421,7 +428,7 @@ describe('PATCH /users/:id/role', () => {
test('should fail to demote admin to member', async () => {
const response = await memberAgent.patch(`/users/${admin.id}/role`).send({
newRole: { scope: 'global', name: 'member' },
newRoleName: 'member',
});
expect(response.statusCode).toBe(403);
@ -430,7 +437,7 @@ describe('PATCH /users/:id/role', () => {
test('should fail to promote other member to owner', async () => {
const response = await memberAgent.patch(`/users/${otherMember.id}/role`).send({
newRole: { scope: 'global', name: 'owner' },
newRoleName: 'owner',
});
expect(response.statusCode).toBe(403);
@ -439,7 +446,7 @@ describe('PATCH /users/:id/role', () => {
test('should fail to promote other member to admin', async () => {
const response = await memberAgent.patch(`/users/${otherMember.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(403);
@ -448,7 +455,7 @@ describe('PATCH /users/:id/role', () => {
test('should fail to promote self to admin', async () => {
const response = await memberAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(403);
@ -457,7 +464,7 @@ describe('PATCH /users/:id/role', () => {
test('should fail to promote self to owner', async () => {
const response = await memberAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'owner' },
newRoleName: 'owner',
});
expect(response.statusCode).toBe(403);
@ -466,25 +473,11 @@ describe('PATCH /users/:id/role', () => {
});
describe('admin', () => {
test('should receive 400 on invalid payload', async () => {
const response = await adminAgent.patch(`/users/${member.id}/role`).send({});
expect(response.statusCode).toBe(400);
expect(response.body.message).toBe(MISSING_NEW_ROLE_KEY);
const _response = await adminAgent.patch(`/users/${member.id}/role`).send({
newRole: {},
});
expect(_response.statusCode).toBe(400);
expect(_response.body.message).toBe(MISSING_NEW_ROLE_VALUE);
});
test('should receive 404 on unknown target user', async () => {
const response = await adminAgent
.patch('/users/c2317ff3-7a9f-4fd4-ad2b-7331f6359260/role')
.send({
newRole: { scope: 'global', name: 'member' },
newRoleName: 'member',
});
expect(response.statusCode).toBe(404);
@ -493,7 +486,7 @@ describe('PATCH /users/:id/role', () => {
test('should fail to demote owner to admin', async () => {
const response = await adminAgent.patch(`/users/${owner.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(403);
@ -502,45 +495,27 @@ describe('PATCH /users/:id/role', () => {
test('should fail to demote owner to member', async () => {
const response = await adminAgent.patch(`/users/${owner.id}/role`).send({
newRole: { scope: 'global', name: 'member' },
newRoleName: 'member',
});
expect(response.statusCode).toBe(403);
expect(response.body.message).toBe(NO_ADMIN_ON_OWNER);
});
test('should fail to promote member to owner', async () => {
const response = await adminAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'owner' },
});
expect(response.statusCode).toBe(403);
expect(response.body.message).toBe(NO_USER_TO_OWNER);
});
test('should fail to promote admin to owner', async () => {
const response = await adminAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'owner' },
});
expect(response.statusCode).toBe(403);
expect(response.body.message).toBe(NO_USER_TO_OWNER);
});
test('should fail to promote member to admin if not licensed', async () => {
testServer.license.disable('feat:advancedPermissions');
const response = await adminAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(403);
expect(response.body.message).toBe(NO_ADMIN_IF_UNLICENSED);
expect(response.body.message).toBe('Plan lacks license for this feature');
});
test('should be able to demote admin to member', async () => {
const response = await adminAgent.patch(`/users/${otherAdmin.id}/role`).send({
newRole: { scope: 'global', name: 'member' },
newRoleName: 'member',
});
expect(response.statusCode).toBe(200);
@ -559,7 +534,7 @@ describe('PATCH /users/:id/role', () => {
test('should be able to demote self to member', async () => {
const response = await adminAgent.patch(`/users/${admin.id}/role`).send({
newRole: { scope: 'global', name: 'member' },
newRoleName: 'member',
});
expect(response.statusCode).toBe(200);
@ -578,7 +553,7 @@ describe('PATCH /users/:id/role', () => {
test('should be able to promote member to admin if licensed', async () => {
const response = await adminAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(200);
@ -599,7 +574,7 @@ describe('PATCH /users/:id/role', () => {
describe('owner', () => {
test('should fail to demote self to admin', async () => {
const response = await ownerAgent.patch(`/users/${owner.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(403);
@ -608,45 +583,27 @@ describe('PATCH /users/:id/role', () => {
test('should fail to demote self to member', async () => {
const response = await ownerAgent.patch(`/users/${owner.id}/role`).send({
newRole: { scope: 'global', name: 'member' },
newRoleName: 'member',
});
expect(response.statusCode).toBe(403);
expect(response.body.message).toBe(NO_OWNER_ON_OWNER);
});
test('should fail to promote admin to owner', async () => {
const response = await ownerAgent.patch(`/users/${admin.id}/role`).send({
newRole: { scope: 'global', name: 'owner' },
});
expect(response.statusCode).toBe(403);
expect(response.body.message).toBe(NO_USER_TO_OWNER);
});
test('should fail to promote member to owner', async () => {
const response = await ownerAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'owner' },
});
expect(response.statusCode).toBe(403);
expect(response.body.message).toBe(NO_USER_TO_OWNER);
});
test('should fail to promote member to admin if not licensed', async () => {
testServer.license.disable('feat:advancedPermissions');
const response = await ownerAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(403);
expect(response.body.message).toBe(NO_ADMIN_IF_UNLICENSED);
expect(response.body.message).toBe('Plan lacks license for this feature');
});
test('should be able to promote member to admin if licensed', async () => {
const response = await ownerAgent.patch(`/users/${member.id}/role`).send({
newRole: { scope: 'global', name: 'admin' },
newRoleName: 'admin',
});
expect(response.statusCode).toBe(200);
@ -665,7 +622,7 @@ describe('PATCH /users/:id/role', () => {
test('should be able to demote admin to member', async () => {
const response = await ownerAgent.patch(`/users/${admin.id}/role`).send({
newRole: { scope: 'global', name: 'member' },
newRoleName: 'member',
});
expect(response.statusCode).toBe(200);

View file

@ -7,7 +7,6 @@ import type {
} from '@/Interface';
import type { IDataObject } from 'n8n-workflow';
import { makeRestApiRequest } from '@/utils/apiUtils';
import type { ScopeLevel } from '@n8n/permissions';
export async function loginCurrentUser(
context: IRestApiContext,
@ -146,9 +145,14 @@ export async function submitPersonalizationSurvey(
await makeRestApiRequest(context, 'POST', '/me/survey', params as unknown as IDataObject);
}
export async function updateRole(
context: IRestApiContext,
{ id, role }: { id: string; role: { scope: ScopeLevel; name: IRole } },
): Promise<IUserResponse> {
return makeRestApiRequest(context, 'PATCH', `/users/${id}/role`, { newRole: role });
export interface UpdateGlobalRolePayload {
id: string;
newRoleName: Exclude<IRole, 'default' | 'owner'>;
}
export async function updateGlobalRole(
context: IRestApiContext,
{ id, newRoleName }: UpdateGlobalRolePayload,
): Promise<IUserResponse> {
return makeRestApiRequest(context, 'PATCH', `/users/${id}/role`, { newRoleName });
}

View file

@ -1,3 +1,4 @@
import type { UpdateGlobalRolePayload } from '@/api/users';
import {
changePassword,
deleteUser,
@ -15,7 +16,7 @@ import {
updateOtherUserSettings,
validatePasswordToken,
validateSignupToken,
updateRole,
updateGlobalRole,
} from '@/api/users';
import { PERSONALIZATION_MODAL_KEY, STORES } from '@/constants';
import type {
@ -40,7 +41,7 @@ import { useCloudPlanStore } from './cloudPlan.store';
import { disableMfa, enableMfa, getMfaQR, verifyMfaToken } from '@/api/mfa';
import { confirmEmail, getCloudUserInfo } from '@/api/cloudPlans';
import { useRBACStore } from '@/stores/rbac.store';
import type { Scope, ScopeLevel } from '@n8n/permissions';
import type { Scope } from '@n8n/permissions';
import { inviteUsers, acceptInvitation } from '@/api/invitation';
const isPendingUser = (user: IUserResponse | null) => !!user?.isPending;
@ -379,9 +380,9 @@ export const useUsersStore = defineStore(STORES.USERS, {
await confirmEmail(useRootStore().getRestApiContext);
},
async updateRole({ id, role }: { id: string; role: { scope: ScopeLevel; name: IRole } }) {
async updateGlobalRole({ id, newRoleName }: UpdateGlobalRolePayload) {
const rootStore = useRootStore();
await updateRole(rootStore.getRestApiContext, { id, role });
await updateGlobalRole(rootStore.getRestApiContext, { id, newRoleName });
await this.fetchUsers();
},
},

View file

@ -99,6 +99,7 @@ import { useSSOStore } from '@/stores/sso.store';
import { hasPermission } from '@/rbac/permissions';
import { ROLE } from '@/utils/userUtils';
import { useClipboard } from '@/composables/useClipboard';
import type { UpdateGlobalRolePayload } from '@/api/users';
export default defineComponent({
name: 'SettingsUsersView',
@ -280,8 +281,8 @@ export default defineComponent({
goToUpgradeAdvancedPermissions() {
void this.uiStore.goToUpgrade('settings-users', 'upgrade-advanced-permissions');
},
async onRoleChange(user: IUser, name: IRole) {
await this.usersStore.updateRole({ id: user.id, role: { scope: 'global', name } });
async onRoleChange(user: IUser, newRoleName: UpdateGlobalRolePayload['newRoleName']) {
await this.usersStore.updateGlobalRole({ id: user.id, newRoleName });
},
},
});