From 62eb3c71caa61fa5060391b84d668baeb8ef4a69 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Tue, 12 Apr 2022 21:33:58 +0700
Subject: [PATCH] Enforce Setting Permissions for sharing APIs (#21610)
* disallow adding recipients by non-admins with monitoring permission
* enforce sharing and embedding apis
Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>
---
.../advanced_permissions/api/setting_test.clj | 96 ++++++++++++++++++-
src/metabase/api/card.clj | 8 +-
src/metabase/api/common/validation.clj | 6 +-
src/metabase/api/dashboard.clj | 6 +-
test/metabase/api/card_test.clj | 15 ++-
test/metabase/api/dashboard_test.clj | 9 +-
6 files changed, 123 insertions(+), 17 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj
index 2edff32c469..7dfc574085f 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj
@@ -5,12 +5,14 @@
[metabase.api.ldap-test :as ldap-test]
[metabase.email :as email]
[metabase.integrations.slack :as slack]
+ [metabase.models :refer [Card Dashboard]]
[metabase.models.permissions :as perms]
[metabase.models.setting-test :refer [test-setting-1 test-setting-2]]
[metabase.public-settings.premium-features-test :as premium-features-test]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
- [metabase.test.integrations.ldap :as ldap.test]))
+ [metabase.test.integrations.ldap :as ldap.test])
+ (:import java.util.UUID))
(use-fixtures :once (fixtures/initialize :db))
@@ -238,3 +240,95 @@
(perms/grant-general-permissions! group :setting)
(get-permission-groups user 200)
(get-permission-groups :crowberto 200))))))))
+
+(deftest dashboard-api-test
+ (testing "/api/dashboard"
+ (mt/with-temporary-setting-values [enable-public-sharing true
+ enable-embedding true]
+ (mt/with-user-in-groups
+ [group {:name "New Group"}
+ user [group]]
+ (letfn [(get-public-dashboards [user status]
+ (testing (format "get public dashboards with %s user" (user->name user))
+ (mt/user-http-request user :get status "dashboard/public")))
+
+ (get-embeddable-dashboards [user status]
+ (testing (format "get embeddable dashboards with %s user" (user->name user))
+ (mt/with-temp Dashboard [_ {:enable_embedding true}]
+ (mt/user-http-request user :get status "dashboard/embeddable"))))
+
+ (delete-public-dashboard [user status]
+ (testing (format "delete public dashboard with %s user" (user->name user))
+ (mt/with-temp Dashboard [{dashboard-id :id} {:public_uuid (str (UUID/randomUUID))
+ :made_public_by_id (mt/user->id :crowberto)}]
+ (mt/user-http-request user :delete status (format "dashboard/%d/public_link" dashboard-id)))))]
+
+ (testing "if `advanced-permissions` is disabled, require admins,"
+ (premium-features-test/with-premium-features #{}
+ (get-public-dashboards user 403)
+ (get-embeddable-dashboards user 403)
+ (delete-public-dashboard user 403)
+ (get-embeddable-dashboards :crowberto 200)
+ (delete-public-dashboard :crowberto 204)))
+
+ (testing "if `advanced-permissions` is enabled,"
+ (premium-features-test/with-premium-features #{:advanced-permissions}
+ (testing "still fail if user's group doesn't have `setting` permission"
+ (get-public-dashboards user 403)
+ (get-embeddable-dashboards user 403)
+ (delete-public-dashboard user 403)
+ (get-public-dashboards :crowberto 200)
+ (delete-public-dashboard :crowberto 204))
+
+ (testing "succeed if user's group has `setting` permission,"
+ (perms/grant-general-permissions! group :setting)
+ (get-public-dashboards user 200)
+ (get-embeddable-dashboards user 200)
+ (delete-public-dashboard user 204)))))))))
+
+(deftest card-api-test
+ (testing "/api/card"
+ (mt/with-temporary-setting-values [enable-public-sharing true
+ enable-embedding true]
+ (mt/with-user-in-groups
+ [group {:name "New Group"}
+ user [group]]
+ (letfn [(get-public-cards [user status]
+ (testing (format "get public cards with %s user" (user->name user))
+ (mt/user-http-request user :get status "card/public")))
+
+ (get-embeddable-cards [user status]
+ (testing (format "get embeddable dashboards with %s user" (user->name user))
+ (mt/with-temp Card[_ {:enable_embedding true}]
+ (mt/user-http-request user :get status "card/embeddable"))))
+
+ (delete-public-card [user status]
+ (testing (format "delete public card with %s user" (user->name user))
+ (mt/with-temp Card [{card-id :id} {:public_uuid (str (UUID/randomUUID))
+ :made_public_by_id (mt/user->id :crowberto)}]
+ (mt/user-http-request user :delete status (format "card/%d/public_link" card-id)))))]
+
+ (testing "if `advanced-permissions` is disabled, require admins,"
+ (premium-features-test/with-premium-features #{}
+ (get-public-cards user 403)
+ (get-embeddable-cards user 403)
+ (delete-public-card user 403)
+ (get-public-cards :crowberto 200)
+ (get-embeddable-cards :crowberto 200)
+ (delete-public-card :crowberto 204)))
+
+ (testing "if `advanced-permissions` is enabled"
+ (premium-features-test/with-premium-features #{:advanced-permissions}
+ (testing "still fail if user's group doesn't have `setting` permission,"
+ (get-public-cards user 403)
+ (get-embeddable-cards user 403)
+ (delete-public-card user 403)
+ (get-public-cards :crowberto 200)
+ (get-embeddable-cards :crowberto 200)
+ (delete-public-card :crowberto 204))
+
+ (testing "succeed if user's group has `setting` permission,"
+ (perms/grant-general-permissions! group :setting)
+ (get-public-cards user 200)
+ (get-embeddable-cards user 200)
+ (delete-public-card user 204)))))))))
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 2f58a724264..cfa75db4282 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -698,7 +698,7 @@
already been shared, it will return the existing public link rather than creating a new one.) Public sharing must
be enabled."
[card-id]
- (api/check-superuser)
+ (validation/check-has-general-permission :setting)
(validation/check-public-sharing-enabled)
(api/check-not-archived (api/read-check Card card-id))
{:uuid (or (db/select-one-field :public_uuid Card :id card-id)
@@ -710,7 +710,7 @@
(api/defendpoint DELETE "/:card-id/public_link"
"Delete the publicly-accessible link to this Card."
[card-id]
- (api/check-superuser)
+ (validation/check-has-general-permission :setting)
(validation/check-public-sharing-enabled)
(api/check-exists? Card :id card-id, :public_uuid [:not= nil])
(db/update! Card card-id
@@ -721,7 +721,7 @@
(api/defendpoint GET "/public"
"Fetch a list of Cards with public UUIDs. These cards are publicly-accessible *if* public sharing is enabled."
[]
- (api/check-superuser)
+ (validation/check-has-general-permission :setting)
(validation/check-public-sharing-enabled)
(db/select [Card :name :id :public_uuid], :public_uuid [:not= nil], :archived false))
@@ -729,7 +729,7 @@
"Fetch a list of Cards where `enable_embedding` is `true`. The cards can be embedded using the embedding endpoints
and a signed JWT."
[]
- (api/check-superuser)
+ (validation/check-has-general-permission :setting)
(validation/check-embedding-enabled)
(db/select [Card :name :id], :enable_embedding true, :archived false))
diff --git a/src/metabase/api/common/validation.clj b/src/metabase/api/common/validation.clj
index 7a15cbdf509..bb83ec22ed7 100644
--- a/src/metabase/api/common/validation.clj
+++ b/src/metabase/api/common/validation.clj
@@ -30,8 +30,8 @@
(check-has-general-permission perm-type true))
([perm-type require-superuser?]
- (if (and (premium-features/enable-advanced-permissions?)
- (resolve 'metabase-enterprise.advanced-permissions.common/current-user-has-general-permissions?))
- (api/check-403 ((resolve 'metabase-enterprise.advanced-permissions.common/current-user-has-general-permissions?) perm-type))
+ (if-let [f (and (premium-features/enable-advanced-permissions?)
+ (resolve 'metabase-enterprise.advanced-permissions.common/current-user-has-general-permissions?))]
+ (api/check-403 (f perm-type))
(when require-superuser?
(api/check-superuser)))))
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index 01bbb2eed11..257775ac621 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -490,7 +490,7 @@
(api/defendpoint DELETE "/:dashboard-id/public_link"
"Delete the publicly-accessible link to this Dashboard."
[dashboard-id]
- (api/check-superuser)
+ (validation/check-has-general-permission :setting)
(validation/check-public-sharing-enabled)
(api/check-exists? Dashboard :id dashboard-id, :public_uuid [:not= nil], :archived false)
(db/update! Dashboard dashboard-id
@@ -502,7 +502,7 @@
"Fetch a list of Dashboards with public UUIDs. These dashboards are publicly-accessible *if* public sharing is
enabled."
[]
- (api/check-superuser)
+ (validation/check-has-general-permission :setting)
(validation/check-public-sharing-enabled)
(db/select [Dashboard :name :id :public_uuid], :public_uuid [:not= nil], :archived false))
@@ -510,7 +510,7 @@
"Fetch a list of Dashboards where `enable_embedding` is `true`. The dashboards can be embedded using the embedding
endpoints and a signed JWT."
[]
- (api/check-superuser)
+ (validation/check-has-general-permission :setting)
(validation/check-embedding-enabled)
(db/select [Dashboard :name :id], :enable_embedding true, :archived false))
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 5b49d236dff..c0215804bf8 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -1827,6 +1827,10 @@
(testing "DELETE /api/card/:id/public_link"
(mt/with-temporary-setting-values [enable-public-sharing true]
(mt/with-temp Card [card (shared-card)]
+ (testing "requires superuser"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :delete 403 (format "card/%d/public_link" (u/the-id card))))))
+
(mt/user-http-request :crowberto :delete 204 (format "card/%d/public_link" (u/the-id card)))
(is (= false
(db/exists? Card :id (u/the-id card), :public_uuid (:public_uuid card))))))))
@@ -1852,9 +1856,14 @@
(testing "GET /api/card/public"
(mt/with-temporary-setting-values [enable-public-sharing true]
(mt/with-temp Card [card (shared-card)]
- (is (= [{:name true, :id true, :public_uuid true}]
- (for [card (mt/user-http-request :crowberto :get 200 "card/public")]
- (m/map-vals boolean (select-keys card [:name :id :public_uuid])))))))))
+ (testing "Test that it requires superuser"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 "card/public"))))
+
+ (testing "Test that superusers can fetch a list of publicly-accessible cards"
+ (is (= [{:name true, :id true, :public_uuid true}]
+ (for [card (mt/user-http-request :crowberto :get 200 "card/public")]
+ (m/map-vals boolean (select-keys card [:name :id :public_uuid]))))))))))
(deftest test-that-we-can-fetch-a-list-of-embeddable-cards
(testing "GET /api/card/embeddable"
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index d7949f8b987..a5528c9808a 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -1172,9 +1172,12 @@
;;
(deftest fetch-public-dashboards-test
(testing "GET /api/dashboard/public"
- (testing "Test that we can fetch a list of publicly-accessible dashboards"
- (mt/with-temporary-setting-values [enable-public-sharing true]
- (mt/with-temp Dashboard [dashboard (shared-dashboard)]
+ (mt/with-temporary-setting-values [enable-public-sharing true]
+ (mt/with-temp Dashboard [_dashboard (shared-dashboard)]
+ (testing "Test that it requires superuser"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 "dashboard/public"))))
+ (testing "Test that superusers can fetch a list of publicly-accessible dashboards"
(is (= [{:name true, :id true, :public_uuid true}]
(for [dash (mt/user-http-request :crowberto :get 200 "dashboard/public")]
(m/map-vals boolean (select-keys dash [:name :id :public_uuid]))))))))))
--
GitLab