From fa5f3e228c102d4b9f7c6dde6eb07ef1f5880bbd Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 13 Jul 2022 13:58:01 +1000 Subject: [PATCH] DEV: Refactor user mute/ignore/disallow PM checks into central class (#17366) The idea behind this refactor is to centralise all of the user ignoring / muting / disallow PM checks in a single place, so they can be used consistently in core as well as for plugins like chat, while improving the main bulk of the checks to run in a single fast non-AR query. Also fixed up the invite error when someone is muting/ignoring the user that is trying to invite them to the topic. --- app/controllers/invites_controller.rb | 2 +- app/models/topic.rb | 42 ++--- app/services/post_alerter.rb | 15 +- lib/post_creator.rb | 48 +----- lib/user_comm_screener.rb | 217 ++++++++++++++++++++++++++ spec/lib/user_comm_screener_spec.rb | 153 ++++++++++++++++++ 6 files changed, 391 insertions(+), 86 deletions(-) create mode 100644 lib/user_comm_screener.rb create mode 100644 spec/lib/user_comm_screener_spec.rb diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 221aa2a138e..f66efca157d 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -482,7 +482,7 @@ class InvitesController < ApplicationController topic.create_invite_notification!( user, Notification.types[:invited_to_topic], - invite.invited_by.username + invite.invited_by ) end end diff --git a/app/models/topic.rb b/app/models/topic.rb index c133bc734d0..437b8e6d896 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -1040,7 +1040,10 @@ class Topic < ActiveRecord::Base raise UserExists.new(I18n.t("topic_invite.user_exists")) end - ensure_can_invite!(target_user, invited_by) + comm_screener = UserCommScreener.new(acting_user: invited_by, target_usernames: target_user.username) + if comm_screener.ignoring_or_muting_actor?(target_user.username) + raise NotAllowed.new(I18n.t("not_accepting_pms", username: target_user.username)) + end if TopicUser .where(topic: self, @@ -1050,15 +1053,11 @@ class Topic < ActiveRecord::Base raise NotAllowed.new(I18n.t("topic_invite.muted_topic")) end - if !target_user.staff? && - target_user&.user_option&.enable_allowed_pm_users && - !AllowedPmUser.where(user: target_user, allowed_pm_user: invited_by).exists? + if comm_screener.disallowing_pms_from_actor?(target_user.username) raise NotAllowed.new(I18n.t("topic_invite.receiver_does_not_allow_pm")) end - if !target_user.staff? && - invited_by&.user_option&.enable_allowed_pm_users && - !AllowedPmUser.where(user: invited_by, allowed_pm_user: target_user).exists? + if UserCommScreener.new(acting_user: target_user, target_usernames: invited_by.username).disallowing_pms_from_actor?(invited_by.username) raise NotAllowed.new(I18n.t("topic_invite.sender_does_not_allow_pm")) end @@ -1078,22 +1077,6 @@ class Topic < ActiveRecord::Base end end - def ensure_can_invite!(target_user, invited_by) - if MutedUser - .where(user: target_user, muted_user: invited_by) - .joins(:muted_user) - .where('NOT admin AND NOT moderator') - .exists? - raise NotAllowed - elsif IgnoredUser - .where(user: target_user, ignored_user: invited_by) - .joins(:ignored_user) - .where('NOT admin AND NOT moderator') - .exists? - raise NotAllowed - end - end - def email_already_exists_for?(invite) invite.email_already_exists && private_message? end @@ -1775,9 +1758,10 @@ class Topic < ActiveRecord::Base email_addresses.to_a end - def create_invite_notification!(target_user, notification_type, username, post_number: 1) - invited_by = User.find_by_username(username) - ensure_can_invite!(target_user, invited_by) + def create_invite_notification!(target_user, notification_type, invited_by, post_number: 1) + if UserCommScreener.new(acting_user: invited_by, target_usernames: target_user.username).ignoring_or_muting_actor?(target_user.username) + raise NotAllowed.new(I18n.t("not_accepting_pms", username: target_user.username)) + end target_user.notifications.create!( notification_type: notification_type, @@ -1785,7 +1769,7 @@ class Topic < ActiveRecord::Base post_number: post_number, data: { topic_title: self.title, - display_username: username, + display_username: invited_by.username, original_user_id: user.id, original_username: user.username }.to_json @@ -1876,7 +1860,7 @@ class Topic < ActiveRecord::Base create_invite_notification!( target_user, Notification.types[:invited_to_private_message], - invited_by.username + invited_by ) end end @@ -1904,7 +1888,7 @@ class Topic < ActiveRecord::Base create_invite_notification!( target_user, Notification.types[:invited_to_topic], - invited_by.username + invited_by ) end end diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 409aea80974..c61cb1ad2ae 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -393,18 +393,9 @@ class PostAlerter return if user.staged? && topic.category&.mailinglist_mirror? notifier_id = opts[:user_id] || post.user_id # xxxxx look at revision history - - # apply muting here - return if notifier_id && MutedUser.where(user_id: user.id, muted_user_id: notifier_id) - .joins(:muted_user) - .where('NOT admin AND NOT moderator') - .exists? - - # apply ignored here - return if notifier_id && IgnoredUser.where(user_id: user.id, ignored_user_id: notifier_id) - .joins(:ignored_user) - .where('NOT admin AND NOT moderator') - .exists? + return if notifier_id && UserCommScreener.new( + acting_user_id: notifier_id, target_usernames: user.username + ).ignoring_or_muting_actor?(user.username) # skip if muted on the topic return if TopicUser.where( diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 0158bcad498..c8853e4f4c5 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -111,39 +111,10 @@ class PostCreator return false end - # Make sure none of the users have muted or ignored the creator - users = User.where(username_lower: names).pluck(:id, :username).to_h - - User - .joins("LEFT JOIN user_options ON user_options.user_id = users.id") - .joins("LEFT JOIN muted_users ON muted_users.user_id = users.id AND muted_users.muted_user_id = #{@user.id.to_i}") - .joins("LEFT JOIN ignored_users ON ignored_users.user_id = users.id AND ignored_users.ignored_user_id = #{@user.id.to_i}") - .where("user_options.user_id IS NOT NULL") - .where(" - (user_options.user_id IN (:user_ids) AND NOT user_options.allow_private_messages) OR - muted_users.user_id IN (:user_ids) OR - ignored_users.user_id IN (:user_ids) - ", user_ids: users.keys) - .pluck(:id).each do |m| - - errors.add(:base, I18n.t(:not_accepting_pms, username: users[m])) - end - - # Is Allowed PM users list enabled for any recipients? - users_with_allowed_pms = allowed_pms_enabled(users).pluck(:id).uniq - - # If any of the users has allowed_pm_users enabled check to see if the creator - # is in their list - if users_with_allowed_pms.any? - users_sender_can_pm = allowed_pms_enabled(users) - .where("allowed_pm_users.allowed_pm_user_id" => @user.id.to_i) - .pluck(:id).uniq - - # If not in the list add an error - users_not_allowed = users_with_allowed_pms - users_sender_can_pm - users_not_allowed.each do |id| - errors.add(:base, I18n.t(:not_accepting_pms, username: users[id])) - end + # Make sure none of the users have muted or ignored the creator or prevented + # PMs from being sent to them + UserCommScreener.new(acting_user: @user, target_usernames: names).preventing_actor_communication.each do |username| + errors.add(:base, I18n.t(:not_accepting_pms, username: username)) end return false if errors[:base].present? @@ -487,17 +458,6 @@ class PostCreator private - def allowed_pms_enabled(users) - User - .joins("LEFT JOIN user_options ON user_options.user_id = users.id") - .joins("LEFT JOIN allowed_pm_users ON allowed_pm_users.user_id = users.id") - .where(" - user_options.user_id IS NOT NULL AND - user_options.user_id IN (:user_ids) AND - user_options.enable_allowed_pm_users - ", user_ids: users.keys) - end - def create_topic return if @topic begin diff --git a/lib/user_comm_screener.rb b/lib/user_comm_screener.rb new file mode 100644 index 00000000000..716c5ba841a --- /dev/null +++ b/lib/user_comm_screener.rb @@ -0,0 +1,217 @@ +# frozen_string_literal: true + +# There are various ways within Discourse that a user can prevent +# other users communicating with them. The purpose of this class is to +# find which of the target users are ignoring, muting, or preventing +# private messages from the acting user, so we can take alternative +# action (such as raising an error or showing a helpful message) if so. +# +# Users may Mute another user (the actor), which will: +# +# * Prevent PMs from the actor +# * Prevent notifications from the actor +# +# Users may Ignore another user (the actor), which will: +# +# * Do everything that Mute does as well as suppressing content made by +# the actor (such as posts) from the UI +# +# Users may also either: +# +# a) disallow PMs from being sent to them or +# b) disallow PMs except from a certain allowlist of users +# +# A user may have this preference but have no Muted or Ignored users, which +# necessitates the difference between methods in this class. +# +# An important note is that **all of these settings do not apply when the actor +# is a staff member**. So admins and moderators can PM and notify anyone they please. +class UserCommScreener + attr_reader :acting_user, :preferences + + class UserCommPref + attr_accessor :username, :is_muting, :is_ignoring, :is_disallowing_all_pms, + :is_disallowing_pms_from_acting_user + + def initialize(preferences) + @username = preferences[:username] + @is_muting = preferences[:is_muting] + @is_ignoring = preferences[:is_ignoring] + @is_disallowing_all_pms = preferences[:is_disallowing_all_pms] + @is_disallowing_pms_from_acting_user = preferences[:is_disallowing_pms_from_acting_user] + end + + def communication_prevented? + ignoring_or_muting? || disallowing_pms? + end + + def ignoring_or_muting? + is_muting || is_ignoring + end + + def disallowing_pms? + is_disallowing_all_pms || is_disallowing_pms_from_acting_user + end + end + + UserCommPrefs = Struct.new(:acting_user, :user_preference_map) do + def acting_user_staff? + acting_user.staff? + end + + def usernames + user_preference_map.values.map(&:username) + end + + def for_user(username) + user_preference_map.values.find { |pref| pref.username.downcase == username.downcase } + end + + def allowing_actor_communication + return user_preference_map.values if acting_user_staff? + user_preference_map.reject do |user_id, pref| + pref.communication_prevented? + end.values + end + + def preventing_actor_communication + return [] if acting_user_staff? + user_preference_map.select do |user_id, pref| + pref.communication_prevented? + end.values + end + + def ignoring_or_muting?(username) + return false if acting_user_staff? + pref = for_user(username) + pref.present? && pref.ignoring_or_muting? + end + + def disallowing_pms?(username) + return false if acting_user_staff? + pref = for_user(username) + pref.present? && pref.disallowing_pms? + end + end + private_constant :UserCommPref + private_constant :UserCommPrefs + + def initialize(acting_user: nil, acting_user_id: nil, target_usernames:) + raise ArgumentError if acting_user.blank? && acting_user_id.blank? + @acting_user = acting_user.present? ? acting_user : User.find(acting_user_id) + @target_users = User.where( + username_lower: Array.wrap(target_usernames).map(&:downcase) + ).pluck(:id, :username).to_h + @preferences = load_preference_map + end + + ## + # Users who have preferences are the only ones initially loaded by the query, + # so implicitly the leftover usernames have no preferences that mute, ignore, + # or disallow PMs from any other user. + def allowing_actor_communication + (preferences.allowing_actor_communication.map(&:username) + usernames_with_no_preference).uniq + end + + ## + # Any users who are either ignoring, muting, or disallowing PMs from the actor. + # Ignoring and muting implicitly ignore PMs which is why they fall under this + # umbrella as well. + def preventing_actor_communication + preferences.preventing_actor_communication.map(&:username) + end + + ## + # Whether the user is ignoring or muting the actor, meaning the actor cannot + # PM or send notifications to this target user. + def ignoring_or_muting_actor?(username) + preferences.ignoring_or_muting?(username) + end + + ## + # Whether the user is disallowing PMs from the actor specifically or in general, + # meaning the actor cannot send PMs to this target user. Ignoring or muting + # implicitly disallows PMs, so we need to take into account those preferences + # here too. + def disallowing_pms_from_actor?(username) + preferences.disallowing_pms?(username) || ignoring_or_muting_actor?(username) + end + + private + + def usernames_with_no_preference + @target_users.values - @preferences.usernames + end + + def load_preference_map + resolved_user_communication_preferences = {} + + # Since noone can prevent staff communicating with them there is no + # need to load their preferences. + if @acting_user.staff? + return UserCommPrefs.new(acting_user, resolved_user_communication_preferences) + end + + # Add all users who have muted or ignored the acting user, or have + # disabled PMs from them or anyone at all. + user_communication_preferences.each do |user| + resolved_user_communication_preferences[user.id] = UserCommPref.new( + username: @target_users[user.id], + is_muting: user.is_muting, + is_ignoring: user.is_ignoring, + is_disallowing_all_pms: user.is_disallowing_all_pms, + is_disallowing_pms_from_acting_user: false + ) + end + + # If any of the users has allowed_pm_users enabled check to see if the creator + # is in their list. + users_with_allowed_pms = user_communication_preferences.select(&:enable_allowed_pm_users) + + if users_with_allowed_pms.any? + user_ids_with_allowed_pms = users_with_allowed_pms.map(&:id) + user_ids_acting_can_pm = AllowedPmUser.where( + allowed_pm_user_id: acting_user.id, user_id: user_ids_with_allowed_pms + ).pluck(:user_id).uniq + + # If not in the list mark them as not accepting communication. + user_ids_acting_cannot_pm = user_ids_with_allowed_pms - user_ids_acting_can_pm + user_ids_acting_cannot_pm.each do |user_id| + if resolved_user_communication_preferences[user_id] + resolved_user_communication_preferences[user_id].is_disallowing_pms_from_acting_user = true + else + resolved_user_communication_preferences[user_id] = UserCommPref.new( + username: @target_users[user_id], + is_muting: false, + is_ignoring: false, + is_disallowing_all_pms: false, + is_disallowing_pms_from_acting_user: true + ) + end + end + end + + UserCommPrefs.new(acting_user, resolved_user_communication_preferences) + end + + def user_communication_preferences + @user_communication_preferences ||= DB.query(<<~SQL, acting_user_id: acting_user.id, target_user_ids: @target_users.keys) + SELECT users.id, + CASE WHEN muted_users.muted_user_id IS NOT NULL THEN true ELSE false END AS is_muting, + CASE WHEN ignored_users.ignored_user_id IS NOT NULL THEN true ELSE false END AS is_ignoring, + CASE WHEN user_options.allow_private_messages THEN false ELSE true END AS is_disallowing_all_pms, + user_options.enable_allowed_pm_users + FROM users + LEFT JOIN user_options ON user_options.user_id = users.id + LEFT JOIN muted_users ON muted_users.user_id = users.id AND muted_users.muted_user_id = :acting_user_id + LEFT JOIN ignored_users ON ignored_users.user_id = users.id AND ignored_users.ignored_user_id = :acting_user_id + WHERE users.id IN (:target_user_ids) AND + ( + NOT user_options.allow_private_messages OR + user_options.enable_allowed_pm_users OR + muted_users.user_id IS NOT NULL OR + ignored_users.user_id IS NOT NULL + ) + SQL + end +end diff --git a/spec/lib/user_comm_screener_spec.rb b/spec/lib/user_comm_screener_spec.rb new file mode 100644 index 00000000000..582b55ffb44 --- /dev/null +++ b/spec/lib/user_comm_screener_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +describe UserCommScreener do + fab!(:target_user1) { Fabricate(:user, username: "bobscreen") } + fab!(:target_user2) { Fabricate(:user, username: "hughscreen") } + fab!(:target_user3) do + user = Fabricate(:user, username: "alicescreen") + user.user_option.update(allow_private_messages: false) + user + end + fab!(:target_user4) { Fabricate(:user, username: "janescreen") } + fab!(:target_user5) { Fabricate(:user, username: "maryscreen") } + + subject do + described_class.new( + acting_user: acting_user, target_usernames: ["bobscreen", "hughscreen", "alicescreen", "janescreen", "maryscreen"] + ) + end + + it "allows initializing the class with both an acting_user_id and an acting_user" do + acting_user = Fabricate(:user) + screener = described_class.new(acting_user: acting_user, target_usernames: ["bobscreen"]) + expect(screener.allowing_actor_communication).to eq(["bobscreen"]) + screener = described_class.new(acting_user_id: acting_user.id, target_usernames: ["bobscreen"]) + expect(screener.allowing_actor_communication).to eq(["bobscreen"]) + end + + it "makes sure to lowercase target usernames" do + acting_user = Fabricate(:user) + screener = described_class.new(acting_user: acting_user, target_usernames: ["BoBscrEEN", "HUghSCreen"]) + expect(screener.allowing_actor_communication).to eq(["bobscreen", "hughscreen"]) + end + + context "when the actor is not staff" do + fab!(:acting_user) { Fabricate(:user) } + fab!(:muted_user) { Fabricate(:muted_user, user: target_user1, muted_user: acting_user) } + fab!(:ignored_user) { Fabricate(:ignored_user, user: target_user2, ignored_user: acting_user, expiring_at: 2.days.from_now) } + + describe "#allowing_actor_communication" do + it "returns the usernames of people not ignoring, muting, or disallowing PMs from the actor" do + expect(subject.allowing_actor_communication).to match_array(["janescreen", "maryscreen"]) + end + end + + describe "#preventing_actor_communication" do + it "returns the usernames of people ignoring, muting, or disallowing PMs from the actor" do + expect(subject.preventing_actor_communication).to match_array(["bobscreen", "hughscreen", "alicescreen"]) + end + end + + describe "#ignoring_or_muting_actor?" do + it "does not raise an error when looking for a user who has no communication preferences" do + expect { subject.ignoring_or_muting_actor?(target_user5.username) }.not_to raise_error + end + + it "returns true for a user muting the actor" do + expect(subject.ignoring_or_muting_actor?(target_user1.username)).to eq(true) + end + + it "returns true for a user ignoring the actor" do + expect(subject.ignoring_or_muting_actor?(target_user2.username)).to eq(true) + end + + it "returns false for a user neither ignoring or muting the actor" do + expect(subject.ignoring_or_muting_actor?(target_user3.username)).to eq(false) + end + end + + describe "#disallowing_pms_from_actor?" do + it "returns true for a user disallowing all PMs" do + expect(subject.disallowing_pms_from_actor?(target_user3.username)).to eq(true) + end + + it "returns true for a user allowing only PMs for certain users but not the actor" do + target_user4.user_option.update!(enable_allowed_pm_users: true) + expect(subject.disallowing_pms_from_actor?(target_user4.username)).to eq(true) + end + + it "returns false for a user allowing only PMs for certain users which the actor allowed" do + target_user4.user_option.update!(enable_allowed_pm_users: true) + AllowedPmUser.create!(user: target_user4, allowed_pm_user: acting_user) + expect(subject.disallowing_pms_from_actor?(target_user4.username)).to eq(false) + end + + it "returns false for a user not disallowing PMs or muting or ignoring" do + expect(subject.disallowing_pms_from_actor?(target_user5.username)).to eq(false) + end + + it "returns true for a user not disallowing PMs but still ignoring" do + expect(subject.disallowing_pms_from_actor?(target_user1.username)).to eq(true) + end + + it "returns true for a user not disallowing PMs but still muting" do + expect(subject.disallowing_pms_from_actor?(target_user2.username)).to eq(true) + end + end + end + + context "when the actor is staff" do + fab!(:acting_user) { Fabricate(:admin) } + fab!(:muted_user) { Fabricate(:muted_user, user: target_user1, muted_user: acting_user) } + fab!(:ignored_user) { Fabricate(:ignored_user, user: target_user1, ignored_user: acting_user, expiring_at: 2.days.from_now) } + + describe "#allowing_actor_communication" do + it "returns all usernames since staff can communicate with anyone" do + expect(subject.allowing_actor_communication).to match_array(["bobscreen", "hughscreen", "alicescreen", "janescreen", "maryscreen"]) + end + end + + describe "#preventing_actor_communication" do + it "does not return any usernames since no users can prevent staff communicating with them" do + expect(subject.preventing_actor_communication).to eq([]) + end + end + + describe "#ignoring_or_muting_actor?" do + it "returns false for a user muting the staff" do + expect(subject.ignoring_or_muting_actor?(target_user1.username)).to eq(false) + end + + it "returns false for a user ignoring the staff actor" do + expect(subject.ignoring_or_muting_actor?(target_user2.username)).to eq(false) + end + + it "returns false for a user neither ignoring or muting the actor" do + expect(subject.ignoring_or_muting_actor?(target_user3.username)).to eq(false) + end + end + + describe "#disallowing_pms_from_actor?" do + it "returns false for a user disallowing all PMs" do + expect(subject.disallowing_pms_from_actor?(target_user3.username)).to eq(false) + end + + it "returns false for a user allowing only PMs for certain users but not the actor" do + target_user4.user_option.update!(enable_allowed_pm_users: true) + expect(subject.disallowing_pms_from_actor?(target_user4.username)).to eq(false) + end + + it "returns false for a user not disallowing PMs or muting or ignoring" do + expect(subject.disallowing_pms_from_actor?(target_user5.username)).to eq(false) + end + + it "returns false for a user not disallowing PMs but still ignoring" do + expect(subject.disallowing_pms_from_actor?(target_user1.username)).to eq(false) + end + + it "returns false for a user not disallowing PMs but still muting" do + expect(subject.disallowing_pms_from_actor?(target_user2.username)).to eq(false) + end + end + end +end