Sutty/trabajo-afectivo
5
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

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

Browse source 
- 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.
This commit is contained in:
Thorsten Eckel 2020-03-06 15:31:43 +01:00
parent 584680e7e4
commit bb7950255e
4 changed files with 79 additions and 54 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

31
app/controllers/application_controller/handles_errors.rb
View file

@ -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,29 +56,30 @@ 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
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
@ -87,7 +88,7 @@ module ApplicationController::HandlesErrors
Rails.logger.error "#{error_code_prefix} #{data[:error]}"
data[:error] = "#{error_code_prefix} Please contact your administrator."
end
end
data
end
end

12
app/models/application_model/can_associations.rb
View file

@ -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

4
app/models/application_model/can_cleanup_param.rb
View file

@ -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

58
spec/requests/error_spec.rb
View file

@ -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,25 +33,40 @@ 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 '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 'admin user' do
let(:requesting_user) { create(:admin_user, groups: Group.all) }
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
@ -63,6 +78,7 @@ RSpec.describe 'Error handling', type: :request do
include_examples 'HTML response format'
end
end
end
context 'URL route does not exist' do
@ -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