Skip to content
Snippets Groups Projects
Unverified Commit e6358cc9 authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

Fix setting Block permissions in the permissions/graph API endpoints (#17651) 

* Fix setting Block permissions in the permissions/graph API endpoints

* Revert changes to .dir-locals

* Perms graph: don't validate DB :schemas + :native unless both are present.

* PR feedback
parent 3344e09e
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
enterprise/backend/test/metabase_enterprise/enhancements/models/permissions/block_permissions_test.clj
+ 89
− 46
View file @ e6358cc9
......@@ -11,6 +11,7 @@
[metabase.query-processor.middleware.permissions :as qp.perms]
[metabase.test :as mt]
[metabase.util :as u]
[schema.core :as s]
[toucan.db :as db]))
;;;; Graph-related stuff
......@@ -18,53 +19,84 @@
(defn- test-db-perms [group-id]
(get-in (perms/data-perms-graph) [:groups group-id (mt/id)]))
(defn- api-test-db-perms [group-id]
(into {}
(map (fn [[k v]]
[k (cond-> v (string? v) keyword)]))
(get-in (mt/user-http-request :crowberto :get 200 "permissions/graph")
[:groups
(keyword (str group-id))
(keyword (str (mt/id)))])))
(deftest graph-test
(testing "block permissions should come back from the graph function"
(mt/with-temp* [PermissionsGroup [{group-id :id}]
Permissions [_ {:group_id group-id
:object (perms/database-block-perms-path (mt/id))}]]
(is (= {:schemas :block}
(test-db-perms group-id)))
(testing (str "\nBlock perms and data perms shouldn't exist together at the same time, but if they do for some "
"reason, then the graph endpoint should ignore the data perms.")
(doseq [path [(perms/data-perms-path (mt/id))
(perms/data-perms-path (mt/id) "public")
(perms/data-perms-path (mt/id) "public" (mt/id :venues))]]
(testing (format "\nPath = %s" (pr-str path))
(mt/with-temp* [Permissions [_ {:group_id group-id
:object path}]]
(is (= (merge {:schemas :block}
;; block perms won't affect the value of `:native`; if a given group has both
;; `/db/1/` and `/block/db/1/` then the graph will come back with `:native
;; :write` and `:schemas :block`. This state isn't normally allowed, but the
;; graph code doesn't currently correct it if it happens. Not sure it's worth
;; the extra code complexity since it should never happen in the first place.
(when (= path (perms/data-perms-path (mt/id)))
{:native :write}))
(test-db-perms group-id))))))))))
(testing "block permissions should come back from"
(doseq [[message perms] {"the graph function"
test-db-perms
"the API"
api-test-db-perms}]
(testing (str message "\n"))
(mt/with-temp* [PermissionsGroup [{group-id :id}]
Permissions [_ {:group_id group-id
:object (perms/database-block-perms-path (mt/id))}]]
(is (= {:schemas :block}
(perms group-id)))
(testing (str "\nBlock perms and data perms shouldn't exist together at the same time, but if they do for some "
"reason, then the graph endpoint should ignore the data perms.")
(doseq [path [(perms/data-perms-path (mt/id))
(perms/data-perms-path (mt/id) "public")
(perms/data-perms-path (mt/id) "public" (mt/id :venues))]]
(testing (format "\nPath = %s" (pr-str path))
(mt/with-temp* [Permissions [_ {:group_id group-id
:object path}]]
(is (= (merge {:schemas :block}
;; block perms won't affect the value of `:native`; if a given group has both
;; `/db/1/` and `/block/db/1/` then the graph will come back with `:native
;; :write` and `:schemas :block`. This state isn't normally allowed, but the
;; graph code doesn't currently correct it if it happens. Not sure it's worth
;; the extra code complexity since it should never happen in the first place.
(when (= path (perms/data-perms-path (mt/id)))
{:native :write}))
(perms group-id)))))))))))
(defn- grant-block-perms! [group-id]
(perms/update-data-perms-graph! [group-id (mt/id)] {:schemas :block}))
(defn- api-grant-block-perms! [group-id]
(let [current-graph (perms/data-perms-graph)
new-graph (assoc-in current-graph [:groups group-id (mt/id)] {:schemas :block})
result (mt/user-http-request :crowberto :put 200 "permissions/graph" new-graph)]
(is (= "block"
(get-in result [:groups
(keyword (str group-id))
(keyword (str (mt/id)))
:schemas])))))
(deftest update-graph-test
(testing "Should be able to set block permissions with the graph update function"
(mt/with-temp PermissionsGroup [{group-id :id}]
(testing "Group should have no perms upon creation"
(is (= nil
(test-db-perms group-id))))
(testing "group has no existing permissions"
(mt/with-model-cleanup [Permissions]
(grant-block-perms! group-id)
(is (= {:schemas :block}
(test-db-perms group-id)))))
(testing "group has existing data permissions... :block should remove them"
(mt/with-model-cleanup [Permissions]
(perms/grant-full-db-permissions! group-id (mt/id))
(grant-block-perms! group-id)
(is (= {:schemas :block}
(test-db-perms group-id)))
(is (= #{(perms/database-block-perms-path (mt/id))}
(db/select-field :object Permissions :group_id group-id))))))))
(testing "Should be able to set block permissions with"
(doseq [[description grant!] {"the graph update function"
grant-block-perms!
"the perms graph API endpoint"
api-grant-block-perms!}]
(testing (str description "\n")
(mt/with-temp PermissionsGroup [{group-id :id}]
(testing "Group should have no perms upon creation"
(is (= nil
(test-db-perms group-id))))
(testing "group has no existing permissions"
(mt/with-model-cleanup [Permissions]
(grant! group-id)
(is (= {:schemas :block}
(test-db-perms group-id)))))
(testing "group has existing data permissions... :block should remove them"
(mt/with-model-cleanup [Permissions]
(perms/grant-full-db-permissions! group-id (mt/id))
(grant! group-id)
(is (= {:schemas :block}
(test-db-perms group-id)))
(is (= #{(perms/database-block-perms-path (mt/id))}
(db/select-field :object Permissions :group_id group-id))))))))))
(deftest update-graph-delete-sandboxes-test
(testing "When setting `:block` permissions any GTAP rows for that Group/Database should get deleted."
......@@ -92,11 +124,22 @@
(testing "Disallow block permissions + native query permissions"
(mt/with-temp* [PermissionsGroup [{group-id :id}]
Permissions [_ {:group_id group-id, :object (perms/data-perms-path (mt/id))}]]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
;; TODO -- this error message is totally garbage, fix this
#"DB permissions with a valid combination of values for :native and :schemas"
(perms/update-data-perms-graph! [group-id (mt/id) :schemas] :block))))))
(testing "via the fn"
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
;; TODO -- this error message is totally garbage, fix this
#"DB permissions with a valid combination of values for :native and :schemas"
;; #"DB permissions with a valid combination of values for :native and :schemas"
(perms/update-data-perms-graph! [group-id (mt/id)]
{:schemas :block, :native :write}))))
(testing "via the API"
(let [current-graph (perms/data-perms-graph)
new-graph (assoc-in current-graph
[:groups group-id (mt/id)]
{:schemas :block, :native :write})]
(is (schema= {:message #".*DB permissions with a valid combination of values for :native and :schemas.*"
s/Keyword s/Any}
(mt/user-http-request :crowberto :put 500 "permissions/graph" new-graph))))))))
(deftest delete-database-delete-block-perms-test
(testing "If a Database gets DELETED, any block permissions for it should get deleted too."
......
src/metabase/api/permission_graph.clj
+ 1
− 1
View file @ e6358cc9
......@@ -65,7 +65,7 @@
:conform-keys true))
;; {:groups {1 {:schemas ::schemas}}}
(s/def ::schemas (s/or :str->kw #{"all" "segmented" "none"}
(s/def ::schemas (s/or :str->kw #{"all" "segmented" "none" "block"}
:nil->none nil?
:identity ::schema-graph))
......
src/metabase/api/permissions.clj
+ 10
− 2
View file @ e6358cc9
(ns metabase.api.permissions
"/api/permissions endpoints."
(:require [compojure.core :refer [DELETE GET POST PUT]]
(:require [clojure.spec.alpha :as spec]
[compojure.core :refer [DELETE GET POST PUT]]
[honeysql.helpers :as hh]
[metabase.api.common :as api]
[metabase.api.permission-graph :as pg]
......@@ -10,6 +11,7 @@
[metabase.models.permissions-group-membership :refer [PermissionsGroupMembership]]
[metabase.server.middleware.offset-paging :as offset-paging]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.schema :as su]
[toucan.db :as db]
[toucan.hydrate :refer [hydrate]]))
......@@ -38,7 +40,13 @@
[:as {body :body}]
{body su/Map}
(api/check-superuser)
(perms/update-data-perms-graph! (pg/converted-json->graph ::pg/data-permissions-graph body))
(let [graph (pg/converted-json->graph ::pg/data-permissions-graph body)]
(when (= graph :clojure.spec.alpha/invalid)
(throw (ex-info (tru "Cannot parse permissions graph because it is invalid: {0}"
(spec/explain-str ::pg/data-permissions-graph body))
{:status-code 400
:error (spec/explain-data ::pg/data-permissions-graph body)})))
(perms/update-data-perms-graph! graph))
(perms/data-perms-graph))
......
src/metabase/models/permissions.clj
+ 7
− 11
View file @ e6358cc9
......@@ -169,8 +169,7 @@
/db/:id/schema/:name/table/:id/query/segmented/ ; allow ad-hoc MBQL queries. Sandbox all queries against this Table.
/block/db/:id/ ; disallow queries against this DB unless User has data perms.
/ ; full root perms"
(:require [clojure.core.match :refer [match]]
[clojure.data :as data]
(:require [clojure.data :as data]
[clojure.string :as str]
[clojure.tools.logging :as log]
[medley.core :as m]
......@@ -573,15 +572,12 @@
;; somewhat useful?
(defn- check-native-and-schemas-permissions-allowed-together [{:keys [native schemas]}]
;; Only do the check when we have both, e.g. when the entire graph is coming in
(if-not (and native schemas)
:ok
(match [native schemas]
[:write :all] :ok
[:write _] (log/warn "Invalid DB permissions: if you have write access for native queries, you must have full data access.")
[:read :none] (log/warn "Invalid DB permissions: if you have readonly native query access, you must also have data access.")
;; TODO -- log a warning if we end up keeping this code.
[_ :block] false
[_ _] :ok)))
(if (and (= native :write)
schemas
(not= schemas :all))
(do (log/warn (trs "Invalid DB permissions: If you have write access for native queries, you must have full data access."))
nil)
:ok))
(def ^:private StrictDBPermissionsGraph
(s/constrained DBPermissionsGraph
......
test/metabase/models/permissions_test.clj
+ 38
− 5
View file @ e6358cc9
......@@ -608,13 +608,46 @@
(deftest root-permissions-graph-test
(testing "A \"/\" permission grants all dataset permissions"
(mt/with-temp Database [{db_id :id}]
(let [{:keys [group_id]} (db/select-one 'Permissions {:object "/"})]
(is (= {db_id {:native :write
(mt/with-temp Database [{db-id :id}]
(let [{:keys [group_id]} (db/select-one Permissions {:object "/"})]
(is (= {db-id {:native :write
:schemas :all}}
(-> (perms/data-perms-graph)
(get-in [:groups group_id])
(select-keys [db_id]))))))))
(select-keys [db-id]))))))))
(deftest update-graph-validate-db-perms-test
(testing "Check that validation of DB `:schemas` and `:native` perms doesn't fail if only one of them changes"
(mt/with-temp Database [{db-id :id}]
(perms/revoke-data-perms! (group/all-users) db-id)
(let [ks [:groups (u/the-id (group/all-users)) db-id]]
(letfn [(perms []
(get-in (perms/data-perms-graph) ks))
(set-perms! [new-perms]
(perms/update-data-perms-graph! (assoc-in (perms/data-perms-graph) ks new-perms))
(perms))]
(testing "Should initially have no perms"
(is (= nil
(perms))))
(testing "grant schema perms"
(is (= {:schemas :all}
(set-perms! {:schemas :all}))))
(testing "grant native perms"
(is (= {:schemas :all, :native :write}
(set-perms! {:schemas :all, :native :write}))))
(testing "revoke native perms"
(is (= {:schemas :all}
(set-perms! {:schemas :all, :native :none}))))
(testing "revoke schema perms"
(is (= nil
(set-perms! {:schemas :none}))))
(testing "disallow schemas :none + :native :write"
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"DB permissions with a valid combination of values for :native and :schemas"
(set-perms! {:schemas :none, :native :write})))
(is (= nil
(perms)))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
......@@ -663,7 +696,7 @@
(deftest grant-revoke-root-collection-permissions-test
(mt/with-temp PermissionsGroup [{group-id :id}]
(letfn [(perms []
(db/select-field :object 'Permissions :group_id group-id))]
(db/select-field :object Permissions :group_id group-id))]
(is (= nil
(perms)))
(testing "Should be able to grant Root Collection perms"
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment