diff --git a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj index dcee6aa55754453702a89b7c5f0e35c939ae62c2..9775695ab43e2e03b40f6db4cba3aad1e4e93f36 100644 --- a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj +++ b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj @@ -62,7 +62,7 @@ (defenterprise-schema find-user :- (s/maybe EEUserInfo) "Get user information for the supplied username." - :feature :any + :feature :sso [ldap-connection :- LDAPConnectionPool username :- su/NonBlankString settings :- i/LDAPSettings] @@ -76,7 +76,7 @@ (defenterprise-schema fetch-or-create-user! :- (mi/InstanceOf User) "Using the `user-info` (from `find-user`) get the corresponding Metabase user, creating it if necessary." - :feature :any + :feature :sso [{:keys [first-name last-name email groups attributes], :as user-info} :- EEUserInfo {:keys [sync-groups?], :as settings} :- i/LDAPSettings] (let [user (or (attribute-synced-user user-info) diff --git a/enterprise/backend/src/metabase_enterprise/enhancements/models/native_query_snippet/permissions.clj b/enterprise/backend/src/metabase_enterprise/enhancements/models/native_query_snippet/permissions.clj index ed49995236ee82079ad872305ed0903570a117f9..5f3c751650dccf7d0d7ed4b330ac817e3b4699d5 100644 --- a/enterprise/backend/src/metabase_enterprise/enhancements/models/native_query_snippet/permissions.clj +++ b/enterprise/backend/src/metabase_enterprise/enhancements/models/native_query_snippet/permissions.clj @@ -18,7 +18,7 @@ (defenterprise can-read? "Can the current User read this `snippet`?" - :feature :any + :feature :content-management ([snippet] (and (not (premium-features/sandboxed-user?)) @@ -29,7 +29,7 @@ (defenterprise can-write? "Can the current User edit this `snippet`?" - :feature :any + :feature :content-management ([snippet] (and (not (premium-features/sandboxed-user?)) @@ -40,7 +40,7 @@ (defenterprise can-create? "Can the current User save a new Snippet with the values in `m`?" - :feature :any + :feature :content-management [_model m] (and (not (premium-features/sandboxed-user?)) @@ -49,7 +49,7 @@ (defenterprise can-update? "Can the current User apply a map of `changes` to a `snippet`?" - :feature :any + :feature :content-management [snippet changes] (and (not (premium-features/sandboxed-user?)) diff --git a/enterprise/backend/src/metabase_enterprise/pulse.clj b/enterprise/backend/src/metabase_enterprise/pulse.clj index f772eae99d353884b0cdccb4005a93fa8f3f61b1..a71ce3223cff82ecef085b55d5a338fd6e447f74 100644 --- a/enterprise/backend/src/metabase_enterprise/pulse.clj +++ b/enterprise/backend/src/metabase_enterprise/pulse.clj @@ -7,7 +7,7 @@ (defenterprise the-parameters "Enterprise way of getting dashboard filter parameters. Blends parameters from dashboard subscription and the dashboard itself." - :feature :any + :feature :advanced-config [pulse dashboard] (let [pulse-params (:parameters pulse) dashboard-params (:parameters dashboard) diff --git a/enterprise/backend/src/metabase_enterprise/search/scoring.clj b/enterprise/backend/src/metabase_enterprise/search/scoring.clj index 038f98af5f7b999dfeb63bd84549448316bcfe66..6c13920d21857167fa3b90ea0164909db0f84757 100644 --- a/enterprise/backend/src/metabase_enterprise/search/scoring.clj +++ b/enterprise/backend/src/metabase_enterprise/search/scoring.clj @@ -20,7 +20,7 @@ (defenterprise score-result "Scoring implementation that adds score for items in official collections." - :feature :any + :feature :content-management [result] (conj (scoring/weights-and-scores result) {:weight 2 diff --git a/enterprise/backend/test/metabase_enterprise/enhancements/api/native_query_snippet_test.clj b/enterprise/backend/test/metabase_enterprise/enhancements/api/native_query_snippet_test.clj index 7f34dc9878bc03ae2e0a0fb8379af64c1059f75a..cb17cf630fdccb918dfce9884ea448a627f39b66 100644 --- a/enterprise/backend/test/metabase_enterprise/enhancements/api/native_query_snippet_test.clj +++ b/enterprise/backend/test/metabase_enterprise/enhancements/api/native_query_snippet_test.clj @@ -29,7 +29,7 @@ (has-perms? snippet)) "allowed?"))) (testing "\nWith EE features enabled" - (premium-features-test/with-premium-features #{:enhancements} + (premium-features-test/with-premium-features #{:content-management} (testing (format "\nShould not be allowed with no perms for %s" collection-name) (is (= false (has-perms? snippet)) @@ -118,7 +118,7 @@ (premium-features-test/with-premium-features #{} (is (= true (has-perms?))))) - (premium-features-test/with-premium-features #{:enhancements} + (premium-features-test/with-premium-features #{:content-management} (doseq [c [source-collection dest-collection]] (testing (format "\nPerms for only %s should fail" (:name c)) (try diff --git a/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj b/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj index d49603fd86cd53c81aef4bb37f00581c3996e4d6..93e69d659bbe46645f6d89e5e7bf3d592fe08cdb 100644 --- a/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj +++ b/enterprise/backend/test/metabase_enterprise/enhancements/integrations/ldap_test.clj @@ -4,7 +4,8 @@ [metabase-enterprise.enhancements.integrations.ldap :as ldap-ee] [metabase.integrations.ldap :as ldap] [metabase.models.user :as user :refer [User]] - [metabase.public-settings.premium-features :as premium-features] + [metabase.public-settings.premium-features-test + :as premium-features-test] [metabase.test :as mt] [metabase.test.integrations.ldap :as ldap.test] [metabase.util.schema :as su] @@ -12,7 +13,7 @@ [toucan2.core :as t2])) (deftest find-test - (with-redefs [#_{:clj-kondo/ignore [:deprecated-var]} premium-features/enable-enhancements? (constantly true)] + (premium-features-test/with-premium-features #{:sso} (ldap.test/with-ldap-server (testing "find by username" (is (= {:dn "cn=John Smith,ou=People,dc=metabase,dc=com" @@ -92,7 +93,7 @@ (ldap/find-user "sally.brown@metabase.com")))))))) (deftest attribute-sync-test - (with-redefs [#_{:clj-kondo/ignore [:deprecated-var]} premium-features/enable-enhancements? (constantly true)] + (premium-features-test/with-premium-features #{:sso} (ldap.test/with-ldap-server (testing "find by email/username should return other attributes as well" (is (= {:dn "cn=Lucky Pigeon,ou=Birds,dc=metabase,dc=com" @@ -165,7 +166,7 @@ (t2/delete! User :%lower.email "john.smith@metabase.com")))))))) (deftest update-attributes-on-login-test - (with-redefs [#_{:clj-kondo/ignore [:deprecated-var]} premium-features/enable-enhancements? (constantly true)] + (premium-features-test/with-premium-features #{:sso} (ldap.test/with-ldap-server (testing "Existing user's attributes are updated on fetch" (try @@ -214,7 +215,7 @@ (t2/delete! User :%lower.email "john.smith@metabase.com"))))))) (deftest fetch-or-create-user-test - (with-redefs [#_{:clj-kondo/ignore [:deprecated-var]} premium-features/enable-enhancements? (constantly true)] + (premium-features-test/with-premium-features #{:sso} (ldap.test/with-ldap-server (testing "a new user is created when they don't already exist" (try diff --git a/enterprise/backend/test/metabase_enterprise/enhancements/models/native_query_snippet/permissions_test.clj b/enterprise/backend/test/metabase_enterprise/enhancements/models/native_query_snippet/permissions_test.clj index 86b11cbab9bdb8db0674d351d8c4168de35a4cbd..e6ff5c33eeba1db1c7ff82ad7e46437b9675d653 100644 --- a/enterprise/backend/test/metabase_enterprise/enhancements/models/native_query_snippet/permissions_test.clj +++ b/enterprise/backend/test/metabase_enterprise/enhancements/models/native_query_snippet/permissions_test.clj @@ -36,7 +36,7 @@ (test-perms* true))))) (testing "if EE perms are enabled: " - (premium-features-test/with-premium-features #{:enhancements} + (premium-features-test/with-premium-features #{:content-management} (with-redefs [snippet.perms/has-any-native-permissions? (constantly true)] (testing "should be allowed if you have collection perms, native perms for at least one DB, and are not sandboxed" (grant-collection-perms!) diff --git a/enterprise/backend/test/metabase_enterprise/pulse_test.clj b/enterprise/backend/test/metabase_enterprise/pulse_test.clj index 9231a26bc0b918e0a9d78e1ebe323051b881a9d2..a43772801e7a78a79e97759f72bcf12e894c6c4e 100644 --- a/enterprise/backend/test/metabase_enterprise/pulse_test.clj +++ b/enterprise/backend/test/metabase_enterprise/pulse_test.clj @@ -2,13 +2,15 @@ (:require [clojure.test :refer :all] [metabase-enterprise.pulse :as pulse] - [metabase.public-settings.premium-features :as premium-features])) + [metabase.public-settings.premium-features-test + :as + premium-features-test])) (deftest parameters-test (is (= [{:id "1" :v "a"} {:id "2" :v "b"} {:id "3" :v "yes"}] - (with-redefs [#_{:clj-kondo/ignore [:deprecated-var]} premium-features/enable-enhancements? (constantly true)] - (pulse/the-parameters - {:parameters [{:id "1" :v "a"} {:id "2" :v "b"}]} - {:parameters [{:id "1" :v "no, since it's trumped by the pulse"} {:id "3" :v "yes"}]}))))) + (premium-features-test/with-premium-features #{:advanced-config} + (pulse/the-parameters + {:parameters [{:id "1" :v "a"} {:id "2" :v "b"}]} + {:parameters [{:id "1" :v "no, since it's trumped by the pulse"} {:id "3" :v "yes"}]}))))) diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj b/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj index c12eebcdc8e40fa8a25d0135c36563d16ac5713a..812258d2e6a87e39c39c34b741827b6f2fbfeff3 100644 --- a/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj +++ b/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj @@ -70,7 +70,7 @@ (let [{:keys [gtaps attributes]} (s/validate WithGTAPsArgs (args-fn))] ;; set user login_attributes (with-user-attributes test-user-name-or-user-id attributes - (premium-features-test/with-premium-features #{:sandboxes} + (premium-features-test/with-additional-premium-features #{:sandboxes} ;; create Cards/GTAPs from defs (do-with-gtap-defs group gtaps (fn [] diff --git a/enterprise/backend/test/metabase_enterprise/search/scoring_test.clj b/enterprise/backend/test/metabase_enterprise/search/scoring_test.clj index fa2f77ab33411e7013c0e73da3c1400c7a2de04a..549a4f01c18bc061c4c7543e36566b29c6c41570 100644 --- a/enterprise/backend/test/metabase_enterprise/search/scoring_test.clj +++ b/enterprise/backend/test/metabase_enterprise/search/scoring_test.clj @@ -6,7 +6,8 @@ [clojure.test :refer :all] [java-time :as t] [metabase-enterprise.search.scoring :as ee-scoring] - [metabase.public-settings.premium-features :as premium-features] + [metabase.public-settings.premium-features-test + :as premium-features-test] [metabase.search.scoring :as scoring])) (deftest ^:parallel verified-score-test @@ -23,14 +24,12 @@ (defn- ee-score [search-string item] - (with-redefs [#_{:clj-kondo/ignore [:deprecated-var]} - premium-features/enable-enhancements? (constantly true)] + (premium-features-test/with-premium-features #{:content-management} (-> (scoring/score-and-result search-string item) :score))) (defn- oss-score [search-string item] - (with-redefs [#_{:clj-kondo/ignore [:deprecated-var]} - premium-features/enable-enhancements? (constantly false)] + (premium-features-test/with-premium-features #{} (-> (scoring/score-and-result search-string item) :score))) (deftest official-collection-tests @@ -49,7 +48,10 @@ (doseq [item [a b c d]] (is (> (ee-score search-string (assoc item :collection_authority_level "official")) (ee-score search-string item)) - (str "Score should be greater for item: " item " vs " (assoc item :collection_authority_level "official")))) + (str "On EE, should be greater for item: " item " vs " (assoc item :collection_authority_level "official"))) + (is (= (oss-score search-string (assoc item :collection_authority_level "official")) + (oss-score search-string item)) + (str "On OSS, should be the same for item: " item " vs " (assoc item :collection_authority_level "official")))) (is (= ["customer examples of bad sorting" "customer success stories" "examples of custom expressions" diff --git a/enterprise/backend/test/metabase_enterprise/util_test.clj b/enterprise/backend/test/metabase_enterprise/util_test.clj index edc6423b3752400d74192f4aef9e6c659ab7a3fb..88736371fe14a28c5d9c83c209ad9c6fa056bde2 100644 --- a/enterprise/backend/test/metabase_enterprise/util_test.clj +++ b/enterprise/backend/test/metabase_enterprise/util_test.clj @@ -16,13 +16,6 @@ [username] (format "Hi %s, you're running the Enterprise Edition of Metabase!" (name username))) -(defenterprise greeting-with-valid-token - "Returns an extra special greeting for a user if the instance has a valid premium features token. Else, returns the - default (OSS) greeting." - :feature :any - [username] - (format "Hi %s, you're an EE customer with a valid token!" (name username))) - (defenterprise special-greeting "Returns an extra special greeting for a user if the instance has a :special-greeting feature token. Else, returns the default (OSS) greeting." diff --git a/src/metabase/public_settings/premium_features.clj b/src/metabase/public_settings/premium_features.clj index 5251008dbc62c9b7ca230cc3d9db1e271a262847..0283b2d7bc2e56cd180fdcf1d0af9b8915769b23 100644 --- a/src/metabase/public_settings/premium_features.clj +++ b/src/metabase/public_settings/premium_features.clj @@ -221,7 +221,7 @@ (log/debug e (trs "Error validating token"))) ;; log every five minutes :ttl/threshold (* 1000 60 5))] - (schema/defn ^:private token-features :- #{su/NonBlankString} + (schema/defn token-features :- #{su/NonBlankString} "Get the features associated with the system's premium features token." [] (try @@ -363,10 +363,7 @@ (defn- check-feature [feature] (or (= feature :none) - (if (= feature :any) - #_{:clj-kondo/ignore [:deprecated-var]} - (enable-enhancements?) - (has-feature? feature)))) + (has-feature? feature))) (defn dynamic-ee-oss-fn "Dynamically tries to require an enterprise namespace and determine the correct implementation to call, based on the @@ -473,9 +470,9 @@ ###### `:feature` - A keyword representing a premium feature which must be present for the EE implementation to be used. Use `:any` to - require a valid premium token with at least one feature, but no specific feature. Use `:none` to always run the - EE implementation if available, regardless of token. + A keyword representing a premium feature which must be present for the EE implementation to be used. Use `:none` to + always run the EE implementation if available, regardless of token (WARNING: this is not recommended for most use + cases. You probably want to gate your code by a specific premium feature.) ###### `:fallback` diff --git a/src/metabase/search/scoring.clj b/src/metabase/search/scoring.clj index 366ece3b4e61628cb8f5c82aa147b0fe4e50e62d..6dfac99eface528cbe55b259038c44c2d67f8c4a 100644 --- a/src/metabase/search/scoring.clj +++ b/src/metabase/search/scoring.clj @@ -223,14 +223,14 @@ (defenterprise score-result "Score a result, returning a collection of maps with score and weight. Should not include the text scoring, done - separately. Should return a sequence of maps with + separately. Should return a sequence of maps with {:weight number, :score number, :name string}" - metabase-enterprise.search.scoring - [result] - (weights-and-scores result)) + metabase-enterprise.search.scoring + [result] + (weights-and-scores result)) (defn- sum-weights [weights] (reduce diff --git a/test/metabase/integrations/ldap_test.clj b/test/metabase/integrations/ldap_test.clj index 0841c8af51fa593f65434a7f64733d325eb88e0f..28f7640dc228e9b632b72c9fcf7f32dc08f58ebd 100644 --- a/test/metabase/integrations/ldap_test.clj +++ b/test/metabase/integrations/ldap_test.clj @@ -4,7 +4,8 @@ [metabase.integrations.ldap :as ldap] [metabase.integrations.ldap.default-implementation :as default-impl] [metabase.models.user :as user :refer [User]] - [metabase.public-settings.premium-features :as premium-features] + [metabase.public-settings.premium-features-test + :as premium-features-test] [metabase.test :as mt] [metabase.test.integrations.ldap :as ldap.test] [toucan2.core :as t2]) @@ -70,7 +71,7 @@ (deftest find-test ;; there are EE-specific versions of this test in [[metabase-enterprise.enhancements.integrations.ldap-test]] - (with-redefs [#_{:clj-kondo/ignore [:deprecated-var]} premium-features/enable-enhancements? (constantly false)] + (premium-features-test/with-premium-features #{} (ldap.test/with-ldap-server (testing "find by username" (is (= {:dn "cn=John Smith,ou=People,dc=metabase,dc=com" @@ -125,40 +126,40 @@ (deftest fetch-or-create-user-test ;; there are EE-specific versions of this test in [[metabase-enterprise.enhancements.integrations.ldap-test]] - (with-redefs [#_{:clj-kondo/ignore [:deprecated-var]} premium-features/enable-enhancements? (constantly false)] + (premium-features-test/with-premium-features #{} (ldap.test/with-ldap-server (testing "a new user is created when they don't already exist" (try - (ldap/fetch-or-create-user! (ldap/find-user "jsmith1")) - (is (= {:first_name "John" - :last_name "Smith" - :common_name "John Smith" - :email "john.smith@metabase.com"} - (into {} (t2/select-one [User :first_name :last_name :email] :email "john.smith@metabase.com")))) - (finally (t2/delete! User :email "john.smith@metabase.com")))) + (ldap/fetch-or-create-user! (ldap/find-user "jsmith1")) + (is (= {:first_name "John" + :last_name "Smith" + :common_name "John Smith" + :email "john.smith@metabase.com"} + (into {} (t2/select-one [User :first_name :last_name :email] :email "john.smith@metabase.com")))) + (finally (t2/delete! User :email "john.smith@metabase.com")))) (try - (testing "a user without a givenName attribute has `nil` for that attribute" - (ldap/fetch-or-create-user! (ldap/find-user "jmiller")) - (is (= {:first_name nil - :last_name "Miller" - :common_name "Miller"} - (into {} (t2/select-one [User :first_name :last_name] :email "jane.miller@metabase.com"))))) - - (testing "when givenName or sn attributes change in LDAP, they are updated in Metabase on next login" - (ldap/fetch-or-create-user! (assoc (ldap/find-user "jmiller") :first-name "Jane" :last-name "Doe")) - (is (= {:first_name "Jane" - :last_name "Doe" - :common_name "Jane Doe"} - (into {} (t2/select-one [User :first_name :last_name] :email "jane.miller@metabase.com"))))) - - (testing "if givenName or sn attributes are removed, values stored in Metabase are updated to `nil` to respect the IdP response." - (ldap/fetch-or-create-user! (assoc (ldap/find-user "jmiller") :first-name nil :last-name nil)) - (is (= {:first_name nil - :last_name nil - :common_name "jane.miller@metabase.com"} - (select-keys (t2/select-one User :email "jane.miller@metabase.com") [:first_name :last_name :common_name])))) - (finally (t2/delete! User :email "jane.miller@metabase.com")))))) + (testing "a user without a givenName attribute has `nil` for that attribute" + (ldap/fetch-or-create-user! (ldap/find-user "jmiller")) + (is (= {:first_name nil + :last_name "Miller" + :common_name "Miller"} + (into {} (t2/select-one [User :first_name :last_name] :email "jane.miller@metabase.com"))))) + + (testing "when givenName or sn attributes change in LDAP, they are updated in Metabase on next login" + (ldap/fetch-or-create-user! (assoc (ldap/find-user "jmiller") :first-name "Jane" :last-name "Doe")) + (is (= {:first_name "Jane" + :last_name "Doe" + :common_name "Jane Doe"} + (into {} (t2/select-one [User :first_name :last_name] :email "jane.miller@metabase.com"))))) + + (testing "if givenName or sn attributes are removed, values stored in Metabase are updated to `nil` to respect the IdP response." + (ldap/fetch-or-create-user! (assoc (ldap/find-user "jmiller") :first-name nil :last-name nil)) + (is (= {:first_name nil + :last_name nil + :common_name "jane.miller@metabase.com"} + (select-keys (t2/select-one User :email "jane.miller@metabase.com") [:first_name :last_name :common_name])))) + (finally (t2/delete! User :email "jane.miller@metabase.com")))))) (deftest group-matching-test (testing "LDAP group matching should identify Metabase groups using DN equality rules" diff --git a/test/metabase/public_settings/premium_features_test.clj b/test/metabase/public_settings/premium_features_test.clj index 63ab9b13f8dc46870e47ef6e7748a1a149cf51bc..2852e32559725b58129b9bd48532378bf33a9412 100644 --- a/test/metabase/public_settings/premium_features_test.clj +++ b/test/metabase/public_settings/premium_features_test.clj @@ -3,6 +3,7 @@ [cheshire.core :as json] [clj-http.client :as http] [clj-http.fake :as http-fake] + [clojure.set :as set] [clojure.test :refer :all] [metabase.config :as config] [metabase.db.connection :as mdb.connection] @@ -35,6 +36,19 @@ [features & body] `(do-with-premium-features ~features (fn [] ~@body))) +(defmacro with-additional-premium-features + "Execute `body` with the allowed premium features for the Premium-Features token set to the union of `features` and + the current feature set. Intended for use testing feature-flagging, if you don't want to override other features + that are already enabled. + + (with-additional-premium-features #{:audit-app} + ;; audit app will be enabled for body, as well as any that are already enabled + ...)" + {:style/indent 1} + [features & body] + `(do-with-premium-features (set/union (premium-features/token-features) ~features) + (fn [] ~@body))) + (defn- token-status-response [token premium-features-response] (http-fake/with-fake-routes-in-isolation @@ -130,12 +144,6 @@ [username] (format "Hi %s, you're an OSS customer!" (name username))) -(defenterprise greeting-with-valid-token - "Returns a non-special greeting for OSS users, and EE users who don't have a valid premium token" - metabase-enterprise.util-test - [username] - (format "Hi %s, you're not extra special :(" (name username))) - (defenterprise special-greeting "Returns a non-special greeting for OSS users, and EE users who don't have the :special-greeting feature token." metabase-enterprise.util-test @@ -160,15 +168,6 @@ (is (= "Hi rasta, you're running the Enterprise Edition of Metabase!" (greeting :rasta)))) - (testing "if :feature = :any or nil, it will check if any feature exists, and fall back to the OSS version by default" - (with-premium-features #{:some-feature} - (is (= "Hi rasta, you're an EE customer with a valid token!" - (greeting-with-valid-token :rasta)))) - - (with-premium-features #{} - (is (= "Hi rasta, you're not extra special :(" - (greeting-with-valid-token :rasta))))) - (testing "if a specific premium feature is required, it will check for it, and fall back to the OSS version by default" (with-premium-features #{:special-greeting} (is (= "Hi rasta, you're an extra special EE customer!"