From c4fd58f0d905aeb47f06df67dc25851f5241b0b9 Mon Sep 17 00:00:00 2001
From: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>
Date: Fri, 3 Feb 2023 02:37:34 +0100
Subject: [PATCH] Revert "Added logic to correctly return collections for only
the current user (#27962)" (#28046)
This reverts commit dca5f4414e4caacc76c75ad1975e137c490fc74a.
---
src/metabase/api/collection.clj | 270 ++++++++++++--------------
test/metabase/api/collection_test.clj | 28 +--
2 files changed, 124 insertions(+), 174 deletions(-)
diff --git a/src/metabase/api/collection.clj b/src/metabase/api/collection.clj
index 6fa57812f85..57d3e7b5b8e 100644
--- a/src/metabase/api/collection.clj
+++ b/src/metabase/api/collection.clj
@@ -37,42 +37,13 @@
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db]
- [toucan.hydrate :refer [hydrate]]
- [toucan.models :as models]))
+ [toucan.hydrate :refer [hydrate]]))
;;; when alias defined for namespaced keywords is run through kondo macro, ns should be regarded as used
(comment collection.root/keep-me)
(declare root-collection)
-(defn- coll-query
- "Generate a query to return collections viewable by the current user.
- If provided, additional and clauses will be added to the final where clause.
- Note that and clauses should be aliased as c (for collection). Note also that
- MySQL 5.7 does not support CTEs so this was all inlined. A much more readable
- solution existed with CTEs, but we needed to inline for backwards compatibility."
- [owner-id & and-clauses]
- {:select [:c.*]
- :from [[:collection :c]]
- :left-join [[{:select [[:cwo.id :id]
- [true :visible]]
- :from [[{:select [[:c.id :id]
- [[:coalesce
- :c.personal_owner_id
- :pr.personal_owner_id] :owner]]
- :from [[:collection :c]]
- :left-join [[{:select [:c.personal_owner_id
- [[:concat "/" :c.id "/%"] :pc_prefix]]
- :from [[:collection :c]]
- :where [:not= :c.personal_owner_id nil]} :pr]
- [:like :c.location :pr.pc_prefix]]} :cwo]]
- :where [:or
- [:= :cwo.owner owner-id]
- [:= :cwo.owner nil]]} :a]
- [:= :a.id :c.id]]
- :where (into [:and [:= :a.visible true]] and-clauses)
- :order-by [[:%lower.name :asc]]})
-
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema GET "/"
"Fetch a list of all Collections that the current user has read permissions for (`:can_write` is returned as an
@@ -83,30 +54,29 @@
[archived namespace]
{archived (s/maybe su/BooleanString)
namespace (s/maybe su/NonBlankString)}
- (let [archived? (Boolean/parseBoolean archived)
- q (coll-query
- api/*current-user-id*
- [:= :c.archived archived?]
- [:= :c.namespace namespace]
- (collection/visible-collection-ids->honeysql-filter-clause
- :c.id
- (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*)))]
- (as-> (->> (mdb.query/query q)
- (map (partial models/do-post-select Collection))) collections
- ;; include Root Collection at beginning or results if archived isn't `true`
- (if archived?
- collections
- (let [root (root-collection namespace)]
- (cond->> collections
- (mi/can-read? root)
- (cons root))))
- (hydrate collections :can_write)
- ;; remove the :metabase.models.collection.root/is-root? tag since FE doesn't need it
- ;; and for personal collections we translate the name to user's locale
- (for [collection collections]
- (-> collection
- (dissoc ::collection.root/is-root?)
- collection/personal-collection-with-ui-details)))))
+ (let [archived? (Boolean/parseBoolean archived)]
+ (as-> (db/select Collection
+ {:where [:and
+ [:= :archived archived?]
+ [:= :namespace namespace]
+ (collection/visible-collection-ids->honeysql-filter-clause
+ :id
+ (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*))]
+ :order-by [[:%lower.name :asc]]}) collections
+ ;; include Root Collection at beginning or results if archived isn't `true`
+ (if archived?
+ collections
+ (let [root (root-collection namespace)]
+ (cond->> collections
+ (mi/can-read? root)
+ (cons root))))
+ (hydrate collections :can_write)
+ ;; remove the :metabase.models.collection.root/is-root? tag since FE doesn't need it
+ ;; and for personal collections we translate the name to user's locale
+ (for [collection collections]
+ (-> collection
+ (dissoc ::collection.root/is-root?)
+ collection/personal-collection-with-ui-details)))))
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema GET "/tree"
@@ -140,16 +110,14 @@
(mdb.query/reducible-query {:select-distinct [:collection_id :dataset]
:from [:report_card]
:where [:= :archived false]}))
- q (coll-query
- api/*current-user-id*
- (when exclude-archived
- [:= :c.archived false])
- [:= :c.namespace namespace]
- (collection/visible-collection-ids->honeysql-filter-clause
- :c.id
- (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*)))
- colls (->> (mdb.query/query q)
- (map (partial models/do-post-select Collection)))
+ colls (db/select Collection
+ {:where [:and
+ (when exclude-archived
+ [:= :archived false])
+ [:= :namespace namespace]
+ (collection/visible-collection-ids->honeysql-filter-clause
+ :id
+ (collection/permissions-set->visible-collection-ids @api/*current-user-permissions-set*))]})
colls (map collection/personal-collection-with-ui-details colls)]
(collection/collections->tree coll-type-ids colls)))
@@ -217,8 +185,8 @@
(pinned-state->clause pinned-state :collection_position))
([pinned-state col]
(case pinned-state
- :all always-true-hsql-expr
- :is_pinned [:<> col nil]
+ :all always-true-hsql-expr
+ :is_pinned [:<> col nil]
:is_not_pinned [:= col nil]
always-true-hsql-expr)))
@@ -249,10 +217,10 @@
:from [[:pulse :p]]
:left-join [[:pulse_card :pc] [:= :p.id :pc.pulse_id]]
:where [:and
- [:= :p.collection_id (:id collection)]
- [:= :p.archived (boolean archived?)]
+ [:= :p.collection_id (:id collection)]
+ [:= :p.archived (boolean archived?)]
;; exclude alerts
- [:= :p.alert_condition nil]
+ [:= :p.alert_condition nil]
;; exclude dashboard subscriptions
[:= :p.dashboard_id nil]]}
(sql.helpers/where (pinned-state->clause pinned-state :p.collection_position))))
@@ -439,15 +407,15 @@
collection
[:= :archived archived?]
[:= :namespace (u/qualified-name collection-namespace)])
- ;; We get from the effective-children-query a normal set of columns selected:
- ;; want to make it fit the others to make UNION ALL work
- :select [:id
- :name
- :description
- :entity_id
- :personal_owner_id
- [(h2x/literal "collection") :model]
- :authority_level])
+ ;; We get from the effective-children-query a normal set of columns selected:
+ ;; want to make it fit the others to make UNION ALL work
+ :select [:id
+ :name
+ :description
+ :entity_id
+ :personal_owner_id
+ [(h2x/literal "collection") :model]
+ :authority_level])
;; the nil indicates that collections are never pinned.
(sql.helpers/where (pinned-state->clause pinned-state nil))))
@@ -463,11 +431,11 @@
;; Previous examination with logging to DB says that there's no N+1 query for this.
;; However, this was only tested on H2 and Postgres
(cond-> row
- ;; when fetching root collection, we might have personal collection
- (:personal_owner_id row) (assoc :name (collection/user->personal-collection-name (:personal_owner_id row) :user))
- true (assoc :can_write (mi/can-write? Collection (:id row)))
- true (dissoc :collection_position :display :moderated_status :icon :personal_owner_id
- :collection_preview :dataset_query))))
+ ;; when fetching root collection, we might have personal collection
+ (:personal_owner_id row) (assoc :name (collection/user->personal-collection-name (:personal_owner_id row) :user))
+ true (assoc :can_write (mi/can-write? Collection (:id row)))
+ true (dissoc :collection_position :display :moderated_status :icon :personal_owner_id
+ :collection_preview :dataset_query))))
(s/defn ^:private coalesce-edit-info :- last-edit/MaybeAnnotated
"Hoist all of the last edit information into a map under the key :last-edit-info. Considers this information present
@@ -483,8 +451,8 @@
:last_edit_email :email
:last_edit_timestamp :timestamp}]
(cond-> (apply dissoc row :model_ranking (keys mapping))
- ;; don't use contains as they all have the key, we care about a value present
- (:last_edit_user row) (assoc :last-edit-info (select-as row mapping))))))
+ ;; don't use contains as they all have the key, we care about a value present
+ (:last_edit_user row) (assoc :last-edit-info (select-as row mapping))))))
(defn- post-process-rows
"Post process any data. Have a chance to process all of the same type at once using
@@ -502,12 +470,12 @@
(defn- model-name->toucan-model [model-name]
(case (keyword model-name)
:collection Collection
- :card Card
- :dataset Card
- :dashboard Dashboard
- :pulse Pulse
- :snippet NativeQuerySnippet
- :timeline Timeline))
+ :card Card
+ :dataset Card
+ :dashboard Dashboard
+ :pulse Pulse
+ :snippet NativeQuerySnippet
+ :timeline Timeline))
(defn- select-name
"Takes a honeysql select column and returns a keyword of which column it is.
@@ -566,56 +534,56 @@
treatment of nulls in the different app db types."
[sort-info db-type]
(case sort-info
- nil [[:%lower.name :asc]]
- [:name :asc] [[:%lower.name :asc]]
- [:name :desc] [[:%lower.name :desc]]
- [:last-edited-at :asc] [(if (= db-type :mysql)
- [:%isnull.last_edit_timestamp]
- [:last_edit_timestamp :nulls-last])
- [:last_edit_timestamp :asc]
- [:%lower.name :asc]]
+ nil [[:%lower.name :asc]]
+ [:name :asc] [[:%lower.name :asc]]
+ [:name :desc] [[:%lower.name :desc]]
+ [:last-edited-at :asc] [(if (= db-type :mysql)
+ [:%isnull.last_edit_timestamp]
+ [:last_edit_timestamp :nulls-last])
+ [:last_edit_timestamp :asc]
+ [:%lower.name :asc]]
[:last-edited-at :desc] (remove nil?
[(case db-type
- :mysql [:%isnull.last_edit_timestamp]
+ :mysql [:%isnull.last_edit_timestamp]
:postgres [:last_edit_timestamp :desc-nulls-last]
- :h2 nil)
+ :h2 nil)
[:last_edit_timestamp :desc]
[:%lower.name :asc]])
- [:last-edited-by :asc] [(if (= db-type :mysql)
- [:%isnull.last_edit_last_name]
- [:last_edit_last_name :nulls-last])
- [:last_edit_last_name :asc]
- (if (= db-type :mysql)
- [:%isnull.last_edit_first_name]
- [:last_edit_first_name :nulls-last])
- [:last_edit_first_name :asc]
- [:%lower.name :asc]]
+ [:last-edited-by :asc] [(if (= db-type :mysql)
+ [:%isnull.last_edit_last_name]
+ [:last_edit_last_name :nulls-last])
+ [:last_edit_last_name :asc]
+ (if (= db-type :mysql)
+ [:%isnull.last_edit_first_name]
+ [:last_edit_first_name :nulls-last])
+ [:last_edit_first_name :asc]
+ [:%lower.name :asc]]
[:last-edited-by :desc] (remove nil?
[(case db-type
- :mysql [:%isnull.last_edit_last_name]
+ :mysql [:%isnull.last_edit_last_name]
:postgres [:last_edit_last_name :desc-nulls-last]
- :h2 nil)
+ :h2 nil)
[:last_edit_last_name :desc]
(case db-type
- :mysql [:%isnull.last_edit_first_name]
+ :mysql [:%isnull.last_edit_first_name]
:postgres [:last_edit_last_name :desc-nulls-last]
- :h2 nil)
+ :h2 nil)
[:last_edit_first_name :desc]
[:%lower.name :asc]])
- [:model :asc] [[:model_ranking :asc] [:%lower.name :asc]]
- [:model :desc] [[:model_ranking :desc] [:%lower.name :asc]]))
+ [:model :asc] [[:model_ranking :asc] [:%lower.name :asc]]
+ [:model :desc] [[:model_ranking :desc] [:%lower.name :asc]]))
(defn- collection-children*
[collection models {:keys [sort-info] :as options}]
(let [sql-order (children-sort-clause sort-info (mdb/db-type))
models (sort (map keyword models))
queries (for [model models
- :let [query (collection-children-query model collection options)
- select-clause-type (some
- (fn [k]
- (when (get query k)
- k))
- [:select :select-distinct])]]
+ :let [query (collection-children-query model collection options)
+ select-clause-type (some
+ (fn [k]
+ (when (get query k)
+ k))
+ [:select :select-distinct])]]
(-> query
(update select-clause-type add-missing-columns all-select-columns)
(update select-clause-type add-model-ranking model)))
@@ -632,14 +600,14 @@
(= (:collection-namespace options) "snippets"))
rows-query
(assoc rows-query
- :limit mw.offset-paging/*limit*
- :offset mw.offset-paging/*offset*))
+ :limit mw.offset-paging/*limit*
+ :offset mw.offset-paging/*offset*))
res {:total (->> (mdb.query/query total-query) first :count)
:data (->> (mdb.query/query limit-query) post-process-rows)
:models models}
limit-res (assoc res
- :limit mw.offset-paging/*limit*
- :offset mw.offset-paging/*offset*)]
+ :limit mw.offset-paging/*limit*
+ :offset mw.offset-paging/*offset*)]
(if (= (:collection-namespace options) "snippets")
res
limit-res)))
@@ -647,14 +615,14 @@
(s/defn ^:private collection-children
"Fetch a sequence of 'child' objects belonging to a Collection, filtered using `options`."
[{collection-namespace :namespace, :as collection} :- collection/CollectionWithLocationAndIDOrRoot
- {:keys [models], :as options} :- CollectionChildrenOptions]
+ {:keys [models], :as options} :- CollectionChildrenOptions]
(let [valid-models (for [model-kw [:collection :dataset :card :dashboard :pulse :snippet :timeline]
;; only fetch models that are specified by the `model` param; or everything if it's empty
- :when (or (empty? models) (contains? models model-kw))
- :let [toucan-model (model-name->toucan-model model-kw)
- allowed-namespaces (collection/allowed-namespaces toucan-model)]
- :when (or (= model-kw :collection)
- (contains? allowed-namespaces (keyword collection-namespace)))]
+ :when (or (empty? models) (contains? models model-kw))
+ :let [toucan-model (model-name->toucan-model model-kw)
+ allowed-namespaces (collection/allowed-namespaces toucan-model)]
+ :when (or (= model-kw :collection)
+ (contains? allowed-namespaces (keyword collection-namespace)))]
model-kw)]
(if (seq valid-models)
(collection-children* collection valid-models (assoc options :collection-namespace collection-namespace))
@@ -776,12 +744,12 @@
model-set (set (map keyword (u/one-or-many models)))
model-kwds (visible-model-kwds root-collection model-set)]
(collection-children
- root-collection
- {:models model-kwds
- :archived? (Boolean/parseBoolean archived)
- :pinned-state (keyword pinned_state)
- :sort-info [(or (some-> sort_column normalize-sort-choice) :name)
- (or (some-> sort_direction normalize-sort-choice) :asc)]})))
+ root-collection
+ {:models model-kwds
+ :archived? (Boolean/parseBoolean archived)
+ :pinned-state (keyword pinned_state)
+ :sort-info [(or (some-> sort_column normalize-sort-choice) :name)
+ (or (some-> sort_direction normalize-sort-choice) :asc)]})))
;;; ----------------------------------------- Creating/Editing a Collection ------------------------------------------
@@ -793,7 +761,7 @@
(api/write-check (if collection-id
(db/select-one Collection :id collection-id)
(cond-> collection/root-collection
- collection-namespace (assoc :namespace collection-namespace)))))
+ collection-namespace (assoc :namespace collection-namespace)))))
(defn create-collection!
"Create a new collection."
@@ -805,11 +773,11 @@
(and api/*is-superuser?* authority_level)))
(db/insert! Collection
(merge
- {:name name
- :color color
- :description description
+ {:name name
+ :color color
+ :description description
:authority_level authority_level
- :namespace namespace}
+ :namespace namespace}
(when parent_id
{:location (collection/children-location (db/select-one [Collection :location :id] :id parent_id))}))))
@@ -846,7 +814,7 @@
;; ok, make sure we have perms to do this operation
(api/check-403
(perms/set-has-full-permissions-for-set? @api/*current-user-permissions-set*
- (collection/perms-for-moving collection-before-update new-parent)))
+ (collection/perms-for-moving collection-before-update new-parent)))
;; ok, we're good to move!
(collection/move-collection! collection-before-update new-location)))))
@@ -860,7 +828,7 @@
;; Check that we have approprate perms
(api/check-403
(perms/set-has-full-permissions-for-set? @api/*current-user-permissions-set*
- (collection/perms-for-archiving collection-before-update)))))
+ (collection/perms-for-archiving collection-before-update)))))
(defn- maybe-send-archived-notificaitons!
"When a collection is archived, all of it's cards are also marked as archived, but this is down in the model layer
@@ -876,12 +844,12 @@
(api/defendpoint-schema PUT "/:id"
"Modify an existing Collection, including archiving or unarchiving it, or moving it."
[id, :as {{:keys [name color description archived parent_id authority_level], :as collection-updates} :body}]
- {name (s/maybe su/NonBlankString)
- color (s/maybe collection/hex-color-regex)
- description (s/maybe su/NonBlankString)
- archived (s/maybe s/Bool)
- parent_id (s/maybe su/IntGreaterThanZero)
- authority_level collection/AuthorityLevel}
+ {name (s/maybe su/NonBlankString)
+ color (s/maybe collection/hex-color-regex)
+ description (s/maybe su/NonBlankString)
+ archived (s/maybe s/Bool)
+ parent_id (s/maybe su/IntGreaterThanZero)
+ authority_level collection/AuthorityLevel}
;; do we have perms to edit this Collection?
(let [collection-before-update (api/write-check Collection id)]
;; if we're trying to *archive* the Collection, make sure we're allowed to do that
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index 9d689a05ed1..ff98db02236 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -29,7 +29,6 @@
[metabase.models.permissions-group :as perms-group]
[metabase.models.revision :as revision]
[metabase.test :as mt]
- [metabase.test.data.users :as test.users]
[metabase.test.fixtures :as fixtures]
[metabase.util :as u]
[metabase.util.schema :as su]
@@ -95,33 +94,16 @@
(filter :personal_owner_id)
(map :name))))
- (testing "...even if we are *admins*"
- (is (= ["Crowberto Corv's Personal Collection"]
+ (testing "...unless we are *admins*"
+ (is (= ["Crowberto Corv's Personal Collection"
+ "Lucky Pigeon's Personal Collection"
+ "Rasta Toucan's Personal Collection"
+ "Trash Bird's Personal Collection"]
(->> (mt/user-http-request :crowberto :get 200 "collection")
(filter #((set (map mt/user->id [:crowberto :lucky :rasta :trashbird])) (:personal_owner_id %)))
(map :name)
sort)))))
- (testing "You should only see your collection and public collections"
- (let [user-id (u/the-id (test.users/fetch-user :crowberto))
- crowberto-root (db/select-one Collection :personal_owner_id user-id)]
- (mt/with-temp* [Collection [collection]
- Collection [{collection-id :id} {:name "Collection with Items"}]
- Collection [_ {:name "subcollection"
- :location (format "/%d/" collection-id)
- :authority_level "official"}]
- Collection [_ {:name "Crowberto's Child Collection"
- :location (collection/location-path crowberto-root)}]]
- (let [public-collections #{"Our analytics" (:name collection) "Collection with Items" "subcollection"}
- crowbertos (set (map :name (mt/user-http-request :crowberto :get 200 "collection")))
- luckys (set (map :name (mt/user-http-request :lucky :get 200 "collection")))]
- (is (= (into public-collections #{"Crowberto Corv's Personal Collection" "Crowberto's Child Collection"})
- crowbertos))
- (is (false? (contains? crowbertos "Lucky Pigeon's Personal Collection")))
- (is (= (conj public-collections (:name collection) "Lucky Pigeon's Personal Collection")
- luckys))
- (is (false? (contains? luckys "Crowberto Corv's Personal Collection")))))))
-
(testing "Personal Collection's name and slug should be returned in user's locale"
(with-french-user-and-personal-collection user _collection
(is (= [{:name "Collection personnelle de Taco Bell"
--
GitLab