Merge pull request #4276 from metabase/test-for-4000

Fix permissions graph behavior with empty schemas

src/metabase/models/permissions.clj

@@ -208,7 +208,7 @@
 (def ^:private DBPermissionsGraph
   {(s/optional-key :native)  NativePermissionsGraph
    (s/optional-key :schemas) (s/cond-pre (s/enum :all :none)
-                                         {(s/maybe s/Str) SchemaPermissionsGraph})})
+                                         {s/Str SchemaPermissionsGraph})})
 
 (def ^:private GroupPermissionsGraph
   {su/IntGreaterThanZero DBPermissionsGraph})
@@ -268,7 +268,7 @@
     :all  :all
     :none :none
     :some (into {} (for [table tables]
-                     {(:id table) (permissions-for-path permissions-set (table->table-object-path table))}))))
+                     {(u/get-id table) (permissions-for-path permissions-set (table->table-object-path table))}))))
 
 (s/defn ^:private db-graph :- DBPermissionsGraph [permissions-set tables]
   {:native  (case (permissions-for-path permissions-set (table->native-readwrite-path (first tables)))
@@ -278,8 +278,9 @@
    :schemas (case (permissions-for-path permissions-set (table->all-schemas-path (first tables)))
               :all  :all
               :none :none
-              (m/map-vals (partial schema-graph permissions-set)
-                          (group-by :schema tables)))})
+              (into {} (for [[schema tables] (group-by :schema tables)]
+                         ;; if schema is nil, replace it with an empty string, since that's how it will get encoded in JSON :D
+                         {(str schema) (schema-graph permissions-set tables)})))})
 
 (s/defn ^:private group-graph :- GroupPermissionsGraph [permissions-set tables]
   (m/map-vals (partial db-graph permissions-set)

test/metabase/models/permissions_test.clj

 (ns metabase.models.permissions-test
   (:require [expectations :refer :all]
+            [toucan.db :as db]
             [toucan.util.test :as tt]
-            (metabase.models [permissions :as perms]
-                             [permissions-group :refer [PermissionsGroup]])
+            (metabase.models [database :refer [Database]]
+                             [permissions :as perms]
+                             [permissions-group :refer [PermissionsGroup]]
+                             [permissions-group-membership :refer [PermissionsGroupMembership]]
+                             [table :refer [Table]])
             [metabase.test.data :as data]
             [metabase.test.util :as tu]
             [metabase.util :as u]))
@@ -505,7 +509,7 @@
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
-;;; |                                                                 TODO - Permissions Graph Tests                                                                 |
+;;; |                                                                    Permissions Graph Tests                                                                     |
 ;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
 
 (defn- test-data-graph [group]
@@ -523,3 +527,16 @@
      (do
        (perms/update-graph! [(u/get-id group) (data/id) :schemas "PUBLIC" (data/id :categories)] :all)
        (test-data-graph group))]))
+
+;;; Make sure that the graph functions work correctly for DBs with no schemas
+;; See https://github.com/metabase/metabase/issues/4000
+(tt/expect-with-temp [PermissionsGroup [group]
+                      Database         [database]
+                      Table            [table    {:db_id (u/get-id database)}]]
+  {"" {(u/get-id table) :all}}
+  (do
+    ;; try to grant idential permissions to the table twice
+    (perms/update-graph! [(u/get-id group) (u/get-id database) :schemas] {"" {(u/get-id table) :all}})
+    (perms/update-graph! [(u/get-id group) (u/get-id database) :schemas] {"" {(u/get-id table) :all}})
+    ;; now fetch the perms that have been granted
+    (get-in (perms/graph) [:groups (u/get-id group) (u/get-id database) :schemas])))