refactoring RelayableRetraction and SignedRetraction signing

lib/diaspora_federation/entities/relayable_retraction.rb

@@ -58,41 +58,39 @@ module DiasporaFederation
       # @return [Nokogiri::XML::Element] root element containing properties as child elements
       def to_xml
         entity_xml.tap do |xml|
-          hash = to_h
+          hash = to_signed_h
-          RelayableRetraction.update_signatures!(hash)
           xml.at_xpath("target_author_signature").content = hash[:target_author_signature]
           xml.at_xpath("parent_author_signature").content = hash[:parent_author_signature]
         end
       end
 
-      # Adds signatures to a given hash with the keys of the author and the parent
+      # Adds signatures to the hash with the keys of the author and the parent
       # if the signatures are not in the hash yet and if the keys are available.
       #
-      # @param [Hash] hash hash given for a signing
+      # @return [Hash] entity data hash with updated signatures
-      def self.update_signatures!(hash)
+      def to_signed_h
-        target_author = DiasporaFederation.callbacks.trigger(
+        target_author = DiasporaFederation.callbacks.trigger(:fetch_entity_author_id_by_guid, target_type, target_guid)
-          :fetch_entity_author_id_by_guid,
+        privkey = DiasporaFederation.callbacks.trigger(:fetch_private_key_by_diaspora_id, diaspora_id)
-          hash[:target_type],
-          hash[:target_guid]
-        )
-        privkey = DiasporaFederation.callbacks.trigger(:fetch_private_key_by_diaspora_id, hash[:diaspora_id])
 
-        fill_required_signature(target_author, privkey, hash) unless privkey.nil?
+        to_h.tap do |hash|
+          fill_required_signature(target_author, privkey, hash) unless privkey.nil?
+        end
       end
 
+      private
+
       # @param [String] target_author the author of the entity to retract
       # @param [OpenSSL::PKey::RSA] privkey private key of sender
       # @param [Hash] hash hash given for a signing
-      def self.fill_required_signature(target_author, privkey, hash)
+      def fill_required_signature(target_author, privkey, hash)
-        if target_author == hash[:diaspora_id] && hash[:target_author_signature].nil?
+        if target_author == diaspora_id && target_author_signature.nil?
           hash[:target_author_signature] =
             Signing.sign_with_key(SignedRetraction.apply_signable_exceptions(hash), privkey)
-        elsif target_author != hash[:diaspora_id] && hash[:parent_author_signature].nil?
+        elsif target_author != diaspora_id && parent_author_signature.nil?
           hash[:parent_author_signature] =
             Signing.sign_with_key(SignedRetraction.apply_signable_exceptions(hash), privkey)
         end
       end
-      private_class_method :fill_required_signature
     end
   end
 end

lib/diaspora_federation/entities/signed_retraction.rb

@@ -35,22 +35,22 @@ module DiasporaFederation
       # @return [Nokogiri::XML::Element] root element containing properties as child elements
       def to_xml
         entity_xml.tap do |xml|
-          hash = to_h
+          xml.at_xpath("target_author_signature").content = to_signed_h[:target_author_signature]
-          SignedRetraction.update_signatures!(hash)
-
-          xml.at_xpath("target_author_signature").content = hash[:target_author_signature]
         end
       end
 
-      # Adds signature to a given hash with the key of the author
+      # Adds signature to the hash with the key of the author
       # if the signature is not in the hash yet and if the key is available.
       #
-      # @param [Hash] data hash given for a signing
+      # @return [Hash] entity data hash with updated signatures
-      def self.update_signatures!(data)
+      def to_signed_h
-        if data[:target_author_signature].nil?
+        to_h.tap do |hash|
-          privkey = DiasporaFederation.callbacks.trigger(:fetch_private_key_by_diaspora_id, data[:diaspora_id])
+          if target_author_signature.nil?
-          unless privkey.nil?
+            privkey = DiasporaFederation.callbacks.trigger(:fetch_private_key_by_diaspora_id, diaspora_id)
-            data[:target_author_signature] = Signing.sign_with_key(apply_signable_exceptions(data), privkey)
+            unless privkey.nil?
+              hash[:target_author_signature] =
+                Signing.sign_with_key(SignedRetraction.apply_signable_exceptions(hash), privkey)
+            end
           end
         end
       end

lib/diaspora_federation/test.rb

@@ -3,49 +3,12 @@ require "diaspora_federation/test/factories"
 module DiasporaFederation
   # This module encapsulates helper functions maybe wanted by a testsuite of a diaspora_federation gem user application
   module Test
-    # Sort hash according to an entity class's property sequence.
-    # This is used for rspec tests in order to generate correct input hash to
-    # compare results with.
-    #
-    # @param [Hash] data input hash for sorting
-    # @param [Entity.Class] klass entity type to sort according to
-    # @return [Hash] sorted hash
-    def self.sort_hash(data, klass)
-      Hash[klass.class_props.map {|prop|
-        [prop[:name], data[prop[:name]]] unless data[prop[:name]].nil?
-      }.compact]
-    end
-
     # Generates attributes for entity constructor with correct signatures in it
     #
     # @param [Symbol] factory_name the factory to generate attributes for (normally entity name)
     # @return [Hash] hash with correct signatures
-    def self.relayable_attributes_with_signatures(factory_name)
+    def self.attributes_with_signatures(factory_name)
       FactoryGirl.build(factory_name).to_signed_h
     end
-
-    # Generates attributes for signed retraction entity constructor with correct signatures in it
-    #
-    # @return [Hash] hash with correct signatures
-    def self.signed_retraction_attributes_with_signatures
-      sort_hash(FactoryGirl.attributes_for(:signed_retraction_entity), Entities::SignedRetraction).tap do |data|
-        Entities::SignedRetraction.update_signatures!(data)
-      end
-    end
-
-    # Generates attributes for relayable retraction entity constructor with correct signatures in it
-    #
-    # @return [Hash] hash with correct signatures
-    def self.relayable_retraction_attributes_with_signatures
-      sort_hash(
-        FactoryGirl.attributes_for(
-          :relayable_retraction_entity,
-          target_author_signature: "false sig"
-        ),
-        Entities::RelayableRetraction
-      ).tap do |data|
-        Entities::RelayableRetraction.update_signatures!(data)
-      end
-    end
   end
 end

spec/lib/diaspora_federation/entities/comment_spec.rb

@@ -1,6 +1,6 @@
 module DiasporaFederation
   describe Entities::Comment do
-    let(:data) { Test.relayable_attributes_with_signatures(:comment_entity) }
+    let(:data) { Test.attributes_with_signatures(:comment_entity) }
 
     let(:xml) {
       <<-XML

spec/lib/diaspora_federation/entities/conversation_spec.rb

@@ -1,7 +1,7 @@
 module DiasporaFederation
   describe Entities::Conversation do
-    let(:msg1_data) { Test.relayable_attributes_with_signatures(:message_entity) }
+    let(:msg1_data) { Test.attributes_with_signatures(:message_entity) }
-    let(:msg2_data) { Test.relayable_attributes_with_signatures(:message_entity) }
+    let(:msg2_data) { Test.attributes_with_signatures(:message_entity) }
     let(:msg1) { FactoryGirl.build(:message_entity, msg1_data) }
     let(:msg2) { FactoryGirl.build(:message_entity, msg2_data) }
     let(:data) {

spec/lib/diaspora_federation/entities/like_spec.rb

@@ -1,6 +1,6 @@
 module DiasporaFederation
   describe Entities::Like do
-    let(:data) { Test.relayable_attributes_with_signatures(:like_entity) }
+    let(:data) { Test.attributes_with_signatures(:like_entity) }
 
     let(:xml) {
       <<-XML

spec/lib/diaspora_federation/entities/message_spec.rb

@@ -1,6 +1,6 @@
 module DiasporaFederation
   describe Entities::Message do
-    let(:data) { Test.relayable_attributes_with_signatures(:message_entity) }
+    let(:data) { Test.attributes_with_signatures(:message_entity) }
 
     let(:xml) {
       <<-XML

spec/lib/diaspora_federation/entities/participation_spec.rb

@@ -1,6 +1,6 @@
 module DiasporaFederation
   describe Entities::Participation do
-    let(:data) { Test.relayable_attributes_with_signatures(:participation_entity) }
+    let(:data) { Test.attributes_with_signatures(:participation_entity) }
 
     let(:xml) {
       <<-XML

spec/lib/diaspora_federation/entities/poll_participation_spec.rb

@@ -1,6 +1,6 @@
 module DiasporaFederation
   describe Entities::PollParticipation do
-    let(:data) { Test.relayable_attributes_with_signatures(:poll_participation_entity) }
+    let(:data) { Test.attributes_with_signatures(:poll_participation_entity) }
 
     let(:xml) {
       <<-XML

spec/lib/diaspora_federation/entities/relayable_retraction_spec.rb

@@ -1,6 +1,6 @@
 module DiasporaFederation
   describe Entities::RelayableRetraction do
-    let(:data) { Test.relayable_retraction_attributes_with_signatures }
+    let(:data) { Test.attributes_with_signatures(:relayable_retraction_entity) }
 
     let(:xml) {
       <<-XML
@@ -9,7 +9,7 @@ module DiasporaFederation
   <target_guid>#{data[:target_guid]}</target_guid>
   <target_type>#{data[:target_type]}</target_type>
   <sender_handle>#{data[:diaspora_id]}</sender_handle>
-  <target_author_signature>#{data[:target_author_signature]}</target_author_signature>
+  <target_author_signature/>
 </relayable_retraction>
 XML
     }
@@ -18,7 +18,7 @@ XML
 
     it_behaves_like "an XML Entity"
 
-    describe ".update_singatures!" do
+    describe "#to_signed_h" do
       let(:author_pkey) { OpenSSL::PKey::RSA.generate(1024) }
       let(:hash) { FactoryGirl.attributes_for(:relayable_retraction_entity) }
 
@@ -31,12 +31,12 @@ XML
           :fetch_private_key_by_diaspora_id, hash[:diaspora_id]
         ).and_return(author_pkey)
 
-        Entities::RelayableRetraction.update_signatures!(hash)
+        signed_hash = Entities::RelayableRetraction.new(hash).to_signed_h
 
         signable_hash = hash.select do |key, _|
           %i(target_guid target_type).include?(key)
         end
-        expect(Signing.verify_signature(signable_hash, hash[:target_author_signature], author_pkey)).to be_truthy
+        expect(Signing.verify_signature(signable_hash, signed_hash[:target_author_signature], author_pkey)).to be_truthy
       end
 
       it "updates parent author signature when it was nil, key was supplied and sender is not author of the target" do
@@ -48,20 +48,18 @@ XML
           :fetch_private_key_by_diaspora_id, hash[:diaspora_id]
         ).and_return(author_pkey)
 
-        Entities::RelayableRetraction.update_signatures!(hash)
+        signed_hash = Entities::RelayableRetraction.new(hash).to_signed_h
 
         signable_hash = hash.select do |key, _|
           %i(target_guid target_type).include?(key)
         end
-        expect(Signing.verify_signature(signable_hash, hash[:parent_author_signature], author_pkey)).to be_truthy
+        expect(Signing.verify_signature(signable_hash, signed_hash[:parent_author_signature], author_pkey)).to be_truthy
       end
 
       it "doesn't change signatures if they are already set" do
-        signatures = {target_author_signature: "aa"}
+        hash.merge!(target_author_signature: "aa", parent_author_signature: "bb")
-        hash.merge!(signatures)
 
-        Entities::RelayableRetraction.update_signatures!(hash)
+        expect(Entities::RelayableRetraction.new(hash).to_signed_h).to eq(hash)
-        expect(hash[:target_author_signature]).to eq(signatures[:target_author_signature])
       end
 
       it "doesn't change signatures if keys weren't supplied" do
@@ -73,8 +71,8 @@ XML
           :fetch_entity_author_id_by_guid, "Comment", hash[:target_guid]
         ).and_return(hash[:diaspora_id])
 
-        Entities::RelayableRetraction.update_signatures!(hash)
+        signed_hash = Entities::RelayableRetraction.new(hash).to_signed_h
-        expect(hash[:target_author_signature]).to eq(nil)
+        expect(signed_hash[:target_author_signature]).to eq(nil)
       end
     end
   end

spec/lib/diaspora_federation/entities/signed_retraction_spec.rb

@@ -1,6 +1,6 @@
 module DiasporaFederation
   describe Entities::SignedRetraction do
-    let(:data) { Test.signed_retraction_attributes_with_signatures }
+    let(:data) { Test.attributes_with_signatures(:signed_retraction_entity) }
 
     let(:xml) {
       <<-XML
@@ -17,7 +17,7 @@ XML
 
     it_behaves_like "an XML Entity"
 
-    describe ".update_singatures!" do
+    describe "#to_signed_h" do
       let(:author_pkey) { OpenSSL::PKey::RSA.generate(1024) }
       let(:hash) { FactoryGirl.attributes_for(:signed_retraction_entity) }
 
@@ -30,17 +30,15 @@ XML
           %i(target_guid target_type).include?(key)
         end
 
-        Entities::SignedRetraction.update_signatures!(hash)
+        signed_hash = Entities::SignedRetraction.new(hash).to_signed_h
 
-        expect(Signing.verify_signature(signable_hash, hash[:target_author_signature], author_pkey)).to be_truthy
+        expect(Signing.verify_signature(signable_hash, signed_hash[:target_author_signature], author_pkey)).to be_truthy
       end
 
       it "doesn't change signature if it is already set" do
-        signatures = {target_author_signature: "aa"}
+        hash[:target_author_signature] = "aa"
-        hash.merge!(signatures)
 
-        Entities::SignedRetraction.update_signatures!(hash)
+        expect(Entities::SignedRetraction.new(hash).to_signed_h).to eq(hash)
-        expect(hash[:target_author_signature]).to eq(signatures[:target_author_signature])
       end
 
       it "doesn't change signature if a key wasn't supplied" do
@@ -48,8 +46,8 @@ XML
           :fetch_private_key_by_diaspora_id, hash[:diaspora_id]
         ).and_return(nil)
 
-        Entities::SignedRetraction.update_signatures!(hash)
+        signed_hash = Entities::SignedRetraction.new(hash).to_signed_h
-        expect(hash[:author_signature]).to eq(nil)
+        expect(signed_hash[:author_signature]).to eq(nil)
       end
     end
   end