Improve EPP error handling

Fixes #539

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,20 +91,8 @@ 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
 
@@ -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

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
 
     #

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,10 +182,12 @@ 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)
@@ -272,18 +279,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

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

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