Search clean-up: separate permissions and logical filters (#50046)

.clj-kondo/config.edn

@@ -668,7 +668,7 @@
     metabase.search.config                                        search.config
     metabase.search.filter                                        search.filter
     metabase.search.in-place.filter                               search.in-place.filter
-    metabase.search.util                                          search.util
+    metabase.search.in-place.util                                 search.util
     metabase.search.in-place.scoring                              scoring
     metabase.server.middleware.auth                               mw.auth
     metabase.server.middleware.browser-cookie                     mw.browser-cookie

src/metabase/models/table.clj

@@ -311,7 +311,7 @@
                   :view-count    true
                   :created-at    true
                   :updated-at    true}
-   :search-terms [:name :description :display_name]
+   :search-terms [:name :display_name :description]
    :render-terms {:initial-sync-status true
                   :table-id            :id
                   :table-description   :description

src/metabase/public_settings.clj

@@ -1052,7 +1052,6 @@ See [fonts](../configuring-metabase/fonts.md).")
 (defsetting experimental-search-weight-overrides
   (deferred-tru "Used to override weights used for search ranking")
   :visibility :internal
-  :cache?     false
   :encryption :no
   :export?    false
   :default    nil

src/metabase/search/filter.clj

@@ -8,7 +8,8 @@
    [metabase.search.permissions :as search.permissions]
    [metabase.search.spec :as search.spec]
    [metabase.util.date-2 :as u.date]
-   [metabase.util.i18n :refer [tru]])
+   [metabase.util.i18n :refer [tru]]
+   [toucan2.core :as t2])
   (:import
    (java.time LocalDate)))
 
@@ -77,6 +78,31 @@
 
 (defmethod where-clause* ::list [_ k v] [:in k v])
 
+(defn personal-collections-where-clause
+  "Build a clause limiting the entries to those (not) within or within personal collections, if relevant.
+  WARNING: this method queries the appdb, and its approach will get very slow when there are many users!"
+  [{filter-type :filter-items-in-personal-collection} collection-id-col]
+  (when filter-type
+    (let [parent-ids     (t2/select-pks-vec :model/Collection :personal_owner_id [:not= nil])
+          child-patterns (for [id parent-ids] (format "/%d/%%" id))]
+      (case filter-type
+        "only"
+        `[:or
+          ;; top level personal collections
+          [:and [:not= :collection.personal_owner_id nil] [:= :collection.location "/"]]
+          ;; their sub-collections
+          ~@(for [p child-patterns] [:like :collection.location p])]
+
+        "exclude"
+        `[:or
+          ;; not in a collection
+          [:= ~collection-id-col nil]
+          [:and
+           ;; neither in a top-level personal collection
+           [:= :collection.personal_owner_id nil]
+           ;; nor within one of their sub-collections
+           ~@(for [p child-patterns] [:not-like :collection.location p])]]))))
+
 (defn with-filters
   "Return a HoneySQL clause corresponding to all the optional search filters."
   [search-context qry]

src/metabase/search/in_place/filter.clj

@@ -19,8 +19,8 @@
    [metabase.search.config
     :as search.config
     :refer [SearchableModel SearchContext]]
+   [metabase.search.in-place.util :as search.util]
    [metabase.search.permissions :as search.permissions]
-   [metabase.search.util :as search.util]
    [metabase.util.date-2 :as u.date]
    [metabase.util.i18n :refer [tru]]
    [metabase.util.malli :as mu])

src/metabase/search/in_place/scoring.clj

@@ -115,7 +115,7 @@
    [java-time.api :as t]
    [metabase.public-settings.premium-features :refer [defenterprise]]
    [metabase.search.config :as search.config]
-   [metabase.search.util :as search.util]
+   [metabase.search.in-place.util :as search.util]
    [metabase.util :as u]))
 
 (defn- matches?

src/metabase/search/util.clj → src/metabase/search/in_place/util.clj

-(ns metabase.search.util
+(ns metabase.search.in-place.util
   (:require
    [clojure.core.memoize :as memoize]
    [clojure.string :as str]

src/metabase/search/legacy.clj

@@ -11,10 +11,11 @@
    [metabase.search.config
     :as search.config
     :refer [SearchContext SearchableModel]]
+   [metabase.search.filter :as search.filter]
    [metabase.search.in-place.filter :as search.in-place.filter]
    [metabase.search.in-place.scoring :as scoring]
+   [metabase.search.in-place.util :as search.util]
    [metabase.search.permissions :as search.permissions]
-   [metabase.search.util :as search.util]
    [metabase.util :as u]
    [metabase.util.honey-sql-2 :as h2x]
    [metabase.util.log :as log]
@@ -151,6 +152,24 @@
     (sql.helpers/where query [:= id :database_id])
     query))
 
+(mu/defn add-collection-join-and-where-clauses
+  "Add a `WHERE` clause to the query to only return Collections the Current User has access to; join against Collection,
+  so we can return its `:name`."
+  [honeysql-query :- :map
+   model          :- [:maybe :string]
+   search-ctx     :- SearchContext]
+  (let [collection-id-col      (case model
+                                 "collection"    :collection.id
+                                 "search-index" :search_index.collection_id
+                                 :collection_id)
+        permitted-clause       (search.permissions/permitted-collections-clause search-ctx collection-id-col)
+        personal-clause        (search.filter/personal-collections-where-clause search-ctx collection-id-col)]
+    (cond-> honeysql-query
+      ;; add a JOIN against Collection *unless* the source table is already Collection
+      (not= model "collection") (sql.helpers/left-join [:collection :collection] [:= collection-id-col :collection.id])
+      true                      (sql.helpers/where permitted-clause)
+      personal-clause           (sql.helpers/where personal-clause))))
+
 (mu/defn- replace-select :- :map
   "Replace a select from query that has alias is `target-alias` with [`with` `target-alias`] column, throw an error if
   can't find the target select.
@@ -418,7 +437,7 @@
                              [:and
                               [:= :bookmark.card_id :card.id]
                               [:= :bookmark.user_id (:current-user-id search-ctx)]])
-      (search.permissions/add-collection-join-and-where-clauses "card" search-ctx)
+      (add-collection-join-and-where-clauses "card" search-ctx)
       (add-card-db-id-clause (:table-db-id search-ctx))
       (with-last-editing-info "card")
       (with-moderated-status "card")))
@@ -430,7 +449,7 @@
                              [:= :model.id :action.model_id])
       (sql.helpers/left-join :query_action
                              [:= :query_action.action_id :action.id])
-      (search.permissions/add-collection-join-and-where-clauses model search-ctx)))
+      (add-collection-join-and-where-clauses model search-ctx)))
 
 (defmethod search-query-for-model "card"
   [_model search-ctx]
@@ -455,7 +474,7 @@
                              [:and
                               [:= :bookmark.collection_id :collection.id]
                               [:= :bookmark.user_id (:current-user-id search-ctx)]])
-      (search.permissions/add-collection-join-and-where-clauses model search-ctx)))
+      (add-collection-join-and-where-clauses model search-ctx)))
 
 (defmethod search-query-for-model "database"
   [model search-ctx]
@@ -469,7 +488,7 @@
                               [:= :bookmark.dashboard_id :dashboard.id]
                               [:= :bookmark.user_id (:current-user-id search-ctx)]])
       (with-moderated-status "dashboard")
-      (search.permissions/add-collection-join-and-where-clauses model search-ctx)
+      (add-collection-join-and-where-clauses model search-ctx)
       (with-last-editing-info "dashboard")))
 
 (defn- add-model-index-permissions-clause

src/metabase/search/permissions.clj

 (ns metabase.search.permissions
   (:require
-   [honey.sql.helpers :as sql.helpers]
    [metabase.models.collection :as collection]
    [metabase.models.permissions :as perms]
    [metabase.public-settings.premium-features :as premium-features]
    [metabase.search.config :refer [SearchContext]]
-   [metabase.util.malli :as mu]
-   [toucan2.core :as t2]))
+   [metabase.util.malli :as mu]))
 
 (defn- assert-current-user! [missing-param]
   (assert @@(requiring-resolve 'metabase.api.common/*current-user*)
@@ -31,54 +29,15 @@
   [search-ctx]
   (or (impersonated-user? search-ctx) (sandboxed-user? search-ctx)))
 
-(mu/defn add-collection-join-and-where-clauses
-  "Add a `WHERE` clause to the query to only return Collections the Current User has access to; join against Collection,
-  so we can return its `:name`."
-  [honeysql-query :- :map
-   model :- [:maybe :string]
-   {:keys [filter-items-in-personal-collection
-           archived
-           current-user-id
-           is-superuser?]} :- SearchContext]
-  (let [collection-id-col        (case model
-                                   "collection"   :collection.id
-                                   "search-index" :search_index.collection_id
-                                   :collection_id)
-        collection-filter-clause (collection/visible-collection-filter-clause
-                                  collection-id-col
-                                  {:include-archived-items    :all
-                                   :include-trash-collection? true
-                                   :permission-level          (if archived
-                                                                :write
-                                                                :read)}
-                                  {:current-user-id current-user-id
-                                   :is-superuser?   is-superuser?})]
-    (cond-> honeysql-query
-      true
-      (sql.helpers/where collection-filter-clause (perms/audit-namespace-clause :collection.namespace nil))
-      ;; add a JOIN against Collection *unless* the source table is already Collection
-      (not= model "collection")
-      (sql.helpers/left-join [:collection :collection]
-                             [:= collection-id-col :collection.id])
-
-      ;; TODO This is not really about permissions, it should really be handled in search.filter
-      (some? filter-items-in-personal-collection)
-      (sql.helpers/where
-       (case filter-items-in-personal-collection
-         "only"
-         (concat [:or]
-                 ;; sub personal collections
-                 (for [id (t2/select-pks-set :model/Collection :personal_owner_id [:not= nil])]
-                   [:like :collection.location (format "/%d/%%" id)])
-                 ;; top level personal collections
-                 [[:and
-                   [:= :collection.location "/"]
-                   [:not= :collection.personal_owner_id nil]]])
-
-         "exclude"
-         (conj [:or]
-               (into
-                [:and [:= :collection.personal_owner_id nil]]
-                (for [id (t2/select-pks-set :model/Collection :personal_owner_id [:not= nil])]
-                  [:not-like :collection.location (format "/%d/%%" id)]))
-               [:= collection-id-col nil]))))))
+(mu/defn permitted-collections-clause
+  "Build the WHERE clause corresponding to which collections the given user has access to."
+  [{:keys [archived current-user-id is-superuser?]} :- SearchContext collection-id-col :- :keyword]
+  [:and
+   (collection/visible-collection-filter-clause
+    collection-id-col
+    {:include-archived-items    :all
+     :include-trash-collection? true
+     :permission-level          (if archived :write :read)}
+    {:current-user-id current-user-id
+     :is-superuser?   is-superuser?})
+   (perms/audit-namespace-clause :collection.namespace nil)])

src/metabase/search/postgres/core.clj

@@ -81,14 +81,26 @@
       (update :updated_at parse-datetime)
       (update :last_edited_at parse-datetime)))
 
+(defn add-collection-join-and-where-clauses
+  "Add a `WHERE` clause to the query to only return Collections the Current User has access to; join against Collection,
+  so we can return its `:name`."
+  [search-ctx qry]
+  (let [collection-id-col      :search_index.collection_id
+        permitted-clause       (search.permissions/permitted-collections-clause search-ctx collection-id-col)
+        personal-clause        (search.filter/personal-collections-where-clause search-ctx collection-id-col)]
+    (cond-> qry
+      true (sql.helpers/left-join [:collection :collection] [:= collection-id-col :collection.id])
+      true (sql.helpers/where permitted-clause)
+      personal-clause (sql.helpers/where personal-clause))))
+
 (defn- fulltext
   "Search purely using the index."
   [search-term & {:as search-ctx}]
   (when-not @#'search.index/initialized?
     (throw (ex-info "Search index is not initialized. Use [[init!]] to ensure it exists."
                     {:search-engine :postgres})))
-  (->> (let [base-query (search.index/search-query search-term search-ctx [:legacy_input])]
-         (search.permissions/add-collection-join-and-where-clauses base-query "search-index" search-ctx))
+  (->> (search.index/search-query search-term search-ctx [:legacy_input])
+       (add-collection-join-and-where-clauses search-ctx)
        (search.scoring/with-scores search-ctx)
        (search.filter/with-filters search-ctx)
        (t2/query)
@@ -114,8 +126,8 @@
   [search-ctx]
   ;; We ignore any current models filter
   (let [search-ctx (assoc search-ctx :models search.config/all-models)]
-    (->> (-> (search.index/search-query (:search-string search-ctx) search-ctx [[[:distinct :model] :model]])
-             (search.permissions/add-collection-join-and-where-clauses "search-index" search-ctx))
+    (->> (search.index/search-query (:search-string search-ctx) search-ctx [[[:distinct :model] :model]])
+         (add-collection-join-and-where-clauses search-ctx)
          (search.filter/with-filters search-ctx)
          t2/query
          (into #{} (map :model)))))

test/metabase/api/search_test.clj

@@ -313,7 +313,7 @@
   (when (search/supports-index?)
     (testing "It can use an alternate search engine"
       (with-search-items-in-root-collection "test"
-        (let [resp (search-request :crowberto :q "test" :search_engine "fulltext")]
+        (let [resp (search-request :crowberto :q "test" :search_engine "fulltext" :limit 1)]
           ;; The index is not populated here, so there's not much interesting to assert.
           (is (= "search.engine/fulltext" (:engine resp))))))))
 

test/metabase/search/util_test.clj → test/metabase/search/in_place/util_test.clj

-(ns metabase.search.util-test
+(ns metabase.search.in-place.util-test
   (:require
    [clojure.test :refer :all]
-   [metabase.search.util :as search.util]))
+   [metabase.search.in-place.util :as search.util]))
 
 (deftest ^:parallel tokenize-test
   (testing "basic tokenization"