Merge pull request #3460 from metabase/legacy-card-perms-fix

Fix permissions barf on legacy cards 

src/metabase/models/card.clj

@@ -41,11 +41,16 @@
 
 (defn- permissions-path-set:mbql [{database-id :database, :as query}]
   {:pre [(integer? database-id) (map? (:query query))]}
-  (let [{{:keys [source-table join-tables]} :query} (qp/expand query)]
-    (set (for [table (cons source-table join-tables)]
-           (perms/object-path database-id
-                              (:schema table)
-                              (or (:id table) (:table-id table)))))))
+  (try (let [{{:keys [source-table join-tables]} :query} (qp/expand query)]
+         (set (for [table (cons source-table join-tables)]
+                (perms/object-path database-id
+                                   (:schema table)
+                                   (or (:id table) (:table-id table))))))
+       ;; if for some reason we can't expand the Card (i.e. it's an invalid legacy card)
+       ;; just return a set of permissions that means no one will ever get to see it
+       (catch Throwable e
+         (log/warn "Error getting permissions for card:" (.getMessage e) "\n" (u/pprint-to-str (u/filtered-stacktrace e)))
+         #{"/db/0/"}))) ; DB 0 will never exist
 
 (defn- permissions-path-set:native [read-or-write {database-id :database}]
   #{((case read-or-write

test/metabase/models/card_test.clj

@@ -6,6 +6,7 @@
                              [dashboard-card :refer [DashboardCard]]
                              [interface :as models]
                              [permissions :as perms])
+            [metabase.query-processor.expand :as ql]
             [metabase.test.data :refer [id]]
             [metabase.test.data.users :refer :all]
             [metabase.test.util :refer [random-name with-temp]]))
@@ -75,3 +76,48 @@
   (with-temp Card [card {:dataset_query {:database (id), :type "native"}}]
     (binding [*current-user-permissions-set* (delay #{(perms/native-readwrite-path (id))})]
       (models/can-write? card))))
+
+
+;;; check permissions sets for queries
+;; native read
+(defn- native [query]
+  {:database 1
+   :type     :native
+   :native   {:query query}})
+
+(expect
+  #{"/db/1/native/read/"}
+  (query-perms-set (native "SELECT count(*) FROM toucan_sightings;") :read))
+
+;; native write
+(expect
+  #{"/db/1/native/"}
+  (query-perms-set (native "SELECT count(*) FROM toucan_sightings;") :write))
+
+
+(defn- mbql [query]
+  {:database (id)
+   :type     :query
+   :query    query})
+
+;; MBQL w/o JOIN
+(expect
+  #{(perms/object-path (id) "PUBLIC" (id :venues))}
+  (query-perms-set (mbql (ql/query
+                           (ql/source-table (id :venues))))
+                   :read))
+
+;; MBQL w/ JOIN
+(expect
+  #{(perms/object-path (id) "PUBLIC" (id :checkins))
+    (perms/object-path (id) "PUBLIC" (id :venues))}
+  (query-perms-set (mbql (ql/query
+                           (ql/source-table (id :checkins))
+                           (ql/order-by (ql/asc (ql/fk-> (id :checkins :venue_id) (id :venues :name))))))
+                   :read))
+
+;; invalid/legacy card should return perms for something that doesn't exist so no one gets to see it
+(expect
+  #{"/db/0/"}
+  (query-perms-set (mbql {:filter [:WOW 100 200]})
+                   :read))