From 0f2c97adf84e77aff18bb51563f80b8ef6e2036d Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Thu, 15 Sep 2016 22:48:52 +0300 Subject: [PATCH] Validate entities that are created through message_to_objects To safeguard invalid entities being passed on to library users, validate entities that are created in the message_to_objects calls. Any failures are logged as errors. This means for Diaspora Profile messages we must fetch the remote GUID from the hcard. Diaspora Profile XML message does not contain the guid but our data structure enforces GUID as a required attribute. This ensures library users will get a full profile back always. Implement in the Diaspora entities a hook fill_extra_attributes that is called in message_to_objects. --- CHANGELOG.md | 10 ++++++++ federation/entities/diaspora/entities.py | 24 ++++++++++++++++++ federation/entities/diaspora/mappers.py | 20 ++++++++++++--- .../tests/entities/diaspora/test_entities.py | 18 +++++++++++++ .../tests/entities/diaspora/test_mappers.py | 25 ++++++++++++++----- federation/tests/fixtures/payloads.py | 23 +++++++++++++---- 6 files changed, 106 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe29f8b..7864683 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ ## [unreleased] +### Backwards incompatible changes +* Made `guid` mandatory for `Profile` entity. Library users should always be able to get a full validated object as we consider `guid` a core attribute of a profile. +* Always validate entities created through `federation.entities.diaspora.mappers.message_to_objects`. This is the code that transforms federation messages for the Diaspora protocol to actual entity objects. Previously no validation was done and callers of `federation.inbound.handle_receive` received entities that were not always valid, for example they were missing a `guid`. Now validation is done in the conversion stage and errors are pushed to the `social-federation` logger in the event of invalid messages. + * Note Diaspora Profile XML messages do not provide a GUID. This is handled internally by fetching the guid from the remote hCard so that a valid `Profile` entity can be created. + +### Added +* Raise a warning if unknown parameters are passed to entities. +* Ensure entity required attributes are validated for `None` or empty string values. Required attributes must not only exist but also have a value. +* Add validation to entities with the attribute `public`. Only `bool` values are accepted. + ### Changed * Function `federation.utils.diaspora.parse_profile_from_hcard` now requires a second argument, `handle`. Since in the future Diaspora hCard is not guaranteed to have username and domain, we now pass handle to the parser directly. diff --git a/federation/entities/diaspora/entities.py b/federation/entities/diaspora/entities.py index a467ca6..4bedb47 100644 --- a/federation/entities/diaspora/entities.py +++ b/federation/entities/diaspora/entities.py @@ -3,6 +3,7 @@ from lxml import etree from federation.entities.base import Comment, Post, Reaction, Relationship, Profile from federation.entities.diaspora.utils import format_dt, struct_to_xml, get_base_attributes +from federation.utils.diaspora import retrieve_and_parse_profile class DiasporaEntityMixin(object): @@ -10,6 +11,20 @@ class DiasporaEntityMixin(object): def from_base(cls, entity): return cls(**get_base_attributes(entity)) + @staticmethod + def fill_extra_attributes(attributes): + """Implement in subclasses to fill extra attributes when an XML is transformed to an object. + + This is called just before initializing the entity. + + Args: + attributes (dict) - Already transformed attributes that will be passed to entity create. + + Returns: + Must return the attributes dictionary, possibly with changed or additional values. + """ + return attributes + class DiasporaComment(DiasporaEntityMixin, Comment): """Diaspora comment.""" @@ -97,3 +112,12 @@ class DiasporaProfile(DiasporaEntityMixin, Profile): {"tag_string": " ".join(["#%s" % tag for tag in self.tag_list])}, ]) return element + + @staticmethod + def fill_extra_attributes(attributes): + """Diaspora Profile XML message contains no GUID. We need the guid. Fetch it.""" + if not attributes.get("handle"): + raise ValueError("Can't fill GUID for profile creation since there is no handle! Attrs: %s" % attributes) + profile = retrieve_and_parse_profile(attributes.get("handle")) + attributes["guid"] = profile.guid + return attributes diff --git a/federation/entities/diaspora/mappers.py b/federation/entities/diaspora/mappers.py index 5412497..9c9890c 100644 --- a/federation/entities/diaspora/mappers.py +++ b/federation/entities/diaspora/mappers.py @@ -1,11 +1,15 @@ # -*- coding: utf-8 -*- +import logging from datetime import datetime from lxml import etree from federation.entities.base import Image, Relationship, Post, Reaction, Comment, Profile -from federation.entities.diaspora.entities import DiasporaPost, DiasporaComment, DiasporaLike, DiasporaRequest, \ - DiasporaProfile +from federation.entities.diaspora.entities import ( + DiasporaPost, DiasporaComment, DiasporaLike, DiasporaRequest, DiasporaProfile) + + +logger = logging.getLogger("social-federation") MAPPINGS = { "status_message": DiasporaPost, @@ -43,8 +47,18 @@ def message_to_objects(message): if cls: 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) - entities.append(entity) + try: + entity.validate() + entities.append(entity) + except ValueError as ex: + logger.error("Failed to validate entity %s: %s", entity, ex, extra={ + "attrs": attrs, + "transformed": transformed, + }) + continue if cls == DiasporaRequest: # We support sharing/following separately, so also generate base Relationship for the following part transformed.update({"relationship": "following"}) diff --git a/federation/tests/entities/diaspora/test_entities.py b/federation/tests/entities/diaspora/test_entities.py index e7e120f..1b0dae6 100644 --- a/federation/tests/entities/diaspora/test_entities.py +++ b/federation/tests/entities/diaspora/test_entities.py @@ -1,6 +1,10 @@ # -*- coding: utf-8 -*- +from unittest.mock import patch + +import pytest from lxml import etree +from federation.entities.base import Profile from federation.entities.diaspora.entities import DiasporaComment, DiasporaPost, DiasporaLike, DiasporaRequest, \ DiasporaProfile @@ -61,3 +65,17 @@ class TestEntitiesConvertToXML(object): b"foobartrue" \ b"false#socialfederation #federation" assert etree.tostring(result) == converted + + +class TestDiasporaProfileFillExtraAttributes(object): + def test_raises_if_no_handle(self): + attrs = {"foo": "bar"} + with pytest.raises(ValueError): + DiasporaProfile.fill_extra_attributes(attrs) + + @patch("federation.entities.diaspora.entities.retrieve_and_parse_profile") + def test_calls_retrieve_and_parse_profile(self, mock_retrieve): + mock_retrieve.return_value = Profile(guid="guidguidguidguid") + attrs = {"handle": "foo"} + attrs = DiasporaProfile.fill_extra_attributes(attrs) + assert attrs == {"handle": "foo", "guid": "guidguidguidguid"} diff --git a/federation/tests/entities/diaspora/test_mappers.py b/federation/tests/entities/diaspora/test_mappers.py index 1f6b965..17aadb2 100644 --- a/federation/tests/entities/diaspora/test_mappers.py +++ b/federation/tests/entities/diaspora/test_mappers.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- from datetime import datetime +from unittest.mock import patch import pytest @@ -8,7 +9,12 @@ from federation.entities.diaspora.entities import DiasporaPost, DiasporaComment, DiasporaProfile from federation.entities.diaspora.mappers import message_to_objects, get_outbound_entity from federation.tests.fixtures.payloads import DIASPORA_POST_SIMPLE, DIASPORA_POST_COMMENT, DIASPORA_POST_LIKE, \ - DIASPORA_REQUEST, DIASPORA_PROFILE + DIASPORA_REQUEST, DIASPORA_PROFILE, DIASPORA_POST_INVALID + + +def mock_fill(attributes): + attributes["guid"] = "guidguidguidguidguid" + return attributes class TestDiasporaEntityMappersReceive(object): @@ -19,7 +25,7 @@ class TestDiasporaEntityMappersReceive(object): assert isinstance(post, DiasporaPost) assert isinstance(post, Post) assert post.raw_content == "((status message))" - assert post.guid == "((guid))" + 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) @@ -31,8 +37,8 @@ class TestDiasporaEntityMappersReceive(object): comment = entities[0] assert isinstance(comment, DiasporaComment) assert isinstance(comment, Comment) - assert comment.target_guid == "((parent_guid))" - assert comment.guid == "((guid))" + assert comment.target_guid == "((parent_guidparent_guidparent_guidparent_guid))" + assert comment.guid == "((guidguidguidguidguidguid))" assert comment.handle == "alice@alice.diaspora.example.org" assert comment.participation == "comment" assert comment.raw_content == "((text))" @@ -43,8 +49,8 @@ class TestDiasporaEntityMappersReceive(object): like = entities[0] assert isinstance(like, DiasporaLike) assert isinstance(like, Reaction) - assert like.target_guid == "((parent_guid))" - assert like.guid == "((guid))" + assert like.target_guid == "((parent_guidparent_guidparent_guidparent_guid))" + assert like.guid == "((guidguidguidguidguidguid))" assert like.handle == "alice@alice.diaspora.example.org" assert like.participation == "reaction" assert like.reaction == "like" @@ -65,6 +71,7 @@ class TestDiasporaEntityMappersReceive(object): assert sharing.relationship == "sharing" assert following.relationship == "following" + @patch("federation.entities.diaspora.entities.DiasporaProfile.fill_extra_attributes", new=mock_fill) def test_message_to_objects_profile(self): entities = message_to_objects(DIASPORA_PROFILE) assert len(entities) == 1 @@ -83,6 +90,12 @@ class TestDiasporaEntityMappersReceive(object): assert profile.nsfw == False assert profile.tag_list == ["socialfederation", "federation"] + @patch("federation.entities.diaspora.mappers.logger.error") + def test_invalid_entity_logs_an_error(self, mock_logger): + entities = message_to_objects(DIASPORA_POST_INVALID) + assert len(entities) == 0 + assert mock_logger.called + class TestGetOutboundEntity(object): def test_already_fine_entities_are_returned_as_is(self): diff --git a/federation/tests/fixtures/payloads.py b/federation/tests/fixtures/payloads.py index d3dab6f..aaf27f9 100644 --- a/federation/tests/fixtures/payloads.py +++ b/federation/tests/fixtures/payloads.py @@ -32,7 +32,20 @@ DIASPORA_POST_SIMPLE = """ ((status message)) - ((guid)) + ((guidguidguidguidguidguidguid)) + alice@alice.diaspora.example.org + false + 2011-07-20 01:36:07 UTC + Socialhome + + + +""" + +DIASPORA_POST_INVALID = """ + + + ((status message)) alice@alice.diaspora.example.org false 2011-07-20 01:36:07 UTC @@ -45,8 +58,8 @@ DIASPORA_POST_SIMPLE = """ DIASPORA_POST_COMMENT = """ - ((guid)) - ((parent_guid)) + ((guidguidguidguidguidguid)) + ((parent_guidparent_guidparent_guidparent_guid)) ((base64-encoded data)) ((text)) alice@alice.diaspora.example.org @@ -59,8 +72,8 @@ DIASPORA_POST_LIKE = """ Post - ((guid)) - ((parent_guid)) + ((guidguidguidguidguidguid)) + ((parent_guidparent_guidparent_guidparent_guid)) ((base64-encoded data)) true alice@alice.diaspora.example.org