From 690f17bcbea17fc44d26b5e6962beb6a9023c140 Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Mon, 20 Jul 2020 15:23:49 -0600 Subject: [PATCH] FEATURE: Allow List for PMs (#10270) * FEATURE: Allow List for PMs This feature adds a new user setting that is disabled by default that allows them to specify a list of users that are allowed to send them private messages. This way they don't have to maintain a large list of users they don't want to here from and instead just list the people they know they do want. Staff will still always be able to send messages to the user. * Update PR based on feedback --- .../app/controllers/preferences/interface.js | 1 + .../controllers/preferences/notifications.js | 9 +- .../app/controllers/preferences/users.js | 30 +++- .../javascripts/discourse/app/models/user.js | 2 + .../templates/preferences/notifications.hbs | 8 ++ .../app/templates/preferences/users.hbs | 19 +++ app/controllers/users_controller.rb | 1 + app/models/allowed_pm_user.rb | 22 +++ app/models/user_option.rb | 1 + app/serializers/user_option_serializer.rb | 1 + app/serializers/user_serializer.rb | 5 + app/services/user_updater.rb | 27 ++++ config/locales/client.en.yml | 3 + .../20200717193118_create_allowed_pm_users.rb | 13 ++ ...enable_allowed_pm_users_to_user_options.rb | 7 + lib/post_creator.rb | 30 +++- spec/components/post_creator_spec.rb | 135 ++++++++++++++++++ spec/fabricators/allowed_pm_users.rb | 5 + .../web_hook_user_serializer_spec.rb | 2 +- 19 files changed, 316 insertions(+), 5 deletions(-) create mode 100644 app/models/allowed_pm_user.rb create mode 100644 db/migrate/20200717193118_create_allowed_pm_users.rb create mode 100644 db/migrate/20200718154308_add_enable_allowed_pm_users_to_user_options.rb create mode 100644 spec/fabricators/allowed_pm_users.rb diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/interface.js b/app/assets/javascripts/discourse/app/controllers/preferences/interface.js index 2dc3fa46570..9baf458d696 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/interface.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/interface.js @@ -38,6 +38,7 @@ export default Controller.extend({ "enable_defer", "automatically_unpin_topics", "allow_private_messages", + "enable_allowed_pm_users", "homepage_id", "hide_profile_and_presence", "text_size", diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js b/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js index f738f82bb6c..45686195ee6 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js @@ -2,6 +2,7 @@ import I18n from "I18n"; import Controller from "@ember/controller"; import { NotificationLevels } from "discourse/lib/notification-levels"; import { popupAjaxError } from "discourse/lib/ajax-error"; +import discourseComputed from "discourse-common/utils/decorators"; export default Controller.extend({ init() { @@ -14,7 +15,8 @@ export default Controller.extend({ "auto_track_topics_after_msecs", "notification_level_when_replying", "like_notification_frequency", - "allow_private_messages" + "allow_private_messages", + "enable_allowed_pm_users" ]; this.likeNotificationFrequencies = [ @@ -91,6 +93,11 @@ export default Controller.extend({ this.isIOS = caps.isIOS; }, + @discourseComputed("model.user_option.allow_private_messages") + disableAllowPmUsersSetting(allowPrivateMessages) { + return !allowPrivateMessages; + }, + actions: { save() { this.set("saved", false); diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/users.js b/app/assets/javascripts/discourse/app/controllers/preferences/users.js index b0ac0260253..d16f196f20e 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/users.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/users.js @@ -1,13 +1,18 @@ import { makeArray } from "discourse-common/lib/helpers"; -import { alias, gte, or } from "@ember/object/computed"; +import { alias, gte, or, and } from "@ember/object/computed"; import { action, computed } from "@ember/object"; import Controller from "@ember/controller"; import { popupAjaxError } from "discourse/lib/ajax-error"; export default Controller.extend({ ignoredUsernames: alias("model.ignored_usernames"), + allowedPmUsernames: alias("model.allowed_pm_usernames"), userIsMemberOrAbove: gte("model.trust_level", 2), ignoredEnabled: or("userIsMemberOrAbove", "model.staff"), + allowPmUsersEnabled: and( + "model.user_option.enable_allowed_pm_users", + "model.user_option.allow_private_messages" + ), mutedUsernames: computed("model.muted_usernames", { get() { @@ -21,10 +26,26 @@ export default Controller.extend({ } }), + allowedPmUsernames: computed("model.allowed_pm_usernames", { + get() { + let usernames = this.model.allowed_pm_usernames; + + if (typeof usernames === "string") { + usernames = usernames.split(",").filter(Boolean); + } + + return makeArray(usernames).uniq(); + } + }), + init() { this._super(...arguments); - this.saveAttrNames = ["muted_usernames", "ignored_usernames"]; + this.saveAttrNames = [ + "muted_usernames", + "ignored_usernames", + "allowed_pm_usernames" + ]; }, @action @@ -32,6 +53,11 @@ export default Controller.extend({ this.model.set("muted_usernames", usernames.uniq().join(",")); }, + @action + onChangeAllowedPmUsernames(usernames) { + this.model.set("allowed_pm_usernames", usernames.uniq().join(",")); + }, + @action save() { this.set("saved", false); diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js index 72c2695aa7d..c4bf6b5f584 100644 --- a/app/assets/javascripts/discourse/app/models/user.js +++ b/app/assets/javascripts/discourse/app/models/user.js @@ -276,6 +276,7 @@ const User = RestModel.extend({ "user_fields", "muted_usernames", "ignored_usernames", + "allowed_pm_usernames", "profile_background_upload_url", "card_background_upload_url", "muted_tags", @@ -311,6 +312,7 @@ const User = RestModel.extend({ "include_tl0_in_digests", "theme_ids", "allow_private_messages", + "enable_allowed_pm_users", "homepage_id", "hide_profile_and_presence", "text_size", diff --git a/app/assets/javascripts/discourse/app/templates/preferences/notifications.hbs b/app/assets/javascripts/discourse/app/templates/preferences/notifications.hbs index 9e079fd5c5b..450ebde95b4 100644 --- a/app/assets/javascripts/discourse/app/templates/preferences/notifications.hbs +++ b/app/assets/javascripts/discourse/app/templates/preferences/notifications.hbs @@ -61,7 +61,15 @@ labelKey="user.allow_private_messages" checked=model.user_option.allow_private_messages}} + +
+ {{preference-checkbox + labelKey="user.allow_private_messages_from_specific_users" + checked=model.user_option.enable_allowed_pm_users + disabled=disableAllowPmUsersSetting}} +
+ {{/if}} {{plugin-outlet name="user-preferences-notifications" args=(hash model=model save=(action "save"))}} diff --git a/app/assets/javascripts/discourse/app/templates/preferences/users.hbs b/app/assets/javascripts/discourse/app/templates/preferences/users.hbs index 7db61c6aa2b..91eb15dc900 100644 --- a/app/assets/javascripts/discourse/app/templates/preferences/users.hbs +++ b/app/assets/javascripts/discourse/app/templates/preferences/users.hbs @@ -25,6 +25,25 @@
{{i18n "user.muted_users_instructions"}}
+{{#if allowPmUsersEnabled}} +
+
+ + {{user-chooser + value=allowedPmUsernames + onChange=(action "onChangeAllowedPmUsernames") + options=(hash + excludeCurrentUser=true + ) + }} +
+
{{i18n "user.allowed_pm_users_instructions"}}
+
+{{/if}} + {{plugin-outlet name="user-custom-controls" args=(hash model=model)}} {{#save-controls model=model action=(action "save") saved=saved}}{{/save-controls}} diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 5127dc8d5b8..fccb772d984 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1581,6 +1581,7 @@ class UsersController < ApplicationController :date_of_birth, :muted_usernames, :ignored_usernames, + :allowed_pm_usernames, :theme_ids, :locale, :bio_raw, diff --git a/app/models/allowed_pm_user.rb b/app/models/allowed_pm_user.rb new file mode 100644 index 00000000000..dea883585cc --- /dev/null +++ b/app/models/allowed_pm_user.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class AllowedPmUser < ActiveRecord::Base + belongs_to :user + belongs_to :allowed_pm_user, class_name: "User" +end + +# == Schema Information +# +# Table name: allowed_pm_users +# +# id :bigint not null, primary key +# user_id :integer not null +# allowed_pm_user_id :integer not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_allowed_pm_users_on_allowed_pm_user_id_and_user_id (allowed_pm_user_id,user_id) UNIQUE +# index_allowed_pm_users_on_user_id_and_allowed_pm_user_id (user_id,allowed_pm_user_id) UNIQUE +# diff --git a/app/models/user_option.rb b/app/models/user_option.rb index f5db5c37b1a..f3b48c6eadc 100644 --- a/app/models/user_option.rb +++ b/app/models/user_option.rb @@ -227,6 +227,7 @@ end # title_count_mode_key :integer default(0), not null # enable_defer :boolean default(FALSE), not null # timezone :string +# enable_allowed_pm_users :boolean default(FALSE), not null # # Indexes # diff --git a/app/serializers/user_option_serializer.rb b/app/serializers/user_option_serializer.rb index 4c78c7b5bda..c95f454d089 100644 --- a/app/serializers/user_option_serializer.rb +++ b/app/serializers/user_option_serializer.rb @@ -23,6 +23,7 @@ class UserOptionSerializer < ApplicationSerializer :theme_ids, :theme_key_seq, :allow_private_messages, + :enable_allowed_pm_users, :homepage_id, :hide_profile_and_presence, :text_size, diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 9d9232af95d..f78e9f8ac12 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -49,6 +49,7 @@ class UserSerializer < UserCardSerializer :has_title_badges, :muted_usernames, :ignored_usernames, + :allowed_pm_usernames, :mailing_list_posts_per_day, :can_change_bio, :can_change_location, @@ -228,6 +229,10 @@ class UserSerializer < UserCardSerializer IgnoredUser.where(user_id: object.id).joins(:ignored_user).pluck(:username) end + def allowed_pm_usernames + AllowedPmUser.where(user_id: object.id).joins(:allowed_pm_user).pluck(:username) + end + def system_avatar_upload_id # should be left blank end diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index b30f5b13c79..4293f4aeed5 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -37,6 +37,7 @@ class UserUpdater :include_tl0_in_digests, :theme_ids, :allow_private_messages, + :enable_allowed_pm_users, :homepage_id, :hide_profile_and_presence, :text_size, @@ -164,6 +165,10 @@ class UserUpdater update_ignored_users(attributes[:ignored_usernames]) end + if attributes.key?(:allowed_pm_usernames) + update_allowed_pm_users(attributes[:allowed_pm_usernames]) + end + name_changed = user.name_changed? if (saved = (!save_options || user.user_option.save) && user_profile.save && user.save) && (name_changed && old_user_name.casecmp(attributes.fetch(:name)) != 0) @@ -225,6 +230,28 @@ class UserUpdater end end + def update_allowed_pm_users(usernames) + #return unless guardian.can_ignore_users? + + usernames ||= "" + desired_usernames = usernames.split(",").reject { |username| user.username == username } + desired_ids = User.where(username: desired_usernames).where(admin: false, moderator: false).pluck(:id) + if desired_ids.empty? + AllowedPmUser.where(user_id: user.id).destroy_all + else + AllowedPmUser.where('user_id = ? AND allowed_pm_user_id not in (?)', user.id, desired_ids).destroy_all + + # SQL is easier here than figuring out how to do the same in AR + DB.exec(<<~SQL, now: Time.zone.now, user_id: user.id, desired_ids: desired_ids) + INSERT into allowed_pm_users(user_id, allowed_pm_user_id, created_at, updated_at) + SELECT :user_id, id, :now, :now + FROM users + WHERE id in (:desired_ids) + ON CONFLICT DO NOTHING + SQL + end + end + private attr_reader :user, :guardian diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 404a8c14811..1aaed416515 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -975,6 +975,9 @@ en: users: "Users" muted_users: "Muted" muted_users_instructions: "Suppress all notifications and PMs from these users." + allowed_pm_users: "Allowed" + allowed_pm_users_instructions: "Only allow PMs from these users." + allow_private_messages_from_specific_users: "Only allow specific users to send me personal messages" ignored_users: "Ignored" ignored_users_instructions: "Suppress all posts, notifications, and PMs from these users." tracked_topics_link: "Show" diff --git a/db/migrate/20200717193118_create_allowed_pm_users.rb b/db/migrate/20200717193118_create_allowed_pm_users.rb new file mode 100644 index 00000000000..5c368eccf20 --- /dev/null +++ b/db/migrate/20200717193118_create_allowed_pm_users.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true +class CreateAllowedPmUsers < ActiveRecord::Migration[6.0] + def change + create_table :allowed_pm_users do |t| + t.integer :user_id, null: false + t.integer :allowed_pm_user_id, null: false + t.timestamps null: false + end + + add_index :allowed_pm_users, [:user_id, :allowed_pm_user_id], unique: true + add_index :allowed_pm_users, [:allowed_pm_user_id, :user_id], unique: true + end +end diff --git a/db/migrate/20200718154308_add_enable_allowed_pm_users_to_user_options.rb b/db/migrate/20200718154308_add_enable_allowed_pm_users_to_user_options.rb new file mode 100644 index 00000000000..c65d8a04975 --- /dev/null +++ b/db/migrate/20200718154308_add_enable_allowed_pm_users_to_user_options.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddEnableAllowedPmUsersToUserOptions < ActiveRecord::Migration[6.0] + def change + add_column :user_options, :enable_allowed_pm_users, :boolean, default: false, null: false + end +end diff --git a/lib/post_creator.rb b/lib/post_creator.rb index b5b13a2961c..9e9441982cf 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -110,7 +110,7 @@ class PostCreator return false end - # Make sure none of the users have muted the creator + # Make sure none of the users have muted or ignored the creator users = User.where(username_lower: names).pluck(:id, :username).to_h User @@ -128,6 +128,23 @@ class PostCreator 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 + end + return false if errors[:base].present? end @@ -430,6 +447,17 @@ 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/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 27fba7c4861..e05fa1397ad 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -1321,6 +1321,141 @@ describe PostCreator do end + context "private message to user in allow list" do + fab!(:sender) { Fabricate(:evil_trout) } + fab!(:allowed_user) { Fabricate(:user) } + + context "when post author is allowed" do + let!(:allowed_pm_user) { Fabricate(:allowed_pm_user, user: allowed_user, allowed_pm_user: sender) } + + it 'should succeed' do + allowed_user.user_option.update!(enable_allowed_pm_users: true) + + pc = PostCreator.new( + sender, + title: 'this message is to someone who is in my allow list!', + raw: "you will have to see this because I'm in your allow list!", + archetype: Archetype.private_message, + target_usernames: "#{allowed_user.username}" + ) + + expect(pc).to be_valid + expect(pc.errors).to be_blank + end + end + + context "when personal messages are disabled" do + let!(:allowed_pm_user) { Fabricate(:allowed_pm_user, user: allowed_user, allowed_pm_user: sender) } + + it 'should fail' do + allowed_user.user_option.update!(allow_private_messages: false) + allowed_user.user_option.update!(enable_allowed_pm_users: true) + + pc = PostCreator.new( + sender, + title: 'this message is to someone who is in my allow list!', + raw: "you will have to see this because I'm in your allow list!", + archetype: Archetype.private_message, + target_usernames: "#{allowed_user.username}" + ) + + expect(pc).not_to be_valid + expect(pc.errors.full_messages).to contain_exactly( + I18n.t(:not_accepting_pms, username: allowed_user.username) + ) + end + end + end + + context "private message to user not in allow list" do + fab!(:sender) { Fabricate(:evil_trout) } + fab!(:allowed_user) { Fabricate(:user) } + fab!(:not_allowed_user) { Fabricate(:user) } + + context "when post author is not allowed" do + let!(:allowed_pm_user) { Fabricate(:allowed_pm_user, user: not_allowed_user, allowed_pm_user: allowed_user) } + + it 'should fail' do + not_allowed_user.user_option.update!(enable_allowed_pm_users: true) + + pc = PostCreator.new( + sender, + title: 'this message is to someone who is not in my allowed list!', + raw: "you will have to see this even if you don't want message from me!", + archetype: Archetype.private_message, + target_usernames: "#{not_allowed_user.username}" + ) + + expect(pc).not_to be_valid + expect(pc.errors.full_messages).to contain_exactly( + I18n.t(:not_accepting_pms, username: not_allowed_user.username) + ) + end + + it 'should succeed when not enabled' do + not_allowed_user.user_option.update!(enable_allowed_pm_users: false) + + pc = PostCreator.new( + sender, + title: 'this message is to someone who is not in my allowed list!', + raw: "you will have to see this even if you don't want message from me!", + archetype: Archetype.private_message, + target_usernames: "#{not_allowed_user.username}" + ) + + expect(pc).to be_valid + expect(pc.errors).to be_blank + end + end + end + + context "private message when post author is admin who is not in allow list" do + fab!(:staff_user) { Fabricate(:admin) } + fab!(:allowed_user) { Fabricate(:user) } + fab!(:not_allowed_user) { Fabricate(:user) } + fab!(:allowed_pm_user) { Fabricate(:allowed_pm_user, user: staff_user, allowed_pm_user: allowed_user) } + + it 'succeeds if the user is staff' do + pc = PostCreator.new( + staff_user, + title: 'this message is to someone who did not allow me!', + raw: "you will have to see this even if you did not allow me!", + archetype: Archetype.private_message, + target_usernames: "#{not_allowed_user.username}" + ) + expect(pc).to be_valid + expect(pc.errors).to be_blank + end + end + + context "private message to multiple users and one is not allowed" do + fab!(:sender) { Fabricate(:evil_trout) } + fab!(:allowed_user) { Fabricate(:user) } + fab!(:not_allowed_user) { Fabricate(:user) } + + context "when post author is not allowed" do + let!(:allowed_pm_user) { Fabricate(:allowed_pm_user, user: allowed_user, allowed_pm_user: sender) } + + it 'should fail' do + allowed_user.user_option.update!(enable_allowed_pm_users: true) + not_allowed_user.user_option.update!(enable_allowed_pm_users: true) + + pc = PostCreator.new( + sender, + title: 'this message is to someone who is not in my allowed list!', + raw: "you will have to see this even if you don't want message from me!", + archetype: Archetype.private_message, + target_usernames: "#{allowed_user.username},#{not_allowed_user.username}" + ) + + expect(pc).not_to be_valid + expect(pc.errors.full_messages).to contain_exactly( + I18n.t(:not_accepting_pms, username: not_allowed_user.username) + ) + end + end + end + context "private message recipients limit (max_allowed_message_recipients) reached" do fab!(:target_user1) { Fabricate(:coding_horror) } fab!(:target_user2) { Fabricate(:evil_trout) } diff --git a/spec/fabricators/allowed_pm_users.rb b/spec/fabricators/allowed_pm_users.rb new file mode 100644 index 00000000000..c160aef83dd --- /dev/null +++ b/spec/fabricators/allowed_pm_users.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +Fabricator(:allowed_pm_user) do + user +end diff --git a/spec/serializers/web_hook_user_serializer_spec.rb b/spec/serializers/web_hook_user_serializer_spec.rb index 0059639054f..0d32e1a2076 100644 --- a/spec/serializers/web_hook_user_serializer_spec.rb +++ b/spec/serializers/web_hook_user_serializer_spec.rb @@ -23,7 +23,7 @@ RSpec.describe WebHookUserSerializer do it 'should only include the required keys' do count = serializer.as_json.keys.count - difference = count - 46 + difference = count - 47 expect(difference).to eq(0), lambda { message = (difference < 0 ?