From 5e5d445890c6c555dff48b1613c285da983d71c8 Mon Sep 17 00:00:00 2001 From: Jessica Tallon Date: Mon, 28 Jul 2014 23:36:39 +0100 Subject: [PATCH] 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 | 4 - mediagoblin/config_spec.ini | 4 + mediagoblin/db/models.py | 12 +- mediagoblin/federation/views.py | 123 ++++++++++++--------- mediagoblin/media_types/image/__init__.py | 35 ------ mediagoblin/submit/lib.py | 31 ++++++ mediagoblin/{federation => submit}/task.py | 33 ++---- mediagoblin/tests/test_api.py | 43 ++----- mediagoblin/tests/test_misc.py | 41 +++++++ mediagoblin/tools/response.py | 8 ++ 10 files changed, 182 insertions(+), 152 deletions(-) rename mediagoblin/{federation => submit}/task.py (58%) diff --git a/mediagoblin.ini b/mediagoblin.ini index 6ccfa4f7..5e2477a4 100644 --- a/mediagoblin.ini +++ b/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/ diff --git a/mediagoblin/config_spec.ini b/mediagoblin/config_spec.ini index ba2b4519..c35b709d 100644 --- a/mediagoblin/config_spec.ini +++ b/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 diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index c6424e71..b3f7e23d 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -683,8 +683,18 @@ class MediaComment(Base, MediaCommentMixin): # Validate inReplyTo has ID if "id" not in data["inReplyTo"]: return False + + # 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 = data["inReplyTo"]["id"] + self.media_entry = media.id self.content = data["content"] return True diff --git a/mediagoblin/federation/views.py b/mediagoblin/federation/views.py index 86670857..a6912166 100644 --- a/mediagoblin/federation/views.py +++ b/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_response({"error": error}, status=404) + return json_error("No such 'user' with id '{0}'".format(user), 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_response({"error": error}, status=404) + return json_error("No such 'user' with id '{0}'".format(user), 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_response({"error": error}, status=400) + return json_error( + "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"): - return media_manager.api_upload_request(request, file_data, entry) - else: - return json_response({"error": "Not yet implemented"}, status=501) + entry.media_type = IMAGE_MEDIA_TYPE + return api_upload_request(request, file_data, entry) - 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_response({"error": error}, status=404) + return json_error("No such 'user' with id '{0}'".format(user), 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_response(error, status=400) + return json_error("Could not find 'object' element.") 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(error, status=404) + return json_response( + "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_response({"error": error}, status=400) + return json_error( + "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_response(error, status=400) + return json_error("No objectType specified.") else: # Oh no! We don't know about this type of object (yet) - error_message = "Unknown object type '{0}'.".format( - obj.get("objectType", None) - ) - - error = {"error": error_message} - return json_response(error, status=400) + object_type = obj.get("objectType", None) + return json_error("Unknown object type '{0}'.".format(object_type)) 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_response(error, status=400) + return json_error("Could not find 'object' element.") if "objectType" not in obj: - error = {"error": "No objectType specified."} - return json_response(error, status=400) + return json_error("No objectType specified.") if "id" not in obj: - error = {"error": "Object ID has not been specified."} - return json_response(error, status=400) + return json_error("Object ID has not been specified.") 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_response({"error": error}, status=400) + return json_error( + "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_response({"error": error}, status=400) + return json_error( + "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_response({"error": error}, status=400) + return json_error( + "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_response({"error": error}, status=400) + return json_error( + "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 - error = "Unsupported HTTP method {0}".format(request.method) - return json_response({"error": error}, status=501) + return json_error( + "Unsupported HTTP method {0}".format(request.method), + 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, diff --git a/mediagoblin/media_types/image/__init__.py b/mediagoblin/media_types/image/__init__.py index 96081068..11f90ca5 100644 --- a/mediagoblin/media_types/image/__init__.py +++ b/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 diff --git a/mediagoblin/submit/lib.py b/mediagoblin/submit/lib.py index 93ae7a1f..327ebbd8 100644 --- a/mediagoblin/submit/lib.py +++ b/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)) \ No newline at end of file diff --git a/mediagoblin/federation/task.py b/mediagoblin/submit/task.py similarity index 58% rename from mediagoblin/federation/task.py rename to mediagoblin/submit/task.py index 1d42e851..4ebde502 100755 --- a/mediagoblin/federation/task.py +++ b/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 - - 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() - + """ + 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. + """ + 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() diff --git a/mediagoblin/tests/test_api.py b/mediagoblin/tests/test_api.py index 55228edc..88d86053 100644 --- a/mediagoblin/tests/test_api.py +++ b/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 . 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): - """ Test old media entry are removed by GC task """ - # 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(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 + def test_whoami_without_login(self, test_app): + """ Test that whoami endpoint returns error when not logged in """ + with pytest.raises(AppError) as excinfo: + response = test_app.get("/api/whoami") + + assert "401 UNAUTHORIZED" in excinfo.value.message diff --git a/mediagoblin/tests/test_misc.py b/mediagoblin/tests/test_misc.py index 43ad0b6d..b3e59c09 100644 --- a/mediagoblin/tests/test_misc.py +++ b/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 . +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 diff --git a/mediagoblin/tools/response.py b/mediagoblin/tools/response.py index cd99a230..57552963 100644 --- a/mediagoblin/tools/response.py +++ b/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 -- 2.25.1