diff --git a/enterprise/backend/src/metabase_enterprise/advanced_permissions/common.clj b/enterprise/backend/src/metabase_enterprise/advanced_permissions/common.clj
index 2c2d29039db3d4ef6b43beb7a62641b66dca2af2..29ac5c39b40ea56f7f0b98262af97fda059d889b 100644
--- a/enterprise/backend/src/metabase_enterprise/advanced_permissions/common.clj
+++ b/enterprise/backend/src/metabase_enterprise/advanced_permissions/common.clj
@@ -179,6 +179,19 @@
:blocked
:unrestricted))
+(defenterprise new-table-view-data-permission-level
+ "Returns the view-data permission level to set for a new table in a given group and database. This is `blocked`
+ if the group has `blocked` for the DB or any table in the DB; otherwise it is `unrestricted`."
+ :feature :advanced-permissions
+ [db-id group-id]
+ (if (t2/exists? :model/DataPermissions
+ :db_id db-id
+ :perm_type :perms/view-data
+ :perm_value :blocked
+ :group_id group-id)
+ :blocked
+ :unrestricted))
+
(defenterprise new-group-view-data-permission-level
"Returns the default view-data permission level for a new group for a given database. This is `blocked` if All Users
has block permissions for the database, or if any connection impersonation policies or sandboxes exist. Otherwise, it
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 55a15bed8a72f5c15a0ff6c8d68e10c32cb672e8..bd7964fa5f09a733ba4b0c65ca9cdb0774ed8e9a 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
@@ -5,7 +5,6 @@
:as advanced-perms.api.tu]
[metabase-enterprise.advanced-permissions.common
:as advanced-permissions.common]
- [metabase-enterprise.test :as met]
[metabase.api.database :as api.database]
[metabase.driver :as driver]
[metabase.models
@@ -66,29 +65,56 @@
(user-permissions :rasta)))))))))
(deftest new-database-view-data-permission-level-test
+ (mt/with-additional-premium-features #{:sandboxes :advanced-permissions}
+ (mt/with-temp [:model/Database {db-id :id} {}
+ :model/PermissionsGroup {group-id :id} {}]
+ (let [perm-value (fn [db-id] (t2/select-one-fn :perm_value
+ :model/DataPermissions
+ :db_id db-id
+ :group_id group-id
+ :table_id nil
+ :perm_type :perms/view-data))]
+ (testing "A new database defaults to `:unrestricted` if no other perms are set"
+ (mt/with-temp [:model/Database {db-id-2 :id} {}]
+ (is (= :unrestricted (perm-value db-id-2)))))
+
+ (testing "A new database defaults to `:blocked` if the group has `:blocked` for any other database"
+ (data-perms/set-database-permission! group-id db-id :perms/view-data :blocked)
+ (mt/with-temp [:model/Database {db-id-2 :id} {}]
+ (is (= :blocked (perm-value db-id-2)))))
+
+ (testing "A new database defaults to `:blocked` if the group has any connection impersonation"
+ (data-perms/set-database-permission! group-id db-id :perms/view-data :unrestricted)
+ (mt/with-temp [:model/ConnectionImpersonation _ {:group_id group-id
+ :db_id db-id}
+ :model/Database {db-id-2 :id} {}]
+ (is (= :blocked (perm-value db-id-2)))))
+
+ (testing "A new database defaults to `:blocked` if the group has a sandbox for any table"
+ (mt/with-temp [:model/Table {table-id :id} {:db_id db-id}
+ :model/GroupTableAccessPolicy _ {:group_id group-id
+ :table_id table-id}
+ :model/Database {db-id-2 :id} {}]
+ (is (= :blocked (perm-value db-id-2)))))))))
+
+(deftest new-table-view-data-permission-level-test
(mt/with-additional-premium-features #{:sandboxes :advanced-permissions}
(mt/with-temp [:model/PermissionsGroup {group-id :id} {}
- :model/Database {db-id :id} {}]
- (testing "A new database defaults to `:unrestricted` if no other perms are set"
- ;; First delete the default permissions for the group so we start with a clean slate
- (t2/delete! :model/DataPermissions :group_id group-id)
- (is (= :unrestricted (advanced-permissions.common/new-database-view-data-permission-level group-id))))
-
- (testing "A new database defaults to `:blocked` if the group has `:blocked` for any other database"
- (data-perms/set-database-permission! group-id db-id :perms/view-data :blocked)
- (is (= :blocked (advanced-permissions.common/new-database-view-data-permission-level group-id))))
-
- (testing "A new database defaults to `:blocked` if the group has any connection impersonation"
- (data-perms/set-database-permission! group-id db-id :perms/view-data :unrestricted)
- (advanced-perms.api.tu/with-impersonations! {:impersonations [{:db-id db-id
- :attribute "impersonation_attr"
- :attributes {"impersonation_attr" "impersonation_role"}}]}
- (is (= :blocked (advanced-permissions.common/new-database-view-data-permission-level (u/the-id &group))))))
-
- (testing "A new database defaults to `:blocked` if the group has any sandbox"
- (data-perms/set-database-permission! group-id db-id :perms/view-data :unrestricted)
- (met/with-gtaps! {:gtaps {:venues {}}, :attributes {"a" 50}}
- (is (= :blocked (advanced-permissions.common/new-database-view-data-permission-level (u/the-id &group)))))))))
+ :model/Database {db-id :id} {}
+ :model/Table {table-id-1 :id} {:db_id db-id :schema "PUBLIC"}
+ :model/Table {table-id-2 :id} {:db_id db-id :schema "PUBLIC"}]
+ (let [perm-value (fn [table-id] (t2/select-one-fn :perm_value
+ :model/DataPermissions
+ :db_id db-id
+ :group_id group-id
+ :table_id table-id
+ :perm_type :perms/view-data))]
+ (testing "New table gets `blocked` view-data perms if any tables in the DB are `blocked`"
+ (data-perms/set-table-permission! group-id table-id-1 :perms/view-data :blocked)
+ (data-perms/set-table-permission! group-id table-id-2 :perms/view-data :unrestricted)
+ (mt/with-temp [:model/Table {table-id-3 :id} {:db_id db-id :schema "PUBLIC"}]
+ (is (nil? (perm-value nil)))
+ (is (= :blocked (perm-value table-id-3)))))))))
(deftest new-group-view-data-permission-level
(mt/with-additional-premium-features #{:sandboxes :advanced-permissions}
diff --git a/src/metabase/models/data_permissions.clj b/src/metabase/models/data_permissions.clj
index 6cb5fd89cdf59e8300bccb0f1f2364183770b381..43d50dd83c3d27cbefbffd271e18f1e0268133b4 100644
--- a/src/metabase/models/data_permissions.clj
+++ b/src/metabase/models/data_permissions.clj
@@ -267,32 +267,6 @@
(defn- get-additional-table-permission! [{:keys [db-id table-id]} perm-type]
(get-in *additional-table-permissions* [db-id table-id perm-type]))
-(mu/defn database-permission-for-group :- PermissionValue
- "Returns the effective permission value for a given *group*, permission type, and database ID"
- [group-id perm-type database-id]
- (when (not= :model/Database (model-by-perm-type perm-type))
- (throw (ex-info (tru "Permission type {0} is not a database-level permission." perm-type)
- {perm-type (Permissions perm-type)})))
- (let [perm-values (t2/select-fn-set :value
- :model/DataPermissions
- {:select [[:p.perm_value :value]]
- :from [[:data_permissions :p]]
- :where [:and
- [:= :p.group_id group-id]
- [:= :p.perm_type (u/qualified-name perm-type)]
- [:= :p.db_id database-id]
- [:= :table_id nil]]})]
- (or (coalesce perm-type perm-values)
- (least-permissive-value perm-type))))
-
-(mu/defn group-has-permission-for-database? :- :boolean
- "Returns a Boolean indicating whether the group has the specified permission value for the given database ID,
- or a more permissive value."
- [group-id perm-type perm-value database-id]
- (at-least-as-permissive? perm-type
- (database-permission-for-group group-id perm-type database-id)
- perm-value))
-
(mu/defn table-permission-for-groups :- PermissionValue
"Returns the effective permission value provided by a set of *group-ids*, for a provided permission type, database
ID, and table ID."
@@ -847,8 +821,8 @@
(set-table-permissions! group-or-id perm-type {table-or-id value}))
(defn- schema-permission-value
- "Infers the permission value for a new table based on existing permissions in the schema.
- Returns the uniform permission value if one exists, otherwise nil."
+ "Infers the permission value for a new table based on existing permissions in the schema. Returns a permission value
+ if every table in the schema has the same value, otherwise returns nil."
[db-id group-id schema-name perm-type]
(let [possible-values (:values (get Permissions perm-type))
schema-perms-check (mapv (fn [value]
@@ -863,20 +837,26 @@
(when single-perm-val?
(nth possible-values (.indexOf ^PersistentVector schema-perms-check true)))))
+(defenterprise new-table-view-data-permission-level
+ "Returns the view-data permission level to set for a new table in a given group and database. On OSS, this is always
+ `unrestricted`."
+ metabase-enterprise.advanced-permissions.common
+ [_db-id _group-id]
+ :unrestricted)
+
(mu/defn set-new-table-permissions!
- "Sets permissions for a single table to the specified value in all the provided groups.
- If all tables in the schema have the same permission value, the new table permission is added with that value.
- Otherwise, the new table permission is added with the provided value."
+ "Sets permissions for a single table all the provided groups, based on the following rules:
+ - :view-data is set to :blocked if any other tables in the DB are :blocked
+ - If all existing tables in the schema have the same permission value, the new table is set to match them.
+ - If permissions are set at the DB-level, no table permission is inserted.
+ - Otherwise we use the provided `default-value`."
[groups-or-ids :- [:sequential TheIdable]
table-or-id :- TheIdable
perm-type :- PermissionType
- value :- :keyword]
+ default-value :- :keyword]
(when (not= :model/Table (model-by-perm-type perm-type))
(throw (ex-info (tru "Permission type {0} cannot be set on tables." perm-type)
{perm-type (Permissions perm-type)})))
- (when (= value :blocked)
- (throw (ex-info (tru "Block permissions must be set at the database-level only.")
- {})))
(when (seq groups-or-ids)
(t2/with-transaction [_conn]
(let [group-ids (map u/the-id groups-or-ids)
@@ -895,8 +875,17 @@
new-perms (map (fn [group-id]
{:perm_type perm-type
:group_id group-id
- :perm_value (or (schema-permission-value db-id group-id schema-name perm-type)
- value)
+ :perm_value (or
+ ;; Make sure we set `blocked` data access if any other table in the
+ ;; DB has `blocked`
+ (and (= perm-type :perms/view-data)
+ (new-table-view-data-permission-level db-id group-id))
+
+ ;; If all tables in the schema have the same value, use that value
+ ;; for the new table
+ (schema-permission-value db-id group-id schema-name perm-type)
+
+ default-value)
:db_id db-id
:table_id (u/the-id table)
:schema_name schema-name})
diff --git a/src/metabase/models/table.clj b/src/metabase/models/table.clj
index dabffabcd9df246668aa3101289cce1e5f5a3b8f..d05d06e558c934685fb59c1d41ce714b2c177ec0 100644
--- a/src/metabase/models/table.clj
+++ b/src/metabase/models/table.clj
@@ -81,7 +81,7 @@
(data-perms/set-new-table-permissions! non-admin-groups table :perms/create-queries :no))
(do
;; Normal tables start out with unrestricted data access in all groups, but query access only in All Users
- (data-perms/set-new-table-permissions! (conj non-magic-groups all-users-group) table :perms/view-data :unrestricted)
+ (data-perms/set-new-table-permissions! non-admin-groups table :perms/view-data :unrestricted)
(data-perms/set-new-table-permissions! [all-users-group] table :perms/create-queries :query-builder)
(data-perms/set-new-table-permissions! non-magic-groups table :perms/create-queries :no)))
;; Download permissions
diff --git a/test/metabase/models/data_permissions_test.clj b/test/metabase/models/data_permissions_test.clj
index 7f8d7c35af4c0ff3fa9e14a3e4e739ba779717a0..cf402f638a557af4e00199935a652b8685dd0d49 100644
--- a/test/metabase/models/data_permissions_test.clj
+++ b/test/metabase/models/data_permissions_test.clj
@@ -614,34 +614,34 @@
:group_id group-id
:table_id table-id
:perm_type :perms/create-queries))]
- (mt/with-restored-data-perms-for-group! group-id
- (testing "New table inherits DB-level permission if set"
- (data-perms/set-table-permission! group-id table-id-1 :perms/create-queries :query-builder)
- (data-perms/set-table-permission! group-id table-id-2 :perms/create-queries :query-builder)
- (data-perms/set-table-permission! group-id table-id-3 :perms/create-queries :query-builder)
- (mt/with-temp [:model/Table {table-id-4 :id} {:db_id db-id :schema "PUBLIC"}]
- (is (= :query-builder (perm-value nil)))
- (is (nil? (perm-value table-id-4)))))
-
- (testing "New table inherits uniform permission value from schema"
- (data-perms/set-table-permission! group-id table-id-1 :perms/create-queries :query-builder)
- (data-perms/set-table-permission! group-id table-id-2 :perms/create-queries :query-builder)
- (data-perms/set-table-permission! group-id table-id-3 :perms/create-queries :no)
- (mt/with-temp [:model/Table {table-id-4 :id} {:db_id db-id :schema "PUBLIC"}]
- (is (= :query-builder (perm-value table-id-4))))
-
- (data-perms/set-table-permission! group-id table-id-1 :perms/create-queries :no)
- (data-perms/set-table-permission! group-id table-id-2 :perms/create-queries :no)
- (data-perms/set-table-permission! group-id table-id-3 :perms/create-queries :query-builder)
- (mt/with-temp [:model/Table {table-id-4 :id} {:db_id db-id :schema "PUBLIC"}]
- (is (= :no (perm-value table-id-4)))))
-
- (testing "New table uses default value when schema permissions are not uniform"
- (data-perms/set-table-permission! group-id table-id-1 :perms/create-queries :query-builder)
- (data-perms/set-table-permission! group-id table-id-2 :perms/create-queries :no)
- (data-perms/set-table-permission! group-id table-id-3 :perms/create-queries :no)
- (mt/with-temp [:model/Table {table-id-4 :id} {:db_id db-id :schema "PUBLIC"}]
- (is (= :no (perm-value table-id-4)))))))))
+ (testing "New table inherits DB-level permission if set"
+ (data-perms/set-table-permission! group-id table-id-1 :perms/create-queries :query-builder)
+ (data-perms/set-table-permission! group-id table-id-2 :perms/create-queries :query-builder)
+ (data-perms/set-table-permission! group-id table-id-3 :perms/create-queries :query-builder)
+ (mt/with-temp [:model/Table {table-id-4 :id} {:db_id db-id :schema "PUBLIC"}]
+ ;; nil table ID is passed to check DB-level value
+ (is (= :query-builder (perm-value nil)))
+ (is (nil? (perm-value table-id-4)))))
+
+ (testing "New table inherits uniform permission value from schema"
+ (data-perms/set-table-permission! group-id table-id-1 :perms/create-queries :query-builder)
+ (data-perms/set-table-permission! group-id table-id-2 :perms/create-queries :query-builder)
+ (data-perms/set-table-permission! group-id table-id-3 :perms/create-queries :no)
+ (mt/with-temp [:model/Table {table-id-4 :id} {:db_id db-id :schema "PUBLIC"}]
+ (is (= :query-builder (perm-value table-id-4))))
+
+ (data-perms/set-table-permission! group-id table-id-1 :perms/create-queries :no)
+ (data-perms/set-table-permission! group-id table-id-2 :perms/create-queries :no)
+ (data-perms/set-table-permission! group-id table-id-3 :perms/create-queries :query-builder)
+ (mt/with-temp [:model/Table {table-id-4 :id} {:db_id db-id :schema "PUBLIC"}]
+ (is (= :no (perm-value table-id-4)))))
+
+ (testing "New table uses default value when schema permissions are not uniform"
+ (data-perms/set-table-permission! group-id table-id-1 :perms/create-queries :query-builder)
+ (data-perms/set-table-permission! group-id table-id-2 :perms/create-queries :no)
+ (data-perms/set-table-permission! group-id table-id-3 :perms/create-queries :no)
+ (mt/with-temp [:model/Table {table-id-4 :id} {:db_id db-id :schema "PUBLIC"}]
+ (is (= :no (perm-value table-id-4))))))))
(deftest additional-table-permissions-works
(mt/with-temp [:model/PermissionsGroup {group-id :id} {}