From 414c83f5ef9753b23b62c5f945fe4832c46b0cae Mon Sep 17 00:00:00 2001 From: Mendes Hugo Date: Tue, 19 Sep 2023 14:10:11 +0200 Subject: [PATCH] fix(user): forbid user update for other users --- .../src/backend/http/v1/users.http.spec.ts | 68 ++++++++----------- apps/backend/src/app/auth/auth.controller.ts | 23 +++---- apps/backend/src/app/auth/auth.user.param.ts | 23 +++++++ apps/backend/src/app/user/user.controller.ts | 27 +++++++- .../backend/src/app/user/user.service.spec.ts | 8 +-- apps/backend/src/app/user/user.service.ts | 9 ++- .../src/app/user/dtos/user.update.dto.ts | 4 +- 7 files changed, 97 insertions(+), 65 deletions(-) create mode 100644 apps/backend/src/app/auth/auth.user.param.ts diff --git a/apps/backend-e2e/src/backend/http/v1/users.http.spec.ts b/apps/backend-e2e/src/backend/http/v1/users.http.spec.ts index 02397b39..9cf04f48 100644 --- a/apps/backend-e2e/src/backend/http/v1/users.http.spec.ts +++ b/apps/backend-e2e/src/backend/http/v1/users.http.spec.ts @@ -13,9 +13,11 @@ describe(`Backend HTTP ${USERS_ENDPOINT_PREFIX}`, () => { const dbHelper = DbE2eHelper.getHelper("base"); const db = JSON.parse(JSON.stringify(dbHelper.db)) as Jsonify; + const { users } = db; + const [user] = users; beforeEach(async () => { - const [{ email, password }] = db.users; + const { email, password } = user; await dbHelper.refresh(); await client.setAuth(email, password); @@ -191,10 +193,9 @@ describe(`Backend HTTP ${USERS_ENDPOINT_PREFIX}`, () => { pagination: { total: beforeTotal } } = await client.findMany(); - const [, user] = db.users; - const toUpdate: UserUpdateDto = { email: `${user.email}.mail` }; + const toUpdate: UserUpdateDto = { firstname: `${user.firstname || "abc"}.123` }; const updated = await client.update(user._id, toUpdate); - expect(updated.email).toBe(toUpdate.email); + expect(updated.firstname).toBe(toUpdate.firstname); expect(updated._updated_at > user._updated_at).toBeTrue(); const { @@ -208,48 +209,27 @@ describe(`Backend HTTP ${USERS_ENDPOINT_PREFIX}`, () => { expect(updated).toStrictEqual(found); }); - it("should not update the date if there's no change", async () => { - const [, user] = db.users; - const toUpdate: UserUpdateDto = { email: user.email }; + it("should not be able to update a user's email", async () => { + const toUpdate = { email: `abc.${user.email}` } satisfies Pick; const updated = await client.update(user._id, toUpdate); - expect(updated).toStrictEqual(omit(user, ["password"])); + + expect(updated.email).toBe(user.email); + expect(updated.email).not.toBe(toUpdate.email); }); - it("should fail when a uniqueness constraint is not respected", async () => { - const [user1, user2] = db.users; + it("should not be able to update another user (404)", async () => { + const { _id } = users.find(({ _id }) => _id !== user._id)!; - const toUpdate: UserUpdateDto = { email: user1.email }; const response = await client - .updateResponse(user2._id, toUpdate) + .updateResponse(_id, { firstname: "abc123" } satisfies UserUpdateDto) .catch(({ response }: AxiosError) => response!); - expect(response.status).toBe(HttpStatusCode.Conflict); + expect(response.status).toBe(404); }); - // Use theses to test when the values are valid or not. - // Use another to determine the correct error(s) returned. - it("should fail with a BAD_REQUEST status code when the payload is invalid", async () => { - const [user] = db.users; - for (const toUpdate of [ - { email: "" }, - { email: "not an email" } - ] satisfies UserUpdateDto[]) { - const response = await client - .updateResponse(user._id, toUpdate) - .catch(({ response }: AxiosError) => response!); - expect(response.status).toBe(HttpStatusCode.BadRequest); - } - }); - - it("should fail with a BAD_REQUEST status code when the payload is incorrectly typed", async () => { - const [user] = db.users; - for (const toUpdate of [ - { email: 123 as unknown as string } - ] satisfies UserUpdateDto[]) { - const response = await client - .updateResponse(user._id, toUpdate) - .catch(({ response }: AxiosError) => response!); - expect(response.status).toBe(HttpStatusCode.BadRequest); - } + it("should not update the date if there's no change", async () => { + const toUpdate: UserUpdateDto = { firstname: user.firstname }; + const updated = await client.update(user._id, toUpdate); + expect(updated).toStrictEqual(omit(user, ["password"])); }); }); @@ -259,10 +239,11 @@ describe(`Backend HTTP ${USERS_ENDPOINT_PREFIX}`, () => { pagination: { total: beforeTotal } } = await client.findMany(); - const [, user] = db.users; const deleted = await client.delete(user._id); expect(deleted).toStrictEqual(omit(user, ["password"])); + await client.setAuth(users[1].email, users[1].password); + const { data: after, pagination: { total: afterTotal } @@ -271,6 +252,15 @@ describe(`Backend HTTP ${USERS_ENDPOINT_PREFIX}`, () => { expect(after.some(({ _id }) => _id === deleted._id)).toBeFalse(); }); + it("should not be able to delete another user (404)", async () => { + const { _id } = users.find(({ _id }) => _id !== user._id)!; + + const response = await client + .deleteResponse(_id) + .catch(({ response }: AxiosError) => response!); + expect(response.status).toBe(404); + }); + it("should not delete an unknown id", async () => { const id = Math.max(...db.users.map(({ _id }) => _id)) + 1; const response = await client diff --git a/apps/backend/src/app/auth/auth.controller.ts b/apps/backend/src/app/auth/auth.controller.ts index 716cea0c..f1e753e2 100644 --- a/apps/backend/src/app/auth/auth.controller.ts +++ b/apps/backend/src/app/auth/auth.controller.ts @@ -1,4 +1,4 @@ -import { Body, Controller, Get, Post, Req, Res, UseGuards } from "@nestjs/common"; +import { Body, Controller, Get, Post, Res, UseGuards } from "@nestjs/common"; import { ApiCreatedResponse, ApiOkResponse, ApiTags } from "@nestjs/swagger"; import { Response } from "express"; import { CookieOptions } from "express-serve-static-core"; @@ -11,16 +11,9 @@ import { authOptions } from "~/lib/common/options"; import { UseAuth } from "./auth.guard"; import { AuthLocalGuard } from "./auth.local-guard"; import { AuthService } from "./auth.service"; +import { AuthUser, AuthUserParam } from "./auth.user.param"; import { UserEntity } from "../user/user.entity"; -declare global { - // eslint-disable-next-line @typescript-eslint/no-namespace -- TODO: set it as a custom global type AND still working for e2e tests - namespace Express { - // eslint-disable-next-line @typescript-eslint/no-empty-interface -- For declaration merging - interface User extends UserEntity {} - } -} - /** * The controller for authentication. */ @@ -47,9 +40,9 @@ export class AuthController implements AuthEndpoint { @ApiOkResponse({ type: UserDto }) @Get(AuthEndpoints.PROFILE) @UseAuth() - public getProfile(@Req() req?: Express.Request): Promise { + public getProfile(@AuthUserParam() user?: AuthUser): Promise { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- From decorator and the Guard - return Promise.resolve(req!.user!); + return Promise.resolve(user!); } /** @inheritDoc */ @@ -58,11 +51,11 @@ export class AuthController implements AuthEndpoint { @UseGuards(AuthLocalGuard) public login( @Body() body: AuthLoginDto, - @Req() req?: Express.Request, + @AuthUserParam() user?: AuthUser, @Res({ passthrough: true }) res?: Response ) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- From guards and decorators (optional for the interface) - return this.loginOrRefresh(req!.user!, body, res!); + return this.loginOrRefresh(user!, body, res!); } /** @inheritDoc */ @@ -80,11 +73,11 @@ export class AuthController implements AuthEndpoint { @UseAuth() public refresh( @Body() body: AuthRefreshDto, - @Req() req?: Express.Request, + @AuthUserParam() user?: AuthUser, @Res({ passthrough: true }) res?: Response ) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- From guards and decorators (optional for the interface) - return this.loginOrRefresh(req!.user!, body, res!); + return this.loginOrRefresh(user!, body, res!); } private loginOrRefresh(user: UserEntity, body: AuthRefreshDto, res: Response) { diff --git a/apps/backend/src/app/auth/auth.user.param.ts b/apps/backend/src/app/auth/auth.user.param.ts new file mode 100644 index 00000000..f6a147b1 --- /dev/null +++ b/apps/backend/src/app/auth/auth.user.param.ts @@ -0,0 +1,23 @@ +import { createParamDecorator, ExecutionContext } from "@nestjs/common"; + +import { UserEntity } from "../user/user.entity"; + +/** + * The authenticated user data on Request + */ +export type AuthUser = UserEntity; + +/** + * Inject the connected user from the request if exists + */ +export const AuthUserParam = createParamDecorator( + (_, context) => context.switchToHttp().getRequest().user +); + +declare global { + // eslint-disable-next-line @typescript-eslint/no-namespace -- TODO: set it as a custom global type AND still working for e2e tests + namespace Express { + // eslint-disable-next-line @typescript-eslint/no-empty-interface -- For declaration merging + interface User extends AuthUser {} + } +} diff --git a/apps/backend/src/app/user/user.controller.ts b/apps/backend/src/app/user/user.controller.ts index 955cfab2..e8a1a15c 100644 --- a/apps/backend/src/app/user/user.controller.ts +++ b/apps/backend/src/app/user/user.controller.ts @@ -1,4 +1,14 @@ -import { Body, Controller, Delete, Get, Param, Patch, Post, Query } from "@nestjs/common"; +import { + Body, + Controller, + Delete, + Get, + NotFoundException, + Param, + Patch, + Post, + Query +} from "@nestjs/common"; import { ApiCreatedResponse, ApiOkResponse, ApiTags } from "@nestjs/swagger"; import { UserCreateDto, @@ -12,6 +22,7 @@ import { UserEndpoint, USERS_ENDPOINT_PREFIX } from "~/lib/common/app/user/endpo import { UserEntity } from "./user.entity"; import { UserService } from "./user.service"; import { UseAuth } from "../auth/auth.guard"; +import { AuthUser, AuthUserParam } from "../auth/auth.user.param"; /** * The main controller for [users]{@link UserDto}. @@ -51,14 +62,24 @@ export class UserController implements UserEndpoint { /** @inheritDoc */ @ApiOkResponse({ type: UserDto }) @Patch("/:id") - public update(@Param("id") id: number, @Body() body: UserUpdateDto) { + public update( + @Param("id") id: number, + @Body() body: UserUpdateDto, + @AuthUserParam() user?: AuthUser + ) { + if (user?._id !== id) { + return Promise.reject(new NotFoundException()); + } return this.service.update(id, body); } /** @inheritDoc */ @ApiOkResponse({ type: UserDto }) @Delete("/:id") - public delete(@Param("id") id: number) { + public delete(@Param("id") id: number, @AuthUserParam() user?: AuthUser) { + if (user?._id !== id) { + return Promise.reject(new NotFoundException()); + } return this.service.delete(id); } } diff --git a/apps/backend/src/app/user/user.service.spec.ts b/apps/backend/src/app/user/user.service.spec.ts index 44bf9f2d..e922c31f 100644 --- a/apps/backend/src/app/user/user.service.spec.ts +++ b/apps/backend/src/app/user/user.service.spec.ts @@ -1,9 +1,9 @@ import { NotFoundError, UniqueConstraintViolationException } from "@mikro-orm/core"; import { Test, TestingModule } from "@nestjs/testing"; -import { UserUpdateDto } from "~/lib/common/app/user/dtos"; import { omit } from "~/lib/common/utils/object-fns"; import { UserModule } from "./user.module"; +import { UserUpdateEntity } from "./user.service"; import { UserService } from "./user.service"; import { DbTestHelper } from "../../../test/db-test"; import { OrmModule } from "../../orm/orm.module"; @@ -114,7 +114,7 @@ describe("UserService", () => { // Update an entity and check its content const [user] = dbTest.db.users; - const toUpdate: UserUpdateDto = { email: `${user.email}-${user.email}` }; + const toUpdate: UserUpdateEntity = { email: `${user.email}-${user.email}` }; const updated = await service.update(user._id, toUpdate); expect(updated.email).toBe(toUpdate.email); @@ -130,7 +130,7 @@ describe("UserService", () => { it("should not update the date if there's no change", async () => { const [, user] = dbTest.db.users; - const toUpdate: UserUpdateDto = { email: user.email }; + const toUpdate: UserUpdateEntity = { email: user.email }; const updated = await service.update(user._id, toUpdate); expect(updated.toJSON()).toStrictEqual(omit(user, ["password"])); }); @@ -138,7 +138,7 @@ describe("UserService", () => { it("should fail when a uniqueness constraint is not respected", async () => { const [user1, user2] = dbTest.db.users; - const toUpdate: UserUpdateDto = { email: user1.email }; + const toUpdate: UserUpdateEntity = { email: user1.email }; await expect(service.update(user2._id, toUpdate)).rejects.toThrow( UniqueConstraintViolationException ); diff --git a/apps/backend/src/app/user/user.service.ts b/apps/backend/src/app/user/user.service.ts index ba424257..63b6cc3e 100644 --- a/apps/backend/src/app/user/user.service.ts +++ b/apps/backend/src/app/user/user.service.ts @@ -1,15 +1,20 @@ import { Injectable } from "@nestjs/common"; -import { UserCreateDto, UserUpdateDto } from "~/lib/common/app/user/dtos"; +import { UserCreateDto, UserDto, UserUpdateDto } from "~/lib/common/app/user/dtos"; import { UserEntity } from "./user.entity"; import { UserRepository } from "./user.repository"; import { EntityService } from "../_lib/entity"; +/** + * Update of an user from the service + */ +export interface UserUpdateEntity extends UserUpdateDto, Pick, "email"> {} + /** * Service to manages [users]{@link UserEntity}. */ @Injectable() -export class UserService extends EntityService { +export class UserService extends EntityService { /** * Constructor with "dependency injection" * diff --git a/libs/common/src/app/user/dtos/user.update.dto.ts b/libs/common/src/app/user/dtos/user.update.dto.ts index b732288c..16c07bd1 100644 --- a/libs/common/src/app/user/dtos/user.update.dto.ts +++ b/libs/common/src/app/user/dtos/user.update.dto.ts @@ -1,4 +1,4 @@ -import { PartialType } from "@nestjs/mapped-types"; +import { OmitType, PartialType } from "@nestjs/mapped-types"; import { UserCreateDto } from "./user.create.dto"; @@ -6,4 +6,4 @@ import { UserCreateDto } from "./user.create.dto"; * DTO used to update [users]{@link UserDto} * in its {@link UserEndpoint endpoint}. */ -export class UserUpdateDto extends PartialType(UserCreateDto) {} +export class UserUpdateDto extends PartialType(OmitType(UserCreateDto, ["email"])) {}