From f7cbeeefb87062ad17cc48206c7bad447bbd9350 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Wed, 22 Sep 2021 14:35:29 +0200 Subject: [PATCH 1/3] Add userProfile controller and PUT /userProfile/:id endpoint --- CHANGELOG.md | 4 + backend/core/src/auth/auth.module.ts | 3 +- backend/core/src/auth/authz_policy.csv | 4 +- .../controllers/user-profile.controller.ts | 31 +++++++ .../src/auth/controllers/user.controller.ts | 5 +- backend/core/src/auth/dto/user-profile.dto.ts | 46 ++++++++++ backend/core/src/auth/dto/user-update.dto.ts | 7 ++ .../core/src/auth/services/user.service.ts | 12 +-- backend/core/test/authz/authz.e2e-spec.ts | 4 +- backend/core/test/user/user.e2e-spec.ts | 59 ++++++++++++- backend/core/types/src/backend-swagger.ts | 87 ++++++++++++++++++- 11 files changed, 241 insertions(+), 21 deletions(-) create mode 100644 backend/core/src/auth/controllers/user-profile.controller.ts create mode 100644 backend/core/src/auth/dto/user-profile.dto.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 45612b2c9d..0c9f6ecfc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ All notable changes to this project will be documented in this file. The format - Add POST /users/invite endpoint and extend PUT /users/confirm with optional password change ([#1801](https://github.com/bloom-housing/bloom/pull/1801)) - Add `isPartner` filter to GET /user/list endpoint ([#1830](https://github.com/bloom-housing/bloom/pull/1830)) - Add logic for connecting newly created user account to existing applications (matching based on applicant.emailAddress) ([#1807](https://github.com/bloom-housing/bloom/pull/1807)) + - Added new userProfile resource and endpoint `PUT /userProfile/:id` suited specifically for users updating their own profiles ([#1862](https://github.com/bloom-housing/bloom/pull/1862)) + +- Changed: + - Endpoint `PUT /user/:id` is admin only now, because it allows edits over entire `user` table ([#1862](https://github.com/bloom-housing/bloom/pull/1862)) ## Frontend diff --git a/backend/core/src/auth/auth.module.ts b/backend/core/src/auth/auth.module.ts index 341552aed6..432a0987d6 100644 --- a/backend/core/src/auth/auth.module.ts +++ b/backend/core/src/auth/auth.module.ts @@ -17,6 +17,7 @@ import { EmailModule } from "../shared/email/email.module" import { PasswordService } from "./services/password.service" import { JurisdictionsModule } from "../jurisdictions/jurisdictions.module" import { Application } from "../applications/entities/application.entity" +import { UserProfileController } from "./controllers/user-profile.controller" @Module({ imports: [ @@ -38,6 +39,6 @@ import { Application } from "../applications/entities/application.entity" ], providers: [LocalStrategy, JwtStrategy, AuthService, AuthzService, UserService, PasswordService], exports: [AuthzService, AuthService, UserService], - controllers: [AuthController, UserController], + controllers: [AuthController, UserController, UserProfileController], }) export class AuthModule {} diff --git a/backend/core/src/auth/authz_policy.csv b/backend/core/src/auth/authz_policy.csv index 02fde6175b..e4b03d1c56 100644 --- a/backend/core/src/auth/authz_policy.csv +++ b/backend/core/src/auth/authz_policy.csv @@ -3,7 +3,9 @@ p, user, application, !r.obj || (r.sub == r.obj.user_id), (read|submit) p, anonymous, application, true, submit p, admin, user, true, .* -p, user, user, !r.obj || (r.sub == r.obj.id), (read|update) +p, admin, userProfile, true, .* +p, user, user, !r.obj || (r.sub == r.obj.id), read +p, user, userProfile, !r.obj || (r.sub == r.obj.id), (read|update) p, anonymous, user, true, create p, admin, asset, true, .* diff --git a/backend/core/src/auth/controllers/user-profile.controller.ts b/backend/core/src/auth/controllers/user-profile.controller.ts new file mode 100644 index 0000000000..7d4a52248d --- /dev/null +++ b/backend/core/src/auth/controllers/user-profile.controller.ts @@ -0,0 +1,31 @@ +import { Body, Controller, Put, Request, UseGuards, UsePipes, ValidationPipe } from "@nestjs/common" +import { ApiBearerAuth, ApiOperation, ApiTags } from "@nestjs/swagger" +import { Request as ExpressRequest } from "express" +import { ResourceType } from "../decorators/resource-type.decorator" +import { defaultValidationPipeOptions } from "../../shared/default-validation-pipe-options" +import { UserService } from "../services/user.service" +import { AuthzGuard } from "../guards/authz.guard" +import { UserDto } from "../dto/user.dto" +import { mapTo } from "../../shared/mapTo" +import { AuthContext } from "../types/auth-context" +import { User } from "../entities/user.entity" +import { DefaultAuthGuard } from "../guards/default.guard" +import { UserProfileUpdateDto } from "../dto/user-profile.dto" + +@Controller("userProfile") +@ApiBearerAuth() +@ApiTags("userProfile") +@ResourceType("userProfile") +@UsePipes(new ValidationPipe(defaultValidationPipeOptions)) +export class UserProfileController { + constructor(private readonly userService: UserService) {} + @Put(":id") + @UseGuards(DefaultAuthGuard, AuthzGuard) + @ApiOperation({ summary: "Update profile user", operationId: "update" }) + async update( + @Request() req: ExpressRequest, + @Body() dto: UserProfileUpdateDto + ): Promise { + return mapTo(UserDto, await this.userService.update(dto, new AuthContext(req.user as User))) + } +} diff --git a/backend/core/src/auth/controllers/user.controller.ts b/backend/core/src/auth/controllers/user.controller.ts index 5ed3ad9b56..17f2fab92d 100644 --- a/backend/core/src/auth/controllers/user.controller.ts +++ b/backend/core/src/auth/controllers/user.controller.ts @@ -38,6 +38,7 @@ import { LoginResponseDto } from "../dto/login-response.dto" import { authzActions } from "../enum/authz-actions.enum" import { UserCreateQueryParams } from "../dto/user-create-query-params" import { UserFilterParams } from "../dto/user-filter-params" +import { DefaultAuthGuard } from "../guards/default.guard" @Controller("user") @ApiBearerAuth() @@ -48,7 +49,7 @@ export class UserController { constructor(private readonly userService: UserService) {} @Get() - @UseGuards(OptionalAuthGuard, AuthzGuard) + @UseGuards(DefaultAuthGuard, AuthzGuard) profile(@Request() req): UserDto { return mapTo(UserDto, req.user) } @@ -101,7 +102,7 @@ export class UserController { } @Put(":id") - @UseGuards(OptionalAuthGuard, AuthzGuard) + @UseGuards(DefaultAuthGuard, AuthzGuard) @ApiOperation({ summary: "Update user", operationId: "update" }) async update(@Request() req: ExpressRequest, @Body() dto: UserUpdateDto): Promise { return mapTo(UserDto, await this.userService.update(dto, new AuthContext(req.user as User))) diff --git a/backend/core/src/auth/dto/user-profile.dto.ts b/backend/core/src/auth/dto/user-profile.dto.ts new file mode 100644 index 0000000000..6a537ea082 --- /dev/null +++ b/backend/core/src/auth/dto/user-profile.dto.ts @@ -0,0 +1,46 @@ +import { PickType } from "@nestjs/swagger" +import { User } from "../entities/user.entity" +import { Expose, Type } from "class-transformer" +import { + IsDefined, + IsNotEmpty, + IsOptional, + IsString, + Matches, + ValidateIf, + ValidateNested, +} from "class-validator" +import { ValidationsGroupsEnum } from "../../shared/types/validations-groups-enum" +import { passwordRegex } from "../../shared/password-regex" +import { IdDto } from "../../shared/dto/id.dto" + +export class UserProfileUpdateDto extends PickType(User, [ + "id", + "firstName", + "middleName", + "lastName", + "dob", + "createdAt", + "updatedAt", + "language", +] as const) { + @Expose() + @IsOptional({ groups: [ValidationsGroupsEnum.default] }) + @IsString({ groups: [ValidationsGroupsEnum.default] }) + @Matches(passwordRegex, { + message: "passwordTooWeak", + groups: [ValidationsGroupsEnum.default], + }) + password?: string + + @Expose() + @ValidateIf((o) => o.password, { groups: [ValidationsGroupsEnum.default] }) + @IsNotEmpty({ groups: [ValidationsGroupsEnum.default] }) + currentPassword?: string + + @Expose() + @IsDefined({ groups: [ValidationsGroupsEnum.default] }) + @ValidateNested({ groups: [ValidationsGroupsEnum.default], each: true }) + @Type(() => IdDto) + jurisdictions: IdDto[] +} diff --git a/backend/core/src/auth/dto/user-update.dto.ts b/backend/core/src/auth/dto/user-update.dto.ts index 24a3097051..d30e32c69b 100644 --- a/backend/core/src/auth/dto/user-update.dto.ts +++ b/backend/core/src/auth/dto/user-update.dto.ts @@ -3,6 +3,7 @@ import { Expose, Type } from "class-transformer" import { IsDate, IsDefined, + IsEmail, IsNotEmpty, IsOptional, IsString, @@ -18,6 +19,7 @@ import { UserDto } from "./user.dto" export class UserUpdateDto extends OmitType(UserDto, [ "id", + "email", "createdAt", "updatedAt", "leasingAgentInListings", @@ -29,6 +31,11 @@ export class UserUpdateDto extends OmitType(UserDto, [ @IsUUID(4, { groups: [ValidationsGroupsEnum.default] }) id?: string + @Expose() + @IsOptional({ groups: [ValidationsGroupsEnum.default] }) + @IsEmail({}, { groups: [ValidationsGroupsEnum.default] }) + email?: string + @Expose() @IsOptional({ groups: [ValidationsGroupsEnum.default] }) @IsDate({ groups: [ValidationsGroupsEnum.default] }) diff --git a/backend/core/src/auth/services/user.service.ts b/backend/core/src/auth/services/user.service.ts index e838c321dc..3b6b898001 100644 --- a/backend/core/src/auth/services/user.service.ts +++ b/backend/core/src/auth/services/user.service.ts @@ -94,7 +94,7 @@ export class UserService { return result } - async update(dto: Partial, authContext: AuthContext) { + async update(dto: UserUpdateDto, authContext: AuthContext) { const user = await this.find({ id: dto.id, }) @@ -102,16 +102,6 @@ export class UserService { throw new NotFoundException() } - await this.authzService.canOrThrow(authContext.user, "user", authzActions.update, { - ...dto, - }) - - if (user.confirmedAt?.getTime() !== dto.confirmedAt?.getTime()) { - await this.authzService.canOrThrow(authContext.user, "user", authzActions.confirm, { - ...dto, - }) - } - let passwordHash if (dto.password) { if (!dto.currentPassword) { diff --git a/backend/core/test/authz/authz.e2e-spec.ts b/backend/core/test/authz/authz.e2e-spec.ts index 6ad1fd824e..f3ec6f1cfc 100644 --- a/backend/core/test/authz/authz.e2e-spec.ts +++ b/backend/core/test/authz/authz.e2e-spec.ts @@ -106,7 +106,7 @@ describe("Authz", () => { describe("user", () => { it(`should not allow anonymous user to GET to get any user profile`, async () => { - await supertest(app.getHttpServer()).get(userEndpoint).expect(403) + await supertest(app.getHttpServer()).get(userEndpoint).expect(401) }) it(`should allow a logged in user to GET to get any user profile`, async () => { @@ -225,7 +225,7 @@ describe("Authz", () => { expect(profileRes.body.roles).toBe(null) await supertest(app.getHttpServer()) - .put(`/user/${profileRes.body.id}`) + .put(`/userProfile/${profileRes.body.id}`) .send({ ...profileRes.body, roles: { isAdmin: true, isPartner: false } }) .set(...setAuthorization(userAccessToken)) .expect(200) diff --git a/backend/core/test/user/user.e2e-spec.ts b/backend/core/test/user/user.e2e-spec.ts index 900242507d..239af3e2ca 100644 --- a/backend/core/test/user/user.e2e-spec.ts +++ b/backend/core/test/user/user.e2e-spec.ts @@ -19,6 +19,8 @@ import { UserInviteDto } from "../../src/auth/dto/user-invite.dto" import { Listing } from "../../src/listings/entities/listing.entity" import { Repository } from "typeorm" import { Jurisdiction } from "../../src/jurisdictions/entities/jurisdiction.entity" +import { UserProfileUpdateDto } from "../../src/auth/dto/user-profile.dto" +import { Language } from "../../src/shared/types/language-enum" // Cypress brings in Chai types for the global expect, but we want to use jest // expect here so we need to re-declare it. @@ -270,7 +272,7 @@ describe("Applications", () => { await supertest(app.getHttpServer()) .put(`/user/${user2UpdateDto.id}`) .send(user2UpdateDto) - .expect(403) + .expect(401) }) it("should allow user to resend confirmation", async () => { @@ -398,4 +400,59 @@ describe("Applications", () => { const token = await getUserAccessToken(app, newUser.email, password) expect(token).toBeDefined() }) + + it("should allow user to update user profile throguh PUT /userProfile/:id endpoint", async () => { + const userCreateDto: UserCreateDto = { + password: "Abcdef1!", + passwordConfirmation: "Abcdef1!", + email: "userProfile@b.com", + emailConfirmation: "userProfile@b.com", + firstName: "First", + middleName: "Mid", + lastName: "Last", + dob: new Date(), + language: Language.en, + } + const userCreateResponse = await supertest(app.getHttpServer()) + .post(`/user/`) + .set("jurisdictionName", "Alameda") + .send(userCreateDto) + .expect(201) + + const userService = await app.resolve(UserService) + const user = await userService.findByEmail(userCreateDto.email) + + await supertest(app.getHttpServer()) + .put(`/user/confirm/`) + .send({ token: user.confirmationToken }) + .expect(200) + + const userAccessToken = await getUserAccessToken( + app, + userCreateDto.email, + userCreateDto.password + ) + + const userProfileUpdateDto: UserProfileUpdateDto = { + id: userCreateResponse.body.id, + createdAt: userCreateResponse.body.createdAt, + updatedAt: userCreateResponse.body.updatedAt, + jurisdictions: userCreateResponse.body.jurisdictions, + ...userCreateDto, + currentPassword: userCreateDto.password, + firstName: "NewFirstName", + } + + await supertest(app.getHttpServer()) + .put(`/userProfile/${userCreateResponse.body.id}`) + .send(userProfileUpdateDto) + .expect(401) + + const userProfileUpdateResponse = await supertest(app.getHttpServer()) + .put(`/userProfile/${userCreateResponse.body.id}`) + .send(userProfileUpdateDto) + .set(...setAuthorization(userAccessToken)) + .expect(200) + expect(userProfileUpdateResponse.body.firstName).toBe(userProfileUpdateDto.firstName) + }) }) diff --git a/backend/core/types/src/backend-swagger.ts b/backend/core/types/src/backend-swagger.ts index 7bbd913b5c..795293e549 100644 --- a/backend/core/types/src/backend-swagger.ts +++ b/backend/core/types/src/backend-swagger.ts @@ -900,6 +900,30 @@ export class UserService { } } +export class UserProfileService { + /** + * Update profile user + */ + update( + params: { + /** requestBody */ + body?: UserProfileUpdate + } = {} as any, + options: IRequestOptions = {} + ): Promise { + return new Promise((resolve, reject) => { + let url = basePath + "/userProfile/{id}" + + const configs: IRequestConfig = getConfigs("put", "application/json", url, options) + + let data = params.body + + configs.data = data + axios(configs, resolve, reject) + }) + } +} + export class JurisdictionsService { /** * List jurisdictions @@ -998,6 +1022,28 @@ export class JurisdictionsService { let data = null + configs.data = data + axios(configs, resolve, reject) + }) + } + /** + * Get jurisdiction by name + */ + retrieveByName( + params: { + /** */ + jurisdictionName: string + } = {} as any, + options: IRequestOptions = {} + ): Promise { + return new Promise((resolve, reject) => { + let url = basePath + "/jurisdictions/byName/{jurisdictionName}" + url = url.replace("{jurisdictionName}", params["jurisdictionName"] + "") + + const configs: IRequestConfig = getConfigs("get", "application/json", url, options) + + let data = null + configs.data = data axios(configs, resolve, reject) }) @@ -3741,6 +3787,9 @@ export interface UserUpdate { /** */ id?: string + /** */ + email?: string + /** */ createdAt?: Date @@ -3759,9 +3808,6 @@ export interface UserUpdate { /** */ confirmedAt?: Date - /** */ - email: string - /** */ firstName: string @@ -3831,6 +3877,41 @@ export interface UserInvite { dob: Date } +export interface UserProfileUpdate { + /** */ + language?: Language + + /** */ + password?: string + + /** */ + currentPassword?: string + + /** */ + jurisdictions: Id[] + + /** */ + id: string + + /** */ + firstName: string + + /** */ + middleName?: string + + /** */ + lastName: string + + /** */ + dob: Date + + /** */ + createdAt: Date + + /** */ + updatedAt: Date +} + export interface JurisdictionCreate { /** */ name: string From f49561d0a4df5ac0d8c8cb848a0c23e7aecc02c5 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Wed, 29 Sep 2021 14:56:17 +0200 Subject: [PATCH 2/3] Fix userProfile update security hole allowing user's to edit each others profiles --- backend/core/src/auth/guards/authz.guard.ts | 9 ++- backend/core/test/user/user.e2e-spec.ts | 72 +++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/backend/core/src/auth/guards/authz.guard.ts b/backend/core/src/auth/guards/authz.guard.ts index 2cae61b0a7..1751b4916c 100644 --- a/backend/core/src/auth/guards/authz.guard.ts +++ b/backend/core/src/auth/guards/authz.guard.ts @@ -25,6 +25,13 @@ export class AuthzGuard implements CanActivate { this.reflector.get("authz_action", context.getHandler()) || httpMethodsToAction[req.method] - return this.authzService.can(authUser, type, action) + let resource + if (req.params.id) { + // NOTE: implicit assumption that if request.params contains an ID it also means that body contains one too and it should be the same + // This prevents a security hole where user specifies params.id different than dto.id to pass authorization but actually edit a different resource + resource = { id: req.body.id } + } + + return this.authzService.can(authUser, type, action, resource) } } diff --git a/backend/core/test/user/user.e2e-spec.ts b/backend/core/test/user/user.e2e-spec.ts index 239af3e2ca..46ec55b10a 100644 --- a/backend/core/test/user/user.e2e-spec.ts +++ b/backend/core/test/user/user.e2e-spec.ts @@ -413,6 +413,7 @@ describe("Applications", () => { dob: new Date(), language: Language.en, } + const userCreateResponse = await supertest(app.getHttpServer()) .post(`/user/`) .set("jurisdictionName", "Alameda") @@ -455,4 +456,75 @@ describe("Applications", () => { .expect(200) expect(userProfileUpdateResponse.body.firstName).toBe(userProfileUpdateDto.firstName) }) + + it("should not allow user A to edit user B profile (with /userProfile)", async () => { + const createAndConfirmUser = async (createDto: UserCreateDto) => { + const userCreateResponse = await supertest(app.getHttpServer()) + .post(`/user/`) + .set("jurisdictionName", "Alameda") + .send(createDto) + .expect(201) + + const userService = await app.resolve(UserService) + const user = await userService.findByEmail(createDto.email) + + await supertest(app.getHttpServer()) + .put(`/user/confirm/`) + .send({ token: user.confirmationToken }) + .expect(200) + + const accessToken = await getUserAccessToken(app, createDto.email, createDto.password) + return { accessToken, userId: userCreateResponse.body.id } + } + + const userACreateDto: UserCreateDto = { + password: "Abcdef1!", + passwordConfirmation: "Abcdef1!", + email: "user-a@example.com", + emailConfirmation: "user-a@example.com", + firstName: "First", + middleName: "Mid", + lastName: "Last", + dob: new Date(), + language: Language.en, + } + + const userBCreateDto: UserCreateDto = { + password: "Abcdef1!", + passwordConfirmation: "Abcdef1!", + email: "user-b@example.com", + emailConfirmation: "user-b@example.com", + firstName: "First", + middleName: "Mid", + lastName: "Last", + dob: new Date(), + language: Language.en, + } + + const { userId: userAId } = await createAndConfirmUser(userACreateDto) + const { accessToken: userBAccessToken } = await createAndConfirmUser(userBCreateDto) + + const userAProfileUpdateDto: UserProfileUpdateDto = { + id: userAId, + createdAt: new Date(), + updatedAt: new Date(), + ...userACreateDto, + password: undefined, + jurisdictions: [], + } + + // Restrict user B editing user A's profile + await supertest(app.getHttpServer()) + .put(`/userProfile/${userAId}`) + .send(userAProfileUpdateDto) + .set(...setAuthorization(userBAccessToken)) + .expect(403) + + // Allow admin to edit userA + await supertest(app.getHttpServer()) + .put(`/userProfile/${userAId}`) + .send(userAProfileUpdateDto) + .set(...setAuthorization(adminAccessToken)) + .expect(200) + }) }) From f7d72e6671cf5e4454787bc409fd9f8045bfd50c Mon Sep 17 00:00:00 2001 From: Lint Action Date: Wed, 29 Sep 2021 13:00:44 +0000 Subject: [PATCH 3/3] Fix code style issues with Prettier --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c9f6ecfc1..59c6972dca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,8 +13,8 @@ All notable changes to this project will be documented in this file. The format - Add POST /users/invite endpoint and extend PUT /users/confirm with optional password change ([#1801](https://github.com/bloom-housing/bloom/pull/1801)) - Add `isPartner` filter to GET /user/list endpoint ([#1830](https://github.com/bloom-housing/bloom/pull/1830)) - Add logic for connecting newly created user account to existing applications (matching based on applicant.emailAddress) ([#1807](https://github.com/bloom-housing/bloom/pull/1807)) - - Added new userProfile resource and endpoint `PUT /userProfile/:id` suited specifically for users updating their own profiles ([#1862](https://github.com/bloom-housing/bloom/pull/1862)) - + - Added new userProfile resource and endpoint `PUT /userProfile/:id` suited specifically for users updating their own profiles ([#1862](https://github.com/bloom-housing/bloom/pull/1862)) + - Changed: - Endpoint `PUT /user/:id` is admin only now, because it allows edits over entire `user` table ([#1862](https://github.com/bloom-housing/bloom/pull/1862))