Skip to content

Commit

Permalink
SQL-215 Support Attachment and Document Scanning (#88)
Browse files Browse the repository at this point in the history
* SQL-215 generic scanner function support

* SQL-215 doc scanning

* SQL-215 attempt to fix conf tests

* SQL-215 catch scanning errors

* SQL-215 set proper content type on document scan fail

* SQL-215 force json error resp

* SQL-215 test for file scanner fn

* SQL-215 refactor doc route fn

* SQL-215 refactor scan-attachments

* formatting

* SQL-215 add comment about deleting tempfiles

* SQL-215 explicitly provide noop scanner in test
  • Loading branch information
milt authored Nov 22, 2023
1 parent e93ba63 commit 9658210
Show file tree
Hide file tree
Showing 8 changed files with 312 additions and 45 deletions.
3 changes: 2 additions & 1 deletion deps.edn
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@
:sha "423bd6c301ce8503f5c18b43df098bbe64f8f1ab"}
com.yetanalytics/lrs-test-runner
{:git/url "https://github.com/yetanalytics/lrs-test-runner.git"
:sha "0fcd42854f9c79a043c13d436d629826bfc5133d"}}}
:sha "0fcd42854f9c79a043c13d436d629826bfc5133d"}
babashka/babashka.curl {:mvn/version "0.1.2"}}}
;; bench utils for LRS implementations
:bench
{:extra-paths ["src/bench"]
Expand Down
45 changes: 45 additions & 0 deletions dev-resources/attachments/attachment_post_body.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
--105423a5219f5a63362a375ba7a64a8f234da19c7d01e56800c3c64b26bb2fa0
Content-Type:application/json

{
"actor": {
"mbox": "mailto:sample.agent@example.com",
"name": "Sample Agent",
"objectType": "Agent"
},
"verb": {
"id": "http://adlnet.gov/expapi/verbs/answered",
"display": {
"en-US": "answered"
}
},
"object": {
"id": "http://www.example.com/tincan/activities/multipart",
"objectType": "Activity",
"definition": {
"name": {
"en-US": "Multi Part Activity"
},
"description": {
"en-US": "Multi Part Activity Description"
}
}
},
"attachments": [
{
"usageType": "http://example.com/attachment-usage/test",
"display": { "en-US": "A test attachment" },
"description": { "en-US": "A test attachment (description)" },
"contentType": "text/plain; charset=ascii",
"length": 27,
"sha2": "495395e777cd98da653df9615d09c0fd6bb2f8d4788394cd53c56a3bfdcd848a"
}
]
}
--105423a5219f5a63362a375ba7a64a8f234da19c7d01e56800c3c64b26bb2fa0
Content-Type:text/plain
Content-Transfer-Encoding:binary
X-Experience-API-Hash:495395e777cd98da653df9615d09c0fd6bb2f8d4788394cd53c56a3bfdcd848a

here is a simple attachment
--105423a5219f5a63362a375ba7a64a8f234da19c7d01e56800c3c64b26bb2fa0--
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
(ns com.yetanalytics.lrs.pedestal.interceptor.xapi.document
"Document Interceptors"
(:require
[io.pedestal.interceptor.chain :as chain]
[com.yetanalytics.lrs.util :as u]
#?(:clj [clojure.java.io :as io])
#?@(:cljs [[goog.string :refer [format]]
[goog.string.format]]))
#?(:clj (:import [java.io ByteArrayOutputStream ByteArrayInputStream])))

#?(:clj
(defn stream->bytes
[input-stream]
(let [baos (ByteArrayOutputStream.)]
(io/copy input-stream baos)
(.toByteArray baos))))

(defn scan-document
"Return an interceptor that will scan document request bodies given a
file-scanner fn. Reads body into a byte array (in clojure)"
[file-scanner]
{:name ::scan-document
:enter
(fn [ctx]
(let [body-bytes (-> ctx
(get-in [:request :body])
#?(:clj stream->bytes :cljs identity))]
(if-let [scan-error (try
(file-scanner
#?(:clj (ByteArrayInputStream. body-bytes)
:cljs body-bytes))
(catch #?(:clj Exception :cljs js/Error) _
{:message "Scan Error"}))]
(assoc (chain/terminate ctx)
:response
{:status 400
:headers {"Content-Type" "application/json"}
:body (u/json-string
{:error
{:message
(format "Document scan failed, Error: %s"
(:message scan-error))}})})
(assoc-in ctx
[:request :body]
#?(:clj (ByteArrayInputStream. body-bytes)
:cljs body-bytes)))))})
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,36 @@
{:status 400
:body {:error {:message "No Statement Data Provided"}}}))))})

(defn scan-attachments
"Scan attachment files with a user-provided function."
[file-scanner]
{:name ::scan-attachments
:enter
(fn [ctx]
(let [attachments (get-in ctx [:xapi :xapi.statements/attachments])]
(if-let [attachment-errors
(some-> attachments
(->>
(keep (fn [{:keys [content]}]
(try
(file-scanner content)
(catch #?(:clj Exception :cljs js/Error) _
{:message "Scan Error"})))))
not-empty)]
(do
;; Delete (possibly) unsafe tempfiles
(attachment/delete-attachments! attachments)
(assoc (chain/terminate ctx)
:response
{:status 400
:body {:error
{:message
(format "Attachment scan failed, Errors: %s"
(cs/join
", "
(map :message attachment-errors)))}}}))
ctx)))})

(def set-consistent-through
{:name ::set-consistent-through
:leave
Expand Down
101 changes: 61 additions & 40 deletions src/main/com/yetanalytics/lrs/pedestal/routes.cljc
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
(:require
[com.yetanalytics.lrs.pedestal.interceptor :as i]
[com.yetanalytics.lrs.pedestal.interceptor.xapi :as xapi-i]
[com.yetanalytics.lrs.pedestal.interceptor.xapi.document :as doc-i]
[com.yetanalytics.lrs.pedestal.interceptor.xapi.statements :as statements-i]
[com.yetanalytics.lrs.pedestal.routes.about :as about]
[com.yetanalytics.lrs.pedestal.routes.statements :as statements]
Expand All @@ -17,7 +18,9 @@
{:status 405})

(defn build-document-routes
[interceptors & {:keys [path-prefix] :or {path-prefix "/xapi"}}]
[interceptors & {:keys [path-prefix
file-scanner]
:or {path-prefix "/xapi"}}]
;; Build all possible doc routes by looping over each pair of
;; resource + doc type and HTTP method
(into
Expand All @@ -38,30 +41,37 @@
method
method-not-allowed
:route-name (keyword route-name-ns (name method))]
(let [params-interceptors
[(xapi-i/params-interceptor
(case resource-tuple
["activities" "state"]
(case method
:put :xapi.activities.state.PUT.request/params
:post :xapi.activities.state.POST.request/params
:get :xapi.activities.state.GET.request/params
:head :xapi.activities.state.GET.request/params
:delete :xapi.activities.state.DELETE.request/params)
["activities" "profile"]
(case method
:put :xapi.activities.profile.PUT.request/params
:post :xapi.activities.profile.POST.request/params
:get :xapi.activities.profile.GET.request/params
:head :xapi.activities.profile.GET.request/params
:delete :xapi.activities.profile.DELETE.request/params)
["agents" "profile"]
(case method
:put :xapi.agents.profile.PUT.request/params
:post :xapi.agents.profile.POST.request/params
:get :xapi.agents.profile.GET.request/params
:head :xapi.agents.profile.GET.request/params
:delete :xapi.agents.profile.DELETE.request/params)))]
(let [doc-params-interceptor
(xapi-i/params-interceptor
(case resource-tuple
["activities" "state"]
(case method
:put :xapi.activities.state.PUT.request/params
:post :xapi.activities.state.POST.request/params
:get :xapi.activities.state.GET.request/params
:head :xapi.activities.state.GET.request/params
:delete :xapi.activities.state.DELETE.request/params)
["activities" "profile"]
(case method
:put :xapi.activities.profile.PUT.request/params
:post :xapi.activities.profile.POST.request/params
:get :xapi.activities.profile.GET.request/params
:head :xapi.activities.profile.GET.request/params
:delete :xapi.activities.profile.DELETE.request/params)
["agents" "profile"]
(case method
:put :xapi.agents.profile.PUT.request/params
:post :xapi.agents.profile.POST.request/params
:get :xapi.agents.profile.GET.request/params
:head :xapi.agents.profile.GET.request/params
:delete :xapi.agents.profile.DELETE.request/params)))
params-interceptors
(cond-> [doc-params-interceptor]
;; Scan files if scanner is present on PUT/POST
(and file-scanner
(contains? #{:put :post} method))
(conj
(doc-i/scan-document file-scanner)))
method-interceptor
(case method
:put documents/handle-put
Expand Down Expand Up @@ -93,10 +103,14 @@
:path-prefix - defines the prefix from root for xAPI routes, default /xapi
:wrap-interceptors - a vector of interceptors to apply to every route.
The default vector includes an error interceptor which should be replaced
if this setting is provided."
if this setting is provided.
:file-scanner - a function that takes the content of any arbitrary
user-submitted file and returns nil if it is safe, or a map with :message
describing why it is unsafe. If unsafe the request will fail with a 400."
[{:keys [lrs
path-prefix
wrap-interceptors]
wrap-interceptors
file-scanner]
:or {path-prefix "/xapi"
wrap-interceptors [i/error-interceptor]}}]
(let [lrs-i (i/lrs-interceptor lrs)
Expand Down Expand Up @@ -150,19 +164,25 @@
statements/handle-get)
:route-name :com.yetanalytics.lrs.xapi.statements/head]
[(format "%s/statements" path-prefix)
:put (conj protected-interceptors
statements-i/set-consistent-through
(xapi-i/params-interceptor
:xapi.statements.PUT.request/params)
statements-i/parse-multiparts
statements-i/validate-request-statements
statements/handle-put)]
:put (-> protected-interceptors
(into [statements-i/set-consistent-through
(xapi-i/params-interceptor
:xapi.statements.PUT.request/params)
statements-i/parse-multiparts
statements-i/validate-request-statements])
(cond->
file-scanner
(conj (statements-i/scan-attachments file-scanner)))
(conj statements/handle-put))]
[(format "%s/statements" path-prefix)
:post (conj protected-interceptors
statements-i/set-consistent-through
statements-i/parse-multiparts
statements-i/validate-request-statements
statements/handle-post)]
:post (-> protected-interceptors
(into [statements-i/set-consistent-through
statements-i/parse-multiparts
statements-i/validate-request-statements])
(cond->
file-scanner
(conj (statements-i/scan-attachments file-scanner)))
(conj statements/handle-post))]
[(format "%s/statements" path-prefix)
:any method-not-allowed
:route-name :com.yetanalytics.lrs.xapi.statements/any]
Expand Down Expand Up @@ -201,4 +221,5 @@

;; documents
(build-document-routes document-interceptors
:path-prefix path-prefix))))
:path-prefix path-prefix
:file-scanner file-scanner))))
85 changes: 85 additions & 0 deletions src/test/com/yetanalytics/lrs/scan_test.clj
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
(ns com.yetanalytics.lrs.scan-test
(:require [clojure.test :refer [deftest testing is use-fixtures]]
[babashka.curl :as curl]
[io.pedestal.http :as http]
[com.yetanalytics.test-support :as support]))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Tests
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;


(def attachment-post-params
{:basic-auth ["username" "password"]
:headers
{"X-Experience-API-Version" "1.0.3"
"Content-Type"
"multipart/mixed; boundary=105423a5219f5a63362a375ba7a64a8f234da19c7d01e56800c3c64b26bb2fa0"}
:body (slurp "dev-resources/attachments/attachment_post_body.txt")
:throw false})

(def doc-post-params
{:basic-auth ["username" "password"]
:headers {"X-Experience-API-Version" "1.0.3"}
:query-params
{"activityId" "http://www.example.com/activityId/hashset"
"agent"
"{\"objectType\":\"Agent\",\"account\":{\"homePage\":\"http://www.example.com/agentId/1\",\"name\":\"Rick James\"}}"
"stateId" "f8128f68-74e2-4951-8c5f-ef7cce73b4ff"}
:body "I'm a little teapot"
:throw false})

(deftest scan-test
(testing "File/Document Scanning"
;; Stub out a scanner that always fails
(testing "Failure"
(let [server (support/test-server
:route-opts
{:file-scanner
(fn [in]
(slurp in)
{:message "Scan Fail!"})})]
(try
(http/start server)
(testing "Attachment"
(is (= {:status 400
:body "{\"error\":{\"message\":\"Attachment scan failed, Errors: Scan Fail!\"}}"}
(select-keys
(curl/post
"http://localhost:8080/xapi/statements"
attachment-post-params)
[:body :status]))))
(testing "Document"
(is (= {:status 400
:body "{\"error\":{\"message\":\"Document scan failed, Error: Scan Fail!\"}}"}
(select-keys
(curl/post
"http://localhost:8080/xapi/activities/state"
doc-post-params)
[:body :status]))))
(finally
(http/stop server)))))
;; And a scanner that always passes
(testing "Success"
(let [server (support/test-server
:route-opts
{:file-scanner
(fn [in]
(slurp in)
nil)})]
(try
(http/start server)
(testing "Attachment"
(is (= 200
(:status
(curl/post
"http://localhost:8080/xapi/statements"
attachment-post-params)))))
(testing "Document"
(is (= 204
(:status
(curl/post
"http://localhost:8080/xapi/activities/state"
doc-post-params)))))
(finally
(http/stop server)))))))
8 changes: 5 additions & 3 deletions src/test/com/yetanalytics/test_runner.cljc
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
clojure.test.check.properties
[clojure.test :as test
:refer [run-tests] :include-macros true]
#?(:clj com.yetanalytics.lrs-test
:cljs [cljs.nodejs :refer [process]])
#?@(:clj [com.yetanalytics.lrs-test
com.yetanalytics.lrs.scan-test]
:cljs [[cljs.nodejs :refer [process]]])
com.yetanalytics.lrs.xapi.activities-test
com.yetanalytics.lrs.xapi.agents-test
com.yetanalytics.lrs.xapi.document-test
Expand Down Expand Up @@ -43,7 +44,8 @@
(defn- run-lrs-tests
[]
(run-tests
#?(:clj 'com.yetanalytics.lrs-test)
#?@(:clj ['com.yetanalytics.lrs-test
'com.yetanalytics.lrs.scan-test])
'com.yetanalytics.lrs.xapi.activities-test
'com.yetanalytics.lrs.xapi.agents-test
'com.yetanalytics.lrs.xapi.document-test
Expand Down
Loading

0 comments on commit 9658210

Please sign in to comment.