Repository: jaywink/federation

Issue:
Ensure Diaspora entity converters understand all new attributes
Diaspora is unifying the attributes for the entities (see https://github.com/diaspora/diaspora_federation/issues/29). Make sure our converters handle all the new entities since some of the converters have been built from example old entity XML's which are still sent.

Old attributes should still be supported in the converters.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7ef04da..789ed19 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,12 +13,15 @@ Additionally, Diaspora entity mappers `message_to_objects` and `element_to_objec
 ### Other backwards incompatible changes
 * A failed payload signature verification now raises a `SignatureVerificationError` instead of a less specific `AssertionError`.
 
-### Other additions
-
+### Added
 * Three new attributes added to entities.
     * Add protocol name to all entities to attribute `_source_protocol`. This might be useful for applications to know which protocol payload the entity was created from once multiple protocols are implemented.
     * Add source payload object to the entity at `_source_object` when processing it.
     * Add sender public key to the entity at `_sender_key`, but only if it was used for validating signatures.
+* Add support for the new Diaspora payload properties coming in the next protocol version. Old XML payloads are and will be still supported.
+
+### Changed
+* Refactor processing of Diaspora payload XML into entities. Diaspora protocol is dropping the `<XML><post></post></XML>` wrapper for the payloads. Payloads with the wrapper will still be parsed as before.
 
 ## [0.10.1] - 2017-03-09
 
diff --git a/federation/entities/diaspora/mappers.py b/federation/entities/diaspora/mappers.py
index 1094513..66da0d5 100644
--- a/federation/entities/diaspora/mappers.py
+++ b/federation/entities/diaspora/mappers.py
@@ -21,19 +21,24 @@ MAPPINGS = {
     "retraction": DiasporaRetraction,
 }
 
-BOOLEAN_KEYS = [
+TAGS = [
+    # Order is important. Any top level tags should be before possibly child tags
+    "status_message", "comment", "like", "request", "profile", "retraction", "photo",
+]
+
+BOOLEAN_KEYS = (
     "public",
     "nsfw",
-]
+)
 
-DATETIME_KEYS = [
+DATETIME_KEYS = (
     "created_at",
-]
+)
 
-INTEGER_KEYS = [
+INTEGER_KEYS = (
     "height",
     "width",
-]
+)
 
 def xml_children_as_dict(node):
     """Turn the children of node <xml> into a dict, keyed by tag name.
@@ -43,8 +48,8 @@ def xml_children_as_dict(node):
     return dict((e.tag, e.text) for e in node)
 
 
-def element_to_objects(tree, sender_key_fetcher=None):
-    """Transform an Element tree to a list of entities recursively.
+def element_to_objects(element, sender_key_fetcher=None):
+    """Transform an Element to a list of entities recursively.
 
     Possible child entities are added to each entity `_children` list.
 
@@ -54,45 +59,45 @@ def element_to_objects(tree, sender_key_fetcher=None):
     :returns: list of entities
     """
     entities = []
-    for element in tree:
-        cls = MAPPINGS.get(element.tag, None)
-        if not cls:
-            continue
-
-        attrs = xml_children_as_dict(element)
-        transformed = transform_attributes(attrs)
-        if hasattr(cls, "fill_extra_attributes"):
-            transformed = cls.fill_extra_attributes(transformed)
-        entity = cls(**transformed)
-        # Add protocol name
-        entity._source_protocol = "diaspora"
-        # Save element object to entity for possible later use
-        entity._source_object = element
-        # If relayable, fetch sender key for validation
-        if issubclass(cls, DiasporaRelayableMixin):
-            if sender_key_fetcher:
-                entity._sender_key = sender_key_fetcher(entity.handle)
-            else:
-                profile = retrieve_and_parse_profile(entity.handle)
-                if profile:
-                    entity._sender_key = profile.public_key
-        try:
-            entity.validate()
-        except ValueError as ex:
-            logger.error("Failed to validate entity %s: %s", entity, ex, extra={
-                "attrs": attrs,
-                "transformed": transformed,
-            })
-            continue
-        # Do child elements
-        entity._children = element_to_objects(element)
-        # Add to entities list
-        entities.append(entity)
-        if cls == DiasporaRequest:
-            # We support sharing/following separately, so also generate base Relationship for the following part
-            transformed.update({"relationship": "following"})
-            relationship = Relationship(**transformed)
-            entities.append(relationship)
+    cls = MAPPINGS.get(element.tag, None)
+    if not cls:
+        return []
+
+    attrs = xml_children_as_dict(element)
+    transformed = transform_attributes(attrs, cls)
+    if hasattr(cls, "fill_extra_attributes"):
+        transformed = cls.fill_extra_attributes(transformed)
+    entity = cls(**transformed)
+    # Add protocol name
+    entity._source_protocol = "diaspora"
+    # Save element object to entity for possible later use
+    entity._source_object = element
+    # If relayable, fetch sender key for validation
+    if issubclass(cls, DiasporaRelayableMixin):
+        if sender_key_fetcher:
+            entity._sender_key = sender_key_fetcher(entity.handle)
+        else:
+            profile = retrieve_and_parse_profile(entity.handle)
+            if profile:
+                entity._sender_key = profile.public_key
+    try:
+        entity.validate()
+    except ValueError as ex:
+        logger.error("Failed to validate entity %s: %s", entity, ex, extra={
+            "attrs": attrs,
+            "transformed": transformed,
+        })
+        return []
+    # Do child elements
+    for child in element:
+        entity._children = element_to_objects(child)
+    # Add to entities list
+    entities.append(entity)
+    if cls == DiasporaRequest:
+        # We support sharing/following separately, so also generate base Relationship for the following part
+        transformed.update({"relationship": "following"})
+        relationship = Relationship(**transformed)
+        entities.append(relationship)
     return entities
 
 
@@ -106,22 +111,32 @@ def message_to_objects(message, sender_key_fetcher=None):
     :returns: list of entities
     """
     doc = etree.fromstring(message)
-    if doc[0].tag == "post":
-        # Skip the top <post> element if it exists
-        doc = doc[0]
-    entities = element_to_objects(doc, sender_key_fetcher)
-    return entities
-
-
-def transform_attributes(attrs):
-    """Transform some attribute keys."""
+    # Future Diaspora protocol version contains the element at top level
+    if doc.tag in TAGS:
+        return element_to_objects(doc, sender_key_fetcher)
+    # Legacy Diaspora protocol wraps the element in <XML><post></post></XML>, so find the right element
+    for tag in TAGS:
+        element = doc.find(".//%s" % tag)
+        if element is not None:
+            return element_to_objects(element, sender_key_fetcher)
+    return []
+
+
+def transform_attributes(attrs, cls):
+    """Transform some attribute keys.
+    
+    :param attrs: Properties from the XML
+    :type attrs: dict
+    :param cls: Class of the entity
+    :type cls: class
+    """
     transformed = {}
     for key, value in attrs.items():
         if key in ["raw_message", "text"]:
             transformed["raw_content"] = value
         elif key in ["diaspora_handle", "sender_handle", "author"]:
             transformed["handle"] = value
-        elif key == "recipient_handle":
+        elif key in ["recipient_handle", "recipient"]:
             transformed["target_handle"] = value
         elif key == "parent_guid":
             transformed["target_guid"] = value
@@ -145,7 +160,7 @@ def transform_attributes(attrs):
             transformed["raw_content"] = value
         elif key == "searchable":
             transformed["public"] = True if value == "true" else False
-        elif key == "target_type":
+        elif key in ["target_type", "type"] and cls == DiasporaRetraction:
             transformed["entity_type"] = DiasporaRetraction.entity_type_from_remote(value)
         elif key == "remote_photo_path":
             transformed["remote_path"] = value
@@ -156,6 +171,8 @@ def transform_attributes(attrs):
             transformed["linked_type"] = "Post"
         elif key == "author_signature":
             transformed["signature"] = value
+        elif key == "post_guid":
+            transformed["target_guid"] = value
         elif key in BOOLEAN_KEYS:
             transformed[key] = True if value == "true" else False
         elif key in DATETIME_KEYS:


Test patch:
diff --git a/federation/tests/entities/diaspora/test_mappers.py b/federation/tests/entities/diaspora/test_mappers.py
index 0dddc99..848cfc8 100644
--- a/federation/tests/entities/diaspora/test_mappers.py
+++ b/federation/tests/entities/diaspora/test_mappers.py
@@ -15,7 +15,7 @@ from federation.tests.fixtures.keys import get_dummy_private_key
 from federation.tests.fixtures.payloads import (
     DIASPORA_POST_SIMPLE, DIASPORA_POST_COMMENT, DIASPORA_POST_LIKE,
     DIASPORA_REQUEST, DIASPORA_PROFILE, DIASPORA_POST_INVALID, DIASPORA_RETRACTION,
-    DIASPORA_POST_WITH_PHOTOS, DIASPORA_POST_LEGACY_TIMESTAMP)
+    DIASPORA_POST_WITH_PHOTOS, DIASPORA_POST_LEGACY_TIMESTAMP, DIASPORA_POST_LEGACY)
 
 
 def mock_fill(attributes):
@@ -23,7 +23,7 @@ def mock_fill(attributes):
     return attributes
 
 
-class TestDiasporaEntityMappersReceive(object):
+class TestDiasporaEntityMappersReceive():
     def test_message_to_objects_simple_post(self):
         entities = message_to_objects(DIASPORA_POST_SIMPLE)
         assert len(entities) == 1
@@ -37,6 +37,20 @@ class TestDiasporaEntityMappersReceive(object):
         assert post.created_at == datetime(2011, 7, 20, 1, 36, 7)
         assert post.provider_display_name == "Socialhome"
 
+    def test_message_to_objects_post_legacy(self):
+        # This is the previous XML schema used before renewal of protocol
+        entities = message_to_objects(DIASPORA_POST_LEGACY)
+        assert len(entities) == 1
+        post = entities[0]
+        assert isinstance(post, DiasporaPost)
+        assert isinstance(post, Post)
+        assert post.raw_content == "((status message))"
+        assert post.guid == "((guidguidguidguidguidguidguid))"
+        assert post.handle == "alice@alice.diaspora.example.org"
+        assert post.public == False
+        assert post.created_at == datetime(2011, 7, 20, 1, 36, 7)
+        assert post.provider_display_name == "Socialhome"
+
     def test_message_to_objects_legact_timestamp(self):
         entities = message_to_objects(DIASPORA_POST_LEGACY_TIMESTAMP)
         post = entities[0]
@@ -60,19 +74,6 @@ class TestDiasporaEntityMappersReceive(object):
         assert photo.handle == "alice@alice.diaspora.example.org"
         assert photo.public == False
         assert photo.created_at == datetime(2011, 7, 20, 1, 36, 7)
-        photo = post._children[1]
-        assert isinstance(photo, Image)
-        assert photo.remote_path == "https://alice.diaspora.example.org/uploads/images/"
-        assert photo.remote_name == "12345.jpg"
-        assert photo.raw_content == "foobar"
-        assert photo.linked_type == "Post"
-        assert photo.linked_guid == "((guidguidguidguidguidguidguid))"
-        assert photo.height == 120
-        assert photo.width == 120
-        assert photo.guid == "((guidguidguidguidguidguidguig))"
-        assert photo.handle == "alice@alice.diaspora.example.org"
-        assert photo.public == False
-        assert photo.created_at == datetime(2011, 7, 20, 1, 36, 7)
 
     @patch("federation.entities.diaspora.mappers.DiasporaComment._validate_signatures")
     def test_message_to_objects_comment(self, mock_validate):
@@ -171,7 +172,7 @@ class TestDiasporaEntityMappersReceive(object):
         mock_retrieve.assert_called_once_with("alice@alice.diaspora.example.org")
 
 
-class TestGetOutboundEntity(object):
+class TestGetOutboundEntity():
     def test_already_fine_entities_are_returned_as_is(self):
         dummy_key = get_dummy_private_key()
         entity = DiasporaPost()
diff --git a/federation/tests/fixtures/payloads.py b/federation/tests/fixtures/payloads.py
index a376140..97f88e6 100644
--- a/federation/tests/fixtures/payloads.py
+++ b/federation/tests/fixtures/payloads.py
@@ -1,5 +1,3 @@
-# -*- coding: utf-8 -*-
-
 ENCRYPTED_DIASPORA_PAYLOAD = """<?xml version='1.0'?>
             <diaspora xmlns="https://joindiaspora.com/protocol" xmlns:me="http://salmon-protocol.org/ns/magic-env">
                 <encrypted_header>{encrypted_header}</encrypted_header>
@@ -28,7 +26,7 @@ UNENCRYPTED_DIASPORA_PAYLOAD = """<?xml version='1.0'?>
         """
 
 
-DIASPORA_POST_SIMPLE = """<XML>
+DIASPORA_POST_LEGACY = """<XML>
       <post>
         <status_message>
           <raw_message>((status message))</raw_message>
@@ -43,139 +41,115 @@ DIASPORA_POST_SIMPLE = """<XML>
 """
 
 
-DIASPORA_POST_LEGACY_TIMESTAMP = """<XML>
-      <post>
-        <status_message>
-          <raw_message>((status message))</raw_message>
-          <guid>((guidguidguidguidguidguidguid))</guid>
-          <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
-          <public>false</public>
-          <created_at>2011-07-20 01:36:07 UTC</created_at>
-          <provider_display_name>Socialhome</provider_display_name>
-        </status_message>
-      </post>
-    </XML>
+DIASPORA_POST_SIMPLE = """
+    <status_message>
+      <text>((status message))</text>
+      <guid>((guidguidguidguidguidguidguid))</guid>
+      <author>alice@alice.diaspora.example.org</author>
+      <public>false</public>
+      <created_at>2011-07-20T01:36:07Z</created_at>
+      <provider_display_name>Socialhome</provider_display_name>
+    </status_message>
 """
 
 
-DIASPORA_POST_WITH_PHOTOS = """<XML>
-      <post>
-        <status_message>
-          <raw_message>((status message))</raw_message>
-          <guid>((guidguidguidguidguidguidguid))</guid>
-          <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
-          <public>false</public>
-          <created_at>2011-07-20T01:36:07Z</created_at>
-          <provider_display_name>Socialhome</provider_display_name>
-          <photo>
-            <guid>((guidguidguidguidguidguidguif))</guid>
-            <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
-            <public>false</public>
-            <created_at>2011-07-20T01:36:07Z</created_at>
-            <remote_photo_path>https://alice.diaspora.example.org/uploads/images/</remote_photo_path>
-            <remote_photo_name>1234.jpg</remote_photo_name>
-            <text/>
-            <status_message_guid>((guidguidguidguidguidguidguid))</status_message_guid>
-            <height>120</height>
-            <width>120</width>
-          </photo>
-          <photo>
-            <guid>((guidguidguidguidguidguidguig))</guid>
-            <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
-            <public>false</public>
-            <created_at>2011-07-20T01:36:07Z</created_at>
-            <remote_photo_path>https://alice.diaspora.example.org/uploads/images/</remote_photo_path>
-            <remote_photo_name>12345.jpg</remote_photo_name>
-            <text>foobar</text>
-            <status_message_guid>((guidguidguidguidguidguidguid))</status_message_guid>
-            <height>120</height>
-            <width>120</width>
-          </photo>
-        </status_message>
-      </post>
-    </XML>
+DIASPORA_POST_LEGACY_TIMESTAMP = """
+    <status_message>
+      <text>((status message))</text>
+      <guid>((guidguidguidguidguidguidguid))</guid>
+      <author>alice@alice.diaspora.example.org</author>
+      <public>false</public>
+      <created_at>2011-07-20 01:36:07 UTC</created_at>
+      <provider_display_name>Socialhome</provider_display_name>
+    </status_message>
 """
 
 
-DIASPORA_POST_INVALID = """<XML>
-      <post>
-        <status_message>
-          <raw_message>((status message))</raw_message>
-          <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
-          <public>false</public>
-          <created_at>2011-07-20T01:36:07Z</created_at>
-          <provider_display_name>Socialhome</provider_display_name>
-        </status_message>
-      </post>
-    </XML>
+DIASPORA_POST_WITH_PHOTOS = """
+    <status_message>
+      <text>((status message))</text>
+      <guid>((guidguidguidguidguidguidguid))</guid>
+      <author>alice@alice.diaspora.example.org</author>
+      <public>false</public>
+      <created_at>2011-07-20T01:36:07Z</created_at>
+      <provider_display_name>Socialhome</provider_display_name>
+      <photo>
+        <guid>((guidguidguidguidguidguidguif))</guid>
+        <author>alice@alice.diaspora.example.org</author>
+        <public>false</public>
+        <created_at>2011-07-20T01:36:07Z</created_at>
+        <remote_photo_path>https://alice.diaspora.example.org/uploads/images/</remote_photo_path>
+        <remote_photo_name>1234.jpg</remote_photo_name>
+        <text/>
+        <status_message_guid>((guidguidguidguidguidguidguid))</status_message_guid>
+        <height>120</height>
+        <width>120</width>
+      </photo>
+    </status_message>
 """
 
-DIASPORA_POST_COMMENT = """<XML>
-      <post>
-        <comment>
-          <guid>((guidguidguidguidguidguid))</guid>
-          <parent_guid>((parent_guidparent_guidparent_guidparent_guid))</parent_guid>
-          <author_signature>((base64-encoded data))</author_signature>
-          <text>((text))</text>
-          <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
-          <author_signature>((signature))</author_signature>
-        </comment>
-      </post>
-    </XML>
+
+DIASPORA_POST_INVALID = """
+    <status_message>
+      <text>((status message))</text>
+      <author>alice@alice.diaspora.example.org</author>
+      <public>false</public>
+      <created_at>2011-07-20T01:36:07Z</created_at>
+      <provider_display_name>Socialhome</provider_display_name>
+    </status_message>
 """
 
-DIASPORA_POST_LIKE = """<XML>
-      <post>
-        <like>
-          <target_type>Post</target_type>
-          <guid>((guidguidguidguidguidguid))</guid>
-          <parent_guid>((parent_guidparent_guidparent_guidparent_guid))</parent_guid>
-          <author_signature>((base64-encoded data))</author_signature>
-          <positive>true</positive>
-          <diaspora_handle>alice@alice.diaspora.example.org</diaspora_handle>
-          <author_signature>((signature))</author_signature>
-        </like>
-      </post>
-    </XML>
+DIASPORA_POST_COMMENT = """
+    <comment>
+      <guid>((guidguidguidguidguidguid))</guid>
+      <parent_guid>((parent_guidparent_guidparent_guidparent_guid))</parent_guid>
+      <author_signature>((base64-encoded data))</author_signature>
+      <text>((text))</text>
+      <author>alice@alice.diaspora.example.org</author>
+      <author_signature>((signature))</author_signature>
+    </comment>
 """
 
-DIASPORA_REQUEST = """<XML>
-      <post>
-        <request>
-          <sender_handle>bob@example.com</sender_handle>
-          <recipient_handle>alice@alice.diaspora.example.org</recipient_handle>
-        </request>
-      </post>
-    </XML>
+DIASPORA_POST_LIKE = """
+    <like>
+      <parent_type>Post</parent_type>
+      <guid>((guidguidguidguidguidguid))</guid>
+      <parent_guid>((parent_guidparent_guidparent_guidparent_guid))</parent_guid>
+      <author_signature>((base64-encoded data))</author_signature>
+      <positive>true</positive>
+      <author>alice@alice.diaspora.example.org</author>
+      <author_signature>((signature))</author_signature>
+    </like>
+"""
+
+DIASPORA_REQUEST = """
+    <request>
+      <author>bob@example.com</author>
+      <recipient>alice@alice.diaspora.example.org</recipient>
+    </request>
 """
 
-DIASPORA_PROFILE = """<XML>
-    <post>
-        <profile>
-            <diaspora_handle>bob@example.com</diaspora_handle>
-            <first_name>Bob Bobertson</first_name>
-            <last_name></last_name>
-            <image_url>https://example.com/uploads/images/thumb_large_c833747578b5.jpg</image_url>
-            <image_url_small>https://example.com/uploads/images/thumb_small_c8b147578b5.jpg</image_url_small>
-            <image_url_medium>https://example.com/uploads/images/thumb_medium_c8b1aab04f3.jpg</image_url_medium>
-            <gender></gender>
-            <bio>A cool bio</bio>
-            <location>Helsinki</location>
-            <searchable>true</searchable>
-            <nsfw>false</nsfw>
-            <tag_string>#socialfederation #federation</tag_string>
-        </profile>
-    </post>
-</XML>
+DIASPORA_PROFILE = """
+    <profile>
+        <author>bob@example.com</author>
+        <first_name>Bob Bobertson</first_name>
+        <last_name></last_name>
+        <image_url>https://example.com/uploads/images/thumb_large_c833747578b5.jpg</image_url>
+        <image_url_small>https://example.com/uploads/images/thumb_small_c8b147578b5.jpg</image_url_small>
+        <image_url_medium>https://example.com/uploads/images/thumb_medium_c8b1aab04f3.jpg</image_url_medium>
+        <gender></gender>
+        <bio>A cool bio</bio>
+        <location>Helsinki</location>
+        <searchable>true</searchable>
+        <nsfw>false</nsfw>
+        <tag_string>#socialfederation #federation</tag_string>
+    </profile>
 """
 
-DIASPORA_RETRACTION = """<XML>
-    <post>
-        <retraction>
-            <author>bob@example.com</author>
-            <target_guid>xxxxxxxxxxxxxxxx</target_guid>
-            <target_type>Post</target_type>
-        </retraction>
-    </post>
-</XML>
+DIASPORA_RETRACTION = """
+    <retraction>
+        <author>bob@example.com</author>
+        <target_guid>xxxxxxxxxxxxxxxx</target_guid>
+        <target_type>Post</target_type>
+    </retraction>
 """