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.

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

[TM-1311] Implement password reset #46

Open
wants to merge 3 commits into
base: staging
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 17 additions & 3 deletions apps/user-service/src/app.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,30 @@ import { CommonModule } from "@terramatch-microservices/common";
import { HealthModule } from "./health/health.module";
import { SentryGlobalFilter, SentryModule } from "@sentry/nestjs/setup";
import { APP_FILTER } from "@nestjs/core";
import { EmailModule } from "./email/email.module";
import { ResetPasswordController } from "./auth/reset-password.controller";
import { ResetPasswordService } from "./auth/reset-password.service";
import { ConfigModule } from "@nestjs/config";

@Module({
imports: [SentryModule.forRoot(), DatabaseModule, CommonModule, HealthModule],
controllers: [LoginController, UsersController],
imports: [
SentryModule.forRoot(),
DatabaseModule,
CommonModule,
HealthModule,
ConfigModule.forRoot({
isGlobal: true,
}),
EmailModule
],
controllers: [LoginController, UsersController, ResetPasswordController],
providers: [
{
provide: APP_FILTER,
useClass: SentryGlobalFilter
},
AuthService
AuthService,
ResetPasswordService,
]
})
export class AppModule {}
14 changes: 14 additions & 0 deletions apps/user-service/src/auth/dto/reset-password-request.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { ApiProperty } from "@nestjs/swagger";
import { JsonApiDto } from "@terramatch-microservices/common/decorators";
import { JsonApiAttributes } from "@terramatch-microservices/common/dto/json-api-attributes";


@JsonApiDto({ type: 'logins', id: 'number' })
export class RequestResetPasswordDto extends JsonApiAttributes<RequestResetPasswordDto> {
@ApiProperty({
description:
'User email',
example: '[email protected]',
})
emailAddress: string;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { JsonApiDto } from "@terramatch-microservices/common/decorators";
import { JsonApiAttributes } from "@terramatch-microservices/common/dto/json-api-attributes";
import { ApiProperty } from "@nestjs/swagger";

@JsonApiDto({ type: 'logins', id: 'number' })
export class ResetPasswordResponseOperationDto extends JsonApiAttributes<ResetPasswordResponseOperationDto> {
@ApiProperty({
description:
'User email',
example: '[email protected]',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't seen how this is used yet, but this description / example doesn't seem right for a property named message.

})
message: string;
}
25 changes: 25 additions & 0 deletions apps/user-service/src/auth/dto/reset-password-response.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { ApiProperty } from "@nestjs/swagger";
import { JsonApiDto } from "@terramatch-microservices/common/decorators";
import { JsonApiAttributes } from "@terramatch-microservices/common/dto/json-api-attributes";

@JsonApiDto({ type: 'logins', id: 'number' })
export class ResetPasswordResponseDto extends JsonApiAttributes<ResetPasswordResponseDto> {
@ApiProperty({
description: 'User id',
example: 'ac905c37-025c-4548-9851-f749ed15b5e1'
})
uuid: string;

@ApiProperty({
description:
'User email',
example: '[email protected]',
})
emailAddress: string;

@ApiProperty({
description: 'User Id',
example: '12345',
})
userId: number;
}
3 changes: 3 additions & 0 deletions apps/user-service/src/auth/dto/reset-password.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export class ResetPasswordDto {
newPassword: string;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should have some validation on it. See login-request.dto.ts for an example.

55 changes: 55 additions & 0 deletions apps/user-service/src/auth/reset-password.controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import {
Controller,
Body,
Post,
HttpStatus,
BadRequestException,
Param,
} from '@nestjs/common';
import { ResetPasswordService } from './reset-password.service';
import { ApiOperation } from '@nestjs/swagger';
import { JsonApiResponse } from '@terramatch-microservices/common/decorators';
import { buildJsonApi, JsonApiDocument } from '@terramatch-microservices/common/util';
import { ApiException } from '@nanogiants/nestjs-swagger-api-exception-decorator';
import { RequestResetPasswordDto } from './dto/reset-password-request.dto';
import { ResetPasswordDto } from './dto/reset-password.dto';
import { NoBearerAuth } from '@terramatch-microservices/common/guards';
import { ResetPasswordResponseOperationDto } from "./dto/reset-password-response-operation.dto";

@Controller('auth/v3/reset-password')
Copy link
Collaborator

Choose a reason for hiding this comment

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

For resource named endpoints in this codebase, they should be camel case and plural. I would probably call this passwordResets.

export class ResetPasswordController {
constructor(private readonly resetPasswordService: ResetPasswordService) {}

@Post('request')
Copy link
Collaborator

Choose a reason for hiding this comment

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

When creating a resource (in the case of this controller, a password reset), it's more typical in REST to post directly to the root resource path. So, this would be simply Post(), and the client will then issue a POST to auth/v3/passwordResets as a create action.

@NoBearerAuth
@ApiOperation({
operationId: 'requestPasswordReset',
description: 'Send password reset email with a token',
})
@JsonApiResponse({ status: HttpStatus.CREATED, data: { type: RequestResetPasswordDto } })
@ApiException(() => BadRequestException, { description: 'Invalid request or email.' })
async requestReset(@Body() { emailAddress }: RequestResetPasswordDto): Promise<JsonApiDocument> {
const response = await this.resetPasswordService.sendResetPasswordEmail(emailAddress);
return buildJsonApi()
.addData(`${response.userId}`, response)
.document.serialize();
}

@Post('reset/:token')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would follow REST conventions better if this were @Put(':token'). Symantically, that would mean that you're updating the given passwordReset resource, which aligns a little better with REST for this kind of weird virtual resource.

@NoBearerAuth
@ApiOperation({
operationId: 'resetPassword',
description: 'Reset password using the provided token',
})
@JsonApiResponse({ status: HttpStatus.OK, data: { type: ResetPasswordResponseOperationDto } })
Copy link
Collaborator

Choose a reason for hiding this comment

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

These endpoints are returning different resources, which is not in keeping with a resource-oriented API. A given resource controller should return the same payload (DTO) for all create and update endpoints so that the FE can expect a consistent shape for the resource.

@ApiException(() => BadRequestException, { description: 'Invalid or expired token.' })
async resetPassword(
@Param('token') token: string,
@Body() { newPassword }: ResetPasswordDto
): Promise<JsonApiDocument> {
const response = await this.resetPasswordService.resetPassword(token, newPassword);
return buildJsonApi()
.addData('sads',response)
Copy link
Collaborator

Choose a reason for hiding this comment

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

'sads' - is this a testing / debug value? In keeping with the comment above, the ID used for each of these endpoints in its response should also match. That's important for the FE caching layer. In this case I would use the user id for all of them to match the login controller.

.document.serialize();
}
}
72 changes: 72 additions & 0 deletions apps/user-service/src/auth/reset-password.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { Injectable, NotFoundException, BadRequestException } from '@nestjs/common';
import bcrypt from 'bcryptjs';
import { JwtService } from '@nestjs/jwt';
import { User, LocalizationKeys } from '@terramatch-microservices/database/entities';
import { ResetPasswordDto } from './dto/reset-password.dto';
import { RequestResetPasswordDto } from './dto/reset-password-request.dto';
import { ResetPasswordResponseDto } from './dto/reset-password-response.dto';
import { EmailService } from '../email/email.service';
import {ConfigService} from "@nestjs/config";
import { ResetPasswordResponseOperationDto } from "./dto/reset-password-response-operation.dto";

@Injectable()
export class ResetPasswordService {
constructor(
private readonly jwtService: JwtService,
private readonly emailService: EmailService,
private readonly configService: ConfigService,
//private readonly userService: UserService // Assuming you have a User service to interact with the database
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't leave in commented out code.

) {}

async sendResetPasswordEmail(emailAddress: string) {
const user = await User.findOne({ where: { emailAddress } });
if (!user) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎨 I haven't set up a linter rule for this yet so you can leave it for now, but I greatly prefer explicit null checks (I'm a little surprised this passed the linter actually, I'll have to look into it)

if (user == null) {

throw new NotFoundException('User not found');
}

const resetToken = await this.jwtService.signAsync(
{ sub: user.uuid }, // user id as the subject
{ expiresIn: '1h', secret: 'reset_password_secret' } // token expires in 1 hour
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this secret accomplishing? Since our JWTs are signed with a secret key, I don't think there's any real value here. If there is some value to including it, it should be from the environment (and be a random string), not included in our publicly accessible codebase.

);

const localizationKeys = await LocalizationKeys.findOne({where: { key: 'reset-password.body'}});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised this is passing the linter too. I clearly have some work to do there. I recommend setting up your editor to use our prettier config on save so that the styling of your code matches the codebase (the thing that stands out here is the lack of spaces inside the curly brace ({where vs { where)


if (!localizationKeys) {
throw new NotFoundException('Localization body not found');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like to have anything that can be checked and might throw an error to happen at the top of the method so additional work isn't done when something fails. In this case that would mean moving the await this.jwtService.signAsync below this check.

}
const url = this.configService.get('TERRAMATCH_WEBSITE_URL');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make sure new ENV variables are included in .env.local.sample.

const resetLink = `${url}/auth/reset-password/${resetToken}`;
const bodyEmail = localizationKeys.value.replace('link', `<a href="${resetLink}">link</a>`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably include target="_blank" in the anchor tag so it always opens in a new browser tab. Probably not really necessary for email clients as the typically enforce that anyway, but it can't hurt.

await this.emailService.sendEmail(
user.emailAddress,
'Reset Password',
Copy link
Collaborator

Choose a reason for hiding this comment

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

The title needs to be translated as well as the body.

bodyEmail,
);

return new ResetPasswordResponseDto({emailAddress: user.emailAddress, uuid: user.uuid, userId: user.id});
}

async resetPassword(resetToken: string, newPassword: string) {
console.log(" token: ", resetToken)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't leave console.log in the PR. If you want to log this in prod, use the TMLogService (but I suspect this was for debugging only?)

let userId;
try {
const payload = await this.jwtService.verifyAsync(resetToken, {
secret: 'reset_password_secret',
});
userId = payload.sub;
} catch (error) {
console.log(error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be kept as a log, but use the log service instead.

throw new BadRequestException('Invalid or provide token is expired');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small typo here - should be provided. I would probably reword a little: Provided token is invalid or expired

}

const user = await User.findOne({ where: { id: userId } });
if (!user) {
throw new NotFoundException('User not found');
}

const hashedPassword = await bcrypt.hash(newPassword, 10);
await User.update({ password: hashedPassword }, { where: { id: userId } });

return new ResetPasswordResponseOperationDto({ message: 'Password successfully reset' });
}
}
10 changes: 10 additions & 0 deletions apps/user-service/src/email/email.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { Module } from '@nestjs/common';
import { ConfigModule } from '@nestjs/config';
import { EmailService } from './email.service';

@Module({
imports: [ConfigModule],
providers: [EmailService],
exports: [EmailService],
})
export class EmailModule {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this module to the common lib. It will be needed across many apps.

31 changes: 31 additions & 0 deletions apps/user-service/src/email/email.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { Injectable } from '@nestjs/common';
import * as nodemailer from 'nodemailer';
import { ConfigService } from '@nestjs/config';

@Injectable()
export class EmailService {
private transporter: nodemailer.Transporter;

constructor(private readonly configService: ConfigService) {
this.transporter = nodemailer.createTransport({
host: this.configService.get<string>('MAIL_HOST'),
port: this.configService.get<number>('MAIL_PORT'),
secure: false, // true for 465, false for other ports
auth: {
user: this.configService.get<string>('MAIL_USERNAME'),
pass: this.configService.get<string>('MAIL_PASSWORD'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure these new config values are documented in .env.local.sample - even if a local development user shouldn't have them defined (there are some examples in there), it's the only central place we currently document what a full environment config for this codebase looks like.

},
});
}

async sendEmail(to: string, subject: string, body: string): Promise<void> {
const mailOptions = {
from: this.configService.get<string>('MAIL_FROM_ADDRESS'),
to,
subject,
html: body,
};

await this.transporter.sendMail(mailOptions);
}
}
4 changes: 3 additions & 1 deletion libs/common/src/lib/dto/json-api-attributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,16 @@ export function pickApiProperties<Source, DTO>(source: Source, dtoClass: Type<DT
return pick(source, fields) as Common<Source | DTO>;
}

export type JsonApiAttributesInput<DTO> = Omit<DTO, "type">;

/**
* A simple class to make it easy to create a typed attributes DTO with new()
*
* See users.controller.ts findOne and user.dto.ts for a complex example.
* See auth.controller.ts login for a simple example.
*/
export class JsonApiAttributes<DTO> {
constructor(source: Omit<DTO, "type">) {
constructor(source: JsonApiAttributesInput<DTO>) {
Object.assign(this, pickApiProperties(source, this.constructor as Type<DTO>));
}
}
1 change: 1 addition & 0 deletions libs/database/src/lib/entities/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,4 @@ export * from "./tree-species.entity";
export * from "./tree-species-research.entity";
export * from "./user.entity";
export * from "./workday.entity";
export * from "./localization-keys.entity";
35 changes: 35 additions & 0 deletions libs/database/src/lib/entities/localization-keys.entity.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import {
AllowNull,
AutoIncrement,
Column,
Model,
PrimaryKey,
Table,
Unique
} from "sequelize-typescript";
import { BIGINT, NUMBER, STRING } from "sequelize";



@Table({ tableName: "localization_keys", underscored: true, paranoid: false })
Copy link
Collaborator

Choose a reason for hiding this comment

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

paranoid: false is the default and may be left out of this config.

export class LocalizationKeys extends Model<LocalizationKeys> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The convention in laravel is to have the database table name be pluralized, but in this codebase all entities are singular: LocalizationKey


@PrimaryKey
@AutoIncrement
@Column(BIGINT.UNSIGNED)
override id: number;

@AllowNull
@Column(STRING)
key: string | null;

@AllowNull
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the current DB schema, key and value are not allowed to be null. Please try to follow the current schema as closely as possible when defining columns in this codebase.

@Column(STRING)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This column appears to be a TEXT type, not STRING in the DB (text vs varchar(255))

value: string | null;

@AllowNull
@Unique
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if these value ids are required to be unique or not, but I do see that the current DB schema doesn't set a uniqueness constraint on this column, so we shouldn't here either.

@Column(NUMBER)
valueId: number;

}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"mariadb": "^3.3.2",
"mysql2": "^3.11.2",
"nestjs-request-context": "^3.0.0",
"nodemailer": "^6.9.16",
"reflect-metadata": "^0.1.14",
"rxjs": "^7.8.0",
"sequelize": "^6.37.3",
Expand Down
Loading