diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index 50629f0df5910135d14c3f6e1bf40fb73a1f4b6c..ae749495e13aa891da9405956507a35ec9bd6a3d 100644
--- a/src/metabase/models/permissions.clj
+++ b/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."
diff --git a/test/metabase/models/permissions_test.clj b/test/metabase/models/permissions_test.clj
index 39ed678fb60f111edcd4a67bb830551a22b7d89d..a5dcd170c2554b439406170a61c6402de0062211 100644
--- a/test/metabase/models/permissions_test.clj
+++ b/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 ---------------------------------------------------