From 9f9825bb6b9bdb7322c4f06ff5a49eb34a24be34 Mon Sep 17 00:00:00 2001 From: Luke Granger-Brown Date: Mon, 11 May 2015 00:10:10 +0100 Subject: [PATCH] FIX: don't send emails to anonymous users Also changes behaviour of real to not return anonymous users. This means user counts will no longer include them, and the mailing list system will ignore them even if they somehow end up with the feature turned on. --- app/jobs/regular/user_email.rb | 1 + app/models/user.rb | 11 +++++- config/locales/server.en.yml | 1 + spec/fabricators/user_fabricator.rb | 13 +++++++ .../notify_mailing_list_subscribers_spec.rb | 10 +++++ spec/jobs/user_email_spec.rb | 39 +++++++++++++++++++ 6 files changed, 73 insertions(+), 2 deletions(-) diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index 67f299eabd..0ef4811ea1 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -16,6 +16,7 @@ module Jobs # Find the user @user = User.find_by(id: args[:user_id]) return skip(I18n.t("email_log.no_user", user_id: args[:user_id])) unless @user + return skip(I18n.t("email_log.anonymous_user")) if @user.anonymous? return skip(I18n.t("email_log.suspended_not_pm")) if @user.suspended? && args[:type] != :user_private_message seen_recently = (@user.last_seen_at.present? && @user.last_seen_at > SiteSetting.email_time_window_mins.minutes.ago) diff --git a/app/models/user.rb b/app/models/user.rb index e9f1ebbb85..0874f3cf67 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -108,8 +108,15 @@ class User < ActiveRecord::Base # set to true to optimize creation and save for imports attr_accessor :import_mode - # excluding fake users like the system user - scope :real, -> { where('id > 0') } + # excluding fake users like the system user or anonymous users + scope :real, -> { where('id > 0').where('NOT EXISTS( + SELECT 1 + FROM user_custom_fields ucf + WHERE + ucf.user_id = users.id AND + ucf.name = ? AND + ucf.value::int > 0 + )', 'master_id') } scope :staff, -> { where("admin OR moderator") } diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 76c2b5ffe1..252cc789af 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2006,6 +2006,7 @@ en: email_log: no_user: "Can't find user with id %{user_id}" + anonymous_user: "User is anonymous" suspended_not_pm: "User is suspended, not a message" seen_recently: "User was seen recently" post_not_found: "Can't find a post with id %{post_id}" diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb index de200ec4c2..13e0ce61c6 100644 --- a/spec/fabricators/user_fabricator.rb +++ b/spec/fabricators/user_fabricator.rb @@ -86,3 +86,16 @@ Fabricator(:trust_level_4, from: :user) do email { sequence(:email) { |i| "tl4#{i}@elderfun.com" } } trust_level TrustLevel[4] end + +Fabricator(:anonymous, from: :user) do + name '' + username { sequence(:username) { |i| "anonymous#{i}" } } + email { sequence(:email) { |i| "anonymous#{i}@anonymous.com" } } + trust_level TrustLevel[1] + trust_level_locked true + + after_create do |user| + user.custom_fields["master_id"] = 1 + user.save! + end +end diff --git a/spec/jobs/notify_mailing_list_subscribers_spec.rb b/spec/jobs/notify_mailing_list_subscribers_spec.rb index c7b9923b3c..bd70b7d3d9 100644 --- a/spec/jobs/notify_mailing_list_subscribers_spec.rb +++ b/spec/jobs/notify_mailing_list_subscribers_spec.rb @@ -47,6 +47,16 @@ describe Jobs::NotifyMailingListSubscribers do end + context "to an anonymous user with mailing list on" do + let(:user) { Fabricate(:anonymous, mailing_list_mode: true) } + let!(:post) { Fabricate(:post, user: user) } + + it "doesn't send the email to the user" do + UserNotifications.expects(:mailing_list_notify).with(user, post).never + Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) + end + end + context "with mailing list off" do let(:user) { Fabricate(:user, mailing_list_mode: false) } let!(:post) { Fabricate(:post, user: user) } diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index a36fff4018..d9ac680445 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -9,6 +9,7 @@ describe Jobs::UserEmail do let(:user) { Fabricate(:user, last_seen_at: 11.minutes.ago ) } let(:suspended) { Fabricate(:user, last_seen_at: 10.minutes.ago, suspended_at: 5.minutes.ago, suspended_till: 7.days.from_now ) } + let(:anonymous) { Fabricate(:anonymous, last_seen_at: 11.minutes.ago ) } let(:mailer) { Mail::Message.new(to: user.email) } it "raises an error when there is no user" do @@ -96,6 +97,22 @@ describe Jobs::UserEmail do Jobs::UserEmail.new.execute(type: :private_message, user_id: suspended.id, post_id: pm_from_staff.id) end end + + context 'user is anonymous' do + before { SiteSetting.stubs(:allow_anonymous_posting).returns(true) } + + it "doesn't send email for a pm from a regular user" do + Email::Sender.any_instance.expects(:send).never + Jobs::UserEmail.new.execute(type: :private_message, user_id: anonymous.id, post_id: post.id) + end + + it "doesn't send email for a pm from a staff user" do + pm_from_staff = Fabricate(:post, user: Fabricate(:moderator)) + pm_from_staff.topic.topic_allowed_users.create!(user_id: anonymous.id) + Email::Sender.any_instance.expects(:send).never + Jobs::UserEmail.new.execute(type: :private_message, user_id: anonymous.id, post_id: pm_from_staff.id) + end + end end @@ -169,6 +186,28 @@ describe Jobs::UserEmail do end end end + + context 'user is anonymous' do + before { SiteSetting.stubs(:allow_anonymous_posting).returns(true) } + + it "doesn't send email for a pm from a regular user" do + Email::Sender.any_instance.expects(:send).never + Jobs::UserEmail.new.execute(type: :user_private_message, user_id: anonymous.id, notification_id: notification.id) + end + + it "doesn't send email for a pm from staff" do + pm_from_staff = Fabricate(:post, user: Fabricate(:moderator)) + pm_from_staff.topic.topic_allowed_users.create!(user_id: anonymous.id) + pm_notification = Fabricate(:notification, + user: anonymous, + topic: pm_from_staff.topic, + post_number: pm_from_staff.post_number, + data: { original_post_id: pm_from_staff.id }.to_json + ) + Email::Sender.any_instance.expects(:send).never + Jobs::UserEmail.new.execute(type: :user_private_message, user_id: anonymous.id, notification_id: pm_notification.id) + end + end end end