From 5a3494b1e1aed999b1216d53e8a7f019ba300a34 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 4 Aug 2020 14:19:57 +1000 Subject: [PATCH] FIX: IMAP archive fix and group list mailbox code unification (#10355) * Fixed an issue I introduced in the last PR where I am just archiving everything regardless of whether it is actually archived in Discourse man_facepalming * Refactor group list_mailboxes IMAP code to use providers, add specs, and add provider code to get the correct prodivder --- app/models/group.rb | 27 ++++--- lib/demon/email_sync.rb | 2 +- lib/imap/providers/detector.rb | 14 ++++ lib/imap/providers/generic.rb | 7 +- lib/imap/providers/gmail.rb | 7 ++ lib/imap/sync.rb | 35 ++++----- spec/components/imap/sync_spec.rb | 90 +++++++++++++++++++++++- spec/lib/imap/providers/detector_spec.rb | 27 +++++++ spec/lib/imap/providers/gmail_spec.rb | 73 +++++++++++++++++++ spec/models/group_spec.rb | 74 +++++++++++++++++++ 10 files changed, 320 insertions(+), 36 deletions(-) create mode 100644 lib/imap/providers/detector.rb create mode 100644 spec/lib/imap/providers/detector_spec.rb create mode 100644 spec/lib/imap/providers/gmail_spec.rb diff --git a/app/models/group.rb b/app/models/group.rb index 7bbc04e46ca..ded3acbe37f 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -758,23 +758,24 @@ class Group < ActiveRecord::Base def imap_mailboxes return [] if self.imap_server.blank? || self.email_username.blank? || - self.email_password.blank? + self.email_password.blank? || + !SiteSetting.enable_imap Discourse.cache.fetch("group_imap_mailboxes_#{self.id}", expires_in: 30.minutes) do Rails.logger.info("[IMAP] Refreshing mailboxes list for group #{self.name}") mailboxes = [] begin - @imap = Net::IMAP.new(self.imap_server, self.imap_port, self.imap_ssl) - @imap.login(self.email_username, self.email_password) - - @imap.list('', '*').each do |m| - next if m.attr.include?(:Noselect) - mailboxes << m.name - end + imap_provider = Imap::Providers::Detector.init_with_detected_provider( + self.imap_config + ) + imap_provider.connect! + mailboxes = imap_provider.list_mailboxes + imap_provider.disconnect! update_columns(imap_last_error: nil) rescue => ex + Rails.logger.warn("[IMAP] Mailbox refresh failed for group #{self.name} with error: #{ex}") update_columns(imap_last_error: ex.message) end @@ -782,6 +783,16 @@ class Group < ActiveRecord::Base end end + def imap_config + { + server: self.imap_server, + port: self.imap_port, + ssl: self.imap_ssl, + username: self.email_username, + password: self.email_password + } + end + def email_username_regex user, domain = email_username.split('@') if user.present? && domain.present? diff --git a/lib/demon/email_sync.rb b/lib/demon/email_sync.rb index e4c7f15f2e5..dbf6d6f22dc 100644 --- a/lib/demon/email_sync.rb +++ b/lib/demon/email_sync.rb @@ -25,7 +25,7 @@ class Demon::EmailSync < ::Demon::Base RailsMultisite::ConnectionManagement.with_connection(db) do puts "[EmailSync] Thread started for group #{group.name} (id = #{group.id}) in db #{db}" begin - obj = Imap::Sync.for_group(group) + obj = Imap::Sync.new(group) rescue Net::IMAP::NoResponseError => e group.update(imap_last_error: e.message) Thread.exit diff --git a/lib/imap/providers/detector.rb b/lib/imap/providers/detector.rb new file mode 100644 index 00000000000..41f356517e3 --- /dev/null +++ b/lib/imap/providers/detector.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Imap + module Providers + class Detector + def self.init_with_detected_provider(config) + if config[:server] == 'imap.gmail.com' + return Imap::Providers::Gmail.new(config[:server], config) + end + Imap::Providers::Generic.new(config[:server], config) + end + end + end +end diff --git a/lib/imap/providers/generic.rb b/lib/imap/providers/generic.rb index c7bb6825286..0801c9dc70d 100644 --- a/lib/imap/providers/generic.rb +++ b/lib/imap/providers/generic.rb @@ -4,11 +4,9 @@ require 'net/imap' module Imap module Providers - class WriteDisabledError < StandardError; end class Generic - def initialize(server, options = {}) @server = server @port = options[:port] || 993 @@ -124,7 +122,10 @@ module Imap end def list_mailboxes - imap.list('', '*').map(&:name) + imap.list('', '*').map do |m| + next if m.attr.include?(:Noselect) + m.name + end end def archive(uid) diff --git a/lib/imap/providers/gmail.rb b/lib/imap/providers/gmail.rb index 347fb2d8267..82a9ecdda32 100644 --- a/lib/imap/providers/gmail.rb +++ b/lib/imap/providers/gmail.rb @@ -96,6 +96,10 @@ module Imap # Modified version of the original `msg_att` from here: # https://github.com/ruby/ruby/blob/1cc8ff001da217d0e98d13fe61fbc9f5547ef722/lib/net/imap.rb#L2346 + # + # This is done so we can extract X-GM-LABELS, X-GM-MSGID, and + # X-GM-THRID, all Gmail extended attributes. + # # rubocop:disable Style/RedundantReturn def msg_att(n) match(T_LPAR) @@ -127,6 +131,7 @@ module Imap name, val = uid_data when /\A(?:MODSEQ)\z/ni name, val = modseq_data + # Adding support for GMail extended attributes. when /\A(?:X-GM-LABELS)\z/ni name, val = label_data @@ -134,6 +139,8 @@ module Imap name, val = uid_data when /\A(?:X-GM-THRID)\z/ni name, val = uid_data + # End custom support for Gmail. + else parse_error("unknown attribute `%s' for {%d}", token.value, n) end diff --git a/lib/imap/sync.rb b/lib/imap/sync.rb index 73593b063bd..7a296015cfe 100644 --- a/lib/imap/sync.rb +++ b/lib/imap/sync.rb @@ -14,26 +14,9 @@ module Imap end end - def self.for_group(group, opts = {}) - if group.imap_server == 'imap.gmail.com' - opts[:provider] ||= Imap::Providers::Gmail - end - - Imap::Sync.new(group, opts) - end - def initialize(group, opts = {}) @group = group - - provider_klass ||= opts[:provider] || Imap::Providers::Generic - @provider = provider_klass.new( - @group.imap_server, - port: @group.imap_port, - ssl: @group.imap_ssl, - username: @group.email_username, - password: @group.email_password - ) - + @provider = Imap::Providers::Detector.init_with_detected_provider(@group.imap_config) connect! end @@ -190,6 +173,10 @@ module Imap emails = @provider.emails(new_uids, ['UID', 'FLAGS', 'LABELS', 'RFC822']) processed = 0 + # TODO (maybe): We might need something here to exclusively handle + # the UID of the incoming email, so we don't end up with a race condition + # where the same UID is handled multiple times before the group imap_X + # columns are updated. emails.each do |email| # Synchronously process emails because the order of emails matter # (for example replies must be processed after the original email @@ -310,15 +297,17 @@ module Imap new_labels << '\\Inbox' else Logger.log("[IMAP] (#{@group.name}) Archiving UID #{incoming_email.imap_uid}") + + # some providers need special handling for archiving. this way we preserve + # any new tag-labels, and archive, even though it may cause extra requests + # to the IMAP server + @provider.archive(incoming_email.imap_uid) end + # regardless of whether the topic needs to be archived we still update + # the flags and the labels @provider.store(incoming_email.imap_uid, 'FLAGS', flags, new_flags) @provider.store(incoming_email.imap_uid, 'LABELS', labels, new_labels) - - # some providers need special handling for archiving. this way we preserve - # any new tag-labels, and archive, even though it may cause extra requests - # to the IMAP server - @provider.archive(incoming_email.imap_uid) end end end diff --git a/spec/components/imap/sync_spec.rb b/spec/components/imap/sync_spec.rb index 0ead0f4ec85..14378be72cf 100644 --- a/spec/components/imap/sync_spec.rb +++ b/spec/components/imap/sync_spec.rb @@ -26,7 +26,20 @@ describe Imap::Sync do ) end - let(:sync_handler) { Imap::Sync.new(group, provider: MockedImapProvider) } + let(:sync_handler) { Imap::Sync.new(group) } + + before do + mocked_imap_provider = MockedImapProvider.new( + group.imap_server, + port: group.imap_port, + ssl: group.imap_ssl, + username: group.email_username, + password: group.email_password + ) + Imap::Providers::Detector.stubs(:init_with_detected_provider).returns( + mocked_imap_provider + ) + end context 'no previous sync' do let(:from) { 'john@free.fr' } @@ -252,6 +265,81 @@ describe Imap::Sync do expect(Topic.last.posts.where(post_type: Post.types[:regular]).count).to eq(2) end + + describe "archiving emails" do + let(:provider) { MockedImapProvider.any_instance } + before do + SiteSetting.enable_imap_write = true + provider.stubs(:open_mailbox).returns(uid_validity: 1) + + provider.stubs(:uids).with.returns([100]) + provider.stubs(:emails).with([100], ['UID', 'FLAGS', 'LABELS', 'RFC822'], anything).returns( + [ + { + 'UID' => 100, + 'LABELS' => %w[\\Inbox], + 'FLAGS' => %i[Seen], + 'RFC822' => EmailFabricator( + message_id: first_message_id, + from: first_from, + to: group.email_username, + cc: second_from, + subject: subject, + body: first_body + ) + } + ] + ) + + sync_handler.process + @incoming_email = IncomingEmail.find_by(message_id: first_message_id) + @topic = @incoming_email.topic + + provider.stubs(:uids).with(to: 100).returns([100]) + provider.stubs(:uids).with(from: 101).returns([101]) + provider.stubs(:emails).with([100], ['UID', 'FLAGS', 'LABELS', 'ENVELOPE'], anything).returns( + [ + { + 'UID' => 100, + 'LABELS' => %w[\\Inbox], + 'FLAGS' => %i[Seen] + } + ] + ) + provider.stubs(:emails).with([101], ['UID', 'FLAGS', 'LABELS', 'RFC822'], anything).returns( + [] + ) + provider.stubs(:emails).with(100, ['FLAGS', 'LABELS']).returns( + [ + { + 'LABELS' => %w[\\Inbox], + 'FLAGS' => %i[Seen] + } + ] + ) + end + + it "archives an email on the IMAP server when archived in discourse" do + GroupArchivedMessage.archive!(group.id, @topic, skip_imap_sync: false) + @incoming_email.update(imap_sync: true) + + provider.stubs(:store).with(100, 'FLAGS', anything, anything) + provider.stubs(:store).with(100, 'LABELS', ["\\Inbox"], ["seen"]) + + provider.expects(:archive).with(100) + sync_handler.process + end + + it "does not archive email if not archived in discourse" do + @incoming_email.update(imap_sync: true) + provider.stubs(:store).with(100, 'FLAGS', anything, anything) + provider.stubs(:store).with(100, 'LABELS', ["\\Inbox"], ["seen", "\\Inbox"]) + + provider.expects(:archive).with(100).never + sync_handler.process + end + end + end context 'invaidated previous sync' do diff --git a/spec/lib/imap/providers/detector_spec.rb b/spec/lib/imap/providers/detector_spec.rb new file mode 100644 index 00000000000..0316160c3c3 --- /dev/null +++ b/spec/lib/imap/providers/detector_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Imap::Providers::Detector do + it "returns the gmail provider if the gmail imap server is used" do + config = { + server: "imap.gmail.com", + port: 993, + ssl: true, + username: "test@gmail.com", + password: "testpassword1" + } + expect(described_class.init_with_detected_provider(config)).to be_a(Imap::Providers::Gmail) + end + + it "returns the generic provider if we don't have a special provider defined" do + config = { + server: "imap.yo.com", + port: 993, + ssl: true, + username: "test@yo.com", + password: "testpassword1" + } + expect(described_class.init_with_detected_provider(config)).to be_a(Imap::Providers::Generic) + end +end diff --git a/spec/lib/imap/providers/gmail_spec.rb b/spec/lib/imap/providers/gmail_spec.rb new file mode 100644 index 00000000000..dc6880e2a21 --- /dev/null +++ b/spec/lib/imap/providers/gmail_spec.rb @@ -0,0 +1,73 @@ + +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Imap::Providers::Gmail do + fab!(:username) { "test@generic.com" } + fab!(:password) { "test1!" } + fab!(:provider) do + described_class.new( + "imap.generic.com", + { + port: 993, + ssl: true, + username: username, + password: password + } + ) + end + + let(:imap_stub) { stub } + let(:x_gm_thrid) { Imap::Providers::Gmail::X_GM_THRID } + let(:x_gm_labels) { Imap::Providers::Gmail::X_GM_LABELS } + before do + described_class.any_instance.stubs(:imap).returns(imap_stub) + end + + describe "#store" do + it "converts LABELS store to special X-GM-LABELS" do + Imap::Providers::Generic.any_instance.expects(:store).with( + 63, x_gm_labels, ["\\Inbox"], ["\\Inbox", "test"] + ) + provider.store(63, "LABELS", ["\\Inbox"], ["\\Inbox", "test"]) + end + end + + describe "#tag_to_label" do + it "converts important to special gmail label \\Important" do + expect(provider.tag_to_label("important")).to eq("\\Important") + end + + it "converts starred to special gmail label \\Starred" do + expect(provider.tag_to_label("starred")).to eq("\\Starred") + end + end + + describe "#archive" do + it "gets the thread ID for the UID, and removes the Inbox label from all UIDs in the thread" do + main_uid = 78 + fake_thrid = '4398634986239754' + imap_stub.expects(:uid_fetch).with(main_uid, [x_gm_thrid]).returns( + [stub(attr: { x_gm_thrid => fake_thrid })] + ) + imap_stub.expects(:uid_search).with("#{x_gm_thrid} #{fake_thrid}").returns([79, 80]) + provider.expects(:emails).with([79, 80], ["UID", "LABELS"]).returns( + [ + { + "UID" => 79, + "LABELS" => ["\\Inbox", "seen"] + }, + { + "UID" => 80, + "LABELS" => ["\\Inbox", "seen"] + } + ] + ) + provider.expects(:store).with(79, "LABELS", ["\\Inbox", "seen"], ["seen"]) + provider.expects(:store).with(80, "LABELS", ["\\Inbox", "seen"], ["seen"]) + + provider.archive(main_uid) + end + end +end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 42e50cedf4e..0eef371dbda 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'rails_helper' +require_relative '../components/imap/imap_helper' describe Group do let(:admin) { Fabricate(:admin) } @@ -972,6 +973,79 @@ describe Group do end end + describe "#imap_mailboxes" do + let(:group) { Fabricate(:group) } + + def mock_imap + @mocked_imap_provider = MockedImapProvider.new( + group.imap_server, + port: group.imap_port, + ssl: group.imap_ssl, + username: group.email_username, + password: group.email_password + ) + Imap::Providers::Detector.stubs(:init_with_detected_provider).returns( + @mocked_imap_provider + ) + end + + def configure_imap + group.update( + imap_server: "imap.gmail.com", + imap_port: 993, + imap_ssl: true, + email_username: "test@gmail.com", + email_password: "testPassword1!" + ) + end + + def enable_imap + SiteSetting.enable_imap = true + @mocked_imap_provider.stubs(:connect!) + @mocked_imap_provider.stubs(:list_mailboxes).returns(["Inbox"]) + @mocked_imap_provider.stubs(:disconnect!) + end + + before do + Discourse.redis.del("group_imap_mailboxes_#{group.id}") + end + + it "returns an empty array if group imap is not configured" do + expect(group.imap_mailboxes).to eq([]) + end + + it "returns an empty array and does not contact IMAP server if group imap is configured but the setting is disabled" do + configure_imap + Imap::Providers::Detector.expects(:init_with_detected_provider).never + expect(group.imap_mailboxes).to eq([]) + end + + it "logs the imap error if one occurs" do + configure_imap + mock_imap + SiteSetting.enable_imap = true + @mocked_imap_provider.stubs(:connect!).raises(Net::IMAP::NoResponseError) + group.imap_mailboxes + expect(group.reload.imap_last_error).not_to eq(nil) + end + + it "returns a list of mailboxes from the IMAP provider" do + configure_imap + mock_imap + enable_imap + expect(group.imap_mailboxes).to eq(["Inbox"]) + end + + it "caches the login and mailbox fetch" do + configure_imap + mock_imap + enable_imap + group.imap_mailboxes + Imap::Providers::Detector.expects(:init_with_detected_provider).never + group.imap_mailboxes + end + end + context "Unicode usernames and group names" do before { SiteSetting.unicode_usernames = true }