diff --git a/CHANGELOG.md b/CHANGELOG.md index ecd4dff09..5febabc55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,15 @@ All notable changes to this project will be documented in this file. Dates are d Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). +#### [v0.18.1](https://github.com/isomerpages/isomercms-backend/compare/v0.18.0...v0.18.1) + +- fix(review): return 200 for unmigrated sites [`bd69c29`](https://github.com/isomerpages/isomercms-backend/commit/bd69c29023554c5dcf8cb361227ba4ebf0d1ac08) +- fix(sanitize): use same setup for dompurify as FE [`c25f448`](https://github.com/isomerpages/isomercms-backend/commit/c25f448813e1addce67d0209375ec35ef9cb6c7b) + #### [v0.18.0](https://github.com/isomerpages/isomercms-backend/compare/v0.17.0...v0.18.0) +> 30 March 2023 + - feat(identity): phase 2 [`#509`](https://github.com/isomerpages/isomercms-backend/pull/509) - chore(docker compose): remove local emulation of lambdas [`#666`](https://github.com/isomerpages/isomercms-backend/pull/666) - Chore(Site launch microservices):managing cloud environments [`#657`](https://github.com/isomerpages/isomercms-backend/pull/657) @@ -141,15 +148,15 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - Feat/add access token table [`#441`](https://github.com/isomerpages/isomercms-backend/pull/441) - Refactor/consolidate misc GitHub requests [`#440`](https://github.com/isomerpages/isomercms-backend/pull/440) - 0.5.0 [`#434`](https://github.com/isomerpages/isomercms-backend/pull/434) -- Fix/migrate script [`#432`](https://github.com/isomerpages/isomercms-backend/pull/432) -- Feat/move whitelist into database [`#422`](https://github.com/isomerpages/isomercms-backend/pull/422) -- Refactor/use test fixture [`#430`](https://github.com/isomerpages/isomercms-backend/pull/430) -- Chore: remove duplicate validation in User model [`#429`](https://github.com/isomerpages/isomercms-backend/pull/429) #### [v0.5.0](https://github.com/isomerpages/isomercms-backend/compare/v0.4.0...v0.5.0) -> 14 April 2022 +> 21 April 2022 +- Fix/migrate script [`#432`](https://github.com/isomerpages/isomercms-backend/pull/432) +- Feat/move whitelist into database [`#422`](https://github.com/isomerpages/isomercms-backend/pull/422) +- Refactor/use test fixture [`#430`](https://github.com/isomerpages/isomercms-backend/pull/430) +- Chore: remove duplicate validation in User model [`#429`](https://github.com/isomerpages/isomercms-backend/pull/429) - build(deps-dev): bump eslint-config-prettier from 8.1.0 to 8.5.0 [`#371`](https://github.com/isomerpages/isomercms-backend/pull/371) - build(deps): bump async from 3.2.0 to 3.2.3 [`#424`](https://github.com/isomerpages/isomercms-backend/pull/424) - chore(mergify): change to lower case [`#425`](https://github.com/isomerpages/isomercms-backend/pull/425) diff --git a/package-lock.json b/package-lock.json index 92ed900d5..20c5b36de 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "isomercms", - "version": "0.18.0", + "version": "0.18.1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index ef11d6825..25eee45c9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "isomercms", - "version": "0.18.0", + "version": "0.18.1", "private": true, "scripts": { "build": "tsc -p tsconfig.build.json", diff --git a/src/integration/Reviews.spec.ts b/src/integration/Reviews.spec.ts index 5aa024117..cfbb48e10 100644 --- a/src/integration/Reviews.spec.ts +++ b/src/integration/Reviews.spec.ts @@ -113,13 +113,17 @@ const ReviewsRouter = new _ReviewsRouter( usersService, sitesService, collaboratorsService, - notificationsService + notificationsService, + gitHubService ) const reviewsSubrouter = ReviewsRouter.getRouter() const subrouter = express() subrouter.use("/:siteName", reviewsSubrouter) const mockGenericAxios = mockAxios.create() +const migrateSpy = jest + .spyOn(ReviewsRouter, "checkIfSiteIsUnmigrated") + .mockResolvedValue(true) describe("Review Requests Integration Tests", () => { beforeAll(async () => { @@ -477,6 +481,7 @@ describe("Review Requests Integration Tests", () => { MOCK_USER_SESSION_DATA_ONE, MOCK_REPO_NAME_TWO ) + migrateSpy.mockResolvedValueOnce(false) // Act const actual = await request(app).get(`/${MOCK_REPO_NAME_TWO}/summary`) @@ -722,6 +727,7 @@ describe("Review Requests Integration Tests", () => { MOCK_USER_SESSION_DATA_ONE, MOCK_REPO_NAME_TWO ) + migrateSpy.mockResolvedValueOnce(false) // Act const actual = await request(app).get( @@ -1617,6 +1623,7 @@ describe("Review Requests Integration Tests", () => { MOCK_USER_SESSION_DATA_TWO, MOCK_REPO_NAME_TWO ) + migrateSpy.mockResolvedValueOnce(false) // Act const actual = await request(app).get( diff --git a/src/routes/v2/authenticated/__tests__/review.spec.ts b/src/routes/v2/authenticated/__tests__/review.spec.ts index ba3cea684..6d9cecf53 100644 --- a/src/routes/v2/authenticated/__tests__/review.spec.ts +++ b/src/routes/v2/authenticated/__tests__/review.spec.ts @@ -11,6 +11,7 @@ import { generateRouterForDefaultUserWithSite } from "@fixtures/app" import { mockUserId } from "@fixtures/identity" import { MOCK_USER_EMAIL_ONE, MOCK_USER_EMAIL_TWO } from "@fixtures/users" import { CollaboratorRoles, ReviewRequestStatus } from "@root/constants" +import { GitHubService } from "@root/services/db/GitHubService" import CollaboratorsService from "@services/identity/CollaboratorsService" import NotificationsService from "@services/identity/NotificationsService" import SitesService from "@services/identity/SitesService" @@ -18,6 +19,9 @@ import UsersService from "@services/identity/UsersService" import ReviewRequestService from "@services/review/ReviewRequestService" describe("Review Requests Router", () => { + const mockGithubService = { + getRepoInfo: jest.fn().mockResolvedValue(true), + } const mockReviewRequestService = { approveReviewRequest: jest.fn(), closeReviewRequest: jest.fn(), @@ -60,7 +64,8 @@ describe("Review Requests Router", () => { (mockIdentityUsersService as unknown) as UsersService, (mockSitesService as unknown) as SitesService, (mockCollaboratorsService as unknown) as CollaboratorsService, - (mockNotificationsService as unknown) as NotificationsService + (mockNotificationsService as unknown) as NotificationsService, + (mockGithubService as unknown) as GitHubService ) const subrouter = express() @@ -138,6 +143,7 @@ describe("Review Requests Router", () => { mockReviewRequestService.compareDiff.mockResolvedValueOnce( mockFilesChanged ) + mockSitesService.getBySiteName.mockResolvedValueOnce(true) // Act const response = await request(app).get("/mockSite/review/compare") @@ -152,6 +158,7 @@ describe("Review Requests Router", () => { it("should return 404 if user is not a site member", async () => { // Arrange mockIdentityUsersService.getSiteMember.mockResolvedValueOnce(null) + mockSitesService.getBySiteName.mockResolvedValueOnce(true) // Act const response = await request(app).get("/mockSite/review/compare") @@ -335,6 +342,7 @@ describe("Review Requests Router", () => { it("should return 404 if the site does not exist", async () => { // Arrange mockSitesService.getBySiteName.mockResolvedValueOnce(null) + mockGithubService.getRepoInfo.mockRejectedValueOnce(false) // Act const response = await request(app).get("/mockSite/review/summary") @@ -519,6 +527,7 @@ describe("Review Requests Router", () => { it("should return 404 if the site does not exist", async () => { // Arrange mockSitesService.getBySiteName.mockResolvedValueOnce(null) + mockGithubService.getRepoInfo.mockRejectedValueOnce(false) // Act const response = await request(app).get(`/mockSite/review/12345`) @@ -913,6 +922,7 @@ describe("Review Requests Router", () => { it("should return 404 if the site does not exist", async () => { // Arrange mockSitesService.getBySiteName.mockResolvedValueOnce(null) + mockGithubService.getRepoInfo.mockRejectedValueOnce(false) // Act const response = await request(app).get(`/mockSite/review/12345/comments`) diff --git a/src/routes/v2/authenticated/review.ts b/src/routes/v2/authenticated/review.ts index ab69a363a..1ae3f98d3 100644 --- a/src/routes/v2/authenticated/review.ts +++ b/src/routes/v2/authenticated/review.ts @@ -14,6 +14,7 @@ import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" import { CollaboratorRoles, ReviewRequestStatus } from "@root/constants" import { SiteMember, User } from "@root/database/models" +import { GitHubService } from "@root/services/db/GitHubService" import CollaboratorsService from "@root/services/identity/CollaboratorsService" import NotificationsService from "@root/services/identity/NotificationsService" import SitesService from "@root/services/identity/SitesService" @@ -41,25 +42,50 @@ export class ReviewsRouter { private readonly notificationsService + private readonly githubService + constructor( reviewRequestService: ReviewRequestService, identityUsersService: UsersService, sitesService: SitesService, collaboratorsService: CollaboratorsService, - notificationsService: NotificationsService + notificationsService: NotificationsService, + githubService: GitHubService ) { this.reviewRequestService = reviewRequestService this.identityUsersService = identityUsersService this.sitesService = sitesService this.collaboratorsService = collaboratorsService this.notificationsService = notificationsService + this.githubService = githubService autoBind(this) } + // NOTE: This is required because we want to distinguish + // between unmigrated sites that don't exist in our db + // vs sites that truly don't exist. + // This is to avoid flooding the frontend with 404s + // and triggering false alarms + // TODO (IS-58): Remove this function and related calls when + // all sites are migrated over to our db + checkIfSiteIsUnmigrated = async ( + sessionData: UserWithSiteSessionData + ): Promise => { + try { + // Site exists on github but isn't migrated + await this.githubService.getRepoInfo(sessionData) + return true + } catch { + // If there are any errors, we failed to fetch the site + // and the site is assumed to not exist. + return false + } + } + compareDiff: RequestHandler< { siteName: string }, - { items: EditedItemDto[] }, + { items: EditedItemDto[] } | ResponseErrorBody, unknown, unknown, { userWithSiteSessionData: UserWithSiteSessionData } @@ -70,6 +96,21 @@ export class ReviewsRouter { const { userWithSiteSessionData } = res.locals const { siteName } = req.params + const site = await this.sitesService.getBySiteName(siteName) + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) + return res.status(200).json({ message: "Unmigrated site" }) + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } + // Check if they have access to site const possibleSiteMember = await this.identityUsersService.getSiteMember( userWithSiteSessionData.isomerUserId, @@ -77,7 +118,7 @@ export class ReviewsRouter { ) if (!possibleSiteMember) { - return res.status(404).send() + return res.status(404).json({ message: "No site members found" }) } const files = await this.reviewRequestService.compareDiff( @@ -226,6 +267,19 @@ export class ReviewsRouter { const { siteName } = req.params const site = await this.sitesService.getBySiteName(siteName) const { userWithSiteSessionData } = res.locals + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) + return res.status(200).json({ message: "Unmigrated site" }) + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } if (!site) { logger.error({ @@ -395,6 +449,20 @@ export class ReviewsRouter { const { siteName, requestId } = req.params const { userWithSiteSessionData } = res.locals const site = await this.sitesService.getBySiteName(siteName) + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) { + return res.status(200).json({ message: "Unmigrated site" }) + } + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } if (!site) { logger.error({ @@ -760,6 +828,21 @@ export class ReviewsRouter { const { userWithSiteSessionData } = res.locals // Step 1: Check that the site exists const site = await this.sitesService.getBySiteName(siteName) + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) { + return res.status(200).json({ message: "Unmigrated site" }) + } + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } + if (!site) { logger.error({ message: "Invalid site requested", @@ -932,11 +1015,10 @@ export class ReviewsRouter { }) } - // Step 2: Check that user exists. + // Step 2. Check that user exists. // Having session data is proof that this user exists // as otherwise, they would be rejected by our middleware const { userWithSiteSessionData } = res.locals - // Check if they are a collaborator const role = await this.collaboratorsService.getRole( siteName, @@ -1155,6 +1237,20 @@ export class ReviewsRouter { const { path } = req.query const { userWithSiteSessionData } = res.locals const site = await this.sitesService.getBySiteName(siteName) + const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated( + userWithSiteSessionData + ) + + if (!site && doesUnmigratedSiteExist) { + return res.status(200).json({ message: "Unmigrated site" }) + } + + const siteMembers = await this.collaboratorsService.list(siteName) + // NOTE: This is an initial migrated site but + // we haven't migrated the users. + if (siteMembers.length === 0 && site) { + return res.status(200).json({ message: "Unmigrated users" }) + } if (!site) { logger.error({ diff --git a/src/server.js b/src/server.js index f8e80b693..cd9ec6aba 100644 --- a/src/server.js +++ b/src/server.js @@ -198,7 +198,8 @@ const reviewRouter = new ReviewsRouter( usersService, sitesService, collaboratorsService, - notificationsService + notificationsService, + gitHubService ) const authenticatedSubrouterV1 = getAuthenticatedSubrouterV1({ authenticationMiddleware, diff --git a/src/services/utilServices/Sanitizer.ts b/src/services/utilServices/Sanitizer.ts new file mode 100644 index 000000000..741306547 --- /dev/null +++ b/src/services/utilServices/Sanitizer.ts @@ -0,0 +1,42 @@ +import DOMPurify from "isomorphic-dompurify" + +DOMPurify.setConfig({ + ADD_TAGS: ["iframe", "#comment", "script"], + ADD_ATTR: [ + "allow", + "allowfullscreen", + "frameborder", + "scrolling", + "marginheight", + "marginwidth", + "target", + "async", + ], + // required in case