From d4775ba5c548bde256baf508f32af77de3ed55e2 Mon Sep 17 00:00:00 2001 From: Alex Sherman Date: Tue, 4 May 2021 13:44:11 +0500 Subject: [PATCH] Use errors as first-class objects --- app/controllers/epp/base_controller.rb | 118 ++++++++------------- app/controllers/epp/contacts_controller.rb | 50 ++++----- app/controllers/epp/domains_controller.rb | 52 ++++----- app/controllers/epp/errors_controller.rb | 4 +- app/controllers/epp/polls_controller.rb | 10 +- app/controllers/epp/sessions_controller.rb | 63 +++++------ app/models/concerns/epp_errors.rb | 31 +++--- app/views/epp/error.xml.builder | 4 +- 8 files changed, 146 insertions(+), 186 deletions(-) diff --git a/app/controllers/epp/base_controller.rb b/app/controllers/epp/base_controller.rb index db28d5685..4e3ea32d1 100644 --- a/app/controllers/epp/base_controller.rb +++ b/app/controllers/epp/base_controller.rb @@ -28,27 +28,24 @@ module Epp protected def respond_with_command_failed_error(exception) - epp_errors << { - code: '2400', - msg: 'Command failed', - } + epp_errors.add(:epp_errors, + code: '2400', + message: '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', - } + epp_errors.add(:epp_errors, + code: '2303', + msg: 'Object does not exist') handle_errors end def respond_with_authorization_error - epp_errors << { - code: '2201', - msg: 'Authorization error', - } + epp_errors.add(:epp_errors, + code: '2201', + msg: 'Authorization error') handle_errors end @@ -63,10 +60,9 @@ module Epp def validate_against_schema return if %w[hello error].include?(params[:action]) schema.validate(params[:nokogiri_frame]).each do |error| - epp_errors << { - code: 2001, - msg: error - } + epp_errors.add(:epp_errors, + code: 2001, + msg: error) end handle_errors and return if epp_errors.any? end @@ -94,7 +90,7 @@ module Epp # ERROR + RESPONSE HANDLING def epp_errors - @errors ||= [] + @errors ||= ActiveModel::Errors.new(self) end def handle_errors(obj = nil) @@ -102,21 +98,9 @@ module Epp if obj obj.construct_epp_errors - @errors += obj.errors.where[:epp_errors].flatten + obj.errors.each { |error| @errors.import error } end - if params[:parsed_frame]&.at_css('update') - @errors.each_with_index do |errors, index| - if errors[:code] == '2304' && - errors[:value].present? && - errors[:value][:val] == DomainStatus::SERVER_DELETE_PROHIBITED && - errors[:value][:obj] == 'status' - @errors[index][:value][:val] = DomainStatus::PENDING_UPDATE - end - end - end - @errors.uniq! - render_epp_response '/epp/error' end @@ -133,10 +117,9 @@ module Epp return true end - epp_errors << { - msg: 'Parameter value policy error. Allowed only Latin characters.', - code: '2306' - } + epp_errors.add(:epp_errors, + msg: 'Parameter value policy error. Allowed only Latin characters.', + code: '2306') handle_errors and return false end @@ -180,10 +163,9 @@ module Epp else missing = el.present? ? el.text.blank? : true end - epp_errors << { - code: '2003', - msg: I18n.t('errors.messages.required_parameter_missing', key: "#{full_selector} [#{attr}]") - } if missing + epp_errors.add(:epp_errors, + code: '2003', + message: I18n.t('errors.messages.required_parameter_missing', key: "#{full_selector} [#{attr}]")) if missing end missing ? false : el # return last selector if it was present @@ -201,25 +183,22 @@ module Epp attribute = element[attribute_selector] unless attribute - epp_errors << { - code: '2003', - msg: I18n.t('errors.messages.required_parameter_missing', key: attribute_selector) - } + epp_errors.add(:epp_errors, + code: '2003', + msg: I18n.t('errors.messages.required_parameter_missing', key: attribute_selector)) return end return if options[:values].include?(attribute) if options[:policy] - epp_errors << { - code: '2306', - msg: I18n.t('attribute_is_invalid', attribute: attribute_selector) - } + epp_errors.add(:epp_errors, + code: '2306', + msg: I18n.t('attribute_is_invalid', attribute: attribute_selector)) else - epp_errors << { - code: '2004', - msg: I18n.t('parameter_value_range_error', key: attribute_selector) - } + epp_errors.add(:epp_errors, + code: '2004', + msg: I18n.t('parameter_value_range_error', key: attribute_selector)) end end @@ -231,30 +210,27 @@ module Epp attribute = element[attribute_selector] return if (attribute && options[:values].include?(attribute)) || !attribute - epp_errors << { - code: '2306', - msg: I18n.t('attribute_is_invalid', attribute: attribute_selector) - } + epp_errors.add(:epp_errors, + code: '2306', + msg: I18n.t('attribute_is_invalid', attribute: attribute_selector)) end def exactly_one_of(*selectors) full_selectors = create_full_selectors(*selectors) return if element_count(*full_selectors, use_prefix: false) == 1 - epp_errors << { - code: '2306', - msg: I18n.t(:exactly_one_parameter_required, params: full_selectors.join(' OR ')) - } + epp_errors.add(:epp_errors, + code: '2306', + msg: I18n.t(:exactly_one_parameter_required, params: full_selectors.join(' OR '))) end def mutually_exclusive(*selectors) full_selectors = create_full_selectors(*selectors) return if element_count(*full_selectors, use_prefix: false) <= 1 - epp_errors << { - code: '2306', - msg: I18n.t(:mutally_exclusive_params, params: full_selectors.join(', ')) - } + epp_errors.add(:epp_errors, + code: '2306', + msg: I18n.t(:mutally_exclusive_params, params: full_selectors.join(', '))) end def optional(selector, *validations) @@ -265,8 +241,8 @@ module Epp validations.each do |x| validator = "#{x.first[0]}_validator".camelize.constantize - err = validator.validate_epp(selector.split(' ').last, value) - epp_errors << err if err + result = validator.validate_epp(selector.split(' ').last, value) + epp_errors.add(:epp_errors, result) if result end end @@ -297,10 +273,9 @@ module Epp def xml_attrs_present?(ph, attributes) # TODO: THIS IS DEPRECATED AND WILL BE REMOVED IN FUTURE attributes.each do |x| - epp_errors << { - code: '2003', - msg: I18n.t('errors.messages.required_parameter_missing', key: x.last) - } unless has_attribute(ph, x) + epp_errors.add(:epp_errors, + code: '2003', + msg: I18n.t('errors.messages.required_parameter_missing', key: x.last)) unless has_attribute(ph, x) end epp_errors.empty? end @@ -355,10 +330,9 @@ module Epp def enforce_epp_session_timeout if epp_session.timed_out? - epp_errors << { - code: '2201', - msg: 'Authorization error: Session timeout', - } + epp_errors.add(:epp_errors, + code: '2201', + msg: 'Authorization error: Session timeout') handle_errors epp_session.destroy! else diff --git a/app/controllers/epp/contacts_controller.rb b/app/controllers/epp/contacts_controller.rb index 65354ff48..7d81c6e54 100644 --- a/app/controllers/epp/contacts_controller.rb +++ b/app/controllers/epp/contacts_controller.rb @@ -72,9 +72,10 @@ module Epp end def action_call_response(action:) - # rubocop:disable Style/AndOr - (handle_errors(@contact) and return) unless action.call - # rubocop:enable Style/AndOr + unless action.call + handle_errors(@contact) + return + end if opt_addr? @response_code = 1100 @@ -134,24 +135,16 @@ module Epp ident = params[:parsed_frame].css('ident') if ident.present? && ident.attr('type').blank? - epp_errors << { - code: '2003', - msg: I18n.t('errors.messages.required_ident_attribute_missing', key: 'type') - } + epp_errors.add(:epp_errors, + code: '2003', + msg: I18n.t('errors.messages.required_ident_attribute_missing', key: 'type')) end if ident.present? && ident.text != 'birthday' && ident.attr('cc').blank? - epp_errors << { - code: '2003', - msg: I18n.t('errors.messages.required_ident_attribute_missing', key: 'cc') - } + epp_errors.add(:epp_errors, + code: '2003', + msg: I18n.t('errors.messages.required_ident_attribute_missing', key: 'cc')) end - # if ident.present? && ident.attr('cc').blank? - # epp_errors << { - # code: '2003', - # msg: I18n.t('errors.messages.required_ident_attribute_missing', key: 'cc') - # } - # end contact_org_disabled fax_disabled status_editing_disabled @@ -178,28 +171,27 @@ module Epp return true if ENV['contact_org_enabled'] == 'true' return true if params[:parsed_frame].css('postalInfo org').text.blank? - epp_errors << { - code: '2306', - msg: "#{I18n.t(:contact_org_error)}: postalInfo > org [org]" - } + epp_errors.add(:epp_errors, + code: '2306', + msg: "#{I18n.t(:contact_org_error)}: postalInfo > org [org]" + ) end def fax_disabled return true if ENV['fax_enabled'] == 'true' return true if params[:parsed_frame].css('fax').text.blank? - epp_errors << { - code: '2306', - msg: "#{I18n.t(:contact_fax_error)}: fax [fax]" - } + epp_errors.add(:epp_errors, + code: '2306', + msg: "#{I18n.t(:contact_fax_error)}: fax [fax]") end def status_editing_disabled return true if Setting.client_status_editing_enabled return true if params[:parsed_frame].css('status').empty? - epp_errors << { - code: '2306', - msg: "#{I18n.t(:client_side_status_editing_error)}: status [status]" - } + epp_errors.add(:epp_errors, + code: '2306', + msg: "#{I18n.t(:client_side_status_editing_error)}: status [status]" + ) end def address_given? diff --git a/app/controllers/epp/domains_controller.rb b/app/controllers/epp/domains_controller.rb index 47a40857a..d2ec74fa9 100644 --- a/app/controllers/epp/domains_controller.rb +++ b/app/controllers/epp/domains_controller.rb @@ -90,10 +90,9 @@ module Epp action = params[:parsed_frame].css('transfer').first[:op] if @domain.non_transferable? - epp_errors << { - code: '2304', - msg: I18n.t(:object_status_prohibits_operation), - } + epp_errors.add(:epp_errors, + code: '2304', + msg: I18n.t(:object_status_prohibits_operation)) handle_errors return end @@ -102,10 +101,9 @@ module Epp wrong_transfer_code = provided_transfer_code != @domain.transfer_code if wrong_transfer_code - epp_errors << { - code: '2202', - msg: 'Invalid authorization information', - } + epp_errors.add(:epp_errors, + code: '2202', + msg: 'Invalid authorization information') handle_errors return end @@ -120,10 +118,9 @@ module Epp if @domain_transfer render_epp_response '/epp/domains/transfer' else - epp_errors << { - code: '2303', - msg: I18n.t('no_transfers_found') - } + epp_errors.add(:epp_errors, + code: '2303', + msg: I18n.t('no_transfers_found')) handle_errors end end @@ -184,11 +181,10 @@ module Epp def validate_transfer # period element is disabled for now if params[:parsed_frame].css('period').any? - epp_errors << { - code: '2307', - msg: I18n.t(:unimplemented_object_service), - value: { obj: 'period' } - } + epp_errors.add(:epp_errors, + code: '2307', + msg: I18n.t(:unimplemented_object_service), + value: { obj: 'period' }) end requires 'transfer > transfer' @@ -217,10 +213,10 @@ module Epp return true if Setting.client_status_editing_enabled return true if check_client_hold return true if params[:parsed_frame].css('status').empty? - epp_errors << { - code: '2306', - msg: "#{I18n.t(:client_side_status_editing_error)}: status [status]" - } + epp_errors.add(:epp_errors, + code: '2306', + msg: "#{I18n.t(:client_side_status_editing_error)}: status [status]" + ) end def check_client_hold @@ -232,17 +228,15 @@ module Epp @domain_pricelist = @domain.pricelist(operation, period.try(:to_i), unit) if @domain_pricelist.try(:price) # checking if price list is not found if current_user.registrar.balance < @domain_pricelist.price.amount - epp_errors << { - code: '2104', - msg: I18n.t('billing_failure_credit_balance_low') - } + epp_errors.add(:epp_errors, + code: '2104', + msg: I18n.t('billing_failure_credit_balance_low')) return false end else - epp_errors << { - code: '2104', - msg: I18n.t(:active_price_missing_for_this_operation) - } + epp_errors.add(:epp_errors, + code: '2104', + msg: I18n.t(:active_price_missing_for_this_operation)) return false end true diff --git a/app/controllers/epp/errors_controller.rb b/app/controllers/epp/errors_controller.rb index 6cb689166..e9f1ecea8 100644 --- a/app/controllers/epp/errors_controller.rb +++ b/app/controllers/epp/errors_controller.rb @@ -3,12 +3,12 @@ module Epp skip_authorization_check def error - epp_errors << { code: params[:code], msg: params[:msg] } + epp_errors.add(:epp_errors, code: params[:code], msg: params[:msg] ) render_epp_response '/epp/error' end def command_handler - epp_errors << { code: '2000', msg: 'Unknown command' } + epp_errors.add(:epp_errors, code: '2000', msg: 'Unknown command' ) render_epp_response '/epp/error' end end diff --git a/app/controllers/epp/polls_controller.rb b/app/controllers/epp/polls_controller.rb index a93fa300d..86218336b 100644 --- a/app/controllers/epp/polls_controller.rb +++ b/app/controllers/epp/polls_controller.rb @@ -43,11 +43,11 @@ module Epp @notification = current_user.unread_notifications.find_by(id: params[:parsed_frame].css('poll').first['msgID']) unless @notification - epp_errors << { - code: '2303', - msg: I18n.t('message_was_not_found'), - value: { obj: 'msgID', val: params[:parsed_frame].css('poll').first['msgID'] } - } + epp_errors.add(:epp_errors, + code: '2303', + msg: I18n.t('message_was_not_found'), + value: { obj: 'msgID', + val: params[:parsed_frame].css('poll').first['msgID'] }) handle_errors and return end diff --git a/app/controllers/epp/sessions_controller.rb b/app/controllers/epp/sessions_controller.rb index 04603dbe7..dea42458b 100644 --- a/app/controllers/epp/sessions_controller.rb +++ b/app/controllers/epp/sessions_controller.rb @@ -20,10 +20,9 @@ module Epp server_md5 = Certificate.parse_md_from_string(File.read(ENV['cert_path'])) if client_md5 != server_md5 - epp_errors << { - msg: 'Authentication error; server closing connection (certificate is not valid)', - code: '2501' - } + epp_errors.add(:epp_errors, + msg: 'Authentication error; server closing connection (certificate is not valid)', + code: '2501') success = false end @@ -32,56 +31,50 @@ module Epp if !Rails.env.development? && (!webclient_request && @api_user) unless @api_user.pki_ok?(request.env['HTTP_SSL_CLIENT_CERT'], request.env['HTTP_SSL_CLIENT_S_DN_CN']) - epp_errors << { - msg: 'Authentication error; server closing connection (certificate is not valid)', - code: '2501' - } + epp_errors.add(:epp_errors, + msg: 'Authentication error; server closing connection (certificate is not valid)', + code: '2501') success = false end end if success && !@api_user - epp_errors << { - msg: 'Authentication error; server closing connection (API user not found)', - code: '2501' - } + epp_errors.add(:epp_errors, + msg: 'Authentication error; server closing connection (API user not found)', + code: '2501') success = false end if success && !@api_user.try(:active) - epp_errors << { - msg: 'Authentication error; server closing connection (API user is not active)', - code: '2501' - } + epp_errors.add(:epp_errors, + msg: 'Authentication error; server closing connection (API user is not active)', + code: '2501') success = false end if success && @api_user.cannot?(:create, :epp_login) - epp_errors << { - msg: 'Authentication error; server closing connection (API user does not have epp role)', - code: '2501' - } + epp_errors.add(:epp_errors, + msg: 'Authentication error; server closing connection (API user does not have epp role)', + code: '2501') success = false end if success && !ip_white? - epp_errors << { - msg: 'Authentication error; server closing connection (IP is not whitelisted)', - code: '2501' - } + epp_errors.add(:epp_errors, + msg: 'Authentication error; server closing connection (IP is not whitelisted)', + code: '2501') success = false end if success && EppSession.limit_reached?(@api_user.registrar) - epp_errors << { - msg: 'Session limit exceeded; server closing connection (connection limit reached)', - code: '2502', - } + epp_errors.add(:epp_errors, + msg: 'Session limit exceeded; server closing connection (connection limit reached)', + code: '2502') success = false end @@ -98,10 +91,9 @@ module Epp already_authenticated = EppSession.exists?(session_id: epp_session_id) if already_authenticated - epp_errors << { - msg: 'Command use error; Already authenticated', - code: 2002, - } + epp_errors.add(:epp_errors, + msg: 'Command use error; Already authenticated', + code: 2002) handle_errors return end @@ -127,10 +119,9 @@ module Epp def logout unless signed_in? - epp_errors << { - code: 2201, - msg: 'Authorization error' - } + epp_errors.add(:epp_errors, + code: 2201, + msg: 'Authorization error') handle_errors return end diff --git a/app/models/concerns/epp_errors.rb b/app/models/concerns/epp_errors.rb index f1a3b7ede..ba2665ac9 100644 --- a/app/models/concerns/epp_errors.rb +++ b/app/models/concerns/epp_errors.rb @@ -5,35 +5,41 @@ module EppErrors end def construct_epp_errors - epp_errors = [] + epp_errors = ActiveModel::Errors.new(self) errors.each do |error| attr = error.attribute.to_s.split('.')[0].to_sym next if attr == :epp_errors if self.class.reflect_on_association(attr) - epp_errors << collect_child_errors(attr) + collect_child_errors(attr).each do |child_error| + epp_errors.import child_error + end end if self.class.reflect_on_aggregation(attr) aggregation = send(attr) - epp_errors << collect_aggregation_errors(aggregation) + collect_aggregation_errors(aggregation).each do |aggregation_error| + epp_errors.import aggregation_error + end next end - - epp_errors << collect_parent_errors(attr, error.message) + collect_parent_errors(attr, error.message).each do |parent_error| + epp_errors.import parent_error + end end - errors.add(:epp_errors, epp_errors.flatten) unless epp_errors.empty? + epp_errors.each { |epp_error| errors.import epp_error} + errors end def collect_parent_errors(attr, errors) errors = [errors] if errors.is_a?(String) - epp_errors = [] + epp_errors = ActiveModel::Errors.new(self) errors.each do |err| code, value = find_epp_code_and_value(err) next unless code msg = attr.to_sym == :base ? err : "#{err} [#{attr}]" - epp_errors << { code: code, msg: msg, value: value } + epp_errors.add(attr, code: code, msg: msg, value: value) end epp_errors end @@ -41,12 +47,13 @@ module EppErrors def collect_child_errors(attr) macro = self.class.reflect_on_association(attr).macro multi = [:has_and_belongs_to_many, :has_many] - # single = [:belongs_to, :has_one] - epp_errors = [] + epp_errors = ActiveModel::Errors.new(self) send(attr).each do |x| - x.errors.messages.each do |attribute, errors| - epp_errors << x.collect_parent_errors(attribute, errors) + x.errors.each do |error| + x.collect_parent_errors(error.attribute, error.message).each do |parent_error| + epp_errors.import parent_error + end end end if multi.include?(macro) diff --git a/app/views/epp/error.xml.builder b/app/views/epp/error.xml.builder index fceb800b7..bb3ffd117 100644 --- a/app/views/epp/error.xml.builder +++ b/app/views/epp/error.xml.builder @@ -1,6 +1,8 @@ xml.epp_head do xml.response do - @errors.each do |x| + @errors.each do |error| + x = error&.options + next if x.empty? xml.result('code' => x[:code]) do xml.msg(x[:msg], 'lang' => 'en') model_name = resource ? resource.model_name.singular.sub('epp_','') : controller.controller_name.singularize