Merge branch 'develop' into private-pull-request-1238

https://github.com/zammad/zammad/pull/1238

CHANGELOG.md

@@ -1,7 +1,7 @@
 # Change Log
 
-## [1.6.0](https://github.com/zammad/zammad/tree/1.6.0) (2017-xx-xx)
+## [2.1.0](https://github.com/zammad/zammad/tree/2.1.0) (2017-xx-xx)
-[Full Changelog](https://github.com/zammad/zammad/compare/1.4.0...1.5.0)
+[Full Changelog](https://github.com/zammad/zammad/compare/2.0.0...2.1.0)
 
 **Implemented enhancements:**
 

VERSION

@@ -1 +1 @@
-1.6.x
+2.1.x

app/assets/javascripts/app/controllers/monitoring.coffee

@@ -4,6 +4,7 @@ class Index extends App.ControllerSubContent
   events:
     'click .js-resetToken': 'resetToken'
     'click .js-select': 'selectAll'
+    'click .js-restartFailedJobs': 'restartFailedJobs'
 
   constructor: ->
     super
@@ -42,4 +43,14 @@ class Index extends App.ControllerSubContent
         @load()
     )
 
+  restartFailedJobs: (e) =>
+    e.preventDefault()
+    @ajax(
+      id:    'restart_failed_jobs_request'
+      type:  'POST'
+      url:   "#{@apiPath}/monitoring/restart_failed_jobs"
+      success: (data) =>
+        @load()
+    )
+
 App.Config.set('Monitoring', { prio: 3600, name: 'Monitoring', parent: '#system', target: '#system/monitoring', controller: Index, permission: ['admin.monitoring'] }, 'NavBarAdmin')

app/assets/javascripts/app/lib/app_post/_object_organization_autocompletion.coffee

@@ -113,6 +113,10 @@ class App.ObjectOrganizationAutocompletion extends App.Controller
       @createToken name, objectId
     else
       if object.email
+
+        # quote name for special character
+        if name.match(/\@|,|;|\^|\+|#|§|\$|%|&|\/|\(|\)|=|\?|!|\*|\[|\]/)
+          name = "\"#{name}\""
         name += " <#{object.email}>"
 
       @objectSelect.val(name)

app/assets/javascripts/app/views/monitoring.jst.eco

@@ -34,6 +34,9 @@
         <% end %>
       <% end %>
     </ul>
+    <% if _.contains(@data.actions, 'restart_failed_jobs'): %>
+      <button class="btn btn--primary js-restartFailedJobs"><%- @T('Restart failed jobs') %></button>
+    <% end %>
   </div>
 
-</div>
+</div>

app/controllers/monitoring_controller.rb

@@ -30,6 +30,7 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX
     token_or_permission_check
 
     issues = []
+    actions = Set.new
 
     # channel check
     last_run_tolerance = Time.zone.now - 1.hour
@@ -81,6 +82,11 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX
       issues.push 'scheduler not running'
     end
 
+    Scheduler.failed_jobs.each do |job|
+      issues.push "Failed to run scheduled job '#{job.name}'. Cause: #{job.error_message}"
+      actions.add(:restart_failed_jobs)
+    end
+
     token = Setting.get('monitoring_token')
 
     if issues.empty?
@@ -96,8 +102,9 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX
     result = {
       healthy: false,
       message: issues.join(';'),
-      issues: issues,
+      issues:  issues,
-      token: token,
+      actions: actions,
+      token:   token,
     }
     render json: result
   end
@@ -173,6 +180,14 @@ curl http://localhost/api/v1/monitoring/status?token=XXX
     render json: result, status: :created
   end
 
+  def restart_failed_jobs
+    access_check
+
+    Scheduler.restart_failed_jobs
+
+    render json: {}, status: :ok
+  end
+
   private
 
   def token_or_permission_check

app/models/scheduler.rb

@@ -169,9 +169,13 @@ class Scheduler < ApplicationModel
   end
 
   def self._start_job(job, try_count = 0, try_run_time = Time.zone.now)
-    job.last_run = Time.zone.now
+    job.update(
-    job.pid      = Thread.current.object_id
+      last_run:      Time.zone.now,
-    job.save
+      pid:           Thread.current.object_id,
+      status:        'ok',
+      error_message: '',
+    )
+
     logger.info "execute #{job.method} (try_count #{try_count})..."
     eval job.method() # rubocop:disable Lint/Eval
   rescue => e
@@ -197,7 +201,15 @@ class Scheduler < ApplicationModel
     if try_run_max > try_count
       _start_job(job, try_count, try_run_time)
     else
-      raise "STOP thread for #{job.method} after #{try_count} tries (#{e.inspect})"
+      @@jobs_started[ job.id ] = false
+      error = "Failed to run #{job.method} after #{try_count} tries #{e.inspect}"
+      logger.error error
+
+      job.update(
+        error_message: error,
+        status: 'error',
+        active: false,
+      )
     end
   end
 
@@ -255,4 +267,28 @@ class Scheduler < ApplicationModel
 
   end
 
+  # This function returns a list of failed jobs
+  #
+  # @example
+  #   Scheduler.failed_jobs
+  #
+  # return [Array]
+  def self.failed_jobs
+    where(status: 'error', active: false)
+  end
+
+  # This function restarts failed jobs to retry them
+  #
+  # @example
+  #   Scheduler.restart_failed_jobs
+  #
+  # return [true]
+  def self.restart_failed_jobs
+    failed_jobs.each do |job|
+      job.update(active: true)
+    end
+
+    true
+  end
+
 end

app/models/ticket/article.rb

@@ -16,8 +16,8 @@ class Ticket::Article < ApplicationModel
   belongs_to    :created_by,  class_name: 'User'
   belongs_to    :updated_by,  class_name: 'User'
   store         :preferences
-  before_create :check_subject, :check_message_id_md5
+  before_create :check_subject, :check_body, :check_message_id_md5
-  before_update :check_subject, :check_message_id_md5
+  before_update :check_subject, :check_body, :check_message_id_md5
 
   sanitized_html :body
 
@@ -284,6 +284,22 @@ returns
     true
   end
 
+  # strip body length or raise exception
+  def check_body
+    return true if body.blank?
+    limit = 1_500_000
+    current_length = body.length
+    if body.length > limit
+      if ApplicationHandleInfo.current.present? && ApplicationHandleInfo.current.split('.')[1] == 'postmaster'
+        logger.warn "WARNING: cut string because of database length #{self.class}.body(#{limit} but is #{current_length})"
+        self.body = body[0, limit]
+      else
+        raise Exceptions::UnprocessableEntity, "body if article is to large, #{current_length} chars - only #{limit} allowed"
+      end
+    end
+    true
+  end
+
   def history_log_attributes
     {
       related_o_id:           self['ticket_id'],

config/routes/monitoring.rb

@@ -1,8 +1,9 @@
 Zammad::Application.routes.draw do
   api_path = Rails.configuration.api_path
 
-  match api_path + '/monitoring/health_check',         to: 'monitoring#health_check', via: :get
+  match api_path + '/monitoring/health_check',         to: 'monitoring#health_check',        via: :get
-  match api_path + '/monitoring/status',               to: 'monitoring#status',       via: :get
+  match api_path + '/monitoring/status',               to: 'monitoring#status',              via: :get
-  match api_path + '/monitoring/token',                to: 'monitoring#token',        via: :post
+  match api_path + '/monitoring/token',                to: 'monitoring#token',               via: :post
+  match api_path + '/monitoring/restart_failed_jobs',  to: 'monitoring#restart_failed_jobs', via: :post
 
 end

db/migrate/20120101000001_create_base.rb

@@ -514,6 +514,8 @@ class CreateBase < ActiveRecord::Migration
       t.integer :prio,                                  null: false
       t.string :pid,                      limit: 250,   null: true
       t.string :note,                     limit: 250,   null: true
+      t.string :error_message,                          null: true
+      t.string :status,                                 null: true
       t.boolean :active,                                null: false, default: false
       t.integer :updated_by_id,                         null: false
       t.integer :created_by_id,                         null: false

db/migrate/20170515000001_scheduler_status.rb

@@ -0,0 +1,12 @@
+class SchedulerStatus < ActiveRecord::Migration
+  def up
+
+    # return if it's a new setup
+    return if !Setting.find_by(name: 'system_init_done')
+
+    change_table :schedulers do |t|
+      t.string :error_message, null: true
+      t.string :status, null: true
+    end
+  end
+end

spec/factories/scheduler.rb

@@ -0,0 +1,25 @@
+FactoryGirl.define do
+  sequence :test_scheduler_name do |n|
+    "Testscheduler#{n}"
+  end
+end
+
+FactoryGirl.define do
+
+  factory :scheduler do
+    name          { generate(:test_scheduler_name) }
+    last_run      { Time.zone.now }
+    pid           1337
+    prio          1
+    status        'ok'
+    active        true
+    period        { 10.minutes }
+    running       false
+    note          'test'
+    updated_by_id 1
+    created_by_id 1
+    created_at    1
+    updated_at    1
+    add_attribute(:method) { 'test' }
+  end
+end

spec/models/scheduler_spec.rb

@@ -26,6 +26,45 @@ RSpec.describe Scheduler do
     SpecSpace.send(:remove_const, :DelayedJobBackend)
   end
 
+  describe '.failed_jobs' do
+
+    it 'does list failed jobs' do
+      job = create(:scheduler, status: 'error', active: false)
+      failed_list = described_class.failed_jobs
+      expect(failed_list).to be_present
+      expect(failed_list).to include(job)
+    end
+
+  end
+
+  describe '.restart_failed_jobs' do
+
+    it 'does restart failed jobs' do
+      job = create(:scheduler, status: 'error', active: false)
+      described_class.restart_failed_jobs
+      job.reload
+      expect(job.active).to be true
+    end
+  end
+
+  describe '._start_job' do
+
+    it 'sets error status/message for failed jobs' do
+      job = create(:scheduler)
+      described_class._start_job(job)
+      expect(job.status).to eq 'error'
+      expect(job.active).to be false
+      expect(job.error_message).to be_present
+    end
+
+    it 'executes job that is expected to succeed' do
+      expect(Setting).to receive(:reload)
+      job = create(:scheduler, method: 'Setting.reload')
+      described_class._start_job(job)
+      expect(job.status).to eq 'ok'
+    end
+  end
+
   describe '.cleanup' do
 
     it 'gets called by .threads' do

test/controllers/monitoring_controller_test.rb

@@ -391,4 +391,10 @@ class MonitoringControllerTest < ActionDispatch::IntegrationTest
 
   end
 
+  test '09 check restart_failed_jobs' do
+    credentials = ActionController::HttpAuthentication::Basic.encode_credentials('monitoring-admin@example.com', 'adminpw')
+    post '/api/v1/monitoring/restart_failed_jobs', {}, @headers.merge('Authorization' => credentials)
+    assert_response(200)
+  end
+
 end

test/unit/ticket_article_dos_test.rb

@@ -0,0 +1,103 @@
+# encoding: utf-8
+require 'test_helper'
+
+class TicketArticleDos < ActiveSupport::TestCase
+
+  test 'check body size' do
+
+    org_community = Organization.create_if_not_exists(
+      name: 'Zammad Foundation',
+    )
+    user_community = User.create_or_update(
+      login: 'article.dos@example.org',
+      firstname: 'Article',
+      lastname: 'Dos',
+      email: 'article.dos@example.org',
+      password: '',
+      active: true,
+      roles: [ Role.find_by(name: 'Customer') ],
+      organization_id: org_community.id,
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+
+    UserInfo.current_user_id = user_community.id
+    ApplicationHandleInfo.current = 'test.postmaster'
+
+    ticket1 = Ticket.create!(
+      group_id: Group.first.id,
+      customer_id: user_community.id,
+      title: 'DoS 1!',
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+    article1 = Ticket::Article.create!(
+      ticket_id: ticket1.id,
+      type_id: Ticket::Article::Type.find_by(name: 'phone').id,
+      sender_id: Ticket::Article::Sender.find_by(name: 'Customer').id,
+      from: 'Zammad Feedback <feedback@example.org>',
+      body: Array.new(2_000_000) { [*'0'..'9', *'a'..'z', ' ', ' ', ' ', '. '].sample }.join,
+      internal: false,
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+    assert_equal(1_500_000, article1.body.length)
+
+    ticket2 = Ticket.create!(
+      group_id: Group.first.id,
+      customer_id: user_community.id,
+      title: 'DoS 2!',
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+    article2 = Ticket::Article.create!(
+      ticket_id: ticket2.id,
+      type_id: Ticket::Article::Type.find_by(name: 'phone').id,
+      sender_id: Ticket::Article::Sender.find_by(name: 'Customer').id,
+      from: 'Zammad Feedback <feedback@example.org>',
+      body: "\u0000#{Array.new(2_000_000) { [*'0'..'9', *'a'..'z', ' ', ' ', ' ', '. '].sample }.join}",
+      internal: false,
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+    assert_equal(1_500_000, article2.body.length)
+
+    ApplicationHandleInfo.current = 'web'
+
+    ticket3 = Ticket.create!(
+      group_id: Group.first.id,
+      customer_id: user_community.id,
+      title: 'DoS 3!',
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+
+    assert_raises(Exceptions::UnprocessableEntity) {
+      article3 = Ticket::Article.create!(
+        ticket_id: ticket3.id,
+        type_id: Ticket::Article::Type.find_by(name: 'phone').id,
+        sender_id: Ticket::Article::Sender.find_by(name: 'Customer').id,
+        from: 'Zammad Feedback <feedback@example.org>',
+        body: "\u0000#{Array.new(2_000_000) { [*'0'..'9', *'a'..'z', ' ', ' ', ' ', '. '].sample }.join}",
+        internal: false,
+        updated_by_id: 1,
+        created_by_id: 1,
+      )
+    }
+
+  end
+
+  test 'check body size / cut if email' do
+
+    email_raw_string = "From: me@example.com
+To: customer@example.com
+Subject: some new subject
+
+Some Text" + Array.new(2_000_000) { [*'0'..'9', *'a'..'z', ' ', ' ', ' ', '. '].sample }.join
+
+    ticket_p, article_p, user_p, mail = Channel::EmailParser.new.process({}, email_raw_string)
+    assert_equal(1_500_000, article_p.body.length)
+
+  end
+
+end