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>

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)))))))))

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))
 

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)))))

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))
 

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"

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]))))))))))