diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index b30d433f3b2432a3349dc4c7244b72a7358bc392..e39c160e2197a79ecfb7457bb18d0377818306d7 100644
--- a/src/metabase/models/permissions.clj
+++ b/src/metabase/models/permissions.clj
@@ -311,7 +311,7 @@
(db/cascade-delete! Permissions where))))
(defn revoke-permissions!
- "Revoke permissions for GROUP-OR-ID to object with PATH-COMPONENTS."
+ "Revoke all permissions for GROUP-OR-ID to object with PATH-COMPONENTS, *including* related permissions."
[group-or-id & path-components]
(delete-related-permissions! group-or-id (apply object-path path-components)))
@@ -345,7 +345,7 @@
(grant-permissions! group-or-id (native-readwrite-path database-id)))
(defn revoke-db-schema-permissions!
- "Remove all permissions entires for a DB and any child objects.
+ "Remove all permissions entires for a DB and *any* child objects.
This does *not* revoke native permissions; use `revoke-native-permssions!` to do that."
[group-or-id database-id]
;; TODO - if permissions for this DB are DB root entries like `/db/1/` won't this end up removing our native perms?
@@ -375,10 +375,10 @@
:none (revoke-permissions! group-id db-id schema table-id)))
(s/defn ^:private ^:always-validate update-schema-perms! [group-id :- s/Int, db-id :- s/Int, schema :- s/Str, new-schema-perms :- SchemaPermissionsGraph]
- (revoke-permissions! group-id db-id schema)
(cond
- (= new-schema-perms :all) (grant-permissions! group-id db-id schema)
- (= new-schema-perms :none) nil
+ (= new-schema-perms :all) (do (revoke-permissions! group-id db-id schema) ; clear out any existing related permissions
+ (grant-permissions! group-id db-id schema)) ; then grant full perms for the schema
+ (= new-schema-perms :none) (revoke-permissions! group-id db-id schema)
(map? new-schema-perms) (doseq [[table-id table-perms] new-schema-perms]
(update-table-perms! group-id db-id schema table-id table-perms))))
@@ -400,10 +400,10 @@
(when-let [new-native-perms (:native new-db-perms)]
(update-native-permissions! group-id db-id new-native-perms))
(when-let [schemas (:schemas new-db-perms)]
- (revoke-db-schema-permissions! group-id db-id)
(cond
- (= schemas :all) (grant-permissions-for-all-schemas! group-id db-id)
- (= schemas :none) nil
+ (= schemas :all) (do (revoke-db-schema-permissions! group-id db-id)
+ (grant-permissions-for-all-schemas! group-id db-id))
+ (= schemas :none) (revoke-db-schema-permissions! group-id db-id)
(map? schemas) (doseq [schema (keys schemas)]
(update-schema-perms! group-id db-id schema (get-in new-db-perms [:schemas schema]))))))
@@ -444,13 +444,13 @@
[old new] (data/diff (:groups old-graph) (:groups new-graph))]
(when (or (seq old) (seq new))
(log/debug (format "Changing permissions: ðŸ”\nFROM:\n%s\nTO:\n%s"
- (u/pprint-to-str 'magenta old)
- (u/pprint-to-str 'blue new)))
+ (u/pprint-to-str 'magenta old)
+ (u/pprint-to-str 'blue new)))
(check-revision-numbers old-graph new-graph)
(db/transaction
- (doseq [group-id (keys new)]
- (update-group-permissions! group-id (get new group-id)))
- (save-perms-revision! (:revision old-graph) old new)))))
+ (doseq [group-id (keys new)]
+ (update-group-permissions! group-id (get new group-id)))
+ (save-perms-revision! (:revision old-graph) old new)))))
;; The following arity is provided soley for convenience for tests/REPL usage
([ks new-value]
{:pre [(sequential? ks)]}
diff --git a/src/metabase/query_processor/parameters.clj b/src/metabase/query_processor/parameters.clj
index 396b70f779ef7e912cf68cc5a39abbb71fe51fab..af2793c722d002bb37962d3773ac7966516f9e64 100644
--- a/src/metabase/query_processor/parameters.clj
+++ b/src/metabase/query_processor/parameters.clj
@@ -46,7 +46,7 @@
:start (t/first-day-of-the-month dt)})
(defn- year-range [^DateTime dt]
- {:end (t/last-day-of-the-month (.withMonthOfYear dt DateTimeConstants/DECEMBER))
+ {:end (t/last-day-of-the-month (.withMonthOfYear dt DateTimeConstants/DECEMBER))
:start (t/first-day-of-the-month (.withMonthOfYear dt DateTimeConstants/JANUARY))})
(defn- absolute-date->range
diff --git a/test/metabase/models/permissions_test.clj b/test/metabase/models/permissions_test.clj
index c6ad55f07388e9c3d009d1fefbaf1efb2bf97a3b..fb20af01d0a7f7fd4c16e687962223ddae6f369e 100644
--- a/test/metabase/models/permissions_test.clj
+++ b/test/metabase/models/permissions_test.clj
@@ -1,6 +1,10 @@
(ns metabase.models.permissions-test
(:require [expectations :refer :all]
- [metabase.models.permissions :as perms]))
+ (metabase.models [permissions :as perms]
+ [permissions-group :refer [PermissionsGroup]])
+ [metabase.test.data :as data]
+ [metabase.test.util :as tu]
+ [metabase.util :as u]))
;;; ------------------------------------------------------------ valid-object-path? ------------------------------------------------------------
@@ -502,3 +506,19 @@
;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
;;; | TODO - Permissions Graph Tests |
;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+
+(defn- test-data-graph [group]
+ (get-in (perms/graph) [:groups (u/get-id group) (data/id) :schemas "PUBLIC"]))
+
+;; Test that setting partial permissions for a table retains permissions for other tables -- #3888
+(tu/expect-with-temp [PermissionsGroup [group]]
+ [{(data/id :categories) :none, (data/id :checkins) :none, (data/id :users) :none, (data/id :venues) :all}
+ {(data/id :categories) :all, (data/id :checkins) :none, (data/id :users) :none, (data/id :venues) :all}]
+ (do
+ ;; first, graph permissions only for VENUES
+ (perms/grant-permissions! group (perms/object-path (data/id) "PUBLIC" (data/id :venues)))
+ [(test-data-graph group)
+ ;; next, grant permissions via `update-graph!` for CATEGORIES as well. Make sure permissions for VENUES are retained (#3888)
+ (do
+ (perms/update-graph! [(u/get-id group) (data/id) :schemas "PUBLIC" (data/id :categories)] :all)
+ (test-data-graph group))]))