From e9d84d5dda3b626275ffb970b8ef49e85c0b821e Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Thu, 1 Sep 2022 00:03:16 +0300
Subject: [PATCH] Prevent moving Apps away from the root collection (#25103)
See #25097 for context.
---
src/metabase/api/app.clj | 5 ++--
src/metabase/api/collection.clj | 4 +++
test/metabase/api/app_test.clj | 36 ++++++++-------------------
test/metabase/api/collection_test.clj | 8 ++++++
4 files changed, 24 insertions(+), 29 deletions(-)
diff --git a/src/metabase/api/app.clj b/src/metabase/api/app.clj
index b9d350f4dc4..aaf44324834 100644
--- a/src/metabase/api/app.clj
+++ b/src/metabase/api/app.clj
@@ -16,7 +16,7 @@
(api/defendpoint POST "/"
"Endpoint to create an app"
[:as {{:keys [collection dashboard_id options nav_items]
- {:keys [name color description parent_id namespace authority_level]} :collection
+ {:keys [name color description namespace authority_level]} :collection
:as body} :body}]
{dashboard_id (s/maybe su/IntGreaterThanOrEqualToZero)
options (s/maybe su/Map)
@@ -24,11 +24,10 @@
name su/NonBlankString
color collection/hex-color-regex
description (s/maybe su/NonBlankString)
- parent_id (s/maybe su/IntGreaterThanZero)
namespace (s/maybe su/NonBlankString)
authority_level collection/AuthorityLevel}
(db/transaction
- (let [coll-params (select-keys collection [:name :color :description :parent_id :namespace :authority_level])
+ (let [coll-params (select-keys collection [:name :color :description :namespace :authority_level])
collection-instance (api.collection/create-collection! coll-params)
app-params (-> body
(select-keys [:dashboard_id :options :nav_items])
diff --git a/src/metabase/api/collection.clj b/src/metabase/api/collection.clj
index 01a19fddb7a..d937f8461c1 100644
--- a/src/metabase/api/collection.clj
+++ b/src/metabase/api/collection.clj
@@ -774,6 +774,10 @@
(api/check-403
(perms/set-has-full-permissions-for-set? @api/*current-user-permissions-set*
(collection/perms-for-moving collection-before-update new-parent)))
+ (when (not= new-parent collection/root-collection)
+ ;; apps are not allowed to be moved away from the root collection
+ (api/check-403
+ (nil? (:app_id (hydrate collection-before-update :app_id)))))
;; ok, we're good to move!
(collection/move-collection! collection-before-update new-location)))))
diff --git a/test/metabase/api/app_test.clj b/test/metabase/api/app_test.clj
index e6976fb80a4..e8efa526a9c 100644
--- a/test/metabase/api/app_test.clj
+++ b/test/metabase/api/app_test.clj
@@ -11,44 +11,29 @@
(let [base-params {:name "App collection"
:color "#123456"}]
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
- (testing "Create app in non-root collection"
+ (testing "parent_id is ignored when creating apps"
(mt/with-temp* [Collection [{collection-id :id}]]
(let [coll-params (assoc base-params :parent_id collection-id)
response (mt/user-http-request :crowberto :post 200 "app" {:collection coll-params})]
(is (pos-int? (:id response)))
(is (pos-int? (:collection_id response)))
- (is (partial= (assoc base-params :location (format "/%d/" collection-id))
+ (is (partial= (assoc base-params :location "/")
(:collection response))))))
- (testing "Create aoo in the root"
+ (testing "Create app in the root"
(let [response (mt/user-http-request :crowberto :post 200 "app" {:collection base-params})]
(is (pos-int? (:id response)))
(is (pos-int? (:collection_id response)))
(is (partial= (assoc base-params :location "/")
(:collection response)))))
- (testing "Collection permissions"
- (mt/with-non-admin-groups-no-root-collection-perms
- (mt/with-temp* [Collection [{collection-id :id}]]
- (let [coll-params (assoc base-params :parent_id collection-id)]
- (is (= "You don't have permissions to do that."
- (mt/user-http-request :rasta :post 403 "app" {:collection coll-params}))))))
- (mt/with-temp* [Collection [{collection-id :id}]]
- (let [coll-params (assoc base-params :parent_id collection-id)
- response (mt/user-http-request :rasta :post 200 "app" {:collection coll-params})]
- (is (pos-int? (:id response)))
- (is (pos-int? (:collection_id response)))
- (is (partial= (assoc base-params :location (format "/%d/" collection-id))
- (:collection response))))))
(testing "With initial dashboard and nav_items"
- (mt/with-temp* [Collection [{collection-id :id}]
- Dashboard [{dashboard-id :id}]]
- (let [coll-params (assoc base-params :parent_id collection-id)
- nav_items [{:options {:click_behavior {}}}]]
- (is (partial= {:collection (assoc base-params :location (format "/%d/" collection-id))
+ (mt/with-temp Dashboard [{dashboard-id :id}]
+ (let [nav_items [{:options {:click_behavior {}}}]]
+ (is (partial= {:collection (assoc base-params :location "/")
:dashboard_id dashboard-id
:nav_items nav_items}
- (mt/user-http-request :crowberto :post 200 "app" {:collection coll-params
- :dashboard_id dashboard-id
- :nav_items nav_items}))))))))))
+ (mt/user-http-request :rasta :post 200 "app" {:collection base-params
+ :dashboard_id dashboard-id
+ :nav_items nav_items}))))))))))
(deftest update-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
@@ -57,8 +42,7 @@
(mt/with-temp* [Collection [{collection_id :id}]
App [{app_id :id} (assoc app-data :collection_id collection_id)]
Dashboard [{dashboard_id :id}]]
- (let [expected (merge app-data {:collection_id collection_id
- :dashboard_id dashboard_id})]
+ (let [expected (assoc app-data :collection_id collection_id :dashboard_id dashboard_id)]
(testing "setting the dashboard_id doesn't affect the other fields"
(is (partial= expected
(mt/user-http-request :crowberto :put 200 (str "app/" app_id) {:dashboard_id dashboard_id}))))
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index 9eb357391bd..10fdd0262a8 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -1470,6 +1470,14 @@
(update :id integer?)
(update :entity_id string?))))))
+ (testing "I shouldn't be allowed to move an App away from root."
+ (mt/with-temp* [Collection [collection-a]
+ App [_app {:collection_id (:id collection-a)}]
+ Collection [collection-b]]
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :put 403 (str "collection/" (u/the-id collection-a))
+ {:parent_id (u/the-id collection-b)})))))
+
(testing "I shouldn't be allowed to move the Collection without proper perms."
(testing "If I want to move A into B, I should need permissions for both A and B"
(mt/with-non-admin-groups-no-root-collection-perms
--
GitLab