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 2a7538bf1cf8f4817cb8a2c27ab78bf2cbbc0581..ee3c341599dae44ee5a9f14846d29de42143e887 100644
--- a/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions.clj
+++ b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions.clj
@@ -122,6 +122,11 @@
;;; | Data model permissions |
;;; +----------------------------------------------------------------------------------------------------------------+
+(defn data-model-write-perms-path
+ "Returns the permissions path required to edit the data model for a table specified by `path-components`."
+ [& path-components]
+ (apply (partial perms/feature-perms-path :data-model :all) path-components))
+
(defn- update-table-data-model-permissions!
[group-id db-id schema table-id new-table-perms]
(condp = new-table-perms
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
index 3dfdc9cf30163f51b459ffaa11099f9ce916290c..78846d7ceeb5975c62e4a1f638d8e304ba41e005 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
@@ -1,8 +1,10 @@
(ns metabase-enterprise.advanced-permissions.common-test
- (:require [clojure.test :refer :all]
+ (:require [cheshire.core :as json]
+ [clojure.test :refer :all]
[metabase-enterprise.advanced-permissions.models.permissions :as ee-perms]
- [metabase.models :refer [Permissions]]
+ [metabase.models :refer [Field Permissions Table]]
[metabase.models.database :as database]
+ [metabase.models.permissions :as perms]
[metabase.models.permissions-group :as group]
[metabase.public-settings.premium-features-test :as premium-features-test]
[metabase.test :as mt]
@@ -40,20 +42,138 @@
(is (partial= {:can_access_data_model true}
(user-permissions :rasta)))))))))
-(deftest fetch-database-metadata-exclude-uneditable-test
- (testing "GET /api/database/:id/metadata?exclude_uneditable=true"
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Data model permission enforcement |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- do-with-all-user-data-perms
+ [graph f]
+ (let [all-users-group-id (u/the-id (group/all-users))]
(premium-features-test/with-premium-features #{:advanced-permissions}
(mt/with-model-cleanup [Permissions]
- (let [[id-1 id-2 id-3 id-4] (map u/the-id (database/tables (mt/db)))]
- (ee-perms/update-db-data-model-permissions! (u/the-id (group/all-users))
- (mt/id)
- {:schemas {"PUBLIC" {id-1 :all
- id-2 :none
- id-3 :none
- id-4 :none}}})
- (let [tables (->> (mt/user-http-request :rasta
- :get
- 200
- (format "database/%d/metadata?exclude_uneditable=true" (mt/id)))
- :tables)]
- (is (= [id-1] (map :id tables)))))))))
+ (@#'perms/update-group-permissions! all-users-group-id graph)
+ (f)))))
+
+(defmacro ^:private with-all-users-data-perms
+ "Runs `f` with perms for the All Users group temporarily set to the values in `graph`"
+ [graph & body]
+ `(do-with-all-user-data-perms ~graph (fn [] ~@body)))
+
+(deftest fetch-database-metadata-exclude-uneditable-test
+ (testing "GET /api/database/:id/metadata?exclude_uneditable=true"
+ (let [[id-1 id-2 id-3 id-4] (map u/the-id (database/tables (mt/db)))]
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {id-1 :all
+ id-2 :none
+ id-3 :none
+ id-4 :none}}}}}
+ (let [tables (->> (mt/user-http-request :rasta
+ :get
+ 200
+ (format "database/%d/metadata?exclude_uneditable=true" (mt/id)))
+ :tables)]
+ (is (= [id-1] (map :id tables))))))))
+
+(deftest update-field-test
+ (mt/with-temp Field [{field-id :id, table-id :table_id} {:name "Field Test"}]
+ (let [{table-id :id, schema :schema, db-id :db_id} (Table table-id)]
+ (testing "PUT /api/field/:id"
+ (let [endpoint (format "field/%d" field-id)]
+ (testing "a non-admin cannot update field metadata if they have no data model permissions for the DB"
+ (with-all-users-data-perms {db-id {:data-model {:schemas :none}}}
+ (mt/user-http-request :rasta :put 403 endpoint {:name "Field Test 2"})))
+
+ (testing "a non-admin cannot update field metadata if they only have data model permissions for other schemas"
+ (with-all-users-data-perms {db-id {:data-model {:schemas {schema :none
+ "different schema" :all}}}}
+
+ (mt/user-http-request :rasta :put 403 endpoint {:name "Field Test 2"})))
+
+ (testing "a non-admin cannot update field metadata if they only have data model permissions for other tables"
+ (with-all-users-data-perms {db-id {:data-model {:schemas {schema {table-id :none
+ (inc table-id) :all}}}}}
+ (mt/user-http-request :rasta :put 403 endpoint {:name "Field Test 2"})))
+
+ (testing "a non-admin can update field metadata if they have data model perms for the DB"
+ (with-all-users-data-perms {db-id {:data-model {:schemas :all}}}
+ (mt/user-http-request :rasta :put 200 endpoint {:name "Field Test 2"})))
+
+ (testing "a non-admin can update field metadata if they have data model perms for the schema"
+ (with-all-users-data-perms {db-id {:data-model {:schemas {schema :all}}}}
+ (mt/user-http-request :rasta :put 200 endpoint {:name "Field Test 3"})))
+
+ (testing "a non-admin can update field metadata if they have data model perms for the table"
+ (with-all-users-data-perms {db-id {:data-model {:schemas {schema {table-id :all}}}}}
+ (mt/user-http-request :rasta :put 200 endpoint {:name "Field Test 3"})))))
+
+ (testing "POST /api/field/:id/rescan_values"
+ (testing "A non-admin can trigger a rescan of field values if they have data model perms for the table"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :none}}}}}
+ (mt/user-http-request :rasta :post 403 (format "field/%d/rescan_values" field-id)))
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :all}}}}}
+ (mt/user-http-request :rasta :post 200 (format "field/%d/rescan_values" field-id)))))
+
+ (testing "POST /api/field/:id/discard_values"
+ (testing "A non-admin can discard field values if they have data model perms for the table"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :none}}}}}
+ (mt/user-http-request :rasta :post 403 (format "field/%d/discard_values" field-id)))
+
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :all}}}}}
+ (mt/user-http-request :rasta :post 200 (format "field/%d/discard_values" field-id))))))))
+
+(deftest update-table-test
+ (mt/with-temp Table [{table-id :id} {:db_id (mt/id) :schema "PUBLIC"}]
+ (testing "PUT /api/table/:id"
+ (let [endpoint (format "table/%d" table-id)]
+ (testing "a non-admin cannot update table metadata if they have no data model permissions for the DB"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas :none}}}
+ (mt/user-http-request :rasta :put 403 endpoint {:name "Table Test 2"})))
+
+ (testing "a non-admin cannot update table metadata if they only have data model permissions for other schemas"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" :none
+ "different schema" :all}}}}
+ (mt/user-http-request :rasta :put 403 endpoint {:name "Table Test 2"})))
+
+ (testing "a non-admin cannot update table metadata if they only have data model permissions for other tables"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :none
+ (inc table-id) :all}}}}}
+ (mt/user-http-request :rasta :put 403 endpoint {:name "Table Test 2"})))
+
+ (testing "a non-admin can update table metadata if they have data model perms for the DB"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas :all}}}
+ (mt/user-http-request :rasta :put 200 endpoint {:name "Table Test 2"})))
+
+ (testing "a non-admin can update table metadata if they have data model perms for the schema"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" :all}}}}
+ (mt/user-http-request :rasta :put 200 endpoint {:name "Table Test 3"})))
+
+ (testing "a non-admin can update table metadata if they have data model perms for the table"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :all}}}}}
+ (mt/user-http-request :rasta :put 200 endpoint {:name "Table Test 3"})))))
+
+ (testing "POST /api/table/:id/rescan_values"
+ (testing "A non-admin can trigger a rescan of field values if they have data model perms for the table"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :none}}}}}
+ (mt/user-http-request :rasta :post 403 (format "table/%d/rescan_values" table-id)))
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :all}}}}}
+ (mt/user-http-request :rasta :post 200 (format "table/%d/rescan_values" table-id)))))
+
+ (testing "POST /api/table/:id/discard_values"
+ (testing "A non-admin can discard field values if they have data model perms for the table"
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :none}}}}}
+ (mt/user-http-request :rasta :post 403 (format "table/%d/discard_values" table-id)))
+
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :all}}}}}
+ (mt/user-http-request :rasta :post 200 (format "table/%d/discard_values" table-id)))))
+
+ (testing "POST /api/table/:id/fields/order"
+ (testing "A non-admin can set a custom field ordering if they have data model perms for the table"
+ (mt/with-temp* [Field [{field-1-id :id} {:table_id table-id}]
+ Field [{field-2-id :id} {:table_id table-id}]]
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :none}}}}}
+ (mt/user-http-request :rasta :put 403 (format "table/%d/fields/order" table-id)
+ {:request-options {:body (json/encode [field-2-id field-1-id])}}))
+
+ (with-all-users-data-perms {(mt/id) {:data-model {:schemas {"PUBLIC" {table-id :all}}}}}
+ (mt/user-http-request :rasta :put 200 (format "table/%d/fields/order" table-id)
+ {:request-options {:body (json/encode [field-2-id field-1-id])}})))))))
diff --git a/src/metabase/api/field.clj b/src/metabase/api/field.clj
index fd20af3c566e8155ed53f7516d2f88ba4fbfa0ea..0215c19cbf76d2f2ecbf84d5b996ed15fe07a53a 100644
--- a/src/metabase/api/field.clj
+++ b/src/metabase/api/field.clj
@@ -276,21 +276,18 @@
(create-field-values! field value-pairs)))
{:status :success})
-
(api/defendpoint POST "/:id/rescan_values"
"Manually trigger an update for the FieldValues for this Field. Only applies to Fields that are eligible for
FieldValues."
[id]
- (api/check-superuser)
- (field-values/create-or-update-field-values! (api/check-404 (Field id)))
+ (field-values/create-or-update-field-values! (api/write-check (Field id)))
{:status :success})
(api/defendpoint POST "/:id/discard_values"
"Discard the FieldValues belonging to this Field. Only applies to fields that have FieldValues. If this Field's
Database is set up to automatically sync FieldValues, they will be recreated during the next cycle."
[id]
- (api/check-superuser)
- (field-values/clear-field-values! (api/check-404 (Field id)))
+ (field-values/clear-field-values! (api/write-check (Field id)))
{:status :success})
diff --git a/src/metabase/api/table.clj b/src/metabase/api/table.clj
index bca7760529d6d075f6be136bd121ff5089a2845e..ab17db564fffe22d877968646b8d989844c03a1a 100644
--- a/src/metabase/api/table.clj
+++ b/src/metabase/api/table.clj
@@ -388,7 +388,6 @@
[]
[]) ; return empty array
-
(api/defendpoint GET "/:id/fks"
"Get all foreign keys whose destination is a `Field` that belongs to this `Table`."
[id]
@@ -407,19 +406,19 @@
"Manually trigger an update for the FieldValues for the Fields belonging to this Table. Only applies to Fields that
are eligible for FieldValues."
[id]
- (api/check-superuser)
- ;; async so as not to block the UI
- (sync.concurrent/submit-task
- (fn []
- (sync-field-values/update-field-values-for-table! (api/check-404 (Table id)))))
- {:status :success})
+ (let [table (Table id)]
+ (api/write-check table)
+ ;; async so as not to block the UI
+ (sync.concurrent/submit-task
+ (fn []
+ (sync-field-values/update-field-values-for-table! table)))
+ {:status :success}))
(api/defendpoint POST "/:id/discard_values"
"Discard the FieldValues belonging to the Fields in this Table. Only applies to fields that have FieldValues. If
this Table's Database is set up to automatically sync FieldValues, they will be recreated during the next cycle."
[id]
- (api/check-superuser)
- (api/check-404 (Table id))
+ (api/write-check (Table id))
(when-let [field-ids (db/select-ids Field :table_id id)]
(db/simple-delete! FieldValues :field_id [:in field-ids]))
{:status :success})
@@ -433,7 +432,6 @@
"Reorder fields"
[id :as {field_order :body}]
{field_order [su/IntGreaterThanZero]}
- (api/check-superuser)
- (-> id Table api/check-404 (table/custom-order-fields! field_order)))
+ (-> id Table api/write-check (table/custom-order-fields! field_order)))
(api/define-routes)
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index b728a995133b7861164c962d244e1171122c01ac..f4dae0563ad15dad58f2fa58836e16953cc6269a 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -116,7 +116,13 @@
;; 2) Failing that, we cache the corresponding permissions sets for each *Table ID* for a few seconds to minimize the
;; number of DB calls that are made. See discussion below for more details.
-(def ^:private ^{:arglists '([table-id])} perms-objects-set*
+(defn- perms-objects-set*
+ [db-id schema table-id read-or-write]
+ #{(case read-or-write
+ :read (perms/data-perms-path db-id schema table-id)
+ :write (perms/data-model-write-perms-path db-id schema table-id))})
+
+(def ^:private ^{:arglists '([table-id read-or-write])} cached-perms-object-set
"Cached lookup for the permissions set for a table with `table-id`. This is done so a single API call or other unit of
computation doesn't accidentally end up in a situation where thousands of DB calls end up being made to calculate
permissions for a large number of Fields. Thus, the cache only persists for 5 seconds.
@@ -129,21 +135,21 @@
see), would require only a few megs of RAM, and again only if every single Table was looked up in a span of 5
seconds."
(memoize/ttl
- (fn [table-id]
- (let [{schema :schema, database-id :db_id} (db/select-one ['Table :schema :db_id] :id table-id)]
- #{(perms/data-perms-path database-id schema table-id)}))
+ (fn [table-id read-or-write]
+ (let [{schema :schema, db-id :db_id} (db/select-one ['Table :schema :db_id] :id table-id)]
+ (perms-objects-set* db-id schema table-id read-or-write)))
:ttl/threshold 5000))
(defn- perms-objects-set
"Calculate set of permissions required to access a Field. For the time being permissions to access a Field are the
same as permissions to access its parent Table, and there are not separate permissions for reading/writing."
- [{table-id :table_id, {db-id :db_id, schema :schema} :table} _]
+ [{table-id :table_id, {db-id :db_id, schema :schema} :table} read-or-write]
{:arglists '([field read-or-write])}
(if db-id
;; if Field already has a hydrated `:table`, then just use that to generate perms set (no DB calls required)
- #{(perms/data-perms-path db-id schema table-id)}
+ (perms-objects-set* db-id schema table-id read-or-write)
;; otherwise we need to fetch additional info about Field's Table. This is cached for 5 seconds (see above)
- (perms-objects-set* table-id)))
+ (cached-perms-object-set table-id read-or-write)))
(defn- maybe-parse-semantic-numeric-values [maybe-double-value]
(if (string? maybe-double-value)
@@ -182,7 +188,7 @@
(merge i/IObjectPermissionsDefaults
{:perms-objects-set perms-objects-set
:can-read? (partial i/current-user-has-partial-permissions? :read)
- :can-write? i/superuser?}))
+ :can-write? (partial i/current-user-has-full-permissions? :write)}))
;;; ---------------------------------------------- Hydration / Util Fns ----------------------------------------------
diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index b06f6881cae562e6982aeceb39748c256baee55c..683d03d0d2df4b3fe971c611282a307d7e5ac4ab 100644
--- a/src/metabase/models/permissions.clj
+++ b/src/metabase/models/permissions.clj
@@ -481,6 +481,18 @@
[perm-type perm-value database-or-id]
(base->feature-perms-path perm-type perm-value (adhoc-native-query-path database-or-id)))
+(s/defn data-model-write-perms-path :- Path
+ "Returns the permission path required to edit the table specified by the provided args, or a field in the table.
+ If Enterprise Edition code is available, and a valid :advanced-permissions token is present, returns the data model
+ permissions path for the table. Otherwise, defaults to the root path ('/'), thus restricting writes to admins."
+ [db-id schema table-id]
+ (let [f (u/ignore-exceptions
+ (classloader/require ' metabase-enterprise.advanced-permissions.models.permissions)
+ (resolve ' metabase-enterprise.advanced-permissions.models.permissions/data-model-write-perms-path))]
+ (if (and f premium-features/enable-advanced-permissions?)
+ (f db-id schema table-id)
+ "/")))
+
(s/defn general-perms-path :- Path
"Returns the permissions path for *full* access a general permission."
[perm-type]
diff --git a/src/metabase/models/table.clj b/src/metabase/models/table.clj
index 3b451dfd3a45c7d6514f86ddc798cbc1903fb183..31ab156e7258947bf0c4949b4143259d7530dc7b 100644
--- a/src/metabase/models/table.clj
+++ b/src/metabase/models/table.clj
@@ -46,11 +46,14 @@
(db/delete! Permissions :object [:like (str (perms/data-perms-path db_id schema id) "%")]))
(defn- perms-objects-set [table read-or-write]
- ;; To read (e.g., fetch metadata) a Table you (predictably) have read permissions; to write a Table (e.g. update its
- ;; metadata) you must have *full* permissions.
+ ;; To read (e.g., fetch metadata) a Table you (predictably) have read permissions
+ ;; To write a Table (e.g. update its metadata):
+ ;; * If Enterprise Edition code is available and the :advanced-permissions feature is enabled, you must have
+ ;; data-model permissions for othe table
+ ;; * Else, you must be an admin
#{(case read-or-write
:read (perms/table-read-path table)
- :write (perms/data-perms-path (:db_id table) (:schema table) (:id table)))})
+ :write (perms/data-model-write-perms-path (:db_id table) (:schema table) (:id table)))})
(u/strict-extend (class Table)
models/IModel
@@ -65,7 +68,7 @@
i/IObjectPermissions
(merge i/IObjectPermissionsDefaults
{:can-read? (partial i/current-user-has-full-permissions? :read)
- :can-write? i/superuser?
+ :can-write? (partial i/current-user-has-full-permissions? :write)
:perms-objects-set perms-objects-set}))
diff --git a/test/metabase/api/field_test.clj b/test/metabase/api/field_test.clj
index d533906d6c9a255d62e526ec5f6bc51a60d4c1a3..9aee3fed0885698966d8c602441f83b687108d7d 100644
--- a/test/metabase/api/field_test.clj
+++ b/test/metabase/api/field_test.clj
@@ -166,7 +166,11 @@
(set-strategy! :Coercion/UNIXSeconds->DateTime)
(let [field (Field field-id)]
(is (= :type/Instant (:effective_type field)))
- (is (contains? (get-in field [:fingerprint :type]) :type/DateTime))))))))))
+ (is (contains? (get-in field [:fingerprint :type]) :type/DateTime))))))))
+
+ (testing "A field can only be updated by a superuser"
+ (mt/with-temp Field [{field-id :id} {:name "Field Test"}]
+ (mt/user-http-request :rasta :put 403 (format "field/%d" field-id) {:name "Field Test 2"})))))
(deftest remove-fk-semantic-type-test
(testing "PUT /api/field/:id"
diff --git a/test/metabase/api/table_test.clj b/test/metabase/api/table_test.clj
index 06360002ebfaa8308a92ada93d360a6746794113..bcaf71c49ebc74f9d2e0c55516bf8a16dd0a2842 100644
--- a/test/metabase/api/table_test.clj
+++ b/test/metabase/api/table_test.clj
@@ -282,7 +282,11 @@
:display_name "Userz"
:pk_field (table/pk-field-id table)})
(dissoc (mt/user-http-request :crowberto :get 200 (format "table/%d" (u/the-id table)))
- :updated_at))))))
+ :updated_at))))
+
+ (testing "A table can only be updated by a superuser"
+ (mt/with-temp Table [table]
+ (mt/user-http-request :rasta :put 403 (format "table/%d" (u/the-id table)) {:display_name "Userz"})))))
;; see how many times sync-table! gets called when we call the PUT endpoint. It should happen when you switch from
;; hidden -> not hidden at the spots marked below, twice total