Skip to content
Snippets Groups Projects
Unverified Commit 174afe58 authored by adam-james's avatar adam-james Committed by GitHub
Browse files

Adjust JWT and SAML fetch-and-update user to save new attributes (#23005) 

* Adjust JWT and SAML fetch-and-update user to save new attributes

Before this change, JWT/SAML logins would attempt to update attributes, but never considered the first-name or
last-name attributes.

* Attempts to fix tests to prevent pulluting test users with "Unknown"

* No deleting users.

* Unit tests checking that first/last names are updated for SSO users

When an SSO user is first logged in, they might not have first_name and/or last_name keys. This is allowed, but the
names will be "Unknown" in the app-db. Subsequently, a User may log in again with SSO but have fisrt/last name
attributes, which should update the Metabase user data in the app-db.

These unit tests set up such a scenario to check that the :first_name and :last_name keys are indeed updated.

* Adjust Enterprise LDAP to also use SSO-UTILS

Trying to unify the LDAP implementation with JWT/SAML a bit here.

* Lint error

* Reverting LDAP ns changes to get the PR unstuck

This is to keep the ball rolling on SSO fixes. I'll add LDAP as an item in the Epic to address this separately.
parent 8b023067
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 219 additions and 33 deletions
enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
+ 11
− 8
View file @ 174afe58
......@@ -19,12 +19,15 @@
[first-name last-name email user-attributes]
(when-not (sso-settings/jwt-configured?)
(throw (IllegalArgumentException. (str (tru "Can't create new JWT user when JWT is not configured")))))
(or (sso-utils/fetch-and-update-login-attributes! email user-attributes)
(sso-utils/create-new-sso-user! {:first_name first-name
:last_name last-name
:email email
:sso_source "jwt"
:login_attributes user-attributes})))
(let [user {:first_name first-name
:last_name last-name
:email email
:sso_source "jwt"
:login_attributes user-attributes}]
(or (sso-utils/fetch-and-update-login-attributes! user)
(sso-utils/create-new-sso-user! (merge user
(when-not first-name {:first_name (trs "Unknown")})
(when-not last-name {:last_name (trs "Unknown")}))))))
(def ^:private ^{:arglists '([])} jwt-attribute-email (comp keyword sso-settings/jwt-attribute-email))
(def ^:private ^{:arglists '([])} jwt-attribute-firstname (comp keyword sso-settings/jwt-attribute-firstname))
......@@ -80,8 +83,8 @@
e))))
login-attrs (jwt-data->login-attributes jwt-data)
email (get jwt-data (jwt-attribute-email))
first-name (get jwt-data (jwt-attribute-firstname) (trs "Unknown"))
last-name (get jwt-data (jwt-attribute-lastname) (trs "Unknown"))
first-name (get jwt-data (jwt-attribute-firstname))
last-name (get jwt-data (jwt-attribute-lastname))
user (fetch-or-create-user! first-name last-name email login-attrs)
session (api.session/create-session! :sso user (request.u/device-info request))]
(sync-groups! user jwt-data)
......
enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj
+ 12
− 10
View file @ 174afe58
......@@ -74,16 +74,18 @@
(sso-settings/saml-attribute-email))
" "
(tru "Please make sure your SAML IdP is properly configured."))
{:status-code 400, :user-attributes (keys user-attributes)})))
(when-let [user (or (sso-utils/fetch-and-update-login-attributes! email user-attributes)
(sso-utils/create-new-sso-user! {:first_name first-name
:last_name last-name
:email email
:sso_source "saml"
:login_attributes user-attributes}))]
(sync-groups! user group-names)
(api.session/create-session! :sso user device-info)))
{:status-code 400, :user-attributes (keys user-attributes)})))
(let [new-user {:first_name first-name
:last_name last-name
:email email
:sso_source "saml"
:login_attributes user-attributes}]
(when-let [user (or (sso-utils/fetch-and-update-login-attributes! new-user)
(sso-utils/create-new-sso-user! (merge new-user
(when-not first-name {:first_name (trs "Unknown")})
(when-not last-name {:last_name (trs "Unknown")}))))]
(sync-groups! user group-names)
(api.session/create-session! :sso user device-info))))
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;
......
enterprise/backend/src/metabase_enterprise/sso/integrations/sso_utils.clj
+ 12
− 8
View file @ 174afe58
......@@ -35,14 +35,18 @@
(messages/send-user-joined-admin-notification-email! <>, :google-auth? true))))
(defn fetch-and-update-login-attributes!
"Update the login attributes for the user at `email`. This call is a no-op if the login attributes are the same"
[email new-user-attributes]
(when-let [{:keys [id login_attributes] :as user} (db/select-one User :%lower.email (u/lower-case-en email))]
(if (= login_attributes new-user-attributes)
user
(do
(db/update! User id :login_attributes new-user-attributes)
(User id)))))
"Update `:first_name`, `:last_name`, and `:login_attributes` for the user at `email`.
This call is a no-op if the mentioned key values are equal."
[{:keys [email] :as user-from-sso}]
(when-let [{:keys [id] :as user} (db/select-one User :%lower.email (u/lower-case-en email))]
(let [user-keys (keys user-from-sso)
;; remove keys with `nil` values
user-data (into {} (filter second user-from-sso))]
(if (= (select-keys user user-keys) user-data)
user
(do
(db/update! User id user-data)
(User id))))))
(defn check-sso-redirect
"Check if open redirect is being exploited in SSO, blurts out a 400 if so"
......
enterprise/backend/test/metabase_enterprise/sso/integrations/jwt_test.clj
+ 52
− 5
View file @ 174afe58
......@@ -30,7 +30,7 @@
(def ^:private default-jwt-secret (crypto-random/hex 32))
(deftest sso-prereqs-test
(testing "SSO requests fail if SAML hasn't been enabled"
(testing "SSO requests fail if JWT hasn't been enabled"
(mt/with-temporary-setting-values [jwt-enabled false]
(saml-test/with-valid-premium-features-token
(is (= "SSO has not been enabled and/or configured"
......@@ -41,16 +41,18 @@
(is (= "SSO requires a valid token"
(saml-test/client :get 403 "/auth/sso")))))))
(testing "SSO requests fail if SAML is enabled but hasn't been configured"
(testing "SSO requests fail if JWT is enabled but hasn't been configured"
(saml-test/with-valid-premium-features-token
(mt/with-temporary-setting-values [jwt-enabled true]
(mt/with-temporary-setting-values [jwt-enabled true
jwt-identity-provider-uri nil]
(is (= "JWT SSO has not been enabled and/or configured"
(saml-test/client :get 400 "/auth/sso"))))))
(testing "The IdP provider certificate must also be included for SSO to be configured"
(testing "The JWT Shared Secret must also be included for SSO to be configured"
(saml-test/with-valid-premium-features-token
(mt/with-temporary-setting-values [jwt-enabled true
jwt-identity-provider-uri default-idp-uri]
jwt-identity-provider-uri default-idp-uri
jwt-shared-secret nil]
(is (= "JWT SSO has not been enabled and/or configured"
(saml-test/client :get 400 "/auth/sso")))))))
......@@ -173,6 +175,51 @@
"for" "the new user"}
(db/select-one-field :login_attributes User :email "newuser@metabase.com"))))))))))
(deftest update-account-test
(testing "A new account with 'Unknown' name will be created for a new JWT user without a first or last name."
(with-jwt-default-setup
(with-users-with-email-deleted "newuser@metabase.com"
(letfn [(new-user-exists? []
(boolean (seq (db/select User :%lower.email "newuser@metabase.com"))))]
(is (= false
(new-user-exists?)))
(let [response (saml-test/client-full-response :get 302 "/auth/sso"
{:request-options {:redirect-strategy :none}}
:return_to default-redirect-uri
:jwt (jwt/sign {:email "newuser@metabase.com"}
default-jwt-secret))]
(is (saml-test/successful-login? response))
(testing "new user with no first or last name"
(is (= [{:email "newuser@metabase.com"
:first_name "Unknown"
:is_qbnewb true
:is_superuser false
:id true
:last_name "Unknown"
:date_joined true
:common_name "Unknown Unknown"}]
(->> (mt/boolean-ids-and-timestamps (db/select User :email "newuser@metabase.com"))
(map #(dissoc % :last_login)))))))
(let [response (saml-test/client-full-response :get 302 "/auth/sso"
{:request-options {:redirect-strategy :none}}
:return_to default-redirect-uri
:jwt (jwt/sign {:email "newuser@metabase.com"
:first_name "New"
:last_name "User"}
default-jwt-secret))]
(is (saml-test/successful-login? response))
(testing "update user first and last name"
(is (= [{:email "newuser@metabase.com"
:first_name "New"
:is_qbnewb true
:is_superuser false
:id true
:last_name "User"
:date_joined true
:common_name "New User"}]
(->> (mt/boolean-ids-and-timestamps (db/select User :email "newuser@metabase.com"))
(map #(dissoc % :last_login))))))))))))
(deftest group-mappings-test
(testing "make sure our setting for mapping group names -> IDs works"
(mt/with-temporary-setting-values [jwt-group-mappings {"group_1" [1 2 3]
......
enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj
+ 45
− 2
View file @ 174afe58
......@@ -5,6 +5,7 @@
[metabase-enterprise.sso.integrations.sso-settings :as sso-settings]
[metabase.config :as config]
[metabase.http-client :as client]
[metabase.integrations.ldap :refer [ldap-enabled]]
[metabase.models.permissions-group :refer [PermissionsGroup]]
[metabase.models.permissions-group-membership :refer [PermissionsGroupMembership]]
[metabase.models.user :refer [User]]
......@@ -126,7 +127,8 @@
(try
(f)
(finally
(u/ignore-exceptions (db/update-where! User {} :login_attributes nil)))))
(u/ignore-exceptions (do (db/update-where! User {} :login_attributes nil)
(db/update-where! User {:email "rasta@metabase.com"} :first_name "Rasta" :last_name "Toucan"))))))
(defmacro ^:private with-saml-default-setup [& body]
`(with-valid-premium-features-token
......@@ -231,6 +233,9 @@
(defn- new-user-saml-test-response []
(saml-response-from-file "test_resources/saml-test-response-new-user.xml"))
(defn- new-user-no-names-saml-test-response []
(saml-response-from-file "test_resources/saml-test-response-new-user-no-names.xml"))
(defn- new-user-with-single-group-saml-test-response []
(saml-response-from-file "test_resources/saml-test-response-new-user-with-single-group.xml"))
......@@ -254,7 +259,7 @@
(select-keys attribute-keys))))
(deftest validate-request-id-test
(testing "Sample response shoudl fail because _1 isn't a request ID that we issued."
(testing "Sample response should fail because _1 isn't a request ID that we issued."
(with-saml-default-setup
(do-with-some-validators-disabled
(fn []
......@@ -408,6 +413,44 @@
(finally
(db/delete! User :%lower.email "newuser@metabase.com"))))))))
(deftest login-update-account-test
(testing "A new 'Unknown' name account will be created for a SAML user with no configured first or last name"
(do-with-some-validators-disabled
(fn []
(with-saml-default-setup
(try
(is (not (db/exists? User :%lower.email "newuser@metabase.com")))
;; login with a user with no givenname or surname attributes
(let [req-options (saml-post-request-options (new-user-no-names-saml-test-response)
(saml/str->base64 default-redirect-uri))]
(is (successful-login? (client-full-response :post 302 "/auth/sso" req-options)))
(is (= [{:email "newuser@metabase.com"
:first_name "Unknown"
:is_qbnewb true
:is_superuser false
:id true
:last_name "Unknown"
:date_joined true
:common_name "Unknown Unknown"}]
(->> (mt/boolean-ids-and-timestamps (db/select User :email "newuser@metabase.com"))
(map #(dissoc % :last_login))))))
;; login with the same user, but now givenname and surname attributes exist
(let [req-options (saml-post-request-options (new-user-saml-test-response)
(saml/str->base64 default-redirect-uri))]
(is (successful-login? (client-full-response :post 302 "/auth/sso" req-options)))
(is (= [{:email "newuser@metabase.com"
:first_name "New"
:is_qbnewb true
:is_superuser false
:id true
:last_name "User"
:date_joined true
:common_name "New User"}]
(->> (mt/boolean-ids-and-timestamps (db/select User :email "newuser@metabase.com"))
(map #(dissoc % :last_login))))))
(finally
(db/delete! User :%lower.email "newuser@metabase.com"))))))))
(defn- group-memberships [user-or-id]
(when-let [group-ids (seq (db/select-field :group_id PermissionsGroupMembership :user_id (u/the-id user-or-id)))]
(db/select-field :name PermissionsGroup :id [:in group-ids])))
......
test_resources/saml-test-response-new-user-no-names.xml 0 → 100644
+ 87
− 0
View file @ 174afe58
<?xml version="1.0"?>
<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ID="_6643ab567200fd4a5c73" InResponseTo="_1" Version="2.0" IssueInstant="2018-07-05T18:02:53Z" Destination="http://localhost:3000/auth/sso">
<saml:Issuer xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">urn:saml-metabase-test.auth0.com</saml:Issuer>
<samlp:Status>
<samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/>
</samlp:Status>
<saml:Assertion xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" Version="2.0" ID="_vUq3VKutPOwCGartAC4Cbdmql23G3PE2" IssueInstant="2018-07-05T18:02:53.262Z">
<saml:Issuer>urn:saml-metabase-test.auth0.com</saml:Issuer>
<Signature xmlns="http://www.w3.org/2000/09/xmldsig#">
<SignedInfo>
<CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
<SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
<Reference URI="#_vUq3VKutPOwCGartAC4Cbdmql23G3PE2">
<Transforms>
<Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
<Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
</Transforms>
<DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
<DigestValue>4SH+6iSi9cU3PCPqmJDnP6iK+BY=</DigestValue>
</Reference>
</SignedInfo>
<SignatureValue>Dvfajt2ZBDHmHGl0yLETE1trkR99Bv+kVbgVtqNTRBeeb2oUShBY9EQJKmFtdReuamqr1Tn3kjaxZefxpd1XzB4dzsWNKF0duThKP5YTg5njM3PBsN/yXFAIrIHTjedHEOdwuDgPm+endqEtaFm8yQkn0VhYGK86udpPsMvxwPbYdy2eCAWuHXpNFynfWSRJz7rckubIGvXm7Lar/bMbMYiJa5u+wVqmeH53IddjIEQOeZLsQfPyNeZolWGqBv0TzpM/yEejDjQnnj55fMFXpGS3lKJ02vArYLdgtjW2isgB8/m5dY0gjFob0k9ioy+nghdH/rDKFIvqS/Jb8A50zg==</SignatureValue>
<KeyInfo>
<X509Data>
<X509Certificate>MIIDEzCCAfugAwIBAgIJYpjQiNMYxf1GMA0GCSqGSIb3DQEBCwUAMCcxJTAjBgNVBAMTHHNhbWwtbWV0YWJhc2UtdGVzdC5hdXRoMC5jb20wHhcNMTgwNTI5MjEwMDIzWhcNMzIwMjA1MjEwMDIzWjAnMSUwIwYDVQQDExxzYW1sLW1ldGFiYXNlLXRlc3QuYXV0aDAuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAzNcrpju4sILZQNe1adwg3beXtAMFGB+Buuc414+FDv2OG7X7b9OSYar/nsYfWwiazZRxEGriagd0Sj5mJ4Qqx+zmB/r4UgX3q/KgocRLlShvvz5gTD99hR7LonDPSWET1E9PD4XE1fRaq+BwftFBl45pKTcCR9QrUAFZJ2R/3g06NPZdhe4bg/lTssY5emCxaZpQEku/v+zzpV2nLF4by0vSj7AHsubrsLgsCfV3JvJyTxCyo1aIOlv4Vrx7h9rOgl9eEmoU5XJAl3D7DuvSTEOy7MyDnKF17m7l5nOPZCVOSzmCWvxSCyysijgsM5DSgAE8DPJyoYezV3gTX2OO2QIDAQABo0IwQDAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSpB3lvrtbSDuXkB6fhbjeUpFmL2DAOBgNVHQ8BAf8EBAMCAoQwDQYJKoZIhvcNAQELBQADggEBAAEHGIAhR5GPD2JxgLtpNtZMCYiAM4Gr7hoTQMaKiXgVtdQu4iMFfbpEwIr6UVaDU2HKhvSRFIilOjRGmCGrIzvJgR2l+RL1Z3KrZypI1AXKJT5pF5g5FitBsZq+kiUpdRILl2hICzw9Q1M2Le+JSUcHcbHTVgF24xuzOZonxeE56Oc26Ju4CorLpM3Nb5iYaGOlQ+48/GP82cLxlVyi02va8tp7KP03ePSaZeBEKGpFtBtEN/dC3NKO1mmrT9284H0tvete6KLUH+dsS6bDEYGHZM5KGoSLWRr3qYlCB3AmAw+KvuiuSczLg9oYBkdxlhK9zZvkjCgaLCen+0aY67A=</X509Certificate>
</X509Data>
</KeyInfo>
</Signature>
<saml:Subject>
<saml:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified">auth0|5b0dd0185d7d1617fd8065b5</saml:NameID>
<saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
<saml:SubjectConfirmationData NotOnOrAfter="2018-07-05T19:02:53.262Z" Recipient="http://localhost:3000/auth/sso" InResponseTo="_1"/>
</saml:SubjectConfirmation>
</saml:Subject>
<saml:Conditions NotBefore="2018-07-05T18:02:53.262Z" NotOnOrAfter="2018-07-05T19:02:53.262Z">
<saml:AudienceRestriction>
<saml:Audience>Metabase</saml:Audience>
</saml:AudienceRestriction>
</saml:Conditions>
<saml:AuthnStatement AuthnInstant="2018-07-05T18:02:53.262Z" SessionIndex="_8vKjURdHz4jghbYbj48khvBkLaEeyEqc">
<saml:AuthnContext>
<saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:unspecified</saml:AuthnContextClassRef>
</saml:AuthnContext>
</saml:AuthnStatement>
<saml:AttributeStatement xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
<saml:Attribute Name="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:string">auth0|5b0dd0185d7d1617fd8065b5</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:string">newuser@metabase.com</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:string">newuser@metabase.com</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/upn" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:string">newuser@metabase.com</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.auth0.com/identities/default/provider" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:string">auth0</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.auth0.com/identities/default/connection" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:string">Username-Password-Authentication</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.auth0.com/identities/default/isSocial" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:boolean">false</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.auth0.com/email_verified" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:boolean">true</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.auth0.com/clientID" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:string">W1R5HozFA9cnbFVbmIT8KPdVmH0pU85k</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.auth0.com/picture" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:string">https://s.gravatar.com/avatar/9e18d6fcddec348d9131522c3c535646?s=480&amp;r=pg&amp;d=https%3A%2F%2Fcdn.auth0.com%2Favatars%2Fry.png</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.auth0.com/nickname" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:string">newuser</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.auth0.com/updated_at" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:anyType">Thu Jul 05 2018 18:02:53 GMT+0000 (UTC)</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="http://schemas.auth0.com/created_at" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
<saml:AttributeValue xsi:type="xs:anyType">Tue May 29 2018 22:11:36 GMT+0000 (UTC)</saml:AttributeValue>
</saml:Attribute>
</saml:AttributeStatement>
</saml:Assertion>
</samlp:Response>
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment