diff --git a/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions.clj b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions.clj index ab3b5136cb35839d3b98720dd0aab91038954952..3335141198bdee444d7c58908d79d0260796f59d 100644 --- a/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions.clj +++ b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions.clj @@ -198,3 +198,17 @@ :no (revoke-permissions! :details :yes group-id db-id))) + +(s/defn update-db-execute-permissions! + "Update the DB details permissions for a database." + [group-id :- su/IntGreaterThanZero db-id :- su/IntGreaterThanZero new-perms :- perms/DetailsPermissions] + (when-not (premium-features/enable-advanced-permissions?) + (throw (perms/ee-permissions-exception :execute))) + (case new-perms + :all + (do + (revoke-permissions! :execute :all group-id db-id) + (grant-permissions! :execute :all group-id db-id)) + + :none + (revoke-permissions! :execute :all group-id db-id))) diff --git a/src/metabase/actions.clj b/src/metabase/actions.clj index 30eb468357fd28314b2b6b5d2bd2ed7858feae20..3a3336328a88017de0dbce1bd7f6f9dbe7d04548 100644 --- a/src/metabase/actions.clj +++ b/src/metabase/actions.clj @@ -168,11 +168,6 @@ (when-not (database-enable-actions) (throw (ex-info (i18n/tru "Actions are not enabled for Database {0}." database-id) {:status-code 400}))) - ;; TODO -- need to check permissions once we have Actions-specific perms in place. For now just make sure the - ;; current User is an admin. This check is only done if [[api/*current-user*]] is bound (which will always be - ;; the case when invoked from an API endpoint) to make Actions testable separately from the API endpoints. - (when api/*current-user* - (api/check-superuser)) ;; Ok, now we can hand off to [[perform-action!*]] (perform-action!* driver action db arg-map))))) diff --git a/src/metabase/actions/execution.clj b/src/metabase/actions/execution.clj index 018f7f4c44fb6826857f0830a4f050ee8c0194c9..58da59969822a6d66a333119f309841fb48f6c2f 100644 --- a/src/metabase/actions/execution.clj +++ b/src/metabase/actions/execution.clj @@ -9,6 +9,7 @@ [metabase.models :refer [Dashboard DashboardCard]] [metabase.models.action :as action] [metabase.query-processor.error-type :as qp.error-type] + [metabase.query-processor.middleware.permissions :as qp.perms] [metabase.query-processor.writeback :as qp.writeback] [metabase.util :as u] [metabase.util.i18n :refer [tru]] @@ -51,7 +52,6 @@ (assoc parameter :target target))) parameters))) - (defn- execute-query-action! "Execute a `QueryAction` with parameters as passed in from an endpoint (see [[map-parameters]] for a description of their shape). @@ -70,12 +70,15 @@ (try (let [query (assoc (:dataset_query card) :parameters parameters)] (log/debugf "Query (before preprocessing):\n\n%s" (u/pprint-to-str query)) - (qp.writeback/execute-write-query! query)) + (binding [qp.perms/*card-id* (:id card)] + (qp.writeback/execute-write-query! query))) (catch Throwable e - (throw (ex-info (tru "Error executing Action: {0}" (ex-message e)) - {:action action - :parameters parameters} - e))))) + (if (= (:type (u/all-ex-data e)) qp.error-type/missing-required-permissions) + (api/throw-403 e) + (throw (ex-info (tru "Error executing Action: {0}" (ex-message e)) + {:action action + :parameters parameters} + e)))))) (defn- execute-http-action! [action parameters] @@ -89,7 +92,6 @@ See [[map-parameters]] for a description of their expected shapes." [dashboard-id dashcard-id unmapped-parameters extra-parameters] (actions/check-actions-enabled) - (api/check-superuser) (api/read-check Dashboard dashboard-id) (let [dashcard (api/check-404 (db/select-one DashboardCard :id dashcard-id diff --git a/src/metabase/api/action.clj b/src/metabase/api/action.clj index 5da7f5bc738e7ae5943400257d6c8e94bedf1537..b25dd82483250a49684eacfcc1bcef77c2f6c372 100644 --- a/src/metabase/api/action.clj +++ b/src/metabase/api/action.clj @@ -109,4 +109,4 @@ (db/update! HTTPAction id action) (first (hydrate (action/select-actions :id id) :action/emitter-usages))) -(api/define-routes actions/+check-actions-enabled api/+check-superuser) +(api/define-routes actions/+check-actions-enabled) diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj index 038d32e5596117e7343e752ed6677eeb10da6c6f..44516dbe28237ec112d874383fb0c58371c643c2 100644 --- a/src/metabase/api/card.clj +++ b/src/metabase/api/card.clj @@ -261,8 +261,6 @@ after (:is_write card-updates)] (log/tracef "is_write value will change from %s => %s" (pr-str before) (pr-str after)) (when-not (= before after) - ;; make sure current User is a superuser - (api/check-superuser) (try ;; make sure Card is not a Dataset (when (:dataset (merge card-updates card-before-update)) diff --git a/src/metabase/api/permission_graph.clj b/src/metabase/api/permission_graph.clj index 936780cdd144fd0c41984fbeae1ac70fba0e6819..bbb4bd8e9d2068ca3afda95df8c19f8338e20ae1 100644 --- a/src/metabase/api/permission_graph.clj +++ b/src/metabase/api/permission_graph.clj @@ -84,10 +84,20 @@ ;; language used on the frontend. (s/def ::details (s/or :str->kw #{"yes" "no"})) -(s/def ::db-perms (s/keys :opt-un [::data ::download ::data-model ::details])) - -(s/def ::db-graph (s/map-of ::id ::db-perms - :conform-keys true)) +(s/def ::execute (s/or :str->kw #{"all" "none"})) + +(s/def ::db-perms (s/keys :opt-un [::data ::download ::data-model ::details ::execute])) + +(s/def ::db-graph + (s/and (s/map-of (s/or :global-execute #{:execute} + :db-perms ::id) + (s/or :global-execute ::execute + :db-perms ::db-perms) + :conform-keys true) + (s/conformer (fn [m] + (if (every? (fn [[[key-tag] [value-tag]]] (= key-tag value-tag)) m) + (into {} (map (partial mapv second)) m) + :clojure.spec.alpha/invalid))))) (s/def :metabase.api.permission-graph.data/groups (s/map-of ::id ::db-graph diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj index 1b3aac9d16cfec9ffdd57c7d85bc28fd48e03d28..311a66427c56b5357bf09015afc8c943aba77802 100644 --- a/src/metabase/models/permissions.clj +++ b/src/metabase/models/permissions.clj @@ -264,6 +264,10 @@ ;; any path starting with /details/ is a DATABASE CONNECTION DETAILS permissions path ;; /details/db/:id/ -> permissions to edit the connection details and settings for the DB (and "details/" #"db/\d+/") + ;; .../execute/ -> permissions to run query actions in the DB + (and "execute/" + (or "" + #"db/\d+/")) ;; any path starting with /collection/ is a COLLECTION permissions path (and "collection/" (or @@ -439,6 +443,12 @@ ([database-or-id schema-name table-or-id] (str (data-perms-path (u/the-id database-or-id) schema-name (u/the-id table-or-id)) "query/segmented/"))) +(s/defn execute-query-perms-path :- Path + "Return the execute query action permissions path for a database. + This grants you permissions to run arbitary query actions." + [database-or-id :- MapOrID] + (str "/execute" (data-perms-path database-or-id))) + (s/defn database-block-perms-path :- Path "Return the permissions path for the Block 'anti-permissions'. Block anti-permissions means a User cannot run a query against a Database unless they have data permissions, regardless of whether segmented permissions would normally give @@ -462,7 +472,10 @@ (str "/data-model" base-path) [:details :yes] - (str "/details" base-path))) + (str "/details" base-path) + + [:execute :all] + (str "/execute" base-path))) (s/defn feature-perms-path :- Path "Returns the permissions path to use for a given feature-level permission type (e.g. download) and value (e.g. full @@ -652,6 +665,11 @@ (s/enum :write :none) "Valid native perms option for a database")) +(def ^:private ExecutePermissionsGraph + (s/named + (s/enum :all :none) + "Valid execute perms option for a database")) + (def ^:private DataPermissionsGraph (s/named {(s/optional-key :native) NativePermissionsGraph @@ -731,10 +749,12 @@ "Valid details perms graph for a database")) (def ^:private StrictDBPermissionsGraph - {su/IntGreaterThanZero {(s/optional-key :data) StrictDataPermissionsGraph + {(s/optional-key :execute) ExecutePermissionsGraph + su/IntGreaterThanZero {(s/optional-key :data) StrictDataPermissionsGraph (s/optional-key :download) DownloadPermissionsGraph (s/optional-key :data-model) DataModelPermissionsGraph - (s/optional-key :details) DetailsPermissions}}) + (s/optional-key :details) DetailsPermissions + (s/optional-key :execute) ExecutePermissionsGraph}}) (def ^:private StrictPermissionsGraph {:revision s/Int @@ -749,12 +769,14 @@ "Handle '/' permission" [db-ids] (reduce (fn [g db-id] - (assoc g db-id {:data {:native :write - :schemas :all} - :download {:native :full - :schemas :full} - :data-model {:schemas :all} - :details :yes})) + (assoc g + db-id {:data {:native :write + :schemas :all} + :download {:native :full + :schemas :full} + :data-model {:schemas :all} + :details :yes} + :execute :all)) {} db-ids)) @@ -764,7 +786,7 @@ [] (let [permissions (db/select [Permissions [:group_id :group-id] [:object :path]] {:where [:or - [:= :object (hx/literal "/")] + [:in :object [(hx/literal "/") (hx/literal "/execute/")]] [:like :object (hx/literal "%/db/%")]]}) db-ids (delay (db/select-ids 'Database)) group-id->paths (reduce @@ -775,9 +797,11 @@ group-id->graph (m/map-vals (fn [paths] (let [permissions-graph (perms-parse/permissions->graph paths)] - (if (= :all permissions-graph) + (if (= permissions-graph :all) (all-permissions @db-ids) - (:db permissions-graph)))) + (cond-> (:db permissions-graph) + (= (get permissions-graph :execute) :all) + (assoc :execute :all))))) group-id->paths)] {:revision (perms-revision/latest-id) :groups group-id->graph})) @@ -1132,22 +1156,38 @@ (update-fn group-id db-id new-perms) (throw (ee-permissions-exception perm-type)))) +(defn update-global-execution-permission + "Set the global execution permission (\"/execute/\") for the group + with ID `group-id` to `new-perms`." + [group-id new-perms] + (when-not (or (= group-id (:id (perms-group/all-users))) + (premium-features/has-feature? :advanced-permissions)) + (throw (ee-permissions-exception :execute))) + (delete-related-permissions! group-id "/execute/") + (when (= new-perms :all) + (grant-permissions! group-id "/execute/"))) + (s/defn ^:private update-group-permissions! [group-id :- su/IntGreaterThanZero new-group-perms :- StrictDBPermissionsGraph] (doseq [[db-id new-db-perms] new-group-perms] - (doseq [[perm-type new-perms] new-db-perms] - (case perm-type - :data - (update-db-data-access-permissions! group-id db-id new-perms) + (if (= db-id :execute) + (update-global-execution-permission group-id new-db-perms) + (doseq [[perm-type new-perms] new-db-perms] + (case perm-type + :data + (update-db-data-access-permissions! group-id db-id new-perms) + + :download + (update-feature-level-permission! group-id db-id new-perms :download) - :download - (update-feature-level-permission! group-id db-id new-perms :download) + :data-model + (update-feature-level-permission! group-id db-id new-perms :data-model) - :data-model - (update-feature-level-permission! group-id db-id new-perms :data-model) + :details + (update-feature-level-permission! group-id db-id new-perms :details) - :details - (update-feature-level-permission! group-id db-id new-perms :details))))) + :execute + (update-feature-level-permission! group-id db-id new-perms :execute)))))) (defn check-revision-numbers "Check that the revision number coming in as part of `new-graph` matches the one from `old-graph`. This way we can diff --git a/src/metabase/models/permissions/parse.clj b/src/metabase/models/permissions/parse.clj index 4bc9cdd88d590f519db768f52f8e35e984dfb50c..73b2d57ba273a56d1f345e01feb589b502eed3cc 100644 --- a/src/metabase/models/permissions/parse.clj +++ b/src/metabase/models/permissions/parse.clj @@ -13,9 +13,10 @@ (def ^:private grammar "Describes permission strings like /db/3/ or /collection/root/read/" - "permission = ( all | db | block | download | data-model | details | collection ) + "permission = ( all | <'/'> execute | db | block | download | data-model | details | collection ) all = <'/'> - db = <'/db/'> #'\\d+' <'/'> ( native | schemas )? + db = <'/db/'> #'\\d+' <'/'> ( native | execute | schemas )? + execute = <'execute/'> native = <'native/'> schemas = <'schema/'> schema? schema = schema-name <'/'> table? @@ -89,6 +90,7 @@ "read" [:read :all] "query" [:query :all] "query/segmented" [:query :segmented]) + [:execute] [:execute :all] [:native] [:data :native :write] ;; block perms. Parse something like /block/db/1/ to {:db {1 {:schemas :block}}} [:block db-id] [:db (Long/parseUnsignedLong db-id) :data :schemas :block] diff --git a/src/metabase/query_processor/middleware/permissions.clj b/src/metabase/query_processor/middleware/permissions.clj index 4d541431a007071b206c61646b888016c90a0f0a..51c366fcc611ce36f8b1dc4bf9aab45758589d27 100644 --- a/src/metabase/query_processor/middleware/permissions.clj +++ b/src/metabase/query_processor/middleware/permissions.clj @@ -115,6 +115,32 @@ [query] (dissoc query ::perms)) +;;; +----------------------------------------------------------------------------------------------------------------+ +;;; | Writeback fns | +;;; +----------------------------------------------------------------------------------------------------------------+ + +(defn- query-action-perms + [{:keys [database]}] + #{(perms/execute-query-perms-path database)}) + +(s/defn ^:private 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] + (when-not *current-user-id* + (throw (ex-info (tru "Query actions have to executed by a user.") {}))) + (log/tracef "Checking query permissions. Current user perms set = %s" (pr-str @*current-user-permissions-set*)) + (check-card-read-perms *card-id*) + (when-not (has-data-perms? (query-action-perms outer-query)) + (throw (perms-exception required-perms)))) + +(defn check-query-action-permissions + "Middleware that check that the current user has permissions to run the current query action. This only applies if + `*current-user-id*` is bound." + [qp] + (fn [query rff context] + (check-query-action-permissions* query) + (qp query rff context))) + ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | Non-middleware util fns | diff --git a/src/metabase/query_processor/writeback.clj b/src/metabase/query_processor/writeback.clj index 15167e1d80228b5ea44947bf451e81c180001942..b912e9c56bd9f362e9010e6585a939813da2e5f0 100644 --- a/src/metabase/query_processor/writeback.clj +++ b/src/metabase/query_processor/writeback.clj @@ -2,15 +2,23 @@ "Code for executing writeback queries." (:require [clojure.tools.logging :as log] + [metabase.api.common :as api] [metabase.driver :as driver] [metabase.query-processor :as qp] [metabase.query-processor.error-type :as qp.error-type] [metabase.query-processor.middleware.parameters :as parameters] + [metabase.query-processor.middleware.permissions :as qp.perms] [metabase.util :as u] [metabase.util.i18n :refer [tru]] [metabase.util.schema :as su] [schema.core :as s])) +(def ^:private execution-middleware + "Middleware that happens after compilation, AROUND query execution itself. Has the form + + (f (f query rff context)) -> (f query rff context)" + [#'qp.perms/check-query-action-permissions]) + (defn- map-parameters "Take the `parameters` map passed in to an endpoint like `POST /api/emitter/:id/execute` and map it to the parameters in the underlying `Action` so they can be attached to the query that gets passed to [[qp/execute-write-query!]]. @@ -62,7 +70,7 @@ ;; ok, now execute the query. (log/debugf "Executing query\n\n%s" (u/pprint-to-str query)) (driver/execute-write-query! driver/*driver* query)))] - (apply-middleware qp* qp/around-middleware))) + (apply-middleware qp* (concat execution-middleware qp/around-middleware)))) (defn execute-write-query! "Execute an writeback query from an `is_write` SavedQuestion." @@ -104,9 +112,12 @@ (let [parameters (map-parameters parameters (:parameter_mappings emitter)) query (assoc (:dataset_query card) :parameters parameters)] (log/debugf "Query (before preprocessing):\n\n%s" (u/pprint-to-str query)) - (execute-write-query! query)) + (binding [qp.perms/*card-id* (:id card)] + (execute-write-query! query))) (catch Throwable e - (throw (ex-info (tru "Error executing QueryEmitter: {0}" (ex-message e)) - {:emitter emitter - :parameters parameters} - e))))) + (if (= (:type (u/all-ex-data e)) qp.error-type/missing-required-permissions) + (api/throw-403 e) + (throw (ex-info (tru "Error executing QueryEmitter: {0}" (ex-message e)) + {:emitter emitter + :parameters parameters} + e)))))) diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj index 4b2e14438d289ddd0e0f33378d35e99ea909ea19..903618b4cc2119d20789849f74e44d6fdb84c978 100644 --- a/test/metabase/api/card_test.clj +++ b/test/metabase/api/card_test.clj @@ -2360,16 +2360,6 @@ (is (= {:errors {:is_write "Cannot mark Saved Question as 'is_write': Saved Question is a Dataset."}} result)))}))) -(deftest set-is-write-user-is-not-admin-test - (with-actions-enabled - (doseq [f [test-update-is-write-card - test-create-is-write-card]] - (f {:status-code 403 - :user :rasta - :result-fn (fn [result _] - (is (= "You don't have permissions to do that." - result)))})))) - (deftest set-is-write-card-query-is-not-native-query-test (with-actions-enabled (doseq [f [test-update-is-write-card diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj index 470a90002816cfc67675d8711c9229d30d98523f..80260d1901f04a4e9496259ed765a4a6eddf0833 100644 --- a/test/metabase/api/dashboard_test.clj +++ b/test/metabase/api/dashboard_test.clj @@ -1956,23 +1956,33 @@ "Problem building request:")))))))))) (deftest dashcard-action-execution-auth-test - (actions.test-util/with-actions-test-data - (actions.test-util/with-action [{:keys [action-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 - :parameter_mappings [{:parameter_id "my_id" - :target [:variable [:template-tag "id"]]}]}]] - (let [execute-path (format "dashboard/%s/dashcard/%s/action/execute" - dashboard-id - dashcard-id)] - (testing "Without actions enabled" - (is (= "Actions are not enabled." - (mt/user-http-request :crowberto :post 400 execute-path - {:parameters [{:id "my_id" :type :number/= :value 1}]})))) - (testing "Without admin" - (actions.test-util/with-actions-enabled - (is (= "You don't have permissions to do that." - (mt/user-http-request :rasta :post 403 execute-path - {:parameters [{:id "my_id" :type :number/= :value 1}]}))))))))))) + (mt/with-temp-copy-of-db + (actions.test-util/with-actions-test-data + (actions.test-util/with-action [{:keys [action-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 + :parameter_mappings [{:parameter_id "my_id" + :target [:variable [:template-tag "id"]]}]}]] + (let [execute-path (format "dashboard/%s/dashcard/%s/action/execute" + dashboard-id + dashcard-id)] + (testing "Without actions enabled" + (is (= "Actions are not enabled." + (mt/user-http-request :crowberto :post 400 execute-path + {:parameters [{:id "my_id" :type :number/= :value 1}]})))) + (testing "Without execute rights on the DB" + (actions.test-util/with-actions-enabled + (is (= "You don't have permissions to do that." + (mt/user-http-request :rasta :post 403 execute-path + {:parameters [{:id "my_id" :type :number/= :value 1}]}))))) + (testing "With execute rights on the DB" + (perms/update-global-execution-permission (:id (perms-group/all-users)) :all) + (try + (actions.test-util/with-actions-enabled + (is (= {:rows-affected 1} + (mt/user-http-request :rasta :post 200 execute-path + {:parameters [{:id "my_id" :type :number/= :value 1}]})))) + (finally + (perms/update-global-execution-permission (:id (perms-group/all-users)) :none))))))))))) diff --git a/test/metabase/api/permissions_test.clj b/test/metabase/api/permissions_test.clj index 0b87bfc23a4cd318c22686b8afd3af4f8c5a8250..0fb56358a6085be2d848313d90e6c2ea1598707e 100644 --- a/test/metabase/api/permissions_test.clj +++ b/test/metabase/api/permissions_test.clj @@ -157,8 +157,20 @@ [:groups (u/the-id group) db-id :data :schemas] :all)) (is (= :all - (get-in (perms/data-perms-graph) [:groups (u/the-id group) db-id :data :schemas]))))))) + (get-in (perms/data-perms-graph) [:groups (u/the-id group) db-id :data :schemas]))))) + (testing "global execute permission" + (let [group-id (:id (perms-group/all-users))] + (try + (mt/user-http-request + :crowberto :put 200 "permissions/graph" + (assoc-in (perms/data-perms-graph) + [:groups group-id :execute] + :all)) + (is (= :all + (get-in (perms/data-perms-graph) [:groups group-id :execute]))) + (finally + (perms/update-global-execution-permission group-id :none))))))) ;;; +---------------------------------------------- permissions membership apis -----------------------------------------------------------+ diff --git a/test/metabase/models/permissions_test.clj b/test/metabase/models/permissions_test.clj index 557f8787d1c834061e7cbac084e57e43f499184a..397a43958a35fa0250f2a0c617d1c590217c1123 100644 --- a/test/metabase/models/permissions_test.clj +++ b/test/metabase/models/permissions_test.clj @@ -49,6 +49,8 @@ "/data-model/db/1/schema/PUBLIC/table/1/" ;; db details permissions "/details/db/1/" + ;; execution permissions + "/execute/" ;; full admin (everything) root permissions "/"]) @@ -672,15 +674,16 @@ (testing "A \"/\" permission grants all dataset permissions" (mt/with-temp Database [{db-id :id}] (let [{:keys [group_id]} (db/select-one Permissions :object "/")] - (is (= {db-id {:data {:native :write - :schemas :all} - :download {:native :full - :schemas :full} - :data-model {:schemas :all} - :details :yes}} + (is (= {db-id {:data {:native :write + :schemas :all} + :download {:native :full + :schemas :full} + :data-model {:schemas :all} + :details :yes} + :execute :all} (-> (perms/data-perms-graph) (get-in [:groups group_id]) - (select-keys [db-id])))))))) + (select-keys [db-id :execute])))))))) (deftest update-graph-validate-db-perms-test (testing "Check that validation of DB `:schemas` and `:native` perms doesn't fail if only one of them changes"