From 8bdb36868b1a7e40823346fbea4c5b5f774151f0 Mon Sep 17 00:00:00 2001 From: Leandro Doctors Date: Thu, 5 Sep 2024 11:15:13 +0200 Subject: [PATCH] Make sure the `query-params` are not only coerced, but also validated Before this change, the `query-params` value was coerced, but never validated (which was a bug). * `get-param-value` - add parameter: `validator` (optional) - refactor - document * implement and use validators: - `validate-date` - `validate-string` - `validate-noop` * implement and use coercer: - `coercer-noop` *rename `coerce-report-type` to `coerce-and-validate-report-type`. * implement and use anomalies: - create `invalid-string-param-anom` - create `invalid-date-param-anom` * create `invalid-string-param-msg` (`invalid-date-param-msg` already existed) * add regression test Misc: * document `handler` * add regression test * reformat with `cljfmt` --- .../evaluate_measure/middleware/params.clj | 76 +++++++++++++++---- .../middleware/params_test.clj | 76 +++++++++++++------ 2 files changed, 115 insertions(+), 37 deletions(-) diff --git a/modules/operation-measure-evaluate-measure/src/blaze/fhir/operation/evaluate_measure/middleware/params.clj b/modules/operation-measure-evaluate-measure/src/blaze/fhir/operation/evaluate_measure/middleware/params.clj index 3d6272a53..c7aa9297d 100644 --- a/modules/operation-measure-evaluate-measure/src/blaze/fhir/operation/evaluate_measure/middleware/params.clj +++ b/modules/operation-measure-evaluate-measure/src/blaze/fhir/operation/evaluate_measure/middleware/params.clj @@ -13,29 +13,75 @@ (format "Invalid parameter `%s` with value `%s`. Should be a date in format YYYY, YYYY-MM or YYYY-MM-DD." name value)) +(defn- invalid-string-param-msg [name value] + (format "Invalid parameter `%s` with value `%s`. Should be a string." + name value)) + +(defn- invalid-date-param-anom + [name value] + (ba/incorrect + (invalid-date-param-msg name value) + :fhir/issue "value" + :fhir/operation-outcome "MSG_PARAM_INVALID" + :fhir.issue/expression name)) + +(defn- invalid-string-param-anom + [name value] + (ba/incorrect + (invalid-string-param-msg name value) + :fhir/issue "value" + :fhir/operation-outcome "MSG_PARAM_INVALID" + :fhir.issue/expression name)) + +(defn- validate-noop [_name value] + value) + +(defn- validate-date [name value] + (if (system/date? value) value (invalid-date-param-anom name value))) + +(defn- validate-string [name value] + (if (string? value) value (invalid-string-param-anom name value))) + (defn- get-param-value-from-resource [body name] (when (identical? :fhir/Parameters (fhir-spec/fhir-type body)) (some #(when (= name (:name %)) (:value %)) (:parameter body)))) -(defn- get-param-value [{:keys [params body]} name coercer] +(defn- get-param-value* [{:keys [params body]} name coercer] (or (some->> (get params name) (coercer name)) (get-param-value-from-resource body name))) -(defn- get-required-param-value [request name coercer] - (or (get-param-value request name coercer) +(defn- get-param-value + "Given a `request`, tries to get the value of the param with `name`. + + There are two types of sources for the values: + * query-param values, and + * param values from the parameters resource. + + Query-param values will need to be coerced to FHIR types, since they will + always be strings. They will be coerced with `coercer` (a function that takes + `name` and the found value, and returns the coerced value or an anomaly). + + Param values don't need to be coerced, since they already are FHIR types. + + Both types of values will then be validated by an optional `validator` (a + function that takes `name` and the coerced value, returning either the + validated value or an anomaly)." + ([request name coercer] + (get-param-value request name coercer validate-noop)) + ([request name coercer validator] + (when-ok [value (get-param-value* request name coercer)] + (some->> value (validator name))))) + +(defn- get-required-param-value [request name coercer validator] + (or (get-param-value request name coercer validator) (ba/incorrect (format "Missing required parameter `%s`." name) :fhir/issue "value" :fhir/operation-outcome "MSG_PARAM_INVALID" :fhir.issue/expression name))) -(defn- invalid-date-param-anom - [name value] - (ba/incorrect - (invalid-date-param-msg name value) - :fhir/issue "value" - :fhir/operation-outcome "MSG_PARAM_INVALID" - :fhir.issue/expression name)) +(defn- coerce-noop [_name value] + value) (defn- coerce-date "Coerces `value` into a System.Date. @@ -51,7 +97,7 @@ (format "Invalid parameter `reportType` with value `%s`. Should be one of `subject`, `subject-list` or `population`." report-type)) -(defn- coerce-report-type [_ value] +(defn- coerce-and-validate-report-type [_ value] (if-not (s/valid? ::measure/report-type value) (ba/incorrect (invalid-report-type-param-msg value) :fhir/issue "value") (type/code value))) @@ -78,13 +124,13 @@ (defn- params-request [{:keys [request-method] :as request}] (when-ok [period-start (get-required-param-value - request "periodStart" coerce-date) + request "periodStart" coerce-date validate-date) period-end (get-required-param-value - request "periodEnd" coerce-date) + request "periodEnd" coerce-date validate-date) measure (get-param-value - request "measure" (fn [_n v] v)) + request "measure" coerce-noop validate-string) report-type (get-param-value - request "reportType" coerce-report-type) + request "reportType" coerce-and-validate-report-type) subject-ref (coerce-subject-ref-param request)] (let [report-type (some-> report-type type/value)] (if (and (= :get request-method) (= "subject-list" report-type)) diff --git a/modules/operation-measure-evaluate-measure/test/blaze/fhir/operation/evaluate_measure/middleware/params_test.clj b/modules/operation-measure-evaluate-measure/test/blaze/fhir/operation/evaluate_measure/middleware/params_test.clj index 2387eabd3..38493bc18 100644 --- a/modules/operation-measure-evaluate-measure/test/blaze/fhir/operation/evaluate_measure/middleware/params_test.clj +++ b/modules/operation-measure-evaluate-measure/test/blaze/fhir/operation/evaluate_measure/middleware/params_test.clj @@ -17,7 +17,13 @@ (def operation-outcome-uri #fhir/uri"http://terminology.hl7.org/CodeSystem/operation-outcome") -(def handler (-> (params/wrap-coerce-params ac/completed-future) wrap-error)) +(def handler + "This testing handler will wrap the request into a future. + + If an error arises, it returns a normal ring response map. + If no error arises, it returns the original request." + (-> (params/wrap-coerce-params ac/completed-future) + wrap-error)) (deftest wrap-coerce-params-test (testing "period start" @@ -114,28 +120,54 @@ [:period 1] := #fhir/date"2021")))) (testing "measure" - (doseq [request - [{:params - {"periodStart" "2015" - "periodEnd" "2016" - "measure" "measure-202606"}} - {:body - {:fhir/type :fhir/Parameters - :parameter - [{:fhir/type :fhir.Parameters/parameter - :name "periodStart" - :value #fhir/date"2014"} - {:fhir/type :fhir.Parameters/parameter - :name "periodEnd" - :value #fhir/date"2015"} - {:fhir/type :fhir.Parameters/parameter - :name "measure" - :value #fhir/string"measure-202606"}]}}]] - (let [{:blaze.fhir.operation.evaluate-measure/keys [params]} - @(handler request)] + (testing "invalid (only POST)" + (let [request {:request-method :post + :body + {:fhir/type :fhir/Parameters + :parameter + [{:fhir/type :fhir.Parameters/parameter + :name "periodStart" + :value #fhir/date"2014"} + {:fhir/type :fhir.Parameters/parameter + :name "periodEnd" + :value #fhir/date"2015"} + {:fhir/type :fhir.Parameters/parameter + :name "measure" + :value #fhir/date"2015"}]}} + {:keys [status body]} @(handler request)] - (given params - :measure := "measure-202606")))) + (is (= 400 status)) + + (is (= :fhir/OperationOutcome (:fhir/type body))) + + (given (-> body :issue first) + :severity := #fhir/code"error" + :code := #fhir/code"value" + :diagnostics := "Invalid parameter `measure` with value `2015`. Should be a string."))) + + (testing "valid (both GET and POST)" + (doseq [request + [{:params + {"periodStart" "2015" + "periodEnd" "2016" + "measure" "measure-202606"}} + {:body + {:fhir/type :fhir/Parameters + :parameter + [{:fhir/type :fhir.Parameters/parameter + :name "periodStart" + :value #fhir/date"2014"} + {:fhir/type :fhir.Parameters/parameter + :name "periodEnd" + :value #fhir/date"2015"} + {:fhir/type :fhir.Parameters/parameter + :name "measure" + :value #fhir/string"measure-202606"}]}}]] + (let [{:blaze.fhir.operation.evaluate-measure/keys [params]} + @(handler request)] + + (given params + :measure := "measure-202606"))))) (testing "report type" (testing "invalid"