From 461dd9717cce6c5b4d40bb4e76ca65d9d898d1df Mon Sep 17 00:00:00 2001 From: Elrond Date: Fri, 11 Jan 2013 14:18:27 +0100 Subject: [PATCH] Start to use the media_id in "admin" URLs. We have a bunch of URLs that are more for internal use. At least they're definitely not intended to be posted somewhere for long term useage. When those things affect a media, it's much better to reference the media by its id. This can't change, ever. This is better for races. Like someone posting a comment while the owner corrects a typo in the slug. --- mediagoblin/decorators.py | 8 ++++++-- mediagoblin/edit/views.py | 3 ++- mediagoblin/templates/mediagoblin/edit/edit.html | 2 +- .../templates/mediagoblin/user_pages/media.html | 6 +++--- .../mediagoblin/user_pages/media_confirm_delete.html | 2 +- mediagoblin/tests/test_submission.py | 10 ++++++++-- mediagoblin/user_pages/routing.py | 6 +++--- mediagoblin/user_pages/views.py | 5 +++-- 8 files changed, 27 insertions(+), 15 deletions(-) diff --git a/mediagoblin/decorators.py b/mediagoblin/decorators.py index 5533e81d..a40f1d5a 100644 --- a/mediagoblin/decorators.py +++ b/mediagoblin/decorators.py @@ -69,7 +69,7 @@ def user_may_delete_media(controller): """ @wraps(controller) def wrapper(request, *args, **kwargs): - uploader_id = MediaEntry.query.get(request.matchdict['media']).uploader + uploader_id = kwargs['media'].uploader if not (request.user.is_admin or request.user.id == uploader_id): raise Forbidden() @@ -209,12 +209,16 @@ def get_media_entry_by_id(controller): @wraps(controller) def wrapper(request, *args, **kwargs): media = MediaEntry.query.filter_by( - id=request.matchdict['media'], + id=request.matchdict['media_id'], state=u'processed').first() # Still no media? Okay, 404. if not media: return render_404(request) + given_username = request.matchdict.get('user') + if given_username and (given_username != media.get_uploader.username): + return render_404(request) + return controller(request, media=media, *args, **kwargs) return wrapper diff --git a/mediagoblin/edit/views.py b/mediagoblin/edit/views.py index 2f669c66..505106a4 100644 --- a/mediagoblin/edit/views.py +++ b/mediagoblin/edit/views.py @@ -27,6 +27,7 @@ from mediagoblin.auth import lib as auth_lib from mediagoblin.edit import forms from mediagoblin.edit.lib import may_edit_media from mediagoblin.decorators import (require_active_login, active_user_from_url, + get_media_entry_by_id, get_user_media_entry, user_may_alter_collection, get_user_collection) from mediagoblin.tools.response import render_to_response, redirect from mediagoblin.tools.translate import pass_to_ugettext as _ @@ -37,7 +38,7 @@ from mediagoblin.db.util import check_media_slug_used, check_collection_slug_use import mimetypes -@get_user_media_entry +@get_media_entry_by_id @require_active_login def edit_media(request, media): if not may_edit_media(request, media): diff --git a/mediagoblin/templates/mediagoblin/edit/edit.html b/mediagoblin/templates/mediagoblin/edit/edit.html index 1f5b91f7..9a040095 100644 --- a/mediagoblin/templates/mediagoblin/edit/edit.html +++ b/mediagoblin/templates/mediagoblin/edit/edit.html @@ -29,7 +29,7 @@

{% trans media_title=media.title %}Editing {{ media_title }}{% endtrans %}

diff --git a/mediagoblin/templates/mediagoblin/user_pages/media.html b/mediagoblin/templates/mediagoblin/user_pages/media.html index 11f2a2a1..10b48296 100644 --- a/mediagoblin/templates/mediagoblin/user_pages/media.html +++ b/mediagoblin/templates/mediagoblin/user_pages/media.html @@ -83,11 +83,11 @@ request.user.is_admin) %} {% set edit_url = request.urlgen('mediagoblin.edit.edit_media', user= media.get_uploader.username, - media= media.id) %} + media_id=media.id) %} {% trans %}Edit{% endtrans %} {% set delete_url = request.urlgen('mediagoblin.user_pages.media_confirm_delete', user= media.get_uploader.username, - media= media.id) %} + media_id=media.id) %} {% trans %}Delete{% endtrans %} {% endif %} {% autoescape False %} @@ -104,7 +104,7 @@ {% if request.user %} + media_id=media.id) }}" method="POST" id="form_comment">

{% trans %}You can use Markdown for formatting.{% endtrans %}

diff --git a/mediagoblin/templates/mediagoblin/user_pages/media_confirm_delete.html b/mediagoblin/templates/mediagoblin/user_pages/media_confirm_delete.html index 833f500d..d2a5655e 100644 --- a/mediagoblin/templates/mediagoblin/user_pages/media_confirm_delete.html +++ b/mediagoblin/templates/mediagoblin/user_pages/media_confirm_delete.html @@ -23,7 +23,7 @@

diff --git a/mediagoblin/tests/test_submission.py b/mediagoblin/tests/test_submission.py index faf4e744..53330c48 100644 --- a/mediagoblin/tests/test_submission.py +++ b/mediagoblin/tests/test_submission.py @@ -161,11 +161,17 @@ class TestSubmission: media = self.check_media(request, {'title': u'Balanced Goblin'}, 1) media_id = media.id + # At least render the edit page + edit_url = request.urlgen( + 'mediagoblin.edit.edit_media', + user=self.test_user.username, media_id=media_id) + self.test_app.get(edit_url) + # Add a comment, so we can test for its deletion later. self.check_comments(request, media_id, 0) comment_url = request.urlgen( 'mediagoblin.user_pages.media_post_comment', - user=self.test_user.username, media=media_id) + user=self.test_user.username, media_id=media_id) response = self.do_post({'comment_content': 'i love this test'}, url=comment_url, do_follow=True)[0] self.check_comments(request, media_id, 1) @@ -174,7 +180,7 @@ class TestSubmission: # --------------------------------------------------- delete_url = request.urlgen( 'mediagoblin.user_pages.media_confirm_delete', - user=self.test_user.username, media=media_id) + user=self.test_user.username, media_id=media_id) # Empty data means don't confirm response = self.do_post({}, do_follow=True, url=delete_url)[0] media = self.check_media(request, {'title': u'Balanced Goblin'}, 1) diff --git a/mediagoblin/user_pages/routing.py b/mediagoblin/user_pages/routing.py index 63bf5c2a..d36e7598 100644 --- a/mediagoblin/user_pages/routing.py +++ b/mediagoblin/user_pages/routing.py @@ -24,12 +24,12 @@ add_route('mediagoblin.user_pages.media_home', 'mediagoblin.user_pages.views:media_home') add_route('mediagoblin.user_pages.media_confirm_delete', - '/u//m//confirm-delete/', + '/u//m//confirm-delete/', 'mediagoblin.user_pages.views:media_confirm_delete') # Submission handling of new comments. TODO: only allow for POST methods add_route('mediagoblin.user_pages.media_post_comment', - '/u//m//comment/add/', + '/u//m//comment/add/', 'mediagoblin.user_pages.views:media_post_comment') add_route('mediagoblin.user_pages.user_gallery', @@ -74,7 +74,7 @@ add_route('mediagoblin.user_pages.processing_panel', # Stray edit routes add_route('mediagoblin.edit.edit_media', - '/u//m//edit/', + '/u//m//edit/', 'mediagoblin.edit.views:edit_media') add_route('mediagoblin.edit.attachments', diff --git a/mediagoblin/user_pages/views.py b/mediagoblin/user_pages/views.py index f115c3b8..a3327a9e 100644 --- a/mediagoblin/user_pages/views.py +++ b/mediagoblin/user_pages/views.py @@ -28,6 +28,7 @@ from mediagoblin.user_pages import forms as user_forms from mediagoblin.user_pages.lib import send_comment_email from mediagoblin.decorators import (uses_pagination, get_user_media_entry, + get_media_entry_by_id, require_active_login, user_may_delete_media, user_may_alter_collection, get_user_collection, get_user_collection_item, active_user_from_url) @@ -138,7 +139,7 @@ def media_home(request, media, page, **kwargs): 'app_config': mg_globals.app_config}) -@get_user_media_entry +@get_media_entry_by_id @require_active_login def media_post_comment(request, media): """ @@ -258,7 +259,7 @@ def media_collect(request, media): #TODO: Why does @user_may_delete_media not implicate @require_active_login? -@get_user_media_entry +@get_media_entry_by_id @require_active_login @user_may_delete_media def media_confirm_delete(request, media): -- 2.25.1