From e86e80adf2f50ce66013e31e926b746959695825 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Thu, 7 Apr 2022 12:05:22 -0400
Subject: [PATCH] Data model permission enforcement part 2 (#21475)
* add can_access_data_model key to api/user/current
* add exclude_uneditable flag to /api/database/:id/metadata
* clean ns
* WIP figuring out how to update perm checks for Field model
* fix errors
* fix more errors
* tests for field APIs
* table perms changes
* tests for table API
* fix function call
* clean ns
* perm enforcement for other table APIs
* perm enforcement for other field APIs
* address comments
---
.../models/permissions.clj | 5 +
.../advanced_permissions/common_test.clj | 154 ++++++++++++++++--
src/metabase/api/field.clj | 7 +-
src/metabase/api/table.clj | 20 +--
src/metabase/models/field.clj | 22 ++-
src/metabase/models/permissions.clj | 12 ++
src/metabase/models/table.clj | 11 +-
test/metabase/api/field_test.clj | 6 +-
test/metabase/api/table_test.clj | 6 +-
9 files changed, 196 insertions(+), 47 deletions(-)
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 2a7538bf1cf..ee3c341599d 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 3dfdc9cf301..78846d7ceeb 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 fd20af3c566..0215c19cbf7 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 bca7760529d..ab17db564ff 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 b728a995133..f4dae0563ad 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 b06f6881cae..683d03d0d2d 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 3b451dfd3a4..31ab156e725 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 d533906d6c9..9aee3fed088 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 06360002ebf..bcaf71c49eb 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
--
GitLab