Skip to content
Snippets Groups Projects
Unverified Commit 4ca3dd70 authored by metabase-bot[bot]'s avatar metabase-bot[bot] Committed by GitHub
Browse files

Users can run actions if they have access to the database (#29088) (#29217) 


* Users can execute actions if they can read the model and the DB is not blocked

[Fixes #28425]

Co-authored-by: default avatarTim Macdonald <tim@metabase.com>
parent 8fe9e98e
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
+ 26
− 1
View file @ 4ca3dd70
......@@ -4,7 +4,7 @@
[clojure.core.memoize :as memoize]
[clojure.test :refer :all]
[metabase.api.database :as api.database]
[metabase.models :refer [Database Field FieldValues Permissions Table]]
[metabase.models :refer [Dashboard DashboardCard Database Field FieldValues Permissions Table]]
[metabase.models.database :as database]
[metabase.models.field :as field]
[metabase.models.permissions :as perms]
......@@ -433,3 +433,28 @@
:details :yes}}
(is (partial= {:details {}}
(mt/user-http-request :rasta :get 200 (format "database/%d?exclude_uneditable_details=true" db-id))))))))
(deftest actions-test
(mt/with-temp-copy-of-db
(mt/with-actions-test-data
(mt/with-actions [{:keys [action-id model-id]} {}]
(testing "Executing dashcard with action"
(mt/with-temp* [Dashboard [{dashboard-id :id}]
DashboardCard [{dashcard-id :id} {:dashboard_id dashboard-id
:action_id action-id
:card_id model-id}]]
(let [execute-path (format "dashboard/%s/dashcard/%s/execute"
dashboard-id
dashcard-id)]
(testing "Fails with access to the DB blocked"
(with-all-users-data-perms {(u/the-id (mt/db)) {:data {:native :none :schemas :block}
:details :yes}}
(mt/with-actions-enabled
(is (partial= {:message "You don't have permissions to do that."}
(mt/user-http-request :rasta :post 403 execute-path
{:parameters {"id" 1}}))))))
(testing "Works with access to the DB not blocked"
(mt/with-actions-enabled
(is (= {:rows-affected 1}
(mt/user-http-request :rasta :post 200 execute-path
{:parameters {"id" 1}}))))))))))))
src/metabase/query_processor/middleware/permissions.clj
+ 1
− 7
View file @ 4ca3dd70
......@@ -136,10 +136,6 @@
;;; | Writeback fns |
;;; +----------------------------------------------------------------------------------------------------------------+
(defn- query-action-perms
[{:keys [database]}]
#{(perms/data-perms-path database)})
(s/defn check-query-action-permissions*
"Check that User with `user-id` has permissions to run query action `query`, or throw an exception."
[outer-query :- su/Map]
......@@ -147,9 +143,7 @@
(when *card-id*
(check-card-read-perms *card-id*))
(when-not (has-data-perms? (required-perms outer-query))
(check-block-permissions outer-query))
(when-not (has-data-perms? (query-action-perms outer-query))
(throw (perms-exception (required-perms outer-query)))))
(check-block-permissions outer-query)))
(defn check-query-action-permissions
"Middleware that check that the current user has permissions to run the current query action."
......
test/metabase/api/dashboard_test.clj
+ 74
− 67
View file @ 4ca3dd70
......@@ -2872,20 +2872,25 @@
(:cause
(mt/user-http-request :crowberto :post 400 execute-path
{:parameters {"name" "Birds"}})))))
(testing "Without read rights on the DB"
;; Actions cannot run with access to the DB blocked, which is an enterprise feature. See tests in
;; enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj and at the bottom of
;; this file
(testing "Works without read rights on the DB (but access not blocked)"
(perms/revoke-data-perms! (perms-group/all-users) (mt/db))
(mt/with-actions-enabled
(is (partial= {:message "You do not have permissions to run this query."}
(mt/user-http-request :rasta :post 403 execute-path
{:parameters {"name" "Birds"}})))))
(testing "With execute rights on the DB"
(is (contains? #{{:ID 76, :NAME "Birds"}
{:id 76, :name "Birds"}}
(-> (mt/user-http-request :rasta :post 200 execute-path
{:parameters {"name" "Birds"}})
:created-row)))))
(testing "Works with execute rights on the DB"
(perms/grant-full-data-permissions! (perms-group/all-users) (mt/db))
(try
(mt/with-actions-enabled
(is (contains? #{{:ID 76, :NAME "Birds"}
{:id 76, :name "Birds"}}
(is (contains? #{{:ID 77, :NAME "Avians"}
{:id 77, :name "Avians"}}
(-> (mt/user-http-request :rasta :post 200 execute-path
{:parameters {"name" "Birds"}})
{:parameters {"name" "Avians"}})
:created-row))))
(finally
(perms/revoke-data-perms! (perms-group/all-users) (mt/db))))))))))))
......@@ -2902,26 +2907,29 @@
(let [execute-path (format "dashboard/%s/dashcard/%s/execute"
dashboard-id
dashcard-id)]
(testing "Without actions enabled"
(testing "Fails with actions disabled"
(is (= "Actions are not enabled."
(:cause
(mt/user-http-request :crowberto :post 400 execute-path
{:parameters {"id" 1}})))))
(testing "Without read rights on the DB"
(perms/revoke-data-perms! (perms-group/all-users) (mt/db))
(mt/with-actions-enabled
(is (partial= {:message "You don't have permissions to do that."}
(mt/user-http-request :rasta :post 403 execute-path
{:parameters {"id" 1}})))))
(testing "With read rights on the DB"
(perms/grant-full-data-permissions! (perms-group/all-users) (mt/db))
;; Actions cannot run with access to the DB blocked, which is an enterprise feature. See tests in
;; enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj and at the bottom of
;; this file.
(testing "Works with read rights on the DB"
(perms/grant-permissions-for-all-schemas! (perms-group/all-users) (mt/db))
(try
(mt/with-actions-enabled
(is (= {:rows-affected 1}
(mt/user-http-request :rasta :post 200 execute-path
{:parameters {"id" 1}}))))
(finally
(perms/revoke-data-perms! (perms-group/all-users) (mt/db))))))))))))
(perms/revoke-data-perms! (perms-group/all-users) (mt/db)))))
(testing "Works with no particular permissions"
(perms/revoke-data-perms! (perms-group/all-users) (mt/db))
(mt/with-actions-enabled
(is (= {:rows-affected 1}
(mt/user-http-request :rasta :post 200 execute-path
{:parameters {"id" 1}}))))))))))))
(deftest dashcard-execution-fetch-prefill-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions)
......@@ -2966,53 +2974,52 @@
(is (partial= {:message "No destination parameter found for #{\"price\"}. Found: #{\"id\" \"name\"}"}
(mt/user-http-request :crowberto :post 400 execute-path {:parameters {"id" 1 "name" "Blueberries" "price" 1234}})))))))))))
;; Not relevant for 46; uncomment and make work for 47
(comment
(defn- ee-features-enabled? []
(u/ignore-exceptions
(classloader/require 'metabase-enterprise.advanced-permissions.models.permissions)
(some? (resolve 'metabase-enterprise.advanced-permissions.models.permissions/update-db-execute-permissions!))))
(defn- ee-features-enabled? []
(u/ignore-exceptions
(classloader/require 'metabase-enterprise.advanced-permissions.models.permissions)
(some? (resolve 'metabase-enterprise.advanced-permissions.models.permissions/update-db-execute-permissions!))))
(deftest dashcard-action-execution-granular-auth-test
(when (ee-features-enabled?)
(mt/with-temp-copy-of-db
(mt/with-actions-test-data-and-actions-enabled
(mt/with-actions [{:keys [action-id model-id]} {}]
(testing "Executing dashcard with action"
(mt/with-temp* [Dashboard [{dashboard-id :id}]
DashboardCard [{dashcard-id :id}
{:dashboard_id dashboard-id
:action_id action-id
:card_id model-id}]]
(let [execute-path (format "dashboard/%s/dashcard/%s/execute"
dashboard-id
dashcard-id)]
(testing "with :advanced-permissions feature flag"
(premium-features-test/with-premium-features #{:advanced-permissions}
(testing "for non-magic group"
(mt/with-temp* [PermissionsGroup [{group-id :id}]
PermissionsGroupMembership [_ {:user_id (mt/user->id :rasta)
:group_id group-id}]]
(is (partial= {:message "You don't have permissions to do that."}
(mt/user-http-request :rasta :post 403 execute-path
{:parameters {"id" 1}}))
"Execution permission should be required")
(mt/user-http-request
:crowberto :put 200 "permissions/execution/graph"
(assoc-in (perms/execution-perms-graph) [:groups group-id (mt/id)] :all))
(is (= :all
(get-in (perms/execution-perms-graph) [:groups group-id (mt/id)]))
"Should be able to set execution permission")
(is (= {:rows-affected 1}
(mt/user-http-request :rasta :post 200 execute-path
{:parameters {"id" 1}}))
"Execution and data permissions should be enough")
(perms/update-data-perms-graph! [group-id (mt/id) :data]
{:schemas :block})
(perms/update-data-perms-graph! [(:id (perms-group/all-users)) (mt/id) :data]
{:schemas :block})
(is (partial= {:message "You don't have permissions to do that."}
(mt/user-http-request :rasta :post 403 execute-path
{:parameters {"id" 1}}))
"Data permissions should be required"))))))))))))))
(deftest dashcard-action-execution-granular-auth-test
(when (ee-features-enabled?)
(mt/with-temp-copy-of-db
(mt/with-actions-test-data-and-actions-enabled
(mt/with-actions [{:keys [action-id model-id]} {}]
(testing "Executing dashcard with action"
(mt/with-temp* [Dashboard [{dashboard-id :id}]
DashboardCard [{dashcard-id :id}
{:dashboard_id dashboard-id
:action_id action-id
:card_id model-id}]]
(let [execute-path (format "dashboard/%s/dashcard/%s/execute"
dashboard-id
dashcard-id)]
(testing "with :advanced-permissions feature flag"
(premium-features-test/with-premium-features #{:advanced-permissions}
(testing "for non-magic group"
(mt/with-temp* [PermissionsGroup [{group-id :id}]
PermissionsGroupMembership [_ {:user_id (mt/user->id :rasta)
:group_id group-id}]]
(comment ;; We do not currently support /execute/ permission
(is (partial= {:message "You don't have permissions to do that."}
(mt/user-http-request :rasta :post 403 execute-path
{:parameters {"id" 1}}))
"Execution permission should be required"))
(mt/user-http-request
:crowberto :put 200 "permissions/execution/graph"
(assoc-in (perms/execution-perms-graph) [:groups group-id (mt/id)] :all))
(is (= :all
(get-in (perms/execution-perms-graph) [:groups group-id (mt/id)]))
"Should be able to set execution permission")
(is (= {:rows-affected 1}
(mt/user-http-request :rasta :post 200 execute-path
{:parameters {"id" 1}}))
"Execution and data permissions should be enough")
(perms/update-data-perms-graph! [group-id (mt/id) :data]
{:schemas :block})
(perms/update-data-perms-graph! [(:id (perms-group/all-users)) (mt/id) :data]
{:schemas :block})
(is (partial= {:message "You don't have permissions to do that."}
(mt/user-http-request :rasta :post 403 execute-path
{:parameters {"id" 1}}))
"Data permissions should be required")))))))))))))
test/metabase/query_processor/middleware/permissions_test.clj
+ 25
− 0
View file @ 4ca3dd70
......@@ -211,6 +211,31 @@
{:id tag-name, :name tag-name, :display-name tag-name,
:type "card", :card card-id}}}})))))))
(deftest query-action-permissions-test
(testing "Query action permissions"
(mt/with-non-admin-groups-no-root-collection-perms
(mt/with-temp-copy-of-db
(perms/revoke-data-perms! (perms-group/all-users) (mt/id))
(let [query (mt/mbql-query venues {:order-by [[:asc $id]], :limit 2})
check! qp.perms/check-query-action-permissions*]
(mt/with-temp Collection [collection]
(mt/with-temp Card [{model-id :id} {:collection_id (u/the-id collection)
:dataset_query query}]
(testing "are granted by default"
(check! query))
(testing "are revoked without access to the model"
(binding [qp.perms/*card-id* model-id]
(is (thrown-with-msg?
ExceptionInfo
(re-pattern #"You do not have permissions to view Card [\d,]+")
(check! query)))))
;; Are revoked with DB access blocked: requires EE, see test in
;; enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
(testing "are granted with access to the model"
(binding [api/*current-user-permissions-set* (delay #{(perms/collection-read-path (u/the-id collection))})
qp.perms/*card-id* model-id]
(check! query))))))))))
(deftest end-to-end-test
(testing (str "Make sure it works end-to-end: make sure bound `*current-user-id*` and `*current-user-permissions-set*` "
"are used to permissions check queries")
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment