Merge pull request #2701 from riking/email-pr-ssl

Rename POP3 settings, fix multisite SSL state leak

app/jobs/scheduled/poll_mailbox.rb

@@ -8,14 +8,14 @@ require_dependency 'email/message_builder'
 
 module Jobs
   class PollMailbox < Jobs::Scheduled
-    every SiteSetting.pop3s_polling_period_mins.minutes
+    every SiteSetting.pop3_polling_period_mins.minutes
     sidekiq_options retry: false
     include Email::BuildEmailHelper
 
     def execute(args)
       @args = args
-      if SiteSetting.pop3s_polling_enabled?
+      if SiteSetting.pop3_polling_enabled?
-        poll_pop3s
+        poll_pop3
       end
     end
 
@@ -72,14 +72,11 @@ module Jobs
       end
     end
 
-    def poll_pop3s
+    def poll_pop3
-      if !SiteSetting.pop3s_polling_insecure
+      connection = Net::POP3.new(SiteSetting.pop3_polling_host, SiteSetting.pop3_polling_port)
-        Net::POP3.enable_ssl(OpenSSL::SSL::VERIFY_NONE)
+      connection.enable_ssl if SiteSetting.pop3_polling_ssl
-      end
+
-      Net::POP3.start(SiteSetting.pop3s_polling_host,
+      connection.start(SiteSetting.pop3_polling_username, SiteSetting.pop3_polling_password) do |pop|
-                      SiteSetting.pop3s_polling_port,
-                      SiteSetting.pop3s_polling_username,
-                      SiteSetting.pop3s_polling_password) do |pop|
         unless pop.mails.empty?
           pop.each do |mail|
             handle_mail(mail)

config/application.rb

@@ -91,7 +91,7 @@ module Discourse
     # Configure sensitive parameters which will be filtered from the log file.
     config.filter_parameters += [
         :password,
-        :pop3s_polling_password,
+        :pop3_polling_password,
         :s3_secret_access_key,
         :twitter_consumer_secret,
         :facebook_app_secret,

config/locales/server.en.yml

@@ -926,13 +926,13 @@ en:
 
     disable_emails: "Prevent Discourse from sending any kind of emails"
 
-    pop3s_polling_enabled: "Poll via POP3S for email replies."
+    pop3_polling_enabled: "Poll via POP3 for email replies."
-    pop3s_polling_insecure: "Poll using plain text POP3 without SSL."
+    pop3_polling_ssl: "Use SSL while connecting to the POP3 server. (Recommended)"
-    pop3s_polling_period_mins: "The period in minutes between checking the POP3S account for email. NOTE: requires restart."
+    pop3_polling_period_mins: "The period in minutes between checking the POP3 account for email. NOTE: requires restart."
-    pop3s_polling_port: "The port to poll a POP3S account on."
+    pop3_polling_port: "The port to poll a POP3 account on."
-    pop3s_polling_host: "The host to poll for email via POP3S."
+    pop3_polling_host: "The host to poll for email via POP3."
-    pop3s_polling_username: "The username for the POP3S account to poll for email."
+    pop3_polling_username: "The username for the POP3 account to poll for email."
-    pop3s_polling_password: "The password for the POP3S account to poll for email."
+    pop3_polling_password: "The password for the POP3 account to poll for email."
     email_in: "Allow users to post new topics via email (requires pop3 polling). Configure the addresses in the \"Settings\" tab of each category."
     email_in_min_trust: "The minimum trust level a user needs to have to be allowed to post new topics via email."
     email_prefix: "The [label] used in the subject of emails. It will default to 'title' if not set."

config/site_settings.yml

@@ -385,13 +385,13 @@ email:
   email_custom_headers: 'Auto-Submitted: auto-generated'
   reply_by_email_enabled: false
   reply_by_email_address: ''
-  pop3s_polling_enabled: false
+  pop3_polling_enabled: false
-  pop3s_polling_insecure: false
+  pop3_polling_ssl: true
-  pop3s_polling_period_mins: 5
+  pop3_polling_period_mins: 5
-  pop3s_polling_host: ''
+  pop3_polling_host: ''
-  pop3s_polling_port: 995
+  pop3_polling_port: 995
-  pop3s_polling_username: ''
+  pop3_polling_username: ''
-  pop3s_polling_password: ''
+  pop3_polling_password: ''
   email_in:
     default: false
     client: true

db/migrate/20140826234625_rename_settings_pop3s_to_pop3.rb

@@ -0,0 +1,9 @@
+class RenameSettingsPop3sToPop3 < ActiveRecord::Migration
+  def up
+    execute "UPDATE site_settings SET name = replace(name, 'pop3s', 'pop3') WHERE name ILIKE 'pop3%'"
+  end
+
+  def down
+    execute "UPDATE site_settings SET name = replace(name, 'pop3', 'pop3s') WHERE name ILIKE 'pop3%'"
+  end
+end

spec/jobs/poll_mailbox_spec.rb

@@ -7,18 +7,18 @@ describe Jobs::PollMailbox do
 
   describe ".execute" do
 
-    it "does no polling if pop3s_polling_enabled is false" do
+    it "does no polling if pop3_polling_enabled is false" do
-      SiteSetting.expects(:pop3s_polling_enabled?).returns(false)
+      SiteSetting.expects(:pop3_polling_enabled?).returns(false)
-      poller.expects(:poll_pop3s).never
+      poller.expects(:poll_pop3).never
 
       poller.execute({})
     end
 
-    describe "with pop3s_polling_enabled" do
+    describe "with pop3_polling_enabled" do
 
-      it "calls poll_pop3s" do
+      it "calls poll_pop3" do
-        SiteSetting.expects(:pop3s_polling_enabled?).returns(true)
+        SiteSetting.expects(:pop3_polling_enabled?).returns(true)
-        poller.expects(:poll_pop3s).once
+        poller.expects(:poll_pop3).once
 
         poller.execute({})
       end
@@ -26,19 +26,34 @@ describe Jobs::PollMailbox do
 
   end
 
-  describe ".poll_pop3s" do
+  describe ".poll_pop3" do
 
     it "logs an error on pop authentication error" do
       error = Net::POPAuthenticationError.new
       data = { limit_once_per: 1.hour, message_params: { error: error }}
 
-      Net::POP3.expects(:start).raises(error)
+      Net::POP3.any_instance.expects(:start).raises(error)
 
       Discourse.expects(:handle_exception)
 
-      poller.poll_pop3s
+      poller.poll_pop3
     end
 
+    it "calls enable_ssl when the setting is enabled" do
+      SiteSetting.pop3_polling_ssl = true
+      Net::POP3.any_instance.stubs(:start)
+      Net::POP3.any_instance.expects(:enable_ssl)
+
+      poller.poll_pop3
+    end
+
+    it "does not call enable_ssl when the setting is off" do
+      SiteSetting.pop3_polling_ssl = false
+      Net::POP3.any_instance.stubs(:start)
+      Net::POP3.any_instance.expects(:enable_ssl).never
+
+      poller.poll_pop3
+    end
   end
 
   # Testing mock for the email objects that you get