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

refactor: use combined route validators and handlers #2819

Closed
12 changes: 6 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ npm run dev

After the Docker image has finished building, the application can be accessed at [localhost:5000](localhost:5000).

If there have been no dependency changes in `package.json` or changes in the
If there are no dependency changes in `package.json` or changes in the
`src/app/server.ts` file, you can run

```bash
Expand All @@ -75,7 +75,7 @@ only takes ~15 seconds to finish starting up the image.

### Accessing email locally

We use [MailDev](https://github.com/maildev/maildev) to access emails in the development environment. The MailDev UI can be accessed at [localhost:1080](localhost:1080) when the Docker container is running.
We use [MailDev](https://github.com/maildev/maildev) to access emails in the development environment. The MailDev UI can be accessed at [localhost:1080](localhost:1080) when the Docker container runs.

### Environment variables

Expand All @@ -87,8 +87,8 @@ The following is the order of priority:
- Environment file
- Dockerfile

FormSG requires some environment variables in order to function.
More information about the required environment variables can be seen in
FormSG requires some environment variables to function.
More information about the required environment variables are in
[DEPLOYMENT_SETUP.md](/docs/DEPLOYMENT_SETUP.md).

We provide a [`.template-env`](./.template-env) file with the secrets blanked out. You can copy and
Expand Down Expand Up @@ -149,15 +149,15 @@ Removed in [#3146](https://github.com/opengovsg/FormSG/pull/3146). Will be reimp

## Architecture

An overview of the architecture can be found [here](docs/ARCHITECTURE.md).
The architecture overview is [here](docs/ARCHITECTURE.md).

## MongoDB Scripts

Scripts for common tasks in MongoDB can be found [here](docs/MONGODB.md).

## Contributing

We welcome all contributions, bug reports, bug fixes, documentation improvements, enhancements, and ideas to code open sourced by the Government Technology Agency of Singapore. Contributors should read [CONTRIBUTING.md](CONTRIBUTING.md) and will also be asked to sign a Contributor License Agreement (CLA) in order to ensure that everybody is free to use their contributions.
We welcome all contributions, bug reports, bug fixes, documentation improvements, enhancements, and ideas to code open sourced by the Government Technology Agency of Singapore. Contributors should read [CONTRIBUTING.md](CONTRIBUTING.md) and will also be asked to sign a Contributor License Agreement (CLA) to ensure that everybody is free to use their contributions.

## Support

Expand Down
22 changes: 11 additions & 11 deletions src/app/modules/auth/__tests__/auth.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('auth.controller', () => {
)

// Act
await AuthController.handleCheckUser(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleCheckUser(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.sendStatus).toBeCalledWith(200)
Expand All @@ -57,7 +57,7 @@ describe('auth.controller', () => {
)

// Act
await AuthController.handleCheckUser(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleCheckUser(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(401)
Expand All @@ -82,7 +82,7 @@ describe('auth.controller', () => {
MockMailService.sendLoginOtp.mockReturnValueOnce(okAsync(true))

// Act
await AuthController.handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(200)
Expand All @@ -101,7 +101,7 @@ describe('auth.controller', () => {
)

// Act
await AuthController.handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(401)
Expand All @@ -120,7 +120,7 @@ describe('auth.controller', () => {
)

// Act
await AuthController.handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(500)
Expand All @@ -146,7 +146,7 @@ describe('auth.controller', () => {
)

// Act
await AuthController.handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(500)
Expand Down Expand Up @@ -183,7 +183,7 @@ describe('auth.controller', () => {
MockUserService.retrieveUser.mockReturnValueOnce(okAsync(mockUser))

// Act
await AuthController.handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(200)
Expand All @@ -199,7 +199,7 @@ describe('auth.controller', () => {
)

// Act
await AuthController.handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(401)
Expand All @@ -219,7 +219,7 @@ describe('auth.controller', () => {
)

// Act
await AuthController.handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(422)
Expand All @@ -241,7 +241,7 @@ describe('auth.controller', () => {
)

// Act
await AuthController.handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(500)
Expand All @@ -265,7 +265,7 @@ describe('auth.controller', () => {
)

// Act
await AuthController.handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())
await AuthController._handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(mockRes.status).toBeCalledWith(500)
Expand Down
46 changes: 33 additions & 13 deletions src/app/modules/auth/auth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,18 @@ import { createReqMeta, getRequestIp } from '../../utils/request'
import { ControllerHandler } from '../core/core.types'
import * as UserService from '../user/user.service'

import {
validateCheckUserParams,
validateLoginSendOtpParams,
validateVerifyOtpParams,
} from './auth.middlewares'
import * as AuthService from './auth.service'
import { SessionUser } from './auth.types'
import { mapRouteError } from './auth.utils'

const logger = createLoggerWithLabel(module)

/**
* Handler for GET /auth/checkuser endpoint.
* @returns 500 when there was an error validating body.email
* @returns 401 when domain of body.email is invalid
* @returns 200 if domain of body.email is valid
*/
export const handleCheckUser: ControllerHandler<
export const _handleCheckUser: ControllerHandler<
karrui marked this conversation as resolved.
Show resolved Hide resolved
unknown,
string,
{ email: string }
Expand All @@ -46,12 +45,17 @@ export const handleCheckUser: ControllerHandler<
}

/**
* Handler for POST /auth/sendotp endpoint.
* @return 200 when OTP has been been successfully sent
* @return 401 when email domain is invalid
* @return 500 when unknown errors occurs during generate OTP, or create/send the email that delivers the OTP to the user's email address
* Handler for GET /auth/checkuser endpoint.
* @returns 500 when there was an error validating body.email
* @returns 401 when domain of body.email is invalid
* @returns 200 if domain of body.email is valid
*/
export const handleLoginSendOtp: ControllerHandler<
export const handleCheckUser = [
validateCheckUserParams,
_handleCheckUser,
] as ControllerHandler[]

export const _handleLoginSendOtp: ControllerHandler<
unknown,
{ message: string } | string,
{ email: string }
Expand Down Expand Up @@ -103,14 +107,25 @@ export const handleLoginSendOtp: ControllerHandler<
)
}

/**
* Handler for POST /auth/sendotp endpoint.
* @return 200 when OTP has been been successfully sent
* @return 401 when email domain is invalid
* @return 500 when unknown errors occurs during generate OTP, or create/send the email that delivers the OTP to the user's email address
*/
export const handleLoginSendOtp = [
validateLoginSendOtpParams,
_handleLoginSendOtp,
] as ControllerHandler[]

/**
* Handler for POST /auth/verifyotp endpoint.
* @returns 200 when user has successfully logged in, with session cookie set
* @returns 401 when the email domain is invalid
* @returns 422 when the OTP is invalid
* @returns 500 when error occurred whilst verifying the OTP
*/
export const handleLoginVerifyOtp: ControllerHandler<
export const _handleLoginVerifyOtp: ControllerHandler<
unknown,
string | SessionUser,
{ email: string; otp: string }
Expand Down Expand Up @@ -185,6 +200,11 @@ export const handleLoginVerifyOtp: ControllerHandler<
)
}

export const handleLoginVerifyOtp = [
validateVerifyOtpParams,
_handleLoginVerifyOtp,
] as ControllerHandler[]

export const handleSignout: ControllerHandler = async (req, res) => {
if (!req.session || isEmpty(req.session)) {
logger.error({
Expand Down
35 changes: 35 additions & 0 deletions src/app/modules/auth/auth.middlewares.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { celebrate, Joi, Segments } from 'celebrate'
import { AuthedSessionData } from 'express-session'
import { StatusCodes } from 'http-status-codes'

Expand Down Expand Up @@ -57,3 +58,37 @@ export const logAdminAction: ControllerHandler<{ formId: string }> = async (

return next()
}

export const validateCheckUserParams = celebrate({
[Segments.BODY]: Joi.object().keys({
email: Joi.string()
.required()
.email()
.message('Please enter a valid email')
.lowercase(),
}),
})

export const validateLoginSendOtpParams = celebrate({
[Segments.BODY]: Joi.object().keys({
email: Joi.string()
.required()
.email()
.message('Please enter a valid email')
.lowercase(),
}),
})

export const validateVerifyOtpParams = celebrate({
[Segments.BODY]: Joi.object().keys({
email: Joi.string()
.required()
.email()
.message('Please enter a valid email')
.lowercase(),
otp: Joi.string()
.required()
.regex(/^\d{6}$/)
.message('Please enter a valid OTP'),
}),
})
42 changes: 2 additions & 40 deletions src/app/modules/auth/auth.routes.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { celebrate, Joi, Segments } from 'celebrate'
import { Router } from 'express'

import { rateLimitConfig } from '../../config/config'
Expand All @@ -16,19 +15,7 @@ export const AuthRouter = Router()
* @return 200 when email domain is valid
* @return 401 when email domain is invalid
*/
AuthRouter.post(
'/checkuser',
celebrate({
[Segments.BODY]: Joi.object().keys({
email: Joi.string()
.required()
.email()
.message('Please enter a valid email')
.lowercase(),
}),
}),
AuthController.handleCheckUser,
)
AuthRouter.post('/checkuser', AuthController.handleCheckUser)

/**
* Send a one-time password (OTP) to the specified email address
Expand All @@ -45,15 +32,6 @@ AuthRouter.post(
AuthRouter.post(
'/sendotp',
limitRate({ max: rateLimitConfig.sendAuthOtp }),
celebrate({
[Segments.BODY]: Joi.object().keys({
email: Joi.string()
.required()
.email()
.message('Please enter a valid email')
.lowercase(),
}),
}),
AuthController.handleLoginSendOtp,
)

Expand All @@ -70,23 +48,7 @@ AuthRouter.post(
* @returns 422 when the OTP is invalid
* @returns 500 when error occurred whilst verifying the OTP
*/
AuthRouter.post(
'/verifyotp',
celebrate({
[Segments.BODY]: Joi.object().keys({
email: Joi.string()
.required()
.email()
.message('Please enter a valid email')
.lowercase(),
otp: Joi.string()
.required()
.regex(/^\d{6}$/)
.message('Please enter a valid OTP'),
}),
}),
AuthController.handleLoginVerifyOtp,
)
AuthRouter.post('/verifyotp', AuthController.handleLoginVerifyOtp)

/**
* Sign the user out of the session by clearing the relevant session cookie
Expand Down
4 changes: 2 additions & 2 deletions src/app/modules/billing/__tests__/billing.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ describe('billing.controller', () => {
)

// Act
await BillingController.handleGetBillInfo(MOCK_REQ, mockRes, jest.fn())
await BillingController._handleGetBillInfo(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(MockBillingService.getSpLoginStats).toHaveBeenCalledWith(
Expand All @@ -86,7 +86,7 @@ describe('billing.controller', () => {
)

// Act
await BillingController.handleGetBillInfo(MOCK_REQ, mockRes, jest.fn())
await BillingController._handleGetBillInfo(MOCK_REQ, mockRes, jest.fn())

// Assert
expect(MockBillingService.getSpLoginStats).toHaveBeenCalledWith(
Expand Down
Loading