Merge pull request #30 from RusticiSoftware/main

Improve/fix HTTP status code conformance with actor usage and session endings across various statement types
adlnet · Feb 1, 2022 · 9a257c2 · 9a257c2
2 parents a553f84 + 08263cb
commit 9a257c2
Show file tree

Hide file tree

Showing 9 changed files with 54 additions and 20 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -153,7 +153,7 @@ module.exports = {
                     -1, 0, 1,
                     // These are HTTP statuses. Ideally we would make these constants, but that's a
                     // job for a more thorough eslint unification task.
-                    200, 204, 302, 400, 403, 404, 500
+                    200, 204, 302, 400, 401, 403, 404, 500
                 ],
                 "ignoreArrayIndexes": true
             }

diff --git a/lts/pkg/src/005-1-invalid-au/005-1-invalid-au.js b/lts/pkg/src/005-1-invalid-au/005-1-invalid-au.js
@@ -98,17 +98,25 @@ const saveLearnerPrefs = async (
 
                 return false;
             }
-            // this treats non-401 and non-403 (like 400, 500, etc.) as not acceptable responses
+            // If no actor is provided to an agent profile endpoint, xAPI rules apply and requires a 400, not a 403.
+            else if (noActor) {
+                if (response.status !== 400) {
+                    Helpers.storeResult(false, false, {reqId, msg: `Save learner preferences with no actor query param should have responded with a status code: 400, but instead had a response status code: ${response.status} ${responseContent} (Testing ${reqId})`});
+
+                    return false;
+                }
+            }
+            // this treats other non-403 (like 400, 500, etc.) as not acceptable responses
             // for denying the request
             // eslint-disable-next-line no-magic-numbers
-            else if (response.status !== 401 && response.status !== 403) {
+            else if (response.status !== 403) {
                 Helpers.storeResult(false, false, {reqId: "4.2.0.0-2", msg: `Save learner preferences response status code: ${response.status} ${responseContent} (Testing ${reqId})`});
 
                 return false;
             }
         }
         catch (ex) {
-            // request should succeed, but provide a 401 or 403, so an exception here is an error
+            // request should succeed but provide a 403, so an exception here is an error
             Helpers.storeResult(false, true, {reqId, msg: `Failed request to save learner preferences: ${ex}`});
 
             return false;
@@ -135,7 +143,7 @@ const saveLearnerPrefs = async (
         // send statement prior to getting auth token, with value
         //
         cmi5.setAuth("Basic Y2F0YXB1bHQ6ZmlyZSBzb21lIFJ1c3RpY2kgU29mdHdhcmUgcm9ja3Mh");
-        if (! await Helpers.sendStatement(cmi5, preInitializedSt, "8.1.2.0-2 (d)")) {
+        if (! await Helpers.sendStatement(cmi5, preInitializedSt, "8.1.2.0-2 (d)", {shouldSucceed: false, acceptUnauthorized: true})) {
             return;
         }
 
@@ -330,7 +338,7 @@ const saveLearnerPrefs = async (
         const origAuth = cmi5.getAuth();
 
         cmi5.setAuth("");
-        if (! await Helpers.sendStatement(cmi5, preInitializedSt, "8.1.2.0-5 (d)")) {
+        if (! await Helpers.sendStatement(cmi5, preInitializedSt, "8.1.2.0-5 (d)", {shouldSucceed: false, acceptUnauthorized: true})) {
             return;
         }
 
@@ -414,22 +422,28 @@ const saveLearnerPrefs = async (
                 }
             },
             {
-                // statement with actor objectType set to "Group"
+                // defined statement with actor objectType set to "Group"
                 reqId: "9.2.0.0-2",
+                type: "passed",
                 alter: (st) => {
                     st.actor.objectType = "Group";
                 }
             },
             {
-                // statement with actor objectType set to an invalid value
+                // defined statement with actor objectType set to an invalid value
                 reqId: "9.2.0.0-2",
+                type: "passed",
                 alter: (st) => {
                     st.actor.objectType = "Unknown";
+                },
+                cfg: {
+                    expectedStatuses: [400, 403]
                 }
             },
             {
-                // statement with actor using an mbox IFI instead of account
+                // defined statement with actor using an mbox IFI instead of account
                 reqId: "9.2.0.0-3",
+                type: "passed",
                 alter: (st) => {
                     delete st.actor.account;
                     st.actor.mbox = "mailto:catapult@example.com";
@@ -760,7 +774,7 @@ const saveLearnerPrefs = async (
         // send statement after terminated
         // Also validates: 9.3.0.0-2 (d)
         //
-        if (! await Helpers.sendStatement(cmi5, allowedSt, "9.3.0.0-5 (d2)")) {
+        if (! await Helpers.sendStatement(cmi5, allowedSt, "9.3.0.0-5 (d2)", {shouldSucceed: false, acceptUnauthorized: true})) {
             return;
         }
 
@@ -798,7 +812,7 @@ const saveLearnerPrefs = async (
             return;
         }
 
-        Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with either 401 or 403 response status"});
+        Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with 403 response status"});
     };
     /* eslint-enable padding-line-between-statements */
 

diff --git a/lts/pkg/src/005-2-invalid-au/005-2-invalid-au.js b/lts/pkg/src/005-2-invalid-au/005-2-invalid-au.js
@@ -71,7 +71,7 @@ const execute = async () => {
 
     await Helpers.closeAU(cmi5);
 
-    Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with either 401 or 403 response status"});
+    Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with 403 response status"});
 };
 
 execute();
diff --git a/lts/pkg/src/006-launchMode/006-launchMode.js b/lts/pkg/src/006-launchMode/006-launchMode.js
@@ -49,7 +49,7 @@ const execute = async () => {
         return;
     }
 
-    Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with either 401 or 403 response status"});
+    Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with 403 response status"});
 
     await Helpers.closeAU(cmi5);
 };

diff --git a/lts/pkg/src/007-1-multi-session/007-1-multi-session.js b/lts/pkg/src/007-1-multi-session/007-1-multi-session.js
@@ -54,7 +54,7 @@ const execute = async () => {
             return;
         }
 
-        Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with either 401 or 403 response status"});
+        Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with 403 response status"});
     }
 
     await Helpers.closeAU(cmi5);

diff --git a/lts/pkg/src/007-2-multi-session/007-2-multi-session.js b/lts/pkg/src/007-2-multi-session/007-2-multi-session.js
@@ -61,7 +61,7 @@ const execute = async () => {
 
         await Helpers.closeAU(cmi5);
 
-        Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with either 401 or 403 response status"});
+        Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with 403 response status"});
     }
 };
 

diff --git a/lts/pkg/src/008-1-abandoned/008-1-abandoned.js b/lts/pkg/src/008-1-abandoned/008-1-abandoned.js
@@ -50,7 +50,8 @@ const execute = async () => {
             //
             const allowedSt = cmi5.prepareStatement("http://adlnet.gov/expapi/verbs/experienced");
 
-            if (! await Helpers.sendStatement(cmi5, allowedSt, "9.3.6.0-2 (d2)")) {
+            if (! await Helpers.sendStatement(cmi5, allowedSt, "9.3.6.0-2 (d2)",
+                {expectedStatuses: [401, 403]})) {
                 return;
             }
 

diff --git a/lts/pkg/src/lib/helpers.js b/lts/pkg/src/lib/helpers.js
@@ -147,22 +147,38 @@ const Helpers = {
                 console.log(reqId, stContent);
             }
 
+            // If we provide an explicit set of statuses to match, exclude fallthrough cases.
+            if (cfg.expectedStatuses) {
+                if (cfg.expectedStatuses.indexOf(stResponse.status) > -1) {
+                    return true;
+                }
+
+                Helpers.storeResult(false, false, {reqId, msg: `Statement should have been rejected with a status code of (one of) ${cfg.expectedStatuses}, received response status code: ${stResponse.status} ${stContent} (Testing ${reqId})`});
+
+                return false;
+            }
+
             if (stResponse.status === 204 || stResponse.status === 200) {
                 Helpers.storeResult(false, false, {reqId, msg: `Statement not rejected (${stResponse.status})`});
 
                 return false;
             }
-            // this treats non-401 and non-403 (like 400, 500, etc.) as not acceptable responses
+            // treats specific tests for unauthorized requests as acceptable responses.
+            // eslint-disable-next-line no-magic-numbers
+            else if (stResponse.status === 401 && cfg.acceptUnauthorized) {
+                return true;
+            }
+            // this treats non-403 (like 400, 500, etc.) as not acceptable responses
             // for denying the statement
             // eslint-disable-next-line no-magic-numbers
-            else if (stResponse.status !== 401 && stResponse.status !== 403) {
+            else if (stResponse.status !== 403) {
                 Helpers.storeResult(false, false, {reqId: "4.2.0.0-2", msg: `Statement request status code: ${stResponse.status} ${stContent} (Testing ${reqId})`});
 
                 return false;
             }
         }
         catch (ex) {
-            // request should succeed, but provide a 401 or 403, so an exception here is an error
+            // request should succeed so an exception here is an error
             Helpers.storeResult(false, true, {reqId, msg: `Failed request to store statement: ${ex}`});
 
             return false;

diff --git a/player/service/plugins/routes/lrs.js b/player/service/plugins/routes/lrs.js
@@ -388,6 +388,9 @@ const Boom = require("@hapi/boom"),
         }
         else if (resource === "agents") {
             // all agents requests require an actor, and that actor must be the launch actor
+
+            let parsedAgent;
+
             try {
                 parsedAgent = JSON.parse(req.query.agent);
             }
@@ -400,7 +403,7 @@ const Boom = require("@hapi/boom"),
         else if (resource === "agents/profile") {
             // all agents/profile requests require an actor,
             if (typeof req.query.agent === "undefined") {
-                throw Helpers.buildViolatedReqId("11.0.0.0-1", "'agent' parameter missing");
+                throw Helpers.buildViolatedReqId("11.0.0.0-1", "'agent' parameter missing", "badRequest");
             }
 
             let parsedAgent;
-Original file line number
+Diff line change
@@ Expand Up / @@ -49,7 +49,7 @@ const execute = async () => { @@
             return;
         }
-        Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with either 401 or 403 response status"});
+        Helpers.storeResult(true, false, {msg: "LMS rejected set of statements with 403 response status"});
         await Helpers.closeAU(cmi5);
     };
@@ Expand Down @@