From aa45a70c26000190d75f03aa65683881aa9e7ada Mon Sep 17 00:00:00 2001 From: csmig <33138761+csmig@users.noreply.github.com> Date: Mon, 11 Mar 2024 19:54:40 +0000 Subject: [PATCH] fix: update userId and ts only when review evaluation fields are present (#1226) * fix: update userId and ts only when review fields change * store UTC_TIMESTAMP() value in variable to prevent timestamp creep during query execution * debug test failure: log timestamp value * remove debug code * debug test failure: query for @utcTimestamp, no logging * debug test failure: putting console logging back * remove debug code; fix newman expected object properties * patching resultEngine requires result * update userId/ts if request contains eval fields * squashed merge of main * Added checks for timestamp updates and resultEngine patches * fixed issue with new test statements --------- Co-authored-by: cd-rite <61710958+cd-rite@users.noreply.github.com> --- api/source/controllers/Review.js | 3 + api/source/service/ReviewService.js | 21 +- api/source/specification/stig-manager.yaml | 3 +- test/api/postman_collection.json | 444 +++++++++++++++++++-- 4 files changed, 434 insertions(+), 37 deletions(-) diff --git a/api/source/controllers/Review.js b/api/source/controllers/Review.js index d009a1a8c..5e866aca8 100644 --- a/api/source/controllers/Review.js +++ b/api/source/controllers/Review.js @@ -168,6 +168,9 @@ module.exports.putReviewByAssetRule = async function (req, res, next) { module.exports.patchReviewByAssetRule = async function (req, res, next) { try { + if (Object.hasOwn(req.body, 'resultEngine') && !Object.hasOwn(req.body, 'result')) { + throw new SmError.UnprocessableError('Request body with resultEngine must include a result') + } const collectionId = Collection.getCollectionIdAndCheckPermission(req, Security.ACCESS_LEVEL.Restricted) const {assetId, ruleId} = {...req.params} const currentReviews = await ReviewService.getReviews([], { assetId, ruleId }, req.userObject) diff --git a/api/source/service/ReviewService.js b/api/source/service/ReviewService.js index 05d8a2993..ca402637f 100644 --- a/api/source/service/ReviewService.js +++ b/api/source/service/ReviewService.js @@ -1057,7 +1057,7 @@ select or review.reviewId is null -- no existing review or (cteCollectionSetting.resetCriteria = 'result' and rChangedResult.reviewId is not null) -- status meets criteria for resetting or (cteCollectionSetting.resetCriteria = 'any' and rChangedAny.reviewId is not null) -- status meets criteria for resetting - THEN UTC_TIMESTAMP() -- now + THEN @utcTimestamp -- now ELSE review.statusTs -- saved time END as statusTs, @@ -1069,8 +1069,21 @@ select ELSE review.statusUserId -- saved user END as statusUserId, - @userId as userId, - UTC_TIMESTAMP() as ts + CASE WHEN review.reviewId is null -- no existing review + or cteIncoming.resultId is not null -- patch request contains result + or cteIncoming.detail is not null -- patch request contains detail + or cteIncoming.comment is not null -- patch request contains comment + THEN @userId -- this user + ELSE review.userId -- saved user + END as userId, + + CASE WHEN review.reviewId is null -- no existing review + or cteIncoming.resultId is not null -- patch request contains result + or cteIncoming.detail is not null -- patch request contains detail + or cteIncoming.comment is not null -- patch request contains comment + THEN @utcTimestamp -- now + ELSE review.ts -- saved time + END as ts from cteIncoming LEFT JOIN cteGrant on cteIncoming.ruleId = cteGrant.ruleId @@ -1267,7 +1280,7 @@ where try { connection = await dbUtils.pool.getConnection() - const sqlSetVariables = `set @collectionId = ?, @assetId = ?, @userId = ?, @reviews = ?` + const sqlSetVariables = `set @collectionId = ?, @assetId = ?, @userId = ?, @reviews = ?, @utcTimestamp = UTC_TIMESTAMP()` await connection.query(sqlSetVariables, [parseInt(collectionId), parseInt(assetId), parseInt(userId), JSON.stringify(reviews)]) const [settings] = await connection.query(`select c.settings->>"$.history.maxReviews" as maxReviews FROM collection c where collectionId = @collectionId`) const historyMaxReviews = settings[0].maxReviews diff --git a/api/source/specification/stig-manager.yaml b/api/source/specification/stig-manager.yaml index 4bf0e2cbf..3517b9f4d 100644 --- a/api/source/specification/stig-manager.yaml +++ b/api/source/specification/stig-manager.yaml @@ -1393,7 +1393,8 @@ paths: description: | The request MUST target an existing Review and the request body MUST contain one or more Review properties. The overall request is validated as follows: - - conformance with the OAS schema for `ReviewAssetRulePatch` + - the request body conforms with the OAS schema for `ReviewAssetRulePatch` + - a request body that includes `resultEngine` must also include `result` - the requesting user has been granted access to the {collectionId} - a Review already exists for the {assetId} and {ruleId} diff --git a/test/api/postman_collection.json b/test/api/postman_collection.json index 73dea1111..9a52b4656 100644 --- a/test/api/postman_collection.json +++ b/test/api/postman_collection.json @@ -1,6 +1,6 @@ { "info": { - "_postman_id": "afe87add-1b90-4d7a-9b0a-4a42fb2ca9fa", + "_postman_id": "e8acbc2b-8ae9-478c-9359-fe458570a7a5", "name": "STIGMan OSS", "description": "An API for managing evaluations of Security Technical Implementation Guide (STIG) assessments.\n\nContact Support:\n Name: Carl Smigielski\n Email: carl.a.smigielski@saic.com", "schema": "https://schema.getpostman.com/json/collection/v2.1.0/collection.json", @@ -36478,9 +36478,9 @@ " userId: userId,\r", " username: user,\r", " ts: respData.ts,\r", - " touchTs: respData.ts,\r", + " touchTs: respData.touchTs,\r", " status: {\r", - " ts: respData.ts,\r", + " ts: respData.status.ts,\r", " text: null,\r", " user: {\r", " userId: userId,\r", @@ -37359,6 +37359,95 @@ }, "response": [] }, + { + "name": "Import and overwrite application data (as elevated Admin) Copy 2", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "let user = pm.environment.get(\"user\");\r", + "console.log(\"user: \" + user);\r", + "\r", + "if (pm.request.url.getQueryString().match(/elevate=true/)) {\r", + " user = \"elevated\";\r", + " console.log(\"setting user to 'elevated'\");\r", + "}\r", + "\r", + "if (user == \"elevated\") { //placeholder for \"users\" that should fail\r", + " pm.test(\"Status should be is 200 for elevated stigmanadmin user\", function () {\r", + " pm.response.to.have.status(200);\r", + " });\r", + "}\r", + "else {\r", + " pm.test(\"Status code is 403\", function () {\r", + " pm.response.to.have.status(403);\r", + " });\r", + "}\r", + "if (pm.response.code !== 200) {\r", + " return;\r", + "}\r", + "\r", + "\r", + "let response = pm.response.text();\r", + "console.log(response)\r", + "\r", + "pm.test(\"Body contains string\",() => {\r", + " pm.expect(response).to.include(\"Commit successful\");\r", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "auth": { + "type": "bearer", + "bearer": [ + { + "key": "token", + "value": "{{token.stigmanadmin}}", + "type": "string" + } + ] + }, + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "multipart/form-data" + } + ], + "body": { + "mode": "formdata", + "formdata": [ + { + "key": "importFile", + "type": "file", + "src": "./{{formDataFiles}}/{{appDataFile}}" + } + ] + }, + "url": { + "raw": "{{baseUrl}}/op/appdata?elevate=true", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "op", + "appdata" + ], + "query": [ + { + "key": "elevate", + "value": "true", + "description": "Elevate the user context for this request if user is permitted (canAdmin)" + } + ] + } + }, + "response": [] + }, { "name": "PATCH Review to Accepted", "event": [ @@ -37393,8 +37482,19 @@ " case \"lvl4\":\r", " case \"stigmanadmin\":\r", " pm.test(\"Status is 200\", () => pm.response.to.have.status(200))\r", + " pm.test(\"Response has proper timestamps\", () => {\r", + " pm.expect(jsonData).to.have.property(\"touchTs\").to.eql(jsonData.status.ts)\r", + " pm.expect(jsonData.status).to.have.property(\"ts\").to.not.eql(jsonData.ts) \r", + " })\r", " break\r", - "}" + "}\r", + "\r", + "// pm.test(\"Response has proper timestamps\", () => {\r", + "// pm.expect(jsonData).to.be.an(\"object\");\r", + "// pm.expect(jsonData).to.have.property(\"result\").to.eql(\"pass\");\r", + "// pm.expect(jsonData).to.have.property(\"touchTs\").to.eql(jsonData.ts)\r", + "// pm.expect(jsonData.status).to.have.property(\"ts\").to.not.eql(jsonData.ts)\r", + "// });" ], "type": "text/javascript" } @@ -38057,6 +38157,286 @@ }, "response": [] }, + { + "name": "Return the Review for an Asset and Rule Copy", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "let user = pm.environment.get(\"user\");\r", + "console.log(\"user: \" + user);\r", + "\r", + "if (pm.request.url.getQueryString().match(/elevate=true/)) {\r", + " user = \"elevated\";\r", + " console.log(\"setting user to 'elevated'\");\r", + "}\r", + "\r", + "if (user == \"collectioncreator\" || user == \"bizarroLvl1\" ) {\r", + " pm.test(\"Status should be is 403 for user collectioncreator, bizarroLvl1\", function () {\r", + " pm.response.to.have.status(403);\r", + " });\r", + " return;\r", + "}\r", + "else {\r", + " pm.test(\"Status code is 200\", function () {\r", + " pm.response.to.have.status(200);\r", + " });\r", + "}\r", + "if (pm.response.code !== 200) {\r", + " return;\r", + "}\r", + "\r", + "let jsonData = pm.response.json();\r", + "\r", + "pm.test(\"Response JSON is an object, timestamps match\", function () {\r", + " pm.expect(jsonData).to.be.an('object');\r", + " pm.expect(jsonData).to.have.property(\"touchTs\").to.eql(jsonData.ts)\r", + " pm.expect(jsonData.status).to.have.property(\"ts\").to.eql(jsonData.ts) \r", + "});\r", + "\r", + "\r", + "if (pm.request.url.getQueryString().match(/projection=stigs/)) {\r", + " pm.expect(jsonData.stigs).to.exist;\r", + "}\r", + "if (pm.request.url.getQueryString().match(/projection=history/)) {\r", + " pm.expect(jsonData.history).to.exist;\r", + "}\r", + "if (pm.request.url.getQueryString().match(/projection=rule/)) {\r", + " pm.expect(jsonData.rule).to.exist;\r", + "}\r", + "if (pm.request.url.getQueryString().match(/projection=metadata/)) {\r", + " pm.expect(jsonData.metadata).to.exist;\r", + "}\r", + "pm.test(\"Check if object contains all provided keys\", function () {\r", + " // pm.expect(jsonData).to.have.all.keys(reviewKeys);\r", + "});\r", + "\r", + "pm.test(\"Check if object contains proper ruleId\", function () {\r", + " let testRuleId = pm.environment.get(\"testRuleId\");\r", + " pm.expect(jsonData.ruleId).to.eql(testRuleId);\r", + "});\r", + "\r", + "pm.test(\"Check review comment for regex match string\", function () {\r", + " let reviewMatchString = pm.environment.get(\"reviewMatchString\");\r", + " var regex = new RegExp(reviewMatchString);\r", + " pm.expect(jsonData.detail).to.match(regex);\r", + "});\r", + "" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [], + "url": { + "raw": "{{baseUrl}}/collections/:collectionId/reviews/:assetId/:ruleId?projection=history&projection=stigs&projection=rule&projection=metadata", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "collections", + ":collectionId", + "reviews", + ":assetId", + ":ruleId" + ], + "query": [ + { + "key": "projection", + "value": "history" + }, + { + "key": "projection", + "value": "stigs" + }, + { + "key": "projection", + "value": "rule" + }, + { + "key": "projection", + "value": "metadata" + } + ], + "variable": [ + { + "key": "collectionId", + "value": "{{testCollection}}", + "description": "(Required) A path parameter that indentifies a Collection" + }, + { + "key": "assetId", + "value": "{{testAsset}}", + "description": "(Required) A path parameter that indentifies an Asset" + }, + { + "key": "ruleId", + "value": "{{testRuleId}}", + "description": "(Required) A path parameter that indentifies a Rule" + } + ] + } + }, + "response": [] + }, + { + "name": "resultEngine only - expect fail", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "let user = pm.environment.get(\"user\")\r", + "console.log(\"user: \" + user)\r", + "\r", + "let jsonData = pm.response.json()\r", + "\r", + "pm.test(\"Status is 422\", () => pm.response.to.have.status(422))\r", + "" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "PATCH", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\n \"resultEngine\": {\n \"type\": \"script\",\n \"product\": \"Evaluate-STIG\",\n \"version\": \"1.2310.1\",\n \"time\": \"2023-12-11T12:56:14.3576272-05:00\",\n \"checkContent\": {\n \"location\": \"VPN_Checks:1.2023.7.24\"\n },\n \"overrides\": [\n {\n \"authority\": \"Some_AnswerFile.xml\",\n \"oldResult\": \"unknown\",\n \"newResult\": \"pass\",\n \"remark\": \"Evaluate-STIG Answer File\"\n }\n ]\n }\n}" + }, + "url": { + "raw": "{{baseUrl}}/collections/:collectionId/reviews/:assetId/:ruleId", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "collections", + ":collectionId", + "reviews", + ":assetId", + ":ruleId" + ], + "variable": [ + { + "key": "collectionId", + "value": "{{testCollection}}", + "description": "(Required) A path parameter that indentifies a Collection" + }, + { + "key": "assetId", + "value": "{{testAsset}}", + "description": "(Required) A path parameter that indentifies an Asset" + }, + { + "key": "ruleId", + "value": "{{testRuleId}}", + "description": "(Required) A path parameter that indentifies a Rule" + } + ] + }, + "description": "Create a new Review, or update all properties of an existing Review, setting missing properties to null" + }, + "response": [] + }, + { + "name": "resultEngine only - expect success", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "let user = pm.environment.get('user');\r", + "\r", + "console.log('user: ' + user)\r", + "\r", + "\r", + "if ( user == \"collectioncreator\" ) { //placeholder for \"users\" that should fail\r", + " pm.test(\"Status should be is 403 for collectioncreator\", function () {\r", + " pm.response.to.have.status(403);\r", + " });\r", + " return;\r", + "}\r", + "else {\r", + " pm.test(\"Status code is 200 for all users but collectioncreator\", function () {\r", + " pm.response.to.have.status(200);\r", + " });\r", + "}\r", + "if (pm.response.code !== 200) {\r", + " return;\r", + "}\r", + "\r", + "let jsonData = pm.response.json();\r", + "\r", + "pm.test('Status is 200', () => pm.response.to.have.status(200))\r", + "\r", + "pm.test(\"Response has proper timestamps\", () => {\r", + " pm.expect(jsonData).to.be.an(\"object\");\r", + " pm.expect(jsonData).to.have.property(\"result\").to.eql(\"pass\");\r", + " pm.expect(jsonData).to.have.property(\"touchTs\").to.eql(jsonData.ts)\r", + " pm.expect(jsonData.status).to.have.property(\"ts\").to.not.eql(jsonData.ts)\r", + "});\r", + "\r", + "" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "PATCH", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\n \"result\": \"pass\",\n \"resultEngine\": {\n \"type\": \"script\",\n \"product\": \"Evaluate-STIG\",\n \"version\": \"1.2310.1\",\n \"time\": \"2023-12-11T12:56:14.3576272-05:00\",\n \"checkContent\": {\n \"location\": \"VPN_Checks:1.2023.7.24\"\n },\n \"overrides\": [\n {\n \"authority\": \"Some_AnswerFile.xml\",\n \"oldResult\": \"unknown\",\n \"newResult\": \"pass\",\n \"remark\": \"Evaluate-STIG Answer File\"\n }\n ]\n }\n}" + }, + "url": { + "raw": "{{baseUrl}}/collections/:collectionId/reviews/:assetId/:ruleId", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "collections", + ":collectionId", + "reviews", + ":assetId", + ":ruleId" + ], + "variable": [ + { + "key": "collectionId", + "value": "{{testCollection}}", + "description": "(Required) A path parameter that indentifies a Collection" + }, + { + "key": "assetId", + "value": "{{testAsset}}", + "description": "(Required) A path parameter that indentifies an Asset" + }, + { + "key": "ruleId", + "value": "{{testRuleId}}", + "description": "(Required) A path parameter that indentifies a Rule" + } + ] + }, + "description": "Create a new Review, or update all properties of an existing Review, setting missing properties to null" + }, + "response": [] + }, { "name": "PUT Review: no resultEngine - check response does not include \"resultEngine\": 0", "event": [ @@ -38110,9 +38490,9 @@ " userId: userId,\r", " username: user,\r", " ts: respData.ts,\r", - " touchTs: respData.ts,\r", + " touchTs: respData.touchTs,\r", " status: {\r", - " ts: respData.ts,\r", + " ts: respData.status.ts,\r", " text: null,\r", " user: {\r", " userId: userId,\r", @@ -39644,9 +40024,9 @@ "// userId: userId,\r", "// username: user,\r", "// ts: respData.ts,\r", - "// touchTs: respData.ts,\r", + "// touchTs: respData.touchTs,\r", "// status: {\r", - "// ts: respData.ts,\r", + "// ts: respData.status.ts,\r", "// text: null,\r", "// user: {\r", "// userId: userId,\r", @@ -40238,9 +40618,9 @@ "// userId: userId,\r", "// username: user,\r", "// ts: respData.ts,\r", - "// touchTs: respData.ts,\r", + "// touchTs: respData.touchTs,\r", "// status: {\r", - "// ts: respData.ts,\r", + "// ts: respData.status.ts,\r", "// text: null,\r", "// user: {\r", "// userId: userId,\r", @@ -40833,9 +41213,9 @@ "// userId: userId,\r", "// username: user,\r", "// ts: respData.ts,\r", - "// touchTs: respData.ts,\r", + "// touchTs: respData.touchTs,\r", "// status: {\r", - "// ts: respData.ts,\r", + "// ts: respData.status.ts,\r", "// text: null,\r", "// user: {\r", "// userId: userId,\r", @@ -41427,9 +41807,9 @@ "// userId: userId,\r", "// username: user,\r", "// ts: respData.ts,\r", - "// touchTs: respData.ts,\r", + "// touchTs: respData.touchTs,\r", "// status: {\r", - "// ts: respData.ts,\r", + "// ts: respData.status.ts,\r", "// text: null,\r", "// user: {\r", "// userId: userId,\r", @@ -42021,9 +42401,9 @@ "// userId: userId,\r", "// username: user,\r", "// ts: respData.ts,\r", - "// touchTs: respData.ts,\r", + "// touchTs: respData.touchTs,\r", "// status: {\r", - "// ts: respData.ts,\r", + "// ts: respData.status.ts,\r", "// text: null,\r", "// user: {\r", "// userId: userId,\r", @@ -42616,9 +42996,9 @@ "// userId: userId,\r", "// username: user,\r", "// ts: respData.ts,\r", - "// touchTs: respData.ts,\r", + "// touchTs: respData.touchTs,\r", "// status: {\r", - "// ts: respData.ts,\r", + "// ts: respData.status.ts,\r", "// text: null,\r", "// user: {\r", "// userId: userId,\r", @@ -47752,9 +48132,9 @@ "// userId: userId,\r", "// username: user,\r", "// ts: respData.ts,\r", - "// touchTs: respData.ts,\r", + "// touchTs: respData.touchTs,\r", "// status: {\r", - "// ts: respData.ts,\r", + "// ts: respData.status.ts,\r", "// text: null,\r", "// user: {\r", "// userId: userId,\r", @@ -51144,9 +51524,9 @@ " userId: userId,\r", " username: user,\r", " ts: respData.ts,\r", - " touchTs: respData.ts,\r", + " touchTs: respData.touchTs,\r", " status: {\r", - " ts: respData.ts,\r", + " ts: respData.status.ts,\r", " text: null,\r", " user: {\r", " userId: userId,\r", @@ -51311,9 +51691,9 @@ " userId: userId,\r", " username: user,\r", " ts: respData.ts,\r", - " touchTs: respData.ts,\r", + " touchTs: respData.touchTs,\r", " status: {\r", - " ts: respData.ts,\r", + " ts: respData.status.ts,\r", " text: null,\r", " user: {\r", " userId: userId,\r", @@ -51478,9 +51858,9 @@ " userId: userId,\r", " username: user,\r", " ts: respData.ts,\r", - " touchTs: respData.ts,\r", + " touchTs: respData.touchTs,\r", " status: {\r", - " ts: respData.ts,\r", + " ts: respData.status.ts,\r", " text: null,\r", " user: {\r", " userId: userId,\r", @@ -63010,9 +63390,9 @@ " userId: userId,\r", " username: user,\r", " ts: respData.ts,\r", - " touchTs: respData.ts,\r", + " touchTs: respData.touchTs,\r", " status: {\r", - " ts: respData.ts,\r", + " ts: respData.status.ts,\r", " text: null,\r", " user: {\r", " userId: userId,\r", @@ -68403,9 +68783,9 @@ "// userId: userId,\r", "// username: user,\r", "// ts: respData.ts,\r", - "// touchTs: respData.ts,\r", + "// touchTs: respData.touchTs,\r", "// status: {\r", - "// ts: respData.ts,\r", + "// ts: respData.status.ts,\r", "// text: null,\r", "// user: {\r", "// userId: userId,\r", @@ -69013,9 +69393,9 @@ "// userId: userId,\r", "// username: user,\r", "// ts: respData.ts,\r", - "// touchTs: respData.ts,\r", + "// touchTs: respData.touchTs,\r", "// status: {\r", - "// ts: respData.ts,\r", + "// ts: respData.status.ts,\r", "// text: null,\r", "// user: {\r", "// userId: userId,\r",