Merge branch 'develop' of git.znuny.com:zammad/zammad into develop

app/models/import_job.rb

@@ -5,6 +5,8 @@ class ImportJob < ApplicationModel
   store :payload
   store :result
 
+  scope :running, -> { where(finished_at: nil, dry_run: false).where.not(started_at: nil) }
+
   # Starts the import backend class based on the name attribute.
   # Import backend class is initialized with the current instance.
   # Logs the start and end time (if ended successfully) and logs
@@ -109,15 +111,7 @@ class ImportJob < ApplicationModel
   #
   # return [nil]
   def self.queue_registered
-    import_backends = Setting.get('import_backends')
-    return if import_backends.blank?
-
-    import_backends.each do |backend|
-
-      if !backend_valid?(backend)
-        Rails.logger.error "Invalid import backend '#{backend}'"
-        next
-      end
+    backends.each do |backend|
 
       # skip backends that are not "ready" yet
       next if !backend.constantize.queueable?

app/models/scheduler.rb

@@ -121,9 +121,24 @@ class Scheduler < ApplicationModel
       raise 'This method should only get called when Scheduler.threads are initialized. Use `force: true` to start anyway.'
     end
 
-    log_start_finish(:info, 'Cleanup of left over locked delayed jobs') do
+    start_time = Time.zone.now
 
-      Delayed::Job.all.each do |job|
+    cleanup_delayed_jobs(start_time)
+    cleanup_import_jobs(start_time)
+  end
+
+  # Checks for locked delayed jobs and tries to reschedule or destroy each of them.
+  #
+  # @param [ActiveSupport::TimeWithZone] after the time the cleanup was started
+  #
+  # @example
+  #   Scheduler.cleanup_delayed_jobs(TimeZone.now)
+  #
+  # return [nil]
+  def self.cleanup_delayed_jobs(after)
+    log_start_finish(:info, "Cleanup of left over locked delayed jobs #{after}") do
+
+      Delayed::Job.where('updated_at < ?', after).where.not(locked_at: nil).each do |job|
         log_start_finish(:info, "Checking left over delayed job #{job.inspect}") do
           cleanup_delayed(job)
         end
@@ -131,13 +146,13 @@ class Scheduler < ApplicationModel
     end
   end
 
-  # Checks if the given job can be rescheduled or destroys it. Logs the action as warn.
-  # Works only for locked jobs. Jobs that are not locked are ignored and
+  # Checks if the given delayed job can be rescheduled or destroys it. Logs the action as warn.
+  # Works only for locked delayed jobs. Delayed jobs that are not locked are ignored and
   # should get destroyed directly.
-  # Checks the delayed job object for a method called .reschedule?. The memthod is called
-  # with the delayed job as a parameter. The result value is expected as a Boolean. If the
-  # result is true the lock gets removed and the delayed job gets rescheduled. If the return
-  # value is false it will get destroyed which is the default behaviour.
+  # Checks the Delayed::Job instance for a method called .reschedule?. The method is called
+  # with the Delayed::Job instance as a parameter. The result value is expected to be a Boolean.
+  # If the result is true the lock gets removed and the delayed job gets rescheduled.
+  # If the return value is false it will get destroyed which is the default behaviour.
   #
   # @param [Delayed::Job] job the job that should get checked for destroying/rescheduling.
   #
@@ -181,6 +196,33 @@ class Scheduler < ApplicationModel
     logger.warn "#{action} locked delayed job: #{job_name}"
   end
 
+  # Checks for killed import jobs and marks them as finished and adds a note.
+  #
+  # @param [ActiveSupport::TimeWithZone] after the time the cleanup was started
+  #
+  # @example
+  #   Scheduler.cleanup_import_jobs(TimeZone.now)
+  #
+  # return [nil]
+  def self.cleanup_import_jobs(after)
+    log_start_finish(:info, "Cleanup of left over import jobs #{after}") do
+      error = 'Interrupted by scheduler restart. Please restart manually or wait till next execution time.'.freeze
+
+      # we need to exclude jobs that were updated at or since we started
+      # cleaning up (via the #reschedule? call) because they might
+      # were started `.delay`-ed and are flagged for restart
+      ImportJob.running.where('updated_at < ?', after).each do |job|
+
+        job.update!(
+          finished_at: after,
+          result:      {
+            error: error
+          }
+        )
+      end
+    end
+  end
+
   def self.start_job(job)
 
     # start job and return thread handle

config/application.rb

@@ -2,6 +2,9 @@ require File.expand_path('boot', __dir__)
 
 require 'rails/all'
 
+# DO NOT REMOVE THIS LINE - see issue #2037
+Bundler.setup
+
 # Require the gems listed in Gemfile, including any gems
 # you've limited to :test, :development, or :production.
 Bundler.require(*Rails.groups)

lib/html_sanitizer.rb

@@ -380,7 +380,28 @@ cleanup html string:
                   else
                     /[[:space:]]|\t|\n|\r/
                   end
-    string.strip.gsub(blank_regex, '').gsub(%r{/\*.*?\*/}, '').gsub(/<!--.*?-->/, '').gsub(/\[.+?\]/, '').delete("\u0000")
+    cleaned_string = string.strip.gsub(blank_regex, '').gsub(%r{/\*.*?\*/}, '').gsub(/<!--.*?-->/, '').gsub(/\[.+?\]/, '').delete("\u0000")
+    sanitize_attachment_disposition(cleaned_string)
+  end
+
+  def self.sanitize_attachment_disposition(url)
+    uri = URI(url)
+    return url if uri.host != Setting.get('fqdn')
+
+    params = CGI.parse(uri.query || '')
+    if params.key?('disposition')
+      params['disposition'] = 'attachment'
+    end
+
+    uri.query = if params.blank?
+                  nil
+                else
+                  URI.encode_www_form(params)
+                end
+
+    uri.to_s
+  rescue URI::InvalidURIError
+    url
   end
 
   def self.url_same?(url_new, url_old)

lib/sequencer/unit/import/common/object_attribute/sanitized_name.rb

@@ -12,6 +12,7 @@ class Sequencer
               # model_nos
               # model_name
               # model_name
+              # model_name
               without_double_underscores.gsub(/_id(s?)$/, '_no\1')
             end
 
@@ -20,6 +21,7 @@ class Sequencer
               # model_ids
               # model_name
               # model_name
+              # model_name
               without_spaces_and_slashes.gsub(/_{2,}/, '_')
             end
 
@@ -28,7 +30,17 @@ class Sequencer
               # model_ids
               # model___name
               # model_name
-              unsanitized_name.gsub(%r{[\s\/]}, '_').underscore
+              # model_name
+              transliterated.gsub(%r{[\s\/]}, '_').underscore
+            end
+
+            def transliterated
+              # Model ID
+              # Model IDs
+              # Model / Name
+              # Model Name
+              # Model Name
+              ::ActiveSupport::Inflector.transliterate(unsanitized_name, '_'.freeze)
             end
 
             def unsanitized_name
@@ -36,6 +48,9 @@ class Sequencer
               # Model IDs
               # Model / Name
               # Model Name
+              # rubocop:disable Style/AsciiComments
+              # Mödel Nâmé
+              # rubocop:enable Style/AsciiComments
               raise 'Missing implementation for unsanitized_name method'
             end
           end

lib/sequencer/unit/import/zendesk/ticket/comment/source_based.rb

@@ -9,9 +9,12 @@ class Sequencer
               uses :resource
 
               def value
-                method_name = resource.via.channel.to_sym
-                return if !respond_to?(method_name, true)
-                send(method_name)
+                return if private_methods(false).exclude?(value_method_name)
+                send(value_method_name)
+              end
+
+              def value_method_name
+                @value_method_name ||= resource.via.channel.to_sym
               end
             end
           end

lib/tasks/bootstrap.rake

@@ -21,7 +21,7 @@ module BootstrapRakeHelper
   end
 
   def add_database_config
-    raise Errno::ENOENT, 'contrib/database.yml not found' unless File.exist?(DB_CONFIG[:source])
+    raise Errno::ENOENT, 'config/database.yml not found' unless File.exist?(DB_CONFIG[:source])
 
     if File.exist?(DB_CONFIG[:dest])
       return if FileUtils.identical?(DB_CONFIG[:source], DB_CONFIG[:dest])

spec/lib/sequencer/unit/import/common/object_attribute/sanitized_name_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName,
 
   context 'sanitizes' do
 
-    it 'whitespaces' do
+    it 'replaces whitespaces' do
       provided = process do |instance|
         expect(instance).to receive(:unsanitized_name).and_return('model name')
       end
@@ -19,7 +19,7 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName,
       expect(provided[:sanitized_name]).to eq('model_name')
     end
 
-    it 'dashes' do
+    it 'replaces dashes' do
       provided = process do |instance|
         expect(instance).to receive(:unsanitized_name).and_return('model-name')
       end
@@ -27,7 +27,7 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName,
       expect(provided[:sanitized_name]).to eq('model_name')
     end
 
-    it 'ids suffix' do
+    it 'replaces ids suffix' do
       provided = process do |instance|
         expect(instance).to receive(:unsanitized_name).and_return('Model Ids')
       end
@@ -35,12 +35,20 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName,
       expect(provided[:sanitized_name]).to eq('model_nos')
     end
 
-    it 'id suffix' do
+    it 'replaces id suffix' do
       provided = process do |instance|
         expect(instance).to receive(:unsanitized_name).and_return('Model Id')
       end
 
       expect(provided[:sanitized_name]).to eq('model_no')
     end
+
+    it 'replaces non-ASCII characters' do
+      provided = process do |instance|
+        expect(instance).to receive(:unsanitized_name).and_return('Ærøskøbing Ät Mödél')
+      end
+
+      expect(provided[:sanitized_name]).to eq('a_eroskobing_at_model')
+    end
   end
 end

spec/lib/sequencer/unit/import/zendesk/ticket/comment/source_based_spec.rb

@@ -0,0 +1,39 @@
+require 'rails_helper'
+
+RSpec.describe Sequencer::Unit::Import::Zendesk::Ticket::Comment::SourceBased, sequencer: :unit do
+
+  before(:all) do
+
+    described_class.class_eval do
+
+      private
+
+      def email
+        'test@example.com'
+      end
+    end
+  end
+
+  def parameters_for_channel(channel)
+    {
+      resource: double(
+        via: double(
+          channel: channel
+        )
+      )
+    }
+  end
+
+  context 'for resource.via.channel attribute' do
+
+    it 'provides from existing method' do
+      provided = process(parameters_for_channel('email'))
+      expect(provided[:source_based]).to eq('test@example.com')
+    end
+
+    it 'provides nil for non existing method' do
+      provided = process(parameters_for_channel('system'))
+      expect(provided[:source_based]).to be_nil
+    end
+  end
+end

spec/models/import_job_spec.rb

@@ -120,7 +120,7 @@ RSpec.describe ImportJob do
     it 'logs errors for invalid registered backends' do
       allow(Setting).to receive(:get)
       expect(Setting).to receive(:get).with('import_backends').and_return(['InvalidBackend'])
-      expect(Rails.logger).to receive(:error)
+      expect(described_class.logger).to receive(:error)
       described_class.queue_registered
     end
 

spec/models/scheduler_spec.rb

@@ -99,7 +99,9 @@ RSpec.describe Scheduler do
       described_class.cleanup
     end
 
-    it 'keeps unlocked Delayed::Job-s' do
+    context 'Delayed::Job' do
+
+      it 'keeps unlocked' do
         # meta :)
         described_class.delay.cleanup
 
@@ -110,7 +112,7 @@ RSpec.describe Scheduler do
         }
       end
 
-    context 'locked Delayed::Job' do
+      context 'locked' do
 
         it 'gets destroyed' do
           # meta :)
@@ -163,4 +165,42 @@ RSpec.describe Scheduler do
         end
       end
     end
+
+    context 'ImportJob' do
+
+      context 'affected job' do
+
+        let(:job) { create(:import_job, started_at: 5.minutes.ago) }
+
+        it 'finishes stuck jobs' do
+
+          expect do
+            simulate_threads_call
+          end.to change {
+            job.reload.finished_at
+          }
+        end
+
+        it 'adds an error message to the result' do
+
+          expect do
+            simulate_threads_call
+          end.to change {
+            job.reload.result[:error]
+          }
+        end
+      end
+
+      it "doesn't change jobs added after stop" do
+
+        job = create(:import_job)
+
+        expect do
+          simulate_threads_call
+        end.not_to change {
+          job.reload
+        }
+      end
+    end
+  end
 end

test/browser/agent_ticket_overview_level0_test.rb

@@ -304,7 +304,7 @@ class AgentTicketOverviewLevel0Test < TestCase
 
     set(
       css: '.content.active .bulkAction [data-item="date"]',
-      value: '05/23/2088',
+      value: '05/23/2037',
     )
 
     select(

test/unit/html_sanitizer_test.rb

@@ -113,9 +113,26 @@ test 123
     assert_equal(HtmlSanitizer.strict('<table><tr style=" Font-size:0%"><td>123</td></tr></table>'), '<table><tr><td>123</td></tr></table>')
     assert_equal(HtmlSanitizer.strict('<table><tr style="font-size:0%;display: none;"><td>123</td></tr></table>'), '<table><tr><td>123</td></tr></table>')
     assert_equal(HtmlSanitizer.strict('<table><tr style="font-size:0%;visibility:hidden;"><td>123</td></tr></table>'), '<table><tr><td>123</td></tr></table>')
+    assert_equal(HtmlSanitizer.strict('<table><tr style="font-size:0%;visibility:hidden;"><td>123</td></tr></table>'), '<table><tr><td>123</td></tr></table>')
     assert_equal(HtmlSanitizer.strict('<a href="/some/path%20test.pdf">test</a>'), '<a href="/some/path%20test.pdf">test</a>')
     assert_equal(HtmlSanitizer.strict('<a href="https://somehost.domain/path%20test.pdf">test</a>'), '<a href="https://somehost.domain/path%20test.pdf" rel="nofollow noreferrer noopener" target="_blank" title="https://somehost.domain/path test.pdf">test</a>')
     assert_equal(HtmlSanitizer.strict('<a href="https://somehost.domain/zaihan%20test">test</a>'), '<a href="https://somehost.domain/zaihan%20test" rel="nofollow noreferrer noopener" target="_blank" title="https://somehost.domain/zaihan test">test</a>')
-  end
 
+    api_path            = Rails.configuration.api_path
+    http_type           = Setting.get('http_type')
+    fqdn                = Setting.get('fqdn')
+    attachment_url      = "#{http_type}://#{fqdn}#{api_path}/ticket_attachment/239/986/1653"
+    attachment_url_good = "#{attachment_url}?disposition=attachment"
+    attachment_url_evil = "#{attachment_url}?disposition=inline"
+    assert_equal(HtmlSanitizer.strict("<a href=\"#{attachment_url_evil}\">Evil link</a>"), "<a href=\"#{attachment_url_good}\" rel=\"nofollow noreferrer noopener\" target=\"_blank\" title=\"#{attachment_url_good}\">Evil link</a>")
+
+    assert_equal(HtmlSanitizer.strict("<a href=\"#{attachment_url_good}\">Good link</a>"), "<a href=\"#{attachment_url_good}\" rel=\"nofollow noreferrer noopener\" target=\"_blank\" title=\"#{attachment_url_good}\">Good link</a>")
+    assert_equal(HtmlSanitizer.strict("<a href=\"#{attachment_url}\">No disposition</a>"), "<a href=\"#{attachment_url}\" rel=\"nofollow noreferrer noopener\" target=\"_blank\" title=\"#{attachment_url}\">No disposition</a>")
+
+    different_fqdn_url = attachment_url_evil.gsub(fqdn, 'some.other.tld')
+    assert_equal(HtmlSanitizer.strict("<a href=\"#{different_fqdn_url}\">Different FQDN</a>"), "<a href=\"#{different_fqdn_url}\" rel=\"nofollow noreferrer noopener\" target=\"_blank\" title=\"#{different_fqdn_url}\">Different FQDN</a>")
+
+    attachment_url_evil_other = "#{attachment_url}?disposition=some_other"
+    assert_equal(HtmlSanitizer.strict("<a href=\"#{attachment_url_evil_other}\">Evil link</a>"), "<a href=\"#{attachment_url_good}\" rel=\"nofollow noreferrer noopener\" target=\"_blank\" title=\"#{attachment_url_good}\">Evil link</a>")
+  end
 end