diff --git a/src/metabase/api/getting_started.clj b/src/metabase/api/getting_started.clj
deleted file mode 100644
index a0a4d3a1b7cb01448a794b76c629cf58910f308c..0000000000000000000000000000000000000000
--- a/src/metabase/api/getting_started.clj
+++ /dev/null
@@ -1,45 +0,0 @@
-(ns metabase.api.getting-started
- "/api/getting_started routes."
- (:require [compojure.core :refer [GET]]
- [medley.core :as m]
- [metabase.api.common :as api]
- [metabase.models
- [interface :as mi]
- [setting :refer [defsetting]]]
- [metabase.util :as u]
- [puppetlabs.i18n.core :refer [tru]]
- [toucan.db :as db]))
-
-(defsetting getting-started-things-to-know
- (tru "''Some things to know'' text field for the Getting Started guide."))
-
-(defsetting getting-started-contact-name
- (tru "Name of somebody users can contact for help in the Getting Started guide."))
-
-(defsetting getting-started-contact-email
- (tru "Email of somebody users can contact for help in the Getting Started guide."))
-
-
-(api/defendpoint GET "/"
- "Fetch basic info for the Getting Started guide."
- []
- (let [metric-ids (map :id (filter mi/can-read? (db/select ['Metric :table_id :id] :show_in_getting_started true, {:order-by [:%lower.name]})))
- table-ids (map :id (filter mi/can-read? (db/select ['Table :db_id :schema :id] :show_in_getting_started true, {:order-by [:%lower.name]})))
- segment-ids (map :id (filter mi/can-read? (db/select ['Segment :table_id :id] :show_in_getting_started true, {:order-by [:%lower.name]})))]
- {:things_to_know (getting-started-things-to-know)
- :contact {:name (getting-started-contact-name)
- :email (getting-started-contact-email)}
- :most_important_dashboard (when-let [dashboard (db/select-one ['Dashboard :id :collection_id]
- :show_in_getting_started true)]
- (when (mi/can-read? dashboard)
- (u/get-id dashboard)))
- :important_metrics metric-ids
- :important_tables table-ids
- :important_segments segment-ids
- ;; A map of metric_id -> sequence of important field_ids
- :metric_important_fields (m/map-vals (partial map :field_id)
- (group-by :metric_id (when (seq metric-ids)
- (db/select ['MetricImportantField :field_id :metric_id]
- :metric_id [:in metric-ids]))))}))
-
-(api/define-routes)
diff --git a/src/metabase/api/routes.clj b/src/metabase/api/routes.clj
index 903ba777e79038eeed20ebf7f3182753e51c0007..bbbb5e900e54d696828c54ee2a26f240b3a53d6e 100644
--- a/src/metabase/api/routes.clj
+++ b/src/metabase/api/routes.clj
@@ -15,7 +15,6 @@
[embed :as embed]
[field :as field]
[geojson :as geojson]
- [getting-started :as getting-started]
[ldap :as ldap]
[metric :as metric]
[notify :as notify]
@@ -67,7 +66,6 @@
(context "/email" [] (+auth email/routes))
(context "/embed" [] (+message-only-exceptions embed/routes))
(context "/field" [] (+auth field/routes))
- (context "/getting_started" [] (+auth getting-started/routes))
(context "/geojson" [] (+auth geojson/routes))
(context "/ldap" [] (+auth ldap/routes))
(context "/metric" [] (+auth metric/routes))
diff --git a/src/metabase/db/migrations.clj b/src/metabase/db/migrations.clj
index 1aeed2b266b17732a92614d5ed061272f0564256..8e6a7c240dd67148ad065f3f461bab3258f684c3 100644
--- a/src/metabase/db/migrations.clj
+++ b/src/metabase/db/migrations.clj
@@ -15,23 +15,23 @@
[db :as mdb]
[public-settings :as public-settings]
[util :as u]]
- [metabase.events.activity-feed :refer [activity-feed-topics]]
[metabase.models
- [activity :refer [Activity]]
[card :refer [Card]]
- [dashboard-card :refer [DashboardCard]]
+ [collection :as collection :refer [Collection]]
+ [dashboard :refer [Dashboard]]
[database :refer [Database virtual-id]]
[field :refer [Field]]
[humanization :as humanization]
[permissions :as perms :refer [Permissions]]
[permissions-group :as perm-group]
[permissions-group-membership :as perm-membership :refer [PermissionsGroupMembership]]
+ [pulse :refer [Pulse]]
[query-execution :as query-execution :refer [QueryExecution]]
[setting :as setting :refer [Setting]]
- [table :as table :refer [Table]]
[user :refer [User]]]
[metabase.query-processor.util :as qputil]
[metabase.util.date :as du]
+ [puppetlabs.i18n.core :refer [trs]]
[toucan
[db :as db]
[models :as models]])
@@ -74,73 +74,6 @@
(log/info "Finished running data migrations."))
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | MIGRATIONS |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-;; Set the `:ssl` key in `details` to `false` for all existing MongoDB `Databases`.
-;; UI was automatically setting `:ssl` to `true` for every database added as part of the auto-SSL detection.
-;; Since Mongo did *not* support SSL, all existing Mongo DBs should actually have this key set to `false`.
-(defmigration ^{:author "camsaul", :added "0.13.0"} set-mongodb-databases-ssl-false
- (doseq [{:keys [id details]} (db/select [Database :id :details], :engine "mongo")]
- (db/update! Database id, :details (assoc details :ssl false))))
-
-
-;; Set default values for :schema in existing tables now that we've added the column
-;; That way sync won't get confused next time around
-(defmigration ^{:author "camsaul", :added "0.13.0"} set-default-schemas
- (doseq [[engine default-schema] [["postgres" "public"]
- ["h2" "PUBLIC"]]]
- (when-let [db-ids (db/select-ids Database :engine engine)]
- (db/update-where! Table {:schema nil
- :db_id [:in db-ids]}
- :schema default-schema))))
-
-
-;; Populate the initial value for the `:admin-email` setting for anyone who hasn't done it yet
-(defmigration ^{:author "agilliland", :added "0.13.0"} set-admin-email
- (when-not (setting/get :admin-email)
- (when-let [email (db/select-one-field :email 'User
- :is_superuser true
- :is_active true)]
- (setting/set! :admin-email email))))
-
-
-;; Remove old `database-sync` activity feed entries
-(defmigration ^{:author "agilliland", :added "0.13.0"} remove-database-sync-activity-entries
- (when-not (contains? activity-feed-topics :database-sync-begin)
- (db/simple-delete! Activity :topic "database-sync")))
-
-
-;; Migrate dashboards to the new grid
-;; NOTE: this scales the dashboards by 4x in the Y-scale and 3x in the X-scale
-(defmigration ^{:author "agilliland",:added "0.16.0"} update-dashboards-to-new-grid
- (doseq [{:keys [id row col sizeX sizeY]} (db/select DashboardCard)]
- (db/update! DashboardCard id
- :row (when row (* row 4))
- :col (when col (* col 3))
- :sizeX (when sizeX (* sizeX 3))
- :sizeY (when sizeY (* sizeY 4)))))
-
-
-;; migrate data to new visibility_type column on field
-(defmigration ^{:author "agilliland",:added "0.16.0"} migrate-field-visibility-type
- (when-not (zero? (db/count Field :visibility_type "unset"))
- ;; start by marking all inactive fields as :retired
- (db/update-where! Field {:visibility_type "unset"
- :active false}
- :visibility_type "retired")
- ;; if field is active but preview_display = false then it becomes :details-only
- (db/update-where! Field {:visibility_type "unset"
- :active true
- :preview_display false}
- :visibility_type "details-only")
- ;; everything else should end up as a :normal field
- (db/update-where! Field {:visibility_type "unset"
- :active true}
- :visibility_type "normal")))
-
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | PERMISSIONS v1 |
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -400,3 +333,43 @@
(db/transaction
(doseq [user (db/select [User :id :password_salt] :ldap_auth [:= true])]
(db/update! User (u/get-id user) :password (creds/hash-bcrypt (str (:password_salt user) (UUID/randomUUID)))))))
+
+
+;; In 0.30 dashboards and pulses will be saved in collections rather than on separate list pages. Additionally, there
+;; will no longer be any notion of saved questions existing outside of a collection (i.e. in the weird "Everything
+;; Else" area where they can currently be saved).
+;;
+;; Consequently we'll need to move existing dashboards, pulses, and questions-not-in-a-collection to a new location
+;; when users upgrade their instance to 0.30 from a previous version.
+;;
+;; The user feedback we've received points to a UX that would do the following:
+;;
+;; 1. Set permissions to the Root Collection to readwrite perms access for All User group.
+;;
+;; 2. Create three new collections within the root collection: "Migrated dashboards," "Migrated pulses," and "Migrated
+;; questions."
+;;
+;; 3. The permissions settings for these new collections should be set to No Access for all user groups except
+;; Administrators.
+;;
+;; 4. Existing Dashboards, Pulses, and Questions from the "Everything Else" area should now be present within these
+;; new collections.
+;;
+(defmigration ^{:author "camsaul", :added "0.30.0"} add-migrated-collections
+ ;; 1. Grant Root Collection readwrite perms to All Users
+ (perms/grant-collection-readwrite-permissions! (perm-group/all-users) collection/root-collection)
+ ;; 2. Create the new collections.
+ (doseq [[model new-collection-name] {Dashboard (trs "Migrated Dashboards")
+ Pulse (trs "Migrated Pulses")
+ Card (trs "Migrated Questions")}
+ :when (db/exists? model :collection_id nil)
+ :let [new-collection (db/insert! Collection
+ :name new-collection-name
+ :color "#509ee3")]] ; MB brand color
+ ;; 3. make sure the All Users group doesn't have any perms for this Collection.
+ (perms/revoke-collection-permissions! (perm-group/all-users) new-collection)
+ ;; 4. move everything not in this Collection to a new Collection
+ (log/info (trs "Moving instances of {0} that aren't in a Collection to {1} Collection {2}"
+ (name model) new-collection-name (u/get-id new-collection)))
+ (db/update-where! model {:collection_id nil}
+ :collection_id (u/get-id new-collection))))
diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index b829fb90d1e12efb259d91f9769b8d2ecc48328c..5e0dd75d288be4a32201407b9838c943909815af 100644
--- a/src/metabase/models/permissions.clj
+++ b/src/metabase/models/permissions.clj
@@ -420,7 +420,7 @@
:object path)
;; on some occasions through weirdness we might accidentally try to insert a key that's already been inserted
(catch Throwable e
- (log/error (u/format-color 'red "Failed to grant permissions: %s" (.getMessage e)))))))
+ (log/error (u/format-color 'red (tru "Failed to grant permissions: {0}" (.getMessage e))))))))
(defn revoke-native-permissions!
"Revoke all native query permissions for `group-or-id` to database with `database-id`."
diff --git a/test/metabase/api/alert_test.clj b/test/metabase/api/alert_test.clj
index 1a1312353d95b141c46d18bd37809ef60083f169..87fa8f7a84f0811f9f225a0dac6f28c2acd6e7c4 100644
--- a/test/metabase/api/alert_test.clj
+++ b/test/metabase/api/alert_test.clj
@@ -6,7 +6,6 @@
[http-client :as http]
[middleware :as middleware]
[util :as u]]
- [metabase.api.card-test :as card-api-test]
[metabase.models
[card :refer [Card]]
[collection :refer [Collection]]
@@ -22,9 +21,7 @@
[util :as tu]]
[metabase.test.data.users :as users :refer :all]
[metabase.test.mock.util :refer [pulse-channel-defaults]]
- [toucan
- [db :as db]
- [hydrate :refer [hydrate]]]
+ [toucan.db :as db]
[toucan.util.test :as tt]))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -101,14 +98,15 @@
(The name of this function is somewhat of a misnomer since the Alerts themselves aren't in Collections; it is their
Cards that are. Alerts do not go in Collections; their perms are derived from their Cards.)"
[grant-collection-perms-fn! alerts-or-ids f]
- (tt/with-temp Collection [collection]
- (grant-collection-perms-fn! (group/all-users) collection)
- ;; Go ahead and put all the Cards for all of the Alerts in the temp Collection
- (when (seq alerts-or-ids)
- (doseq [alert (db/select Pulse :id [:in (map u/get-id alerts-or-ids)])
- :let [card (#'metabase.models.pulse/alert->card alert)]]
- (db/update! Card (u/get-id card) :collection_id (u/get-id collection))))
- (f)))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (grant-collection-perms-fn! (group/all-users) collection)
+ ;; Go ahead and put all the Cards for all of the Alerts in the temp Collection
+ (when (seq alerts-or-ids)
+ (doseq [alert (db/select Pulse :id [:in (map u/get-id alerts-or-ids)])
+ :let [card (#'metabase.models.pulse/alert->card alert)]]
+ (db/update! Card (u/get-id card) :collection_id (u/get-id collection))))
+ (f))))
(defmacro ^:private with-alerts-in-readable-collection [alerts-or-ids & body]
`(do-with-alerts-in-a-collection perms/grant-collection-read-permissions! ~alerts-or-ids (fn [] ~@body)))
@@ -248,26 +246,27 @@
:recipients []})
;; Check creation of a new rows alert with email notification
-(tt/expect-with-temp [Collection [collection]
- Card [card {:name "My question"
- :collection_id (u/get-id collection)}]]
+(tt/expect-with-temp [Card [card {:name "My question"}]]
[(-> (default-alert card)
(assoc-in [:card :include_csv] true)
(assoc-in [:card :collection_id] true)
(update-in [:channels 0] merge {:schedule_hour 12, :schedule_type "daily", :recipients []}))
(rasta-new-alert-email {"has any results" true})]
- (with-alert-setup
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- [(et/with-expected-messages 1
- ((alert-client :rasta) :post 200 "alert"
- {:card {:id (u/get-id card), :include_csv false, :include_xls false}
- :collection_id (u/get-id collection)
- :alert_condition "rows"
- :alert_first_only false
- :channels [daily-email-channel]}))
- (et/regex-email-bodies #"https://metabase.com/testmb"
- #"has any results"
- #"My question")]))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (db/update! Card (u/get-id card) :collection_id (u/get-id collection))
+ (with-alert-setup
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ [(et/with-expected-messages 1
+ ((alert-client :rasta) :post 200 "alert"
+ {:card {:id (u/get-id card), :include_csv false, :include_xls false}
+ :collection_id (u/get-id collection)
+ :alert_condition "rows"
+ :alert_first_only false
+ :channels [daily-email-channel]}))
+ (et/regex-email-bodies #"https://metabase.com/testmb"
+ #"has any results"
+ #"My question")]))))
(defn- setify-recipient-emails [results]
(update results :channels (fn [channels]
@@ -309,43 +308,45 @@
;; Check creation of a below goal alert
(expect
(rasta-new-alert-email {"goes below its goal" true})
- (tt/with-temp* [Collection [collection]
- Card [card {:name "My question"
- :display "line"
- :collection_id (u/get-id collection)}]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- (with-alert-setup
- (et/with-expected-messages 1
- ((user->client :rasta) :post 200 "alert"
- {:card {:id (u/get-id card), :include_csv false, :include_xls false}
- :alert_condition "goal"
- :alert_above_goal false
- :alert_first_only false
- :channels [daily-email-channel]}))
- (et/regex-email-bodies #"https://metabase.com/testmb"
- #"goes below its goal"
- #"My question"))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Card [card {:name "My question"
+ :display "line"
+ :collection_id (u/get-id collection)}]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ (with-alert-setup
+ (et/with-expected-messages 1
+ ((user->client :rasta) :post 200 "alert"
+ {:card {:id (u/get-id card), :include_csv false, :include_xls false}
+ :alert_condition "goal"
+ :alert_above_goal false
+ :alert_first_only false
+ :channels [daily-email-channel]}))
+ (et/regex-email-bodies #"https://metabase.com/testmb"
+ #"goes below its goal"
+ #"My question")))))
;; Check creation of a above goal alert
(expect
(rasta-new-alert-email {"meets its goal" true})
- (tt/with-temp* [Collection [collection]
- Card [card {:name "My question"
- :display "bar"
- :collection_id (u/get-id collection)}]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- (with-alert-setup
- (et/with-expected-messages 1
- ((user->client :rasta) :post 200 "alert"
- {:card {:id (u/get-id card), :include_csv false, :include_xls false}
- :collection_id (u/get-id collection)
- :alert_condition "goal"
- :alert_above_goal true
- :alert_first_only false
- :channels [daily-email-channel]}))
- (et/regex-email-bodies #"https://metabase.com/testmb"
- #"meets its goal"
- #"My question"))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Card [card {:name "My question"
+ :display "bar"
+ :collection_id (u/get-id collection)}]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ (with-alert-setup
+ (et/with-expected-messages 1
+ ((user->client :rasta) :post 200 "alert"
+ {:card {:id (u/get-id card), :include_csv false, :include_xls false}
+ :collection_id (u/get-id collection)
+ :alert_condition "goal"
+ :alert_above_goal true
+ :alert_first_only false
+ :channels [daily-email-channel]}))
+ (et/regex-email-bodies #"https://metabase.com/testmb"
+ #"meets its goal"
+ #"My question")))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -507,21 +508,23 @@
PulseCard [_ (pulse-card alert card)]
PulseChannel [pc (pulse-channel alert)]
PulseChannelRecipient [_ (recipient pc :rasta)]]
- (with-alert-setup
- ((alert-client :rasta) :put 403 (alert-url alert)
- (default-alert-req card pc)))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-alert-setup
+ ((alert-client :rasta) :put 403 (alert-url alert)
+ (default-alert-req card pc))))))
;; Non-admin users can't edit alerts if they're not in the recipient list
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Pulse [alert (basic-alert)]
- Card [card]
- PulseCard [_ (pulse-card alert card)]
- PulseChannel [pc (pulse-channel alert)]
- PulseChannelRecipient [_ (recipient pc :crowberto)]]
- (with-alert-setup
- ((alert-client :rasta) :put 403 (alert-url alert)
- (default-alert-req card pc)))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Pulse [alert (basic-alert)]
+ Card [card]
+ PulseCard [_ (pulse-card alert card)]
+ PulseChannel [pc (pulse-channel alert)]
+ PulseChannelRecipient [_ (recipient pc :crowberto)]]
+ (with-alert-setup
+ ((alert-client :rasta) :put 403 (alert-url alert)
+ (default-alert-req card pc))))))
;; Can we archive an Alert?
(expect
@@ -574,9 +577,10 @@
(assoc :read_only true)
(update-in [:channels 0] merge {:schedule_hour 15, :schedule_type "daily"})
(assoc-in [:card :collection_id] true))]
- (with-alert-setup
- (with-alerts-in-readable-collection [alert]
- ((alert-client :rasta) :get 200 (alert-question-url card)))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-alert-setup
+ (with-alerts-in-readable-collection [alert]
+ ((alert-client :rasta) :get 200 (alert-question-url card))))))
;; Non-admin users shouldn't see alerts they created if they're no longer recipients
(expect
@@ -793,37 +797,39 @@
(expect
{:count 1
:response "You don't have permissions to do that."}
- (tt/with-temp* [Card [card (basic-alert-query)]
- Pulse [alert (basic-alert)]
- PulseCard [_ (pulse-card alert card)]
- PulseChannel [pc (pulse-channel alert)]
- PulseChannelRecipient [_ (recipient pc :rasta)]]
- (with-alerts-in-readable-collection [alert]
- (with-alert-setup
- (array-map
- :count (api:alert-question-count :rasta card)
- :response (api:delete! :rasta 403 alert))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Card [card (basic-alert-query)]
+ Pulse [alert (basic-alert)]
+ PulseCard [_ (pulse-card alert card)]
+ PulseChannel [pc (pulse-channel alert)]
+ PulseChannelRecipient [_ (recipient pc :rasta)]]
+ (with-alerts-in-readable-collection [alert]
+ (with-alert-setup
+ (array-map
+ :count (api:alert-question-count :rasta card)
+ :response (api:delete! :rasta 403 alert)))))))
;; Testing a user can't delete an admin's alert
(expect
{:count-1 1
:response nil
:count-2 0}
- (tt/with-temp* [Card [card (basic-alert-query)]
- Pulse [alert (basic-alert)]
- PulseCard [_ (pulse-card alert card)]
- PulseChannel [pc (pulse-channel alert)]
- PulseChannelRecipient [_ (recipient pc :rasta)]]
- (with-alert-setup
- (let [original-alert-response ((user->client :crowberto) :get 200 (alert-question-url card))]
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Card [card (basic-alert-query)]
+ Pulse [alert (basic-alert)]
+ PulseCard [_ (pulse-card alert card)]
+ PulseChannel [pc (pulse-channel alert)]
+ PulseChannelRecipient [_ (recipient pc :rasta)]]
+ (with-alert-setup
+ (let [original-alert-response ((user->client :crowberto) :get 200 (alert-question-url card))]
- ;; A user can't delete an admin's alert
- (api:delete! :rasta 403 alert)
+ ;; A user can't delete an admin's alert
+ (api:delete! :rasta 403 alert)
- (array-map
- :count-1 (count original-alert-response)
- :response (api:delete! :crowberto 204 alert)
- :count-2 (api:alert-question-count :rasta card))))))
+ (array-map
+ :count-1 (count original-alert-response)
+ :response (api:delete! :crowberto 204 alert)
+ :count-2 (api:alert-question-count :rasta card)))))))
;; An admin can delete a user's alert
(expect
diff --git a/test/metabase/api/automagic_dashboards_test.clj b/test/metabase/api/automagic_dashboards_test.clj
index 7a1a18cc33943565d5dd723dcea39fdfa9907c10..bd82be6d2107682e0ea5242e631c41e863a54844 100644
--- a/test/metabase/api/automagic_dashboards_test.clj
+++ b/test/metabase/api/automagic_dashboards_test.clj
@@ -1,6 +1,5 @@
(ns metabase.api.automagic-dashboards-test
(:require [expectations :refer :all]
- [metabase.api.card-test :refer [with-cards-in-readable-collection]]
[metabase.automagic-dashboards.core :as magic]
[metabase.models
[card :refer [Card]]
@@ -9,8 +8,10 @@
[permissions :as perms]
[permissions-group :as perms-group]
[segment :refer [Segment]]]
- [metabase.test.automagic-dashboards :refer :all]
- [metabase.test.data :as data]
+ [metabase.test
+ [automagic-dashboards :refer :all]
+ [data :as data]
+ [util :as tu]]
[metabase.test.data.users :as test-users]
[toucan.util.test :as tt]))
@@ -61,42 +62,45 @@
(perms/revoke-collection-permissions! (perms-group/all-users) collection-id))
(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id (data/id :venues)
- :collection_id collection-id
- :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
- :source_table (data/id :venues)}
- :type :query
- :database (data/id)}}]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (api-call "question/%s" [card-id] #(revoke-collection-permissions! collection-id))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id (data/id :venues)
+ :collection_id collection-id
+ :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
+ :source_table (data/id :venues)}
+ :type :query
+ :database (data/id)}}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (api-call "question/%s" [card-id] #(revoke-collection-permissions! collection-id)))))
(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id (data/id :venues)
- :collection_id collection-id
- :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
- :source_table (data/id :venues)}
- :type :query
- :database (data/id)}}]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (api-call "question/%s/cell/%s" [card-id (->> [:> [:field-id (data/id :venues :price)] 5]
- (#'magic/encode-base64-json))]
- #(revoke-collection-permissions! collection-id))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id (data/id :venues)
+ :collection_id collection-id
+ :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
+ :source_table (data/id :venues)}
+ :type :query
+ :database (data/id)}}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (api-call "question/%s/cell/%s" [card-id (->> [:> [:field-id (data/id :venues :price)] 5]
+ (#'magic/encode-base64-json))]
+ #(revoke-collection-permissions! collection-id)))))
(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id (data/id :venues)
- :collection_id collection-id
- :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
- :source_table (data/id :venues)}
- :type :query
- :database (data/id)}}]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (api-call "question/%s/cell/%s/rule/example/indepth"
- [card-id (->> [:> [:field-id (data/id :venues :price)] 5]
- (#'magic/encode-base64-json))]
- #(revoke-collection-permissions! collection-id))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id (data/id :venues)
+ :collection_id collection-id
+ :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
+ :source_table (data/id :venues)}
+ :type :query
+ :database (data/id)}}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (api-call "question/%s/cell/%s/rule/example/indepth"
+ [card-id (->> [:> [:field-id (data/id :venues :price)] 5]
+ (#'magic/encode-base64-json))]
+ #(revoke-collection-permissions! collection-id)))))
(expect (api-call "adhoc/%s" [(->> {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 0fbb7252afa73f151a8994a222520d4db6f5b52c..d9c0c4cd7f62aa7d12329dc72354ef4045f3d5bf 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -90,16 +90,17 @@
(defn do-with-cards-in-a-collection [card-or-cards-or-ids grant-perms-fn! f]
- (tt/with-temp Collection [collection]
- ;; put all the Card(s) in our temp `collection`
- (doseq [card-or-id (if (sequential? card-or-cards-or-ids)
- card-or-cards-or-ids
- [card-or-cards-or-ids])]
- (db/update! Card (u/get-id card-or-id) {:collection_id (u/get-id collection)}))
- ;; now use `grant-perms-fn!` to grant appropriate perms
- (grant-perms-fn! (perms-group/all-users) collection)
- ;; call (f)
- (f)))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ ;; put all the Card(s) in our temp `collection`
+ (doseq [card-or-id (if (sequential? card-or-cards-or-ids)
+ card-or-cards-or-ids
+ [card-or-cards-or-ids])]
+ (db/update! Card (u/get-id card-or-id) {:collection_id (u/get-id collection)}))
+ ;; now use `grant-perms-fn!` to grant appropriate perms
+ (grant-perms-fn! (perms-group/all-users) collection)
+ ;; call (f)
+ (f))))
(defmacro with-cards-in-readable-collection
"Execute `body` with `card-or-cards-or-ids` added to a temporary Collection that All Users have read permissions for."
@@ -260,18 +261,16 @@
;; Test that we can make a card
(let [card-name (random-name)]
- (tt/expect-with-temp [Database [db]
- Table [table {:db_id (u/get-id db)}]
- Collection [collection]]
+ (expect
(merge card-defaults
{:name card-name
- :collection_id (u/get-id collection)
- :collection collection
+ :collection_id true
+ :collection true
:creator_id (user->id :rasta)
- :dataset_query (mbql-count-query (u/get-id db) (u/get-id table))
+ :dataset_query true
:visualization_settings {:global {:title nil}}
- :database_id (u/get-id db) ; these should be inferred automatically
- :table_id (u/get-id table)
+ :database_id true
+ :table_id true
:can_write true
:dashboard_count 0
:read_permissions nil
@@ -285,12 +284,21 @@
:date_joined $
:email "rasta@metabase.com"
:id $})})
- (tu/with-model-cleanup [Card]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- (-> ((user->client :rasta) :post 200 "card"
- (assoc (card-with-name-and-query card-name (mbql-count-query (u/get-id db) (u/get-id table)))
- :collection_id (u/get-id collection)))
- (dissoc :created_at :updated_at :id)))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Database [db]
+ Table [table {:db_id (u/get-id db)}]
+ Collection [collection]]
+ (tu/with-model-cleanup [Card]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (-> ((user->client :rasta) :post 200 "card"
+ (assoc (card-with-name-and-query card-name (mbql-count-query (u/get-id db) (u/get-id table)))
+ :collection_id (u/get-id collection)))
+ (dissoc :created_at :updated_at :id)
+ (update :table_id integer?)
+ (update :database_id integer?)
+ (update :collection_id integer?)
+ (update :dataset_query map?)
+ (update :collection map?)))))))
;; Make sure when saving a Card the query metadata is saved (if correct)
(expect
@@ -298,22 +306,23 @@
:display_name "Count Chocula"
:name "count_chocula"
:special_type "type/Number"}]
- (let [metadata [{:base_type :type/Integer
- :display_name "Count Chocula"
- :name "count_chocula"
- :special_type :type/Number}]
- card-name (tu/random-name)]
- (tt/with-temp Collection [collection]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- (tu/with-model-cleanup [Card]
- ;; create a card with the metadata
- ((user->client :rasta) :post 200 "card"
- (assoc (card-with-name-and-query card-name)
- :collection_id (u/get-id collection)
- :result_metadata metadata
- :metadata_checksum (#'results-metadata/metadata-checksum metadata)))
- ;; now check the metadata that was saved in the DB
- (db/select-one-field :result_metadata Card :name card-name)))))
+ (tu/with-all-users-no-root-collection-perms
+ (let [metadata [{:base_type :type/Integer
+ :display_name "Count Chocula"
+ :name "count_chocula"
+ :special_type :type/Number}]
+ card-name (tu/random-name)]
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (tu/with-model-cleanup [Card]
+ ;; create a card with the metadata
+ ((user->client :rasta) :post 200 "card"
+ (assoc (card-with-name-and-query card-name)
+ :collection_id (u/get-id collection)
+ :result_metadata metadata
+ :metadata_checksum (#'results-metadata/metadata-checksum metadata)))
+ ;; now check the metadata that was saved in the DB
+ (db/select-one-field :result_metadata Card :name card-name))))))
;; make sure when saving a Card the correct query metadata is fetched (if incorrect)
(expect
@@ -323,45 +332,48 @@
:special_type "type/Quantity"
:fingerprint {:global {:distinct-count 1},
:type {:type/Number {:min 100.0, :max 100.0, :avg 100.0}}}}]
- (let [metadata [{:base_type :type/Integer
- :display_name "Count Chocula"
- :name "count_chocula"
- :special_type :type/Quantity}]
- card-name (tu/random-name)]
- (tt/with-temp Collection [collection]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- (tu/with-model-cleanup [Card]
- ;; create a card with the metadata
- ((user->client :rasta) :post 200 "card"
- (assoc (card-with-name-and-query card-name)
- :collection_id (u/get-id collection)
- :result_metadata metadata
- :metadata_checksum "ABCDEF")) ; bad checksum
- ;; now check the correct metadata was fetched and was saved in the DB
- (db/select-one-field :result_metadata Card :name card-name)))))
+ (tu/with-all-users-no-root-collection-perms
+ (let [metadata [{:base_type :type/Integer
+ :display_name "Count Chocula"
+ :name "count_chocula"
+ :special_type :type/Quantity}]
+ card-name (tu/random-name)]
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (tu/with-model-cleanup [Card]
+ ;; create a card with the metadata
+ ((user->client :rasta) :post 200 "card"
+ (assoc (card-with-name-and-query card-name)
+ :collection_id (u/get-id collection)
+ :result_metadata metadata
+ :metadata_checksum "ABCDEF")) ; bad checksum
+ ;; now check the correct metadata was fetched and was saved in the DB
+ (db/select-one-field :result_metadata Card :name card-name))))))
;; Make sure we can create a Card with a Collection position
(expect
#metabase.models.card.CardInstance{:collection_id true, :collection_position 1}
- (tu/with-model-cleanup [Card]
- (let [card-name (tu/random-name)]
- (tt/with-temp Collection [collection]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :post 200 "card" (assoc (card-with-name-and-query card-name)
- :collection_id (u/get-id collection), :collection_position 1))
- (some-> (db/select-one [Card :collection_id :collection_position] :name card-name)
- (update :collection_id (partial = (u/get-id collection))))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tu/with-model-cleanup [Card]
+ (let [card-name (tu/random-name)]
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :post 200 "card" (assoc (card-with-name-and-query card-name)
+ :collection_id (u/get-id collection), :collection_position 1))
+ (some-> (db/select-one [Card :collection_id :collection_position] :name card-name)
+ (update :collection_id (partial = (u/get-id collection)))))))))
;; ...but not if we don't have permissions for the Collection
(expect
nil
- (tu/with-model-cleanup [Card]
- (let [card-name (tu/random-name)]
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :post 403 "card" (assoc (card-with-name-and-query card-name)
- :collection_id (u/get-id collection), :collection_position 1))
- (some-> (db/select-one [Card :collection_id :collection_position] :name card-name)
- (update :collection_id (partial = (u/get-id collection))))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tu/with-model-cleanup [Card]
+ (let [card-name (tu/random-name)]
+ (tt/with-temp Collection [collection]
+ ((user->client :rasta) :post 403 "card" (assoc (card-with-name-and-query card-name)
+ :collection_id (u/get-id collection), :collection_position 1))
+ (some-> (db/select-one [Card :collection_id :collection_position] :name card-name)
+ (update :collection_id (partial = (u/get-id collection)))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -395,7 +407,7 @@
:id $
:display "table"
:visualization_settings {}
- :can_write false
+ :can_write true
:created_at $
:database_id (u/get-id db) ; these should be inferred from the dataset_query
:table_id (u/get-id table)
@@ -408,13 +420,14 @@
;; Check that a user without permissions isn't allowed to fetch the card
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Database [db]
- Table [table {:db_id (u/get-id db)}]
- Card [card {:dataset_query (mbql-count-query (u/get-id db) (u/get-id table))}]]
- ;; revoke permissions for default group to this database
- (perms/revoke-permissions! (perms-group/all-users) (u/get-id db))
- ;; now a non-admin user shouldn't be able to fetch this card
- ((user->client :rasta) :get 403 (str "card/" (u/get-id card)))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Database [db]
+ Table [table {:db_id (u/get-id db)}]
+ Card [card {:dataset_query (mbql-count-query (u/get-id db) (u/get-id table))}]]
+ ;; revoke permissions for default group to this database
+ (perms/revoke-permissions! (perms-group/all-users) (u/get-id db))
+ ;; now a non-admin user shouldn't be able to fetch this card
+ ((user->client :rasta) :get 403 (str "card/" (u/get-id card))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -455,10 +468,11 @@
;; we shouldn't be able to update archived status if we don't have collection *write* perms
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [collection]
- Card [card {:collection_id (u/get-id collection)}]]
- (perms/grant-collection-read-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:archived true})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Card [card {:collection_id (u/get-id collection)}]]
+ (perms/grant-collection-read-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:archived true}))))
;; Can we clear the description of a Card? (#4738)
(expect
@@ -561,19 +575,21 @@
;; ...we shouldn't be able to if we don't have permissions for the Collection
(expect
nil
- (tt/with-temp* [Collection [collection]
- Card [card {:collection_id (u/get-id collection)}]]
- ((user->client :rasta) :put 403 (str "card/" (u/get-id card))
- {:collection_position 1})
- (db/select-one-field :collection_position Card :id (u/get-id card))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Card [card {:collection_id (u/get-id collection)}]]
+ ((user->client :rasta) :put 403 (str "card/" (u/get-id card))
+ {:collection_position 1})
+ (db/select-one-field :collection_position Card :id (u/get-id card)))))
(expect
1
- (tt/with-temp* [Collection [collection]
- Card [card {:collection_id (u/get-id collection), :collection_position 1}]]
- ((user->client :rasta) :put 403 (str "card/" (u/get-id card))
- {:collection_position nil})
- (db/select-one-field :collection_position Card :id (u/get-id card))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Card [card {:collection_id (u/get-id collection), :collection_position 1}]]
+ ((user->client :rasta) :put 403 (str "card/" (u/get-id card))
+ {:collection_position nil})
+ (db/select-one-field :collection_position Card :id (u/get-id card)))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | UPDATING THE POSITION OF A CARDS |
@@ -606,15 +622,16 @@
"a" 2
"b" 3
"d" 4}
- (tt/with-temp Collection [collection]
- (with-ordered-items collection [Card a
- Card b
- Card c
- Card d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "card/" (u/get-id c))
- {:collection_position 1})
- (get-name->collection-position :rasta collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (with-ordered-items collection [Card a
+ Card b
+ Card c
+ Card d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "card/" (u/get-id c))
+ {:collection_position 1})
+ (get-name->collection-position :rasta collection)))))
;; Change the position of the 4th card to 1st, all other cards should inc their position
(expect
@@ -622,15 +639,16 @@
"a" 2
"b" 3
"c" 4}
- (tt/with-temp Collection [collection]
- (with-ordered-items collection [Dashboard a
- Dashboard b
- Pulse c
- Card d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "card/" (u/get-id d))
- {:collection_position 1})
- (get-name->collection-position :rasta collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (with-ordered-items collection [Dashboard a
+ Dashboard b
+ Pulse c
+ Card d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "card/" (u/get-id d))
+ {:collection_position 1})
+ (get-name->collection-position :rasta collection)))))
;; Change the position of the 1st card to the 4th, all of the other items dec
(expect
@@ -638,15 +656,16 @@
"c" 2
"d" 3
"a" 4}
- (tt/with-temp Collection [collection]
- (with-ordered-items collection [Card a
- Dashboard b
- Pulse c
- Dashboard d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "card/" (u/get-id a))
- {:collection_position 4})
- (get-name->collection-position :rasta collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (with-ordered-items collection [Card a
+ Dashboard b
+ Pulse c
+ Dashboard d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "card/" (u/get-id a))
+ {:collection_position 4})
+ (get-name->collection-position :rasta collection)))))
;; Change the position of a card from nil to 2nd, should adjust the existing items
(expect
@@ -654,16 +673,17 @@
"b" 2
"c" 3
"d" 4}
- (tt/with-temp* [Collection [{coll-id :id :as collection}]
- Card [_ {:name "a", :collection_id coll-id, :collection_position 1}]
- ;; Card b does not start with a collection_position
- Card [b {:name "b", :collection_id coll-id}]
- Dashboard [_ {:name "c", :collection_id coll-id, :collection_position 2}]
- Card [_ {:name "d", :collection_id coll-id, :collection_position 3}]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "card/" (u/get-id b))
- {:collection_position 2})
- (get-name->collection-position :rasta coll-id)))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{coll-id :id :as collection}]
+ Card [_ {:name "a", :collection_id coll-id, :collection_position 1}]
+ ;; Card b does not start with a collection_position
+ Card [b {:name "b", :collection_id coll-id}]
+ Dashboard [_ {:name "c", :collection_id coll-id, :collection_position 2}]
+ Card [_ {:name "d", :collection_id coll-id, :collection_position 3}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "card/" (u/get-id b))
+ {:collection_position 2})
+ (get-name->collection-position :rasta coll-id))))
;; Update an existing card to no longer have a position, should dec items after it's position
(expect
@@ -671,15 +691,16 @@
"b" nil
"c" 2
"d" 3}
- (tt/with-temp Collection [collection]
- (with-ordered-items collection [Card a
- Card b
- Dashboard c
- Pulse d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "card/" (u/get-id b))
- {:collection_position nil})
- (get-name->collection-position :rasta collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (with-ordered-items collection [Card a
+ Card b
+ Dashboard c
+ Pulse d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "card/" (u/get-id b))
+ {:collection_position nil})
+ (get-name->collection-position :rasta collection)))))
;; Change the collection the card is in, leave the position, should cause old and new collection to have their
;; positions updated
@@ -692,22 +713,23 @@
{"e" 1
"g" 2
"h" 3}]
- (tt/with-temp* [Collection [collection-1]
- Collection [collection-2]]
- (with-ordered-items collection-1 [Dashboard a
- Card b
- Pulse c
- Dashboard d]
- (with-ordered-items collection-2 [Pulse e
- Card f
- Card g
- Dashboard h]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
- ((user->client :rasta) :put 200 (str "card/" (u/get-id f))
- {:collection_id (u/get-id collection-1)})
- [(get-name->collection-position :rasta collection-1)
- (get-name->collection-position :rasta collection-2)]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-1]
+ Collection [collection-2]]
+ (with-ordered-items collection-1 [Dashboard a
+ Card b
+ Pulse c
+ Dashboard d]
+ (with-ordered-items collection-2 [Pulse e
+ Card f
+ Card g
+ Dashboard h]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
+ ((user->client :rasta) :put 200 (str "card/" (u/get-id f))
+ {:collection_id (u/get-id collection-1)})
+ [(get-name->collection-position :rasta collection-1)
+ (get-name->collection-position :rasta collection-2)])))))
;; Change the collection and the position, causing both collections and the updated card to have their order changed
(expect
@@ -719,22 +741,23 @@
{"e" 1
"f" 2
"g" 3}]
- (tt/with-temp* [Collection [collection-1]
- Collection [collection-2]]
- (with-ordered-items collection-1 [Pulse a
- Pulse b
- Dashboard c
- Dashboard d]
- (with-ordered-items collection-2 [Dashboard e
- Dashboard f
- Pulse g
- Card h]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
- ((user->client :rasta) :put 200 (str "card/" (u/get-id h))
- {:collection_position 1, :collection_id (u/get-id collection-1)})
- [(get-name->collection-position :rasta collection-1)
- (get-name->collection-position :rasta collection-2)]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-1]
+ Collection [collection-2]]
+ (with-ordered-items collection-1 [Pulse a
+ Pulse b
+ Dashboard c
+ Dashboard d]
+ (with-ordered-items collection-2 [Dashboard e
+ Dashboard f
+ Pulse g
+ Card h]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
+ ((user->client :rasta) :put 200 (str "card/" (u/get-id h))
+ {:collection_position 1, :collection_id (u/get-id collection-1)})
+ [(get-name->collection-position :rasta collection-1)
+ (get-name->collection-position :rasta collection-2)])))))
;; Add a new card to an existing collection at position 1, will cause all existing positions to increment by 1
(expect
@@ -747,19 +770,20 @@
"b" 2
"c" 3
"d" 4}]
- (tt/with-temp Collection [collection]
- (tu/with-model-cleanup [Card]
- (with-ordered-items collection [Dashboard b
- Pulse c
- Card d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- [(get-name->collection-position :rasta collection)
- (do
- ((user->client :rasta) :post 200 "card"
- (merge (card-with-name-and-query "a")
- {:collection_id (u/get-id collection)
- :collection_position 1}))
- (get-name->collection-position :rasta collection))]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (tu/with-model-cleanup [Card]
+ (with-ordered-items collection [Dashboard b
+ Pulse c
+ Card d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ [(get-name->collection-position :rasta collection)
+ (do
+ ((user->client :rasta) :post 200 "card"
+ (merge (card-with-name-and-query "a")
+ {:collection_id (u/get-id collection)
+ :collection_position 1}))
+ (get-name->collection-position :rasta collection))])))))
;; Add a new card to the end of an existing collection
(expect
@@ -772,19 +796,20 @@
"b" 2
"c" 3
"d" 4}]
- (tt/with-temp Collection [collection]
- (tu/with-model-cleanup [Card]
- (with-ordered-items collection [Card a
- Dashboard b
- Pulse c]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- [(get-name->collection-position :rasta collection)
- (do
- ((user->client :rasta) :post 200 "card"
- (merge (card-with-name-and-query "d")
- {:collection_id (u/get-id collection)
- :collection_position 4}))
- (get-name->collection-position :rasta collection))]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (tu/with-model-cleanup [Card]
+ (with-ordered-items collection [Card a
+ Dashboard b
+ Pulse c]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ [(get-name->collection-position :rasta collection)
+ (do
+ ((user->client :rasta) :post 200 "card"
+ (merge (card-with-name-and-query "d")
+ {:collection_id (u/get-id collection)
+ :collection_position 4}))
+ (get-name->collection-position :rasta collection))])))))
;; When adding a new card to a collection that does not have a position, it should not change existing positions
(expect
@@ -797,19 +822,20 @@
"b" 2
"c" 3
"d" nil}]
- (tt/with-temp Collection [collection]
- (tu/with-model-cleanup [Card]
- (with-ordered-items collection [Pulse a
- Card b
- Dashboard c]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- [(get-name->collection-position :rasta collection)
- (do
- ((user->client :rasta) :post 200 "card"
- (merge (card-with-name-and-query "d")
- {:collection_id (u/get-id collection)
- :collection_position nil}))
- (get-name->collection-position :rasta collection))]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (tu/with-model-cleanup [Card]
+ (with-ordered-items collection [Pulse a
+ Card b
+ Dashboard c]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ [(get-name->collection-position :rasta collection)
+ (do
+ ((user->client :rasta) :post 200 "card"
+ (merge (card-with-name-and-query "d")
+ {:collection_id (u/get-id collection)
+ :collection_position nil}))
+ (get-name->collection-position :rasta collection))])))))
(expect
{"d" 1
@@ -818,17 +844,18 @@
"c" 4
"e" 5
"f" 6}
- (tt/with-temp Collection [collection]
- (with-ordered-items collection [Dashboard a
- Dashboard b
- Card c
- Card d
- Pulse e
- Pulse f]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "card/" (u/get-id d))
- {:collection_position 1, :collection_id (u/get-id collection)})
- (name->position ((user->client :rasta) :get 200 (format "collection/%s/items" (u/get-id collection)))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (with-ordered-items collection [Dashboard a
+ Dashboard b
+ Card c
+ Card d
+ Pulse e
+ Pulse f]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "card/" (u/get-id d))
+ {:collection_position 1, :collection_id (u/get-id collection)})
+ (name->position ((user->client :rasta) :get 200 (format "collection/%s/items" (u/get-id collection))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Card updates that impact alerts |
@@ -1141,23 +1168,25 @@
;; Make sure we can create a card and specify its `collection_id` at the same time
(expect
- (tt/with-temp Collection [collection]
- (tu/with-model-cleanup [Card]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- (let [card ((user->client :rasta) :post 200 "card"
- (assoc (card-with-name-and-query)
- :collection_id (u/get-id collection)))]
- (= (db/select-one-field :collection_id Card :id (u/get-id card))
- (u/get-id collection))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (tu/with-model-cleanup [Card]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (let [card ((user->client :rasta) :post 200 "card"
+ (assoc (card-with-name-and-query)
+ :collection_id (u/get-id collection)))]
+ (= (db/select-one-field :collection_id Card :id (u/get-id card))
+ (u/get-id collection)))))))
;; Make sure we card creation fails if we try to set a `collection_id` we don't have permissions for
(expect
"You don't have permissions to do that."
- (tu/with-model-cleanup [Card]
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :post 403 "card"
- (assoc (card-with-name-and-query)
- :collection_id (u/get-id collection))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tu/with-model-cleanup [Card]
+ (tt/with-temp Collection [collection]
+ ((user->client :rasta) :post 403 "card"
+ (assoc (card-with-name-and-query)
+ :collection_id (u/get-id collection)))))))
;; Make sure we can change the `collection_id` of a Card if it's not in any collection
(expect
@@ -1170,40 +1199,44 @@
;; Make sure we can still change *anything* for a Card if we don't have permissions for the Collection it belongs to
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [collection]
- Card [card {:collection_id (u/get-id collection)}]]
- ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:name "Number of Blueberries Consumed Per Month"})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Card [card {:collection_id (u/get-id collection)}]]
+ ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:name "Number of Blueberries Consumed Per Month"}))))
;; Make sure that we can't change the `collection_id` of a Card if we don't have write permissions for the new
;; collection
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [original-collection]
- Collection [new-collection]
- Card [card {:collection_id (u/get-id original-collection)}]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) original-collection)
- ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:collection_id (u/get-id new-collection)})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [original-collection]
+ Collection [new-collection]
+ Card [card {:collection_id (u/get-id original-collection)}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) original-collection)
+ ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:collection_id (u/get-id new-collection)}))))
;; Make sure that we can't change the `collection_id` of a Card if we don't have write permissions for the current
;; collection
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [original-collection]
- Collection [new-collection]
- Card [card {:collection_id (u/get-id original-collection)}]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) new-collection)
- ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:collection_id (u/get-id new-collection)})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [original-collection]
+ Collection [new-collection]
+ Card [card {:collection_id (u/get-id original-collection)}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) new-collection)
+ ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:collection_id (u/get-id new-collection)}))))
;; But if we do have permissions for both, we should be able to change it.
(expect
- (tt/with-temp* [Collection [original-collection]
- Collection [new-collection]
- Card [card {:collection_id (u/get-id original-collection)}]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) original-collection)
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) new-collection)
- ((user->client :rasta) :put 200 (str "card/" (u/get-id card)) {:collection_id (u/get-id new-collection)})
- (= (db/select-one-field :collection_id Card :id (u/get-id card))
- (u/get-id new-collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [original-collection]
+ Collection [new-collection]
+ Card [card {:collection_id (u/get-id original-collection)}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) original-collection)
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) new-collection)
+ ((user->client :rasta) :put 200 (str "card/" (u/get-id card)) {:collection_id (u/get-id new-collection)})
+ (= (db/select-one-field :collection_id Card :id (u/get-id card))
+ (u/get-id new-collection)))))
;;; ------------------------------ Bulk Collections Update (POST /api/card/collections) ------------------------------
@@ -1267,32 +1300,35 @@
(expect
{:response "You don't have permissions to do that."
:collections [nil nil]}
- (tt/with-temp* [Collection [collection]
- Card [card-1]
- Card [card-2]]
- (POST-card-collections! :rasta 403 collection [card-1 card-2])))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Card [card-1]
+ Card [card-2]]
+ (POST-card-collections! :rasta 403 collection [card-1 card-2]))))
;; Check that we aren't allowed to move Cards if we don't have permissions for source collection
(expect
{:response "You don't have permissions to do that."
:collections ["Horseshoe Collection" "Horseshoe Collection"]}
- (tt/with-temp* [Collection [collection {:name "Horseshoe Collection"}]
- Card [card-1 {:collection_id (u/get-id collection)}]
- Card [card-2 {:collection_id (u/get-id collection)}]]
- (POST-card-collections! :rasta 403 nil [card-1 card-2])))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection {:name "Horseshoe Collection"}]
+ Card [card-1 {:collection_id (u/get-id collection)}]
+ Card [card-2 {:collection_id (u/get-id collection)}]]
+ (POST-card-collections! :rasta 403 nil [card-1 card-2]))))
;; Check that we aren't allowed to move Cards if we don't have permissions for the Card
(expect
{:response "You don't have permissions to do that."
:collections [nil nil]}
- (tt/with-temp* [Collection [collection]
- Database [database]
- Table [table {:db_id (u/get-id database)}]
- Card [card-1 {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]
- Card [card-2 {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]]
- (perms/revoke-permissions! (perms-group/all-users) (u/get-id database))
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- (POST-card-collections! :rasta 403 collection [card-1 card-2])))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Database [database]
+ Table [table {:db_id (u/get-id database)}]
+ Card [card-1 {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]
+ Card [card-2 {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]]
+ (perms/revoke-permissions! (perms-group/all-users) (u/get-id database))
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (POST-card-collections! :rasta 403 collection [card-1 card-2]))))
;; Test that we can bulk move some Cards from one collection to another, while updating the collection position of the
;; old collection and the new collection
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index 1d37635a8eb62086e70b6fc052b0301a262fc2e7..1577c3d23b9d0324f763d08a1706501a39a83501 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -60,11 +60,12 @@
["Our analytics"
"Collection 1"
"Rasta Toucan's Personal Collection"]
- (tt/with-temp* [Collection [collection-1 {:name "Collection 1"}]
- Collection [collection-2 {:name "Collection 2"}]]
- (perms/grant-collection-read-permissions! (group/all-users) collection-1)
- (collection-test/force-create-personal-collections!)
- (map :name ((user->client :rasta) :get 200 "collection"))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-1 {:name "Collection 1"}]
+ Collection [collection-2 {:name "Collection 2"}]]
+ (perms/grant-collection-read-permissions! (group/all-users) collection-1)
+ (collection-test/force-create-personal-collections!)
+ (map :name ((user->client :rasta) :get 200 "collection")))))
;; check that we don't see collections if they're archived
(expect
@@ -103,8 +104,9 @@
;; check that collections detail properly checks permissions
(expect
"You don't have permissions to do that."
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :get 403 (str "collection/" (u/get-id collection)))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ ((user->client :rasta) :get 403 (str "collection/" (u/get-id collection))))))
;;; ----------------------------------------- Cards, Dashboards, and Pulses ------------------------------------------
@@ -125,12 +127,13 @@
(defn- do-with-some-children-of-collection [collection-or-id-or-nil f]
(collection-test/force-create-personal-collections!)
- (let [collection-id-or-nil (when collection-or-id-or-nil
- (u/get-id collection-or-id-or-nil))]
- (tt/with-temp* [Card [_ {:name "Birthday Card", :collection_id collection-id-or-nil}]
- Dashboard [_ {:name "Dine & Dashboard", :collection_id collection-id-or-nil}]
- Pulse [_ {:name "Electro-Magnetic Pulse", :collection_id collection-id-or-nil}]]
- (f))))
+ (tu/with-all-users-no-root-collection-perms
+ (let [collection-id-or-nil (when collection-or-id-or-nil
+ (u/get-id collection-or-id-or-nil))]
+ (tt/with-temp* [Card [_ {:name "Birthday Card", :collection_id collection-id-or-nil}]
+ Dashboard [_ {:name "Dine & Dashboard", :collection_id collection-id-or-nil}]
+ Pulse [_ {:name "Electro-Magnetic Pulse", :collection_id collection-id-or-nil}]]
+ (f)))))
(defmacro ^:private with-some-children-of-collection {:style/indent 1} [collection-or-id-or-nil & body]
`(do-with-some-children-of-collection ~collection-or-id-or-nil (fn [] ~@body)))
@@ -523,9 +526,10 @@
;; check that users without write perms aren't allowed to update a Collection
(expect
"You don't have permissions to do that."
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
- {:name "My Beautiful Collection", :color "#ABCDEF"})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
+ {:name "My Beautiful Collection", :color "#ABCDEF"}))))
;; Archiving a collection should delete any alerts associated with questions in the collection
(expect
@@ -560,9 +564,10 @@
;; I shouldn't be allowed to archive a Collection without proper perms
(expect
"You don't have permissions to do that."
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
- {:archived true})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
+ {:archived true}))))
;; Perms checking should be recursive as well...
;;
@@ -570,11 +575,12 @@
;; also need perms for B
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b {:location (collection/children-location collection-a)}]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:archived true})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-a]
+ Collection [collection-b {:location (collection/children-location collection-a)}]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
+ ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:archived true}))))
;; Can I *change* the `location` of a Collection? (i.e. move it into a different parent Colleciton)
(expect
@@ -596,11 +602,12 @@
;; If I want to move A into B, I should need permissions for both A and B
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:parent_id (u/get-id collection-b)})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-a]
+ Collection [collection-b]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
+ ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:parent_id (u/get-id collection-b)}))))
;; Perms checking should be recursive as well...
;;
@@ -612,13 +619,14 @@
;; C
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b {:location (collection/children-location collection-a)}]
- Collection [collection-c]]
- (doseq [collection [collection-a collection-b]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:parent_id (u/get-id collection-c)})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-a]
+ Collection [collection-b {:location (collection/children-location collection-a)}]
+ Collection [collection-c]]
+ (doseq [collection [collection-a collection-b]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
+ ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:parent_id (u/get-id collection-c)}))))
;; Create A, B, and C; B is a child of A. Grant perms for A and C. Moving A into C should fail because we need perms
@@ -629,13 +637,14 @@
;; C*
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b {:location (collection/children-location collection-a)}]
- Collection [collection-c]]
- (doseq [collection [collection-a collection-c]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:parent_id (u/get-id collection-c)})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-a]
+ Collection [collection-b {:location (collection/children-location collection-a)}]
+ Collection [collection-c]]
+ (doseq [collection [collection-a collection-c]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
+ ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:parent_id (u/get-id collection-c)}))))
;; Create A, B, and C; B is a child of A. Grant perms for B and C. Moving A into C should fail because we need perms
;; for A:
@@ -645,10 +654,11 @@
;; C*
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b {:location (collection/children-location collection-a)}]
- Collection [collection-c]]
- (doseq [collection [collection-b collection-c]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:parent_id (u/get-id collection-c)})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-a]
+ Collection [collection-b {:location (collection/children-location collection-a)}]
+ Collection [collection-c]]
+ (doseq [collection [collection-b collection-c]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
+ ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:parent_id (u/get-id collection-c)}))))
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 107bd4defbe6d5c8a6358b5c9dc769607db0ed79..74f82808c213e54a024cc625e9064704e80e40c5 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -76,11 +76,12 @@
ordered_cards (update :ordered_cards #(mapv dashcard-response %)))))
(defn- do-with-dashboards-in-a-collection [grant-collection-perms-fn! dashboards-or-ids f]
- (tt/with-temp Collection [collection]
- (grant-collection-perms-fn! (group/all-users) collection)
- (doseq [dashboard-or-id dashboards-or-ids]
- (db/update! Dashboard (u/get-id dashboard-or-id) :collection_id (u/get-id collection)))
- (f)))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (grant-collection-perms-fn! (group/all-users) collection)
+ (doseq [dashboard-or-id dashboards-or-ids]
+ (db/update! Dashboard (u/get-id dashboard-or-id) :collection_id (u/get-id collection)))
+ (f))))
(defmacro ^:private with-dashboards-in-readable-collection [dashboards-or-ids & body]
`(do-with-dashboards-in-a-collection perms/grant-collection-read-permissions! ~dashboards-or-ids (fn [] ~@body)))
@@ -138,38 +139,41 @@
:updated_at true
:created_at true
:collection_id true})
- (tu/with-model-cleanup [Dashboard]
- (tt/with-temp Collection [collection]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- (-> ((user->client :rasta) :post 200 "dashboard" {:name "Test Create Dashboard"
- :parameters [{:hash "abc123", :name "test", :type "date"}]
- :collection_id (u/get-id collection)})
- dashboard-response))))
+ (tu/with-all-users-no-root-collection-perms
+ (tu/with-model-cleanup [Dashboard]
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ (-> ((user->client :rasta) :post 200 "dashboard" {:name "Test Create Dashboard"
+ :parameters [{:hash "abc123", :name "test", :type "date"}]
+ :collection_id (u/get-id collection)})
+ dashboard-response)))))
;; Make sure we can create a Dashboard with a Collection position
(expect
#metabase.models.dashboard.DashboardInstance{:collection_id true, :collection_position 1000}
- (tu/with-model-cleanup [Dashboard]
- (let [dashboard-name (tu/random-name)]
- (tt/with-temp Collection [collection]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- ((user->client :rasta) :post 200 "dashboard" {:name dashboard-name
- :collection_id (u/get-id collection)
- :collection_position 1000})
- (some-> (db/select-one [Dashboard :collection_id :collection_position] :name dashboard-name)
- (update :collection_id (partial = (u/get-id collection))))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tu/with-model-cleanup [Dashboard]
+ (let [dashboard-name (tu/random-name)]
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ ((user->client :rasta) :post 200 "dashboard" {:name dashboard-name
+ :collection_id (u/get-id collection)
+ :collection_position 1000})
+ (some-> (db/select-one [Dashboard :collection_id :collection_position] :name dashboard-name)
+ (update :collection_id (partial = (u/get-id collection)))))))))
;; ...but not if we don't have permissions for the Collection
(expect
nil
- (tu/with-model-cleanup [Dashboard]
- (let [dashboard-name (tu/random-name)]
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :post 403 "dashboard" {:name dashboard-name
- :collection_id (u/get-id collection)
- :collection_position 1000})
- (some-> (db/select-one [Dashboard :collection_id :collection_position] :name dashboard-name)
- (update :collection_id (partial = (u/get-id collection))))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tu/with-model-cleanup [Dashboard]
+ (let [dashboard-name (tu/random-name)]
+ (tt/with-temp Collection [collection]
+ ((user->client :rasta) :post 403 "dashboard" {:name dashboard-name
+ :collection_id (u/get-id collection)
+ :collection_position 1000})
+ (some-> (db/select-one [Dashboard :collection_id :collection_position] :name dashboard-name)
+ (update :collection_id (partial = (u/get-id collection)))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -212,16 +216,17 @@
;; ## GET /api/dashboard/:id with a series, should fail if the user doesn't have access to the collection
(expect
"You don't have permissions to do that."
- (tt/with-temp* [Collection [{coll-id :id} {:name "Collection 1"}]
- Dashboard [{dashboard-id :id} {:name "Test Dashboard"
- :creator_id (user->id :crowberto)}]
- Card [{card-id :id} {:name "Dashboard Test Card"
- :collection_id coll-id}]
- Card [{card-id2 :id} {:name "Dashboard Test Card 2"
- :collection_id coll-id}]
- DashboardCard [{dbc_id :id} {:dashboard_id dashboard-id, :card_id card-id}]
- DashboardCardSeries [_ {:dashboardcard_id dbc_id, :card_id card-id2, :position 0}]]
- ((user->client :rasta) :get 403 (format "dashboard/%d" dashboard-id))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{coll-id :id} {:name "Collection 1"}]
+ Dashboard [{dashboard-id :id} {:name "Test Dashboard"
+ :creator_id (user->id :crowberto)}]
+ Card [{card-id :id} {:name "Dashboard Test Card"
+ :collection_id coll-id}]
+ Card [{card-id2 :id} {:name "Dashboard Test Card 2"
+ :collection_id coll-id}]
+ DashboardCard [{dbc_id :id} {:dashboard_id dashboard-id, :card_id card-id}]
+ DashboardCardSeries [_ {:dashboardcard_id dbc_id, :card_id card-id2, :position 0}]]
+ ((user->client :rasta) :get 403 (format "dashboard/%d" dashboard-id)))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -298,59 +303,65 @@
;; ...but if we don't have the Permissions for the old collection, we should get an Exception
(expect
"You don't have permissions to do that."
- (dashboard-test/with-dash-in-collection [db collection dash]
- (tt/with-temp Collection [new-collection]
- ;; grant Permissions for only the *new* collection
- (perms/grant-collection-readwrite-permissions! (group/all-users) new-collection)
- ;; now make an API call to move collections. Should fail
- ((user->client :rasta) :put 403 (str "dashboard/" (u/get-id dash)) {:collection_id (u/get-id new-collection)}))))
+ (tu/with-all-users-no-root-collection-perms
+ (dashboard-test/with-dash-in-collection [db collection dash]
+ (tt/with-temp Collection [new-collection]
+ ;; grant Permissions for only the *new* collection
+ (perms/grant-collection-readwrite-permissions! (group/all-users) new-collection)
+ ;; now make an API call to move collections. Should fail
+ ((user->client :rasta) :put 403 (str "dashboard/" (u/get-id dash)) {:collection_id (u/get-id new-collection)})))))
;; ...and if we don't have the Permissions for the new collection, we should get an Exception
(expect
"You don't have permissions to do that."
- (dashboard-test/with-dash-in-collection [db collection dash]
- (tt/with-temp Collection [new-collection]
- ;; grant Permissions for only the *old* collection
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- ;; now make an API call to move collections. Should fail
- ((user->client :rasta) :put 403 (str "dashboard/" (u/get-id dash)) {:collection_id (u/get-id new-collection)}))))
+ (tu/with-all-users-no-root-collection-perms
+ (dashboard-test/with-dash-in-collection [db collection dash]
+ (tt/with-temp Collection [new-collection]
+ ;; grant Permissions for only the *old* collection
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ ;; now make an API call to move collections. Should fail
+ ((user->client :rasta) :put 403 (str "dashboard/" (u/get-id dash)) {:collection_id (u/get-id new-collection)})))))
;; Can we change the Collection position of a Dashboard?
(expect
1
- (tt/with-temp* [Collection [collection]
- Dashboard [dashboard {:collection_id (u/get-id collection)}]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id dashboard))
- {:collection_position 1})
- (db/select-one-field :collection_position Dashboard :id (u/get-id dashboard))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Dashboard [dashboard {:collection_id (u/get-id collection)}]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id dashboard))
+ {:collection_position 1})
+ (db/select-one-field :collection_position Dashboard :id (u/get-id dashboard)))))
;; ...and unset (unpin) it as well?
(expect
nil
- (tt/with-temp* [Collection [collection]
- Dashboard [dashboard {:collection_id (u/get-id collection), :collection_position 1}]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id dashboard))
- {:collection_position nil})
- (db/select-one-field :collection_position Dashboard :id (u/get-id dashboard))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Dashboard [dashboard {:collection_id (u/get-id collection), :collection_position 1}]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id dashboard))
+ {:collection_position nil})
+ (db/select-one-field :collection_position Dashboard :id (u/get-id dashboard)))))
;; ...we shouldn't be able to if we don't have permissions for the Collection
(expect
nil
- (tt/with-temp* [Collection [collection]
- Dashboard [dashboard {:collection_id (u/get-id collection)}]]
- ((user->client :rasta) :put 403 (str "dashboard/" (u/get-id dashboard))
- {:collection_position 1})
- (db/select-one-field :collection_position Dashboard :id (u/get-id dashboard))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Dashboard [dashboard {:collection_id (u/get-id collection)}]]
+ ((user->client :rasta) :put 403 (str "dashboard/" (u/get-id dashboard))
+ {:collection_position 1})
+ (db/select-one-field :collection_position Dashboard :id (u/get-id dashboard)))))
(expect
1
- (tt/with-temp* [Collection [collection]
- Dashboard [dashboard {:collection_id (u/get-id collection), :collection_position 1}]]
- ((user->client :rasta) :put 403 (str "dashboard/" (u/get-id dashboard))
- {:collection_position nil})
- (db/select-one-field :collection_position Dashboard :id (u/get-id dashboard))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Dashboard [dashboard {:collection_id (u/get-id collection), :collection_position 1}]]
+ ((user->client :rasta) :put 403 (str "dashboard/" (u/get-id dashboard))
+ {:collection_position nil})
+ (db/select-one-field :collection_position Dashboard :id (u/get-id dashboard)))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | UPDATING DASHBOARD COLLECTION POSITIONS |
@@ -362,15 +373,16 @@
"c" 2
"d" 3
"b" 4}
- (tt/with-temp Collection [collection]
- (card-api-test/with-ordered-items collection [Dashboard a
- Dashboard b
- Dashboard c
- Dashboard d]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id b))
- {:collection_position 4})
- (card-api-test/get-name->collection-position :rasta collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (card-api-test/with-ordered-items collection [Dashboard a
+ Dashboard b
+ Dashboard c
+ Dashboard d]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id b))
+ {:collection_position 4})
+ (card-api-test/get-name->collection-position :rasta collection)))))
;; Check that updating a dashboard at position 3 to position 1 will increment the positions before 3, not after
(expect
@@ -378,15 +390,16 @@
"a" 2
"b" 3
"d" 4}
- (tt/with-temp Collection [collection]
- (card-api-test/with-ordered-items collection [Card a
- Pulse b
- Dashboard c
- Dashboard d]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id c))
- {:collection_position 1})
- (card-api-test/get-name->collection-position :rasta collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (card-api-test/with-ordered-items collection [Card a
+ Pulse b
+ Dashboard c
+ Dashboard d]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id c))
+ {:collection_position 1})
+ (card-api-test/get-name->collection-position :rasta collection)))))
;; Check that updating position 1 to 3 will cause b and c to be decremented
(expect
@@ -394,15 +407,16 @@
"c" 2
"a" 3
"d" 4}
- (tt/with-temp Collection [collection]
- (card-api-test/with-ordered-items collection [Dashboard a
- Card b
- Pulse c
- Dashboard d]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id a))
- {:collection_position 3})
- (card-api-test/get-name->collection-position :rasta collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (card-api-test/with-ordered-items collection [Dashboard a
+ Card b
+ Pulse c
+ Dashboard d]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id a))
+ {:collection_position 3})
+ (card-api-test/get-name->collection-position :rasta collection)))))
;; Check that updating position 1 to 4 will cause a through c to be decremented
(expect
@@ -410,15 +424,16 @@
"c" 2
"d" 3
"a" 4}
- (tt/with-temp Collection [collection]
- (card-api-test/with-ordered-items collection [Dashboard a
- Card b
- Pulse c
- Pulse d]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id a))
- {:collection_position 4})
- (card-api-test/get-name->collection-position :rasta collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (card-api-test/with-ordered-items collection [Dashboard a
+ Card b
+ Pulse c
+ Pulse d]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id a))
+ {:collection_position 4})
+ (card-api-test/get-name->collection-position :rasta collection)))))
;; Check that updating position 4 to 1 will cause a through c to be incremented
(expect
@@ -426,15 +441,16 @@
"a" 2
"b" 3
"c" 4}
- (tt/with-temp Collection [collection]
- (card-api-test/with-ordered-items collection [Card a
- Pulse b
- Card c
- Dashboard d]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id d))
- {:collection_position 1})
- (card-api-test/get-name->collection-position :rasta collection))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (card-api-test/with-ordered-items collection [Card a
+ Pulse b
+ Card c
+ Dashboard d]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id d))
+ {:collection_position 1})
+ (card-api-test/get-name->collection-position :rasta collection)))))
;; Check that moving a dashboard to another collection will fixup both collections
(expect
@@ -446,25 +462,26 @@
"f" 3
"g" 4
"h" 5}]
- (tt/with-temp* [Collection [collection-1]
- Collection [collection-2]]
- (card-api-test/with-ordered-items collection-1 [Dashboard a
- Card b
- Card c
- Pulse d]
- (card-api-test/with-ordered-items collection-2 [Pulse e
- Pulse f
- Dashboard g
- Card h]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection-1)
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection-2)
- ;; Move the first dashboard in collection-1 to collection-1
- ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id a))
- {:collection_position 1, :collection_id (u/get-id collection-2)})
- ;; "a" should now be gone from collection-1 and all the existing dashboards bumped down in position
- [(card-api-test/get-name->collection-position :rasta collection-1)
- ;; "a" is now first, all other dashboards in collection-2 bumped down 1
- (card-api-test/get-name->collection-position :rasta collection-2)]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-1]
+ Collection [collection-2]]
+ (card-api-test/with-ordered-items collection-1 [Dashboard a
+ Card b
+ Card c
+ Pulse d]
+ (card-api-test/with-ordered-items collection-2 [Pulse e
+ Pulse f
+ Dashboard g
+ Card h]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection-1)
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection-2)
+ ;; Move the first dashboard in collection-1 to collection-1
+ ((user->client :rasta) :put 200 (str "dashboard/" (u/get-id a))
+ {:collection_position 1, :collection_id (u/get-id collection-2)})
+ ;; "a" should now be gone from collection-1 and all the existing dashboards bumped down in position
+ [(card-api-test/get-name->collection-position :rasta collection-1)
+ ;; "a" is now first, all other dashboards in collection-2 bumped down 1
+ (card-api-test/get-name->collection-position :rasta collection-2)])))))
;; Check that adding a new card at position 3 will cause the existing card at 3 to be incremented
(expect
@@ -475,19 +492,20 @@
"b" 2
"c" 3
"d" 4}]
- (tt/with-temp Collection [collection]
- (tu/with-model-cleanup [Dashboard]
- (card-api-test/with-ordered-items collection [Card a
- Pulse b
- Card d]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- [(card-api-test/get-name->collection-position :rasta collection)
- (do
- ((user->client :rasta) :post 200 "dashboard" {:name "c"
- :parameters [{}]
- :collection_id (u/get-id collection)
- :collection_position 3})
- (card-api-test/get-name->collection-position :rasta collection))]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (tu/with-model-cleanup [Dashboard]
+ (card-api-test/with-ordered-items collection [Card a
+ Pulse b
+ Card d]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ [(card-api-test/get-name->collection-position :rasta collection)
+ (do
+ ((user->client :rasta) :post 200 "dashboard" {:name "c"
+ :parameters [{}]
+ :collection_id (u/get-id collection)
+ :collection_position 3})
+ (card-api-test/get-name->collection-position :rasta collection))])))))
;; Check that adding a new card without a position, leaves the existing positions unchanged
(expect
@@ -498,18 +516,19 @@
"b" 2
"c" nil
"d" 3}]
- (tt/with-temp Collection [collection]
- (tu/with-model-cleanup [Dashboard]
- (card-api-test/with-ordered-items collection [Dashboard a
- Card b
- Pulse d]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- [(card-api-test/get-name->collection-position :rasta collection)
- (do
- ((user->client :rasta) :post 200 "dashboard" {:name "c"
- :parameters [{}]
- :collection_id (u/get-id collection)})
- (card-api-test/get-name->collection-position :rasta collection))]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (tu/with-model-cleanup [Dashboard]
+ (card-api-test/with-ordered-items collection [Dashboard a
+ Card b
+ Pulse d]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ [(card-api-test/get-name->collection-position :rasta collection)
+ (do
+ ((user->client :rasta) :post 200 "dashboard" {:name "c"
+ :parameters [{}]
+ :collection_id (u/get-id collection)})
+ (card-api-test/get-name->collection-position :rasta collection))])))))
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/test/metabase/api/getting_started_guide_test.clj b/test/metabase/api/getting_started_guide_test.clj
deleted file mode 100644
index 6cca9720f8ebd03f67565f42713af693b9e18ee7..0000000000000000000000000000000000000000
--- a/test/metabase/api/getting_started_guide_test.clj
+++ /dev/null
@@ -1,8 +0,0 @@
-(ns metabase.api.getting-started-guide-test
- (:require [expectations :refer :all]
- [metabase.test.data.users :refer [user->client]]))
-
-;; just make sure the getting started guide endpoint works and returns the correct keys
-(expect
- #{:metric_important_fields :most_important_dashboard :things_to_know :important_metrics :important_segments :important_tables :contact}
- (set (keys ((user->client :rasta) :get 200 "getting_started"))))
diff --git a/test/metabase/api/getting_started_test.clj b/test/metabase/api/getting_started_test.clj
deleted file mode 100644
index c7052dad7db73284180df5caa79a85079f0b8cf8..0000000000000000000000000000000000000000
--- a/test/metabase/api/getting_started_test.clj
+++ /dev/null
@@ -1,40 +0,0 @@
-(ns metabase.api.getting-started-test
- (:require [expectations :refer [expect]]
- [metabase.models
- [collection :refer [Collection]]
- [dashboard :refer [Dashboard]]
- [permissions :as perms]
- [permissions-group :as group]]
- [metabase.test.data.users :as test-users]
- [metabase.util :as u]
- [toucan.util.test :as tt]))
-
-;; make sure that we can fetch the GSG stuff with a 'show in getting started' Dashboard
-;; ...but you shouldn't know about it if you don't have read perms for it
-(expect
- {:things_to_know nil
- :contact {:name nil, :email nil}
- :most_important_dashboard false
- :important_metrics []
- :important_tables []
- :important_segments []
- :metric_important_fields {}}
- (tt/with-temp Dashboard [_ {:show_in_getting_started true}]
- (-> ((test-users/user->client :rasta) :get 200 "getting_started")
- (update :most_important_dashboard integer?))))
-
-;; ...but if you do have read perms, then you should get to see it!
-(expect
- {:things_to_know nil
- :contact {:name nil, :email nil}
- :most_important_dashboard true
- :important_metrics []
- :important_tables []
- :important_segments []
- :metric_important_fields {}}
- (tt/with-temp* [Collection [collection]
- Dashboard [_ {:collection_id (u/get-id collection)
- :show_in_getting_started true}]]
- (perms/grant-collection-read-permissions! (group/all-users) collection)
- (-> ((test-users/user->client :rasta) :get 200 "getting_started")
- (update :most_important_dashboard integer?))))
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index f04dbbb2e5b052862cd266ad9a5d3d85cf51a703..2029f44516c33b61983506092e3b878b6de8273f 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -72,14 +72,15 @@
(update card :collection_id boolean)))))
(defn- do-with-pulses-in-a-collection [grant-collection-perms-fn! pulses-or-ids f]
- (tt/with-temp Collection [collection]
- (grant-collection-perms-fn! (perms-group/all-users) collection)
- ;; use db/execute! instead of db/update! so the updated_at field doesn't get automatically updated!
- (when (seq pulses-or-ids)
- (db/execute! {:update Pulse
- :set [[:collection_id (u/get-id collection)]]
- :where [:in :id (set (map u/get-id pulses-or-ids))]}))
- (f)))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (grant-collection-perms-fn! (perms-group/all-users) collection)
+ ;; use db/execute! instead of db/update! so the updated_at field doesn't get automatically updated!
+ (when (seq pulses-or-ids)
+ (db/execute! {:update Pulse
+ :set [[:collection_id (u/get-id collection)]]
+ :where [:in :id (set (map u/get-id pulses-or-ids))]}))
+ (f))))
(defmacro ^:private with-pulses-in-readable-collection [pulses-or-ids & body]
`(do-with-pulses-in-a-collection perms/grant-collection-read-permissions! ~pulses-or-ids (fn [] ~@body)))
@@ -253,33 +254,54 @@
:schedule_hour 12
:recipients []})]
:collection_id true})
- (tt/with-temp Collection [collection]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- (tu/with-model-cleanup [Pulse]
- (card-api-test/with-cards-in-readable-collection [card-1 card-2]
- (-> ((user->client :rasta) :post 200 "pulse" {:name "A Pulse"
- :collection_id (u/get-id collection)
- :cards [{:id (u/get-id card-1)
- :include_csv true
- :include_xls true}
- {:id (u/get-id card-2)
- :include_csv false
- :include_xls false}]
- :channels [daily-email-channel]
- :skip_if_empty false})
- pulse-response
- (update :channels remove-extra-channels-fields))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (tu/with-model-cleanup [Pulse]
+ (card-api-test/with-cards-in-readable-collection [card-1 card-2]
+ (-> ((user->client :rasta) :post 200 "pulse" {:name "A Pulse"
+ :collection_id (u/get-id collection)
+ :cards [{:id (u/get-id card-1)
+ :include_csv true
+ :include_xls true}
+ {:id (u/get-id card-2)
+ :include_csv false
+ :include_xls false}]
+ :channels [daily-email-channel]
+ :skip_if_empty false})
+ pulse-response
+ (update :channels remove-extra-channels-fields)))))))
;; Make sure we can create a Pulse with a Collection position
(expect
#metabase.models.pulse.PulseInstance{:collection_id true, :collection_position 1}
- (tu/with-model-cleanup [Pulse]
- (let [pulse-name (tu/random-name)]
- (tt/with-temp* [Card [card]
- Collection [collection]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- (card-api-test/with-cards-in-readable-collection [card]
- ((user->client :rasta) :post 200 "pulse" {:name pulse-name
+ (tu/with-all-users-no-root-collection-perms
+ (tu/with-model-cleanup [Pulse]
+ (let [pulse-name (tu/random-name)]
+ (tt/with-temp* [Card [card]
+ Collection [collection]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (card-api-test/with-cards-in-readable-collection [card]
+ ((user->client :rasta) :post 200 "pulse" {:name pulse-name
+ :cards [{:id (u/get-id card)
+ :include_csv false
+ :include_xls false}]
+ :channels [daily-email-channel]
+ :skip_if_empty false
+ :collection_id (u/get-id collection)
+ :collection_position 1})
+ (some-> (db/select-one [Pulse :collection_id :collection_position] :name pulse-name)
+ (update :collection_id (partial = (u/get-id collection))))))))))
+
+;; ...but not if we don't have permissions for the Collection
+(expect
+ nil
+ (tu/with-all-users-no-root-collection-perms
+ (tu/with-model-cleanup [Pulse]
+ (let [pulse-name (tu/random-name)]
+ (tt/with-temp* [Card [card]
+ Collection [collection]]
+ ((user->client :rasta) :post 403 "pulse" {:name pulse-name
:cards [{:id (u/get-id card)
:include_csv false
:include_xls false}]
@@ -290,24 +312,6 @@
(some-> (db/select-one [Pulse :collection_id :collection_position] :name pulse-name)
(update :collection_id (partial = (u/get-id collection)))))))))
-;; ...but not if we don't have permissions for the Collection
-(expect
- nil
- (tu/with-model-cleanup [Pulse]
- (let [pulse-name (tu/random-name)]
- (tt/with-temp* [Card [card]
- Collection [collection]]
- ((user->client :rasta) :post 403 "pulse" {:name pulse-name
- :cards [{:id (u/get-id card)
- :include_csv false
- :include_xls false}]
- :channels [daily-email-channel]
- :skip_if_empty false
- :collection_id (u/get-id collection)
- :collection_position 1})
- (some-> (db/select-one [Pulse :collection_id :collection_position] :name pulse-name)
- (update :collection_id (partial = (u/get-id collection))))))))
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | PUT /api/pulse/:id |
@@ -513,18 +517,19 @@
;; Does unarchiving a Pulse affect its Cards & Recipients? It shouldn't. This should behave as a PATCH-style endpoint!
(expect
- (tt/with-temp* [Collection [collection]
- Pulse [pulse {:collection_id (u/get-id collection)}]
- PulseChannel [pc {:pulse_id (u/get-id pulse)}]
- PulseChannelRecipient [pcr {:pulse_channel_id (u/get-id pc), :user_id (user->id :rasta)}]
- Card [card]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "pulse/" (u/get-id pulse))
- {:archived true})
- ((user->client :rasta) :put 200 (str "pulse/" (u/get-id pulse))
- {:archived false})
- (and (db/exists? PulseChannel :id (u/get-id pc))
- (db/exists? PulseChannelRecipient :id (u/get-id pcr)))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Pulse [pulse {:collection_id (u/get-id collection)}]
+ PulseChannel [pc {:pulse_id (u/get-id pulse)}]
+ PulseChannelRecipient [pcr {:pulse_channel_id (u/get-id pc), :user_id (user->id :rasta)}]
+ Card [card]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "pulse/" (u/get-id pulse))
+ {:archived true})
+ ((user->client :rasta) :put 200 (str "pulse/" (u/get-id pulse))
+ {:archived false})
+ (and (db/exists? PulseChannel :id (u/get-id pc))
+ (db/exists? PulseChannelRecipient :id (u/get-id pcr))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -537,15 +542,16 @@
"a" 2
"b" 3
"c" 4}
- (tt/with-temp Collection [{coll-id :id :as collection}]
- (card-api-test/with-ordered-items collection [Pulse a
- Pulse b
- Pulse c
- Pulse d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "pulse/" (u/get-id d))
- {:collection_position 1})
- (card-api-test/get-name->collection-position :rasta coll-id))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [{coll-id :id :as collection}]
+ (card-api-test/with-ordered-items collection [Pulse a
+ Pulse b
+ Pulse c
+ Pulse d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "pulse/" (u/get-id d))
+ {:collection_position 1})
+ (card-api-test/get-name->collection-position :rasta coll-id)))))
;; Change the position of b to 4, will dec c and d
(expect
@@ -553,15 +559,16 @@
"c" 2
"d" 3
"b" 4}
- (tt/with-temp Collection [{coll-id :id :as collection}]
- (card-api-test/with-ordered-items collection [Card a
- Pulse b
- Card c
- Dashboard d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "pulse/" (u/get-id b))
- {:collection_position 4})
- (card-api-test/get-name->collection-position :rasta coll-id))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [{coll-id :id :as collection}]
+ (card-api-test/with-ordered-items collection [Card a
+ Pulse b
+ Card c
+ Dashboard d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "pulse/" (u/get-id b))
+ {:collection_position 4})
+ (card-api-test/get-name->collection-position :rasta coll-id)))))
;; Change the position of d to the 2, should inc b and c
(expect
@@ -569,15 +576,16 @@
"d" 2
"b" 3
"c" 4}
- (tt/with-temp Collection [{coll-id :id :as collection}]
- (card-api-test/with-ordered-items collection [Card a
- Card b
- Dashboard c
- Pulse d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "pulse/" (u/get-id d))
- {:collection_position 2})
- (card-api-test/get-name->collection-position :rasta coll-id))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [{coll-id :id :as collection}]
+ (card-api-test/with-ordered-items collection [Card a
+ Card b
+ Dashboard c
+ Pulse d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "pulse/" (u/get-id d))
+ {:collection_position 2})
+ (card-api-test/get-name->collection-position :rasta coll-id)))))
;; Change the position of a to the 4th, will decrement all existing items
(expect
@@ -585,15 +593,16 @@
"c" 2
"d" 3
"a" 4}
- (tt/with-temp Collection [{coll-id :id :as collection}]
- (card-api-test/with-ordered-items collection [Pulse a
- Dashboard b
- Card c
- Pulse d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "pulse/" (u/get-id a))
- {:collection_position 4})
- (card-api-test/get-name->collection-position :rasta coll-id))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [{coll-id :id :as collection}]
+ (card-api-test/with-ordered-items collection [Pulse a
+ Dashboard b
+ Card c
+ Pulse d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "pulse/" (u/get-id a))
+ {:collection_position 4})
+ (card-api-test/get-name->collection-position :rasta coll-id)))))
;; Change the position of the d to the 1st, will increment all existing items
(expect
@@ -601,15 +610,16 @@
"a" 2
"b" 3
"c" 4}
- (tt/with-temp Collection [{coll-id :id :as collection}]
- (card-api-test/with-ordered-items collection [Dashboard a
- Dashboard b
- Card c
- Pulse d]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ((user->client :rasta) :put 200 (str "pulse/" (u/get-id d))
- {:collection_position 1})
- (card-api-test/get-name->collection-position :rasta coll-id))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [{coll-id :id :as collection}]
+ (card-api-test/with-ordered-items collection [Dashboard a
+ Dashboard b
+ Card c
+ Pulse d]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ ((user->client :rasta) :put 200 (str "pulse/" (u/get-id d))
+ {:collection_position 1})
+ (card-api-test/get-name->collection-position :rasta coll-id)))))
;; Check that no position change, but changing collections still triggers a fixup of both collections
;; Moving `c` from collection-1 to collection-2, `c` is now at position 3 in collection 2
@@ -622,22 +632,23 @@
"c" 3
"g" 4
"h" 5}]
- (tt/with-temp* [Collection [collection-1]
- Collection [collection-2]]
- (card-api-test/with-ordered-items collection-1 [Pulse a
- Card b
- Pulse c
- Dashboard d]
- (card-api-test/with-ordered-items collection-2 [Card e
- Card f
- Dashboard g
- Dashboard h]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
- ((user->client :rasta) :put 200 (str "pulse/" (u/get-id c))
- {:collection_id (u/get-id collection-2)})
- [(card-api-test/get-name->collection-position :rasta (u/get-id collection-1))
- (card-api-test/get-name->collection-position :rasta (u/get-id collection-2))]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-1]
+ Collection [collection-2]]
+ (card-api-test/with-ordered-items collection-1 [Pulse a
+ Card b
+ Pulse c
+ Dashboard d]
+ (card-api-test/with-ordered-items collection-2 [Card e
+ Card f
+ Dashboard g
+ Dashboard h]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
+ ((user->client :rasta) :put 200 (str "pulse/" (u/get-id c))
+ {:collection_id (u/get-id collection-2)})
+ [(card-api-test/get-name->collection-position :rasta (u/get-id collection-1))
+ (card-api-test/get-name->collection-position :rasta (u/get-id collection-2))])))))
;; Check that moving a pulse to another collection, with a changed position will fixup both collections
;; Moving `b` to collection 2, giving it a position of 1
@@ -650,22 +661,23 @@
"f" 3
"g" 4
"h" 5}]
- (tt/with-temp* [Collection [collection-1]
- Collection [collection-2]]
- (card-api-test/with-ordered-items collection-1 [Pulse a
- Pulse b
- Dashboard c
- Card d]
- (card-api-test/with-ordered-items collection-2 [Card e
- Card f
- Pulse g
- Dashboard h]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
- ((user->client :rasta) :put 200 (str "pulse/" (u/get-id b))
- {:collection_id (u/get-id collection-2), :collection_position 1})
- [(card-api-test/get-name->collection-position :rasta (u/get-id collection-1))
- (card-api-test/get-name->collection-position :rasta (u/get-id collection-2))]))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection-1]
+ Collection [collection-2]]
+ (card-api-test/with-ordered-items collection-1 [Pulse a
+ Pulse b
+ Dashboard c
+ Card d]
+ (card-api-test/with-ordered-items collection-2 [Card e
+ Card f
+ Pulse g
+ Dashboard h]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-1)
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-2)
+ ((user->client :rasta) :put 200 (str "pulse/" (u/get-id b))
+ {:collection_id (u/get-id collection-2), :collection_position 1})
+ [(card-api-test/get-name->collection-position :rasta (u/get-id collection-1))
+ (card-api-test/get-name->collection-position :rasta (u/get-id collection-2))])))))
;; Add a new pulse at position 2, causing existing pulses to be incremented
(expect
@@ -676,24 +688,25 @@
"b" 2
"c" 3
"d" 4}]
- (tt/with-temp* [Collection [{coll-id :id :as collection}]
- Card [card-1]]
- (card-api-test/with-cards-in-readable-collection [card-1]
- (card-api-test/with-ordered-items collection [Card a
- Dashboard c
- Pulse d]
- (tu/with-model-cleanup [Pulse]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- [(card-api-test/get-name->collection-position :rasta coll-id)
- (do ((user->client :rasta) :post 200 "pulse" {:name "b"
- :collection_id (u/get-id collection)
- :cards [{:id (u/get-id card-1)
- :include_csv false
- :include_xls false}]
- :channels [daily-email-channel]
- :skip_if_empty false
- :collection_position 2})
- (card-api-test/get-name->collection-position :rasta coll-id))])))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{coll-id :id :as collection}]
+ Card [card-1]]
+ (card-api-test/with-cards-in-readable-collection [card-1]
+ (card-api-test/with-ordered-items collection [Card a
+ Dashboard c
+ Pulse d]
+ (tu/with-model-cleanup [Pulse]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ [(card-api-test/get-name->collection-position :rasta coll-id)
+ (do ((user->client :rasta) :post 200 "pulse" {:name "b"
+ :collection_id (u/get-id collection)
+ :cards [{:id (u/get-id card-1)
+ :include_csv false
+ :include_xls false}]
+ :channels [daily-email-channel]
+ :skip_if_empty false
+ :collection_position 2})
+ (card-api-test/get-name->collection-position :rasta coll-id))]))))))
;; Add a new pulse without a position, should leave existing positions unchanged
(expect
@@ -704,23 +717,24 @@
"b" nil
"c" 2
"d" 3}]
- (tt/with-temp* [Collection [{coll-id :id :as collection}]
- Card [card-1]]
- (card-api-test/with-cards-in-readable-collection [card-1]
- (card-api-test/with-ordered-items collection [Pulse a
- Card c
- Dashboard d]
- (tu/with-model-cleanup [Pulse]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- [(card-api-test/get-name->collection-position :rasta coll-id)
- (do ((user->client :rasta) :post 200 "pulse" {:name "b"
- :collection_id (u/get-id collection)
- :cards [{:id (u/get-id card-1)
- :include_csv false
- :include_xls false}]
- :channels [daily-email-channel]
- :skip_if_empty false})
- (card-api-test/get-name->collection-position :rasta coll-id))])))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{coll-id :id :as collection}]
+ Card [card-1]]
+ (card-api-test/with-cards-in-readable-collection [card-1]
+ (card-api-test/with-ordered-items collection [Pulse a
+ Card c
+ Dashboard d]
+ (tu/with-model-cleanup [Pulse]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ [(card-api-test/get-name->collection-position :rasta coll-id)
+ (do ((user->client :rasta) :post 200 "pulse" {:name "b"
+ :collection_id (u/get-id collection)
+ :cards [{:id (u/get-id card-1)
+ :include_csv false
+ :include_xls false}]
+ :channels [daily-email-channel]
+ :skip_if_empty false})
+ (card-api-test/get-name->collection-position :rasta coll-id))]))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -837,32 +851,33 @@
{:response {:ok true}
:emails (et/email-to :rasta {:subject "Pulse: Daily Sad Toucans"
:body {"Daily Sad Toucans" true}})}
- (tu/with-model-cleanup [Pulse]
- (et/with-fake-inbox
- (data/with-db (data/get-or-create-database! defs/sad-toucan-incidents)
- (tt/with-temp* [Collection [collection]
- Database [db]
- Table [table {:db_id (u/get-id db)}]
- Card [card {:dataset_query {:database (u/get-id db)
- :type "query"
- :query {:source-table (u/get-id table),
- :aggregation {:aggregation-type "count"}}}}]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- (card-api-test/with-cards-in-readable-collection [card]
- (array-map
- :response
- ((user->client :rasta) :post 200 "pulse/test" {:name "Daily Sad Toucans"
- :collection_id (u/get-id collection)
- :cards [{:id (u/get-id card)
- :include_csv false
- :include_xls false}]
- :channels [{:enabled true
- :channel_type "email"
- :schedule_type "daily"
- :schedule_hour 12
- :schedule_day nil
- :recipients [(fetch-user :rasta)]}]
- :skip_if_empty false})
-
- :emails
- (et/regex-email-bodies #"Daily Sad Toucans"))))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tu/with-model-cleanup [Pulse]
+ (et/with-fake-inbox
+ (data/with-db (data/get-or-create-database! defs/sad-toucan-incidents)
+ (tt/with-temp* [Collection [collection]
+ Database [db]
+ Table [table {:db_id (u/get-id db)}]
+ Card [card {:dataset_query {:database (u/get-id db)
+ :type "query"
+ :query {:source-table (u/get-id table),
+ :aggregation {:aggregation-type "count"}}}}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (card-api-test/with-cards-in-readable-collection [card]
+ (array-map
+ :response
+ ((user->client :rasta) :post 200 "pulse/test" {:name "Daily Sad Toucans"
+ :collection_id (u/get-id collection)
+ :cards [{:id (u/get-id card)
+ :include_csv false
+ :include_xls false}]
+ :channels [{:enabled true
+ :channel_type "email"
+ :schedule_type "daily"
+ :schedule_hour 12
+ :schedule_day nil
+ :recipients [(fetch-user :rasta)]}]
+ :skip_if_empty false})
+
+ :emails
+ (et/regex-email-bodies #"Daily Sad Toucans")))))))))
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index a3e4d4c167f82048aa90f50d97565117994af87f..3939f4798add69ada850afceb1a5d8ab14157cfc 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -98,17 +98,19 @@
;; NOTE: Metrics and segments don't have collections, so they'll be returned
(expect
default-metric-segment-results
- (with-search-items-in-root-collection "test"
- (search-request :rasta :q "test")))
+ (tu/with-all-users-no-root-collection-perms
+ (with-search-items-in-root-collection "test"
+ (search-request :rasta :q "test"))))
;; Users that have root collection permissions should get root collection search results
(expect
(set (remove (comp #{"collection"} :model) default-search-results))
- (with-search-items-in-root-collection "test"
- (tt/with-temp* [PermissionsGroup [group]
- PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
- (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection/is-root? true}))
- (search-request :rasta :q "test"))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-search-items-in-root-collection "test"
+ (tt/with-temp* [PermissionsGroup [group]
+ PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
+ (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection/is-root? true}))
+ (search-request :rasta :q "test")))))
;; Users without root collection permissions should still see other collections they have access to
(expect
@@ -116,12 +118,13 @@
(map #(merge default-search-row %)
[{:name "metric test2 metric", :description "Lookin' for a blueberry", :model "metric"}
{:name "segment test2 segment", :description "Lookin' for a blueberry", :model "segment"}]))
- (with-search-items-in-collection {:keys [collection]} "test"
- (with-search-items-in-root-collection "test2"
- (tt/with-temp* [PermissionsGroup [group]
- PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
- (perms/grant-collection-read-permissions! group (u/get-id collection))
- (search-request :rasta :q "test")))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-search-items-in-collection {:keys [collection]} "test"
+ (with-search-items-in-root-collection "test2"
+ (tt/with-temp* [PermissionsGroup [group]
+ PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
+ (perms/grant-collection-read-permissions! group (u/get-id collection))
+ (search-request :rasta :q "test"))))))
;; Users with root collection permissions should be able to search root collection data long with collections they
;; have access to
@@ -130,13 +133,14 @@
(for [row default-search-results
:when (not= "collection" (:model row))]
(update row :name #(str/replace % "test" "test2"))))
- (with-search-items-in-collection {:keys [collection]} "test"
- (with-search-items-in-root-collection "test2"
- (tt/with-temp* [PermissionsGroup [group]
- PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
- (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection/is-root? true}))
- (perms/grant-collection-read-permissions! group collection)
- (search-request :rasta :q "test")))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-search-items-in-collection {:keys [collection]} "test"
+ (with-search-items-in-root-collection "test2"
+ (tt/with-temp* [PermissionsGroup [group]
+ PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
+ (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection/is-root? true}))
+ (perms/grant-collection-read-permissions! group collection)
+ (search-request :rasta :q "test"))))))
;; Users with access to multiple collections should see results from all collections they have access to
(expect
@@ -157,12 +161,13 @@
(map #(merge default-search-row %)
[{:name "metric test2 metric", :description "Lookin' for a blueberry", :model "metric"}
{:name "segment test2 segment", :description "Lookin' for a blueberry", :model "segment"}]))
- (with-search-items-in-collection {coll-1 :collection} "test"
- (with-search-items-in-collection {coll-2 :collection} "test2"
- (tt/with-temp* [PermissionsGroup [group]
- PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
- (perms/grant-collection-read-permissions! group (u/get-id coll-1))
- (search-request :rasta :q "test")))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-search-items-in-collection {coll-1 :collection} "test"
+ (with-search-items-in-collection {coll-2 :collection} "test2"
+ (tt/with-temp* [PermissionsGroup [group]
+ PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
+ (perms/grant-collection-read-permissions! group (u/get-id coll-1))
+ (search-request :rasta :q "test"))))))
;; Favorites are per user, so other user's favorites don't cause search results to be favorited
(expect
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index d2e059683bf5f43ee1a4de3f6674288459eccefc..59125fadce9e3374a21190dc6dc380586e3e4650 100644
--- a/test/metabase/api/user_test.clj
+++ b/test/metabase/api/user_test.clj
@@ -40,7 +40,7 @@
[{:id (test-users/user->id :crowberto), :email "crowberto@metabase.com", :first_name "Crowberto", :last_name "Corv", :common_name "Crowberto Corv"}
{:id (test-users/user->id :lucky), :email "lucky@metabase.com", :first_name "Lucky", :last_name "Pigeon", :common_name "Lucky Pigeon"}
{:id (test-users/user->id :rasta), :email "rasta@metabase.com", :first_name "Rasta", :last_name "Toucan", :common_name "Rasta Toucan"}]
- (do
+ (tu/with-all-users-no-root-collection-perms
;; Delete all the other random Users we've created so far
(test-users/delete-temp-users!)
;; Make sure personal Collections have been created
diff --git a/test/metabase/automagic_dashboards/core_test.clj b/test/metabase/automagic_dashboards/core_test.clj
index 48f55ab8a281218d08aad0c0abbbd6ecec85bdcc..cfca4477d99fbc99e07ccc3e46978a18447774e5 100644
--- a/test/metabase/automagic_dashboards/core_test.clj
+++ b/test/metabase/automagic_dashboards/core_test.clj
@@ -3,9 +3,8 @@
[core :as t]
[format :as t.format]]
[expectations :refer :all]
- [metabase.api.common :as api]
[metabase.automagic-dashboards
- [core :refer :all :as magic]
+ [core :as magic :refer :all]
[rules :as rules]]
[metabase.models
[card :refer [Card]]
@@ -16,9 +15,11 @@
[permissions :as perms]
[permissions-group :as perms-group]
[query :as query]
- [table :refer [Table] :as table]]
- [metabase.test.data :as data]
- [metabase.test.automagic-dashboards :refer :all]
+ [table :as table :refer [Table]]]
+ [metabase.test
+ [automagic-dashboards :refer :all]
+ [data :as data]
+ [util :as tu]]
[metabase.util.date :as date]
[puppetlabs.i18n.core :as i18n :refer [tru]]
[toucan.db :as db]
@@ -104,121 +105,129 @@
(->> (Metric) (every? test-automagic-analysis))))))
(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id (data/id :venues)
- :collection_id collection-id
- :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
- :source_table (data/id :venues)}
- :type :query
- :database (data/id)}}]]
- (with-rasta
- (with-dashboard-cleanup
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (-> card-id Card test-automagic-analysis)))))
-
-(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id (data/id :venues)
- :collection_id collection-id
- :dataset_query {:query {:aggregation [[:count]]
- :breakout [[:field-id (data/id :venues :category_id)]]
- :source_table (data/id :venues)}
- :type :query
- :database (data/id)}}]]
- (with-rasta
- (with-dashboard-cleanup
- (-> card-id Card test-automagic-analysis)))))
-
-(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id nil
- :collection_id collection-id
- :dataset_query {:native {:query "select * from users"}
- :type :native
- :database (data/id)}}]]
- (with-rasta
- (with-dashboard-cleanup
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (-> card-id Card test-automagic-analysis)))))
-
-(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{source-id :id} {:table_id (data/id :venues)
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id (data/id :venues)
:collection_id collection-id
- :dataset_query {:query {:source_table (data/id :venues)}
+ :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
+ :source_table (data/id :venues)}
:type :query
- :database (data/id)}}]
- Card [{card-id :id} {:table_id (data/id :venues)
- :collection_id collection-id
- :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
- :source_table (str "card__" source-id)}
- :type :query
- :database -1337}}]]
- (with-rasta
- (with-dashboard-cleanup
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (-> card-id Card test-automagic-analysis)))))
+ :database (data/id)}}]]
+ (with-rasta
+ (with-dashboard-cleanup
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (-> card-id Card test-automagic-analysis))))))
(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{source-id :id} {:table_id nil
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id (data/id :venues)
:collection_id collection-id
- :dataset_query {:native {:query "select * from users"}
- :type :native
- :database (data/id)}}]
- Card [{card-id :id} {:table_id (data/id :venues)
- :collection_id collection-id
- :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
- :source_table (str "card__" source-id)}
- :type :query
- :database -1337}}]]
- (with-rasta
- (with-dashboard-cleanup
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (-> card-id Card test-automagic-analysis)))))
+ :dataset_query {:query {:aggregation [[:count]]
+ :breakout [[:field-id (data/id :venues :category_id)]]
+ :source_table (data/id :venues)}
+ :type :query
+ :database (data/id)}}]]
+ (with-rasta
+ (with-dashboard-cleanup
+ (-> card-id Card test-automagic-analysis))))))
(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id nil
- :collection_id collection-id
- :dataset_query {:native {:query "select * from users"}
- :type :native
- :database (data/id)}}]]
- (with-rasta
- (with-dashboard-cleanup
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (-> card-id Card test-automagic-analysis)))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id nil
+ :collection_id collection-id
+ :dataset_query {:native {:query "select * from users"}
+ :type :native
+ :database (data/id)}}]]
+ (with-rasta
+ (with-dashboard-cleanup
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (-> card-id Card test-automagic-analysis))))))
+
+(expect
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{source-id :id} {:table_id (data/id :venues)
+ :collection_id collection-id
+ :dataset_query {:query {:source_table (data/id :venues)}
+ :type :query
+ :database (data/id)}}]
+ Card [{card-id :id} {:table_id (data/id :venues)
+ :collection_id collection-id
+ :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
+ :source_table (str "card__" source-id)}
+ :type :query
+ :database -1337}}]]
+ (with-rasta
+ (with-dashboard-cleanup
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (-> card-id Card test-automagic-analysis))))))
+
+(expect
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{source-id :id} {:table_id nil
+ :collection_id collection-id
+ :dataset_query {:native {:query "select * from users"}
+ :type :native
+ :database (data/id)}}]
+ Card [{card-id :id} {:table_id (data/id :venues)
+ :collection_id collection-id
+ :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
+ :source_table (str "card__" source-id)}
+ :type :query
+ :database -1337}}]]
+ (with-rasta
+ (with-dashboard-cleanup
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (-> card-id Card test-automagic-analysis))))))
(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id (data/id :venues)
- :collection_id collection-id
- :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
- :source_table (data/id :venues)}
- :type :query
- :database (data/id)}}]]
- (with-rasta
- (with-dashboard-cleanup
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (-> card-id
- Card
- (test-automagic-analysis [:= [:field-id (data/id :venues :category_id)] 2]))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id nil
+ :collection_id collection-id
+ :dataset_query {:native {:query "select * from users"}
+ :type :native
+ :database (data/id)}}]]
+ (with-rasta
+ (with-dashboard-cleanup
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (-> card-id Card test-automagic-analysis))))))
+
+(expect
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id (data/id :venues)
+ :collection_id collection-id
+ :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
+ :source_table (data/id :venues)}
+ :type :query
+ :database (data/id)}}]]
+ (with-rasta
+ (with-dashboard-cleanup
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (-> card-id
+ Card
+ (test-automagic-analysis [:= [:field-id (data/id :venues :category_id)] 2])))))))
(expect
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id (data/id :venues)
- :collection_id collection-id
- :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
- :source_table (data/id :venues)}
- :type :query
- :database (data/id)}}]]
- (with-rasta
- (with-dashboard-cleanup
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (-> card-id
- Card
- (test-automagic-analysis [:= [:field-id (data/id :venues :category_id)] 2]))))))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id (data/id :venues)
+ :collection_id collection-id
+ :dataset_query {:query {:filter [:> [:field-id (data/id :venues :price)] 10]
+ :source_table (data/id :venues)}
+ :type :query
+ :database (data/id)}}]]
+ (with-rasta
+ (with-dashboard-cleanup
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (-> card-id
+ Card
+ (test-automagic-analysis [:= [:field-id (data/id :venues :category_id)] 2])))))))
(expect
diff --git a/test/metabase/db/migrations_test.clj b/test/metabase/db/migrations_test.clj
index 3e4f70ad67184dce85b0c6e55919d878966d32b7..d769a1305f418fa26f10daafdc4f775e9a51fd2a 100644
--- a/test/metabase/db/migrations_test.clj
+++ b/test/metabase/db/migrations_test.clj
@@ -1,12 +1,18 @@
(ns metabase.db.migrations-test
"Tests to make sure the data migrations actually work as expected and don't break things. Shamefully, we have way less
of these than we should... but that doesn't mean we can't write them for our new ones :)"
- (:require [expectations :refer :all]
+ (:require [clojure.set :as set]
+ [expectations :refer :all]
[medley.core :as m]
[metabase.db.migrations :as migrations]
[metabase.models
[card :refer [Card]]
+ [collection :as collection :refer [Collection]]
+ [dashboard :refer [Dashboard]]
[database :refer [Database]]
+ [permissions :as perms :refer [Permissions]]
+ [permissions-group :as perm-group]
+ [pulse :refer [Pulse]]
[user :refer [User]]]
[metabase.util :as u]
[metabase.util.password :as upass]
@@ -60,3 +66,87 @@
[(upass/verify-password "something secret" ldap-salt ldap-pass)
;; There should be no change for a non ldap user
(upass/verify-password "no change" user-salt user-pass)]))))
+
+
+;;; -------------------------------------------- add-migrated-collections --------------------------------------------
+
+(def ^:private migrated-collection-names #{"Migrated Dashboards" "Migrated Pulses" "Migrated Questions"})
+
+(defn- do-with-add-migrated-collections-cleanup [f]
+ ;; remove the root collection perms if they're already there so we don't see warnings about duplicate perms
+ (perms/revoke-collection-permissions! (perm-group/all-users) collection/root-collection)
+ (try
+ (f)
+ (finally
+ (doseq [collection-name migrated-collection-names]
+ (db/delete! Collection :name collection-name)))))
+
+(defmacro ^:private with-add-migrated-collections-cleanup [& body]
+ `(do-with-add-migrated-collections-cleanup (fn [] ~@body)))
+
+;; Should grant All Users Root Collection read permissions
+(expect
+ #{"/collection/root/"}
+ (with-add-migrated-collections-cleanup
+ (#'migrations/add-migrated-collections)
+ (db/select-field :object Permissions
+ :group_id (u/get-id (perm-group/all-users))
+ :object [:like "/collection/root/%"])))
+
+;; Should create the new Collections
+(expect
+ migrated-collection-names
+ (with-add-migrated-collections-cleanup
+ (tt/with-temp* [Pulse [_]
+ Card [_]
+ Dashboard [_]]
+ (let [collections-before (db/select-field :name Collection)]
+ (#'migrations/add-migrated-collections)
+ (set/difference (db/select-field :name Collection) collections-before)))))
+
+;; Shouldn't create new Collections for models where there's nothing to migrate
+(expect
+ #{"Migrated Dashboards"}
+ (with-add-migrated-collections-cleanup
+ (tt/with-temp* [Dashboard [_]]
+ (let [collections-before (db/select-field :name Collection)]
+ (#'migrations/add-migrated-collections)
+ (set/difference (db/select-field :name Collection) collections-before)))))
+
+;; Should move stuff into the new Collections as appropriate
+(expect
+ (with-add-migrated-collections-cleanup
+ (tt/with-temp Pulse [pulse]
+ (#'migrations/add-migrated-collections)
+ (= (db/select-one-field :collection_id Pulse :id (u/get-id pulse))
+ (db/select-one-id Collection :name "Migrated Pulses")))))
+
+(expect
+ (with-add-migrated-collections-cleanup
+ (tt/with-temp Card [card]
+ (#'migrations/add-migrated-collections)
+ (= (db/select-one-field :collection_id Card :id (u/get-id card))
+ (db/select-one-id Collection :name "Migrated Questions")))))
+
+(expect
+ (with-add-migrated-collections-cleanup
+ (tt/with-temp Dashboard [dashboard]
+ (#'migrations/add-migrated-collections)
+ (= (db/select-one-field :collection_id Dashboard :id (u/get-id dashboard))
+ (db/select-one-id Collection :name "Migrated Dashboards")))))
+
+;; All Users shouldn't get any permissions for the 'migrated' groups
+(expect
+ []
+ (with-add-migrated-collections-cleanup
+ (tt/with-temp* [Pulse [_ {:creator_id 1}]
+ Card [_]
+ Dashboard [_]]
+ (#'migrations/add-migrated-collections)
+ (db/select Permissions
+ {:where [:and
+ [:= :group_id (u/get-id (perm-group/all-users))]
+ (cons
+ :or
+ (for [migrated-collection-id (db/select-ids Collection :name [:in migrated-collection-names])]
+ [:like :object (format "/collection/%d/%%" migrated-collection-id)]))]}))))
diff --git a/test/metabase/models/collection_test.clj b/test/metabase/models/collection_test.clj
index 8eb19cb86089d0076a2aea9f264b9b144bc0adac..787d2add032f3c324daf191e109e1ea976c7c259 100644
--- a/test/metabase/models/collection_test.clj
+++ b/test/metabase/models/collection_test.clj
@@ -2,6 +2,7 @@
(:refer-clojure :exclude [ancestors descendants])
(:require [clojure.string :as str]
[expectations :refer :all]
+ [medley.core :as m]
[metabase.api.common :refer [*current-user-id* *current-user-permissions-set*]]
[metabase.models
[card :refer [Card]]
@@ -30,6 +31,15 @@
(defn- lucky-collection-children-location []
(collection/children-location (collection/user->personal-collection (test-users/user->id :lucky))))
+(defn- replace-collection-ids
+ "In Collection perms `graph`, replace instances of the ID of `collection-or-id` with `:COLLECTION`, making it possible
+ to write tests that don't need to know its actual numeric ID."
+ [collection-or-id graph]
+ (update graph :groups (partial m/map-vals (partial m/map-keys (fn [collection-id]
+ (if (= collection-id (u/get-id collection-or-id))
+ :COLLECTION
+ collection-id))))))
+
;; test that we can create a new Collection with valid inputs
(expect
{:name "My Favorite Cards"
@@ -130,35 +140,40 @@
:groups {(u/get-id (group/all-users)) {:root :none}
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}}}
- (graph :clear-revisions? true))
+ (tu/with-all-users-no-root-collection-perms
+ (graph :clear-revisions? true)))
;; Creating a new Collection shouldn't give perms to anyone but admins
-(tt/expect-with-temp [Collection [collection]]
+(expect
{:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none, (u/get-id collection) :none}
- (u/get-id (group/metabot)) {:root :none, (u/get-id collection) :none}
- (u/get-id (group/admin)) {:root :write, (u/get-id collection) :write}}}
- (graph :clear-revisions? true))
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (replace-collection-ids collection (graph :clear-revisions? true)))))
;; make sure read perms show up correctly
-(tt/expect-with-temp [Collection [collection]]
+(expect
{:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none, (u/get-id collection) :read}
- (u/get-id (group/metabot)) {:root :none, (u/get-id collection) :none}
- (u/get-id (group/admin)) {:root :write, (u/get-id collection) :write}}}
- (do
- (perms/grant-collection-read-permissions! (group/all-users) collection)
- (graph :clear-revisions? true)))
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :read}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-read-permissions! (group/all-users) collection)
+ (replace-collection-ids collection (graph :clear-revisions? true)))))
;; make sure we can grant write perms for new collections (!)
-(tt/expect-with-temp [Collection [collection]]
+(expect
{:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none, (u/get-id collection) :write}
- (u/get-id (group/metabot)) {:root :none, (u/get-id collection) :none}
- (u/get-id (group/admin)) {:root :write, (u/get-id collection) :write}}}
- (do
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- (graph :clear-revisions? true)))
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :write}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ (replace-collection-ids collection (graph :clear-revisions? true)))))
;; make sure a non-magical group will show up
(tt/expect-with-temp [PermissionsGroup [new-group]]
@@ -167,7 +182,8 @@
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}
(u/get-id new-group) {:root :none}}}
- (graph :clear-revisions? true))
+ (tu/with-all-users-no-root-collection-perms
+ (graph :clear-revisions? true)))
;; How abut *read* permissions for the Root Collection?
(tt/expect-with-temp [PermissionsGroup [new-group]]
@@ -176,7 +192,7 @@
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}
(u/get-id new-group) {:root :read}}}
- (do
+ (tu/with-all-users-no-root-collection-perms
(perms/grant-collection-read-permissions! new-group collection/root-collection)
(graph :clear-revisions? true)))
@@ -187,7 +203,7 @@
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}
(u/get-id new-group) {:root :write}}}
- (do
+ (tu/with-all-users-no-root-collection-perms
(perms/grant-collection-readwrite-permissions! new-group collection/root-collection)
(graph :clear-revisions? true)))
@@ -199,9 +215,10 @@
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}}}
;; need to bind *current-user-id* or the Revision won't get updated
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (graph :clear-revisions? true))
- (graph)))
+ (tu/with-all-users-no-root-collection-perms
+ (binding [*current-user-id* (test-users/user->id :crowberto)]
+ (collection/update-graph! (graph :clear-revisions? true))
+ (graph))))
;; Can we give someone read perms via the graph?
(tt/expect-with-temp [Collection [collection]]
@@ -209,36 +226,41 @@
:groups {(u/get-id (group/all-users)) {:root :none, (u/get-id collection) :read}
(u/get-id (group/metabot)) {:root :none, (u/get-id collection) :none}
(u/get-id (group/admin)) {:root :write, (u/get-id collection) :write}}}
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id (group/all-users)) (u/get-id collection)]
- :read))
- (graph)))
+ (tu/with-all-users-no-root-collection-perms
+ (binding [*current-user-id* (test-users/user->id :crowberto)]
+ (collection/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id (group/all-users)) (u/get-id collection)]
+ :read))
+ (graph))))
;; can we give them *write* perms?
-(tt/expect-with-temp [Collection [collection]]
- {:revision 1
- :groups {(u/get-id (group/all-users)) {:root :none, (u/get-id collection) :write}
- (u/get-id (group/metabot)) {:root :none, (u/get-id collection) :none}
- (u/get-id (group/admin)) {:root :write, (u/get-id collection) :write}}}
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id (group/all-users)) (u/get-id collection)]
- :write))
- (graph)))
+(expect
+ {:revision 1
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :write}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (binding [*current-user-id* (test-users/user->id :crowberto)]
+ (collection/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id (group/all-users)) (u/get-id collection)]
+ :write))
+ (replace-collection-ids collection (graph))))))
;; can we *revoke* perms?
-(tt/expect-with-temp [Collection [collection]]
+(expect
{:revision 1
- :groups {(u/get-id (group/all-users)) {:root :none, (u/get-id collection) :none}
- (u/get-id (group/metabot)) {:root :none, (u/get-id collection) :none}
- (u/get-id (group/admin)) {:root :write, (u/get-id collection) :write}}}
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (perms/grant-collection-read-permissions! (group/all-users) collection)
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id (group/all-users)) (u/get-id collection)]
- :none))
- (graph)))
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [collection]
+ (binding [*current-user-id* (test-users/user->id :crowberto)]
+ (perms/grant-collection-read-permissions! (group/all-users) collection)
+ (collection/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id (group/all-users)) (u/get-id collection)]
+ :none))
+ (replace-collection-ids collection (graph))))))
;; How abut *read* permissions for the Root Collection?
(tt/expect-with-temp [PermissionsGroup [new-group]]
@@ -247,11 +269,12 @@
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}
(u/get-id new-group) {:root :read}}}
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id new-group) :root]
- :read))
- (graph)))
+ (tu/with-all-users-no-root-collection-perms
+ (binding [*current-user-id* (test-users/user->id :crowberto)]
+ (collection/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id new-group) :root]
+ :read))
+ (graph))))
;; How about granting *write* permissions for the Root Collection?
(tt/expect-with-temp [PermissionsGroup [new-group]]
@@ -260,11 +283,12 @@
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}
(u/get-id new-group) {:root :write}}}
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id new-group) :root]
- :write))
- (graph)))
+ (tu/with-all-users-no-root-collection-perms
+ (binding [*current-user-id* (test-users/user->id :crowberto)]
+ (collection/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id new-group) :root]
+ :write))
+ (graph))))
;; can we *revoke* RootCollection perms?
(tt/expect-with-temp [PermissionsGroup [new-group]]
@@ -273,12 +297,13 @@
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}
(u/get-id new-group) {:root :none}}}
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (perms/grant-collection-readwrite-permissions! new-group collection/root-collection)
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id new-group) :root]
- :none))
- (graph)))
+ (tu/with-all-users-no-root-collection-perms
+ (binding [*current-user-id* (test-users/user->id :crowberto)]
+ (perms/grant-collection-readwrite-permissions! new-group collection/root-collection)
+ (collection/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id new-group) :root]
+ :none))
+ (graph))))
;; Make sure that personal Collections *do not* appear in the Collections graph
(expect
@@ -286,7 +311,7 @@
:groups {(u/get-id (group/all-users)) {:root :none}
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}}}
- (do
+ (tu/with-all-users-no-root-collection-perms
(force-create-personal-collections!)
(graph :clear-revisions? true)))
@@ -304,7 +329,7 @@
:groups {(u/get-id (group/all-users)) {:root :none}
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}}}
- (do
+ (tu/with-all-users-no-root-collection-perms
(u/ignore-exceptions
(let [lucky-personal-collection-id (u/get-id (collection/user->personal-collection (test-users/user->id :lucky)))]
(collection/update-graph! (assoc-in (graph :clear-revisions? true)
@@ -318,8 +343,9 @@
:groups {(u/get-id (group/all-users)) {:root :none}
(u/get-id (group/metabot)) {:root :none}
(u/get-id (group/admin)) {:root :write}}}
- (tt/with-temp Collection [_ {:location (lucky-collection-children-location)}]
- (graph)))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp Collection [_ {:location (lucky-collection-children-location)}]
+ (graph))))
;; ...and that you can't be sneaky and try to edit them either...
(expect
@@ -339,14 +365,15 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(defn do-with-collection-hierarchy [a-fn]
- (tt/with-temp* [Collection [a {:name "A"}]
- Collection [b {:name "B", :location (collection/location-path a)}]
- Collection [c {:name "C", :location (collection/location-path a)}]
- Collection [d {:name "D", :location (collection/location-path a c)}]
- Collection [e {:name "E", :location (collection/location-path a c d)}]
- Collection [f {:name "F", :location (collection/location-path a c)}]
- Collection [g {:name "G", :location (collection/location-path a c f)}]]
- (a-fn {:a a, :b b, :c c, :d d, :e e, :f f, :g g})))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [a {:name "A"}]
+ Collection [b {:name "B", :location (collection/location-path a)}]
+ Collection [c {:name "C", :location (collection/location-path a)}]
+ Collection [d {:name "D", :location (collection/location-path a c)}]
+ Collection [e {:name "E", :location (collection/location-path a c d)}]
+ Collection [f {:name "F", :location (collection/location-path a c)}]
+ Collection [g {:name "G", :location (collection/location-path a c f)}]]
+ (a-fn {:a a, :b b, :c c, :d d, :e e, :f f, :g g}))))
(defmacro with-collection-hierarchy
"Run `body` with a hierarchy of Collections that looks like:
@@ -854,15 +881,18 @@
;; |
;; +-> F -> G
-(defn- perms-path-ids->names
- "Given a set of permissions and the `collections` map returned by the `with-collection-hierarchy` macro above, replace
- the numeric IDs in the permissions paths with corresponding Collection names, making our tests easier to read."
+(defn perms-path-ids->names
+ "Given a set of permissions and `collections` (the map returned by the `with-collection-hierarchy` macro above, or a
+ sequential collection), replace the numeric IDs in the permissions paths with corresponding Collection names, making
+ our tests easier to read."
[collections perms-set]
;; first build a function that will replace any instances of numeric IDs with their respective names
;; e.g. /123/ would become something like /A/
;; Do this by composing together a series of functions that will handle one string replacement for each ID + name
;; pair
- (let [replace-ids-with-names (reduce comp (for [{:keys [id name]} (vals collections)]
+ (let [replace-ids-with-names (reduce comp (for [{:keys [id name]} (if (sequential? collections)
+ collections
+ (vals collections))]
#(str/replace % (re-pattern (format "/%d/" id)) (str "/" name "/"))))]
(set (for [perms-path perms-set]
(replace-ids-with-names perms-path)))))
diff --git a/test/metabase/models/dashboard_test.clj b/test/metabase/models/dashboard_test.clj
index e057ddd370b309f357038ad6f4afe81641c3945c..c413a5b03993a81dd04ed390d16bfbd8c776490f 100644
--- a/test/metabase/models/dashboard_test.clj
+++ b/test/metabase/models/dashboard_test.clj
@@ -183,15 +183,16 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(defn do-with-dash-in-collection [f]
- (tt/with-temp* [Collection [collection]
- Dashboard [dash {:collection_id (u/get-id collection)}]
- Database [db {:engine :h2}]
- Table [table {:db_id (u/get-id db)}]
- Card [card {:dataset_query {:database (u/get-id db)
- :type :query
- :query {:source-table (u/get-id table)}}}]
- DashboardCard [_ {:dashboard_id (u/get-id dash), :card_id (u/get-id card)}]]
- (f db collection dash)))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Dashboard [dash {:collection_id (u/get-id collection)}]
+ Database [db {:engine :h2}]
+ Table [table {:db_id (u/get-id db)}]
+ Card [card {:dataset_query {:database (u/get-id db)
+ :type :query
+ :query {:source-table (u/get-id table)}}}]
+ DashboardCard [_ {:dashboard_id (u/get-id dash), :card_id (u/get-id card)}]]
+ (f db collection dash))))
(defmacro with-dash-in-collection
"Execute `body` with a Dashboard in a Collection. Dashboard will contain one Card in a Database."
diff --git a/test/metabase/models/pulse_test.clj b/test/metabase/models/pulse_test.clj
index 7b68c6ad4bc7cf5a22676471f84f8353ef2bdffc..410509259c19b8ac4e4bc0b346a9414f1d23296d 100644
--- a/test/metabase/models/pulse_test.clj
+++ b/test/metabase/models/pulse_test.clj
@@ -223,15 +223,16 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(defn do-with-pulse-in-collection [f]
- (tt/with-temp* [Collection [collection]
- Pulse [pulse {:collection_id (u/get-id collection)}]
- Database [db {:engine :h2}]
- Table [table {:db_id (u/get-id db)}]
- Card [card {:dataset_query {:database (u/get-id db)
- :type :query
- :query {:source-table (u/get-id table)}}}]
- PulseCard [_ {:pulse_id (u/get-id pulse), :card_id (u/get-id card)}]]
- (f db collection pulse card)))
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [collection]
+ Pulse [pulse {:collection_id (u/get-id collection)}]
+ Database [db {:engine :h2}]
+ Table [table {:db_id (u/get-id db)}]
+ Card [card {:dataset_query {:database (u/get-id db)
+ :type :query
+ :query {:source-table (u/get-id table)}}}]
+ PulseCard [_ {:pulse_id (u/get-id pulse), :card_id (u/get-id card)}]]
+ (f db collection pulse card))))
(defmacro with-pulse-in-collection
"Execute `body` with a temporary Pulse, in a Colleciton, containing a single Card."
diff --git a/test/metabase/models/user_test.clj b/test/metabase/models/user_test.clj
index 26e67a8e4841172634c2c88c03e8f207d2dc93b8..32b89ddd11f7a8d813d61353a8259d4786df8993 100644
--- a/test/metabase/models/user_test.clj
+++ b/test/metabase/models/user_test.clj
@@ -6,6 +6,7 @@
[http-client :as http]]
[metabase.models
[collection :as collection :refer [Collection]]
+ [collection-test :as collection-test]
[permissions :as perms]
[permissions-group :refer [PermissionsGroup]]
[permissions-group-membership :refer [PermissionsGroupMembership]]
@@ -38,26 +39,32 @@
perms-path)))
(expect
#{(perms/collection-readwrite-path (collection/user->personal-collection (user->id :lucky)))}
- (-> (user/permissions-set (user->id :lucky))
- remove-non-collection-perms))
+ (tu/with-all-users-no-root-collection-perms
+ (-> (user/permissions-set (user->id :lucky))
+ remove-non-collection-perms)))
;; ...and for any descendant Collections of my Personal Collection?
-(tt/expect-with-temp [Collection [child-collection {:location (collection/children-location
- (collection/user->personal-collection
- (user->id :lucky)))}]
- Collection [grandchild-collection {:location (collection/children-location child-collection)}]]
+(expect
#{(perms/collection-readwrite-path (collection/user->personal-collection (user->id :lucky)))
- (perms/collection-readwrite-path child-collection)
- (perms/collection-readwrite-path grandchild-collection)}
- (-> (user/permissions-set (user->id :lucky))
- remove-non-collection-perms))
+ "/collection/child/"
+ "/collection/grandchild/"}
+ (tu/with-all-users-no-root-collection-perms
+ (tt/with-temp* [Collection [child-collection {:name "child"
+ :location (collection/children-location
+ (collection/user->personal-collection
+ (user->id :lucky)))}]
+ Collection [grandchild-collection {:name "grandchild"
+ :location (collection/children-location child-collection)}]]
+ (->> (user/permissions-set (user->id :lucky))
+ remove-non-collection-perms
+ (collection-test/perms-path-ids->names [child-collection grandchild-collection])))))
;;; Tests for invite-user and create-new-google-auth-user!
(defn- maybe-accept-invite!
- "Accept an invite if applicable. Look in the body of the content of the invite email for the reset token
- since this is the only place to get it (the token stored in the DB is an encrypted hash)."
+ "Accept an invite if applicable. Look in the body of the content of the invite email for the reset token since this is
+ the only place to get it (the token stored in the DB is an encrypted hash)."
[new-user-email-address]
(when-let [[{[{invite-email :content}] :body}] (get @email-test/inbox new-user-email-address)]
(let [[_ reset-token] (re-find #"/auth/reset_password/(\d+_[\w_-]+)#new" invite-email)]
@@ -66,7 +73,7 @@
(defn sent-emails
"Fetch the emails that have been sent in the form of a map of email address -> sequence of email subjects.
- For test-writing convenience the random email and names assigned to the new user are replaced with `<New User>`."
+ For test-writing convenience the random email and names assigned to the new user are replaced with `<New User>`."
[new-user-email-address new-user-first-name new-user-last-name]
(into {} (for [[address emails] @email-test/inbox
:let [address (if (= address new-user-email-address)
@@ -78,7 +85,7 @@
(defn- invite-user-accept-and-check-inboxes!
"Create user by passing INVITE-USER-ARGS to `invite-user!` or `create-new-google-auth-user!`,
- and return a map of addresses emails were sent to to the email subjects."
+ and return a map of addresses emails were sent to to the email subjects."
[& {:keys [google-auth? accept-invite? password invitor]
:or {accept-invite? true}}]
(tu/with-temporary-setting-values [site-name "Metabase"]
diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj
index d1d7be8641e11855b9fc6139120956828bdc63e4..064bec917222b1ba94bc91af757bdebdc1b68f64 100644
--- a/test/metabase/query_processor_test/nested_queries_test.clj
+++ b/test/metabase/query_processor_test/nested_queries_test.clj
@@ -525,6 +525,10 @@
(tt/with-temp Database [db {:details (:details (data/db)), :engine "h2"}]
(f db))))
+(defmacro ^:private with-temp-copy-of-test-db {:style/indent 1} [[db-binding] & body]
+ `(do-with-temp-copy-of-test-db (fn [~(or db-binding '_)]
+ ~@body)))
+
(defn- save-card-via-API-with-native-source-query!
"Attempt to save a Card that uses a native source query and belongs to a Collection with `collection-id` via the API
using Rasta. Use this to test how the API endpoint behaves based on certain permissions grants for the `All Users`
@@ -546,14 +550,14 @@
;; Card is in, and write permissions for the Collection you're trying to save the new Card in
(expect
:ok
- (do-with-temp-copy-of-test-db
- (fn [db]
- (tt/with-temp* [Collection [source-card-collection]
- Collection [dest-card-collection]]
- (perms/grant-collection-read-permissions! (group/all-users) source-card-collection)
- (perms/grant-collection-readwrite-permissions! (group/all-users) dest-card-collection)
- (save-card-via-API-with-native-source-query! 200 db source-card-collection dest-card-collection)
- :ok))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-temp-copy-of-test-db [db]
+ (tt/with-temp* [Collection [source-card-collection]
+ Collection [dest-card-collection]]
+ (perms/grant-collection-read-permissions! (group/all-users) source-card-collection)
+ (perms/grant-collection-readwrite-permissions! (group/all-users) dest-card-collection)
+ (save-card-via-API-with-native-source-query! 200 db source-card-collection dest-card-collection)
+ :ok))))
;; however, if we do *not* have read permissions for the source Card's collection we shouldn't be allowed to save the
;; query. This API call should fail
@@ -561,39 +565,39 @@
;; Card in the Root Collection
(expect
"You don't have permissions to do that."
- (do-with-temp-copy-of-test-db
- (fn [db]
- (tt/with-temp Collection [dest-card-collection]
- (perms/grant-collection-readwrite-permissions! (group/all-users) dest-card-collection)
- (save-card-via-API-with-native-source-query! 403 db nil dest-card-collection)))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-temp-copy-of-test-db [db]
+ (tt/with-temp Collection [dest-card-collection]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) dest-card-collection)
+ (save-card-via-API-with-native-source-query! 403 db nil dest-card-collection)))))
;; Card in a different Collection for which we do not have perms
(expect
"You don't have permissions to do that."
- (do-with-temp-copy-of-test-db
- (fn [db]
- (tt/with-temp* [Collection [source-card-collection]
- Collection [dest-card-collection]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) dest-card-collection)
- (save-card-via-API-with-native-source-query! 403 db source-card-collection dest-card-collection)))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-temp-copy-of-test-db [db]
+ (tt/with-temp* [Collection [source-card-collection]
+ Collection [dest-card-collection]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) dest-card-collection)
+ (save-card-via-API-with-native-source-query! 403 db source-card-collection dest-card-collection)))))
;; similarly, if we don't have *write* perms for the dest collection it should also fail
;; Try to save in the Root Collection
(expect
"You don't have permissions to do that."
- (do-with-temp-copy-of-test-db
- (fn [db]
- (tt/with-temp Collection [source-card-collection]
- (perms/grant-collection-read-permissions! (group/all-users) source-card-collection)
- (save-card-via-API-with-native-source-query! 403 db source-card-collection nil)))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-temp-copy-of-test-db [db]
+ (tt/with-temp Collection [source-card-collection]
+ (perms/grant-collection-read-permissions! (group/all-users) source-card-collection)
+ (save-card-via-API-with-native-source-query! 403 db source-card-collection nil)))))
;; Try to save in a different Collection for which we do not have perms
(expect
"You don't have permissions to do that."
- (do-with-temp-copy-of-test-db
- (fn [db]
- (tt/with-temp* [Collection [source-card-collection]
- Collection [dest-card-collection]]
- (perms/grant-collection-read-permissions! (group/all-users) source-card-collection)
- (save-card-via-API-with-native-source-query! 403 db source-card-collection dest-card-collection)))))
+ (tu/with-all-users-no-root-collection-perms
+ (with-temp-copy-of-test-db [db]
+ (tt/with-temp* [Collection [source-card-collection]
+ Collection [dest-card-collection]]
+ (perms/grant-collection-read-permissions! (group/all-users) source-card-collection)
+ (save-card-via-API-with-native-source-query! 403 db source-card-collection dest-card-collection)))))
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index 304763012af27fa8462cea0706ca89344c01a66d..6d91fc754bc781a4f07e7169234b32e9c8f4cb1f 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -13,14 +13,15 @@
[metabase.driver.generic-sql :as sql]
[metabase.models
[card :refer [Card]]
- [collection :refer [Collection]]
+ [collection :as collection :refer [Collection]]
[dashboard :refer [Dashboard]]
[dashboard-card-series :refer [DashboardCardSeries]]
[database :refer [Database]]
[dimension :refer [Dimension]]
[field :refer [Field]]
[metric :refer [Metric]]
- [permissions-group :refer [PermissionsGroup]]
+ [permissions :as perms :refer [Permissions]]
+ [permissions-group :as group :refer [PermissionsGroup]]
[pulse :refer [Pulse]]
[pulse-card :refer [PulseCard]]
[pulse-channel :refer [PulseChannel]]
@@ -29,12 +30,12 @@
[setting :as setting]
[table :refer [Table]]
[user :refer [User]]]
- [metabase.query-processor.util :as qputil]
[metabase.query-processor.middleware.expand :as ql]
+ [metabase.query-processor.util :as qputil]
[metabase.test.data :as data]
[metabase.test.data
- [datasets :refer [*driver*]]
- [dataset-definitions :as defs]]
+ [dataset-definitions :as defs]
+ [datasets :refer [*driver*]]]
[toucan.db :as db]
[toucan.util.test :as test])
(:import com.mchange.v2.c3p0.PooledDataSource
@@ -575,3 +576,19 @@
`(with-redefs [~fn-var (fn [& args#]
(throw (RuntimeException. "Should not be called!")))]
~@body))
+
+(defn do-with-all-users-no-root-collection-perms [f]
+ (try
+ (perms/revoke-collection-permissions! (group/all-users) collection/root-collection)
+ (f)
+ (finally
+ (when-not (db/exists? Permissions
+ :group_id (u/get-id (group/all-users))
+ :object (perms/collection-readwrite-path collection/root-collection))
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection/root-collection)))))
+
+(defmacro with-all-users-no-root-collection-perms
+ "Temporarily remove Root Collection perms for All Users. By default, All Users have full readwrite perms for the Root
+ Collection; use this macro to test situations where an admin has removed them."
+ [& body]
+ `(do-with-all-users-no-root-collection-perms (fn [] ~@body)))