From b5d4038925c43d8727d9f9dffb4bda6c75bdaa74 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Tue, 10 May 2022 14:49:08 -0400 Subject: [PATCH 01/13] LRS-70 sketch out current multipart validation behavior with some tests --- .../xapi/statements/attachment_test.cljc | 78 +++++++++++++++++++ src/test/com/yetanalytics/test_runner.cljc | 2 + 2 files changed, 80 insertions(+) create mode 100644 src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc diff --git a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc new file mode 100644 index 00000000..a494d434 --- /dev/null +++ b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc @@ -0,0 +1,78 @@ +(ns com.yetanalytics.lrs.pedestal.interceptor.xapi.statements.attachment-test + (:require [clojure.test :refer [deftest testing is] :include-macros true] + [clojure.spec.test.alpha :as stest :include-macros true] + [com.yetanalytics.test-support :refer [failures stc-opts]] + [com.yetanalytics.lrs.pedestal.interceptor.xapi.statements.attachment + :as attachment + :refer [validate-statements-multiparts]]) + #?(:clj (:import [java.io ByteArrayInputStream]))) + +(deftest validate-statements-multiparts-test + (let [s-template {"id" "78efaab3-1c65-4cb7-9289-f34e0594b274" + "actor" {"mbox" "mailto:bob@example.com" + "objectType" "Agent"} + "verb" {"id" "https://example.com/verb"} + "timestamp" "2022-05-04T13:32:10.486195Z" + "version" "1.0.3" + "object" {"id" "https://example.com/activity"}} + multipart {:content-type "application/octet-stream" + :content-length 20 + :headers {"Content-Type" "application/octet-stream" + "Content-Transfer-Encoding" "binary" + "X-Experience-API-Hash" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"} + :input-stream #?(:clj (ByteArrayInputStream. + (.getBytes "some text\n some more" "UTF-8")) + :cljs "some text\n some more")}] + (testing "empty" + (is (= [[] []] + (validate-statements-multiparts + [] + [])))) + + (testing "simple" + (let [statements + [(assoc s-template + "attachments" + [{"usageType" "https://example.com/usagetype" + "display" {"en-US" "someattachment"} + "contentType" "application/octet-stream" + "length" 20 + "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])] + multiparts + [multipart]] + (is (= [statements multiparts] + (validate-statements-multiparts + statements + multiparts))))) + (testing "dup reference" + ;; TODO: per this test, the lib currently requires that each referenced + ;; multipart be provided, even if they share a SHA. + ;; Is this what we intend? + (let [statements + [(assoc s-template + "attachments" + [{"usageType" "https://example.com/usagetype" + "display" {"en-US" "someattachment"} + "contentType" "application/octet-stream" + "length" 20 + "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"} + {"usageType" "https://example.com/usagetype" + "display" {"en-US" "someattachment"} + "contentType" "application/octet-stream" + "length" 20 + "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])] + multiparts + [multipart + multipart]] + (testing "works with a dup multipart" + (is (= [statements multiparts] + (validate-statements-multiparts + statements + multiparts)))) + (testing "fails with a single multipart" + (is (= ::attachment/invalid-multipart-format + (try (validate-statements-multiparts + statements + [multipart]) + (catch clojure.lang.ExceptionInfo exi + (-> exi ex-data :type)))))))))) diff --git a/src/test/com/yetanalytics/test_runner.cljc b/src/test/com/yetanalytics/test_runner.cljc index bb7fd132..a5685317 100644 --- a/src/test/com/yetanalytics/test_runner.cljc +++ b/src/test/com/yetanalytics/test_runner.cljc @@ -14,6 +14,7 @@ com.yetanalytics.lrs.impl.memory-test com.yetanalytics.lrs.pedestal.http.multipart-mixed-test com.yetanalytics.lrs.auth-test + com.yetanalytics.lrs.pedestal.interceptor.xapi.statements.attachment-test com.yetanalytics.lrs.pedestal.interceptor.xapi.statements.attachment.response-test)) (defmethod test/report #?(:cljs [::test/default :begin-test-ns] @@ -51,6 +52,7 @@ 'com.yetanalytics.lrs.impl.memory-test 'com.yetanalytics.lrs.pedestal.http.multipart-mixed-test 'com.yetanalytics.lrs.auth-test + 'com.yetanalytics.lrs.pedestal.interceptor.xapi.statements.attachment-test 'com.yetanalytics.lrs.pedestal.interceptor.xapi.statements.attachment.response-test)) (defn ^:export -main [] From 6cefe99d200129a67ae2e2d6600cd14323c52953 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Tue, 10 May 2022 14:53:55 -0400 Subject: [PATCH 02/13] LRS-70 add cljs dispatch for exinfo --- .../pedestal/interceptor/xapi/statements/attachment_test.cljc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc index a494d434..84638577 100644 --- a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc +++ b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc @@ -74,5 +74,6 @@ (try (validate-statements-multiparts statements [multipart]) - (catch clojure.lang.ExceptionInfo exi + (catch #?(:clj clojure.lang.ExceptionInfo + :cljs ExceptionInfo) exi (-> exi ex-data :type)))))))))) From 46a3eb993f1c34a371b282096f7092592755fc30 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Tue, 10 May 2022 15:53:58 -0400 Subject: [PATCH 03/13] fix requires in lrs-test --- src/test/com/yetanalytics/lrs_test.clj | 100 ++++++++++++------------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/src/test/com/yetanalytics/lrs_test.clj b/src/test/com/yetanalytics/lrs_test.clj index 7a8053a0..fb01a555 100644 --- a/src/test/com/yetanalytics/lrs_test.clj +++ b/src/test/com/yetanalytics/lrs_test.clj @@ -2,7 +2,7 @@ (:require [clojure.test :refer [deftest testing is]] [com.yetanalytics.test-support :as support :refer [deftest-check-ns]] [com.yetanalytics.lrs.impl.memory :as mem] - [com.yetanalytics.lrs :refer [get-statements store-statements]] + [com.yetanalytics.lrs :as lrs] [clojure.string :as cs] [com.yetanalytics.datasim.input :as sim-input] [com.yetanalytics.datasim.sim :as sim] @@ -27,32 +27,32 @@ :input :json "dev-resources/datasim/input/tc3.json")))) ;; instrument LRS api fns throughout tests -(stest/instrument `[get-about - get-about-async - set-document - set-document-async - get-document - get-document-async - get-document-ids - get-document-ids-async - delete-document - delete-document-async - delete-documents - delete-documents-async - get-activity - get-activity-async - get-person - get-person-async - store-statements - store-statements-async - get-statements - get-statements-async - consistent-through - consistent-through-async - authenticate - authenticate-async - authorize - authorize-async]) +(stest/instrument `[lrs/get-about + lrs/get-about-async + lrs/set-document + lrs/set-document-async + lrs/get-document + lrs/get-document-async + lrs/get-document-ids + lrs/get-document-ids-async + lrs/delete-document + lrs/delete-document-async + lrs/delete-documents + lrs/delete-documents-async + lrs/get-activity + lrs/get-activity-async + lrs/get-person + lrs/get-person-async + lrs/store-statements + lrs/store-statements-async + lrs/get-statements + lrs/get-statements-async + lrs/consistent-through + lrs/consistent-through-async + lrs/authenticate + lrs/authenticate-async + lrs/authorize + lrs/authorize-async]) (deftest query-test (let [auth-id {:auth {:no-op {}} @@ -66,12 +66,12 @@ ;; no normalization going on s-count 100 lrs (doto (mem/new-lrs {:statements-result-max s-count}) - (store-statements auth-id - (into [] (take s-count) - test-statements) - [])) + (lrs/store-statements auth-id + (into [] (take s-count) + test-statements) + [])) get-ss #(into [] - (get-in (get-statements lrs auth-id % #{"en-US"}) + (get-in (lrs/get-statements lrs auth-id % #{"en-US"}) [:statement-result :statements])) ret-statements (get-ss {:limit 100})] (testing (format "%s valid return statements?" (count ret-statements)) @@ -166,10 +166,10 @@ (get "id"))] (is (:statement - (get-statements lrs - auth-id - {:statementId (cs/upper-case id)} - #{"en-US"}))))) + (lrs/get-statements lrs + auth-id + {:statementId (cs/upper-case id)} + #{"en-US"}))))) (testing "registration param is normalized" (let [reg (-> ret-statements @@ -178,30 +178,30 @@ (is reg) (is (not-empty - (get-in (get-statements lrs - auth-id - {:registration (cs/upper-case reg)} - #{"en-US"}) + (get-in (lrs/get-statements lrs + auth-id + {:registration (cs/upper-case reg)} + #{"en-US"}) [:statement-result :statements]))))) (testing "ID keys are normalized" (let [s (first test-statements) id (get s "id") lrs (doto (mem/new-lrs {:statements-result-max s-count}) - (store-statements + (lrs/store-statements auth-id [(-> s (update "id" cs/upper-case) (update-in ["context" "registration"] cs/upper-case))] []))] - (is (:statement (get-statements + (is (:statement (lrs/get-statements lrs auth-id {:statementId id} #{"en-US"}))) - ;; This test will pass even w/o normalized IDs, but it makes sure we - ;; don't screw up the rel index - (is (not-empty (get-in (get-statements + ;; This test will pass even w/o normalized IDs, but it makes sure we + ;; don't screw up the rel index + (is (not-empty (get-in (lrs/get-statements lrs auth-id {:verb (get-in s ["verb" "id"])} @@ -209,7 +209,7 @@ [:statement-result :statements]))) (testing "reg index" - (is (not-empty (get-in (get-statements + (is (not-empty (get-in (lrs/get-statements lrs auth-id {:registration (get-in s ["context" @@ -219,8 +219,8 @@ (testing "original case is preserved" (is (= (cs/upper-case id) - (get-in (get-statements lrs - auth-id - {:statementId id} - #{"en-US"}) + (get-in (lrs/get-statements lrs + auth-id + {:statementId id} + #{"en-US"}) [:statement "id"])))))))) From 8df0ef8b39570809b47281181383d2826faa5790 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Wed, 11 May 2022 14:36:33 -0400 Subject: [PATCH 04/13] LRS-72 clean up and refactor in prep to decouple sig checking --- .../xapi/statements/attachment.cljc | 104 +++++++----------- .../xapi/statements/attachment_test.cljc | 80 ++++++++++++-- 2 files changed, 115 insertions(+), 69 deletions(-) diff --git a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc index 81275d76..43611ae5 100644 --- a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc +++ b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc @@ -133,35 +133,46 @@ (defn validate-sig "Validate a signed statement against a multipart and return if valid." - [statement {:keys [#?(:clj ^InputStream input-stream - :cljs ^String input-stream)] :as multipart}] - (let [^String jws - #?(:clj (with-open [in input-stream] - (slurp in :encoding "UTF-8")) - :cljs input-stream) - {:keys [headers payload]} - (decode-sig jws) - {:keys [alg _typ]} - headers] - (cond - (not (#{"RS256" "RS384" "RS512"} - alg)) - (throw (ex-info "JWS Algorithm MUST be RS256, RS384, or RS512" - {:type ::invalid-signature-alg - :jws jws - :statement statement})) - (not (ss/statements-immut-equal? - (dissoc statement "attachments") - payload)) - (throw (ex-info "Statement signature does not match statement" - {:type ::invalid-signature-mismatch - :jws jws - :statement statement})) - :else ; If everything's good, return the multipart w/ a new input stream - (assoc multipart - :input-stream - #?(:clj (ByteArrayInputStream. (.getBytes jws "UTF-8")) - :cljs jws))))) + [statement + attachment-object + {:keys [#?(:clj ^InputStream input-stream + :cljs ^String input-stream)] :as multipart}] + (if-not (= (:content-type multipart) + (get attachment-object "contentType") + "application/octet-stream") + (throw + (ex-info + "Statement signature attachment contentType must be application/octet-stream" + {:type ::invalid-signature-attachment-content-type + :attachment-object attachment-object + :attachment-multipart multipart})) + (let [^String jws + #?(:clj (with-open [in input-stream] + (slurp in :encoding "UTF-8")) + :cljs input-stream) + {:keys [headers payload]} + (decode-sig jws) + {:keys [alg _typ]} + headers] + (cond + (not (#{"RS256" "RS384" "RS512"} + alg)) + (throw (ex-info "JWS Algorithm MUST be RS256, RS384, or RS512" + {:type ::invalid-signature-alg + :jws jws + :statement statement})) + (not (ss/statements-immut-equal? + (dissoc statement "attachments") + payload)) + (throw (ex-info "Statement signature does not match statement" + {:type ::invalid-signature-mismatch + :jws jws + :statement statement})) + :else ; If everything's good, return the multipart w/ a new input stream + (assoc multipart + :input-stream + #?(:clj (ByteArrayInputStream. (.getBytes jws "UTF-8")) + :cljs jws)))))) (defn multipart-map "Given a list of multiparts, make a map of vectors of them by xapi hash" @@ -183,24 +194,13 @@ (defn make-sha-multipart-pair "Given the attachment object `att-obj`, return a pair of its SHA and the multipart, or `nil` if `att-obj` is a fileURL." - [sig multi-parts att-obj] + [statement multi-parts att-obj] ;; If we match... (if-let [[sha mps] (find multi-parts (get att-obj "sha2"))] (let [mp (first mps)] ;; If it's a signature... (if (sig? att-obj) - (if (= (:content-type mp) - (get att-obj "contentType") - "application/octet-stream") - ;; If the ctype is valid, validate the sig - [sha (validate-sig sig mp)] - ;; If that ctype was wrong, throw - (throw - (ex-info - "Statement signature attachment contentType must be application/octet-stream" - {:type ::invalid-signature-attachment-content-type - :attachment-object att-obj - :attachment-multipart mp}))) + [sha (validate-sig statement att-obj mp)] ;; If not, return the sha and multipart [sha mp])) ;; If we don't, this better be a fileURL @@ -255,23 +255,3 @@ :leftover-multiparts (into [] leftover-multiparts)})) ;; If not, let's return the statements and multiparts [valid-statements valid-multiparts]))) - -;; The following comment block is left for dev purposes -(comment - - (def s-json "{\"actor\":{\"objectType\":\"Agent\",\"name\":\"xAPI mbox\",\"mbox\":\"mailto:xapi@adlnet.gov\"},\"verb\":{\"id\":\"http://adlnet.gov/expapi/verbs/attended\",\"display\":{\"en-GB\":\"attended\",\"en-US\":\"attended\"}},\"object\":{\"objectType\":\"Activity\",\"id\":\"http://www.example.com/meetings/occurances/34534\"},\"id\":\"2e2f1ad7-8d10-4c73-ae6e-2842729e25ce\",\"attachments\":[{\"usageType\":\"http://adlnet.gov/expapi/attachments/signature\",\"display\":{\"en-US\":\"Signed by the Test Suite\"},\"description\":{\"en-US\":\"Signed by the Test Suite\"},\"contentType\":\"application/octet-stream\",\"length\":796,\"sha2\":\"f7db3634a22ea2fe4de1fc519751046a3bdf1e5605a316a19343109bd6daa388\"}]}") - - (def sig "eyJhbGciOiJSUzI1NiJ9.eyJhY3RvciI6eyJvYmplY3RUeXBlIjoiQWdlbnQiLCJuYW1lIjoieEFQSSBtYm94IiwibWJveCI6Im1haWx0bzp4YXBpQGFkbG5ldC5nb3YifSwidmVyYiI6eyJpZCI6Imh0dHA6Ly9hZGxuZXQuZ292L2V4cGFwaS92ZXJicy9hdHRlbmRlZCIsImRpc3BsYXkiOnsiZW4tR0IiOiJhdHRlbmRlZCIsImVuLVVTIjoiYXR0ZW5kZWQifX0sIm9iamVjdCI6eyJvYmplY3RUeXBlIjoiQWN0aXZpdHkiLCJpZCI6Imh0dHA6Ly93d3cuZXhhbXBsZS5jb20vbWVldGluZ3Mvb2NjdXJhbmNlcy8zNDUzNCJ9LCJpZCI6IjJlMmYxYWQ3LThkMTAtNGM3My1hZTZlLTI4NDI3MjllMjVjZSJ9.roBpi7viDC4DyNikcWtjuvfXEfrVqNtukVfOjoj-VEGbskcxc9H21GKQBsw3LxnpblIpiDPithCs2AOZK7RFy4vB9wsL5HmX8jpxGvGnYCWNEbVRGoYyntFWjF3wFtTaJMHvZLnirL6k1qhxdfJPcV2C-uc-FXC9AR4__xYbJioJDb37wvPtetD8x8YTdkMkM7nlv20GjV3YF-wa_cxt9hWVS-8LDikCswY6PpMLFR6eYeqIqrZxJQtqDhsZK3k28eHDxAnNB-dGoYeiSeFSbcToyVh4iz2lZGNUmfkltiVs7mLTVJNilU0Z41JIFrdYEGXEfYQwFmiIf5denL5_lg") - - (def s-parsed (parse-string s-json)) - - (= (dissoc s-parsed "attachments") (:payload (decode-sig sig))) - - (keys s-parsed) - - (validate-sig s-parsed - {:input-stream sig}) - - #?(:clj - (= (json/parse-string-strict s-json) - (json/parse-string s-json)))) diff --git a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc index 84638577..0d74abb5 100644 --- a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc +++ b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc @@ -4,9 +4,70 @@ [com.yetanalytics.test-support :refer [failures stc-opts]] [com.yetanalytics.lrs.pedestal.interceptor.xapi.statements.attachment :as attachment - :refer [validate-statements-multiparts]]) + :refer [decode-sig + validate-sig + validate-statements-multiparts]]) #?(:clj (:import [java.io ByteArrayInputStream]))) +(deftest sig-test + (let [att-obj {"usageType" "http://adlnet.gov/expapi/attachments/signature", + "display" {"en-US" "Signed by the Test Suite"}, + "description" {"en-US" "Signed by the Test Suite"}, + "contentType" "application/octet-stream", + "length" 796, + "sha2" + "f7db3634a22ea2fe4de1fc519751046a3bdf1e5605a316a19343109bd6daa388"} + statement {"actor" + {"objectType" "Agent", + "name" "xAPI mbox", + "mbox" "mailto:xapi@adlnet.gov"}, + "verb" + {"id" "http://adlnet.gov/expapi/verbs/attended", + "display" {"en-GB" "attended", "en-US" "attended"}}, + "object" + {"objectType" "Activity", + "id" "http://www.example.com/meetings/occurances/34534"}, + "id" "2e2f1ad7-8d10-4c73-ae6e-2842729e25ce", + "attachments" + [att-obj]} + sig "eyJhbGciOiJSUzI1NiJ9.eyJhY3RvciI6eyJvYmplY3RUeXBlIjoiQWdlbnQiLCJuYW1lIjoieEFQSSBtYm94IiwibWJveCI6Im1haWx0bzp4YXBpQGFkbG5ldC5nb3YifSwidmVyYiI6eyJpZCI6Imh0dHA6Ly9hZGxuZXQuZ292L2V4cGFwaS92ZXJicy9hdHRlbmRlZCIsImRpc3BsYXkiOnsiZW4tR0IiOiJhdHRlbmRlZCIsImVuLVVTIjoiYXR0ZW5kZWQifX0sIm9iamVjdCI6eyJvYmplY3RUeXBlIjoiQWN0aXZpdHkiLCJpZCI6Imh0dHA6Ly93d3cuZXhhbXBsZS5jb20vbWVldGluZ3Mvb2NjdXJhbmNlcy8zNDUzNCJ9LCJpZCI6IjJlMmYxYWQ3LThkMTAtNGM3My1hZTZlLTI4NDI3MjllMjVjZSJ9.roBpi7viDC4DyNikcWtjuvfXEfrVqNtukVfOjoj-VEGbskcxc9H21GKQBsw3LxnpblIpiDPithCs2AOZK7RFy4vB9wsL5HmX8jpxGvGnYCWNEbVRGoYyntFWjF3wFtTaJMHvZLnirL6k1qhxdfJPcV2C-uc-FXC9AR4__xYbJioJDb37wvPtetD8x8YTdkMkM7nlv20GjV3YF-wa_cxt9hWVS-8LDikCswY6PpMLFR6eYeqIqrZxJQtqDhsZK3k28eHDxAnNB-dGoYeiSeFSbcToyVh4iz2lZGNUmfkltiVs7mLTVJNilU0Z41JIFrdYEGXEfYQwFmiIf5denL5_lg" + multipart {:content-type "application/octet-stream" + :content-length 796 + :headers {"Content-Type" "application/octet-stream" + "Content-Transfer-Encoding" "binary" + "X-Experience-API-Hash" "f7db3634a22ea2fe4de1fc519751046a3bdf1e5605a316a19343109bd6daa388"}}] + (testing "decode-sig" + (= (dissoc statement "attachments") + (decode-sig sig))) + (testing "valid-sig" + (is + (= sig + (-> (validate-sig + statement + att-obj + (assoc multipart + :input-stream + #?(:clj (ByteArrayInputStream. + (.getBytes sig "UTF-8")) + :cljs sig))) + :input-stream + slurp)))) + (testing "invalid-sig" + (let [invalid-sig (apply str (drop 10 sig))] + (is (= ::attachment/invalid-signature-json + (try + (validate-sig + statement + att-obj + (assoc multipart + :input-stream + #?(:clj (ByteArrayInputStream. + (.getBytes invalid-sig "UTF-8")) + :cljs invalid-sig))) + (catch #?(:clj clojure.lang.ExceptionInfo + :cljs ExceptionInfo) exi + (-> exi ex-data :type))))))))) + (deftest validate-statements-multiparts-test (let [s-template {"id" "78efaab3-1c65-4cb7-9289-f34e0594b274" "actor" {"mbox" "mailto:bob@example.com" @@ -60,15 +121,20 @@ "display" {"en-US" "someattachment"} "contentType" "application/octet-stream" "length" 20 - "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])] - multiparts - [multipart - multipart]] + "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])]] (testing "works with a dup multipart" - (is (= [statements multiparts] + (is (= [statements [multipart + multipart]] (validate-statements-multiparts statements - multiparts)))) + [multipart + multipart])))) + #_(testing "works with a dedup multipart" + (is (= [statements [multipart]] + (validate-statements-multiparts + statements + [multipart + multipart])))) (testing "fails with a single multipart" (is (= ::attachment/invalid-multipart-format (try (validate-statements-multiparts From e76daf6c3aa77ff5a4a395419d63bc051911b429 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Wed, 11 May 2022 14:40:21 -0400 Subject: [PATCH 05/13] LRS-72 don't slurp in cljs --- .../pedestal/interceptor/xapi/statements/attachment_test.cljc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc index 0d74abb5..30a65bfe 100644 --- a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc +++ b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc @@ -51,7 +51,7 @@ (.getBytes sig "UTF-8")) :cljs sig))) :input-stream - slurp)))) + #?(:clj slurp))))) (testing "invalid-sig" (let [invalid-sig (apply str (drop 10 sig))] (is (= ::attachment/invalid-signature-json From 94a9d44cdfe5a1c1905f7b1b37306ecce8d85f26 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Wed, 11 May 2022 15:26:12 -0400 Subject: [PATCH 06/13] LRS-72 make an xapi fn for pulling att objects generally with statement + path --- .../com/yetanalytics/lrs/xapi/statements.cljc | 84 +++++++++++++------ .../lrs/xapi/statements_test.cljc | 6 ++ 2 files changed, 64 insertions(+), 26 deletions(-) diff --git a/src/main/com/yetanalytics/lrs/xapi/statements.cljc b/src/main/com/yetanalytics/lrs/xapi/statements.cljc index 1d9a2398..7b086809 100644 --- a/src/main/com/yetanalytics/lrs/xapi/statements.cljc +++ b/src/main/com/yetanalytics/lrs/xapi/statements.cljc @@ -535,32 +535,6 @@ (defn voiding-statement? [s] (some-> s (get-in ["verb" "id"]) (= "http://adlnet.gov/expapi/verbs/voided"))) -(defn all-attachment-hashes - "For each statement, get any attachment hashes. If skip-file-urls is true, - will only return sha2s from attachments w/o fileURL." - [statements & [skip-file-urls]] - (distinct - (keep - (fn [{:strs [sha2 fileUrl] :as _attachment}] - (if skip-file-urls - (when-not fileUrl - sha2) - sha2)) - (mapcat - (fn [{:strs [attachments - object] - :as _statement}] - (concat attachments - (get object "attachments"))) - statements)))) - -(s/fdef all-attachment-hashes - :args (s/cat :statements - (s/coll-of ::xs/statement) - :skip-file-urls - (s/? boolean?)) - :ret (s/coll-of :attachment/sha2)) - ;; A representation of a stored attachment ;; TODO: generalize (s/def :attachment/content @@ -594,6 +568,64 @@ (s/def ::attachments (s/coll-of ::attachment)) +(s/def ::attachment-path + (s/cat + :root-or-sub (s/? #{"object"}) + :attachment-key #{"attachments"} + :index nat-int?)) + +(defn all-attachment-objects + "Given a collection of statements, return a lazy seq of maps containing: + :statement - the containing statement + :attachment - the attachment object + :attachment-path - the path of the attachment object in the statement" + [statements] + (mapcat + (fn [{:strs [attachments + object] + :as statement}] + (concat (for [[idx att] (map-indexed vector attachments)] + {:statement statement + :attachment att + :attachment-path ["attachments" idx]}) + (for [[idx att] (map-indexed vector + (get object "attachments"))] + {:statement statement + :attachment att + :attachment-path ["object" "attachments" idx]}))) + statements)) + +(s/fdef all-attachment-objects + :args (s/cat :statements + (s/coll-of ::xs/statement)) + :ret (s/coll-of + (s/keys :req-un + [::xs/statement + ::xs/attachment + ::attachment-path]))) + +(defn all-attachment-hashes + "For each statement, get any attachment hashes. If skip-file-urls is true, + will only return sha2s from attachments w/o fileURL." + [statements & [skip-file-urls]] + (distinct + (keep + (fn [{:strs [sha2 fileUrl] :as _attachment}] + (if skip-file-urls + (when-not fileUrl + sha2) + sha2)) + (map + :attachment + (all-attachment-objects statements))))) + +(s/fdef all-attachment-hashes + :args (s/cat :statements + (s/coll-of ::xs/statement) + :skip-file-urls + (s/? boolean?)) + :ret (s/coll-of :attachment/sha2)) + (defn statement-rel-docs "Get related activities and agents" [statement] diff --git a/src/test/com/yetanalytics/lrs/xapi/statements_test.cljc b/src/test/com/yetanalytics/lrs/xapi/statements_test.cljc index a4b116e9..2b031195 100644 --- a/src/test/com/yetanalytics/lrs/xapi/statements_test.cljc +++ b/src/test/com/yetanalytics/lrs/xapi/statements_test.cljc @@ -137,6 +137,12 @@ (stest/check `ss/statement-ref? {stc-opts {:num-tests 10 :max-size 3}}))))) +(deftest all-attachment-objects-test + (is (empty? + (failures + (stest/check `ss/all-attachment-objects + {stc-opts {:num-tests 2 :max-size 2}}))))) + (deftest all-attachment-hashes-test (is (empty? (failures From ceee77be91b07325f41cc31f96ae4f4afd80e7c4 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Wed, 11 May 2022 15:50:44 -0400 Subject: [PATCH 07/13] LRS-72 additional testing on current multipart behavior --- .../xapi/statements/attachment_test.cljc | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc index 30a65bfe..a5939e69 100644 --- a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc +++ b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc @@ -123,18 +123,33 @@ "length" 20 "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])]] (testing "works with a dup multipart" - (is (= [statements [multipart - multipart]] + (is (= [statements [multipart multipart]] (validate-statements-multiparts statements [multipart multipart])))) - #_(testing "works with a dedup multipart" - (is (= [statements [multipart]] - (validate-statements-multiparts - statements - [multipart - multipart])))) + (testing "fails with left over multiparts" + (is (= ::attachment/statement-attachment-mismatch + (try (validate-statements-multiparts + [s-template] + [multipart]) + (catch #?(:clj clojure.lang.ExceptionInfo + :cljs ExceptionInfo) exi + (-> exi ex-data :type))))) + (is (= ::attachment/statement-attachment-mismatch + (try (validate-statements-multiparts + [(assoc s-template + "attachments" + [{"usageType" "https://example.com/usagetype" + "display" {"en-US" "someattachment"} + "contentType" "application/octet-stream" + "length" 20 + "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])] + [multipart + multipart]) + (catch #?(:clj clojure.lang.ExceptionInfo + :cljs ExceptionInfo) exi + (-> exi ex-data :type)))))) (testing "fails with a single multipart" (is (= ::attachment/invalid-multipart-format (try (validate-statements-multiparts From d5b1023e0a5474444b9ef5e8f26d1cbefe7fa2fd Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Wed, 11 May 2022 16:54:11 -0400 Subject: [PATCH 08/13] LRS-72 validate multiparts, handle dup input and dedupe output --- .../pedestal/interceptor/xapi/statements.cljc | 8 +- .../xapi/statements/attachment.cljc | 136 +++++++----------- .../xapi/statements/attachment_test.cljc | 69 ++++----- 3 files changed, 83 insertions(+), 130 deletions(-) diff --git a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements.cljc b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements.cljc index 3026bd78..5640028b 100644 --- a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements.cljc +++ b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements.cljc @@ -186,8 +186,8 @@ (if-let [statement-data (get-in ctx [:request :json-params])] (try (condp s/valid? statement-data ::xs/statement - (let [[_ valid-multiparts] - (attachment/validate-statements-multiparts + (let [valid-multiparts + (attachment/validate-multiparts [statement-data] multiparts) attachment-data @@ -199,8 +199,8 @@ ::xs/statement statement-data :xapi.statements/attachments attachment-data)) ::xs/statements - (let [[_ valid-multiparts] - (attachment/validate-statements-multiparts + (let [valid-multiparts + (attachment/validate-multiparts statement-data multiparts) attachment-data diff --git a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc index 43611ae5..06aa9163 100644 --- a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc +++ b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc @@ -85,17 +85,6 @@ [multiparts] (mapv save-attachment multiparts)) -(defn statements-attachments - "For each statement, get any attachment objects, - and make of them a seq of seqs." - [statements] - (for [{:strs [attachments - object] - :as statement} statements] - (->> (get object "attachments") - (concat attachments) - (cons statement)))) - (defn delete-attachments! "Delete all tempfiles for a sequence of attachments" [attachments] @@ -162,6 +151,7 @@ :jws jws :statement statement})) (not (ss/statements-immut-equal? + ;; TODO: make sure this doesn't trigger on substatement paths (dissoc statement "attachments") payload)) (throw (ex-info "Statement signature does not match statement" @@ -174,84 +164,68 @@ #?(:clj (ByteArrayInputStream. (.getBytes jws "UTF-8")) :cljs jws)))))) -(defn multipart-map - "Given a list of multiparts, make a map of vectors of them by xapi hash" - [multiparts] - (reduce (fn [m {:keys [headers] :as multipart}] - (update m - (get headers "X-Experience-API-Hash") - (fnil conj []) - multipart)) - {} - multiparts)) - (defn sig? "Predicate, returns true if the given attachment object is a signature" [attachment-object] (= "http://adlnet.gov/expapi/attachments/signature" (get attachment-object "usageType"))) -(defn make-sha-multipart-pair - "Given the attachment object `att-obj`, return a pair of its SHA and the - multipart, or `nil` if `att-obj` is a fileURL." - [statement multi-parts att-obj] - ;; If we match... - (if-let [[sha mps] (find multi-parts (get att-obj "sha2"))] - (let [mp (first mps)] - ;; If it's a signature... - (if (sig? att-obj) - [sha (validate-sig statement att-obj mp)] - ;; If not, return the sha and multipart - [sha mp])) - ;; If we don't, this better be a fileURL - (when-not (get att-obj "fileUrl") - (throw (ex-info "Invalid multipart format" - {:type ::invalid-multipart-format}))))) +(defn multipart-map-dedupe + "Given a list of multiparts, make a map of vectors of them by xapi hash. + Removes duplicate multiparts." + [multiparts] + (reduce (fn [m {:keys [headers] :as multipart}] + (let [sha (get headers "X-Experience-API-Hash")] + (if-let [extant (get m sha)] + m + (assoc m sha multipart)))) + {} + multiparts)) -(defn validate-statements-multiparts - "Validate and return statements and their multipart attachments" - [statements multiparts] - (let [;; collect the attachments per statement and reduce over them - {valid-statements :s-acc - valid-multiparts :a-acc - leftover-multiparts :mps} +(defn validate-multiparts + "Given a list of statements and a list of multiparts, return valid multiparts, + deduplicated." + [statements + multiparts] + (let [{:keys [sha2s + mpart-map]} (reduce - (fn [{:keys [mps] :as m} - [s & att-objs]] - ;; match the attachment objects to a multipart - ;; or assert that they are a fileURL - (let [;; Reduce over multiparts and find relevant attachments - [next-mps - valid-matched] - (reduce - (fn [[mps' acc :as state] ao] - (if-let [[sha mp :as match] - (make-sha-multipart-pair - s mps' ao)] - [(if (< 1 (count (get mps' sha))) - (update mps' sha #(into [] (rest %))) - (dissoc mps' sha)) - (conj acc match)] - state)) - [mps []] - att-objs) - - valid-shas (map first valid-matched)] - (-> m - (update :s-acc conj s) - (update :a-acc into (map second valid-matched)) - (assoc :mps - next-mps)))) - {;; accumulators for the statements + attachments - :s-acc [] - :a-acc [] - ;; the multiparts to join up, by Hash - :mps (multipart-map multiparts)} - (statements-attachments statements))] - (if (seq leftover-multiparts) - ;; If we have leftovers, it's bad + (fn [{:keys [sha2s mpart-map] + :as state} + {:keys [statement + attachment-path] + {:strs [sha2 fileUrl] + :as att-obj} :attachment}] + (if-let [match-mp (get mpart-map sha2)] + (cond-> (update state :sha2s conj sha2) + ;; Validate + recreate sig mp + (and (= "attachments" + (first attachment-path)) + (sig? att-obj)) + (update-in + [:mpart-map sha2] + (partial validate-sig + statement + att-obj))) + ;; Allow attachments w/o a fileUrl to silently drop + (if fileUrl + state + (throw + ;; TODO: name of this error? + (ex-info "Invalid multipart format" + {:type ::invalid-multipart-format}))))) + {:sha2s [] + :mpart-map (multipart-map-dedupe multiparts)} + (ss/all-attachment-objects statements)) + sha2s-out (distinct sha2s)] + (if-let [leftover-multiparts (-> (apply dissoc + mpart-map + sha2s-out) + not-empty + vals)] (throw (ex-info "Attachment sha2s differ from statement sha2s" {:type ::statement-attachment-mismatch :leftover-multiparts (into [] leftover-multiparts)})) - ;; If not, let's return the statements and multiparts - [valid-statements valid-multiparts]))) + (into [] + (for [sha2 sha2s-out] + (get mpart-map sha2)))))) diff --git a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc index a5939e69..cb5dbae4 100644 --- a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc +++ b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc @@ -6,7 +6,7 @@ :as attachment :refer [decode-sig validate-sig - validate-statements-multiparts]]) + validate-multiparts]]) #?(:clj (:import [java.io ByteArrayInputStream]))) (deftest sig-test @@ -68,7 +68,7 @@ :cljs ExceptionInfo) exi (-> exi ex-data :type))))))))) -(deftest validate-statements-multiparts-test +(deftest validate-multiparts-test (let [s-template {"id" "78efaab3-1c65-4cb7-9289-f34e0594b274" "actor" {"mbox" "mailto:bob@example.com" "objectType" "Agent"} @@ -85,26 +85,22 @@ (.getBytes "some text\n some more" "UTF-8")) :cljs "some text\n some more")}] (testing "empty" - (is (= [[] []] - (validate-statements-multiparts + (is (= [] + (validate-multiparts [] [])))) (testing "simple" - (let [statements - [(assoc s-template - "attachments" - [{"usageType" "https://example.com/usagetype" - "display" {"en-US" "someattachment"} - "contentType" "application/octet-stream" - "length" 20 - "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])] - multiparts - [multipart]] - (is (= [statements multiparts] - (validate-statements-multiparts - statements - multiparts))))) + (is (= [multipart] + (validate-multiparts + [(assoc s-template + "attachments" + [{"usageType" "https://example.com/usagetype" + "display" {"en-US" "someattachment"} + "contentType" "application/octet-stream" + "length" 20 + "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])] + [multipart])))) (testing "dup reference" ;; TODO: per this test, the lib currently requires that each referenced ;; multipart be provided, even if they share a SHA. @@ -122,38 +118,21 @@ "contentType" "application/octet-stream" "length" 20 "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])]] - (testing "works with a dup multipart" - (is (= [statements [multipart multipart]] - (validate-statements-multiparts + (testing "works with a dup multipart, deduplicates" + (is (= [multipart] + (validate-multiparts statements [multipart multipart])))) + (testing "works with a dedup multipart" + (is (= [multipart] + (validate-multiparts + statements + [multipart])))) (testing "fails with left over multiparts" (is (= ::attachment/statement-attachment-mismatch - (try (validate-statements-multiparts - [s-template] - [multipart]) - (catch #?(:clj clojure.lang.ExceptionInfo - :cljs ExceptionInfo) exi - (-> exi ex-data :type))))) - (is (= ::attachment/statement-attachment-mismatch - (try (validate-statements-multiparts - [(assoc s-template - "attachments" - [{"usageType" "https://example.com/usagetype" - "display" {"en-US" "someattachment"} - "contentType" "application/octet-stream" - "length" 20 - "sha2" "7e0c4bbe6280e85cf8525dd7afe8d6ffe9051fbc5fadff71d4aded1ba4c74b53"}])] - [multipart - multipart]) - (catch #?(:clj clojure.lang.ExceptionInfo - :cljs ExceptionInfo) exi - (-> exi ex-data :type)))))) - (testing "fails with a single multipart" - (is (= ::attachment/invalid-multipart-format - (try (validate-statements-multiparts - statements + (try (validate-multiparts + [s-template] ;; no attachments [multipart]) (catch #?(:clj clojure.lang.ExceptionInfo :cljs ExceptionInfo) exi From 447162967023c7e7311d83a90d43d67de4d32737 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Thu, 12 May 2022 09:22:37 -0400 Subject: [PATCH 09/13] LRS-72 impl notes for attachment batches --- README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/README.md b/README.md index e059a34f..1d93cc70 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,18 @@ To use the dev profile, which contains all dev/repl/test stuff, use the `:dev` a LRS Applications implementing the `com.yetanalytics.lrs.protocol/-get-statements-async` method return a channel containing results. Since the body is streamed it is not possible to change the status of the request if an error is encountered. Applications can immediately terminate the stream (resulting in a malformed body) by passing `:com.yetanalytics.lrs.protocol/async-error` to the channel. This is preferable to returning a structurally valid response that is missing data. See [this PR](https://github.com/yetanalytics/lrs/pull/78) for more information. +### Statement Attachment Handling & Normalization + +#### `PUT/POST /statements` + +xAPI clients can send statement data with arbitrary file attachments using the `multipart/mixed` Content-Type [per the spec](https://github.com/adlnet/xAPI-Spec/blob/master/xAPI-Communication.md#requirements-for-attachment-statement-batches). An attachment referenced in an xAPI statement (or substatement) that does not have a `fileUrl` property must be included *at least once* in the request body as a part identified by hash. In practice this means that duplicate attachments may be present on the request. `lrs` will normalize/deduplicate attachments before sending them to implementation code such that every attachment in the list has a distinct hash. See [this PR](https://github.com/yetanalytics/lrs/pull/81) for more information. + +Note that requests containing attachments *not* referenced in statement data will fail with a 400 status. + +#### `GET /statements` + +LRS implementations should return attachments in the same normalized form mentioned above, though this is not checked or enforced by `lrs`. + ## Bench Testing Facilities to bench test any LRS with [DATASIM](https://github.com/yetanalytics/datasim) are available. For instance, to bench the in-memory LRS: From 8d95ed13e117e84355ca312f4331de95f74423b2 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Thu, 12 May 2022 09:53:58 -0400 Subject: [PATCH 10/13] LRS-72 added to impl note --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 1d93cc70..3345a331 100644 --- a/README.md +++ b/README.md @@ -37,7 +37,9 @@ LRS Applications implementing the `com.yetanalytics.lrs.protocol/-get-statements #### `PUT/POST /statements` -xAPI clients can send statement data with arbitrary file attachments using the `multipart/mixed` Content-Type [per the spec](https://github.com/adlnet/xAPI-Spec/blob/master/xAPI-Communication.md#requirements-for-attachment-statement-batches). An attachment referenced in an xAPI statement (or substatement) that does not have a `fileUrl` property must be included *at least once* in the request body as a part identified by hash. In practice this means that duplicate attachments may be present on the request. `lrs` will normalize/deduplicate attachments before sending them to implementation code such that every attachment in the list has a distinct hash. See [this PR](https://github.com/yetanalytics/lrs/pull/81) for more information. +xAPI clients can send statement data with arbitrary file attachments using the `multipart/mixed` Content-Type [per the spec](https://github.com/adlnet/xAPI-Spec/blob/master/xAPI-Communication.md#requirements-for-attachment-statement-batches). An attachment referenced in an xAPI statement (or substatement) that does not have a `fileUrl` property must be included *at least once* in the request body as a part identified by hash. In addition, multiple attachment objects, either in a single statement or across multiple statements in the same request, can refer to the same attachment, as recommended by the spec. + +In practice this means that duplicate attachments *may* be present on the request. `lrs` will normalize/deduplicate attachments before sending them to implementation code such that every attachment in the list has a distinct hash. See [this PR](https://github.com/yetanalytics/lrs/pull/81) for more information. Note that requests containing attachments *not* referenced in statement data will fail with a 400 status. From 437bf7bccdc78863846b5090a657b3fbd0f55b96 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Thu, 12 May 2022 10:01:33 -0400 Subject: [PATCH 11/13] LRS-72 extract big blobs from sig test --- .../xapi/statements/attachment_test.cljc | 122 ++++++++++-------- 1 file changed, 65 insertions(+), 57 deletions(-) diff --git a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc index cb5dbae4..9ba067c4 100644 --- a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc +++ b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc @@ -9,64 +9,72 @@ validate-multiparts]]) #?(:clj (:import [java.io ByteArrayInputStream]))) +(def sig-attachment-object + {"usageType" "http://adlnet.gov/expapi/attachments/signature", + "display" {"en-US" "Signed by the Test Suite"}, + "description" {"en-US" "Signed by the Test Suite"}, + "contentType" "application/octet-stream", + "length" 796, + "sha2" + "f7db3634a22ea2fe4de1fc519751046a3bdf1e5605a316a19343109bd6daa388"}) + +(def sig-statement + {"actor" + {"objectType" "Agent", + "name" "xAPI mbox", + "mbox" "mailto:xapi@adlnet.gov"}, + "verb" + {"id" "http://adlnet.gov/expapi/verbs/attended", + "display" {"en-GB" "attended", "en-US" "attended"}}, + "object" + {"objectType" "Activity", + "id" "http://www.example.com/meetings/occurances/34534"}, + "id" "2e2f1ad7-8d10-4c73-ae6e-2842729e25ce", + "attachments" + [sig-attachment-object]}) + +(def sig + "eyJhbGciOiJSUzI1NiJ9.eyJhY3RvciI6eyJvYmplY3RUeXBlIjoiQWdlbnQiLCJuYW1lIjoieEFQSSBtYm94IiwibWJveCI6Im1haWx0bzp4YXBpQGFkbG5ldC5nb3YifSwidmVyYiI6eyJpZCI6Imh0dHA6Ly9hZGxuZXQuZ292L2V4cGFwaS92ZXJicy9hdHRlbmRlZCIsImRpc3BsYXkiOnsiZW4tR0IiOiJhdHRlbmRlZCIsImVuLVVTIjoiYXR0ZW5kZWQifX0sIm9iamVjdCI6eyJvYmplY3RUeXBlIjoiQWN0aXZpdHkiLCJpZCI6Imh0dHA6Ly93d3cuZXhhbXBsZS5jb20vbWVldGluZ3Mvb2NjdXJhbmNlcy8zNDUzNCJ9LCJpZCI6IjJlMmYxYWQ3LThkMTAtNGM3My1hZTZlLTI4NDI3MjllMjVjZSJ9.roBpi7viDC4DyNikcWtjuvfXEfrVqNtukVfOjoj-VEGbskcxc9H21GKQBsw3LxnpblIpiDPithCs2AOZK7RFy4vB9wsL5HmX8jpxGvGnYCWNEbVRGoYyntFWjF3wFtTaJMHvZLnirL6k1qhxdfJPcV2C-uc-FXC9AR4__xYbJioJDb37wvPtetD8x8YTdkMkM7nlv20GjV3YF-wa_cxt9hWVS-8LDikCswY6PpMLFR6eYeqIqrZxJQtqDhsZK3k28eHDxAnNB-dGoYeiSeFSbcToyVh4iz2lZGNUmfkltiVs7mLTVJNilU0Z41JIFrdYEGXEfYQwFmiIf5denL5_lg") + +(def sig-partial-multipart + {:content-type "application/octet-stream" + :content-length 796 + :headers {"Content-Type" "application/octet-stream" + "Content-Transfer-Encoding" "binary" + "X-Experience-API-Hash" "f7db3634a22ea2fe4de1fc519751046a3bdf1e5605a316a19343109bd6daa388"}}) + (deftest sig-test - (let [att-obj {"usageType" "http://adlnet.gov/expapi/attachments/signature", - "display" {"en-US" "Signed by the Test Suite"}, - "description" {"en-US" "Signed by the Test Suite"}, - "contentType" "application/octet-stream", - "length" 796, - "sha2" - "f7db3634a22ea2fe4de1fc519751046a3bdf1e5605a316a19343109bd6daa388"} - statement {"actor" - {"objectType" "Agent", - "name" "xAPI mbox", - "mbox" "mailto:xapi@adlnet.gov"}, - "verb" - {"id" "http://adlnet.gov/expapi/verbs/attended", - "display" {"en-GB" "attended", "en-US" "attended"}}, - "object" - {"objectType" "Activity", - "id" "http://www.example.com/meetings/occurances/34534"}, - "id" "2e2f1ad7-8d10-4c73-ae6e-2842729e25ce", - "attachments" - [att-obj]} - sig "eyJhbGciOiJSUzI1NiJ9.eyJhY3RvciI6eyJvYmplY3RUeXBlIjoiQWdlbnQiLCJuYW1lIjoieEFQSSBtYm94IiwibWJveCI6Im1haWx0bzp4YXBpQGFkbG5ldC5nb3YifSwidmVyYiI6eyJpZCI6Imh0dHA6Ly9hZGxuZXQuZ292L2V4cGFwaS92ZXJicy9hdHRlbmRlZCIsImRpc3BsYXkiOnsiZW4tR0IiOiJhdHRlbmRlZCIsImVuLVVTIjoiYXR0ZW5kZWQifX0sIm9iamVjdCI6eyJvYmplY3RUeXBlIjoiQWN0aXZpdHkiLCJpZCI6Imh0dHA6Ly93d3cuZXhhbXBsZS5jb20vbWVldGluZ3Mvb2NjdXJhbmNlcy8zNDUzNCJ9LCJpZCI6IjJlMmYxYWQ3LThkMTAtNGM3My1hZTZlLTI4NDI3MjllMjVjZSJ9.roBpi7viDC4DyNikcWtjuvfXEfrVqNtukVfOjoj-VEGbskcxc9H21GKQBsw3LxnpblIpiDPithCs2AOZK7RFy4vB9wsL5HmX8jpxGvGnYCWNEbVRGoYyntFWjF3wFtTaJMHvZLnirL6k1qhxdfJPcV2C-uc-FXC9AR4__xYbJioJDb37wvPtetD8x8YTdkMkM7nlv20GjV3YF-wa_cxt9hWVS-8LDikCswY6PpMLFR6eYeqIqrZxJQtqDhsZK3k28eHDxAnNB-dGoYeiSeFSbcToyVh4iz2lZGNUmfkltiVs7mLTVJNilU0Z41JIFrdYEGXEfYQwFmiIf5denL5_lg" - multipart {:content-type "application/octet-stream" - :content-length 796 - :headers {"Content-Type" "application/octet-stream" - "Content-Transfer-Encoding" "binary" - "X-Experience-API-Hash" "f7db3634a22ea2fe4de1fc519751046a3bdf1e5605a316a19343109bd6daa388"}}] - (testing "decode-sig" - (= (dissoc statement "attachments") - (decode-sig sig))) - (testing "valid-sig" - (is - (= sig - (-> (validate-sig - statement - att-obj - (assoc multipart - :input-stream - #?(:clj (ByteArrayInputStream. - (.getBytes sig "UTF-8")) - :cljs sig))) - :input-stream - #?(:clj slurp))))) - (testing "invalid-sig" - (let [invalid-sig (apply str (drop 10 sig))] - (is (= ::attachment/invalid-signature-json - (try - (validate-sig - statement - att-obj - (assoc multipart - :input-stream - #?(:clj (ByteArrayInputStream. - (.getBytes invalid-sig "UTF-8")) - :cljs invalid-sig))) - (catch #?(:clj clojure.lang.ExceptionInfo - :cljs ExceptionInfo) exi - (-> exi ex-data :type))))))))) + (testing "decode-sig" + (= (dissoc sig-statement "attachments") + (decode-sig sig))) + (testing "valid-sig" + (is + (= sig + (-> (validate-sig + sig-statement + sig-attachment-object + (assoc sig-partial-multipart + :input-stream + #?(:clj (ByteArrayInputStream. + (.getBytes sig "UTF-8")) + :cljs sig))) + :input-stream + #?(:clj slurp))))) + (testing "invalid-sig" + (let [invalid-sig (apply str (drop 10 sig))] + (is (= ::attachment/invalid-signature-json + (try + (validate-sig + sig-statement + sig-attachment-object + (assoc sig-partial-multipart + :input-stream + #?(:clj (ByteArrayInputStream. + (.getBytes invalid-sig "UTF-8")) + :cljs invalid-sig))) + (catch #?(:clj clojure.lang.ExceptionInfo + :cljs ExceptionInfo) exi + (-> exi ex-data :type)))))))) (deftest validate-multiparts-test (let [s-template {"id" "78efaab3-1c65-4cb7-9289-f34e0594b274" From bcda832044eab35fc2d16d5ffc1244657d70042b Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Thu, 12 May 2022 10:19:55 -0400 Subject: [PATCH 12/13] LRS-72 better error for missing attachments + test --- .../pedestal/interceptor/xapi/statements/attachment.cljc | 5 ++--- .../interceptor/xapi/statements/attachment_test.cljc | 8 ++++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc index 06aa9163..abd61a2b 100644 --- a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc +++ b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc @@ -211,9 +211,8 @@ (if fileUrl state (throw - ;; TODO: name of this error? - (ex-info "Invalid multipart format" - {:type ::invalid-multipart-format}))))) + (ex-info "Statement references missing attachment and no fileUrl is present" + {:type ::statement-attachment-missing}))))) {:sha2s [] :mpart-map (multipart-map-dedupe multiparts)} (ss/all-attachment-objects statements)) diff --git a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc index 9ba067c4..be268168 100644 --- a/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc +++ b/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment_test.cljc @@ -137,6 +137,14 @@ (validate-multiparts statements [multipart])))) + (testing "fails with missing attachment" + (is (= ::attachment/statement-attachment-missing + (try (validate-multiparts + statements + []) ;; no attachments + (catch #?(:clj clojure.lang.ExceptionInfo + :cljs ExceptionInfo) exi + (-> exi ex-data :type)))))) (testing "fails with left over multiparts" (is (= ::attachment/statement-attachment-mismatch (try (validate-multiparts From e4a6cd87204f9cdaaa2890e4e3dd6aeb1c6b8465 Mon Sep 17 00:00:00 2001 From: Milt Reder Date: Thu, 12 May 2022 10:28:39 -0400 Subject: [PATCH 13/13] LRS-72 refactor reduce-fn for validate-multiparts --- .../xapi/statements/attachment.cljc | 63 ++++++++++--------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc index abd61a2b..3aa3fa78 100644 --- a/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc +++ b/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment.cljc @@ -182,40 +182,47 @@ {} multiparts)) +(defn- attachment-reduce-fn + "Reduce over statement attachment references and attempt to match them to the + provided map of multiparts. Validates signatures when appropriate. + Throws on missing attachment or signature errors." + [{:keys [sha2s mpart-map] + :as state} + {:keys [statement + attachment-path] + {:strs [sha2 fileUrl] + :as att-obj} :attachment}] + (if-let [match-mp (get mpart-map sha2)] + (cond-> (update state :sha2s conj sha2) + ;; Validate + recreate sig mp + (and (= "attachments" + (first attachment-path)) + (sig? att-obj)) + (update-in + [:mpart-map sha2] + (partial validate-sig + statement + att-obj))) + ;; Allow attachments w/o a fileUrl to silently drop + (if fileUrl + state + (throw + (ex-info + "Statement references missing attachment and no fileUrl is present" + {:type ::statement-attachment-missing}))))) + (defn validate-multiparts "Given a list of statements and a list of multiparts, return valid multiparts, deduplicated." [statements multiparts] (let [{:keys [sha2s - mpart-map]} - (reduce - (fn [{:keys [sha2s mpart-map] - :as state} - {:keys [statement - attachment-path] - {:strs [sha2 fileUrl] - :as att-obj} :attachment}] - (if-let [match-mp (get mpart-map sha2)] - (cond-> (update state :sha2s conj sha2) - ;; Validate + recreate sig mp - (and (= "attachments" - (first attachment-path)) - (sig? att-obj)) - (update-in - [:mpart-map sha2] - (partial validate-sig - statement - att-obj))) - ;; Allow attachments w/o a fileUrl to silently drop - (if fileUrl - state - (throw - (ex-info "Statement references missing attachment and no fileUrl is present" - {:type ::statement-attachment-missing}))))) - {:sha2s [] - :mpart-map (multipart-map-dedupe multiparts)} - (ss/all-attachment-objects statements)) + mpart-map]} (reduce + attachment-reduce-fn + {:sha2s [] + :mpart-map (multipart-map-dedupe + multiparts)} + (ss/all-attachment-objects statements)) sha2s-out (distinct sha2s)] (if-let [leftover-multiparts (-> (apply dissoc mpart-map