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

fix(verification): prevents otp request when limit exceeded #2586

Merged
merged 2 commits into from
Aug 16, 2021
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
NonVerifiedFieldTypeError,
OtpExpiredError,
OtpRetryExceededError,
SmsLimitExceededError,
TransactionExpiredError,
TransactionNotFoundError,
WaitForOtpError,
Expand Down Expand Up @@ -613,7 +614,6 @@ describe('Verification controller', () => {
fieldId: MOCK_FIELD_ID,
},
})

const MOCK_FORM = {
admin: {
_id: new ObjectId(),
Expand All @@ -640,6 +640,7 @@ describe('Verification controller', () => {
MockVerificationService.sendNewOtp.mockReturnValue(
okAsync(mockTransaction),
)
MockVerificationService.shouldGenerateOtp.mockReturnValue(okAsync(true))
})

it('should return 201 when params are valid', async () => {
Expand Down Expand Up @@ -835,6 +836,28 @@ describe('Verification controller', () => {
expect(mockRes.json).toHaveBeenCalledWith(expectedResponse)
})

it('should return 400 when sms limit has been exceeded and the form is not onboarded', async () => {
// Arrange
MockVerificationService.shouldGenerateOtp.mockReturnValueOnce(
errAsync(new SmsLimitExceededError()),
)
const expected = {
message:
'Sorry, this form is outdated. Please refresh your browser to get the latest version of the form',
}

// Act
await VerificationController._handleGenerateOtp(
MOCK_REQ,
mockRes,
jest.fn(),
)

// Assert
expect(mockRes.status).toHaveBeenCalledWith(400)
expect(mockRes.json).toBeCalledWith(expected)
})

it('should return 400 when field type is not verifiable', async () => {
// Arrange
MockVerificationService.sendNewOtp.mockReturnValueOnce(
Expand Down
109 changes: 109 additions & 0 deletions src/app/modules/verification/__tests__/verification.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import { mocked } from 'ts-jest/utils'
import { smsConfig } from 'src/app/config/features/sms.config'
import formsgSdk from 'src/app/config/formsg-sdk'
import * as FormService from 'src/app/modules/form/form.service'
import {
OtpRequestError,
SmsLimitExceededError,
} from 'src/app/modules/verification/verification.errors'
import {
MailGenerationError,
MailSendError,
Expand All @@ -25,6 +29,7 @@ import {
UpdateFieldData,
} from 'src/types'

import { generateDefaultField } from 'tests/unit/backend/helpers/generate-form-data'
import dbHandler from 'tests/unit/backend/helpers/jest-db'

import { SMS_WARNING_TIERS } from '../../../../../shared/utils/verification'
Expand Down Expand Up @@ -839,4 +844,108 @@ describe('Verification service', () => {
expect(retrievalSpy).toBeCalledWith(String(MOCK_FORM.admin._id))
})
})

describe('shouldGenerateOtp', () => {
it('should return true when sms counts is less than limit', async () => {
// Arrange
const MOCK_FORM = {
admin: {
_id: 'something',
},
form_fields: [
generateDefaultField(BasicField.Mobile, { isVerifiable: true }),
],
}
const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts')
retrieveSpy.mockReturnValueOnce(
okAsync(smsConfig.smsVerificationLimit - 1),
)

// Act
const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM)

// Assert
expect(actual._unsafeUnwrap()).toBe(true)
expect(retrieveSpy).toBeCalledWith(MOCK_FORM.admin._id)
})

it('should return true when the form is onboarded', async () => {
// Arrange
const MOCK_FORM = {
msgSrvcName: 'somename',
admin: {
_id: 'something',
},
}
const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts')

// Act
const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM)

// Assert
expect(actual._unsafeUnwrap()).toBe(true)
expect(retrieveSpy).not.toBeCalled()
})

it('should return SmsLimitExceeded error when sms counts exceeds limit and form is not onboarded', async () => {
// Arrange
const MOCK_FORM = {
admin: {
_id: 'something',
},
form_fields: [
generateDefaultField(BasicField.Mobile, { isVerifiable: true }),
],
}
const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts')
retrieveSpy.mockReturnValueOnce(
okAsync(smsConfig.smsVerificationLimit + 1),
)

// Act
const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM)

// Assert
expect(actual._unsafeUnwrapErr()).toBeInstanceOf(SmsLimitExceededError)
expect(retrieveSpy).toBeCalledWith(MOCK_FORM.admin._id)
})

it('should return OtpRequestError when an OTP is requested on a form without OTP enabled', async () => {
// Arrange
const MOCK_FORM = {
admin: {
_id: 'something',
},
}
const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts')

// Act
const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM)

// Assert
expect(actual._unsafeUnwrapErr()).toBeInstanceOf(OtpRequestError)
expect(retrieveSpy).not.toBeCalled()
})

it('should propagate any errors during sms counts retrieval', async () => {
// Arrange
const MOCK_FORM = {
admin: {
_id: 'something',
},
form_fields: [
generateDefaultField(BasicField.Mobile, { isVerifiable: true }),
],
}
const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts')
retrieveSpy.mockReturnValueOnce(errAsync(new DatabaseError()))

// Act
const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM)

// Assert
expect(actual._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError)
expect(retrieveSpy).toBeCalledWith(MOCK_FORM.admin._id)
})
})
})
6 changes: 4 additions & 2 deletions src/app/modules/verification/verification.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,12 @@ export const _handleGenerateOtp: ControllerHandler<
// Step 1: Ensure that the form for the specified transaction exists
return (
FormService.retrieveFormById(formId)
// Step 2: Generate hash and otp
// Step 2: Check if we should allow public user to request for OTP
.andThen((form) => VerificationService.shouldGenerateOtp(form))
// Step 3: Generate hash and otp
.andThen(() => generateOtpWithHash(logMeta, SALT_ROUNDS))
.andThen(({ otp, hashedOtp }) =>
// Step 3: Send otp
// Step 4: Send otp
VerificationService.sendNewOtp({
fieldId,
hashedOtp,
Expand Down
11 changes: 11 additions & 0 deletions src/app/modules/verification/verification.errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,14 @@ export class SmsLimitExceededError extends ApplicationError {
super(message)
}
}

/**
* Public user attempts to request for an OTP on a form without OTP enabled.
*/
export class OtpRequestError extends ApplicationError {
constructor(
message = 'Please ensure that the form you are trying to request OTP for has the feature enabled.',
) {
super(message)
}
}
35 changes: 35 additions & 0 deletions src/app/modules/verification/verification.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
} from '../../../../shared/utils/verification'
import {
BasicField,
IFormSchema,
IPopulatedForm,
IVerificationFieldSchema,
IVerificationSchema,
Expand Down Expand Up @@ -39,7 +40,9 @@ import {
MissingHashDataError,
NonVerifiedFieldTypeError,
OtpExpiredError,
OtpRequestError,
OtpRetryExceededError,
SmsLimitExceededError,
TransactionExpiredError,
TransactionNotFoundError,
WaitForOtpError,
Expand Down Expand Up @@ -546,3 +549,35 @@ const checkSmsCountAndPerformAction = (

return okAsync(true)
}

export const shouldGenerateOtp = ({
msgSrvcName,
admin,
form_fields,
}: Pick<IFormSchema, 'msgSrvcName' | 'admin' | 'form_fields'>): ResultAsync<
true,
SmsLimitExceededError | PossibleDatabaseError
> => {
// This check is here to ensure that programmatic pings are rejected.
// If the check is solely on whether the form is onboarded,
// Pings to form that are not onboarded will go through
// Even if the form has no mobile field to verify.
const hasVerifiableMobileFields = form_fields?.filter(
({ fieldType, isVerifiable }) =>
fieldType === BasicField.Mobile && isVerifiable,
)

if (msgSrvcName) {
return okAsync(true)
}

if (!hasVerifiableMobileFields) {
return errAsync(new OtpRequestError())
}

return SmsService.retrieveFreeSmsCounts(admin._id).andThen((counts) =>
hasAdminExceededFreeSmsLimit(counts)
? errAsync(new SmsLimitExceededError())
: okAsync(true),
)
}
7 changes: 7 additions & 0 deletions src/app/modules/verification/verification.util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from '../../../types'
import { smsConfig } from '../../config/features/sms.config'
import { createLoggerWithLabel } from '../../config/logger'
import { SmsLimitExceededError } from '../../modules/verification/verification.errors'
import { MailSendError } from '../../services/mail/mail.errors'
import { InvalidNumberError, SmsSendError } from '../../services/sms/sms.errors'
import { HashingError } from '../../utils/hash'
Expand Down Expand Up @@ -191,6 +192,12 @@ export const mapRouteError: MapRouteError = (
errorMessage: coreErrorMsg,
statusCode: StatusCodes.NOT_FOUND,
}
case SmsLimitExceededError:
return {
errorMessage:
'Sorry, this form is outdated. Please refresh your browser to get the latest version of the form',
statusCode: StatusCodes.BAD_REQUEST,
}
case HashingError:
case DatabaseError:
return {
Expand Down