Merge pull request #9430 from metabase/only-admins-should-see-user-signups

Only show user signups in activity feed to admins

src/metabase/api/activity.clj

@@ -13,8 +13,8 @@
              [hydrate :refer [hydrate]]]))
 
 (defn- dashcard-activity? [activity]
-  (contains? #{:dashboard-add-cards :dashboard-remove-cards}
-             (:topic activity)))
+  (#{:dashboard-add-cards :dashboard-remove-cards}
+   (:topic activity)))
 
 (defn- activities->referenced-objects
   "Get a map of model name to a set of referenced IDs in these ACTIVITIES.

src/metabase/events/activity_feed.clj

@@ -15,7 +15,7 @@
             [toucan.db :as db]))
 
 (def ^:private activity-feed-topics
-  "The `Set` of event topics which are subscribed to for use in the Metabase activity feed."
+  "The set of event topics which are subscribed to for use in the Metabase activity feed."
   #{:alert-create
     :alert-delete
     :card-create
@@ -34,7 +34,7 @@
     :segment-create
     :segment-update
     :segment-delete
-    :user-login})
+    :user-login}) ; this is only used these days the first time someone logs in to record 'user-joined' events
 
 (def ^:private activity-feed-channel
   "Channel for receiving event notifications we want to subscribe to for the activity feed."

src/metabase/models/activity.clj

@@ -2,6 +2,7 @@
   (:require [metabase
              [events :as events]
              [util :as u]]
+            [metabase.api.common :as api]
             [metabase.models
              [card :refer [Card]]
              [dashboard :refer [Dashboard]]
@@ -23,10 +24,26 @@
    "pulse"     Pulse
    "segment"   Segment})
 
-(defn- can-? [f {model :model, model-id :model_id, :as activity}]
+(defmulti can-?
+  "Implementation for `can-read?`/`can-write?` for items in the activity feed. Dispatches off of the activity `:topic`,
+  e.g. `:user-joined`. `perms-check-fn` is `can-read?` or `can-write?` and should be called as needed on models the
+  activity records."
+  {:arglists '([perms-check-fn activity])}
+  (fn [_ {:keys [topic]}]
+    topic))
+
+;; For now only admins can see when another user joined -- we don't want every user knowing about every other user. In
+;; the future we might want to change this and come up with some sort of system where we can determine which users get
+;; to see other users -- perhaps if they are in a group together other than 'All Users'
+(defmethod can-? :user-joined [_ _]
+  api/*is-superuser?*)
+
+;; For every other activity topic we'll look at the read/write perms for the object the activty is about (e.g. a Card
+;; or Dashboard). For all other activity feed items with no model everyone can read/write
+(defmethod can-? :default [perms-check-fn {model :model, model-id :model_id, :as activity}]
   (if-let [object (when-let [entity (model->entity model)]
                     (entity model-id))]
-    (f object)
+    (perms-check-fn object)
     true))
 
 
@@ -47,6 +64,7 @@
   i/IObjectPermissions
   (merge i/IObjectPermissionsDefaults
          {:can-read?  (partial can-? i/can-read?)
+          ;; TODO - when do people *write* activities?
           :can-write? (partial can-? i/can-write?)}))
 
 

test/metabase/api/activity_test.clj

@@ -97,8 +97,8 @@
       :details      $})]
   ;; clear any other activities from the DB just in case; not sure this step is needed any more
   (do (db/delete! Activity :id [:not-in #{(:id activity1)
-                                                  (:id activity2)
-                                                  (:id activity3)}])
+                                          (:id activity2)
+                                          (:id activity3)}])
       (for [activity ((user->client :crowberto) :get 200 "activity")]
         (dissoc activity :timestamp))))
 
@@ -211,3 +211,17 @@
                                          {:model "card",      :model_id 0}
                                          {:model "dashboard", :model_id 0, :topic :dashboard-remove-cards, :details {:dashcards [{:card_id card-id}
                                                                                                                                  {:card_id 0}]}}]))
+
+;; Only admins should get to see user-joined activities
+(defn- user-can-see-user-joined-activity? [user]
+  ;; clear out all existing Activity entries
+  (db/delete! Activity)
+  (-> (tt/with-temp Activity [activity {:topic     "user-joined"
+                                        :details   {}
+                                        :timestamp (du/->Timestamp #inst "2019-02-15T11:55:00.000Z")}]
+        ((user->client user) :get 200 "activity"))
+      seq
+      boolean))
+
+(expect true  (user-can-see-user-joined-activity? :crowberto))
+(expect false (user-can-see-user-joined-activity? :rasta))