Allow creating & editing dashboard subscriptions if you have view-only access...

Allow creating & editing dashboard subscriptions if you have view-only access to a collection (#17722)

src/metabase/api/pulse.clj

@@ -62,11 +62,13 @@
    parameters          [su/Map]}
   ;; make sure we are allowed to *read* all the Cards we want to put in this Pulse
   (check-card-read-permissions cards)
-  ;; if we're trying to create this Pulse inside a Collection, make sure we have write permissions for that collection
-  (collection/check-write-perms-for-collection collection_id)
-  ;; prohibit sending dashboard subs for unauthorized dashboards
+  ;; if we're trying to create this Pulse inside a Collection, and it is not a dashboard subscription,
+  ;; make sure we have write permissions for that collection
+  (when-not dashboard_id
+    (collection/check-write-perms-for-collection collection_id))
+  ;; prohibit creating dashboard subs if the the user doesn't have at least read access for the dashboard
   (when dashboard_id
-    (api/write-check Dashboard dashboard_id))
+    (api/read-check Dashboard dashboard_id))
   (let [pulse-data {:name                name
                     :creator_id          api/*current-user-id*
                     :skip_if_empty       skip_if_empty

src/metabase/models/pulse.clj

@@ -17,6 +17,7 @@
   (:require [clojure.string :as str]
             [clojure.tools.logging :as log]
             [medley.core :as m]
+            [metabase.api.common :as api]
             [metabase.events :as events]
             [metabase.models.card :refer [Card]]
             [metabase.models.collection :as collection]
@@ -94,8 +95,13 @@
   [notification]
   (boolean (:alert_condition notification)))
 
+(defn- is-dashboard-subscription?
+  "Whether `notification` is a Dashboard Subscription (as opposed to a regular Pulse or an Alert)."
+  [notification]
+  (boolean (:dashboard_id notification)))
+
 (defn- perms-objects-set
-  "Permissions to read or write a *Pulse* are the same as those of its parent Collection.
+  "Permissions to read or write a *Pulse* or *Dashboard Subscription* are the same as those of its parent Collection.
 
   Permissions to read or write an *Alert* are the same as those of its 'parent' *Card*. For all intents and purposes,
   an Alert cannot be put into a Collection."
@@ -104,6 +110,16 @@
     (i/perms-objects-set (alert->card notification) read-or-write)
     (perms/perms-objects-set-for-parent-collection notification read-or-write)))
 
+(defn- can-write?
+  "A user with read-only permissions for a dashboard should be able to create subscriptions, and update
+  subscriptions that they created, but not edit anyone else's subscriptions."
+  [notification]
+  (if (and (is-dashboard-subscription? notification)
+           (i/current-user-has-full-permissions? :read notification)
+           (not (i/current-user-has-full-permissions? :write notification)))
+    (= api/*current-user-id* (:creator_id notification))
+    (i/current-user-has-full-permissions? :write notification)))
+
 (u/strict-extend (class Pulse)
   models/IModel
   (merge
@@ -117,7 +133,7 @@
   (merge
    i/IObjectPermissionsDefaults
    {:can-read?         (partial i/current-user-has-full-permissions? :read)
-    :can-write?        (partial i/current-user-has-full-permissions? :write)
+    :can-write?        can-write?
     :perms-objects-set perms-objects-set}))
 
 

test/metabase/models/pulse_test.clj

@@ -275,7 +275,7 @@
              (count (:cards (pulse/retrieve-pulse (u/the-id pulse)))))))))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
-;;; |                                         Collections Permissions Tests                                          |
+;;; |                                   Pulse Collections Permissions Tests                                          |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
 (defn do-with-pulse-in-collection [f]
@@ -291,7 +291,7 @@
       (f db collection pulse card))))
 
 (defmacro with-pulse-in-collection
-  "Execute `body` with a temporary Pulse, in a Colleciton, containing a single Card."
+  "Execute `body` with a temporary Pulse, in a Collection, containing a single Card."
   {:style/indent 1}
   [[db-binding collection-binding pulse-binding card-binding] & body]
   `(do-with-pulse-in-collection
@@ -331,3 +331,48 @@
              clojure.lang.ExceptionInfo
              #"A Pulse can only go in Collections in the \"default\" namespace"
              (db/update! Pulse card-id {:collection_id collection-id})))))))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                         Dashboard Subscription Collections Permissions Tests                                   |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- do-with-dashboard-subscription-in-collection [f]
+  (mt/with-non-admin-groups-no-root-collection-perms
+    (mt/with-temp* [Collection [collection]
+                    Dashboard  [dashboard {:collection_id (u/the-id collection)}]
+                    Pulse      [pulse     {:collection_id (u/the-id collection)
+                                           :dashboard_id  (u/the-id dashboard)
+                                           :creator_id    (mt/user->id :rasta)}]
+                    Database   [db        {:engine :h2}]]
+      (f db collection dashboard pulse))))
+
+(defmacro with-dashboard-subscription-in-collection
+  "Execute `body` with a temporary Dashboard Subscription for a Dashboard in a Collection"
+  {:style/indent 1}
+  [[db-binding collection-binding dashboard-binding subscription-binding] & body]
+  `(do-with-dashboard-subscription-in-collection
+    (fn [~(or db-binding '_) ~(or collection-binding '_) ~(or dashboard-binding '_) ~(or subscription-binding '_)]
+      ~@body)))
+
+(deftest dashboard-subscription-permissions-test
+  (with-dashboard-subscription-in-collection [_ collection _ subscription]
+    (testing "If we have read and write access to a collection, we have read and write access to
+             a dashboard subscription"
+      (binding [api/*current-user-permissions-set* (atom #{(perms/collection-readwrite-path collection)})]
+        (is (mi/can-read? subscription))
+        (is (mi/can-write? subscription))))
+
+    (testing "If we have read-only access to a collection, we can create dashboard subscriptions, or
+             modify subscriptions that we have created"
+      (binding [api/*current-user-permissions-set* (atom #{(perms/collection-read-path collection)})
+                api/*current-user-id*              (:creator_id subscription)]
+        (is (mi/can-read? subscription))
+        (is (mi/can-write? subscription))
+        (is (not (mi/can-write? (assoc subscription :creator_id (mt/user->id :lucky)))))))
+
+    (testing "If we have no access to a collection, we cannot read or write dashboard subscriptions,
+             even if we created them"
+      (binding [api/*current-user-id* (:creator_id subscription)]
+        (is (not (mi/can-read? subscription)))
+        (is (not (mi/can-write? subscription)))))))