alternative implemenation of permission graph conversion (#10919)

* simple fixes for data permissions, new model for conforming permissions

* collection permission conversion

* only assoc values if they're present

* improve docs

* progress on testing permissions changing

* test that setting permissions on new table works

* remove permission graph reworking to introduce in another branch

* clean ns

* alternative impl of permission graph conversion

* correct the permission graph specs

* fix whitespace

* address ci failure

* undo ci config change

* delete mysteriously undeleted merge conflict

src/metabase/api/permission_graph.clj

+(ns metabase.api.permission-graph
+  "Convert the permission graph's naive json conversion into the correct types.
+
+  The strategy here is to use s/conform to tag every value that needs to be converted with the conversion strategy,
+  then postwalk to actually perform the conversion."
+  (:require [clojure.spec.alpha :as s]
+            [clojure.spec.gen.alpha :as gen]
+            [clojure.walk :as walk]))
+
+(defmulti convert
+  "convert values from the naively converted json to what we REALLY WANT"
+  first)
+
+(defmethod convert :kw->int
+  [[_ k]]
+  (Integer/parseInt (name k)))
+
+(defmethod convert :str->kw
+  [[_ s]]
+  (keyword s))
+
+(defmethod convert :kw->str
+  [[_ s]]
+  (name s))
+
+(defmethod convert :nil->none
+  [[_ _]]
+  :none)
+
+(defmethod convert :identity
+  [[_ x]]
+  x)
+
+;;; --------------------------------------------------- Common ----------------------------------------------------
+
+;; ids come in asa keywordized numbers
+(s/def ::id (s/with-gen (s/or :kw->int (s/and keyword? #(re-find #"^\d+$" (name %))))
+              #(gen/fmap (comp keyword str) (s/gen pos-int?))))
+
+(s/def ::native (s/or :str->kw #{"write" "none"}
+                      :nil->none nil?))
+
+;;; --------------------------------------------------- Data Permissions ----------------------------------------------------
+
+(s/def ::schema-name (s/or :kw->str keyword?))
+
+;; {:groups {1 {:schemas {"PUBLIC" ::schema-perms-granular}}}} =>
+;; {:groups {1 {:schemas {"PUBLIC" {1 :all}}}}}
+(s/def ::read (s/or :str->kw #{"all" "none"}))
+(s/def ::query (s/or :str->kw #{"all" "none" "segmented"}))
+
+(s/def ::table-perms-granular (s/keys :opt-un [::read ::query]))
+
+(s/def ::table-perms (s/or :str->kw #{"all" "segmented" "none"}
+                           :identity ::table-perms-granular))
+
+(s/def ::table-graph (s/map-of ::id ::table-perms
+                               :conform-keys true))
+
+(s/def ::schema-perms (s/or :str->kw #{"all" "segmented" "none"}
+                            :identity ::table-graph))
+
+;; {:groups {1 {:schemas {"PUBLIC" ::schema-perms}}}}
+(s/def ::schema-graph (s/map-of ::schema-name ::schema-perms
+                                :conform-keys true))
+
+;; {:groups {1 {:schemas ::schemas}}}
+(s/def ::schemas (s/or :str->kw   #{"all" "segmented" "none"}
+                       :nil->none nil?
+                       :identity  ::schema-graph))
+
+(s/def ::db-perms (s/keys :opt-un [::native ::schemas]))
+
+(s/def ::db-graph (s/map-of ::id ::db-perms
+                            :conform-keys true))
+
+
+(s/def :metabase.api.permission-graph.data/groups
+  (s/map-of ::id ::db-graph
+            :conform-keys true))
+
+(s/def ::data-permissions-graph
+  (s/keys :req-un [:metabase.api.permission-graph.data/groups]))
+
+
+;;; --------------------------------------------------- Collection Permissions ----------------------------------------------------
+
+(s/def ::collections
+  (s/map-of (s/or :identity ::id
+                  :str->kw  #{"root"})
+            (s/or :str->kw #{"read" "write" "none"})))
+
+(s/def ::collection-graph
+  (s/map-of ::id ::collections))
+
+(s/def :metabase.api.permission-graph.collection/groups
+  (s/map-of ::id ::collection-graph
+            :conform-keys true))
+
+(s/def ::collection-permissions-graph
+  (s/keys :req-un [:metabase.api.permission-graph.collection/groups]))
+
+(defn converted-json->graph
+  "The permissions graph is received as JSON. That JSON is naively converted. This performs a further conversion to
+  convert graph keys and values to the types we want to work with."
+  [spec kwj]
+  (->> (s/conform spec kwj)
+       (walk/postwalk (fn [x]
+                        (if (and (vector? x) (get-method convert (first x)))
+                          (convert x)
+                          x)))))

src/metabase/api/permissions.clj

@@ -4,7 +4,9 @@
             [metabase
              [metabot :as metabot]
              [util :as u]]
-            [metabase.api.common :as api]
+            [metabase.api
+             [common :as api]
+             [permission-graph :as pg]]
             [metabase.models
              [permissions :as perms]
              [permissions-group :as group :refer [PermissionsGroup]]
@@ -18,45 +20,6 @@
 ;;; |                                          PERMISSIONS GRAPH ENDPOINTS                                           |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-;;; ------------------------------------------------- DeJSONifaction -------------------------------------------------
-
-(defn- ->int [id] (Integer/parseInt (name id)))
-
-(defn- dejsonify-table-perms [table-perms]
-  (if-not (map? table-perms)
-    (keyword table-perms)
-    (into {} (for [[k v] table-perms]
-               [(keyword k) (keyword v)]))))
-
-(defn- dejsonify-tables [tables]
-  (if (string? tables)
-    (keyword tables)
-    (into {} (for [[table-id perms] tables]
-               {(->int table-id) (dejsonify-table-perms perms)}))))
-
-(defn- dejsonify-schemas [schemas]
-  (if (string? schemas)
-    (keyword schemas)
-    (into {} (for [[schema tables] schemas]
-               {(name schema) (dejsonify-tables tables)}))))
-
-(defn- dejsonify-dbs [dbs]
-  (into {} (for [[db-id {:keys [native schemas]}] dbs]
-             {(->int db-id) (cond-> {}
-                              native (assoc :native (keyword native))
-                              schemas (assoc :schemas (dejsonify-schemas schemas)))})))
-
-(defn- dejsonify-groups [groups]
-  (into {} (for [[group-id dbs] groups]
-             {(->int group-id) (dejsonify-dbs dbs)})))
-
-(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))
-
-
 ;;; --------------------------------------------------- Endpoints ----------------------------------------------------
 
 (api/defendpoint GET "/graph"
@@ -78,7 +41,7 @@
   [:as {body :body}]
   {body su/Map}
   (api/check-superuser)
-  (perms/update-graph! (dejsonify-graph body))
+  (perms/update-graph! (pg/converted-json->graph ::pg/data-permissions-graph body))
   (perms/graph))
 
 
@@ -91,14 +54,14 @@
   groups.)"
   []
   (let [results (db/query
-                 {:select    [[:pgm.group_id :group_id] [:%count.pgm.id :members]]
-                  :from      [[:permissions_group_membership :pgm]]
-                  :left-join [[:core_user :user] [:= :pgm.user_id :user.id]]
-                  :where     [:= :user.is_active true]
-                  :group-by  [:pgm.group_id]})]
+                  {:select    [[:pgm.group_id :group_id] [:%count.pgm.id :members]]
+                   :from      [[:permissions_group_membership :pgm]]
+                   :left-join [[:core_user :user] [:= :pgm.user_id :user.id]]
+                   :where     [:= :user.is_active true]
+                   :group-by  [:pgm.group_id]})]
     (zipmap
-     (map :group_id results)
-     (map :members results))))
+      (map :group_id results)
+      (map :members results))))
 
 (defn- ordered-groups
   "Return a sequence of ordered `PermissionsGroups`, including the `MetaBot` group only if MetaBot is enabled."