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

@@ -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_provider = Imap::Providers::Detector.init_with_detected_provider(
-        @imap.login(self.email_username, self.email_password)
+          self.imap_config
-
+        )
-        @imap.list('', '*').each do |m|
+        imap_provider.connect!
-          next if m.attr.include?(:Noselect)
+        mailboxes = imap_provider.list_mailboxes
-          mailboxes << m.name
+        imap_provider.disconnect!
-        end
 
         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?

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

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

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)

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

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 = Imap::Providers::Detector.init_with_detected_provider(@group.imap_config)
-      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
-      )
-
       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

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

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

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

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 }