Merge pull request #1463 from bookwyrm-social/list-item-perms

Updates how permissions are verified in views

bookwyrm/forms.py

@@ -228,7 +228,7 @@ class ExpiryWidget(widgets.Select):
         elif selected_string == "forever":
             return None
         else:
-            return selected_string  # "This will raise
+            return selected_string  # This will raise
 
         return timezone.now() + interval
 

bookwyrm/models/base_model.py

@@ -1,8 +1,11 @@
 """ base model with default fields """
 import base64
 from Crypto import Random
+
+from django.core.exceptions import PermissionDenied
 from django.db import models
 from django.dispatch import receiver
+from django.http import Http404
 from django.utils.translation import gettext_lazy as _
 
 from bookwyrm.settings import DOMAIN
@@ -48,26 +51,26 @@ class BookWyrmModel(models.Model):
         """how to link to this object in the local app"""
         return self.get_remote_id().replace(f"https://{DOMAIN}", "")
 
-    def visible_to_user(self, viewer):
+    def raise_visible_to_user(self, viewer):
         """is a user authorized to view an object?"""
         # make sure this is an object with privacy owned by a user
         if not hasattr(self, "user") or not hasattr(self, "privacy"):
-            return None
+            return
 
         # viewer can't see it if the object's owner blocked them
         if viewer in self.user.blocks.all():
-            return False
+            raise Http404()
 
         # you can see your own posts and any public or unlisted posts
         if viewer == self.user or self.privacy in ["public", "unlisted"]:
-            return True
+            return
 
         # you can see the followers only posts of people you follow
         if (
             self.privacy == "followers"
             and self.user.followers.filter(id=viewer.id).first()
         ):
-            return True
+            return
 
         # you can see dms you are tagged in
         if hasattr(self, "mention_users"):
@@ -75,8 +78,32 @@ class BookWyrmModel(models.Model):
                 self.privacy == "direct"
                 and self.mention_users.filter(id=viewer.id).first()
             ):
-                return True
-        return False
+                return
+        raise Http404()
+
+    def raise_not_editable(self, viewer):
+        """does this user have permission to edit this object? liable to be overwritten
+        by models that inherit this base model class"""
+        if not hasattr(self, "user"):
+            return
+
+        # generally moderators shouldn't be able to edit other people's stuff
+        if self.user == viewer:
+            return
+
+        raise PermissionDenied()
+
+    def raise_not_deletable(self, viewer):
+        """does this user have permission to delete this object? liable to be
+        overwritten by models that inherit this base model class"""
+        if not hasattr(self, "user"):
+            return
+
+        # but generally moderators can delete other people's stuff
+        if self.user == viewer or viewer.has_perm("moderate_post"):
+            return
+
+        raise PermissionDenied()
 
 
 @receiver(models.signals.post_save)

bookwyrm/models/list.py

@@ -92,6 +92,12 @@ class ListItem(CollectionItemMixin, BookWyrmModel):
                 notification_type="ADD",
             )
 
+    def raise_not_deletable(self, viewer):
+        """the associated user OR the list owner can delete"""
+        if self.book_list.user == viewer:
+            return
+        super().raise_not_deletable(viewer)
+
     class Meta:
         """A book may only be placed into a list once,
         and each order in the list may be used only once"""

bookwyrm/models/shelf.py

@@ -1,5 +1,6 @@
 """ puttin' books on shelves """
 import re
+from django.core.exceptions import PermissionDenied
 from django.db import models
 from django.utils import timezone
 
@@ -57,6 +58,12 @@ class Shelf(OrderedCollectionMixin, BookWyrmModel):
         identifier = self.identifier or self.get_identifier()
         return f"{base_path}/books/{identifier}"
 
+    def raise_not_deletable(self, viewer):
+        """don't let anyone delete a default shelf"""
+        super().raise_not_deletable(viewer)
+        if not self.editable:
+            raise PermissionDenied()
+
     class Meta:
         """user/shelf unqiueness"""
 

bookwyrm/models/status.py

@@ -3,6 +3,7 @@ from dataclasses import MISSING
 import re
 
 from django.apps import apps
+from django.core.exceptions import PermissionDenied
 from django.core.validators import MaxValueValidator, MinValueValidator
 from django.db import models
 from django.dispatch import receiver
@@ -187,6 +188,13 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel):
         """json serialized activitypub class"""
         return self.to_activity_dataclass(pure=pure).serialize()
 
+    def raise_not_editable(self, viewer):
+        """certain types of status aren't editable"""
+        # first, the standard raise
+        super().raise_not_editable(viewer)
+        if isinstance(self, (GeneratedNote, ReviewRating)):
+            raise PermissionDenied()
+
 
 class GeneratedNote(Status):
     """these are app-generated messages about user activity"""

bookwyrm/templates/lists/list.html

@@ -66,14 +66,14 @@
                             <p>{% blocktrans with username=item.user.display_name user_path=item.user.local_path %}Added by <a href="{{ user_path }}">{{ username }}</a>{% endblocktrans %}</p>
                             </div>
                         </div>
-                        {% if list.user == request.user or list.curation == 'open' and item.user == request.user %}
+                        {% if list.user == request.user %}
                         <div class="card-footer-item">
                             <form name="set-position" method="post" action="{% url 'list-set-book-position' item.id %}">
+                                {% csrf_token %}
                                 <div class="field has-addons mb-0">
                                     <div class="control">
                                         <label for="input-list-position" class="button is-transparent is-small">{% trans "List position" %}</label>
                                     </div>
-                                    {% csrf_token %}
                                     <div class="control">
                                         <input id="input_list_position" class="input is-small" type="number" min="1" name="position" value="{{ item.order }}">
                                     </div>
@@ -83,7 +83,9 @@
                                 </div>
                             </form>
                         </div>
-                        <form name="add-book" method="post" action="{% url 'list-remove-book' list.id %}" class="card-footer-item">
+                        {% endif %}
+                        {% if list.user == request.user or list.curation == 'open' and item.user == request.user %}
+                        <form name="remove-book" method="post" action="{% url 'list-remove-book' list.id %}" class="card-footer-item">
                             {% csrf_token %}
                             <input type="hidden" name="item" value="{{ item.id }}">
                             <button type="submit" class="button is-small is-danger">{% trans "Remove" %}</button>

bookwyrm/tests/models/test_base_model.py

@@ -1,5 +1,6 @@
 """ testing models """
 from unittest.mock import patch
+from django.http import Http404
 from django.test import TestCase
 
 from bookwyrm import models
@@ -39,14 +40,14 @@ class BaseModel(TestCase):
         """these should be generated"""
         self.test_model.id = 1
         expected = self.test_model.get_remote_id()
-        self.assertEqual(expected, "https://%s/bookwyrmtestmodel/1" % DOMAIN)
+        self.assertEqual(expected, f"https://{DOMAIN}/bookwyrmtestmodel/1")
 
     def test_remote_id_with_user(self):
         """format of remote id when there's a user object"""
         self.test_model.user = self.local_user
         self.test_model.id = 1
         expected = self.test_model.get_remote_id()
-        self.assertEqual(expected, "https://%s/user/mouse/bookwyrmtestmodel/1" % DOMAIN)
+        self.assertEqual(expected, f"https://{DOMAIN}/user/mouse/bookwyrmtestmodel/1")
 
     def test_set_remote_id(self):
         """this function sets remote ids after creation"""
@@ -55,9 +56,7 @@ class BaseModel(TestCase):
         instance = models.Work.objects.create(title="work title")
         instance.remote_id = None
         base_model.set_remote_id(None, instance, True)
-        self.assertEqual(
-            instance.remote_id, "https://%s/book/%d" % (DOMAIN, instance.id)
-        )
+        self.assertEqual(instance.remote_id, f"https://{DOMAIN}/book/{instance.id}")
 
         # shouldn't set remote_id if it's not created
         instance.remote_id = None
@@ -70,28 +69,30 @@ class BaseModel(TestCase):
         obj = models.Status.objects.create(
             content="hi", user=self.remote_user, privacy="public"
         )
-        self.assertTrue(obj.visible_to_user(self.local_user))
+        self.assertIsNone(obj.raise_visible_to_user(self.local_user))
 
         obj = models.Shelf.objects.create(
             name="test", user=self.remote_user, privacy="unlisted"
         )
-        self.assertTrue(obj.visible_to_user(self.local_user))
+        self.assertIsNone(obj.raise_visible_to_user(self.local_user))
 
         obj = models.Status.objects.create(
             content="hi", user=self.remote_user, privacy="followers"
         )
-        self.assertFalse(obj.visible_to_user(self.local_user))
+        with self.assertRaises(Http404):
+            obj.raise_visible_to_user(self.local_user)
 
         obj = models.Status.objects.create(
             content="hi", user=self.remote_user, privacy="direct"
         )
-        self.assertFalse(obj.visible_to_user(self.local_user))
+        with self.assertRaises(Http404):
+            obj.raise_visible_to_user(self.local_user)
 
         obj = models.Status.objects.create(
             content="hi", user=self.remote_user, privacy="direct"
         )
         obj.mention_users.add(self.local_user)
-        self.assertTrue(obj.visible_to_user(self.local_user))
+        self.assertIsNone(obj.raise_visible_to_user(self.local_user))
 
     @patch("bookwyrm.activitystreams.add_status_task.delay")
     def test_object_visible_to_user_follower(self, _):
@@ -100,18 +101,19 @@ class BaseModel(TestCase):
         obj = models.Status.objects.create(
             content="hi", user=self.remote_user, privacy="followers"
         )
-        self.assertTrue(obj.visible_to_user(self.local_user))
+        self.assertIsNone(obj.raise_visible_to_user(self.local_user))
 
         obj = models.Status.objects.create(
             content="hi", user=self.remote_user, privacy="direct"
         )
-        self.assertFalse(obj.visible_to_user(self.local_user))
+        with self.assertRaises(Http404):
+            obj.raise_visible_to_user(self.local_user)
 
         obj = models.Status.objects.create(
             content="hi", user=self.remote_user, privacy="direct"
         )
         obj.mention_users.add(self.local_user)
-        self.assertTrue(obj.visible_to_user(self.local_user))
+        self.assertIsNone(obj.raise_visible_to_user(self.local_user))
 
     @patch("bookwyrm.activitystreams.add_status_task.delay")
     def test_object_visible_to_user_blocked(self, _):
@@ -120,9 +122,11 @@ class BaseModel(TestCase):
         obj = models.Status.objects.create(
             content="hi", user=self.remote_user, privacy="public"
         )
-        self.assertFalse(obj.visible_to_user(self.local_user))
+        with self.assertRaises(Http404):
+            obj.raise_visible_to_user(self.local_user)
 
         obj = models.Shelf.objects.create(
             name="test", user=self.remote_user, privacy="unlisted"
         )
-        self.assertFalse(obj.visible_to_user(self.local_user))
+        with self.assertRaises(Http404):
+            obj.raise_visible_to_user(self.local_user)

bookwyrm/tests/views/inbox/test_inbox.py

@@ -3,6 +3,7 @@ import json
 import pathlib
 from unittest.mock import patch
 
+from django.core.exceptions import PermissionDenied
 from django.http import HttpResponseNotAllowed, HttpResponseNotFound
 from django.test import TestCase, Client
 from django.test.client import RequestFactory
@@ -130,22 +131,24 @@ class Inbox(TestCase):
             "",
             HTTP_USER_AGENT="http.rb/4.4.1 (Mastodon/3.3.0; +https://mastodon.social/)",
         )
-        self.assertFalse(views.inbox.is_blocked_user_agent(request))
+        self.assertIsNone(views.inbox.raise_is_blocked_user_agent(request))
 
         models.FederatedServer.objects.create(
             server_name="mastodon.social", status="blocked"
         )
-        self.assertTrue(views.inbox.is_blocked_user_agent(request))
+        with self.assertRaises(PermissionDenied):
+            views.inbox.raise_is_blocked_user_agent(request)
 
     def test_is_blocked_activity(self):
         """check for blocked servers"""
         activity = {"actor": "https://mastodon.social/user/whaatever/else"}
-        self.assertFalse(views.inbox.is_blocked_activity(activity))
+        self.assertIsNone(views.inbox.raise_is_blocked_activity(activity))
 
         models.FederatedServer.objects.create(
             server_name="mastodon.social", status="blocked"
         )
-        self.assertTrue(views.inbox.is_blocked_activity(activity))
+        with self.assertRaises(PermissionDenied):
+            views.inbox.raise_is_blocked_activity(activity)
 
     @patch("bookwyrm.suggested_users.remove_user_task.delay")
     def test_create_by_deactivated_user(self, _):
@@ -157,11 +160,11 @@ class Inbox(TestCase):
         activity = self.create_json
         activity["actor"] = self.remote_user.remote_id
         activity["object"] = status_data
+        activity["type"] = "Create"
 
-        with patch("bookwyrm.views.inbox.has_valid_signature") as mock_valid:
-            mock_valid.return_value = True
-
-            result = self.client.post(
-                "/inbox", json.dumps(activity), content_type="application/json"
-            )
-        self.assertEqual(result.status_code, 403)
+        response = self.client.post(
+            "/inbox",
+            json.dumps(activity),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, 403)

bookwyrm/tests/views/test_feed.py

@@ -5,6 +5,7 @@ import pathlib
 
 from PIL import Image
 from django.core.files.base import ContentFile
+from django.http import Http404
 from django.template.response import TemplateResponse
 from django.test import TestCase
 from django.test.client import RequestFactory
@@ -81,9 +82,8 @@ class FeedViews(TestCase):
         request.user = self.local_user
         with patch("bookwyrm.views.feed.is_api_request") as is_api:
             is_api.return_value = False
-            result = view(request, "mouse", 12345)
-
-        self.assertEqual(result.status_code, 404)
+            with self.assertRaises(Http404):
+                view(request, "mouse", 12345)
 
     def test_status_page_not_found_wrong_user(self, *_):
         """there are so many views, this just makes sure it LOADS"""
@@ -102,9 +102,8 @@ class FeedViews(TestCase):
         request.user = self.local_user
         with patch("bookwyrm.views.feed.is_api_request") as is_api:
             is_api.return_value = False
-            result = view(request, "mouse", status.id)
-
-        self.assertEqual(result.status_code, 404)
+            with self.assertRaises(Http404):
+                view(request, "mouse", status.id)
 
     def test_status_page_with_image(self, *_):
         """there are so many views, this just makes sure it LOADS"""

bookwyrm/tests/views/test_goal.py

@@ -3,6 +3,7 @@ from unittest.mock import patch
 from django.utils import timezone
 
 from django.contrib.auth.models import AnonymousUser
+from django.http import Http404
 from django.template.response import TemplateResponse
 from django.test import TestCase
 from django.test.client import RequestFactory
@@ -103,8 +104,8 @@ class GoalViews(TestCase):
         request = self.factory.get("")
         request.user = self.rat
 
-        result = view(request, self.local_user.localname, self.year)
-        self.assertEqual(result.status_code, 404)
+        with self.assertRaises(Http404):
+            view(request, self.local_user.localname, self.year)
 
     @patch("bookwyrm.activitystreams.add_status_task.delay")
     def test_create_goal(self, _):

bookwyrm/tests/views/test_list_actions.py

@@ -568,5 +568,6 @@ class ListActionViews(TestCase):
         )
         request.user = self.rat
 
-        views.list.remove_book(request, self.list.id)
+        with self.assertRaises(PermissionDenied):
+            views.list.remove_book(request, self.list.id)
         self.assertTrue(self.list.listitem_set.exists())

bookwyrm/tests/views/test_shelf.py

@@ -105,7 +105,7 @@ class ShelfViews(TestCase):
         shelf.refresh_from_db()
 
         self.assertEqual(shelf.name, "cool name")
-        self.assertEqual(shelf.identifier, "testshelf-%d" % shelf.id)
+        self.assertEqual(shelf.identifier, f"testshelf-{shelf.id}")
 
     def test_edit_shelf_name_not_editable(self, *_):
         """can't change the name of an non-editable shelf"""

bookwyrm/tests/views/test_status.py

@@ -1,6 +1,7 @@
 """ test for app action functionality """
 import json
 from unittest.mock import patch
+from django.core.exceptions import PermissionDenied
 from django.test import TestCase
 from django.test.client import RequestFactory
 
@@ -196,9 +197,9 @@ class StatusViews(TestCase):
             )
 
         with patch("bookwyrm.activitystreams.remove_status_task.delay") as mock:
-            result = view(request, status.id)
+            with self.assertRaises(PermissionDenied):
+                view(request, status.id)
             self.assertFalse(mock.called)
-        self.assertEqual(result.status_code, 400)
 
         status.refresh_from_db()
         self.assertFalse(status.deleted)
@@ -214,9 +215,9 @@ class StatusViews(TestCase):
             )
 
         with patch("bookwyrm.activitystreams.remove_status_task.delay") as mock:
-            result = view(request, status.id)
+            with self.assertRaises(PermissionDenied):
+                view(request, status.id)
             self.assertFalse(mock.called)
-        self.assertEqual(result.status_code, 400)
 
         status.refresh_from_db()
         self.assertFalse(status.deleted)
@@ -375,7 +376,8 @@ http://www.fish.com/"""
         request = self.factory.post("")
         request.user = self.remote_user
 
-        view(request, status.id)
+        with self.assertRaises(PermissionDenied):
+            view(request, status.id)
 
         status.refresh_from_db()
         self.assertFalse(status.deleted)

bookwyrm/urls.py

@@ -276,7 +276,7 @@ urlpatterns = [
     # User books
     re_path(rf"{USER_PATH}/books/?$", views.Shelf.as_view(), name="user-shelves"),
     re_path(
-        rf"^{USER_PATH}/(helf|books)/(?P<shelf_identifier>[\w-]+)(.json)?/?$",
+        rf"^{USER_PATH}/(shelf|books)/(?P<shelf_identifier>[\w-]+)(.json)?/?$",
         views.Shelf.as_view(),
         name="shelf",
     ),

bookwyrm/views/admin/reports.py

@@ -105,7 +105,7 @@ def moderator_delete_user(request, user_id):
 
     # we can't delete users on other instances
     if not user.local:
-        raise PermissionDenied
+        raise PermissionDenied()
 
     form = forms.DeleteUserForm(request.POST, instance=user)
 

bookwyrm/views/block.py

@@ -1,6 +1,5 @@
 """ views for actions you can take in the application """
 from django.contrib.auth.decorators import login_required
-from django.http import HttpResponseNotFound
 from django.shortcuts import get_object_or_404, redirect
 from django.template.response import TemplateResponse
 from django.utils.decorators import method_decorator
@@ -32,12 +31,10 @@ class Block(View):
 def unblock(request, user_id):
     """undo a block"""
     to_unblock = get_object_or_404(models.User, id=user_id)
-    try:
-        block = models.UserBlocks.objects.get(
-            user_subject=request.user,
-            user_object=to_unblock,
-        )
-    except models.UserBlocks.DoesNotExist:
-        return HttpResponseNotFound()
+    block = get_object_or_404(
+        models.UserBlocks,
+        user_subject=request.user,
+        user_object=to_unblock,
+    )
     block.delete()
     return redirect("prefs-block")

bookwyrm/views/books.py

@@ -50,7 +50,7 @@ class Book(View):
         )
 
         if not book or not book.parent_work:
-            raise Http404
+            raise Http404()
 
         # all reviews for all editions of the book
         reviews = privacy_filter(

bookwyrm/views/feed.py

@@ -3,6 +3,7 @@ from django.contrib.auth.decorators import login_required
 from django.core.paginator import Paginator
 from django.db.models import Q
 from django.http import HttpResponseNotFound, Http404
+from django.shortcuts import get_object_or_404
 from django.template.response import TemplateResponse
 from django.utils import timezone
 from django.utils.decorators import method_decorator
@@ -93,17 +94,15 @@ class Status(View):
 
     def get(self, request, username, status_id):
         """display a particular status (and replies, etc)"""
-        try:
-            user = get_user_from_username(request.user, username)
-            status = models.Status.objects.select_subclasses().get(
-                user=user, id=status_id, deleted=False
-            )
-        except (ValueError, models.Status.DoesNotExist):
-            return HttpResponseNotFound()
-
+        user = get_user_from_username(request.user, username)
+        status = get_object_or_404(
+            models.Status.objects.select_subclasses(),
+            user=user,
+            id=status_id,
+            deleted=False,
+        )
         # make sure the user is authorized to see the status
-        if not status.visible_to_user(request.user):
-            return HttpResponseNotFound()
+        status.raise_visible_to_user(request.user)
 
         if is_api_request(request):
             return ActivitypubResponse(
@@ -133,6 +132,7 @@ class Replies(View):
         status = models.Status.objects.get(id=status_id)
         if status.user.localname != username:
             return HttpResponseNotFound()
+        status.raise_visible_to_user(request.user)
 
         return ActivitypubResponse(status.to_replies(**request.GET))
 

bookwyrm/views/follow.py

@@ -1,8 +1,7 @@
 """ views for actions you can take in the application """
 from django.contrib.auth.decorators import login_required
 from django.db import IntegrityError
-from django.http import HttpResponseBadRequest
-from django.shortcuts import redirect
+from django.shortcuts import get_object_or_404, redirect
 from django.views.decorators.http import require_POST
 
 from bookwyrm import models
@@ -78,12 +77,10 @@ def delete_follow_request(request):
     username = request.POST["user"]
     requester = get_user_from_username(request.user, username)
 
-    try:
-        follow_request = models.UserFollowRequest.objects.get(
-            user_subject=requester, user_object=request.user
-        )
-    except models.UserFollowRequest.DoesNotExist:
-        return HttpResponseBadRequest()
+    follow_request = get_object_or_404(
+        models.UserFollowRequest, user_subject=requester, user_object=request.user
+    )
+    follow_request.raise_not_deletable(request.user)
 
     follow_request.delete()
     return redirect(f"/user/{request.user.localname}")

bookwyrm/views/get_started.py

@@ -5,7 +5,6 @@ from django.contrib.auth.decorators import login_required
 from django.contrib.postgres.search import TrigramSimilarity
 from django.db.models.functions import Greatest
 from django.db.models import Count, Q
-from django.http import HttpResponseNotFound
 from django.shortcuts import get_object_or_404, redirect
 from django.template.response import TemplateResponse
 from django.utils.decorators import method_decorator
@@ -91,9 +90,8 @@ class GetStartedBooks(View):
         for (book_id, shelf_id) in shelve_actions:
             book = get_object_or_404(models.Edition, id=book_id)
             shelf = get_object_or_404(models.Shelf, id=shelf_id)
-            if shelf.user != request.user:
-                # hmmmmm
-                return HttpResponseNotFound()
+            shelf.raise_not_editable(request.user)
+
             models.ShelfBook.objects.create(book=book, shelf=shelf, user=request.user)
         return redirect(self.next_view)
 

bookwyrm/views/goal.py

@@ -31,8 +31,8 @@ class Goal(View):
         if not goal and year != timezone.now().year:
             return redirect("user-goal", username, current_year)
 
-        if goal and not goal.visible_to_user(request.user):
-            return HttpResponseNotFound()
+        if goal:
+            goal.raise_visible_to_user(request.user)
 
         data = {
             "goal_form": forms.GoalForm(instance=goal),
@@ -45,12 +45,12 @@ class Goal(View):
 
     def post(self, request, username, year):
         """update or create an annual goal"""
-        user = get_user_from_username(request.user, username)
-        if user != request.user:
-            return HttpResponseNotFound()
-
         year = int(year)
-        goal = models.AnnualGoal.objects.filter(year=year, user=request.user).first()
+        user = get_user_from_username(request.user, username)
+        goal = models.AnnualGoal.objects.filter(year=year, user=user).first()
+        if goal:
+            goal.raise_not_editable(request.user)
+
         form = forms.GoalForm(request.POST, instance=goal)
         if not form.is_valid():
             data = {
@@ -62,11 +62,11 @@ class Goal(View):
         goal = form.save()
 
         if request.POST.get("post-status"):
-            # create status, if appropraite
+            # create status, if appropriate
             template = get_template("snippets/generated_status/goal.html")
             create_generated_note(
                 request.user,
-                template.render({"goal": goal, "user": request.user}).strip(),
+                template.render({"goal": goal, "user": user}).strip(),
                 privacy=goal.privacy,
             )
 
@@ -78,5 +78,5 @@ class Goal(View):
 def hide_goal(request):
     """don't keep bugging people to set a goal"""
     request.user.show_goal = False
-    request.user.save(broadcast=False)
+    request.user.save(broadcast=False, update_fields=["show_goal"])
     return redirect(request.headers.get("Referer", "/"))

bookwyrm/views/import_data.py

@@ -80,7 +80,7 @@ class ImportStatus(View):
         """status of an import job"""
         job = get_object_or_404(models.ImportJob, id=job_id)
         if job.user != request.user:
-            raise PermissionDenied
+            raise PermissionDenied()
 
         try:
             task = app.AsyncResult(job.task_id)

bookwyrm/views/inbox.py

@@ -3,8 +3,9 @@ import json
 import re
 from urllib.parse import urldefrag
 
-from django.http import HttpResponse, HttpResponseNotFound
-from django.http import HttpResponseBadRequest, HttpResponseForbidden
+from django.http import HttpResponse, Http404
+from django.core.exceptions import BadRequest, PermissionDenied
+from django.shortcuts import get_object_or_404
 from django.utils.decorators import method_decorator
 from django.views import View
 from django.views.decorators.csrf import csrf_exempt
@@ -21,36 +22,30 @@ from bookwyrm.utils import regex
 class Inbox(View):
     """requests sent by outside servers"""
 
-    # pylint: disable=too-many-return-statements
     def post(self, request, username=None):
         """only works as POST request"""
         # first check if this server is on our shitlist
-        if is_blocked_user_agent(request):
-            return HttpResponseForbidden()
+        raise_is_blocked_user_agent(request)
 
         # make sure the user's inbox even exists
         if username:
-            try:
-                models.User.objects.get(localname=username)
-            except models.User.DoesNotExist:
-                return HttpResponseNotFound()
+            get_object_or_404(models.User, localname=username, is_active=True)
 
         # is it valid json? does it at least vaguely resemble an activity?
         try:
             activity_json = json.loads(request.body)
         except json.decoder.JSONDecodeError:
-            return HttpResponseBadRequest()
+            raise BadRequest()
 
         # let's be extra sure we didn't block this domain
-        if is_blocked_activity(activity_json):
-            return HttpResponseForbidden()
+        raise_is_blocked_activity(activity_json)
 
         if (
             not "object" in activity_json
             or not "type" in activity_json
             or not activity_json["type"] in activitypub.activity_objects
         ):
-            return HttpResponseNotFound()
+            raise Http404()
 
         # verify the signature
         if not has_valid_signature(request, activity_json):
@@ -65,32 +60,35 @@ class Inbox(View):
         return HttpResponse()
 
 
-def is_blocked_user_agent(request):
+def raise_is_blocked_user_agent(request):
     """check if a request is from a blocked server based on user agent"""
     # check user agent
     user_agent = request.headers.get("User-Agent")
     if not user_agent:
-        return False
+        return
     url = re.search(rf"https?://{regex.DOMAIN}/?", user_agent)
     if not url:
-        return False
+        return
     url = url.group()
-    return models.FederatedServer.is_blocked(url)
+    if models.FederatedServer.is_blocked(url):
+        raise PermissionDenied()
 
 
-def is_blocked_activity(activity_json):
+def raise_is_blocked_activity(activity_json):
     """get the sender out of activity json and check if it's blocked"""
     actor = activity_json.get("actor")
 
     # check if the user is banned/deleted
     existing = models.User.find_existing_by_remote_id(actor)
     if existing and existing.deleted:
-        return True
+        raise PermissionDenied()
 
     if not actor:
         # well I guess it's not even a valid activity so who knows
-        return False
-    return models.FederatedServer.is_blocked(actor)
+        return
+
+    if models.FederatedServer.is_blocked(actor):
+        raise PermissionDenied()
 
 
 @app.task(queue="medium_priority")

bookwyrm/views/list.py

@@ -3,12 +3,11 @@ from typing import Optional
 from urllib.parse import urlencode
 
 from django.contrib.auth.decorators import login_required
-from django.core.exceptions import PermissionDenied
 from django.core.paginator import Paginator
 from django.db import IntegrityError, transaction
 from django.db.models import Avg, Count, DecimalField, Q, Max
 from django.db.models.functions import Coalesce
-from django.http import HttpResponseNotFound, HttpResponseBadRequest, HttpResponse
+from django.http import HttpResponseBadRequest, HttpResponse
 from django.shortcuts import get_object_or_404, redirect
 from django.template.response import TemplateResponse
 from django.urls import reverse
@@ -111,8 +110,7 @@ class List(View):
     def get(self, request, list_id):
         """display a book list"""
         book_list = get_object_or_404(models.List, id=list_id)
-        if not book_list.visible_to_user(request.user):
-            return HttpResponseNotFound()
+        book_list.raise_visible_to_user(request.user)
 
         if is_api_request(request):
             return ActivitypubResponse(book_list.to_activity(**request.GET))
@@ -193,6 +191,8 @@ class List(View):
     def post(self, request, list_id):
         """edit a list"""
         book_list = get_object_or_404(models.List, id=list_id)
+        book_list.raise_not_editable(request.user)
+
         form = forms.ListForm(request.POST, instance=book_list)
         if not form.is_valid():
             return redirect("list", book_list.id)
@@ -207,9 +207,7 @@ class Curate(View):
     def get(self, request, list_id):
         """display a pending list"""
         book_list = get_object_or_404(models.List, id=list_id)
-        if not book_list.user == request.user:
-            # only the creater can curate the list
-            return HttpResponseNotFound()
+        book_list.raise_not_editable(request.user)
 
         data = {
             "list": book_list,
@@ -223,6 +221,8 @@ class Curate(View):
     def post(self, request, list_id):
         """edit a book_list"""
         book_list = get_object_or_404(models.List, id=list_id)
+        book_list.raise_not_editable(request.user)
+
         suggestion = get_object_or_404(models.ListItem, id=request.POST.get("item"))
         approved = request.POST.get("approved") == "true"
         if approved:
@@ -270,8 +270,7 @@ def delete_list(request, list_id):
     book_list = get_object_or_404(models.List, id=list_id)
 
     # only the owner or a moderator can delete a list
-    if book_list.user != request.user and not request.user.has_perm("moderate_post"):
-        raise PermissionDenied
+    book_list.raise_not_deletable(request.user)
 
     book_list.delete()
     return redirect("lists")
@@ -282,8 +281,7 @@ def delete_list(request, list_id):
 def add_book(request):
     """put a book on a list"""
     book_list = get_object_or_404(models.List, id=request.POST.get("list"))
-    if not book_list.visible_to_user(request.user):
-        return HttpResponseNotFound()
+    book_list.raise_visible_to_user(request.user)
 
     book = get_object_or_404(models.Edition, id=request.POST.get("book"))
     # do you have permission to add to the list?
@@ -331,16 +329,14 @@ def add_book(request):
 @login_required
 def remove_book(request, list_id):
     """remove a book from a list"""
+    book_list = get_object_or_404(models.List, id=list_id)
+    item = get_object_or_404(models.ListItem, id=request.POST.get("item"))
+    item.raise_not_deletable(request.user)
+
     with transaction.atomic():
-        book_list = get_object_or_404(models.List, id=list_id)
-        item = get_object_or_404(models.ListItem, id=request.POST.get("item"))
-
-        if not book_list.user == request.user and not item.user == request.user:
-            return HttpResponseNotFound()
-
         deleted_order = item.order
         item.delete()
-    normalize_book_list_ordering(book_list.id, start=deleted_order)
+        normalize_book_list_ordering(book_list.id, start=deleted_order)
     return redirect("list", list_id)
 
 
@@ -351,34 +347,32 @@ def set_book_position(request, list_item_id):
     Action for when the list user manually specifies a list position, takes
     special care with the unique ordering per list.
     """
+    list_item = get_object_or_404(models.ListItem, id=list_item_id)
+    list_item.book_list.raise_not_editable(request.user)
+    try:
+        int_position = int(request.POST.get("position"))
+    except ValueError:
+        return HttpResponseBadRequest("bad value for position. should be an integer")
+
+    if int_position < 1:
+        return HttpResponseBadRequest("position cannot be less than 1")
+
+    book_list = list_item.book_list
+
+    # the max position to which a book may be set is the highest order for
+    # books which are approved
+    order_max = book_list.listitem_set.filter(approved=True).aggregate(Max("order"))[
+        "order__max"
+    ]
+
+    int_position = min(int_position, order_max)
+
+    original_order = list_item.order
+    if original_order == int_position:
+        # no change
+        return HttpResponse(status=204)
+
     with transaction.atomic():
-        list_item = get_object_or_404(models.ListItem, id=list_item_id)
-        try:
-            int_position = int(request.POST.get("position"))
-        except ValueError:
-            return HttpResponseBadRequest(
-                "bad value for position. should be an integer"
-            )
-
-        if int_position < 1:
-            return HttpResponseBadRequest("position cannot be less than 1")
-
-        book_list = list_item.book_list
-
-        # the max position to which a book may be set is the highest order for
-        # books which are approved
-        order_max = book_list.listitem_set.filter(approved=True).aggregate(
-            Max("order")
-        )["order__max"]
-
-        int_position = min(int_position, order_max)
-
-        if request.user not in (book_list.user, list_item.user):
-            return HttpResponseNotFound()
-
-        original_order = list_item.order
-        if original_order == int_position:
-            return HttpResponse(status=204)
         if original_order > int_position:
             list_item.order = -1
             list_item.save()

bookwyrm/views/password.py

@@ -54,9 +54,9 @@ class PasswordReset(View):
         try:
             reset_code = models.PasswordReset.objects.get(code=code)
             if not reset_code.valid():
-                raise PermissionDenied
+                raise PermissionDenied()
         except models.PasswordReset.DoesNotExist:
-            raise PermissionDenied
+            raise PermissionDenied()
 
         return TemplateResponse(request, "password_reset.html", {"code": code})
 

bookwyrm/views/reading.py

@@ -45,9 +45,9 @@ class ReadingStatus(View):
         if not identifier:
             return HttpResponseBadRequest()
 
-        desired_shelf = models.Shelf.objects.filter(
-            identifier=identifier, user=request.user
-        ).first()
+        desired_shelf = get_object_or_404(
+            models.Shelf, identifier=identifier, user=request.user
+        )
 
         book = (
             models.Edition.viewer_aware_objects(request.user)
@@ -138,10 +138,7 @@ def update_readthrough_on_shelve(
 def edit_readthrough(request):
     """can't use the form because the dates are too finnicky"""
     readthrough = get_object_or_404(models.ReadThrough, id=request.POST.get("id"))
-
-    # don't let people edit other people's data
-    if request.user != readthrough.user:
-        return HttpResponseBadRequest()
+    readthrough.raise_not_editable(request.user)
 
     readthrough.start_date = load_date_in_user_tz_as_utc(
         request.POST.get("start_date"), request.user
@@ -178,10 +175,7 @@ def edit_readthrough(request):
 def delete_readthrough(request):
     """remove a readthrough"""
     readthrough = get_object_or_404(models.ReadThrough, id=request.POST.get("id"))
-
-    # don't let people edit other people's data
-    if request.user != readthrough.user:
-        return HttpResponseBadRequest()
+    readthrough.raise_not_deletable(request.user)
 
     readthrough.delete()
     return redirect(request.headers.get("Referer", "/"))
@@ -225,10 +219,7 @@ def load_date_in_user_tz_as_utc(date_str: str, user: models.User) -> datetime:
 def delete_progressupdate(request):
     """remove a progress update"""
     update = get_object_or_404(models.ProgressUpdate, id=request.POST.get("id"))
-
-    # don't let people edit other people's data
-    if request.user != update.user:
-        return HttpResponseBadRequest()
+    update.raise_not_deletable(request.user)
 
     update.delete()
     return redirect(request.headers.get("Referer", "/"))

bookwyrm/views/register.py

@@ -29,11 +29,11 @@ class Register(View):
             invite_code = request.POST.get("invite_code")
 
             if not invite_code:
-                raise PermissionDenied
+                raise PermissionDenied()
 
             invite = get_object_or_404(models.SiteInvite, code=invite_code)
             if not invite.valid():
-                raise PermissionDenied
+                raise PermissionDenied()
         else:
             invite = None
 

bookwyrm/views/shelf.py

@@ -1,11 +1,11 @@
-""" shelf views"""
+""" shelf views """
 from collections import namedtuple
 
-from django.db import IntegrityError
+from django.db import IntegrityError, transaction
 from django.db.models import OuterRef, Subquery, F
 from django.contrib.auth.decorators import login_required
 from django.core.paginator import Paginator
-from django.http import HttpResponseBadRequest, HttpResponseNotFound
+from django.http import HttpResponseBadRequest
 from django.shortcuts import get_object_or_404, redirect
 from django.template.response import TemplateResponse
 from django.utils.decorators import method_decorator
@@ -16,7 +16,7 @@ from django.views.decorators.http import require_POST
 from bookwyrm import forms, models
 from bookwyrm.activitypub import ActivitypubResponse
 from bookwyrm.settings import PAGE_LENGTH
-from .helpers import is_api_request, get_edition, get_user_from_username
+from .helpers import is_api_request, get_user_from_username
 from .helpers import privacy_filter
 
 
@@ -37,15 +37,11 @@ class Shelf(View):
 
         # get the shelf and make sure the logged in user should be able to see it
         if shelf_identifier:
-            try:
-                shelf = user.shelf_set.get(identifier=shelf_identifier)
-            except models.Shelf.DoesNotExist:
-                return HttpResponseNotFound()
-            if not shelf.visible_to_user(request.user):
-                return HttpResponseNotFound()
+            shelf = get_object_or_404(user.shelf_set, identifier=shelf_identifier)
+            shelf.raise_visible_to_user(request.user)
             books = shelf.books
-        # this is a constructed "all books" view, with a fake "shelf" obj
         else:
+            # this is a constructed "all books" view, with a fake "shelf" obj
             FakeShelf = namedtuple(
                 "Shelf", ("identifier", "name", "user", "books", "privacy")
             )
@@ -100,13 +96,11 @@ class Shelf(View):
     # pylint: disable=unused-argument
     def post(self, request, username, shelf_identifier):
         """edit a shelf"""
-        try:
-            shelf = request.user.shelf_set.get(identifier=shelf_identifier)
-        except models.Shelf.DoesNotExist:
-            return HttpResponseNotFound()
+        user = get_user_from_username(request.user, username)
+        shelf = get_object_or_404(user.shelf_set, identifier=shelf_identifier)
+        shelf.raise_not_editable(request.user)
 
-        if request.user != shelf.user:
-            return HttpResponseBadRequest()
+        # you can't change the name of the default shelves
         if not shelf.editable and request.POST.get("name") != shelf.name:
             return HttpResponseBadRequest()
 
@@ -134,8 +128,7 @@ def create_shelf(request):
 def delete_shelf(request, shelf_id):
     """user generated shelves"""
     shelf = get_object_or_404(models.Shelf, id=shelf_id)
-    if request.user != shelf.user or not shelf.editable:
-        return HttpResponseBadRequest()
+    shelf.raise_not_deletable()
 
     shelf.delete()
     return redirect("user-shelves", request.user.localname)
@@ -143,25 +136,28 @@ def delete_shelf(request, shelf_id):
 
 @login_required
 @require_POST
+@transaction.atomic
 def shelve(request):
     """put a book on a user's shelf"""
-    book = get_edition(request.POST.get("book"))
-
-    desired_shelf = models.Shelf.objects.filter(
-        identifier=request.POST.get("shelf"), user=request.user
-    ).first()
-    if not desired_shelf:
-        return HttpResponseNotFound()
+    book = get_object_or_404(models.Edition, id=request.POST.get("book"))
+    desired_shelf = get_object_or_404(
+        request.user.shelf_set, identifier=request.POST.get("shelf")
+    )
 
+    # first we need to remove from the specified shelf
     change_from_current_identifier = request.POST.get("change-shelf-from")
-    if change_from_current_identifier is not None:
-        current_shelf = models.Shelf.objects.get(
-            user=request.user, identifier=change_from_current_identifier
-        )
-        handle_unshelve(book, current_shelf)
+    if change_from_current_identifier:
+        # find the shelfbook obj and delete it
+        get_object_or_404(
+            models.ShelfBook,
+            book=book,
+            user=request.user,
+            shelf__identifier=change_from_current_identifier,
+        ).delete()
 
     # A book can be on multiple shelves, but only on one read status shelf at a time
     if desired_shelf.identifier in models.Shelf.READ_STATUS_IDENTIFIERS:
+        # figure out where state shelf it's currently on (if any)
         current_read_status_shelfbook = (
             models.ShelfBook.objects.select_related("shelf")
             .filter(
@@ -176,14 +172,16 @@ def shelve(request):
                 current_read_status_shelfbook.shelf.identifier
                 != desired_shelf.identifier
             ):
-                handle_unshelve(book, current_read_status_shelfbook.shelf)
+                current_read_status_shelfbook.delete()
             else:  # It is already on the shelf
                 return redirect(request.headers.get("Referer", "/"))
 
+        # create the new shelf-book entry
         models.ShelfBook.objects.create(
             book=book, shelf=desired_shelf, user=request.user
         )
     else:
+        # we're putting it on a custom shelf
         try:
             models.ShelfBook.objects.create(
                 book=book, shelf=desired_shelf, user=request.user
@@ -198,15 +196,12 @@ def shelve(request):
 @login_required
 @require_POST
 def unshelve(request):
-    """put a  on a user's shelf"""
-    book = models.Edition.objects.get(id=request.POST["book"])
-    current_shelf = models.Shelf.objects.get(id=request.POST["shelf"])
+    """put a on a user's shelf"""
+    book = get_object_or_404(models.Edition, id=request.POST.get("book"))
+    shelf_book = get_object_or_404(
+        models.ShelfBook, book=book, shelf__id=request.POST["shelf"]
+    )
+    shelf_book.raise_not_deletable(request.user)
 
-    handle_unshelve(book, current_shelf)
+    shelf_book.delete()
     return redirect(request.headers.get("Referer", "/"))
-
-
-def handle_unshelve(book, shelf):
-    """unshelve a book"""
-    row = models.ShelfBook.objects.get(book=book, shelf=shelf)
-    row.delete()

bookwyrm/views/status.py

@@ -98,8 +98,7 @@ class DeleteStatus(View):
         status = get_object_or_404(models.Status, id=status_id)
 
         # don't let people delete other people's statuses
-        if status.user != request.user and not request.user.has_perm("moderate_post"):
-            return HttpResponseBadRequest()
+        status.raise_not_deletable(request.user)
 
         # perform deletion
         status.delete()
@@ -115,12 +114,8 @@ class DeleteAndRedraft(View):
         status = get_object_or_404(
             models.Status.objects.select_subclasses(), id=status_id
         )
-        if isinstance(status, (models.GeneratedNote, models.ReviewRating)):
-            return HttpResponseBadRequest()
-
         # don't let people redraft other people's statuses
-        if status.user != request.user:
-            return HttpResponseBadRequest()
+        status.raise_not_editable(request.user)
 
         status_type = status.status_type.lower()
         if status.reply_parent:

bookwyrm/views/user.py

@@ -1,6 +1,7 @@
 """ non-interactive pages """
 from django.contrib.auth.decorators import login_required
 from django.core.paginator import Paginator
+from django.http import Http404
 from django.shortcuts import redirect
 from django.template.response import TemplateResponse
 from django.utils import timezone
@@ -77,8 +78,12 @@ class User(View):
         goal = models.AnnualGoal.objects.filter(
             user=user, year=timezone.now().year
         ).first()
-        if goal and not goal.visible_to_user(request.user):
-            goal = None
+        if goal:
+            try:
+                goal.raise_visible_to_user(request.user)
+            except Http404:
+                goal = None
+
         data = {
             "user": user,
             "is_self": is_self,

bookwyrm/views/wellknown.py

@@ -3,6 +3,7 @@
 from dateutil.relativedelta import relativedelta
 from django.http import HttpResponseNotFound
 from django.http import JsonResponse
+from django.shortcuts import get_object_or_404
 from django.template.response import TemplateResponse
 from django.utils import timezone
 from django.views.decorators.http import require_GET
@@ -19,10 +20,7 @@ def webfinger(request):
         return HttpResponseNotFound()
 
     username = resource.replace("acct:", "")
-    try:
-        user = models.User.objects.get(username__iexact=username)
-    except models.User.DoesNotExist:
-        return HttpResponseNotFound("No account found")
+    user = get_object_or_404(models.User, username__iexact=username)
 
     return JsonResponse(
         {