From aadcdcacb1a72cd588eaf01ad198a170d7429985 Mon Sep 17 00:00:00 2001 From: Noah Moss <32746338+noahmoss@users.noreply.github.com> Date: Thu, 6 Jul 2023 14:47:36 -0400 Subject: [PATCH] Fix several backend feature flag checks and remove `:any` flag from `defenterprise` (#32100) * put search enhancements under content-management flag * fix token check for snippet perms and add with-additional-premium-features macro * remove :feature :any option for defenterprise * fix whitespace in docstring * fix snippet API tests * fix final tests * fix feature for snippets, plus some small test fixes * fix more tests * Clean ns --- .../enhancements/integrations/ldap.clj | 4 +- .../native_query_snippet/permissions.clj | 8 +-- .../backend/src/metabase_enterprise/pulse.clj | 2 +- .../metabase_enterprise/search/scoring.clj | 2 +- .../api/native_query_snippet_test.clj | 4 +- .../enhancements/integrations/ldap_test.clj | 11 ++-- .../native_query_snippet/permissions_test.clj | 2 +- .../test/metabase_enterprise/pulse_test.clj | 12 ++-- .../metabase_enterprise/sandbox/test_util.clj | 2 +- .../search/scoring_test.clj | 14 +++-- .../test/metabase_enterprise/util_test.clj | 7 --- .../public_settings/premium_features.clj | 13 ++-- src/metabase/search/scoring.clj | 8 +-- test/metabase/integrations/ldap_test.clj | 63 ++++++++++--------- .../public_settings/premium_features_test.clj | 29 +++++---- 15 files changed, 88 insertions(+), 93 deletions(-) diff --git a/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj b/enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj index dcee6aa5575..9775695ab43 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 ed49995236e..5f3c751650d 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 f772eae99d3..a71ce3223cf 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 038f98af5f7..6c13920d218 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 7f34dc9878b..cb17cf630fd 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 d49603fd86c..93e69d659bb 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 86b11cbab9b..e6ff5c33eeb 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 9231a26bc0b..a43772801e7 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 c12eebcdc8e..812258d2e6a 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 fa2f77ab334..549a4f01c18 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 edc6423b375..88736371fe1 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 5251008dbc6..0283b2d7bc2 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 366ece3b4e6..6dfac99efac 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 0841c8af51f..28f7640dc22 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 63ab9b13f8d..2852e325597 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!" -- GitLab