From a1894202ed1d5a2e80221e15e797f435ae2eab22 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Wed, 19 Oct 2022 08:39:48 +0300
Subject: [PATCH] Use "apps" collection namespace for app collections (#25963)
* Create app collections in the "apps" collection namespace
* Add test macro for setting global app permissions for "All Users"
* Introduce app permission audit table
---
.clj-kondo/config.edn | 1 +
.../models/permissions/app_permissions.clj | 118 ++++------
.../permissions/app_permissions_test.clj | 134 ++++++++---
.../models/entity_id_test.clj | 1 +
resources/migrations/000_migrations.yaml | 49 ++++
src/metabase/api/app.clj | 23 +-
src/metabase/api/collection.clj | 7 +-
src/metabase/api/search.clj | 4 +-
src/metabase/cmd/copy.clj | 2 +
src/metabase/models.clj | 3 +
src/metabase/models/app.clj | 14 +-
src/metabase/models/app/graph.clj | 65 ++----
.../models/app_permission_graph_revision.clj | 22 ++
src/metabase/models/collection.clj | 24 +-
src/metabase/models/collection/graph.clj | 33 ++-
src/metabase/models/permissions.clj | 11 +-
test/metabase/api/activity_test.clj | 4 +-
test/metabase/api/app_test.clj | 218 ++++++++++--------
test/metabase/api/bookmark_test.clj | 93 ++++----
test/metabase/api/collection_test.clj | 146 +++++++-----
test/metabase/api/search_test.clj | 36 +--
test/metabase/models/card_test.clj | 4 +-
.../metabase/models/collection/graph_test.clj | 5 +-
test/metabase/models/collection_test.clj | 2 +-
test/metabase/models/dashboard_test.clj | 4 +-
test/metabase/models/pulse_test.clj | 4 +-
test/metabase/test.clj | 1 +
test/metabase/test/util.clj | 19 ++
28 files changed, 624 insertions(+), 423 deletions(-)
create mode 100644 src/metabase/models/app_permission_graph_revision.clj
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 07a753e529a..8645d909489 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -455,6 +455,7 @@
metabase.api.public-test/with-temp-public-dashboard hooks.common/let-one-with-optional-value
metabase.api.public-test/with-temp-public-dashboard-and-card hooks.common/with-three-bindings
metabase.api.search-test/with-search-items-in-collection hooks.metabase.api.search-test/with-search-items-in-collection
+ metabase.api.search-test/with-search-items-in-app-collection hooks.metabase.api.search-test/with-search-items-in-collection
metabase.api.search-test/with-search-items-in-root-collection hooks.common/do*
metabase.api.user-test/with-temp-user-email hooks.common/with-one-binding
metabase.async.streaming-response-test/with-start-execution-chan hooks.common/with-one-binding
diff --git a/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/app_permissions.clj b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/app_permissions.clj
index f590bb204e6..f24d3288a3e 100644
--- a/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/app_permissions.clj
+++ b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/app_permissions.clj
@@ -3,100 +3,74 @@
on the app collections."
(:require [clojure.data :as data]
[clojure.set :as set]
- [clojure.tools.logging :as log]
- [metabase.models.app.graph :as app.graph]
- [metabase.models.collection-permission-graph-revision :as c-perm-revision
- :refer [CollectionPermissionGraphRevision]]
+ [metabase.models.app-permission-graph-revision :as app-perm-revision
+ :refer [AppPermissionGraphRevision]]
[metabase.models.collection.graph :as graph]
[metabase.models.permissions :as perms]
[metabase.public-settings.premium-features :as premium-features]
[metabase.util.i18n :as i18n]
- [metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db]))
-(def ^:private GroupPermissionsGraph
- "collection-id -> status"
- {su/IntGreaterThanZero app.graph/AppPermissions}) ; be present, which is why it's *optional*
-
-(def ^:private PermissionsGraph
- {:revision s/Int
- :groups {su/IntGreaterThanZero GroupPermissionsGraph}})
-
(defn- check-advanced-permissions []
- (log/fatal "checking advanced permsissions")
(when-not (premium-features/has-feature? :advanced-permissions)
(throw (ex-info (i18n/tru "The granular app permission functionality is only enabled if you have a premium token with the advanced-permissions feature.")
{:status-code 402}))))
(defn- replace-collection-ids
- "Convert the collection group permission graph `g` into an app group
- permission graph or vice versa by removing the root collection and replacing
- the collection IDs by app IDs according to `mapping`.
-
- Note that the app permission graph never contains the root collection. So it's
- removed when converting a collection graph to an app graph and plays no role
- when converting an app graph to a collection graph."
- [g mapping]
- (update-vals g (fn [group-permissions]
- (-> group-permissions
- (dissoc :root)
- (update-keys mapping)))))
-
-(defn- merge-graphs
- [base override]
- (merge-with merge base override))
+ "Convert the collection group permission graph `permission-graph` into an
+ app group permission graph or vice versa by replacing the collection IDs
+ by app IDs according to `mapping`.
+ Stray collections in the :apps namespace are ignored."
+ [group-permissions mapping]
+ (let [full-mapping (assoc mapping :root :root)]
+ (update-vals group-permissions (fn [coll-perms]
+ (into {}
+ (for [[coll-id perm] coll-perms
+ :let [app-id (full-mapping coll-id)]
+ :when app-id]
+ [app-id perm]))))))
-(s/defn graph :- PermissionsGraph
+(s/defn graph :- graph/PermissionsGraph
"Returns the app permission graph. Throws an exception if the
- advanced-permissions feature is not enabled."
+ advanced-permissions feature is not enabled.
+
+ This works by reading the permissions for app collections and replacing
+ the IDs of the collections with the corresponding app IDs."
[]
(check-advanced-permissions)
(db/transaction
- (let [collection-id->app-id (set/map-invert (db/select-id->field :collection_id 'App))]
- (-> collection-id->app-id
- keys
- graph/collection-permission-graph
- (update :groups replace-collection-ids collection-id->app-id)))))
-
-(defn update-collection-graph!
- "Update the Collections permissions graph for Collections of `collection-namespace` (default `nil`, the 'default'
- namespace). This works just like [[metabase.models.permission/update-data-perms-graph!]], but for Collections;
- refer to that function's extensive documentation to get a sense for how this works."
- [old-graph new-graph]
- (let [old-perms (:groups old-graph)
- new-perms (:groups new-graph)
- ;; filter out any groups not in the old graph
- new-perms (select-keys new-perms (keys old-perms))
- ;; filter out any collections not in the old graph
- new-perms (into {} (for [[group-id collection-id->perms] new-perms]
- [group-id (select-keys collection-id->perms (keys (get old-perms group-id)))]))
- [diff-old changes] (data/diff old-perms new-perms)]
- (perms/log-permissions-changes diff-old changes)
- (perms/check-revision-numbers old-graph new-graph)
- (when (seq changes)
- (db/transaction
- (doseq [[group-id new-group-perms] changes
- [collection-id new-perms] new-group-perms]
- (graph/update-collection-permissions! nil group-id collection-id new-perms))
- (perms/save-perms-revision! CollectionPermissionGraphRevision (:revision old-graph)
- (assoc old-graph :namespace nil) changes)))))
+ (let [collection-id->app-id (set/map-invert (db/select-id->field :collection_id 'App))]
+ (-> (graph/graph :apps)
+ (assoc :revision (app-perm-revision/latest-id))
+ (update :groups replace-collection-ids collection-id->app-id)))))
-(s/defn update-graph! :- PermissionsGraph
+(s/defn update-graph! :- graph/PermissionsGraph
"Updates the app permissions according to `new-graph` and returns the
resulting graph as read from the database. Throws an exception if the
advanced-permissions feature is not enabled."
- [new-graph :- PermissionsGraph]
+ [new-graph :- graph/PermissionsGraph]
(check-advanced-permissions)
(db/transaction
- (let [{:keys [revision groups]} new-graph
- app-id->collection-id (db/select-id->field :collection_id 'App)
- collection-ids (vals app-id->collection-id)
- old-graph (graph/graph)
- new-graph (-> old-graph
- (assoc :revision revision)
- (update :groups update-vals (fn [group-permissions]
- (apply dissoc group-permissions collection-ids)))
- (update :groups merge-graphs (replace-collection-ids groups app-id->collection-id)))]
- (update-collection-graph! old-graph new-graph))
+ (let [old-graph (graph)
+ old-perms (:groups old-graph)
+ new-perms (:groups new-graph)
+ ;; filter out any groups not in the old graph
+ new-perms (select-keys new-perms (keys old-perms))
+ ;; filter out any collections not in the old graph
+ new-perms (into {} (for [[group-id collection-id->perms] new-perms]
+ [group-id (select-keys collection-id->perms
+ (keys (get old-perms group-id)))]))
+ [diff-old changes] (data/diff old-perms new-perms)]
+ (perms/log-permissions-changes diff-old changes)
+ (perms/check-revision-numbers old-graph new-graph)
+ (when (seq changes)
+ (let [app-id->collection-id
+ (assoc (db/select-id->field :collection_id 'App) :root :root)]
+ (doseq [[group-id new-group-perms] changes
+ [app-id new-perms] new-group-perms
+ :let [collection-id (app-id->collection-id app-id)]]
+ (graph/update-collection-permissions! :apps group-id collection-id new-perms))
+ (perms/save-perms-revision! AppPermissionGraphRevision (:revision old-graph)
+ old-graph changes))))
(graph)))
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/app_permissions_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/app_permissions_test.clj
index 10413ff5fda..80d47aa0a4a 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/app_permissions_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/app_permissions_test.clj
@@ -1,10 +1,37 @@
(ns metabase-enterprise.advanced-permissions.models.permissions.app-permissions-test
"Tests for /api/collection endpoints."
(:require [clojure.test :refer :all]
- [metabase.models :refer [App Collection PermissionsGroup]]
+ [metabase.models :refer [App Card Collection Dashboard
+ Permissions PermissionsGroup PermissionsGroupMembership]]
+ [metabase.models.interface :as mi]
[metabase.models.permissions-group :as perms-group]
[metabase.public-settings.premium-features-test :as premium-features-test]
- [metabase.test :as mt]))
+ [metabase.test :as mt]
+ [metabase.test.data :as data]
+ [toucan.db :as db]))
+
+(deftest permission-set-test
+ (premium-features-test/with-premium-features #{}
+ (testing "App permission set without advanced permission feature"
+ (is (= #{"/collection/namespace/apps/root/"}
+ (mi/perms-objects-set (mi/instance App) :write)))
+ (is (= #{"/collection/namespace/apps/root/read/"}
+ (mi/perms-objects-set (mi/instance App) :read)))
+ (is (= #{"/collection/namespace/apps/root/read/"}
+ (mi/perms-objects-set (mi/instance App {:collection_id 1}) :read)))
+ (is (= #{"/collection/namespace/apps/root/"}
+ (mi/perms-objects-set (mi/instance App {:collection_id 1}) :write)))))
+
+ (premium-features-test/with-premium-features #{:advanced-permissions}
+ (testing "App permission set with advanced permission feature"
+ (is (= #{"/collection/namespace/apps/root/"}
+ (mi/perms-objects-set (mi/instance App) :write)))
+ (is (= #{"/collection/namespace/apps/root/read/"}
+ (mi/perms-objects-set (mi/instance App) :read)))
+ (is (= #{"/collection/1/read/"}
+ (mi/perms-objects-set (mi/instance App {:collection_id 1}) :read)))
+ (is (= #{"/collection/1/"}
+ (mi/perms-objects-set (mi/instance App {:collection_id 1}) :write))))))
(deftest graph-test
(testing "GET /api/app/graph works only with advanced permissions"
@@ -13,13 +40,13 @@
(mt/user-http-request :crowberto :get 402 "app/graph")))))
(premium-features-test/with-premium-features #{:advanced-permissions}
- (mt/with-temp* [Collection [{app-coll-id :id} {:location "/"}]
+ (mt/with-temp* [Collection [{app-coll-id :id} {:location "/", :namespace :apps}]
App [{app-id :id} {:collection_id app-coll-id}]
PermissionsGroup [{group-id :id}]]
(testing "GET /api/app/graph\n"
(testing "Should be able to fetch the permissions graph for apps"
(is (partial= {(:id (perms-group/admin)) {app-id "write"}
- (:id (perms-group/all-users)) {app-id "write"}
+ (:id (perms-group/all-users)) {app-id "none"}
group-id {app-id "none"}}
(:groups (mt/user-http-request :crowberto :get 200 "app/graph")))))
@@ -27,18 +54,19 @@
(is (= "You don't have permissions to do that."
(mt/user-http-request :rasta :get 403 "app/graph"))))))
- (mt/with-non-admin-groups-no-root-collection-perms
- (mt/with-temp* [Collection [_ {:location "/"}]
- Collection [{app-coll-id :id} {:location "/"}]
- App [{app-id :id} {:collection_id app-coll-id}]
- PermissionsGroup [{group-id :id}]]
- (testing "All users' right to root collection is respected"
- (let [group-perms (:groups (mt/user-http-request :crowberto :get 200 "app/graph"))]
- (is (partial= {(:id (perms-group/admin)) {app-id "write"}
- (:id (perms-group/all-users)) {app-id "none"}
- group-id {app-id "none"}}
- group-perms))
- (is (= #{app-id} (into #{} (mapcat keys) (vals group-perms)))
+ (mt/with-temp* [Collection [_ {:location "/"}]
+ Collection [{app-coll-id :id} {:location "/", :namespace :apps}]
+ App [{app-id :id} {:collection_id app-coll-id}]
+ PermissionsGroup [{group-id :id}]]
+ (testing "All users' right to root collection is respected"
+ (let [group-perms (:groups (mt/user-http-request :crowberto :get 200 "app/graph"))]
+ (is (partial= {(:id (perms-group/admin)) {app-id "write"}
+ (:id (perms-group/all-users)) {app-id "none"}
+ group-id {app-id "none"}}
+ group-perms))
+ (let [app-ids (into #{} (mapcat keys) (vals group-perms))]
+ (is (every? #(contains? app-ids %) [:root app-id]))
+ (is (not (contains? app-ids app-coll-id))
"Shouldn't confuse collection IDs and app IDs")))))))
(deftest graph-update-test
@@ -48,24 +76,60 @@
(mt/user-http-request :crowberto :put 402 "app/graph" {:revision 0
:groups {1 {1 "write"}}})))))
- (premium-features-test/with-premium-features #{:advanced-permissions}
- (mt/with-temp* [Collection [{app-coll-id :id} {:location "/"}]
- App [{app-id :id} {:collection_id app-coll-id}]
- PermissionsGroup [{group-id :id}]]
- (testing "PUT /api/app/graph\n"
- (testing "Should be able to update the permissions graph for apps"
- (let [initial-graph (mt/user-http-request :crowberto :get 200 "app/graph")
- updated-graph (assoc-in initial-graph [:groups group-id app-id] "read")]
- (is (partial= {(:id (perms-group/admin)) {app-id "write"}
- (:id (perms-group/all-users)) {app-id "write"}
- group-id {app-id "none"}}
- (:groups initial-graph))
- "Unexpected initial state")
+ (mt/with-model-cleanup [Card Dashboard Collection Permissions]
+ (premium-features-test/with-premium-features #{:advanced-permissions}
+ (let [max-coll-id (:id (db/select-one [Collection [:%max.id :id]]))
+ max-app-id (:id (db/select-one [App [:%max.id :id]]))]
+ (mt/with-temp* [Collection [{coll0-id :id} {:location "/", :namespace :apps}]
+ Collection [{coll1-id :id} {:location "/", :namespace :apps}]
+ ;; make sure that the :id and :collection_id of the app are different
+ App [{app-id :id :as app} {:collection_id (if (= max-app-id max-coll-id)
+ coll1-id
+ coll0-id)}]
+ PermissionsGroup [{group-id :id}]
+ PermissionsGroupMembership [_ {:user_id (mt/user->id :rasta)
+ :group_id group-id}]]
+ (testing "PUT /api/app/graph\n"
+ (testing "Initial assumptions should hold"
+ (let [initial-graph (mt/user-http-request :crowberto :get 200 "app/graph")]
+ (is (not= app-id (:collection_id app))
+ "The IDs of the app and its collection should be different. Fix the test!")
+ (is (partial= {(:id (perms-group/admin)) {app-id "write"}
+ (:id (perms-group/all-users)) {app-id "none"}
+ group-id {app-id "none"}}
+ (:groups initial-graph))
+ "Unexpected initial state")))
+
+ (testing "Should be able to update the permissions graph for a specific app\n"
+ (let [initial-graph (mt/user-http-request :crowberto :get 200 "app/graph")
+ updated-graph (assoc-in initial-graph [:groups group-id app-id] "write")]
+ (testing "Have to be a superuser"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :put 403 "app/graph" updated-graph))))
+
+ (testing "Cannot scaffold an app without write permission"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :post 403 (format "app/%s/scaffold" app-id)
+ {:table-ids [(data/id :venues)]}))))
+
+ (testing "Superuser can update permissions for an app"
+ (is (= (:groups updated-graph)
+ (:groups (mt/user-http-request :crowberto :put 200 "app/graph"
+ updated-graph)))))
+
+ (testing "Normal user can scaffold an app with write permission"
+ (is (partial= (dissoc app :updated_at :nav_items)
+ (mt/user-http-request :rasta :post 200 (format "app/%s/scaffold" app-id)
+ {:table-ids [(data/id :venues)]}))))))
- (testing "have to be a superuser"
- (is (= "You don't have permissions to do that."
- (mt/user-http-request :rasta :put 403 "app/graph" updated-graph))))
+ (testing "Should be able to update the permissions graph for the root\n"
+ (let [initial-graph (mt/user-http-request :crowberto :get 200 "app/graph")
+ updated-graph (assoc-in initial-graph [:groups group-id :root] "read")]
+ (testing "Have to be a superuser"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :put 403 "app/graph" updated-graph))))
- (testing "superuser can update"
- (is (= (:groups updated-graph)
- (:groups (mt/user-http-request :crowberto :put 200 "app/graph" updated-graph)))))))))))
+ (testing "Superuser can update permissions for the root"
+ (is (= (:groups updated-graph)
+ (:groups (mt/user-http-request :crowberto :put 200 "app/graph"
+ updated-graph)))))))))))))
diff --git a/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj b/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj
index d4e0de1ad0b..63f7c65ef11 100644
--- a/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj
@@ -35,6 +35,7 @@
metabase.models.action.HTTPActionInstance
metabase.models.action.QueryActionInstance
metabase.models.activity.ActivityInstance
+ metabase.models.app_permission_graph_revision.AppPermissionGraphRevisionInstance
metabase.models.application_permissions_revision.ApplicationPermissionsRevisionInstance
metabase.models.bookmark.BookmarkOrderingInstance
metabase.models.bookmark.CardBookmarkInstance
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index d230622e796..0433bc62283 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -12993,6 +12993,55 @@ databaseChangeLog:
tableName: metabase_database
columnName: details
+ - changeSet:
+ id: v45.00-044
+ author: metamben
+ comment: Added 0.45.0 -- create app permission graph revision table
+ changes:
+ - createTable:
+ tableName: app_permission_graph_revision
+ remarks: 'Used to keep track of changes made to app permissions.'
+ columns:
+ - column:
+ name: id
+ type: int
+ autoIncrement: true
+ constraints:
+ primaryKey: true
+ nullable: false
+ - column:
+ name: before
+ type: ${text.type}
+ remarks: 'Serialized JSON of the apps graph before the changes.'
+ constraints:
+ nullable: false
+ - column:
+ name: after
+ type: ${text.type}
+ remarks: 'Serialized JSON of the apps graph after the changes.'
+ constraints:
+ nullable: false
+ - column:
+ name: user_id
+ type: int
+ remarks: 'The ID of the admin who made this set of changes.'
+ constraints:
+ nullable: false
+ referencedTableName: core_user
+ referencedColumnNames: id
+ foreignKeyName: fk_app_permission_graph_revision_user_id
+ - column:
+ name: created_at
+ type: ${timestamp_type}
+ remarks: 'The timestamp of when these changes were made.'
+ defaultValueComputed: current_timestamp
+ constraints:
+ nullable: false
+ - column:
+ name: remark
+ type: ${text.type}
+ remarks: 'Optional remarks explaining why these changes were made.'
+
# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
########################################################################################################################
diff --git a/src/metabase/api/app.clj b/src/metabase/api/app.clj
index a9fa964d32b..8cf52be81b3 100644
--- a/src/metabase/api/app.clj
+++ b/src/metabase/api/app.clj
@@ -8,7 +8,7 @@
[metabase.api.collection :as api.collection]
[metabase.api.common :as api]
[metabase.mbql.schema :as mbql.s]
- [metabase.models :refer [App Collection Dashboard ModelAction Table]]
+ [metabase.models :refer [App Dashboard ModelAction Table]]
[metabase.models.app.graph :as app.graph]
[metabase.models.collection :as collection]
[metabase.models.dashboard :as dashboard]
@@ -26,13 +26,13 @@
(defn- create-app! [{:keys [collection] :as app}]
(db/transaction
- (let [coll-params (select-keys collection [:name :color :description :namespace :authority_level])
- collection-instance (api.collection/create-collection! coll-params)
+ (let [coll-params (select-keys collection [:name :color :description :authority_level])
+ collection-instance (api.collection/create-collection!
+ (assoc coll-params :namespace :apps))
app-params (-> app
(select-keys [:dashboard_id :options :nav_items])
(assoc :collection_id (:id collection-instance)))
app (db/insert! App app-params)]
- (app.graph/set-default-permissions! app)
(hydrate-details app))))
(api/defendpoint POST "/"
@@ -57,7 +57,7 @@
dashboard_id (s/maybe su/IntGreaterThanOrEqualToZero)
options (s/maybe su/Map)
nav_items (s/maybe [(s/maybe su/Map)])}
- (api/write-check Collection (db/select-one-field :collection_id App :id app-id))
+ (api/write-check App app-id)
(db/update! App app-id (select-keys body [:dashboard_id :options :nav_items]))
(hydrate-details (db/select-one App :id app-id)))
@@ -287,6 +287,7 @@
(api/defendpoint POST "/:app-id/scaffold"
"Endpoint to scaffold a new table onto an existing data-app"
[app-id :as {{:keys [table-ids]} :body}]
+ (api/write-check App app-id)
(db/transaction
(let [{app-id :id app-name :name nav-items :nav_items {collection-id :id} :collection} (hydrate-details (db/select-one App :id app-id))
;; We can scaffold this as a new app, but use the existing collection-id and nav-items to merge into the existing app
@@ -328,12 +329,12 @@
(let [graph (resolve-advanced-app-permission-function 'graph)]
(graph)))
-(defn- ->int [id] (Integer/parseInt (name id)))
+(defn- ->int [id] (parse-long (name id)))
(defn- dejsonify-with [f m]
(into {}
(map (fn [[k v]]
- [(->int k) (f v)]))
+ [(or (->int k) k) (f v)]))
m))
(defn- dejsonify-id->permission-map [m]
@@ -342,12 +343,6 @@
(defn- dejsonify-groups-map [m]
(dejsonify-with dejsonify-id->permission-map m))
-(defn- dejsonify-global-graph
- "Fix the types in the graph when it comes in from the API, e.g. converting things like `\"none\"` to `:none` and
- parsing object keys as integers."
- [graph]
- (update graph :groups dejsonify-id->permission-map))
-
(defn- dejsonify-graph
"Fix the types in the graph when it comes in from the API, e.g. converting things like `\"none\"` to `:none` and
parsing object keys as integers."
@@ -360,7 +355,7 @@
{body su/Map}
(api/check-superuser)
(-> body
- dejsonify-global-graph
+ dejsonify-graph
app.graph/update-global-graph!))
(api/defendpoint PUT "/graph"
diff --git a/src/metabase/api/collection.clj b/src/metabase/api/collection.clj
index 40df45f6f9b..f5456e69e1f 100644
--- a/src/metabase/api/collection.clj
+++ b/src/metabase/api/collection.clj
@@ -760,16 +760,17 @@
(defn- write-check-collection-or-root-collection
"Check that you're allowed to write Collection with `collection-id`; if `collection-id` is `nil`, check that you have
Root Collection perms."
- [collection-id]
+ [collection-id collection-namespace]
(api/write-check (if collection-id
(db/select-one Collection :id collection-id)
- collection/root-collection)))
+ (cond-> collection/root-collection
+ collection-namespace (assoc :namespace collection-namespace)))))
(defn create-collection!
"Create a new collection."
[{:keys [name color description parent_id namespace authority_level]}]
;; To create a new collection, you need write perms for the location you are going to be putting it in...
- (write-check-collection-or-root-collection parent_id)
+ (write-check-collection-or-root-collection parent_id namespace)
;; Now create the new Collection :)
(api/check-403 (or (nil? authority_level)
(and api/*is-superuser?* authority_level)))
diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj
index 2b1fa4a9502..4f5e536d632 100644
--- a/src/metabase/api/search.clj
+++ b/src/metabase/api/search.clj
@@ -230,7 +230,9 @@
visible-collections)
honeysql-query (-> honeysql-query
(hh/merge-where collection-filter-clause)
- (hh/merge-where [:= :collection.namespace nil]))]
+ (hh/merge-where [:or
+ [:= :collection.namespace nil]
+ [:= :collection.namespace "apps"]]))]
;; add a JOIN against Collection *unless* the source table is already Collection
(cond-> honeysql-query
(not= collection-id-column :collection.id)
diff --git a/src/metabase/cmd/copy.clj b/src/metabase/cmd/copy.clj
index 2e06bc50a2f..d92e523152b 100644
--- a/src/metabase/cmd/copy.clj
+++ b/src/metabase/cmd/copy.clj
@@ -11,6 +11,7 @@
[metabase.db.setup :as mdb.setup]
[metabase.models :refer [Activity
App
+ AppPermissionGraphRevision
ApplicationPermissionsRevision
BookmarkOrdering
Card
@@ -107,6 +108,7 @@
DashboardCardSeries
Activity
App
+ AppPermissionGraphRevision
Pulse
PulseCard
PulseChannel
diff --git a/src/metabase/models.clj b/src/metabase/models.clj
index 12e0ad9dd55..bc8857cfcba 100644
--- a/src/metabase/models.clj
+++ b/src/metabase/models.clj
@@ -2,6 +2,7 @@
(:require [metabase.models.action :as action]
[metabase.models.activity :as activity]
[metabase.models.app :as app]
+ [metabase.models.app-permission-graph-revision :as app-perm-revision]
[metabase.models.application-permissions-revision :as a-perm-revision]
[metabase.models.bookmark :as bookmark]
[metabase.models.card :as card]
@@ -48,6 +49,7 @@
(comment action/keep-me
activity/keep-me
app/keep-me
+ app-perm-revision/keep-me
card/keep-me
bookmark/keep-me
collection/keep-me
@@ -93,6 +95,7 @@
[action Action HTTPAction ModelAction QueryAction]
[activity Activity]
[app App]
+ [app-perm-revision AppPermissionGraphRevision]
[bookmark CardBookmark]
[bookmark DashboardBookmark]
[bookmark CollectionBookmark]
diff --git a/src/metabase/models/app.clj b/src/metabase/models/app.clj
index 52bb6fa2408..e49f55c78ba 100644
--- a/src/metabase/models/app.clj
+++ b/src/metabase/models/app.clj
@@ -1,5 +1,6 @@
(ns metabase.models.app
- (:require [metabase.models.permissions :as perms]
+ (:require [metabase.models.collection :as collection :refer [Collection]]
+ [metabase.models.interface :as mi]
[metabase.models.query :as query]
[metabase.models.serialization.hash :as serdes.hash]
[metabase.util :as u]
@@ -9,7 +10,16 @@
(models/defmodel App :app)
;;; You can read/write an App if you can read/write its Collection
-(derive App ::perms/use-parent-collection-perms)
+(doto App
+ (derive ::mi/read-policy.full-perms-for-perms-set)
+ (derive ::mi/write-policy.full-perms-for-perms-set))
+
+(defmethod mi/perms-objects-set App
+ [instance read-or-write]
+ (let [collection (if-let [collection-id (:collection_id instance)]
+ (mi/instance Collection {:id collection-id, :namespace :apps})
+ (assoc collection/root-collection :namespace :apps))]
+ (mi/perms-objects-set collection read-or-write)))
(u/strict-extend #_{:clj-kondo/ignore [:metabase/disallow-class-or-type-on-model]} (class App)
models/IModel
diff --git a/src/metabase/models/app/graph.clj b/src/metabase/models/app/graph.clj
index 0cabeeccc96..c4d2cb9d666 100644
--- a/src/metabase/models/app/graph.clj
+++ b/src/metabase/models/app/graph.clj
@@ -2,54 +2,38 @@
"Code for generating and updating the App permissions graph. App permissions
are based on the permissions of the app's collection."
(:require [clojure.data :as data]
- [metabase.models.collection-permission-graph-revision :as c-perm-revision
- :refer [CollectionPermissionGraphRevision]]
+ [metabase.models.app-permission-graph-revision :as app-perm-revision
+ :refer [AppPermissionGraphRevision]]
[metabase.models.collection.graph :as graph]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
- [metabase.models.setting :as setting :refer [defsetting]]
- [metabase.public-settings.premium-features :as premium-features]
[metabase.util.i18n :as i18n :refer [tru]]
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db]))
-(def AppPermissions
+(def RootPermissions
"The valid app permissions. Currently corresponds 1:1 to `CollectionPermissions`
since app permissions are implemented in terms of collection permissions."
- graph/CollectionPermissions)
+ {:root graph/CollectionPermissions})
(def ^:private GlobalPermissionsGraph
{:revision s/Int
- :groups {su/IntGreaterThanZero AppPermissions}})
-
-(s/defn ^:private set-all-users-app-permission!
- [permission :- AppPermissions]
- (setting/set-value-of-type! :keyword :all-users-app-permission permission))
-
-(defsetting all-users-app-permission
- "App permission of the All Users group"
- :type :keyword
- :visibility :internal
- :default :none
- :setter (fn [v]
- (set-all-users-app-permission! (cond-> v (string? v) keyword))))
-
-(defn set-default-permissions!
- "Sets the default permissions for the ''All Users'' group on`app` as specified
- by `all-users-app-permission` if advanced permissions are not available."
- [app]
- (when-not (premium-features/has-feature? :advanced-permissions)
- (graph/update-collection-permissions! (:id (perms-group/all-users))
- (:collection_id app)
- (all-users-app-permission))))
+ :groups {su/IntGreaterThanZero RootPermissions}})
(s/defn global-graph :- GlobalPermissionsGraph
- "Fetch the global app permission graph."
+ "Fetch the global app permission graph.
+
+ This works by reading the permissions for app collections, restricting
+ the groups to admin and ''All Users'' and the collections to the root."
[]
- {:revision (c-perm-revision/latest-id)
- :groups {(:id (perms-group/admin)) :write
- (:id (perms-group/all-users)) (all-users-app-permission)}})
+ (-> (graph/graph :apps)
+ (assoc :revision (app-perm-revision/latest-id))
+ (update :groups (fn [group-perms]
+ (-> group-perms
+ (select-keys [(:id (perms-group/admin))
+ (:id (perms-group/all-users))])
+ (update-vals #(select-keys % [:root])))))))
(s/defn update-global-graph!
"Update the global app permission graph to the state specified by
@@ -59,17 +43,16 @@
[diff-old changes] (data/diff (:groups old-graph) (:groups new-graph))]
(perms/log-permissions-changes diff-old changes)
(perms/check-revision-numbers old-graph new-graph)
- (when-let [[[group-id permission] & other-groups] (seq changes)]
- (when (or (not= group-id (:id (perms-group/all-users)))
+ (when-let [[[all-users-group-id [[root permission] & other-colls]] & other-groups] (seq changes)]
+ (when (or (not= all-users-group-id (:id (perms-group/all-users)))
+ (not= root :root)
+ (seq other-colls)
(seq other-groups))
- (throw (ex-info (tru "You can configure for the ''All Users'' group only")
+ (throw (ex-info (tru "You can configure permissions only on the root and only for the ''All Users'' group")
{:group-ids (keys changes)
:status-code 400})))
(db/transaction
- (when (not= permission (all-users-app-permission))
- (all-users-app-permission! permission)
- (doseq [collection-id (db/select-field :collection_id 'App)]
- (graph/update-collection-permissions! group-id collection-id permission))
- (perms/save-perms-revision!
- CollectionPermissionGraphRevision (:revision old-graph) old-graph changes))
+ (graph/update-collection-permissions! :apps all-users-group-id root permission)
+ (perms/save-perms-revision! AppPermissionGraphRevision (:revision old-graph)
+ old-graph changes)
(global-graph)))))
diff --git a/src/metabase/models/app_permission_graph_revision.clj b/src/metabase/models/app_permission_graph_revision.clj
new file mode 100644
index 00000000000..8ec3b81de07
--- /dev/null
+++ b/src/metabase/models/app_permission_graph_revision.clj
@@ -0,0 +1,22 @@
+(ns metabase.models.app-permission-graph-revision
+ (:require [metabase.util :as u]
+ [metabase.util.i18n :refer [tru]]
+ [toucan.db :as db]
+ [toucan.models :as models]))
+
+(models/defmodel AppPermissionGraphRevision :app_permission_graph_revision)
+
+(u/strict-extend #_{:clj-kondo/ignore [:metabase/disallow-class-or-type-on-model]} (class AppPermissionGraphRevision)
+ models/IModel
+ (merge models/IModelDefaults
+ {:types (constantly {:before :json
+ :after :json})
+ :properties (constantly {:created-at-timestamped? true})
+ :pre-update (fn [& _] (throw (Exception. (tru "You cannot update an AppPermissionGraphRevision!"))))}))
+
+(defn latest-id
+ "Return the ID of the newest `AppPermissionGraphRevision`, or zero if none have been made yet.
+ (This is used by the app graph update logic that checks for changes since the original graph was fetched)."
+ []
+ (or (:id (db/select-one [AppPermissionGraphRevision [:%max.id :id]]))
+ 0))
diff --git a/src/metabase/models/collection.clj b/src/metabase/models/collection.clj
index 304d14fb422..01849434b52 100644
--- a/src/metabase/models/collection.clj
+++ b/src/metabase/models/collection.clj
@@ -192,6 +192,7 @@
(m/assoc-some root-collection
:name (case (keyword collection-namespace)
:snippets (tru "Top folder")
+ :apps (tru "All apps")
(tru "Our analytics"))
:namespace collection-namespace
:id "root"))
@@ -871,14 +872,25 @@
(let [collection (if (integer? collection-or-id)
(db/select-one [Collection :id :namespace] :id (collection-or-id))
collection-or-id)]
- ;; HACK Collections in the "snippets" namespace have no-op permissions unless EE enhancements are enabled
- ;;
- ;; TODO -- Pretty sure snippet perms should be feature flagged by `advanced-permissions` instead
- (if (and (= (u/qualified-name (:namespace collection)) "snippets")
- (not (premium-features/enable-enhancements?)))
+ (cond
+ ;; HACK Collections in the "snippets" namespace have no-op permissions unless EE enhancements are enabled
+ ;;
+ ;; TODO -- Pretty sure snippet perms should be feature flagged by `advanced-permissions` instead
+ (and (= (u/qualified-name (:namespace collection)) "snippets")
+ (not (premium-features/enable-enhancements?)))
#{}
+
+ ;; Collections in the "apps" namespace use the permissions of the root unless advanced permissions are enabled
+ (and (= (u/qualified-name (:namespace collection)) "apps")
+ (not (premium-features/has-feature? :advanced-permissions)))
+ #{(let [root (assoc root-collection :namespace :apps)]
+ (case read-or-write
+ :read (perms/collection-read-path root)
+ :write (perms/collection-readwrite-path root)))}
+
;; This is not entirely accurate as you need to be a superuser to modifiy a collection itself (e.g., changing its
;; name) but if you have write perms you can add/remove cards
+ :else
#{(case read-or-write
:read (perms/collection-read-path collection-or-id)
:write (perms/collection-readwrite-path collection-or-id))})))
@@ -1131,7 +1143,7 @@
(defmethod allowed-namespaces :default
[_]
- #{nil})
+ #{nil :apps})
(defn check-collection-namespace
"Check that object's `:collection_id` refers to a Collection in an allowed namespace (see
diff --git a/src/metabase/models/collection/graph.clj b/src/metabase/models/collection/graph.clj
index 777fa69e590..6e9c6891b65 100644
--- a/src/metabase/models/collection/graph.clj
+++ b/src/metabase/models/collection/graph.clj
@@ -29,7 +29,8 @@
{(s/optional-key :root) CollectionPermissions ; when doing a delta between old graph and new graph root won't always
su/IntGreaterThanZero CollectionPermissions}) ; be present, which is why it's *optional*
-(def ^:private PermissionsGraph
+(def PermissionsGraph
+ "A versioned map of group permissions. See [[GroupPermissionsGraph]]."
{:revision s/Int
:groups {su/IntGreaterThanZero GroupPermissionsGraph}})
@@ -69,7 +70,7 @@
[:not [:like :location (hx/literal (format "/%d/%%" collection-id))]]))}]
(set (map :id (db/query honeysql-form)))))
-(defn collection-permission-graph
+(defn- collection-permission-graph
"Return the permission graph for the collections with id in `collection-ids` and the root collection."
([collection-ids] (collection-permission-graph collection-ids nil))
([collection-ids collection-namespace]
@@ -120,21 +121,19 @@
(s/defn update-collection-permissions!
"Update the permissions for group ID with `group-id` on collection with ID
`collection-id` in the optional `collection-namespace` to `new-collection-perms`."
- ([group-id collection-id new-collection-perms]
- (update-collection-permissions! nil group-id collection-id new-collection-perms))
- ([collection-namespace :- (s/maybe su/KeywordOrString)
- group-id :- su/IntGreaterThanZero
- collection-id :- (s/cond-pre (s/eq :root) su/IntGreaterThanZero)
- new-collection-perms :- CollectionPermissions]
- (let [collection-id (if (= collection-id :root)
- (assoc collection/root-collection :namespace collection-namespace)
- collection-id)]
- ;; remove whatever entry is already there (if any) and add a new entry if applicable
- (perms/revoke-collection-permissions! group-id collection-id)
- (case new-collection-perms
- :write (perms/grant-collection-readwrite-permissions! group-id collection-id)
- :read (perms/grant-collection-read-permissions! group-id collection-id)
- :none nil))))
+ [collection-namespace :- (s/maybe su/KeywordOrString)
+ group-id :- su/IntGreaterThanZero
+ collection-id :- (s/cond-pre (s/eq :root) su/IntGreaterThanZero)
+ new-collection-perms :- CollectionPermissions]
+ (let [collection-id (if (= collection-id :root)
+ (assoc collection/root-collection :namespace collection-namespace)
+ collection-id)]
+ ;; remove whatever entry is already there (if any) and add a new entry if applicable
+ (perms/revoke-collection-permissions! group-id collection-id)
+ (case new-collection-perms
+ :write (perms/grant-collection-readwrite-permissions! group-id collection-id)
+ :read (perms/grant-collection-read-permissions! group-id collection-id)
+ :none nil)))
(s/defn ^:private update-group-permissions!
[collection-namespace :- (s/maybe su/KeywordOrString)
diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index 066b6331628..aa36ca3511c 100644
--- a/src/metabase/models/permissions.clj
+++ b/src/metabase/models/permissions.clj
@@ -408,6 +408,14 @@
[collection-or-id :- MapOrID]
(str (collection-readwrite-path collection-or-id) "read/"))
+(s/defn app-root-collection-permission :- Path
+ "Return path for the app root collection permission `read-or-write`."
+ [read-or-write :- (s/enum :read :write)]
+ (let [app-root-collection {:metabase.models.collection.root/is-root? true, :namespace :apps}]
+ (case read-or-write
+ :write (collection-readwrite-path app-root-collection)
+ :read (collection-read-path app-root-collection))))
+
(s/defn table-read-path :- Path
"Return the permissions path required to fetch the Metadata for a Table."
([table-or-id]
@@ -1227,7 +1235,8 @@
"Save changes made to permission graph for logging/auditing purposes.
This doesn't do anything if `*current-user-id*` is unset (e.g. for testing or REPL usage).
* `model` -- revision model, should be one of
- [PermissionsRevision, CollectionPermissionGraphRevision, ApplicationPermissionsRevision]
+ [PermissionsRevision, CollectionPermissionGraphRevision, ApplicationPermissionsRevision
+ AppPermissionGraphRevision]
* `before` -- the graph before the changes
* `changes` -- set of changes applied in this revision."
[model current-revision before changes]
diff --git a/test/metabase/api/activity_test.clj b/test/metabase/api/activity_test.clj
index 471809076fd..65468400a3a 100644
--- a/test/metabase/api/activity_test.clj
+++ b/test/metabase/api/activity_test.clj
@@ -135,7 +135,7 @@
:display "table"
:archived true
:visualization_settings {}}]
- Collection [{coll-id :id} {}]
+ Collection [{coll-id :id} {:namespace :apps}]
App [{app-id :id} {:collection_id coll-id}]
Dashboard [page {:name "rand-name"
:description "rand-name"
@@ -186,7 +186,7 @@
Dashboard [dash1 {:name "rand-name"
:description "rand-name"
:creator_id (mt/user->id :crowberto)}]
- Collection [{coll-id :id} {}]
+ Collection [{coll-id :id} {:namespace :apps}]
App [{app-id :id} {:collection_id coll-id}]
Dashboard [dash2 {:name "other-dashboard"
:description "just another dashboard"
diff --git a/test/metabase/api/app_test.clj b/test/metabase/api/app_test.clj
index 6b212814399..19740aff0d3 100644
--- a/test/metabase/api/app_test.clj
+++ b/test/metabase/api/app_test.clj
@@ -25,42 +25,42 @@
:color "#123456"}]
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(testing "parent_id is ignored when creating apps"
- (mt/with-temporary-setting-values [all-users-app-permission :none]
- (mt/with-temp* [Collection [{collection-id :id}]]
- (let [coll-params (assoc base-params :parent_id collection-id)
- response (mt/user-http-request :crowberto :post 200 "app" {:collection coll-params})]
- (is (pos-int? (:id response)))
- (is (pos-int? (:collection_id response)))
- (is (partial= (assoc base-params :location "/")
- (:collection response)))
- (is (partial= {:groups {(:id (perms-group/all-users)) {(:collection_id response) :none}}}
- (graph/graph))
- "''All Users'' should have the default permission on the app collection")))))
+ (mt/with-temp* [Collection [{collection-id :id}]]
+ (let [coll-params (assoc base-params :parent_id collection-id)
+ response (mt/user-http-request :crowberto :post 200 "app" {:collection coll-params})]
+ (is (pos-int? (:id response)))
+ (is (pos-int? (:collection_id response)))
+ (is (partial= (assoc base-params :location "/")
+ (:collection response)))
+ (is (partial= {:groups {(:id (perms-group/all-users)) {(:collection_id response) :none}}}
+ (graph/graph :apps))
+ "''All Users'' should have the default permission on the app collection"))))
(testing "Create app in the root"
- (mt/with-temporary-setting-values [all-users-app-permission :read]
+ (mt/with-all-users-permission (perms/app-root-collection-permission :read)
(let [response (mt/user-http-request :crowberto :post 200 "app" {:collection base-params})]
- (is (pos-int? (:id response)))
- (is (pos-int? (:collection_id response)))
- (is (partial= (assoc base-params :location "/")
- (:collection response)))
- (is (partial= {:groups {(:id (perms-group/all-users)) {(:collection_id response) :read}}}
- (graph/graph))
- "''All Users'' should have the default permission on the app collection"))))
+ (is (pos-int? (:id response)))
+ (is (pos-int? (:collection_id response)))
+ (is (partial= (assoc base-params :location "/")
+ (:collection response)))
+ (is (partial= {:groups {(:id (perms-group/all-users)) {(:collection_id response) :read}}}
+ (graph/graph :apps))
+ "''All Users'' should have the default permission on the app collection"))))
(testing "With initial dashboard and nav_items"
- (mt/with-temp Dashboard [{dashboard-id :id}]
- (let [nav_items [{:options {:click_behavior {}}}]]
- (is (partial= {:collection (assoc base-params :location "/")
- :dashboard_id dashboard-id
- :nav_items nav_items}
- (mt/user-http-request :rasta :post 200 "app" {:collection base-params
- :dashboard_id dashboard-id
- :nav_items nav_items}))))))))))
+ (mt/with-all-users-permission (perms/app-root-collection-permission :write)
+ (mt/with-temp* [Dashboard [{dashboard-id :id}]]
+ (let [nav_items [{:options {:click_behavior {}}}]]
+ (is (partial= {:collection (assoc base-params :location "/")
+ :dashboard_id dashboard-id
+ :nav_items nav_items}
+ (mt/user-http-request :rasta :post 200 "app" {:collection base-params
+ :dashboard_id dashboard-id
+ :nav_items nav_items})))))))))))
(deftest update-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(let [app-data {:nav_items [{:options {:item "stuff"}}]
:options {:frontend "stuff"}}]
- (mt/with-temp* [Collection [{collection_id :id}]
+ (mt/with-temp* [Collection [{collection_id :id} {:namespace :apps}]
App [{app_id :id} (assoc app-data :collection_id collection_id)]
Dashboard [{dashboard_id :id}]]
(let [expected (assoc app-data :collection_id collection_id :dashboard_id dashboard_id)]
@@ -76,34 +76,36 @@
(mt/user-http-request :crowberto :put 200 (str "app/" app_id) {:dashboard_id dashboard_id
:options nil})))))))
(testing "Collection permissions"
- (mt/with-non-admin-groups-no-root-collection-perms
- (mt/with-temp* [Collection [{collection_id :id}]
- App [{app_id :id} {:collection_id collection_id}]]
- (is (= "You don't have permissions to do that."
- (mt/user-http-request :rasta :put 403 (str "app/" app_id) {})))))
- (mt/with-temp* [Collection [{collection_id :id}]
+ (mt/with-temp* [Collection [{collection_id :id} {:namespace :apps}]
App [{app_id :id} {:collection_id collection_id}]]
- (is (partial= {:collection_id collection_id}
- (mt/user-http-request :rasta :put 200 (str "app/" app_id) {})))))))
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :put 403 (str "app/" app_id) {}))))
+ (mt/with-all-users-permission (perms/app-root-collection-permission :write)
+ (mt/with-temp* [Collection [{collection_id :id} {:namespace :apps}]
+ App [{app_id :id} {:collection_id collection_id}]]
+ (is (partial= {:collection_id collection_id}
+ (mt/user-http-request :rasta :put 200 (str "app/" app_id) {}))))))))
(deftest list-apps-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(let [app-data {:nav_items [{:options {:item "stuff"}}]
:options {:frontend "stuff"}}]
- (mt/with-temp* [Collection [{collection_id :id :as collection}]
+ (mt/with-temp* [Collection [{collection_id :id :as collection} {:namespace :apps}]
Dashboard [{dashboard_id :id}]
App [{app-id :id} (assoc app-data :collection_id collection_id :dashboard_id dashboard_id)]]
(let [expected (merge app-data {:id app-id
:collection_id collection_id
:dashboard_id dashboard_id
- :collection (assoc collection :can_write true)})]
+ :collection (-> collection
+ (assoc :can_write true)
+ (update :namespace name))})]
(testing "can query non-archived apps"
(is (partial= [expected]
(mt/user-http-request :crowberto :get 200 "app"))))))
(testing "can only see apps with permission for"
- (mt/with-non-admin-groups-no-root-collection-perms
- (mt/with-temp* [Collection [collection-1 {:name "Collection 1"}]
- Collection [collection-2 {:name "Collection 2"}]
+ (mt/with-model-cleanup [Permissions]
+ (mt/with-temp* [Collection [collection-1 {:name "Collection 1", :namespace :apps}]
+ Collection [collection-2 {:name "Collection 2", :namespace :apps}]
Dashboard [{dashboard_id :id}]
App [{app-id :id} (assoc app-data :collection_id (:id collection-1) :dashboard_id dashboard_id)]
App [_ (assoc app-data :collection_id (:id collection-2) :dashboard_id dashboard_id)]]
@@ -111,47 +113,56 @@
(let [expected (merge app-data {:id app-id
:collection_id (:id collection-1)
:dashboard_id dashboard_id
- :collection (assoc collection-1 :can_write false)})]
+ :collection (-> collection-1
+ (assoc :can_write false)
+ (update :namespace name))})]
(is (partial= [expected]
(mt/user-http-request :rasta :get 200 "app")))))))
(testing "archives"
- (mt/with-temp* [Collection [collection-1 {:name "Collection 1"}]
- Collection [collection-2 {:name "Collection 2" :archived true}]
- Dashboard [{dashboard_id :id}]
- App [{app-1-id :id} (assoc app-data :collection_id (:id collection-1) :dashboard_id dashboard_id)]
- App [{app-2-id :id} (assoc app-data :collection_id (:id collection-2) :dashboard_id dashboard_id)]]
- (testing "listing normal apps"
- (let [expected (merge app-data {:id app-1-id
- :collection_id (:id collection-1)
- :dashboard_id dashboard_id
- :collection (assoc collection-1 :can_write true)})]
- (is (partial= [expected]
- (mt/user-http-request :rasta :get 200 "app")))))
- (testing "listing archived"
- (let [expected (merge app-data {:id app-2-id
- :collection_id (:id collection-2)
- :dashboard_id dashboard_id
- :collection (assoc collection-2 :can_write true)})]
- (is (partial= [expected]
- (mt/user-http-request :rasta :get 200 "app/?archived=true"))))))))))
+ (mt/with-model-cleanup [Permissions]
+ (mt/with-all-users-permission (perms/app-root-collection-permission :write)
+ (mt/with-temp* [Collection [collection-1 {:name "Collection 1", :namespace :apps}]
+ Collection [collection-2 {:name "Collection 2", :namespace :apps, :archived true}]
+ Dashboard [{dashboard_id :id}]
+ App [{app-1-id :id} (assoc app-data :collection_id (:id collection-1) :dashboard_id dashboard_id)]
+ App [{app-2-id :id} (assoc app-data :collection_id (:id collection-2) :dashboard_id dashboard_id)]]
+ (testing "listing normal apps"
+ (let [expected (merge app-data {:id app-1-id
+ :collection_id (:id collection-1)
+ :dashboard_id dashboard_id
+ :collection (-> collection-1
+ (assoc :can_write true)
+ (update :namespace name))})]
+ (is (partial= [expected]
+ (mt/user-http-request :rasta :get 200 "app")))))
+ (testing "listing archived"
+ (let [expected (merge app-data {:id app-2-id
+ :collection_id (:id collection-2)
+ :dashboard_id dashboard_id
+ :collection (-> collection-2
+ (assoc :can_write true)
+ (update :namespace name))})]
+ (is (partial= [expected]
+ (mt/user-http-request :rasta :get 200 "app/?archived=true"))))))))))))
(deftest fetch-app-test
(let [app-data {:nav_items [{:options {:item "stuff"}}]
:options {:frontend "stuff"}}]
- (mt/with-non-admin-groups-no-root-collection-perms
- (mt/with-temp* [Collection [{collection_id :id :as collection}]
- Dashboard [{dashboard_id :id}]
- App [{app-id :id} (assoc app-data :collection_id collection_id :dashboard_id dashboard_id)]]
- (testing "that we can see app details"
- (let [expected (merge app-data {:id app-id
- :collection_id collection_id
- :dashboard_id dashboard_id
- :collection (assoc collection :can_write true)})]
- (is (partial= expected
- (mt/user-http-request :crowberto :get 200 (str "app/" app-id))))))
- (testing "that app detail properly checks permissions"
- (is (= "You don't have permissions to do that."
- (mt/user-http-request :rasta :get 403 (str "app/" app-id)))))))))
+ (mt/with-temp* [Collection [{collection_id :id :as collection} {:namespace :apps}]
+ Dashboard [{dashboard_id :id}]
+ App [{app-id :id} (assoc app-data :collection_id collection_id :dashboard_id dashboard_id)]]
+ (testing "that we can see app details"
+ (let [expected (merge app-data {:id app-id
+ :collection_id collection_id
+ :dashboard_id dashboard_id
+ :collection (-> collection
+ (assoc :can_write true)
+ (update :namespace name))})]
+ (is (partial= expected
+ (mt/user-http-request :crowberto :get 200 (str "app/" app-id))))))
+ (testing "that app detail properly checks permissions"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 (str "app/" app-id))))))))
(defn- normalized-models [models]
(->> models (sort-by :id) json/generate-string))
@@ -170,8 +181,8 @@
(deftest scaffold-test
(mt/with-model-cleanup [Card Dashboard Collection Permissions]
- (testing "Golden path"
- (mt/with-temporary-setting-values [all-users-app-permission :read]
+ (mt/with-all-users-permission (perms/app-root-collection-permission :read)
+ (testing "Golden path"
(let [app (mt/user-http-request
:crowberto :post 200 "app/scaffold"
{:table-ids [(data/id :venues)]
@@ -202,23 +213,24 @@
[:= :dataset true]]}]
:order-by [:id]}))))
(is (partial= {:groups {(:id (perms-group/all-users)) {(:collection_id app) :read}}}
- (graph/graph))
+ (graph/graph :apps))
"''All Users'' should have the default permission on the app collection")
(is (= (scaffolded-models app)
- (api-models app))))))
- (testing "Bad or duplicate tables"
- (is (= (format "Some tables could not be found. Given: (%s %s) Found: (%s)"
- (data/id :venues)
- Integer/MAX_VALUE
- (data/id :venues))
- (mt/user-http-request
- :crowberto :post 400 "app/scaffold"
- {:table-ids [(data/id :venues) (data/id :venues) Integer/MAX_VALUE]
- :app-name (str "My test app " (gensym))}))))))
+ (api-models app)))))
+
+ (testing "Bad or duplicate tables"
+ (is (= (format "Some tables could not be found. Given: (%s %s) Found: (%s)"
+ (data/id :venues)
+ Integer/MAX_VALUE
+ (data/id :venues))
+ (mt/user-http-request
+ :crowberto :post 400 "app/scaffold"
+ {:table-ids [(data/id :venues) (data/id :venues) Integer/MAX_VALUE]
+ :app-name (str "My test app " (gensym))})))))))
(deftest scaffold-app-test
(mt/with-model-cleanup [Card Dashboard]
- (mt/with-temp* [Collection [{collection-id :id}]
+ (mt/with-temp* [Collection [{collection-id :id} {:namespace :apps}]
App [{app-id :id} {:collection_id collection-id}]]
(testing "Without existing pages"
(let [app (mt/user-http-request
@@ -313,11 +325,23 @@
response2 (mt/user-http-request :crowberto :post 200 "app" {:collection base-params})]
(is (partial= {:groups {(:id (perms-group/all-users)) {(:collection_id response1) :none
(:collection_id response2) :none}}}
- (graph/graph)))
- (mt/user-http-request :crowberto :put 200 "app/global-graph"
- (assoc-in (mt/user-http-request :crowberto :get 200 "app/global-graph")
- [:groups (:id (perms-group/all-users))]
- :write))
- (is (partial= {:groups {(:id (perms-group/all-users)) {(:collection_id response1) :write
- (:collection_id response2) :write}}}
- (graph/graph))))))))))
+ (graph/graph :apps)))
+ (testing "''All Users'' can't see these apps"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 (str "app/" (:id response1)))))
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 (str "app/" (:id response2))))))
+ (is (partial= {:groups {(:id (perms-group/all-users)) {:root "write"}}}
+ (mt/user-http-request :crowberto :put 200 "app/global-graph"
+ (assoc-in (mt/user-http-request :crowberto :get 200 "app/global-graph")
+ [:groups (:id (perms-group/all-users))]
+ {:root :write}))))
+ (is (partial= {:groups {(:id (perms-group/all-users)) {(:collection_id response1) :none
+ (:collection_id response2) :none}}}
+ (graph/graph :apps))
+ "collection permissions shouldn't change")
+ (testing "Now ''All Users'' can see these apps"
+ (is (partial= response1
+ (mt/user-http-request :rasta :get 200 (str "app/" (:id response1)))))
+ (is (partial= response2
+ (mt/user-http-request :rasta :get 200 (str "app/" (:id response2)))))))))))))
diff --git a/test/metabase/api/bookmark_test.clj b/test/metabase/api/bookmark_test.clj
index 5c75ffee198..f363f9fe51e 100644
--- a/test/metabase/api/bookmark_test.clj
+++ b/test/metabase/api/bookmark_test.clj
@@ -9,57 +9,60 @@
[metabase.models.collection :refer [Collection]]
[metabase.models.dashboard :refer [Dashboard]]
[metabase.models.interface :as mi]
+ [metabase.models.permissions :as perms]
[metabase.test :as mt]
[metabase.util :as u]
[toucan.db :as db]))
(deftest bookmarks-test
+ (mt/initialize-if-needed! :db)
(testing "POST /api/bookmark/:model/:model-id"
- (mt/with-temp* [Collection [{coll-id :id :as collection} {:name "Test Collection"}]
- Card [card {:name "Test Card", :display "area", :collection_id coll-id}]
- Dashboard [dashboard {:name "Test Dashboard", :is_app_page true, :collection_id coll-id}]
- App [app {:collection_id coll-id, :dashboard_id (:id dashboard)}]]
- (testing "check that we can bookmark a Collection"
- (is (= (u/the-id collection)
- (->> (mt/user-http-request :rasta :post 200 (str "bookmark/collection/" (u/the-id collection)))
- :collection_id))))
- (testing "check that we can bookmark a Card"
- (is (= (u/the-id card)
- (->> (mt/user-http-request :rasta :post 200 (str "bookmark/card/" (u/the-id card)))
- :card_id))))
- (let [card-result (->> (mt/user-http-request :rasta :get 200 "bookmark")
- (filter #(= (:type %) "card"))
- first)]
- (testing "check a card bookmark has `:display` key"
- (is (contains? card-result :display)))
- (testing "check a card bookmark has `:dataset` key"
- (is (contains? card-result :dataset))))
- (testing "check that we can bookmark a Dashboard"
- (is (= (u/the-id dashboard)
- (->> (mt/user-http-request :rasta :post 200 (str "bookmark/dashboard/" (u/the-id dashboard)))
- :dashboard_id))))
- (testing "check that we can retreive the user's bookmarks"
- (let [result (mt/user-http-request :rasta :get 200 "bookmark")]
- (is (= #{"card" "collection" "dashboard"}
- (into #{} (map :type) result)))
- (testing "that app_id is hydrated on app collections"
- (is (partial= [{:item_id (:id collection), :name "Test Collection", :app_id (:id app)}]
- (filter #(= (:type %) "collection") result))))
- (testing "that is_app_page is returned for dashboards"
- (is (partial= [{:item_id (:id dashboard), :name "Test Dashboard", :is_app_page true, :app_id (:id app)}]
- (filter #(= (:type %) "dashboard") result))))))
- (testing "check that we can delete bookmarks"
- (mt/user-http-request :rasta :delete 204 (str "bookmark/card/" (u/the-id card)))
- (is (= #{"collection" "dashboard"}
- (->> (mt/user-http-request :rasta :get 200 "bookmark")
- (map :type)
- set)))
- (mt/user-http-request :rasta :delete 204 (str "bookmark/collection/" (u/the-id collection)))
- (mt/user-http-request :rasta :delete 204 (str "bookmark/dashboard/" (u/the-id dashboard)))
- (is (= #{}
- (->> (mt/user-http-request :rasta :get 200 "bookmark")
- (map :type)
- set)))))))
+ (mt/with-all-users-permission (perms/app-root-collection-permission :write)
+ (mt/with-temp* [Collection [{coll-id :id :as collection} {:name "Test Collection", :namespace :apps}]
+ Card [card {:name "Test Card", :display "area", :collection_id coll-id}]
+ Dashboard [dashboard {:name "Test Dashboard", :is_app_page true, :collection_id coll-id}]
+ App [app {:collection_id coll-id, :dashboard_id (:id dashboard)}]]
+ (testing "check that we can bookmark a Collection"
+ (is (= (u/the-id collection)
+ (->> (mt/user-http-request :rasta :post 200 (str "bookmark/collection/" (u/the-id collection)))
+ :collection_id))))
+ (testing "check that we can bookmark a Card"
+ (is (= (u/the-id card)
+ (->> (mt/user-http-request :rasta :post 200 (str "bookmark/card/" (u/the-id card)))
+ :card_id))))
+ (let [card-result (->> (mt/user-http-request :rasta :get 200 "bookmark")
+ (filter #(= (:type %) "card"))
+ first)]
+ (testing "check a card bookmark has `:display` key"
+ (is (contains? card-result :display)))
+ (testing "check a card bookmark has `:dataset` key"
+ (is (contains? card-result :dataset))))
+ (testing "check that we can bookmark a Dashboard"
+ (is (= (u/the-id dashboard)
+ (->> (mt/user-http-request :rasta :post 200 (str "bookmark/dashboard/" (u/the-id dashboard)))
+ :dashboard_id))))
+ (testing "check that we can retreive the user's bookmarks"
+ (let [result (mt/user-http-request :rasta :get 200 "bookmark")]
+ (is (= #{"card" "collection" "dashboard"}
+ (into #{} (map :type) result)))
+ (testing "that app_id is hydrated on app collections"
+ (is (partial= [{:item_id (:id collection), :name "Test Collection", :app_id (:id app)}]
+ (filter #(= (:type %) "collection") result))))
+ (testing "that is_app_page is returned for dashboards"
+ (is (partial= [{:item_id (:id dashboard), :name "Test Dashboard", :is_app_page true, :app_id (:id app)}]
+ (filter #(= (:type %) "dashboard") result))))))
+ (testing "check that we can delete bookmarks"
+ (mt/user-http-request :rasta :delete 204 (str "bookmark/card/" (u/the-id card)))
+ (is (= #{"collection" "dashboard"}
+ (->> (mt/user-http-request :rasta :get 200 "bookmark")
+ (map :type)
+ set)))
+ (mt/user-http-request :rasta :delete 204 (str "bookmark/collection/" (u/the-id collection)))
+ (mt/user-http-request :rasta :delete 204 (str "bookmark/dashboard/" (u/the-id dashboard)))
+ (is (= #{}
+ (->> (mt/user-http-request :rasta :get 200 "bookmark")
+ (map :type)
+ set))))))))
(defn bookmark-models [user-id & models]
(doseq [model models]
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index 1a7fbbefe7e..a59eabbfc05 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -5,7 +5,8 @@
[honeysql.core :as hsql]
[metabase.api.collection :as api.collection]
[metabase.models :refer [App Card Collection Dashboard DashboardCard ModerationReview NativeQuerySnippet
- PermissionsGroup PermissionsGroupMembership Pulse PulseCard PulseChannel
+ PermissionsGroup PermissionsGroupMembership
+ Pulse PulseCard PulseChannel
PulseChannelRecipient Revision Timeline TimelineEvent User]]
[metabase.models.collection :as collection]
[metabase.models.collection-test :as collection-test]
@@ -110,28 +111,49 @@
(str/includes? collection-name "Personal Collection"))))
(map :name)))))))
- (mt/with-temp* [Collection [{coll1-id :id} {:name "Archived Collection", :archived true}]
- Collection [{coll2-id :id} {:name "Regular Collection"}]
- App [{app1-id :id} {:collection_id coll1-id}]
- App [{app2-id :id} {:collection_id coll2-id}]]
+ (mt/with-temp* [Collection [_ {:name "Archived Collection", :archived true}]
+ Collection [_ {:name "Regular Collection"}]]
(letfn [(remove-other-collections [collections]
(filter (fn [{collection-name :name}]
(or (#{"Our analytics" "Archived Collection" "Regular Collection"} collection-name)
(str/includes? collection-name "Personal Collection")))
collections))]
(testing "check that we don't see collections if they're archived"
- (is (= [["Our analytics" nil]
- ["Rasta Toucan's Personal Collection" nil]
- ["Regular Collection" app2-id]]
+ (is (= ["Our analytics"
+ "Rasta Toucan's Personal Collection"
+ "Regular Collection"]
(->> (mt/user-http-request :rasta :get 200 "collection")
remove-other-collections
- (map (juxt :name :app_id))))))
+ (map :name)))))
(testing "Check that if we pass `?archived=true` we instead see archived Collections"
- (is (= [["Archived Collection" app1-id]]
+ (is (= ["Archived Collection"]
(->> (mt/user-http-request :rasta :get 200 "collection" :archived :true)
remove-other-collections
- (map (juxt :name :app_id))))))))
+ (map :name)))))))
+
+ (testing "app collections"
+ (mt/with-temp* [Collection [{coll1-id :id} {:name "Archived Collection", :namespace :apps, :archived true}]
+ Collection [{coll2-id :id} {:name "Regular Collection", :namespace :apps}]
+ App [{app1-id :id} {:collection_id coll1-id}]
+ App [{app2-id :id} {:collection_id coll2-id}]]
+ (letfn [(remove-other-collections [collections]
+ (filter (fn [{collection-name :name}]
+ (or (#{"All apps" "Archived Collection" "Regular Collection"} collection-name)
+ (str/includes? collection-name "Personal Collection")))
+ collections))]
+ (testing "check that we don't see collections if they're archived"
+ (is (= [["All apps" nil]
+ ["Regular Collection" app2-id]]
+ (->> (mt/user-http-request :crowberto :get 200 "collection?namespace=apps")
+ remove-other-collections
+ (map (juxt :name :app_id))))))
+
+ (testing "Check that if we pass `?archived=true` we instead see archived Collections"
+ (is (= [["Archived Collection" app1-id]]
+ (->> (mt/user-http-request :crowberto :get 200 "collection?namespace=apps&archived=true")
+ remove-other-collections
+ (map (juxt :name :app_id)))))))))
(testing "?namespace= parameter"
(mt/with-temp* [Collection [{normal-id :id} {:name "Normal Collection"}]
@@ -184,41 +206,35 @@
(testing "sanity check"
(is (some? personal-collection)))
(with-collection-hierarchy [a b c d e f g]
- (mt/with-temp* [App [{app-a-id :id} {:collection_id (:id a)}]
- App [{app-c-id :id} {:collection_id (:id c)}]
- App [{app-g-id :id} {:collection_id (:id g)}]]
- (let [ids (set (map :id (cons personal-collection [a b c d e f g])))
- response (mt/user-http-request :rasta :get 200 "collection/tree")]
- (testing "Make sure overall tree shape of the response is as is expected"
- (is (= [{:name "A"
- :app_id app-a-id
- :children [{:name "B", :children []}
- {:name "C"
- :app_id app-c-id
- :children [{:name "D"
- :children [{:name "E", :children []}]}
- {:name "F"
- :children [{:name "G"
- :app_id app-g-id
- :children []}]}]}]}
- {:name "Rasta Toucan's Personal Collection", :children []}]
- (collection-tree-view ids [:name :app_id] response))))
- (testing "Make sure each Collection comes back with the expected keys"
- (is (= {:description nil
- :archived false
- :entity_id (:entity_id personal-collection)
- :slug "rasta_toucan_s_personal_collection"
- :color "#31698A"
- :name "Rasta Toucan's Personal Collection"
- :personal_owner_id (mt/user->id :rasta)
- :id (:id (collection/user->personal-collection (mt/user->id :rasta)))
- :location "/"
- :namespace nil
- :children []
- :authority_level nil}
- (some #(when (= (:id %) (:id (collection/user->personal-collection (mt/user->id :rasta))))
- %)
- response)))))))
+ (let [ids (set (map :id (cons personal-collection [a b c d e f g])))
+ response (mt/user-http-request :rasta :get 200 "collection/tree")]
+ (testing "Make sure overall tree shape of the response is as is expected"
+ (is (= [{:name "A"
+ :children [{:name "B", :children []}
+ {:name "C"
+ :children [{:name "D"
+ :children [{:name "E", :children []}]}
+ {:name "F"
+ :children [{:name "G"
+ :children []}]}]}]}
+ {:name "Rasta Toucan's Personal Collection", :children []}]
+ (collection-tree-view ids [:name :app_id] response))))
+ (testing "Make sure each Collection comes back with the expected keys"
+ (is (= {:description nil
+ :archived false
+ :entity_id (:entity_id personal-collection)
+ :slug "rasta_toucan_s_personal_collection"
+ :color "#31698A"
+ :name "Rasta Toucan's Personal Collection"
+ :personal_owner_id (mt/user->id :rasta)
+ :id (:id (collection/user->personal-collection (mt/user->id :rasta)))
+ :location "/"
+ :namespace nil
+ :children []
+ :authority_level nil}
+ (some #(when (= (:id %) (:id (collection/user->personal-collection (mt/user->id :rasta))))
+ %)
+ response))))))
(testing "Excludes archived collections (#19603)"
(mt/with-temp* [Collection [a {:name "A"}]
Collection [b {:name "B archived"
@@ -324,13 +340,19 @@
(deftest fetch-collection-test
(testing "GET /api/collection/:id"
(testing "check that we can see collection details"
- (mt/with-temp* [Collection [collection {:name "Coin Collection"}]
- App [{app-id :id} {:collection_id (:id collection)}]]
- (perms/grant-collection-read-permissions! (perms-group/all-users) collection)
- (is (= ["Coin Collection" app-id]
- ((juxt :name :app_id)
+ (mt/with-temp Collection [collection {:name "Coin Collection"}]
+ (is (= "Coin Collection"
+ (:name
(mt/user-http-request :rasta :get 200 (str "collection/" (u/the-id collection))))))))
+ (testing "check that we can see app collection details"
+ (mt/with-all-users-permission (perms/app-root-collection-permission :read)
+ (mt/with-temp* [Collection [collection {:name "Coin Collection", :namespace :apps}]
+ App [{app-id :id} {:collection_id (:id collection)}]]
+ (is (= ["Coin Collection" app-id]
+ ((juxt :name :app_id)
+ (mt/user-http-request :rasta :get 200 (str "collection/" (u/the-id collection)))))))))
+
(testing "check that collections detail properly checks permissions"
(mt/with-non-admin-groups-no-root-collection-perms
(mt/with-temp Collection [collection]
@@ -686,9 +708,10 @@
(deftest filter-facet-test
(testing "Filter facets"
(mt/with-temp* [Collection [_ {:name "Top level collection"}]
- Collection [{app-coll-id :id} {:name "App with items"}]
+ Collection [{app-coll-id :id} {:name "App with items", :namespace :apps}]
App [{app-id :id} {:collection_id app-coll-id}]
Collection [_ {:name "subcollection"
+ :namespace :apps
:location (format "/%d/" app-coll-id)
:authority_level "official"}]
Card [_ {:name "card" :collection_id app-coll-id}]
@@ -697,36 +720,35 @@
Dashboard [_ {:name "page" :collection_id app-coll-id :is_app_page true}]]
(let [items (->> "/items?models=dashboard&models=card&models=collection"
(str "collection/" app-coll-id)
- (mt/user-http-request :rasta :get 200)
+ (mt/user-http-request :crowberto :get 200)
:data)]
(is (= #{"card" "dash" "subcollection"}
(into #{} (map :name) items))))
(let [items (->> "/items?models=dashboard&models=card&models=collection&models=dataset"
(str "collection/" app-coll-id)
- (mt/user-http-request :rasta :get 200)
+ (mt/user-http-request :crowberto :get 200)
:data)]
(is (= #{"card" "dash" "subcollection" "dataset"}
(into #{} (map :name) items))))
(let [items (->> "/items?models=page"
(str "collection/" app-coll-id)
- (mt/user-http-request :rasta :get 200)
+ (mt/user-http-request :crowberto :get 200)
:data)]
(is (= #{"page"}
(into #{} (map :name) items))))
(let [items (mt/user-http-request
- :rasta :get 200 "collection/root/items?models=app")]
+ :crowberto :get 200 "collection/root/items?models=app&namespace=apps")]
(is (partial= [{:id app-coll-id
:app_id app-id
:model "app"}]
(:data items))))
(let [items (mt/user-http-request
- :rasta :get 200 "collection/root/items")]
- (is (= #{["app" "App with items"]
- ["collection" "Top level collection"]
- ["collection" "Rasta Toucan's Personal Collection"]}
+ :crowberto :get 200 "collection/root/items")]
+ (is (= #{["collection" "Top level collection"]
+ ["collection" "Crowberto Corv's Personal Collection"]}
(into #{} (map (juxt :model :name)) (:data items)))))
(let [items (->> (str "collection/" app-coll-id "/items")
- (mt/user-http-request :rasta :get 200)
+ (mt/user-http-request :crowberto :get 200)
:data)]
(is (= #{"card" "dash" "subcollection" "dataset" "page"}
(into #{} (map :name) items)))))))
@@ -1539,7 +1561,7 @@
(update :entity_id string?))))))
(testing "I shouldn't be allowed to move an App away from root."
- (mt/with-temp* [Collection [collection-a]
+ (mt/with-temp* [Collection [collection-a {:namespace :apps}]
App [_app {:collection_id (:id collection-a)}]
Collection [collection-b]]
(is (= "You don't have permissions to do that."
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 413865c120b..19e990f010a 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -97,21 +97,22 @@
#(assoc % :collection {:id true, :name true :authority_level nil :app_id false})
(default-search-results)))
-(defn- do-with-search-items [search-string in-root-collection? f]
- (let [data-map (fn [instance-name]
- {:name (format instance-name search-string)})
+(defn- do-with-search-items [search-string {:keys [in-root-collection? in-app-collection?]} f]
+ (let [data-map (fn [instance-name app-collection?]
+ (cond-> {:name (format instance-name search-string)}
+ app-collection? (assoc :namespace :apps)))
coll-data-map (fn [instance-name collection]
- (merge (data-map instance-name)
+ (merge (data-map instance-name false)
(when-not in-root-collection?
{:collection_id (u/the-id collection)})))]
- (mt/with-temp* [Collection [coll (data-map "collection %s collection")]
+ (mt/with-temp* [Collection [coll (data-map "collection %s collection" in-app-collection?)]
Card [card (coll-data-map "card %s card" coll)]
Card [dataset (assoc (coll-data-map "dataset %s dataset" coll)
:dataset true)]
Dashboard [dashboard (coll-data-map "dashboard %s dashboard" coll)]
Pulse [pulse (coll-data-map "pulse %s pulse" coll)]
- Metric [metric (data-map "metric %s metric")]
- Segment [segment (data-map "segment %s segment")]]
+ Metric [metric (data-map "metric %s metric" false)]
+ Segment [segment (data-map "segment %s segment" false)]]
(f {:collection coll
:card card
:dataset dataset
@@ -121,10 +122,13 @@
:segment segment}))))
(defmacro ^:private with-search-items-in-root-collection [search-string & body]
- `(do-with-search-items ~search-string true (fn [~'_] ~@body)))
+ `(do-with-search-items ~search-string {:in-root-collection? true} (fn [~'_] ~@body)))
(defmacro ^:private with-search-items-in-collection [created-items-sym search-string & body]
- `(do-with-search-items ~search-string false (fn [~created-items-sym] ~@body)))
+ `(do-with-search-items ~search-string nil (fn [~created-items-sym] ~@body)))
+
+(defmacro ^:private with-search-items-in-app-collection [created-items-sym search-string & body]
+ `(do-with-search-items ~search-string {:in-app-collection? true} (fn [~created-items-sym] ~@body)))
(def ^:private ^:dynamic *search-request-results-database-id*
"Filter out all results from `search-request` that don't have this Database ID. Default: the default H2 `test-data`
@@ -614,7 +618,7 @@
(deftest app-test
(testing "App collections should come with app_id set"
- (with-search-items-in-collection {:keys [collection]} "test"
+ (with-search-items-in-app-collection {:keys [collection]} "test"
(mt/with-temp App [_app {:collection_id (:id collection)}]
(is (= (mapv
(fn [result]
@@ -622,14 +626,14 @@
(not (#{"metric" "segment"} (:model result))) (assoc-in [:collection :app_id] true)
(= (:model result) "collection") (assoc :model "app" :app_id true)))
(default-results-with-collection))
- (search-request-data :rasta :q "test"))))))
+ (search-request-data :crowberto :q "test"))))))
(testing "App collections should filterable as \"app\""
- (mt/with-temp* [Collection [collection {:name "App collection to find"}]
+ (mt/with-temp* [Collection [collection {:name "App collection to find", :namespace :apps}]
App [_ {:collection_id (:id collection)}]
Collection [_ {:name "Another collection to find"}]]
(is (partial= [(assoc (select-keys collection [:name])
:model "app")]
- (search-request-data :rasta :q "find" :models "app"))))))
+ (search-request-data :crowberto :q "find" :models "app"))))))
(deftest page-test
(testing "Search results should pages with model \"page\""
@@ -643,10 +647,10 @@
(deftest collection-app-id-test
(testing "app_id and id of containing collection should not be confused (#25213)"
- (mt/with-temp* [Collection [{coll-id :id}]
+ (mt/with-temp* [Collection [{coll-id :id} {:namespace :apps}]
;; The ignored elements are there to make sure the IDs
;; coll-id and app-id are different.
- Collection [{ignored-collection-id :id}]
+ Collection [{ignored-collection-id :id} {:namespace :apps}]
App [_ignored-app {:collection_id ignored-collection-id}]
App [{app-id :id} {:collection_id coll-id}]
Dashboard [_ {:name "Not a page but contains important text!"
@@ -664,4 +668,4 @@
(is (not= app-id coll-id) "app-id and coll-id should be different. Fix the test!")
(is (partial= (repeat 4 {:collection {:app_id app-id
:id coll-id}})
- (:data (make-search-request :rasta [:q "important text"])))))))
+ (:data (make-search-request :crowberto [:q "important text"])))))))
diff --git a/test/metabase/models/card_test.clj b/test/metabase/models/card_test.clj
index 6d5cdb75e91..c2eef2ecaf1 100644
--- a/test/metabase/models/card_test.clj
+++ b/test/metabase/models/card_test.clj
@@ -117,7 +117,7 @@
(try
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"A Card can only go in Collections in the \"default\" namespace"
+ #"A Card can only go in Collections in the \"default\" or :apps namespace"
(db/insert! Card (assoc (tt/with-temp-defaults Card) :collection_id collection-id, :name card-name))))
(finally
(db/delete! Card :name card-name)))))
@@ -126,7 +126,7 @@
(mt/with-temp Card [{card-id :id}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"A Card can only go in Collections in the \"default\" namespace"
+ #"A Card can only go in Collections in the \"default\" or :apps namespace"
(db/update! Card card-id {:collection_id collection-id})))))))
(deftest normalize-result-metadata-test
diff --git a/test/metabase/models/collection/graph_test.clj b/test/metabase/models/collection/graph_test.clj
index 0313290dcfa..5d102962d36 100644
--- a/test/metabase/models/collection/graph_test.clj
+++ b/test/metabase/models/collection/graph_test.clj
@@ -437,9 +437,10 @@
(deftest modify-perms-for-app-collections-test
(testing "that we cannot modify perms for app collections"
- (mt/with-temp* [Collection [{coll-id :id}]
+ (mt/with-temp* [Collection [{coll-id :id} {:namespace :apps}]
App [_app {:collection_id coll-id}]]
(is (thrown-with-msg? clojure.lang.ExceptionInfo #"Cannot set app permissions using this endpoint"
- (graph/update-graph! (assoc-in (graph/graph)
+ (graph/update-graph! :apps
+ (assoc-in (graph/graph :apps)
[:groups (u/the-id (perms-group/all-users)) coll-id]
:read)))))))
diff --git a/test/metabase/models/collection_test.clj b/test/metabase/models/collection_test.clj
index 24ee3ca7714..c4d6eceadf9 100644
--- a/test/metabase/models/collection_test.clj
+++ b/test/metabase/models/collection_test.clj
@@ -1468,7 +1468,7 @@
(mt/with-temp Collection [{collection-id :id} {:namespace "x"}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"A Card can only go in Collections in the \"default\" namespace"
+ #"A Card can only go in Collections in the \"default\" or :apps namespace"
(collection/check-collection-namespace Card collection-id)))))
(testing "Should throw exception if Collection does not exist"
diff --git a/test/metabase/models/dashboard_test.clj b/test/metabase/models/dashboard_test.clj
index bd08f962afa..2786d9e40eb 100644
--- a/test/metabase/models/dashboard_test.clj
+++ b/test/metabase/models/dashboard_test.clj
@@ -270,7 +270,7 @@
(try
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"A Dashboard can only go in Collections in the \"default\" namespace"
+ #"A Dashboard can only go in Collections in the \"default\" or :apps namespace"
(db/insert! Dashboard (assoc (tt/with-temp-defaults Dashboard) :collection_id collection-id, :name dashboard-name))))
(finally
(db/delete! Dashboard :name dashboard-name)))))
@@ -279,7 +279,7 @@
(mt/with-temp Dashboard [{card-id :id}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"A Dashboard can only go in Collections in the \"default\" namespace"
+ #"A Dashboard can only go in Collections in the \"default\" or :apps namespace"
(db/update! Dashboard card-id {:collection_id collection-id})))))))
(deftest validate-parameters-test
diff --git a/test/metabase/models/pulse_test.clj b/test/metabase/models/pulse_test.clj
index 430b9a791fb..fe90b7fa860 100644
--- a/test/metabase/models/pulse_test.clj
+++ b/test/metabase/models/pulse_test.clj
@@ -400,7 +400,7 @@
(try
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"A Pulse can only go in Collections in the \"default\" namespace"
+ #"A Pulse can only go in Collections in the \"default\" or :apps namespace"
(db/insert! Pulse (assoc (tt/with-temp-defaults Pulse) :collection_id collection-id, :name pulse-name))))
(finally
(db/delete! Pulse :name pulse-name)))))
@@ -409,7 +409,7 @@
(mt/with-temp Pulse [{card-id :id}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"A Pulse can only go in Collections in the \"default\" namespace"
+ #"A Pulse can only go in Collections in the \"default\" or :apps namespace"
(db/update! Pulse card-id {:collection_id collection-id})))))))
diff --git a/test/metabase/test.clj b/test/metabase/test.clj
index 4f7ba902547..661016bd29f 100644
--- a/test/metabase/test.clj
+++ b/test/metabase/test.clj
@@ -202,6 +202,7 @@
secret-value-equals?
select-keys-sequentially
throw-if-called
+ with-all-users-permission
with-column-remappings
with-discarded-collections-perms-changes
with-env-keys-renamed-by
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index 654a43600c6..a2ee69735fd 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -806,6 +806,25 @@
:namespace (name ~collection-namespace))
(fn [] ~@body)))
+(defn do-with-all-users-permission
+ "Call `f` without arguments in a context where the ''All Users'' group
+ is granted the permission specified by `permission-path`.
+
+ For most use cases see the macro [[with-all-users-permission]]."
+ [permission-path f]
+ (tt/with-temp Permissions [_ {:group_id (:id (perms-group/all-users))
+ :object permission-path}]
+ (f)))
+
+(defmacro with-all-users-permission
+ "Run `body` with the ''All Users'' group being granted the permission
+ specified by `permission-path`.
+
+ (mt/with-all-users-permission (perms/app-root-collection-permission :read)
+ ...)"
+ [permission-path & body]
+ `(do-with-all-users-permission ~permission-path (fn [] ~@body)))
+
(defn doall-recursive
"Like `doall`, but recursively calls doall on map values and nested sequences, giving you a fully non-lazy object.
Useful for tests when you need the entire object to be realized in the body of a `binding`, `with-redefs`, or
--
GitLab