From cee3f5468de13d5e0439cef96032e3d9b2fd9105 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 11 Sep 2019 18:42:13 +0300 Subject: [PATCH] Improve EPP error handling Fixes #539 --- app/controllers/epp/base_controller.rb | 85 ++++++------------- app/controllers/epp/contacts_controller.rb | 13 +-- app/controllers/epp/domains_controller.rb | 81 +++++++++--------- app/models/domain.rb | 13 +++ app/models/epp/contact.rb | 21 ++--- app/models/epp/domain.rb | 54 ++++++------ app/models/epp/response/result/code.rb | 2 + config/locales/en.yml | 1 - test/integration/epp/base_test.rb | 33 +++++++ test/integration/epp/contact/base_test.rb | 21 +++++ .../integration/epp/contact/info/base_test.rb | 21 ----- .../epp/contact/update/base_test.rb | 26 ------ test/integration/epp/domain/base_test.rb | 21 +++++ .../epp/domain/delete/base_test.rb | 26 ------ test/integration/epp/domain/info/base_test.rb | 21 ----- .../epp/domain/transfer/base_test.rb | 25 ------ test/models/epp/response/result/code_test.rb | 2 + 17 files changed, 195 insertions(+), 271 deletions(-) create mode 100644 test/integration/epp/base_test.rb create mode 100644 test/integration/epp/contact/base_test.rb create mode 100644 test/integration/epp/domain/base_test.rb delete mode 100644 test/integration/epp/domain/transfer/base_test.rb diff --git a/app/controllers/epp/base_controller.rb b/app/controllers/epp/base_controller.rb index bc19670fee..b5dc8f78e3 100644 --- a/app/controllers/epp/base_controller.rb +++ b/app/controllers/epp/base_controller.rb @@ -1,5 +1,5 @@ module Epp - class BaseController < ApplicationController + class BaseController < ActionController::Base layout false skip_before_action :verify_authenticity_token @@ -10,11 +10,31 @@ class BaseController < ApplicationController before_action :validate_request before_action :update_epp_session, if: 'signed_in?' - around_action :catch_epp_errors - helper_method :current_user helper_method :resource + rescue_from StandardError, with: :respond_with_command_failed_error + rescue_from ActiveRecord::RecordNotFound, with: :respond_with_object_does_not_exist_error + + protected + + def respond_with_command_failed_error(exception) + epp_errors << { + code: '2400', + msg: 'Command failed', + } + handle_errors + log_exception(exception) + end + + def respond_with_object_does_not_exist_error + epp_errors << { + code: '2303', + msg: 'Object does not exist', + } + handle_errors + end + def validate_against_schema return if ['hello', 'error', 'keyrelay'].include?(params[:action]) schema.validate(params[:nokogiri_frame]).each do |error| @@ -26,47 +46,6 @@ def validate_against_schema handle_errors and return if epp_errors.any? end - def catch_epp_errors - err = catch(:epp_error) do - yield - nil - end - return unless err - @errors = [err] - handle_errors - end - - rescue_from StandardError do |e| - @errors ||= [] - - if e.class == CanCan::AccessDenied - if @errors.blank? - @errors = [{ - msg: t('errors.messages.epp_authorization_error'), - code: '2201' - }] - end - else - if @errors.blank? - @errors = [{ - msg: 'Internal error.', - code: '2400' - }] - end - - if Rails.env.test? || Rails.env.development? - puts e.backtrace.reverse.join("\n") - puts "\n BACKTRACE REVERSED!\n" - puts "\n FROM-EPP-RESCUE: #{e.message}\n\n\n" - else - logger.error "FROM-EPP-RESCUE: #{e.message}" - logger.error e.backtrace.join("\n") - end - end - - render_epp_response '/epp/error' - end - def schema EPP_ALL_SCHEMA end @@ -112,25 +91,13 @@ def handle_errors(obj = nil) end end - # for debugging - if @errors.blank? - @errors << { - code: '1', - msg: 'handle_errors was executed when there were actually no errors' - } - end - @errors.uniq! - logger.error "\nFOLLOWING ERRORS OCCURRED ON EPP QUERY:" - logger.error @errors.inspect - logger.error "\n" - render_epp_response '/epp/error' end def render_epp_response(*args) - @response = render_to_string(*args) + @response = render_to_string(*args, formats: 'xml') render xml: @response write_to_epp_log end @@ -406,5 +373,9 @@ def counter_update(registrar_code, ip) logger.error "IPTABLES COUNTER UPDATE: cannot write #{ip} to #{counter_proc}: #{e}" end end + + def log_exception(exception) + notify_airbrake(exception) + end end end diff --git a/app/controllers/epp/contacts_controller.rb b/app/controllers/epp/contacts_controller.rb index ff5dc982fa..32b2050ca3 100644 --- a/app/controllers/epp/contacts_controller.rb +++ b/app/controllers/epp/contacts_controller.rb @@ -84,18 +84,7 @@ def find_password def find_contact code = params[:parsed_frame].css('id').text.strip.upcase - - @contact = Epp::Contact.find_by_epp_code(code) - - if @contact.blank? - epp_errors << { - code: '2303', - msg: t('errors.messages.epp_obj_does_not_exist'), - value: { obj: 'id', val: code } - } - fail CanCan::AccessDenied - end - @contact + @contact = Epp::Contact.find_by!(code: code) end # diff --git a/app/controllers/epp/domains_controller.rb b/app/controllers/epp/domains_controller.rb index 64d4e972e0..07c6485708 100644 --- a/app/controllers/epp/domains_controller.rb +++ b/app/controllers/epp/domains_controller.rb @@ -28,26 +28,38 @@ def create domain_name = DNS::DomainName.new(SimpleIDN.to_unicode(request_domain_name)) if domain_name.at_auction? - throw :epp_error, - code: '2306', - msg: 'Parameter value policy error: domain is at auction' + epp_errors << { + code: '2306', + msg: 'Parameter value policy error: domain is at auction', + } + handle_errors + return elsif domain_name.awaiting_payment? - throw :epp_error, - code: '2003', - msg: 'Required parameter missing; reserved>pw element required for reserved domains' + epp_errors << { + code: '2003', + msg: 'Required parameter missing; reserved>pw element required for reserved domains', + } + handle_errors + return elsif domain_name.pending_registration? registration_code = params[:parsed_frame].css('reserved > pw').text if registration_code.empty? - throw :epp_error, - code: '2003', - msg: 'Required parameter missing; reserved>pw element is required' + epp_errors << { + code: '2003', + msg: 'Required parameter missing; reserved>pw element is required', + } + handle_errors + return end unless domain_name.available_with_code?(registration_code) - throw :epp_error, - code: '2202', - msg: 'Invalid authorization information; invalid reserved>pw value' + epp_errors << { + code: '2202', + msg: 'Invalid authorization information; invalid reserved>pw value', + } + handle_errors + return end end end @@ -85,22 +97,15 @@ def create def update authorize! :update, @domain, @password - begin - if @domain.update(params[:parsed_frame], current_user) - if @domain.epp_pending_update.present? - render_epp_response '/epp/domains/success_pending' - else - render_epp_response '/epp/domains/success' - end - else - handle_errors(@domain) - end - rescue => e - if @domain.errors.any? - handle_errors(@domain) + + if @domain.update(params[:parsed_frame], current_user) + if @domain.epp_pending_update.present? + render_epp_response '/epp/domains/success_pending' else - throw e + render_epp_response '/epp/domains/success' end + else + handle_errors(@domain) end end @@ -177,14 +182,21 @@ def transfer action = params[:parsed_frame].css('transfer').first[:op] if @domain.non_transferable? - throw :epp_error, { + epp_errors << { code: '2304', - msg: I18n.t(:object_status_prohibits_operation) + msg: I18n.t(:object_status_prohibits_operation), } + handle_errors + return end @domain_transfer = @domain.transfer(params[:parsed_frame], action, current_user) + if @domain.errors[:epp_errors].any? + handle_errors(@domain) + return + end + if @domain_transfer render_epp_response '/epp/domains/transfer' else @@ -272,18 +284,7 @@ def validate_transfer def find_domain domain_name = params[:parsed_frame].css('name').text.strip.downcase - @domain = Epp::Domain.find_by_idn domain_name - - unless @domain - epp_errors << { - code: '2303', - msg: I18n.t('errors.messages.epp_domain_not_found'), - value: { obj: 'name', val: domain_name } - } - fail CanCan::AccessDenied - end - - @domain + @domain = Epp::Domain.find_by_idn!(domain_name) end def find_password diff --git a/app/models/domain.rb b/app/models/domain.rb index beef862e36..ee003ee9b0 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -261,6 +261,19 @@ def self.find_by_idn(name) domain end + def self.find_by_idn!(name) + begin + domain = find_by!(name: name) + rescue ActiveRecord::RecordNotFound + if name.include?('-') + unicode_name = SimpleIDN.to_unicode(name) + domain = find_by!(name: unicode_name) + end + end + + domain + end + def roid "EIS-#{id}" end diff --git a/app/models/epp/contact.rb b/app/models/epp/contact.rb index a33e55a0ee..8ea01b67d2 100644 --- a/app/models/epp/contact.rb +++ b/app/models/epp/contact.rb @@ -154,10 +154,17 @@ def update_attributes(frame, current_user) type: ident_frame.attr('type'), country_code: ident_frame.attr('cc')) - report_valid_ident_error if submitted_ident != identifier + if submitted_ident != identifier + add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.valid_ident')) + return + end else ident_update_attempt = ident_frame.text.present? && (ident_frame.text != ident) - report_ident_update_error if ident_update_attempt + + if ident_update_attempt + add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.ident_update')) + return + end identifier = Ident.new(code: ident, type: ident_frame.attr('type'), @@ -243,14 +250,4 @@ def add_legal_file_to_new frame frame.css("legalDocument").first.content = doc.path if doc&.persisted? self.legal_document_id = doc.id end - - private - - def report_valid_ident_error - throw :epp_error, { code: '2308', msg: I18n.t('epp.contacts.errors.valid_ident') } - end - - def report_ident_update_error - throw :epp_error, { code: '2308', msg: I18n.t('epp.contacts.errors.ident_update') } - end end diff --git a/app/models/epp/domain.rb b/app/models/epp/domain.rb index ff60efe02c..8de67e4c49 100644 --- a/app/models/epp/domain.rb +++ b/app/models/epp/domain.rb @@ -450,7 +450,10 @@ def domain_status_list_from(frame) def update(frame, current_user, verify = true) return super if frame.blank? - check_discarded + if discarded? + add_epp_error('2105', nil, nil, I18n.t(:object_is_not_eligible_for_renewal)) + return + end at = {}.with_indifferent_access at.deep_merge!(attrs_from(frame.css('chg'), current_user, 'chg')) @@ -525,7 +528,10 @@ def attach_legal_document(legal_document_data) end def epp_destroy(frame, user_id) - check_discarded + if discarded? + add_epp_error('2105', nil, nil, I18n.t(:object_is_not_eligible_for_renewal)) + return + end if doc = attach_legal_document(Epp::Domain.parse_legal_document_from_frame(frame)) frame.css("legalDocument").first.content = doc.path if doc&.persisted? @@ -544,10 +550,10 @@ def epp_destroy(frame, user_id) end def set_pending_delete! - throw :epp_error, { - code: '2304', - msg: I18n.t(:object_status_prohibits_operation) - } unless pending_deletable? + unless pending_deletable? + add_epp_error('2304', nil, nil, I18n.t(:object_status_prohibits_operation)) + return + end self.delete_date = Time.zone.today + Setting.redemption_grace_period.days + 1.day set_pending_delete @@ -590,7 +596,10 @@ def renew(cur_exp_date, period, unit = 'y') ### TRANSFER ### def transfer(frame, action, current_user) - check_discarded + if discarded? + add_epp_error('2105', nil, nil, I18n.t(:object_is_not_eligible_for_renewal)) + return + end @is_transfer = true @@ -609,10 +618,8 @@ def transfer(frame, action, current_user) def query_transfer(frame, current_user) if current_user.registrar == registrar - throw :epp_error, { - code: '2002', - msg: I18n.t(:domain_already_belongs_to_the_querying_registrar) - } + add_epp_error('2002', nil, nil, I18n.t(:domain_already_belongs_to_the_querying_registrar)) + return end transaction do @@ -646,11 +653,10 @@ def query_transfer(frame, current_user) def approve_transfer(frame, current_user) pt = pending_transfer + if current_user.registrar != pt.old_registrar - throw :epp_error, { - msg: I18n.t('transfer_can_be_approved_only_by_current_registrar'), - code: '2304' - } + add_epp_error('2304', nil, nil, I18n.t('transfer_can_be_approved_only_by_current_registrar')) + return end transaction do @@ -672,11 +678,10 @@ def approve_transfer(frame, current_user) def reject_transfer(frame, current_user) pt = pending_transfer + if current_user.registrar != pt.old_registrar - throw :epp_error, { - msg: I18n.t('transfer_can_be_rejected_only_by_current_registrar'), - code: '2304' - } + add_epp_error('2304', nil, nil, I18n.t('transfer_can_be_rejected_only_by_current_registrar')) + return end transaction do @@ -814,15 +819,4 @@ def check_availability(domain_names) result end end - - private - - def check_discarded - if discarded? - throw :epp_error, { - code: '2105', - msg: I18n.t(:object_is_not_eligible_for_renewal), - } - end - end end diff --git a/app/models/epp/response/result/code.rb b/app/models/epp/response/result/code.rb index 2566279f54..13e83969a9 100644 --- a/app/models/epp/response/result/code.rb +++ b/app/models/epp/response/result/code.rb @@ -25,6 +25,7 @@ class Code object_association_prohibits_operation: 2305, parameter_value_policy_error: 2306, data_management_policy_violation: 2308, + command_failed: 2400, authentication_error_server_closing_connection: 2501, }.freeze private_constant :KEY_TO_VALUE @@ -50,6 +51,7 @@ class Code 2305 => 'Object association prohibits operation', 2306 => 'Parameter value policy error', 2308 => 'Data management policy violation', + 2400 => 'Command failed', 2501 => 'Authentication error; server closing connection', }.freeze private_constant :DEFAULT_DESCRIPTIONS diff --git a/config/locales/en.yml b/config/locales/en.yml index 8beb4bed23..9d89726b17 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -202,7 +202,6 @@ en: epp_domain_zone_with_same_origin: Zone with the same origin exists epp_domain_at_auction: Domain is at auction epp_domain_awaiting_payment: Awaiting payment - epp_obj_does_not_exist: 'Object does not exist' epp_authorization_error: 'Authorization error' epp_id_taken: 'Contact id already exists' epp_domain_not_found: 'Domain not found' diff --git a/test/integration/epp/base_test.rb b/test/integration/epp/base_test.rb new file mode 100644 index 0000000000..050b2e5b85 --- /dev/null +++ b/test/integration/epp/base_test.rb @@ -0,0 +1,33 @@ +require 'test_helper' + +class DummyEppController < Epp::BaseController + def internal_error + raise StandardError + end +end + +class EppBaseTest < EppTestCase + def test_graceful_exception_handling + Rails.application.routes.draw do + post 'epp/command/internal_error', to: 'dummy_epp#internal_error', constraints: EppConstraint.new(:poll) + end + + request_xml = <<-XML + + + + + + any + + + + + XML + post '/epp/command/internal_error', { frame: request_xml }, 'HTTP_COOKIE' => 'session=api_bestnames' + + assert_epp_response :command_failed + + Rails.application.reload_routes! + end +end diff --git a/test/integration/epp/contact/base_test.rb b/test/integration/epp/contact/base_test.rb new file mode 100644 index 0000000000..c332e7b361 --- /dev/null +++ b/test/integration/epp/contact/base_test.rb @@ -0,0 +1,21 @@ +require 'test_helper' + +class EppContactBaseTest < EppTestCase + def test_non_existent_contact + request_xml = <<-XML + + + + + + non-existent + + + + + XML + post '/epp/command/info', { frame: request_xml }, 'HTTP_COOKIE' => 'session=api_bestnames' + + assert_epp_response :object_does_not_exist + end +end diff --git a/test/integration/epp/contact/info/base_test.rb b/test/integration/epp/contact/info/base_test.rb index c871eb636a..d535d14d5e 100644 --- a/test/integration/epp/contact/info/base_test.rb +++ b/test/integration/epp/contact/info/base_test.rb @@ -43,27 +43,6 @@ def test_returns_valid_response contact: xml_schema).text end - def test_contact_not_found - assert_nil Contact.find_by(code: 'non-existing') - - request_xml = <<-XML - - - - - - non-existing - - - - - XML - - post '/epp/command/info', { frame: request_xml }, 'HTTP_COOKIE' => 'session=api_bestnames' - - assert_epp_response :object_does_not_exist - end - private def xml_schema diff --git a/test/integration/epp/contact/update/base_test.rb b/test/integration/epp/contact/update/base_test.rb index a8bb1d7a20..2362a44697 100644 --- a/test/integration/epp/contact/update/base_test.rb +++ b/test/integration/epp/contact/update/base_test.rb @@ -133,32 +133,6 @@ def test_skips_notifying_a_contact_when_a_contact_is_not_a_registrant assert_no_emails end - def test_non_existing_contact - assert_nil Contact.find_by(code: 'non-existing') - - request_xml = <<-XML - - - - - - non-existing - - - any - - - - - - - XML - - post '/epp/command/update', { frame: request_xml }, 'HTTP_COOKIE' => 'session=api_bestnames' - - assert_epp_response :object_does_not_exist - end - private def make_contact_free_of_domains_where_it_acts_as_a_registrant(contact) diff --git a/test/integration/epp/domain/base_test.rb b/test/integration/epp/domain/base_test.rb new file mode 100644 index 0000000000..b12fcf8da2 --- /dev/null +++ b/test/integration/epp/domain/base_test.rb @@ -0,0 +1,21 @@ +require 'test_helper' + +class EppDomainBaseTest < EppTestCase + def test_non_existent_domain + request_xml = <<-XML + + + + + + non-existent.test + + + + + XML + post '/epp/command/info', { frame: request_xml }, 'HTTP_COOKIE' => 'session=api_bestnames' + + assert_epp_response :object_does_not_exist + end +end diff --git a/test/integration/epp/domain/delete/base_test.rb b/test/integration/epp/domain/delete/base_test.rb index c1f52166dd..cf22f4a2c2 100644 --- a/test/integration/epp/domain/delete/base_test.rb +++ b/test/integration/epp/domain/delete/base_test.rb @@ -207,30 +207,4 @@ def test_domain_cannot_be_deleted_when_explicitly_prohibited_by_registrar assert_epp_response :object_status_prohibits_operation end - - def test_domain_not_found - assert_nil Domain.find_by(name: 'non-existing.test') - - request_xml = <<-XML - - - - - - non-existing.test - - - - - dGVzdCBmYWlsCg== - - - - - XML - - post '/epp/command/delete', { frame: request_xml }, 'HTTP_COOKIE' => 'session=api_bestnames' - - assert_epp_response :object_does_not_exist - end end \ No newline at end of file diff --git a/test/integration/epp/domain/info/base_test.rb b/test/integration/epp/domain/info/base_test.rb index 0aebc4de51..fd90177ca3 100644 --- a/test/integration/epp/domain/info/base_test.rb +++ b/test/integration/epp/domain/info/base_test.rb @@ -105,25 +105,4 @@ def test_conceals_transfer_code_when_domain_is_not_owned_by_current_user assert_nil response_xml.at_xpath('//domain:authInfo/domain:pw', 'domain' => 'https://epp.tld.ee/schema/domain-eis-1.0.xsd') end - - def test_returns_not_found_error_when_domain_is_not_registered - assert DNS::DomainName.new('not-registered.test').not_registered? - - request_xml = <<-XML - - - - - - not-registered.test - - - - - XML - - post '/epp/command/info', { frame: request_xml }, 'HTTP_COOKIE' => 'session=api_bestnames' - - assert_epp_response :object_does_not_exist - end end \ No newline at end of file diff --git a/test/integration/epp/domain/transfer/base_test.rb b/test/integration/epp/domain/transfer/base_test.rb deleted file mode 100644 index 5c15ae8816..0000000000 --- a/test/integration/epp/domain/transfer/base_test.rb +++ /dev/null @@ -1,25 +0,0 @@ -require 'test_helper' - -class EppDomainTransferBaseTest < EppTestCase - def test_non_existent_domain - request_xml = <<-XML - - - - - - non-existent.test - - any - - - - - - XML - - post '/epp/command/transfer', { frame: request_xml }, { 'HTTP_COOKIE' => 'session=api_goodnames' } - - assert_epp_response :object_does_not_exist - end -end diff --git a/test/models/epp/response/result/code_test.rb b/test/models/epp/response/result/code_test.rb index 0a5f507d39..90009a37e2 100644 --- a/test/models/epp/response/result/code_test.rb +++ b/test/models/epp/response/result/code_test.rb @@ -46,6 +46,7 @@ def test_returns_code_values object_association_prohibits_operation: 2305, parameter_value_policy_error: 2306, data_management_policy_violation: 2308, + command_failed: 2400, authentication_error_server_closing_connection: 2501, } assert_equal codes, Epp::Response::Result::Code.codes @@ -73,6 +74,7 @@ def test_returns_default_descriptions 2305 => 'Object association prohibits operation', 2306 => 'Parameter value policy error', 2308 => 'Data management policy violation', + 2400 => 'Command failed', 2501 => 'Authentication error; server closing connection', } assert_equal descriptions, Epp::Response::Result::Code.default_descriptions