Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
@@ -1,9 +1,8 @@
export namespace AuthenticateUserErrors {
export class AuthenticationFailedError {
public message: string
public constructor(email: string, message: string) {
this.message = `Authentication for user with ${email} failed: ${message}`
}
export class AuthenticationFailedError extends Error {
public constructor(email: string, message: string) {
super()
this.message = `Authentication for user with ${email} failed: ${message}`
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import express from 'express'
import httpMocks from 'node-mocks-http'
import { AppError } from '../../../../../../shared/core/app-error'
import { Result } from '../../../../../../shared/core/result'
import { AuthorizeUserDTO } from '../authorize-user-dto'
import { AuthorizeUserErrors } from '../authorize-user-errors'
import { AuthorizeUserUseCase } from '../authorize-user-use-case'
import { AuthorizeUserController } from '../authorize-user-controller'
import { mocks } from '../../../../../../test-utils'
import { ParamList, ParamPair } from '../../../../../../shared/app/param-list'

jest.mock('../authorize-user-use-case')
jest.mock('../authorize-user-use-case')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seeing double

Suggested change
jest.mock('../authorize-user-use-case')
jest.mock('../authorize-user-use-case')
jest.mock('../authorize-user-use-case')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW I've noticed that jest.mock doesn't actually need to be called if we use jest.spyOn on the object rather than on its class prototype.

i.e. here, you can set up a variable authorizeUserUseCase = authorizeUser.authorizeUserUseCase, then call jest.spyOn(authorizeUserUseCase, 'execute') instead of jest.spyOn(AuthorizeUserUseCase.prototype, 'execute').


describe('AuthorizeUserController', () => {
let authorizeUserDTO: AuthorizeUserDTO
let authorizeUserController: AuthorizeUserController
let mockResponse: express.Response

beforeAll(async () => {
const authorizeUser = await mocks.mockAuthorizeUser()
authorizeUserController = authorizeUser.authorizeUserController
mockResponse = httpMocks.createResponse()
authorizeUserDTO = {
req: httpMocks.createRequest(),
params: {
client_id: 'i291u92jksdn',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

are the ids supposed to be hex strings or just any alphanumeric string?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hex strings, but since it doesn't matter for the controller unit test I just made it that. Adding use-case unit tests soon.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the test should reflect real life then and use a hex string. (I'm a massive hypocrite because waterpark is filled with unrealistic test IDs)

response_type: 'code',
redirect_uri: 'www.loolabs.com',
scope: 'openid',
},
}
})

test('When the AuthorizeUserUseCase returns Ok, the AuthorizeUserController returns 302 Redirect', async () => {
const useCaseResolvedValue = {
redirectParams: new ParamList([new ParamPair('type', 'test')]),
redirectUrl: 'test@loolabs.com',
}
jest
.spyOn(AuthorizeUserUseCase.prototype, 'execute')
.mockResolvedValue(Result.ok(useCaseResolvedValue))

const result = await authorizeUserController.executeImpl(authorizeUserDTO, mockResponse)
expect(result.statusCode).toBe(302)
})

test('When the AuthorizeUserUseCase returns AuthorizeUserErrors.InvalidRequestParameters, AuthorizeUserController returns 400 Bad Request', async () => {
jest
.spyOn(AuthorizeUserUseCase.prototype, 'execute')
.mockResolvedValue(Result.err(new AuthorizeUserErrors.InvalidRequestParameters()))

const result = await authorizeUserController.executeImpl(authorizeUserDTO, mockResponse)

expect(result.statusCode).toBe(400)
})

test('When the AuthorizeUserUseCase returns AuthorizeUserErrors.UserNotAuthenticated, AuthorizeUserController returns 302 Redirect', async () => {
const useCaseErrorValue = {
redirectParams: new ParamList([new ParamPair('type', 'test')]),
redirectUrl: 'test@loolabs.com',
}
jest
.spyOn(AuthorizeUserUseCase.prototype, 'execute')
.mockResolvedValue(Result.err(useCaseErrorValue))

const result = await authorizeUserController.executeImpl(authorizeUserDTO, mockResponse)
expect(result.statusCode).toBe(302)
})

test('When the AuthorizeUserUseCase returns AppError.UnexpectedError, AuthorizeUserController returns 500 Internal Server Error', async () => {
jest
.spyOn(AuthorizeUserUseCase.prototype, 'execute')
.mockResolvedValue(Result.err(new AppError.UnexpectedError('Unexpected error')))

const result = await authorizeUserController.executeImpl(authorizeUserDTO, mockResponse)

expect(result.statusCode).toBe(500)
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import express from 'express'
import { ControllerWithDTO } from '../../../../../shared/app/controller-with-dto'
import { AuthorizeUserUseCase } from './authorize-user-use-case'
import { AuthorizeUserDTO, AuthorizeUserDTOSchema } from './authorize-user-dto'
import { AuthorizeUserErrors } from './authorize-user-errors'
import { Result } from '../../../../../shared/core/result'
import { ValidationError } from 'joi'

export class AuthorizeUserController extends ControllerWithDTO<AuthorizeUserUseCase> {
constructor(useCase: AuthorizeUserUseCase) {
super(useCase)
}

buildDTO(req: express.Request): Result<AuthorizeUserDTO, Array<ValidationError>> {
let params: any = req.params
const errs: Array<ValidationError> = []
const compiledRequest = {
req,
params,
}
const bodyResult = this.validate(compiledRequest, AuthorizeUserDTOSchema)
if (bodyResult.isOk()) {
const body = bodyResult.value
return Result.ok(body)
} else {
errs.push(bodyResult.error)
return Result.err(errs)
}
}

async executeImpl<Res extends express.Response>(dto: AuthorizeUserDTO, res: Res): Promise<Res> {
try {
const result = await this.useCase.execute(dto)

if (result.isOk()) {
return this.redirect(res, result.value.redirectUrl, result.value.redirectParams)
} else {
const error = result.error
if ('redirectParams' in error) {
return this.redirect(res, error.redirectUrl, error.redirectParams)
}
switch (error.constructor) {
case AuthorizeUserErrors.InvalidRequestParameters:
return this.clientError(res, error.message)
default:
return this.fail(res, error.message)
}
}
} catch (err) {
return this.fail(res, err)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import Joi from 'joi'
import express from 'express'

export const SUPPORTED_OPEN_ID_RESPONSE_TYPES = ['code']
export const SUPPORTED_OPEN_ID_SCOPE = ['openid']

export interface AuthorizeUserDTOParams {
client_id: string
scope: string
response_type: string
redirect_uri: string
}

export interface AuthorizeUserDTO {
req: express.Request
params: AuthorizeUserDTOParams
}

export const AuthorizeUserDTOParamsSchema = Joi.object<AuthorizeUserDTOParams>({
client_id: Joi.string().required(),
scope: Joi.string()
.valid(...SUPPORTED_OPEN_ID_SCOPE)
.required(),
response_type: Joi.string()
.valid(...SUPPORTED_OPEN_ID_RESPONSE_TYPES)
.required(),
redirect_uri: Joi.string().uri().required(),
}).options({ abortEarly: false })

export const AuthorizeUserDTOSchema = Joi.object<AuthorizeUserDTO>({
req: Joi.object().required(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I understand correctly, the req object is only needed for its .user field. Can we narrow this part of the schema then?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The issue is that by narrowing the schema at this point, I wouldn't be able to redirect the user to /login if the user doesn't exist, like done in the use-case (we return a 400 for all schema errors currently). The overridable method you showed me earlier would let me change that. Is that something I should include here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since loolabs/waterpark#221 is merged now, feel free to copy over https://github.com/loolabs/waterpark/blob/main/server/src/shared/app/typed-controller.ts and its dependencies to try it out. A word of warning -- it uses Zod. You might be able to modify it to use Joi validation.

params: AuthorizeUserDTOParamsSchema.optional(), // this ensures that all of the necessary request params for client authentication are present, not just an insufficient subset
}).options({ abortEarly: false })
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export namespace AuthorizeUserErrors {
export class InvalidRequestParameters extends Error {
public constructor() {
super(`Invalid openid request parameters supplied.`)
}
}
export class UserNotAuthenticated extends Error {
public constructor(email: string) {
super(`The user with email ${email} is not authenticated.`)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import { UseCaseWithDTO } from '../../../../../shared/app/use-case-with-dto'
import { AppError } from '../../../../../shared/core/app-error'
import { Result } from '../../../../../shared/core/result'
import { AuthorizeUserDTO } from './authorize-user-dto'
import { AuthorizeUserErrors } from './authorize-user-errors'
import { ParamList, ParamPair } from '../../../../../shared/app/param-list'
import { AuthCodeRepo } from '../../../infra/repos/auth-code-repo/auth-code-repo'
import { AuthSecretRepo } from '../../../infra/repos/auth-secret-repo/auth-secret-repo'
import { AuthCode } from '../../../domain/entities/auth-code'
import { AuthCodeString } from '../../../domain/value-objects/auth-code-string'
import { User } from '../../../domain/entities/user'

export type AuthorizeUserUseCaseClientError =
| AuthorizeUserErrors.InvalidRequestParameters
| AppError.UnexpectedError

export type AuthorizeUserUseCaseRedirectError = {
redirectParams: ParamList
redirectUrl: string
}

export type AuthorizeUserUseCaseError =
| AuthorizeUserUseCaseClientError
| AuthorizeUserUseCaseRedirectError

export interface AuthorizeUserSuccess {
redirectParams: ParamList
redirectUrl: string
}

export type AuthorizeUserUseCaseResponse = Result<AuthorizeUserSuccess, AuthorizeUserUseCaseError>

export class AuthorizeUserUseCase
implements UseCaseWithDTO<AuthorizeUserDTO, AuthorizeUserUseCaseResponse>
{
constructor(private authCodeRepo: AuthCodeRepo, private authSecretRepo: AuthSecretRepo) {}

async execute(dto: AuthorizeUserDTO): Promise<AuthorizeUserUseCaseResponse> {
const params = dto.params
const decodedUri = decodeURI(params.redirect_uri)
const authSecretExists = await this.authSecretRepo.exists(params.client_id, decodedUri)
if (authSecretExists.isErr() || authSecretExists.value === false) {
return Result.err(new AuthorizeUserErrors.InvalidRequestParameters())
}
const user = dto.req.user as User
if (user === undefined) {
const redirectParams = new ParamList(
Object.entries(params).map((paramPair) => new ParamPair(paramPair[0], paramPair[1]))
)
return Result.err({
redirectParams,
redirectUrl: `${process.env.PUBLIC_HOST}/login`,
})
}
const authCode = AuthCode.create({
clientId: params.client_id,
userId: user.userId.id.toString(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

userId.id? I think we changed something in waterpark to reduce the level of object nesting here. cc @KTong821

userEmail: user.email.value,
userEmailVerified: user.isEmailVerified || false,
authCodeString: new AuthCodeString(),
})
if (authCode.isErr()) {
return Result.err(new AppError.UnexpectedError('Authcode creation failed'))
}
await this.authCodeRepo.save(authCode.value)
const redirectParams = new ParamList([
new ParamPair('code', authCode.value.authCodeString.getValue()),
])
const AuthorizeUserSuccessResponse: AuthorizeUserSuccess = {
redirectParams: redirectParams,
redirectUrl: params.redirect_uri,
}

return Result.ok(AuthorizeUserSuccessResponse)
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import Joi from 'joi'

export const SUPPORTED_OPEN_ID_RESPONSE_TYPES = ['code']
export const SUPPORTED_OPEN_ID_SCOPE = ['openid']

export interface CreateUserDTOBody {
email: string
password: string
Expand All @@ -8,12 +11,12 @@ export interface CreateUserDTOBody {
export interface CreateUserDTOParams {
client_id: string
scope: string
response_type: string,
response_type: string
redirect_uri: string
}

export interface CreateUserDTO {
body: CreateUserDTOBody,
export interface CreateUserDTO {
body: CreateUserDTOBody
params?: CreateUserDTOParams
}

Expand All @@ -22,14 +25,7 @@ export const createUserDTOBodySchema = Joi.object<CreateUserDTOBody>({
password: Joi.string().required(),
}).options({ abortEarly: false })

export const createUserDTOParamsSchema = Joi.object<CreateUserDTOParams>({
client_id: Joi.string().required(),
scope: Joi.string().required(),
response_type: Joi.string().required(),
redirect_uri: Joi.string().required()
}).options({ abortEarly: false })

export const createUserDTOSchema = Joi.object<CreateUserDTO>({
body: createUserDTOBodySchema.required(),
params: createUserDTOParamsSchema.optional() // this ensures that all of the necessary request params for client authentication are present, not just an insufficient subset
params: Joi.object().optional(),
}).options({ abortEarly: false })
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,4 @@ export namespace CreateUserErrors {
super(`An account with the email ${email} already exists`)
}
}

export class InvalidOpenIDParamsError extends Error {
public constructor() {
super(`Invalid OpenID parameters were provided.`)
}
}
}
Loading