From 10fa2cf846cbf4a15ffc565e5184e5cdebd891dd Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Fri, 21 Jul 2017 22:43:30 +0300 Subject: [PATCH] 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 | 5 ++++ federation/entities/base.py | 9 ++++-- federation/entities/diaspora/entities.py | 13 +++++++-- federation/entities/diaspora/mappers.py | 5 +++- federation/entities/diaspora/utils.py | 8 +++++ federation/protocols/diaspora/protocol.py | 6 +++- .../tests/entities/diaspora/test_entities.py | 22 +++++++------- .../tests/entities/diaspora/test_mappers.py | 7 +++++ .../tests/entities/diaspora/test_utils.py | 29 ++++++++++++++++--- .../tests/protocols/diaspora/test_protocol.py | 2 +- 10 files changed, 82 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 71d76fb..8d64de5 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/federation/entities/base.py b/federation/entities/base.py index 530af8c..213d592 100644 --- a/federation/entities/base.py +++ b/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 diff --git a/federation/entities/diaspora/entities.py b/federation/entities/diaspora/entities.py index d316f34..13f375a 100644 --- a/federation/entities/diaspora/entities.py +++ b/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): diff --git a/federation/entities/diaspora/mappers.py b/federation/entities/diaspora/mappers.py index d430446..6a1d4cb 100644 --- a/federation/entities/diaspora/mappers.py +++ b/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, diff --git a/federation/entities/diaspora/utils.py b/federation/entities/diaspora/utils.py index 929cdc9..75a6f34 100644 --- a/federation/entities/diaspora/utils.py +++ b/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 "%s" % 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 diff --git a/federation/protocols/diaspora/protocol.py b/federation/protocols/diaspora/protocol.py index 92c1948..4e094bc 100644 --- a/federation/protocols/diaspora/protocol.py +++ b/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} diff --git a/federation/tests/entities/diaspora/test_entities.py b/federation/tests/entities/diaspora/test_entities.py index e5115fb..dcbedff 100644 --- a/federation/tests/entities/diaspora/test_entities.py +++ b/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" \ - "HGFd1WbaD7h5Fzi2uh4pl8u75ELhN0qTfWsd5hULj6eCkun0ytc2W+cwJAmRzhyxlmCkxwvmUoP4AS7M" \ - "OVmV/79PkVfyJWp9XcPn0TB4IBifI/i6iA2PBPrczcAnopzmIg7xehqwd7aX/dGaRruAPR9mxDTMrKmd" \ - "w8cuLarcMfHQTU5lu9Py2kCie+kGYbg7O92khaQdZrLkly1i2tyLZGpC6uFdGXYOYfLcZ7e2aOWHnwzp" \ - "QxbyIb7jhjSWf9i97GTtAA==" + 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") @@ -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 = "" 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") diff --git a/federation/tests/entities/diaspora/test_mappers.py b/federation/tests/entities/diaspora/test_mappers.py index ccf1834..ca189c4 100644 --- a/federation/tests/entities/diaspora/test_mappers.py +++ b/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() diff --git a/federation/tests/entities/diaspora/test_utils.py b/federation/tests/entities/diaspora/test_utils.py index ae09133..bff0a8e 100644 --- a/federation/tests/entities/diaspora/test_utils.py +++ b/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(): "" -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("foobarbarfoo" + "") + add_element_to_doc(doc, "parent_author_signature", "newsig") + assert etree.tostring(doc) == b"foobarnewsig" \ + b"" + # Adding value to an empty tag + doc = etree.fromstring("foobar") + add_element_to_doc(doc, "parent_author_signature", "newsig") + assert etree.tostring(doc) == b"foobarnewsig" \ + b"" + # Adding missing tag + doc = etree.fromstring("foobar") + add_element_to_doc(doc, "parent_author_signature", "newsig") + assert etree.tostring(doc) == b"foobarnewsig" \ + b"" diff --git a/federation/tests/protocols/diaspora/test_protocol.py b/federation/tests/protocols/diaspora/test_protocol.py index 078c88f..faceafc 100644 --- a/federation/tests/protocols/diaspora/test_protocol.py +++ b/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)