permission parser (#10933)

introduce a new permission parser to:

* handle permissions in a more declarative way
* allow permissions to be converted to a permission graph for databases without tables

project.clj

@@ -91,6 +91,7 @@
     :exclusions [org.clojure/clojure
                  org.flatland/ordered
                  org.yaml/snakeyaml]]
+   [instaparse "1.4.10"]                                              ; Make your own parser
    [kixi/stats "0.4.4" :exclusions [org.clojure/data.avl]]            ; Various statistic measures implemented as transducers
    [log4j/log4j "1.2.17"                                              ; logging framework. TODO - consider upgrading to Log4j 2 -- see https://logging.apache.org/log4j/log4j-2.6.1/manual/migration.html
     :exclusions [javax.mail/mail
@@ -123,7 +124,8 @@
    [ring/ring-json "0.4.0"]                                           ; Ring middleware for reading/writing JSON automatically
    [stencil "0.5.0"]                                                  ; Mustache templates for Clojure
    [toucan "1.14.0" :exclusions [org.clojure/java.jdbc honeysql]]     ; Model layer, hydration, and DB utilities
-   [weavejester/dependency "0.2.1"]]                                  ; Dependency graphs and topological sorting
+   [weavejester/dependency "0.2.1"]                                   ; Dependency graphs and topological sorting
+   ]
 
   :main ^:skip-aot metabase.core
 

src/metabase/models/permissions.clj

@@ -13,6 +13,7 @@
              [interface :as i]
              [permissions-group :as group]
              [permissions-revision :as perms-revision :refer [PermissionsRevision]]]
+            [metabase.models.permissions.parse :as perms-parse]
             [metabase.util
              [honeysql-extensions :as hx]
              [i18n :as ui18n :refer [deferred-tru trs tru]]
@@ -301,10 +302,12 @@
 
 (def ^:private TablePermissionsGraph
   (s/named
-   (s/cond-pre (s/enum :none :all)
-               {:read  (s/enum :all :none)
-                :query (s/enum :all :segmented :none)})
-   "Valid perms graph for a Table"))
+    (s/cond-pre (s/enum :none :all)
+                (s/constrained
+                  {(s/optional-key :read)  (s/enum :all :none)
+                   (s/optional-key :query) (s/enum :all :segmented :none)}
+                  not-empty))
+    "Valid perms graph for a Table"))
 
 (def ^:private SchemaPermissionsGraph
   (s/named
@@ -368,72 +371,29 @@
 ;;; |                                                  GRAPH FETCH                                                   |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn- permissions-for-path
-  "Given a `permissions-set` of all allowed permissions paths for a Group, return the corresponding permissions status
-   for an object with PATH."
-  [permissions-set path]
-  (u/prog1 (cond
-             (set-has-full-permissions? permissions-set path)    :all
-             (set-has-partial-permissions? permissions-set path) :some
-             :else                                               :none)))
-
-(defn- table->adhoc-native-query-path [table] (adhoc-native-query-path (:db_id table)))
-(defn- table->schema-object-path      [table] (object-path (:db_id table) (:schema table)))
-(defn- table->table-object-path       [table] (object-path (:db_id table) (:schema table) (:id table)))
-(defn- table->all-schemas-path        [table] (all-schemas-path (:db_id table)))
-
-(s/defn ^:private table-graph :- TablePermissionsGraph [permissions-set table]
-  (case (permissions-for-path permissions-set (table->table-object-path table))
-    :all  :all
-    :none :none
-    :some {:read  (permissions-for-path permissions-set (table-read-path table))
-           :query (case (permissions-for-path permissions-set (table-query-path table))
-                    :all  :all
-                    :none :none
-                    :some (case (permissions-for-path permissions-set (table-segmented-query-path table))
-                            :all  :segmented
-                            :none :none))}))
-
-
-(s/defn ^:private schema-graph :- SchemaPermissionsGraph [permissions-set tables]
-  (case (permissions-for-path permissions-set (table->schema-object-path (first tables)))
-    :all  :all
-    :none :none
-    :some (into {} (for [table tables]
-                     {(u/get-id table) (table-graph permissions-set table)}))))
-
-(s/defn ^:private db-graph :- DBPermissionsGraph [permissions-set tables]
-  {:native
-   (case (permissions-for-path permissions-set (table->adhoc-native-query-path (first tables)))
-     :all  :write
-     :some :read
-     :none :none)
-
-   :schemas
-   (case (permissions-for-path permissions-set (table->all-schemas-path (first tables)))
-     :all  :all
-     :none :none
-     (into {} (for [[schema tables] (group-by :schema tables)]
-                ;; if schema is nil, replace it with an empty string, since that's how it will get encoded in JSON :D
-                {(str schema) (schema-graph permissions-set tables)})))})
-
-(s/defn ^:private group-graph :- GroupPermissionsGraph [permissions-set tables]
-  (m/map-vals (partial db-graph permissions-set)
-              tables))
-
-;; TODO - if a DB has no tables, then it won't show up in the permissions graph!
+(defn all-permissions
+  "Handle '/' permission"
+  [db-ids]
+  (reduce (fn [g db-id]
+            (assoc g db-id {:native  :write
+                            :schemas :all}))
+          {}
+          db-ids))
+
 (s/defn graph :- PermissionsGraph
   "Fetch a graph representing the current permissions status for every Group and all permissioned databases."
   []
   (let [permissions (db/select [Permissions :group_id :object], :group_id [:not= (:id (group/metabot))])
-        tables      (group-by :db_id (db/select ['Table :schema :id :db_id]))]
+        db-ids      (db/select-ids 'Database)]
     {:revision (perms-revision/latest-id)
-     :groups   (into {} (for [group-id (db/select-ids 'PermissionsGroup, :id [:not= (:id (group/metabot))])]
-                          (let [group-permissions-set (set (for [perms permissions
-                                                                 :when (= (:group_id perms) group-id)]
-                                                             (:object perms)))]
-                            {group-id (group-graph group-permissions-set tables)})))}))
-
+     :groups   (->> permissions
+                    (filter (comp #(re-find #"(^/db|^/$)" %) :object))
+                    (group-by :group_id)
+                    (m/map-vals (fn [group-permissions]
+                                  (let [permissions-graph (perms-parse/permissions->graph (map :object group-permissions))]
+                                    (if (= :all permissions-graph)
+                                      (all-permissions db-ids)
+                                      (:db permissions-graph))))))}))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+

src/metabase/models/permissions/parse.clj

+(ns metabase.models.permissions.parse
+  "Parses sets of permissions to create a permission graph. Strategy is:
+
+  - Convert strings to parse tree
+  - Convert parse tree to path, e.g. ['3' :all] or ['3' :schemas :all]
+  - Convert set of paths to a map, the permission graph"
+  (:require [clojure.core.match :as match]
+            [instaparse.core :as insta]))
+
+(def ^:private grammar
+  "Describes permission strings like /db/3/ or /collection/root/read/"
+  "permission = ( all | db | collection )
+  all         = <'/'>
+  db          = <'/db/'> #'\\d+' <'/'> ( native | schemas )?
+  native      = <'native/'>
+  schemas     = <'schema/'> schema?
+  schema      = #'[^/]*' <'/'> table?
+  table       = <'table/'> #'\\d+' <'/'> (table-perm <'/'>)?
+  table-perm  = ('read'|'query'|'query/segmented')
+
+  collection  = <'/collection/'> #'[^/]*' <'/'> ('read' <'/'>)?")
+
+(def ^:private parser
+  "Function thqat parses permission strings"
+  (insta/parser grammar))
+
+(defn- collection-id
+  [id]
+  (if (= id "root") :root (Integer/parseInt id)))
+
+(defn- path
+  "Recursively build permission path from parse tree"
+  [tree]
+  (match/match tree
+    [:permission t]              (path t)
+    [:all]                       [:all] ;; admin permissions
+    [:db db-id]                  (let [db-id (Integer/parseInt db-id)]
+                                   [[:db db-id :native :write]
+                                    [:db db-id :schemas :all]])
+    [:db db-id db-node]          (let [db-id (Integer/parseInt db-id)]
+                                   (into [:db db-id] (path db-node)))
+    [:schemas]                   [:schemas :all]
+    [:schemas schema]            (into [:schemas] (path schema))
+    [:schema schema-name]        [schema-name :all]
+    [:schema schema-name table]  (into [schema-name] (path table))
+    [:table table-id]            [(Integer/parseInt table-id) :all]
+    [:table table-id table-perm] (into [(Integer/parseInt table-id)] (path table-perm))
+    [:table-perm perm]           (case perm
+                                   "read"            [:read :all]
+                                   "query"           [:query :all]
+                                   "query/segmented" [:query :segmented])
+    [:native]                    [:native :write]
+
+    [:collection id]             [:collection (collection-id id) :write]
+    [:collection id "read"]      [:collection (collection-id id) :read]))
+
+(defn- graph
+  "Given a set of permission paths, return a graph that expresses the most permissions possible for the set
+
+  Works by first doing a conversion like
+  [[3 :schemas :all]
+   [3 :schemas \"PUBLIC\" :all]
+  ->
+  {3 {:schemas {:all ()
+                :public {:all ()}}}}
+
+  Then converting that to
+  {3 {:schemas :all}}"
+  [paths]
+  (->> paths
+       (reduce (fn [paths path]
+                 (if (every? vector? path) ;; handle case wher /db/x/ returns two vectors
+                   (into paths path)
+                   (conj paths path)))
+               [])
+       (clojure.walk/prewalk (fn [x]
+                               (if (and (sequential? x)
+                                        (sequential? (first x))
+                                        (seq (first x)))
+                                 (->> x
+                                      (group-by first)
+                                      (reduce-kv (fn [m k v]
+                                                   (assoc m k (->> (map rest v)
+                                                                   (filter seq))))
+                                                 {}))
+                                 x)))
+       (clojure.walk/prewalk (fn [x]
+                               (if-let [terminal (and (map? x)
+                                                      (some #(and (= (% x) '()) %)
+                                                            [:all :some :write :read :segmented]))]
+                                 terminal
+                                 x)))))
+
+(defn permissions->graph
+  "Given a set of permission strings, return a graph that expresses the most permissions possible for the set"
+  [permissions]
+  (->> permissions
+       (map (comp path parser))
+       (graph)))

test/metabase/api/permissions_test.clj

@@ -48,10 +48,7 @@
 
 ;; make sure we can update the perms graph from the API
 (expect
-  {(data/id :categories) :none
-   (data/id :checkins)   :none
-   (data/id :users)      :none
-   (data/id :venues)     :all}
+  {(data/id :venues) :all}
   (tt/with-temp PermissionsGroup [group]
     ((test-users/user->client :crowberto) :put 200 "permissions/graph"
      (assoc-in (perms/graph)
@@ -60,20 +57,17 @@
     (get-in (perms/graph) [:groups (u/get-id group) (data/id) :schemas "PUBLIC"])))
 
 (expect
- {(data/id :categories) :none
-  (data/id :checkins)   :none
-  (data/id :users)      :none
-  (data/id :venues)     {:read  :all
-                         :query :segmented}}
- (tt/with-temp PermissionsGroup [group]
-   (test-users/create-users-if-needed!)
-   ((test-users/user->client :crowberto) :put 200 "permissions/graph"
-    (assoc-in (perms/graph)
-              [:groups (u/get-id group) (data/id) :schemas]
-              {"PUBLIC" {(data/id :venues) {:read :all, :query :segmented}}}))
-   (get-in (perms/graph) [:groups (u/get-id group) (data/id) :schemas "PUBLIC"])))
+  {(data/id :venues) {:read  :all
+                      :query :segmented}}
+  (tt/with-temp PermissionsGroup [group]
+    (test-users/create-users-if-needed!)
+    ((test-users/user->client :crowberto) :put 200 "permissions/graph"
+     (assoc-in (perms/graph)
+               [:groups (u/get-id group) (data/id) :schemas]
+               {"PUBLIC" {(data/id :venues) {:read :all, :query :segmented}}}))
+    (get-in (perms/graph) [:groups (u/get-id group) (data/id) :schemas "PUBLIC"])))
 
-;; permissions for new dba
+;; permissions for new db
 (expect
   :all
   (let [new-id (inc (data/id))]
@@ -87,4 +81,15 @@
                  :all))
       (get-in (perms/graph) [:groups (u/get-id group) db-id :schemas]))))
 
-;; figure out failing case for when old doesn't exist
+;; permissions for new db with no tables
+(expect
+  :all
+  (let [new-id (inc (data/id))]
+    (tt/with-temp* [PermissionsGroup [group]
+                    Database         [{db-id :id}]]
+      (test-users/create-users-if-needed!)
+      ((test-users/user->client :crowberto) :put 200 "permissions/graph"
+       (assoc-in (perms/graph)
+                 [:groups (u/get-id group) db-id :schemas]
+                 :all))
+      (get-in (perms/graph) [:groups (u/get-id group) db-id :schemas]))))

test/metabase/models/permissions/parse_test.clj

+(ns metabase.models.permissions.parse-test
+  (:require [clojure.test :refer :all]
+            [metabase.models.permissions.parse :as parse]))
+
+(deftest permissions->graph
+  (testing "Parses each permission string to the correct graph"
+    (are [x y] (= y (parse/permissions->graph [x]))
+      "/db/3/"                                       {:db {3 {:native  :write
+                                                              :schemas :all}}}
+      "/db/3/native/"                                {:db {3 {:native :write}}}
+      "/db/3/schema/"                                {:db {3 {:schemas :all}}}
+      "/db/3/schema/PUBLIC/"                         {:db {3 {:schemas {"PUBLIC" :all}}}}
+      "/db/3/schema/PUBLIC/table/4/"                 {:db {3 {:schemas {"PUBLIC" {4 :all}}}}}
+      "/db/3/schema/PUBLIC/table/4/read/"            {:db {3 {:schemas {"PUBLIC" {4 {:read :all}}}}}}
+      "/db/3/schema/PUBLIC/table/4/query/"           {:db {3 {:schemas {"PUBLIC" {4 {:query :all}}}}}}
+      "/db/3/schema/PUBLIC/table/4/query/segmented/" {:db {3 {:schemas {"PUBLIC" {4 {:query :segmented}}}}}})))
+
+
+(deftest combines-permissions-for-graph
+  (testing "When given multiple permission hierarchies, chooses the one with the most permission"
+    ;; This works by creating progressively smaller groups of permissions and asserting the constructed graph
+    ;; expresses the most permissions
+    ;;
+    ;; On the first iteration, it tests that a permission set that includes ALL the keys below
+    ;; gets converted to the graph
+    ;;
+    ;; {3 {:native :all :schemas :all}}
+    ;;
+    ;; The next iteration removes the pair ["/db/3/" {3 {:native :all :schemas :all}}] from the permission set
+    ;; and asserts that the permission graph returned is the one with the next most permissions:
+    ;;
+    ;; {3 {:schemas :all}}
+    ;;
+    ;; Visually, the map is structured to mean "When the permission set includes only this key and the ones below it,
+    ;; the permission graph for this key should be returned"
+    (doseq [group (let [groups (->> {"/db/3/"                                       {:db {3 {:native  :write
+                                                                                             :schemas :all}}}
+
+                                     "/db/3/schema/"                                {:db {3 {:schemas :all}}}
+                                     "/db/3/schema/PUBLIC/"                         {:db {3 {:schemas {"PUBLIC" :all}}}}
+                                     "/db/3/schema/PUBLIC/table/4/"                 {:db {3 {:schemas {"PUBLIC" {4 :all}}}}}
+                                     "/db/3/schema/PUBLIC/table/4/query/"           {:db {3 {:schemas {"PUBLIC" {4 {:read  :all
+                                                                                                                    :query :all}}}}}}
+                                     "/db/3/schema/PUBLIC/table/4/query/segmented/" {:db {3 {:schemas {"PUBLIC" {4 {:read  :all
+                                                                                                                    :query :segmented}}}}}}
+                                     "/db/3/schema/PUBLIC/table/4/read/"            {:db {3 {:schemas {"PUBLIC" {4 {:read :all}}}}}}}
+                                    (into [])
+                                    (sort-by first))]
+                    (->> groups
+                         (partition-all (count groups) 1)
+                         (map vec)))]
+      (is (= (get-in group [0 1])
+             (parse/permissions->graph (map first group)))))))
+
+(deftest permissions->graph-collections
+  (are [x y] (= y (parse/permissions->graph [x]))
+    "/collection/root/"      {:collection {:root :write}}
+    "/collection/root/read/" {:collection {:root :read}}
+    "/collection/1/"         {:collection {1 :write}}
+    "/collection/1/read/"    {:collection {1 :read}}))
+
+(deftest combines-all-permissions
+  (testing "Permision graph includes broadest permissions for all dbs in permission set"
+    (is (= {:db         {3 {:native  :write
+                            :schemas :all}
+                         5 {:schemas {"PUBLIC" {10 {:read :all}}}}}
+            :collection {:root :write
+                         1     :read}}
+           (parse/permissions->graph #{"/db/3/"
+                                       "/db/5/schema/PUBLIC/table/10/read/"
+                                       "/collection/root/"
+                                       "/collection/1/read/"})))))

test/metabase/models/permissions_test.clj

@@ -555,8 +555,8 @@
 
 ;; Test that setting partial permissions for a table retains permissions for other tables -- #3888
 (expect
-  [{(data/id :categories) :none, (data/id :checkins) :none, (data/id :users) :none, (data/id :venues) :all}
-   {(data/id :categories) :all,  (data/id :checkins) :none, (data/id :users) :none, (data/id :venues) :all}]
+  [{(data/id :venues) :all}
+   {(data/id :categories) :all, (data/id :venues) :all}]
   (tt/with-temp PermissionsGroup [group]
     ;; first, graph permissions only for VENUES
     (perms/grant-permissions! group (perms/object-path (data/id) "PUBLIC" (data/id :venues)))
@@ -592,31 +592,20 @@
 
 ;; Make sure we can set the new broken-out read/query perms for a Table and the graph works as we'd expect
 (expect
-  {(data/id :categories) :none
-   (data/id :checkins)   :none
-   (data/id :users)      :none
-   (data/id :venues)     {:read  :all
-                          :query :none}}
+  {(data/id :venues) {:read :all}}
   (tt/with-temp PermissionsGroup [group]
     (perms/grant-permissions! group (perms/table-read-path (Table (data/id :venues))))
     (test-data-graph group)))
 
 (expect
-  {(data/id :categories) :none
-   (data/id :checkins)   :none
-   (data/id :users)      :none
-   (data/id :venues)     {:read  :none
-                          :query :segmented}}
+  {(data/id :venues) {:query :segmented}}
   (tt/with-temp PermissionsGroup [group]
     (perms/grant-permissions! group (perms/table-segmented-query-path (Table (data/id :venues))))
     (test-data-graph group)))
 
 (expect
-  {(data/id :categories) :none
-   (data/id :checkins)   :none
-   (data/id :users)      :none
-   (data/id :venues)     {:read  :all
-                          :query :segmented}}
+  {(data/id :venues) {:read  :all
+                      :query :segmented}}
   (tt/with-temp PermissionsGroup [group]
     (perms/update-graph! [(u/get-id group) (data/id) :schemas]
                          {"PUBLIC"
@@ -624,6 +613,15 @@
                            {:read :all, :query :segmented}}})
     (test-data-graph group)))
 
+;; A "/" permission grants all dataset permissions
+(tt/expect-with-temp [Database [{db_id :id}]]
+  {db_id {:native  :write
+          :schemas :all}}
+  (let [{:keys [group_id]} (db/select-one 'Permissions {:object "/"})]
+    (-> (perms/graph)
+        (get-in [:groups group_id])
+        (select-keys [db_id]))))
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                 Granting/Revoking Permissions Helper Functions                                 |
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -634,8 +632,8 @@
   (do
     (collection-test/force-create-personal-collections!)
     (perms/revoke-collection-permissions!
-     (group/all-users)
-     (u/get-id (db/select-one 'Collection :personal_owner_id (test-users/user->id :lucky))))))
+      (group/all-users)
+      (u/get-id (db/select-one 'Collection :personal_owner_id (test-users/user->id :lucky))))))
 
 ;; (should apply to descendants as well)
 (expect