Add new use cases to authorize user, register SPs, and get id tokens #22

ShreyasPrasad · 2021-08-26T08:05:20Z

xujustinj · 2021-08-26T17:59:12Z

.../users/application/use-cases/authorize-user/__tests__/authorize-user-controller.test.unit.ts

+    authorizeUserDTO = {
+      req: httpMocks.createRequest(),
+      params: {
+        client_id: 'i291u92jksdn',


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

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

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)

xujustinj · 2021-08-28T18:09:05Z

.../users/application/use-cases/authorize-user/__tests__/authorize-user-controller.test.unit.ts

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


Seeing double

Suggested change

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

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

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

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').

xujustinj · 2021-08-28T18:19:29Z

.../users/application/use-cases/authorize-user/__tests__/authorize-user-controller.test.unit.ts

+    authorizeUserDTO = {
+      req: httpMocks.createRequest(),
+      params: {
+        client_id: 'i291u92jksdn',


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)

xujustinj

reviewed 1/3, gotta go now but will finish the rest later

xujustinj · 2021-08-28T19:58:12Z

server/src/modules/users/application/use-cases/authorize-user/authorize-user-dto.ts

+}).options({ abortEarly: false })
+
+export const AuthorizeUserDTOSchema = Joi.object<AuthorizeUserDTO>({
+  req: Joi.object().required(),


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

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?

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.

xujustinj · 2021-08-28T20:01:46Z

server/src/modules/users/application/use-cases/authorize-user/authorize-user-use-case.ts

+    }
+    const authCode = AuthCode.create({
+      clientId: params.client_id,
+      userId: user.userId.id.toString(),


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

xujustinj · 2021-08-28T20:04:55Z

server/src/modules/users/application/use-cases/create-user/create-user-use-case.ts

@@ -60,47 +54,41 @@ export class CreateUserUseCase implements UseCaseWithDTO<CreateUserDTO, CreateUs
    try {
      const userAlreadyExists = await this.userRepo.exists(email)

-      if (userAlreadyExists && userAlreadyExists.isOk()){
+      if (userAlreadyExists && userAlreadyExists.isOk()) {


I'm pretty sure userAlreadyExists is always truthy. Did you mean

Suggested change

if (userAlreadyExists && userAlreadyExists.isOk()) {

if (userAlreadyExists.isOk() && userAlreadyExists.value) {

xujustinj · 2021-08-28T20:08:04Z

server/src/modules/users/application/use-cases/create-user/create-user-use-case.ts

        return Result.err(new AppError.UnexpectedError(updatedUser.error.message))

-      if(!dto.params || !dto.params.scope){
+      if (!dto.params || !dto.params.scope) {


I think there was discussion in the past about spliting the use case into separate use cases, and then moving this if/else splitting into the controller to decide which use case to call

You're right - will add that when I add the /logout use-case. Coming soon to a commit near you.

xujustinj · 2021-08-28T20:11:24Z

server/src/modules/users/application/use-cases/get-user/get-user-errors.ts

+      super()
+      this.message = `No account with the id ${id} exists`


Suggested change

super()

this.message = `No account with the id ${id} exists`

super(`No account with the id ${id} exists`)

xujustinj · 2021-08-28T20:11:49Z

server/src/modules/users/application/use-cases/login-user/login-user-errors.ts

+      super()
      this.message = `Incorrect email/password combination provided.`


Suggested change

super()

this.message = `Incorrect email/password combination provided.`

super(`Incorrect email/password combination provided.`)

xujustinj · 2021-08-28T20:14:11Z

server/src/modules/users/domain/value-objects/auth-code-string.ts

  public constructor(hashedValue?: string) {
-    if(hashedValue){
+    if (hashedValue) {


you might run into truthiness problems with empty strings

Suggested change

if (hashedValue) {

if (hashedValue !== undefined) {

xujustinj · 2021-08-28T20:15:05Z

server/src/modules/users/domain/value-objects/auth-code-string.ts

  private getRandomCode() {
-    /*motivation for a 256 bit (= 32 byte) crypographic key can be found here
+    /*motivation for a 256 bit (= 32 byte) cryptographic key can be found here
      https://www.geeksforgeeks.org/node-js-crypto-randombytes-method/
    */
    const authCodeBuffer = crypto.randomBytes(32)
    return authCodeBuffer.toString('hex')
  }


this function can just be an unexported global function in this file, rather than a member method

xujustinj · 2021-08-28T20:16:25Z

server/src/shared/app/base-controller.ts

+    res.redirect(params.getFormattedUrlWithParams(url))
+    return res


I'm curious what the change here does; what is the difference between the two implementations?

It was overwriting the 302 redirect status of the res. This status code is what instructs the frontend browser to change its page target.

xujustinj

second batch of comments; slowly chipping away at this PR

xujustinj · 2021-09-09T09:47:10Z

server/scripts/approve_client.py

+    if(len(sys.argv)!=2):
+        print("Invalid number of cmd line arguments provided.")
+    client_id = base64.b64decode(sys.argv[1])


no need to reinvent the wheel, let's use argparse instead

xujustinj · 2021-09-09T09:59:40Z

.../modules/users/application/use-cases/create-user/__tests__/create-user-use-case.test.unit.ts

+    const createUserErr = createUserResult as Err<
+      CreateUserSuccess,
+      UserValueObjectErrors.InvalidEmail
+    >
    expect(createUserErr.error instanceof UserValueObjectErrors.InvalidEmail).toBe(true)


another (also-ugly, but typesafe) way to do it is

Suggested change

const createUserErr = createUserResult as Err<

CreateUserSuccess,

UserValueObjectErrors.InvalidEmail

>

expect(createUserErr.error instanceof UserValueObjectErrors.InvalidEmail).toBe(true)

if (!createUserResult.isErr()) throw new Error('this is impossible')

expect(createUserResult.error instanceof UserValueObjectErrors.InvalidEmail).toBe(true)

xujustinj · 2021-09-09T10:21:43Z

server/src/modules/users/application/use-cases/discover-sp/discover-sp-errors.ts

+      super()
+      this.message = `The provided client name ${clientName} is already in use.`


Suggested change

super()

this.message = `The provided client name ${clientName} is already in use.`

super(`The provided client name ${clientName} is already in use.`)

xujustinj · 2021-09-09T10:24:12Z

server/src/modules/users/application/use-cases/get-token/get-token-dto.ts

+
+export const getTokenDTOSchema = Joi.object<GetTokenDTO>({
+  //Example: Authorization: Basic 3904238orfiefiekfhjri3u24r789
+  authHeader: Joi.string().pattern(new RegExp('^Basic .+$')).required(),


I think the regex could be a bit more specific than that

xujustinj · 2021-09-09T10:24:47Z

server/src/modules/users/application/use-cases/get-token/get-token-errors.ts

+      super()
+      this.message = `Incorrect authentication credentials provided.`


Suggested change

super()

this.message = `Incorrect authentication credentials provided.`

super(`Incorrect authentication credentials provided.`)

xujustinj · 2021-09-09T10:27:09Z

server/src/modules/users/application/use-cases/login-user/login-user-dto.ts

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


what is the reason for dropping this?

xujustinj · 2021-09-09T10:28:53Z

server/src/modules/users/domain/entities/user/__tests__/user.test.unit.ts

@@ -30,29 +29,6 @@ describe('User AggregateRoot', () => {
    expect(DomainEvents.markAggregateForDispatch).toBeCalled()
  })

-  test('it adds a UserLoggedIn domain event on user login', () => {


what happened to this test?

Add new use cases to authorize user, register SPs, and get id tokens

9a417d1

xujustinj reviewed Aug 26, 2021

View reviewed changes

xujustinj reviewed Aug 28, 2021

View reviewed changes

xujustinj requested changes Aug 28, 2021

View reviewed changes

jumbodogwoof added 2 commits August 29, 2021 01:56

Address compiler errors and add is_verified switch script

50efdca

Address first batch of review comments

07e68d3

xujustinj reviewed Sep 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new use cases to authorize user, register SPs, and get id tokens #22

Add new use cases to authorize user, register SPs, and get id tokens #22

ShreyasPrasad commented Aug 26, 2021

xujustinj Aug 26, 2021

ShreyasPrasad Aug 27, 2021

xujustinj Aug 28, 2021

xujustinj Aug 28, 2021

xujustinj Aug 28, 2021

xujustinj Aug 28, 2021

xujustinj left a comment

xujustinj Aug 28, 2021

ShreyasPrasad Aug 29, 2021

xujustinj Aug 29, 2021

xujustinj Aug 28, 2021

xujustinj Aug 28, 2021

xujustinj Aug 28, 2021

ShreyasPrasad Aug 29, 2021

xujustinj Aug 28, 2021

xujustinj Aug 28, 2021

xujustinj Aug 28, 2021

xujustinj Aug 28, 2021

xujustinj Aug 28, 2021

ShreyasPrasad Aug 29, 2021

xujustinj left a comment

xujustinj Sep 9, 2021

xujustinj Sep 9, 2021

xujustinj Sep 9, 2021

xujustinj Sep 9, 2021

xujustinj Sep 9, 2021

xujustinj Sep 9, 2021

xujustinj Sep 9, 2021

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

	if (userAlreadyExists && userAlreadyExists.isOk()) {
	if (userAlreadyExists.isOk() && userAlreadyExists.value) {

	super()
	this.message = `No account with the id ${id} exists`
	super(`No account with the id ${id} exists`)

		super()
		this.message = `Incorrect email/password combination provided.`

	super()
	this.message = `Incorrect email/password combination provided.`
	super(`Incorrect email/password combination provided.`)

		res.redirect(params.getFormattedUrlWithParams(url))
		return res

		super()
		this.message = `The provided client name ${clientName} is already in use.`

	super()
	this.message = `The provided client name ${clientName} is already in use.`
	super(`The provided client name ${clientName} is already in use.`)

		super()
		this.message = `Incorrect authentication credentials provided.`

	super()
	this.message = `Incorrect authentication credentials provided.`
	super(`Incorrect authentication credentials provided.`)

Add new use cases to authorize user, register SPs, and get id tokens #22

Are you sure you want to change the base?

Add new use cases to authorize user, register SPs, and get id tokens #22

Conversation

ShreyasPrasad commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xujustinj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xujustinj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment