Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jan 3, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bf46869
fix(core): Better input validation for the changeRole endpoint
netroy Dec 30, 2023
b9fbafa
switch to class-validator
netroy Jan 2, 2024
19d7418
improve typing on the frontend
netroy Jan 2, 2024
156caf0
switch to the `Licensed` decorator
netroy Jan 2, 2024
0912e57
fix the tests
netroy Jan 2, 2024
5a2a1b3
roleName -> newRoleName
netroy Jan 2, 2024
bb8f93a
lintfix
netroy Jan 2, 2024
a14d5e9
Update users.api.test.ts
netroy Jan 2, 2024
302fe7f
Merge remote-tracking branch 'origin/master' into fix-changeRole-inpu…
netroy Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/cli/src/GenericHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand All @@ -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);

Expand Down
86 changes: 29 additions & 57 deletions packages/cli/src/controllers/users.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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')
Expand All @@ -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;

Expand Down Expand Up @@ -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;

const { newRole } = req.body;

if (!newRole) {
throw new BadRequestError(MISSING_NEW_ROLE_KEY);
}
@Licensed('feat:advancedPermissions')
async changeGlobalRole(req: UserRequest.ChangeRole) {
const { NO_ADMIN_ON_OWNER, NO_USER, NO_OWNER_ON_OWNER } =
UsersController.ERROR_MESSAGES.CHANGE_ROLE;

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,
});

Expand Down
17 changes: 9 additions & 8 deletions packages/cli/src/requests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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';
Expand All @@ -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()
Expand All @@ -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 = {},
Expand Down Expand Up @@ -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 },
Expand Down
Loading
Loading