From 9a396007f904828b8c38eb9ea88c674584d0501a Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Tue, 4 Oct 2022 15:33:12 +0800 Subject: [PATCH] feat: collaborators (#510) * build(devdeps): add lodash types * fix(model): rectify db model definitions * refactor: add message param to ForbiddenError * feat: add CollaboratorsService * test: add tests for CollaboratorsService * feat: use CollaboratorService in authorization middleware * test: add tests for authorization middleware * feat: add CollaboratorsRouter * test: add tests for CollaboratorsRouter * feat(db-migration): change site_members role enum in the database * feat: modify authzMiddlewareService tests * fix: error in mock collaborators fixture --- package-lock.json | 46 +- package.json | 1 + src/constants/constants.ts | 5 + .../20220811070630-change-role-enum.js | 59 +++ src/database/models/Site.ts | 3 +- src/database/models/SiteMember.ts | 6 +- src/database/models/User.ts | 2 +- src/errors/ForbiddenError.js | 4 +- src/fixtures/identity.ts | 120 +++++ src/middleware/__tests__/authorization.ts | 111 ++++ src/middleware/authorization.ts | 40 +- src/middleware/index.ts | 4 + src/routes/v1/authenticatedSites/index.js | 2 +- .../__tests__/collaborators.spec.ts | 179 +++++++ src/routes/v2/authenticated/collaborators.ts | 135 +++++ src/routes/v2/authenticated/index.js | 13 +- src/routes/v2/authenticatedSites/index.js | 2 +- src/server.js | 12 + src/services/identity/CollaboratorsService.ts | 240 +++++++++ .../__tests__/CollaboratorsService.spec.ts | 476 ++++++++++++++++++ .../AuthorizationMiddlewareService.ts | 68 ++- .../AuthorizationMiddlewareService.spec.ts | 67 ++- 22 files changed, 1522 insertions(+), 73 deletions(-) create mode 100644 src/database/migrations/20220811070630-change-role-enum.js create mode 100644 src/middleware/__tests__/authorization.ts create mode 100644 src/routes/v2/authenticated/__tests__/collaborators.spec.ts create mode 100644 src/routes/v2/authenticated/collaborators.ts create mode 100644 src/services/identity/CollaboratorsService.ts create mode 100644 src/services/identity/__tests__/CollaboratorsService.spec.ts diff --git a/package-lock.json b/package-lock.json index a2b988d4f..77594ce71 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4744,6 +4744,12 @@ "integrity": "sha1-7ihweulOEdK4J7y+UnC86n8+ce4=", "dev": true }, + "@types/lodash": { + "version": "4.14.186", + "resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.14.186.tgz", + "integrity": "sha512-eHcVlLXP0c2FlMPm56ITode2AgLMSa6aJ05JTTbYbI+7EMkCEE5qk2E41d5g2lCVTqRe0GnnRFurmlCsDODrPw==", + "dev": true + }, "@types/long": { "version": "4.0.2", "resolved": "https://registry.npmjs.org/@types/long/-/long-4.0.2.tgz", @@ -6119,7 +6125,8 @@ "dependencies": { "ansi-regex": { "version": "5.0.0", - "resolved": "", + "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-5.0.0.tgz", + "integrity": "sha512-bY6fj56OUQ0hU1KjFNDQuJFezqKdrAyFdIevADiqrWHwSlbmBNMHp5ak2f40Pm8JTFyM2mqxkG6ngkHO11f/lg==", "dev": true }, "ansi-styles": { @@ -6783,7 +6790,7 @@ "detect-file": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/detect-file/-/detect-file-1.0.0.tgz", - "integrity": "sha512-DtCOLG98P007x7wiiOmfI0fi3eIKyWiLTGJ2MDnVi/E04lWGbf+JzrRHMm0rgIIZJGtHpKpbVgLWHrv8xXpc3Q==", + "integrity": "sha1-8NZtA2cqglyxtzvbP+YjEMjlUrc=", "dev": true }, "detect-indent": { @@ -7717,7 +7724,7 @@ "expand-tilde": { "version": "2.0.2", "resolved": "https://registry.npmjs.org/expand-tilde/-/expand-tilde-2.0.2.tgz", - "integrity": "sha512-A5EmesHW6rfnZ9ysHQjPdJRni0SRar0tjtG5MNtm9n5TUvsYU8oozprtRD4AqHxcZWWlVuAmQo2nWKfN9oyjTw==", + "integrity": "sha1-l+gBqgUt8CRU3kawK/YhZCzchQI=", "dev": true, "requires": { "homedir-polyfill": "^1.0.1" @@ -8139,7 +8146,7 @@ "ftp": { "version": "0.3.10", "resolved": "https://registry.npmjs.org/ftp/-/ftp-0.3.10.tgz", - "integrity": "sha512-faFVML1aBx2UoDStmLwv2Wptt4vw5x03xxX172nhA5Y5HBshW5JweqQ2W4xL4dezQTG8inJsuYcpPHHU3X5OTQ==", + "integrity": "sha1-kZfYYa2BQvPmPVqDv+TFn3MwiF0=", "requires": { "readable-stream": "1.1.x", "xregexp": "2.0.0" @@ -8148,12 +8155,12 @@ "isarray": { "version": "0.0.1", "resolved": "https://registry.npmjs.org/isarray/-/isarray-0.0.1.tgz", - "integrity": "sha512-D2S+3GLxWH+uhrNEcoh/fnmYeP8E8/zHl644d/jdA0g2uyXvy3sb0qxotE+ne0LtccHknQzWwZEzhak7oJ0COQ==" + "integrity": "sha1-ihis/Kmo9Bd+Cav8YDiTmwXR7t8=" }, "readable-stream": { "version": "1.1.14", "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-1.1.14.tgz", - "integrity": "sha512-+MeVjFf4L44XUkhM1eYbD8fyEsxcV81pqMSR5gblfcLCHfZvbrqy4/qYHE+/R5HoBUT11WV5O08Cr1n3YXkWVQ==", + "integrity": "sha1-fPTFTvZI44EwhMY23SB54WbAgdk=", "requires": { "core-util-is": "~1.0.0", "inherits": "~2.0.1", @@ -8164,7 +8171,7 @@ "string_decoder": { "version": "0.10.31", "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-0.10.31.tgz", - "integrity": "sha512-ev2QzSzWPYmy9GuqfIVildA4OdcGLeFZQrq5ys6RtiuF+RQQiZWr8TZNyAcuVXyQRYfEO+MsoB/1BuQVhOJuoQ==" + "integrity": "sha1-YuIDvEF2bGwoyfyEMB2rHFMQ+pQ=" } } }, @@ -8354,7 +8361,7 @@ "global-prefix": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/global-prefix/-/global-prefix-1.0.2.tgz", - "integrity": "sha512-5lsx1NUDHtSjfg0eHlmYvZKv8/nVqX4ckFbM+FrGcQ+04KWcWFo9P5MxPZYSzUvyzmdTbI7Eix8Q4IbELDqzKg==", + "integrity": "sha1-2/dDxsFJklk8ZVVoy2btMsASLr4=", "dev": true, "requires": { "expand-tilde": "^2.0.2", @@ -8995,7 +9002,7 @@ "is-utf8": { "version": "0.2.1", "resolved": "https://registry.npmjs.org/is-utf8/-/is-utf8-0.2.1.tgz", - "integrity": "sha512-rMYPYvCzsXywIsldgLaSoPlw5PfoB/ssr7hY4pLfcodrA5M/eArza1a9VmTiNIBNMjOGr1Ow9mTyU2o69U6U9Q==", + "integrity": "sha1-Sw2hRCEE0bM2NA6AeX6GXPOffXI=", "dev": true }, "is-weakref": { @@ -11004,7 +11011,8 @@ "dependencies": { "ansi-regex": { "version": "5.0.0", - "resolved": "", + "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-5.0.0.tgz", + "integrity": "sha512-bY6fj56OUQ0hU1KjFNDQuJFezqKdrAyFdIevADiqrWHwSlbmBNMHp5ak2f40Pm8JTFyM2mqxkG6ngkHO11f/lg==", "dev": true }, "ansi-styles": { @@ -11087,7 +11095,7 @@ "lodash.assign": { "version": "4.2.0", "resolved": "https://registry.npmjs.org/lodash.assign/-/lodash.assign-4.2.0.tgz", - "integrity": "sha512-hFuH8TY+Yji7Eja3mGiuAxBqLagejScbG8GbG0j6o9vzn0YL14My+ktnqtZgFTosKymC9/44wP6s7xyuLfnClw==" + "integrity": "sha1-DZnzzNem0mHRm9rrkkUAXShYCOc=" }, "lodash.clonedeep": { "version": "4.5.0", @@ -11098,7 +11106,7 @@ "lodash.find": { "version": "4.6.0", "resolved": "https://registry.npmjs.org/lodash.find/-/lodash.find-4.6.0.tgz", - "integrity": "sha512-yaRZoAV3Xq28F1iafWN1+a0rflOej93l1DQUejs3SZ41h2O9UJBoS9aueGjPDgAl4B6tPC0NuuchLKaDQQ3Isg==" + "integrity": "sha1-ywcE1Hq3F4n/oN6Ll92Sb7iLE7E=" }, "lodash.includes": { "version": "4.3.0", @@ -11113,12 +11121,12 @@ "lodash.isempty": { "version": "4.4.0", "resolved": "https://registry.npmjs.org/lodash.isempty/-/lodash.isempty-4.4.0.tgz", - "integrity": "sha512-oKMuF3xEeqDltrGMfDxAPGIVMSSRv8tbRSODbrs4KGsRRLEhrW8N8Rd4DRgB2+621hY8A8XwwrTVhXWpxFvMzg==" + "integrity": "sha1-b4bL7di+TsmHvpqvM8loTbGzHn4=" }, "lodash.iserror": { "version": "3.1.1", "resolved": "https://registry.npmjs.org/lodash.iserror/-/lodash.iserror-3.1.1.tgz", - "integrity": "sha512-eT/VeNns9hS7vAj1NKW/rRX6b+C3UX3/IAAqEE7aC4Oo2C0iD82NaP5IS4bSlQsammTii4qBJ8G1zd1LTL8hCw==" + "integrity": "sha1-KXuaBfq2cUvCRE18wZ0dfES17Ow=" }, "lodash.isinteger": { "version": "4.0.4", @@ -12223,7 +12231,7 @@ "os-tmpdir": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/os-tmpdir/-/os-tmpdir-1.0.2.tgz", - "integrity": "sha512-D2FR03Vir7FIu45XBY20mTb+/ZSWB00sjU9jdQXt83gDrI4Ztz5Fs7/yy74g2N5SVQY4xY1qDr4rNddwYRVX0g==", + "integrity": "sha1-u+Z0BseaqFxc/sdm/lc0VV36EnQ=", "dev": true }, "otplib": { @@ -12350,7 +12358,7 @@ "parse-passwd": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/parse-passwd/-/parse-passwd-1.0.0.tgz", - "integrity": "sha512-1Y1A//QUXEZK7YKz+rD9WydcE1+EuPr6ZBgKecAB8tmoW6UFv0NREVJe1p+jRxtThkcbbKkfwIbWJe/IeE6m2Q==", + "integrity": "sha1-bVuTSkVpk7I9N/QKOC1vFmao5cY=", "dev": true }, "parse5": { @@ -12843,7 +12851,7 @@ "resolve-dir": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/resolve-dir/-/resolve-dir-1.0.1.tgz", - "integrity": "sha512-R7uiTjECzvOsWSfdM0QKFNBVFcK27aHOUwdvK53BcW8zqnGdYp0Fbj82cy54+2A4P2tFM22J5kRfe1R+lM/1yg==", + "integrity": "sha1-eaQGRMNivoLybv/nOcm7U4IEb0M=", "dev": true, "requires": { "expand-tilde": "^2.0.0", @@ -13867,7 +13875,7 @@ "toposort-class": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/toposort-class/-/toposort-class-1.0.1.tgz", - "integrity": "sha512-OsLcGGbYF3rMjPUf8oKktyvCiUxSbqMMS39m33MAjLTC1DVIH6x3WSt63/M77ihI09+Sdfk1AXvfhCEeUmC7mg==" + "integrity": "sha1-f/0feMi+KMO6Rc1OGj9e4ZO9mYg=" }, "tough-cookie": { "version": "4.0.0", @@ -14685,7 +14693,7 @@ "xregexp": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/xregexp/-/xregexp-2.0.0.tgz", - "integrity": "sha512-xl/50/Cf32VsGq/1R8jJE5ajH1yMCQkpmoS10QbFZWl2Oor4H0Me64Pu2yxvsRWK3m6soJbmGfzSR7BYmDcWAA==" + "integrity": "sha1-UqY+VsoLhKfzpfPWGHLxJq16WUM=" }, "xtend": { "version": "4.0.2", diff --git a/package.json b/package.json index 67647b7b4..5ff330b58 100644 --- a/package.json +++ b/package.json @@ -84,6 +84,7 @@ "@tsconfig/recommended": "^1.0.1", "@types/express": "^4.17.13", "@types/jest": "^27.4.1", + "@types/lodash": "^4.14.186", "@types/node": "^17.0.21", "@types/supertest": "^2.0.11", "@types/validator": "^13.7.1", diff --git a/src/constants/constants.ts b/src/constants/constants.ts index b4a23c177..ad125703f 100644 --- a/src/constants/constants.ts +++ b/src/constants/constants.ts @@ -10,6 +10,11 @@ export enum SiteStatus { Launched = "LAUNCHED", } +export enum CollaboratorRoles { + Admin = "ADMIN", + Contributor = "CONTRIBUTOR", +} + export const E2E_ISOMER_ID = "-1" export const E2E_TEST_EMAIL = "test@e2e" export const E2E_TEST_CONTACT = "12345678" diff --git a/src/database/migrations/20220811070630-change-role-enum.js b/src/database/migrations/20220811070630-change-role-enum.js new file mode 100644 index 000000000..aee6b357e --- /dev/null +++ b/src/database/migrations/20220811070630-change-role-enum.js @@ -0,0 +1,59 @@ +module.exports = { + async up(queryInterface, Sequelize) { + // Change the role enum values in the site_members table + await queryInterface.sequelize.transaction(async (transaction) => { + // 1. Change column type to TEXT + await queryInterface.changeColumn( + "site_members", // name of Source model + "role", // name of column we're modifying + { + type: Sequelize.TEXT, + }, + { transaction } + ) + // 2. Discard enum type + await queryInterface.sequelize.query( + "drop type enum_site_members_role;", + { transaction } + ) + // 3. Change column type to new enum type (fails if inconsistent with existing data) + await queryInterface.changeColumn( + "site_members", // name of Source model + "role", // name of column we're modifying + { + type: Sequelize.ENUM("ADMIN", "CONTRIBUTOR"), + }, + { transaction } + ) + }) + }, + + async down(queryInterface, Sequelize) { + // Change the role enum values in the site_members table + await queryInterface.sequelize.transaction(async (transaction) => { + // 1. Change column type to TEXT + await queryInterface.changeColumn( + "site_members", // name of Source model + "role", // name of column we're modifying + { + type: Sequelize.TEXT, + }, + { transaction } + ) + // 2. Discard enum type + await queryInterface.sequelize.query( + "drop type enum_site_members_role;", + { transaction } + ) + // 3. Change column type to new enum type (fails if inconsistent with existing data) + await queryInterface.changeColumn( + "site_members", // name of Source model + "role", // name of column we're modifying + { + type: Sequelize.ENUM("ADMIN", "USER"), + }, + { transaction } + ) + }) + }, +} diff --git a/src/database/models/Site.ts b/src/database/models/Site.ts index 8ab687cec..43f9ce141 100644 --- a/src/database/models/Site.ts +++ b/src/database/models/Site.ts @@ -32,6 +32,7 @@ export class Site extends Model { @Column({ allowNull: false, type: DataType.TEXT, + unique: true, }) name!: string @@ -70,7 +71,7 @@ export class Site extends Model { through: () => SiteMember, as: "site_members", }) - users!: User[] + site_members!: Array @HasOne(() => Repo) repo?: Repo diff --git a/src/database/models/SiteMember.ts b/src/database/models/SiteMember.ts index cf8a37f28..d5c466691 100644 --- a/src/database/models/SiteMember.ts +++ b/src/database/models/SiteMember.ts @@ -8,6 +8,8 @@ import { UpdatedAt, } from "sequelize-typescript" +import { CollaboratorRoles } from "@constants/index" + import { Site } from "@database/models/Site" import { User } from "@database/models/User" @@ -23,9 +25,9 @@ export class SiteMember extends Model { @Column({ allowNull: false, - type: DataType.ENUM("ADMIN", "USER"), + type: DataType.ENUM("ADMIN", "CONTRIBUTOR"), }) - role!: boolean + role!: CollaboratorRoles @CreatedAt createdAt!: Date diff --git a/src/database/models/User.ts b/src/database/models/User.ts index e5eb64c6f..185ccc0ed 100644 --- a/src/database/models/User.ts +++ b/src/database/models/User.ts @@ -67,7 +67,7 @@ export class User extends Model { through: () => SiteMember, as: "site_members", }) - sites!: Site[] + sites!: Array @HasMany(() => Site, { as: "sites_created", diff --git a/src/errors/ForbiddenError.js b/src/errors/ForbiddenError.js index 8c0b066cd..3e5919184 100644 --- a/src/errors/ForbiddenError.js +++ b/src/errors/ForbiddenError.js @@ -2,8 +2,8 @@ const { BaseIsomerError } = require("@errors/BaseError") class ForbiddenError extends BaseIsomerError { - constructor() { - super(403, "Access forbidden") + constructor(message) { + super(403, message || "Access forbidden") } } diff --git a/src/fixtures/identity.ts b/src/fixtures/identity.ts index a4953d4a4..8d0990547 100644 --- a/src/fixtures/identity.ts +++ b/src/fixtures/identity.ts @@ -1,3 +1,7 @@ +import { Attributes } from "sequelize/types" + +import { User, SiteMember } from "@database/models" + export const mockRecipient = "hello@world.com" export const mockSubject = "mock subject" export const mockBody = "somebody" @@ -18,3 +22,119 @@ export const mockBearerTokenHeaders = { Authorization: `Bearer ${process.env.POSTMAN_API_KEY}`, }, } + +const mockCollaboratorContributor1: Attributes & { + SiteMember: Attributes +} = { + id: 1, + email: "test1@test.gov.sg", + githubId: "test1", + contactNumber: "12331231", + lastLoggedIn: new Date("2022-07-30T07:41:09.661Z"), + createdAt: new Date("2022-04-04T07:25:41.013Z"), + updatedAt: new Date("2022-07-30T07:41:09.662Z"), + deletedAt: undefined, + SiteMember: { + userId: 1, + siteId: "16", + role: "CONTRIBUTOR", + createdAt: new Date("2022-07-29T03:50:49.145Z"), + updatedAt: new Date("2022-07-29T03:50:49.145Z"), + }, + sites: [], +} + +const mockCollaboratorAdmin1: Attributes & { + SiteMember: Attributes +} = { + id: 2, + email: "test2@test.gov.sg", + githubId: "test2", + contactNumber: "12331232", + lastLoggedIn: new Date("2022-07-30T07:41:09.661Z"), + createdAt: new Date("2022-04-04T07:25:41.013Z"), + updatedAt: new Date("2022-07-30T07:41:09.662Z"), + deletedAt: undefined, + SiteMember: { + userId: 2, + siteId: "16", + role: "ADMIN", + createdAt: new Date("2022-07-29T03:50:49.145Z"), + updatedAt: new Date("2022-07-29T03:50:49.145Z"), + }, + sites: [], +} +const mockCollaboratorAdmin2: Attributes & { + SiteMember: Attributes +} = { + id: 3, + email: "test3@test.gov.sg", + githubId: "test3", + contactNumber: "12331233", + lastLoggedIn: new Date("2022-06-30T07:41:09.661Z"), + createdAt: new Date("2022-04-04T07:25:41.013Z"), + updatedAt: new Date("2022-07-30T07:41:09.662Z"), + deletedAt: undefined, + SiteMember: { + userId: 3, + siteId: "16", + role: "ADMIN", + createdAt: new Date("2022-07-29T03:50:49.145Z"), + updatedAt: new Date("2022-07-29T03:50:49.145Z"), + }, + sites: [], +} +const mockCollaboratorContributor2: Attributes & { + SiteMember: Attributes +} = { + id: 4, + email: "test4@test.gov.sg", + githubId: "test4", + contactNumber: "12331234", + lastLoggedIn: new Date("2022-07-30T07:41:09.661Z"), + createdAt: new Date("2022-04-04T07:25:41.013Z"), + updatedAt: new Date("2022-07-30T07:41:09.662Z"), + deletedAt: undefined, + SiteMember: { + userId: 4, + siteId: "16", + role: "CONTRIBUTOR", + createdAt: new Date("2022-07-29T03:50:49.145Z"), + updatedAt: new Date("2022-07-29T03:50:49.145Z"), + }, + sites: [], +} + +export const unsortedMockCollaboratorsList = [ + mockCollaboratorContributor1, + mockCollaboratorAdmin1, + mockCollaboratorAdmin2, + mockCollaboratorContributor2, +] + +export const expectedSortedMockCollaboratorsList = [ + mockCollaboratorAdmin2, + mockCollaboratorAdmin1, + mockCollaboratorContributor1, + mockCollaboratorContributor2, +] + +export const mockSiteOrmResponseWithAllCollaborators = { + id: 1, + name: "", + site_members: unsortedMockCollaboratorsList, +} +export const mockSiteOrmResponseWithOneAdminCollaborator = { + id: 1, + name: "", + site_members: [mockCollaboratorAdmin1], +} +export const mockSiteOrmResponseWithOneContributorCollaborator = { + id: 1, + name: "", + site_members: [mockCollaboratorContributor2], +} +export const mockSiteOrmResponseWithNoCollaborators = { + id: 1, + site_members: "", +} diff --git a/src/middleware/__tests__/authorization.ts b/src/middleware/__tests__/authorization.ts new file mode 100644 index 000000000..14f254f11 --- /dev/null +++ b/src/middleware/__tests__/authorization.ts @@ -0,0 +1,111 @@ +import { NextFunction, Request, Response } from "express" + +import { ForbiddenError } from "@errors/ForbiddenError" + +import { AuthorizationMiddleware } from "@middleware/authorization" + +import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" +import AuthorizationMiddlewareService from "@root/services/middlewareServices/AuthorizationMiddlewareService" + +describe("Authorization middleware", () => { + const TEST_SITE_NAME = "sitename" + const TEST_ISOMER_USER_ID = "1" + const mockAuthorizationMiddlewareService = { + checkIsSiteAdmin: jest.fn(), + checkIsSiteMember: jest.fn(), + } + const mockReq = ({ + params: { siteName: TEST_SITE_NAME }, + } as unknown) as Request< + never, + unknown, + unknown, + never, + { userWithSiteSessionData: UserWithSiteSessionData } + > + const mockRes = ({ + locals: { + sessionData: { getIsomerUserId: jest.fn(() => TEST_ISOMER_USER_ID) }, + }, + } as unknown) as Response< + unknown, + { userWithSiteSessionData: UserWithSiteSessionData } + > + const mockNext = jest.fn() as NextFunction + + const authorizationMiddleware = new AuthorizationMiddleware({ + authorizationMiddlewareService: (mockAuthorizationMiddlewareService as unknown) as AuthorizationMiddlewareService, + }) + + beforeEach(() => { + jest.clearAllMocks() + }) + + describe("verifySiteAdmin", () => { + it("correctly verifies that user is a site admin if no error is thrown in the authorization middleware service", async () => { + // Arrange + mockAuthorizationMiddlewareService.checkIsSiteAdmin.mockResolvedValue( + undefined + ) + + // Act + await authorizationMiddleware.verifySiteAdmin(mockReq, mockRes, mockNext) + + // Assert + expect( + mockAuthorizationMiddlewareService.checkIsSiteAdmin + ).toHaveBeenCalled() + expect(mockNext).toHaveBeenCalledWith() + }) + + it("correctly verifies that user is not site admin if an error is thrown in the authorization middleware service", async () => { + // Arrange + mockAuthorizationMiddlewareService.checkIsSiteAdmin.mockResolvedValue( + new ForbiddenError() + ) + + // Act + await authorizationMiddleware.verifySiteAdmin(mockReq, mockRes, mockNext) + + // Assert + expect( + mockAuthorizationMiddlewareService.checkIsSiteAdmin + ).toHaveBeenCalled() + expect(mockNext).toHaveBeenCalledWith(new ForbiddenError()) + }) + }) + + describe("verifySiteMember", () => { + it("correctly verifies that user is a site member if no error is thrown in the authorization middleware service", async () => { + // Arrange + mockAuthorizationMiddlewareService.checkIsSiteMember.mockResolvedValue( + undefined + ) + + // Act + await authorizationMiddleware.verifySiteMember(mockReq, mockRes, mockNext) + + // Assert + expect( + mockAuthorizationMiddlewareService.checkIsSiteMember + ).toHaveBeenCalled() + expect(mockNext).toHaveBeenCalledWith() + }) + + it("correctly verifies that user is not site member if an error is thrown in the authorization middleware service", async () => { + // Arrange + mockAuthorizationMiddlewareService.checkIsSiteMember.mockResolvedValue( + new ForbiddenError() + ) + + // Act + await authorizationMiddleware.verifySiteMember(mockReq, mockRes, mockNext) + + // Assert + expect( + mockAuthorizationMiddlewareService.checkIsSiteMember + ).toHaveBeenCalled() + expect(mockNext).toHaveBeenCalledWith(new ForbiddenError()) + }) + }) +}) diff --git a/src/middleware/authorization.ts b/src/middleware/authorization.ts index 777409aa2..05ec124f2 100644 --- a/src/middleware/authorization.ts +++ b/src/middleware/authorization.ts @@ -1,5 +1,6 @@ import autoBind from "auto-bind" -import { NextFunction, Request, Response } from "express" + +import { ForbiddenError } from "@errors/ForbiddenError" import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" @@ -19,8 +20,30 @@ export class AuthorizationMiddleware { autoBind(this) } + // Check whether a user is a site admin + verifySiteAdmin: RequestHandler< + never, + unknown, + unknown, + never, + { userWithSiteSessionData: UserWithSiteSessionData } + > = async (req, res, next) => { + const { userWithSiteSessionData } = res.locals + + try { + const result = await this.authorizationMiddlewareService.checkIsSiteAdmin( + userWithSiteSessionData + ) + if (result instanceof ForbiddenError) return next(new ForbiddenError()) + + return next() + } catch (err) { + return next(err) + } + } + // Check whether a user is a site member - checkIsSiteMember: RequestHandler< + verifySiteMember: RequestHandler< never, unknown, unknown, @@ -29,10 +52,15 @@ export class AuthorizationMiddleware { > = async (req, res, next) => { const { userWithSiteSessionData } = res.locals - await this.authorizationMiddlewareService.checkIsSiteMember( - userWithSiteSessionData - ) + try { + const result = await this.authorizationMiddlewareService.checkIsSiteMember( + userWithSiteSessionData + ) + if (result instanceof ForbiddenError) return next(new ForbiddenError()) - return next() + return next() + } catch (err) { + return next(err) + } } } diff --git a/src/middleware/index.ts b/src/middleware/index.ts index ea6a7fbf6..3138e3c05 100644 --- a/src/middleware/index.ts +++ b/src/middleware/index.ts @@ -7,6 +7,7 @@ import { AuthorizationMiddleware } from "@middleware/authorization" import UserSessionData from "@classes/UserSessionData" import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" +import CollaboratorsService from "@root/services/identity/CollaboratorsService" import { RequestHandler } from "@root/types" import IdentityAuthService from "@services/identity/AuthService" import IsomerAdminsService from "@services/identity/IsomerAdminsService" @@ -27,15 +28,18 @@ const getAuthorizationMiddleware = ({ identityAuthService, usersService, isomerAdminsService, + collaboratorsService, }: { identityAuthService: IdentityAuthService usersService: UsersService isomerAdminsService: IsomerAdminsService + collaboratorsService: CollaboratorsService }) => { const authorizationMiddlewareService = new AuthorizationMiddlewareService({ identityAuthService, usersService, isomerAdminsService, + collaboratorsService, }) const authorizationMiddleware = new AuthorizationMiddleware({ authorizationMiddlewareService, diff --git a/src/routes/v1/authenticatedSites/index.js b/src/routes/v1/authenticatedSites/index.js index 908fe4ed1..bea659c86 100644 --- a/src/routes/v1/authenticatedSites/index.js +++ b/src/routes/v1/authenticatedSites/index.js @@ -26,7 +26,7 @@ const getAuthenticatedSitesSubrouter = ({ authenticatedSitesSubrouter.use(authenticationMiddleware.verifyJwt) authenticatedSitesSubrouter.use(attachSiteHandler) - authenticatedSitesSubrouter.use(authorizationMiddleware.checkIsSiteMember) + authenticatedSitesSubrouter.use(authorizationMiddleware.verifySiteMember) authenticatedSitesSubrouter.use("/pages", pagesRouter) authenticatedSitesSubrouter.use("/collections", collectionsRouter) diff --git a/src/routes/v2/authenticated/__tests__/collaborators.spec.ts b/src/routes/v2/authenticated/__tests__/collaborators.spec.ts new file mode 100644 index 000000000..d2d16cea3 --- /dev/null +++ b/src/routes/v2/authenticated/__tests__/collaborators.spec.ts @@ -0,0 +1,179 @@ +import express from "express" +import request from "supertest" + +import { attachReadRouteHandlerWrapper } from "@middleware/routeHandler" + +import { CollaboratorsRouter } from "@routes/v2/authenticated/collaborators" + +import { generateRouter } from "@fixtures/app" +import { mockSiteName, mockIsomerUserId } from "@fixtures/sessionData" +import { NotFoundError } from "@root/errors/NotFoundError" +import { UnprocessableError } from "@root/errors/UnprocessableError" +import { AuthorizationMiddleware } from "@root/middleware/authorization" +import CollaboratorsService from "@root/services/identity/CollaboratorsService" + +describe("Collaborator Router", () => { + const MOCK_EMAIL = "mockemail" + const MOCK_ACK_VALUE = true + const mockCollaboratorsService = { + create: jest.fn(), + delete: jest.fn(), + list: jest.fn(), + getRole: jest.fn(), + } + const mockAuthorizationMiddleware = { + verifySiteAdmin: jest.fn(), + verifySiteMember: jest.fn(), + } + + const collaboratorsRouter = new CollaboratorsRouter({ + collaboratorsService: (mockCollaboratorsService as unknown) as CollaboratorsService, + authorizationMiddleware: (mockAuthorizationMiddleware as unknown) as AuthorizationMiddleware, + }) + + const subrouter = express() + + // We can use read route handler here because we don't need to lock the repo + subrouter.get( + `/:siteName/collaborators/role`, + attachReadRouteHandlerWrapper(collaboratorsRouter.getCollaboratorRole) + ) + subrouter.get( + `/:siteName/collaborators/`, + attachReadRouteHandlerWrapper(collaboratorsRouter.listCollaborators) + ) + subrouter.post( + `/:siteName/collaborators/`, + attachReadRouteHandlerWrapper(collaboratorsRouter.createCollaborator) + ) + subrouter.delete( + `/:siteName/collaborators/:userId`, + attachReadRouteHandlerWrapper(collaboratorsRouter.deleteCollaborator) + ) + + const app = generateRouter(subrouter) + + beforeEach(() => { + jest.clearAllMocks() + }) + + describe("list collaborators", () => { + it("should retrieve the list of collaborators for a site", async () => { + // Arrange + const mockCollaboratorsValue: never[] = [] + const mockCollaboratorsResponse = { + collaborators: mockCollaboratorsValue, + } + mockCollaboratorsService.list.mockResolvedValue(mockCollaboratorsValue) + + // Act + const resp = await request(app) + .get(`/${mockSiteName}/collaborators/`) + .expect(200) + + // Assert + expect(resp.body).toStrictEqual(mockCollaboratorsResponse) + expect(mockCollaboratorsService.list).toHaveBeenCalledWith( + mockSiteName, + mockIsomerUserId + ) + }) + }) + + describe("create collaborators", () => { + it("should create a new collaborator", async () => { + // Arrange + const mockRequestBody = { email: MOCK_EMAIL, acknowledge: MOCK_ACK_VALUE } + + // Act + await request(app) + .post(`/${mockSiteName}/collaborators/`) + .send(mockRequestBody) + .expect(200) + + // Assert + expect(mockCollaboratorsService.create).toHaveBeenCalledWith( + mockSiteName, + MOCK_EMAIL, + MOCK_ACK_VALUE + ) + }) + }) + + describe("delete collaborator", () => { + it("should delete collaborator successfully", async () => { + // Arrange + mockCollaboratorsService.delete.mockResolvedValue(1) + + // Act + await request(app) + .delete(`/${mockSiteName}/collaborators/${mockIsomerUserId}`) + .expect(200) + + // Assert + expect(mockCollaboratorsService.delete).toHaveBeenCalledWith( + mockSiteName, + mockIsomerUserId + ) + }) + + it("should not delete last admin collaborator", async () => { + // Arrange + mockCollaboratorsService.delete.mockResolvedValue( + new UnprocessableError("") + ) + + // Act + await request(app) + .delete(`/${mockSiteName}/collaborators/${mockIsomerUserId}`) + .expect(422) + + // Assert + expect(mockCollaboratorsService.delete).toHaveBeenCalledWith( + mockSiteName, + mockIsomerUserId + ) + }) + + it("should not delete user if user is not a site collaborator", async () => { + // Arrange + mockCollaboratorsService.delete.mockResolvedValue(new NotFoundError("")) + + // Act + await request(app) + .delete(`/${mockSiteName}/collaborators/${mockIsomerUserId}`) + .expect(404) + + // Assert + expect(mockCollaboratorsService.delete).toHaveBeenCalledWith( + mockSiteName, + mockIsomerUserId + ) + }) + }) + + describe("get collaborator role", () => { + it("should get collaborator role", async () => { + // Arrange + const MOCK_COLLABORATOR_ROLE_VALUE = "role" + const mockGetCollaboratorRoleResponse = { + role: MOCK_COLLABORATOR_ROLE_VALUE, + } + mockCollaboratorsService.getRole.mockResolvedValue( + MOCK_COLLABORATOR_ROLE_VALUE + ) + + // Act + const resp = await request(app) + .get(`/${mockSiteName}/collaborators/role`) + .expect(200) + + // Assert + expect(resp.body).toStrictEqual(mockGetCollaboratorRoleResponse) + expect(mockCollaboratorsService.getRole).toHaveBeenCalledWith( + mockSiteName, + mockIsomerUserId + ) + }) + }) +}) diff --git a/src/routes/v2/authenticated/collaborators.ts b/src/routes/v2/authenticated/collaborators.ts new file mode 100644 index 000000000..25b97317f --- /dev/null +++ b/src/routes/v2/authenticated/collaborators.ts @@ -0,0 +1,135 @@ +import autoBind from "auto-bind" +import express from "express" + +import { AuthorizationMiddleware } from "@middleware/authorization" +import { attachReadRouteHandlerWrapper } from "@middleware/routeHandler" + +import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" + +import { BaseIsomerError } from "@root/errors/BaseError" +import { attachSiteHandler } from "@root/middleware" +import { RequestHandler } from "@root/types" +import CollaboratorsService from "@services/identity/CollaboratorsService" + +interface CollaboratorsRouterProps { + collaboratorsService: CollaboratorsService + authorizationMiddleware: AuthorizationMiddleware +} + +// eslint-disable-next-line import/prefer-default-export +export class CollaboratorsRouter { + private readonly collaboratorsService + + private readonly authorizationMiddleware + + constructor({ + collaboratorsService, + authorizationMiddleware, + }: CollaboratorsRouterProps) { + this.collaboratorsService = collaboratorsService + this.authorizationMiddleware = authorizationMiddleware + autoBind(this) + } + + createCollaborator: RequestHandler< + never, + unknown, + { email: string; acknowledge?: boolean }, + { siteName: string }, + { userWithSiteSessionData: UserWithSiteSessionData } + > = async (req, res) => { + const { email, acknowledge = false } = req.body + const { siteName } = req.params + const resp = await this.collaboratorsService.create( + siteName, + email, + acknowledge + ) + + // Check for error and throw + if (resp instanceof BaseIsomerError) { + throw resp + } + return res.sendStatus(200) + } + + deleteCollaborator: RequestHandler< + never, + unknown, + never, + { siteName: string; userId: string }, + { userWithSiteSessionData: UserWithSiteSessionData } + > = async (req, res) => { + const { siteName, userId } = req.params + const resp = await this.collaboratorsService.delete(siteName, userId) + + // Check for error and throw + if (resp instanceof BaseIsomerError) { + throw resp + } + return res.sendStatus(200) + } + + listCollaborators: RequestHandler< + never, + unknown, + never, + { siteName: string }, + { userWithSiteSessionData: UserWithSiteSessionData } + > = async (req, res) => { + const { siteName } = req.params + const { userWithSiteSessionData } = res.locals + const collaborators = await this.collaboratorsService.list( + siteName, + userWithSiteSessionData.isomerUserId + ) + + return res.status(200).json({ collaborators }) + } + + getCollaboratorRole: RequestHandler< + never, + unknown, + never, + { siteName: string }, + { userWithSiteSessionData: UserWithSiteSessionData } + > = async (req, res) => { + const { siteName } = req.params + const { userWithSiteSessionData } = res.locals + const role = await this.collaboratorsService.getRole( + siteName, + userWithSiteSessionData.isomerUserId + ) + return res.status(200).json({ role }) + } + + getRouter() { + const router = express.Router({ mergeParams: true }) + router.get( + "/role", + attachSiteHandler, + this.authorizationMiddleware.verifySiteMember, + attachReadRouteHandlerWrapper(this.getCollaboratorRole) + ) + router.get( + "/", + attachSiteHandler, + this.authorizationMiddleware.verifySiteMember, + attachReadRouteHandlerWrapper(this.listCollaborators) + ) + router.post( + "/", + attachSiteHandler, + this.authorizationMiddleware.verifySiteAdmin, + attachReadRouteHandlerWrapper(this.createCollaborator) + ) + router.delete( + "/:userId", + attachSiteHandler, + this.authorizationMiddleware.verifySiteAdmin, + attachReadRouteHandlerWrapper(this.deleteCollaborator) + ) + + return router + } +} diff --git a/src/routes/v2/authenticated/index.js b/src/routes/v2/authenticated/index.js index 89a2f93c4..62ebdb028 100644 --- a/src/routes/v2/authenticated/index.js +++ b/src/routes/v2/authenticated/index.js @@ -1,5 +1,3 @@ -import InfraService from "@services/infra/InfraService" - const express = require("express") const { @@ -7,6 +5,7 @@ const { } = require("@services/configServices/NetlifyTomlService") const { SitesService } = require("@services/utilServices/SitesService") +const { CollaboratorsRouter } = require("./collaborators") const { NetlifyTomlRouter } = require("./netlifyToml") const { SitesRouter } = require("./sites") const { UsersRouter } = require("./users") @@ -17,6 +16,8 @@ const getAuthenticatedSubrouter = ({ configYmlService, usersService, isomerAdminsService, + collaboratorsService, + authorizationMiddleware, }) => { const sitesService = new SitesService({ gitHubService, @@ -27,6 +28,10 @@ const getAuthenticatedSubrouter = ({ const netlifyTomlService = new NetlifyTomlService() const sitesV2Router = new SitesRouter({ sitesService }) + const collaboratorsRouter = new CollaboratorsRouter({ + collaboratorsService, + authorizationMiddleware, + }) const usersRouter = new UsersRouter({ usersService }) const netlifyTomlV2Router = new NetlifyTomlRouter({ netlifyTomlService }) @@ -34,6 +39,10 @@ const getAuthenticatedSubrouter = ({ authenticatedSubrouter.use(authenticationMiddleware.verifyJwt) + authenticatedSubrouter.use( + "/sites/:siteName/collaborators", + collaboratorsRouter.getRouter() + ) authenticatedSubrouter.use("/sites", sitesV2Router.getRouter()) authenticatedSubrouter.use("/user", usersRouter.getRouter()) authenticatedSubrouter.use("/netlify-toml", netlifyTomlV2Router.getRouter()) diff --git a/src/routes/v2/authenticatedSites/index.js b/src/routes/v2/authenticatedSites/index.js index f7c40716d..c97d7dfe6 100644 --- a/src/routes/v2/authenticatedSites/index.js +++ b/src/routes/v2/authenticatedSites/index.js @@ -189,7 +189,7 @@ const getAuthenticatedSitesSubrouter = ({ authenticatedSitesSubrouter.use(authenticationMiddleware.verifyJwt) authenticatedSitesSubrouter.use(attachSiteHandler) - authenticatedSitesSubrouter.use(authorizationMiddleware.checkIsSiteMember) + authenticatedSitesSubrouter.use(authorizationMiddleware.verifySiteMember) authenticatedSitesSubrouter.use( "/collections/:collectionName", diff --git a/src/server.js b/src/server.js index f444e90c7..e40469447 100644 --- a/src/server.js +++ b/src/server.js @@ -30,10 +30,12 @@ import DeploymentsService from "@services/identity/DeploymentsService" import ReposService from "@services/identity/ReposService" import InfraService from "@services/infra/InfraService" +import { AuthorizationMiddleware } from "./middleware/authorization" import getAuthenticatedSubrouterV1 from "./routes/v1/authenticated" import getAuthenticatedSitesSubrouterV1 from "./routes/v1/authenticatedSites" import getAuthenticatedSubrouter from "./routes/v2/authenticated" import getAuthenticatedSitesSubrouter from "./routes/v2/authenticatedSites" +import CollaboratorsService from "./services/identity/CollaboratorsService" const path = require("path") @@ -81,6 +83,13 @@ const infraService = new InfraService({ reposService, deploymentsService, }) +const collaboratorsService = new CollaboratorsService({ + siteRepository: Site, + siteMemberRepository: SiteMember, + sitesService, + usersService, + whitelist: Whitelist, +}) const gitHubService = new GitHubService({ axiosInstance: isomerRepoAxiosInstance, @@ -94,6 +103,7 @@ const authorizationMiddleware = getAuthorizationMiddleware({ identityAuthService, usersService, isomerAdminsService, + collaboratorsService, }) const authenticatedSubrouterV1 = getAuthenticatedSubrouterV1({ @@ -113,6 +123,8 @@ const authenticatedSubrouterV2 = getAuthenticatedSubrouter({ reposService, deploymentsService, isomerAdminsService, + collaboratorsService, + authorizationMiddleware, }) const authenticatedSitesSubrouterV2 = getAuthenticatedSitesSubrouter({ authorizationMiddleware, diff --git a/src/services/identity/CollaboratorsService.ts b/src/services/identity/CollaboratorsService.ts new file mode 100644 index 000000000..6035695e9 --- /dev/null +++ b/src/services/identity/CollaboratorsService.ts @@ -0,0 +1,240 @@ +import _ from "lodash" +import { ModelStatic, Op } from "sequelize" +import validator from "validator" + +import { ForbiddenError } from "@errors/ForbiddenError" +import { NotFoundError } from "@errors/NotFoundError" +import { UnprocessableError } from "@errors/UnprocessableError" + +import { CollaboratorRoles } from "@constants/constants" + +import { Whitelist, User, Site, SiteMember } from "@database/models" +import { BadRequestError } from "@root/errors/BadRequestError" +import { ConflictError } from "@root/errors/ConflictError" +import logger from "@root/logger/logger" + +import SitesService from "./SitesService" +import UsersService from "./UsersService" + +interface CollaboratorsServiceProps { + siteRepository: ModelStatic + siteMemberRepository: ModelStatic + sitesService: SitesService + usersService: UsersService + whitelist: ModelStatic +} + +class CollaboratorsService { + // NOTE: Explicitly specifying using keyed properties to ensure + // that the types are synced. + private readonly siteRepository: CollaboratorsServiceProps["siteRepository"] + + private readonly siteMemberRepository: CollaboratorsServiceProps["siteMemberRepository"] + + private readonly sitesService: CollaboratorsServiceProps["sitesService"] + + private readonly usersService: CollaboratorsServiceProps["usersService"] + + private readonly whitelist: CollaboratorsServiceProps["whitelist"] + + constructor({ + siteRepository, + siteMemberRepository, + sitesService, + usersService, + whitelist, + }: CollaboratorsServiceProps) { + this.siteRepository = siteRepository + this.siteMemberRepository = siteMemberRepository + this.sitesService = sitesService + this.usersService = usersService + this.whitelist = whitelist + } + + deriveAllowedRoleFromEmail = async (fullEmail: string) => { + const whitelistEntries = await this.whitelist.findAll({ + where: { + expiry: { + [Op.or]: [{ [Op.is]: null }, { [Op.gt]: new Date() }], + }, + }, + }) + + const matchedDomains = whitelistEntries.filter((entry) => + fullEmail.endsWith(entry.email) + ) + + if (!matchedDomains.length) return null + + // TODO: Modify this method because the presence of the expiry field is not + // the best way of differentiating Admin/Contributor roles + return matchedDomains[0].expiry + ? CollaboratorRoles.Contributor + : CollaboratorRoles.Admin + } + + list = async (siteName: string, requesterId?: string) => { + // Note: + // =============================================== + // We need to query from the Sites table instead of the SiteMembers table + // because Sequelize only recognizes that there is a relationship between Sites <-> Users. + // This means that we cannot retrieve joins if we start the query in the SiteMembers table. + // + // However, the converse is possible, i.e. we can query the Sites table and retrieve joined + // records from the Users table, along with the SiteMember records. + const site = await this.siteRepository.findOne({ + where: { name: siteName }, + include: [ + { + model: User, + as: "site_members", + }, + ], + }) + const collaborators = site?.site_members ?? [] + + // We perform the following sort via application code because: + // - sorting it via the ORM code alone is quite complicated + // - putting the sorting logic into a stored SQL function involves DB migration work + // - we can achieve this easily with lodash, and there is unlikely to be a performance hit + // given the small number of collaborators in each site + return _.orderBy( + collaborators, + [ + // Prioritize Admins over Contributors + ( + collaborator: User & { + SiteMember: SiteMember + } + ) => collaborator.SiteMember.role === CollaboratorRoles.Admin, + // Prioritize elements where the userId matches the requesterId (i.e. "you") + ( + collaborator: User & { + SiteMember: SiteMember + } + ) => collaborator.id.toString() === requesterId, + // Prioritize the user that has not logged in for the longest time + ( + collaborator: User & { + SiteMember: SiteMember + } + ) => collaborator.lastLoggedIn, + ], + ["desc", "desc", "asc"] + ) + } + + create = async (siteName: string, email: string, acknowledged: boolean) => { + if (!email || !validator.isEmail(email)) { + return new BadRequestError( + "That doesn't look like a valid email. Try a gov.sg or other whitelisted email." + ) + } + + // 1. Check if email address is whitelisted, and derive the collaborator role + const derivedRole = await this.deriveAllowedRoleFromEmail(email) + if (!derivedRole) { + // Error - the user email is not whitelisted + logger.error( + `create collaborators error: user email ${email} is not whitelisted` + ) + return new ForbiddenError( + `This collaborator couldn't be added. Visit our guide for more assistance.` + ) + } + + // 2. Check if site exists + const site = await this.sitesService.getBySiteName(siteName) + if (!site) { + // Error - site does not exist + logger.error(`create collaborators error: site ${siteName} is not valid`) + return new NotFoundError(`Site does not exist`) + } + + // 3. Check if valid user exists + const user = await this.usersService.findByEmail(email) + if (!user) { + // Error - user with a valid gov email does not exist + logger.error(`create collaborators error: user ${email} is not valid`) + return new NotFoundError( + `This user does not have an Isomer account. Ask them to log in to Isomer and try adding them again.` + ) + } + + // 4. Check if user is already a site member + const existingSiteMember = await this.siteMemberRepository.findOne({ + where: { + siteId: site.id, + userId: user.id, + }, + }) + if (existingSiteMember) { + return new ConflictError(`User is already a member of the site`) + } + + // 5. Ensure that acknowledgement is true if the email role is contributor + if (derivedRole === CollaboratorRoles.Contributor && !acknowledged) { + return new UnprocessableError("Acknowledgement required") + } + + // 6. Create the SiteMembers record + return this.siteMemberRepository.create({ + siteId: site.id, + userId: user.id, + role: derivedRole, + }) + } + + delete = async (siteName: string, userId: string) => { + const site = await this.siteRepository.findOne({ + where: { name: siteName }, + include: [ + { + model: User, + as: "site_members", + }, + ], + }) + + const siteMembers = site?.site_members ?? [] + const isUserSiteMember = + _.filter(siteMembers, (member) => member.id.toString() === userId) + .length > 0 + if (!isUserSiteMember) { + return new NotFoundError(`User is not a site member`) + } + + const siteAdmins = siteMembers.filter( + (member) => member.SiteMember.role === CollaboratorRoles.Admin + ) + if ( + siteAdmins.length === 1 && + siteAdmins[0].id.toString() === userId // Required to check if the collaborator being deleted is an admin + ) { + return new UnprocessableError(`Cannot delete final site admin`) + } + + return this.siteMemberRepository.destroy({ + where: { siteId: site?.id, userId }, + }) + } + + getRole = async (siteName: string, userId: string) => { + const site = await this.siteRepository.findOne({ + where: { name: siteName }, + include: [ + { + model: User, + as: "site_members", + where: { + id: userId, + }, + }, + ], + }) + + return (site?.site_members?.[0]?.SiteMember?.role as string | null) ?? null + } +} + +export default CollaboratorsService diff --git a/src/services/identity/__tests__/CollaboratorsService.spec.ts b/src/services/identity/__tests__/CollaboratorsService.spec.ts new file mode 100644 index 000000000..b90208ff8 --- /dev/null +++ b/src/services/identity/__tests__/CollaboratorsService.spec.ts @@ -0,0 +1,476 @@ +import { ModelStatic } from "sequelize" + +import { ForbiddenError } from "@errors/ForbiddenError" +import { NotFoundError } from "@errors/NotFoundError" +import { UnprocessableError } from "@errors/UnprocessableError" + +import { Site, SiteMember, Whitelist } from "@database/models" +import { + expectedSortedMockCollaboratorsList, + mockSiteOrmResponseWithAllCollaborators, + mockSiteOrmResponseWithOneAdminCollaborator, + mockSiteOrmResponseWithOneContributorCollaborator, + mockSiteOrmResponseWithNoCollaborators, +} from "@fixtures/identity" +import { CollaboratorRoles } from "@root/constants" +import { BadRequestError } from "@root/errors/BadRequestError" +import { ConflictError } from "@root/errors/ConflictError" +import CollaboratorsService from "@services/identity/CollaboratorsService" +import SitesService from "@services/identity/SitesService" +import UsersService from "@services/identity/UsersService" + +describe("CollaboratorsService", () => { + const mockSiteName = "sitename" + const mockEmailAddress = "test1@test.gov.sg" + const mockSiteId = 1 + const mockUserId = "2" + const mockWhitelistId = 3 + const mockSiteRepo = { + findOne: jest.fn(), + } + const mockSiteMemberRepo = { + destroy: jest.fn(), + findOne: jest.fn(), + create: jest.fn(), + } + const mockWhitelistRepo = { + findAll: jest.fn(), + } + + const mockSitesService = { + getBySiteName: jest.fn(), + } + const mockUsersService = { + findByEmail: jest.fn(), + } + + const collaboratorsService = new CollaboratorsService({ + siteRepository: (mockSiteRepo as unknown) as ModelStatic, + siteMemberRepository: (mockSiteMemberRepo as unknown) as ModelStatic, + sitesService: (mockSitesService as unknown) as SitesService, + usersService: (mockUsersService as unknown) as UsersService, + whitelist: (mockWhitelistRepo as unknown) as ModelStatic, + }) + + // Prevent inter-test pollution of mocks + afterEach(() => jest.clearAllMocks()) + + describe("deriveAllowedRoleFromEmail", () => { + it("should derive admin role for valid admin-eligible emails", async () => { + // Arrange + const mockWhitelistEntries = [ + { + id: mockWhitelistId, + email: mockEmailAddress, + expiry: null, + createdAt: new Date(), + updatedAt: new Date(), + }, + ] + mockWhitelistRepo.findAll.mockResolvedValue( + (mockWhitelistEntries as unknown) as Whitelist[] + ) + + // Act + const role = await collaboratorsService.deriveAllowedRoleFromEmail( + mockEmailAddress + ) + + // Assert + expect(role).toStrictEqual(CollaboratorRoles.Admin) + expect(mockWhitelistRepo.findAll).toHaveBeenCalled() + }) + + it("should derive contributor role for valid contributor-eligible emails", async () => { + // Arrange + const mockWhitelistEntries = [ + { + id: mockWhitelistId, + email: mockEmailAddress, + expiry: new Date(), + createdAt: new Date(), + updatedAt: new Date(), + }, + ] + mockWhitelistRepo.findAll.mockResolvedValue( + (mockWhitelistEntries as unknown) as Whitelist[] + ) + + // Act + const role = await collaboratorsService.deriveAllowedRoleFromEmail( + mockEmailAddress + ) + + // Assert + expect(role).toStrictEqual(CollaboratorRoles.Contributor) + expect(mockWhitelistRepo.findAll).toHaveBeenCalled() + }) + + it("should derive no role for emails from non-whitelisted domains", async () => { + // Arrange + const mockWhitelistEntries: never[] = [] + mockWhitelistRepo.findAll.mockResolvedValue( + mockWhitelistEntries as Whitelist[] + ) + + // Act + const role = await collaboratorsService.deriveAllowedRoleFromEmail( + mockEmailAddress + ) + + // Assert + expect(role).toStrictEqual(null) + expect(mockWhitelistRepo.findAll).toHaveBeenCalled() + }) + }) + + describe("list", () => { + it("should list all collaborators in the correct sequence", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue( + mockSiteOrmResponseWithAllCollaborators + ) + + // Act + const collaborators = await collaboratorsService.list( + mockSiteName, + mockEmailAddress + ) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(collaborators).toStrictEqual(expectedSortedMockCollaboratorsList) + }) + + it("should return empty array if no collaborators are found", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue( + mockSiteOrmResponseWithNoCollaborators + ) + + // Act + const collaborators = await collaboratorsService.list(mockSiteName) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(collaborators).toStrictEqual([]) + }) + + it("should return empty array if no site with the specified id is found", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue([]) + + // Act + const collaborators = await collaboratorsService.list(mockSiteName) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(collaborators).toStrictEqual([]) + }) + }) + + describe("getRole", () => { + it("should retrieve correct admin role", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue( + mockSiteOrmResponseWithOneAdminCollaborator + ) + + // Act + const role = await collaboratorsService.getRole(mockSiteName, mockUserId) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(role).toStrictEqual(CollaboratorRoles.Admin) + }) + + it("should retrieve correct contributor role", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue( + mockSiteOrmResponseWithOneContributorCollaborator + ) + + // Act + const role = await collaboratorsService.getRole(mockSiteName, mockUserId) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(role).toStrictEqual(CollaboratorRoles.Contributor) + }) + + it("should retrieve correct null role if site has no collaborators", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue( + mockSiteOrmResponseWithNoCollaborators + ) + + // Act + const role = await collaboratorsService.getRole(mockSiteName, mockUserId) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(role).toStrictEqual(null) + }) + + it("should retrieve correct null role if site does not exist", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue([]) + + // Act + const role = await collaboratorsService.getRole(mockSiteName, mockUserId) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(role).toStrictEqual(null) + }) + }) + + describe("delete", () => { + it("should delete contributor", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue( + mockSiteOrmResponseWithAllCollaborators + ) + + // Act + await collaboratorsService.delete(mockSiteName, mockUserId) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(mockSiteMemberRepo.destroy).toHaveBeenCalled() + }) + + it("should throw error if user is not a member of the site", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue( + mockSiteOrmResponseWithNoCollaborators + ) + + // Act + const resp = await collaboratorsService.delete(mockSiteName, mockUserId) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(mockSiteMemberRepo.destroy).not.toHaveBeenCalled() + expect(resp instanceof NotFoundError).toBe(true) + }) + + it("should not delete admin if there is only one admin left", async () => { + // Arrange + mockSiteRepo.findOne.mockResolvedValue( + mockSiteOrmResponseWithOneAdminCollaborator + ) + + // Act + const resp = await collaboratorsService.delete(mockSiteName, mockUserId) + + // Assert + expect(mockSiteRepo.findOne).toHaveBeenCalled() + expect(mockSiteMemberRepo.destroy).not.toHaveBeenCalled() + expect(resp instanceof UnprocessableError).toBe(true) + }) + }) + + describe("create", () => { + const mockSiteMemberRecord = { + siteId: mockSiteId, + userId: mockUserId, + role: CollaboratorRoles.Contributor, + } + + it("should create contributor", async () => { + // Arrange + collaboratorsService.deriveAllowedRoleFromEmail = (jest.fn( + () => CollaboratorRoles.Admin + ) as unknown) as () => Promise + mockSitesService.getBySiteName.mockResolvedValue({ id: mockSiteId }) + mockUsersService.findByEmail.mockResolvedValue({ id: mockUserId }) + mockSiteMemberRepo.findOne.mockResolvedValue(null) + mockSiteMemberRepo.create.mockResolvedValue(mockSiteMemberRecord) + + // Act + const resp = await collaboratorsService.create( + mockSiteName, + mockEmailAddress, + true + ) + + // Assert + expect(collaboratorsService.deriveAllowedRoleFromEmail).toBeCalledWith( + mockEmailAddress + ) + expect(mockSitesService.getBySiteName).toBeCalledWith(mockSiteName) + expect(mockUsersService.findByEmail).toBeCalledWith(mockEmailAddress) + expect(mockSiteMemberRepo.findOne).toBeCalled() + expect(mockSiteMemberRepo.create).toBeCalled() + expect(resp).toStrictEqual(mockSiteMemberRecord) + }) + + it("should return error if email is malformed", async () => { + // Arrange + const MALFORMED_EMAIL = "test" + collaboratorsService.deriveAllowedRoleFromEmail = (jest.fn( + () => CollaboratorRoles.Admin + ) as unknown) as () => Promise + mockSitesService.getBySiteName.mockResolvedValue({ id: mockSiteId }) + mockUsersService.findByEmail.mockResolvedValue({ id: mockUserId }) + mockSiteMemberRepo.findOne.mockResolvedValue(null) + mockSiteMemberRepo.create.mockResolvedValue(mockSiteMemberRecord) + + // Act + const resp = await collaboratorsService.create( + mockSiteName, + MALFORMED_EMAIL, + false + ) + + // Assert + expect(collaboratorsService.deriveAllowedRoleFromEmail).not.toBeCalled() + expect(mockSitesService.getBySiteName).not.toBeCalledWith(mockSiteName) + expect(mockUsersService.findByEmail).not.toBeCalledWith(mockEmailAddress) + expect(mockSiteMemberRepo.findOne).not.toBeCalled() + expect(mockSiteMemberRepo.create).not.toBeCalled() + expect(resp instanceof BadRequestError).toBe(true) + }) + + it("should return error if email domain is not whitelisted", async () => { + // Arrange + collaboratorsService.deriveAllowedRoleFromEmail = (jest.fn( + () => null + ) as unknown) as () => Promise + mockSitesService.getBySiteName.mockResolvedValue({ id: mockSiteId }) + mockUsersService.findByEmail.mockResolvedValue({ id: mockUserId }) + mockSiteMemberRepo.findOne.mockResolvedValue(null) + mockSiteMemberRepo.create.mockResolvedValue(mockSiteMemberRecord) + + // Act + const resp = await collaboratorsService.create( + mockSiteName, + mockEmailAddress, + false + ) + + // Assert + expect(collaboratorsService.deriveAllowedRoleFromEmail).toBeCalledWith( + mockEmailAddress + ) + expect(mockSitesService.getBySiteName).not.toBeCalled() + expect(mockUsersService.findByEmail).not.toBeCalled() + expect(mockSiteMemberRepo.findOne).not.toBeCalled() + expect(mockSiteMemberRepo.create).not.toBeCalled() + expect(resp instanceof ForbiddenError).toBe(true) + }) + + it("should return error if site does not exist", async () => { + // Arrange + collaboratorsService.deriveAllowedRoleFromEmail = (jest.fn( + () => CollaboratorRoles.Admin + ) as unknown) as () => Promise + mockSitesService.getBySiteName.mockResolvedValue(null) + mockUsersService.findByEmail.mockResolvedValue({ id: mockUserId }) + mockSiteMemberRepo.findOne.mockResolvedValue(null) + mockSiteMemberRepo.create.mockResolvedValue(mockSiteMemberRecord) + + // Act + const resp = await collaboratorsService.create( + mockSiteName, + mockEmailAddress, + false + ) + + // Assert + expect(collaboratorsService.deriveAllowedRoleFromEmail).toBeCalledWith( + mockEmailAddress + ) + expect(mockSitesService.getBySiteName).toBeCalledWith(mockSiteName) + expect(mockUsersService.findByEmail).not.toBeCalled() + expect(mockSiteMemberRepo.findOne).not.toBeCalled() + expect(mockSiteMemberRepo.create).not.toBeCalled() + expect(resp instanceof NotFoundError).toBe(true) + }) + + it("should return error if user does not exist", async () => { + // Arrange + collaboratorsService.deriveAllowedRoleFromEmail = (jest.fn( + () => CollaboratorRoles.Admin + ) as unknown) as () => Promise + mockSitesService.getBySiteName.mockResolvedValue({ id: mockSiteId }) + mockUsersService.findByEmail.mockResolvedValue(null) + mockSiteMemberRepo.findOne.mockResolvedValue(null) + mockSiteMemberRepo.create.mockResolvedValue(mockSiteMemberRecord) + + // Act + const resp = await collaboratorsService.create( + mockSiteName, + mockEmailAddress, + false + ) + + // Assert + expect(collaboratorsService.deriveAllowedRoleFromEmail).toBeCalledWith( + mockEmailAddress + ) + expect(mockSitesService.getBySiteName).toBeCalledWith(mockSiteName) + expect(mockUsersService.findByEmail).toBeCalledWith(mockEmailAddress) + expect(mockSiteMemberRepo.findOne).not.toBeCalled() + expect(mockSiteMemberRepo.create).not.toBeCalled() + expect(resp instanceof NotFoundError).toBe(true) + }) + + it("should return error if user already is a site member", async () => { + // Arrange + collaboratorsService.deriveAllowedRoleFromEmail = (jest.fn( + () => CollaboratorRoles.Admin + ) as unknown) as () => Promise + mockSitesService.getBySiteName.mockResolvedValue({ id: mockSiteId }) + mockUsersService.findByEmail.mockResolvedValue({ id: mockUserId }) + mockSiteMemberRepo.findOne.mockResolvedValue(mockSiteMemberRecord) + mockSiteMemberRepo.create.mockResolvedValue(mockSiteMemberRecord) + + // Act + const resp = await collaboratorsService.create( + mockSiteName, + mockEmailAddress, + false + ) + + // Assert + expect(collaboratorsService.deriveAllowedRoleFromEmail).toBeCalledWith( + mockEmailAddress + ) + expect(mockSitesService.getBySiteName).toBeCalledWith(mockSiteName) + expect(mockUsersService.findByEmail).toBeCalledWith(mockEmailAddress) + expect(mockSiteMemberRepo.findOne).toBeCalled() + expect(mockSiteMemberRepo.create).not.toBeCalled() + expect(resp instanceof ConflictError).toBe(true) + }) + + it("should return error if acknowledgement is not done and if the user is going to be a contributor", async () => { + // Arrange + collaboratorsService.deriveAllowedRoleFromEmail = (jest.fn( + () => CollaboratorRoles.Contributor + ) as unknown) as () => Promise + mockSitesService.getBySiteName.mockResolvedValue({ id: mockSiteId }) + mockUsersService.findByEmail.mockResolvedValue({ id: mockUserId }) + mockSiteMemberRepo.findOne.mockResolvedValue(null) + mockSiteMemberRepo.create.mockResolvedValue(mockSiteMemberRecord) + + // Act + const resp = await collaboratorsService.create( + mockSiteName, + mockEmailAddress, + false + ) + + // Assert + expect(collaboratorsService.deriveAllowedRoleFromEmail).toBeCalledWith( + mockEmailAddress + ) + expect(mockSitesService.getBySiteName).toBeCalledWith(mockSiteName) + expect(mockUsersService.findByEmail).toBeCalledWith(mockEmailAddress) + expect(mockSiteMemberRepo.findOne).toBeCalled() + expect(mockSiteMemberRepo.create).not.toBeCalled() + expect(resp instanceof UnprocessableError).toBe(true) + }) + }) +}) diff --git a/src/services/middlewareServices/AuthorizationMiddlewareService.ts b/src/services/middlewareServices/AuthorizationMiddlewareService.ts index 2b964bf39..a2579fbc5 100644 --- a/src/services/middlewareServices/AuthorizationMiddlewareService.ts +++ b/src/services/middlewareServices/AuthorizationMiddlewareService.ts @@ -2,8 +2,10 @@ import { NotFoundError } from "@errors/NotFoundError" import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" -import { E2E_ISOMER_ID } from "@root/constants" +import { CollaboratorRoles, E2E_ISOMER_ID } from "@root/constants" +import { ForbiddenError } from "@root/errors/ForbiddenError" import AuthService from "@services/identity/AuthService" +import CollaboratorsService from "@services/identity/CollaboratorsService" import IsomerAdminsService from "@services/identity/IsomerAdminsService" import UsersService from "@services/identity/UsersService" @@ -14,6 +16,7 @@ interface AuthorizationMiddlewareServiceProps { identityAuthService: AuthService usersService: UsersService isomerAdminsService: IsomerAdminsService + collaboratorsService: CollaboratorsService } export default class AuthorizationMiddlewareService { @@ -23,38 +26,77 @@ export default class AuthorizationMiddlewareService { readonly isomerAdminsService: AuthorizationMiddlewareServiceProps["isomerAdminsService"] + readonly collaboratorsService: AuthorizationMiddlewareServiceProps["collaboratorsService"] + constructor({ identityAuthService, usersService, isomerAdminsService, + collaboratorsService, }: AuthorizationMiddlewareServiceProps) { this.identityAuthService = identityAuthService this.usersService = usersService this.isomerAdminsService = isomerAdminsService + this.collaboratorsService = collaboratorsService } - async checkIsSiteMember(sessionData: UserWithSiteSessionData) { + async doesUserHaveCollaboratorLevelAccess( + siteName: string, + userId: string, + collaboratorType: CollaboratorRoles + ) { + const collaboratorRole = await this.collaboratorsService.getRole( + siteName, + userId + ) + return collaboratorType === CollaboratorRoles.Admin + ? collaboratorRole === CollaboratorRoles.Admin + : collaboratorRole === CollaboratorRoles.Admin || + collaboratorRole === CollaboratorRoles.Contributor + } + + async checkIsSiteCollaborator( + sessionData: UserWithSiteSessionData, + collaboratorType: CollaboratorRoles + ) { // Check if user has access to site const { siteName, isomerUserId: userId } = sessionData // Should always be defined - authorization middleware only exists if siteName is defined - if (!siteName) throw Error("No site name in authorization middleware") + if (!siteName) { + logger.error("No site name in authorization middleware") + return new ForbiddenError() + } logger.info(`Verifying user's access to ${siteName}`) + const isSiteCollaboratorOfType = sessionData.isEmailUser() + ? await this.doesUserHaveCollaboratorLevelAccess( + siteName, + userId, + collaboratorType + ) + : await this.identityAuthService.hasAccessToSite(sessionData) + const isIsomerCoreAdmin = await this.isomerAdminsService.getByUserId(userId) const isE2EUser = userId === E2E_ISOMER_ID - if (isE2EUser) return - - const isSiteMember = await (sessionData.isEmailUser() - ? this.usersService.hasAccessToSite(userId, siteName) - : this.identityAuthService.hasAccessToSite(sessionData)) + if (!isSiteCollaboratorOfType && !isIsomerCoreAdmin && !isE2EUser) { + logger.error("Site does not exist") + return new ForbiddenError() + } - const isAdminUser = await this.isomerAdminsService.getByUserId(userId) + logger.info( + `User ${sessionData.isomerUserId} has ${collaboratorType} access to ${sessionData.siteName}` + ) + } - if (!isSiteMember && !isAdminUser && !isE2EUser) { - throw new NotFoundError("Site does not exist") - } + async checkIsSiteMember(sessionData: UserWithSiteSessionData) { + return this.checkIsSiteCollaborator( + sessionData, + CollaboratorRoles.Contributor + ) + } - logger.info(`User ${userId} has access to ${siteName}`) + async checkIsSiteAdmin(sessionData: UserWithSiteSessionData) { + return this.checkIsSiteCollaborator(sessionData, CollaboratorRoles.Admin) } } diff --git a/src/services/middlewareServices/__tests__/AuthorizationMiddlewareService.spec.ts b/src/services/middlewareServices/__tests__/AuthorizationMiddlewareService.spec.ts index 64c28bed2..3c38e7191 100644 --- a/src/services/middlewareServices/__tests__/AuthorizationMiddlewareService.spec.ts +++ b/src/services/middlewareServices/__tests__/AuthorizationMiddlewareService.spec.ts @@ -1,14 +1,13 @@ -import { rejects } from "assert" - -import { NotFoundError } from "@errors/NotFoundError" - import { mockUserWithSiteSessionData, mockIsomerUserId, mockSessionDataEmailUserWithSite, mockSiteName, } from "@fixtures/sessionData" +import { CollaboratorRoles } from "@root/constants" +import { ForbiddenError } from "@root/errors/ForbiddenError" import AuthService from "@root/services/identity/AuthService" +import CollaboratorsService from "@root/services/identity/CollaboratorsService" import IsomerAdminsService from "@root/services/identity/IsomerAdminsService" import UsersService from "@root/services/identity/UsersService" @@ -27,10 +26,15 @@ describe("Authorization Middleware Service", () => { getByUserId: jest.fn(), } + const mockCollaboratorsService = { + getRole: jest.fn(), + } + const service = new AuthorizationMiddlewareService({ identityAuthService: (mockIdentityAuthService as unknown) as AuthService, usersService: (mockUsersService as unknown) as UsersService, isomerAdminsService: (mockIsomerAdminsService as unknown) as IsomerAdminsService, + collaboratorsService: (mockCollaboratorsService as unknown) as CollaboratorsService, }) beforeEach(() => { @@ -41,17 +45,21 @@ describe("Authorization Middleware Service", () => { it("Allows access for email users with site access", async () => { // Arrange mockIsomerAdminsService.getByUserId.mockImplementationOnce(() => null) - mockUsersService.hasAccessToSite.mockImplementationOnce(() => true) + mockCollaboratorsService.getRole.mockImplementationOnce( + () => CollaboratorRoles.Contributor + ) // Act - const actual = service.checkIsSiteMember(mockSessionDataEmailUserWithSite) + const actual = await service.checkIsSiteMember( + mockSessionDataEmailUserWithSite + ) // Assert - await expect(actual).resolves.not.toThrow() + expect(actual instanceof ForbiddenError).toBe(false) expect(mockIdentityAuthService.hasAccessToSite).toHaveBeenCalledTimes(0) - expect(mockUsersService.hasAccessToSite).toHaveBeenCalledWith( - mockIsomerUserId, - mockSiteName + expect(mockCollaboratorsService.getRole).toHaveBeenCalledWith( + mockSiteName, + mockIsomerUserId ) expect(mockIsomerAdminsService.getByUserId).toHaveBeenCalledWith( mockIsomerUserId @@ -64,11 +72,13 @@ describe("Authorization Middleware Service", () => { mockIdentityAuthService.hasAccessToSite.mockImplementationOnce(() => true) // Act - const actual = service.checkIsSiteMember(mockUserWithSiteSessionData) + const actual = await service.checkIsSiteMember( + mockUserWithSiteSessionData + ) // Assert - await expect(actual).resolves.not.toThrow() - expect(mockUsersService.hasAccessToSite).toHaveBeenCalledTimes(0) + expect(actual instanceof ForbiddenError).toBe(false) + expect(mockCollaboratorsService.getRole).toHaveBeenCalledTimes(0) expect(mockIdentityAuthService.hasAccessToSite).toHaveBeenCalledWith( mockUserWithSiteSessionData ) @@ -82,17 +92,21 @@ describe("Authorization Middleware Service", () => { mockIsomerAdminsService.getByUserId.mockImplementationOnce( () => "adminObj" ) - mockUsersService.hasAccessToSite.mockImplementationOnce(() => false) + mockCollaboratorsService.getRole.mockImplementationOnce( + () => CollaboratorRoles.Admin + ) // Act - const actual = service.checkIsSiteMember(mockSessionDataEmailUserWithSite) + const actual = await service.checkIsSiteMember( + mockSessionDataEmailUserWithSite + ) // Assert - await expect(actual).resolves.not.toThrow() + expect(actual instanceof ForbiddenError).toBe(false) expect(mockIdentityAuthService.hasAccessToSite).toHaveBeenCalledTimes(0) - expect(mockUsersService.hasAccessToSite).toHaveBeenCalledWith( - mockIsomerUserId, - mockSiteName + expect(mockCollaboratorsService.getRole).toHaveBeenCalledWith( + mockSiteName, + mockIsomerUserId ) expect(mockIsomerAdminsService.getByUserId).toHaveBeenCalledWith( mockIsomerUserId @@ -102,17 +116,20 @@ describe("Authorization Middleware Service", () => { it("Throws error for users without site access", async () => { // Arrange mockIsomerAdminsService.getByUserId.mockImplementationOnce(() => null) - mockUsersService.hasAccessToSite.mockImplementationOnce(() => false) + mockCollaboratorsService.getRole.mockImplementationOnce(() => null) // Act - const actual = service.checkIsSiteMember(mockSessionDataEmailUserWithSite) + const actual = await service.checkIsSiteMember( + mockSessionDataEmailUserWithSite + ) // Assert - await expect(actual).rejects.toThrowError(NotFoundError) + expect(actual) + expect(actual instanceof ForbiddenError).toBe(true) expect(mockIdentityAuthService.hasAccessToSite).toHaveBeenCalledTimes(0) - expect(mockUsersService.hasAccessToSite).toHaveBeenCalledWith( - mockIsomerUserId, - mockSiteName + expect(mockCollaboratorsService.getRole).toHaveBeenCalledWith( + mockSiteName, + mockIsomerUserId ) expect(mockIsomerAdminsService.getByUserId).toHaveBeenCalledWith( mockIsomerUserId