Support forward slashes in schema names (#13476)

src/metabase/api/database.clj

@@ -667,7 +667,7 @@
   at least some of its tables?)"
   [database-id schema-name]
   (perms/set-has-partial-permissions? @api/*current-user-permissions-set*
-    (perms/object-path database-id schema-name)))
+                                      (perms/object-path database-id schema-name)))
 
 (api/defendpoint GET "/:id/schemas"
   "Returns a list of all the schemas found for the database `id`"

src/metabase/models/permissions.clj

@@ -18,6 +18,7 @@
             [metabase.util
              [honeysql-extensions :as hx]
              [i18n :as ui18n :refer [deferred-tru trs tru]]
+             [regex :as u.regex]
              [schema :as su]]
             [schema.core :as s]
             [toucan
@@ -43,23 +44,71 @@
 
 ;;; --------------------------------------------------- Validation ---------------------------------------------------
 
+(def ^:private path-char
+  "Valid character for a name that appears in a permissions path (e.g. a schema name or a Collection name). Character is
+  valid if it is either:
+
+  1. Any character other than a slash
+  2. A forward slash, escaped by a backslash: `\\/`
+  3. A backslash escaped by a backslash: `\\\\`"
+  (u.regex/rx (or #"[^\\/]" #"\\/" #"\\\\")))
+
+(def ^:private valid-object-path-regex
+  "Regex for a valid permissions path. `rx` macro is used to make the big-and-hairy macro somewhat readable."
+  (u.regex/rx "^/"
+              ;; any path starting with /db/ is a DATA PERMISSIONS path
+              (or
+               ;; /db/:id/ -> permissions for the entire DB -- native and all schemas
+               (and #"db/\d+/"
+                    (opt (or
+                          ;; .../native/ -> permissions to create new native queries for the DB
+                          "native/"
+                          ;; .../schema/ -> permissions for all schemas in the DB
+                          (and "schema/"
+                               ;; .../schema/:name/ -> permissions for a specific schema
+                               (opt (and path-char "*/"
+                                         ;; .../schema/:name/table/:id/ -> FULL permissions for a specific table
+                                         (opt (and #"table/\d+/"
+                                                   (opt (or
+                                                         ;; .../read/ -> Perms to fetch the Metadata for Table
+                                                         "read/"
+                                                         ;; .../query/ -> Perms to run any sort of query against Table
+                                                         (and "query/"
+                                                              ;; .../segmented/ -> Permissions to run a query against
+                                                              ;; a Table using GTAP
+                                                              (opt "segmented/"))))))))))))
+               ;; any path starting with /collection/ is a COLLECTION permissions path
+               (and "collection/"
+                    (or
+                     ;; /collection/:id/ -> readwrite perms for a specific Collection
+                     (and #"\d+/"
+                          ;; /collection/:id/read/ -> read perms for a specific Collection
+                          (opt "read/"))
+                     ;; /collection/root/ -> readwrite perms for the Root Collection
+                     (and "root/"
+                          ;; /collection/root/read/ -> read perms for the Root Collection
+                          (opt "read/"))
+                     ;; /collection/namespace/:namespace/root/ -> readwrite perms for 'Root' Collection in non-default
+                     ;; namespace (only really used for EE)
+                     (and "namespace/" path-char "+/root/"
+                          ;; /collection/namespace/:namespace/root/read/ -> read perms for 'Root' Collection in
+                          ;; non-default namespace
+                          (opt "read/")))))
+              "$"))
+
 (def segmented-perm-regex
   "Regex that matches a segmented permission"
-  #"^/db/(\d+)/schema/([^\\/]*)/table/(\d+)/query/segmented/$")
-
-(def ^:private valid-object-path-patterns
-  [#"^/db/(\d+)/$"                                        ; permissions for the entire DB -- native and all schemas
-   #"^/db/(\d+)/native/$"                                 ; permissions to create new native queries for the DB
-   #"^/db/(\d+)/schema/$"                                 ; permissions for all schemas in the DB
-   #"^/db/(\d+)/schema/([^/]*)/$"                         ; permissions for a specific schema
-   #"^/db/(\d+)/schema/([^/]*)/table/(\d+)/$"             ; FULL permissions for a specific table
-   #"^/db/(\d+)/schema/([^/]*)/table/(\d+)/read/$"        ; Permissions to fetch the Metadata for a specific Table
-   #"^/db/(\d+)/schema/([^/]*)/table/(\d+)/query/$"       ; Permissions to run any sort of query against a Table
-   segmented-perm-regex                                   ; Permissions to run a query against a Table using GTAP
-   #"^/collection/(\d+)/$"                                ; readwrite permissions for a collection
-   #"^/collection/(\d+)/read/$"                           ; read permissions for a collection
-   #"^/collection/root/$"                                 ; readwrite permissions for the 'Root' Collection (things with `nil` collection_id)
-   #"^/collection/root/read/$"])                          ; read permissions for the 'Root' Collection
+  (re-pattern (str #"^/db/\d+/schema/" path-char "*" #"/table/\d+/query/segmented/$")))
+
+(defn- escape-path-component
+  "Escape slashes in something that might be passed as a string part of a permissions path (e.g. DB schema name or
+  Collection name).
+
+    (escape-path-component \"a/b\") ;-> \"a\\/b\""
+  [s]
+  (some-> s
+          (str/replace #"\\" "\\\\\\\\")   ; \ -> \\
+          (str/replace #"/" "\\\\/"))) ; / -> \/
 
 (defn valid-object-path?
   "Does `object-path` follow a known, allowed format to an *object*? (The root path, \"/\", is not considered an object;
@@ -67,8 +116,7 @@
   ^Boolean [^String object-path]
   (boolean (when (and (string? object-path)
                       (seq object-path))
-             (some #(re-matches % object-path)
-                   valid-object-path-patterns))))
+             (re-matches valid-object-path-regex object-path))))
 
 (def ObjectPath
   "Schema for a valid permissions path to an object."
@@ -128,8 +176,10 @@
   you don't. Tables, however, have separate read and write perms.)"
   ([database-or-id :- MapOrID]
    (str "/db/" (u/get-id database-or-id) "/"))
+
   ([database-or-id :- MapOrID, schema-name :- (s/maybe s/Str)]
-   (str (object-path database-or-id) "schema/" schema-name "/"))
+   (str (object-path database-or-id) "schema/" (escape-path-component schema-name) "/"))
+
   ([database-or-id :- MapOrID, schema-name :- (s/maybe s/Str), table-or-id :- MapOrID]
    (str (object-path database-or-id schema-name) "table/" (u/get-id table-or-id) "/" )))
 
@@ -147,59 +197,59 @@
 (s/defn collection-readwrite-path :- ObjectPath
   "Return the permissions path for *readwrite* access for a `collection-or-id`."
   [collection-or-id :- MapOrID]
-  (str "/collection/"
-       (if (get collection-or-id :metabase.models.collection.root/is-root?)
-         "root"
-         (u/get-id collection-or-id))
-       "/"))
+  (if-not (get collection-or-id :metabase.models.collection.root/is-root?)
+    (format "/collection/%d/" (u/get-id collection-or-id))
+    (if-let [collection-namespace (:namespace collection-or-id)]
+      (format "/collection/namespace/%s/root/" (escape-path-component (u/qualified-name collection-namespace)))
+      "/collection/root/")))
 
 (s/defn collection-read-path :- ObjectPath
   "Return the permissions path for *read* access for a `collection-or-id`."
   [collection-or-id :- MapOrID]
   (str (collection-readwrite-path collection-or-id) "read/"))
 
-(defn table-read-path
+(s/defn table-read-path :- ObjectPath
   "Return the permissions path required to fetch the Metadata for a Table."
-  (^String [table]
+  ([table]
    (table-read-path (:db_id table) (:schema table) table))
-  (^String [database-or-id schema-name table-or-id]
+
+  ([database-or-id schema-name table-or-id]
    {:post [(valid-object-path? %)]}
    (str (object-path (u/get-id database-or-id) schema-name (u/get-id table-or-id)) "read/")))
 
-(defn table-query-path
+(s/defn table-query-path :- ObjectPath
   "Return the permissions path for *full* query access for a Table. Full query access means you can run any (MBQL) query
   you wish against a given Table, with no GTAP-specified mandatory query alterations."
-  (^String [table]
+  ([table]
    (table-query-path (:db_id table) (:schema table) table))
-  (^String [database-or-id schema-name table-or-id]
-   {:post [(valid-object-path? %)]}
+
+  ([database-or-id schema-name table-or-id]
    (str (object-path (u/get-id database-or-id) schema-name (u/get-id table-or-id)) "query/")))
 
-(defn table-segmented-query-path
+(s/defn table-segmented-query-path :- ObjectPath
   "Return the permissions path for *segmented* query access for a Table. Segmented access means running queries against
   the Table will automatically replace the Table with a GTAP-specified question as the new source of the query,
   obstensibly limiting access to the results."
-  (^String [table]
+  ([table]
    (table-segmented-query-path (:db_id table) (:schema table) table))
-  (^String [database-or-id schema-name table-or-id]
-   {:post [(valid-object-path? %)]}
+
+  ([database-or-id schema-name table-or-id]
    (str (object-path (u/get-id database-or-id) schema-name (u/get-id table-or-id)) "query/segmented/")))
 
 
 ;;; -------------------------------------------- Permissions Checking Fns --------------------------------------------
 
 (defn is-permissions-for-object?
-  "Does `permissions`-PATH grant *full* access for OBJECT-PATH?"
+  "Does `permissions-path` grant *full* access for `object-path`?"
   [permissions-path object-path]
   (str/starts-with? object-path permissions-path))
 
 (defn is-partial-permissions-for-object?
-  "Does `permissions`-PATH grant access full access for OBJECT-PATH *or* for a descendant of OBJECT-PATH?"
+  "Does `permissions-path` grant access full access for `object-path` *or* for a descendant of `object-path`?"
   [permissions-path object-path]
   (or (is-permissions-for-object? permissions-path object-path)
       (str/starts-with? permissions-path object-path)))
 
-
 (defn is-permissions-set?
   "Is `permissions-set` a valid set of permissions object paths?"
   ^Boolean [permissions-set]
@@ -248,9 +298,9 @@
                    {:metabase.models.collection.root/is-root? true}))}))
 
 (def IObjectPermissionsForParentCollection
-  "Implementation of `IObjectPermissions` for objects that have a `collection_id`, and thus, a parent Collection.
-   Using this will mean the current User is allowed to read or write these objects if they are allowed to read or
-  write their parent Collection."
+  "Implementation of `IObjectPermissions` for objects that have a `collection_id`, and thus, a parent Collection. Using
+  this will mean the current User is allowed to read or write these objects if they are allowed to read or write their
+  parent Collection."
   (merge i/IObjectPermissionsDefaults
          ;; TODO - we use these same partial implementations of `can-read?` and `can-write?` all over the place for
          ;; different models. Consider making them a mixin of some sort. (I was going to do this but I couldn't come
@@ -269,14 +319,16 @@
 (defn- pre-insert [permissions]
   (u/prog1 permissions
     (assert-valid permissions)
-    (log/debug (u/format-color 'green "Granting permissions for group %d: %s" (:group_id permissions) (:object permissions)))))
+    (log/debug (u/format-color 'green "Granting permissions for group %d: %s"
+                 (:group_id permissions) (:object permissions)))))
 
 (defn- pre-update [_]
   (throw (Exception. (str (deferred-tru "You cannot update a permissions entry!")
                           (deferred-tru "Delete it and create a new one.")))))
 
 (defn- pre-delete [permissions]
-  (log/debug (u/format-color 'red "Revoking permissions for group %d: %s" (:group_id permissions) (:object permissions)))
+  (log/debug (u/format-color 'red "Revoking permissions for group %d: %s"
+               (:group_id permissions) (:object permissions)))
   (assert-not-admin-group permissions))
 
 (u/strict-extend (class Permissions)
@@ -291,8 +343,8 @@
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
 ;; TODO - there is so much stuff related to the perms graph I think we should really move it into a separate
-;; `metabase.models.permissions-graph.data` namespace or something and move the collections graph from
-;; `metabase.models.collection` to `metabase.models.permissions-graph.collection` (?)
+;; `metabase.models.permissions.graph.data` namespace or something and move the collections graph from
+;; `metabase.models.collection` to `metabase.models.permissions.graph.collection` (?)
 
 (def ^:private TablePermissionsGraph
   (s/named

src/metabase/util/regex.clj

+(ns metabase.util.regex
+  "Regex-related utility functions"
+  (:require [clojure.string :as str]))
+
+(defn non-capturing-group
+  "Wrap regex `pattern` in a non-capturing group."
+  [pattern]
+  (re-pattern (format "(?:%s)" pattern)))
+
+(defn re-or
+  "Combine regex `patterns` into a single pattern by joining with or (i.e., a logical disjunction)."
+  [& patterns]
+  (non-capturing-group (str/join "|" (map non-capturing-group patterns))))
+
+(defn re-optional
+  "Make regex `pattern` optional."
+  [pattern]
+  (str (non-capturing-group pattern) "?"))
+
+(defmulti ^:private rx-dispatch
+  {:arglists '([listt])}
+  first)
+
+(declare rx*)
+
+(defmethod rx-dispatch :default
+  [x]
+  x)
+
+(defmethod rx-dispatch 'opt
+  [[_ expr]]
+  `(re-optional (rx* ~expr)))
+
+(defmethod rx-dispatch 'or
+  [[_ & args]]
+  `(re-or ~@(for [arg args]
+              `(rx* ~arg))))
+
+(defmethod rx-dispatch 'and
+  [[_ & args]]
+  `(str ~@(for [arg args]
+            `(rx* ~arg))))
+
+(defmacro rx*
+  "Impl of `rx` macro."
+  [x]
+  (if (seqable? x)
+    (rx-dispatch x)
+    x))
+
+(defmacro rx
+  "Cam's quick-and-dirty port of the Emacs Lisp `rx` macro (`C-h f rx`) but not currently as fully-featured. Convenient
+  macro for building mega-huge regular expressions from a sexpr representation.
+
+    (rx (and (or \"Cam\" \"can\") (opt #\"\\s+\") #\"\\d+\"))
+    ;; -> #\"(?:(?:Cam)|(?:can))\\s+?\\d+\"
+
+  Feel free to add support for more stuff as needed.
+  ([x]
+   `(re-pattern (str (rx* ~x))))"
+
+  ([x]
+   `(re-pattern (rx* ~x)))
+
+  ([x & more]
+   `(rx (~'and ~x ~@more))))

test/metabase/api/database_test.clj

@@ -23,6 +23,7 @@
              [fixtures :as fixtures]
              [util :as tu]]
             [metabase.util.schema :as su]
+            [ring.util.codec :as codec]
             [schema.core :as s]
             [toucan
              [db :as db]
@@ -915,3 +916,27 @@
       (testing "to fetch Tables with `nil` or empty schemas, use the blank string"
         (is (= ["t1" "t2"]
                (map :name ((mt/user->client :lucky) :get 200 (format "database/%d/schema/" db-id)))))))))
+
+(deftest slashes-in-identifiers-test
+  (testing "We should handle Databases with slashes in identifiers correctly (#12450)"
+    (mt/with-temp Database [{db-id :id} {:name "my/database"}]
+      (doseq [schema-name ["my/schema"
+                           "my//schema"
+                           "my\\schema"
+                           "my\\\\schema"
+                           "my\\//schema"
+                           "my_schema/"
+                           "my_schema\\"]]
+        (testing (format "\nschema name = %s" (pr-str schema-name))
+          (mt/with-temp Table [{table-id :id} {:db_id db-id, :schema schema-name, :name "my/table"}]
+            (testing "\nFetch schemas"
+              (testing "\nGET /api/database/:id/schemas/"
+                (is (= [schema-name]
+                       ((mt/user->client :rasta) :get 200 (format "database/%d/schemas" db-id))))))
+            (testing (str "\nFetch schema tables -- should work if you URL escape the schema name"
+                          "\nGET /api/database/:id/schema/:schema")
+              (let [url (format "database/%d/schema/%s" db-id (codec/url-encode schema-name))]
+                (testing (str "\nGET /api/" url)
+                  (is (schema= [{:schema (s/eq schema-name)
+                                 s/Keyword s/Any}]
+                               ((mt/user->client :rasta) :get 200 url))))))))))))

test/metabase/models/permissions_test.clj

@@ -32,15 +32,27 @@
              "/db/1/schema/public/table/1/"
              "/db/1/schema/PUBLIC/table/1/"
              "/db/1/schema//table/1/"
-             "/db/1/schema/1234/table/1/"]]
-      (testing path
+             "/db/1/schema/1234/table/1/"
+             "/db/1/schema/PUBLIC/table/1/query/"
+             "/db/1/schema/PUBLIC/table/1/query/segmented/"]]
+      (testing (pr-str path)
         (is (= true
                (perms/valid-object-path? path)))))
 
-    ;; TODO -- we need to esacpe forward-slashes as well
-    (testing "We should allow backslashes in permissions paths? (#8693)"
-      (is (= true
-             (perms/valid-object-path? "/db/16/schema/COMPANY-NET\\john.doe/")))))
+    (testing "\nWe should allow slashes in permissions paths? (#8693, #13263)\n"
+      (doseq [path [ ;; COMPANY-NET\ should get escaped to COMPANY-NET\\
+                    "/db/16/schema/COMPANY-NET\\\\john.doe/"
+                    ;; COMPANY-NET/ should get escaped to COMPANY-NET\/
+                    "/db/16/schema/COMPANY-NET\\/john.doe/"
+                    ;; my\schema should get escaped to my\\schema
+                    "/db/1/schema/my\\\\schema/table/1/"
+                    ;; my\\schema should get escaped to my\\\\schema
+                    "/db/1/schema/my\\\\\\\\schema/table/1/"
+                    ;; my/schema should get escaped to my\/schema
+                    "/db/1/schema/my\\/schema/table/1/"]]
+        (testing (pr-str path)
+          (is (= true
+                 (perms/valid-object-path? path)))))))
 
   (testing "invalid paths"
     (doseq [[reason paths]
@@ -127,13 +139,34 @@
               "/DB/1/"
               "/db/1/SCHEMA/"
               "/db/1/SCHEMA/PUBLIC/"
-              "/db/1/schema/PUBLIC/TABLE/1/"]}]
+              "/db/1/schema/PUBLIC/TABLE/1/"]
+
+             "odd number of backslashes: backslash must be escaped by another backslash" ; e.g. \ -> \\
+             ["/db/1/schema/my\\schema/table/1/"
+              "/db/1/schema/my\\\\\\schema/table/1/"]
+
+             "forward slash must be escaped by a backslash" ; e.g. / -> \/
+             ["/db/1/schema/my/schema/table/1/"]}]
       (testing reason
         (doseq [path paths]
-          (testing path
+          (testing (str "\n" (pr-str path))
             (is (= false
                    (perms/valid-object-path? path)))))))))
 
+(deftest valid-object-path-backslashes-test
+  (testing "Only even numbers of backslashes should be valid (backslash must be escaped by another backslash)"
+    (doseq [[num-backslashes expected schema-name] [[0 true "PUBLIC"]
+                                                    [0 true  "my_schema"]
+                                                    [2 false "my\\schema"]
+                                                    [4 true  "my\\\\schema"]
+                                                    [6 false "my\\\\\\schema"]
+                                                    [8 true  "my\\\\\\\\schema"]]]
+      (doseq [path [(format "/db/1/schema/%s/table/2/" schema-name)
+                    (format "/db/1/schema/%s/table/2/query/" schema-name)]]
+        (testing (str "\n" (pr-str path))
+          (is (= expected
+                 (perms/valid-object-path? path))))))))
+
 
 ;;; -------------------------------------------------- object-path ---------------------------------------------------
 
@@ -171,8 +204,37 @@
                     [1 "public" {}]
                     [1 "public" []]]]
         (testing (pr-str (cons 'perms/object-path args))
-          (is (thrown? Exception
-                       (apply perms/object-path args))))))))
+          (is (thrown?
+               Exception
+               (apply perms/object-path args))))))))
+
+(deftest object-path-escape-slashes-test
+  (doseq [{:keys [slash-direction schema-name expected-escaped]} [{:slash-direction  "back (#8693)"
+                                                                   :schema-name      "my\\schema"
+                                                                   :expected-escaped "my\\\\schema"}
+                                                                  {:slash-direction  "back (multiple)"
+                                                                   :schema-name      "my\\\\schema"
+                                                                   :expected-escaped "my\\\\\\\\schema"}
+                                                                  {:slash-direction  "forward (#12450)"
+                                                                   :schema-name      "my/schema"
+                                                                   :expected-escaped "my\\/schema"}
+                                                                  {:slash-direction  "both"
+                                                                   :schema-name      "my\\/schema"
+                                                                   :expected-escaped "my\\\\\\/schema"}
+                                                                  {:slash-direction  "both (multiple)"
+                                                                   :schema-name      "my\\\\/schema"
+                                                                   :expected-escaped "my\\\\\\\\\\/schema"}]]
+    (testing (format "We should handle slashes in permissions paths\nDirection = %s\nSchema = %s\n"
+                     slash-direction (pr-str schema-name))
+      (testing (pr-str (list 'object-path {:id 1}))
+        (is (= "/db/1/"
+               (perms/object-path {:id 1}))))
+      (testing (pr-str (list 'object-path {:id 1} schema-name))
+        (is (= (format "/db/1/schema/%s/" expected-escaped)
+               (perms/object-path {:id 1} schema-name))))
+      (testing (pr-str (list 'object-path {:id 1} schema-name {:id 2}))
+        (is (= (format "/db/1/schema/%s/table/2/" expected-escaped)
+               (perms/object-path {:id 1} schema-name {:id 2})))))))
 
 
 ;;; ---------------------------------- Generating permissions paths for Collections ----------------------------------

test/metabase/models/table_test.clj

+(ns metabase.models.table-test
+  (:require [clojure.test :refer :all]
+            [metabase.models.table :refer [Table]]
+            [metabase.test :as mt]
+            [toucan.db :as db]))
+
+(deftest slashes-in-schema-names-test
+  (testing "Schema names should allow forward or back slashes (#8693, #12450)"
+    (doseq [schema-name ["my\\schema"
+                         "my\\\\schema"
+                         "my/schema"
+                         "my\\/schema"
+                         "my\\\\/schema"]]
+      (testing (format "Should be able to create/delete Table with schema name %s" (pr-str schema-name))
+        (mt/with-temp Table [{table-id :id} {:schema schema-name}]
+          (is (= schema-name
+                 (db/select-one-field :schema Table :id table-id))))))))

test/metabase/util/regex_test.clj

+(ns metabase.util.regex-test
+  (:require [clojure.test :refer :all]
+            [metabase.util.regex :as u.regex]))
+
+(deftest rx-test
+  (let [regex (u.regex/rx (and "^" (or "Cam" "can") (opt #"\s+") #"\d+"))]
+    (is (instance? java.util.regex.Pattern regex))
+    (is (= (str #"^(?:(?:Cam)|(?:can))(?:\s+)?\d+")
+           (str regex)))))