Fix Permissions Graph failed when schema name has path char (#22090)

* MVP fix

* make parser more robust

* adding tests

enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions_test.clj

@@ -226,3 +226,27 @@
                clojure.lang.ExceptionInfo
                #"The details permissions functionality is only enabled if you have a premium token with the advanced-permissions feature."
                (ee-perms/update-db-details-permissions! group-id (mt/id) :yes))))))))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                                    Graph                                                       |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(deftest get-graph-should-unescape-slashes-test
+  (testing "If a schema name contains slash, getting graph should unescape it"
+    (doseq [[perm-type perm-value] [[:data-model :all] [:download :full]]]
+      (testing (pr-str perm-type)
+        (testing "slash"
+          (mt/with-temp PermissionsGroup [group]
+            (#'ee-perms/grant-permissions! perm-type perm-value (:id group) (mt/id) "schema/with_slash" )
+            (is (= "schema/with_slash"
+                   (-> (get-in (perms/data-perms-graph) [:groups (u/the-id group) (mt/id) perm-type :schemas])
+                       keys
+                       first)))))
+
+        (testing "backslash"
+          (mt/with-temp PermissionsGroup [group]
+            (#'ee-perms/grant-permissions! perm-type perm-value (:id group) (mt/id) "schema\\with_backslash")
+            (is (= "schema\\with_backslash"
+                   (-> (get-in (perms/data-perms-graph) [:groups (u/the-id group) (mt/id) perm-type :schemas])
+                       keys
+                       first)))))))))

src/metabase/api/permission_graph.clj

@@ -5,7 +5,8 @@
   then postwalk to actually perform the conversion."
   (:require [clojure.spec.alpha :as s]
             [clojure.spec.gen.alpha :as gen]
-            [clojure.walk :as walk]))
+            [clojure.walk :as walk]
+            [metabase.util :as u]))
 
 (defmulti ^:private convert
   "convert values from the naively converted json to what we REALLY WANT"
@@ -19,9 +20,13 @@
   [[_ s]]
   (keyword s))
 
+;; Convert a keyword to string without excluding the namespace.
+;; e.g: :schema/name => "schema/name".
+;; Primarily used for schema-name since schema are allowed to have "/"
+;; and calling (name s) returning a substring after "/".
 (defmethod convert :kw->str
   [[_ s]]
-  (name s))
+  (u/qualified-name s))
 
 (defmethod convert :nil->none
   [[_ _]]

src/metabase/models/permissions/parse.clj

@@ -5,6 +5,7 @@
   - 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 :refer [match]]
+            [clojure.string :as str]
             [clojure.tools.logging :as log]
             [clojure.walk :as walk]
             [instaparse.core :as insta]
@@ -17,7 +18,7 @@
   db          = <'/db/'> #'\\d+' <'/'> ( native | schemas )?
   native      = <'native/'>
   schemas     = <'schema/'> schema?
-  schema      = #'[^/]*' <'/'> table?
+  schema      = schema-name <'/'> table?
   table       = <'table/'> #'\\d+' <'/'> (table-perm <'/'>)?
   table-perm  = ('read'|'query'|'query/segmented')
 
@@ -28,16 +29,18 @@
   dl-db       = <'/db/'> #'\\d+' <'/'> ( dl-native | dl-schemas )?
   dl-native   = <'native/'>
   dl-schemas  = <'schema/'> dl-schema?
-  dl-schema   = #'[^/]*' <'/'> dl-table?
+  dl-schema   = schema-name <'/'> dl-table?
   dl-table    = <'table/'> #'\\d+' <'/'>
 
   data-model  = <'/data-model'> dm-db
   dm-db       = <'/db/'> #'\\d+' <'/'> dm-schema?
-  dm-schema   = <'schema/'> #'[^/]*' <'/'> dm-table?
+  dm-schema   = <'schema/'> schema-name <'/'> dm-table?
   dm-table    = <'table/'> #'\\d+' <'/'>
 
   details  = <'/details'> <'/db/'> #'\\d+' <'/'>
 
+  schema-name = #'(\\\\/|[^/])*' (* schema name can have \\/ but not /*)
+
   collection  = <'/collection/'> #'[^/]*' <'/'> ('read' <'/'>)?")
 
 (def ^:private ^{:arglists '([s])} parser
@@ -48,6 +51,16 @@
   [id]
   (if (= id "root") :root (Long/parseUnsignedLong id)))
 
+(defn- unescape-path-component
+  "Unescape slashes for things that has been escaped before storing in DB (e.g: DB schema name).
+  To find things that were being escaped: check references of [[metabase.models.permissions/escape-path-component]].
+
+    (unescape-path-component \"a\\/b\" => \"a/b\")."
+  [s]
+  (some-> s
+          (str/replace "\\/" "/")     ; \/ -> /
+          (str/replace "\\\\" "\\"))) ; \\ -> \
+
 (defn- append-to-all
   "If `path-or-paths` is a single path, append `x` to the end of it. If it's a vector of paths, append `x` to each path."
   [path-or-paths x]
@@ -59,6 +72,7 @@
   [tree]
   (match tree
     [:permission t]                (path1 t)
+    [:schema-name schema-name]     (unescape-path-component schema-name)
     [:all]                         [:all] ; admin permissions
     [:db db-id]                    (let [db-id (Long/parseUnsignedLong db-id)]
                                      [[:db db-id :data :native :write]
@@ -67,8 +81,8 @@
                                      (into [:db db-id] (path1 db-node)))
     [:schemas]                     [:data :schemas :all]
     [:schemas schema]              (into [:data :schemas] (path1 schema))
-    [:schema schema-name]          [schema-name :all]
-    [:schema schema-name table]    (into [schema-name] (path1 table))
+    [:schema schema-name]          [(path1 schema-name) :all]
+    [:schema schema-name table]    (into [(path1 schema-name)] (path1 table))
     [:table table-id]              [(Long/parseUnsignedLong table-id) :all]
     [:table table-id table-perm]   (into [(Long/parseUnsignedLong table-id)] (path1 table-perm))
     [:table-perm perm]              (case perm
@@ -89,8 +103,8 @@
                                      (into [:db db-id] (path1 db-node)))
     [:dl-schemas]                  [:download :schemas]
     [:dl-schemas schema]           (into [:download :schemas] (path1 schema))
-    [:dl-schema schema-name]       [schema-name]
-    [:dl-schema schema-name table] (into [schema-name] (path1 table))
+    [:dl-schema schema-name]       [(path1 schema-name)]
+    [:dl-schema schema-name table] (into [(path1 schema-name)] (path1 table))
     [:dl-table table-id]           [(Long/parseUnsignedLong table-id)]
     [:dl-native]                   [:download :native]
     ;; collection perms
@@ -104,14 +118,15 @@
   (match tree
     (_ :guard insta/failure?)      (log/error (trs "Error parsing permissions tree {0}" (pr-str tree)))
     [:permission t]                (path2 t)
+    [:schema-name schema-name]     (unescape-path-component schema-name)
     ;; data model perms
     [:data-model db-node]          (path2 db-node)
     [:dm-db db-id]                 (let [db-id (Long/parseUnsignedLong db-id)]
                                      [:db db-id :data-model :schemas :all])
     [:dm-db db-id db-node]         (let [db-id (Long/parseUnsignedLong db-id)]
                                      (into [:db db-id :data-model :schemas] (path2 db-node)))
-    [:dm-schema schema-name]       [schema-name :all]
-    [:dm-schema schema-name table] (into [schema-name] (path2 table))
+    [:dm-schema schema-name]       [(path2 schema-name) :all]
+    [:dm-schema schema-name table] (into [(path2 schema-name)] (path2 table))
     [:dm-table table-id]           [(Long/parseUnsignedLong table-id) :all]
     ;; DB details perms
     [:details db-id]            (let [db-id (Long/parseUnsignedLong db-id)]

test/metabase/models/permissions_test.clj

@@ -716,6 +716,24 @@
                    (perms)))))))))
 
 
+(deftest get-graph-should-unescape-slashes-test
+  (testing "If a schema name contains slash, getting graph should unescape it"
+    (testing "slash"
+      (mt/with-temp PermissionsGroup [group]
+        (perms/grant-permissions! group (perms/data-perms-path (mt/id) "schema/with_slash" (mt/id :venues)))
+        (is (= "schema/with_slash"
+               (-> (get-in (perms/data-perms-graph) [:groups (u/the-id group) (mt/id) :data :schemas])
+                   keys
+                   first)))))
+
+    (testing "back slash"
+      (mt/with-temp PermissionsGroup [group]
+        (perms/grant-permissions! group (perms/data-perms-path (mt/id) "schema\\with_backslash" (mt/id :venues)))
+        (is (= "schema\\with_backslash"
+               (-> (get-in (perms/data-perms-graph) [:groups (u/the-id group) (mt/id) :data :schemas])
+                   keys
+                   first)))))))
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                 Granting/Revoking Permissions Helper Functions                                 |
 ;;; +----------------------------------------------------------------------------------------------------------------+