Merge pull request #92 from jaywink/add-created-at-to-diaspora-comment

Add created_at to Diaspora Comment entity XML creator + fix relayable support

CHANGELOG.md

@@ -2,15 +2,21 @@
 
 ## [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.
+* Add `created_at` to Diaspora `Comment` entity XML creator. This is required in renewed Diaspora protocol. ([related issue](https://github.com/jaywink/federation/issues/59))
 
 ### Fixed
 * Fix getting sender from a combination of legacy Diaspora encrypted payload and new entity names (for example `author`). This combination probably only existed in this library.
 * 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.

dev-requirements.txt

@@ -18,3 +18,4 @@ recommonmark
 
 # Some datetime magic
 arrow
+freezegun

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)
+        self.parent_signature = create_relayable_signature(private_key, doc)
+        add_element_to_doc(doc, "parent_author_signature", self.parent_signature)
+        self.outbound_doc = doc
 
 
 class DiasporaComment(DiasporaRelayableMixin, Comment):
@@ -64,6 +71,7 @@ class DiasporaComment(DiasporaRelayableMixin, Comment):
             {"parent_author_signature": self.parent_signature},
             {"text": self.raw_content},
             {"diaspora_handle": self.handle},
+            {"created_at": format_dt(self.created_at)},
         ])
         return element
 

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,4 @@
-from unittest.mock import patch
+from unittest.mock import patch, Mock
 
 import pytest
 from lxml import etree
@@ -7,11 +7,13 @@ from federation.entities.base import Profile
 from federation.entities.diaspora.entities import (
     DiasporaComment, DiasporaPost, DiasporaLike, DiasporaRequest, DiasporaProfile, DiasporaRetraction,
     DiasporaContact)
+from federation.entities.diaspora.mappers import message_to_objects
 from federation.exceptions import SignatureVerificationError
 from federation.tests.fixtures.keys import get_dummy_private_key
+from federation.tests.fixtures.payloads import DIASPORA_POST_COMMENT
 
 
-class TestEntitiesConvertToXML():
+class TestEntitiesConvertToXML:
     def test_post_to_xml(self):
         entity = DiasporaPost(
             raw_content="raw_content", guid="guid", handle="handle", public=True,
@@ -33,10 +35,12 @@ class TestEntitiesConvertToXML():
         )
         result = entity.to_xml()
         assert result.tag == "comment"
+        assert len(result.find("created_at").text) > 0
+        result.find("created_at").text = ""  # timestamp makes testing painful
         converted = b"<comment><guid>guid</guid><parent_guid>target_guid</parent_guid>" \
                     b"<author_signature>signature</author_signature><parent_author_signature>" \
                     b"</parent_author_signature><text>raw_content</text><diaspora_handle>handle</diaspora_handle>" \
-                    b"</comment>"
+                    b"<created_at></created_at></comment>"
         assert etree.tostring(result) == converted
 
     def test_like_to_xml(self):
@@ -90,7 +94,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):
@@ -104,7 +108,7 @@ class TestDiasporaProfileFillExtraAttributes():
         assert attrs == {"handle": "foo", "guid": "guidguidguidguid"}
 
 
-class TestDiasporaRetractionEntityConverters():
+class TestDiasporaRetractionEntityConverters:
     def test_entity_type_from_remote(self):
         assert DiasporaRetraction.entity_type_from_remote("Post") == "Post"
         assert DiasporaRetraction.entity_type_from_remote("Like") == "Reaction"
@@ -118,17 +122,19 @@ class TestDiasporaRetractionEntityConverters():
         assert DiasporaRetraction.entity_type_to_remote("Comment") == "Comment"
 
 
-class TestDiasporaRelayableEntitySigning():
-    def test_signing_comment_works(self):
+class TestDiasporaRelayableMixin:
+    @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="created_at",
         )
         entity.sign(get_dummy_private_key())
-        assert entity.signature == "f3wkKDEhlT8zThEfaBcuKs4s0MbbWm9XPyx2ivrAg3jBtXQ6lXm5mgi9buwm+QyzxAGnk5Zth6HrYYB+" \
-                                   "NoieyoR4j54ryyPMB0gHwUO05tzjAMpvLyDlOyxLYFIl302ib2In9LJ5wa15VaEm9DW2+1WlCK72FonO" \
-                                   "oGx0qXDUc+NRn4s/UXBPNgM/Xsz3466AM1y98rUowHnpa0bxDjKcf7HMy4zuJ7XcsJAlofUHXCMX9TOm" \
-                                   "SBIwF5MlCkFL28R2cRAzJgNOBLw+a8arfi613bqo1Xq26+2PuFF0ng/OVOQOVFsO60H5wi/49FREWYdG" \
-                                   "ZdmHltxf76yWG6R1Zqpvag=="
+        assert entity.signature == "OWvW/Yxw4uCnx0WDn0n5/B4uhyZ8Pr6h3FZaw8J7PCXyPluOfYXFoHO21bykP8c2aVnuJNHe+lmeAkUC" \
+                                   "/kHnl4yxk/jqe3uroW842OWvsyDRQ11vHxhIqNMjiepFPkZmXX3vqrYYh5FrC/tUsZrEc8hHoOIHXFR2" \
+                                   "kGD0gPV+4EEG6pbMNNZ+SBVun0hvruX8iKQVnBdc/+zUI9+T/MZmLyqTq/CvuPxDyHzQPSHi68N9rJyr" \
+                                   "4Xa1K+R33Xq8eHHxs8LVNRqzaHGeD3DX8yBu/vP9TYmZsiWlymbuGwLCa4Yfv/VS1hQZovhg6YTxV4CR" \
+                                   "v4ToGL+CAJ7UHEugRRBwDw=="
 
     def test_signing_like_works(self):
         entity = DiasporaLike(guid="guid", target_guid="target_guid", handle="handle")
@@ -139,6 +145,30 @@ class TestDiasporaRelayableEntitySigning():
                                    "sC28oRHqHpIzOfhkIHyt+hOjO/mpuZLd7qOPfIySnGW6hM1iKewoJVDuVMN5w5VB46ETRum8JpvTQO8i" \
                                    "DPB+ZqbqcEasfm2CQIxVLA=="
 
+    @patch("federation.entities.diaspora.mappers.DiasporaComment._validate_signatures")
+    def test_sign_with_parent(self, mock_validate):
+        entities = message_to_objects(DIASPORA_POST_COMMENT, sender_key_fetcher=Mock())
+        entity = entities[0]
+        entity.sign_with_parent(get_dummy_private_key())
+        assert entity.parent_signature == "UTIDiFZqjxfU6ssVlmjz2RwOD/WPmMTFv57qOm0BZvBhF8Ef49Ynse1c2XTtx3rs8DyRMn54" \
+                                          "Uw4E0T+3t0Q5SHEQTLtRnOdRXrgNGAnlJ2xRmBWqe6xvvgc4nJ8OnffXhVgI8DBx6YUFRDjJ" \
+                                          "fnVQhnqbWr4ZAcpywCyL9IDkap3cTyn6wHo2WFRtq5syTCtMS8RZLXgpVLCeMfHhrXlePIA/" \
+                                          "YwMNn0GGi+9qSWXYVFG75cPjcWeY4t5q8EHCQReSSxG4a3HGbc7MigLvHzuhdOWOV8563dYo" \
+                                          "/5xS3zlQUt8I3AwXOzHr+57r1egMBHYyXTXsS8gFisj7mH4TsLM+Yw=="
+        assert etree.tostring(entity.outbound_doc) == b'<comment>\n      <guid>((guidguidguidguidguidguid))</guid>\n' \
+                                                      b'      <parent_guid>((parent_guidparent_guidparent_guidparent' \
+                                                      b'_guid))</parent_guid>\n      <author_signature>((base64-enco' \
+                                                      b'ded data))</author_signature>\n      <text>((text))</text>\n' \
+                                                      b'      <author>alice@alice.diaspora.example.org</author>\n   ' \
+                                                      b'   <author_signature>((signature))</author_signature>\n    ' \
+                                                      b'<parent_author_signature>UTIDiFZqjxfU6ssVlmjz2RwOD/WPmMTFv57' \
+                                                      b'qOm0BZvBhF8Ef49Ynse1c2XTtx3rs8DyRMn54Uw4E0T+3t0Q5SHEQTLtRnOd' \
+                                                      b'RXrgNGAnlJ2xRmBWqe6xvvgc4nJ8OnffXhVgI8DBx6YUFRDjJfnVQhnqbWr4' \
+                                                      b'ZAcpywCyL9IDkap3cTyn6wHo2WFRtq5syTCtMS8RZLXgpVLCeMfHhrXlePIA' \
+                                                      b'/YwMNn0GGi+9qSWXYVFG75cPjcWeY4t5q8EHCQReSSxG4a3HGbc7MigLvHzu' \
+                                                      b'hdOWOV8563dYo/5xS3zlQUt8I3AwXOzHr+57r1egMBHYyXTXsS8gFisj7mH4' \
+                                                      b'TsLM+Yw==</parent_author_signature></comment>'
+
 
 class TestDiasporaRelayableEntityValidate():
     def test_raises_if_no_sender_key(self):
@@ -150,13 +180,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()
@@ -271,3 +278,8 @@ class TestGetOutboundEntity():
         dummy_key = get_dummy_private_key()
         outbound = get_outbound_entity(entity, dummy_key)
         assert outbound.signature != ""
+
+    def test_returns_entity_if_outbound_doc_on_entity(self):
+        entity = Comment()
+        entity.outbound_doc = "foobar"
+        assert get_outbound_entity(entity, "private_key") == entity

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,12 +151,21 @@ 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)
         assert data == {"xml": "xmldata"}
 
+    @patch.object(Protocol, "init_message")
+    @patch.object(Protocol, "create_salmon_envelope")
+    def test_build_send_uses_outbound_doc(self, mock_create_salmon, mock_init_message):
+        protocol = self.init_protocol()
+        entity = Mock(to_xml=Mock(return_value=Mock()), outbound_doc="outbound_doc")
+        from_user = Mock(handle="foobar", private_key="barfoo")
+        protocol.build_send(entity, from_user)
+        mock_init_message.assert_called_once_with("outbound_doc", from_user.handle, from_user.private_key)
+
     @patch("federation.protocols.diaspora.protocol.EncryptedPayload.decrypt")
     def test_get_json_payload_magic_envelope(self, mock_decrypt):
         protocol = Protocol()