Skip to content
Snippets Groups Projects
Unverified Commit a9ca102c authored by Howon Lee's avatar Howon Lee Committed by GitHub
Browse files

Deal with SAML responses having whitespace (#23451) (#23633)

Pursuant to #23451.

The end effect of whitespace existing in a SAML response is us choking on it as reported in #23451. Two possible interpretations of causes of this bug:

There was an upstream change in our fork of the clojure SAML lib as flamber noted,
The decoding of base64 in our SAML endpoint (which uses the SAML lib) chokes on whitespace.
The proximate cause is the second one and ultimate cause is the first. However, I tend to believe that fixing the second one would be the better fix. For comparison, onelogin's first party SAML thing for java decodes base64 (https://github.com/onelogin/java-saml/blob/master/core/src/main/java/com/onelogin/saml2/util/Util.java) via apache's lib, which seems to do the thing that a lot of base64 decoders do of skipping whitespace.
parent 6548a621
No related branches found
No related tags found
No related merge requests found
......@@ -31,11 +31,10 @@
[metabase.server.request.util :as request.u]
[metabase.util :as u]
[metabase.util.i18n :refer [trs tru]]
[ring.util.codec :as codec]
[ring.util.response :as response]
[saml20-clj.core :as saml]
[schema.core :as s])
(:import java.util.UUID))
(:import [java.util Base64 UUID]))
(defn- group-names->ids
"Translate a user's group names to a set of MB group IDs using the configured mappings"
......@@ -118,7 +117,7 @@
(try
(let [idp-url (sso-settings/saml-identity-provider-uri)
saml-request (saml/request
{:request-id (str "id-" (java.util.UUID/randomUUID))
{:request-id (str "id-" (UUID/randomUUID))
:sp-name (sso-settings/saml-application-name)
:issuer (sso-settings/saml-application-name)
:acs-url (acs-url)
......@@ -166,9 +165,10 @@
{:status-code 401})))
attrs))
(defn- base64-decode [s]
(defn- base64-decode [^String s]
(when (u/base64-string? s)
(codecs/bytes->str (codec/base64-decode s))))
(codecs/bytes->str
(.decode (Base64/getMimeDecoder) s))))
(defmethod sso.i/sso-post :saml
;; Does the verification of the IDP's response and 'logs the user in'. The attributes are available in the response:
......@@ -180,7 +180,7 @@
(when-not (str/blank? s)
s)))]
(sso-utils/check-sso-redirect continue-url)
(let [xml-string (base64-decode (:SAMLResponse params))
(let [xml-string (str/trim (base64-decode (:SAMLResponse params)))
saml-response (xml-string->saml-response xml-string)
attrs (saml-response->attributes saml-response)
email (get attrs (sso-settings/saml-attribute-email))
......
......@@ -242,6 +242,9 @@
(defn- new-user-with-groups-saml-test-response []
(saml-response-from-file "test_resources/saml-test-response-new-user-with-groups.xml"))
(defn- whitespace-response []
(str (saml-response-from-file "test_resources/saml-test-response.xml") "\n\n"))
(defn- new-user-with-groups-in-separate-attribute-nodes-saml-test-response []
(saml-response-from-file "test_resources/saml-test-response-new-user-with-groups-in-separate-attribute-nodes.xml"))
......@@ -347,14 +350,26 @@
;; Part of accepting the POST is validating the response and the relay state so we can redirect the user to their
;; original destination
(deftest login-test
(with-saml-default-setup
(do-with-some-validators-disabled
(fn []
(testing "After a successful login with the identity provider, the SAML provider will POST to the `/auth/sso` route."
(testing "After a successful login with the identity provider, the SAML provider will POST to the `/auth/sso` route."
(with-saml-default-setup
(do-with-some-validators-disabled
(fn []
(let [req-options (saml-post-request-options (saml-test-response)
(saml/str->base64 default-redirect-uri))
response (client-full-response :post 302 "/auth/sso" req-options)]
(is (successful-login? response))
(is (= default-redirect-uri
(get-in response [:headers "Location"])))
(is (= (some-saml-attributes "rasta")
(saml-login-attributes "rasta@metabase.com"))))))))
(testing "Still works with whitespace in the SAML post response (#23451)"
(with-saml-default-setup
(do-with-some-validators-disabled
(fn []
(let [req-options (saml-post-request-options (whitespace-response)
(saml/str->base64 default-redirect-uri))
response (client-full-response :post 302 "/auth/sso" req-options)]
(is (successful-login? response))
(is (= default-redirect-uri
(get-in response [:headers "Location"])))
(is (= (some-saml-attributes "rasta")
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment