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

feat(api): collapse spcp/myinfo redirect endpoint to new endpoint #1672

Merged
merged 35 commits into from
Apr 26, 2021
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
d6b4590
docs(myinfo/spcp routes): added deprecation notice on redirects for m…
seaerchin Apr 19, 2021
6475c4f
style(public-form/form): separated route from middleware in router
seaerchin Apr 19, 2021
cf769c0
refactor(public-forms/routes): refactored old route for redirect
seaerchin Apr 19, 2021
d0662b3
feat(spcp): adds new errors and types for spcp
seaerchin Apr 19, 2021
7760feb
feat(spcp/util): add new utility method to check if spcp form is valid
seaerchin Apr 19, 2021
e3cfadd
refactor(auth/controller): adds new controller for redirect and utili…
seaerchin Apr 19, 2021
b7a55d9
refactor(public/services): adds new AuthService for redirect URL
seaerchin Apr 19, 2021
badeea6
refactor(directives): replaced MyInfoService's createRedirectUrl with…
seaerchin Apr 19, 2021
5b724fd
test(myinfo): fixes flaky my info tests
seaerchin Apr 19, 2021
9b71196
fix(auth/controller): fixed imports
seaerchin Apr 20, 2021
d4a64ba
refactor(auth/controller): added more finegrained error handling for …
seaerchin Apr 20, 2021
a20f45d
fix(public-forms/auth): shifted auth routes into /forms folder
seaerchin Apr 20, 2021
bc3a3e2
fix(authservice): removed extra /
seaerchin Apr 20, 2021
3341f28
style(submit-form/directive): removed unused variables
seaerchin Apr 20, 2021
a1573e9
docs(public-forms/auth): fixed docs
seaerchin Apr 20, 2021
4ae64c2
fix(submit-form.directive): fixed bug where fields disappeared due to…
seaerchin Apr 20, 2021
7f4fbd2
refactor(spcp-redirect): removed unused file
seaerchin Apr 20, 2021
5a1b17b
style(auth/controller): removed console log
seaerchin Apr 20, 2021
3b18e8d
refactor(auth): changed isPersistentLogin to be a required parameter
seaerchin Apr 21, 2021
b4f85eb
style(spcp/types): fixed casign
seaerchin Apr 21, 2021
28173b2
style(services/authservice): renamed to publicFormAuthService
seaerchin Apr 21, 2021
07c52fc
refactor(public-form): shifted handleFormAuthRedirect from auth to pu…
seaerchin Apr 21, 2021
5ba8c02
refactor(maprouteerror): shifted mapRedirectUrlErrors in auth utils t…
seaerchin Apr 21, 2021
50d7f80
style(spcp/types): absolute imports changed to relative imports
seaerchin Apr 21, 2021
86473f8
Merge branch 'develop' into refactor/collapse-spcp-endpoint
seaerchin Apr 21, 2021
94a1b4b
Merge branch 'develop' into refactor/collapse-spcp-endpoint
seaerchin Apr 21, 2021
62bd1ce
refactor(form/errors): added new error types for auth mismatch and no…
seaerchin Apr 22, 2021
3a1ee96
refactor(myinfo): updated to use new types and typeguards
seaerchin Apr 22, 2021
1740e0b
refactor(spcp): refactored to ues new types and typeguards
seaerchin Apr 22, 2021
521d7f4
refactor(public-form/utils): added utility method and additional mapF…
seaerchin Apr 22, 2021
50900e6
test(public-form/controller/test): fixed tests due to refactors
seaerchin Apr 22, 2021
5b3b163
refactor(submit-form/directive): removed authtype and changed remembe…
seaerchin Apr 22, 2021
ed883e3
chore(myinfoservice): delete unused code
seaerchin Apr 26, 2021
0c4df79
style(auth): updated naming of RedirectDto to be more specific; updat…
seaerchin Apr 26, 2021
035f5c9
docs(form/errors; myinfo; public-form/controllers): made variables mo…
seaerchin Apr 26, 2021
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
2 changes: 1 addition & 1 deletion src/app/modules/auth/auth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export const handleLoginSendOtp: RequestHandler<
}

/**
* Handler for POST /auth/verifyotp endpoint.
* 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
Expand Down
109 changes: 106 additions & 3 deletions src/app/modules/form/public-form/public-form.controller.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
import { celebrate, Joi, Segments } from 'celebrate'
import { RequestHandler } from 'express'
import { Query } from 'express-serve-static-core'
import { StatusCodes } from 'http-status-codes'
import { err } from 'neverthrow'
import querystring from 'querystring'
import { UnreachableCaseError } from 'ts-essentials'

import { AuthType } from '../../../../types'
import { ErrorDto, PrivateFormErrorDto } from '../../../../types/api'
import {
ErrorDto,
PrivateFormErrorDto,
RedirectUrlDto,
} from '../../../../types/api'
import { createLoggerWithLabel } from '../../../config/logger'
import { isMongoError } from '../../../utils/handle-mongo-error'
import { createReqMeta, getRequestIp } from '../../../utils/request'
Expand All @@ -19,9 +25,17 @@ import {
MyInfoMissingAccessTokenError,
} from '../../myinfo/myinfo.errors'
import { MyInfoFactory } from '../../myinfo/myinfo.factory'
import { extractAndAssertMyInfoCookieValidity } from '../../myinfo/myinfo.util'
import { InvalidJwtError, VerifyJwtError } from '../../spcp/spcp.errors'
import {
extractAndAssertMyInfoCookieValidity,
validateMyInfoForm,
} from '../../myinfo/myinfo.util'
import {
AuthTypeMismatchError,
InvalidJwtError,
VerifyJwtError,
} from '../../spcp/spcp.errors'
import { SpcpFactory } from '../../spcp/spcp.factory'
import { validateSpcpForm } from '../../spcp/spcp.util'
import { PrivateFormError } from '../form.errors'
import * as FormService from '../form.service'

Expand Down Expand Up @@ -350,3 +364,92 @@ export const handleGetPublicForm: RequestHandler<
return new UnreachableCaseError(authType)
}
}

/**
* NOTE: This is exported only for testing
* Generates redirect URL to Official SingPass/CorpPass log in page
* @param isPersistentLogin whether the client wants to have their login information stored
* @returns 200 with the redirect url when the user authenticates successfully
* @returns 400 when there is an error on the authType of the form
* @returns 400 when the eServiceId of the form does not exist
* @returns 404 when form with given ID does not exist
* @returns 500 when database error occurs
* @returns 500 when the redirect url could not be created
* @returns 500 when the redirect feature is not enabled
*/
export const _handleFormAuthRedirect: RequestHandler<
{ formId: string },
RedirectUrlDto | ErrorDto,
unknown,
Query & { isPersistentLogin: boolean }
> = (req, res) => {
const { formId } = req.params
const { isPersistentLogin } = req.query
const logMeta = {
action: 'handleFormAuthRedirect',
...createReqMeta(req),
formId,
}
// NOTE: Using retrieveFullForm instead of retrieveForm to ensure authType always exists
return FormService.retrieveFullFormById(formId)
.andThen((form) => {
switch (form.authType) {
case AuthType.MyInfo:
return validateMyInfoForm(form).andThen((form) =>
MyInfoFactory.createRedirectURL({
formEsrvcId: form.esrvcId,
formId,
requestedAttributes: form.getUniqueMyInfoAttrs(),
}),
)
case AuthType.SP:
case AuthType.CP: {
// NOTE: Persistent login is only set (and relevant) when the authType is SP.
// If authType is not SP, assume that it was set erroneously and default it to false
return validateSpcpForm(form).andThen((form) => {
const target = `/${formId},${
// Need to cast to boolean because undefined is allowed as a valid value
// We are not following corppass's official spec for
// the target parameter
form.authType === AuthType.SP ? !!isPersistentLogin : false
}`
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
return SpcpFactory.createRedirectUrl(
form.authType,
target,
form.esrvcId,
)
})
}
// NOTE: Only MyInfo and SPCP should have redirects as the point of a redirect is
// to provide auth for users from a third party
default:
return err<never, AuthTypeMismatchError>(
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
new AuthTypeMismatchError(form.authType),
)
}
})
.map((redirectURL) => {
return res.status(StatusCodes.OK).json({ redirectURL })
})
.mapErr((error) => {
logger.error({
message: 'Error while creating redirect URL',
meta: logMeta,
error,
})
const { statusCode, errorMessage } = mapRouteError(error)
return res.status(statusCode).json({ message: errorMessage })
})
}

/**
* Handler for /forms/:formId/auth/redirect
*/
export const handleFormAuthRedirect = [
celebrate({
[Segments.QUERY]: Joi.object({
isPersistentLogin: Joi.boolean().optional(),
}),
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
}),
_handleFormAuthRedirect,
] as RequestHandler[]
38 changes: 37 additions & 1 deletion src/app/modules/form/public-form/public-form.utils.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
import { getReasonPhrase, StatusCodes } from 'http-status-codes'

import { createLoggerWithLabel } from '../../../config/logger'
import { ApplicationError, DatabaseError } from '../../core/core.errors'
import {
ApplicationError,
DatabaseError,
MissingFeatureError,
} from '../../core/core.errors'
import { ErrorResponseData } from '../../core/core.types'
import {
MyInfoAuthTypeError,
MyInfoNoESrvcIdError,
} from '../../myinfo/myinfo.errors'
import {
AuthTypeMismatchError,
CreateRedirectUrlError,
SpcpAuthTypeError,
SpcpNoESrvcIdError,
} from '../../spcp/spcp.errors'
import * as FormErrors from '../form.errors'

const logger = createLoggerWithLabel(module)
Expand Down Expand Up @@ -34,10 +48,32 @@ export const mapRouteError = (
errorMessage: error.message,
}
case DatabaseError:
case MissingFeatureError:
case CreateRedirectUrlError:
return {
statusCode: StatusCodes.INTERNAL_SERVER_ERROR,
errorMessage: coreErrorMessage ?? error.message,
}
case MyInfoAuthTypeError:
case MyInfoNoESrvcIdError:
return {
statusCode: StatusCodes.BAD_REQUEST,
errorMessage:
'This form does not have MyInfo enabled. Please refresh and try again.',
}
case SpcpNoESrvcIdError:
case SpcpAuthTypeError:
return {
statusCode: StatusCodes.BAD_REQUEST,
errorMessage:
'This form does not have Singpass or Corppass enabled. Please refresh and try again.',
}
case AuthTypeMismatchError:
return {
statusCode: StatusCodes.BAD_REQUEST,
errorMessage:
'Please ensure that the form has authentication enabled. Please refresh and try again.',
}
default:
logger.error({
message: 'Unknown route error observed in PublicFormController',
Expand Down
29 changes: 14 additions & 15 deletions src/app/modules/myinfo/__tests__/myinfo.routes.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { MyInfoGovClient } from '@opengovsg/myinfo-gov-client'
import MyInfoClient, { IMyInfoConfig } from '@opengovsg/myinfo-gov-client'
import SPCPAuthClient from '@opengovsg/spcp-auth-client'
import axios from 'axios'
import { ObjectId } from 'bson'
Expand Down Expand Up @@ -30,23 +30,20 @@ jest.mock('@opengovsg/spcp-auth-client')
const MockAuthClient = mocked(SPCPAuthClient, true)

jest.mock('@opengovsg/myinfo-gov-client', () => ({
MyInfoGovClient: jest.fn(),
MyInfoGovClient: jest.fn().mockReturnValue({
createRedirectURL: jest.fn(),
getAccessToken: jest.fn(),
}),
MyInfoMode: jest.requireActual('@opengovsg/myinfo-gov-client').MyInfoMode,
MyInfoSource: jest.requireActual('@opengovsg/myinfo-gov-client').MyInfoSource,
MyInfoAddressType: jest.requireActual('@opengovsg/myinfo-gov-client')
.MyInfoAddressType,
MyInfoAttribute: jest.requireActual('@opengovsg/myinfo-gov-client')
.MyInfoAttribute,
}))
const MockMyInfoGovClient = mocked(MyInfoGovClient, true)
const mockCreateRedirectURL = jest.fn()
const mockGetAccessToken = jest.fn()
MockMyInfoGovClient.mockImplementation(
() =>
(({
createRedirectURL: mockCreateRedirectURL,
getAccessToken: mockGetAccessToken,
} as unknown) as MyInfoGovClient),
const MockMyInfoGovClient = mocked(
new MyInfoClient.MyInfoGovClient({} as IMyInfoConfig),
true,
)

// Import last so that mocks are imported correctly
Expand All @@ -68,7 +65,7 @@ describe('myinfo.routes', () => {
let request: Session

beforeAll(() => {
mockCreateRedirectURL.mockReturnValue(MOCK_REDIRECT_URL)
MockMyInfoGovClient.createRedirectURL.mockReturnValue(MOCK_REDIRECT_URL)
})

beforeEach(async () => {
Expand Down Expand Up @@ -218,7 +215,9 @@ describe('myinfo.routes', () => {

beforeEach(async () => {
request = session(myInfoApp)
mockGetAccessToken.mockResolvedValueOnce(MOCK_ACCESS_TOKEN)
MockMyInfoGovClient.getAccessToken.mockResolvedValueOnce(
MOCK_ACCESS_TOKEN,
)
await dbHandler.insertEmailForm({
formId: new ObjectId(MOCK_FORM_ID),
formOptions: {
Expand Down Expand Up @@ -346,8 +345,8 @@ describe('myinfo.routes', () => {

it('should redirect to form with error cookie when access token cannot be retrieved', async () => {
// Clear default mock implementation from beforeEach
mockGetAccessToken.mockReset()
mockGetAccessToken.mockRejectedValueOnce('rejected')
MockMyInfoGovClient.getAccessToken.mockReset()
MockMyInfoGovClient.getAccessToken.mockRejectedValueOnce('rejected')

const response = await request
.get(ROUTE)
Expand Down
1 change: 1 addition & 0 deletions src/app/modules/myinfo/myinfo.routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const MyInfoRouter = Router()
/**
* Serves requests to supply a redirect URL to log in to
* MyInfo.
* @deprecated in favour of GET /api/v3/forms/:formId/auth/redirect
*/
MyInfoRouter.get('/redirect', handleRedirectURLRequest)

Expand Down
21 changes: 21 additions & 0 deletions src/app/modules/spcp/spcp.errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,24 @@ export class MissingJwtError extends ApplicationError {
super(message)
}
}

/**
* SPCP form missing e-service ID.
*/
export class SpcpNoESrvcIdError extends ApplicationError {
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
constructor(message = 'Form does not have e-service ID') {
super(message)
}
}

/**
* Attempt to perform a Spcp-related operation on a form without Spcp
* authentication enabled.
*/
export class SpcpAuthTypeError extends ApplicationError {
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
constructor(
message = 'SPCP function called on form without SPCP authentication type',
) {
super(message)
}
}
1 change: 1 addition & 0 deletions src/app/modules/spcp/spcp.routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const SpcpRouter = Router()
* Provide a URL that web agents are obliged to redirect users to.
* Due to cross-origin restrictions in place by SingPass/CorpPass,
* this endpoint cannot and should not issue a 302 redirect
* @deprecated in favour of GET /api/v3/forms/:formId/auth/redirect
* @route GET /spcp/redirect
* @group SPCP - SingPass/CorpPass logins for form-fillers
* @param target.query.required - the destination URL after login
Expand Down
7 changes: 7 additions & 0 deletions src/app/modules/spcp/spcp.types.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { AuthType, IFormSchema } from '../../../types'

export enum JwtName {
SP = 'jwtSp',
CP = 'jwtCp',
Expand Down Expand Up @@ -43,3 +45,8 @@ export interface ParsedSpcpParams {
rememberMe: boolean
cookieDuration: number
}

export interface ISpcpForm extends IFormSchema {
authType: AuthType.SP | AuthType.CP
esrvcId: string
}
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
30 changes: 28 additions & 2 deletions src/app/modules/spcp/spcp.util.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
import SPCPAuthClient from '@opengovsg/spcp-auth-client'
import crypto from 'crypto'
import { StatusCodes } from 'http-status-codes'
import { err, ok, Result } from 'neverthrow'

import { BasicField, MapRouteError, SPCPFieldTitle } from '../../../types'
import {
AuthType,
BasicField,
IFormSchema,
IPopulatedForm,
MapRouteError,
SPCPFieldTitle,
} from '../../../types'
import { createLoggerWithLabel } from '../../config/logger'
import { hasProp } from '../../utils/has-prop'
import { MissingFeatureError } from '../core/core.errors'
Expand All @@ -14,9 +22,11 @@ import {
InvalidJwtError,
LoginPageValidationError,
MissingJwtError,
SpcpAuthTypeError,
SpcpNoESrvcIdError,
VerifyJwtError,
} from './spcp.errors'
import { CorppassJwtPayload, SingpassJwtPayload } from './spcp.types'
import { CorppassJwtPayload, ISpcpForm, SingpassJwtPayload } from './spcp.types'

const logger = createLoggerWithLabel(module)
const DESTINATION_REGEX = /^\/([\w]+)\/?/
Expand Down Expand Up @@ -210,6 +220,22 @@ export const createCorppassParsedResponses = (
]
}

/**
* Validates that a form is a SPCP form with an e-service ID
* @param form Form to validate
*/
export const validateSpcpForm = (
form: IFormSchema | IPopulatedForm,
): Result<ISpcpForm, SpcpNoESrvcIdError | SpcpAuthTypeError> => {
if (!form.esrvcId) {
return err(new SpcpNoESrvcIdError())
}
if (form.authType !== AuthType.SP && form.authType !== AuthType.CP) {
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
return err(new SpcpAuthTypeError())
}
return ok(form as ISpcpForm)
}

/**
* Maps errors to status codes and error messages to return to frontend.
* @param error
Expand Down
Loading