diff --git a/CHANGELOG.md b/CHANGELOG.md index fc2a6371d..068c11fbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,22 @@ All notable changes to this project will be documented in this file. Dates are d Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). -#### [v0.69.0](https://github.com/isomerpages/isomercms-backend/compare/v0.68.2...v0.69.0) - +#### [v0.70.0](https://github.com/isomerpages/isomercms-backend/compare/v0.69.0...v0.70.0) + +- fix: schema [`#1203`](https://github.com/isomerpages/isomercms-backend/pull/1203) +- fix(otp): unwrap ResultAsync value [`#1204`](https://github.com/isomerpages/isomercms-backend/pull/1204) +- fix(autoLogoutIssue): failing whoami [`#1196`](https://github.com/isomerpages/isomercms-backend/pull/1196) +- Fix/add validators [`#1197`](https://github.com/isomerpages/isomercms-backend/pull/1197) +- fix(otp): increment instead of update for concurrency [`#1186`](https://github.com/isomerpages/isomercms-backend/pull/1186) +- 0.69.0 [`#1190`](https://github.com/isomerpages/isomercms-backend/pull/1190) - Revert "fix(rateLimiter): correct rate limits (#1183)" [`#1195`](https://github.com/isomerpages/isomercms-backend/pull/1195) - fix(site checker): dont trigger alarms [`#1194`](https://github.com/isomerpages/isomercms-backend/pull/1194) - chore(audit): update form question phrasing [`#1192`](https://github.com/isomerpages/isomercms-backend/pull/1192) + +#### [v0.69.0](https://github.com/isomerpages/isomercms-backend/compare/v0.68.2...v0.69.0) + +> 7 March 2024 + - fix(api): fix media route timeout [`#1170`](https://github.com/isomerpages/isomercms-backend/pull/1170) - fix(linkChecker): bug fixes [`#1187`](https://github.com/isomerpages/isomercms-backend/pull/1187) - Feat/add back repair form lock [`#1179`](https://github.com/isomerpages/isomercms-backend/pull/1179) @@ -68,14 +79,13 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - Release/0.66.2 [`#1144`](https://github.com/isomerpages/isomercms-backend/pull/1144) - release(0.66.1): merge to develop [`#1142`](https://github.com/isomerpages/isomercms-backend/pull/1142) - 0.66.0 [`#1140`](https://github.com/isomerpages/isomercms-backend/pull/1140) -- fix(tsak-def): add env vars [`fdf1230`](https://github.com/isomerpages/isomercms-backend/commit/fdf123001fc5fa6d8feaed0545eacb2670ff4ffb) #### [v0.66.4](https://github.com/isomerpages/isomercms-backend/compare/v0.66.3...v0.66.4) > 20 February 2024 - fix(ci): use workflwo [`a5a225d`](https://github.com/isomerpages/isomercms-backend/commit/a5a225dd203826726ad9f8e39c420bdb4a8e0d2c) -- fix(tsak-def): add env vars [`ad02a2f`](https://github.com/isomerpages/isomercms-backend/commit/ad02a2f630d657b468bb48fd48f0572051ae85e3) +- fix(tsak-def): add env vars [`fdf1230`](https://github.com/isomerpages/isomercms-backend/commit/fdf123001fc5fa6d8feaed0545eacb2670ff4ffb) #### [v0.66.3](https://github.com/isomerpages/isomercms-backend/compare/v0.66.2...v0.66.3) @@ -138,12 +148,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - fix: remove unnecessary push logs [`#1109`](https://github.com/isomerpages/isomercms-backend/pull/1109) - fix(rr): skip checking the existence of review request [`#1102`](https://github.com/isomerpages/isomercms-backend/pull/1102) - release/0.61.0 [`#1104`](https://github.com/isomerpages/isomercms-backend/pull/1104) -- fix(sl): include issuewild if CAA records are needed [`#1106`](https://github.com/isomerpages/isomercms-backend/pull/1106) #### [v0.61.0](https://github.com/isomerpages/isomercms-backend/compare/v0.60.0...v0.61.0) -> 10 January 2024 +> 11 January 2024 +- fix(sl): include issuewild if CAA records are needed [`#1106`](https://github.com/isomerpages/isomercms-backend/pull/1106) - chore: upgrade axios [`#1100`](https://github.com/isomerpages/isomercms-backend/pull/1100) - build(deps): bump follow-redirects from 1.15.2 to 1.15.4 [`#1101`](https://github.com/isomerpages/isomercms-backend/pull/1101) - fix(ci): reverts ci changes to allow staging updates [`#1084`](https://github.com/isomerpages/isomercms-backend/pull/1084) @@ -225,12 +235,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - fix(siteCreate): add redirect rules [`#1036`](https://github.com/isomerpages/isomercms-backend/pull/1036) - chore: remove extra and unused submodules [`#1031`](https://github.com/isomerpages/isomercms-backend/pull/1031) - release/0.54.0 [`#1033`](https://github.com/isomerpages/isomercms-backend/pull/1033) -- fix: use cTimeMs instead of birthtime due to EFS [`#1035`](https://github.com/isomerpages/isomercms-backend/pull/1035) #### [v0.54.0](https://github.com/isomerpages/isomercms-backend/compare/v0.53.0...v0.54.0) > 14 November 2023 +- fix: use cTimeMs instead of birthtime due to EFS [`#1035`](https://github.com/isomerpages/isomercms-backend/pull/1035) - fix(pagination): total length [`#1032`](https://github.com/isomerpages/isomercms-backend/pull/1032) - fix(staging-lite): apps were created for wrong br [`#1014`](https://github.com/isomerpages/isomercms-backend/pull/1014) - fix(cm): extra timeout [`#1027`](https://github.com/isomerpages/isomercms-backend/pull/1027) @@ -357,12 +367,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - build(deps-dev): bump @babel/traverse from 7.22.8 to 7.23.2 [`#984`](https://github.com/isomerpages/isomercms-backend/pull/984) - release/v0.48.0 [`#979`](https://github.com/isomerpages/isomercms-backend/pull/979) - feat(staging-id): add column to store the id [`#983`](https://github.com/isomerpages/isomercms-backend/pull/983) -- Fix: collaborators service tests [`#978`](https://github.com/isomerpages/isomercms-backend/pull/978) #### [v0.48.0](https://github.com/isomerpages/isomercms-backend/compare/v0.47.0...v0.48.0) > 18 October 2023 +- Fix: collaborators service tests [`#978`](https://github.com/isomerpages/isomercms-backend/pull/978) - chore(commitService): proper naming [`#975`](https://github.com/isomerpages/isomercms-backend/pull/975) - Feat/is 585 govt sgid login rollout [`#976`](https://github.com/isomerpages/isomercms-backend/pull/976) - test(quickie): unit tests [`#973`](https://github.com/isomerpages/isomercms-backend/pull/973) diff --git a/package-lock.json b/package-lock.json index 113f49213..c24b4c0a6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "isomercms", - "version": "0.69.0", + "version": "0.70.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "isomercms", - "version": "0.69.0", + "version": "0.70.0", "dependencies": { "@aws-sdk/client-amplify": "^3.370.0", "@aws-sdk/client-cloudwatch-logs": "^3.370.0", diff --git a/package.json b/package.json index e6b895f30..c3718b2d7 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "isomercms", - "version": "0.69.0", + "version": "0.70.0", "private": true, "scripts": { "build": "tsc -p tsconfig.build.json", diff --git a/src/config/config.ts b/src/config/config.ts index e02c3ee5f..9f289f94d 100644 --- a/src/config/config.ts +++ b/src/config/config.ts @@ -96,7 +96,7 @@ const config = convict({ format: ["localhost", "cms.isomer.gov.sg", "isomer.gov.sg"], default: "localhost", }, - tokenExpiry: { + tokenExpiryInMs: { doc: "Expiry duration for auth token in milliseconds", env: "AUTH_TOKEN_EXPIRY_DURATION_IN_MILLISECONDS", format: "required-positive-number", diff --git a/src/fixtures/sessionData.ts b/src/fixtures/sessionData.ts index 638c1e433..b6505ac98 100644 --- a/src/fixtures/sessionData.ts +++ b/src/fixtures/sessionData.ts @@ -21,7 +21,7 @@ import { export const mockAccessToken = "mockAccessToken" export const mockGithubId = "mockGithubId" export const mockIsomerUserId = "1" -export const mockEmail = "mockEmail" +export const mockEmail = "mockEmail@email.com" export const mockTreeSha = "mockTreeSha" export const mockCurrentCommitSha = "mockCurrentCommitSha" export const mockSiteName = "mockSiteName" diff --git a/src/integration/Reviews.spec.ts b/src/integration/Reviews.spec.ts index 849830137..5dfbed801 100644 --- a/src/integration/Reviews.spec.ts +++ b/src/integration/Reviews.spec.ts @@ -975,9 +975,11 @@ describe("Review Requests Integration Tests", () => { ) // Act - const actual = await request(app).post( - `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}` - ) + const actual = await request(app) + .post(`/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`) + .send({ + reviewers: [MOCK_USER_EMAIL_THREE], + }) // Assert expect(actual.statusCode).toEqual(404) @@ -992,7 +994,11 @@ describe("Review Requests Integration Tests", () => { ) // Act - const actual = await request(app).post(`/${MOCK_REPO_NAME_ONE}/123456`) + const actual = await request(app) + .post(`/${MOCK_REPO_NAME_ONE}/123456`) + .send({ + reviewers: [MOCK_USER_EMAIL_THREE], + }) // Assert expect(actual.statusCode).toEqual(404) @@ -1007,9 +1013,11 @@ describe("Review Requests Integration Tests", () => { ) // Act - const actual = await request(app).post( - `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}` - ) + const actual = await request(app) + .post(`/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`) + .send({ + reviewers: [MOCK_USER_EMAIL_THREE], + }) // Assert expect(actual.statusCode).toEqual(403) @@ -1078,66 +1086,65 @@ describe("Review Requests Integration Tests", () => { }) }) - it("should close the review request successfully", async () => { + it("should return 404 if site is not found", async () => { // Arrange const app = generateRouterForUserWithSite( subrouter, MOCK_USER_SESSION_DATA_ONE, - MOCK_REPO_NAME_ONE + MOCK_REPO_NAME_TWO ) - mockGenericAxios.patch.mockResolvedValueOnce(null) // Act const actual = await request(app).delete( - `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}` + `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}` ) // Assert - expect(actual.statusCode).toEqual(200) + expect(actual.statusCode).toEqual(404) }) - it("should return 404 if site is not found", async () => { + it("should return 404 if the review request is not found", async () => { // Arrange const app = generateRouterForUserWithSite( subrouter, MOCK_USER_SESSION_DATA_ONE, - MOCK_REPO_NAME_TWO + MOCK_REPO_NAME_ONE ) // Act - const actual = await request(app).post( - `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}` - ) + const actual = await request(app).delete(`/${MOCK_REPO_NAME_ONE}/123456`) // Assert expect(actual.statusCode).toEqual(404) }) - it("should return 404 if the review request is not found", async () => { + it("should return 403 if user is not the original requestor", async () => { // Arrange const app = generateRouterForUserWithSite( subrouter, - MOCK_USER_SESSION_DATA_ONE, + MOCK_USER_SESSION_DATA_TWO, MOCK_REPO_NAME_ONE ) // Act - const actual = await request(app).post(`/${MOCK_REPO_NAME_ONE}/123456`) + const actual = await request(app).delete( + `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}` + ) // Assert - expect(actual.statusCode).toEqual(404) + expect(actual.statusCode).toEqual(403) }) - it("should return 403 if user is not the original requestor", async () => { + it("should return 403 if the user is not a valid site member", async () => { // Arrange const app = generateRouterForUserWithSite( subrouter, - MOCK_USER_SESSION_DATA_TWO, + MOCK_USER_SESSION_DATA_THREE, MOCK_REPO_NAME_ONE ) // Act - const actual = await request(app).post( + const actual = await request(app).delete( `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}` ) @@ -1145,21 +1152,22 @@ describe("Review Requests Integration Tests", () => { expect(actual.statusCode).toEqual(403) }) - it("should return 403 if the user is not a valid site member", async () => { + it("should close the review request successfully", async () => { // Arrange const app = generateRouterForUserWithSite( subrouter, - MOCK_USER_SESSION_DATA_THREE, + MOCK_USER_SESSION_DATA_ONE, MOCK_REPO_NAME_ONE ) + mockGenericAxios.patch.mockResolvedValueOnce(null) // Act - const actual = await request(app).post( + const actual = await request(app).delete( `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}` ) // Assert - expect(actual.statusCode).toEqual(403) + expect(actual.statusCode).toEqual(200) }) }) @@ -1871,9 +1879,11 @@ describe("Review Requests Integration Tests", () => { mockGenericAxios.post.mockResolvedValueOnce(null) // Act - const actual = await request(app).post( - `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments` - ) + const actual = await request(app) + .post( + `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments` + ) + .send({ message: "blahblah" }) // Assert expect(actual.statusCode).toEqual(200) @@ -1888,9 +1898,11 @@ describe("Review Requests Integration Tests", () => { ) // Act - const actual = await request(app).post( - `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments` - ) + const actual = await request(app) + .post( + `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments` + ) + .send({ message: "blahblah" }) // Assert expect(actual.statusCode).toEqual(404) @@ -1905,9 +1917,11 @@ describe("Review Requests Integration Tests", () => { ) // Act - const actual = await request(app).post( - `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments` - ) + const actual = await request(app) + .post( + `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments` + ) + .send({ message: "blahblah" }) // Assert expect(actual.statusCode).toEqual(404) @@ -1922,9 +1936,9 @@ describe("Review Requests Integration Tests", () => { ) // Act - const actual = await request(app).post( - `/${MOCK_REPO_NAME_ONE}/123456/comments` - ) + const actual = await request(app) + .post(`/${MOCK_REPO_NAME_ONE}/123456/comments`) + .send({ message: "blahblah" }) // Assert expect(actual.statusCode).toEqual(404) diff --git a/src/integration/Users.spec.ts b/src/integration/Users.spec.ts index 10ae0936b..b7dbc4676 100644 --- a/src/integration/Users.spec.ts +++ b/src/integration/Users.spec.ts @@ -40,7 +40,7 @@ const subrouter = express() // that allows us to set this properties also subrouter.use((req, res, next) => { const userSessionData = new UserSessionData({ - isomerUserId: req.body.userId, + isomerUserId: mockIsomerUserId, githubId: req.body.githubId, email: req.body.email, }) @@ -223,7 +223,6 @@ describe("Users Router", () => { const actual = await request(app).post("/email/verifyOtp").send({ email: mockValidEmail, otp, - userId: mockIsomerUserId, }) const updatedUser = await User.findOne({ where: { @@ -252,7 +251,6 @@ describe("Users Router", () => { const actual = await request(app).post("/email/verifyOtp").send({ email: mockValidEmail, otp: wrongOtp, - userId: mockIsomerUserId, }) // Assert @@ -274,7 +272,6 @@ describe("Users Router", () => { const actual = await request(app).post("/email/verifyOtp").send({ email: mockValidEmail, otp: "", - userId: mockIsomerUserId, }) // Assert @@ -296,7 +293,6 @@ describe("Users Router", () => { const actual = await request(app).post("/email/verifyOtp").send({ email: mockValidEmail, otp: undefined, - userId: mockIsomerUserId, }) // Assert @@ -326,7 +322,6 @@ describe("Users Router", () => { const actual = await request(app).post("/email/verifyOtp").send({ email: mockValidEmail, otp, - userId: mockIsomerUserId, }) const oldOtp = otp @@ -342,7 +337,6 @@ describe("Users Router", () => { const newActual = await request(app).post("/email/verifyOtp").send({ email: mockValidEmail, otp: oldOtp, - userId: mockIsomerUserId, }) // Assert @@ -366,7 +360,6 @@ describe("Users Router", () => { const actual = await request(app).post("/email/verifyOtp").send({ email: mockValidEmail, otp: mockInvalidOtp, - userId: mockIsomerUserId, }) const otpEntry = await Otp.findOne({ where: { email: mockValidEmail }, @@ -377,12 +370,10 @@ describe("Users Router", () => { if (i <= maxNumOfOtpAttempts) { expect(otpEntry?.attempts).toBe(i) - expect(actual.body.error.message).toBe("OTP is not valid") + expect(actual.body.message).toBe("OTP is not valid") } else { expect(otpEntry?.attempts).toBe(maxNumOfOtpAttempts) - expect(actual.body.error.message).toBe( - "Max number of attempts reached" - ) + expect(actual.body.message).toBe("Max number of attempts reached") } } }) @@ -402,7 +393,6 @@ describe("Users Router", () => { await request(app).post("/email/verifyOtp").send({ email: mockValidEmail, otp: mockInvalidOtp, - userId: mockIsomerUserId, }) } @@ -504,7 +494,6 @@ describe("Users Router", () => { const actual = await request(app).post("/mobile/verifyOtp").send({ mobile: mockValidNumber, otp, - userId: mockIsomerUserId, }) const updatedUser = await User.findOne({ where: { @@ -531,13 +520,34 @@ describe("Users Router", () => { const actual = await request(app).post("/mobile/verifyOtp").send({ mobile: mockValidNumber, otp: wrongOtp, - userId: mockIsomerUserId, }) // Assert expect(actual.statusCode).toBe(expected) }) + it("should return 400 when the request body format is wrong", async () => { + // Arrange + const expected = 400 + const otp = "123456" + mockAxios.post.mockResolvedValueOnce(200) + await User.create({ id: mockIsomerUserId }) + await request(app).post("/mobile/otp").send({ + mobile: mockValidNumber, + }) + + // Act + const actual = await request(app) + .post("/mobile/verifyOtp") + .send({ + mobile: [mockValidNumber, "98765432"], + otp, + }) + + // Assert + expect(actual.statusCode).toBe(expected) + }) + it("should return 400 when there is no otp", async () => { // Arrange const expected = 400 @@ -551,7 +561,6 @@ describe("Users Router", () => { const actual = await request(app).post("/mobile/verifyOtp").send({ mobile: mockValidNumber, otp: "", - userId: mockIsomerUserId, }) // Assert @@ -571,7 +580,6 @@ describe("Users Router", () => { const actual = await request(app).post("/mobile/verifyOtp").send({ mobile: mockValidNumber, otp: undefined, - userId: mockIsomerUserId, }) // Assert @@ -596,7 +604,6 @@ describe("Users Router", () => { const actual = await request(app).post("/mobile/verifyOtp").send({ mobile: mockValidNumber, otp, - userId: mockIsomerUserId, }) const oldOtp = otp @@ -612,7 +619,6 @@ describe("Users Router", () => { const newActual = await request(app).post("/mobile/verifyOtp").send({ mobile: mockValidNumber, otp: oldOtp, - userId: mockIsomerUserId, }) // Assert @@ -634,7 +640,6 @@ describe("Users Router", () => { const actual = await request(app).post("/mobile/verifyOtp").send({ mobile: mockValidNumber, otp: mockInvalidOtp, - userId: mockIsomerUserId, }) const otpEntry = await Otp.findOne({ where: { mobileNumber: mockValidNumber }, @@ -645,12 +650,10 @@ describe("Users Router", () => { if (i <= maxNumOfOtpAttempts) { expect(otpEntry?.attempts).toBe(i) - expect(actual.body.error.message).toBe("OTP is not valid") + expect(actual.body.message).toBe("OTP is not valid") } else { expect(otpEntry?.attempts).toBe(maxNumOfOtpAttempts) - expect(actual.body.error.message).toBe( - "Max number of attempts reached" - ) + expect(actual.body.message).toBe("Max number of attempts reached") } } }) @@ -668,7 +671,6 @@ describe("Users Router", () => { await request(app).post("/mobile/verifyOtp").send({ mobile: mockValidNumber, otp: mockInvalidOtp, - userId: mockIsomerUserId, }) } diff --git a/src/routes/v2/auth.js b/src/routes/v2/auth.js index d439766e3..e404ae488 100644 --- a/src/routes/v2/auth.js +++ b/src/routes/v2/auth.js @@ -11,6 +11,11 @@ const { attachReadRouteHandlerWrapper } = require("@middleware/routeHandler") const FRONTEND_URL = config.get("app.frontendUrl") const { isSecure } = require("@utils/auth-utils") +const { + EmailSchema, + VerifyRequestSchema, +} = require("@root/validators/RequestSchema") + const CSRF_TOKEN_EXPIRY_MS = 600000 const CSRF_COOKIE_NAME = "isomer-csrf" const COOKIE_NAME = "isomercms" @@ -76,6 +81,11 @@ class AuthRouter { async login(req, res) { const { email: rawEmail } = req.body + const { error } = EmailSchema.validate(rawEmail) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) const email = rawEmail.toLowerCase() try { await this.authService.sendOtp(email) @@ -90,8 +100,13 @@ class AuthRouter { async verify(req, res) { const { email: rawEmail, otp } = req.body + const { error } = VerifyRequestSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) const email = rawEmail.toLowerCase() - const userInfo = await this.authService.verifyOtp({ email, otp }) + const userInfo = (await this.authService.verifyOtp({ email, otp })).value Object.assign(req.session, { userInfo }) logger.info(`User ${userInfo.email} successfully logged in`) return res.sendStatus(200) diff --git a/src/routes/v2/authenticated/__tests__/collaborators.spec.ts b/src/routes/v2/authenticated/__tests__/collaborators.spec.ts index ed53caf79..c89dd75ef 100644 --- a/src/routes/v2/authenticated/__tests__/collaborators.spec.ts +++ b/src/routes/v2/authenticated/__tests__/collaborators.spec.ts @@ -13,7 +13,7 @@ import { AuthorizationMiddleware } from "@root/middleware/authorization" import CollaboratorsService from "@root/services/identity/CollaboratorsService" describe("Collaborator Router", () => { - const MOCK_EMAIL = "mockemail" + const MOCK_EMAIL = "mockemail@email.com" const MOCK_ACK_VALUE = true const mockCollaboratorsService = { create: jest.fn(), diff --git a/src/routes/v2/authenticated/__tests__/review.spec.ts b/src/routes/v2/authenticated/__tests__/review.spec.ts index dbfcd1da7..fa55cedbe 100644 --- a/src/routes/v2/authenticated/__tests__/review.spec.ts +++ b/src/routes/v2/authenticated/__tests__/review.spec.ts @@ -182,6 +182,7 @@ describe("Review Requests Router", () => { }) describe("createReviewRequest", () => { + const mockEmail = "mock@test.gov.sg" it("should return 200 with the pull request number of the created review request", async () => { // Arrange const mockPullRequestNumber = 1 @@ -235,7 +236,11 @@ describe("Review Requests Router", () => { // Act const response = await request(app) .post("/mockSite/review/request") - .send({}) + .send({ + reviewers: [mockEmail], + title: "Fake title", + description: "", + }) // Assert expect(response.status).toEqual(404) @@ -257,7 +262,11 @@ describe("Review Requests Router", () => { // Act const response = await request(app) .post("/mockSite/review/request") - .send({}) + .send({ + reviewers: [mockEmail], + title: "Fake title", + description: "", + }) // Assert expect(response.status).toEqual(404) @@ -614,10 +623,10 @@ describe("Review Requests Router", () => { }) describe("updateReviewRequest", () => { + const mockReviewer = "reviewer@test.gov.sg" it("should return 200 with the updated review request", async () => { // Arrange const mockReviewRequest = { requestor: { email: MOCK_USER_EMAIL_ONE } } - const mockReviewer = "reviewer@test.gov.sg" mockReviewRequestService.getReviewRequest.mockResolvedValueOnce( mockReviewRequest @@ -659,7 +668,11 @@ describe("Review Requests Router", () => { ) // Act - const response = await request(app).post(`/mockSite/review/12345`) + const response = await request(app) + .post(`/mockSite/review/12345`) + .send({ + reviewers: [mockReviewer], + }) // Assert expect(response.status).toEqual(404) @@ -679,7 +692,11 @@ describe("Review Requests Router", () => { ) // Act - const response = await request(app).post(`/mockSite/review/12345`) + const response = await request(app) + .post(`/mockSite/review/12345`) + .send({ + reviewers: [mockReviewer], + }) // Assert expect(response.status).toEqual(404) @@ -700,7 +717,11 @@ describe("Review Requests Router", () => { ) // Act - const response = await request(app).post(`/mockSite/review/12345`) + const response = await request(app) + .post(`/mockSite/review/12345`) + .send({ + reviewers: [mockReviewer], + }) // Assert expect(response.status).toEqual(403) @@ -1274,7 +1295,7 @@ describe("Review Requests Router", () => { expect(mockNotificationsService.create).not.toHaveBeenCalled() }) - it("should return 404 if the user is not the requestor of the review request", async () => { + it("should return 403 if the user is not the requestor of the review request", async () => { // Arrange const mockReviewRequest = { requestor: { email: "other@test.gov.sg" } } @@ -1286,7 +1307,7 @@ describe("Review Requests Router", () => { const response = await request(app).delete(`/mockSite/review/12345`) // Assert - expect(response.status).toEqual(404) + expect(response.status).toEqual(403) expect(mockSitesService.getBySiteName).toHaveBeenCalledTimes(1) expect(mockReviewRequestService.getReviewRequest).toHaveBeenCalledTimes(1) expect(mockReviewRequestService.closeReviewRequest).not.toHaveBeenCalled() diff --git a/src/routes/v2/authenticated/collaborators.ts b/src/routes/v2/authenticated/collaborators.ts index b507f8ff7..82ffaa9dd 100644 --- a/src/routes/v2/authenticated/collaborators.ts +++ b/src/routes/v2/authenticated/collaborators.ts @@ -13,6 +13,7 @@ import { attachSiteHandler } from "@root/middleware" import { RouteCheckerMiddleware } from "@root/middleware/routeChecker" import { RequestHandler } from "@root/types" import { UserDto } from "@root/types/dto/review" +import { CreateCollaboratorRequestSchema } from "@root/validators/RequestSchema" import CollaboratorsService from "@services/identity/CollaboratorsService" interface CollaboratorsRouterProps { @@ -43,6 +44,11 @@ export class CollaboratorsRouter { { userWithSiteSessionData: UserWithSiteSessionData } > = async (req, res) => { const { email, acknowledge = false } = req.body + const { error } = CreateCollaboratorRequestSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) const { siteName } = req.params const { userWithSiteSessionData } = res.locals logger.info( diff --git a/src/routes/v2/authenticated/metrics.ts b/src/routes/v2/authenticated/metrics.ts index 85ed99b34..65c49f620 100644 --- a/src/routes/v2/authenticated/metrics.ts +++ b/src/routes/v2/authenticated/metrics.ts @@ -5,6 +5,7 @@ import { ISOMER_ADMIN_EMAIL } from "@root/constants" import { mailer } from "@root/services/utilServices/MailClient" import { RequestHandler } from "@root/types" import { FeedbackDto } from "@root/types/dto/feedback" +import { CollateUserFeedbackRequestSchema } from "@root/validators/RequestSchema" import { statsService } from "@services/infra/StatsService" const getFeedbackHtml = ({ @@ -30,6 +31,11 @@ export class MetricsRouter { { userWithSiteSessionData: UserWithSiteSessionData } > = (req, res) => { const { userType } = req.body + const { error } = CollateUserFeedbackRequestSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) mailer.sendMail( ISOMER_ADMIN_EMAIL, "[METRICS] User feedback", diff --git a/src/routes/v2/authenticated/review.ts b/src/routes/v2/authenticated/review.ts index aa8f4250c..e4e448954 100644 --- a/src/routes/v2/authenticated/review.ts +++ b/src/routes/v2/authenticated/review.ts @@ -31,6 +31,11 @@ import { BlobDiffDto, EditedItemDto, } from "@root/types/dto/review" +import { + CreateCommentSchema, + CreateReviewRequestSchema, + UpdateReviewRequestSchema, +} from "@root/validators/RequestSchema" import ReviewRequestService from "@services/review/ReviewRequestService" // eslint-disable-next-line import/prefer-default-export export class ReviewsRouter { @@ -147,6 +152,11 @@ export class ReviewsRouter { unknown, { userWithSiteSessionData: UserWithSiteSessionData } > = async (req, res) => { + const { error } = CreateReviewRequestSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) // Step 1: Check that the site exists const { siteName } = req.params const site = await this.sitesService.getBySiteName(siteName) @@ -566,6 +576,11 @@ export class ReviewsRouter { unknown, { userWithSiteSessionData: UserWithSiteSessionData } > = async (req, res) => { + const { error } = UpdateReviewRequestSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) // Step 1: Check that the site exists const { siteName, requestId } = req.params const { userWithSiteSessionData } = res.locals @@ -959,6 +974,11 @@ export class ReviewsRouter { unknown, { userWithSiteSessionData: UserWithSiteSessionData } > = async (req, res) => { + const { error } = CreateCommentSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) const { siteName, requestId } = req.params const { message } = req.body const { userWithSiteSessionData } = res.locals @@ -1152,7 +1172,7 @@ export class ReviewsRouter { requestId, }, }) - return res.status(404).json({ + return res.status(403).json({ message: "Only the requestor can close the Review Request!", }) } diff --git a/src/routes/v2/authenticated/sites.ts b/src/routes/v2/authenticated/sites.ts index 26c83a7ac..8aeafc5df 100644 --- a/src/routes/v2/authenticated/sites.ts +++ b/src/routes/v2/authenticated/sites.ts @@ -21,6 +21,10 @@ import { RepositoryData } from "@root/types/repoInfo" import { BrokenLinkErrorDto } from "@root/types/siteChecker" import { SiteInfo, SiteLaunchDto } from "@root/types/siteInfo" import { StagingBuildStatus } from "@root/types/stagingBuildStatus" +import { + GetPreviewInfoSchema, + LaunchSiteSchema, +} from "@root/validators/RequestSchema" import type SitesService from "@services/identity/SitesService" type SitesRouterProps = { @@ -137,6 +141,11 @@ export class SitesRouter { never, { userWithSiteSessionData: UserWithSiteSessionData } > = (req, res) => { + const { error } = LaunchSiteSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) const { userWithSiteSessionData } = res.locals const { email } = userWithSiteSessionData // Note, launching the site is an async operation, @@ -170,13 +179,19 @@ export class SitesRouter { getPreviewInfo: RequestHandler< { siteName: string }, PreviewInfo[] | ResponseErrorBody, - { sites: string[]; email: string }, + { sites: string[] }, never, { userSessionData: UserSessionData } - > = async (req, res) => - this.sitesService + > = async (req, res) => { + const { error } = GetPreviewInfoSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) + return this.sitesService .getSitesPreview(req.body.sites, res.locals.userSessionData) .then((previews) => res.status(200).json(previews)) + } getUserStagingSiteBuildStatus: RequestHandler< { siteName: string }, diff --git a/src/routes/v2/authenticated/users.ts b/src/routes/v2/authenticated/users.ts index 2a8afafe0..a77974edb 100644 --- a/src/routes/v2/authenticated/users.ts +++ b/src/routes/v2/authenticated/users.ts @@ -1,5 +1,6 @@ import autoBind from "auto-bind" import express from "express" +import { ResultAsync } from "neverthrow" import validator from "validator" import logger from "@logger/logger" @@ -10,7 +11,12 @@ import { attachReadRouteHandlerWrapper } from "@middleware/routeHandler" import UserSessionData from "@classes/UserSessionData" +import DatabaseError from "@root/errors/DatabaseError" import { isError, RequestHandler } from "@root/types" +import { + VerifyEmailOtpSchema, + VerifyMobileNumberOtpSchema, +} from "@root/validators/RequestSchema" import UsersService from "@services/identity/UsersService" interface UsersRouterProps { @@ -66,17 +72,39 @@ export class UsersRouter { { userSessionData: UserSessionData } > = async (req, res) => { const { email, otp } = req.body + const { error } = VerifyEmailOtpSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) const { userSessionData } = res.locals const userId = userSessionData.isomerUserId const parsedEmail = email.toLowerCase() - const isOtpValid = await this.usersService.verifyEmailOtp(parsedEmail, otp) - if (!isOtpValid) { - throw new BadRequestError("Invalid OTP") - } - - await this.usersService.updateUserByIsomerId(userId, { email: parsedEmail }) - res.sendStatus(200) + return this.usersService + .verifyEmailOtp(parsedEmail, otp) + .andThen(() => + ResultAsync.fromPromise( + this.usersService.updateUserByIsomerId(userId, { + email: parsedEmail, + }), + (error) => { + logger.error( + `Error updating user email by Isomer ID: ${JSON.stringify(error)}` + ) + return new DatabaseError( + "An error occurred when updating the database" + ) + } + ) + ) + .map(() => res.sendStatus(200)) + .mapErr((error) => { + if (error instanceof BadRequestError) { + return res.status(400).json({ message: error.message }) + } + return res.status(500).json({ message: error.message }) + }) } sendMobileNumberOtp: RequestHandler< @@ -101,18 +129,43 @@ export class UsersRouter { { userSessionData: UserSessionData } > = async (req, res) => { const { mobile, otp } = req.body + const { error } = VerifyMobileNumberOtpSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) + if (!mobile || !validator.isMobilePhone(mobile)) { + throw new BadRequestError("Please provide a valid mobile number") + } const { userSessionData } = res.locals const userId = userSessionData.isomerUserId - const isOtpValid = await this.usersService.verifyMobileOtp(mobile, otp) - if (!isOtpValid) { - throw new BadRequestError("Invalid OTP") - } - - await this.usersService.updateUserByIsomerId(userId, { - contactNumber: mobile, - }) - return res.sendStatus(200) + return this.usersService + .verifyMobileOtp(mobile, otp) + .andThen(() => + ResultAsync.fromPromise( + this.usersService.updateUserByIsomerId(userId, { + contactNumber: mobile, + }), + (error) => { + logger.error( + `Error updating user contact number by Isomer ID: ${JSON.stringify( + error + )}` + ) + return new DatabaseError( + "An error occurred when updating the database" + ) + } + ) + ) + .map(() => res.sendStatus(200)) + .mapErr((error) => { + if (error instanceof BadRequestError) { + return res.status(400).json({ message: error.message }) + } + return res.status(500).json({ message: error.message }) + }) } getRouter() { diff --git a/src/routes/v2/authenticatedSites/repoManagement.ts b/src/routes/v2/authenticatedSites/repoManagement.ts index 7d2a4d539..294fb5a85 100644 --- a/src/routes/v2/authenticatedSites/repoManagement.ts +++ b/src/routes/v2/authenticatedSites/repoManagement.ts @@ -11,6 +11,7 @@ import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" import GitFileSystemError from "@root/errors/GitFileSystemError" import { attachSiteHandler } from "@root/middleware" import type { RequestHandler } from "@root/types" +import { ResetRepoSchema } from "@root/validators/RequestSchema" import RepoManagementService from "@services/admin/RepoManagementService" interface RepoManagementRouterProps { @@ -41,6 +42,11 @@ export class RepoManagementRouter { > = async (req, res) => { const { userWithSiteSessionData } = res.locals const { branchName, commitSha } = req.body + const { error } = ResetRepoSchema.validate(req.body) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) return this.repoManagementService .resetRepo(userWithSiteSessionData, branchName, commitSha) diff --git a/src/routes/v2/sgidAuth.ts b/src/routes/v2/sgidAuth.ts index 4ff12c64b..a1766794b 100644 --- a/src/routes/v2/sgidAuth.ts +++ b/src/routes/v2/sgidAuth.ts @@ -20,6 +20,7 @@ import { RequestHandler } from "@root/types" import { ResponseErrorBody } from "@root/types/dto/error" import { isPublicOfficerData, PublicOfficerData } from "@root/types/sgid" import { isSecure } from "@root/utils/auth-utils" +import { EmailSchema } from "@root/validators/RequestSchema" import UsersService from "@services/identity/UsersService" import SgidAuthService from "@services/utilServices/SgidAuthService" @@ -167,6 +168,11 @@ export class SgidAuthRouter { never > = async (req, res) => { const { email } = req.body + const { error } = EmailSchema.validate(email) + if (error) + return res.status(400).json({ + message: `Invalid request format: ${error.message}`, + }) const token = req.cookies[SGID_MULTIUSER_COOKIE_NAME] res.clearCookie(SGID_MULTIUSER_COOKIE_NAME, { path: "/" }) diff --git a/src/server.js b/src/server.js index 0eb1a0aae..894dae598 100644 --- a/src/server.js +++ b/src/server.js @@ -90,8 +90,8 @@ import CollaboratorsService from "./services/identity/CollaboratorsService" import LaunchClient from "./services/identity/LaunchClient" import LaunchesService from "./services/identity/LaunchesService" import DynamoDBDocClient from "./services/infra/DynamoDBClient" -import ReviewCommentService from "./services/review/ReviewCommentService" import RepoCheckerService from "./services/review/RepoCheckerService" +import ReviewCommentService from "./services/review/ReviewCommentService" import { rateLimiter } from "./services/utilServices/RateLimiter" import SgidAuthService from "./services/utilServices/SgidAuthService" import { isSecure } from "./utils/auth-utils" @@ -99,7 +99,7 @@ import { setBrowserPolyfills } from "./utils/growthbook-utils" const path = require("path") -const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiry") +const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiryInMs") const sequelize = initSequelize([ Site, diff --git a/src/services/identity/UsersService.ts b/src/services/identity/UsersService.ts index 228f46f40..7e621291b 100644 --- a/src/services/identity/UsersService.ts +++ b/src/services/identity/UsersService.ts @@ -1,4 +1,5 @@ -import { Op, ModelStatic } from "sequelize" +import { ResultAsync, errAsync, okAsync } from "neverthrow" +import { Op, ModelStatic, Transaction } from "sequelize" import { Sequelize } from "sequelize-typescript" import { RequireAtLeastOne } from "type-fest" @@ -6,6 +7,8 @@ import { config } from "@config/config" import { Otp, Repo, Site, User, Whitelist, SiteMember } from "@database/models" import { BadRequestError } from "@root/errors/BadRequestError" +import DatabaseError from "@root/errors/DatabaseError" +import logger from "@root/logger/logger" import { milliSecondsToMinutes } from "@root/utils/time-utils" import SmsClient from "@services/identity/SmsClient" import MailClient from "@services/utilServices/MailClient" @@ -230,56 +233,201 @@ class UsersService { await this.smsClient.sendSms(mobileNumber, message) } - private async verifyOtp(otpEntry: Otp | null, otp: string) { + private verifyOtp( + otpEntry: Otp | null, + otp: string, + transaction: Transaction + ) { // TODO: Change all the following to use AuthError after FE fix - if (!otp || otp === "") { - throw new BadRequestError("Empty OTP provided") - } - - if (!otpEntry) { - throw new BadRequestError("OTP not found") - } - - if (otpEntry.attempts >= MAX_NUM_OTP_ATTEMPTS) { - throw new BadRequestError("Max number of attempts reached") - } - - if (!otpEntry?.hashedOtp) { - await otpEntry.destroy() - throw new BadRequestError("Hashed OTP not found") - } + return okAsync(otp) + .andThen((otp) => { + if (!otp) { + return errAsync(new BadRequestError("Empty OTP provided")) + } - // increment attempts - await otpEntry.update({ attempts: otpEntry.attempts + 1 }) - - const isValidOtp = await this.otpService.verifyOtp(otp, otpEntry.hashedOtp) - if (!isValidOtp) { - throw new BadRequestError("OTP is not valid") - } + return okAsync(otp) + }) + .andThen(() => { + if (!otpEntry) { + return errAsync(new BadRequestError("OTP not found")) + } - if (isValidOtp && otpEntry.expiresAt < new Date()) { - await otpEntry.destroy() - throw new BadRequestError("OTP has expired") - } + return okAsync(otpEntry) + }) + .andThen((otpDbEntry) => { + if (otpDbEntry.attempts >= MAX_NUM_OTP_ATTEMPTS) { + return errAsync(new BadRequestError("Max number of attempts reached")) + } - // destroy otp before returning true since otp has been "used" - await otpEntry.destroy() - return true + return okAsync(otpDbEntry) + }) + .andThen((otpDbEntry) => { + if (!otpDbEntry.hashedOtp) { + return ResultAsync.fromPromise( + otpDbEntry.destroy({ transaction }), + (error) => { + logger.error( + `Error destroying OTP entry: ${JSON.stringify(error)}` + ) + + return new DatabaseError("Error destroying OTP entry in database") + } + ).andThen(() => errAsync(new BadRequestError("Hashed OTP not found"))) + } + + return okAsync(otpDbEntry) + }) + .andThen((otpDbEntry) => + // increment attempts + ResultAsync.fromPromise( + this.otpRepository.increment("attempts", { + where: { id: otpDbEntry.id }, + transaction, + }), + (error) => { + logger.error( + `Error incrementing OTP attempts: ${JSON.stringify(error)}` + ) + + return new DatabaseError("Error incrementing OTP attempts") + } + ).map(() => otpDbEntry) + ) + .andThen((otpDbEntry) => + ResultAsync.combine([ + okAsync(otpDbEntry), + ResultAsync.fromPromise( + this.otpService.verifyOtp(otp, otpDbEntry.hashedOtp), + (error) => { + logger.error(`Error verifying OTP: ${JSON.stringify(error)}`) + + return new BadRequestError("Error verifying OTP") + } + ), + ]) + ) + .andThen(([otpDbEntry, isValidOtp]) => { + if (!isValidOtp) { + return errAsync(new BadRequestError("OTP is not valid")) + } + + if (isValidOtp && otpDbEntry.expiresAt < new Date()) { + return ResultAsync.fromPromise( + otpDbEntry.destroy({ transaction }), + (error) => { + logger.error( + `Error destroying OTP entry: ${JSON.stringify(error)}` + ) + + return new DatabaseError("Error destroying OTP entry in database") + } + ).andThen(() => errAsync(new BadRequestError("OTP has expired"))) + } + + return okAsync(otpDbEntry) + }) + .andThen((otpDbEntry) => + // destroy otp before returning true since otp has been "used" + ResultAsync.fromPromise( + otpDbEntry.destroy({ transaction }), + (error) => { + logger.error(`Error destroying OTP entry: ${JSON.stringify(error)}`) + + return new DatabaseError("Error destroying OTP entry in database") + } + ) + .andThen(() => + ResultAsync.fromPromise(transaction.commit(), (txError) => { + logger.error( + `Error committing transaction: ${JSON.stringify(txError)}` + ) + return new DatabaseError("Error committing transaction") + }) + ) + .map(() => true) + ) + .orElse((error) => + ResultAsync.fromPromise(transaction.commit(), (txError) => { + logger.error( + `Error committing transaction: ${JSON.stringify(txError)}` + ) + return new DatabaseError("Error committing transaction") + }).andThen(() => errAsync(error)) + ) } - async verifyEmailOtp(email: string, otp: string) { + verifyEmailOtp(email: string, otp: string) { const parsedEmail = email.toLowerCase() - const otpEntry = await this.otpRepository.findOne({ - where: { email: parsedEmail }, + + return ResultAsync.fromPromise(this.sequelize.transaction(), (error) => { + logger.error( + `Error starting database transaction: ${JSON.stringify(error)}` + ) + + return new BadRequestError("Error starting database transaction") }) - return this.verifyOtp(otpEntry, otp) + .andThen((transaction) => + ResultAsync.combine([ + ResultAsync.fromPromise( + this.otpRepository.findOne({ + where: { email: parsedEmail }, + lock: true, + transaction, + }), + (error) => { + logger.error( + `Error finding OTP entry when verifying email OTP: ${JSON.stringify( + error + )}` + ) + + return new BadRequestError( + "Error finding OTP entry when verifying email OTP" + ) + } + ), + okAsync(transaction), + ]) + ) + .andThen(([otpEntry, transaction]) => + this.verifyOtp(otpEntry, otp, transaction) + ) } - async verifyMobileOtp(mobileNumber: string, otp: string) { - const otpEntry = await this.otpRepository.findOne({ - where: { mobileNumber }, + verifyMobileOtp(mobileNumber: string, otp: string) { + return ResultAsync.fromPromise(this.sequelize.transaction(), (error) => { + logger.error( + `Error starting database transaction: ${JSON.stringify(error)}` + ) + + return new BadRequestError("Error starting database transaction") }) - return this.verifyOtp(otpEntry, otp) + .andThen((transaction) => + ResultAsync.combine([ + ResultAsync.fromPromise( + this.otpRepository.findOne({ + where: { mobileNumber }, + lock: true, + transaction, + }), + (error) => { + logger.error( + `Error finding OTP entry when verifying mobile OTP: ${JSON.stringify( + error + )}` + ) + + return new BadRequestError( + "Error finding OTP entry when verifying mobile OTP" + ) + } + ), + okAsync(transaction), + ]) + ) + .andThen(([otpEntry, transaction]) => + this.verifyOtp(otpEntry, otp, transaction) + ) } private getOtpExpiry() { diff --git a/src/services/utilServices/AuthService.js b/src/services/utilServices/AuthService.js index dcb4be2d2..fde329584 100644 --- a/src/services/utilServices/AuthService.js +++ b/src/services/utilServices/AuthService.js @@ -1,4 +1,5 @@ const axios = require("axios") +const { ResultAsync } = require("neverthrow") const queryString = require("query-string") const uuid = require("uuid/v4") @@ -17,6 +18,7 @@ const { E2E_TEST_EMAIL, } = require("@root/constants") const { BadRequestError } = require("@root/errors/BadRequestError") +const { DatabaseError } = require("@root/errors/DatabaseError") const logger = require("@root/logger/logger").default const { isError } = require("@root/types") @@ -124,18 +126,28 @@ class AuthService { } async verifyOtp({ email, otp }) { - const isOtpValid = await this.usersService.verifyEmailOtp(email, otp) - - if (!isOtpValid) { - throw new BadRequestError("You have entered an invalid OTP.") - } - // Create user if does not exists. Set last logged in to current time. - const user = await this.usersService.loginWithEmail(email) - const userInfo = { - isomerUserId: user.id, - email: user.email, - } - return userInfo + return this.usersService + .verifyEmailOtp(email, otp) + .andThen(() => + ResultAsync.fromPromise( + this.usersService.loginWithEmail(email), + (error) => { + logger.error( + `Error logging user in via email: ${JSON.stringify(error)}` + ) + return new DatabaseError( + "An error occurred when logging user in via email" + ) + } + ) + ) + .map((user) => ({ + isomerUserId: user.id, + email: user.email, + })) + .mapErr((error) => { + throw error + }) } async getUserInfo(sessionData) { diff --git a/src/services/utilServices/RateLimiter.ts b/src/services/utilServices/RateLimiter.ts index e8caf981a..60ae16168 100644 --- a/src/services/utilServices/RateLimiter.ts +++ b/src/services/utilServices/RateLimiter.ts @@ -1,6 +1,7 @@ import rateLimit from "express-rate-limit" -import { config } from "@config/config" +import { BaseIsomerError } from "@root/errors/BaseError" +import { isSecure } from "@root/utils/auth-utils" const DEFAULT_AUTH_TOKEN_EXPIRY_MILLISECONDS = 900000 @@ -13,8 +14,21 @@ const DEFAULT_AUTH_TOKEN_EXPIRY_MILLISECONDS = 900000 // but not on the other, leading to inconsistent behaviour. // eslint-disable-next-line import/prefer-default-export export const rateLimiter = rateLimit({ - windowMs: config.get("auth.tokenExpiry") / (1000 * 60), + windowMs: DEFAULT_AUTH_TOKEN_EXPIRY_MILLISECONDS, max: 100, // Limit each IP to 100 requests per `window` (here, per 15 minutes) standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers legacyHeaders: false, // Disable the `X-RateLimit-*` headers + // We know that this key exists in a secure env (Cloudflare) + // See https://developers.cloudflare.com/fundamentals/reference/http-request-headers/#cf-connecting-ip + keyGenerator: (req) => { + const userIp = isSecure ? req.get("cf-connecting-ip") : req.ip + if (!userIp) { + // This should never happen, but if it does, we should know about it + throw new BaseIsomerError({ + status: 500, + message: "No user IP found in the request", + }) + } + return userIp + }, }) diff --git a/src/services/utilServices/__tests__/AuthService.spec.js b/src/services/utilServices/__tests__/AuthService.spec.js index 57edc2ee5..a4a5c570b 100644 --- a/src/services/utilServices/__tests__/AuthService.spec.js +++ b/src/services/utilServices/__tests__/AuthService.spec.js @@ -1,5 +1,7 @@ const { validateStatus } = require("@utils/axios-utils") +const { okAsync, errAsync } = require("neverthrow") + jest.mock("axios", () => ({ get: jest.fn(), post: jest.fn(), @@ -148,12 +150,18 @@ describe("Auth Service", () => { describe("verifyOtp", () => { const mockOtp = "123456" it("should be able to verify otp, login, and return token if correct", async () => { - mockUsersService.verifyEmailOtp.mockImplementationOnce(() => true) + mockUsersService.verifyEmailOtp.mockImplementationOnce(() => + okAsync(true) + ) + mockUsersService.loginWithEmail.mockResolvedValueOnce({ + id: mockIsomerUserId, + email: mockEmail, + }) jwtUtils.signToken.mockImplementationOnce(() => signedEmailToken) - await expect( - service.verifyOtp({ email: mockEmail, otp: mockOtp }) - ).resolves.toEqual(signedEmailToken) + const result = await service.verifyOtp({ email: mockEmail, otp: mockOtp }) + + expect(result._unsafeUnwrap()).toEqual(signedEmailToken) expect(mockUsersService.verifyEmailOtp).toHaveBeenCalledWith( mockEmail, mockOtp @@ -162,7 +170,9 @@ describe("Auth Service", () => { }) it("should throw an error if otp is incorrect", async () => { - mockUsersService.verifyEmailOtp.mockImplementationOnce(() => false) + mockUsersService.verifyEmailOtp.mockImplementationOnce(() => + errAsync(new BadRequestError("Invalid OTP")) + ) await expect( service.verifyOtp({ email: mockEmail, otp: mockOtp }) diff --git a/src/types/dto/feedback.ts b/src/types/dto/feedback.ts index df62abc91..79fee3bf0 100644 --- a/src/types/dto/feedback.ts +++ b/src/types/dto/feedback.ts @@ -2,7 +2,7 @@ import { UserType } from "../user" export interface FeedbackDto { rating: number - feedback: string + feedback?: string email: string userType: UserType } diff --git a/src/utils/jwt-utils.js b/src/utils/jwt-utils.js index f1107a56c..8ea4d14b1 100644 --- a/src/utils/jwt-utils.js +++ b/src/utils/jwt-utils.js @@ -7,7 +7,7 @@ const { config } = require("@config/config") const JWT_SECRET = config.get("auth.jwtSecret") const ENCRYPTION_SECRET = config.get("auth.encryptionSecret") -const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiry").toString() +const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiryInMs").toString() const jwtUtil = { decodeToken: _.wrap(jwt.decode, (decode, token) => decode(token)), diff --git a/src/validators/RequestSchema.js b/src/validators/RequestSchema.js index 9a02c5b89..ce2726ce2 100644 --- a/src/validators/RequestSchema.js +++ b/src/validators/RequestSchema.js @@ -6,6 +6,9 @@ const { MAX_TEXTCARDS_CARDS, MAX_INFOCOLS_BOXES, } = require("@root/constants") +const { UserTypes } = require("@root/types/user") + +const EmailSchema = Joi.string().email().required() const FileSchema = Joi.object().keys({ name: Joi.string().required(), @@ -435,7 +438,63 @@ const UpdateRepoPasswordRequestSchema = Joi.object().keys({ enablePassword: Joi.boolean().required(), }) +const VerifyRequestSchema = Joi.object().keys({ + email: EmailSchema, + otp: Joi.string().required(), +}) + +const CreateCollaboratorRequestSchema = Joi.object().keys({ + email: EmailSchema, + acknowledge: Joi.boolean().optional(), +}) + +const CollateUserFeedbackRequestSchema = Joi.object().keys({ + userType: Joi.string().valid(...Object.values(UserTypes)), + rating: Joi.number().required(), + feedback: Joi.string().optional(), + email: Joi.string().required(), +}) + +const CreateReviewRequestSchema = Joi.object().keys({ + reviewers: Joi.array().items(Joi.string()).required(), + title: Joi.string().required(), + description: Joi.string().allow(""), +}) + +const UpdateReviewRequestSchema = Joi.object().keys({ + reviewers: Joi.array().items(Joi.string()).required(), +}) + +const CreateCommentSchema = Joi.object().keys({ + message: Joi.string().required(), +}) + +const LaunchSiteSchema = Joi.object().keys({ + siteUrl: Joi.string().required(), + useWwwSubdomain: Joi.boolean().required(), +}) + +const GetPreviewInfoSchema = Joi.object().keys({ + sites: Joi.array().items(Joi.string()).required(), +}) + +const VerifyEmailOtpSchema = Joi.object().keys({ + email: EmailSchema, + otp: Joi.string().length(6).required(), +}) + +const VerifyMobileNumberOtpSchema = Joi.object().keys({ + mobile: Joi.string().required(), + otp: Joi.string().length(6).required(), +}) + +const ResetRepoSchema = Joi.object().keys({ + branchName: Joi.string().required(), + commitSha: Joi.string().required(), +}) + module.exports = { + EmailSchema, UpdateContactUsSchema, UpdateHomepageSchema, CreatePageRequestSchema, @@ -461,4 +520,15 @@ module.exports = { UpdateNavigationRequestSchema, UpdateSettingsRequestSchema, UpdateRepoPasswordRequestSchema, + VerifyRequestSchema, + CreateCollaboratorRequestSchema, + CollateUserFeedbackRequestSchema, + CreateReviewRequestSchema, + UpdateReviewRequestSchema, + CreateCommentSchema, + LaunchSiteSchema, + GetPreviewInfoSchema, + VerifyEmailOtpSchema, + VerifyMobileNumberOtpSchema, + ResetRepoSchema, }