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

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

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}]

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."

src/metabase/integrations/common.clj

 (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

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 ------------------------------------------------------
 

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)))))))