Better validation of request parameters (#39)

metabase · Sep 29, 2020 · d8dcddd · d8dcddd
1 parent 5e103a9
commit d8dcddd
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 7 deletions.
diff --git a/src/saml20_clj/sp/request.clj b/src/saml20_clj/sp/request.clj
@@ -12,17 +12,21 @@
   [instant]
   (t/format (t/format "YYYY-MM-dd'T'HH:mm:ss'Z'" (t/offset-date-time instant (t/zone-offset 0)))))
 
+(defn- non-blank-string? [s]
+  (and (string? s)
+       (not (str/blank? s))))
+
 (defn request
   "Return XML elements that represent a SAML 2.0 auth request."
-  ^org.w3c.dom.Element [{:keys [ ;; e.g. something like a UUID
+  ^org.w3c.dom.Element [{:keys [ ;; e.g. something like a UUID. Random UUID will be used if no other ID is provided
                                 request-id
                                 ;; e.g. "Metabase"
                                 sp-name
-                                ;; e.g. ttp://sp.example.com/demo1/index.php?acs
-                                acs-url
                                 ;; e.g. http://sp.example.com/demo1/index.php?acs
-                                idp-url
+                                acs-url
                                 ;; e.g. http://idp.example.com/SSOService.php
+                                idp-url
+                                ;; e.g. http://sp.example.com/demo1/metadata.php
                                 issuer
                                 ;; If present, record the request
                                 state-manager
@@ -31,9 +35,10 @@
                                 instant]
                          :or   {request-id (str (java.util.UUID/randomUUID))
                                 instant (t/instant)}}]
-  (assert acs-url)
-  (assert idp-url)
-  (assert sp-name)
+  (assert (non-blank-string? acs-url) "acs-url is required")
+  (assert (non-blank-string? idp-url) "idp-url is required")
+  (assert (non-blank-string? sp-name) "sp-name is required")
+  (assert (non-blank-string? issuer) "issuer is required")
   (let [request (coerce/->Element (coerce/->xml-string
                                    [:samlp:AuthnRequest
                                     {:xmlns:samlp                 "urn:oasis:names:tc:SAML:2.0:protocol"

diff --git a/test/saml20_clj/sp/request_test.clj b/test/saml20_clj/sp/request_test.clj
@@ -154,3 +154,40 @@
                 ;; for some reason it indents the XML differently on the REPL and in the tests
                 (map str/trim)
                 (filter seq))))))
+
+(deftest request-validation-test
+  (let [request {:acs-url    "http://sp.example.com/demo1/index.php?acs"
+                 :sp-name    "My Example SP"
+                 :idp-url    "http://idp.example.com/SSOService.php"
+                 :issuer     "http://sp.example.com/demo1/metadata.php"
+                 :request-id "_1"
+                 :instant    (t/instant "2020-09-29T20:12:00.000Z")}]
+    (testing "Make sure we can create a valid request given the input"
+      (is (= (str "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+                  "<samlp:AuthnRequest"
+                  " xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\""
+                  " AssertionConsumerServiceURL=\"http://sp.example.com/demo1/index.php?acs\""
+                  " Destination=\"http://idp.example.com/SSOService.php\" ID=\"_1\""
+                  " IssueInstant=\"2020-09-29T20:12:00Z\""
+                  " ProtocolBinding=\"urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST\""
+                  " ProviderName=\"My Example SP\""
+                  " Version=\"2.0\">"
+                  "<saml:Issuer xmlns:saml=\"urn:oasis:names:tc:SAML:2.0:assertion\">"
+                  "http://sp.example.com/demo1/metadata.php"
+                  "</saml:Issuer>"
+                  "</samlp:AuthnRequest>")
+             (-> (request/request request)
+                 coerce/->xml-string
+                 (str/replace #"\n\s*" "")))))
+    (testing "Should validate that required params are non-blank strings"
+      (doseq [k [:acs-url
+                 :sp-name
+                 :idp-url
+                 :issuer]]
+        (doseq [v [nil "" "    " false true 100]]
+          (testing (format "\n%s = %s" k (pr-str v))
+            (let [request (assoc request k v)]
+              (is (thrown-with-msg?
+                   java.lang.AssertionError
+                   (re-pattern (format "%s is required" (name k)))
+                   (request/request request))))))))))