From cc9977460e0885479aa7d737bc47301b6e4459ea Mon Sep 17 00:00:00 2001
From: Rolf Schmidt <rolf.schmidt@zammad.com>
Date: Thu, 3 Mar 2022 10:04:12 +0100
Subject: [PATCH] Fixes #3964 - Selecting custom ports for mail channel ends in
 'host not reachable'.

---
 .../app/controllers/_channel/email.coffee     | 19 ++++++++++++---
 .../getting_started/channel_email.coffee      | 19 ++++++++++++---
 app/models/channel/driver/imap.rb             | 14 +++++------
 app/models/channel/driver/pop3.rb             | 22 ++++++++---------
 ...216131241_issue3964_inbound_fix_options.rb | 21 ++++++++++++++++
 i18n/zammad.pot                               | 15 ++++++++++++
 lib/external_credential/google.rb             |  2 +-
 lib/external_credential/microsoft365.rb       |  2 +-
 .../issue_3964_inbound_fix_options_spec.rb    | 24 +++++++++++++++++++
 spec/factories/channel.rb                     |  4 ++--
 spec/lib/auto_wizard_spec.rb                  |  4 ++--
 spec/lib/external_credential/google_spec.rb   |  2 +-
 .../external_credential/microsoft365_spec.rb  |  2 +-
 spec/system/channels/email_spec.rb            |  4 ++--
 14 files changed, 118 insertions(+), 36 deletions(-)
 create mode 100644 db/migrate/20220216131241_issue3964_inbound_fix_options.rb
 create mode 100644 spec/db/migrate/issue_3964_inbound_fix_options_spec.rb

diff --git a/app/assets/javascripts/app/controllers/_channel/email.coffee b/app/assets/javascripts/app/controllers/_channel/email.coffee
index 7c0e1af85..b61652e6c 100644
--- a/app/assets/javascripts/app/controllers/_channel/email.coffee
+++ b/app/assets/javascripts/app/controllers/_channel/email.coffee
@@ -307,6 +307,7 @@ class ChannelEmailAccountWizard extends App.ControllerWizardModal
   events:
     'submit .js-intro':                   'probeBasedOnIntro'
     'submit .js-inbound':                 'probeInbound'
+    'change .js-inbound [name=adapter]':  'toggleInboundAdapter'
     'change .js-outbound [name=adapter]': 'toggleOutboundAdapter'
     'submit .js-outbound':                'probleOutbound'
     'click  .js-goToSlide':               'goToSlide'
@@ -404,7 +405,7 @@ class ChannelEmailAccountWizard extends App.ControllerWizardModal
       { name: 'options::host',            display: __('Host'),     tag: 'input',  type: 'text', limit: 120, null: false, autocapitalize: false },
       { name: 'options::user',            display: __('User'),     tag: 'input',  type: 'text', limit: 120, null: false, autocapitalize: false, autocomplete: 'off' },
       { name: 'options::password',        display: __('Password'), tag: 'input',  type: 'password', limit: 120, null: false, autocapitalize: false, autocomplete: 'new-password', single: true },
-      { name: 'options::ssl',             display: __('SSL/STARTTLS'),      tag: 'boolean', null: true, options: { true: 'yes', false: 'no'  }, default: true, translate: true, item_class: 'formGroup--halfSize' },
+      { name: 'options::ssl',             display: __('SSL/STARTTLS'), tag: 'select', null: true, options: { 'off': __('No SSL'), 'ssl': __('SSL'), 'starttls': __('STARTTLS')  }, default: 'ssl', translate: true, item_class: 'formGroup--halfSize' },
       { name: 'options::port',            display: __('Port'),     tag: 'input',  type: 'text', limit: 6,   null: true, autocapitalize: false,  default: '993', item_class: 'formGroup--halfSize' },
       { name: 'options::folder',          display: __('Folder'),   tag: 'input',  type: 'text', limit: 120, null: true, autocapitalize: false, item_class: 'formGroup--halfSize' },
       { name: 'options::keep_on_server',  display: __('Keep messages on server'), tag: 'boolean', null: true, options: { true: 'yes', false: 'no' }, translate: true, default: false, item_class: 'formGroup--halfSize' },
@@ -438,14 +439,26 @@ class ChannelEmailAccountWizard extends App.ControllerWizardModal
         showHideFolder,
       ]
     )
+    @toggleInboundAdapter()
 
     form.el.find("select[name='options::ssl']").off('change').on('change', (e) ->
-      if $(e.target).val() is 'true'
+      if $(e.target).val() is 'ssl'
         form.el.find("[name='options::port']").val('993')
-      else
+      else if $(e.target).val() is 'off'
         form.el.find("[name='options::port']").val('143')
     )
 
+  toggleInboundAdapter: =>
+    form     = @$('.base-inbound-settings')
+    adapter  = form.find("select[name='adapter']")
+    starttls = form.find("select[name='options::ssl'] option[value='starttls']")
+
+    if adapter.val() isnt 'imap'
+      starttls.remove()
+    else if starttls.length < 1
+      starttls = $('<option/>').attr('value', 'starttls').text(__('STARTTLS'))
+      form.find("select[name='options::ssl']").append(starttls)
+
   toggleOutboundAdapter: =>
 
     # fill user / password based on intro info
diff --git a/app/assets/javascripts/app/controllers/getting_started/channel_email.coffee b/app/assets/javascripts/app/controllers/getting_started/channel_email.coffee
index dc94ac661..c86368a99 100644
--- a/app/assets/javascripts/app/controllers/getting_started/channel_email.coffee
+++ b/app/assets/javascripts/app/controllers/getting_started/channel_email.coffee
@@ -2,6 +2,7 @@ class GettingStartedChannelEmail extends App.ControllerWizardFullScreen
   events:
     'submit .js-intro':                   'probeBasedOnIntro'
     'submit .js-inbound':                 'probeInbound'
+    'change .js-inbound [name=adapter]':  'toggleInboundAdapter'
     'change .js-outbound [name=adapter]': 'toggleOutboundAdapter'
     'submit .js-outbound':                'probleOutbound'
     'click  .js-goToSlide':               'goToSlide'
@@ -76,7 +77,7 @@ class GettingStartedChannelEmail extends App.ControllerWizardFullScreen
       { name: 'options::host',            display: __('Host'),     tag: 'input',  type: 'text', limit: 120, null: false, autocapitalize: false },
       { name: 'options::user',            display: __('User'),     tag: 'input',  type: 'text', limit: 120, null: false, autocapitalize: false, autocomplete: 'off', },
       { name: 'options::password',        display: __('Password'), tag: 'input',  type: 'password', limit: 120, null: false, autocapitalize: false, autocomplete: 'off', single: true },
-      { name: 'options::ssl',             display: __('SSL/STARTTLS'), tag: 'boolean', null: true, options: { true: 'yes', false: 'no'  }, default: true, translate: true, item_class: 'formGroup--halfSize' },
+      { name: 'options::ssl',             display: __('SSL/STARTTLS'), tag: 'select', null: true, options: { 'off': __('No SSL'), 'ssl': __('SSL'), 'starttls': __('STARTTLS')  }, default: 'ssl', translate: true, item_class: 'formGroup--halfSize' },
       { name: 'options::port',            display: __('Port'),     tag: 'input',  type: 'text', limit: 6,   null: true, autocapitalize: false,  default: '993', item_class: 'formGroup--halfSize' },
       { name: 'options::folder',          display: __('Folder'),   tag: 'input',  type: 'text', limit: 120, null: true, autocapitalize: false, item_class: 'formGroup--halfSize' },
       { name: 'options::keep_on_server',  display: __('Keep messages on server'), tag: 'boolean', null: true, options: { true: 'yes', false: 'no' }, translate: true, default: false, item_class: 'formGroup--halfSize' },
@@ -101,14 +102,26 @@ class GettingStartedChannelEmail extends App.ControllerWizardFullScreen
         showHideFolder,
       ]
     )
+    @toggleInboundAdapter()
 
     form.el.find("select[name='options::ssl']").off('change').on('change', (e) ->
-      if $(e.target).val() is 'true'
+      if $(e.target).val() is 'ssl'
         form.el.find("[name='options::port']").val('993')
-      else
+      else if $(e.target).val() is 'off'
         form.el.find("[name='options::port']").val('143')
     )
 
+  toggleInboundAdapter: =>
+    form     = @$('.base-inbound-settings')
+    adapter  = form.find("select[name='adapter']")
+    starttls = form.find("select[name='options::ssl'] option[value='starttls']")
+
+    if adapter.val() isnt 'imap'
+      starttls.remove()
+    else if starttls.length < 1
+      starttls = $('<option/>').attr('value', 'starttls').text(__('STARTTLS'))
+      form.find("select[name='options::ssl']").append(starttls)
+
   toggleOutboundAdapter: =>
 
     # fill user / password based on intro info
diff --git a/app/models/channel/driver/imap.rb b/app/models/channel/driver/imap.rb
index 736802c9e..f12f8502f 100644
--- a/app/models/channel/driver/imap.rb
+++ b/app/models/channel/driver/imap.rb
@@ -87,9 +87,12 @@ example
       keep_on_server = true
     end
 
-    if options.key?(:ssl) && options[:ssl] == false
-      ssl  = false
-      port = 143
+    case options[:ssl]
+    when 'off'
+      ssl = false
+    when 'starttls'
+      ssl = false
+      starttls = true
     end
 
     port = if options.key?(:port) && options[:port].present?
@@ -100,11 +103,6 @@ example
              143
            end
 
-    if ssl == true && port != 993
-      ssl = false
-      starttls = true
-    end
-
     if options[:folder].present?
       folder = options[:folder]
     end
diff --git a/app/models/channel/driver/pop3.rb b/app/models/channel/driver/pop3.rb
index 253daccf2..c4ef8d541 100644
--- a/app/models/channel/driver/pop3.rb
+++ b/app/models/channel/driver/pop3.rb
@@ -45,20 +45,18 @@ returns
 =end
 
   def fetch(options, channel, check_type = '', verify_string = '')
-    ssl  = true
-    port = 995
-    if options.key?(:ssl) && options[:ssl] == false
-      ssl  = false
-      port = 110
+    ssl = true
+    if options[:ssl] == 'off'
+      ssl = false
     end
-    if options.key?(:port) && options[:port].present?
-      port = options[:port]
 
-      # disable ssl for non ssl ports
-      if port == 110 && !options.key?(:ssl)
-        ssl = false
-      end
-    end
+    port = if options.key?(:port) && options[:port].present?
+             options[:port].to_i
+           elsif ssl == true
+             995
+           else
+             110
+           end
 
     Rails.logger.info "fetching pop3 (#{options[:host]}/#{options[:user]} port=#{port},ssl=#{ssl})"
 
diff --git a/db/migrate/20220216131241_issue3964_inbound_fix_options.rb b/db/migrate/20220216131241_issue3964_inbound_fix_options.rb
new file mode 100644
index 000000000..2efb338f9
--- /dev/null
+++ b/db/migrate/20220216131241_issue3964_inbound_fix_options.rb
@@ -0,0 +1,21 @@
+# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/
+
+class Issue3964InboundFixOptions < ActiveRecord::Migration[6.0]
+  def change
+    # return if it's a new setup
+    return if !Setting.exists?(name: 'system_init_done')
+
+    Channel.where(area: ['Email::Account', 'Google::Account', 'Microsoft365::Account']).find_each do |channel|
+      ssl = channel.options.dig(:inbound, :options, :ssl)
+      next if ssl.nil?
+
+      channel.options[:inbound][:options][:ssl] = if ssl == true
+                                                    'ssl'
+                                                  else
+                                                    'off'
+                                                  end
+
+      channel.save!
+    end
+  end
+end
diff --git a/i18n/zammad.pot b/i18n/zammad.pot
index 01e0336ef..6897e564b 100644
--- a/i18n/zammad.pot
+++ b/i18n/zammad.pot
@@ -6350,6 +6350,11 @@ msgstr ""
 msgid "No Proxy"
 msgstr ""
 
+#: app/assets/javascripts/app/controllers/_channel/email.coffee
+#: app/assets/javascripts/app/controllers/getting_started/channel_email.coffee
+msgid "No SSL"
+msgstr ""
+
 #: app/assets/javascripts/app/controllers/_plugin/translation_support.coffee
 msgid "No Thanks!"
 msgstr ""
@@ -7770,6 +7775,11 @@ msgstr ""
 msgid "SMTP - configure your own outgoing SMTP settings"
 msgstr ""
 
+#: app/assets/javascripts/app/controllers/_channel/email.coffee
+#: app/assets/javascripts/app/controllers/getting_started/channel_email.coffee
+msgid "SSL"
+msgstr ""
+
 #: app/assets/javascripts/app/models/webhook.coffee
 msgid "SSL Verify"
 msgstr ""
@@ -7787,6 +7797,11 @@ msgstr ""
 msgid "SSO"
 msgstr ""
 
+#: app/assets/javascripts/app/controllers/_channel/email.coffee
+#: app/assets/javascripts/app/controllers/getting_started/channel_email.coffee
+msgid "STARTTLS"
+msgstr ""
+
 #: app/assets/javascripts/app/controllers/_ui_element/basedate.coffee
 msgid "Sat"
 msgstr ""
diff --git a/lib/external_credential/google.rb b/lib/external_credential/google.rb
index a5d989fd5..66ac56b31 100644
--- a/lib/external_credential/google.rb
+++ b/lib/external_credential/google.rb
@@ -49,7 +49,7 @@ class ExternalCredential::Google
         options: {
           auth_type: 'XOAUTH2',
           host:      'imap.gmail.com',
-          ssl:       true,
+          ssl:       'ssl',
           user:      user_data[:email],
         },
       },
diff --git a/lib/external_credential/microsoft365.rb b/lib/external_credential/microsoft365.rb
index 56cd34688..19412c4ec 100644
--- a/lib/external_credential/microsoft365.rb
+++ b/lib/external_credential/microsoft365.rb
@@ -53,7 +53,7 @@ class ExternalCredential::Microsoft365
         options: {
           auth_type: 'XOAUTH2',
           host:      'outlook.office365.com',
-          ssl:       true,
+          ssl:       'ssl',
           user:      user_data[:preferred_username],
         },
       },
diff --git a/spec/db/migrate/issue_3964_inbound_fix_options_spec.rb b/spec/db/migrate/issue_3964_inbound_fix_options_spec.rb
new file mode 100644
index 000000000..442a16b30
--- /dev/null
+++ b/spec/db/migrate/issue_3964_inbound_fix_options_spec.rb
@@ -0,0 +1,24 @@
+# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe Issue3964InboundFixOptions, type: :db_migration do
+  let(:channel1) { create(:google_channel) }
+  let(:channel2) { create(:google_channel) }
+
+  before do
+    channel1.options[:inbound][:options][:ssl] = true
+    channel1.save
+    channel2.options[:inbound][:options][:ssl] = false
+    channel2.save
+    migrate
+  end
+
+  it 'sets the correct ssl value for ssl true' do
+    expect(channel1.reload.options[:inbound][:options][:ssl]).to eq('ssl')
+  end
+
+  it 'sets the correct ssl value for ssl false' do
+    expect(channel2.reload.options[:inbound][:options][:ssl]).to eq('off')
+  end
+end
diff --git a/spec/factories/channel.rb b/spec/factories/channel.rb
index 995f12a79..7a8e2ac23 100644
--- a/spec/factories/channel.rb
+++ b/spec/factories/channel.rb
@@ -133,7 +133,7 @@ FactoryBot.define do
             'options' => {
               'auth_type'      => 'XOAUTH2',
               'host'           => 'imap.gmail.com',
-              'ssl'            => true,
+              'ssl'            => 'ssl',
               'user'           => ENV['GMAIL_USER'],
               'folder'         => '',
               'keep_on_server' => false,
@@ -175,7 +175,7 @@ FactoryBot.define do
             'options' => {
               'auth_type'      => 'XOAUTH2',
               'host'           => 'outlook.office365.com',
-              'ssl'            => true,
+              'ssl'            => 'ssl',
               'user'           => ENV['MICROSOFT365_USER'],
               'folder'         => '',
               'keep_on_server' => false,
diff --git a/spec/lib/auto_wizard_spec.rb b/spec/lib/auto_wizard_spec.rb
index 105ac7870..b06867d1d 100644
--- a/spec/lib/auto_wizard_spec.rb
+++ b/spec/lib/auto_wizard_spec.rb
@@ -188,7 +188,7 @@ RSpec.describe AutoWizard do
                     host:     'mx1.example.com',
                     user:     'not_existing',
                     password: 'some_pass',
-                    ssl:      true
+                    ssl:      'ssl'
                   }
                 },
                 outbound: {
@@ -227,7 +227,7 @@ RSpec.describe AutoWizard do
                     host:     'mx1.example.com',
                     user:     'not_existing',
                     password: 'some_pass',
-                    ssl:      true
+                    ssl:      'ssl'
                   }
                 },
                 outbound: {
diff --git a/spec/lib/external_credential/google_spec.rb b/spec/lib/external_credential/google_spec.rb
index 342f650f4..60f3e7fd8 100644
--- a/spec/lib/external_credential/google_spec.rb
+++ b/spec/lib/external_credential/google_spec.rb
@@ -121,7 +121,7 @@ RSpec.describe ExternalCredential::Google do
               'options' => a_hash_including(
                 'auth_type' => 'XOAUTH2',
                 'host'      => 'imap.gmail.com',
-                'ssl'       => true,
+                'ssl'       => 'ssl',
                 'user'      => primary_email,
               )
             ),
diff --git a/spec/lib/external_credential/microsoft365_spec.rb b/spec/lib/external_credential/microsoft365_spec.rb
index 7953be892..a88697a23 100644
--- a/spec/lib/external_credential/microsoft365_spec.rb
+++ b/spec/lib/external_credential/microsoft365_spec.rb
@@ -87,7 +87,7 @@ RSpec.describe ExternalCredential::Microsoft365 do
               'options' => a_hash_including(
                 'auth_type' => 'XOAUTH2',
                 'host'      => 'outlook.office365.com',
-                'ssl'       => true,
+                'ssl'       => 'ssl',
                 'user'      => email_address,
               )
             ),
diff --git a/spec/system/channels/email_spec.rb b/spec/system/channels/email_spec.rb
index d5dff5963..9e7eb15c8 100644
--- a/spec/system/channels/email_spec.rb
+++ b/spec/system/channels/email_spec.rb
@@ -151,8 +151,8 @@ RSpec.describe 'Manage > Channels > Email', type: :system do
         click '.js-expert'
         expect(find('input[name="options::port"]').value).to eq('993')
         field = find('select[name="options::ssl"]')
-        option_yes = field.find(:option, 'yes')
-        option_no = field.find(:option, 'no')
+        option_yes = field.find(:option, 'SSL')
+        option_no = field.find(:option, 'No SSL')
         option_no.select_option
         expect(find('input[name="options::port"]').value).to eq('143')
         option_yes.select_option