diff --git a/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/block_permissions.clj b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/block_permissions.clj
index e2a20d1148a1f71e18c4139603e702982e6a328a..aa76cc131103db721880c332f6dea443c9457338 100644
--- a/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/block_permissions.clj
+++ b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/block_permissions.clj
@@ -15,8 +15,8 @@
:feature :advanced-permissions
[{database-id :database :as query}]
(or
- (not= :blocked (data-perms/full-db-permission-for-user api/*current-user-id* :perms/view-data database-id))
- (let [table-ids (query-perms/query->source-table-ids query)]
+ #_(not= :blocked (data-perms/full-db-permission-for-user api/*current-user-id* :perms/view-data database-id))
+ (let [table-ids #p (query-perms/query->source-table-ids query)]
(= #{:unrestricted}
(set
(map (partial data-perms/table-permission-for-user api/*current-user-id* :perms/view-data database-id)
diff --git a/src/metabase/models/query/permissions.clj b/src/metabase/models/query/permissions.clj
index 189c45cca0695dafeb28ac7bb63a2e6ac218ea87..16f9576b035e3f210c6a3a58d9bd3dfc28a612ef 100644
--- a/src/metabase/models/query/permissions.clj
+++ b/src/metabase/models/query/permissions.clj
@@ -186,12 +186,11 @@
"Checks that the current user has at least `required-perm` for the entire DB specified by `db-id`."
[perm-type required-perm gtap-perms db-id]
(or
- (data-perms/at-least-as-permissive? perm-type
- (data-perms/full-db-permission-for-user api/*current-user-id* perm-type db-id)
- required-perm)
- (when gtap-perms
- (data-perms/at-least-as-permissive? perm-type gtap-perms required-perm))
- (throw (perms-exception {db-id {perm-type required-perm}}))))
+ (data-perms/at-least-as-permissive? perm-type
+ (data-perms/full-db-permission-for-user api/*current-user-id* perm-type db-id)
+ required-perm)
+ (when gtap-perms
+ (data-perms/at-least-as-permissive? perm-type gtap-perms required-perm))))
(defn check-table-level-perms
"Checks that the current user has the permissions for tables specified in `table-id->perm`. This can be satisfied via
@@ -199,24 +198,46 @@
row-level-restrictions QP middleware when sandboxing is in effect. Throws an exception if the permission check fails;
else returns `true`."
[perm-type table-id->required-perm gtap-table-perms db-id]
- (if table-id->required-perm
- (doseq [[table-id required-perm] table-id->required-perm]
- (or
- (data-perms/user-has-permission-for-table?
- api/*current-user-id*
- perm-type
- required-perm
- db-id
- table-id)
- (when-let [gtap-perm (if (keyword? gtap-table-perms)
- ;; gtap-table-perms can be a keyword representing the DB permission...
- gtap-table-perms
- ;; ...or a map from table IDs to table permissions
- (get gtap-table-perms table-id))]
- (data-perms/at-least-as-permissive? perm-type
- gtap-perm
- required-perm))
- (throw (perms-exception {db-id {perm-type {table-id required-perm}}}))))
+ (let [table-id->has-required-perms?
+ (into {}
+ (for [[table-id required-perm] table-id->required-perm]
+ [table-id
+ (boolean
+ (or
+ (data-perms/user-has-permission-for-table?
+ api/*current-user-id*
+ perm-type
+ required-perm
+ db-id
+ table-id)
+ (when-let [gtap-perm (if (keyword? gtap-table-perms)
+ ;; gtap-table-perms can be a keyword representing the DB permission...
+ gtap-table-perms
+ ;; ...or a map from table IDs to table permissions
+ (get gtap-table-perms table-id))]
+ (data-perms/at-least-as-permissive? perm-type
+ gtap-perm
+ required-perm))))]))]
+ (every? true? (vals table-id->has-required-perms?))))
+
+(defn has-perm-for-query? [{{gtap-perms :gtaps} ::perms, db-id :database
+ :as _query}
+ perm-type
+ required-perms]
+ (if-let [db-or-table-perms (perm-type required-perms)]
+ (cond
+ (keyword? db-or-table-perms)
+ (check-db-level-perms perm-type db-or-table-perms (perm-type gtap-perms) db-id)
+
+ (map? db-or-table-perms)
+ (check-table-level-perms perm-type
+ db-or-table-perms
+ (perm-type gtap-perms)
+ db-id)
+
+ :else
+ (throw (ex-info (tru "Invalid required permissions")
+ required-perms)))
true))
(defn check-data-perms
@@ -225,8 +246,9 @@
`throw-exceptions?` to `false`).
If the [:gtap ::perms] path is present in the query, these perms are implicitly granted to the current user."
- [{{gtap-perms :gtaps} ::perms, db-id :database} required-perms & {:keys [throw-exceptions?]
- :or {throw-exceptions? true}}]
+ [{{gtap-perms :gtaps} ::perms, :as query}
+ required-perms & {:keys [throw-exceptions?]
+ :or {throw-exceptions? true}}]
(try
;; Check any required v1 paths
(when-let [paths (:paths required-perms)]
@@ -236,23 +258,8 @@
;; Check view-data and create-queries permissions, for individual tables or the entire DB
(doseq [perm-type [:perms/view-data :perms/create-queries]]
- (when-let [db-or-table-perms (perm-type required-perms)]
- ;; In practice, `view-data` will be defined at the table-level, and `create-queries` will either be table-level
- ;; or :query-builder-and-native for the entire DB. But we should enforce whatever `required-perms` are provided,
- ;; in case that ever changes.
- (cond
- (keyword? db-or-table-perms)
- (check-db-level-perms perm-type db-or-table-perms (perm-type gtap-perms) db-id)
-
- (map? db-or-table-perms)
- (check-table-level-perms perm-type
- db-or-table-perms
- (perm-type gtap-perms)
- db-id)
-
- :else
- (throw (ex-info (tru "Invalid required permissions")
- required-perms)))))
+ (when-not (has-perm-for-query? query perm-type required-perms)
+ (throw (perms-exception required-perms))))
true
(catch clojure.lang.ExceptionInfo e
diff --git a/src/metabase/query_processor/middleware/permissions.clj b/src/metabase/query_processor/middleware/permissions.clj
index 88436378cf8f4b99e841bb9bff9d927688e056fa..9ef866b36cacd9b479ac5bacb6191bccaecd0057 100644
--- a/src/metabase/query_processor/middleware/permissions.clj
+++ b/src/metabase/query_processor/middleware/permissions.clj
@@ -90,14 +90,17 @@
card-id
(do
(check-card-read-perms database-id card-id)
- (when-not (query-perms/check-data-perms outer-query required-perms :throw-exceptions? false)
- (check-block-permissions outer-query)))
+ ;; FIXME we shouldn't need to do this condition, just check the data perms.
+ ;; this is saying: do i have `view-data` and `create-query` permissions? If not,
+ ;; check if I have `view-data` and then we're good.
+ (when-not (query-perms/has-perm-for-query? outer-query :perms/view-data required-perms)
+ (throw (query-perms/perms-exception required-perms))))
;; set when querying for field values of dashboard filters, which only require
;; collection perms for the dashboard and not ad-hoc query perms
*param-values-query*
- (when-not (query-perms/check-data-perms outer-query required-perms :throw-exceptions? false)
- (check-block-permissions outer-query))
+ (when-not (query-perms/has-perm-for-query? outer-query :perms/view-data required-perms)
+ (throw (query-perms/perms-exception required-perms)))
:else
(do