From 6a3e22baac41ea0b2f5a4f63f0e69f641f9d4f01 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Thu, 28 Apr 2022 15:29:59 +0700
Subject: [PATCH] Fix Permissions Graph failed when schema name has path char
(#22090)
* MVP fix
* make parser more robust
* adding tests
---
.../models/permissions_test.clj | 24 ++++++++++++++
src/metabase/api/permission_graph.clj | 9 +++--
src/metabase/models/permissions/parse.clj | 33 ++++++++++++++-----
test/metabase/models/permissions_test.clj | 18 ++++++++++
4 files changed, 73 insertions(+), 11 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions_test.clj
index dc7cc527688..7b6b134944b 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions_test.clj
+++ b/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)))))))))
diff --git a/src/metabase/api/permission_graph.clj b/src/metabase/api/permission_graph.clj
index 57749c2469c..936780cdd14 100644
--- a/src/metabase/api/permission_graph.clj
+++ b/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
[[_ _]]
diff --git a/src/metabase/models/permissions/parse.clj b/src/metabase/models/permissions/parse.clj
index b19e7882d2f..4bc9cdd88d5 100644
--- a/src/metabase/models/permissions/parse.clj
+++ b/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)]
diff --git a/test/metabase/models/permissions_test.clj b/test/metabase/models/permissions_test.clj
index 0e886d862c3..337d6c3e815 100644
--- a/test/metabase/models/permissions_test.clj
+++ b/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 |
;;; +----------------------------------------------------------------------------------------------------------------+
--
GitLab