From bb80fae99db71b0bf2fc55ac3c1a297e7b344791 Mon Sep 17 00:00:00 2001 From: jkay Date: Wed, 25 Oct 2023 11:05:27 +1300 Subject: [PATCH 1/3] removed admin check flag --- src/config/feature-flags.ts | 1 - .../jira-admin-permission-middleware.test.ts | 53 ++----------------- .../jira-admin-permission-middleware.ts | 7 +-- .../middleware/jira-admin/jira-admin-check.ts | 7 +-- src/routes/github/github-oauth.test.ts | 3 -- 5 files changed, 6 insertions(+), 65 deletions(-) diff --git a/src/config/feature-flags.ts b/src/config/feature-flags.ts index 4a78130dc0..3ae5622e95 100644 --- a/src/config/feature-flags.ts +++ b/src/config/feature-flags.ts @@ -16,7 +16,6 @@ export enum BooleanFlags { INNO_DRAFT_PR = "inno-draft-pr", VERBOSE_LOGGING = "verbose-logging", SEND_PR_COMMENTS_TO_JIRA = "send-pr-comments-to-jira_zy5ib", - JIRA_ADMIN_CHECK = "jira-admin-check", REMOVE_STALE_MESSAGES = "remove-stale-messages", USE_NEW_PULL_ALGO = "use-new-pull-algo", USE_DYNAMODB_FOR_DEPLOYMENT_WEBHOOK = "use-dynamodb-for-deployment-webhook", diff --git a/src/middleware/jira-admin-permission-middleware.test.ts b/src/middleware/jira-admin-permission-middleware.test.ts index 29543f8edb..caa30f4cb9 100644 --- a/src/middleware/jira-admin-permission-middleware.test.ts +++ b/src/middleware/jira-admin-permission-middleware.test.ts @@ -4,8 +4,6 @@ import { fetchAndSaveUserJiraAdminStatus, jiraAdminPermissionsMiddleware } from "middleware/jira-admin-permission-middleware"; -import { booleanFlag, BooleanFlags } from "config/feature-flags"; -import { when } from "jest-when"; jest.mock("config/feature-flags"); @@ -24,67 +22,22 @@ describe("jiraAdminPermissionsMiddleware", () => { send: jest.fn() }; mockNext = jest.fn(); - - when(booleanFlag).calledWith( - BooleanFlags.JIRA_ADMIN_CHECK - ).mockResolvedValue(true); }); test("should return 403 Forbidden if session is undefined", async () => { - await jiraAdminPermissionsMiddleware(mockRequest, mockResponse, mockNext); + jiraAdminPermissionsMiddleware(mockRequest, mockResponse, mockNext); expect(mockResponse.status).toHaveBeenCalledWith(403); }); test("should return 403 Forbidden if hasAdminPermissions is false", async () => { mockRequest.session.isJiraAdmin = false; - await jiraAdminPermissionsMiddleware(mockRequest, mockResponse, mockNext); + jiraAdminPermissionsMiddleware(mockRequest, mockResponse, mockNext); expect(mockResponse.status).toHaveBeenCalledWith(403); }); test("should call next() if user has Jira admin permissions", async () => { mockRequest.session.isJiraAdmin = true; - await jiraAdminPermissionsMiddleware(mockRequest, mockResponse, mockNext); - expect(mockNext).toHaveBeenCalled(); - }); -}); - -// Delete this describe block during flag clean up -describe("jiraAdminPermissionsMiddleware - feature flag off", () => { - let mockRequest; - let mockResponse; - let mockNext; - - beforeEach(() => { - mockRequest = { - session: {}, - log: { info: jest.fn() } - }; - mockResponse = { - status: jest.fn().mockReturnThis(), - send: jest.fn() - }; - mockNext = jest.fn(); - - when(booleanFlag).calledWith( - BooleanFlags.JIRA_ADMIN_CHECK - ).mockResolvedValue(false); - }); - - test("should return 403 Forbidden if session is undefined", async () => { - delete mockRequest.session.isJiraAdmin; - await jiraAdminPermissionsMiddleware(mockRequest, mockResponse, mockNext); - expect(mockNext).toHaveBeenCalled(); - }); - - test("should return 403 Forbidden if hasAdminPermissions is false", async () => { - mockRequest.session.isJiraAdmin = false; - await jiraAdminPermissionsMiddleware(mockRequest, mockResponse, mockNext); - expect(mockNext).toHaveBeenCalled(); - }); - - test("should call next() if user has Jira admin permissions", async () => { - mockRequest.session.isJiraAdmin = true; - await jiraAdminPermissionsMiddleware(mockRequest, mockResponse, mockNext); + jiraAdminPermissionsMiddleware(mockRequest, mockResponse, mockNext); expect(mockNext).toHaveBeenCalled(); }); }); diff --git a/src/middleware/jira-admin-permission-middleware.ts b/src/middleware/jira-admin-permission-middleware.ts index 66e99b1f19..07c6394541 100644 --- a/src/middleware/jira-admin-permission-middleware.ts +++ b/src/middleware/jira-admin-permission-middleware.ts @@ -1,7 +1,6 @@ import { NextFunction, Request, Response } from "express"; import { Installation } from "models/installation"; import { JiraClient } from "models/jira-client"; -import { booleanFlag, BooleanFlags } from "config/feature-flags"; export const fetchAndSaveUserJiraAdminStatus = async (req: Request, claims: { sub?: string;}, installation: Installation): Promise => { const ADMIN_PERMISSION = "ADMINISTER"; @@ -27,10 +26,8 @@ export const fetchAndSaveUserJiraAdminStatus = async (req: Request, claims: { su } }; -export const jiraAdminPermissionsMiddleware = async (req: Request, res: Response, next: NextFunction): Promise => { - if (!(await booleanFlag(BooleanFlags.JIRA_ADMIN_CHECK))) { - return next(); - } +export const jiraAdminPermissionsMiddleware = (req: Request, res: Response, next: NextFunction): void | Response => { + const { isJiraAdmin } = req.session; if (isJiraAdmin === undefined) { diff --git a/src/rest/middleware/jira-admin/jira-admin-check.ts b/src/rest/middleware/jira-admin/jira-admin-check.ts index a083dcac76..7eaaba6ae8 100644 --- a/src/rest/middleware/jira-admin/jira-admin-check.ts +++ b/src/rest/middleware/jira-admin/jira-admin-check.ts @@ -1,6 +1,5 @@ import { NextFunction, Request, Response } from "express"; import { JiraClient } from "models/jira-client"; -import { booleanFlag, BooleanFlags } from "config/feature-flags"; import { InsufficientPermissionError, InvalidTokenError } from "config/errors"; import { errorWrapper } from "../../helper"; import { BaseLocals } from "../../routes"; @@ -8,11 +7,7 @@ import { BaseLocals } from "../../routes"; const ADMIN_PERMISSION = "ADMINISTER"; export const JiraAdminEnforceMiddleware = errorWrapper("jiraAdminEnforceMiddleware", async (req: Request, res: Response, next: NextFunction): Promise => { - const { accountId, installation, jiraHost } = res.locals; - - if (!(await booleanFlag(BooleanFlags.JIRA_ADMIN_CHECK, jiraHost))) { - return next(); - } + const { accountId, installation } = res.locals; if (!accountId) { throw new InvalidTokenError("Missing userAccountId"); diff --git a/src/routes/github/github-oauth.test.ts b/src/routes/github/github-oauth.test.ts index 57a36dd466..cabfeaeaed 100644 --- a/src/routes/github/github-oauth.test.ts +++ b/src/routes/github/github-oauth.test.ts @@ -12,8 +12,6 @@ import { generateSignedSessionCookieHeader, parseCookiesAndSession } from "test/utils/cookies"; -import { booleanFlag, BooleanFlags } from "config/feature-flags"; -import { when } from "jest-when"; import { Installation } from "models/installation"; jest.mock("config/feature-flags"); @@ -175,7 +173,6 @@ describe("github-oauth", () => { }); it("must work only for Jira admins", async () => { - when(booleanFlag).calledWith(BooleanFlags.JIRA_ADMIN_CHECK).mockResolvedValue(true); const res = await supertest(getFrontendApp()) .get("/github/login?blah=true") From 3f07731f98a93f1c0615718db72748a61fa152fd Mon Sep 17 00:00:00 2001 From: Josh Kay Date: Tue, 31 Oct 2023 11:50:34 +1300 Subject: [PATCH 2/3] remove left over jira admin refs --- src/rest/middleware/jira-admin/jira-admin-check.test.ts | 3 --- src/routes/jira/jira-connected-repos-get.test.ts | 3 --- 2 files changed, 6 deletions(-) diff --git a/src/rest/middleware/jira-admin/jira-admin-check.test.ts b/src/rest/middleware/jira-admin/jira-admin-check.test.ts index add780839d..b89676e397 100644 --- a/src/rest/middleware/jira-admin/jira-admin-check.test.ts +++ b/src/rest/middleware/jira-admin/jira-admin-check.test.ts @@ -2,7 +2,6 @@ import { encodeSymmetric } from "atlassian-jwt"; import { Application } from "express"; import supertest from "supertest"; import { Installation } from "models/installation"; -import { booleanFlag, BooleanFlags } from "config/feature-flags"; import { when } from "jest-when"; import { getFrontendApp } from "~/src/app"; @@ -19,8 +18,6 @@ describe("Jira Admin Check", () => { beforeEach(async () => { - when(booleanFlag).calledWith(BooleanFlags.JIRA_ADMIN_CHECK, jiraHost).mockResolvedValue(true); - app = getFrontendApp(); await Installation.install({ diff --git a/src/routes/jira/jira-connected-repos-get.test.ts b/src/routes/jira/jira-connected-repos-get.test.ts index 35ba654d90..1c4c070c59 100644 --- a/src/routes/jira/jira-connected-repos-get.test.ts +++ b/src/routes/jira/jira-connected-repos-get.test.ts @@ -5,8 +5,6 @@ import { getLogger } from "config/logger"; import { Subscription } from "models/subscription"; import { DatabaseStateCreator } from "test/utils/database-state-creator"; import supertest from "supertest"; -import { booleanFlag, BooleanFlags } from "config/feature-flags"; -import { when } from "jest-when"; import { RepoSyncState } from "models/reposyncstate"; jest.mock("config/feature-flags"); @@ -36,7 +34,6 @@ describe("jira-connected-repos-get", () => { subscription = result.subscription; repoSyncState = result.repoSyncState!; - when(booleanFlag).calledWith(BooleanFlags.JIRA_ADMIN_CHECK).mockResolvedValue(true); }); it("should return 403 when not an admin", async () => { From 6224c4a93f7749818f274b0b4ce4b5edbe558152 Mon Sep 17 00:00:00 2001 From: Josh Kay Date: Tue, 31 Oct 2023 11:58:09 +1300 Subject: [PATCH 3/3] drop unused when --- src/rest/middleware/jira-admin/jira-admin-check.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rest/middleware/jira-admin/jira-admin-check.test.ts b/src/rest/middleware/jira-admin/jira-admin-check.test.ts index b89676e397..c75b2fe5ec 100644 --- a/src/rest/middleware/jira-admin/jira-admin-check.test.ts +++ b/src/rest/middleware/jira-admin/jira-admin-check.test.ts @@ -2,7 +2,6 @@ import { encodeSymmetric } from "atlassian-jwt"; import { Application } from "express"; import supertest from "supertest"; import { Installation } from "models/installation"; -import { when } from "jest-when"; import { getFrontendApp } from "~/src/app"; jest.mock("config/feature-flags");