Add new `valid-path-format?` predicate for schema validation to fix...

Add new `valid-path-format?` predicate for schema validation to fix downgrading issues generically (#21255)

src/metabase/models/permissions.clj

@@ -302,9 +302,19 @@
                       (seq path))
              (re-matches path-regex path))))
 
+(defn valid-path-format?
+  "Is `path` a string with a valid permissions path format? This is a less strict version of [[valid-path?]] which
+  just checks that the path components contain alphanumeric characters or dashes, separated by slashes
+  This should be used for schema validation in most places, to preserve downgradability when new permissions paths are
+  added."
+  ^Boolean [^String path]
+  (boolean (when (and (string? path)
+                      (seq path))
+             (re-matches (re-pattern (str "^/(" path-char "*/)*$")) path))))
+
 (def Path
-  "Schema for a valid permissions path."
-  (s/pred valid-path? "Valid permissions path"))
+  "Schema for a permissions path with a valid format."
+  (s/pred valid-path-format? "Valid permissions path"))
 
 (defn- assert-not-admin-group
   "Check to make sure the `:group_id` for `permissions` entry isn't the admin group."

test/metabase/models/permissions_test.clj

@@ -14,54 +14,59 @@
 
 ;;; ----------------------------------------------- valid-path? -----------------------------------------------
 
+(def ^:private valid-paths
+  ["/db/1/"
+   "/db/1/native/"
+   "/db/1/schema/"
+   "/db/1/schema/public/"
+   "/db/1/schema/PUBLIC/"
+   "/db/1/schema//"
+   "/db/1/schema/1234/"
+   "/db/1/schema/public/table/1/"
+   "/db/1/schema/PUBLIC/table/1/"
+   "/db/1/schema//table/1/"
+   "/db/1/schema/public/table/1/"
+   "/db/1/schema/PUBLIC/table/1/"
+   "/db/1/schema//table/1/"
+   "/db/1/schema/1234/table/1/"
+   "/db/1/schema/PUBLIC/table/1/query/"
+   "/db/1/schema/PUBLIC/table/1/query/segmented/"
+   ;; download permissions
+   "/download/db/1/"
+   "/download/limited/db/1/"
+   "/download/db/1/native/"
+   "/download/limited/db/1/native/"
+   "/download/db/1/schema/PUBLIC/"
+   "/download/limited/db/1/schema/PUBLIC/"
+   "/download/db/1/schema/PUBLIC/table/1/"
+   "/download/limited/db/1/schema/PUBLIC/table/1/"
+   ;; block permissions
+   "/block/db/1/"
+   "/block/db/1000/"
+   ;; full admin (everything) root permissions
+   "/"])
+
+(def ^:private valid-paths-with-slashes
+  [;; 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/"])
+
 (deftest valid-path-test
   (testing "valid paths"
-    (doseq [path
-            ["/db/1/"
-             "/db/1/native/"
-             "/db/1/schema/"
-             "/db/1/schema/public/"
-             "/db/1/schema/PUBLIC/"
-             "/db/1/schema//"
-             "/db/1/schema/1234/"
-             "/db/1/schema/public/table/1/"
-             "/db/1/schema/PUBLIC/table/1/"
-             "/db/1/schema//table/1/"
-             "/db/1/schema/public/table/1/"
-             "/db/1/schema/PUBLIC/table/1/"
-             "/db/1/schema//table/1/"
-             "/db/1/schema/1234/table/1/"
-             "/db/1/schema/PUBLIC/table/1/query/"
-             "/db/1/schema/PUBLIC/table/1/query/segmented/"
-             ;; download permissions
-             "/download/db/1/"
-             "/download/limited/db/1/"
-             "/download/db/1/native/"
-             "/download/limited/db/1/native/"
-             "/download/db/1/schema/PUBLIC/"
-             "/download/limited/db/1/schema/PUBLIC/"
-             "/download/db/1/schema/PUBLIC/table/1/"
-             "/download/limited/db/1/schema/PUBLIC/table/1/"
-             ;; block permissions
-             "/block/db/1/"
-             "/block/db/1000/"
-             ;; full admin (everything) root permissions
-             "/"]]
+    (doseq [path valid-paths]
       (testing (pr-str path)
         (is (= true
                (perms/valid-path? path)))))
 
     (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/"]]
+      (doseq [path valid-paths-with-slashes]
         (testing (pr-str path)
           (is (= true
                  (perms/valid-path? path)))))))
@@ -193,6 +198,42 @@
           (is (= expected
                  (perms/valid-path? path))))))))
 
+(deftest valid-path-format-test
+  (testing "known valid paths"
+    (doseq [path (concat valid-paths valid-paths-with-slashes)]
+      (testing (pr-str path)
+        (is (= true
+               (perms/valid-path-format? path))))))
+
+  (testing "unknown paths with valid path format"
+    (doseq [path ["/asdf/"
+                  "/asdf/ghjk/"
+                  "/asdf-ghjk/"
+                  "/adsf//"
+                  "/asdf/1/ghkl/"
+                  "/asdf\\/ghkl/"
+                  "/asdf\\\\ghkl/"]]
+      (testing (pr-str path)
+        (is (= true
+               (perms/valid-path-format? path))))))
+
+  (testing "invalid paths"
+    (doseq [path
+            [""
+             "/asdf"
+             "asdf/"
+             "123"
+             nil
+             {}
+             []
+             true
+             false
+             (keyword "/asdf/")
+             1234]]
+      (testing (pr-str path)
+        (is (= false
+               (perms/valid-path-format? path)))))))
+
 
 ;;; -------------------------------------------------- data-perms-path ---------------------------------------------------