diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 6e6ce06d043..20be48a6ba5 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -689,7 +689,7 @@ class TopicsController < ApplicationController render json: json, status: 422 end - rescue Topic::UserExists => e + rescue Topic::UserExists, Topic::NotAllowed => e render json: { errors: [e.message] }, status: 422 end end diff --git a/app/models/topic.rb b/app/models/topic.rb index 17fd50c6bef..eb156e6528f 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -2,6 +2,7 @@ class Topic < ActiveRecord::Base class UserExists < StandardError; end + class NotAllowed < StandardError; end include ActionView::Helpers::SanitizeHelper include RateLimiter::OnCreateRecord include HasCustomFields @@ -1020,7 +1021,11 @@ class Topic < ActiveRecord::Base end if invite_existing_muted?(target_user, invited_by) - return true + raise NotAllowed.new(I18n.t("topic_invite.not_allowed")) + end + + if !allowed_pm_user?(target_user, invited_by) + raise NotAllowed.new(I18n.t("topic_invite.not_allowed")) end if private_message? @@ -1055,6 +1060,43 @@ class Topic < ActiveRecord::Base false end + def allowed_pm_user?(target_user, invited_by) + return true if target_user.staff? + users = TopicAllowedUser.where(topic: self).pluck(:user_id) + users << target_user.id << invited_by.id + users.uniq + + users_with_allowed_pms = allowed_pms_enabled(users).pluck(:id).uniq + + if users_with_allowed_pms.any? + users_sender_can_pm = allowed_pms_enabled(users) + .where("allowed_pm_users.allowed_pm_user_id" => invited_by.id) + .pluck(:id).uniq + + return false unless users_sender_can_pm.include? target_user.id + + can_users_receive_from_sender = allowed_pms_enabled([invited_by.id]) + .where("allowed_pm_users.allowed_pm_user_id IN (:user_ids)", user_ids: users.delete(invited_by.id)) + .pluck(:id).uniq + + users_not_allowed = users_with_allowed_pms - users_sender_can_pm - can_users_receive_from_sender + return false if users_not_allowed.any? + end + + true + end + + def allowed_pms_enabled(user_ids) + 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: user_ids) + end + def email_already_exists_for?(invite) invite.email_already_exists && private_message? end diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 4293f4aeed5..3b252a71de6 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -231,11 +231,10 @@ class UserUpdater 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) + desired_ids = User.where(username: desired_usernames).pluck(:id) + if desired_ids.empty? AllowedPmUser.where(user_id: user.id).destroy_all else diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 715227fa4f6..0fdccece691 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2466,6 +2466,7 @@ en: success: "We've invited that user to participate in this message." success_group: "We've invited that group to participate in this message." error: "Sorry, there was an error inviting that user." + not_allowed: "Sorry, that user can't be invited." group_name: "group name" controls: "Topic Controls" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index d70b41b6519..9bf1ce3b970 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -244,6 +244,7 @@ en: topic_invite: failed_to_invite: "The user cannot be invited into this topic without a group membership in either one of the following groups: %{group_names}." user_exists: "Sorry, that user has already been invited. You may only invite a user to a topic once." + not_allowed: "Sorry, that user can't be invited." backup: operation_already_running: "An operation is currently running. Can't start a new job right now." diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index ccb5a31ce70..516c50968e5 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -683,8 +683,9 @@ describe Topic do context "from a muted user" do before { MutedUser.create!(user: another_user, muted_user: user) } - it 'silently fails' do - expect(topic.invite(user, another_user.username)).to eq(true) + it 'fails with an error message' do + expect { topic.invite(user, another_user.username) } + .to raise_error(Topic::NotAllowed) expect(topic.allowed_users).to_not include(another_user) expect(Post.last).to be_blank expect(Notification.last).to be_blank @@ -701,6 +702,49 @@ describe Topic do .to raise_error(Topic::UserExists) end end + + context "when invited_user has enabled allow_list" do + fab!(:user2) { Fabricate(:user) } + fab!(:admin) { Fabricate(:admin) } + fab!(:pm) { Fabricate(:private_message_topic, user: user, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: user), + Fabricate.build(:topic_allowed_user, user: user2) + ]) } + + it 'succeeds when inviter is in allowed list' do + AllowedPmUser.create!(user: another_user, allowed_pm_user: user) + expect(topic.invite(user, another_user.username)).to eq(true) + end + + it 'should raise error when inviter not in allowed list' do + another_user.user_option.update!(enable_allowed_pm_users: true) + AllowedPmUser.create!(user: another_user, allowed_pm_user: user2) + expect { topic.invite(user, another_user.username) } + .to raise_error(Topic::NotAllowed) + end + + it 'should succeed for staff even when not allowed' do + another_user.user_option.update!(enable_allowed_pm_users: true) + AllowedPmUser.create!(user: another_user, allowed_pm_user: user2) + expect(topic.invite(another_user, admin.username)).to eq(true) + end + + it 'should raise error when target_user is not in inviters allowed list' do + user.user_option.update!(enable_allowed_pm_users: true) + another_user.user_option.update!(enable_allowed_pm_users: true) + AllowedPmUser.create!(user: another_user, allowed_pm_user: user) + expect { topic.invite(user, another_user.username) } + .to raise_error(Topic::NotAllowed) + end + + it 'should raise error if target_user has not allowed any of the other participants' do + user2.user_option.update!(enable_allowed_pm_users: true) + AllowedPmUser.create!(user: user2, allowed_pm_user: user) + + expect { pm.invite(user, another_user.username) } + .to raise_error(Topic::NotAllowed) + end + end end describe 'by email' do @@ -820,8 +864,9 @@ describe Topic do context "for a muted topic" do before { TopicUser.change(another_user.id, topic.id, notification_level: TopicUser.notification_levels[:muted]) } - it 'silently fails' do - expect(topic.invite(user, another_user.username)).to eq(true) + it 'fails with an error message' do + expect { topic.invite(user, another_user.username) } + .to raise_error(Topic::NotAllowed) expect(topic.allowed_users).to_not include(another_user) expect(Post.last).to be_blank expect(Notification.last).to be_blank