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

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)

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?))

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)

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

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

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

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!)

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"}]})))))

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 []

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"

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."

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`
 

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

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"

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!"