Fix SSO failed to sync admin group (#20991)

* fix failed to sync admin group

* address noah's comments and add migration script

* document for run-with-data-migration-index

* update comments

* fix name space

* adding data_migrations tests

* add docg

* make sure we don't remove admin group if sso and ldap are not configured

* fix tests for be-ee

* fix tests for oss

* misc docs updates

* remove data-migration-index

* return some newlines

* is it failling here?

* update data_migration docs

* update data_migration docs

* fix all styling comments

* make migration to run both in oss and enterprise and make sure the tests are accounted for that

* fix failed namespace checks

* Add a comment to the test

* fix per comments

* Update permissions.clj

* tweaking with the  icon

* refactor with-temporary-raw-setting-values

* update comments

* Add extension for cert file

* address Noah's comments

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

@@ -30,11 +30,6 @@
   :default "userPassword,dn,distinguishedName"
   :type    :csv)
 
-(defsetting ldap-sync-admin-group
-  (deferred-tru "Sync the admin group?")
-  :type    :boolean
-  :default false)
-
 (defsetting ldap-group-membership-filter
   (deferred-tru "Group membership lookup filter. The placeholders '{dn}' and '{uid}' will be replaced by the user''s Distinguished Name and UID, respectively.")
   :default "(member={dn})")
@@ -92,8 +87,7 @@
               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)))))))
+                                                       all-mapped-group-ids))))))
 
 (def ^:private impl
   (reify

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

@@ -62,11 +62,10 @@
   [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))]
+      (when-let [group-names (get jwt-data groups-attribute)]
         (integrations.common/sync-group-memberships! user
                                                      (group-names->ids group-names)
-                                                     (all-mapped-group-ids)
-                                                     false)))))
+                                                     (all-mapped-group-ids))))))
 
 (defn- login-jwt-user
   [jwt {{redirect :return_to} :params, :as request}]

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

@@ -60,8 +60,7 @@
     (when group-names
       (integrations.common/sync-group-memberships! user
                                                    (group-names->ids group-names)
-                                                   (all-mapped-group-ids)
-                                                   false))))
+                                                   (all-mapped-group-ids)))))
 
 (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."

enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj

@@ -59,26 +59,7 @@
 (def ^:private default-idp-uri            "http://test.idp.metabase.com")
 (def ^:private default-redirect-uri       "http://localhost:3000/test")
 (def ^:private default-idp-uri-with-param (str default-idp-uri "?someparam=true"))
-
-(def ^:private default-idp-cert
-  "Public certificate from Auth0, used to validate mock SAML responses from Auth0"
-  "MIIDEzCCAfugAwIBAgIJYpjQiNMYxf1GMA0GCSqGSIb3DQEBCwUAMCcxJTAjBgNV
-BAMTHHNhbWwtbWV0YWJhc2UtdGVzdC5hdXRoMC5jb20wHhcNMTgwNTI5MjEwMDIz
-WhcNMzIwMjA1MjEwMDIzWjAnMSUwIwYDVQQDExxzYW1sLW1ldGFiYXNlLXRlc3Qu
-YXV0aDAuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAzNcrpju4
-sILZQNe1adwg3beXtAMFGB+Buuc414+FDv2OG7X7b9OSYar/nsYfWwiazZRxEGri
-agd0Sj5mJ4Qqx+zmB/r4UgX3q/KgocRLlShvvz5gTD99hR7LonDPSWET1E9PD4XE
-1fRaq+BwftFBl45pKTcCR9QrUAFZJ2R/3g06NPZdhe4bg/lTssY5emCxaZpQEku/
-v+zzpV2nLF4by0vSj7AHsubrsLgsCfV3JvJyTxCyo1aIOlv4Vrx7h9rOgl9eEmoU
-5XJAl3D7DuvSTEOy7MyDnKF17m7l5nOPZCVOSzmCWvxSCyysijgsM5DSgAE8DPJy
-oYezV3gTX2OO2QIDAQABo0IwQDAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSp
-B3lvrtbSDuXkB6fhbjeUpFmL2DAOBgNVHQ8BAf8EBAMCAoQwDQYJKoZIhvcNAQEL
-BQADggEBAAEHGIAhR5GPD2JxgLtpNtZMCYiAM4Gr7hoTQMaKiXgVtdQu4iMFfbpE
-wIr6UVaDU2HKhvSRFIilOjRGmCGrIzvJgR2l+RL1Z3KrZypI1AXKJT5pF5g5FitB
-sZq+kiUpdRILl2hICzw9Q1M2Le+JSUcHcbHTVgF24xuzOZonxeE56Oc26Ju4CorL
-pM3Nb5iYaGOlQ+48/GP82cLxlVyi02va8tp7KP03ePSaZeBEKGpFtBtEN/dC3NKO
-1mmrT9284H0tvete6KLUH+dsS6bDEYGHZM5KGoSLWRr3qYlCB3AmAw+KvuiuSczL
-g9oYBkdxlhK9zZvkjCgaLCen+0aY67A=")
+(def ^:private default-idp-cert           (slurp "test_resources/sso/auth0-public-idp.cert"))
 
 (defn- do-with-some-validators-disabled
   "The sample responses all have `InResponseTo=\"_1\"` and invalid assertion signatures (they were edited by hand) so

src/metabase/api/session.clj

@@ -104,7 +104,7 @@
         ;; password is ok, return new session if user is not deactivated
         (let [user (ldap/fetch-or-create-user! user-info)]
           (if (:is_active user)
-            (create-session! :sso (ldap/fetch-or-create-user! user-info) device-info)
+            (create-session! :sso user device-info)
             (throw (ex-info (str disabled-account-message)
                             {:status-code 401
                              :errors      {:_error disabled-account-snippet}})))))

src/metabase/db/data_migrations.clj

 (ns ^:deprecated metabase.db.data-migrations
   "Clojure-land data migration definitions and fns for running them.
-  These migrations are all ran once when Metabase is first launched, except when transferring data from an existing
-  H2 database.  When data is transferred from an H2 database, migrations will already have been run against that data;
-  thus, all of these migrations need to be repeatable, e.g.:
+  Data migrations are run once when Metabase is first launched.
+  Note that there is no locking mechanism for data-migration - thus upon launching Metabase, It's possible
+  for a migration to be run multiple times (e.g: when running multiple Metabase instances).
 
+  That said, these migrations should be idempotent, e.g:
      CREATE TABLE IF NOT EXISTS ... -- Good
      CREATE TABLE ...               -- Bad"
   (:require [cheshire.core :as json]
@@ -11,6 +12,8 @@
             [clojure.walk :as walk]
             [medley.core :as m]
             [metabase.models.dashboard-card :refer [DashboardCard]]
+            [metabase.models.permissions-group :as group]
+            [metabase.models.setting :as setting :refer [Setting]]
             [metabase.util :as u]
             [toucan.db :as db]
             [toucan.models :as models]))
@@ -32,11 +35,12 @@
     (when-not (contains? ran-migrations migration-name)
       (log/info (format "Running data migration '%s'..." migration-name))
       (try
-        (@migration-var)
-        (catch Exception e
-          (if catch?
-            (log/warn (format "Data migration %s failed: %s" migration-name (.getMessage e)))
-            (throw e))))
+       (db/transaction
+        (@migration-var))
+       (catch Exception e
+         (if catch?
+           (log/warn (format "Data migration %s failed: %s" migration-name (.getMessage e)))
+           (throw e))))
       (db/insert! DataMigrations
         :id        migration-name
         :timestamp :%now))))
@@ -172,6 +176,41 @@
                                  [:like
                                   :dashcard.visualization_settings "%\"click_link_template\":%"]]})))
 
+(defn- raw-setting
+  "Get raw setting directly from DB.
+  For some reasons during data-migration [[metabase.models.setting/get]] return the default value defined in
+  [[metabase.models.setting/defsetting]] instead of value from Setting table."
+  [k]
+  (db/select-one-field :value Setting :key (name k)))
+
+(defn- remove-admin-group-from-mappings-by-setting-key!
+  [mapping-setting-key]
+  (let [admin-group-id (:id (group/admin))
+        mapping        (try
+                        (json/parse-string (raw-setting mapping-setting-key))
+                        (catch Exception _e
+                          {}))]
+    (when-not (empty? mapping)
+      (db/update! Setting (name mapping-setting-key)
+                  :value
+                  (->> mapping
+                       (map (fn [[k v]] [k (filter #(not= admin-group-id %) v)]))
+                       (into {})
+                       json/generate-string)))))
+
+(defmigration ^{:author "qnkhuat" :added "0.43.0"} migrate-remove-admin-from-group-mapping-if-needed
+  ;;  In the past we have a setting to disable group sync for admin group when using SSO or LDAP, but it's broken and haven't really worked (see #13820)
+  ;;  In #20991 we remove this option entirely and make sync for admin group just like a regular group.
+  ;;  But on upgrade, to make sure we don't unexpectedly begin adding or removing admin users:
+  ;;  - for LDAP, if the `ldap-sync-admin-group` toggle is disabled, we remove all mapping for the admin group
+  ;;  - for SAML, JWT, we remove all mapping for admin group, because they were previously never being synced
+  (when (= (raw-setting :ldap-sync-admin-group) "false")
+    (remove-admin-group-from-mappings-by-setting-key! :ldap-group-mappings))
+  ;; sso are enterprise feature but we still run this even in OSS in case a customer
+  ;; have switched from enterprise -> SSO and stil have this mapping in Setting table
+  (remove-admin-group-from-mappings-by-setting-key! :jwt-group-mappings)
+  (remove-admin-group-from-mappings-by-setting-key! :saml-group-mappings))
+
 ;; !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 ;; !!                                                                                                               !!
 ;; !!    Please seriously consider whether any new migrations you write here could be written as Liquibase ones     !!

src/metabase/integrations/common.clj

@@ -12,21 +12,17 @@
 (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`, 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-or-id new-groups-or-ids mapped-groups-or-ids]
+  (let [mapped-group-ids   (set (map u/the-id mapped-groups-or-ids))
+        excluded-group-ids #{(u/the-id (group/all-users))}
         user-id            (u/the-id user-or-id)
-        ;; Get a set of mapped Group IDs the user currently belongs to
-        current-group-ids  (when (seq included-group-ids)
+        current-group-ids  (when (seq mapped-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 [:in mapped-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)
+                                             mapped-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

src/metabase/integrations/ldap/default_implementation.clj

@@ -48,11 +48,11 @@
 
 (s/defn ^:private user-groups :- (s/maybe [su/NonBlankString])
   "Retrieve groups for a supplied DN."
-  [ldap-connection                              :- LDAPConnectionPool
-   dn                                           :- su/NonBlankString
-   uid                                          :- su/NonBlankString
-   {:keys [group-base]}                         :- i/LDAPSettings
-   group-membership-filter                      :- su/NonBlankString]
+  [ldap-connection         :- LDAPConnectionPool
+   dn                      :- su/NonBlankString
+   uid                     :- su/NonBlankString
+   {:keys [group-base]}    :- i/LDAPSettings
+   group-membership-filter :- su/NonBlankString]
   (when group-base
     (let [results (ldap-client/search
                    ldap-connection
@@ -64,7 +64,7 @@
 (s/defn ldap-search-result->user-info :- (s/maybe i/UserInfo)
   "Convert the result "
   [ldap-connection               :- LDAPConnectionPool
-   {:keys [dn uid], :as result} :- su/Map
+   {:keys [dn uid], :as result}  :- su/Map
    {:keys [first-name-attribute
            last-name-attribute
            email-attribute
@@ -125,15 +125,16 @@
 (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]
-  (let [user (db/select-one [User :id :last_login :first_name :last_name :is_active] :%lower.email (u/lower-case-en email))
+  (let [user     (db/select-one [User :id :last_login :first_name :last_name :is_active]
+                                :%lower.email (u/lower-case-en email))
         new-user (if user
                    (let [old-first-name (:first_name user)
-                         old-last-name (:last_name user)
+                         old-last-name  (:last_name user)
                          new-first-name (updated-name-part first-name old-first-name)
-                         new-last-name (updated-name-part last-name old-last-name)
-                         user-changes (merge
-                                       (when-not (= new-first-name old-first-name) {:first_name new-first-name})
-                                       (when-not (= new-last-name old-last-name) {:last_name new-last-name}))]
+                         new-last-name  (updated-name-part last-name old-last-name)
+                         user-changes   (merge
+                                          (when-not (= new-first-name old-first-name) {:first_name new-first-name})
+                                          (when-not (= new-last-name old-last-name) {:last_name new-last-name}))]
                      (if (seq user-changes)
                        (do
                          (db/update! User (:id user) user-changes)
@@ -145,9 +146,9 @@
                        (assoc :is_active true)))]
     (u/prog1 new-user
       (when sync-groups?
-        (let [group-ids   (ldap-groups->mb-group-ids groups settings)
+        (let [group-ids            (ldap-groups->mb-group-ids groups settings)
               all-mapped-group-ids (all-mapped-group-ids settings)]
-          (integrations.common/sync-group-memberships! new-user group-ids all-mapped-group-ids false))))))
+          (integrations.common/sync-group-memberships! new-user group-ids all-mapped-group-ids))))))
 
 ;;; ------------------------------------------------------ impl ------------------------------------------------------
 

src/metabase/models/permissions.clj

@@ -133,13 +133,13 @@
   | Data perms? | Coll perms? | Block? | Segmented? | Can run? |
   | ----------- | ----------- | ------ | ---------- | -------- |
   |          no |          no |     no |         no |       ⛔ |
-  |          no |          no |     no |        yes |       ⚠ |
+  |          no |          no |     no |        yes |       ⚠️ |
   |          no |          no |    yes |         no |       ⛔ |
-  |          no |          no |    yes |        yes |       ⚠ |
+  |          no |          no |    yes |        yes |       ⚠️ |
   |          no |         yes |     no |         no |       ✅ |
-  |          no |         yes |     no |        yes |       ⚠ |
+  |          no |         yes |     no |        yes |       ⚠️ |
   |          no |         yes |    yes |         no |       ⛔ |
-  |          no |         yes |    yes |        yes |       ⚠ |
+  |          no |         yes |    yes |        yes |       ⚠️ |
   |         yes |          no |     no |         no |       ✅ |
   |         yes |          no |     no |        yes |       ✅ |
   |         yes |          no |    yes |         no |       ✅ |
@@ -149,7 +149,7 @@
   |         yes |         yes |    yes |         no |       ✅ |
   |         yes |         yes |    yes |        yes |       ✅ |
 
-  (`⚠` = runs in sandboxed mode)
+  (`⚠️` = runs in sandboxed mode)
 
   ### Known Permissions Paths
 

src/metabase/models/setting.clj

@@ -123,7 +123,13 @@
   these names to avoid unintended side-effects if an application database still stores values for these settings."
   #{"-site-url"
     "enable-advanced-humanization"
-    "metabot-enabled"})
+    "metabot-enabled"
+    "ldap-sync-admin-group"})
+
+(def ^:dynamic *allow-retired-setting-names*
+  "A dynamic val that controls whether it's allowed to use retired settings.
+  Primarily used in test to disable retired setting check."
+  false)
 
 (models/defmodel Setting
   "The model that underlies [[defsetting]]."
@@ -741,7 +747,7 @@
                                setting-name (:name same-munge))
                           {:existing-setting (dissoc same-munge :on-change :getter :setter)
                            :new-setting      (dissoc <> :on-change :getter :setter)}))))
-      (when (retired-setting-names (name setting-name))
+      (when (and (retired-setting-names (name setting-name)) (not *allow-retired-setting-names*))
         (throw (ex-info (tru "Setting name ''{0}'' is retired; use a different name instead" (name setting-name))
                         {:retired-setting-name (name setting-name)
                          :new-setting          (dissoc <> :on-change :getter :setter)})))

test/metabase/db/data_migrations_test.clj

@@ -4,11 +4,12 @@
   (:require [cheshire.core :as json]
             [clojure.test :refer :all]
             [metabase.db.data-migrations :as migrations]
-            [metabase.models.card :refer [Card]]
-            [metabase.models.dashboard :refer [Dashboard]]
-            [metabase.models.dashboard-card :refer [DashboardCard]]
+            [metabase.models :refer [Card Dashboard DashboardCard Setting]]
+            [metabase.models.permissions-group :as group]
+            [metabase.models.setting :as setting]
             [metabase.test :as mt]
             [metabase.test.fixtures :as fixtures]
+            [metabase.util :as u]
             [toucan.db :as db]))
 
 (use-fixtures :once (fixtures/initialize :db))
@@ -331,3 +332,63 @@
           (testing "And it is idempotent"
             (#'migrations/migrate-click-through)
             (is (= expected-settings (get-settings!)))))))))
+
+(defn- get-json-setting
+  [setting-k]
+  (json/parse-string (db/select-one-field :value Setting :key (name setting-k))))
+
+(defn- call-with-ldap-and-sso-configured [ldap-group-mappings sso-group-mappings f]
+  (mt/with-temporary-raw-setting-values
+    [ldap-group-mappings    (json/generate-string ldap-group-mappings)
+     saml-group-mappings    (json/generate-string sso-group-mappings)
+     jwt-group-mappings     (json/generate-string sso-group-mappings)
+     ldap-sync-admin-group  "false"
+     saml-enabled           "true"
+     ldap-enabled           "true"
+     jwt-enabled            "true"]
+    (f)))
+
+(defmacro ^:private with-ldap-and-sso-configured
+  "Run body with ldap and SSO configured, in which SSO will only be configured if enterprise is available"
+  [ldap-group-mappings sso-group-mappings & body]
+  (binding [setting/*allow-retired-setting-names* true]
+    `(call-with-ldap-and-sso-configured ~ldap-group-mappings ~sso-group-mappings (fn [] ~@body))))
+
+;; The `remove-admin-from-group-mapping-if-needed` migration is written to run in OSS version
+;; even though it might make changes to some enterprise-only settings.
+;; In order to write tests that runs in both OSS and EE, we can't use
+;; [[metabase.models.setting/get]] and [[metabase.test.util/with-temporary-setting-values]]
+;; because they require all settings are defined.
+;; That's why we use a set of helper functions that get setting directly from DB during tests
+(deftest migrate-remove-admin-from-group-mapping-if-needed-test
+  (let [admin-group-id        (u/the-id (group/admin))
+        sso-group-mappings    {"group-mapping-a" [admin-group-id (+ 1 admin-group-id)]
+                               "group-mapping-b" [admin-group-id (+ 1 admin-group-id) (+ 2 admin-group-id)]}
+        ldap-group-mappings   {"dc=metabase,dc=com" [admin-group-id (+ 1 admin-group-id)]}
+        sso-expected-mapping  {"group-mapping-a" [(+ 1 admin-group-id)]
+                               "group-mapping-b" [(+ 1 admin-group-id) (+ 2 admin-group-id)]}
+        ldap-expected-mapping {"dc=metabase,dc=com" [(+ 1 admin-group-id)]}]
+    (testing "Remove admin from group mapping for LDAP, SAML, JWT if they are enabled"
+      (with-ldap-and-sso-configured ldap-group-mappings sso-group-mappings
+        (#'migrations/migrate-remove-admin-from-group-mapping-if-needed)
+        (is (= ldap-expected-mapping (get-json-setting :ldap-group-mappings)))
+        (is (= sso-expected-mapping (get-json-setting :jwt-group-mappings)))
+        (is (= sso-expected-mapping (get-json-setting :saml-group-mappings)))))
+
+    (testing "remove admin from group mapping for LDAP, SAML, JWT even if they are disabled"
+      (with-ldap-and-sso-configured ldap-group-mappings sso-group-mappings
+        (mt/with-temporary-raw-setting-values
+          [ldap-enabled "false"
+           saml-enabled "false"
+           jwt-enabled  "false"]
+          (#'migrations/migrate-remove-admin-from-group-mapping-if-needed)
+          (is (= ldap-expected-mapping (get-json-setting :ldap-group-mappings)))
+          (is (= sso-expected-mapping (get-json-setting :jwt-group-mappings)))
+          (is (= sso-expected-mapping (get-json-setting :saml-group-mappings))))))
+
+    (testing "Don't remove admin group if `ldap-sync-admin-group` is enabled"
+      (with-ldap-and-sso-configured ldap-group-mappings sso-group-mappings
+        (mt/with-temporary-raw-setting-values
+          [ldap-sync-admin-group "true"]
+          (#'migrations/migrate-remove-admin-from-group-mapping-if-needed)
+          (is (= ldap-group-mappings (get-json-setting :ldap-group-mappings))))))))

test/metabase/integrations/common_test.clj

@@ -5,7 +5,6 @@
             [metabase.models.permissions-group-membership :refer [PermissionsGroupMembership]]
             [metabase.test :as mt :refer [with-user-in-groups]]
             [metabase.test.fixtures :as fixtures]
-            [metabase.test.util.log :as tu.log]
             [metabase.util :as u]
             [toucan.db :as db]))
 
@@ -21,7 +20,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} #{group} false)
+      (integrations.common/sync-group-memberships! user #{group} #{group})
       (is (= #{"All Users" ":metabase.integrations.common-test/group"}
              (group-memberships user)))))
 
@@ -32,7 +31,7 @@
                                                       :group_id (u/the-id group)
                                                       :user_id  (u/the-id user))
             original-membership-id (membership-id)]
-        (integrations.common/sync-group-memberships! user #{group} #{group} false)
+        (integrations.common/sync-group-memberships! user #{group} #{group})
         (is (= original-membership-id
                (membership-id))))))
 
@@ -40,7 +39,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} #{group-1 group-2} false)
+      (integrations.common/sync-group-memberships! user #{group-1 group-2} #{group-1 group-2})
       (is (= #{":metabase.integrations.common-test/group-1"
                ":metabase.integrations.common-test/group-2"
                "All Users"}
@@ -50,7 +49,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})
       (is (= #{"All Users"}
              (group-memberships user)))))
 
@@ -58,20 +57,20 @@
     (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-1 group-2} false)
+      (integrations.common/sync-group-memberships! user #{group-2} #{group-1 group-2})
       (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)
+      (integrations.common/sync-group-memberships! user #{group-1} #{})
       (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)
+      (integrations.common/sync-group-memberships! user #{} #{})
       (is (= #{":metabase.integrations.common-test/group-1"
                "All Users"}
              (group-memberships user)))))
@@ -79,34 +78,26 @@
   (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    []]
-      (tu.log/suppress-output
-       (integrations.common/sync-group-memberships! user #{Integer/MAX_VALUE group} #{Integer/MAX_VALUE group} false))
+      (integrations.common/sync-group-memberships! user #{Integer/MAX_VALUE group} #{Integer/MAX_VALUE group})
       (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?"
+    (testing "Admin should be synced just like a other groups group."
+      (testing "remove admin role if users are not mapped to it"
         (with-user-in-groups [user [(group/admin)]]
-          (integrations.common/sync-group-memberships! user [] #{} true)
+          (integrations.common/sync-group-memberships! user #{} #{(group/admin)})
           (is (= #{"All Users"}
-                 (group-memberships user)))))))
+                 (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 "but it's ignored if admin is not in mappings"
+        (with-user-in-groups [user [(group/admin)]]
+          (integrations.common/sync-group-memberships! user #{} #{})
+          (is (= #{"All Users" "Administrators"}
+                 (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)))))))
+      (testing "add admin role if the users are mapped to it"
+        (with-user-in-groups [user []]
+          (integrations.common/sync-group-memberships! user #{(group/admin)} #{(group/admin)})
+          (is (= #{"All Users" "Administrators"}
+                 (group-memberships user))))))))

test/metabase/test.clj

@@ -211,6 +211,7 @@
   with-temp-scheduler
   with-temp-vals-in-db
   with-temporary-setting-values
+  with-temporary-raw-setting-values
   with-user-in-groups]
 
  [tu.async

test/metabase/test/util.clj

@@ -13,7 +13,7 @@
             [metabase.driver :as driver]
             [metabase.models :refer [Card Collection Dashboard DashboardCardSeries Database Dimension Field FieldValues
                                      LoginHistory Metric NativeQuerySnippet Permissions PermissionsGroup PermissionsGroupMembership
-                                     Pulse PulseCard PulseChannel Revision Segment Table TaskHistory Timeline TimelineEvent User]]
+                                     Pulse PulseCard PulseChannel Revision Segment Setting Table TaskHistory Timeline TimelineEvent User]]
             [metabase.models.collection :as collection]
             [metabase.models.permissions :as perms]
             [metabase.models.permissions-group :as group]
@@ -342,41 +342,65 @@
       (list `with-temp-env-var-value '[a])
       (list `with-temp-env-var-value '[a b c]))))
 
+(defn- upsert-raw-setting!
+  [original-value setting-k value]
+  (if original-value
+    (db/update! Setting setting-k :value value)
+    (db/insert! Setting :key setting-k :value value))
+  (setting.cache/restore-cache!))
+
+(defn- restore-raw-setting!
+  [original-value setting-k]
+  (if original-value
+    (db/update! Setting setting-k :value original-value)
+    (db/delete! Setting :key setting-k))
+  (setting.cache/restore-cache!))
+
 (defn do-with-temporary-setting-value
   "Temporarily set the value of the Setting named by keyword `setting-k` to `value` and execute `f`, then re-establish
   the original value. This works much the same way as [[binding]].
 
   If an env var value is set for the setting, this acts as a wrapper around [[do-with-temp-env-var-value]].
 
-  Prefer the macro [[with-temporary-setting-values]] over using this function directly."
-  [setting-k value thunk]
-  (test-runner.parallel/assert-test-is-not-parallel "with-temporary-setting-values")
+  If `raw-setting?` is `true`, this works like [[with-temp*]] against the `Setting` table, but it ensures no exception
+  is thrown if the `setting-k` already exists.
+
+  Prefer the macro [[with-temporary-setting-values]] or [[with-temporary-raw-setting-values]] over using this function directly."
+  [setting-k value thunk & {:keys [raw-setting?]}]
   ;; plugins have to be initialized because changing `report-timezone` will call driver methods
   (initialize/initialize-if-needed! :db :plugins)
-  (let [setting       (#'setting/resolve-setting setting-k)
-        env-var-value (#'setting/env-var-value setting)]
-    (if env-var-value
+  (let [setting-k     (name setting-k)
+        setting       (try
+                       (#'setting/resolve-setting setting-k)
+                       (catch Exception e
+                         (when-not raw-setting?
+                           (throw e))))]
+    (if-let [env-var-value (and (not raw-setting?) (#'setting/env-var-value setting-k))]
       (do-with-temp-env-var-value setting env-var-value thunk)
-      (let [original-value (setting/get setting-k)]
+      (let [original-value (db/select-one-field :value Setting :key setting-k)]
         (try
-          (setting/set! setting-k value)
-          (testing (colorize/blue (format "\nSetting %s = %s\n" (keyword setting-k) (pr-str value)))
-            (thunk))
-          (catch Throwable e
-            (throw (ex-info (str "Error in with-temporary-setting-values: " (ex-message e))
-                            {:setting  setting-k
-                             :location (symbol (name (:namespace setting)) (name setting-k))
-                             :value    value}
-                            e)))
-          (finally
-            (try
-              (setting/set! setting-k original-value)
-              (catch Throwable e
-                (throw (ex-info (str "Error restoring original Setting value: " (ex-message e))
-                                {:setting        setting-k
-                                 :location       (symbol (name (:namespace setting)) (name setting-k))
-                                 :original-value original-value}
-                                e))))))))))
+         (if raw-setting?
+           (upsert-raw-setting! original-value setting-k value)
+           (setting/set! setting-k value))
+         (testing (colorize/blue (format "\nSetting %s = %s\n" (keyword setting-k) (pr-str value)))
+           (thunk))
+         (catch Throwable e
+           (throw (ex-info (str "Error in with-temporary-setting-values: " (ex-message e))
+                           {:setting  setting-k
+                            :location (symbol (name (:namespace setting)) (name setting-k))
+                            :value    value}
+                           e)))
+         (finally
+          (try
+           (if raw-setting?
+             (restore-raw-setting! original-value setting-k)
+             (setting/set! setting-k original-value))
+           (catch Throwable e
+             (throw (ex-info (str "Error restoring original Setting value: " (ex-message e))
+                             {:setting        setting-k
+                              :location       (symbol (name (:namespace setting)) setting-k)
+                              :original-value original-value}
+                             e))))))))))
 
 (defmacro with-temporary-setting-values
   "Temporarily bind the site-wide values of one or more `Settings`, execute body, and re-establish the original values.
@@ -389,6 +413,7 @@
   To temporarily override the value of *read-only* env vars, use [[with-temp-env-var-value]]."
   [[setting-k value & more :as bindings] & body]
   (assert (even? (count bindings)) "mismatched setting/value pairs: is each setting name followed by a value?")
+  (test-runner.parallel/assert-test-is-not-parallel "with-temporary-setting-vales")
   (if (empty? bindings)
     `(do ~@body)
     `(do-with-temporary-setting-value ~(keyword setting-k) ~value
@@ -396,6 +421,19 @@
          (with-temporary-setting-values ~more
            ~@body)))))
 
+(defmacro with-temporary-raw-setting-values
+  "Like `with-temporary-setting-values` but works with raw value and it allows settings that are not defined using `defsetting`."
+  [[setting-k value & more :as bindings] & body]
+  (assert (even? (count bindings)) "mismatched setting/value pairs: is each setting name followed by a value?")
+  (test-runner.parallel/assert-test-is-not-parallel "with-temporary-raw-setting-values")
+  (if (empty? bindings)
+    `(do ~@body)
+    `(do-with-temporary-setting-value ~(keyword setting-k) ~value
+       (fn []
+         (with-temporary-raw-setting-values ~more
+           ~@body))
+       :raw-setting? true)))
+
 (defn do-with-discarded-setting-changes [settings thunk]
   (initialize/initialize-if-needed! :db :plugins)
   ((reduce

test_resources/ldap.ldif

@@ -40,6 +40,7 @@ objectClass: person
 objectClass: organizationalPerson
 objectClass: inetOrgPerson
 cn: Fred Taylor
+# intetionally having some attributes not properly cased to test case-insensitive when querying users
 sN: Taylor
 givenname: Fred
 uid: ftaylor300

test_resources/sso/auth0-public-idp.cert

+MIIDEzCCAfugAwIBAgIJYpjQiNMYxf1GMA0GCSqGSIb3DQEBCwUAMCcxJTAjBgNV
+BAMTHHNhbWwtbWV0YWJhc2UtdGVzdC5hdXRoMC5jb20wHhcNMTgwNTI5MjEwMDIz
+WhcNMzIwMjA1MjEwMDIzWjAnMSUwIwYDVQQDExxzYW1sLW1ldGFiYXNlLXRlc3Qu
+YXV0aDAuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAzNcrpju4
+sILZQNe1adwg3beXtAMFGB+Buuc414+FDv2OG7X7b9OSYar/nsYfWwiazZRxEGri
+agd0Sj5mJ4Qqx+zmB/r4UgX3q/KgocRLlShvvz5gTD99hR7LonDPSWET1E9PD4XE
+1fRaq+BwftFBl45pKTcCR9QrUAFZJ2R/3g06NPZdhe4bg/lTssY5emCxaZpQEku/
+v+zzpV2nLF4by0vSj7AHsubrsLgsCfV3JvJyTxCyo1aIOlv4Vrx7h9rOgl9eEmoU
+5XJAl3D7DuvSTEOy7MyDnKF17m7l5nOPZCVOSzmCWvxSCyysijgsM5DSgAE8DPJy
+oYezV3gTX2OO2QIDAQABo0IwQDAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSp
+B3lvrtbSDuXkB6fhbjeUpFmL2DAOBgNVHQ8BAf8EBAMCAoQwDQYJKoZIhvcNAQEL
+BQADggEBAAEHGIAhR5GPD2JxgLtpNtZMCYiAM4Gr7hoTQMaKiXgVtdQu4iMFfbpE
+wIr6UVaDU2HKhvSRFIilOjRGmCGrIzvJgR2l+RL1Z3KrZypI1AXKJT5pF5g5FitB
+sZq+kiUpdRILl2hICzw9Q1M2Le+JSUcHcbHTVgF24xuzOZonxeE56Oc26Ju4CorL
+pM3Nb5iYaGOlQ+48/GP82cLxlVyi02va8tp7KP03ePSaZeBEKGpFtBtEN/dC3NKO
+1mmrT9284H0tvete6KLUH+dsS6bDEYGHZM5KGoSLWRr3qYlCB3AmAw+KvuiuSczL
+g9oYBkdxlhK9zZvkjCgaLCen+0aY67A="