Another fix to avoid dynamic scope in search (#48052)

src/metabase/models/collection.clj

@@ -500,24 +500,24 @@
    :archive-operation-id nil
    :permission-level :read})
 
-(def ^:private ^{:arglists '([read-or-write])} can-access-root-collection?
+(def ^:private ^{:arglists '([user-scope read-or-write])} can-access-root-collection?
   "Cached function to determine whether the current user can access the root collection"
   (memoize/ttl
-   ^{::memoize/args-fn (fn [[read-or-write]]
+   ^{::memoize/args-fn (fn [[{:keys [current-user-id]} read-or-write]]
                          ;; If this is running in the context of a request, cache it for the duration of that request.
                          ;; Otherwise, don't cache the results at all.)
                          (if-let [req-id *request-id*]
-                           [req-id api/*current-user-id* read-or-write]
-                           [(random-uuid) api/*current-user-id* read-or-write]))}
+                           [req-id current-user-id read-or-write]
+                           [(random-uuid) current-user-id read-or-write]))}
    (fn can-access-root-collection?*
-     [read-or-write]
-     (or api/*is-superuser?*
+     [{:keys [current-user-id is-superuser?]} read-or-write]
+     (or is-superuser?
          (t2/exists? :model/Permissions {:select [:p.*]
                                          :from [[:permissions :p]]
                                          :join [[:permissions_group :pg] [:= :pg.id :p.group_id]
                                                 [:permissions_group_membership :pgm] [:= :pgm.group_id :pg.id]]
                                          :where [:and
-                                                 [:= :pgm.user_id api/*current-user-id*]
+                                                 [:= :pgm.user_id current-user-id]
                                                  [:or
                                                   [:= :p.object "/collection/root/"]
                                                   (when (= :read read-or-write)
@@ -527,19 +527,24 @@
 
 (defn- should-display-root-collection?
   "Should this user be shown the root collection, given the `visibility-config` passed?"
-  [visibility-config]
-  (and
-   ;; we have permission for it.
-   (can-access-root-collection? (:permission-level visibility-config))
+  ([visibility-config]
+   (should-display-root-collection?
+    {:current-user-id api/*current-user-id*
+     :is-superuser?   api/*is-superuser?*}
+    visibility-config))
+  ([user-scope visibility-config]
+   (and
+    ;; we have permission for it.
+    (can-access-root-collection? user-scope (:permission-level visibility-config))
 
-   ;; we're not *only* looking for archived items
-   (not= :only (:include-archived-items visibility-config))
+    ;; we're not *only* looking for archived items
+    (not= :only (:include-archived-items visibility-config))
 
-   ;; we're not looking for a particular `archive_operation_id`
-   (not (:archive-operation-id visibility-config))
+    ;; we're not looking for a particular `archive_operation_id`
+    (not (:archive-operation-id visibility-config))
 
-   ;; we're not looking for the children of a collection (root definitely isn't a child!)
-   (not (:effective-child-of visibility-config))))
+    ;; we're not looking for the children of a collection (root definitely isn't a child!)
+    (not (:effective-child-of visibility-config)))))
 
 (mu/defn visible-collection-filter-clause
   "Given a `CollectionVisibilityConfig`, return a honeysql filter clause ready for use in queries."
@@ -554,7 +559,7 @@
                                       :is-superuser?   api/*is-superuser?*}))
   ([collection-id-field :- [:or [:tuple [:= :coalesce] :keyword :keyword] :keyword]
     visibility-config :- CollectionVisibilityConfig
-    {:keys [current-user-id is-superuser?]} :- UserScope]
+    {:keys [current-user-id is-superuser?] :as user-scope} :- UserScope]
    (let [visibility-config (merge default-visibility-config visibility-config)]
      ;; This giant query looks scary, but it's actually only moderately terrifying! Let's walk through it step by
      ;; step. What we're doing here is adding a filter clause to a surrounding query, to make sure that
@@ -572,7 +577,7 @@
      ;; `should-display-root-collection?` but it's pretty simple. We can't include the root collection along with the
      ;; rest because it's not a Real collection.
      [:or
-      (when (should-display-root-collection? visibility-config)
+      (when (should-display-root-collection? user-scope visibility-config)
         [:= collection-id-field nil])
       ;; the non-root collections are here. We're saying "let this row through if..."
       [:in collection-id-field

src/metabase/search/impl.clj

@@ -337,28 +337,27 @@
   {:arglists '([search-ctx search-result])}
   (fn [_search-ctx search-result] ((comp keyword :model) search-result)))
 
-(defn- assert-current-user-perms-set-is-bound
+(defmacro ^:private ensure-current-user-perms-set-is-bound
   "TODO FIXME -- search actually currently still requires [[metabase.api.common/*current-user-permissions-set*]] to be
   bound (since [[mi/can-write?]] and [[mi/can-read?]] depend on it) despite search context requiring
   `:current-user-perms` to be passed in. We should fix things so search works independently of API-specific dynamic
   variables. This might require updating `can-read?` and `can-write?` to take explicit perms sets instead of relying
   on dynamic variables."
-  []
-  (assert (seq @@(requiring-resolve 'metabase.api.common/*current-user-permissions-set*))
-          "metabase.api.common/*current-user-permissions-set* must be bound in order to check search permissions"))
+  {:style/indent 0}
+  [current-user-perms & body]
+  `(with-bindings {(requiring-resolve 'metabase.api.common/*current-user-permissions-set*) (atom ~current-user-perms)}
+     ~@body))
 
-(defn- can-write? [instance]
-  (assert-current-user-perms-set-is-bound)
-  (mi/can-write? instance))
+(defn- can-write? [{:keys [current-user-perms]} instance]
+  (ensure-current-user-perms-set-is-bound current-user-perms (mi/can-write? instance)))
 
-(defn- can-read? [instance]
-  (assert-current-user-perms-set-is-bound)
-  (mi/can-read? instance))
+(defn- can-read? [{:keys [current-user-perms]} instance]
+  (ensure-current-user-perms-set-is-bound current-user-perms (mi/can-read? instance)))
 
 (defmethod check-permissions-for-model :default
   [search-ctx instance]
   (if (:archived? search-ctx)
-    (can-write? instance)
+    (can-write? search-ctx instance)
     ;; We filter what we can (i.e., everything in a collection) out already when querying
     true))
 
@@ -383,20 +382,20 @@
 (defmethod check-permissions-for-model :metric
   [search-ctx instance]
   (if (:archived? search-ctx)
-    (can-write? instance)
-    (can-read? instance)))
+    (can-write? search-ctx instance)
+    (can-read? search-ctx instance)))
 
 (defmethod check-permissions-for-model :segment
   [search-ctx instance]
   (if (:archived? search-ctx)
-    (can-write? instance)
-    (can-read? instance)))
+    (can-write? search-ctx instance)
+    (can-read? search-ctx instance)))
 
 (defmethod check-permissions-for-model :database
   [search-ctx instance]
   (if (:archived? search-ctx)
-    (can-write? instance)
-    (can-read? instance)))
+    (can-write? search-ctx instance)
+    (can-read? search-ctx instance)))
 
 (mu/defn query-model-set :- [:set SearchableModel]
   "Queries all models with respect to query for one result to see if we get a result or not"
@@ -666,22 +665,22 @@
         ;; Collections require some transformation before being scored and returned by search.
         (cond-> (t2/instance-of? :model/Collection instance) map-collection))))
 
-(defn- add-can-write [row]
+(defn- add-can-write [search-ctx row]
   (if (some #(mi/instance-of? % row) [:model/Dashboard :model/Card])
-    (assoc row :can_write (can-write? row))
+    (assoc row :can_write (can-write? search-ctx row))
     row))
 
 (defn- normalize-result-more
   "Additional normalization that is done after we've filtered by permissions, as its more expensive."
-  [result]
-  (-> (update result :pk_ref json/parse-string)
-      add-can-write))
+  [search-ctx result]
+  (->> (update result :pk_ref json/parse-string)
+       (add-can-write search-ctx)))
 
 (defn- search-results [search-ctx total-results]
   (let [add-perms-for-col  (fn [item]
                              (cond-> item
                                (mi/instance-of? :model/Collection item)
-                               (assoc :can_write (can-write? item))))]
+                               (assoc :can_write (can-write? search-ctx item))))]
     ;; We get to do this slicing and dicing with the result data because
     ;; the pagination of search is for UI improvement, not for performance.
     ;; We intend for the cardinality of the search results to be below the default max before this slicing occurs
@@ -708,7 +707,7 @@
                             (take search.config/*db-max-results*)
                             (map normalize-result)
                             (filter (partial check-permissions-for-model search-ctx))
-                            (map normalize-result-more)
+                            (map (partial normalize-result-more search-ctx))
                              ;; scoring - note that this can also filter further!
                             (map #(scoring/score-and-result % scoring-ctx))
                             (filter #(pos? (:score %))))

src/metabase/search/postgres/core.clj

@@ -87,7 +87,10 @@
   (when s
     (OffsetDateTime/parse s)))
 
-(defn- minimal [search-term & {:as _search-ctx}]
+(defn- minimal
+  "Search via index, and return potentially stale information, without applying filters or
+  restricting to collections we have access to."
+  [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})))

test/metabase/search/postgres/core_test.clj

@@ -63,6 +63,5 @@
     (testing "consistent results between both hybrid implementations\n"
       (doseq [term example-terms]
         (testing term
-          ;; Timestamps are not strings after round trip, but this doesn't matter
           (is (= (hybrid term)
                  (minimal term))))))))