From 8fc98927506686e043b98deee7bf830d7697ca05 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Tue, 12 Nov 2024 10:25:47 -0500
Subject: [PATCH] Clarify cache API permissions and add tests (#49804)
---
.../metabase_enterprise/cache/cache_test.clj | 66 ++++++++++++++++---
src/metabase/api/cache.clj | 19 +++---
2 files changed, 66 insertions(+), 19 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/cache/cache_test.clj b/enterprise/backend/test/metabase_enterprise/cache/cache_test.clj
index 7b3c051fde1..6a019d0e024 100644
--- a/enterprise/backend/test/metabase_enterprise/cache/cache_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/cache/cache_test.clj
@@ -27,16 +27,6 @@
:database_id (:id db)
:collection_id (:id col1)}
:model/Card card3 {:name "card3"}]
-
- (testing "Access from regular users"
- (testing "No general access"
- (is (= "You don't have permissions to do that."
- (mt/user-http-request :rasta :get 403 "cache/"))))
- (testing "But have access to a separate (accessible to them) entities"
- (is (= {:data []}
- (mt/user-http-request :rasta :get 200 "cache/"
- :model "question" :id (:id card1))))))
-
(testing "Can configure root"
(is (mt/user-http-request :crowberto :put 200 "cache/"
{:model "root"
@@ -118,6 +108,62 @@
:strategy {:type "schedule"
:schedule "0/2 * * * * ?"}}))))))))
+(deftest cache-config-permissions-test
+ (mt/with-model-cleanup [:model/CacheConfig]
+ (mt/with-premium-features #{:cache-granular-controls :audit-app}
+ (mt/with-temp [:model/Database {db-id :id} {}
+ :model/Collection {collection-id :id} {:name "col1"}
+ :model/Dashboard {dashboard-id :id} {:name "dash1"
+ :collection_id collection-id}
+ :model/Card {card-id :id} {:name "card1"
+ :database_id db-id
+ :collection_id collection-id}]
+ (testing "Non-admins have no general access to cache config"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 "cache/"))))
+
+ (testing "Non-admins can access cache config only if they have write access to the model"
+ (mt/with-all-users-data-perms-graph! {db-id {:details :yes}}
+ (doseq [[model id] [["question" card-id]
+ ["database" db-id]
+ ["dashboard" dashboard-id]]]
+ (testing (format "\nTesting cache config for %s %d" model id)
+ (is (= {:data []}
+ (mt/user-http-request :rasta :get 200 "cache/"
+ :model model
+ :id id)))
+ (is (mt/user-http-request :rasta :put 200 "cache/"
+ {:model model
+ :model_id id
+ :strategy {:type "nocache" :name "card1"}}))
+ (is (=? {:count 1}
+ (mt/user-http-request :rasta :post 200 "cache/invalidate"
+ (keyword model) [id])))
+ (is (nil? (mt/user-http-request :rasta :delete 204 "cache/"
+ {:model model
+ :model_id id})))))))
+
+ (testing "Non-admins cannot access cache config if they do not have write access to the model"
+ (mt/with-all-users-data-perms-graph! {db-id {:details :no}}
+ (mt/with-non-admin-groups-no-collection-perms collection-id
+ (doseq [[model id] [["question" card-id]
+ ["database" db-id]
+ ["dashboard" dashboard-id]]]
+ (testing (format "\nTesting cache config for %s %d" model id)
+ (mt/user-http-request :rasta :get 403 "cache/"
+ :model model
+ :id id)
+ (mt/user-http-request :rasta :put 403 "cache/"
+ {:model model
+ :model_id id
+ :strategy {:type "nocache" :name "card1"}})
+ (mt/user-http-request :rasta :post 403 "cache/invalidate"
+ (keyword model) [id])
+
+ (mt/user-http-request :rasta :delete 403 "cache/"
+ {:model model
+ :model_id id}))))))))))
+
(deftest invalidation-test
(mt/with-model-cleanup [:model/CacheConfig
[:model/QueryCache :updated_at]]
diff --git a/src/metabase/api/cache.clj b/src/metabase/api/cache.clj
index 1f57df86c2e..cc005c746fc 100644
--- a/src/metabase/api/cache.clj
+++ b/src/metabase/api/cache.clj
@@ -3,7 +3,6 @@
[clojure.walk :as walk]
[compojure.core :refer [GET]]
[metabase.api.common :as api]
- [metabase.api.common.validation :as validation]
[metabase.models.cache-config :as cache-config]
[metabase.public-settings.premium-features :as premium-features :refer [defenterprise]]
[metabase.util.i18n :refer [tru trun]]
@@ -67,8 +66,8 @@
;; sometimes its a sequence and we're going to check for settings access anyway
(not (number? id))
(zero? id))
- ;; if you're not accessing a concrete entity, you should be able to access settings
- (validation/check-has-application-permission :setting)
+ ;; if you're not accessing a concrete entity, you have to be an admin
+ (api/check-superuser)
(api/write-check (case model
"database" :model/Database
"dashboard" :model/Dashboard
@@ -98,7 +97,6 @@
{model cache-config/CachingModel
model_id ms/IntGreaterThanOrEqualToZero
strategy (CacheStrategyAPI)}
- (validation/check-has-application-permission :setting)
(assert-valid-models model [model_id] (premium-features/enable-cache-granular-controls?))
(check-cache-access model model_id)
{:id (cache-config/store! api/*current-user-id* config)})
@@ -108,9 +106,8 @@
[:as {{:keys [model model_id]} :body}]
{model cache-config/CachingModel
model_id (ms/QueryVectorOf ms/IntGreaterThanOrEqualToZero)}
- (validation/check-has-application-permission :setting)
(assert-valid-models model model_id (premium-features/enable-cache-granular-controls?))
- (check-cache-access model model_id)
+ (doseq [id model_id] (check-cache-access model id))
(cache-config/delete! api/*current-user-id* model model_id)
nil)
@@ -124,13 +121,17 @@
touching all nested configurations, or you want your invalidation to trickle down to every card."
[include database dashboard question]
{include [:maybe {:description "All cache configuration overrides should invalidate cache too"} [:= :overrides]]
- database [:maybe {:description "A database id"} (ms/QueryVectorOf ms/IntGreaterThanOrEqualToZero)]
- dashboard [:maybe {:description "A dashboard id"} (ms/QueryVectorOf ms/IntGreaterThanOrEqualToZero)]
- question [:maybe {:description "A question id"} (ms/QueryVectorOf ms/IntGreaterThanOrEqualToZero)]}
+ database [:maybe {:description "A list of database ids"} (ms/QueryVectorOf ms/IntGreaterThanOrEqualToZero)]
+ dashboard [:maybe {:description "A list of dashboard ids"} (ms/QueryVectorOf ms/IntGreaterThanOrEqualToZero)]
+ question [:maybe {:description "A list of question ids"} (ms/QueryVectorOf ms/IntGreaterThanOrEqualToZero)]}
(when-not (premium-features/enable-cache-granular-controls?)
(throw (premium-features/ee-feature-error (tru "Granular Caching"))))
+ (doseq [db-id database] (api/write-check :model/Database db-id))
+ (doseq [dashboard-id dashboard] (api/write-check :model/Dashboard dashboard-id))
+ (doseq [question-id question] (api/write-check :model/Card question-id))
+
(let [cnt (cache-config/invalidate! {:databases database
:dashboards dashboard
:questions question
--
GitLab