From cba445ee5bc2556af6a91df0e8f5dc08dfeb4bdc Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Thu, 25 Jun 2020 12:40:57 +0300 Subject: [PATCH 1/7] Create contact XML deserializer --- lib/deserializers/xml/contact.rb | 39 ++++++++++++ test/lib/deserializers/xml/contact_test.rb | 73 ++++++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 lib/deserializers/xml/contact.rb create mode 100644 test/lib/deserializers/xml/contact_test.rb diff --git a/lib/deserializers/xml/contact.rb b/lib/deserializers/xml/contact.rb new file mode 100644 index 000000000..d0a2ce8f4 --- /dev/null +++ b/lib/deserializers/xml/contact.rb @@ -0,0 +1,39 @@ +module Deserializers + module Xml + class Contact + attr_reader :frame + + def initialize(frame) + @frame = frame + end + + def call + attributes = { + name: if_present('postalInfo name'), + org: if_present('postalInfo org'), + email: if_present('email'), + fax: if_present('fax'), + phone: if_present('voice'), + + # Address fields + city: if_present('postalInfo addr city'), + zip: if_present('postalInfo addr pc'), + street: if_present('postalInfo addr street'), + state: if_present('postalInfo addr sp'), + country_code: if_present('postalInfo addr cc'), + + # Auth info + auth_info: if_present('authInfo pw'), + } + + attributes.compact + end + + def if_present(css_path) + return unless frame.css(css_path).present? + + frame.css(css_path).text + end + end + end +end diff --git a/test/lib/deserializers/xml/contact_test.rb b/test/lib/deserializers/xml/contact_test.rb new file mode 100644 index 000000000..c755e8f24 --- /dev/null +++ b/test/lib/deserializers/xml/contact_test.rb @@ -0,0 +1,73 @@ +require 'test_helper' +require 'deserializers/xml/contact' + +class DeserializersXmlContactTest < ActiveSupport::TestCase + def test_trims_empty_values + xml_string = <<-XML + XML + + nokogiri_frame = Nokogiri::XML(xml_string).remove_namespaces! + instance = ::Deserializers::Xml::Contact.new(nokogiri_frame) + assert_equal instance.call, {} + end + + def test_handles_update + xml_string = <<-XML + + + + + + john-001 + + + new name + + +123.4 + new-email@inbox.test + + + + + + XML + + nokogiri_frame = Nokogiri::XML(xml_string).remove_namespaces! + instance = ::Deserializers::Xml::Contact.new(nokogiri_frame) + assert_equal instance.call, { name: 'new name', + email: 'new-email@inbox.test', + phone: '+123.4' } + end + + def test_handles_create + name = 'new' + email = 'new@registrar.test' + phone = '+1.2' + + xml_string = <<-XML + + + + + + + #{name} + + #{phone} + #{email} + + + + + any + + + + + XML + + nokogiri_frame = Nokogiri::XML(xml_string).remove_namespaces! + instance = ::Deserializers::Xml::Contact.new(nokogiri_frame) + assert_equal instance.call, { name: 'new', email: 'new@registrar.test', phone: '+1.2' } + end +end From d005a618889a3a5fec4ca10ac24b7d5bd12e46ec Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Fri, 26 Jun 2020 11:23:30 +0300 Subject: [PATCH 2/7] Move to new serializer --- app/models/epp/contact.rb | 50 ++++++++-------------- lib/deserializers/xml/contact.rb | 22 ++++++++++ test/lib/deserializers/xml/contact_test.rb | 39 +++++++++++++++++ 3 files changed, 78 insertions(+), 33 deletions(-) diff --git a/app/models/epp/contact.rb b/app/models/epp/contact.rb index d14bf1e1b..c86ed77da 100644 --- a/app/models/epp/contact.rb +++ b/app/models/epp/contact.rb @@ -1,5 +1,6 @@ require 'deserializers/xml/legal_document' require 'deserializers/xml/ident' +require 'deserializers/xml/contact' class Epp::Contact < Contact include EppErrors @@ -23,25 +24,8 @@ class Epp::Contact < Contact end def attrs_from(frame, new_record: false) - f = frame - at = {}.with_indifferent_access - at[:name] = f.css('postalInfo name').text if f.css('postalInfo name').present? - at[:org_name] = f.css('postalInfo org').text if f.css('postalInfo org').present? - at[:email] = f.css('email').text if f.css('email').present? - at[:fax] = f.css('fax').text if f.css('fax').present? - at[:phone] = f.css('voice').text if f.css('voice').present? - - if address_processing? - at[:city] = f.css('postalInfo addr city').text if f.css('postalInfo addr city').present? - at[:zip] = f.css('postalInfo addr pc').text if f.css('postalInfo addr pc').present? - at[:street] = f.css('postalInfo addr street').text if f.css('postalInfo addr street').present? - at[:state] = f.css('postalInfo addr sp').text if f.css('postalInfo addr sp').present? - at[:country_code] = f.css('postalInfo addr cc').text if f.css('postalInfo addr cc').present? - end - - at[:auth_info] = f.css('authInfo pw').text if f.css('authInfo pw').present? - - ident_attrs = ::Deserializers::Xml::Ident.new(f).call + at = ::Deserializers::Xml::Contact.new(frame).call + ident_attrs = ::Deserializers::Xml::Ident.new(frame).call at.merge!(ident_attrs) if new_record at end @@ -72,8 +56,8 @@ class Epp::Contact < Contact res end - end + delegate :ident_attr_valid?, to: :class def epp_code_map @@ -106,11 +90,11 @@ class Epp::Contact < Contact def update_attributes(frame, current_user) return super if frame.blank? - at = {}.with_indifferent_access - at.deep_merge!(self.class.attrs_from(frame.css('chg'), new_record: false)) + new_attributes = self.class.attrs_from(frame, new_record: false) if Setting.client_status_editing_enabled - at[:statuses] = statuses - statuses_attrs(frame.css('rem'), 'rem') + statuses_attrs(frame.css('add'), 'add') + new_attributes[:statuses] = + statuses - new_attributes[:statuses_to_remove] + new_attributes[:statuses_to_add] end if doc = attach_legal_document(::Deserializers::Xml::LegalDocument.new(frame).call) @@ -118,30 +102,30 @@ class Epp::Contact < Contact self.legal_document_id = doc.id end - ident_frame = frame.css('ident').first + ident_attributes = ::Deserializers::Xml::Ident.new(frame).call # https://github.com/internetee/registry/issues/576 - if ident_frame + if ident_attributes[:ident] if identifier.valid? - submitted_ident = Ident.new(code: ident_frame.text, - type: ident_frame.attr('type'), - country_code: ident_frame.attr('cc')) + submitted_ident = Ident.new(code: ident_attributes[:ident], + type: ident_attributes[:ident_type], + country_code: ident_attributes[:ident_country_code]) 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) + ident_update_attempt = ident_attributes[:ident] != ident 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'), - country_code: ident_frame.attr('cc')) + identifier = Ident.new(code: ident_attributes[:ident], + type: ident_attributes[:ident_type], + country_code: ident_attributes[:ident_country_code]) identifier.validate @@ -153,7 +137,7 @@ class Epp::Contact < Contact self.upid = current_user.registrar.id if current_user.registrar self.up_date = Time.zone.now - self.attributes = at + self.attributes = new_attributes email_changed = will_save_change_to_email? old_email = email_was diff --git a/lib/deserializers/xml/contact.rb b/lib/deserializers/xml/contact.rb index d0a2ce8f4..81eb90b51 100644 --- a/lib/deserializers/xml/contact.rb +++ b/lib/deserializers/xml/contact.rb @@ -24,6 +24,10 @@ module Deserializers # Auth info auth_info: if_present('authInfo pw'), + + # statuses + statuses_to_add: statuses_to_add, + statuses_to_remove: statuses_to_remove, } attributes.compact @@ -34,6 +38,24 @@ module Deserializers frame.css(css_path).text end + + def statuses_to_add + statuses_frame = frame.css('add') + return unless statuses_frame.present? + + statuses_frame.css('status').map do |status| + status['s'] + end + end + + def statuses_to_remove + statuses_frame = frame.css('rem') + return unless statuses_frame.present? + + statuses_frame.css('status').map do |status| + status['s'] + end + end end end end diff --git a/test/lib/deserializers/xml/contact_test.rb b/test/lib/deserializers/xml/contact_test.rb index c755e8f24..c49d919b5 100644 --- a/test/lib/deserializers/xml/contact_test.rb +++ b/test/lib/deserializers/xml/contact_test.rb @@ -70,4 +70,43 @@ class DeserializersXmlContactTest < ActiveSupport::TestCase instance = ::Deserializers::Xml::Contact.new(nokogiri_frame) assert_equal instance.call, { name: 'new', email: 'new@registrar.test', phone: '+1.2' } end + + def test_handles_statuses + xml_string = <<-XML + + + + + + john-001 + + + new name + + +123.4 + new-email@inbox.test + + + Payment overdue. + + + + + + + + + + XML + + nokogiri_frame = Nokogiri::XML(xml_string).remove_namespaces! + instance = ::Deserializers::Xml::Contact.new(nokogiri_frame) + assert_equal instance.call, { name: 'new name', + email: 'new-email@inbox.test', + phone: '+123.4', + statuses_to_add: ['clientDeleteProhibited', + 'clientUpdateProhibited'], + statuses_to_remove: ['pendingDelete'] + } + end end From 82a9565c1013d84f26bd76dc42a78e4814d7fec9 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Fri, 26 Jun 2020 14:31:12 +0300 Subject: [PATCH 3/7] Extract action --- app/controllers/epp/contacts_controller.rb | 11 ++- app/models/actions/contact_update.rb | 105 +++++++++++++++++++++ app/models/epp/contact.rb | 62 ------------ lib/deserializers/xml/contact_update.rb | 27 ++++++ 4 files changed, 141 insertions(+), 64 deletions(-) create mode 100644 app/models/actions/contact_update.rb create mode 100644 lib/deserializers/xml/contact_update.rb diff --git a/app/controllers/epp/contacts_controller.rb b/app/controllers/epp/contacts_controller.rb index b6a26a626..df9755af6 100644 --- a/app/controllers/epp/contacts_controller.rb +++ b/app/controllers/epp/contacts_controller.rb @@ -1,3 +1,5 @@ +require 'deserializers/xml/contact_update' + module Epp class ContactsController < BaseController before_action :find_contact, only: [:info, :update, :delete] @@ -43,9 +45,14 @@ module Epp def update authorize! :update, @contact, @password - frame = params[:parsed_frame] + collected_data = ::Deserializers::Xml::ContactUpdate.new(params[:parsed_frame]) + action = Actions::ContactUpdate.new(@contact, + collected_data.contact, + collected_data.legal_document, + collected_data.ident, + current_user) - if @contact.update_attributes(frame, current_user) + if action.call if !address_processing? && address_given? @response_code = 1100 @response_description = t('epp.contacts.completed_without_address') diff --git a/app/models/actions/contact_update.rb b/app/models/actions/contact_update.rb new file mode 100644 index 000000000..6b237ca65 --- /dev/null +++ b/app/models/actions/contact_update.rb @@ -0,0 +1,105 @@ +module Actions + class ContactUpdate + attr_reader :contact + attr_reader :new_attributes + attr_reader :legal_document + attr_reader :ident + attr_reader :user + + def initialize(contact, new_attributes, legal_document, ident, user) + @contact = contact + @new_attributes = new_attributes + @legal_document = legal_document + @ident = ident + @user = user + end + + def call + maybe_remove_address + maybe_update_statuses + maybe_update_ident + maybe_attach_legal_doc + commit + end + + def maybe_remove_address + return if Setting.address_processing? + + new_attributes.delete(:city) + new_attributes.delete(:zip) + new_attributes.delete(:street) + new_attributes.delete(:state) + new_attributes.delete(:country_code) + end + + def maybe_update_statuses + return unless Setting.client_status_editing_enabled + + new_statuses = + contact.statuses - new_attributes[:statuses_to_remove] + new_attributes[:statuses_to_add] + + new_attributes[:statuses] = new_statuses + end + + def maybe_attach_legal_doc + return unless legal_document + + document = contact.legal_documents.create( + document_type: legal_document[:type], + body: legal_document[:body] + ) + + contact.legal_document_id = document.id + end + + def maybe_update_ident + if ident[:ident] + if contact.identifier.valid? + submitted_ident = ::Contact::Ident.new(code: ident[:ident], + type: ident[:ident_type], + country_code: ident[:ident_country_code]) + + if submitted_ident != contact.identifier + contact.add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.valid_ident')) + @error = true + end + else + ident_update_attempt = ident[:ident] != contact.ident + + if ident_update_attempt + contact.add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.ident_update')) + @error = true + end + + identifier = ::Contact::Ident.new(code: ident[:ident], + type: ident[:ident_type], + country_code: ident[:ident_country_code]) + + identifier.validate + + contact.identifier = identifier + contact.ident_updated_at ||= Time.zone.now + end + end + end + + def commit + return false if @error + + contact.upid = user.registrar&.id + contact.up_date = Time.zone.now + + contact.attributes = new_attributes + + email_changed = contact.will_save_change_to_email? + old_email = contact.email_was + updated = contact.save + + if updated && email_changed && contact.registrant? + ContactMailer.email_changed(contact: contact, old_email: old_email).deliver_now + end + + updated + end + end +end diff --git a/app/models/epp/contact.rb b/app/models/epp/contact.rb index c86ed77da..bb2803980 100644 --- a/app/models/epp/contact.rb +++ b/app/models/epp/contact.rb @@ -88,68 +88,6 @@ class Epp::Contact < Contact } end - def update_attributes(frame, current_user) - return super if frame.blank? - new_attributes = self.class.attrs_from(frame, new_record: false) - - if Setting.client_status_editing_enabled - new_attributes[:statuses] = - statuses - new_attributes[:statuses_to_remove] + new_attributes[:statuses_to_add] - end - - if doc = attach_legal_document(::Deserializers::Xml::LegalDocument.new(frame).call) - frame.css("legalDocument").first.content = doc.path if doc&.persisted? - self.legal_document_id = doc.id - end - - ident_attributes = ::Deserializers::Xml::Ident.new(frame).call - - # https://github.com/internetee/registry/issues/576 - if ident_attributes[:ident] - if identifier.valid? - submitted_ident = Ident.new(code: ident_attributes[:ident], - type: ident_attributes[:ident_type], - country_code: ident_attributes[:ident_country_code]) - - if submitted_ident != identifier - add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.valid_ident')) - return - end - else - ident_update_attempt = ident_attributes[:ident] != ident - - if ident_update_attempt - add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.ident_update')) - return - end - - identifier = Ident.new(code: ident_attributes[:ident], - type: ident_attributes[:ident_type], - country_code: ident_attributes[:ident_country_code]) - - identifier.validate - - self.identifier = identifier - self.ident_updated_at ||= Time.zone.now - end - end - - self.upid = current_user.registrar.id if current_user.registrar - self.up_date = Time.zone.now - - self.attributes = new_attributes - - email_changed = will_save_change_to_email? - old_email = email_was - updated = save - - if updated && email_changed && registrant? - ContactMailer.email_changed(contact: self, old_email: old_email).deliver_now - end - - updated - end - def statuses_attrs(frame, action) status_list = status_list_from(frame) diff --git a/lib/deserializers/xml/contact_update.rb b/lib/deserializers/xml/contact_update.rb new file mode 100644 index 000000000..b3bc6fe4a --- /dev/null +++ b/lib/deserializers/xml/contact_update.rb @@ -0,0 +1,27 @@ +require 'deserializers/xml/legal_document' +require 'deserializers/xml/ident' +require 'deserializers/xml/contact' + +module Deserializers + module Xml + class ContactUpdate + attr_reader :frame + + def initialize(frame) + @frame = frame + end + + def contact + @contact ||= ::Deserializers::Xml::Contact.new(frame).call + end + + def ident + @ident ||= ::Deserializers::Xml::Ident.new(frame).call + end + + def legal_document + @legal_document ||= ::Deserializers::Xml::LegalDocument.new(frame).call + end + end + end +end From 8b7f4b2558468b6a546cd1e3045b79be4dd34e21 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Fri, 26 Jun 2020 14:38:18 +0300 Subject: [PATCH 4/7] Reverse conditionals --- lib/deserializers/xml/contact.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/deserializers/xml/contact.rb b/lib/deserializers/xml/contact.rb index 81eb90b51..5502ec237 100644 --- a/lib/deserializers/xml/contact.rb +++ b/lib/deserializers/xml/contact.rb @@ -34,14 +34,14 @@ module Deserializers end def if_present(css_path) - return unless frame.css(css_path).present? + return if frame.css(css_path).blank? frame.css(css_path).text end def statuses_to_add statuses_frame = frame.css('add') - return unless statuses_frame.present? + return if statuses_frame.blank? statuses_frame.css('status').map do |status| status['s'] @@ -50,7 +50,7 @@ module Deserializers def statuses_to_remove statuses_frame = frame.css('rem') - return unless statuses_frame.present? + return if statuses_frame.blank? statuses_frame.css('status').map do |status| status['s'] From fa016aa761fbf20d9fab0fbea67dc8fc5f82a98d Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Fri, 26 Jun 2020 14:45:44 +0300 Subject: [PATCH 5/7] convert if to return unless --- app/models/actions/contact_update.rb | 48 ++++++++++++++-------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/app/models/actions/contact_update.rb b/app/models/actions/contact_update.rb index 6b237ca65..5b94cf918 100644 --- a/app/models/actions/contact_update.rb +++ b/app/models/actions/contact_update.rb @@ -53,33 +53,33 @@ module Actions end def maybe_update_ident - if ident[:ident] - if contact.identifier.valid? - submitted_ident = ::Contact::Ident.new(code: ident[:ident], - type: ident[:ident_type], - country_code: ident[:ident_country_code]) + return unless ident[:ident] - if submitted_ident != contact.identifier - contact.add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.valid_ident')) - @error = true - end - else - ident_update_attempt = ident[:ident] != contact.ident + if contact.identifier.valid? + submitted_ident = ::Contact::Ident.new(code: ident[:ident], + type: ident[:ident_type], + country_code: ident[:ident_country_code]) - if ident_update_attempt - contact.add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.ident_update')) - @error = true - end - - identifier = ::Contact::Ident.new(code: ident[:ident], - type: ident[:ident_type], - country_code: ident[:ident_country_code]) - - identifier.validate - - contact.identifier = identifier - contact.ident_updated_at ||= Time.zone.now + if submitted_ident != contact.identifier + contact.add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.valid_ident')) + @error = true end + else + ident_update_attempt = ident[:ident] != contact.ident + + if ident_update_attempt + contact.add_epp_error('2308', nil, nil, I18n.t('epp.contacts.errors.ident_update')) + @error = true + end + + identifier = ::Contact::Ident.new(code: ident[:ident], + type: ident[:ident_type], + country_code: ident[:ident_country_code]) + + identifier.validate + + contact.identifier = identifier + contact.ident_updated_at ||= Time.zone.now end end From 41d9a364317e5276d858f1c1f4399808a7122e14 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Fri, 26 Jun 2020 14:51:08 +0300 Subject: [PATCH 6/7] Remove unused methods --- app/models/epp/contact.rb | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/app/models/epp/contact.rb b/app/models/epp/contact.rb index bb2803980..3fb87a1f0 100644 --- a/app/models/epp/contact.rb +++ b/app/models/epp/contact.rb @@ -88,40 +88,6 @@ class Epp::Contact < Contact } end - def statuses_attrs(frame, action) - status_list = status_list_from(frame) - - if action == 'rem' - to_destroy = [] - status_list.each do |status| - if statuses.include?(status) - to_destroy << status - else - add_epp_error('2303', 'status', status, [:contact_statuses, :not_found]) - end - end - - return to_destroy - else - return status_list - end - end - - def status_list_from(frame) - status_list = [] - - frame.css('status').each do |status| - unless Contact::CLIENT_STATUSES.include?(status['s']) - add_epp_error('2303', 'status', status['s'], [:domain_statuses, :not_found]) - next - end - - status_list << status['s'] - end - - status_list - end - def attach_legal_document(legal_document_data) return unless legal_document_data From bc3ea52667ca5085b54c25d00176de5faa62e008 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Fri, 26 Jun 2020 22:55:34 +0300 Subject: [PATCH 7/7] Fix typo --- lib/deserializers/xml/contact.rb | 2 +- test/lib/deserializers/xml/contact_test.rb | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/deserializers/xml/contact.rb b/lib/deserializers/xml/contact.rb index 5502ec237..4dd29c683 100644 --- a/lib/deserializers/xml/contact.rb +++ b/lib/deserializers/xml/contact.rb @@ -10,7 +10,7 @@ module Deserializers def call attributes = { name: if_present('postalInfo name'), - org: if_present('postalInfo org'), + org_name: if_present('postalInfo org'), email: if_present('email'), fax: if_present('fax'), phone: if_present('voice'), diff --git a/test/lib/deserializers/xml/contact_test.rb b/test/lib/deserializers/xml/contact_test.rb index c49d919b5..4621efeb2 100644 --- a/test/lib/deserializers/xml/contact_test.rb +++ b/test/lib/deserializers/xml/contact_test.rb @@ -22,6 +22,7 @@ class DeserializersXmlContactTest < ActiveSupport::TestCase new name + Org +123.4 new-email@inbox.test @@ -35,8 +36,9 @@ class DeserializersXmlContactTest < ActiveSupport::TestCase nokogiri_frame = Nokogiri::XML(xml_string).remove_namespaces! instance = ::Deserializers::Xml::Contact.new(nokogiri_frame) assert_equal instance.call, { name: 'new name', - email: 'new-email@inbox.test', - phone: '+123.4' } + org_name: 'Org', + email: 'new-email@inbox.test', + phone: '+123.4' } end def test_handles_create