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

136/sanitize returned api data #373

Merged
merged 2 commits into from
Sep 16, 2023
Merged

136/sanitize returned api data #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5ae635
add class-transform serialization interceptor
esteban-gs Sep 12, 2023
50b1240
sanitize return user-dto vals
esteban-gs Sep 12, 2023
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
5 changes: 4 additions & 1 deletion server/src/acccount-manager/account-manager.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ import { FileInterceptor } from '@nestjs/platform-express';
import { FileSizes } from '../file-storage/domain';
import { FilesStorageService } from '../file-storage/file-storage.service';
import { VerifyEmailDto, ReturnSessionDto, ReturnUserDto } from './dto/auth.dto';
import { UpdateUserInternal } from './dto/create-user.internal';
import { MapTo } from '../shared/serialize.interceptor';

type AuthedRequest = RequestT & { user: User };

Expand All @@ -56,13 +58,14 @@ export class AccountManagerController {
async verifyEmail(@Body() body: VerifyEmailDto): Promise<boolean> {
try {
const user = await this.jwtService.verify(body.token, { secret: process.env.JWT_SECRET });
this.usersService.update(user.id, { ...user, email_verified: true });
this.usersService.update(user.id, { email_verified: true } as UpdateUserInternal);
return true;
} catch {
throw Error('jwt verify fail');
}
}

@MapTo(ReturnUserDto)
@Post('register')
@ApiOperation({ summary: 'Create a new user account' })
async register(@Body() createUserDto: CreateUserDto): Promise<ReturnUserDto> {
Expand Down
23 changes: 19 additions & 4 deletions server/src/acccount-manager/dto/auth.dto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { IsEmail } from 'class-validator';
import { IsEmail, IsNotEmpty, IsString } from 'class-validator';
import { Expose } from 'class-transformer';

export class VerifyEmailDto {
@IsString()
@IsNotEmpty()
token: string;
}

Expand All @@ -16,18 +19,30 @@ export class ResetPasswordDto {
}

export class ReturnUserDto {
@Expose()
id: number;
@Expose()
firstName: string;
last_name: string;
@Expose()
last_name?: string;
@Expose()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's worth to add a migration to update field names to follow a naming convention with camel-case instead of snake-case, or vice-versa? I think my preference is camelCase as it aligns with the front-end, but I'm not sure what is more common with Node/Nest.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. This was done, I believe to make table column names conform to the Postgres standard, but there should be a way to tell Typeorm to use camel case in JS.

email: string;
@Expose()
bio?: string;
city: string;
state: string;
@Expose()
city?: string;
@Expose()
state?: string;
@Expose()
zip_code: string;
@Expose()
email_notification_opt_out: boolean;
@Expose()
email_verified: boolean;
@Expose()
profile_image_url?: string;
}

export class ReturnSessionDto {
user: ReturnUserDto;
}
4 changes: 1 addition & 3 deletions server/src/acccount-manager/user.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { HttpException, HttpStatus, Injectable, Logger } from '@nestjs/common';
import { InjectRepository } from '@nestjs/typeorm';
import * as bcrypt from 'bcryptjs';
import { Repository } from 'typeorm/repository/Repository';
import { ReturnUserDto } from './dto/auth.dto';
import { CreateUserInternal, UpdateUserInternal } from './dto/create-user.internal';
import { User } from './entities/user.entity';

Expand All @@ -12,12 +11,11 @@ const { BCRYPT_WORK_FACTOR = '10' } = process.env;
export class UsersService {
constructor(@InjectRepository(User) private usersRepository: Repository<User>) {}

async create(createUserDto: CreateUserInternal): Promise<ReturnUserDto> {
async create(createUserDto: CreateUserInternal): Promise<User> {
try {
const hashedPw = await bcrypt.hash(createUserDto.password, parseInt(BCRYPT_WORK_FACTOR));
createUserDto.password = hashedPw;
const user = await this.usersRepository.save(createUserDto);
delete user.password;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've verified locally that password is not included in the returned payload even with this line removed. I'm not 100% clear on how this is happening, so describing here what I think is going on. In UsersService.create method, we've replaced ReturnUserDto as the output type with User, but in the controller, we use annotation @MapTo(ReturnUserDto), which I'm assuming is doing the sanitizing. Since password is not one of the properties of ReturnUserDto, what does the @Expose() annotation do and why do we use it?

Copy link
Contributor Author

@esteban-gs esteban-gs Sep 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in a way. changing the return type actually has no bearing on the returned data (thanks to Javascript). Thankfully, class-transformer is very good at this. The class-transformer interceptor is getting set to

 return plainToInstance(this.dto, data, {
          excludeExtraneousValues: true,
          strategy: 'excludeAll',
        });

The excludeAll strategy means that any ReturnDto property not decorated with Expose will be removed from the response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with excludeAll so that we have to explicitly list all the data we want to expose, but we can change it if it becomes to cumbersome.

return user;
} catch (err) {
Logger.error(`${err.message}: \n${err.stack}`, UsersService.name);
Expand Down
25 changes: 25 additions & 0 deletions server/src/shared/serialize.interceptor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { CallHandler, ExecutionContext, NestInterceptor, UseInterceptors } from '@nestjs/common';
import { Observable, map } from 'rxjs';
import { plainToInstance } from 'class-transformer';

export const MapTo = (dto: any) => {
return UseInterceptors(new SerializeInterceptor(dto));
};

export class SerializeInterceptor implements NestInterceptor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is an interceptor similar to a middleware? (I should go to the link and read up on class-transformer :D)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In class-transformer documentation, it looks like we may also need reflect-metadata and don't see it as a dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's satisfied by the nestjs/common peer dependencies, which uses it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is an interceptor similar to a middleware?

In NestJs an interceptor is middle-ware. In this specific case, we're chaining a class-transformer operation on the outgoing handler response. It's applied to any handler (controller method) using the UseInterceptors(new SerializeInterceptor(dto)) decorator, but in our case, I just wrapped it in a shorter decorator MapTo.

constructor(private dto: any) {}

intercept(
context: ExecutionContext,
next: CallHandler<any>,
): Observable<any> | Promise<Observable<any>> {
return next.handle().pipe(
map((data: any) => {
return plainToInstance(this.dto, data, {
excludeExtraneousValues: true,
strategy: 'excludeAll',
});
}),
);
}
}
Loading