From 1ffc3e02667b4218b6d39c36f9cbdfe8418dd2d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Tue, 22 Mar 2022 02:24:21 +0100 Subject: [PATCH] Fix bookmark access restrictions --- bookmarks/tests/test_bookmark_archive_view.py | 11 +++ .../tests/test_bookmark_bulk_edit_view.py | 94 ++++++++++++++++++- bookmarks/tests/test_bookmark_edit_view.py | 12 +++ bookmarks/tests/test_bookmark_remove_view.py | 10 ++ .../tests/test_bookmark_unarchive_view.py | 11 +++ bookmarks/views/bookmarks.py | 25 ++++- 6 files changed, 156 insertions(+), 7 deletions(-) diff --git a/bookmarks/tests/test_bookmark_archive_view.py b/bookmarks/tests/test_bookmark_archive_view.py index 32fa1a09..04a86f31 100644 --- a/bookmarks/tests/test_bookmark_archive_view.py +++ b/bookmarks/tests/test_bookmark_archive_view.py @@ -1,3 +1,4 @@ +from django.contrib.auth.models import User from django.test import TestCase from django.urls import reverse @@ -33,3 +34,13 @@ def test_should_redirect_to_return_url_when_specified(self): ) self.assertRedirects(response, reverse('bookmarks:close')) + + def test_can_only_archive_own_bookmarks(self): + other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') + bookmark = self.setup_bookmark(user=other_user) + + response = self.client.get(reverse('bookmarks:archive', args=[bookmark.id])) + bookmark.refresh_from_db() + + self.assertEqual(response.status_code, 404) + self.assertFalse(bookmark.is_archived) diff --git a/bookmarks/tests/test_bookmark_bulk_edit_view.py b/bookmarks/tests/test_bookmark_bulk_edit_view.py index 0db075aa..5ab6d39e 100644 --- a/bookmarks/tests/test_bookmark_bulk_edit_view.py +++ b/bookmarks/tests/test_bookmark_bulk_edit_view.py @@ -1,3 +1,4 @@ +from django.contrib.auth.models import User from django.forms import model_to_dict from django.test import TestCase from django.urls import reverse @@ -32,6 +33,21 @@ def test_bulk_archive(self): self.assertTrue(Bookmark.objects.get(id=bookmark2.id).is_archived) self.assertTrue(Bookmark.objects.get(id=bookmark3.id).is_archived) + def test_can_only_bulk_archive_own_bookmarks(self): + other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') + bookmark1 = self.setup_bookmark(user=other_user) + bookmark2 = self.setup_bookmark(user=other_user) + bookmark3 = self.setup_bookmark(user=other_user) + + self.client.post(reverse('bookmarks:bulk_edit'), { + 'bulk_archive': [''], + 'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)], + }) + + self.assertFalse(Bookmark.objects.get(id=bookmark1.id).is_archived) + self.assertFalse(Bookmark.objects.get(id=bookmark2.id).is_archived) + self.assertFalse(Bookmark.objects.get(id=bookmark3.id).is_archived) + def test_bulk_unarchive(self): bookmark1 = self.setup_bookmark(is_archived=True) bookmark2 = self.setup_bookmark(is_archived=True) @@ -46,6 +62,21 @@ def test_bulk_unarchive(self): self.assertFalse(Bookmark.objects.get(id=bookmark2.id).is_archived) self.assertFalse(Bookmark.objects.get(id=bookmark3.id).is_archived) + def test_can_only_bulk_unarchive_own_bookmarks(self): + other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') + bookmark1 = self.setup_bookmark(is_archived=True, user=other_user) + bookmark2 = self.setup_bookmark(is_archived=True, user=other_user) + bookmark3 = self.setup_bookmark(is_archived=True, user=other_user) + + self.client.post(reverse('bookmarks:bulk_edit'), { + 'bulk_unarchive': [''], + 'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)], + }) + + self.assertTrue(Bookmark.objects.get(id=bookmark1.id).is_archived) + self.assertTrue(Bookmark.objects.get(id=bookmark2.id).is_archived) + self.assertTrue(Bookmark.objects.get(id=bookmark3.id).is_archived) + def test_bulk_delete(self): bookmark1 = self.setup_bookmark() bookmark2 = self.setup_bookmark() @@ -57,8 +88,23 @@ def test_bulk_delete(self): }) self.assertIsNone(Bookmark.objects.filter(id=bookmark1.id).first()) - self.assertFalse(Bookmark.objects.filter(id=bookmark2.id).first()) - self.assertFalse(Bookmark.objects.filter(id=bookmark3.id).first()) + self.assertIsNone(Bookmark.objects.filter(id=bookmark2.id).first()) + self.assertIsNone(Bookmark.objects.filter(id=bookmark3.id).first()) + + def test_can_only_bulk_delete_own_bookmarks(self): + other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') + bookmark1 = self.setup_bookmark(user=other_user) + bookmark2 = self.setup_bookmark(user=other_user) + bookmark3 = self.setup_bookmark(user=other_user) + + self.client.post(reverse('bookmarks:bulk_edit'), { + 'bulk_delete': [''], + 'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)], + }) + + self.assertIsNotNone(Bookmark.objects.filter(id=bookmark1.id).first()) + self.assertIsNotNone(Bookmark.objects.filter(id=bookmark2.id).first()) + self.assertIsNotNone(Bookmark.objects.filter(id=bookmark3.id).first()) def test_bulk_tag(self): bookmark1 = self.setup_bookmark() @@ -81,6 +127,28 @@ def test_bulk_tag(self): self.assertCountEqual(bookmark2.tags.all(), [tag1, tag2]) self.assertCountEqual(bookmark3.tags.all(), [tag1, tag2]) + def test_can_only_bulk_tag_own_bookmarks(self): + other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') + bookmark1 = self.setup_bookmark(user=other_user) + bookmark2 = self.setup_bookmark(user=other_user) + bookmark3 = self.setup_bookmark(user=other_user) + tag1 = self.setup_tag() + tag2 = self.setup_tag() + + self.client.post(reverse('bookmarks:bulk_edit'), { + 'bulk_tag': [''], + 'bulk_tag_string': [f'{tag1.name} {tag2.name}'], + 'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)], + }) + + bookmark1.refresh_from_db() + bookmark2.refresh_from_db() + bookmark3.refresh_from_db() + + self.assertCountEqual(bookmark1.tags.all(), []) + self.assertCountEqual(bookmark2.tags.all(), []) + self.assertCountEqual(bookmark3.tags.all(), []) + def test_bulk_untag(self): tag1 = self.setup_tag() tag2 = self.setup_tag() @@ -102,6 +170,28 @@ def test_bulk_untag(self): self.assertCountEqual(bookmark2.tags.all(), []) self.assertCountEqual(bookmark3.tags.all(), []) + def test_can_only_bulk_untag_own_bookmarks(self): + other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') + tag1 = self.setup_tag() + tag2 = self.setup_tag() + bookmark1 = self.setup_bookmark(tags=[tag1, tag2], user=other_user) + bookmark2 = self.setup_bookmark(tags=[tag1, tag2], user=other_user) + bookmark3 = self.setup_bookmark(tags=[tag1, tag2], user=other_user) + + self.client.post(reverse('bookmarks:bulk_edit'), { + 'bulk_untag': [''], + 'bulk_tag_string': [f'{tag1.name} {tag2.name}'], + 'bookmark_id': [str(bookmark1.id), str(bookmark2.id), str(bookmark3.id)], + }) + + bookmark1.refresh_from_db() + bookmark2.refresh_from_db() + bookmark3.refresh_from_db() + + self.assertCountEqual(bookmark1.tags.all(), [tag1, tag2]) + self.assertCountEqual(bookmark2.tags.all(), [tag1, tag2]) + self.assertCountEqual(bookmark3.tags.all(), [tag1, tag2]) + def test_bulk_edit_handles_empty_bookmark_id(self): bookmark1 = self.setup_bookmark() bookmark2 = self.setup_bookmark() diff --git a/bookmarks/tests/test_bookmark_edit_view.py b/bookmarks/tests/test_bookmark_edit_view.py index 2209025d..c46d818c 100644 --- a/bookmarks/tests/test_bookmark_edit_view.py +++ b/bookmarks/tests/test_bookmark_edit_view.py @@ -1,3 +1,4 @@ +from django.contrib.auth.models import User from django.test import TestCase from django.urls import reverse @@ -95,3 +96,14 @@ def test_should_redirect_to_return_url(self): response = self.client.post(reverse('bookmarks:edit', args=[bookmark.id]), form_data) self.assertRedirects(response, form_data['return_url']) + + def test_can_only_edit_own_bookmarks(self): + other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') + bookmark = self.setup_bookmark(user=other_user) + form_data = self.create_form_data({'id': bookmark.id}) + + response = self.client.post(reverse('bookmarks:edit', args=[bookmark.id]), form_data) + bookmark.refresh_from_db() + self.assertNotEqual(bookmark.url, form_data['url']) + self.assertEqual(response.status_code, 404) + diff --git a/bookmarks/tests/test_bookmark_remove_view.py b/bookmarks/tests/test_bookmark_remove_view.py index 35ce07e7..32449a00 100644 --- a/bookmarks/tests/test_bookmark_remove_view.py +++ b/bookmarks/tests/test_bookmark_remove_view.py @@ -1,3 +1,4 @@ +from django.contrib.auth.models import User from django.test import TestCase from django.urls import reverse @@ -33,3 +34,12 @@ def test_should_redirect_to_return_url_when_specified(self): ) self.assertRedirects(response, reverse('bookmarks:close')) + + def test_can_only_edit_own_bookmarks(self): + other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') + bookmark = self.setup_bookmark(user=other_user) + + response = self.client.get(reverse('bookmarks:remove', args=[bookmark.id])) + + self.assertEqual(response.status_code, 404) + self.assertTrue(Bookmark.objects.filter(id=bookmark.id).exists()) diff --git a/bookmarks/tests/test_bookmark_unarchive_view.py b/bookmarks/tests/test_bookmark_unarchive_view.py index ece6b0c0..25cc79ad 100644 --- a/bookmarks/tests/test_bookmark_unarchive_view.py +++ b/bookmarks/tests/test_bookmark_unarchive_view.py @@ -1,3 +1,4 @@ +from django.contrib.auth.models import User from django.test import TestCase from django.urls import reverse @@ -33,3 +34,13 @@ def test_should_redirect_to_return_url_when_specified(self): ) self.assertRedirects(response, reverse('bookmarks:close')) + + def test_can_only_archive_own_bookmarks(self): + other_user = User.objects.create_user('otheruser', 'otheruser@example.com', 'password123') + bookmark = self.setup_bookmark(is_archived=True, user=other_user) + + response = self.client.get(reverse('bookmarks:unarchive', args=[bookmark.id])) + bookmark.refresh_from_db() + + self.assertEqual(response.status_code, 404) + self.assertTrue(bookmark.is_archived) diff --git a/bookmarks/views/bookmarks.py b/bookmarks/views/bookmarks.py index d665fe25..662c29a5 100644 --- a/bookmarks/views/bookmarks.py +++ b/bookmarks/views/bookmarks.py @@ -2,7 +2,7 @@ from django.contrib.auth.decorators import login_required from django.core.paginator import Paginator -from django.http import HttpResponseRedirect +from django.http import HttpResponseRedirect, Http404 from django.shortcuts import render from django.urls import reverse @@ -108,7 +108,10 @@ def new(request): @login_required def edit(request, bookmark_id: int): - bookmark = Bookmark.objects.get(pk=bookmark_id) + try: + bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user) + except Bookmark.DoesNotExist: + raise Http404('Bookmark does not exist') if request.method == 'POST': form = BookmarkForm(request.POST, instance=bookmark) @@ -137,7 +140,11 @@ def edit(request, bookmark_id: int): @login_required def remove(request, bookmark_id: int): - bookmark = Bookmark.objects.get(pk=bookmark_id) + try: + bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user) + except Bookmark.DoesNotExist: + raise Http404('Bookmark does not exist') + bookmark.delete() return_url = request.GET.get('return_url') return_url = return_url if return_url else reverse('bookmarks:index') @@ -146,7 +153,11 @@ def remove(request, bookmark_id: int): @login_required def archive(request, bookmark_id: int): - bookmark = Bookmark.objects.get(pk=bookmark_id) + try: + bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user) + except Bookmark.DoesNotExist: + raise Http404('Bookmark does not exist') + archive_bookmark(bookmark) return_url = request.GET.get('return_url') return_url = return_url if return_url else reverse('bookmarks:index') @@ -155,7 +166,11 @@ def archive(request, bookmark_id: int): @login_required def unarchive(request, bookmark_id: int): - bookmark = Bookmark.objects.get(pk=bookmark_id) + try: + bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user) + except Bookmark.DoesNotExist: + raise Http404('Bookmark does not exist') + unarchive_bookmark(bookmark) return_url = request.GET.get('return_url') return_url = return_url if return_url else reverse('bookmarks:archived')