From 0708ce0a0f7ce75d3007504fdb40969a8f5e24a2 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Thu, 7 Apr 2022 15:13:43 +0700
Subject: [PATCH] Fix SSO failed to sync admin group (#20991)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* 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
---
.../enhancements/integrations/ldap.clj | 8 +-
.../sso/integrations/jwt.clj | 5 +-
.../sso/integrations/saml.clj | 3 +-
.../sso/integrations/saml_test.clj | 21 +----
src/metabase/api/session.clj | 2 +-
src/metabase/db/data_migrations.clj | 55 ++++++++++--
src/metabase/integrations/common.clj | 16 ++--
.../ldap/default_implementation.clj | 29 +++---
src/metabase/models/permissions.clj | 10 +--
src/metabase/models/setting.clj | 10 ++-
test/metabase/db/data_migrations_test.clj | 67 +++++++++++++-
test/metabase/integrations/common_test.clj | 53 +++++------
test/metabase/test.clj | 1 +
test/metabase/test/util.clj | 90 +++++++++++++------
test_resources/ldap.ldif | 1 +
test_resources/sso/auth0-public-idp.cert | 17 ++++
16 files changed, 256 insertions(+), 132 deletions(-)
create mode 100644 test_resources/sso/auth0-public-idp.cert
diff --git a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
index 5b79cd6f45c..11ef9a67d09 100644
--- a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj
+++ b/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
diff --git a/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj b/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
index fc946091558..198c1391306 100644
--- a/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
+++ b/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}]
diff --git a/enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj b/enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj
index 89d22dc27f5..83a75f3585d 100644
--- a/enterprise/backend/src/metabase_enterprise/sso/integrations/saml.clj
+++ b/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."
diff --git a/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj b/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj
index b03efd7aef9..fee0665aa0d 100644
--- a/enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj
+++ b/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
diff --git a/src/metabase/api/session.clj b/src/metabase/api/session.clj
index f2ead741ae8..7b62de0dabc 100644
--- a/src/metabase/api/session.clj
+++ b/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}})))))
diff --git a/src/metabase/db/data_migrations.clj b/src/metabase/db/data_migrations.clj
index 4c0943a21ec..014d448adb4 100644
--- a/src/metabase/db/data_migrations.clj
+++ b/src/metabase/db/data_migrations.clj
@@ -1,9 +1,10 @@
(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 !!
diff --git a/src/metabase/integrations/common.clj b/src/metabase/integrations/common.clj
index 13e65970ffc..71611f70414 100644
--- a/src/metabase/integrations/common.clj
+++ b/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
diff --git a/src/metabase/integrations/ldap/default_implementation.clj b/src/metabase/integrations/ldap/default_implementation.clj
index 99be624a0c6..9636d147d71 100644
--- a/src/metabase/integrations/ldap/default_implementation.clj
+++ b/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 ------------------------------------------------------
diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index d67a2e8a52b..b06f6881cae 100644
--- a/src/metabase/models/permissions.clj
+++ b/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
diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index dbb83d177c7..33e344d64da 100644
--- a/src/metabase/models/setting.clj
+++ b/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)})))
diff --git a/test/metabase/db/data_migrations_test.clj b/test/metabase/db/data_migrations_test.clj
index f3030070e0d..5a7534fc291 100644
--- a/test/metabase/db/data_migrations_test.clj
+++ b/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))))))))
diff --git a/test/metabase/integrations/common_test.clj b/test/metabase/integrations/common_test.clj
index 060c4d41198..5ab97c34d0d 100644
--- a/test/metabase/integrations/common_test.clj
+++ b/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))))))))
diff --git a/test/metabase/test.clj b/test/metabase/test.clj
index 5ad9f9afb4e..c4f87a72c39 100644
--- a/test/metabase/test.clj
+++ b/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
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index aa04e55d9f1..3a710e9870a 100644
--- a/test/metabase/test/util.clj
+++ b/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
diff --git a/test_resources/ldap.ldif b/test_resources/ldap.ldif
index 7f176ad995e..843c847be0a 100644
--- a/test_resources/ldap.ldif
+++ b/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
diff --git a/test_resources/sso/auth0-public-idp.cert b/test_resources/sso/auth0-public-idp.cert
new file mode 100644
index 00000000000..67f541db5a8
--- /dev/null
+++ b/test_resources/sso/auth0-public-idp.cert
@@ -0,0 +1,17 @@
+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="
--
GitLab