From cf9b1bf50f2e025ef7a93f88d6a8e18cedfbba7c Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Sat, 8 Oct 2022 01:05:49 +0300
Subject: [PATCH] Implement EE/Pro app permissions (#25764)
* Implement per group and per db app permissions
---
.../models/permissions/app_permissions.clj | 102 ++++++++++++++++++
.../permissions/app_permissions_test.clj | 71 ++++++++++++
src/metabase/api/app.clj | 51 ++++++++-
src/metabase/models/app/graph.clj | 9 +-
src/metabase/models/collection/graph.clj | 22 ++--
5 files changed, 242 insertions(+), 13 deletions(-)
create mode 100644 enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/app_permissions.clj
create mode 100644 enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/app_permissions_test.clj
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
new file mode 100644
index 00000000000..f590bb204e6
--- /dev/null
+++ b/enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/app_permissions.clj
@@ -0,0 +1,102 @@
+(ns metabase-enterprise.advanced-permissions.models.permissions.app-permissions
+ "Implements granular app permissions by reading and writing the permissions
+ 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.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))
+
+(s/defn graph :- PermissionsGraph
+ "Returns the app permission graph. Throws an exception if the
+ advanced-permissions feature is not enabled."
+ []
+ (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)))))
+
+(s/defn update-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]
+ (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))
+ (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
new file mode 100644
index 00000000000..10413ff5fda
--- /dev/null
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/app_permissions_test.clj
@@ -0,0 +1,71 @@
+(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.permissions-group :as perms-group]
+ [metabase.public-settings.premium-features-test :as premium-features-test]
+ [metabase.test :as mt]))
+
+(deftest graph-test
+ (testing "GET /api/app/graph works only with advanced permissions"
+ (premium-features-test/with-premium-features #{}
+ (is (= "The granular app permission functionality is only enabled if you have a premium token with the advanced-permissions feature."
+ (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 "/"}]
+ 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"}
+ group-id {app-id "none"}}
+ (:groups (mt/user-http-request :crowberto :get 200 "app/graph")))))
+
+ (testing "have to be a superuser"
+ (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)))
+ "Shouldn't confuse collection IDs and app IDs")))))))
+
+(deftest graph-update-test
+ (testing "PUT /api/app/graph works only with advanced permissions"
+ (premium-features-test/with-premium-features #{}
+ (is (= "The granular app permission functionality is only enabled if you have a premium token with the advanced-permissions feature."
+ (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")
+
+ (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)))))))))))
diff --git a/src/metabase/api/app.clj b/src/metabase/api/app.clj
index 26a3f86702c..2a8557235fe 100644
--- a/src/metabase/api/app.clj
+++ b/src/metabase/api/app.clj
@@ -11,6 +11,8 @@
[metabase.models.app.graph :as app.graph]
[metabase.models.collection :as collection]
[metabase.models.dashboard :as dashboard]
+ [metabase.plugins.classloader :as classloader]
+ [metabase.util :as u]
[metabase.util.i18n :as i18n]
[metabase.util.schema :as su]
[schema.core :as s]
@@ -279,20 +281,52 @@
(api/check-superuser)
(app.graph/global-graph))
+(defn- resolve-advanced-app-permission-function
+ "Resolve `fn-name` in the advanced app permission namespace. `fn-name` can be
+ a string or an instance of clojure.lang.Named.
+ Throws an exception if `fn-name` cannot be resolved."
+ [fn-name]
+ (or (u/ignore-exceptions
+ (classloader/require 'metabase-enterprise.advanced-permissions.models.permissions.app-permissions)
+ (resolve (symbol "metabase-enterprise.advanced-permissions.models.permissions.app-permissions"
+ (name fn-name))))
+ (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})))
+
+(api/defendpoint GET "/graph"
+ "Fetch the graph of all App Permissions."
+ []
+ (api/check-superuser)
+ (let [graph (resolve-advanced-app-permission-function 'graph)]
+ (graph)))
+
(defn- ->int [id] (Integer/parseInt (name id)))
-(defn- dejsonify-id->permission-map [m]
+(defn- dejsonify-with [f m]
(into {}
(map (fn [[k v]]
- [(->int k) (keyword v)]))
+ [(->int k) (f v)]))
m))
+(defn- dejsonify-id->permission-map [m]
+ (dejsonify-with keyword m))
+
+(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."
+ [graph]
+ (update graph :groups dejsonify-groups-map))
+
(api/defendpoint PUT "/global-graph"
"Do a batch update of the global App Permissions by passing in a modified graph."
[:as {body :body}]
@@ -300,7 +334,16 @@
(api/check-superuser)
(-> body
dejsonify-global-graph
- app.graph/update-global-graph!)
- (app.graph/global-graph))
+ app.graph/update-global-graph!))
+
+(api/defendpoint PUT "/graph"
+ "Do a batch update of the advanced App Permissions by passing in a modified graph."
+ [:as {body :body}]
+ {body su/Map}
+ (api/check-superuser)
+ (let [update-graph! (resolve-advanced-app-permission-function 'update-graph!)]
+ (-> body
+ dejsonify-graph
+ update-graph!)))
(api/define-routes)
diff --git a/src/metabase/models/app/graph.clj b/src/metabase/models/app/graph.clj
index 3f4d3728470..0cabeeccc96 100644
--- a/src/metabase/models/app/graph.clj
+++ b/src/metabase/models/app/graph.clj
@@ -14,7 +14,9 @@
[schema.core :as s]
[toucan.db :as db]))
-(def ^:private AppPermissions
+(def AppPermissions
+ "The valid app permissions. Currently corresponds 1:1 to `CollectionPermissions`
+ since app permissions are implemented in terms of collection permissions."
graph/CollectionPermissions)
(def ^:private GlobalPermissionsGraph
@@ -51,7 +53,7 @@
(s/defn update-global-graph!
"Update the global app permission graph to the state specified by
- `new-graph`."
+ `new-graph`. Returns the new graph as read from the updated database."
[new-graph :- GlobalPermissionsGraph]
(let [old-graph (global-graph)
[diff-old changes] (data/diff (:groups old-graph) (:groups new-graph))]
@@ -69,4 +71,5 @@
(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))))))
+ CollectionPermissionGraphRevision (:revision old-graph) old-graph changes))
+ (global-graph)))))
diff --git a/src/metabase/models/collection/graph.clj b/src/metabase/models/collection/graph.clj
index 32097886363..777fa69e590 100644
--- a/src/metabase/models/collection/graph.clj
+++ b/src/metabase/models/collection/graph.clj
@@ -69,6 +69,17 @@
[:not [:like :location (hx/literal (format "/%d/%%" collection-id))]]))}]
(set (map :id (db/query honeysql-form)))))
+(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]
+ (let [group-id->perms (group-id->permissions-set)]
+ {:revision (c-perm-revision/latest-id)
+ :groups (into {} (for [group-id (db/select-ids PermissionsGroup)]
+ {group-id (group-permissions-graph collection-namespace
+ (group-id->perms group-id)
+ collection-ids)}))})))
+
(s/defn graph :- PermissionsGraph
"Fetch a graph representing the current permissions status for every group and all permissioned collections. This
works just like the function of the same name in `metabase.models.permissions`; see also the documentation for that
@@ -86,11 +97,10 @@
(graph nil))
([collection-namespace :- (s/maybe su/KeywordOrString)]
- (let [group-id->perms (group-id->permissions-set)
- collection-ids (non-personal-collection-ids collection-namespace)]
- {:revision (c-perm-revision/latest-id)
- :groups (into {} (for [group-id (db/select-ids PermissionsGroup)]
- {group-id (group-permissions-graph collection-namespace (group-id->perms group-id) collection-ids)}))})))
+ (db/transaction
+ (-> collection-namespace
+ non-personal-collection-ids
+ (collection-permission-graph collection-namespace)))))
;;; -------------------------------------------------- Update Graph --------------------------------------------------
@@ -152,8 +162,8 @@
[diff-old changes] (data/diff old-perms new-perms)]
(perms/log-permissions-changes diff-old changes)
(perms/check-revision-numbers old-graph new-graph)
- (check-no-app-collections changes)
(when (seq changes)
+ (check-no-app-collections changes)
(db/transaction
(doseq [[group-id changes] changes]
(update-group-permissions! collection-namespace group-id changes))
--
GitLab