Enable correct Diaspora relayable behaviour

Store the original object when signing with parent, then use that for sending, not serializing our entity object.

This fixes relayable support broken with the new Diaspora protocol.

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## [unreleased]
 
+### Backwards incompatible changes
+* When processing Diaspora payloads, entity used to get a `_source_object` stored to it. This was an `etree.Element` created from the source object. Due to serialization issues in applications (for example pushing the object to a task queue or saving to database), `_source_object` is now a byte string representation for the element done with `etree.tostring()`. 
+
 ### Added
 * New style Diaspora private encrypted JSON payloads are now supported in the receiving side. Outbound private Diaspora payloads are still sent as legacy encrypted payloads. ([issue](https://github.com/jaywink/federation/issues/83))
     * No additional changes need to be made when calling `handle_receive` from your task processing. Just pass in the full received XML or JSON payload as a string with recipient user object as before.
@@ -12,6 +15,8 @@
 * Correctly extend entity `_children`. Certain Diaspora payloads caused `_children` for an entity to be written over by an empty list, causing for example status message photos to not be saved. Correctly do an extend on it. ([issue](https://github.com/jaywink/federation/issues/89))
 * Fix parsing Diaspora profile `tag_string` into `Profile.tag_list` if the `tag_string` is an empty string. This caused the whole `Profile` object creation to fail. ([issue](https://github.com/jaywink/federation/issues/88))
 * Fix processing Diaspora payload if it is passed to `handle_receive` as a `bytes` object. ([issue](https://github.com/jaywink/federation/issues/91))
+* Fix broken Diaspora relayables after latest 0.2.0 protocol changes. Previously relayables worked only because they were reverse engineered from the legacy protocol. Now that XML order is not important and tag names can be different depending on which protocol version, the relayable forwarding broke. To fix, we don't regenerate the entity when forwarding it but store the original received object when generating a `parent_author_signature` (which is optional in some cases, but we generate it anyway for now). This happens in the previously existing `entity.sign_with_parent()` method. In the sending part, if the original received object (now with a parent author signature) exists in the entity, we send that to the remote instead of serializing the entity to XML.
+     * To forward a relayable you must call `entity.sign_with_parent()` before calling `handle_send` to send the entity.
 
 ### Removed
 * `Post.photos` entity attribute was never used by any code and has been removed. Child entities of type `Image` are stored in the `Post._children` as before.

federation/entities/base.py

@@ -9,11 +9,12 @@ __all__ = (
 )
 
 
-class BaseEntity(object):
+class BaseEntity:
     _allowed_children = ()
     # If we have a receiver for a private payload, store receiving user guid here
     _receiving_guid = ""
     _source_protocol = ""
+    # Contains the original object from payload as a string
     _source_object = None
     _sender_key = ""
     signature = ""
@@ -92,7 +93,11 @@ class BaseEntity(object):
         pass
 
     def sign(self, private_key):
-        """Implement in subclasses."""
+        """Implement in subclasses if needed."""
+        pass
+
+    def sign_with_parent(self, private_key):
+        """Implement in subclasses if needed."""
         pass
 
 

federation/entities/diaspora/entities.py

@@ -1,13 +1,16 @@
 from lxml import etree
 
 from federation.entities.base import Comment, Post, Reaction, Relationship, Profile, Retraction, BaseEntity, Follow
-from federation.entities.diaspora.utils import format_dt, struct_to_xml, get_base_attributes
+from federation.entities.diaspora.utils import format_dt, struct_to_xml, get_base_attributes, add_element_to_doc
 from federation.exceptions import SignatureVerificationError
 from federation.protocols.diaspora.signatures import verify_relayable_signature, create_relayable_signature
 from federation.utils.diaspora import retrieve_and_parse_profile
 
 
 class DiasporaEntityMixin(BaseEntity):
+    # Normally outbound document is generated from entity. Store one here if at some point we already have a doc
+    outbound_doc = None
+
     def to_xml(self):
         """Override in subclasses."""
         raise NotImplementedError
@@ -43,14 +46,18 @@ class DiasporaRelayableMixin(DiasporaEntityMixin):
         super()._validate_signatures()
         if not self._sender_key:
             raise SignatureVerificationError("Cannot verify entity signature - no sender key available")
-        if not verify_relayable_signature(self._sender_key, self._source_object, self.signature):
+        source_doc = etree.fromstring(self._source_object)
+        if not verify_relayable_signature(self._sender_key, source_doc, self.signature):
             raise SignatureVerificationError("Signature verification failed.")
 
     def sign(self, private_key):
         self.signature = create_relayable_signature(private_key, self.to_xml())
 
     def sign_with_parent(self, private_key):
-        self.parent_signature = create_relayable_signature(private_key, self.to_xml())
+        doc = etree.fromstring(self._source_object)
+        signature = create_relayable_signature(private_key, doc)
+        add_element_to_doc(doc, "parent_author_signature", signature)
+        self.outbound_doc = doc
 
 
 class DiasporaComment(DiasporaRelayableMixin, Comment):

federation/entities/diaspora/mappers.py

@@ -76,7 +76,7 @@ def element_to_objects(element, sender_key_fetcher=None, user=None):
     # Add protocol name
     entity._source_protocol = "diaspora"
     # Save element object to entity for possible later use
-    entity._source_object = element
+    entity._source_object = etree.tostring(element)
     # Save receiving guid to object
     if user and hasattr(user, "guid"):
         entity._receiving_guid = user.guid
@@ -212,6 +212,9 @@ def get_outbound_entity(entity, private_key):
     :returns: Protocol specific entity class instance.
     :raises ValueError: If conversion cannot be done.
     """
+    if getattr(entity, "outbound_doc", None):
+        # If the entity already has an outbound doc, just return the entity as is
+        return entity
     outbound = None
     cls = entity.__class__
     if cls in [DiasporaPost, DiasporaRequest, DiasporaComment, DiasporaLike, DiasporaProfile, DiasporaRetraction,

federation/entities/diaspora/utils.py

@@ -62,3 +62,11 @@ def get_full_xml_representation(entity, private_key):
     diaspora_entity = get_outbound_entity(entity, private_key)
     xml = diaspora_entity.to_xml()
     return "<XML><post>%s</post></XML>" % etree.tostring(xml).decode("utf-8")
+
+
+def add_element_to_doc(doc, tag, value):
+    """Set text value of an etree.Element of tag, appending a new element with given tag if it doesn't exist."""
+    element = doc.find(".//%s" % tag)
+    if element is None:
+        element = etree.SubElement(doc, tag)
+    element.text = value

federation/protocols/diaspora/protocol.py

@@ -244,7 +244,11 @@ class Protocol(BaseProtocol):
 
     def build_send(self, entity, from_user, to_user=None, *args, **kwargs):
         """Build POST data for sending out to remotes."""
-        xml = entity.to_xml()
+        if entity.outbound_doc:
+            # Use pregenerated outbound document
+            xml = entity.outbound_doc
+        else:
+            xml = entity.to_xml()
         self.init_message(xml, from_user.handle, from_user.private_key)
         xml = self.create_salmon_envelope(to_user)
         return {'xml': xml}

federation/tests/entities/diaspora/test_entities.py

@@ -1,4 +1,3 @@
-import datetime
 from unittest.mock import patch
 
 import pytest
@@ -93,7 +92,7 @@ class TestEntitiesConvertToXML:
         assert etree.tostring(result) == converted
 
 
-class TestDiasporaProfileFillExtraAttributes():
+class TestDiasporaProfileFillExtraAttributes:
     def test_raises_if_no_handle(self):
         attrs = {"foo": "bar"}
         with pytest.raises(ValueError):
@@ -122,17 +121,18 @@ class TestDiasporaRetractionEntityConverters:
 
 
 class TestDiasporaRelayableMixin:
-    def test_signing_comment_works(self):
+    @patch("federation.entities.diaspora.entities.format_dt", side_effect=lambda v: v)
+    def test_signing_comment_works(self, mock_format_dt):
         entity = DiasporaComment(
             raw_content="raw_content", guid="guid", target_guid="target_guid", handle="handle",
-            created_at=datetime.datetime(2016, 3, 2),
+            created_at="created_at",
         )
         entity.sign(get_dummy_private_key())
-        assert entity.signature == "Z7Yh/zvH8oSct+UZhvHHLESd5HmjyC9LOhXqO/Kan4DYVwW3aoIwQWtWDESnjzjdNeBTVale5koGI1wI" \
+        assert entity.signature == "OWvW/Yxw4uCnx0WDn0n5/B4uhyZ8Pr6h3FZaw8J7PCXyPluOfYXFoHO21bykP8c2aVnuJNHe+lmeAkUC" \
-                                   "HGFd1WbaD7h5Fzi2uh4pl8u75ELhN0qTfWsd5hULj6eCkun0ytc2W+cwJAmRzhyxlmCkxwvmUoP4AS7M" \
+                                   "/kHnl4yxk/jqe3uroW842OWvsyDRQ11vHxhIqNMjiepFPkZmXX3vqrYYh5FrC/tUsZrEc8hHoOIHXFR2" \
-                                   "OVmV/79PkVfyJWp9XcPn0TB4IBifI/i6iA2PBPrczcAnopzmIg7xehqwd7aX/dGaRruAPR9mxDTMrKmd" \
+                                   "kGD0gPV+4EEG6pbMNNZ+SBVun0hvruX8iKQVnBdc/+zUI9+T/MZmLyqTq/CvuPxDyHzQPSHi68N9rJyr" \
-                                   "w8cuLarcMfHQTU5lu9Py2kCie+kGYbg7O92khaQdZrLkly1i2tyLZGpC6uFdGXYOYfLcZ7e2aOWHnwzp" \
+                                   "4Xa1K+R33Xq8eHHxs8LVNRqzaHGeD3DX8yBu/vP9TYmZsiWlymbuGwLCa4Yfv/VS1hQZovhg6YTxV4CR" \
-                                   "QxbyIb7jhjSWf9i97GTtAA=="
+                                   "v4ToGL+CAJ7UHEugRRBwDw=="
 
     def test_signing_like_works(self):
         entity = DiasporaLike(guid="guid", target_guid="target_guid", handle="handle")
@@ -154,13 +154,11 @@ class TestDiasporaRelayableEntityValidate():
     def test_calls_verify_signature(self, mock_verify):
         entity = DiasporaComment()
         entity._sender_key = "key"
-        entity._source_object = "obj"
+        entity._source_object = "<obj></obj>"
         entity.signature = "sig"
         mock_verify.return_value = False
         with pytest.raises(SignatureVerificationError):
             entity._validate_signatures()
-            mock_verify.assert_called_once_with("key", "obj", "sig")
         mock_verify.reset_mock()
         mock_verify.return_value = True
         entity._validate_signatures()
-        mock_verify.assert_called_once_with("key", "obj", "sig")

federation/tests/entities/diaspora/test_mappers.py

@@ -1,4 +1,5 @@
 from datetime import datetime
+from lxml import etree
 from unittest.mock import patch, Mock
 
 import pytest
@@ -196,6 +197,12 @@ class TestDiasporaEntityMappersReceive():
         entities = message_to_objects(DIASPORA_POST_SIMPLE)
         assert entities[0]._source_protocol == "diaspora"
 
+    @patch("federation.entities.diaspora.mappers.DiasporaComment._validate_signatures")
+    def test_source_object(self, mock_validate):
+        entities = message_to_objects(DIASPORA_POST_COMMENT, sender_key_fetcher=Mock())
+        entity = entities[0]
+        assert entity._source_object == etree.tostring(etree.fromstring(DIASPORA_POST_COMMENT))
+
     @patch("federation.entities.diaspora.mappers.DiasporaComment._validate_signatures")
     def test_element_to_objects_calls_sender_key_fetcher(self, mock_validate):
         mock_fetcher = Mock()

federation/tests/entities/diaspora/test_utils.py

@@ -2,12 +2,14 @@ import datetime
 import re
 
 import arrow
+from lxml import etree
 
 from federation.entities.base import Post
-from federation.entities.diaspora.utils import get_base_attributes, get_full_xml_representation, format_dt
+from federation.entities.diaspora.utils import (
+    get_base_attributes, get_full_xml_representation, format_dt, add_element_to_doc)
 
 
-class TestGetBaseAttributes():
+class TestGetBaseAttributes:
     def test_get_base_attributes_returns_only_intended_attributes(self):
         entity = Post()
         attrs = get_base_attributes(entity).keys()
@@ -17,7 +19,7 @@ class TestGetBaseAttributes():
         }
 
 
-class TestGetFullXMLRepresentation():
+class TestGetFullXMLRepresentation:
     def test_returns_xml_document(self):
         entity = Post()
         document = get_full_xml_representation(entity, "")
@@ -27,7 +29,26 @@ class TestGetFullXMLRepresentation():
                            "<provider_display_name></provider_display_name></status_message></post></XML>"
 
 
-class TestFormatDt():
+class TestFormatDt:
     def test_formatted_string_returned_from_tz_aware_datetime(self):
         dt = arrow.get(datetime.datetime(2017, 1, 28, 3, 2, 3), "Europe/Helsinki").datetime
         assert format_dt(dt) == "2017-01-28T01:02:03Z"
+
+
+def test_add_element_to_doc():
+    # Replacing value
+    doc = etree.fromstring("<comment><text>foobar</text><parent_author_signature>barfoo</parent_author_signature>"
+                           "</comment>")
+    add_element_to_doc(doc, "parent_author_signature", "newsig")
+    assert etree.tostring(doc) == b"<comment><text>foobar</text><parent_author_signature>newsig" \
+                                  b"</parent_author_signature></comment>"
+    # Adding value to an empty tag
+    doc = etree.fromstring("<comment><text>foobar</text><parent_author_signature /></comment>")
+    add_element_to_doc(doc, "parent_author_signature", "newsig")
+    assert etree.tostring(doc) == b"<comment><text>foobar</text><parent_author_signature>newsig" \
+                                  b"</parent_author_signature></comment>"
+    # Adding missing tag
+    doc = etree.fromstring("<comment><text>foobar</text></comment>")
+    add_element_to_doc(doc, "parent_author_signature", "newsig")
+    assert etree.tostring(doc) == b"<comment><text>foobar</text><parent_author_signature>newsig" \
+                                  b"</parent_author_signature></comment>"

federation/tests/protocols/diaspora/test_protocol.py

@@ -151,7 +151,7 @@ class TestDiasporaProtocol(DiasporaTestBase):
         mock_create_salmon.return_value = "xmldata"
         protocol = self.init_protocol()
         mock_entity_xml = Mock()
-        entity = Mock(to_xml=Mock(return_value=mock_entity_xml))
+        entity = Mock(to_xml=Mock(return_value=mock_entity_xml), outbound_doc=None)
         from_user = Mock(handle="foobar", private_key="barfoo")
         data = protocol.build_send(entity, from_user)
         mock_init_message.assert_called_once_with(mock_entity_xml, from_user.handle, from_user.private_key)