From d67fe4c674aaa40a995545c3082cd913b347d9db Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Tue, 25 Jul 2017 17:44:46 +0200
Subject: [PATCH] FIX: block all emails associated to a user when destroying
 their record

---
 app/models/user.rb                   |  4 +--
 app/services/user_destroyer.rb       | 48 +++++++++++++---------------
 spec/services/user_destroyer_spec.rb | 16 +++-------
 3 files changed, 28 insertions(+), 40 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 695f2f0b87d..f6af5defef8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -44,9 +44,7 @@ class User < ActiveRecord::Base
   has_many :user_auth_tokens, dependent: :destroy
   has_many :user_emails, dependent: :destroy
 
-  has_one :primary_email, -> { where(primary: true)  },
-    dependent: :destroy,
-    class_name: 'UserEmail'
+  has_one :primary_email, -> { where(primary: true)  }, class_name: 'UserEmail', dependent: :destroy
 
   has_one :user_option, dependent: :destroy
   has_one :user_avatar, dependent: :destroy
diff --git a/app/services/user_destroyer.rb b/app/services/user_destroyer.rb
index 9af5731fc91..838c2ba59e4 100644
--- a/app/services/user_destroyer.rb
+++ b/app/services/user_destroyer.rb
@@ -31,18 +31,16 @@ class UserDestroyer
           # block all external urls
           if opts[:block_urls]
             post.topic_links.each do |link|
-              unless link.internal ||
-                (Oneboxer.engine(link.url) != Onebox::Engine::WhitelistedGenericOnebox)
-
-                ScreenedUrl.watch(link.url, link.domain, ip_address: user.ip_address)&.record_match!
-              end
+              next if link.internal
+              next if Oneboxer.engine(link.url) != Onebox::Engine::WhitelistedGenericOnebox
+              ScreenedUrl.watch(link.url, link.domain, ip_address: user.ip_address)&.record_match!
             end
           end
 
           PostDestroyer.new(@actor.staff? ? @actor : Discourse.system_user, post).destroy
 
           if post.topic && post.is_first_post?
-            Topic.unscoped.where(id: post.topic.id).update_all(user_id: nil)
+            Topic.unscoped.where(id: post.topic_id).update_all(user_id: nil)
           end
         end
       end
@@ -51,39 +49,39 @@ class UserDestroyer
         post_action.remove_act!(Discourse.system_user)
       end
 
+      # keep track of emails used
+      user_emails = user.user_emails.pluck(:email)
+
       user.destroy.tap do |u|
         if u
-
           if opts[:block_email]
-            b = ScreenedEmail.block(u.email, ip_address: u.ip_address)
-            b.record_match! if b
-          end
-
-          if opts[:block_ip] && u.ip_address
-            b = ScreenedIpAddress.watch(u.ip_address)
-            b.record_match! if b
-            if u.registration_ip_address && u.ip_address != u.registration_ip_address
-              b = ScreenedIpAddress.watch(u.registration_ip_address)
-              b.record_match! if b
+            user_emails.each do |email|
+              ScreenedEmail.block(email, ip_address: u.ip_address)&.record_match!
             end
           end
 
-          Post.with_deleted.where(user_id: user.id).update_all("user_id = NULL")
+          if opts[:block_ip] && u.ip_address
+            ScreenedIpAddress.watch(u.ip_address)&.record_match!
+            if u.registration_ip_address && u.ip_address != u.registration_ip_address
+              ScreenedIpAddress.watch(u.registration_ip_address)&.record_match!
+            end
+          end
+
+          Post.unscoped.where(user_id: u.id).update_all(user_id: nil)
 
           # If this user created categories, fix those up:
-          categories = Category.where(user_id: user.id)
-          categories.each do |c|
-            c.user_id = Discourse.system_user.id
+          Category.where(user_id: u.id).each do |c|
+            c.user_id = Discourse::SYSTEM_USER_ID
             c.save!
-            if topic = Topic.with_deleted.find_by(id: c.topic_id)
-              topic.try(:recover!)
-              topic.user_id = Discourse.system_user.id
+            if topic = Topic.unscoped.find_by(id: c.topic_id)
+              topic.recover!
+              topic.user_id = Discourse::SYSTEM_USER_ID
               topic.save!
             end
           end
 
           StaffActionLogger.new(@actor == user ? Discourse.system_user : @actor).log_user_deletion(user, opts.slice(:context))
-          MessageBus.publish "/file-change", ["refresh"], user_ids: [user.id]
+          MessageBus.publish "/file-change", ["refresh"], user_ids: [u.id]
         end
       end
     end
diff --git a/spec/services/user_destroyer_spec.rb b/spec/services/user_destroyer_spec.rb
index 4d0a23043b8..76c08201e75 100644
--- a/spec/services/user_destroyer_spec.rb
+++ b/spec/services/user_destroyer_spec.rb
@@ -54,10 +54,9 @@ describe UserDestroyer do
       end
 
       it "adds email to block list if block_email is true" do
-        b = Fabricate.build(:screened_email, email: @user.email)
-        ScreenedEmail.expects(:block).with(@user.email, has_key(:ip_address)).returns(b)
-        b.expects(:record_match!).once.returns(true)
-        UserDestroyer.new(@admin).destroy(@user, destroy_opts.merge({block_email: true}))
+        expect {
+          UserDestroyer.new(@admin).destroy(@user, destroy_opts.merge(block_email: true))
+        }.to change { ScreenedEmail.count }.by(1)
       end
     end
 
@@ -228,15 +227,8 @@ describe UserDestroyer do
       context 'and destroy fails' do
         subject(:destroy) { UserDestroyer.new(@admin).destroy(@user) }
 
-        before do
-          @user.stubs(:destroy).returns(false)
-        end
-
-        it 'should return false' do
-          expect(destroy).to eq(false)
-        end
-
         it 'should not log the action' do
+          @user.stubs(:destroy).returns(false)
           StaffActionLogger.any_instance.expects(:log_user_deletion).never
           destroy
         end