FIX: delete staged users when the incoming email is rejected

app/services/user_destroyer.rb

@@ -80,7 +80,10 @@ class UserDestroyer
             end
           end
 
+          unless opts[:quiet]
             StaffActionLogger.new(@actor == user ? Discourse.system_user : @actor).log_user_deletion(user, opts.slice(:context))
+          end
+
           MessageBus.publish "/file-change", ["refresh"], user_ids: [u.id]
         end
       end

lib/email/receiver.rb

@@ -58,6 +58,7 @@ module Email
           error = e.to_s
           error = e.class.name if error.blank?
           @incoming_email.update_columns(error: error) if @incoming_email
+          delete_staged_users
           raise
         end
       end
@@ -752,6 +753,12 @@ module Email
       message = SubscriptionMailer.send(action, user)
       Email::Sender.new(message, :subscription).send
     end
+
+    def delete_staged_users
+      @staged_users.each do |user|
+        UserDestroyer.new(Discourse.system_user).destroy(user, quiet: true) if user.posts.count == 0
+      end
+    end
   end
 
 end

spec/components/email/receiver_spec.rb

@@ -648,10 +648,10 @@ describe Email::Receiver do
       SiteSetting.enable_staged_users = true
     end
 
-    shared_examples "no staged users" do |email_name|
+    shared_examples "no staged users" do |email_name, expected_exception|
       it "does not create staged users" do
         staged_user_count = User.where(staged: true).count
-        process(email_name) rescue nil
+        expect { process(email_name) }.to raise_error(expected_exception)
         expect(User.where(staged: true).count).to eq(staged_user_count)
       end
     end
@@ -661,23 +661,23 @@ describe Email::Receiver do
         ScreenedEmail.expects(:should_block?).with("screened@mail.com").returns(true)
       end
 
-      include_examples "no staged users", :screened_email
+      include_examples "no staged users", :screened_email, Email::Receiver::ScreenedEmailError
     end
 
     context "when the mail is auto generated" do
-      include_examples "no staged users", :auto_generated_header
+      include_examples "no staged users", :auto_generated_header, Email::Receiver::AutoGeneratedEmailError
     end
 
     context "when email is a bounced email" do
-      include_examples "no staged users", :bounced_email
+      include_examples "no staged users", :bounced_email, Email::Receiver::BouncedEmailError
     end
 
     context "when the body is blank" do
-      include_examples "no staged users", :no_body
+      include_examples "no staged users", :no_body, Email::Receiver::NoBodyDetectedError
     end
 
     context "when unsubscribe via email is not allowed" do
-      include_examples "no staged users", :unsubscribe_new_user
+      include_examples "no staged users", :unsubscribe_new_user, Email::Receiver::UnsubscribeNotAllowed
     end
 
     context "when email address is not on whitelist" do
@@ -685,7 +685,7 @@ describe Email::Receiver do
         SiteSetting.email_domains_whitelist = "example.com|bar.com"
       end
 
-      include_examples "no staged users", :blacklist_whitelist_email
+      include_examples "no staged users", :blacklist_whitelist_email, Email::Receiver::EmailNotAllowed
     end
 
     context "when email address is on blacklist" do
@@ -693,7 +693,71 @@ describe Email::Receiver do
         SiteSetting.email_domains_blacklist = "email.com|mail.com"
       end
 
-      include_examples "no staged users", :blacklist_whitelist_email
+      include_examples "no staged users", :blacklist_whitelist_email, Email::Receiver::EmailNotAllowed
+    end
+
+    context "when destinations aren't matching any of the incoming emails" do
+      include_examples "no staged users", :bad_destinations, Email::Receiver::BadDestinationAddress
+    end
+
+    context "when email is sent to category" do
+      context "when email is sent by a new user and category does not allow strangers" do
+        let!(:category) { Fabricate(:category, email_in: "category@foo.com", email_in_allow_strangers: false) }
+
+        include_examples "no staged users", :new_user, Email::Receiver::StrangersNotAllowedError
+      end
+
+      context "when email has no date" do
+        let!(:category) { Fabricate(:category, email_in: "category@foo.com", email_in_allow_strangers: true) }
+
+        include_examples "no staged users", :no_date, Email::Receiver::InvalidPost
+      end
+    end
+
+    context "email is a reply" do
+      let(:reply_key) { "4f97315cc828096c9cb34c6f1a0d6fe8" }
+      let(:category) { Fabricate(:category) }
+      let(:user) { Fabricate(:user, email: "discourse@bar.com") }
+      let(:topic) { create_topic(category: category, user: user) }
+      let(:post) { create_post(topic: topic, user: user) }
+      let!(:email_log) { Fabricate(:email_log, reply_key: reply_key, user: user, topic: topic, post: post) }
+
+      context "when the email address isn't matching the one we sent the notification to" do
+        include_examples "no staged users", :reply_user_not_matching, Email::Receiver::ReplyUserNotMatchingError
+      end
+    end
+
+    context "replying without key is allowed" do
+      let!(:group) { Fabricate(:group, incoming_email: "team@bar.com") }
+      let!(:topic) do
+        SiteSetting.find_related_post_with_key = false
+        process(:email_reply_1)
+        Topic.last
+      end
+
+      context "when the topic was deleted" do
+        before do
+          topic.update_columns(deleted_at: 1.day.ago)
+        end
+
+        include_examples "no staged users", :email_reply_staged, Email::Receiver::TopicNotFoundError
+      end
+
+      context "when the topic was closed" do
+        before do
+          topic.update_columns(closed: true)
+        end
+
+        include_examples "no staged users", :email_reply_staged, Email::Receiver::TopicClosedError
+      end
+
+      context "when they aren't allowed to like a post" do
+        before do
+          topic.update_columns(archived: true)
+        end
+
+        include_examples "no staged users", :email_reply_like, Email::Receiver::InvalidPostAction
+      end
     end
 
   end

spec/fixtures/emails/email_reply_like.eml

@@ -0,0 +1,13 @@
+Return-Path: <four@foo.com>
+From: Four <four@foo.com>
+To: one@foo.com
+Cc: team@bar.com
+Subject: RE: Testing email threading
+Date: Fri, 15 Jan 2016 00:12:43 +0100
+Message-ID: <38@foo.bar.mail>
+In-Reply-To: <34@foo.bar.mail>
+Mime-Version: 1.0
+Content-Type: text/plain
+Content-Transfer-Encoding: 7bit
+
++1

spec/fixtures/emails/email_reply_staged.eml

@@ -0,0 +1,13 @@
+Return-Path: <staged@foo.com>
+From: Staged <staged@foo.com>
+To: one@foo.com
+Cc: team@bar.com, three@foo.com
+Subject: RE: Testing email threading
+Date: Fri, 15 Jan 2016 00:12:43 +0100
+Message-ID: <35@foo.bar.mail>
+In-Reply-To: <34@foo.bar.mail>
+Mime-Version: 1.0
+Content-Type: text/plain
+Content-Transfer-Encoding: 7bit
+
+This is email reply **2**.

spec/fixtures/emails/no_date.eml

@@ -0,0 +1,10 @@
+Return-Path: <discourse@bar.com>
+From: Foo Bar <discourse@bar.com>
+To: category@foo.com
+Subject: This is a topic from a complete stranger
+Message-ID: <31@foo.bar.mail>
+Mime-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: quoted-printable
+
+Hey, this is a topic from a complete stranger ;)

spec/services/user_destroyer_spec.rb

@@ -45,6 +45,12 @@ describe UserDestroyer do
         StaffActionLogger.any_instance.expects(:log_user_deletion).with(@user, anything).once
         destroy
       end
+
+      it "should not log the action if quiet is true" do
+        expect {
+          UserDestroyer.new(@admin).destroy(@user, destroy_opts.merge(quiet: true))
+        }.to_not change { UserHistory.where(action: UserHistory.actions[:delete_user]).count }
+      end
     end
 
     shared_examples "email block list" do
@@ -202,6 +208,7 @@ describe UserDestroyer do
         # out of sync user_stat data shouldn't break UserDestroyer
         @user.user_stat.update_attribute(:post_count, 1)
       end
+      let(:destroy_opts) { {} }
       subject(:destroy) { UserDestroyer.new(@user).destroy(@user, delete_posts: false) }
 
       include_examples "successfully destroy a user"