From d19be7f5978a5e5c93a54c8111cc584888a56965 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Tue, 8 Jun 2021 14:22:19 -0700
Subject: [PATCH] Ignore unmapped groups during SSO group syncs (#16314)
* changes to sync-group-memberships! and unit tests
* update jwt and saml code
* fix admin sync tests
* fix misplaced docstrings
* address review comment
---
.../enhancements/integrations/ldap.clj | 8 +++--
.../sso/integrations/jwt.clj | 21 +++++++++--
.../sso/integrations/saml.clj | 21 +++++++++--
src/metabase/integrations/common.clj | 34 ++++++++++--------
.../ldap/default_implementation.clj | 16 ++++++---
test/metabase/integrations/common_test.clj | 36 +++++++++++++------
6 files changed, 98 insertions(+), 38 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
index 9af9f71abc8..3838351d91d 100644
--- a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
+++ b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
@@ -68,8 +68,12 @@
:login_attributes attributes}))]
(u/prog1 user
(when sync-groups?
- (let [group-ids (default-impl/ldap-groups->mb-group-ids groups settings)]
- (integrations.common/sync-group-memberships! user group-ids (ldap-sync-admin-group)))))))
+ (let [group-ids (default-impl/ldap-groups->mb-group-ids groups settings)
+ all-mapped-group-ids (default-impl/all-mapped-group-ids settings)]
+ (integrations.common/sync-group-memberships! user
+ group-ids
+ all-mapped-group-ids
+ (ldap-sync-admin-group)))))))
(def ^:private impl
(reify
diff --git a/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj b/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
index 4db2f877176..b05adebff27 100644
--- a/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
+++ b/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
@@ -42,15 +42,30 @@
;; used by Zendesk for their JWT SSO, so it seemed like a good place for us to start
(def ^:private ^:const three-minutes-in-seconds 180)
-(defn- group-names->ids [group-names]
+(defn- group-names->ids
+ "Translate a user's group names to a set of MB group IDs using the configured mappings"
+ [group-names]
(set (mapcat (sso-settings/jwt-group-mappings)
(map keyword group-names))))
-(defn- sync-groups! [user jwt-data]
+(defn- all-mapped-group-ids
+ "Returns the set of all MB group IDs that have configured mappings"
+ []
+ (-> (sso-settings/jwt-group-mappings)
+ vals
+ flatten
+ set))
+
+(defn- sync-groups!
+ "Sync a user's groups based on mappings configured in the JWT settings"
+ [user jwt-data]
(when (sso-settings/jwt-group-sync)
(when-let [groups-attribute (jwt-attribute-groups)]
(when-let [group-names (get jwt-data (jwt-attribute-groups))]
- (integrations.common/sync-group-memberships! user (group-names->ids group-names) false)))))
+ (integrations.common/sync-group-memberships! user
+ (group-names->ids group-names)
+ (all-mapped-group-ids)
+ false)))))
(defn- login-jwt-user
[jwt {{redirect :return_to} :params, :as request}]
diff --git a/enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj b/enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj
index 8ab452d0adf..d9fbd0935c2 100644
--- a/enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj
+++ b/enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj
@@ -37,16 +37,31 @@
[schema.core :as s])
(:import java.util.UUID))
-(defn- group-names->ids [group-names]
+(defn- group-names->ids
+ "Translate a user's group names to a set of MB group IDs using the configured mappings"
+ [group-names]
(->> (cond-> group-names (string? group-names) vector)
(map keyword)
(mapcat (sso-settings/saml-group-mappings))
set))
-(defn- sync-groups! [user group-names]
+(defn- all-mapped-group-ids
+ "Returns the set of all MB group IDs that have configured mappings"
+ []
+ (-> (sso-settings/saml-group-mappings)
+ vals
+ flatten
+ set))
+
+(defn- sync-groups!
+ "Sync a user's groups based on mappings configured in the SAML settings"
+ [user group-names]
(when (sso-settings/saml-group-sync)
(when group-names
- (integrations.common/sync-group-memberships! user (group-names->ids group-names) false))))
+ (integrations.common/sync-group-memberships! user
+ (group-names->ids group-names)
+ (all-mapped-group-ids)
+ false))))
(s/defn ^:private fetch-or-create-user! :- (s/maybe {:id UUID, s/Keyword s/Any})
"Returns a Session for the given `email`. Will create the user if needed."
diff --git a/src/metabase/integrations/common.clj b/src/metabase/integrations/common.clj
index e8ca1358e1d..13e65970ffc 100644
--- a/src/metabase/integrations/common.clj
+++ b/src/metabase/integrations/common.clj
@@ -1,6 +1,7 @@
(ns metabase.integrations.common
"Shared functionality used by different integrations."
(:require [clojure.data :as data]
+ [clojure.set :as set]
[clojure.tools.logging :as log]
[metabase.models.permissions-group :as group]
[metabase.models.permissions-group-membership :refer [PermissionsGroupMembership]]
@@ -9,21 +10,24 @@
[toucan.db :as db]))
(defn sync-group-memberships!
- "Update the PermissionsGroups a User belongs to, adding or deleting membership entries as needed so that Users is only
- in `new-groups-or-ids`. Ignores special groups like `all-users`."
- [user-or-id new-groups-or-ids sync-admin-group?]
- ;; if you AREN'T syncing the admin group, it's a "special group", which means it's excluded from being added
- ;; or removed from a user
- (let [special-group-ids (if (false? sync-admin-group?)
- #{(u/the-id (group/admin)) (u/the-id (group/all-users))}
- #{(u/the-id (group/all-users))})
+ "Update the PermissionsGroups a User belongs to, adding or deleting membership entries as needed so that Users is
+ only in `new-groups-or-ids`. Ignores special groups like `all-users`, and only touches groups with mappings set."
+ [user-or-id new-groups-or-ids mapped-groups-or-ids sync-admin-group?]
+ (let [included-group-ids (cond-> (set (map u/the-id mapped-groups-or-ids))
+ sync-admin-group? (conj (u/the-id (group/admin))))
+ excluded-group-ids (cond-> #{(u/the-id (group/all-users))}
+ (not sync-admin-group?) (conj (u/the-id (group/admin))))
user-id (u/the-id user-or-id)
- ;; Get a set of Group IDs the user currently belongs to
- current-group-ids (db/select-field :group_id PermissionsGroupMembership
- :user_id user-id
- :group_id [:not-in special-group-ids])
- new-group-ids (set (map u/the-id new-groups-or-ids))
- ;; determine what's different between current groups and new groups
+ ;; Get a set of mapped Group IDs the user currently belongs to
+ current-group-ids (when (seq included-group-ids)
+ (db/select-field :group_id PermissionsGroupMembership
+ :user_id user-id
+ ;; Add nil to included group ids to ensure valid SQL if set is empty
+ :group_id [:in included-group-ids]
+ :group_id [:not-in excluded-group-ids]))
+ new-group-ids (set/intersection (set (map u/the-id new-groups-or-ids))
+ included-group-ids)
+ ;; determine what's different between current mapped groups and new mapped groups
[to-remove to-add] (data/diff current-group-ids new-group-ids)]
;; remove membership from any groups as needed
(when (seq to-remove)
@@ -31,7 +35,7 @@
(db/delete! PermissionsGroupMembership :group_id [:in to-remove], :user_id user-id))
;; add new memberships for any groups as needed
(doseq [id to-add
- :when (not (special-group-ids id))]
+ :when (not (excluded-group-ids id))]
(log/debugf "Adding user %s to group %s" user-id id)
;; if adding membership fails for one reason or another (i.e. if the group doesn't exist) log the error add the
;; user to the other groups rather than failing entirely
diff --git a/src/metabase/integrations/ldap/default_implementation.clj b/src/metabase/integrations/ldap/default_implementation.clj
index 0e43088938f..5bbc4ac55e1 100644
--- a/src/metabase/integrations/ldap/default_implementation.clj
+++ b/src/metabase/integrations/ldap/default_implementation.clj
@@ -82,7 +82,7 @@
;;; --------------------------------------------- fetch-or-create-user! ----------------------------------------------
(s/defn ldap-groups->mb-group-ids :- #{su/IntGreaterThanZero}
- "Translate a set of DNs to a set of MB group IDs using the configured mappings."
+ "Translate a set of a user's group DNs to a set of MB group IDs using the configured mappings."
[ldap-groups :- (s/maybe [su/NonBlankString])
{:keys [group-mappings]} :- (select-keys i/LDAPSettings [:group-mappings s/Keyword])]
(-> group-mappings
@@ -91,6 +91,14 @@
flatten
set))
+(s/defn all-mapped-group-ids :- #{su/IntGreaterThanZero}
+ "Returns the set of all MB group IDs that have configured mappings."
+ [{:keys [group-mappings]} :- (select-keys i/LDAPSettings [:group-mappings s/Keyword])]
+ (-> group-mappings
+ vals
+ flatten
+ set))
+
(s/defn ^:private fetch-or-create-user!* :- (class User)
[{:keys [first-name last-name email groups]} :- i/UserInfo
{:keys [sync-groups?], :as settings} :- i/LDAPSettings]
@@ -101,9 +109,9 @@
:email email}))]
(u/prog1 user
(when sync-groups?
- (let [group-ids (ldap-groups->mb-group-ids groups settings)]
- (integrations.common/sync-group-memberships! user group-ids false))))))
-
+ (let [group-ids (ldap-groups->mb-group-ids groups settings)
+ all-mapped-group-ids (all-mapped-group-ids settings)]
+ (integrations.common/sync-group-memberships! user group-ids all-mapped-group-ids false))))))
;;; ------------------------------------------------------ impl ------------------------------------------------------
diff --git a/test/metabase/integrations/common_test.clj b/test/metabase/integrations/common_test.clj
index bbe9bd8ca36..4d2c8f232d3 100644
--- a/test/metabase/integrations/common_test.clj
+++ b/test/metabase/integrations/common_test.clj
@@ -53,7 +53,7 @@
(testing "does syncing group memberships leave existing memberships in place if nothing has changed?"
(with-user-in-groups [group {:name (str ::group)}
user [group]]
- (integrations.common/sync-group-memberships! user #{group} false)
+ (integrations.common/sync-group-memberships! user #{group} #{group} false)
(is (= #{"All Users" ":metabase.integrations.common-test/group"}
(group-memberships user)))))
@@ -64,7 +64,7 @@
:group_id (u/get-id group)
:user_id (u/get-id user))
original-membership-id (membership-id)]
- (integrations.common/sync-group-memberships! user #{group} false)
+ (integrations.common/sync-group-memberships! user #{group} #{group} false)
(is (= original-membership-id
(membership-id))))))
@@ -72,7 +72,7 @@
(with-user-in-groups [group-1 {:name (str ::group-1)}
group-2 {:name (str ::group-2)}
user [group-1]]
- (integrations.common/sync-group-memberships! user #{group-1 group-2} false)
+ (integrations.common/sync-group-memberships! user #{group-1 group-2} #{group-1 group-2} false)
(is (= #{":metabase.integrations.common-test/group-1"
":metabase.integrations.common-test/group-2"
"All Users"}
@@ -82,7 +82,7 @@
(with-user-in-groups [group-1 {:name (str ::group-1)}
group-2 {:name (str ::group-2)}
user [group-1]]
- (integrations.common/sync-group-memberships! user #{} false)
+ (integrations.common/sync-group-memberships! user #{} #{group-1 group-2} false)
(is (= #{"All Users"}
(group-memberships user)))))
@@ -90,15 +90,29 @@
(with-user-in-groups [group-1 {:name (str ::group-1)}
group-2 {:name (str ::group-2)}
user [group-1]]
- (integrations.common/sync-group-memberships! user #{group-2} false)
+ (integrations.common/sync-group-memberships! user #{group-2} #{group-1 group-2} false)
(is (= #{":metabase.integrations.common-test/group-2" "All Users"}
(group-memberships user)))))
+ (testing "are unmapped groups ignored when adding group memberships?"
+ (with-user-in-groups [group-1 {:name (str ::group-1)}
+ user []]
+ (integrations.common/sync-group-memberships! user #{group-1} #{} false)
+ (is (= #{"All Users"} (group-memberships user)))))
+
+ (testing "are unmapped groups ignored when removing group memberships?"
+ (with-user-in-groups [group-1 {:name (str ::group-1)}
+ user [group-1]]
+ (integrations.common/sync-group-memberships! user #{} #{} false)
+ (is (= #{":metabase.integrations.common-test/group-1"
+ "All Users"}
+ (group-memberships user)))))
+
(testing "if we attempt to add a user to a group that doesn't exist, does the group sync complete for the other groups?"
(with-user-in-groups [group {:name (str ::group)}
- user [group]]
+ user []]
(tu.log/suppress-output
- (integrations.common/sync-group-memberships! user [Integer/MAX_VALUE group] false))
+ (integrations.common/sync-group-memberships! user #{Integer/MAX_VALUE group} #{Integer/MAX_VALUE group} false))
(is (= #{"All Users" ":metabase.integrations.common-test/group"}
(group-memberships user)))))
@@ -106,25 +120,25 @@
(testing "with admin group sync enabled"
(testing "are admins synced?"
(with-user-in-groups [user]
- (integrations.common/sync-group-memberships! user [(group/admin)] true)
+ (integrations.common/sync-group-memberships! user [(group/admin)] #{} true)
(is (= #{"Administrators" "All Users"}
(group-memberships user)))))
(testing "are administrators removed appropriately?"
(with-user-in-groups [user [(group/admin)]]
- (integrations.common/sync-group-memberships! user [] true)
+ (integrations.common/sync-group-memberships! user [] #{} true)
(is (= #{"All Users"}
(group-memberships user)))))))
(testing "with admin group sync disabled"
(testing "are admins synced?"
(with-user-in-groups [user]
- (integrations.common/sync-group-memberships! user [(group/admin)] false)
+ (integrations.common/sync-group-memberships! user [(group/admin)] #{} false)
(is (= #{"All Users"}
(group-memberships user)))))
(testing "are administrators removed appropriately?"
(with-user-in-groups [user [(group/admin)]]
- (integrations.common/sync-group-memberships! user [] false)
+ (integrations.common/sync-group-memberships! user [] #{} false)
(is (= #{"Administrators" "All Users"}
(group-memberships user)))))))
--
GitLab