Skip to content
Snippets Groups Projects
Unverified Commit dca5f441 authored by Mark Bastian's avatar Mark Bastian Committed by GitHub
Browse files

Added logic to correctly return collections for only the current user (#27962) 

Added logic to select only collections for a user that are public or their own. Note that original CTE logic was rewritten as inline queries as MySQL5.7 doesn't support CTEs.
parent c46337ad
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/collection.clj
+ 151
− 119
View file @ dca5f441
......@@ -37,13 +37,42 @@
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db]
[toucan.hydrate :refer [hydrate]]))
[toucan.hydrate :refer [hydrate]]
[toucan.models :as models]))
;;; 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
......@@ -54,29 +83,30 @@
[archived namespace]
{archived (s/maybe su/BooleanString)
namespace (s/maybe su/NonBlankString)}
(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)))))
(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)))))
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema GET "/tree"
......@@ -110,14 +140,16 @@
(mdb.query/reducible-query {:select-distinct [:collection_id :dataset]
:from [:report_card]
:where [:= :archived false]}))
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*))]})
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 (map collection/personal-collection-with-ui-details colls)]
(collection/collections->tree coll-type-ids colls)))
......@@ -185,8 +217,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)))
......@@ -217,10 +249,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))))
......@@ -407,15 +439,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))))
......@@ -431,11 +463,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
......@@ -451,8 +483,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
......@@ -470,12 +502,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.
......@@ -534,56 +566,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)))
......@@ -600,14 +632,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)))
......@@ -615,14 +647,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))
......@@ -744,12 +776,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 ------------------------------------------
......@@ -761,7 +793,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."
......@@ -773,11 +805,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))}))))
......@@ -814,7 +846,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)))))
......@@ -828,7 +860,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
......@@ -844,12 +876,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
......
test/metabase/api/collection_test.clj
+ 23
− 5
View file @ dca5f441
......@@ -29,6 +29,7 @@
[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]
......@@ -94,16 +95,33 @@
(filter :personal_owner_id)
(map :name))))
(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"]
(testing "...even if we are *admins*"
(is (= ["Crowberto Corv'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"
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment