Skip to content
Snippets Groups Projects
Unverified Commit 5741b9fe authored by Noah Moss's avatar Noah Moss Committed by GitHub
Browse files

Make the sandbox cleanup logic more robust, and reorganize some perms code for... 

Make the sandbox cleanup logic more robust, and reorganize some perms code for better testing (#46102)
parent 19118993
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 134 additions and 157 deletions
enterprise/backend/src/metabase_enterprise/advanced_permissions/models/permissions/application_permissions.clj
+ 4
− 4
View file @ 5741b9fe
......@@ -5,8 +5,8 @@
[clojure.data :as data]
[metabase.models :refer [ApplicationPermissionsRevision Permissions]]
[metabase.models.application-permissions-revision :as a-perm-revision]
[metabase.models.data-permissions.graph :as data-perms.graph]
[metabase.models.permissions :as perms]
[metabase.permissions.util :as perms.u]
[metabase.util.honey-sql-2 :as h2x]
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]
......@@ -81,10 +81,10 @@
old-perms (:groups old-graph)
new-perms (:groups new-graph)
[diff-old changes] (data/diff old-perms new-perms)]
(data-perms.graph/log-permissions-changes diff-old changes)
(data-perms.graph/check-revision-numbers old-graph new-graph)
(perms.u/log-permissions-changes diff-old changes)
(perms.u/check-revision-numbers old-graph new-graph)
(when (seq changes)
(t2/with-transaction [_conn]
(doseq [[group-id changes] changes]
(update-application-permissions! group-id changes))
(data-perms.graph/save-perms-revision! ApplicationPermissionsRevision (:revision old-graph) (:groups old-graph) changes)))))
(perms.u/save-perms-revision! ApplicationPermissionsRevision (:revision old-graph) (:groups old-graph) changes)))))
enterprise/backend/src/metabase_enterprise/sandbox/models/permissions/delete_sandboxes.clj
+ 8
− 19
View file @ 5741b9fe
......@@ -35,44 +35,33 @@
(defn- delete-gtaps-for-group-table! [{:keys [group-id table-id] :as _context} changes]
(log/debugf "Deleting unneeded GTAPs for Group %d for Table %d. Graph changes: %s"
group-id table-id (pr-str changes))
(cond
(= changes :unrestricted)
(if (not= changes :sandboxed)
(do
(log/debugf "Group %d now has full data perms for Table %d, deleting GTAP for this Table if one exists"
group-id table-id)
(delete-gtaps-with-condition! group-id [:= :table.id table-id]))
(= changes :sandboxed)
(log/debugf "Group %d now has full sandboxed query perms for Table %d. Do not need to delete GTAPs."
group-id table-id)))
(defn- delete-gtaps-for-group-schema! [{:keys [group-id database-id schema-name], :as context} changes]
(log/debugf "Deleting unneeded GTAPs for Group %d for Database %d, schema %s. Graph changes: %s"
group-id database-id (pr-str schema-name) (pr-str changes))
(cond
(= changes :unrestricted)
(if (keyword? changes)
(do
(log/debugf "Group %d changes has full data perms for Database %d schema %s, deleting all GTAPs for this schema"
group-id database-id (pr-str schema-name))
(log/debugf "Group %d changes has %s perms for Database %d schema %s, deleting all sandboxes for this schema"
group-id changes database-id (pr-str schema-name))
(delete-gtaps-with-condition! group-id [:and [:= :table.db_id database-id] [:= :table.schema schema-name]]))
:else
(doseq [table-id (set (keys changes))]
(delete-gtaps-for-group-table! (assoc context :table-id table-id) (get changes table-id)))))
(defn- delete-gtaps-for-group-database! [{:keys [group-id database-id], :as context} changes]
(log/debugf "Deleting unneeded GTAPs for Group %d for Database %d. Graph changes: %s"
group-id database-id (pr-str changes))
(if (#{:unrestricted :legacy-no-self-service :blocked :impersonated} changes)
(if (keyword? changes)
;; If we're setting a single permission type for the entire DB, clear all sandboxes in the DB
(do
(log/debugf "Group %d %s for Database %d, deleting all GTAPs for this DB"
group-id
(case changes
:unrestricted "now has full data perms"
:legacy-no-self-service "now has full data perms"
:blocked "is now BLOCKED from all non-data-perms access"
:impersonated "is now using connection impersonation")
database-id)
(log/debugf "Group %d now has %s perms for Database %d, deleting all sandboxes for this schema"
group-id changes database-id)
(delete-gtaps-with-condition! group-id [:= :table.db_id database-id]))
(doseq [schema-name (set (keys changes))]
(delete-gtaps-for-group-schema!
......
enterprise/backend/test/metabase_enterprise/advanced_permissions/models/permissions/block_permissions_test.clj
+ 13
− 10
View file @ 5741b9fe
......@@ -48,7 +48,10 @@
(is (nil? (perms group-id)))))))
(defn- grant-block-perms! [group-id]
(data-perms.graph/update-data-perms-graph! [group-id (mt/id) :view-data] :blocked))
(data-perms.graph/update-data-perms-graph!
(-> (data-perms.graph/api-graph)
(assoc-in [:groups group-id (mt/id) :view-data] :blocked)
(assoc-in [:groups group-id (mt/id) :create-queries] :no))))
(defn- api-grant-block-perms! [group-id]
(let [current-graph (data-perms.graph/api-graph)
......@@ -84,13 +87,13 @@
(t2.with-temp/with-temp [PermissionsGroup {group-id :id}]
(testing "Group should have unrestricted view-data perms upon creation"
(is (= :unrestricted
(test-db-perms group-id))))
;; Revoke native perms so that we can set block perms
(data-perms/set-database-permission! group-id (mt/id) :perms/create-queries :query-builder)
(testing "group has no existing permissions"
(mt/with-restored-data-perms-for-group! group-id
(grant! group-id)
(is (nil? (test-db-perms group-id)))))
(test-db-perms group-id)))
; Revoke native perms so that we can set block perms
(data-perms/set-database-permission! group-id (mt/id) :perms/create-queries :query-builder)
(testing "group has no existing permissions"
(mt/with-restored-data-perms-for-group! group-id
(grant! group-id)
(is (nil? (test-db-perms group-id))))))
(testing "group has existing data permissions... :block should remove them"
(mt/with-restored-data-perms-for-group! group-id
(data-perms/set-database-permission! group-id (mt/id) :perms/view-data :unrestricted)
......@@ -138,9 +141,9 @@
new-graph (assoc-in current-graph
[:groups group-id (mt/id)]
{:view-data :blocked :create-queries :query-builder-and-native})]
(is (=? {:message #".*Invalid DB permissions: If you have write access for native queries, you must have data access to all schemas.*"}
(is (=? #"Cannot parse permissions graph because it is invalid.*"
(mt/with-premium-features #{:advanced-permissions}
(mt/user-http-request :crowberto :put 500 "permissions/graph" new-graph)))))))))
(mt/user-http-request :crowberto :put 400 "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
+ 4
− 17
View file @ 5741b9fe
......@@ -103,20 +103,6 @@
(fn [{:keys [native schemas]}]
(not (and (= native :write) schemas (not (#{:all :impersonated} schemas)))))]])
(def ^:private DbGraph
[:schema {:registry {"DataPerms" DataPerms}}
[:map-of
Id
[:map
[:view-data {:optional true} Schemas]
[:create-queries {:optional true} Schemas]
[:data {:optional true} "DataPerms"]
[:download {:optional true} "DataPerms"]
[:data-model {:optional true} "DataPerms"]
;; We use :yes and :no instead of booleans for consistency with the application perms graph, and
;; consistency with the language used on the frontend.
[:details {:optional true} [:enum :yes :no]]]]])
(def StrictDbGraph
"like db-graph, but with added validations:
- Ensures 'view-data' is not 'blocked' if 'create-queries' is 'query-builder-and-native'."
......@@ -138,10 +124,11 @@
(def DataPermissionsGraph
"Used to transform, and verify data permissions graph"
[:map [:groups [:map-of GroupId [:maybe DbGraph]]]])
[:map
[:groups [:map-of GroupId [:maybe StrictDbGraph]]]])
(def StrictData
"Top level strict data graph schema"
(def StrictApiPermissionsGraph
"Top level strict data graph schema expected over the API. Includes revision ID for avoiding concurrent updates."
[:map
[:groups [:map-of GroupId [:maybe StrictDbGraph]]]
[:revision int?]])
......
src/metabase/api/permissions.clj
+ 30
− 19
View file @ 5741b9fe
(ns metabase.api.permissions
"/api/permissions endpoints."
(:require
[clojure.data :as data]
[compojure.core :refer [DELETE GET POST PUT]]
[honey.sql.helpers :as sql.helpers]
[java-time.api :as t]
......@@ -19,6 +20,7 @@
:refer [PermissionsGroup]]
[metabase.models.permissions-revision :as perms-revision]
[metabase.models.setting :as setting :refer [defsetting]]
[metabase.permissions.util :as perms.u]
[metabase.public-settings.premium-features
:as premium-features
:refer [defenterprise]]
......@@ -129,28 +131,37 @@
{body :map
skip-graph [:maybe :boolean]}
(api/check-superuser)
(let [graph (mc/decode api.permission-graph/DataPermissionsGraph
body
(mtx/transformer
mtx/string-transformer
(mtx/transformer {:name :perm-graph})))]
(when-not (mc/validate api.permission-graph/DataPermissionsGraph graph)
(let [explained (mu/explain api.permission-graph/DataPermissionsGraph graph)]
(let [new-graph (mc/decode api.permission-graph/StrictApiPermissionsGraph
body
(mtx/transformer
mtx/string-transformer
(mtx/transformer {:name :perm-graph})))]
(when-not (mc/validate api.permission-graph/DataPermissionsGraph new-graph)
(let [explained (mu/explain api.permission-graph/DataPermissionsGraph new-graph)]
(throw (ex-info (tru "Cannot parse permissions graph because it is invalid: {0}" (pr-str explained))
{:status-code 400}))))
(t2/with-transaction [_conn]
(data-perms.graph/update-data-perms-graph! (dissoc graph :sandboxes :impersonations))
(let [sandbox-updates (:sandboxes graph)
sandboxes (when sandbox-updates
(upsert-sandboxes! sandbox-updates))
impersonation-updates (:impersonations graph)
impersonations (when impersonation-updates
(insert-impersonations! impersonation-updates))
group-ids (-> graph :groups keys)]
(merge {:revision (perms-revision/latest-id)}
(when-not skip-graph {:groups (:groups (data-perms.graph/api-graph {:group-ids group-ids}))})
(when sandboxes {:sandboxes sandboxes})
(when impersonations {:impersonations impersonations}))))))
(let [group-ids (-> new-graph :groups keys)
old-graph (data-perms.graph/api-graph {:group-ids group-ids})
[old new] (data/diff (:groups old-graph)
(:groups new-graph))
old (or old {})
new (or new {})]
(perms.u/log-permissions-changes old new)
(perms.u/check-revision-numbers old-graph new-graph)
(data-perms.graph/update-data-perms-graph! {:groups new})
(perms.u/save-perms-revision! :model/PermissionsRevision (:revision old-graph) old new)
(let [sandbox-updates (:sandboxes new-graph)
sandboxes (when sandbox-updates
(upsert-sandboxes! sandbox-updates))
impersonation-updates (:impersonations new-graph)
impersonations (when impersonation-updates
(insert-impersonations! impersonation-updates))
group-ids (-> new-graph :groups keys)]
(merge {:revision (perms-revision/latest-id)}
(when-not skip-graph {:groups (:groups (data-perms.graph/api-graph {:group-ids group-ids}))})
(when sandboxes {:sandboxes sandboxes})
(when impersonations {:impersonations impersonations})))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | PERMISSIONS GROUP ENDPOINTS |
......
src/metabase/models/collection/graph.clj
+ 3
− 3
View file @ 5741b9fe
......@@ -9,11 +9,11 @@
[metabase.db.query :as mdb.query]
[metabase.models.collection :as collection :refer [Collection]]
[metabase.models.collection-permission-graph-revision :as c-perm-revision]
[metabase.models.data-permissions.graph :as data-perms.graph]
[metabase.models.permissions :as perms :refer [Permissions]]
[metabase.models.permissions-group
:as perms-group
:refer [PermissionsGroup]]
[metabase.permissions.util :as perms.u]
[metabase.public-settings.premium-features :refer [defenterprise]]
[metabase.util :as u]
[metabase.util.honey-sql-2 :as h2x]
......@@ -198,7 +198,7 @@
new-perms (into {} (for [[group-id collection-id->perms] new-perms]
[group-id (select-keys collection-id->perms (keys (get old-perms group-id)))]))
[diff-old changes] (data/diff old-perms new-perms)]
(data-perms.graph/check-revision-numbers old-graph new-graph)
(perms.u/check-revision-numbers old-graph new-graph)
(when (seq changes)
(let [revision-id (t2/with-transaction [_conn]
(doseq [[group-id changes] changes]
......@@ -206,5 +206,5 @@
(update-group-permissions! collection-namespace group-id changes))
(:id (create-perms-revision! (:revision old-graph))))]
;; The graph is updated infrequently, but `diff-old` and `old-graph` can get huge on larger instances.
(data-perms.graph/log-permissions-changes diff-old changes)
(perms.u/log-permissions-changes diff-old changes)
(fill-revision-details! revision-id (assoc old-graph :namespace collection-namespace) changes))))))
src/metabase/models/data_permissions/graph.clj
+ 13
− 58
View file @ 5741b9fe
......@@ -7,10 +7,8 @@
Essentially, this is a translation layer between the graph used by the v1 permissions schema and the v2 permissions
schema."
(:require
[clojure.data :as data]
[clojure.string :as str]
[medley.core :as m]
[metabase.api.common :as api]
[metabase.api.permission-graph :as api.permission-graph]
[metabase.audit :as audit]
[metabase.models.data-permissions :as data-perms]
......@@ -21,7 +19,6 @@
:refer [defenterprise]]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
[toucan2.core :as t2]))
......@@ -109,7 +106,7 @@
(defn- rename-perm
"Transforms a 'leaf' value with db-level or table-level perms in the data permissions graph into an API-style data permissions value.
There's some tricks in here that ellide table-level and table-level permissions values that are the most-permissive setting."
There's some tricks in here that ellide schema-level and table-level permissions values that are the most-permissive setting."
[perm-map]
(let [granular-keys [:perms/download-results :perms/manage-table-metadata
:perms/view-data :perms/create-queries]]
......@@ -162,7 +159,7 @@
(into {}))
m))
(mu/defn api-graph :- api.permission-graph/StrictData
(mu/defn api-graph :- api.permission-graph/DataPermissionsGraph
"Converts the backend representation of the data permissions graph to the representation we send over the API. Mainly
renames permission types and values from the names stored in the database to the ones expected by the frontend.
- Converts DB key names to API key names
......@@ -364,8 +361,8 @@
(defn check-audit-db-permissions
"Check that the changes coming in does not attempt to change audit database permission. Admins should
change these permissions implicitly via collection permissions."
[changes]
(let [changes-ids (->> changes
[group-updates]
(let [changes-ids (->> group-updates
vals
(map keys)
(apply concat))]
......@@ -374,41 +371,6 @@
(str "Audit database permissions can only be changed by updating audit collection permissions."))
{:status-code 400})))))
(defn log-permissions-changes
"Log changes to the permissions graph."
[old new]
(log/debug "Changing permissions"
"\n FROM:" (u/pprint-to-str :magenta old)
"\n TO:" (u/pprint-to-str :blue new)))
(defn check-revision-numbers
"Check that the revision number coming in as part of `new-graph` matches the one from `old-graph`. This way we can
make sure people don't submit a new graph based on something out of date, which would otherwise stomp over changes
made in the interim. Return a 409 (Conflict) if the numbers don't match up."
[old-graph new-graph]
(when (not= (:revision old-graph) (:revision new-graph))
(throw (ex-info (tru
(str "Looks like someone else edited the permissions and your data is out of date. "
"Please fetch new data and try again."))
{:status-code 409}))))
(defn save-perms-revision!
"Save changes made to permission graph for logging/auditing purposes.
This doesn't do anything if `*current-user-id*` is unset (e.g. for testing or REPL usage).
* `model` -- revision model, should be one of
[PermissionsRevision, CollectionPermissionGraphRevision, ApplicationPermissionsRevision]
* `before` -- the graph before the changes
* `changes` -- set of changes applied in this revision."
[model current-revision before changes]
(when api/*current-user-id*
(first (t2/insert-returning-instances! model
;; manually specify ID here so if one was somehow inserted in the meantime in the fraction of a second since we
;; called `check-revision-numbers` the PK constraint will fail and the transaction will abort
:id (inc current-revision)
:before before
:after changes
:user_id api/*current-user-id*))))
(mu/defn update-data-perms-graph!*
"Takes an API-style perms graph and sets the permissions in the database accordingly."
([graph]
......@@ -427,23 +389,16 @@
(update-data-perms-graph!* (assoc-in (-> api-graph :groups) ks new-value))))
(mu/defn update-data-perms-graph!
"Takes an API-style perms graph and sets the permissions in the database accordingly. Additionally validates the revision number,
logs the changes, and ensures impersonations and sandboxes are consistent."
([new-graph :- api.permission-graph/StrictData]
(let [group-ids (-> new-graph :groups keys)
old-graph (api-graph {:group-ids group-ids})
[old new] (data/diff (:groups old-graph) (:groups new-graph))
old (or old {})
new (or new {})]
(when (or (seq old) (seq new))
(log-permissions-changes old new)
(check-revision-numbers old-graph new-graph)
(check-audit-db-permissions new)
"Takes an API-style perms graph and sets the permissions in the database accordingly. Additionally ensures
impersonations and sandboxes are consistent if necessary."
([graph-updates :- api.permission-graph/DataPermissionsGraph]
(when (seq graph-updates)
(let [group-updates (:groups graph-updates)]
(check-audit-db-permissions group-updates)
(t2/with-transaction [_conn]
(update-data-perms-graph!* new)
(save-perms-revision! :model/PermissionsRevision (:revision old-graph) old new)
(delete-impersonations-if-needed-after-permissions-change! new)
(delete-gtaps-if-needed-after-permissions-change! new)))))
(update-data-perms-graph!* group-updates)
(delete-impersonations-if-needed-after-permissions-change! group-updates)
(delete-gtaps-if-needed-after-permissions-change! group-updates)))))
;; The following arity is provided soley for convenience for tests/REPL usage
([ks :- [:vector :any] new-value]
......
src/metabase/permissions/util.clj
+ 46
− 1
View file @ 5741b9fe
......@@ -4,8 +4,53 @@
(:require
[clojure.string :as str]
[malli.core :as mc]
[metabase.api.common :as api]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
[metabase.util.regex :as u.regex]))
[metabase.util.regex :as u.regex]
[toucan2.core :as t2]))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | API-level helpers |
;;; +----------------------------------------------------------------------------------------------------------------+
(defn log-permissions-changes
"Log changes to the permissions graph."
[old new]
(log/debug "Changing permissions"
"\n FROM:" (u/pprint-to-str :magenta old)
"\n TO:" (u/pprint-to-str :blue new)))
(defn check-revision-numbers
"Check that the revision number coming in as part of `new-graph` matches the one from `old-graph`. This way we can
make sure people don't submit a new graph based on something out of date, which would otherwise stomp over changes
made in the interim. Return a 409 (Conflict) if the numbers don't match up."
[old-graph new-graph]
(when (not= (:revision old-graph) (:revision new-graph))
(throw (ex-info (tru
(str "Looks like someone else edited the permissions and your data is out of date. "
"Please fetch new data and try again."))
{:status-code 409}))))
(defn save-perms-revision!
"Save changes made to permission graph for logging/auditing purposes.
This doesn't do anything if `*current-user-id*` is unset (e.g. for testing or REPL usage).
* `model` -- revision model, should be one of
[PermissionsRevision, CollectionPermissionGraphRevision, ApplicationPermissionsRevision]
* `before` -- the graph before the changes
* `changes` -- set of changes applied in this revision."
[model current-revision before changes]
(when api/*current-user-id*
(first (t2/insert-returning-instances! model
;; manually specify ID here so if one was somehow inserted in the meantime in the fraction of a second since we
;; called `check-revision-numbers` the PK constraint will fail and the transaction will abort
:id (inc current-revision)
:before before
:after changes
:user_id api/*current-user-id*))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | PATH CLASSIFICATION + VALIDATION |
......
test/metabase/api/permissions_test_util.clj
+ 1
− 1
View file @ 5741b9fe
......@@ -4,7 +4,7 @@
[metabase.api.permission-graph :as api.permission-graph]))
(def ^:private graph-output-schema
[:map-of @#'api.permission-graph/GroupId @#'api.permission-graph/DbGraph])
[:map-of @#'api.permission-graph/GroupId @#'api.permission-graph/StrictDbGraph])
(defn- decode-and-validate [schema value]
(mc/validate schema (mc/decode schema value (mtx/string-transformer))))
......
test/metabase/models/data_permissions/graph_test.clj
+ 12
− 25
View file @ 5741b9fe
(ns metabase.models.data-permissions.graph-test
(:require
[clojure.test :refer :all]
[clojure.walk :as walk]
[metabase.audit :as audit]
[metabase.models.data-permissions :as data-perms]
[metabase.models.data-permissions.graph :as data-perms.graph]
......@@ -11,7 +10,7 @@
[toucan2.core :as db]))
(deftest update-db-level-view-data-permissions!-test
(mt/with-premium-features #{:advanced-permissions}
(mt/with-premium-features #{:advanced-permissions :sandboxes}
(mt/with-temp [:model/PermissionsGroup {group-id-1 :id} {}
:model/Database {database-id-1 :id} {}
:model/Table {table-id-1 :id} {:db_id database-id-1
......@@ -21,7 +20,7 @@
(testing "data permissions can be updated via API-style graph"
(are [api-graph db-graph] (= db-graph
(do
(data-perms.graph/update-data-perms-graph!* api-graph)
(data-perms.graph/update-data-perms-graph! {:groups api-graph})
(data-perms/data-permissions-graph :group-id group-id-1)))
{group-id-1
{database-id-1
......@@ -55,7 +54,7 @@
:perms/download-results :no}}})))))
(deftest update-db-level-create-queries-permissions!-test
(mt/with-premium-features #{:advanced-permissions}
(mt/with-premium-features #{:advanced-permissions :sandboxes}
(mt/with-temp [:model/PermissionsGroup {group-id-1 :id} {}
:model/Database {database-id-1 :id} {}
:model/Table {table-id-1 :id} {:db_id database-id-1
......@@ -65,7 +64,7 @@
(do
;; Clear default perms for the group
(db/delete! :model/DataPermissions :group_id group-id-1)
(data-perms.graph/update-data-perms-graph!* api-graph)
(data-perms.graph/update-data-perms-graph! {:groups api-graph})
(data-perms/data-permissions-graph :group-id group-id-1)))
{group-id-1
{database-id-1
......@@ -121,7 +120,7 @@
{:perms/create-queries {"PUBLIC" {table-id-1 :no}}}}})))))
(deftest update-db-level-data-access-permissions!-test
(mt/with-premium-features #{:advanced-permissions}
(mt/with-premium-features #{:advanced-permissions :sandboxes}
(mt/with-temp [:model/PermissionsGroup {group-id-1 :id} {}
:model/Database {database-id-1 :id} {}
:model/Table {table-id-1 :id} {:db_id database-id-1
......@@ -135,7 +134,7 @@
(testing "data-access permissions can be updated via API-style graph"
(are [api-graph db-graph] (= db-graph
(do
(data-perms.graph/update-data-perms-graph!* api-graph)
(data-perms.graph/update-data-perms-graph! {:groups api-graph})
(data-perms/data-permissions-graph :group-id group-id-1)))
;; Setting granular data access permissions
{group-id-1
......@@ -204,7 +203,7 @@
(testing "download permissions can be updated via API-style graph"
(are [api-graph db-graph] (= db-graph
(do
(data-perms.graph/update-data-perms-graph!* api-graph)
(data-perms.graph/update-data-perms-graph! {:groups api-graph})
(data-perms/data-permissions-graph :group-id group-id-1)))
;; Setting granular download permissions
{group-id-1
......@@ -269,7 +268,7 @@
(testing "data model editing permissions can be updated via API-style graph"
(are [api-graph db-graph] (= db-graph
(do
(data-perms.graph/update-data-perms-graph!* api-graph)
(data-perms.graph/update-data-perms-graph! {:groups api-graph})
(data-perms/data-permissions-graph :group-id group-id-1)))
;; Setting granular data model editing permissions
{group-id-1
......@@ -328,7 +327,7 @@
(testing "database details editing permissions can be updated via API-style graph"
(are [api-graph db-graph] (= db-graph
(do
(data-perms.graph/update-data-perms-graph!* api-graph)
(data-perms.graph/update-data-perms-graph! {:groups api-graph})
(data-perms/data-permissions-graph :group-id group-id-1)))
;; Granting permission to edit database details
{group-id-1
......@@ -353,11 +352,6 @@
(is (not (#'data-perms.graph/ellide? :perms/view-data :unrestricted)))
(is (#'data-perms.graph/ellide? :perms/view-data :blocked)))
(defn replace-empty-map-with-nil [graph]
(walk/postwalk
(fn [x] (if (= x {}) nil x))
graph))
(deftest perms-are-renamed-test
(testing "Perm keys and values are correctly renamed, and permissions are ellided as necessary"
(are [db-graph api-graph] (= api-graph (-> db-graph
......@@ -414,13 +408,6 @@
2 :legacy-no-self-service}
"OTHER" :legacy-no-self-service}})))
(defn constrain-graph
"Filters out all non `group-id`X`db-id` permissions"
[group-id db-id graph]
(-> graph
(assoc :groups {group-id (get-in graph [:groups group-id])})
(assoc-in [:groups group-id] {db-id (get-in graph [:groups group-id db-id])})))
(defn- test-query-graph [group]
(get-in (data-perms.graph/api-graph) [:groups (u/the-id group) (mt/id) :create-queries "PUBLIC"]))
......@@ -450,14 +437,14 @@
(deftest update-graph-validate-db-perms-test
(testing "Check that validation of native query perms doesn't fail if only one of them changes"
(mt/with-additional-premium-features #{:advanced-permissions}
(mt/with-additional-premium-features #{:advanced-permissions :sandboxes}
(mt/with-temp [:model/Database {db-id :id}]
(mt/with-no-data-perms-for-all-users!
(let [ks [:groups (u/the-id (perms-group/all-users)) db-id]]
(letfn [(perms []
(get-in (data-perms.graph/api-graph) ks))
(set-perms! [new-perms]
(data-perms.graph/update-data-perms-graph! (assoc-in (data-perms.graph/api-graph) ks new-perms))
(data-perms.graph/update-data-perms-graph! (assoc-in {} ks new-perms))
(perms))]
(testing "Should initially have no perms"
(is (= nil
......@@ -489,7 +476,7 @@
(deftest no-op-partial-graph-updates
(testing "Partial permission graphs with no changes to the existing graph do not error when run repeatedly (#25221)"
(mt/with-additional-premium-features #{:advanced-permissions}
(mt/with-additional-premium-features #{:advanced-permissions :sandboxes}
(mt/with-temp [:model/PermissionsGroup group]
;; Bind *current-user* so that permission revisions are written, which was the source of the original error
(mt/with-current-user (mt/user->id :rasta)
......
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