Merge pull request #3891 from metabase/fix-3888

Fix adding partial permissions for additional tables

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)]}

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

test/metabase/models/permissions_test.clj

 (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))]))