From a8bae86ec87677f6c9f977d098b3f757df6aa64a Mon Sep 17 00:00:00 2001 From: ahsrah7 Date: Thu, 17 Oct 2024 12:04:25 +0530 Subject: [PATCH 1/7] added edit ability for ER --- controllers/extensionRequests.js | 10 +++++ middlewares/skipAuthorizeRolesWrapper.js | 13 ++++++ routes/extensionRequests.js | 3 +- test/integration/extensionRequests.test.js | 51 ++++++++++++++++++++++ 4 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 middlewares/skipAuthorizeRolesWrapper.js diff --git a/controllers/extensionRequests.js b/controllers/extensionRequests.js index 039ab2ede..95aaf20bf 100644 --- a/controllers/extensionRequests.js +++ b/controllers/extensionRequests.js @@ -202,12 +202,22 @@ const getSelfExtensionRequests = async (req, res) => { * @param res {Object} - Express response object */ const updateExtensionRequest = async (req, res) => { + const { dev } = req.query; + const isDev = dev ? Boolean(dev) : false; try { const extensionRequest = await extensionRequestsQuery.fetchExtensionRequest(req.params.id); if (!extensionRequest.extensionRequestData) { return res.boom.notFound("Extension Request not found"); } + if ( + isDev && + !req.userData?.roles.super_user && + extensionRequest.extensionRequestData.status === EXTENSION_REQUEST_STATUS.APPROVED + ) { + return res.boom.badRequest("Extension Request cannot be updated"); + } + if (req.body.assignee) { const { taskData: task } = await tasks.fetchTask(extensionRequest.extensionRequestData.taskId); if (task.assignee !== (await getUsername(req.body.assignee))) { diff --git a/middlewares/skipAuthorizeRolesWrapper.js b/middlewares/skipAuthorizeRolesWrapper.js new file mode 100644 index 000000000..2bf0649c5 --- /dev/null +++ b/middlewares/skipAuthorizeRolesWrapper.js @@ -0,0 +1,13 @@ +const skipIfDev = (authorizeMiddleware) => { + return (req, res, next) => { + const { dev } = req.query; + const isDev = dev ? Boolean(dev) : false; + if (isDev) { + next(); + } else { + authorizeMiddleware(req, res, next); + } + }; +}; + +export default skipIfDev; diff --git a/routes/extensionRequests.js b/routes/extensionRequests.js index a4e93f56c..3096eb122 100644 --- a/routes/extensionRequests.js +++ b/routes/extensionRequests.js @@ -10,6 +10,7 @@ const { updateExtensionRequestStatus, getExtensionRequestsValidator, } = require("../middlewares/validators/extensionRequests"); +const { default: skipIfDev } = require("../middlewares/skipAuthorizeRolesWrapper"); router.post("/", authenticate, createExtensionRequest, extensionRequests.createTaskExtensionRequest); router.get("/", authenticate, getExtensionRequestsValidator, extensionRequests.fetchExtensionRequests); @@ -18,7 +19,7 @@ router.get("/:id", authenticate, authorizeRoles([SUPERUSER, APPOWNER]), extensio router.patch( "/:id", authenticate, - authorizeRoles([SUPERUSER, APPOWNER]), + skipIfDev(authorizeRoles([SUPERUSER, APPOWNER])), updateExtensionRequest, extensionRequests.updateExtensionRequest ); diff --git a/test/integration/extensionRequests.test.js b/test/integration/extensionRequests.test.js index ebe6333f4..2e6e82258 100644 --- a/test/integration/extensionRequests.test.js +++ b/test/integration/extensionRequests.test.js @@ -921,6 +921,57 @@ describe("Extension Requests", function () { }); }); + it("User should be able to update the extensionRequest for the given extensionRequestId", function (done) { + chai + .request(app) + .patch(`/extension-requests/${extensionRequestId4}?dev=true`) + .set("cookie", `${cookieName}=${jwt}`) + .send({ + title: "new-title", + }) + .end((err, res) => { + if (err) { + return done(err); + } + expect(res).to.have.status(204); + return done(); + }); + }); + + it("User should not be able to update the extensionRequest if already approved", function (done) { + chai + .request(app) + .patch(`/extension-requests/${extensionRequestId1}?dev=true`) + .set("cookie", `${cookieName}=${jwt}`) + .send({ + title: "new-title", + }) + .end((err, res) => { + if (err) { + return done(err); + } + expect(res).to.have.status(400); + return done(); + }); + }); + + it("Super user should not be able to update the extensionRequest if already approved", function (done) { + chai + .request(app) + .patch(`/extension-requests/${extensionRequestId1}?dev=true`) + .set("cookie", `${cookieName}=${jwt}`) + .send({ + title: "new-title", + }) + .end((err, res) => { + if (err) { + return done(err); + } + expect(res).to.have.status(400); + return done(); + }); + }); + it("Should return 400 if assignee of the extensionrequest is upated with a different user", function (done) { chai .request(app) From 999d3612c0c80d4e2c5b8ac023b532cec690d75c Mon Sep 17 00:00:00 2001 From: ahsrah7 Date: Fri, 18 Oct 2024 01:02:37 +0530 Subject: [PATCH 2/7] fixing function name and export type --- controllers/extensionRequests.js | 3 ++- middlewares/skipAuthorizeRolesWrapper.js | 4 ++-- routes/extensionRequests.js | 4 ++-- test/integration/extensionRequests.test.js | 6 +++--- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/controllers/extensionRequests.js b/controllers/extensionRequests.js index 95aaf20bf..b392f5fae 100644 --- a/controllers/extensionRequests.js +++ b/controllers/extensionRequests.js @@ -213,7 +213,8 @@ const updateExtensionRequest = async (req, res) => { if ( isDev && !req.userData?.roles.super_user && - extensionRequest.extensionRequestData.status === EXTENSION_REQUEST_STATUS.APPROVED + (extensionRequest.extensionRequestData.status === EXTENSION_REQUEST_STATUS.APPROVED || + extensionRequest.extensionRequestData.status === EXTENSION_REQUEST_STATUS.DENIED) ) { return res.boom.badRequest("Extension Request cannot be updated"); } diff --git a/middlewares/skipAuthorizeRolesWrapper.js b/middlewares/skipAuthorizeRolesWrapper.js index 2bf0649c5..15d0a50bd 100644 --- a/middlewares/skipAuthorizeRolesWrapper.js +++ b/middlewares/skipAuthorizeRolesWrapper.js @@ -1,4 +1,4 @@ -const skipIfDev = (authorizeMiddleware) => { +const skipAuthorizeRolesUnderFF = (authorizeMiddleware) => { return (req, res, next) => { const { dev } = req.query; const isDev = dev ? Boolean(dev) : false; @@ -10,4 +10,4 @@ const skipIfDev = (authorizeMiddleware) => { }; }; -export default skipIfDev; +module.exports = skipAuthorizeRolesUnderFF; diff --git a/routes/extensionRequests.js b/routes/extensionRequests.js index 3096eb122..13519084f 100644 --- a/routes/extensionRequests.js +++ b/routes/extensionRequests.js @@ -10,7 +10,7 @@ const { updateExtensionRequestStatus, getExtensionRequestsValidator, } = require("../middlewares/validators/extensionRequests"); -const { default: skipIfDev } = require("../middlewares/skipAuthorizeRolesWrapper"); +const skipAuthorizeRolesUnderFF = require("../middlewares/skipAuthorizeRolesWrapper"); router.post("/", authenticate, createExtensionRequest, extensionRequests.createTaskExtensionRequest); router.get("/", authenticate, getExtensionRequestsValidator, extensionRequests.fetchExtensionRequests); @@ -19,7 +19,7 @@ router.get("/:id", authenticate, authorizeRoles([SUPERUSER, APPOWNER]), extensio router.patch( "/:id", authenticate, - skipIfDev(authorizeRoles([SUPERUSER, APPOWNER])), + skipAuthorizeRolesUnderFF(authorizeRoles([SUPERUSER, APPOWNER])), updateExtensionRequest, extensionRequests.updateExtensionRequest ); diff --git a/test/integration/extensionRequests.test.js b/test/integration/extensionRequests.test.js index 2e6e82258..2305df5c8 100644 --- a/test/integration/extensionRequests.test.js +++ b/test/integration/extensionRequests.test.js @@ -955,11 +955,11 @@ describe("Extension Requests", function () { }); }); - it("Super user should not be able to update the extensionRequest if already approved", function (done) { + it("Super user should be able to update the extensionRequest if already approved", function (done) { chai .request(app) .patch(`/extension-requests/${extensionRequestId1}?dev=true`) - .set("cookie", `${cookieName}=${jwt}`) + .set("cookie", `${cookieName}=${superUserJwt}`) .send({ title: "new-title", }) @@ -967,7 +967,7 @@ describe("Extension Requests", function () { if (err) { return done(err); } - expect(res).to.have.status(400); + expect(res).to.have.status(200); return done(); }); }); From 3b48e2bb544451b35365154903abfff38a9d7bb5 Mon Sep 17 00:00:00 2001 From: ahsrah7 Date: Fri, 18 Oct 2024 20:46:11 +0530 Subject: [PATCH 3/7] fix feature flag value check --- controllers/extensionRequests.js | 2 +- middlewares/skipAuthorizeRolesWrapper.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/extensionRequests.js b/controllers/extensionRequests.js index b392f5fae..ae9c39dc7 100644 --- a/controllers/extensionRequests.js +++ b/controllers/extensionRequests.js @@ -203,7 +203,7 @@ const getSelfExtensionRequests = async (req, res) => { */ const updateExtensionRequest = async (req, res) => { const { dev } = req.query; - const isDev = dev ? Boolean(dev) : false; + const isDev = dev === "true"; try { const extensionRequest = await extensionRequestsQuery.fetchExtensionRequest(req.params.id); if (!extensionRequest.extensionRequestData) { diff --git a/middlewares/skipAuthorizeRolesWrapper.js b/middlewares/skipAuthorizeRolesWrapper.js index 15d0a50bd..74c72d3c9 100644 --- a/middlewares/skipAuthorizeRolesWrapper.js +++ b/middlewares/skipAuthorizeRolesWrapper.js @@ -1,7 +1,7 @@ const skipAuthorizeRolesUnderFF = (authorizeMiddleware) => { return (req, res, next) => { const { dev } = req.query; - const isDev = dev ? Boolean(dev) : false; + const isDev = dev === "true"; if (isDev) { next(); } else { From e83d93bf1e1c344ed0b265b5cdbdc6a52c163698 Mon Sep 17 00:00:00 2001 From: ahsrah7 Date: Sat, 19 Oct 2024 11:10:28 +0530 Subject: [PATCH 4/7] error message and status code updated --- controllers/extensionRequests.js | 2 +- test/integration/extensionRequests.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/extensionRequests.js b/controllers/extensionRequests.js index ae9c39dc7..50940acc6 100644 --- a/controllers/extensionRequests.js +++ b/controllers/extensionRequests.js @@ -216,7 +216,7 @@ const updateExtensionRequest = async (req, res) => { (extensionRequest.extensionRequestData.status === EXTENSION_REQUEST_STATUS.APPROVED || extensionRequest.extensionRequestData.status === EXTENSION_REQUEST_STATUS.DENIED) ) { - return res.boom.badRequest("Extension Request cannot be updated"); + return res.boom.badRequest("Only pending extension request can be updated"); } if (req.body.assignee) { diff --git a/test/integration/extensionRequests.test.js b/test/integration/extensionRequests.test.js index 2305df5c8..29a5ad255 100644 --- a/test/integration/extensionRequests.test.js +++ b/test/integration/extensionRequests.test.js @@ -967,7 +967,7 @@ describe("Extension Requests", function () { if (err) { return done(err); } - expect(res).to.have.status(200); + expect(res).to.have.status(204); return done(); }); }); From e54746f8f7333b169403d2503a22b62e708d2b07 Mon Sep 17 00:00:00 2001 From: ahsrah7 Date: Sun, 20 Oct 2024 13:18:48 +0530 Subject: [PATCH 5/7] middleware tests added --- .../skipAuthorizeRolesWrapper.test.js | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 test/unit/middlewares/skipAuthorizeRolesWrapper.test.js diff --git a/test/unit/middlewares/skipAuthorizeRolesWrapper.test.js b/test/unit/middlewares/skipAuthorizeRolesWrapper.test.js new file mode 100644 index 000000000..7c4733423 --- /dev/null +++ b/test/unit/middlewares/skipAuthorizeRolesWrapper.test.js @@ -0,0 +1,43 @@ +const chai = require("chai"); +const sinon = require("sinon"); +const { assert } = chai; +const skipAuthorizeRolesUnderFF = require("../../../middlewares/skipAuthorizeRolesWrapper"); + +describe("skipAuthorizeRolesUnderFF Middleware", function () { + let req, res, next, authorizeMiddleware; + + beforeEach(function () { + req = { query: {} }; + res = {}; + next = sinon.spy(); + authorizeMiddleware = sinon.spy(); + }); + + it("should call next() when dev is true", function () { + req.query.dev = "true"; + + const middleware = skipAuthorizeRolesUnderFF(authorizeMiddleware); + middleware(req, res, next); + + assert.isTrue(next.calledOnce, "next() should be called once"); + assert.isFalse(authorizeMiddleware.called, "authorizeMiddleware should not be called"); + }); + + it("should call authorizeMiddleware when dev is false", function () { + req.query.dev = "false"; + + const middleware = skipAuthorizeRolesUnderFF(authorizeMiddleware); + middleware(req, res, next); + + assert.isTrue(authorizeMiddleware.calledOnce, "authorizeMiddleware should be called once"); + assert.isFalse(next.called, "next() should not be called"); + }); + + it("should call authorizeMiddleware when dev is not provided", function () { + const middleware = skipAuthorizeRolesUnderFF(authorizeMiddleware); + middleware(req, res, next); + + assert.isTrue(authorizeMiddleware.calledOnce, "authorizeMiddleware should be called once"); + assert.isFalse(next.called, "next() should not be called"); + }); +}); From 014c0f86daf503b52ce885481745e8e1e5a162f0 Mon Sep 17 00:00:00 2001 From: ahsrah7 Date: Tue, 22 Oct 2024 01:32:09 +0530 Subject: [PATCH 6/7] pending status check added --- controllers/extensionRequests.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/extensionRequests.js b/controllers/extensionRequests.js index 50940acc6..d3c92cc55 100644 --- a/controllers/extensionRequests.js +++ b/controllers/extensionRequests.js @@ -213,8 +213,7 @@ const updateExtensionRequest = async (req, res) => { if ( isDev && !req.userData?.roles.super_user && - (extensionRequest.extensionRequestData.status === EXTENSION_REQUEST_STATUS.APPROVED || - extensionRequest.extensionRequestData.status === EXTENSION_REQUEST_STATUS.DENIED) + extensionRequest.extensionRequestData.status !== EXTENSION_REQUEST_STATUS.PENDING ) { return res.boom.badRequest("Only pending extension request can be updated"); } From a669c71867d6878c05305ece19958426d7c0eb8a Mon Sep 17 00:00:00 2001 From: ahsrah7 Date: Sun, 24 Nov 2024 14:52:56 +0530 Subject: [PATCH 7/7] added comment regarding middleware for FF removal --- routes/extensionRequests.js | 1 + 1 file changed, 1 insertion(+) diff --git a/routes/extensionRequests.js b/routes/extensionRequests.js index 13519084f..e44e65c82 100644 --- a/routes/extensionRequests.js +++ b/routes/extensionRequests.js @@ -16,6 +16,7 @@ router.post("/", authenticate, createExtensionRequest, extensionRequests.createT router.get("/", authenticate, getExtensionRequestsValidator, extensionRequests.fetchExtensionRequests); router.get("/self", authenticate, extensionRequests.getSelfExtensionRequests); router.get("/:id", authenticate, authorizeRoles([SUPERUSER, APPOWNER]), extensionRequests.getExtensionRequest); +// remove the skipAuthorizeRolesUnderFF & authorizeRoles middleware when removing the feature flag router.patch( "/:id", authenticate,