From 63e950f256b115b8756ef631d075d681bb9b4114 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Fri, 7 Oct 2022 18:45:46 +0300
Subject: [PATCH] Implement global app permissions for the "All Users" group
(#25679)
---
src/metabase/api/app.clj | 35 ++++++++
src/metabase/models/app/graph.clj | 72 ++++++++++++++++
src/metabase/models/collection/graph.clj | 35 ++++----
test/metabase/api/app_test.clj | 100 +++++++++++++++--------
4 files changed, 194 insertions(+), 48 deletions(-)
create mode 100644 src/metabase/models/app/graph.clj
diff --git a/src/metabase/api/app.clj b/src/metabase/api/app.clj
index f7b796162ca..26a3f86702c 100644
--- a/src/metabase/api/app.clj
+++ b/src/metabase/api/app.clj
@@ -8,6 +8,7 @@
[metabase.api.collection :as api.collection]
[metabase.api.common :as api]
[metabase.models :refer [App Collection Dashboard Table]]
+ [metabase.models.app.graph :as app.graph]
[metabase.models.collection :as collection]
[metabase.models.dashboard :as dashboard]
[metabase.util.i18n :as i18n]
@@ -27,6 +28,7 @@
(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 "/"
@@ -268,4 +270,37 @@
(create-scaffold-dashcards! scaffold-target->id pages)
(hydrate-details (db/select-one App :id app-id)))))
+
+;;; ------------------------------------------------ GRAPH ENDPOINTS -------------------------------------------------
+
+(api/defendpoint GET "/global-graph"
+ "Fetch the global graph of all App Permissions."
+ []
+ (api/check-superuser)
+ (app.graph/global-graph))
+
+(defn- ->int [id] (Integer/parseInt (name id)))
+
+(defn- dejsonify-id->permission-map [m]
+ (into {}
+ (map (fn [[k v]]
+ [(->int k) (keyword v)]))
+ 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))
+
+(api/defendpoint PUT "/global-graph"
+ "Do a batch update of the global App Permissions by passing in a modified graph."
+ [:as {body :body}]
+ {body su/Map}
+ (api/check-superuser)
+ (-> body
+ dejsonify-global-graph
+ app.graph/update-global-graph!)
+ (app.graph/global-graph))
+
(api/define-routes)
diff --git a/src/metabase/models/app/graph.clj b/src/metabase/models/app/graph.clj
new file mode 100644
index 00000000000..3f4d3728470
--- /dev/null
+++ b/src/metabase/models/app/graph.clj
@@ -0,0 +1,72 @@
+(ns metabase.models.app.graph
+ "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.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 ^:private AppPermissions
+ 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))))
+
+(s/defn global-graph :- GlobalPermissionsGraph
+ "Fetch the global app permission graph."
+ []
+ {:revision (c-perm-revision/latest-id)
+ :groups {(:id (perms-group/admin)) :write
+ (:id (perms-group/all-users)) (all-users-app-permission)}})
+
+(s/defn update-global-graph!
+ "Update the global app permission graph to the state specified by
+ `new-graph`."
+ [new-graph :- GlobalPermissionsGraph]
+ (let [old-graph (global-graph)
+ [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)))
+ (seq other-groups))
+ (throw (ex-info (tru "You can configure for the ''All Users'' group only")
+ {: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))))))
diff --git a/src/metabase/models/collection/graph.clj b/src/metabase/models/collection/graph.clj
index f9b4110bec6..32097886363 100644
--- a/src/metabase/models/collection/graph.clj
+++ b/src/metabase/models/collection/graph.clj
@@ -20,7 +20,8 @@
;;; ---------------------------------------------------- Schemas -----------------------------------------------------
-(def ^:private CollectionPermissions
+(def CollectionPermissions
+ "The valid collection permissions."
(s/enum :write :read :none))
(def ^:private GroupPermissionsGraph
@@ -106,20 +107,24 @@
{:status-code 400
:app-ids app-ids})))))
-(s/defn ^:private update-collection-permissions!
- [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 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))))
(s/defn ^:private update-group-permissions!
[collection-namespace :- (s/maybe su/KeywordOrString)
diff --git a/test/metabase/api/app_test.clj b/test/metabase/api/app_test.clj
index 6bf76590b2f..10b3db9d3c9 100644
--- a/test/metabase/api/app_test.clj
+++ b/test/metabase/api/app_test.clj
@@ -2,7 +2,8 @@
(:require
[clojure.test :refer [deftest is testing]]
[medley.core :as m]
- [metabase.models :refer [App Card Collection Dashboard]]
+ [metabase.models :refer [App Card Collection Dashboard Permissions]]
+ [metabase.models.collection.graph :as graph]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
[metabase.test :as mt]
@@ -11,24 +12,32 @@
[toucan.hydrate :refer [hydrate]]))
(deftest create-test
- (mt/with-model-cleanup [Collection]
+ (mt/with-model-cleanup [Collection Permissions]
(let [base-params {:name "App collection"
:color "#123456"}]
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(testing "parent_id is ignored when creating apps"
- (mt/with-temp* [Collection [{collection-id :id}]]
- (let [coll-params (assoc base-params :parent_id collection-id)
- response (mt/user-http-request :crowberto :post 200 "app" {:collection coll-params})]
- (is (pos-int? (:id response)))
- (is (pos-int? (:collection_id response)))
- (is (partial= (assoc base-params :location "/")
- (:collection response))))))
+ (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")))))
(testing "Create app in the root"
- (let [response (mt/user-http-request :crowberto :post 200 "app" {:collection base-params})]
- (is (pos-int? (:id response)))
- (is (pos-int? (:collection_id response)))
- (is (partial= (assoc base-params :location "/")
- (:collection response)))))
+ (mt/with-temporary-setting-values [all-users-app-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"))))
(testing "With initial dashboard and nav_items"
(mt/with-temp Dashboard [{dashboard-id :id}]
(let [nav_items [{:options {:click_behavior {}}}]]
@@ -137,25 +146,29 @@
(mt/user-http-request :rasta :get 403 (str "app/" app-id)))))))))
(deftest scaffold-test
- (mt/with-model-cleanup [Card Dashboard Collection]
+ (mt/with-model-cleanup [Card Dashboard Collection Permissions]
(testing "Golden path"
- (let [app (mt/user-http-request
- :crowberto :post 200 "app/scaffold"
- {:table-ids [(data/id :venues)]
- :app-name "My test app"})
- pages (m/index-by :name (hydrate (db/select Dashboard :collection_id (:collection_id app)) :ordered_cards))
- list-page (get pages "Venues List")
- detail-page (get pages "Venues Detail")]
- (is (partial= {:nav_items [{:page_id (:id list-page)}
- {:page_id (:id detail-page) :hidden true :indent 1}]
- :dashboard_id (:id list-page)}
- app))
- (is (partial= {:ordered_cards [{:visualization_settings {:click_behavior
- {:type "link",
- :linkType "page",
- :targetId (:id detail-page)}}}
- {}]}
- list-page))))
+ (mt/with-temporary-setting-values [all-users-app-permission :read]
+ (let [app (mt/user-http-request
+ :crowberto :post 200 "app/scaffold"
+ {:table-ids [(data/id :venues)]
+ :app-name "My test app"})
+ pages (m/index-by :name (hydrate (db/select Dashboard :collection_id (:collection_id app)) :ordered_cards))
+ list-page (get pages "Venues List")
+ detail-page (get pages "Venues Detail")]
+ (is (partial= {:nav_items [{:page_id (:id list-page)}
+ {:page_id (:id detail-page) :hidden true :indent 1}]
+ :dashboard_id (:id list-page)}
+ app))
+ (is (partial= {:ordered_cards [{:visualization_settings {:click_behavior
+ {:type "link",
+ :linkType "page",
+ :targetId (:id detail-page)}}}
+ {}]}
+ list-page))
+ (is (partial= {:groups {(:id (perms-group/all-users)) {(:collection_id app) :read}}}
+ (graph/graph))
+ "''All Users'' should have the default permission on the app collection"))))
(testing "Bad or duplicate tables"
(is (= (format "Some tables could not be found. Given: (%s %s) Found: (%s)"
(data/id :venues)
@@ -167,7 +180,7 @@
:app-name (str "My test app " (gensym))}))))))
(deftest scaffold-app-test
- (mt/with-model-cleanup [Card Dashboard Collection]
+ (mt/with-model-cleanup [Card Dashboard]
(mt/with-temp* [Collection [{collection-id :id}]
App [{app-id :id} {:collection_id collection-id}]]
(testing "Without existing pages"
@@ -204,3 +217,24 @@
:targetId (:id detail-page)}}}
{}]}
list-page)))))))
+
+(deftest global-graph-test
+ (mt/with-model-cleanup [Collection Permissions]
+ (let [base-params {:name "App collection"
+ :color "#123456"}]
+ (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+ (testing "changing default permission"
+ (mt/with-temp* [Collection [{collection-id :id}]]
+ (let [coll-params (assoc base-params :parent_id collection-id)
+ response1 (mt/user-http-request :crowberto :post 200 "app" {:collection coll-params})
+ 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))))))))))
--
GitLab