refactor: migrate /auth endpoint handling to Typescript #215

karrui · 2020-08-26T11:16:46Z

Problem

This PR tries to encapsulate the /auth route under a new auth module located in modules/auth.

The first Typescript integration tests are also written for auth.routes, using a new testing directory directly inside the module folder for closer integration. The various tsconfig and eslintrc files has been updated to accommodate this change.

Related to #144

Solution

Features:

New auth module containing
- auth/auth.route
- auth/auth.middlewares
- auth/auth.controller
- auth/auth.service
- auth/auth.errors

Improvements:

Remove unused functions in old auth controller

Tests

Release Tests

Sign out should work properly
Logging in to existing user should work properly
- Invalid OTP should return 422 with invalid OTP message
- Entering wrong OTP 10 times should return 422, and subsequently entering correct OTP will still return 422.
- Requesting for new OTP should work, and login successfully
Attempt to login as a new user by logging in with your open.gov.sg email; e.g. <youremail>+test@open.gov.sg
- Should create new user successfully upon log in, test with creating a form, log out and log back in. Your form should still be there.

This is so that this function can be repurposed to also fetch the agency whilst validating.

# Conflicts: # src/app/controllers/authentication.server.controller.js # src/app/modules/user/user.service.ts # src/loaders/express/index.ts

# Conflicts: # src/app/controllers/authentication.server.controller.js

# Conflicts: # src/app/controllers/authentication.server.controller.js # src/app/routes/authentication.server.routes.js # src/loaders/express/index.ts # tests/unit/backend/controllers/authentication.server.controller.spec.js

karrui · 2020-09-02T04:18:37Z

Ready for review. Please don't be scared by the high line count, it's just because it has a lot of new tests.

karrui · 2020-09-02T04:19:51Z

~~Forgot /signout integration tests...~~

liangyuanruo · 2020-09-03T02:33:48Z

src/app/modules/auth/auth.controller.ts

+  if (verifyErr) {
+    logger.warn({
+      message:
+        verifyErr instanceof InvalidOtpError
+          ? 'Login OTP is invalid'
+          : 'Error occurred when trying to validate login OTP',
+      meta: logMeta,
+      error: verifyErr,
+    })
+
+    if (verifyErr instanceof InvalidOtpError) {
+      return res.status(verifyErr.status).send(verifyErr.message)
+    }


Suggested change

if (verifyErr) {

logger.warn({

message:

verifyErr instanceof InvalidOtpError

? 'Login OTP is invalid'

: 'Error occurred when trying to validate login OTP',

meta: logMeta,

error: verifyErr,

})

if (verifyErr instanceof InvalidOtpError) {

return res.status(verifyErr.status).send(verifyErr.message)

}

if (verifyErr) {

logger.warn({

message:

verifyErr instanceof InvalidOtpError

? 'Login OTP is invalid'

: 'Error occurred when trying to validate login OTP',

meta: logMeta,

error: verifyErr,

return res.status(verifyErr.status).send(verifyErr.message)

}

Would this be safer? Although we have tests, I would like to minimise the chance of a future engineer refactoring wrongly or introducing a vulnerability to the login.

I will change the check to instanceof ApplicationError, and all known errors should be an ApplicationError. However, we will still need the check, since normal errors don't have err.status.

EDIT: Updated the check in 9db99b4

liangyuanruo · 2020-09-03T02:54:51Z

src/app/modules/auth/auth.controller.ts

+
+  // OTP is valid, proceed to login user.
+  try {
+    const agency = await AuthService.getAgencyWithEmail(email)


here we see that the middleware pattern with validateDomain results in an additional network call to the database to fetch the agency object again. can we revisit our discussion? because (1) adhering to a pattern at the cost of performance seems dogmatic, and (2) the controller can help to set the context in which an agency lookup failure occurs, so duplicate logging may not be a real issue.

We can discuss after syncup later; but I also agree. I've experimented with passing agency into locals like before in b6a48db; seems like the best way to save some db calls. Take a look.

src/app/modules/auth/auth.errors.ts

src/app/modules/user/user.service.ts

This prevents developers from using ApplicationError directly and are forced to extend from the class (in Typescript).

# Conflicts: # src/app/controllers/authentication.server.controller.js

# Conflicts: # src/app/models/user.server.model.ts

karrui added 18 commits August 25, 2020 15:16

refactor: replace authentication routes with nested router in ts

1e4997b

feat(AuthService): add validateDomain method

1b33c2d

feat(AuthController): add handleCheckUser function

6228e5c

refactor(AuthMiddleware): extract validateDomain from AuthService

286f208

feat(TokenModel): add upsertOtp static method to model

8630ce2

feat(AuthService): add createLoginOtp function

5ff7333

feat: add UserService to upsert User document on login

bcac535

feat(AuthService): add verifyLoginOtp function

b5c0f80

refactor: rename validateDomain to getAgencyWithEmail

2b7eccd

This is so that this function can be repurposed to also fetch the agency whilst validating.

feat(TokenModel): add incrementAttemptsByEmail function

40828f6

refactor: rename the old AuthController OldAuthController temporarily

e634888

feat: add validateDomain middleware for entire /auth route

e39a1a4

feat(AuthController): add auth controller handler functions

88d6120

fix: update imports to be relative

5dcd1c4

fix: update middleware to not be router level

a91ddd9

feat: add AuthRules

5a1356f

feat: add better documentation for functions

50dad9f

feat: remove unused functions in old auth controller

7dcc274

karrui marked this pull request as draft August 26, 2020 11:17

karrui added 11 commits August 26, 2020 19:21

feat: add documentation for validateDomain middleware

d0d238b

test(AuthController): remove deleted function tests

1f4ba08

Merge branch 'develop' into feat/ts-auth-controller

7871b74

# Conflicts: # src/app/controllers/authentication.server.controller.js # src/app/modules/user/user.service.ts # src/loaders/express/index.ts

fix: invalid otpGenerator import from merging

199ad00

Merge branch 'develop' into feat/ts-auth-controller

8c853ce

# Conflicts: # src/app/controllers/authentication.server.controller.js

fix(Auth): use new logger instantiation

8b70e94

Merge branch 'develop' into feat/ts-auth-controller

07da5ca

# Conflicts: # src/app/controllers/authentication.server.controller.js # src/app/routes/authentication.server.routes.js # src/loaders/express/index.ts # tests/unit/backend/controllers/authentication.server.controller.spec.js

chore: add eslint for all "*.spec.ts" files

153d0b8

test(AuthService): add tests for getAgencyWithEmail function

3380d20

test(AuthService): add tests for createLoginOtp function

4cb9f6f

test(AuthService): add verifyLoginOtp tests

b6a5f9a

karrui requested review from liangyuanruo, mantariksh and arshadali172 September 2, 2020 04:18

karrui marked this pull request as ready for review September 2, 2020 04:18

karrui changed the title ~~wip: migrate /auth endpoint handling to Typescript~~ feat: migrate /auth endpoint handling to Typescript Sep 2, 2020

karrui changed the title ~~feat: migrate /auth endpoint handling to Typescript~~ refactor: migrate /auth endpoint handling to Typescript Sep 2, 2020

karrui added 2 commits September 2, 2020 12:55

test(AuthRoutes): add GET /auth/signout integration tests

ddf9857

fix: add guards for undefinedness

4264a2a

liangyuanruo reviewed Sep 3, 2020

View reviewed changes

src/app/modules/auth/auth.errors.ts Show resolved Hide resolved

liangyuanruo reviewed Sep 3, 2020

View reviewed changes

src/app/modules/user/user.service.ts Outdated Show resolved Hide resolved

karrui added 12 commits September 3, 2020 11:35

refactor(User): move upsertUser method into user schema

754c7ed

refactor: convert ApplicationError into abstract class

c8c0217

This prevents developers from using ApplicationError directly and are forced to extend from the class (in Typescript).

refactor(AuthController): update error type check to ApplicationError

9db99b4

feat: add ResponseWithLocals typing to extend res.locals typing

45ef05d

feat(AuthController): retrieve agency from res.locals instead

b6a48db

refactor(UserService): rename upsertAndReturnUser to retrieveUser

0f910c0

test: update tests to use UserService.retrieveUser instead

e0befed

Merge branch 'develop' into feat/ts-auth-controller

f5c9136

# Conflicts: # src/app/controllers/authentication.server.controller.js

fix(Auth): import constant.ts instead of now deleted defaults.ts

ac85062

style: fix lint from merging

f9b30bc

Merge branch 'develop' into feat/ts-auth-controller

a05faaf

# Conflicts: # src/app/models/user.server.model.ts

lint: fix lint issues from stricter lint rules

04d7209

liangyuanruo approved these changes Sep 7, 2020

View reviewed changes

liangyuanruo merged commit 5cba28a into develop Sep 7, 2020

liangyuanruo deleted the feat/ts-auth-controller branch September 7, 2020 14:36

karrui mentioned this pull request Sep 17, 2020

feat: Using neverthrow to explicitly handle errors in AuthController #332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: migrate /auth endpoint handling to Typescript #215

refactor: migrate /auth endpoint handling to Typescript #215

karrui commented Aug 26, 2020 •

edited

Loading

karrui commented Sep 2, 2020

karrui commented Sep 2, 2020 •

edited

Loading

liangyuanruo Sep 3, 2020

karrui Sep 3, 2020 •

edited

Loading

liangyuanruo Sep 3, 2020 •

edited

Loading

karrui Sep 3, 2020

refactor: migrate /auth endpoint handling to Typescript #215

refactor: migrate /auth endpoint handling to Typescript #215

Conversation

karrui commented Aug 26, 2020 • edited Loading

Problem

Solution

Tests

Release Tests

karrui commented Sep 2, 2020

karrui commented Sep 2, 2020 • edited Loading

liangyuanruo Sep 3, 2020

Choose a reason for hiding this comment

karrui Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

liangyuanruo Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

karrui Sep 3, 2020

Choose a reason for hiding this comment

karrui commented Aug 26, 2020 •

edited

Loading

karrui commented Sep 2, 2020 •

edited

Loading

karrui Sep 3, 2020 •

edited

Loading

liangyuanruo Sep 3, 2020 •

edited

Loading