From 6d357c9c233f7bea9be489078a9a8005aa7a8884 Mon Sep 17 00:00:00 2001 From: riking Date: Tue, 26 Aug 2014 16:52:35 -0700 Subject: [PATCH 1/3] Rename pop3s settings to pop3, remove 'insecure' --- app/jobs/scheduled/poll_mailbox.rb | 19 ++++++++----------- config/application.rb | 2 +- config/site_settings.yml | 13 ++++++------- ...826234625_rename_settings_pop3s_to_pop3.rb | 9 +++++++++ spec/jobs/poll_mailbox_spec.rb | 18 +++++++++--------- 5 files changed, 33 insertions(+), 28 deletions(-) create mode 100644 db/migrate/20140826234625_rename_settings_pop3s_to_pop3.rb diff --git a/app/jobs/scheduled/poll_mailbox.rb b/app/jobs/scheduled/poll_mailbox.rb index 7faf2cfb95d..8d86043c5ce 100644 --- a/app/jobs/scheduled/poll_mailbox.rb +++ b/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? - poll_pop3s + if SiteSetting.pop3_polling_enabled? + poll_pop3 end end @@ -72,14 +72,11 @@ module Jobs end end - def poll_pop3s - if !SiteSetting.pop3s_polling_insecure - Net::POP3.enable_ssl(OpenSSL::SSL::VERIFY_NONE) - end - Net::POP3.start(SiteSetting.pop3s_polling_host, - SiteSetting.pop3s_polling_port, - SiteSetting.pop3s_polling_username, - SiteSetting.pop3s_polling_password) do |pop| + def poll_pop3 + Net::POP3.start(SiteSetting.pop3_polling_host, + SiteSetting.pop3_polling_port, + SiteSetting.pop3_polling_username, + SiteSetting.pop3_polling_password) do |pop| unless pop.mails.empty? pop.each do |mail| handle_mail(mail) diff --git a/config/application.rb b/config/application.rb index f290bd5c9e3..a7326669871 100644 --- a/config/application.rb +++ b/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, diff --git a/config/site_settings.yml b/config/site_settings.yml index 72a13e9c6fa..623e48e60a0 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -385,13 +385,12 @@ email: email_custom_headers: 'Auto-Submitted: auto-generated' reply_by_email_enabled: false reply_by_email_address: '' - pop3s_polling_enabled: false - pop3s_polling_insecure: false - pop3s_polling_period_mins: 5 - pop3s_polling_host: '' - pop3s_polling_port: 995 - pop3s_polling_username: '' - pop3s_polling_password: '' + pop3_polling_enabled: false + pop3_polling_period_mins: 5 + pop3_polling_host: '' + pop3_polling_port: 995 + pop3_polling_username: '' + pop3_polling_password: '' email_in: default: false client: true diff --git a/db/migrate/20140826234625_rename_settings_pop3s_to_pop3.rb b/db/migrate/20140826234625_rename_settings_pop3s_to_pop3.rb new file mode 100644 index 00000000000..0672e5168fd --- /dev/null +++ b/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 diff --git a/spec/jobs/poll_mailbox_spec.rb b/spec/jobs/poll_mailbox_spec.rb index b10dcc09ede..3ba94945f11 100644 --- a/spec/jobs/poll_mailbox_spec.rb +++ b/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 - SiteSetting.expects(:pop3s_polling_enabled?).returns(false) - poller.expects(:poll_pop3s).never + it "does no polling if pop3_polling_enabled is false" do + SiteSetting.expects(:pop3_polling_enabled?).returns(false) + 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 - SiteSetting.expects(:pop3s_polling_enabled?).returns(true) - poller.expects(:poll_pop3s).once + it "calls poll_pop3" do + SiteSetting.expects(:pop3_polling_enabled?).returns(true) + poller.expects(:poll_pop3).once poller.execute({}) end @@ -26,7 +26,7 @@ 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 @@ -36,7 +36,7 @@ describe Jobs::PollMailbox do Discourse.expects(:handle_exception) - poller.poll_pop3s + poller.poll_pop3 end end From e28ef099a4f9213a87e5fb99fccc74629958cad1 Mon Sep 17 00:00:00 2001 From: riking Date: Tue, 26 Aug 2014 17:00:27 -0700 Subject: [PATCH 2/3] Fix pop3 SSL state leaking over multisite --- app/jobs/scheduled/poll_mailbox.rb | 8 ++++---- config/site_settings.yml | 1 + spec/jobs/poll_mailbox_spec.rb | 17 ++++++++++++++++- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/app/jobs/scheduled/poll_mailbox.rb b/app/jobs/scheduled/poll_mailbox.rb index 8d86043c5ce..6226461f10f 100644 --- a/app/jobs/scheduled/poll_mailbox.rb +++ b/app/jobs/scheduled/poll_mailbox.rb @@ -73,10 +73,10 @@ module Jobs end def poll_pop3 - Net::POP3.start(SiteSetting.pop3_polling_host, - SiteSetting.pop3_polling_port, - SiteSetting.pop3_polling_username, - SiteSetting.pop3_polling_password) do |pop| + connection = Net::POP3.new(SiteSetting.pop3_polling_host, SiteSetting.pop3_polling_port) + connection.enable_ssl if SiteSetting.pop3_polling_ssl + + connection.start(SiteSetting.pop3_polling_username, SiteSetting.pop3_polling_password) do |pop| unless pop.mails.empty? pop.each do |mail| handle_mail(mail) diff --git a/config/site_settings.yml b/config/site_settings.yml index 623e48e60a0..0ec749d6c6d 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -386,6 +386,7 @@ email: reply_by_email_enabled: false reply_by_email_address: '' pop3_polling_enabled: false + pop3_polling_ssl: true pop3_polling_period_mins: 5 pop3_polling_host: '' pop3_polling_port: 995 diff --git a/spec/jobs/poll_mailbox_spec.rb b/spec/jobs/poll_mailbox_spec.rb index 3ba94945f11..9b365a40495 100644 --- a/spec/jobs/poll_mailbox_spec.rb +++ b/spec/jobs/poll_mailbox_spec.rb @@ -32,13 +32,28 @@ describe Jobs::PollMailbox 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_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 From 9090df63baf0897085b6a6a008e132a07d287863 Mon Sep 17 00:00:00 2001 From: riking Date: Thu, 28 Aug 2014 10:45:40 -0700 Subject: [PATCH 3/3] Fix pop3 settings in locale file --- config/locales/server.en.yml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 838f0784980..be823fc7c54 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -923,13 +923,13 @@ en: disable_emails: "Prevent Discourse from sending any kind of emails" - pop3s_polling_enabled: "Poll via POP3S for email replies." - pop3s_polling_insecure: "Poll using plain text POP3 without SSL." - pop3s_polling_period_mins: "The period in minutes between checking the POP3S account for email. NOTE: requires restart." - pop3s_polling_port: "The port to poll a POP3S account on." - pop3s_polling_host: "The host to poll for email via POP3S." - pop3s_polling_username: "The username for the POP3S account to poll for email." - pop3s_polling_password: "The password for the POP3S account to poll for email." + pop3_polling_enabled: "Poll via POP3 for email replies." + pop3_polling_ssl: "Use SSL while connecting to the POP3 server. (Recommended)" + pop3_polling_period_mins: "The period in minutes between checking the POP3 account for email. NOTE: requires restart." + pop3_polling_port: "The port to poll a POP3 account on." + pop3_polling_host: "The host to poll for email via POP3." + pop3_polling_username: "The username for the POP3 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."