From bb7950255e562548517ab0ee11f24b8969500984 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 6 Mar 2020 15:31:43 +0100 Subject: [PATCH] Follow up - bca16dee1685797eec5836f129b7e096774c9e9c - Enhancement: Provide unique error code for technical errors to make it easy to find them in the logs. - Show all error messages if `current_user` has `admin` permission(s) right away. - Pass messages of `ActionController::RoutingError`, `ActiveRecord::RecordNotFound`, `Exceptions::UnprocessableEntity` and `Exceptions::NotAuthorized` exceptions to user. - Fix regression: 'Not authorized' errors lack of cause. - Remove diverged behavior for different environments. --- .../application_controller/handles_errors.rb | 47 +++++++------ .../application_model/can_associations.rb | 12 ++-- .../application_model/can_cleanup_param.rb | 4 +- spec/requests/error_spec.rb | 70 +++++++++++++------ 4 files changed, 79 insertions(+), 54 deletions(-) diff --git a/app/controllers/application_controller/handles_errors.rb b/app/controllers/application_controller/handles_errors.rb index 819af4c88..ca8f8bf5d 100644 --- a/app/controllers/application_controller/handles_errors.rb +++ b/app/controllers/application_controller/handles_errors.rb @@ -32,7 +32,7 @@ module ApplicationController::HandlesErrors end def unauthorized(e) - error = humanize_error(e.message) + error = humanize_error(e) response.headers['X-Failure'] = error.fetch(:error_human, error[:error]) respond_to_exception(e, :unauthorized) http_log @@ -44,9 +44,9 @@ module ApplicationController::HandlesErrors status_code = Rack::Utils.status_code(status) respond_to do |format| - format.json { render json: humanize_error(e.message), status: status } + format.json { render json: humanize_error(e), status: status } format.any do - errors = humanize_error(e.message) + errors = humanize_error(e) @exception = e @message = errors[:error_human] || errors[:error] || param[:message] @traceback = !Rails.env.production? @@ -56,38 +56,39 @@ module ApplicationController::HandlesErrors end end - def humanize_error(error) + def humanize_error(e) + data = { - error: error + error: e.message } - case error - when /Validation failed: (.+?)(,|$)/i + if e.message =~ /Validation failed: (.+?)(,|$)/i data[:error_human] = $1 - when /(already exists|duplicate key|duplicate entry)/i + elsif e.message.match?(/(already exists|duplicate key|duplicate entry)/i) data[:error_human] = 'Object already exists!' - when /null value in column "(.+?)" violates not-null constraint/i + elsif e.message =~ /null value in column "(.+?)" violates not-null constraint/i data[:error_human] = "Attribute '#{$1}' required!" - when /Field '(.+?)' doesn't have a default value/i + elsif e.message =~ /Field '(.+?)' doesn't have a default value/i data[:error_human] = "Attribute '#{$1}' required!" - when 'Exceptions::NotAuthorized' + elsif e.message == 'Exceptions::NotAuthorized' data[:error] = 'Not authorized' data[:error_human] = data[:error] + elsif [ActionController::RoutingError, ActiveRecord::RecordNotFound, Exceptions::UnprocessableEntity, Exceptions::NotAuthorized].include?(e.class) + data[:error_human] = data[:error] end - if Rails.env.production? - 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 + if data[:error_human].present? + data[:error] = data[:error_human] + elsif !current_user&.permissions?('admin') + # 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 + data end end diff --git a/app/models/application_model/can_associations.rb b/app/models/application_model/can_associations.rb index 0cb97fde8..4aaa724fb 100644 --- a/app/models/application_model/can_associations.rb +++ b/app/models/application_model/can_associations.rb @@ -51,7 +51,7 @@ returns # complain if we found no reference if !lookup - raise ArgumentError, "No value found for '#{assoc_name}' with id #{item_id.inspect}" + raise Exceptions::UnprocessableEntity, "No value found for '#{assoc_name}' with id #{item_id.inspect}" end list.push item_id @@ -94,7 +94,7 @@ returns # complain if we found no reference if !lookup - raise ArgumentError, "No lookup value found for '#{assoc_name}': #{value.inspect}" + raise Exceptions::UnprocessableEntity, "No lookup value found for '#{assoc_name}': #{value.inspect}" end list.push lookup.id @@ -363,7 +363,7 @@ returns lookup = nil if class_object == User if !value.instance_of?(String) - raise ArgumentError, "String is needed as ref value #{value.inspect} for '#{assoc_name}'" + raise Exceptions::UnprocessableEntity, "String is needed as ref value #{value.inspect} for '#{assoc_name}'" end if !lookup @@ -378,7 +378,7 @@ returns # complain if we found no reference if !lookup - raise ArgumentError, "No lookup value found for '#{assoc_name}': #{value.inspect}" + raise Exceptions::UnprocessableEntity, "No lookup value found for '#{assoc_name}': #{value.inspect}" end # release data value @@ -408,7 +408,7 @@ returns lookup = nil if class_object == User if !item.instance_of?(String) - raise ArgumentError, "String is needed in array ref as ref value #{value.inspect} for '#{assoc_name}'" + raise Exceptions::UnprocessableEntity, "String is needed in array ref as ref value #{value.inspect} for '#{assoc_name}'" end if !lookup @@ -423,7 +423,7 @@ returns # complain if we found no reference if !lookup - raise ArgumentError, "No lookup value found for '#{assoc_name}': #{item.inspect}" + raise Exceptions::UnprocessableEntity, "No lookup value found for '#{assoc_name}': #{item.inspect}" end lookup_ids.push lookup.id diff --git a/app/models/application_model/can_cleanup_param.rb b/app/models/application_model/can_cleanup_param.rb index 778c64140..b900d87e5 100644 --- a/app/models/application_model/can_cleanup_param.rb +++ b/app/models/application_model/can_cleanup_param.rb @@ -28,7 +28,7 @@ returns end if params.nil? - raise ArgumentError, "No params for #{self}!" + raise Exceptions::UnprocessableEntity, "No params for #{self}!" end # cleanup each member of array @@ -61,7 +61,7 @@ returns next if data[name].blank? next if assoc.klass.lookup(id: data[name]) - raise ArgumentError, "Invalid value for param '#{name}': #{data[name].inspect}" + raise Exceptions::UnprocessableEntity, "Invalid value for param '#{name}': #{data[name].inspect}" end clean_params[attribute] = data[attribute] end diff --git a/spec/requests/error_spec.rb b/spec/requests/error_spec.rb index b58f03733..e721cc277 100644 --- a/spec/requests/error_spec.rb +++ b/spec/requests/error_spec.rb @@ -13,7 +13,7 @@ RSpec.describe 'Error handling', type: :request do # 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' + if message == 'Please contact your administrator' || message == 'Mysql2::Error' || message == 'PG::ForeignKeyViolation' expect(json_response['error']).to include(message) else expect(json_response['error']).to eq(message) @@ -33,34 +33,50 @@ RSpec.describe 'Error handling', type: :request do 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) + authenticated_as(requesting_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' + context 'agent user' do + let(:requesting_user) { create(:agent_user, groups: Group.all) } + let(:message) { 'Please contact your administrator' } + + 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 'requesting HTML' do - let(:title) { '422: Unprocessable Entity' } - let(:headline) { '422: The change you wanted was rejected.' } + context 'admin user' do + let(:requesting_user) { create(:admin_user, groups: Group.all) } - include_examples 'HTML response format' + if ActiveRecord::Base.connection_config[:adapter] == 'mysql2' + let(:message) { 'Mysql2::Error' } + else + let(:message) { 'PG::ForeignKeyViolation' } + 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 end @@ -112,12 +128,12 @@ RSpec.describe 'Error handling', type: :request do get '/tests/raised_exception', params: { exception: exception.name, message: message }, as: as end - shared_examples 'handles exception' do |exception, http_status, title, headline| + shared_examples 'exception check' do |message, exception, http_status, title, headline| context "#{exception} is raised" do let(:exception) { exception } let(:http_status) { http_status } - let(:message) { 'some error message' } + let(:message) { message } context 'requesting JSON' do include_examples 'JSON response format' @@ -132,10 +148,18 @@ RSpec.describe 'Error handling', type: :request do end end - include_examples 'handles exception', ActiveRecord::RecordNotFound, :not_found, '404: Not Found', '404: Requested resource was not found' + shared_examples 'handles exception' do |exception, http_status, title, headline| + include_examples 'exception check', 'some error message', exception, http_status, title, headline + end + + shared_examples 'masks exception' do |exception, http_status, title, headline| + include_examples 'exception check', 'Please contact your administrator', exception, http_status, title, headline + end + include_examples 'handles exception', Exceptions::NotAuthorized, :unauthorized, '401: Unauthorized', '401: Unauthorized' + include_examples 'handles exception', ActiveRecord::RecordNotFound, :not_found, '404: Not Found', '404: Requested resource was not found' include_examples 'handles exception', Exceptions::UnprocessableEntity, :unprocessable_entity, '422: Unprocessable Entity', '422: The change you wanted was rejected.' - include_examples 'handles exception', ArgumentError, :unprocessable_entity, '422: Unprocessable Entity', '422: The change you wanted was rejected.' - include_examples 'handles exception', StandardError, :internal_server_error, '500: Something went wrong', "500: We're sorry, but something went wrong." + include_examples 'masks exception', ArgumentError, :unprocessable_entity, '422: Unprocessable Entity', '422: The change you wanted was rejected.' + include_examples 'masks exception', StandardError, :internal_server_error, '500: Something went wrong', "500: We're sorry, but something went wrong." end end