Extract access checks

bookmarks/api/routes.py

@@ -20,6 +20,7 @@ from bookmarks.api.serializers import (
 from bookmarks.models import Bookmark, BookmarkAsset, BookmarkSearch, Tag, User
 from bookmarks.services import assets, bookmarks, auto_tagging, website_loader
 from bookmarks.type_defs import HttpRequest
+from bookmarks.views import access
 
 logger = logging.getLogger(__name__)
 
@@ -169,11 +170,10 @@ class BookmarkAssetViewSet(
 
     def get_queryset(self):
         user = self.request.user
-        bookmark_id = self.kwargs["bookmark_id"]
+        # limit access to assets to the owner of the bookmark for now
-        if not Bookmark.objects.filter(id=bookmark_id, owner=user).exists():
+        bookmark = access.bookmark_write(self.request, self.kwargs["bookmark_id"])
-            raise Http404("Bookmark does not exist")
         return BookmarkAsset.objects.filter(
-            bookmark_id=bookmark_id, bookmark__owner=user
+            bookmark_id=bookmark.id, bookmark__owner=user
         )
 
     def get_serializer_context(self):
@@ -214,9 +214,7 @@ class BookmarkAssetViewSet(
                 {"error": "Asset upload is disabled."},
                 status=status.HTTP_403_FORBIDDEN,
             )
-        bookmark = Bookmark.objects.filter(id=bookmark_id, owner=request.user).first()
+        bookmark = access.bookmark_write(request, bookmark_id)
-        if not bookmark:
-            raise Http404("Bookmark does not exist")
 
         upload_file = request.FILES.get("file")
         if not upload_file:

bookmarks/tests/test_bookmark_details_modal.py

@@ -23,16 +23,18 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin
     def get_index_details_modal(self, bookmark):
         url = reverse("linkding:bookmarks.index") + f"?details={bookmark.id}"
         response = self.client.get(url)
-        soup = self.make_soup(response.content)
+        soup = self.make_soup(response.content.decode())
-        modal = soup.find("turbo-frame", {"id": "details-modal"})
+        return soup.select_one("div.modal.bookmark-details")
-        return modal
 
     def get_shared_details_modal(self, bookmark):
         url = reverse("linkding:bookmarks.shared") + f"?details={bookmark.id}"
         response = self.client.get(url)
-        soup = self.make_soup(response.content)
+        soup = self.make_soup(response.content.decode())
-        modal = soup.find("turbo-frame", {"id": "details-modal"})
+        return soup.select_one("div.modal.bookmark-details")
-        return modal
+
+    def has_details_modal(self, response):
+        soup = self.make_soup(response.content.decode())
+        return soup.select_one("div.modal.bookmark-details") is not None
 
     def find_section_content(self, soup, section_name):
         h3 = soup.find("h3", string=section_name)
@@ -53,35 +55,6 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin
     def find_asset(self, soup, asset):
         return soup.find("div", {"data-asset-id": asset.id})
 
-    def details_route_access_test(self):
-        # own bookmark
-        bookmark = self.setup_bookmark()
-        response = self.client.get(
-            reverse("linkding:bookmarks.index") + f"?details={bookmark.id}"
-        )
-        self.assertEqual(response.status_code, 200)
-
-        # other user's bookmark
-        other_user = self.setup_user()
-        bookmark = self.setup_bookmark(user=other_user)
-        response = self.client.get(
-            reverse("linkding:bookmarks.index") + f"?details={bookmark.id}"
-        )
-        self.assertEqual(response.status_code, 404)
-
-        # non-existent bookmark - just returns without modal in response
-        response = self.client.get(
-            reverse("linkding:bookmarks.index") + "?details=9999"
-        )
-        self.assertEqual(response.status_code, 200)
-
-        # guest user
-        self.client.logout()
-        response = self.client.get(
-            reverse("linkding:bookmarks.shared") + f"?details={bookmark.id}"
-        )
-        self.assertEqual(response.status_code, 404)
-
     def test_access(self):
         # own bookmark
         bookmark = self.setup_bookmark()
@@ -89,6 +62,7 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin
             reverse("linkding:bookmarks.index") + f"?details={bookmark.id}"
         )
         self.assertEqual(response.status_code, 200)
+        self.assertTrue(self.has_details_modal(response))
 
         # other user's bookmark
         other_user = self.setup_user()
@@ -96,20 +70,23 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin
         response = self.client.get(
             reverse("linkding:bookmarks.index") + f"?details={bookmark.id}"
         )
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 200)
+        self.assertFalse(self.has_details_modal(response))
 
         # non-existent bookmark - just returns without modal in response
         response = self.client.get(
             reverse("linkding:bookmarks.index") + "?details=9999"
         )
         self.assertEqual(response.status_code, 200)
+        self.assertFalse(self.has_details_modal(response))
 
         # guest user
         self.client.logout()
         response = self.client.get(
             reverse("linkding:bookmarks.shared") + f"?details={bookmark.id}"
         )
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 200)
+        self.assertFalse(self.has_details_modal(response))
 
     def test_access_with_sharing(self):
         # shared bookmark, sharing disabled
@@ -119,7 +96,8 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin
         response = self.client.get(
             reverse("linkding:bookmarks.shared") + f"?details={bookmark.id}"
         )
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 200)
+        self.assertFalse(self.has_details_modal(response))
 
         # shared bookmark, sharing enabled
         profile = other_user.profile
@@ -130,13 +108,15 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin
             reverse("linkding:bookmarks.shared") + f"?details={bookmark.id}"
         )
         self.assertEqual(response.status_code, 200)
+        self.assertTrue(self.has_details_modal(response))
 
         # shared bookmark, guest user, no public sharing
         self.client.logout()
         response = self.client.get(
             reverse("linkding:bookmarks.shared") + f"?details={bookmark.id}"
         )
-        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.status_code, 200)
+        self.assertFalse(self.has_details_modal(response))
 
         # shared bookmark, guest user, public sharing
         profile.enable_public_sharing = True
@@ -146,6 +126,7 @@ class BookmarkDetailsModalTestCase(TestCase, BookmarkFactoryMixin, HtmlTestMixin
             reverse("linkding:bookmarks.shared") + f"?details={bookmark.id}"
         )
         self.assertEqual(response.status_code, 200)
+        self.assertTrue(self.has_details_modal(response))
 
     def test_displays_title(self):
         # with title

bookmarks/views/access.py

@@ -0,0 +1,56 @@
+from django.http import Http404
+
+from bookmarks.models import Bookmark, BookmarkAsset, Toast
+from bookmarks.type_defs import HttpRequest
+
+
+def bookmark_read(request: HttpRequest, bookmark_id: int | str):
+    try:
+        bookmark = Bookmark.objects.get(pk=int(bookmark_id))
+    except Bookmark.DoesNotExist:
+        raise Http404("Bookmark does not exist")
+
+    is_owner = bookmark.owner == request.user
+    is_shared = (
+        request.user.is_authenticated
+        and bookmark.shared
+        and bookmark.owner.profile.enable_sharing
+    )
+    is_public_shared = bookmark.shared and bookmark.owner.profile.enable_public_sharing
+    if not is_owner and not is_shared and not is_public_shared:
+        raise Http404("Bookmark does not exist")
+    if request.method == "POST" and not is_owner:
+        raise Http404("Bookmark does not exist")
+
+    return bookmark
+
+
+def bookmark_write(request: HttpRequest, bookmark_id: int | str):
+    try:
+        return Bookmark.objects.get(pk=bookmark_id, owner=request.user)
+    except Bookmark.DoesNotExist:
+        raise Http404("Bookmark does not exist")
+
+
+def asset_read(request: HttpRequest, asset_id: int | str):
+    try:
+        asset = BookmarkAsset.objects.get(pk=asset_id)
+    except BookmarkAsset.DoesNotExist:
+        raise Http404("Asset does not exist")
+
+    bookmark_read(request, asset.bookmark_id)
+    return asset
+
+
+def asset_write(request: HttpRequest, asset_id: int | str):
+    try:
+        return BookmarkAsset.objects.get(pk=asset_id, bookmark__owner=request.user)
+    except BookmarkAsset.DoesNotExist:
+        raise Http404("Asset does not exist")
+
+
+def toast_write(request: HttpRequest, toast_id: int | str):
+    try:
+        return Toast.objects.get(pk=toast_id, owner=request.user)
+    except Toast.DoesNotExist:
+        raise Http404("Toast does not exist")

bookmarks/views/assets.py

@@ -8,28 +8,7 @@ from django.http import (
 )
 from django.shortcuts import render
 
-from bookmarks.models import BookmarkAsset
+from bookmarks.views import access
-
-
-def _access_asset(request, asset_id: int):
-    try:
-        asset = BookmarkAsset.objects.get(pk=asset_id)
-    except BookmarkAsset.DoesNotExist:
-        raise Http404("Asset does not exist")
-
-    bookmark = asset.bookmark
-    is_owner = bookmark.owner == request.user
-    is_shared = (
-        request.user.is_authenticated
-        and bookmark.shared
-        and bookmark.owner.profile.enable_sharing
-    )
-    is_public_shared = bookmark.shared and bookmark.owner.profile.enable_public_sharing
-
-    if not is_owner and not is_shared and not is_public_shared:
-        raise Http404("Bookmark does not exist")
-
-    return asset
 
 
 def _get_asset_content(asset):
@@ -49,14 +28,14 @@ def _get_asset_content(asset):
 
 
 def view(request, asset_id: int):
-    asset = _access_asset(request, asset_id)
+    asset = access.asset_read(request, asset_id)
     content = _get_asset_content(asset)
 
     return HttpResponse(content, content_type=asset.content_type)
 
 
 def read(request, asset_id: int):
-    asset = _access_asset(request, asset_id)
+    asset = access.asset_read(request, asset_id)
     content = _get_asset_content(asset)
     content = content.decode("utf-8")
 

bookmarks/views/bookmarks.py

@@ -5,7 +5,6 @@ from django.contrib.auth.decorators import login_required
 from django.db.models import QuerySet
 from django.http import (
     HttpResponseRedirect,
-    Http404,
     HttpResponseBadRequest,
     HttpResponseForbidden,
 )
@@ -15,7 +14,6 @@ from django.urls import reverse
 from bookmarks import queries, utils
 from bookmarks.models import (
     Bookmark,
-    BookmarkAsset,
     BookmarkForm,
     BookmarkSearch,
     build_tag_string,
@@ -38,7 +36,7 @@ from bookmarks.services.bookmarks import (
 )
 from bookmarks.type_defs import HttpRequest
 from bookmarks.utils import get_safe_return_url
-from bookmarks.views import contexts, partials, turbo
+from bookmarks.views import access, contexts, partials, turbo
 
 
 @login_required
@@ -190,10 +188,7 @@ def new(request: HttpRequest):
 
 @login_required
 def edit(request: HttpRequest, bookmark_id: int):
-    try:
+    bookmark = access.bookmark_write(request, bookmark_id)
-        bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
-    except Bookmark.DoesNotExist:
-        raise Http404("Bookmark does not exist")
     return_url = get_safe_return_url(
         request.GET.get("return_url"), reverse("linkding:bookmarks.index")
     )
@@ -216,58 +211,34 @@ def edit(request: HttpRequest, bookmark_id: int):
 
 
 def remove(request: HttpRequest, bookmark_id: int | str):
-    try:
+    bookmark = access.bookmark_write(request, bookmark_id)
-        bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
-    except Bookmark.DoesNotExist:
-        raise Http404("Bookmark does not exist")
-
     bookmark.delete()
 
 
 def archive(request: HttpRequest, bookmark_id: int | str):
-    try:
+    bookmark = access.bookmark_write(request, bookmark_id)
-        bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
-    except Bookmark.DoesNotExist:
-        raise Http404("Bookmark does not exist")
-
     archive_bookmark(bookmark)
 
 
 def unarchive(request: HttpRequest, bookmark_id: int | str):
-    try:
+    bookmark = access.bookmark_write(request, bookmark_id)
-        bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
-    except Bookmark.DoesNotExist:
-        raise Http404("Bookmark does not exist")
-
     unarchive_bookmark(bookmark)
 
 
 def unshare(request: HttpRequest, bookmark_id: int | str):
-    try:
+    bookmark = access.bookmark_write(request, bookmark_id)
-        bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
-    except Bookmark.DoesNotExist:
-        raise Http404("Bookmark does not exist")
-
     bookmark.shared = False
     bookmark.save()
 
 
 def mark_as_read(request: HttpRequest, bookmark_id: int | str):
-    try:
+    bookmark = access.bookmark_write(request, bookmark_id)
-        bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
-    except Bookmark.DoesNotExist:
-        raise Http404("Bookmark does not exist")
-
     bookmark.unread = False
     bookmark.save()
 
 
 def create_html_snapshot(request: HttpRequest, bookmark_id: int | str):
-    try:
+    bookmark = access.bookmark_write(request, bookmark_id)
-        bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
-    except Bookmark.DoesNotExist:
-        raise Http404("Bookmark does not exist")
-
     tasks.create_html_snapshot(bookmark)
 
 
@@ -275,11 +246,7 @@ def upload_asset(request: HttpRequest, bookmark_id: int | str):
     if settings.LD_DISABLE_ASSET_UPLOAD:
         return HttpResponseForbidden("Asset upload is disabled")
 
-    try:
+    bookmark = access.bookmark_write(request, bookmark_id)
-        bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
-    except Bookmark.DoesNotExist:
-        raise Http404("Bookmark does not exist")
-
     file = request.FILES.get("upload_asset_file")
     if not file:
         return HttpResponseBadRequest("No file provided")
@@ -288,20 +255,12 @@ def upload_asset(request: HttpRequest, bookmark_id: int | str):
 
 
 def remove_asset(request: HttpRequest, asset_id: int | str):
-    try:
+    asset = access.asset_write(request, asset_id)
-        asset = BookmarkAsset.objects.get(pk=asset_id, bookmark__owner=request.user)
-    except BookmarkAsset.DoesNotExist:
-        raise Http404("Asset does not exist")
-
     asset.delete()
 
 
 def update_state(request: HttpRequest, bookmark_id: int | str):
-    try:
+    bookmark = access.bookmark_write(request, bookmark_id)
-        bookmark = Bookmark.objects.get(pk=bookmark_id, owner=request.user)
-    except Bookmark.DoesNotExist:
-        raise Http404("Bookmark does not exist")
-
     bookmark.is_archived = request.POST.get("is_archived") == "on"
     bookmark.unread = request.POST.get("unread") == "on"
     bookmark.shared = request.POST.get("shared") == "on"

bookmarks/views/contexts.py

@@ -20,6 +20,7 @@ from bookmarks.models import (
 )
 from bookmarks.services.wayback import generate_fallback_webarchive_url
 from bookmarks.type_defs import HttpRequest
+from bookmarks.views import access
 
 CJK_RE = re.compile(r"[\u4e00-\u9fff]+")
 
@@ -444,22 +445,10 @@ def get_details_context(
         return None
 
     try:
-        bookmark = Bookmark.objects.get(pk=int(bookmark_id))
+        bookmark = access.bookmark_read(request, bookmark_id)
-    except Bookmark.DoesNotExist:
+    except Http404:
         # just ignore, might end up in a situation where the bookmark was deleted
         # in between navigating back and forth
         return None
 
-    is_owner = bookmark.owner == request.user
-    is_shared = (
-        request.user.is_authenticated
-        and bookmark.shared
-        and bookmark.owner.profile.enable_sharing
-    )
-    is_public_shared = bookmark.shared and bookmark.owner.profile.enable_public_sharing
-    if not is_owner and not is_shared and not is_public_shared:
-        raise Http404("Bookmark does not exist")
-    if request.method == "POST" and not is_owner:
-        raise Http404("Bookmark does not exist")
-
     return context_type(request, bookmark)

bookmarks/views/toasts.py

@@ -1,18 +1,14 @@
 from django.contrib.auth.decorators import login_required
-from django.http import HttpResponseRedirect, Http404
+from django.http import HttpResponseRedirect
 from django.urls import reverse
 
-from bookmarks.models import Toast
 from bookmarks.utils import get_safe_return_url
+from bookmarks.views import access
 
 
 @login_required
 def acknowledge(request):
-    toast_id = request.POST["toast"]
+    toast = access.toast_write(request, request.POST["toast"])
-    try:
-        toast = Toast.objects.get(pk=toast_id, owner=request.user)
-    except Toast.DoesNotExist:
-        raise Http404("Toast does not exist")
     toast.acknowledged = True
     toast.save()