Enhancement: Provide unique error code for technical errors to make it easy to find them in the logs.

.gitlab-ci.yml

@@ -423,7 +423,7 @@ test:browser:integration:api_client_php:
     ZAMMAD_PHP_API_CLIENT_UNIT_TESTS_PASSWORD: "test"
   script:
     - RAILS_ENV=test bundle exec rake db:create
-    - bundle exec rake zammad:ci:test:start zammad:setup:auto_wizard
+    - RAILS_ENV=test bundle exec rake zammad:ci:test:start zammad:setup:auto_wizard
     - git clone https://github.com/zammad/zammad-api-client-php.git
     - php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');"
     - php composer-setup.php --install-dir=/usr/local/bin

app/controllers/application_controller/handles_errors.rb

@@ -75,8 +75,18 @@ module ApplicationController::HandlesErrors
       data[:error_human] = data[:error]
     end
 
-    if Rails.env.production? && data[:error_human].present?
+    if Rails.env.production?
-      data[:error] = data.delete(:error_human)
+      if data[:error_human].present?
+        data[:error] = data.delete(:error_human)
+      else
+        # We want to avoid leaking of internal information but also want the user
+        # to give the administrator a reference to find the cause of the error.
+        # Therefore we generate a one time unique error ID that can be used to
+        # search the logs and find the actual error message.
+        error_code_prefix = "Error ID #{SecureRandom.urlsafe_base64(6)}:"
+        Rails.logger.error "#{error_code_prefix} #{data[:error]}"
+        data[:error] = "#{error_code_prefix} Please contact your administrator."
+      end
     end
     data
   end

app/models/object_manager/attribute.rb

@@ -910,34 +910,49 @@ is certain attribute used by triggers, overviews or schedulers
   def check_name
     return if !name
 
-    raise 'Name can\'t get used, *_id and *_ids are not allowed' if name.match?(/.+?_(id|ids)$/i)
+    if name.match?(/.+?_(id|ids)$/i)
-    raise 'Spaces in name are not allowed' if name.match?(/\s/)
+      errors.add(:name, "can't get used because *_id and *_ids are not allowed")
-    raise 'Only letters from a-z, numbers from 0-9, and _ are allowed' if !name.match?(/^[a-z0-9_]+$/)
+    end
-    raise 'At least one letters is needed' if !name.match?(/[a-z]/)
+    if name.match?(/\s/)
+      errors.add(:name, 'spaces are not allowed')
+    end
+    if !name.match?(/^[a-z0-9_]+$/)
+      errors.add(:name, 'Only letters from a-z because numbers from 0-9 and _ are allowed')
+    end
+    if !name.match?(/[a-z]/)
+      errors.add(:name, 'At least one letters is needed')
+    end
 
     # do not allow model method names as attributes
     reserved_words = %w[destroy true false integer select drop create alter index table varchar blob date datetime timestamp url icon initials avatar permission validate subscribe unsubscribe translate search _type _doc _id id]
-    raise "#{name} is a reserved word, please choose a different one" if name.match?(/^(#{reserved_words.join('|')})$/)
+    if name.match?(/^(#{reserved_words.join('|')})$/)
+      errors.add(:name, "#{name} is a reserved word! (1)")
+    end
 
     # fixes issue #2236 - Naming an attribute "attribute" causes ActiveRecord failure
     begin
       ObjectLookup.by_id(object_lookup_id).constantize.instance_method_already_implemented? name
     rescue  ActiveRecord::DangerousAttributeError
-      raise "#{name} is a reserved word, please choose a different one"
+      errors.add(:name, "#{name} is a reserved word! (2)")
     end
 
     record = object_lookup.name.constantize.new
-    return true if !record.respond_to?(name.to_sym)
+    if record.respond_to?(name.to_sym) && record.attributes.key?(name) && new_record?
-    raise "#{name} already exists!" if record.attributes.key?(name) && new_record?
+      errors.add(:name, "#{name} already exists!")
-    return true if record.attributes.key?(name)
+    end
 
-    raise "#{name} is a reserved word, please choose a different one"
+    if errors.present?
+      raise ActiveRecord::RecordInvalid, self
+    end
+
+    true
   end
 
   def check_editable
     return if editable
 
-    raise 'Attribute not editable!'
+    errors.add(:name, 'Attribute not editable!')
+    raise ActiveRecord::RecordInvalid, self
   end
 
   private

spec/models/object_manager/attribute_spec.rb

@@ -66,14 +66,14 @@ RSpec.describe ObjectManager::Attribute, type: :model do
     it 'rejects ActiveRecord reserved word "attribute"' do
       expect do
         described_class.add attributes_for :object_manager_attribute_text, name: 'attribute'
-      end.to raise_error 'attribute is a reserved word, please choose a different one'
+      end.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Name attribute is a reserved word! (2)')
     end
 
     %w[destroy true false integer select drop create alter index table varchar blob date datetime timestamp url icon initials avatar permission validate subscribe unsubscribe translate search _type _doc _id id].each do |reserved_word|
       it "rejects Zammad reserved word '#{reserved_word}'" do
         expect do
           described_class.add attributes_for :object_manager_attribute_text, name: reserved_word
-        end.to raise_error "#{reserved_word} is a reserved word, please choose a different one"
+        end.to raise_error(ActiveRecord::RecordInvalid, /is a reserved word! \(1\)/)
       end
     end
 
@@ -81,7 +81,7 @@ RSpec.describe ObjectManager::Attribute, type: :model do
       it "rejects word '#{reserved_word}' which is used for database references" do
         expect do
           described_class.add attributes_for :object_manager_attribute_text, name: reserved_word
-        end.to raise_error "Name can't get used, *_id and *_ids are not allowed"
+        end.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: Name can't get used because *_id and *_ids are not allowed")
       end
     end
 

spec/requests/error_spec.rb

@@ -7,7 +7,18 @@ RSpec.describe 'Error handling', type: :request do
 
     it { expect(response).to have_http_status(http_status) }
     it { expect(json_response).to be_a_kind_of(Hash) }
-    it { expect(json_response['error']).to eq(message) }
+
+    it do
+      # There is a special case where we mask technical errors and return
+      # a random error code that can be easily found in the logs by an
+      # administrator. However, this makes it hard to check for the exact error
+      # message. Therefore we only check for the substring in this particular case
+      if message == 'Please contact your administrator'
+        expect(json_response['error']).to include(message)
+      else
+        expect(json_response['error']).to eq(message)
+      end
+    end
   end
 
   shared_examples 'HTML response format' do
@@ -20,6 +31,39 @@ RSpec.describe 'Error handling', type: :request do
     it { expect(response.body).to include(message) }
   end
 
+  context 'error with confidential message is raised' do
+
+    let(:admin_user) { create(:admin_user, groups: Group.all) }
+    let!(:ticket) { create(:ticket) }
+    let(:invalid_group_id) { 99_999 }
+    let(:message) { 'Please contact your administrator' }
+    let(:http_status) { :unprocessable_entity }
+
+    before do
+      # fake production ENV to enable error hiding
+      env = double(
+        production?:  true,
+        test?:        false,
+        development?: false
+      )
+      allow(::Rails).to receive(:env).and_return(env)
+
+      authenticated_as(admin_user)
+      put "/api/v1/tickets/#{ticket.id}?all=true", params: { group_id: invalid_group_id }, as: as
+    end
+
+    context 'requesting JSON' do
+      include_examples 'JSON response format'
+    end
+
+    context 'requesting HTML' do
+      let(:title) { '422: Unprocessable Entity' }
+      let(:headline) { '422: The change you wanted was rejected.' }
+
+      include_examples 'HTML response format'
+    end
+  end
+
   context 'URL route does not exist' do
 
     before do

test/integration/object_manager_test.rb

@@ -73,7 +73,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
     assert_not(attribute1)
 
     # create invalid attributes
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       ObjectManager::Attribute.add(
         object:        'Ticket',
         name:          'test2_id',
@@ -91,7 +91,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
         updated_by_id: 1,
       )
     end
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       ObjectManager::Attribute.add(
         object:        'Ticket',
         name:          'test3_ids',
@@ -286,7 +286,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
     )
     assert_equal(false, ObjectManager::Attribute.pending_migration?)
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       ObjectManager::Attribute.add(
         object:        'Ticket',
         name:          'test13|',
@@ -307,7 +307,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
     end
     assert_equal(false, ObjectManager::Attribute.pending_migration?)
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       ObjectManager::Attribute.add(
         object:        'Ticket',
         name:          'test14!',
@@ -328,7 +328,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
     end
     assert_equal(false, ObjectManager::Attribute.pending_migration?)
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       ObjectManager::Attribute.add(
         object:        'Ticket',
         name:          'test15ä',
@@ -370,7 +370,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
     end
     assert_equal(false, ObjectManager::Attribute.pending_migration?)
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       ObjectManager::Attribute.add(
         object:        'Ticket',
         name:          'delete',
@@ -391,7 +391,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
     assert_equal(false, ObjectManager::Attribute.pending_migration?)
 
     attribute_count = ObjectManager::Attribute.count
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       ObjectManager::Attribute.add(
         object:        'Ticket',
         name:          'updated_at',
@@ -413,7 +413,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
     end
     assert_equal(attribute_count, ObjectManager::Attribute.count)
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       ObjectManager::Attribute.add(
         object:        'Ticket',
         name:          'updated_AT',