Skip to content

Commit

Permalink
fix(core): All calls to plainToInstance should exclude extraneous v…
Browse files Browse the repository at this point in the history
…alues (no-changelog) (#9338)
  • Loading branch information
netroy committed May 16, 2024
1 parent 75f7403 commit bdb572c
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 16 deletions.
6 changes: 4 additions & 2 deletions packages/cli/src/controllers/me.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export class MeController {
@Patch('/')
async updateCurrentUser(req: MeRequest.UserUpdate, res: Response): Promise<PublicUser> {
const { id: userId, email: currentEmail } = req.user;
const payload = plainToInstance(UserUpdatePayload, req.body);
const payload = plainToInstance(UserUpdatePayload, req.body, { excludeExtraneousValues: true });

const { email } = payload;
if (!email) {
Expand Down Expand Up @@ -227,7 +227,9 @@ export class MeController {
*/
@Patch('/settings')
async updateCurrentUserSettings(req: MeRequest.UserSettingsUpdate): Promise<User['settings']> {
const payload = plainToInstance(UserSettingsUpdatePayload, req.body);
const payload = plainToInstance(UserSettingsUpdatePayload, req.body, {
excludeExtraneousValues: true,
});
const { id } = req.user;

await this.userService.updateSettings(id, payload);
Expand Down
8 changes: 6 additions & 2 deletions packages/cli/src/controllers/users.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,9 @@ export class UsersController {
@Patch('/:id/settings')
@GlobalScope('user:update')
async updateUserSettings(req: UserRequest.UserSettingsUpdate) {
const payload = plainToInstance(UserSettingsUpdatePayload, req.body);
const payload = plainToInstance(UserSettingsUpdatePayload, req.body, {
excludeExtraneousValues: true,
});

const id = req.params.id;

Expand Down Expand Up @@ -293,7 +295,9 @@ export class UsersController {
const { NO_ADMIN_ON_OWNER, NO_USER, NO_OWNER_ON_OWNER } =
UsersController.ERROR_MESSAGES.CHANGE_ROLE;

const payload = plainToInstance(UserRoleChangePayload, req.body);
const payload = plainToInstance(UserRoleChangePayload, req.body, {
excludeExtraneousValues: true,
});
await validateEntity(payload);

const targetUser = await this.userRepository.findOne({
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/databases/entities/User.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export class User extends WithTimestamps implements IUser {
@AfterLoad()
@AfterUpdate()
computeIsPending(): void {
this.isPending = this.password === null;
this.isPending = this.password === null && this.role !== 'global:owner';
}

/**
Expand Down
7 changes: 7 additions & 0 deletions packages/cli/src/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type {
NodeError,
} from 'n8n-workflow';

import { Expose } from 'class-transformer';
import { IsBoolean, IsEmail, IsIn, IsOptional, IsString, Length } from 'class-validator';
import { NoXss } from '@db/utils/customValidators';
import type { PublicUser, SecretsProvider, SecretsProviderState } from '@/Interfaces';
Expand All @@ -21,31 +22,37 @@ import type { CredentialsEntity } from '@db/entities/CredentialsEntity';
import type { WorkflowHistory } from '@db/entities/WorkflowHistory';

export class UserUpdatePayload implements Pick<User, 'email' | 'firstName' | 'lastName'> {
@Expose()
@IsEmail()
email: string;

@Expose()
@NoXss()
@IsString({ message: 'First name must be of type string.' })
@Length(1, 32, { message: 'First name must be $constraint1 to $constraint2 characters long.' })
firstName: string;

@Expose()
@NoXss()
@IsString({ message: 'Last name must be of type string.' })
@Length(1, 32, { message: 'Last name must be $constraint1 to $constraint2 characters long.' })
lastName: string;
}

export class UserSettingsUpdatePayload {
@Expose()
@IsBoolean({ message: 'userActivated should be a boolean' })
@IsOptional()
userActivated: boolean;

@Expose()
@IsBoolean({ message: 'allowSSOManualLogin should be a boolean' })
@IsOptional()
allowSSOManualLogin?: boolean;
}

export class UserRoleChangePayload {
@Expose()
@IsIn(['global:admin', 'global:member'])
newRoleName: AssignableRole;
}
Expand Down
2 changes: 0 additions & 2 deletions packages/cli/test/integration/me.api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -356,13 +356,11 @@ const VALID_PATCH_ME_PAYLOADS = [
email: randomEmail(),
firstName: randomName(),
lastName: randomName(),
password: randomValidPassword(),
},
{
email: randomEmail().toUpperCase(),
firstName: randomName(),
lastName: randomName(),
password: randomValidPassword(),
},
];

Expand Down
19 changes: 10 additions & 9 deletions packages/cli/test/unit/controllers/me.controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,27 +87,28 @@ describe('MeController', () => {
id: '123',
password: 'password',
authIdentities: [],
role: 'global:owner',
role: 'global:member',
});
const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' };
const req = mock<MeRequest.UserUpdate>({ user, body: reqBody, browserId });
const req = mock<MeRequest.UserUpdate>({ user, browserId });
req.body = reqBody;
const res = mock<Response>();
userRepository.findOneOrFail.mockResolvedValue(user);
jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token');

// Add invalid data to the request payload
Object.assign(reqBody, { id: '0', role: '42' });
Object.assign(reqBody, { id: '0', role: 'global:owner' });

await controller.updateCurrentUser(req, res);

expect(userService.update).toHaveBeenCalled();

const updatedUser = userService.update.mock.calls[0][1];
expect(updatedUser.email).toBe(reqBody.email);
expect(updatedUser.firstName).toBe(reqBody.firstName);
expect(updatedUser.lastName).toBe(reqBody.lastName);
expect(updatedUser.id).not.toBe('0');
expect(updatedUser.role).not.toBe('42');
const updatePayload = userService.update.mock.calls[0][1];
expect(updatePayload.email).toBe(reqBody.email);
expect(updatePayload.firstName).toBe(reqBody.firstName);
expect(updatePayload.lastName).toBe(reqBody.lastName);
expect(updatePayload.id).toBeUndefined();
expect(updatePayload.role).toBeUndefined();
});

it('should throw BadRequestError if beforeUpdate hook throws BadRequestError', async () => {
Expand Down

0 comments on commit bdb572c

Please sign in to comment.