Adding the ability to sync admin groups in EE (#14993)

* Adding the ability to sync admin groups in EE

Adds a LDAP setting for EE to enable to LDAP Administrator group syncs.

Adds the setting to admin UI to enable syncing of the admin group.

Removes two unused settings from the OSS side.

metabase/metabase-enterprise#445

* Apply suggestions from code review

Co-authored-by: Dalton <daltojohnso@users.noreply.github.com>

Co-authored-by: Dalton <daltojohnso@users.noreply.github.com>

enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj

@@ -20,7 +20,7 @@
 
 (defsetting ldap-sync-user-attributes
   (deferred-tru "Should we sync user attributes when someone logs in via LDAP?")
-  :type :boolean
+  :type    :boolean
   :default true)
 
 ;; TODO - maybe we want to add a csv setting type?
@@ -29,6 +29,11 @@
   :default "userPassword,dn,distinguishedName"
   :type    :csv)
 
+(defsetting ldap-sync-admin-group
+  (deferred-tru "Sync the admin group?")
+  :type    :boolean
+  :default false)
+
 (defn- syncable-user-attributes [m]
   (when (ldap-sync-user-attributes)
     (apply dissoc m :objectclass (map (comp keyword u/lower-case-en) (ldap-sync-user-attributes-blacklist)))))
@@ -63,7 +68,7 @@
     (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))))))
+          (integrations.common/sync-group-memberships! user group-ids (ldap-sync-admin-group)))))))
 
 (def ^:private impl
   (reify

enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj

@@ -49,7 +49,7 @@
   (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))))))
+        (integrations.common/sync-group-memberships! user (group-names->ids group-names) false)))))
 
 (defn- login-jwt-user
   [jwt {{redirect :return_to} :params, :as request}]

enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj

@@ -29,7 +29,7 @@
 (defn- sync-groups! [user group-names]
   (when (sso-settings/saml-group-sync)
     (when group-names
-      (integrations.common/sync-group-memberships! user (group-names->ids group-names)))))
+      (integrations.common/sync-group-memberships! user (group-names->ids group-names) false))))
 
 (s/defn saml-auth-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."

enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj

 (ns metabase-enterprise.enhancements.integrations.ldap-test
   (:require [clojure.test :refer :all]
+            [metabase-enterprise.enhancements.integrations.ldap :as ldap-ee]
             [metabase.integrations.ldap :as ldap]
             [metabase.models.user :as user :refer [User]]
             [metabase.public-settings.metastore :as metastore]
@@ -70,7 +71,7 @@
 
       (testing "ignored attributes should not be returned"
         (mt/with-temporary-setting-values [ldap-sync-user-attributes-blacklist
-                                           (cons "title" (ldap/ldap-sync-user-attributes-blacklist))]
+                                           (cons "title" (ldap-ee/ldap-sync-user-attributes-blacklist))]
           (is (= {:dn         "cn=Lucky Pigeon,ou=Birds,dc=metabase,dc=com"
                   :first-name "Lucky"
                   :last-name  "Pigeon"

enterprise/frontend/src/metabase-enterprise/auth/index.js

@@ -261,3 +261,14 @@ PLUGIN_SHOW_CHANGE_PASSWORD_CONDITIONS.push(
     !user.ldap_auth &&
     MetabaseSettings.get("enable-password-login"),
 );
+
+PLUGIN_ADMIN_SETTINGS_UPDATES.push(sections =>
+  updateIn(sections, ["authentication/ldap", "settings"], settings => [
+    ...settings,
+    {
+      key: "ldap-sync-admin-group",
+      display_name: t`Sync Administrator group`,
+      type: "boolean",
+    },
+  ]),
+);

frontend/src/metabase/admin/settings/components/SettingsLdapForm.jsx

@@ -47,7 +47,13 @@ export default class SettingsLdapForm extends React.Component {
           },
           {
             title: t`Group Schema`,
-            settings: ["ldap-group-sync", "ldap-group-base"],
+            settings: [
+              "ldap-group-sync",
+              "ldap-group-base",
+              "ldap-sync-admin-group" in this.props.settingValues
+                ? "ldap-sync-admin-group"
+                : null,
+            ].filter(Boolean),
           },
         ]}
       />

src/metabase/integrations/common.clj

@@ -10,14 +10,18 @@
 
 (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 `admin` and `all-users`."
-  [user-or-id new-groups-or-ids]
-  (let [special-group-ids  #{(u/get-id (group/admin)) (u/get-id (group/all-users))}
+  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/get-id (group/admin)) (u/get-id (group/all-users))}
+                             #{(u/get-id (group/all-users))})
         user-id            (u/get-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])
+                                            :user_id  user-id
+                                            :group_id [:not-in special-group-ids])
         new-group-ids      (set (map u/get-id new-groups-or-ids))
         ;; determine what's different between current groups and new groups
         [to-remove to-add] (data/diff current-group-ids new-group-ids)]

src/metabase/integrations/ldap.clj

@@ -84,17 +84,6 @@
                  (throw (IllegalArgumentException. (tru "{0} is not a valid DN." (name k))))))
              (setting/set-json! :ldap-group-mappings new-value)))
 
-(defsetting ldap-sync-user-attributes
-  (deferred-tru "Should we sync user attributes when someone logs in via LDAP?")
-  :type :boolean
-  :default true)
-
-;; TODO - maybe we want to add a csv setting type?
-(defsetting ldap-sync-user-attributes-blacklist
-  (deferred-tru "Comma-separated list of user attributes to skip syncing for LDAP users.")
-  :default "userPassword,dn,distinguishedName"
-  :type    :csv)
-
 (defsetting ldap-configured?
   "Check if LDAP is enabled and that the mandatory settings are configured."
   :type       :boolean

src/metabase/integrations/ldap/default_implementation.clj

@@ -102,7 +102,7 @@
     (u/prog1 user
       (when sync-groups?
         (let [group-ids (ldap-groups->mb-group-ids groups settings)]
-          (integrations.common/sync-group-memberships! user group-ids))))))
+          (integrations.common/sync-group-memberships! user group-ids false))))))
 
 
 ;;; ------------------------------------------------------ impl ------------------------------------------------------

test/metabase/integrations/common_test.clj

 (ns metabase.integrations.common-test
-  (:require [expectations :refer [expect]]
+  (:require [clojure.test :refer :all]
             [metabase.integrations.common :as integrations.common]
             [metabase.models.permissions-group :as group :refer [PermissionsGroup]]
             [metabase.models.permissions-group-membership :refer [PermissionsGroupMembership]]
             [metabase.models.user :refer [User]]
+            [metabase.test :as mt]
+            [metabase.test.fixtures :as fixtures]
             [metabase.test.util.log :as tu.log]
             [metabase.util :as u]
             [toucan.db :as db]
             [toucan.util.test :as tt]))
 
+(use-fixtures :once (fixtures/initialize :db))
+
 (defn- group-memberships
   "Return set of names of PermissionsGroups `user` currently belongs to."
   [user]
@@ -45,68 +49,82 @@
     (let [[user-binding groups-or-ids-to-put-user-in] bindings]
       `(do-with-user-in-groups (fn [~user-binding] ~@body) ~groups-or-ids-to-put-user-in))))
 
-;; does syncing group memberships leave existing memberships in place if nothing has changed?
-(expect
-  #{"All Users" ":metabase.integrations.common-test/group"}
-  (with-user-in-groups [group {:name (str ::group)}
-                        user  [group]]
-    (integrations.common/sync-group-memberships! user #{group})
-    (group-memberships user)))
-
-;; the actual `PermissionsGroupMembership` object should not have been replaced
-(expect
-  (with-user-in-groups [group {:name (str ::group)}
-                        user  [group]]
-    (let [membership-id          #(db/select-one-id PermissionsGroupMembership
-                                    :group_id (u/get-id group)
-                                    :user_id  (u/get-id user))
-          original-membership-id (membership-id)]
-      (integrations.common/sync-group-memberships! user #{group})
-      (= original-membership-id
-         (membership-id)))))
-
-;; does syncing group memberships add users to new groups correctly?
-(expect
-  #{":metabase.integrations.common-test/group-1"
-    ":metabase.integrations.common-test/group-2"
-    "All Users"}
-  (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})
-    (group-memberships user)))
-
-;; does syncing group memberships remove users from old groups correctly?
-(expect
-  #{"All Users"}
-  (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-memberships user)))
-
-;; does adding & removing at the same time work correctly?
-(expect
-  #{":metabase.integrations.common-test/group-2" "All Users"}
-  (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})
-    (group-memberships user)))
-
-;; if we attempt to add a user to a group that doesn't exist, does the group sync complete for the other groups?
-
-(expect
-  #{"All Users" ":metabase.integrations.common-test/group"}
-  (with-user-in-groups [group {:name (str ::group)}
-                        user    [group]]
-    (tu.log/suppress-output
-      (integrations.common/sync-group-memberships! user [Integer/MAX_VALUE group]))
-    (group-memberships user)))
-
-;; are special groups like admin & all users ignored?
-(expect
-  #{"All Users"}
-  (with-user-in-groups [user]
-    (integrations.common/sync-group-memberships! user [(group/admin)])
-    (group-memberships user)))
+(deftest sync-groups-test
+  (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)
+      (is (= #{"All Users" ":metabase.integrations.common-test/group"}
+             (group-memberships user)))))
+
+  (testing "the actual `PermissionsGroupMembership` object should not have been replaced"
+    (with-user-in-groups [group {:name (str ::group)}
+                          user  [group]]
+      (let [membership-id          #(db/select-one-id PermissionsGroupMembership
+                                                      :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)
+        (is (= original-membership-id
+               (membership-id))))))
+
+  (testing "does syncing group memberships add users to new groups correctly?"
+    (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)
+      (is (= #{":metabase.integrations.common-test/group-1"
+               ":metabase.integrations.common-test/group-2"
+               "All Users"}
+             (group-memberships user)))))
+
+  (testing "does syncing group memberships remove users from old groups correctly?"
+    (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)
+      (is (= #{"All Users"}
+             (group-memberships user)))))
+
+  (testing "does adding & removing at the same time work correctly?"
+    (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)
+      (is (= #{":metabase.integrations.common-test/group-2" "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]]
+      (tu.log/suppress-output
+       (integrations.common/sync-group-memberships! user [Integer/MAX_VALUE group] false))
+      (is (= #{"All Users" ":metabase.integrations.common-test/group"}
+             (group-memberships user)))))
+
+  (mt/with-test-user :crowberto
+    (testing "with admin group sync enabled"
+      (testing "are admins synced?"
+        (with-user-in-groups [user]
+          (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)
+          (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)
+        (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)
+        (is (= #{"Administrators" "All Users"}
+               (group-memberships user)))))))