Merge branch 'master' into refactor-contact-archivation

CHANGELOG.md

@@ -1,3 +1,8 @@
+03.09.2020
+* Refactored session timeout management [#711](https://github.com/internetee/registry/issues/711)
+* Improved error handling for epp requests without proper session [#1276](https://github.com/internetee/registry/pull/1276)
+* Refactored legal document epp extension [#1451](https://github.com/internetee/registry/pull/1451)
+
 01.09.2020
 * Removed some unused settings from admin [#1668](https://github.com/internetee/registry/issues/1668)
 

app/controllers/epp/base_controller.rb

@@ -10,7 +10,8 @@ module Epp
     before_action :latin_only
     before_action :validate_against_schema
     before_action :validate_request
-    before_action :update_epp_session, if: -> { signed_in? }
+    before_action :enforce_epp_session_timeout, if: :signed_in?
+    before_action :iptables_counter_update, if: :signed_in?
 
     around_action :wrap_exceptions
 
@@ -349,32 +350,22 @@ module Epp
       raise 'EPP session id is empty' unless epp_session_id.present?
     end
 
-    def update_epp_session
-      iptables_counter_update
-
-      if session_timeout_reached?
-        @api_user = current_user # cache current_user for logging
-        epp_session.destroy
-
+    def enforce_epp_session_timeout
+      if epp_session.timed_out?
         epp_errors << {
-          msg: t('session_timeout'),
-          code: '2201'
+          code: '2201',
+          msg: 'Authorization error: Session timeout',
         }
-
-        handle_errors and return
+        handle_errors
+        epp_session.destroy!
       else
-        epp_session.update_column(:updated_at, Time.zone.now)
+        epp_session.update_last_access
       end
     end
 
-    def session_timeout_reached?
-      timeout = 5.minutes
-      epp_session.updated_at < (Time.zone.now - timeout)
-    end
-
     def iptables_counter_update
       return if ENV['iptables_counter_enabled'].blank? && ENV['iptables_counter_enabled'] != 'true'
-      return if current_user.blank?
+
       counter_update(current_user.registrar_code, ENV['iptables_server_ip'])
     end
 

app/controllers/epp/polls_controller.rb

@@ -1,8 +1,7 @@
 module Epp
   class PollsController < BaseController
-    skip_authorization_check # TODO: move authorization under ability
-
     def poll
+      authorize! :manage, :poll
       req_poll if params[:parsed_frame].css('poll').first['op'] == 'req'
       ack_poll if params[:parsed_frame].css('poll').first['op'] == 'ack'
     end

app/models/contact.rb

@@ -22,8 +22,6 @@ class Contact < ApplicationRecord
   alias_attribute :kind, :ident_type
   alias_attribute :copy_from_id, :original_id # Old attribute name; for PaperTrail
 
-  accepts_nested_attributes_for :legal_documents
-
   scope :email_verification_failed, lambda {
     joins('LEFT JOIN email_address_verifications emv ON contacts.email = emv.email')
       .where('success = false and verified_at IS NOT NULL')

app/models/domain.rb

@@ -55,7 +55,6 @@ class Domain < ApplicationRecord
   accepts_nested_attributes_for :dnskeys, allow_destroy: true
 
   has_many :legal_documents, as: :documentable
-  accepts_nested_attributes_for :legal_documents, reject_if: proc { |attrs| attrs[:body].blank? }
   has_many :registrant_verifications, dependent: :destroy
 
   after_initialize do

app/models/epp/expired_sessions.rb

@@ -0,0 +1,13 @@
+module Epp
+  class ExpiredSessions
+    attr_reader :sessions
+
+    def initialize(sessions)
+      @sessions = sessions
+    end
+
+    def clear
+      sessions.find_each(&:destroy!)
+    end
+  end
+end

app/models/epp_session.rb

@@ -3,6 +3,11 @@ class EppSession < ApplicationRecord
 
   validates :session_id, uniqueness: true, presence: true
 
+  class_attribute :timeout
+  self.timeout = (ENV['epp_session_timeout_seconds'] || 300).to_i.seconds
+
+  alias_attribute :last_access, :updated_at
+
   def self.limit_per_registrar
     4
   end
@@ -11,4 +16,21 @@ class EppSession < ApplicationRecord
     count = where(user_id: registrar.api_users.ids).where('updated_at >= ?', Time.zone.now - 1.second).count
     count >= limit_per_registrar
   end
+
+  def self.expired
+    interval = "#{timeout.parts.first.second} #{timeout.parts.first.first}"
+    where(':now > (updated_at + interval :interval)', now: Time.zone.now, interval: interval)
+  end
+
+  def update_last_access
+    touch
+  end
+
+  def timed_out?
+    (updated_at + self.class.timeout).past?
+  end
+
+  def expired?
+    timed_out?
+  end
 end

app/models/legal_document.rb

@@ -1,5 +1,4 @@
 class LegalDocument < ApplicationRecord
-  cattr_accessor :explicitly_write_file
   include EppErrors
   MIN_BODY_SIZE = (1.37 * 3.kilobytes).ceil
 
@@ -44,7 +43,7 @@ class LegalDocument < ApplicationRecord
         break unless File.file?(path)
     end
 
-    File.open(path, 'wb') { |f| f.write(binary) } if !Rails.env.test? || self.class.explicitly_write_file
+    File.open(path, 'wb') { |f| f.write(binary) } unless Rails.env.test?
     self.path = path
     self.checksum = digest
   end

config/application.yml.sample

@@ -153,6 +153,8 @@ lhv_keystore_password:
 lhv_ca_file: # Needed only in dev mode
 lhv_dev_mode: 'false'
 
+epp_session_timeout_seconds: '300'
+
 # Since the keys for staging are absent from the repo, we need to supply them separate for testing.
 test:
   payments_seb_bank_certificate: 'test/fixtures/files/seb_bank_cert.pem'

config/routes.rb

@@ -202,7 +202,7 @@ Rails.application.routes.draw do
 
     resources :zonefiles
     resources :zones, controller: 'dns/zones', except: %i[show destroy]
-    resources :legal_documents
+    resources :legal_documents, only: :show
     resources :prices, controller: 'billing/prices', except: %i[show destroy] do
       member do
         patch :expire

db/migrate/20191219112434_change_legal_documents_path_to_not_null.rb

@@ -0,0 +1,5 @@
+class ChangeLegalDocumentsPathToNotNull < ActiveRecord::Migration[5.0]
+  def change
+    change_column_null :legal_documents, :path, false
+  end
+end

db/migrate/20191219124429_change_legal_documents_document_type_to_not_null.rb

@@ -0,0 +1,5 @@
+class ChangeLegalDocumentsDocumentTypeToNotNull < ActiveRecord::Migration[5.0]
+  def change
+    change_column_null :legal_documents, :document_type, false
+  end
+end

db/structure.sql

@@ -1069,12 +1069,12 @@ ALTER SEQUENCE public.invoices_id_seq OWNED BY public.invoices.id;
 
 CREATE TABLE public.legal_documents (
     id integer NOT NULL,
-    document_type character varying,
+    document_type character varying NOT NULL,
     documentable_id integer,
     documentable_type character varying,
     created_at timestamp without time zone,
     creator_str character varying,
-    path character varying,
+    path character varying NOT NULL,
     checksum character varying
 );
 
@@ -4828,6 +4828,8 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20191203083643'),
 ('20191206183853'),
 ('20191212133136'),
+('20191219112434'),
+('20191219124429'),
 ('20191227110904'),
 ('20200113091254'),
 ('20200115102202'),
@@ -4850,3 +4852,4 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20200812090409'),
 ('20200812125810'),
 ('20200902131603');
+

lib/tasks/epp/clear_expired_sessions.rake

@@ -0,0 +1,7 @@
+namespace :epp do
+  desc 'Clear expired EPP sessions'
+
+  task clear_expired_sessions: :environment do
+    Epp::ExpiredSessions.new(EppSession.expired).clear
+  end
+end

test/fixtures/legal_documents.yml

@@ -0,0 +1,4 @@
+one:
+  documentable: shop (Domain)
+  document_type: pdf
+  path: some

test/integration/epp/base_test.rb

@@ -7,6 +7,14 @@ class DummyEppController < Epp::BaseController
 end
 
 class EppBaseTest < EppTestCase
+  setup do
+    @original_session_timeout = EppSession.timeout
+  end
+
+  teardown do
+    EppSession.timeout = @original_session_timeout
+  end
+
   def test_internal_error
     Rails.application.routes.draw do
       post 'epp/command/internal_error', to: 'dummy_epp#internal_error',
@@ -81,6 +89,63 @@ class EppBaseTest < EppTestCase
     assert_epp_response :authorization_error
   end
 
+  def test_deletes_session_when_timed_out
+    now = Time.zone.parse('2010-07-05')
+    travel_to now
+    timeout = 0.second
+    EppSession.timeout = timeout
+    session = epp_sessions(:api_bestnames)
+    session.update!(updated_at: now - timeout - 1.second)
+
+    authentication_enabled_epp_request_xml = <<-XML
+      <?xml version="1.0" encoding="UTF-8" standalone="no"?>
+      <epp xmlns="https://epp.tld.ee/schema/epp-ee-1.0.xsd">
+        <command>
+          <info>
+            <domain:info xmlns:domain="https://epp.tld.ee/schema/domain-eis-1.0.xsd">
+              <domain:name>#{domains(:shop).name}</domain:name>
+            </domain:info>
+          </info>
+        </command>
+      </epp>
+    XML
+    post '/epp/command/info', params: { frame: authentication_enabled_epp_request_xml },
+         headers: { 'HTTP_COOKIE' => "session=#{session.session_id}" }
+
+    assert_epp_response :authorization_error
+    assert_nil EppSession.find_by(session_id: session.session_id)
+  end
+
+  def test_session_last_access_is_updated_when_not_timed_out
+    now = Time.zone.parse('2010-07-05')
+    travel_to now
+    timeout = 1.seconds
+    EppSession.timeout = timeout
+    session = epp_sessions(:api_bestnames)
+    session.last_access = now - timeout
+
+    authentication_enabled_epp_request_xml = <<-XML
+      <?xml version="1.0" encoding="UTF-8" standalone="no"?>
+      <epp xmlns="https://epp.tld.ee/schema/epp-ee-1.0.xsd">
+        <command>
+          <info>
+            <domain:info xmlns:domain="https://epp.tld.ee/schema/domain-eis-1.0.xsd">
+              <domain:name>#{domains(:shop).name}</domain:name>
+            </domain:info>
+          </info>
+        </command>
+      </epp>
+    XML
+
+    post '/epp/command/info', params: { frame: authentication_enabled_epp_request_xml },
+         headers: { 'HTTP_COOKIE' => "session=#{session.session_id}" }
+
+    session.reload
+
+    assert_epp_response :completed_successfully
+    assert_equal now, session.last_access
+  end
+
   private
 
   def valid_command_path

test/integration/epp/domain/delete/base_test.rb

@@ -27,7 +27,7 @@ class EppDomainDeleteBaseTest < EppTestCase
           </delete>
           <extension>
             <eis:extdata xmlns:eis="https://epp.tld.ee/schema/eis-1.0.xsd">
-              <eis:legalDocument type="pdf">dGVzdCBmYWlsCg==</eis:legalDocument>
+              <eis:legalDocument type="pdf">#{'test' * 2000}</eis:legalDocument>
             </eis:extdata>
           </extension>
         </command>
@@ -35,6 +35,7 @@ class EppDomainDeleteBaseTest < EppTestCase
     XML
 
     post epp_delete_path, params: { frame: request_xml }, headers: { 'HTTP_COOKIE' => 'session=api_bestnames' }
+    # binding.pry
     assert_includes Domain.find_by(name: 'invalid.test').statuses, DomainStatus::PENDING_DELETE_CONFIRMATION
     assert_epp_response :completed_successfully_action_pending
   end
@@ -54,7 +55,7 @@ class EppDomainDeleteBaseTest < EppTestCase
           </delete>
           <extension>
             <eis:extdata xmlns:eis="https://epp.tld.ee/schema/eis-1.0.xsd">
-              <eis:legalDocument type="pdf">dGVzdCBmYWlsCg==</eis:legalDocument>
+              <eis:legalDocument type="pdf">#{'test' * 2000}</eis:legalDocument>
             </eis:extdata>
           </extension>
         </command>
@@ -82,7 +83,7 @@ class EppDomainDeleteBaseTest < EppTestCase
           </delete>
           <extension>
             <eis:extdata xmlns:eis="https://epp.tld.ee/schema/eis-1.0.xsd">
-              <eis:legalDocument type="pdf">dGVzdCBmYWlsCg==</eis:legalDocument>
+              <eis:legalDocument type="pdf">#{'test' * 2000}</eis:legalDocument>
             </eis:extdata>
           </extension>
         </command>
@@ -113,7 +114,7 @@ class EppDomainDeleteBaseTest < EppTestCase
           </delete>
           <extension>
             <eis:extdata xmlns:eis="https://epp.tld.ee/schema/eis-1.0.xsd">
-              <eis:legalDocument type="pdf">dGVzdCBmYWlsCg==</eis:legalDocument>
+              <eis:legalDocument type="pdf">#{'test' * 2000}</eis:legalDocument>
             </eis:extdata>
           </extension>
         </command>
@@ -144,7 +145,7 @@ class EppDomainDeleteBaseTest < EppTestCase
           </delete>
           <extension>
             <eis:extdata xmlns:eis="https://epp.tld.ee/schema/eis-1.0.xsd">
-              <eis:legalDocument type="pdf">dGVzdCBmYWlsCg==</eis:legalDocument>
+              <eis:legalDocument type="pdf">#{'test' * 2000}</eis:legalDocument>
             </eis:extdata>
           </extension>
         </command>

test/integration/epp/domain/transfer/request_test.rb

@@ -150,7 +150,7 @@ class EppDomainTransferRequestTest < EppTestCase
           </transfer>
           <extension>
             <eis:extdata xmlns:eis="https://epp.tld.ee/schema/eis-1.0.xsd">
-              <eis:legalDocument type="pdf">test</eis:legalDocument>
+              <eis:legalDocument type="pdf">#{'test' * 2000}</eis:legalDocument>
             </eis:extdata>
           </extension>
         </command>

test/integration/epp/poll_test.rb

@@ -124,4 +124,20 @@ class EppPollTest < EppTestCase
 
     assert_epp_response :object_does_not_exist
   end
+
+  def test_anonymous_user_cannot_access
+    request_xml = <<-XML
+      <?xml version="1.0" encoding="UTF-8" standalone="no"?>
+      <epp xmlns="https://epp.tld.ee/schema/epp-ee-1.0.xsd">
+        <command>
+          <poll op="req"/>
+        </command>
+      </epp>
+    XML
+
+    post '/epp/command/poll', params: { frame: request_xml },
+         headers: { 'HTTP_COOKIE' => 'session=non-existent' }
+
+    assert_epp_response :authorization_error
+  end
 end

test/jobs/domain_update_confirm_job_test.rb

@@ -7,7 +7,7 @@ class DomainUpdateConfirmJobTest < ActiveSupport::TestCase
     @domain = domains(:shop)
     @new_registrant = contacts(:william)
     @user = users(:api_bestnames)
-    @legal_doc_path = 'test/fixtures/files/legaldoc.pdf'
+    @legal_doc_path = "#{'test' * 2000}"
 
     @domain.update!(pending_json: { new_registrant_id: @new_registrant.id,
                                     new_registrant_name: @new_registrant.name,

test/models/epp_session_test.rb

@@ -3,6 +3,11 @@ require 'test_helper'
 class EppSessionTest < ActiveSupport::TestCase
   setup do
     @epp_session = epp_sessions(:api_bestnames)
+    @original_session_timeout = EppSession.timeout
+  end
+
+  teardown do
+    EppSession.timeout = @original_session_timeout
   end
 
   def test_valid
@@ -60,4 +65,39 @@ class EppSessionTest < ActiveSupport::TestCase
 
     refute EppSession.limit_reached?(registrars(:bestnames))
   end
+
+  def test_expired_scope
+    now = Time.zone.parse('2010-07-05')
+    travel_to now
+    session = epp_sessions(:api_bestnames)
+    timeout = 0.seconds
+    EppSession.timeout = timeout
+
+    session.update!(last_access: now - timeout - 1.second)
+    assert_includes EppSession.expired, session, 'Expired session should be returned'
+
+    session.update!(last_access: now - timeout)
+
+    assert_not_includes EppSession.expired, session, 'Unexpired session should not be returned'
+  end
+
+  def test_expired_when_timed_out
+    now = Time.zone.parse('2010-07-05')
+    travel_to now
+    timeout = 0.seconds
+    EppSession.timeout = timeout
+    @epp_session.last_access = now - timeout - 1.second
+
+    assert @epp_session.expired?
+  end
+
+  def test_not_expired_when_not_timed_out
+    now = Time.zone.parse('2010-07-05')
+    travel_to now
+    timeout = 0.seconds
+    EppSession.timeout = timeout
+    @epp_session.last_access = now - timeout
+
+    assert_not @epp_session.expired?
+  end
 end

test/models/legal_document_test.rb

@@ -0,0 +1,14 @@
+require 'test_helper'
+
+class LegalDocumentTest < ActiveSupport::TestCase
+  def test_valid_legal_document_fixture_is_valid
+    assert valid_legal_document.valid?, proc { valid_legal_document.errors.full_messages }
+  end
+
+  private
+
+  def valid_legal_document
+    legal_documents(:one)
+  end
+end
+

test/tasks/epp/clear_expired_sessions_test.rb

@@ -0,0 +1,29 @@
+require 'test_helper'
+
+class EppClearExpiredSessionsTaskTest < ActiveSupport::TestCase
+  setup do
+    @original_session_timeout = EppSession.timeout
+  end
+
+  teardown do
+    EppSession.timeout = @original_session_timeout
+  end
+
+  def test_clears_expired_epp_sessions
+    timeout = EppSession.timeout
+    session = epp_sessions(:api_bestnames)
+    next_session = epp_sessions(:api_goodnames)
+    session.update!(updated_at: Time.zone.now - timeout - 1.second)
+
+    run_task
+
+    assert_nil EppSession.find_by(session_id: session.session_id)
+    assert EppSession.find_by(session_id: next_session.session_id)
+  end
+
+  private
+
+  def run_task
+    Rake::Task['epp:clear_expired_sessions'].execute
+  end
+end