From 4592234964392dfc09ebec5134351814c240bb61 Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Thu, 28 Jul 2022 13:36:31 +0800 Subject: [PATCH 01/24] Chore: migrate authmiddlewareservice to typescript --- .../AuthMiddlewareService.ts | 142 ++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 src/services/middlewareServices/AuthMiddlewareService.ts diff --git a/src/services/middlewareServices/AuthMiddlewareService.ts b/src/services/middlewareServices/AuthMiddlewareService.ts new file mode 100644 index 000000000..459aa8be7 --- /dev/null +++ b/src/services/middlewareServices/AuthMiddlewareService.ts @@ -0,0 +1,142 @@ +// Import logger +import logger from "@logger/logger" + +// Import errors +import { AuthError } from "@errors/AuthError" + +import jwtUtils from "@utils/jwt-utils" + +<<<<<<<< HEAD:src/services/middlewareServices/AuthenticationMiddlewareService.ts +import { E2E_ISOMER_ID } from "@root/constants" +import { BadRequestError } from "@root/errors/BadRequestError" +======== +import { BadRequestError } from "@root/errors/BadRequestError" + +import AuthService from "../identity/AuthService" +>>>>>>>> fe35784 (Chore: migrate authmiddlewareservice to typescript):src/services/middlewareServices/AuthMiddlewareService.ts + +const { E2E_TEST_REPO, E2E_TEST_SECRET, E2E_TEST_GH_TOKEN } = process.env +const E2E_TEST_USER = "e2e-test" +const E2E_TEST_EMAIL = "test@e2e" +const GENERAL_ACCESS_PATHS = [ + "/v1/sites", + "/v1/auth/whoami", + "/v2/sites", + "/v2/auth/whoami", +] + +<<<<<<<< HEAD:src/services/middlewareServices/AuthenticationMiddlewareService.ts +export default class AuthenticationMiddlewareService { + verifyE2E({ + cookies, + url, + }: { + cookies: { + isomercmsE2E?: string + } + url: string + }) { +======== +interface AuthMiddlewareServiceProps { + identityAuthService: AuthService +} + +export default class AuthMiddlewareService { + readonly identityAuthService: AuthMiddlewareServiceProps["identityAuthService"] + + constructor({ identityAuthService }: AuthMiddlewareServiceProps) { + this.identityAuthService = identityAuthService + } + + verifyE2E({ cookies, url }: { cookies: any; url: string }) { +>>>>>>>> fe35784 (Chore: migrate authmiddlewareservice to typescript):src/services/middlewareServices/AuthMiddlewareService.ts + const { isomercmsE2E } = cookies + const urlTokens = url.split("/") // urls take the form "/v1/sites//"" + + if (!isomercmsE2E) return false + + if (isomercmsE2E !== E2E_TEST_SECRET) throw new AuthError("Bad credentials") + + if (urlTokens.length < 3) throw new BadRequestError("Invalid path") + + // General access paths are allowed + if (GENERAL_ACCESS_PATHS.includes(url)) return true + + // Throw an error if accessing a repo other than e2e-test-repo + const repo = urlTokens[3] + if (repo !== E2E_TEST_REPO) + throw new AuthError(`E2E tests can only access the ${E2E_TEST_REPO} repo`) + + return true + } + +<<<<<<<< HEAD:src/services/middlewareServices/AuthenticationMiddlewareService.ts + verifyJwt({ + cookies, + url, + }: { + cookies: { + isomercms: string + isomercmsE2E?: string + } + url: string + }) { +======== + verifyJwt({ cookies, url }: { cookies: any; url: string }) { +>>>>>>>> fe35784 (Chore: migrate authmiddlewareservice to typescript):src/services/middlewareServices/AuthMiddlewareService.ts + const { isomercms } = cookies + const isValidE2E = this.verifyE2E({ cookies, url }) + + if (isValidE2E) { + const accessToken = E2E_TEST_GH_TOKEN + const githubId = E2E_TEST_USER + const isomerUserId = E2E_ISOMER_ID + const email = E2E_TEST_EMAIL + return { accessToken, githubId, isomerUserId, email } + } + if (!isomercms) { + logger.error(`Authentication error: JWT token expired. Url: ${url}`) + throw new AuthError(`JWT token has expired`) + } + try { + const { + access_token: retrievedToken, + user_id: githubId, + isomer_user_id: isomerUserId, + email, + } = jwtUtils.verifyToken(isomercms) + if (!isomerUserId) { + const notLoggedInError = new Error("User not logged in with email") + notLoggedInError.name = "NotLoggedInError" + throw notLoggedInError + } + const accessToken = retrievedToken + ? jwtUtils.decryptToken(retrievedToken) + : "" + return { accessToken, githubId, isomerUserId, email } + } catch (err) { + if (!(err instanceof Error)) { + // NOTE: If the error is of an unknown kind, we bubble it up the stack and block access. + throw err + } + // NOTE: Cookies aren't being logged here because they get caught as "Object object", which is not useful + // The cookies should be converted to a JSON struct before logging + if (err.name === "NotLoggedInError") { + logger.error( + `Authentication error: user not logged in with email. Url: ${url}` + ) + throw new AuthError( + `Authentication error: user not logged in with email` + ) + } else if (err.name === "TokenExpiredError") { + logger.error(`Authentication error: JWT token expired. Url: ${url}`) + throw new AuthError(`JWT token has expired`) + } else { + logger.error( + `Authentication error. Message: ${err.message} Url: ${url}` + ) + } + throw err + } + } +} From 705d2d8a8959a1825f168be503f4a72d332b0d96 Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Thu, 28 Jul 2022 18:45:20 +0800 Subject: [PATCH 02/24] Feat: add site retrieval for email and admin users --- src/services/utilServices/SitesService.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/services/utilServices/SitesService.js b/src/services/utilServices/SitesService.js index 8a2c75076..08e8d606b 100644 --- a/src/services/utilServices/SitesService.js +++ b/src/services/utilServices/SitesService.js @@ -95,6 +95,12 @@ class SitesService { isPrivate, } }) + .filter( + (repoData) => + isAdminUser || + !isEmailUser || + emailAccessibleSites.includes(repoData.repoName) + ) .filter( (repoData) => repoData.permissions.push === true && From d94183a8ff35dab5a4411801e8d584094219a720 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:22:22 +0800 Subject: [PATCH 03/24] build(devdeps): add lodash types --- package-lock.json | 6 ++++++ package.json | 1 + 2 files changed, 7 insertions(+) diff --git a/package-lock.json b/package-lock.json index e84efaf5c..a5e05e306 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4043,6 +4043,12 @@ "integrity": "sha1-7ihweulOEdK4J7y+UnC86n8+ce4=", "dev": true }, + "@types/lodash": { + "version": "4.14.182", + "resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.14.182.tgz", + "integrity": "sha512-/THyiqyQAP9AfARo4pF+aCGcyiQ94tX/Is2I7HofNRqoYLgN1PBoOWu2/zTA5zMxzP5EFutMtWtGAFRKUe961Q==", + "dev": true + }, "@types/long": { "version": "4.0.2", "resolved": "https://registry.npmjs.org/@types/long/-/long-4.0.2.tgz", diff --git a/package.json b/package.json index b28cc7279..0ef48041b 100644 --- a/package.json +++ b/package.json @@ -83,6 +83,7 @@ "@tsconfig/recommended": "^1.0.1", "@types/express": "^4.17.13", "@types/jest": "^27.4.1", + "@types/lodash": "^4.14.182", "@types/node": "^17.0.21", "@types/supertest": "^2.0.11", "@types/validator": "^13.7.1", From 00ec6911d499c9dd9e1b0afe5bf0d18ef3904419 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:22:46 +0800 Subject: [PATCH 04/24] fix(model): rectify db model definitions --- src/database/models/Site.ts | 5 +++-- src/database/models/SiteMember.ts | 2 +- src/database/models/User.ts | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/database/models/Site.ts b/src/database/models/Site.ts index 8ab687cec..9b421b18e 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 @@ -84,5 +85,5 @@ export class Site extends Model { @BelongsTo(() => User, { as: "site_creator", }) - creator!: User + site_creator!: User } diff --git a/src/database/models/SiteMember.ts b/src/database/models/SiteMember.ts index cf8a37f28..bc337a096 100644 --- a/src/database/models/SiteMember.ts +++ b/src/database/models/SiteMember.ts @@ -25,7 +25,7 @@ export class SiteMember extends Model { allowNull: false, type: DataType.ENUM("ADMIN", "USER"), }) - role!: boolean + role!: string @CreatedAt createdAt!: Date diff --git a/src/database/models/User.ts b/src/database/models/User.ts index e5eb64c6f..8514c4a7d 100644 --- a/src/database/models/User.ts +++ b/src/database/models/User.ts @@ -31,7 +31,7 @@ export class User extends Model { email?: string | null @Column({ - allowNull: true, + allowNull: false, unique: true, type: DataType.TEXT, validate: { @@ -67,7 +67,7 @@ export class User extends Model { through: () => SiteMember, as: "site_members", }) - sites!: Site[] + sites!: Array @HasMany(() => Site, { as: "sites_created", From 7528b75f97b9891a9f038f0573e816d2f92614c2 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:25:15 +0800 Subject: [PATCH 05/24] refactor: add message param to ForbiddenError --- src/errors/ForbiddenError.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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") } } From 0bdb767a0322a97f6201e8e75d224b6db2884cd2 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:26:25 +0800 Subject: [PATCH 06/24] fix(SitesService): fix type change from database model --- src/services/identity/SitesService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/identity/SitesService.ts b/src/services/identity/SitesService.ts index 7ed5b124c..3786ef108 100644 --- a/src/services/identity/SitesService.ts +++ b/src/services/identity/SitesService.ts @@ -33,7 +33,7 @@ class SitesService { createParams: Partial & { name: Site["name"] apiTokenName: Site["apiTokenName"] - creator: Site["creator"] + creator: Site["site_creator"] } ) { return this.repository.create(createParams) From 3262749483ec684f1d5d4a13d8e6885ad637b8d1 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:29:17 +0800 Subject: [PATCH 07/24] feat: add CollaboratorsService --- src/constants/constants.ts | 5 + src/services/identity/CollaboratorsService.ts | 239 ++++++++++++++++++ 2 files changed, 244 insertions(+) create mode 100644 src/services/identity/CollaboratorsService.ts diff --git a/src/constants/constants.ts b/src/constants/constants.ts index 32fceecce..064c9f242 100644 --- a/src/constants/constants.ts +++ b/src/constants/constants.ts @@ -11,3 +11,8 @@ export enum SiteStatus { } export const E2E_ISOMER_ID = "e2e-id" + +export enum CollaboratorRoles { + Admin = "ADMIN", + Contributor = "USER", +} diff --git a/src/services/identity/CollaboratorsService.ts b/src/services/identity/CollaboratorsService.ts new file mode 100644 index 000000000..5e367a6fe --- /dev/null +++ b/src/services/identity/CollaboratorsService.ts @@ -0,0 +1,239 @@ +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 last logged in user + ( + collaborator: User & { + SiteMember: SiteMember + } + ) => collaborator.lastLoggedIn, + ], + ["desc", "desc", "asc"] + ) + } + + create = async (siteName: string, email: string, acknowledged: boolean) => { + let site + let user + + 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 + 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 + 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 ?? [] + if ( + !siteMembers.filter((member) => member.id.toString() === userId).length + ) { + 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) { + 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 From f2a1ae237a9f0212a5c1c0f8f7e510660d414904 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:30:16 +0800 Subject: [PATCH 08/24] test: add tests for CollaboratorsService --- src/fixtures/identity.ts | 104 ++++ .../__tests__/CollaboratorsService.spec.ts | 476 ++++++++++++++++++ 2 files changed, 580 insertions(+) create mode 100644 src/services/identity/__tests__/CollaboratorsService.spec.ts diff --git a/src/fixtures/identity.ts b/src/fixtures/identity.ts index a4953d4a4..b420575c2 100644 --- a/src/fixtures/identity.ts +++ b/src/fixtures/identity.ts @@ -18,3 +18,107 @@ export const mockBearerTokenHeaders = { Authorization: `Bearer ${process.env.POSTMAN_API_KEY}`, }, } + +const mockCollaboratorContributor1 = { + id: "1", + email: "test1@test.gov.sg", + githubId: "test1", + contactNumber: "12331231", + lastLoggedIn: "2022-07-30T07:41:09.661Z", + createdAt: "2022-04-04T07:25:41.013Z", + updatedAt: "2022-07-30T07:41:09.662Z", + deletedAt: null, + SiteMember: { + userId: "1", + siteId: "16", + role: "CONTRIBUTOR", + createdAt: "2022-07-29T03:50:49.145Z", + updatedAt: "2022-07-29T03:50:49.145Z", + }, +} + +const mockCollaboratorAdmin1 = { + id: "2", + email: "test2@test.gov.sg", + githubId: "test2", + contactNumber: "12331232", + lastLoggedIn: "2022-07-30T07:41:09.661Z", + createdAt: "2022-04-04T07:25:41.013Z", + updatedAt: "2022-07-30T07:41:09.662Z", + deletedAt: null, + SiteMember: { + userId: "2", + siteId: "16", + role: "ADMIN", + createdAt: "2022-07-29T03:50:49.145Z", + updatedAt: "2022-07-29T03:50:49.145Z", + }, +} +const mockCollaboratorAdmin2 = { + id: "3", + email: "test3@test.gov.sg", + githubId: "test3", + contactNumber: "12331233", + lastLoggedIn: "2022-06-30T07:41:09.661Z", + createdAt: "2022-04-04T07:25:41.013Z", + updatedAt: "2022-07-30T07:41:09.662Z", + deletedAt: null, + SiteMember: { + userId: "3", + siteId: "16", + role: "ADMIN", + createdAt: "2022-07-29T03:50:49.145Z", + updatedAt: "2022-07-29T03:50:49.145Z", + }, +} +const mockCollaboratorContributor2 = { + id: "4", + email: "test4@test.gov.sg", + githubId: "test4", + contactNumber: "12331234", + lastLoggedIn: "2022-07-30T07:41:09.661Z", + createdAt: "2022-04-04T07:25:41.013Z", + updatedAt: "2022-07-30T07:41:09.662Z", + deletedAt: null, + SiteMember: { + userId: "4", + siteId: "16", + role: "CONTRIBUTOR", + createdAt: "2022-07-29T03:50:49.145Z", + updatedAt: "2022-07-29T03:50:49.145Z", + }, +} + +export const unsortedMockCollaboratorsList = [ + mockCollaboratorContributor1, + mockCollaboratorAdmin1, + mockCollaboratorAdmin2, + mockCollaboratorContributor2, +] + +export const expectedSortedMockCollaboratorsList = [ + mockCollaboratorAdmin2, + mockCollaboratorAdmin1, + mockCollaboratorContributor1, + mockCollaboratorContributor2, +] + +export const mockSiteOrmResponseWithAllCollaborators = { + id: 1, + name: "", + users: unsortedMockCollaboratorsList, +} +export const mockSiteOrmResponseWithOneAdminCollaborator = { + id: 1, + name: "", + users: [mockCollaboratorAdmin1], +} +export const mockSiteOrmResponseWithOneContributorCollaborator = { + id: 1, + name: "", + users: [mockCollaboratorContributor2], +} +export const mockSiteOrmResponseWithNoCollaborators = { + id: 1, + name: "", +} 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) + }) + }) +}) From 542d01fe277cf59cff6aec619aa08dfd01be8ec5 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:32:54 +0800 Subject: [PATCH 09/24] feat: use CollaboratorService in authorization middleware --- src/middleware/authorization.ts | 40 ++++++++++++--- src/middleware/index.ts | 4 ++ src/routes/v1/authenticatedSites/index.js | 2 +- src/routes/v2/authenticatedSites/index.js | 2 +- src/server.js | 12 +++++ .../AuthorizationMiddlewareService.ts | 50 ++++++++++++++++--- 6 files changed, 95 insertions(+), 15 deletions(-) diff --git a/src/middleware/authorization.ts b/src/middleware/authorization.ts index 777409aa2..2e36bef5f 100644 --- a/src/middleware/authorization.ts +++ b/src/middleware/authorization.ts @@ -1,8 +1,7 @@ import autoBind from "auto-bind" -import { NextFunction, Request, Response } from "express" import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" - +import { ForbiddenError } from "@errors/ForbiddenError" import { RequestHandler } from "@root/types" import AuthorizationMiddlewareService from "@services/middlewareServices/AuthorizationMiddlewareService" @@ -19,8 +18,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 +50,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..91400850b 100644 --- a/src/middleware/index.ts +++ b/src/middleware/index.ts @@ -9,6 +9,7 @@ import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" import { RequestHandler } from "@root/types" import IdentityAuthService from "@services/identity/AuthService" +import CollaboratorsService from "@root/services/identity/CollaboratorsService" import IsomerAdminsService from "@services/identity/IsomerAdminsService" import UsersService from "@services/identity/UsersService" import AuthenticationMiddlewareService from "@services/middlewareServices/AuthenticationMiddlewareService" @@ -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..499c75168 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.verifySiteAdmin) authenticatedSitesSubrouter.use("/pages", pagesRouter) authenticatedSitesSubrouter.use("/collections", collectionsRouter) diff --git a/src/routes/v2/authenticatedSites/index.js b/src/routes/v2/authenticatedSites/index.js index f7c40716d..3d17f6c53 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.verifySiteAdmin) 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/middlewareServices/AuthorizationMiddlewareService.ts b/src/services/middlewareServices/AuthorizationMiddlewareService.ts index 1a2594c7d..1c8f1d217 100644 --- a/src/services/middlewareServices/AuthorizationMiddlewareService.ts +++ b/src/services/middlewareServices/AuthorizationMiddlewareService.ts @@ -1,9 +1,11 @@ import { NotFoundError } from "@errors/NotFoundError" - +import { CollaboratorRoles } from "@root/constants" +import { ForbiddenError } from "@root/errors/ForbiddenError" import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" import { E2E_ISOMER_ID } from "@root/constants" 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,14 +26,18 @@ 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) { @@ -38,21 +45,52 @@ export default class AuthorizationMiddlewareService { 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 isSiteMember = await (sessionData.isEmailUser() - ? this.usersService.hasAccessToSite(userId, siteName) + ? (await this.collaboratorsService.getRole(siteName, userId)) !== null : this.identityAuthService.hasAccessToSite(sessionData)) - const isAdminUser = await this.isomerAdminsService.getByUserId(userId) + const isIsomerCoreAdmin = await this.isomerAdminsService.getByUserId(userId) const isE2EUser = userId === E2E_ISOMER_ID - if (!isSiteMember && !isAdminUser && !isE2EUser) { - throw new NotFoundError("Site does not exist") + if (!isSiteMember && !isIsomerCoreAdmin && !isE2EUser) { + logger.error("Site does not exist") + return new ForbiddenError() } logger.info(`User ${userId} has access to ${siteName}`) } + + async checkIsSiteAdmin(sessionData: UserWithSiteSessionData) { + // 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) { + logger.error("No site name in authorization middleware") + return new ForbiddenError() + } + + logger.info(`Verifying user's access to ${siteName}`) + + const isSiteAdmin = await (sessionData.isEmailUser() + ? (await this.collaboratorsService.getRole(siteName, userId)) === + CollaboratorRoles.Admin + : this.identityAuthService.hasAccessToSite(sessionData)) + const isIsomerCoreAdmin = await this.isomerAdminsService.getByUserId(userId) + + const isE2EUser = userId === E2E_ISOMER_ID + if (!isSiteAdmin && !isIsomerCoreAdmin && !isE2EUser) { + logger.error("Site does not exist") + return new ForbiddenError() + } + + logger.info(`User ${userId} has admin access to ${siteName}`) + } } From a8e385c5d8ba842bc7210fa16ce80b40b88047e9 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:33:06 +0800 Subject: [PATCH 10/24] test: add tests for authorization middleware --- src/middleware/__tests__/authorization.ts | 111 ++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 src/middleware/__tests__/authorization.ts diff --git a/src/middleware/__tests__/authorization.ts b/src/middleware/__tests__/authorization.ts new file mode 100644 index 000000000..1e53d525a --- /dev/null +++ b/src/middleware/__tests__/authorization.ts @@ -0,0 +1,111 @@ +import autoBind from "auto-bind" +import { NextFunction, Request, Response } from "express" +import { ParamsDictionary } from "express-serve-static-core" +import { ParsedQs } from "qs" + +import { ForbiddenError } from "@errors/ForbiddenError" + +import { AuthorizationMiddleware } from "@middleware/authorization" + +import { CollaboratorRoles } from "@root/constants" +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< + ParamsDictionary, + any, + any, + ParsedQs, + Record + > + const mockRes = ({ + locals: { + sessionData: { getIsomerUserId: jest.fn(() => TEST_ISOMER_USER_ID) }, + }, + } as unknown) as Response + 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()) + }) + }) +}) From e1b4889c189f5bd8f8289f51bae44ca1041e8757 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:33:26 +0800 Subject: [PATCH 11/24] feat: add CollaboratorsRouter --- src/routes/v2/authenticated/collaborators.ts | 135 +++++++++++++++++++ src/routes/v2/authenticated/index.js | 12 ++ 2 files changed, 147 insertions(+) create mode 100644 src/routes/v2/authenticated/collaborators.ts diff --git a/src/routes/v2/authenticated/collaborators.ts b/src/routes/v2/authenticated/collaborators.ts new file mode 100644 index 000000000..221af5118 --- /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 SessionData from "@classes/SessionData" + +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 }, + { sessionData: SessionData } + > = 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 }, + { sessionData: SessionData } + > = 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 }, + { sessionData: SessionData } + > = async (req, res) => { + const { siteName } = req.params + const { sessionData } = res.locals + const collaborators = await this.collaboratorsService.list( + siteName, + sessionData.getIsomerUserId() + ) + + return res.status(200).json({ collaborators }) + } + + getCollaboratorRole: RequestHandler< + never, + unknown, + never, + { siteName: string }, + { sessionData: SessionData } + > = async (req, res) => { + const { siteName } = req.params + const { sessionData } = res.locals + const role = await this.collaboratorsService.getRole( + siteName, + sessionData.getIsomerUserId() + ) + 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..fe6782edf 100644 --- a/src/routes/v2/authenticated/index.js +++ b/src/routes/v2/authenticated/index.js @@ -1,3 +1,4 @@ +import logger from "@root/logger/logger" import InfraService from "@services/infra/InfraService" const express = require("express") @@ -7,6 +8,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 +19,8 @@ const getAuthenticatedSubrouter = ({ configYmlService, usersService, isomerAdminsService, + collaboratorsService, + authorizationMiddleware, }) => { const sitesService = new SitesService({ gitHubService, @@ -27,6 +31,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 +42,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()) From eea564b56166aae3435db28f8627e0d57d71ab9f Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:33:38 +0800 Subject: [PATCH 12/24] test: add tests for CollaboratorsRouter --- .../__tests__/collaborators.spec.ts | 179 ++++++++++++++++++ 1 file changed, 179 insertions(+) create mode 100644 src/routes/v2/authenticated/__tests__/collaborators.spec.ts 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 + ) + }) + }) +}) From 739f0bb6f56c6de8415ffe95103c75f5722dcf19 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 13:55:35 +0800 Subject: [PATCH 13/24] feat(db-migration): change site_members role enum in the database --- src/constants/constants.ts | 2 +- .../20220811070630-change-role-enum.js | 59 +++++++++++++++++++ src/database/models/SiteMember.ts | 2 +- 3 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 src/database/migrations/20220811070630-change-role-enum.js diff --git a/src/constants/constants.ts b/src/constants/constants.ts index 064c9f242..bfd520aae 100644 --- a/src/constants/constants.ts +++ b/src/constants/constants.ts @@ -14,5 +14,5 @@ export const E2E_ISOMER_ID = "e2e-id" export enum CollaboratorRoles { Admin = "ADMIN", - Contributor = "USER", + Contributor = "CONTRIBUTOR", } 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/SiteMember.ts b/src/database/models/SiteMember.ts index bc337a096..11b9c57c3 100644 --- a/src/database/models/SiteMember.ts +++ b/src/database/models/SiteMember.ts @@ -23,7 +23,7 @@ export class SiteMember extends Model { @Column({ allowNull: false, - type: DataType.ENUM("ADMIN", "USER"), + type: DataType.ENUM("ADMIN", "CONTRIBUTOR"), }) role!: string From 4f68c34b14551b44fb1664fa3e2abb825c3d528e Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 11 Aug 2022 15:28:12 +0800 Subject: [PATCH 14/24] fix(models): undo erroneous changes --- src/database/models/Site.ts | 4 ++-- src/database/models/User.ts | 2 +- src/services/identity/CollaboratorsService.ts | 6 +++--- src/services/identity/SitesService.ts | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/database/models/Site.ts b/src/database/models/Site.ts index 9b421b18e..b97fc6b34 100644 --- a/src/database/models/Site.ts +++ b/src/database/models/Site.ts @@ -71,7 +71,7 @@ export class Site extends Model { through: () => SiteMember, as: "site_members", }) - site_members!: Array + users!: Array @HasOne(() => Repo) repo?: Repo @@ -85,5 +85,5 @@ export class Site extends Model { @BelongsTo(() => User, { as: "site_creator", }) - site_creator!: User + creator!: User } diff --git a/src/database/models/User.ts b/src/database/models/User.ts index 8514c4a7d..185ccc0ed 100644 --- a/src/database/models/User.ts +++ b/src/database/models/User.ts @@ -31,7 +31,7 @@ export class User extends Model { email?: string | null @Column({ - allowNull: false, + allowNull: true, unique: true, type: DataType.TEXT, validate: { diff --git a/src/services/identity/CollaboratorsService.ts b/src/services/identity/CollaboratorsService.ts index 5e367a6fe..aba5c08c5 100644 --- a/src/services/identity/CollaboratorsService.ts +++ b/src/services/identity/CollaboratorsService.ts @@ -91,7 +91,7 @@ class CollaboratorsService { }, ], }) - const collaborators = site?.site_members ?? [] + const collaborators = site?.users ?? [] // We perform the following sort via application code because: // - sorting it via the ORM code alone is quite complicated @@ -199,7 +199,7 @@ class CollaboratorsService { ], }) - const siteMembers = site?.site_members ?? [] + const siteMembers = site?.users ?? [] if ( !siteMembers.filter((member) => member.id.toString() === userId).length ) { @@ -232,7 +232,7 @@ class CollaboratorsService { ], }) - return (site?.site_members?.[0]?.SiteMember?.role as string | null) ?? null + return (site?.users?.[0]?.SiteMember?.role as string | null) ?? null } } diff --git a/src/services/identity/SitesService.ts b/src/services/identity/SitesService.ts index 3786ef108..7ed5b124c 100644 --- a/src/services/identity/SitesService.ts +++ b/src/services/identity/SitesService.ts @@ -33,7 +33,7 @@ class SitesService { createParams: Partial & { name: Site["name"] apiTokenName: Site["apiTokenName"] - creator: Site["site_creator"] + creator: Site["creator"] } ) { return this.repository.create(createParams) From 4b700f020dc03bd8f2c768559b4ddd8bc73a85c4 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Mon, 15 Aug 2022 10:24:46 +0800 Subject: [PATCH 15/24] fix: model field and failing tests --- src/database/models/Site.ts | 2 +- src/fixtures/identity.ts | 10 +++++----- src/services/identity/CollaboratorsService.ts | 13 +++++++------ .../identity/__tests__/CollaboratorsService.spec.ts | 2 +- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/database/models/Site.ts b/src/database/models/Site.ts index b97fc6b34..43f9ce141 100644 --- a/src/database/models/Site.ts +++ b/src/database/models/Site.ts @@ -71,7 +71,7 @@ export class Site extends Model { through: () => SiteMember, as: "site_members", }) - users!: Array + site_members!: Array @HasOne(() => Repo) repo?: Repo diff --git a/src/fixtures/identity.ts b/src/fixtures/identity.ts index b420575c2..f8c945120 100644 --- a/src/fixtures/identity.ts +++ b/src/fixtures/identity.ts @@ -97,8 +97,8 @@ export const unsortedMockCollaboratorsList = [ ] export const expectedSortedMockCollaboratorsList = [ - mockCollaboratorAdmin2, mockCollaboratorAdmin1, + mockCollaboratorAdmin2, mockCollaboratorContributor1, mockCollaboratorContributor2, ] @@ -106,19 +106,19 @@ export const expectedSortedMockCollaboratorsList = [ export const mockSiteOrmResponseWithAllCollaborators = { id: 1, name: "", - users: unsortedMockCollaboratorsList, + site_members: unsortedMockCollaboratorsList, } export const mockSiteOrmResponseWithOneAdminCollaborator = { id: 1, name: "", - users: [mockCollaboratorAdmin1], + site_members: [mockCollaboratorAdmin1], } export const mockSiteOrmResponseWithOneContributorCollaborator = { id: 1, name: "", - users: [mockCollaboratorContributor2], + site_members: [mockCollaboratorContributor2], } export const mockSiteOrmResponseWithNoCollaborators = { id: 1, - name: "", + site_members: "", } diff --git a/src/services/identity/CollaboratorsService.ts b/src/services/identity/CollaboratorsService.ts index aba5c08c5..aa1c53439 100644 --- a/src/services/identity/CollaboratorsService.ts +++ b/src/services/identity/CollaboratorsService.ts @@ -91,7 +91,7 @@ class CollaboratorsService { }, ], }) - const collaborators = site?.users ?? [] + const collaborators = site?.site_members ?? [] // We perform the following sort via application code because: // - sorting it via the ORM code alone is quite complicated @@ -199,10 +199,11 @@ class CollaboratorsService { ], }) - const siteMembers = site?.users ?? [] - if ( - !siteMembers.filter((member) => member.id.toString() === userId).length - ) { + const siteMembers = site?.site_members ?? [] + const userIsSiteMember = + _.filter(siteMembers, (member) => member.id.toString() === userId) + .length > 0 + if (!userIsSiteMember) { return new NotFoundError(`User is not a site member`) } @@ -232,7 +233,7 @@ class CollaboratorsService { ], }) - return (site?.users?.[0]?.SiteMember?.role as string | null) ?? null + return (site?.site_members?.[0]?.SiteMember?.role as string | null) ?? null } } diff --git a/src/services/identity/__tests__/CollaboratorsService.spec.ts b/src/services/identity/__tests__/CollaboratorsService.spec.ts index b90208ff8..6c80643de 100644 --- a/src/services/identity/__tests__/CollaboratorsService.spec.ts +++ b/src/services/identity/__tests__/CollaboratorsService.spec.ts @@ -134,7 +134,7 @@ describe("CollaboratorsService", () => { // Act const collaborators = await collaboratorsService.list( mockSiteName, - mockEmailAddress + mockUserId ) // Assert From bd377a0249dacce457795df21d1a4a65bc82a087 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Wed, 24 Aug 2022 14:31:25 +0800 Subject: [PATCH 16/24] fix: clean up painful rebase --- src/middleware/__tests__/authorization.ts | 20 +-- src/routes/v2/authenticated/collaborators.ts | 18 +-- .../AuthMiddlewareService.ts | 142 ------------------ .../AuthorizationMiddlewareService.spec.ts | 67 ++++++--- src/services/utilServices/SitesService.js | 6 - 5 files changed, 61 insertions(+), 192 deletions(-) delete mode 100644 src/services/middlewareServices/AuthMiddlewareService.ts diff --git a/src/middleware/__tests__/authorization.ts b/src/middleware/__tests__/authorization.ts index 1e53d525a..14f254f11 100644 --- a/src/middleware/__tests__/authorization.ts +++ b/src/middleware/__tests__/authorization.ts @@ -1,13 +1,10 @@ -import autoBind from "auto-bind" import { NextFunction, Request, Response } from "express" -import { ParamsDictionary } from "express-serve-static-core" -import { ParsedQs } from "qs" import { ForbiddenError } from "@errors/ForbiddenError" import { AuthorizationMiddleware } from "@middleware/authorization" -import { CollaboratorRoles } from "@root/constants" +import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" import AuthorizationMiddlewareService from "@root/services/middlewareServices/AuthorizationMiddlewareService" describe("Authorization middleware", () => { @@ -20,17 +17,20 @@ describe("Authorization middleware", () => { const mockReq = ({ params: { siteName: TEST_SITE_NAME }, } as unknown) as Request< - ParamsDictionary, - any, - any, - ParsedQs, - Record + never, + unknown, + unknown, + never, + { userWithSiteSessionData: UserWithSiteSessionData } > const mockRes = ({ locals: { sessionData: { getIsomerUserId: jest.fn(() => TEST_ISOMER_USER_ID) }, }, - } as unknown) as Response + } as unknown) as Response< + unknown, + { userWithSiteSessionData: UserWithSiteSessionData } + > const mockNext = jest.fn() as NextFunction const authorizationMiddleware = new AuthorizationMiddleware({ diff --git a/src/routes/v2/authenticated/collaborators.ts b/src/routes/v2/authenticated/collaborators.ts index 221af5118..25b97317f 100644 --- a/src/routes/v2/authenticated/collaborators.ts +++ b/src/routes/v2/authenticated/collaborators.ts @@ -4,7 +4,7 @@ import express from "express" import { AuthorizationMiddleware } from "@middleware/authorization" import { attachReadRouteHandlerWrapper } from "@middleware/routeHandler" -import SessionData from "@classes/SessionData" +import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" import { BaseIsomerError } from "@root/errors/BaseError" import { attachSiteHandler } from "@root/middleware" @@ -36,7 +36,7 @@ export class CollaboratorsRouter { unknown, { email: string; acknowledge?: boolean }, { siteName: string }, - { sessionData: SessionData } + { userWithSiteSessionData: UserWithSiteSessionData } > = async (req, res) => { const { email, acknowledge = false } = req.body const { siteName } = req.params @@ -58,7 +58,7 @@ export class CollaboratorsRouter { unknown, never, { siteName: string; userId: string }, - { sessionData: SessionData } + { userWithSiteSessionData: UserWithSiteSessionData } > = async (req, res) => { const { siteName, userId } = req.params const resp = await this.collaboratorsService.delete(siteName, userId) @@ -75,13 +75,13 @@ export class CollaboratorsRouter { unknown, never, { siteName: string }, - { sessionData: SessionData } + { userWithSiteSessionData: UserWithSiteSessionData } > = async (req, res) => { const { siteName } = req.params - const { sessionData } = res.locals + const { userWithSiteSessionData } = res.locals const collaborators = await this.collaboratorsService.list( siteName, - sessionData.getIsomerUserId() + userWithSiteSessionData.isomerUserId ) return res.status(200).json({ collaborators }) @@ -92,13 +92,13 @@ export class CollaboratorsRouter { unknown, never, { siteName: string }, - { sessionData: SessionData } + { userWithSiteSessionData: UserWithSiteSessionData } > = async (req, res) => { const { siteName } = req.params - const { sessionData } = res.locals + const { userWithSiteSessionData } = res.locals const role = await this.collaboratorsService.getRole( siteName, - sessionData.getIsomerUserId() + userWithSiteSessionData.isomerUserId ) return res.status(200).json({ role }) } diff --git a/src/services/middlewareServices/AuthMiddlewareService.ts b/src/services/middlewareServices/AuthMiddlewareService.ts deleted file mode 100644 index 459aa8be7..000000000 --- a/src/services/middlewareServices/AuthMiddlewareService.ts +++ /dev/null @@ -1,142 +0,0 @@ -// Import logger -import logger from "@logger/logger" - -// Import errors -import { AuthError } from "@errors/AuthError" - -import jwtUtils from "@utils/jwt-utils" - -<<<<<<<< HEAD:src/services/middlewareServices/AuthenticationMiddlewareService.ts -import { E2E_ISOMER_ID } from "@root/constants" -import { BadRequestError } from "@root/errors/BadRequestError" -======== -import { BadRequestError } from "@root/errors/BadRequestError" - -import AuthService from "../identity/AuthService" ->>>>>>>> fe35784 (Chore: migrate authmiddlewareservice to typescript):src/services/middlewareServices/AuthMiddlewareService.ts - -const { E2E_TEST_REPO, E2E_TEST_SECRET, E2E_TEST_GH_TOKEN } = process.env -const E2E_TEST_USER = "e2e-test" -const E2E_TEST_EMAIL = "test@e2e" -const GENERAL_ACCESS_PATHS = [ - "/v1/sites", - "/v1/auth/whoami", - "/v2/sites", - "/v2/auth/whoami", -] - -<<<<<<<< HEAD:src/services/middlewareServices/AuthenticationMiddlewareService.ts -export default class AuthenticationMiddlewareService { - verifyE2E({ - cookies, - url, - }: { - cookies: { - isomercmsE2E?: string - } - url: string - }) { -======== -interface AuthMiddlewareServiceProps { - identityAuthService: AuthService -} - -export default class AuthMiddlewareService { - readonly identityAuthService: AuthMiddlewareServiceProps["identityAuthService"] - - constructor({ identityAuthService }: AuthMiddlewareServiceProps) { - this.identityAuthService = identityAuthService - } - - verifyE2E({ cookies, url }: { cookies: any; url: string }) { ->>>>>>>> fe35784 (Chore: migrate authmiddlewareservice to typescript):src/services/middlewareServices/AuthMiddlewareService.ts - const { isomercmsE2E } = cookies - const urlTokens = url.split("/") // urls take the form "/v1/sites//"" - - if (!isomercmsE2E) return false - - if (isomercmsE2E !== E2E_TEST_SECRET) throw new AuthError("Bad credentials") - - if (urlTokens.length < 3) throw new BadRequestError("Invalid path") - - // General access paths are allowed - if (GENERAL_ACCESS_PATHS.includes(url)) return true - - // Throw an error if accessing a repo other than e2e-test-repo - const repo = urlTokens[3] - if (repo !== E2E_TEST_REPO) - throw new AuthError(`E2E tests can only access the ${E2E_TEST_REPO} repo`) - - return true - } - -<<<<<<<< HEAD:src/services/middlewareServices/AuthenticationMiddlewareService.ts - verifyJwt({ - cookies, - url, - }: { - cookies: { - isomercms: string - isomercmsE2E?: string - } - url: string - }) { -======== - verifyJwt({ cookies, url }: { cookies: any; url: string }) { ->>>>>>>> fe35784 (Chore: migrate authmiddlewareservice to typescript):src/services/middlewareServices/AuthMiddlewareService.ts - const { isomercms } = cookies - const isValidE2E = this.verifyE2E({ cookies, url }) - - if (isValidE2E) { - const accessToken = E2E_TEST_GH_TOKEN - const githubId = E2E_TEST_USER - const isomerUserId = E2E_ISOMER_ID - const email = E2E_TEST_EMAIL - return { accessToken, githubId, isomerUserId, email } - } - if (!isomercms) { - logger.error(`Authentication error: JWT token expired. Url: ${url}`) - throw new AuthError(`JWT token has expired`) - } - try { - const { - access_token: retrievedToken, - user_id: githubId, - isomer_user_id: isomerUserId, - email, - } = jwtUtils.verifyToken(isomercms) - if (!isomerUserId) { - const notLoggedInError = new Error("User not logged in with email") - notLoggedInError.name = "NotLoggedInError" - throw notLoggedInError - } - const accessToken = retrievedToken - ? jwtUtils.decryptToken(retrievedToken) - : "" - return { accessToken, githubId, isomerUserId, email } - } catch (err) { - if (!(err instanceof Error)) { - // NOTE: If the error is of an unknown kind, we bubble it up the stack and block access. - throw err - } - // NOTE: Cookies aren't being logged here because they get caught as "Object object", which is not useful - // The cookies should be converted to a JSON struct before logging - if (err.name === "NotLoggedInError") { - logger.error( - `Authentication error: user not logged in with email. Url: ${url}` - ) - throw new AuthError( - `Authentication error: user not logged in with email` - ) - } else if (err.name === "TokenExpiredError") { - logger.error(`Authentication error: JWT token expired. Url: ${url}`) - throw new AuthError(`JWT token has expired`) - } else { - logger.error( - `Authentication error. Message: ${err.message} Url: ${url}` - ) - } - throw err - } - } -} 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 diff --git a/src/services/utilServices/SitesService.js b/src/services/utilServices/SitesService.js index 08e8d606b..8a2c75076 100644 --- a/src/services/utilServices/SitesService.js +++ b/src/services/utilServices/SitesService.js @@ -95,12 +95,6 @@ class SitesService { isPrivate, } }) - .filter( - (repoData) => - isAdminUser || - !isEmailUser || - emailAccessibleSites.includes(repoData.repoName) - ) .filter( (repoData) => repoData.permissions.push === true && From a41412d6e642df46b1647fc6f1fd0f1bb9dcdccd Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 22 Sep 2022 10:34:41 +0800 Subject: [PATCH 17/24] chore: rename comment for clarity Co-authored-by: Hsu Zhong Jun <27919917+dcshzj@users.noreply.github.com> --- src/services/identity/CollaboratorsService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/identity/CollaboratorsService.ts b/src/services/identity/CollaboratorsService.ts index aa1c53439..1d802c5a5 100644 --- a/src/services/identity/CollaboratorsService.ts +++ b/src/services/identity/CollaboratorsService.ts @@ -113,7 +113,7 @@ class CollaboratorsService { SiteMember: SiteMember } ) => collaborator.id.toString() === requesterId, - // Prioritize the last logged in user + // Prioritize the user that has not logged in for the longest time ( collaborator: User & { SiteMember: SiteMember From ea4e7262de65af24740d5608b2392a47092ed39b Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 22 Sep 2022 10:38:59 +0800 Subject: [PATCH 18/24] chore: remove unnecessary imports --- src/routes/v2/authenticated/index.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/routes/v2/authenticated/index.js b/src/routes/v2/authenticated/index.js index fe6782edf..62ebdb028 100644 --- a/src/routes/v2/authenticated/index.js +++ b/src/routes/v2/authenticated/index.js @@ -1,6 +1,3 @@ -import logger from "@root/logger/logger" -import InfraService from "@services/infra/InfraService" - const express = require("express") const { From 92575fc8c92c44e9218447687e7e2af3e0b2caa5 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 22 Sep 2022 10:42:15 +0800 Subject: [PATCH 19/24] style: rename var to be more boolean-like --- src/services/identity/CollaboratorsService.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/identity/CollaboratorsService.ts b/src/services/identity/CollaboratorsService.ts index 1d802c5a5..6390530fa 100644 --- a/src/services/identity/CollaboratorsService.ts +++ b/src/services/identity/CollaboratorsService.ts @@ -200,10 +200,10 @@ class CollaboratorsService { }) const siteMembers = site?.site_members ?? [] - const userIsSiteMember = + const isUserSiteMember = _.filter(siteMembers, (member) => member.id.toString() === userId) .length > 0 - if (!userIsSiteMember) { + if (!isUserSiteMember) { return new NotFoundError(`User is not a site member`) } From b1b6a0031c4df0e0179161dda3e61a592c84c967 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 22 Sep 2022 10:44:02 +0800 Subject: [PATCH 20/24] style: define let vars as const --- src/services/identity/CollaboratorsService.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/services/identity/CollaboratorsService.ts b/src/services/identity/CollaboratorsService.ts index 6390530fa..71abec443 100644 --- a/src/services/identity/CollaboratorsService.ts +++ b/src/services/identity/CollaboratorsService.ts @@ -125,9 +125,6 @@ class CollaboratorsService { } create = async (siteName: string, email: string, acknowledged: boolean) => { - let site - let user - if (!email || !validator.isEmail(email)) { return new BadRequestError( "That doesn't look like a valid email. Try a gov.sg or other whitelisted email." @@ -147,7 +144,7 @@ class CollaboratorsService { } // 2. Check if site exists - site = await this.sitesService.getBySiteName(siteName) + const site = await this.sitesService.getBySiteName(siteName) if (!site) { // Error - site does not exist logger.error(`create collaborators error: site ${siteName} is not valid`) @@ -155,7 +152,7 @@ class CollaboratorsService { } // 3. Check if valid user exists - user = await this.usersService.findByEmail(email) + 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`) From cef758bc69caa8030618e55d707d165519f10652 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 22 Sep 2022 11:06:00 +0800 Subject: [PATCH 21/24] fix: use correct authZ method --- src/routes/v1/authenticatedSites/index.js | 2 +- src/routes/v2/authenticatedSites/index.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/routes/v1/authenticatedSites/index.js b/src/routes/v1/authenticatedSites/index.js index 499c75168..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.verifySiteAdmin) + authenticatedSitesSubrouter.use(authorizationMiddleware.verifySiteMember) authenticatedSitesSubrouter.use("/pages", pagesRouter) authenticatedSitesSubrouter.use("/collections", collectionsRouter) diff --git a/src/routes/v2/authenticatedSites/index.js b/src/routes/v2/authenticatedSites/index.js index 3d17f6c53..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.verifySiteAdmin) + authenticatedSitesSubrouter.use(authorizationMiddleware.verifySiteMember) authenticatedSitesSubrouter.use( "/collections/:collectionName", From 63bdf5d52d10601ac15e485f51a1c5dbe2a89638 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Thu, 22 Sep 2022 12:45:25 +0800 Subject: [PATCH 22/24] refactor: authZ middleware methods --- .../AuthorizationMiddlewareService.ts | 76 ++++++++++--------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/src/services/middlewareServices/AuthorizationMiddlewareService.ts b/src/services/middlewareServices/AuthorizationMiddlewareService.ts index 1c8f1d217..a2579fbc5 100644 --- a/src/services/middlewareServices/AuthorizationMiddlewareService.ts +++ b/src/services/middlewareServices/AuthorizationMiddlewareService.ts @@ -1,9 +1,9 @@ import { NotFoundError } from "@errors/NotFoundError" -import { CollaboratorRoles } from "@root/constants" -import { ForbiddenError } from "@root/errors/ForbiddenError" + 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" @@ -40,7 +40,25 @@ export default class AuthorizationMiddlewareService { 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 @@ -51,46 +69,34 @@ export default class AuthorizationMiddlewareService { } logger.info(`Verifying user's access to ${siteName}`) - - const isSiteMember = await (sessionData.isEmailUser() - ? (await this.collaboratorsService.getRole(siteName, userId)) !== null - : this.identityAuthService.hasAccessToSite(sessionData)) - + 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 (!isSiteMember && !isIsomerCoreAdmin && !isE2EUser) { + if (!isSiteCollaboratorOfType && !isIsomerCoreAdmin && !isE2EUser) { logger.error("Site does not exist") return new ForbiddenError() } - logger.info(`User ${userId} has access to ${siteName}`) + logger.info( + `User ${sessionData.isomerUserId} has ${collaboratorType} access to ${sessionData.siteName}` + ) } - async checkIsSiteAdmin(sessionData: UserWithSiteSessionData) { - // 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) { - logger.error("No site name in authorization middleware") - return new ForbiddenError() - } - - logger.info(`Verifying user's access to ${siteName}`) - - const isSiteAdmin = await (sessionData.isEmailUser() - ? (await this.collaboratorsService.getRole(siteName, userId)) === - CollaboratorRoles.Admin - : this.identityAuthService.hasAccessToSite(sessionData)) - const isIsomerCoreAdmin = await this.isomerAdminsService.getByUserId(userId) - - const isE2EUser = userId === E2E_ISOMER_ID - if (!isSiteAdmin && !isIsomerCoreAdmin && !isE2EUser) { - logger.error("Site does not exist") - return new ForbiddenError() - } + async checkIsSiteMember(sessionData: UserWithSiteSessionData) { + return this.checkIsSiteCollaborator( + sessionData, + CollaboratorRoles.Contributor + ) + } - logger.info(`User ${userId} has admin access to ${siteName}`) + async checkIsSiteAdmin(sessionData: UserWithSiteSessionData) { + return this.checkIsSiteCollaborator(sessionData, CollaboratorRoles.Admin) } } From 22b28a7beb04df45570657da5d2a218fa8f665e2 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Fri, 23 Sep 2022 12:52:00 +0800 Subject: [PATCH 23/24] chore: add types for collaborator mocks --- src/fixtures/identity.ts | 88 ++++++++++++++++++++++++---------------- 1 file changed, 52 insertions(+), 36 deletions(-) diff --git a/src/fixtures/identity.ts b/src/fixtures/identity.ts index f8c945120..68113a2e3 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" @@ -19,74 +23,86 @@ export const mockBearerTokenHeaders = { }, } -const mockCollaboratorContributor1 = { - id: "1", +const mockCollaboratorContributor1: Attributes & { + SiteMember: Attributes +} = { + id: 1, email: "test1@test.gov.sg", githubId: "test1", contactNumber: "12331231", - lastLoggedIn: "2022-07-30T07:41:09.661Z", - createdAt: "2022-04-04T07:25:41.013Z", - updatedAt: "2022-07-30T07:41:09.662Z", - deletedAt: null, + 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", + userId: 1, siteId: "16", role: "CONTRIBUTOR", - createdAt: "2022-07-29T03:50:49.145Z", - updatedAt: "2022-07-29T03:50:49.145Z", + createdAt: new Date("2022-07-29T03:50:49.145Z"), + updatedAt: new Date("2022-07-29T03:50:49.145Z"), }, + sites: [], } -const mockCollaboratorAdmin1 = { - id: "2", +const mockCollaboratorAdmin1: Attributes & { + SiteMember: Attributes +} = { + id: 2, email: "test2@test.gov.sg", githubId: "test2", contactNumber: "12331232", - lastLoggedIn: "2022-07-30T07:41:09.661Z", - createdAt: "2022-04-04T07:25:41.013Z", - updatedAt: "2022-07-30T07:41:09.662Z", - deletedAt: null, + 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", + userId: 2, siteId: "16", role: "ADMIN", - createdAt: "2022-07-29T03:50:49.145Z", - updatedAt: "2022-07-29T03:50:49.145Z", + createdAt: new Date("2022-07-29T03:50:49.145Z"), + updatedAt: new Date("2022-07-29T03:50:49.145Z"), }, + sites: [], } -const mockCollaboratorAdmin2 = { - id: "3", +const mockCollaboratorAdmin2: Attributes & { + SiteMember: Attributes +} = { + id: 3, email: "test3@test.gov.sg", githubId: "test3", contactNumber: "12331233", - lastLoggedIn: "2022-06-30T07:41:09.661Z", - createdAt: "2022-04-04T07:25:41.013Z", - updatedAt: "2022-07-30T07:41:09.662Z", - deletedAt: null, + 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", + userId: 3, siteId: "16", role: "ADMIN", - createdAt: "2022-07-29T03:50:49.145Z", - updatedAt: "2022-07-29T03:50:49.145Z", + createdAt: new Date("2022-07-29T03:50:49.145Z"), + updatedAt: new Date("2022-07-29T03:50:49.145Z"), }, + sites: [], } -const mockCollaboratorContributor2 = { - id: "4", +const mockCollaboratorContributor2: Attributes & { + SiteMember: Attributes +} = { + id: 4, email: "test4@test.gov.sg", githubId: "test4", contactNumber: "12331234", - lastLoggedIn: "2022-07-30T07:41:09.661Z", - createdAt: "2022-04-04T07:25:41.013Z", - updatedAt: "2022-07-30T07:41:09.662Z", - deletedAt: null, + 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", + userId: 4, siteId: "16", role: "CONTRIBUTOR", - createdAt: "2022-07-29T03:50:49.145Z", - updatedAt: "2022-07-29T03:50:49.145Z", + createdAt: new Date("2022-07-29T03:50:49.145Z"), + updatedAt: new Date("2022-07-29T03:50:49.145Z"), }, + sites: [], } export const unsortedMockCollaboratorsList = [ From bd1649e11c29cffb426322d50d56057b949e4148 Mon Sep 17 00:00:00 2001 From: Preston Lim Date: Wed, 28 Sep 2022 16:46:42 +0800 Subject: [PATCH 24/24] docs: add comment for deletion check --- src/services/identity/CollaboratorsService.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/services/identity/CollaboratorsService.ts b/src/services/identity/CollaboratorsService.ts index 71abec443..6035695e9 100644 --- a/src/services/identity/CollaboratorsService.ts +++ b/src/services/identity/CollaboratorsService.ts @@ -207,7 +207,10 @@ class CollaboratorsService { const siteAdmins = siteMembers.filter( (member) => member.SiteMember.role === CollaboratorRoles.Admin ) - if (siteAdmins.length === 1 && siteAdmins[0].id.toString() === userId) { + 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`) }