Fix #927 - Clean up federation code after Elrond's review

- Add json_error and use inplace of json_response where appropriate.
- Add garbage_collection to config spec file.
- Fix bugs in both garbage collection task and test
- Handle /api/whoami when no user logged in and a test for such a case.
- Validate ID is correct and user has comment privilege to comment.

mediagoblin.ini

@@ -23,10 +23,6 @@ allow_registration = true
 # Set to false to disable the ability for users to report offensive content
 allow_reporting = true
 
-# Frequency garbage collection will run (setting to 0 or false to disable)
-# Setting units are minutes.
-garbage_collection = 60
-
 ## Uncomment this to put some user-overriding templates here
 # local_templates = %(here)s/user_dev/templates/
 

mediagoblin/config_spec.ini

@@ -92,6 +92,10 @@ max_file_size = integer(default=None)
 # Privilege scheme
 user_privilege_scheme = string(default="uploader,commenter,reporter")
 
+# Frequency garbage collection will run (setting to 0 or false to disable)
+# Setting units are minutes.
+garbage_collection = integer(default=60)
+
 [jinja2]
 # Jinja2 supports more directives than the minimum required by mediagoblin. 
 # This setting allows users creating custom templates to specify a list of

mediagoblin/db/models.py

@@ -684,7 +684,17 @@ class MediaComment(Base, MediaCommentMixin):
         if "id" not in data["inReplyTo"]:
             return False
             
-        self.media_entry = data["inReplyTo"]["id"]
+        # Validate that the ID is correct
+        try:
+            media_id = int(data["inReplyTo"]["id"])
+        except ValueError:
+            return False
+        
+        media = MediaEntry.query.filter_by(id=media_id).first()
+        if media is None:
+            return False
+
+        self.media_entry = media.id
         self.content = data["content"]
         return True
 

mediagoblin/federation/views.py

@@ -24,9 +24,13 @@ from mediagoblin.media_types import sniff_media
 from mediagoblin.decorators import oauth_required
 from mediagoblin.federation.decorators import user_has_privilege
 from mediagoblin.db.models import User, MediaEntry, MediaComment
-from mediagoblin.tools.response import redirect, json_response
+from mediagoblin.tools.response import redirect, json_response, json_error
 from mediagoblin.meddleware.csrf import csrf_exempt
-from mediagoblin.submit.lib import new_upload_entry
+from mediagoblin.submit.lib import new_upload_entry, api_upload_request, \
+                                    api_add_to_feed
+
+# MediaTypes
+from mediagoblin.media_types.image import MEDIA_TYPE as IMAGE_MEDIA_TYPE
 
 @oauth_required
 def profile(request, raw=False):
@@ -34,10 +38,8 @@ def profile(request, raw=False):
     user = request.matchdict["username"]
     requested_user = User.query.filter_by(username=user)
 
-    # check if the user exists
     if requested_user is None:
-        error = "No such 'user' with id '{0}'".format(user)
+        return json_error("No such 'user' with id '{0}'".format(user), 404)
-        return json_response({"error": error}, status=404)
 
     user = requested_user[0]
 
@@ -69,15 +71,14 @@ def uploads(request):
     requested_user = User.query.filter_by(username=user)
 
     if requested_user is None:
-        error = "No such 'user' with id '{0}'".format(user)
+        return json_error("No such 'user' with id '{0}'".format(user), 404)
-        return json_response({"error": error}, status=404)
 
     request.user = requested_user[0]
     if request.method == "POST":
         # Wrap the data in the werkzeug file wrapper
         if "Content-Type" not in request.headers:
-            error = "Must supply 'Content-Type' header to upload media."
+            return json_error(
-            return json_response({"error": error}, status=400)
+                "Must supply 'Content-Type' header to upload media.")
         mimetype = request.headers["Content-Type"]
         filename = mimetypes.guess_all_extensions(mimetype)
         filename = 'unknown' + filename[0] if filename else filename
@@ -90,12 +91,10 @@ def uploads(request):
         # Find media manager
         media_type, media_manager = sniff_media(file_data, filename)
         entry = new_upload_entry(request.user)
-        if hasattr(media_manager, "api_upload_request"):
+        entry.media_type = IMAGE_MEDIA_TYPE
-            return media_manager.api_upload_request(request, file_data, entry)
+        return api_upload_request(request, file_data, entry)
-        else:
-            return json_response({"error": "Not yet implemented"}, status=501)
 
-    return json_response({"error": "Not yet implemented"}, status=501)
+    return json_error("Not yet implemented", 501)
 
 @oauth_required
 @csrf_exempt
@@ -106,8 +105,7 @@ def feed(request):
 
     # check if the user exists
     if requested_user is None:
-        error = "No such 'user' with id '{0}'".format(user)
+        return json_error("No such 'user' with id '{0}'".format(user), 404)
-        return json_response({"error": error}, status=404)
 
     request.user = requested_user[0]
     if request.data:
@@ -118,11 +116,16 @@ def feed(request):
     if request.method == "POST" and data["verb"] == "post":
         obj = data.get("object", None)
         if obj is None:
-            error = {"error": "Could not find 'object' element."}
+            return json_error("Could not find 'object' element.")
-            return json_response(error, status=400)
 
         if obj.get("objectType", None) == "comment":
             # post a comment
+            if not request.user.has_privilege(u'commenter'):
+                return json_error(
+                    "Privilege 'commenter' required to comment.",
+                    status=403
+                )
+
             comment = MediaComment(author=request.user.id)
             comment.unserialize(data["object"])
             comment.save()
@@ -134,15 +137,19 @@ def feed(request):
             media_id = int(data["object"]["id"])
             media = MediaEntry.query.filter_by(id=media_id)
             if media is None:
-                error = "No such 'image' with id '{0}'".format(id=media_id)
+                return json_response(
-                return json_response(error, status=404)
+                    "No such 'image' with id '{0}'".format(id=media_id),
+                    status=404
+                )
 
             media = media.first()
             if not media.unserialize(data["object"]):
-                error = "Invalid 'image' with id '{0}'".format(media_id)
+                return json_error(
-                return json_response({"error": error}, status=400)
+                    "Invalid 'image' with id '{0}'".format(media_id)
+                )
+
             media.save()
-            media.media_manager.api_add_to_feed(request, media)
+            api_add_to_feed(request, media)
 
             return json_response({
                 "verb": "post",
@@ -151,46 +158,46 @@ def feed(request):
 
         elif obj.get("objectType", None) is None:
             # They need to tell us what type of object they're giving us.
-            error = {"error": "No objectType specified."}
+            return json_error("No objectType specified.")
-            return json_response(error, status=400)
         else:
             # Oh no! We don't know about this type of object (yet)
-            error_message = "Unknown object type '{0}'.".format(
+            object_type = obj.get("objectType", None)
-                obj.get("objectType", None)
+            return json_error("Unknown object type '{0}'.".format(object_type))
-            )
-
-            error = {"error": error_message}
-            return json_response(error, status=400)
 
     elif request.method in ["PUT", "POST"] and data["verb"] == "update":
         # Check we've got a valid object
         obj = data.get("object", None)
 
         if obj is None:
-            error = {"error": "Could not find 'object' element."}
+            return json_error("Could not find 'object' element.")
-            return json_response(error, status=400)
 
         if "objectType" not in obj:
-            error = {"error": "No objectType specified."}
+            return json_error("No objectType specified.")
-            return json_response(error, status=400)
 
         if "id" not in obj:
-            error = {"error": "Object ID has not been specified."}
+            return json_error("Object ID has not been specified.")
-            return json_response(error, status=400)
 
         obj_id = obj["id"]
 
         # Now try and find object
         if obj["objectType"] == "comment":
+            if not request.user.has_privilege(u'commenter'):
+                return json_error(
+                    "Privilege 'commenter' required to comment.",
+                    status=403
+                )
+
             comment = MediaComment.query.filter_by(id=obj_id)
             if comment is None:
-                error = "No such 'comment' with id '{0}'.".format(obj_id)
+                return json_error(
-                return json_response({"error": error}, status=400)
+                    "No such 'comment' with id '{0}'.".format(obj_id)
+                )
 
             comment = comment[0]
             if not comment.unserialize(data["object"]):
-                error = "Invalid 'comment' with id '{0}'".format(obj_id)
+                return json_error(
-                return json_response({"error": error}, status=400)
+                    "Invalid 'comment' with id '{0}'".format(obj_id)
+                )
 
             comment.save()
 
@@ -203,13 +210,15 @@ def feed(request):
         elif obj["objectType"] == "image":
             image = MediaEntry.query.filter_by(id=obj_id)
             if image is None:
-                error = "No such 'image' with the id '{0}'.".format(obj_id)
+                return json_error(
-                return json_response({"error": error}, status=400)
+                    "No such 'image' with the id '{0}'.".format(obj_id)
+                )
 
             image = image[0]
             if not image.unserialize(obj):
-                "Invalid 'image' with id '{0}'".format(obj_id)
+                return json_error(
-                return json_response({"error": error}, status=400)
+                    "Invalid 'image' with id '{0}'".format(obj_id)
+                )
             image.save()
 
             activity = {
@@ -219,9 +228,10 @@ def feed(request):
             return json_response(activity)
 
     elif request.method != "GET":
-        # Currently unsupported
+        return json_error(
-        error = "Unsupported HTTP method {0}".format(request.method)
+            "Unsupported HTTP method {0}".format(request.method),
-        return json_response({"error": error}, status=501)
+            status=501
+        )
 
     feed_url = request.urlgen(
         "mediagoblin.federation.feed",
@@ -255,7 +265,8 @@ def feed(request):
     }
 
 
-    # Now lookup the user's feed.
+    # Look up all the media to put in the feed (this will be changed
+    # when we get real feeds/inboxes/outboxes/activites)
     for media in MediaEntry.query.all():
         item = {
             "verb": "post",
@@ -283,21 +294,22 @@ def object(request, raw_obj=False):
             request.matchdict["id"],
             object_type
         )
-        return json_response({"error": error}, status=400)
+        return json_error(error)
 
     if object_type not in ["image"]:
-        error = "Unknown type: {0}".format(object_type)
         # not sure why this is 404, maybe ask evan. Maybe 400?
-        return json_response({"error": error}, status=404)
+        return json_error("Unknown type: {0}".format(object_type), status=404)
 
     media = MediaEntry.query.filter_by(id=object_id).first()
     if media is None:
-        # no media found with that uuid
         error = "Can't find '{0}' with ID '{1}'".format(
             object_type,
             object_id
         )
-        return json_response({"error": error}, status=404)
+        return json_error(
+            "Can't find '{0}' with ID '{1}'".format(object_type, object_id),
+            status=404
+        )
 
     if raw_obj:
         return media
@@ -371,6 +383,9 @@ def host_meta(request):
 
 def whoami(request):
     """ /api/whoami - HTTP redirect to API profile """
+    if request.user is None:
+        return json_error("Not logged in.", status=401)
+
     profile = request.urlgen(
         "mediagoblin.federation.user.profile",
         username=request.user.username,

mediagoblin/media_types/image/__init__.py

@@ -19,9 +19,6 @@ import logging
 from mediagoblin.media_types import MediaManagerBase
 from mediagoblin.media_types.image.processing import sniff_handler, \
         ImageProcessingManager
-from mediagoblin.tools.response import json_response
-from mediagoblin.submit.lib import prepare_queue_task, run_process_media
-from mediagoblin.notifications import add_comment_subscription
 
 _log = logging.getLogger(__name__)
 
@@ -58,38 +55,6 @@ class ImageMediaManager(MediaManagerBase):
         except (KeyError, ValueError):
             return None
 
-    @staticmethod
-    def api_upload_request(request, file_data, entry):
-        """ This handles a image upload request """
-        # Use the same kind of method from mediagoblin/submit/views:submit_start
-        entry.media_type = unicode(MEDIA_TYPE)
-        entry.title = file_data.filename
-        entry.generate_slug()
-
-        queue_file = prepare_queue_task(request.app, entry, file_data.filename)
-        with queue_file:
-            queue_file.write(request.data)
-
-        entry.save()
-        return json_response(entry.serialize(request))
-
-    @staticmethod
-    def api_add_to_feed(request, entry):
-        """ Add media to Feed """
-        if entry.title:
-            # Shame we have to do this here but we didn't have the data in
-            # api_upload_request as no filename is usually specified.
-            entry.slug = None
-            entry.generate_slug()
-
-        feed_url = request.urlgen(
-            'mediagoblin.user_pages.atom_feed',
-            qualified=True, user=request.user.username)
-
-        run_process_media(entry, feed_url)
-        add_comment_subscription(request.user, entry)
-        return json_response(entry.serialize(request))
-
 def get_media_type_and_manager(ext):
     if ext in ACCEPTED_EXTENSIONS:
         return MEDIA_TYPE, ImageMediaManager

mediagoblin/submit/lib.py

@@ -22,6 +22,7 @@ from werkzeug.utils import secure_filename
 from werkzeug.datastructures import FileStorage
 
 from mediagoblin import mg_globals
+from mediagoblin.tools.response import json_response
 from mediagoblin.tools.text import convert_to_tag_list_of_dicts
 from mediagoblin.db.models import MediaEntry, ProcessingMetaData
 from mediagoblin.processing import mark_entry_failed
@@ -259,3 +260,33 @@ def run_process_media(entry, feed_url=None,
         mark_entry_failed(entry.id, exc)
         # re-raise the exception
         raise
+
+
+def api_upload_request(request, file_data, entry):
+    """ This handles a image upload request """
+    # Use the same kind of method from mediagoblin/submit/views:submit_start
+    entry.title = file_data.filename
+    entry.generate_slug()
+
+    queue_file = prepare_queue_task(request.app, entry, file_data.filename)
+    with queue_file:
+        queue_file.write(request.data)
+
+    entry.save()
+    return json_response(entry.serialize(request))
+
+def api_add_to_feed(request, entry):
+    """ Add media to Feed """
+    if entry.title:
+        # Shame we have to do this here but we didn't have the data in
+        # api_upload_request as no filename is usually specified.
+        entry.slug = None
+        entry.generate_slug()
+
+    feed_url = request.urlgen(
+        'mediagoblin.user_pages.atom_feed',
+        qualified=True, user=request.user.username)
+
+    run_process_media(entry, feed_url)
+    add_comment_subscription(request.user, entry)
+    return json_response(entry.serialize(request))

mediagoblin/submit/task.py

@@ -16,34 +16,23 @@
 
 import celery
 import datetime
-import logging
 import pytz
 
 from mediagoblin.db.models import MediaEntry
 
-_log = logging.getLogger(__name__)
-logging.basicConfig()
-_log.setLevel(logging.DEBUG)
-
 @celery.task()
 def collect_garbage():
-	"""
+    """
-		Garbage collection to clean up media
+        Garbage collection to clean up media
-
-		This will look for all critera on models to clean
-		up. This is primerally written to clean up media that's
-		entered a erroneous state.
-	"""
-	_log.info("Garbage collection is running.")
-	now = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=1)
-
-	garbage = MediaEntry.query.filter(MediaEntry.created > now)
-	garbage = garbage.filter(MediaEntry.state == "unprocessed")
-
-	for entry in garbage.all():
-		_log.info("Garbage media found with ID '{0}'".format(entry.id))
-		entry.delete()
-
 
+        This will look for all critera on models to clean
+        up. This is primerally written to clean up media that's
+        entered a erroneous state.
+    """
+    cuttoff = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=1)
 
+    garbage = MediaEntry.query.filter(MediaEntry.created < cuttoff)
+    garbage = garbage.filter(MediaEntry.state == "unprocessed")
 
+    for entry in garbage.all():
+        entry.delete()

mediagoblin/tests/test_api.py

@@ -14,22 +14,16 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 import json
-import datetime
 
 import mock
-import pytz
 import pytest
 
 from webtest import AppError
-from werkzeug.datastructures import FileStorage
 
 from .resources import GOOD_JPG
 from mediagoblin import mg_globals
-from mediagoblin.media_types import sniff_media
 from mediagoblin.db.models import User, MediaEntry
-from mediagoblin.submit.lib import new_upload_entry
 from mediagoblin.tests.tools import fixture_add_user
-from mediagoblin.federation.task import collect_garbage
 from mediagoblin.moderation.tools import take_away_privileges
 
 class TestAPI(object):
@@ -40,7 +34,7 @@ class TestAPI(object):
         self.test_app = test_app
         self.db = mg_globals.database
 
-        self.user = fixture_add_user(privileges=[u'active', u'uploader'])
+        self.user = fixture_add_user(privileges=[u'active', u'uploader', u'commenter'])
 
     def _activity_to_feed(self, test_app, activity, headers=None):
         """ Posts an activity to the user's feed """
@@ -265,32 +259,9 @@ class TestAPI(object):
 
             assert "links" in profile
 
-    def test_garbage_collection_task(self, test_app):
+    def test_whoami_without_login(self, test_app):
-        """ Test old media entry are removed by GC task """
+        """ Test that whoami endpoint returns error when not logged in """
-        # Create a media entry that's unprocessed and over an hour old.
+        with pytest.raises(AppError) as excinfo:
-        entry_id = 72
+            response = test_app.get("/api/whoami")
-        now = datetime.datetime.now(pytz.UTC)
-        file_data = FileStorage(
-            stream=open(GOOD_JPG, "rb"),
-            filename="mah_test.jpg",
-            content_type="image/jpeg"
-        )
 
-        # Find media manager
+        assert "401 UNAUTHORIZED" in excinfo.value.message
-        media_type, media_manager = sniff_media(file_data, "mah_test.jpg")
-        entry = new_upload_entry(self.user)
-        entry.id = entry_id
-        entry.title = "Mah Image"
-        entry.slug = "slugy-slug-slug"
-        entry.media_type = 'image'
-        entry.uploaded = now - datetime.timedelta(days=2)
-        entry.save()
-
-        # Validate the model exists
-        assert MediaEntry.query.filter_by(id=entry_id).first() is not None
-
-        # Call the garbage collection task
-        collect_garbage()
-
-        # Now validate the image has been deleted
-        assert MediaEntry.query.filter_by(id=entry_id).first() is None

mediagoblin/tests/test_misc.py

@@ -14,7 +14,16 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import pytz
+import datetime
+
+from werkzeug.datastructures import FileStorage
+
+from .resources import GOOD_JPG
 from mediagoblin.db.base import Session
+from mediagoblin.media_types import sniff_media
+from mediagoblin.submit.lib import new_upload_entry
+from mediagoblin.submit.task import collect_garbage
 from mediagoblin.db.models import User, MediaEntry, MediaComment
 from mediagoblin.tests.tools import fixture_add_user, fixture_media_entry
 
@@ -91,3 +100,35 @@ def test_media_deletes_broken_attachment(test_app):
 
     MediaEntry.query.get(media.id).delete()
     User.query.get(user_a.id).delete()
+
+def test_garbage_collection_task(test_app):
+    """ Test old media entry are removed by GC task """
+    user = fixture_add_user()
+
+    # Create a media entry that's unprocessed and over an hour old.
+    entry_id = 72
+    now = datetime.datetime.now(pytz.UTC)
+    file_data = FileStorage(
+        stream=open(GOOD_JPG, "rb"),
+        filename="mah_test.jpg",
+        content_type="image/jpeg"
+    )
+
+    # Find media manager
+    media_type, media_manager = sniff_media(file_data, "mah_test.jpg")
+    entry = new_upload_entry(user)
+    entry.id = entry_id
+    entry.title = "Mah Image"
+    entry.slug = "slugy-slug-slug"
+    entry.media_type = 'image'
+    entry.created = now - datetime.timedelta(days=2)
+    entry.save()
+
+    # Validate the model exists
+    assert MediaEntry.query.filter_by(id=entry_id).first() is not None
+
+    # Call the garbage collection task
+    collect_garbage()
+
+    # Now validate the image has been deleted
+    assert MediaEntry.query.filter_by(id=entry_id).first() is None

mediagoblin/tools/response.py

@@ -157,6 +157,14 @@ def json_response(serializable, _disable_cors=False, *args, **kw):
 
     return response
 
+def json_error(error_str, status=400, *args, **kwargs):
+    """
+        This is like json_response but takes an error message in and formats
+        it in {"error": error_str}. This also sets the default HTTP status
+        code to 400.
+    """
+    return json_response({"error": error_str}, status=status, *args, **kwargs)
+    
 def form_response(data, *args, **kwargs):
     """
         Responds using application/x-www-form-urlencoded and returns a werkzeug