Skip to content
Snippets Groups Projects
Unverified Commit 12c9bb30 authored by Tim Macdonald's avatar Tim Macdonald Committed by GitHub
Browse files

Disable /execute permission, ensure actions can run with standard perms (#28457) 

This'll be revisited as part of v47

[Fixes #28425]
parent 19086f1e
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/query_processor/middleware/permissions.clj
+ 2
− 2
View file @ 12c9bb30
......@@ -124,7 +124,7 @@
(defn- query-action-perms
[{:keys [database]}]
#{(perms/execute-query-perms-path 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."
......@@ -135,7 +135,7 @@
(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))))
(throw (perms-exception (required-perms 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
+ 58
− 54
View file @ 12c9bb30
......@@ -2826,13 +2826,14 @@
(:cause
(mt/user-http-request :crowberto :post 400 execute-path
{:parameters {"name" "Birds"}})))))
(testing "Without execute rights on the DB"
(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 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"
(perms/update-global-execution-permission! (:id (perms-group/all-users)) :all)
(perms/grant-full-data-permissions! (perms-group/all-users) (mt/db))
(try
(mt/with-actions-enabled
(is (contains? #{{:ID 76, :NAME "Birds"}
......@@ -2841,7 +2842,7 @@
{:parameters {"name" "Birds"}})
:created-row))))
(finally
(perms/update-global-execution-permission! (:id (perms-group/all-users)) :none)))))))))))
(perms/revoke-data-perms! (perms-group/all-users) (mt/db))))))))))))
(deftest dashcard-custom-action-execution-auth-test
(mt/with-temp-copy-of-db
......@@ -2860,20 +2861,21 @@
(:cause
(mt/user-http-request :crowberto :post 400 execute-path
{:parameters {"id" 1}})))))
(testing "Without execute rights on the DB"
(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 execute rights on the DB"
(perms/update-global-execution-permission! (:id (perms-group/all-users)) :all)
(testing "With read rights on the DB"
(perms/grant-full-data-permissions! (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/update-global-execution-permission! (:id (perms-group/all-users)) :none)))))))))))
(perms/revoke-data-perms! (perms-group/all-users) (mt/db))))))))))))
(deftest dashcard-execution-fetch-prefill-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions)
......@@ -2918,51 +2920,53 @@
(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}})))))))))))
(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!))))
;; 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!))))
(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}]]
(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"))))))))))))))
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