From d02fef97bbd2af0fe2a5e41af4077b107fa73a39 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Wed, 22 Mar 2023 10:03:48 -0400
Subject: [PATCH] Show all subscriptions on /account/notifications page
(#29214)
* show all subscriptions in notification management page regardless of collection perms
* strip cards and recipients if necessary
* indentation tweak
* add backend tests
* add cypress repro
* fix permissions check
* slightly different approach
* fix cypress test
* fix test
* address comment
---
...ations-without-collection-perms.cy.spec.js | 38 ++++++++++
.../NotificationsApp/NotificationsApp.jsx | 4 +-
frontend/src/metabase/lib/notifications.js | 10 ++-
src/metabase/api/pulse.clj | 73 ++++++++++++-------
src/metabase/models/pulse.clj | 19 +++--
test/metabase/api/pulse_test.clj | 64 ++++++++++++----
test/metabase/models/pulse_test.clj | 11 +--
7 files changed, 152 insertions(+), 67 deletions(-)
create mode 100644 e2e/test/scenarios/permissions/reproductions/22473-cannot-unsubscribe-from-notifications-without-collection-perms.cy.spec.js
diff --git a/e2e/test/scenarios/permissions/reproductions/22473-cannot-unsubscribe-from-notifications-without-collection-perms.cy.spec.js b/e2e/test/scenarios/permissions/reproductions/22473-cannot-unsubscribe-from-notifications-without-collection-perms.cy.spec.js
new file mode 100644
index 00000000000..1c5130de730
--- /dev/null
+++ b/e2e/test/scenarios/permissions/reproductions/22473-cannot-unsubscribe-from-notifications-without-collection-perms.cy.spec.js
@@ -0,0 +1,38 @@
+import { restore, setupSMTP, sidebar } from "e2e/support/helpers";
+import { modal } from "e2e/support/helpers/e2e-ui-elements-helpers";
+
+import { USERS } from "e2e/support/cypress_data";
+const { nocollection } = USERS;
+
+describe("issue 22473", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ setupSMTP();
+ });
+
+ it("nocollection user should be able to view and unsubscribe themselves from a subscription", () => {
+ cy.visit(`/dashboard/1`);
+ cy.icon("subscription").click();
+ cy.findByText("Email it").click();
+ cy.findByPlaceholderText("Enter user names or email addresses")
+ .click()
+ .type(`${nocollection.first_name} ${nocollection.last_name}{enter}`)
+ .blur();
+ sidebar().within(() => {
+ cy.button("Done").click()
+ });
+
+ cy.signIn("nocollection");
+ cy.visit("/account/notifications");
+
+ cy.findByText("Orders in a dashboard").should("exist");
+ cy.findByTestId("notifications-list").within(() => {
+ cy.findByLabelText("close icon").click();
+ });
+ modal().within(() => {
+ cy.button("Unsubscribe").click()
+ });
+ cy.findByText("Orders in a dashboard").should("not.exist");
+ })
+})
diff --git a/frontend/src/metabase/account/notifications/containers/NotificationsApp/NotificationsApp.jsx b/frontend/src/metabase/account/notifications/containers/NotificationsApp/NotificationsApp.jsx
index 70a8a21964f..2d9bf183c62 100644
--- a/frontend/src/metabase/account/notifications/containers/NotificationsApp/NotificationsApp.jsx
+++ b/frontend/src/metabase/account/notifications/containers/NotificationsApp/NotificationsApp.jsx
@@ -33,8 +33,8 @@ export default _.compose(
reload: true,
}),
Pulses.loadList({
- // Load all pulses the current user can read (i.e. is a creator or recipient of)
- query: state => ({ can_read: true }),
+ // Load all pulses the current user is a creator or recipient of
+ query: state => ({ creator_or_recipient: true }),
reload: true,
}),
connect(mapStateToProps, mapDispatchToProps),
diff --git a/frontend/src/metabase/lib/notifications.js b/frontend/src/metabase/lib/notifications.js
index c0092d54606..a67a8e8443f 100644
--- a/frontend/src/metabase/lib/notifications.js
+++ b/frontend/src/metabase/lib/notifications.js
@@ -117,9 +117,13 @@ export const getRecipientsCount = (item, channelType) => {
};
export const canArchive = (item, user) => {
- const recipients = item.channels.flatMap(channel =>
- channel.recipients.map(recipient => recipient.id),
- );
+ const recipients = item.channels.flatMap(channel => {
+ if (channel.recipients) {
+ return channel.recipients.map(recipient => recipient.id);
+ } else {
+ return [];
+ }
+ });
const isCreator = item.creator?.id === user.id;
const isSubscribed = recipients.includes(user.id);
diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj
index 1e30267cdf7..3c1c9752220 100644
--- a/src/metabase/api/pulse.clj
+++ b/src/metabase/api/pulse.clj
@@ -40,7 +40,7 @@
(u/ignore-exceptions (classloader/require 'metabase-enterprise.sandbox.api.util
'metabase-enterprise.advanced-permissions.common))
-(defn- filter-pulses-recipients
+(defn- maybe-filter-pulses-recipients
"If the current user is sandboxed, remove all Metabase users from the `pulses` recipient lists that are not the user
themselves. Recipients that are plain email addresses are preserved."
[pulses]
@@ -56,9 +56,20 @@
pulses)
pulses))
-(defn- filter-pulse-recipients
+(defn- maybe-filter-pulse-recipients
[pulse]
- (first (filter-pulses-recipients [pulse])))
+ (first (maybe-filter-pulses-recipients [pulse])))
+
+(defn- maybe-strip-sensitive-metadata
+ "If the current user does not have collection read permissions for the pulse, but can still read the pulse due to
+ being the creator or a recipient, we return it with some metadata removed."
+ [pulse]
+ (if (mi/current-user-has-full-permissions? :read pulse)
+ pulse
+ (-> (dissoc pulse :cards)
+ (update :channels
+ (fn [channels]
+ (map #(dissoc % :recipients) channels))))))
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema GET "/"
@@ -67,21 +78,25 @@
If `dashboard_id` is specified, restricts results to subscriptions for that dashboard.
- If `can_read` is `true`, it specifically returns all subscriptions for which the current user
+ If `created_or_receive` is `true`, it specifically returns all subscriptions for which the current user
created *or* is a known recipient of. Note that this is a superset of the default items returned for non-admins,
- and a subset of the default items returned for admins. This is used to power the /account/notifications page."
- [archived dashboard_id can_read]
- {archived (s/maybe su/BooleanString)
- dashboard_id (s/maybe su/IntGreaterThanZero)
- can_read (s/maybe su/BooleanString)}
- (let [can-read? (Boolean/parseBoolean can_read)
- archived? (Boolean/parseBoolean archived)
- pulses (->> (pulse/retrieve-pulses {:archived? archived?
- :dashboard-id dashboard_id
- :user-id api/*current-user-id*
- :can-read? can-read?})
- (filter (if can-read? mi/can-read? mi/can-write?))
- filter-pulses-recipients)]
+ and a subset of the default items returned for admins. This is used to power the /account/notifications page.
+ This may include subscriptions which the current user does not have collection permissions for, in which case
+ some sensitive metadata (the list of cards and recipients) is stripped out."
+ [archived dashboard_id creator_or_recipient]
+ {archived (s/maybe su/BooleanString)
+ dashboard_id (s/maybe su/IntGreaterThanZero)
+ creator_or_recipient (s/maybe su/BooleanString)}
+ (let [creator-or-recipient (Boolean/parseBoolean creator_or_recipient)
+ archived? (Boolean/parseBoolean archived)
+ pulses (->> (pulse/retrieve-pulses {:archived? archived?
+ :dashboard-id dashboard_id
+ :user-id (when creator-or-recipient api/*current-user-id*)})
+ (filter (if creator-or-recipient mi/can-read? mi/can-write?))
+ maybe-filter-pulses-recipients)
+ pulses (if creator-or-recipient
+ (map maybe-strip-sensitive-metadata pulses)
+ pulses)]
(hydrate pulses :can_write)))
(defn check-card-read-permissions
@@ -122,20 +137,24 @@
:dashboard_id dashboard_id
:parameters parameters}]
(db/transaction
- ;; Adding a new pulse at `collection_position` could cause other pulses in this collection to change position,
- ;; check that and fix it if needed
- (api/maybe-reconcile-collection-position! pulse-data)
- ;; ok, now create the Pulse
- (api/check-500
- (pulse/create-pulse! (map pulse/card->ref cards) channels pulse-data)))))
+ ;; Adding a new pulse at `collection_position` could cause other pulses in this collection to change position,
+ ;; check that and fix it if needed
+ (api/maybe-reconcile-collection-position! pulse-data)
+ ;; ok, now create the Pulse
+ (api/check-500
+ (pulse/create-pulse! (map pulse/card->ref cards) channels pulse-data)))))
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema GET "/:id"
- "Fetch `Pulse` with ID."
+ "Fetch `Pulse` with ID. If the user is a recipient of the Pulse but does not have read permissions for its collection,
+ we still return it but with some sensitive metadata removed."
[id]
- (-> (api/read-check (pulse/retrieve-pulse id))
- filter-pulse-recipients
- (hydrate :can_write)))
+ (api/let-404 [pulse (pulse/retrieve-pulse id)]
+ (api/check-403 (mi/can-read? pulse))
+ (-> pulse
+ maybe-filter-pulse-recipients
+ maybe-strip-sensitive-metadata
+ (hydrate :can_write))))
(defn- maybe-add-recipients-for-sandboxed-users
"Sandboxed users can't read the full recipient list for a pulse, so we need to merge in existing recipients
diff --git a/src/metabase/models/pulse.clj b/src/metabase/models/pulse.clj
index 9b483d2dc0e..d8177249b7e 100644
--- a/src/metabase/models/pulse.clj
+++ b/src/metabase/models/pulse.clj
@@ -129,9 +129,8 @@
(if (is-alert? notification)
(mi/current-user-has-full-permissions? :read notification)
(or api/*is-superuser?*
- (and (mi/current-user-has-full-permissions? :read notification)
- (or (current-user-is-creator? notification)
- (current-user-is-recipient? notification))))))
+ (or (current-user-is-creator? notification)
+ (current-user-is-recipient? notification)))))
;; Non-admins should be able to create subscriptions, and update subscriptions that they created, but not edit anyone
;; else's subscriptions (except for unsubscribing themselves, which uses a custom API).
@@ -329,15 +328,15 @@
alert))))
(s/defn retrieve-pulses :- [(mi/InstanceOf Pulse)]
- "Fetch all `Pulses`."
- [{:keys [archived? dashboard-id can-read? user-id]
- :or {archived? false
- can-read? false}}]
+ "Fetch all `Pulses`. When `user-id` is included, only fetches `Pulses` for which the provided user is the creator
+ or a recipient."
+ [{:keys [archived? dashboard-id user-id]
+ :or {archived? false}}]
(let [query {:select-distinct [:p.* [[:lower :p.name] :lower-name]]
:from [[:pulse :p]]
:left-join (concat
[[:report_dashboard :d] [:= :p.dashboard_id :d.id]]
- (when can-read?
+ (when user-id
[[:pulse_channel :pchan] [:= :p.id :pchan.pulse_id]
[:pulse_channel_recipient :pcr] [:= :pchan.id :pcr.pulse_channel_id]]))
:where [:and
@@ -349,9 +348,9 @@
[:= :d.archived false]]
(when dashboard-id
[:= :p.dashboard_id dashboard-id])
- ;; Only return dashboard subscriptions when `can-read?` is `true` so that legacy
+ ;; Only return dashboard subscriptions when `user-id` is passed, so that legacy
;; pulses don't show up in the notification management page
- (when can-read?
+ (when user-id
[:and
[:not= :p.dashboard_id nil]
[:or
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index 13407b1b7b9..c7bab8ae6e1 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -82,6 +82,9 @@
:where [:in :id (set (map u/the-id pulses-or-ids))]}))
(f))))
+(defmacro ^:private with-pulses-in-nonreadable-collection [pulses-or-ids & body]
+ `(do-with-pulses-in-a-collection (constantly nil) ~pulses-or-ids (fn [] ~@body)))
+
(defmacro ^:private with-pulses-in-readable-collection [pulses-or-ids & body]
`(do-with-pulses-in-a-collection perms/grant-collection-read-permissions! ~pulses-or-ids (fn [] ~@body)))
@@ -784,22 +787,34 @@
[(assoc (pulse-details pulse-1) :can_write true, :collection_id true)]
(map #(update % :collection_id boolean) results)))))
- (testing "when `can_read=true`, all users only see pulses they created or are a recipient of"
- (let [results (-> (mt/user-http-request :crowberto :get 200 "pulse?can_read=true")
- (filter-pulse-results :id #{pulse-1-id pulse-2-id pulse-3-id}))]
- (is (= 2 (count results)))
- (is (partial=
- [(assoc (pulse-details pulse-2) :can_write true, :collection_id true)
- (assoc (pulse-details pulse-3) :can_write true, :collection_id true)]
- (map #(update % :collection_id boolean) results))))
-
- (let [results (-> (mt/user-http-request :rasta :get 200 "pulse?can_read=true")
- (filter-pulse-results :id #{pulse-1-id pulse-2-id pulse-3-id}))]
- (is (= 2 (count results)))
- (is (partial=
- [(assoc (pulse-details pulse-1) :can_write true, :collection_id true)
- (assoc (pulse-details pulse-3) :can_write false, :collection_id true)]
- (map #(update % :collection_id boolean) results)))))))
+ (testing "when `creator_or_recipient=true`, all users only see pulses they created or are a recipient of"
+ (let [expected-pulse-shape (fn [pulse] (-> pulse
+ pulse-details
+ (assoc :can_write true, :collection_id true)
+ (dissoc :cards)))]
+ (let [results (-> (mt/user-http-request :crowberto :get 200 "pulse?creator_or_recipient=true")
+ (filter-pulse-results :id #{pulse-1-id pulse-2-id pulse-3-id}))]
+ (is (= 2 (count results)))
+ (is (partial=
+ [(expected-pulse-shape pulse-2) (expected-pulse-shape pulse-3)]
+ (map #(update % :collection_id boolean) results))))
+
+ (let [results (-> (mt/user-http-request :rasta :get 200 "pulse?creator_or_recipient=true")
+ (filter-pulse-results :id #{pulse-1-id pulse-2-id pulse-3-id}))]
+ (is (= 2 (count results)))
+ (is (partial=
+ [(expected-pulse-shape pulse-1)
+ (assoc (expected-pulse-shape pulse-3) :can_write false)]
+ (map #(update % :collection_id boolean) results)))))))
+
+ (with-pulses-in-nonreadable-collection [pulse-3]
+ (testing "when `creator_or_recipient=true`, cards and recipients are not included in results if the user
+ does not have collection perms"
+ (let [result (-> (mt/user-http-request :rasta :get 200 "pulse?creator_or_recipient=true")
+ (filter-pulse-results :id #{pulse-3-id})
+ first)]
+ (is (nil? (:cards result)))
+ (is (nil? (get-in result [:channels 0 :recipients])))))))
(testing "should not return alerts"
(mt/with-temp* [Pulse [pulse-1 {:name "ABCDEF"}]
@@ -845,6 +860,23 @@
(-> (mt/user-http-request :rasta :get 200 (str "pulse/" (u/the-id pulse)))
(update :collection_id boolean))))))
+ (testing "cannot normally fetch a pulse without collection permissions"
+ (mt/with-temp Pulse [pulse {:creator_id (mt/user->id :crowberto)}]
+ (with-pulses-in-nonreadable-collection [pulse]
+ (mt/user-http-request :rasta :get 403 (str "pulse/" (u/the-id pulse))))))
+
+ (testing "can fetch a pulse without collection permissions if you are the creator or a recipient"
+ (mt/with-temp Pulse [pulse {:creator_id (mt/user->id :rasta)}]
+ (with-pulses-in-nonreadable-collection [pulse]
+ (mt/user-http-request :rasta :get 200 (str "pulse/" (u/the-id pulse)))))
+
+ (mt/with-temp* [Pulse [pulse {:creator_id (mt/user->id :crowberto)}]
+ PulseChannel [pc {:pulse_id (u/the-id pulse)}]
+ PulseChannelRecipient [_ {:pulse_channel_id (u/the-id pc)
+ :user_id (mt/user->id :rasta)}]]
+ (with-pulses-in-nonreadable-collection [pulse]
+ (mt/user-http-request :rasta :get 200 (str "pulse/" (u/the-id pulse))))))
+
(testing "should 404 for an Alert"
(mt/with-temp Pulse [{pulse-id :id} {:alert_condition "rows"}]
(is (= "Not found."
diff --git a/test/metabase/models/pulse_test.clj b/test/metabase/models/pulse_test.clj
index 9fe438de306..91a21b15994 100644
--- a/test/metabase/models/pulse_test.clj
+++ b/test/metabase/models/pulse_test.clj
@@ -442,19 +442,12 @@
(is (mi/can-write? subscription))))
(mt/with-current-user (mt/user->id :rasta)
- (testing "A non-admin has no access to a subscription if they don't have read access to the parent collection,
- even if they created the subscription"
- (is (not (mi/can-read? subscription)))
- (is (not (mi/can-write? subscription))))
-
(binding [api/*current-user-permissions-set* (delay #{(perms/collection-read-path collection)})]
- (testing "A non-admin has read and write access to a subscription they created, if they have read access to the
- parent collection"
+ (testing "A non-admin has read and write access to a subscription they created"
(is (mi/can-read? subscription))
(is (mi/can-write? subscription)))
- (testing "A non-admin has read-only access to a subscription they are a recipient of, if they have read access
- to the parent collection"
+ (testing "A non-admin has read-only access to a subscription they are a recipient of"
;; Create a new Dashboard Subscription with an admin creator but non-admin recipient
(mt/with-temp* [Pulse [subscription {:collection_id (u/the-id collection)
:dashboard_id (u/the-id dashboard)
--
GitLab