Skip to content
Snippets Groups Projects
Unverified Commit efba1951 authored by Chris Truter's avatar Chris Truter Committed by GitHub
Browse files

Fix query type and respect user parameters in search (#47631)

parent 73ac5433
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/search.clj
+ 2
− 0
View file @ efba1951
......@@ -28,6 +28,7 @@
:created-at created_at
:created-by (set (u/one-or-many created_by))
:current-user-id api/*current-user-id*
:is-superuser? api/*is-superuser?*
:current-user-perms @api/*current-user-permissions-set*
:filter-items-in-personal-collection filter_items_in_personal_collection
:last-edited-at last_edited_at
......@@ -85,6 +86,7 @@
:created-at created_at
:created-by (set created_by)
:current-user-id api/*current-user-id*
:is-superuser? api/*is-superuser?*
:current-user-perms @api/*current-user-permissions-set*
:filter-items-in-personal-collection filter_items_in_personal_collection
:last-edited-at last_edited_at
......
src/metabase/models/collection.clj
+ 30
− 20
View file @ efba1951
......@@ -488,6 +488,11 @@
[:permission-level {:optional true} [:enum :read :write]]
[:effective-child-of {:optional true} [:maybe CollectionWithLocationAndIDOrRoot]]])
(def ^:private UserScope
[:map
[:current-user-id pos-int?]
[:is-superuser? :boolean]])
(def ^:private default-visibility-config
{:include-archived-items :exclude
:include-trash-collection? false
......@@ -544,6 +549,12 @@
(visible-collection-filter-clause collection-id-field {}))
([collection-id-field :- [:or [:tuple [:= :coalesce] :keyword :keyword] :keyword]
visibility-config :- CollectionVisibilityConfig]
(visible-collection-filter-clause collection-id-field visibility-config
{:current-user-id api/*current-user-id*
:is-superuser? api/*is-superuser?*}))
([collection-id-field :- [:or [:tuple [:= :coalesce] :keyword :keyword] :keyword]
visibility-config :- CollectionVisibilityConfig
{:keys [current-user-id is-superuser?]} :- 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
......@@ -569,27 +580,26 @@
;; the `FROM` clause is where we limit the collections to the ones we have permissions on. For a superuser,
;; that's all of them. For regular users, it's a) the collections they have permission in the DB for, and b)
;; their personal collection and its descendants.
:from [[{:union-all (if api/*is-superuser?*
[{:select [:c.*]
:from [[:collection :c]]}]
[{:select [:c.*]
:from [[:collection :c]]
:join [[:permissions :p]
[:= :c.id :p.collection_id]
[: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*]
[:= :p.perm_type "perms/collection-access"]
[:or
[:= :p.perm_value "read-and-write"]
(when (= :read (:permission-level visibility-config))
[:= :p.perm_value "read"])]]}
(when-let [personal-collection-and-descendant-ids (user->personal-collection-and-descendant-ids api/*current-user-id*)]
:from [(if is-superuser?
[:collection :c]
[{:union-all [{:select [:c.*]
:from [[:collection :c]]
:join [[:permissions :p]
[:= :c.id :p.collection_id]
[: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*]
[:= :p.perm_type "perms/collection-access"]
[:or
[:= :p.perm_value "read-and-write"]
(when (= :read (:permission-level visibility-config))
[:= :p.perm_value "read"])]]}
(when-let [personal-collection-and-descendant-ids (user->personal-collection-and-descendant-ids current-user-id)]
{:select [:c.*]
:from [[:collection :c]]
:where [:in :id personal-collection-and-descendant-ids]})])}
:c]]
:from [[:collection :c]]
:where [:in :id personal-collection-and-descendant-ids]})]}
:c])]
;; The `WHERE` clause is where we apply the other criteria we were given:
:where [:and
;; hiding the trash collection when desired...
......
src/metabase/search/config.clj
+ 1
− 0
View file @ efba1951
......@@ -106,6 +106,7 @@
;;
[:archived? [:maybe :boolean]]
[:current-user-id pos-int?]
[:is-superuser? :boolean]
[:current-user-perms [:set perms.u/PathSchema]]
[:model-ancestors? :boolean]
[:models [:set SearchableModel]]
......
src/metabase/search/impl.clj
+ 12
− 6
View file @ efba1951
......@@ -95,7 +95,7 @@
(mu/defn- base-query-for-model :- [:map {:closed true}
[:select :any]
[:from :any]
[:where :any]
[:where {:optional true} :any]
[:join {:optional true} :any]
[:left-join {:optional true} :any]]
"Create a HoneySQL query map with `:select`, `:from`, and `:where` clauses for `model`, suitable for the `UNION ALL`
......@@ -111,7 +111,9 @@
[honeysql-query :- ms/Map
model :- :string
{:keys [filter-items-in-personal-collection
archived]} :- SearchContext]
archived
current-user-id
is-superuser?]} :- SearchContext]
(let [collection-id-col (if (= model "collection")
:collection.id
:collection_id)
......@@ -121,7 +123,9 @@
:include-trash-collection? true
:permission-level (if archived
:write
:read)})]
: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))
......@@ -238,8 +242,7 @@
[:= :model.id :action.model_id])
(sql.helpers/left-join :query_action
[:= :query_action.action_id :action.id])
(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]
......@@ -604,6 +607,7 @@
[:search-string [:maybe ms/NonBlankString]]
[:models [:maybe [:set SearchableModel]]]
[:current-user-id pos-int?]
[:is-superuser? :boolean]
[:current-user-perms [:set perms.u/PathSchema]]
[:archived {:optional true} [:maybe :boolean]]
[:created-at {:optional true} [:maybe ms/NonBlankString]]
......@@ -625,6 +629,7 @@
created-at
created-by
current-user-id
is-superuser?
current-user-perms
last-edited-at
last-edited-by
......@@ -637,7 +642,7 @@
table-db-id
search-native-query
verified
ids]} :- ::search-context.input]
ids]} :- ::search-context.input]
;; for prod where Malli is disabled
{:pre [(pos-int? current-user-id) (set? current-user-perms)]}
(when (some? verified)
......@@ -647,6 +652,7 @@
(let [models (if (string? models) [models] models)
ctx (cond-> {:archived? (boolean archived)
:current-user-id current-user-id
:is-superuser? is-superuser?
:current-user-perms current-user-perms
:model-ancestors? (boolean model-ancestors?)
:models models
......
test/metabase/search/filter_test.clj
+ 1
− 0
View file @ efba1951
......@@ -12,6 +12,7 @@
:models search.config/all-models
:model-ancestors? false
:current-user-id 1
:is-superuser? true
:current-user-perms #{"/"}})
(deftest ^:parallel ->applicable-models-test
......
test/metabase/search/impl_test.clj
+ 3
− 0
View file @ efba1951
......@@ -63,6 +63,7 @@
:archived? false
:models search.config/all-models
:current-user-id (mt/user->id :crowberto)
:is-superuser? true
:current-user-perms #{"/"}
:model-ancestors? false
:limit-int 100}))]
......@@ -137,6 +138,7 @@
:models search.config/all-models
:created-at created-at
:current-user-id (mt/user->id :crowberto)
:is-superuser? true
:current-user-perms @api/*current-user-permissions-set*}))
:data
(map (juxt :model :id))
......@@ -222,6 +224,7 @@
:models search.config/all-models
:last-edited-at last-edited-at
:current-user-id (mt/user->id :crowberto)
:is-superuser? true
:current-user-perms @api/*current-user-permissions-set*}))
:data
(map (juxt :model :id))
......
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