From 063670e9b42c94bb6c4cf5c2838ab67a62349822 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Sat, 17 Mar 2012 23:43:46 -0400 Subject: [PATCH 01/14] Refactor image resize code, with better filenames (#261) The code to make thumbnail- and medium-sized images in processing.py is pretty similar, so I rolled that out into a separate function that we call with different arguments as appropriate. The new function should work identically to the old code, except it saves images with filenames based on the original filename, like `foobar.medium.jpg` instead of just `medium.jpg`. This fixes bug #261. --- mediagoblin/media_types/image/processing.py | 89 +++++++++++---------- 1 file changed, 48 insertions(+), 41 deletions(-) diff --git a/mediagoblin/media_types/image/processing.py b/mediagoblin/media_types/image/processing.py index 6ba91a15..2a00606a 100644 --- a/mediagoblin/media_types/image/processing.py +++ b/mediagoblin/media_types/image/processing.py @@ -23,6 +23,46 @@ from mediagoblin.processing import BadMediaFail, \ from mediagoblin.tools.exif import exif_fix_image_orientation, \ extract_exif, clean_exif, get_gps_data, get_useful +MAX_FILENAME_LENGTH = 255 # the limit in VFAT -- seems like a good baseline + +def resize_image(entry, filename, basename, file_tail, exif_tags, workdir, + new_size, size_limits=None): + """Store a resized version of an image and return its pathname. + + Arguments: + entry -- the entry for the image to resize + filename -- the filename of the original image being resized + basename -- simple basename of the given filename + file_tail -- ending string and extension for the resized filename + exif_tags -- EXIF data for the original image + workdir -- directory path for storing converted image files + new_size -- 2-tuple size for the resized image + size_limits (optional) -- image is only resized if it exceeds this size + + """ + try: + resized = Image.open(filename) + except IOError: + raise BadMediaFail() + resized = exif_fix_image_orientation(resized, exif_tags) # Fix orientation + + if ((size_limits is None) or + (resized.size[0] > size_limits[0]) or + (resized.size[1] > size_limits[1])): + resized.thumbnail(new_size, Image.ANTIALIAS) + + resized_filename = (basename[:MAX_FILENAME_LENGTH - len(file_tail)] + + file_tail) + resized_filepath = create_pub_filepath(entry, resized_filename) + + # Copy the new file to the conversion subdir, then remotely. + tmp_resized_filename = os.path.join(workdir, resized_filename) + with file(tmp_resized_filename, 'w') as resized_file: + resized.save(resized_file) + mgg.public_store.copy_local_to_storage( + tmp_resized_filename, resized_filepath) + return resized_filepath + def process_image(entry): """ Code to process an image @@ -46,50 +86,17 @@ def process_image(entry): exif_tags = extract_exif(queued_filename) gps_data = get_gps_data(exif_tags) - try: - thumb = Image.open(queued_filename) - except IOError: - raise BadMediaFail() - - thumb = exif_fix_image_orientation(thumb, exif_tags) - - thumb.thumbnail(THUMB_SIZE, Image.ANTIALIAS) - - # Copy the thumb to the conversion subdir, then remotely. - thumb_filename = 'thumbnail' + extension - thumb_filepath = create_pub_filepath(entry, thumb_filename) - - tmp_thumb_filename = os.path.join( - conversions_subdir, thumb_filename) - - with file(tmp_thumb_filename, 'w') as thumb_file: - thumb.save(thumb_file) - - mgg.public_store.copy_local_to_storage( - tmp_thumb_filename, thumb_filepath) + # Always create a small thumbnail + thumb_filepath = resize_image(entry, queued_filename, basename, + '.thumbnail' + extension, exif_tags, + conversions_subdir, THUMB_SIZE) # If the size of the original file exceeds the specified size of a `medium` - # file, a `medium.jpg` files is created and later associated with the media + # file, a `.medium.jpg` files is created and later associated with the media # entry. - medium = Image.open(queued_filename) - - # Fix orientation - medium = exif_fix_image_orientation(medium, exif_tags) - - if medium.size[0] > MEDIUM_SIZE[0] or medium.size[1] > MEDIUM_SIZE[1]: - medium.thumbnail(MEDIUM_SIZE, Image.ANTIALIAS) - - medium_filename = 'medium' + extension - medium_filepath = create_pub_filepath(entry, medium_filename) - - tmp_medium_filename = os.path.join( - conversions_subdir, medium_filename) - - with file(tmp_medium_filename, 'w') as medium_file: - medium.save(medium_file) - - mgg.public_store.copy_local_to_storage( - tmp_medium_filename, medium_filepath) + medium_filepath = resize_image(entry, queued_filename, basename, + '.medium' + extension, exif_tags, + conversions_subdir, MEDIUM_SIZE, MEDIUM_SIZE) # we have to re-read because unlike PIL, not everything reads # things in string representation :) From 176f207b638a5c7ca403173627f1bc47bc31266b Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Sun, 18 Mar 2012 09:35:14 -0400 Subject: [PATCH 02/14] small readability improvements in resize_image --- mediagoblin/media_types/image/processing.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mediagoblin/media_types/image/processing.py b/mediagoblin/media_types/image/processing.py index 2a00606a..b67fedb3 100644 --- a/mediagoblin/media_types/image/processing.py +++ b/mediagoblin/media_types/image/processing.py @@ -26,7 +26,7 @@ from mediagoblin.tools.exif import exif_fix_image_orientation, \ MAX_FILENAME_LENGTH = 255 # the limit in VFAT -- seems like a good baseline def resize_image(entry, filename, basename, file_tail, exif_tags, workdir, - new_size, size_limits=None): + new_size, size_limits=(0, 0)): """Store a resized version of an image and return its pathname. Arguments: @@ -46,11 +46,11 @@ def resize_image(entry, filename, basename, file_tail, exif_tags, workdir, raise BadMediaFail() resized = exif_fix_image_orientation(resized, exif_tags) # Fix orientation - if ((size_limits is None) or - (resized.size[0] > size_limits[0]) or + if ((resized.size[0] > size_limits[0]) or (resized.size[1] > size_limits[1])): resized.thumbnail(new_size, Image.ANTIALIAS) + # Truncate basename as needed so len(basename + file_tail) <= 255 resized_filename = (basename[:MAX_FILENAME_LENGTH - len(file_tail)] + file_tail) resized_filepath = create_pub_filepath(entry, resized_filename) From 31dd6013b895cbd34b022b309eddb1c2d61503a9 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 20 Mar 2012 22:07:32 -0400 Subject: [PATCH 03/14] Refactor data posts into one do_post function. All the data posts in these tests had a lot of common code. Putting all that into a function makes it easier to write more tests (which I'll be doing in a bit) and see what's really being tested. --- mediagoblin/tests/test_submission.py | 164 +++++++++------------------ 1 file changed, 52 insertions(+), 112 deletions(-) diff --git a/mediagoblin/tests/test_submission.py b/mediagoblin/tests/test_submission.py index 2f11bdfb..730efbd2 100644 --- a/mediagoblin/tests/test_submission.py +++ b/mediagoblin/tests/test_submission.py @@ -39,6 +39,8 @@ EVIL_PNG = pkg_resources.resource_filename( GOOD_TAG_STRING = 'yin,yang' BAD_TAG_STRING = 'rage,' + 'f' * 26 + 'u' * 26 +FORM_CONTEXT = ['mediagoblin/submit/start.html', 'submit_form'] +REQUEST_CONTEXT = ['mediagoblin/user_pages/user.html', 'request'] class TestSubmission: def setUp(self): @@ -61,39 +63,39 @@ class TestSubmission: def logout(self): self.test_app.get('/auth/logout/') + def do_post(self, data, *context_keys, **kwargs): + url = kwargs.pop('url', '/submit/') + do_follow = kwargs.pop('do_follow', False) + template.clear_test_template_context() + response = self.test_app.post(url, data, **kwargs) + if do_follow: + response.follow() + context_data = template.TEMPLATE_TEST_CONTEXT + for key in context_keys: + context_data = context_data[key] + return response, context_data + + def upload_data(self, filename): + return {'upload_files': [('file', filename)]} + def test_missing_fields(self): # Test blank form # --------------- - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', {}) - context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/submit/start.html'] - form = context['submit_form'] + response, form = self.do_post({}, *FORM_CONTEXT) assert form.file.errors == [u'You must provide a file.'] # Test blank file # --------------- - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', { - 'title': 'test title'}) - context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/submit/start.html'] - form = context['submit_form'] + response, form = self.do_post({'title': 'test title'}, *FORM_CONTEXT) assert form.file.errors == [u'You must provide a file.'] def test_normal_uploads(self): # Test JPG # -------- - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', { - 'title': 'Normal upload 1' - }, upload_files=[( - 'file', GOOD_JPG)]) - - # User should be redirected - response.follow() + response, context = self.do_post({'title': 'Normal upload 1'}, + do_follow=True, + **self.upload_data(GOOD_JPG)) assert_equal( urlparse.urlsplit(response.location)[2], '/u/chris/') @@ -110,14 +112,9 @@ class TestSubmission: # Test PNG # -------- - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', { - 'title': 'Normal upload 2' - }, upload_files=[( - 'file', GOOD_PNG)]) - - response.follow() + response, context = self.do_post({'title': 'Normal upload 2'}, + do_follow=True, + **self.upload_data(GOOD_PNG)) assert_equal( urlparse.urlsplit(response.location)[2], '/u/chris/') @@ -127,18 +124,10 @@ class TestSubmission: def test_tags(self): # Good tag string # -------- - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', { - 'title': 'Balanced Goblin', - 'tags': GOOD_TAG_STRING - }, upload_files=[( - 'file', GOOD_JPG)]) - - # New media entry with correct tags should be created - response.follow() - context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/user_pages/user.html'] - request = context['request'] + response, request = self.do_post({'title': 'Balanced Goblin', + 'tags': GOOD_TAG_STRING}, + *REQUEST_CONTEXT, do_follow=True, + **self.upload_data(GOOD_JPG)) media = request.db.MediaEntry.find({'title': 'Balanced Goblin'})[0] assert_equal(media.tags, [{'name': u'yin', 'slug': u'yin'}, @@ -146,35 +135,18 @@ class TestSubmission: # Test tags that are too long # --------------- - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', { - 'title': 'Balanced Goblin', - 'tags': BAD_TAG_STRING - }, upload_files=[( - 'file', GOOD_JPG)]) - - # Too long error should be raised - context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/submit/start.html'] - form = context['submit_form'] + response, form = self.do_post({'title': 'Balanced Goblin', + 'tags': BAD_TAG_STRING}, + *FORM_CONTEXT, + **self.upload_data(GOOD_JPG)) assert form.tags.errors == [ u'Tags must be shorter than 50 characters. Tags that are too long'\ ': ffffffffffffffffffffffffffuuuuuuuuuuuuuuuuuuuuuuuuuu'] def test_delete(self): - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', { - 'title': 'Balanced Goblin', - }, upload_files=[( - 'file', GOOD_JPG)]) - - # Post image - response.follow() - - request = template.TEMPLATE_TEST_CONTEXT[ - 'mediagoblin/user_pages/user.html']['request'] - + response, request = self.do_post({'title': 'Balanced Goblin'}, + *REQUEST_CONTEXT, do_follow=True, + **self.upload_data(GOOD_JPG)) media = request.db.MediaEntry.find({'title': 'Balanced Goblin'})[0] # Does media entry exist? @@ -182,19 +154,11 @@ class TestSubmission: # Do not confirm deletion # --------------------------------------------------- - response = self.test_app.post( - request.urlgen('mediagoblin.user_pages.media_confirm_delete', - # No work: user=media.uploader().username, - user=self.test_user.username, - media=media._id), - # no value means no confirm - {}) - - response.follow() - - request = template.TEMPLATE_TEST_CONTEXT[ - 'mediagoblin/user_pages/user.html']['request'] - + delete_url = request.urlgen( + 'mediagoblin.user_pages.media_confirm_delete', + user=self.test_user.username, media=media._id) + # Empty data means don't confirm + response = self.do_post({}, do_follow=True, url=delete_url)[0] media = request.db.MediaEntry.find({'title': 'Balanced Goblin'})[0] # Does media entry still exist? @@ -202,18 +166,8 @@ class TestSubmission: # Confirm deletion # --------------------------------------------------- - response = self.test_app.post( - request.urlgen('mediagoblin.user_pages.media_confirm_delete', - # No work: user=media.uploader().username, - user=self.test_user.username, - media=media._id), - {'confirm': 'y'}) - - response.follow() - - request = template.TEMPLATE_TEST_CONTEXT[ - 'mediagoblin/user_pages/user.html']['request'] - + response, request = self.do_post({'confirm': 'y'}, *REQUEST_CONTEXT, + do_follow=True, url=delete_url) # Does media entry still exist? assert_false( request.db.MediaEntry.find( @@ -222,15 +176,9 @@ class TestSubmission: def test_malicious_uploads(self): # Test non-suppoerted file with non-supported extension # ----------------------------------------------------- - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', { - 'title': 'Malicious Upload 1' - }, upload_files=[( - 'file', EVIL_FILE)]) - - context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/submit/start.html'] - form = context['submit_form'] + response, form = self.do_post({'title': 'Malicious Upload 1'}, + *FORM_CONTEXT, + **self.upload_data(EVIL_FILE)) assert re.match(r'^Could not extract any file extension from ".*?"$', str(form.file.errors[0])) assert len(form.file.errors) == 1 @@ -240,13 +188,9 @@ class TestSubmission: # Test non-supported file with .jpg extension # ------------------------------------------- - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', { - 'title': 'Malicious Upload 2' - }, upload_files=[( - 'file', EVIL_JPG)]) - response.follow() + response, context = self.do_post({'title': 'Malicious Upload 2'}, + do_follow=True, + **self.upload_data(EVIL_JPG)) assert_equal( urlparse.urlsplit(response.location)[2], '/u/chris/') @@ -260,13 +204,9 @@ class TestSubmission: # Test non-supported file with .png extension # ------------------------------------------- - template.clear_test_template_context() - response = self.test_app.post( - '/submit/', { - 'title': 'Malicious Upload 3' - }, upload_files=[( - 'file', EVIL_PNG)]) - response.follow() + response, context = self.do_post({'title': 'Malicious Upload 3'}, + do_follow=True, + **self.upload_data(EVIL_PNG)) assert_equal( urlparse.urlsplit(response.location)[2], '/u/chris/') From 77445d13adbceab08681d77d736c5226a6cbf4db Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 20 Mar 2012 22:16:28 -0400 Subject: [PATCH 04/14] Refactor MediaEntry fetches/checks into check_media(). --- mediagoblin/tests/test_submission.py | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/mediagoblin/tests/test_submission.py b/mediagoblin/tests/test_submission.py index 730efbd2..65c3bac7 100644 --- a/mediagoblin/tests/test_submission.py +++ b/mediagoblin/tests/test_submission.py @@ -121,6 +121,14 @@ class TestSubmission: assert template.TEMPLATE_TEST_CONTEXT.has_key( 'mediagoblin/user_pages/user.html') + def check_media(self, request, find_data, count=None): + media = request.db.MediaEntry.find(find_data) + if count is not None: + assert_equal(media.count(), count) + if count == 0: + return + return media[0] + def test_tags(self): # Good tag string # -------- @@ -128,10 +136,10 @@ class TestSubmission: 'tags': GOOD_TAG_STRING}, *REQUEST_CONTEXT, do_follow=True, **self.upload_data(GOOD_JPG)) - media = request.db.MediaEntry.find({'title': 'Balanced Goblin'})[0] + media = self.check_media(request, {'title': 'Balanced Goblin'}, 1) assert_equal(media.tags, [{'name': u'yin', 'slug': u'yin'}, - {'name': u'yang', 'slug': u'yang'}]) + {'name': u'yang', 'slug': u'yang'}]) # Test tags that are too long # --------------- @@ -147,10 +155,7 @@ class TestSubmission: response, request = self.do_post({'title': 'Balanced Goblin'}, *REQUEST_CONTEXT, do_follow=True, **self.upload_data(GOOD_JPG)) - media = request.db.MediaEntry.find({'title': 'Balanced Goblin'})[0] - - # Does media entry exist? - assert_true(media) + media = self.check_media(request, {'title': 'Balanced Goblin'}, 1) # Do not confirm deletion # --------------------------------------------------- @@ -159,19 +164,13 @@ class TestSubmission: user=self.test_user.username, media=media._id) # Empty data means don't confirm response = self.do_post({}, do_follow=True, url=delete_url)[0] - media = request.db.MediaEntry.find({'title': 'Balanced Goblin'})[0] - - # Does media entry still exist? - assert_true(media) + media = self.check_media(request, {'title': 'Balanced Goblin'}, 1) # Confirm deletion # --------------------------------------------------- response, request = self.do_post({'confirm': 'y'}, *REQUEST_CONTEXT, do_follow=True, url=delete_url) - # Does media entry still exist? - assert_false( - request.db.MediaEntry.find( - {'_id': media._id}).count()) + self.check_media(request, {'_id': media._id}, 0) def test_malicious_uploads(self): # Test non-suppoerted file with non-supported extension From c0e87ec9d2ff79c98372fe802ac64e35d7e30853 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 20 Mar 2012 22:30:57 -0400 Subject: [PATCH 05/14] Refactor normal upload tests. This is nice because it means we do *all* the normal sanity tests for *all* the normal uploads. check_url() can be used in other tests too. --- mediagoblin/tests/test_submission.py | 42 +++++++++++----------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/mediagoblin/tests/test_submission.py b/mediagoblin/tests/test_submission.py index 65c3bac7..70093b58 100644 --- a/mediagoblin/tests/test_submission.py +++ b/mediagoblin/tests/test_submission.py @@ -89,37 +89,27 @@ class TestSubmission: response, form = self.do_post({'title': 'test title'}, *FORM_CONTEXT) assert form.file.errors == [u'You must provide a file.'] + def check_url(self, response, path): + assert_equal(urlparse.urlsplit(response.location)[2], path) - def test_normal_uploads(self): - # Test JPG - # -------- - response, context = self.do_post({'title': 'Normal upload 1'}, - do_follow=True, - **self.upload_data(GOOD_JPG)) - assert_equal( - urlparse.urlsplit(response.location)[2], - '/u/chris/') - assert template.TEMPLATE_TEST_CONTEXT.has_key( - 'mediagoblin/user_pages/user.html') - + def check_normal_upload(self, title, filename): + response, context = self.do_post({'title': title}, do_follow=True, + **self.upload_data(filename)) + self.check_url(response, '/u/{0}/'.format(self.test_user.username)) + assert_true(context.has_key('mediagoblin/user_pages/user.html')) # Make sure the media view is at least reachable, logged in... - self.test_app.get('/u/chris/m/normal-upload-1/') + url = '/u/{0}/m/{1}/'.format(self.test_user.username, + title.lower().replace(' ', '-')) + self.test_app.get(url) # ... and logged out too. self.logout() - self.test_app.get('/u/chris/m/normal-upload-1/') - # Log back in for the remaining tests. - self.login() + self.test_app.get(url) - # Test PNG - # -------- - response, context = self.do_post({'title': 'Normal upload 2'}, - do_follow=True, - **self.upload_data(GOOD_PNG)) - assert_equal( - urlparse.urlsplit(response.location)[2], - '/u/chris/') - assert template.TEMPLATE_TEST_CONTEXT.has_key( - 'mediagoblin/user_pages/user.html') + def test_normal_jpg(self): + self.check_normal_upload('Normal upload 1', GOOD_JPG) + + def test_normal_png(self): + self.check_normal_upload('Normal upload 2', GOOD_PNG) def check_media(self, request, find_data, count=None): media = request.db.MediaEntry.find(find_data) From c373903494486740e24687203683028968f10ce3 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 20 Mar 2012 22:36:56 -0400 Subject: [PATCH 06/14] Refactor false image tests. --- mediagoblin/tests/test_submission.py | 41 +++++++++------------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/mediagoblin/tests/test_submission.py b/mediagoblin/tests/test_submission.py index 70093b58..9926cec0 100644 --- a/mediagoblin/tests/test_submission.py +++ b/mediagoblin/tests/test_submission.py @@ -162,7 +162,7 @@ class TestSubmission: do_follow=True, url=delete_url) self.check_media(request, {'_id': media._id}, 0) - def test_malicious_uploads(self): + def test_evil_file(self): # Test non-suppoerted file with non-supported extension # ----------------------------------------------------- response, form = self.do_post({'title': 'Malicious Upload 1'}, @@ -171,38 +171,23 @@ class TestSubmission: assert re.match(r'^Could not extract any file extension from ".*?"$', str(form.file.errors[0])) assert len(form.file.errors) == 1 - # NOTE: The following 2 tests will ultimately fail, but they + def check_false_image(self, title, filename): + # NOTE: These images should ultimately fail, but they # *will* pass the initial form submission step. Instead, # they'll be caught as failures during the processing step. + response, context = self.do_post({'title': title}, do_follow=True, + **self.upload_data(filename)) + self.check_url(response, '/u/{0}/'.format(self.test_user.username)) + entry = mg_globals.database.MediaEntry.find_one({'title': title}) + assert_equal(entry.state, 'failed') + assert_equal(entry.fail_error, u'mediagoblin.processing:BadMediaFail') + def test_evil_jpg(self): # Test non-supported file with .jpg extension # ------------------------------------------- - response, context = self.do_post({'title': 'Malicious Upload 2'}, - do_follow=True, - **self.upload_data(EVIL_JPG)) - assert_equal( - urlparse.urlsplit(response.location)[2], - '/u/chris/') - - entry = mg_globals.database.MediaEntry.find_one( - {'title': 'Malicious Upload 2'}) - assert_equal(entry.state, 'failed') - assert_equal( - entry.fail_error, - u'mediagoblin.processing:BadMediaFail') + self.check_false_image('Malicious Upload 2', EVIL_JPG) + def test_evil_png(self): # Test non-supported file with .png extension # ------------------------------------------- - response, context = self.do_post({'title': 'Malicious Upload 3'}, - do_follow=True, - **self.upload_data(EVIL_PNG)) - assert_equal( - urlparse.urlsplit(response.location)[2], - '/u/chris/') - - entry = mg_globals.database.MediaEntry.find_one( - {'title': 'Malicious Upload 3'}) - assert_equal(entry.state, 'failed') - assert_equal( - entry.fail_error, - u'mediagoblin.processing:BadMediaFail') + self.check_false_image('Malicious Upload 3', EVIL_PNG) From d1f52dc776edb5a0342d1b2f4b2300563e043e4b Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 20 Mar 2012 22:40:32 -0400 Subject: [PATCH 07/14] Prefer nose assert_* methods to the assert built-in. --- mediagoblin/tests/test_submission.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/mediagoblin/tests/test_submission.py b/mediagoblin/tests/test_submission.py index 9926cec0..18726b15 100644 --- a/mediagoblin/tests/test_submission.py +++ b/mediagoblin/tests/test_submission.py @@ -82,12 +82,12 @@ class TestSubmission: # Test blank form # --------------- response, form = self.do_post({}, *FORM_CONTEXT) - assert form.file.errors == [u'You must provide a file.'] + assert_equal(form.file.errors, [u'You must provide a file.']) # Test blank file # --------------- response, form = self.do_post({'title': 'test title'}, *FORM_CONTEXT) - assert form.file.errors == [u'You must provide a file.'] + assert_equal(form.file.errors, [u'You must provide a file.']) def check_url(self, response, path): assert_equal(urlparse.urlsplit(response.location)[2], path) @@ -137,9 +137,10 @@ class TestSubmission: 'tags': BAD_TAG_STRING}, *FORM_CONTEXT, **self.upload_data(GOOD_JPG)) - assert form.tags.errors == [ - u'Tags must be shorter than 50 characters. Tags that are too long'\ - ': ffffffffffffffffffffffffffuuuuuuuuuuuuuuuuuuuuuuuuuu'] + assert_equal(form.tags.errors, [ + u'Tags must be shorter than 50 characters. ' \ + 'Tags that are too long: ' \ + 'ffffffffffffffffffffffffffuuuuuuuuuuuuuuuuuuuuuuuuuu']) def test_delete(self): response, request = self.do_post({'title': 'Balanced Goblin'}, @@ -168,8 +169,10 @@ class TestSubmission: response, form = self.do_post({'title': 'Malicious Upload 1'}, *FORM_CONTEXT, **self.upload_data(EVIL_FILE)) - assert re.match(r'^Could not extract any file extension from ".*?"$', str(form.file.errors[0])) - assert len(form.file.errors) == 1 + assert_equal(len(form.file.errors), 1) + assert_true(re.match( + r'^Could not extract any file extension from ".*?"$', + str(form.file.errors[0]))) def check_false_image(self, title, filename): # NOTE: These images should ultimately fail, but they From e75d4a0dd5895e7977812ed6a50df0ef24a97df0 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 20 Mar 2012 23:03:11 -0400 Subject: [PATCH 08/14] Make a function to generate test image filenames. --- mediagoblin/tests/test_submission.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/mediagoblin/tests/test_submission.py b/mediagoblin/tests/test_submission.py index 18726b15..36a0debb 100644 --- a/mediagoblin/tests/test_submission.py +++ b/mediagoblin/tests/test_submission.py @@ -15,26 +15,24 @@ # along with this program. If not, see . import urlparse -import pkg_resources import re from nose.tools import assert_equal, assert_true, assert_false +from pkg_resources import resource_filename from mediagoblin.tests.tools import setup_fresh_app, get_test_app, \ fixture_add_user from mediagoblin import mg_globals from mediagoblin.tools import template, common -GOOD_JPG = pkg_resources.resource_filename( - 'mediagoblin.tests', 'test_submission/good.jpg') -GOOD_PNG = pkg_resources.resource_filename( - 'mediagoblin.tests', 'test_submission/good.png') -EVIL_FILE = pkg_resources.resource_filename( - 'mediagoblin.tests', 'test_submission/evil') -EVIL_JPG = pkg_resources.resource_filename( - 'mediagoblin.tests', 'test_submission/evil.jpg') -EVIL_PNG = pkg_resources.resource_filename( - 'mediagoblin.tests', 'test_submission/evil.png') +def resource(filename): + return resource_filename('mediagoblin.tests', 'test_submission/' + filename) + +GOOD_JPG = resource('good.jpg') +GOOD_PNG = resource('good.png') +EVIL_FILE = resource('evil') +EVIL_JPG = resource('evil.jpg') +EVIL_PNG = resource('evil.png') GOOD_TAG_STRING = 'yin,yang' BAD_TAG_STRING = 'rage,' + 'f' * 26 + 'u' * 26 From 6573573dd1650a73fe058df43dfb5770217b2afa Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 20 Mar 2012 23:59:28 -0400 Subject: [PATCH 09/14] Add tests for image processing. Check filenames and image sizes. This test helps verify that bug #261 is actually fixed. In order to test that all the processed images are smaller, I needed to add an image that's bigger than processing.MEDIUM_SIZE, hence bigblue.png. --- mediagoblin/tests/test_submission.py | 24 ++++++++++++++++++ mediagoblin/tests/test_submission/bigblue.png | Bin 0 -> 3142 bytes 2 files changed, 24 insertions(+) create mode 100644 mediagoblin/tests/test_submission/bigblue.png diff --git a/mediagoblin/tests/test_submission.py b/mediagoblin/tests/test_submission.py index 36a0debb..8e30d9a2 100644 --- a/mediagoblin/tests/test_submission.py +++ b/mediagoblin/tests/test_submission.py @@ -15,7 +15,9 @@ # along with this program. If not, see . import urlparse +import os import re +import time from nose.tools import assert_equal, assert_true, assert_false from pkg_resources import resource_filename @@ -23,6 +25,7 @@ from pkg_resources import resource_filename from mediagoblin.tests.tools import setup_fresh_app, get_test_app, \ fixture_add_user from mediagoblin import mg_globals +from mediagoblin.processing import create_pub_filepath from mediagoblin.tools import template, common def resource(filename): @@ -33,6 +36,7 @@ GOOD_PNG = resource('good.png') EVIL_FILE = resource('evil') EVIL_JPG = resource('evil.jpg') EVIL_PNG = resource('evil.png') +BIG_BLUE = resource('bigblue.png') GOOD_TAG_STRING = 'yin,yang' BAD_TAG_STRING = 'rage,' + 'f' * 26 + 'u' * 26 @@ -192,3 +196,23 @@ class TestSubmission: # Test non-supported file with .png extension # ------------------------------------------- self.check_false_image('Malicious Upload 3', EVIL_PNG) + + def test_processing(self): + data = {'title': 'Big Blue'} + response, request = self.do_post(data, *REQUEST_CONTEXT, do_follow=True, + **self.upload_data(BIG_BLUE)) + media = self.check_media(request, data, 1) + last_size = 1024 ** 3 # Needs to be larger than bigblue.png + for key, basename in (('original', 'bigblue.png'), + ('medium', 'bigblue.medium.png'), + ('thumb', 'bigblue.thumbnail.png')): + # Does the processed image have a good filename? + filename = resource_filename( + 'mediagoblin.tests', + os.path.join('test_user_dev/media/public', + *media['media_files'].get(key, []))) + assert_true(filename.endswith('_' + basename)) + # Is it smaller than the last processed image we looked at? + size = os.stat(filename).st_size + assert_true(last_size > size) + last_size = size diff --git a/mediagoblin/tests/test_submission/bigblue.png b/mediagoblin/tests/test_submission/bigblue.png new file mode 100644 index 0000000000000000000000000000000000000000..2b2c2a447e2c5a3d23a5c70152b1f9d3e5eb1805 GIT binary patch literal 3142 zcmeAS@N?(olHy`uVBq!ia0y~yU{(NO4kn;Th|olPAjMc5&&^HED`9XhN=+wt5ey7$+zbrOtPBYo7#MiOz}y1~j0_SwU@l3DKzaz8gro>+I~MD(D}p)^<|gVW zg4qsrIFg%?tV2=+vmF?0AW?#fkU|8c6}w5KDgs3sL6eXb0b>@);YiluP=pk=xDy4b zijcwKAq5*qE0UX#OhQtG6t+agH*y33(;#+} zP!y?-=3QvgL5cyQA{;4jX%d&TiEyNRLR49elut(UF5yxDDN&5(T|^Czz4Roi941Q9 zXx@d_;7A#KH1EQyd8BMfld>A6N*(Ze_rp=9HgUEk8G065fNgvRPgg&ebxsLQ08%&~ A{r~^~ literal 0 HcmV?d00001 From 095fbdaf8d165dae390c4fb61b888309056b8fe6 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Sun, 25 Mar 2012 12:11:13 -0400 Subject: [PATCH 10/14] Add FilenameMunger class to processing, with tests. Munging filenames is something all media type processors want to be able to do, so I'm refactoring it out into a nice bite-sized class. --- mediagoblin/processing.py | 16 ++++++++++++++++ mediagoblin/tests/test_processing.py | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 mediagoblin/tests/test_processing.py diff --git a/mediagoblin/processing.py b/mediagoblin/processing.py index 1c84c557..b0d5751e 100644 --- a/mediagoblin/processing.py +++ b/mediagoblin/processing.py @@ -15,6 +15,7 @@ # along with this program. If not, see . import logging +import os from celery.task import Task @@ -42,6 +43,21 @@ def create_pub_filepath(entry, filename): # Media processing initial steps ################################ +class FilenameMunger(object): + MAX_FILENAME_LENGTH = 255 + + def __init__(self, path): + self.dirpath, self.basename = os.path.split(path) + self.basename, self.ext = os.path.splitext(self.basename) + self.ext = self.ext.lower() + + def munge(self, fmtstr): + basename_len = (self.MAX_FILENAME_LENGTH - + len(fmtstr.format(basename='', ext=self.ext))) + return fmtstr.format(basename=self.basename[:basename_len], + ext=self.ext) + + class ProcessMedia(Task): """ DEPRECATED -- This now resides in the individual media plugins diff --git a/mediagoblin/tests/test_processing.py b/mediagoblin/tests/test_processing.py new file mode 100644 index 00000000..6f3fad70 --- /dev/null +++ b/mediagoblin/tests/test_processing.py @@ -0,0 +1,20 @@ +#!/usr/bin/env python + +from nose.tools import assert_equal, assert_true, assert_false + +from mediagoblin import processing + +class TestProcessing(object): + def run_munge(self, input, format, output=None): + munger = processing.FilenameMunger(input) + result = munger.munge(format) + if output is None: + return result + assert_equal(output, result) + + def test_easy_filename_munge(self): + self.run_munge('/home/user/foo.TXT', '{basename}bar{ext}', 'foobar.txt') + + def test_long_filename_munge(self): + self.run_munge('{0}.png'.format('A' * 300), 'image-{basename}{ext}', + 'image-{0}.png'.format('A' * 245)) From 4774cfa3c0df8ce1e20f0220e560a3c5d2ff0b82 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Sun, 25 Mar 2012 12:16:19 -0400 Subject: [PATCH 11/14] Add documentation to the FilenameMunger class. --- mediagoblin/processing.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/mediagoblin/processing.py b/mediagoblin/processing.py index b0d5751e..fa9192d9 100644 --- a/mediagoblin/processing.py +++ b/mediagoblin/processing.py @@ -44,14 +44,29 @@ def create_pub_filepath(entry, filename): ################################ class FilenameMunger(object): - MAX_FILENAME_LENGTH = 255 + """Easily slice and dice filenames. + + Initialize this class with an original filename, then use the munge() + method to create new filenames based on the original. + + """ + MAX_FILENAME_LENGTH = 255 # VFAT's maximum filename length def __init__(self, path): + """Initialize a munger with one original filename.""" self.dirpath, self.basename = os.path.split(path) self.basename, self.ext = os.path.splitext(self.basename) self.ext = self.ext.lower() def munge(self, fmtstr): + """Return a new filename based on the initialized original. + + The fmtstr argumentcan include {basename} and {ext}, which will + fill in components of the original filename. The extension will + always be lowercased. The filename will also be trunacted to this + class' MAX_FILENAME_LENGTH characters. + + """ basename_len = (self.MAX_FILENAME_LENGTH - len(fmtstr.format(basename='', ext=self.ext))) return fmtstr.format(basename=self.basename[:basename_len], From ab35ad460581febecbf27e9550134c07754a9d9b Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Sun, 25 Mar 2012 13:26:57 -0400 Subject: [PATCH 12/14] Use FilenameMunger. --- mediagoblin/media_types/image/processing.py | 45 ++++++++------------- 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/mediagoblin/media_types/image/processing.py b/mediagoblin/media_types/image/processing.py index b67fedb3..37fa8613 100644 --- a/mediagoblin/media_types/image/processing.py +++ b/mediagoblin/media_types/image/processing.py @@ -19,21 +19,18 @@ import os from mediagoblin import mg_globals as mgg from mediagoblin.processing import BadMediaFail, \ - create_pub_filepath, THUMB_SIZE, MEDIUM_SIZE + create_pub_filepath, THUMB_SIZE, MEDIUM_SIZE, FilenameMunger from mediagoblin.tools.exif import exif_fix_image_orientation, \ extract_exif, clean_exif, get_gps_data, get_useful -MAX_FILENAME_LENGTH = 255 # the limit in VFAT -- seems like a good baseline - -def resize_image(entry, filename, basename, file_tail, exif_tags, workdir, - new_size, size_limits=(0, 0)): +def resize_image(entry, filename, new_path, exif_tags, workdir, new_size, + size_limits=(0, 0)): """Store a resized version of an image and return its pathname. Arguments: entry -- the entry for the image to resize filename -- the filename of the original image being resized - basename -- simple basename of the given filename - file_tail -- ending string and extension for the resized filename + new_path -- public file path for the new resized image exif_tags -- EXIF data for the original image workdir -- directory path for storing converted image files new_size -- 2-tuple size for the resized image @@ -50,18 +47,11 @@ def resize_image(entry, filename, basename, file_tail, exif_tags, workdir, (resized.size[1] > size_limits[1])): resized.thumbnail(new_size, Image.ANTIALIAS) - # Truncate basename as needed so len(basename + file_tail) <= 255 - resized_filename = (basename[:MAX_FILENAME_LENGTH - len(file_tail)] + - file_tail) - resized_filepath = create_pub_filepath(entry, resized_filename) - # Copy the new file to the conversion subdir, then remotely. - tmp_resized_filename = os.path.join(workdir, resized_filename) + tmp_resized_filename = os.path.join(workdir, new_path[-1]) with file(tmp_resized_filename, 'w') as resized_file: resized.save(resized_file) - mgg.public_store.copy_local_to_storage( - tmp_resized_filename, resized_filepath) - return resized_filepath + mgg.public_store.copy_local_to_storage(tmp_resized_filename, new_path) def process_image(entry): """ @@ -77,34 +67,33 @@ def process_image(entry): queued_filename = workbench.localized_file( mgg.queue_store, queued_filepath, 'source') - - filename_bits = os.path.splitext(queued_filename) - basename = os.path.split(filename_bits[0])[1] - extension = filename_bits[1].lower() + name_munger = FilenameMunger(queued_filename) # EXIF extraction exif_tags = extract_exif(queued_filename) gps_data = get_gps_data(exif_tags) # Always create a small thumbnail - thumb_filepath = resize_image(entry, queued_filename, basename, - '.thumbnail' + extension, exif_tags, - conversions_subdir, THUMB_SIZE) + thumb_filepath = create_pub_filepath( + entry, name_munger.munge('{basename}.thumbnail{ext}')) + resize_image(entry, queued_filename, thumb_filepath, + exif_tags, conversions_subdir, THUMB_SIZE) # If the size of the original file exceeds the specified size of a `medium` # file, a `.medium.jpg` files is created and later associated with the media # entry. - medium_filepath = resize_image(entry, queued_filename, basename, - '.medium' + extension, exif_tags, - conversions_subdir, MEDIUM_SIZE, MEDIUM_SIZE) + medium_filepath = create_pub_filepath( + entry, name_munger.munge('{basename}.medium{ext}')) + resize_image(entry, queued_filename, medium_filepath, + exif_tags, conversions_subdir, MEDIUM_SIZE, MEDIUM_SIZE) # we have to re-read because unlike PIL, not everything reads # things in string representation :) queued_file = file(queued_filename, 'rb') with queued_file: - #create_pub_filepath(entry, queued_filepath[-1]) - original_filepath = create_pub_filepath(entry, basename + extension) + original_filepath = create_pub_filepath( + entry, name_munger.munge('{basename}{ext}') ) with mgg.public_store.get_file(original_filepath, 'wb') \ as original_file: From 84725abd6439b6be42cccc074b8f2b63536fa30e Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Sun, 25 Mar 2012 13:32:25 -0400 Subject: [PATCH 13/14] Refactor video processing to use FilenameMunger. --- mediagoblin/media_types/video/processing.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/mediagoblin/media_types/video/processing.py b/mediagoblin/media_types/video/processing.py index 3a479802..98379d52 100644 --- a/mediagoblin/media_types/video/processing.py +++ b/mediagoblin/media_types/video/processing.py @@ -20,7 +20,7 @@ import os from mediagoblin import mg_globals as mgg from mediagoblin.processing import mark_entry_failed, \ - THUMB_SIZE, MEDIUM_SIZE, create_pub_filepath + THUMB_SIZE, MEDIUM_SIZE, create_pub_filepath, FilenameMunger from . import transcoders logging.basicConfig() @@ -49,17 +49,13 @@ def process_video(entry): queued_filename = workbench.localized_file( mgg.queue_store, queued_filepath, 'source') + name_munger = FilenameMunger(queued_filename) medium_filepath = create_pub_filepath( - entry, - '{original}-640p.webm'.format( - original=os.path.splitext( - queued_filepath[-1])[0] # Select the - )) + entry, name_munger.munge('{basename}-640p.webm')) thumbnail_filepath = create_pub_filepath( - entry, 'thumbnail.jpg') - + entry, name_munger.munge('{basename}.thumbnail.jpg')) # Create a temporary file for the video destination tmp_dst = tempfile.NamedTemporaryFile() From 28f364bd6d488955952aebe86033e5ba148da2fb Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Mon, 26 Mar 2012 13:40:35 -0400 Subject: [PATCH 14/14] Rename to FilenameBuilder, with a main method named fill. I think these names better convey what's actually going on. I updated the documentation a bit while I was at it. --- mediagoblin/media_types/image/processing.py | 10 +++++----- mediagoblin/media_types/video/processing.py | 8 ++++---- mediagoblin/processing.py | 20 +++++++++++--------- mediagoblin/tests/test_processing.py | 16 ++++++++-------- 4 files changed, 28 insertions(+), 26 deletions(-) diff --git a/mediagoblin/media_types/image/processing.py b/mediagoblin/media_types/image/processing.py index 37fa8613..554c72b8 100644 --- a/mediagoblin/media_types/image/processing.py +++ b/mediagoblin/media_types/image/processing.py @@ -19,7 +19,7 @@ import os from mediagoblin import mg_globals as mgg from mediagoblin.processing import BadMediaFail, \ - create_pub_filepath, THUMB_SIZE, MEDIUM_SIZE, FilenameMunger + create_pub_filepath, THUMB_SIZE, MEDIUM_SIZE, FilenameBuilder from mediagoblin.tools.exif import exif_fix_image_orientation, \ extract_exif, clean_exif, get_gps_data, get_useful @@ -67,7 +67,7 @@ def process_image(entry): queued_filename = workbench.localized_file( mgg.queue_store, queued_filepath, 'source') - name_munger = FilenameMunger(queued_filename) + name_builder = FilenameBuilder(queued_filename) # EXIF extraction exif_tags = extract_exif(queued_filename) @@ -75,7 +75,7 @@ def process_image(entry): # Always create a small thumbnail thumb_filepath = create_pub_filepath( - entry, name_munger.munge('{basename}.thumbnail{ext}')) + entry, name_builder.fill('{basename}.thumbnail{ext}')) resize_image(entry, queued_filename, thumb_filepath, exif_tags, conversions_subdir, THUMB_SIZE) @@ -83,7 +83,7 @@ def process_image(entry): # file, a `.medium.jpg` files is created and later associated with the media # entry. medium_filepath = create_pub_filepath( - entry, name_munger.munge('{basename}.medium{ext}')) + entry, name_builder.fill('{basename}.medium{ext}')) resize_image(entry, queued_filename, medium_filepath, exif_tags, conversions_subdir, MEDIUM_SIZE, MEDIUM_SIZE) @@ -93,7 +93,7 @@ def process_image(entry): with queued_file: original_filepath = create_pub_filepath( - entry, name_munger.munge('{basename}{ext}') ) + entry, name_builder.fill('{basename}{ext}') ) with mgg.public_store.get_file(original_filepath, 'wb') \ as original_file: diff --git a/mediagoblin/media_types/video/processing.py b/mediagoblin/media_types/video/processing.py index 98379d52..24c03648 100644 --- a/mediagoblin/media_types/video/processing.py +++ b/mediagoblin/media_types/video/processing.py @@ -20,7 +20,7 @@ import os from mediagoblin import mg_globals as mgg from mediagoblin.processing import mark_entry_failed, \ - THUMB_SIZE, MEDIUM_SIZE, create_pub_filepath, FilenameMunger + THUMB_SIZE, MEDIUM_SIZE, create_pub_filepath, FilenameBuilder from . import transcoders logging.basicConfig() @@ -49,13 +49,13 @@ def process_video(entry): queued_filename = workbench.localized_file( mgg.queue_store, queued_filepath, 'source') - name_munger = FilenameMunger(queued_filename) + name_builder = FilenameBuilder(queued_filename) medium_filepath = create_pub_filepath( - entry, name_munger.munge('{basename}-640p.webm')) + entry, name_builder.fill('{basename}-640p.webm')) thumbnail_filepath = create_pub_filepath( - entry, name_munger.munge('{basename}.thumbnail.jpg')) + entry, name_builder.fill('{basename}.thumbnail.jpg')) # Create a temporary file for the video destination tmp_dst = tempfile.NamedTemporaryFile() diff --git a/mediagoblin/processing.py b/mediagoblin/processing.py index fa9192d9..718351d5 100644 --- a/mediagoblin/processing.py +++ b/mediagoblin/processing.py @@ -43,28 +43,30 @@ def create_pub_filepath(entry, filename): # Media processing initial steps ################################ -class FilenameMunger(object): +class FilenameBuilder(object): """Easily slice and dice filenames. - Initialize this class with an original filename, then use the munge() + Initialize this class with an original file path, then use the fill() method to create new filenames based on the original. """ MAX_FILENAME_LENGTH = 255 # VFAT's maximum filename length def __init__(self, path): - """Initialize a munger with one original filename.""" + """Initialize a builder from an original file path.""" self.dirpath, self.basename = os.path.split(path) self.basename, self.ext = os.path.splitext(self.basename) self.ext = self.ext.lower() - def munge(self, fmtstr): - """Return a new filename based on the initialized original. + def fill(self, fmtstr): + """Build a new filename based on the original. - The fmtstr argumentcan include {basename} and {ext}, which will - fill in components of the original filename. The extension will - always be lowercased. The filename will also be trunacted to this - class' MAX_FILENAME_LENGTH characters. + The fmtstr argument can include the following: + {basename} -- the original basename, with the extension removed + {ext} -- the original extension, always lowercase + + If necessary, {basename} will be truncated so the filename does not + exceed this class' MAX_FILENAME_LENGTH in length. """ basename_len = (self.MAX_FILENAME_LENGTH - diff --git a/mediagoblin/tests/test_processing.py b/mediagoblin/tests/test_processing.py index 6f3fad70..417f91f3 100644 --- a/mediagoblin/tests/test_processing.py +++ b/mediagoblin/tests/test_processing.py @@ -5,16 +5,16 @@ from nose.tools import assert_equal, assert_true, assert_false from mediagoblin import processing class TestProcessing(object): - def run_munge(self, input, format, output=None): - munger = processing.FilenameMunger(input) - result = munger.munge(format) + def run_fill(self, input, format, output=None): + builder = processing.FilenameBuilder(input) + result = builder.fill(format) if output is None: return result assert_equal(output, result) - def test_easy_filename_munge(self): - self.run_munge('/home/user/foo.TXT', '{basename}bar{ext}', 'foobar.txt') + def test_easy_filename_fill(self): + self.run_fill('/home/user/foo.TXT', '{basename}bar{ext}', 'foobar.txt') - def test_long_filename_munge(self): - self.run_munge('{0}.png'.format('A' * 300), 'image-{basename}{ext}', - 'image-{0}.png'.format('A' * 245)) + def test_long_filename_fill(self): + self.run_fill('{0}.png'.format('A' * 300), 'image-{basename}{ext}', + 'image-{0}.png'.format('A' * 245))