Cleaned up photos views code

website/photos/models.py

@@ -65,7 +65,7 @@ class Photo(models.Model):
             self._orig_file = ""
 
     def __str__(self):
-        return self.file.name
+        return os.path.basename(self.file.name)
 
     def save(self, *args, **kwargs):
         super().save(*args, **kwargs)

website/photos/services.py

-from django.db.models import When, Value, BooleanField, ExpressionWrapper, Q, \
-    Case
+from django.db.models import (When, Value, BooleanField, ExpressionWrapper, Q,
+                              Case)
+from django.http import Http404
 
 from PIL.JpegImagePlugin import JpegImageFile
 from PIL import ExifTags
@@ -28,6 +29,11 @@ def photo_determine_rotation(pil_image):
     return 0
 
 
+def check_shared_album_token(album, token):
+    if token != album.access_token:
+        raise Http404("Invalid token.")
+
+
 def is_album_accessible(request, album):
     if request.member and request.member.current_membership is not None:
         return True

website/photos/templates/photos/album.html

@@ -11,19 +11,25 @@
         <div class="container">
             <h1 class="text-center section-title">
                 {{ album.title }}
-                <a href="{% url 'photos:album-download' album.slug %}"
+                <a 
+                   {% if album.shareable %}
+                   href="{% url 'photos:shared-album-download' album.slug album.access_token %}"
+                   {% else %}
+                   href="{% url 'photos:album-download' album.slug %}"
+                   {% endif %}
                    target="_blank"
                    class="btn btn-primary btn-first"
                 >
                     <i class="fas fa-download"></i>
                 </a>
             </h1>
+
             <h2 class="text-center mt-2">{{ album.date|date:"d-m-Y" }}</h2>
             {% if album.shareable %}
                 <p class="text-center">
                     {% trans "Note: This album can be shared with people outside the association by sending them the following link:" %}<br>
-                    <small><a href="{% url 'photos:shared_album' album.slug album.access_token %}">
-                        {{ request.get_host }}{% url 'photos:shared_album' album.slug album.access_token %}
+                    <small><a href="{% url 'photos:shared-album' album.slug album.access_token %}">
+                        {{ request.get_host }}{% url 'photos:shared-album' album.slug album.access_token %}
                     </a></small>
                 </p>
             {% endif %}

website/photos/templatetags/photos_cards.py

@@ -45,7 +45,7 @@ def photo_card(photo):
                     args=[photo.album.slug, photo.album.access_token, photo]))
     else:
         anchor_attrs += ' data-download={}'.format(
-            reverse('photos:download', args=[photo]))
+            reverse('photos:download', args=[photo.album.slug, photo]))
 
     image_url = thumbnail(photo.file, settings.THUMBNAIL_SIZES['medium'])
     if photo.album.shareable:

website/photos/templatetags/shared_thumbnail.py

 from django import template
 from django.urls import resolve, reverse
 
+from os.path import basename
+
 from utils.templatetags.thumbnail import thumbnail
 
 register = template.Library()
@@ -9,5 +11,6 @@ register = template.Library()
 @register.simple_tag
 def shared_thumbnail(slug, token, path, size, fit=True):
     thumb = resolve(thumbnail(path, size, fit))
-    args = [slug, token, thumb.kwargs['size_fit'], thumb.kwargs['path']]
+    filename = basename(thumb.kwargs['path'])
+    args = [slug, thumb.kwargs['size_fit'], token, filename]
     return reverse('photos:shared-thumbnail', args=args)

website/photos/tests/test_views.py

@@ -192,7 +192,7 @@ class SharedAlbumTest(TestCase):
                 photo.save()
 
         response = self.client.get(reverse(
-            'photos:shared_album',
+            'photos:shared-album',
             args=(self.album.slug, self.album.access_token,)))
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.context['album'], self.album)
@@ -229,13 +229,13 @@ class DownloadTest(TestCase):
         self.client.force_login(self.member)
 
         response = self.client.get(reverse(
-            'photos:download', args=(self.photo,)))
+            'photos:download', args=(self.album.slug, self.photo,)))
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response['Content-Type'], 'image/jpeg')
 
     def test_logged_out(self):
         response = self.client.get(reverse(
-            'photos:download', args=(self.photo,)))
+            'photos:download', args=(self.album.slug, self.photo,)))
         self.assertEqual(response.status_code, 302)
 
 
@@ -269,7 +269,8 @@ class SharedDownloadTest(TestCase):
         with self.subTest():
             response = self.client.get(reverse(
                 'photos:shared-download',
-                args=(self.album.slug, self.album.access_token, self.photo,)))
+                args=(self.album.slug, self.album.access_token,
+                      self.photo,)))
             self.assertEqual(response.status_code, 200)
             self.assertEqual(response['Content-Type'], 'image/jpeg')
 
@@ -278,7 +279,8 @@ class SharedDownloadTest(TestCase):
         with self.subTest():
             response = self.client.get(reverse(
                 'photos:shared-download',
-                args=(self.album.slug, self.album.access_token, self.photo,)))
+                args=(self.album.slug, self.album.access_token,
+                      self.photo,)))
             self.assertEqual(response.status_code, 200)
             self.assertEqual(response['Content-Type'], 'image/jpeg')
 

website/photos/urls.py

-from django.conf.urls import url
+from django.urls import path, include, re_path
 
 from . import views
 
 app_name = "photos"
 
 urlpatterns = [
-    url(r'^download/(?P<path>.*)', views.download, name='download'),
-    url(r'^shared-download/(?P<slug>[-\w]+)/(?P<token>[a-zA-Z0-9]+)/(?P<path>.*)', views.shared_download, name='shared-download'),
-    url(r'^shared-thumbnail/(?P<slug>[-\w]+)/(?P<token>[a-zA-Z0-9]+)/(?P<size_fit>\d+x\d+_[01])/(?P<path>.*)', views.shared_thumbnail, name='shared-thumbnail'),
-    url(r'^(?P<slug>[-\w]+)/$', views.album, name='album'),
-    url(r'^(?P<slug>[-\w]+)/download$', views.album_download, name='album-download'),
-    url(r'^(?P<slug>[-\w]+)/(?P<token>[a-zA-Z0-9]+)$', views.shared_album, name='shared_album'),
-    url(r'^$', views.index, name='index'),
+    path('', views.index, name='index'),
+    path('<slug>/', include([
+        path('', views.album, name='album'),
+        path('download/', include([
+            path('', views.album_download, name='album-download'),
+            path('<filename>', views.download, name='download'),
+            path('<token>/', include([
+                path('', views.shared_album_download, name='shared-album-download'),
+                path('<filename>', views.shared_download, name='shared-download'),
+            ])),
+        ])),
+        path('thumbnail/', include([
+            re_path(r'(?P<size_fit>\d+x\d+_[01])/', include([
+                path('<token>/<filename>/', views.shared_thumbnail, name='shared-thumbnail'),
+            ])),
+        ])),
+        path('<token>/', views.shared_album, name='shared-album'),
+    ])),
 ]

website/photos/views.py

@@ -2,17 +2,17 @@ import os
 from tempfile import gettempdir
 from zipfile import ZipFile
 
-from django.conf import settings
 from django.contrib.auth.decorators import login_required
-from django.core.exceptions import SuspiciousFileOperation
 from django.core.paginator import EmptyPage, Paginator
 from django.http import Http404
 from django.shortcuts import get_object_or_404, render
 from sendfile import sendfile
 
-from photos import services
+from photos.models import Album, Photo
+from photos.services import (check_shared_album_token,
+                             get_annotated_accessible_albums,
+                             is_album_accessible)
 from utils.views import _private_thumbnails_unauthed
-from .models import Album
 
 COVER_FILENAME = 'cover.jpg'
 
@@ -22,7 +22,7 @@ def index(request):
     # Only show published albums
     albums = Album.objects.filter(hidden=False)
 
-    albums = services.get_annotated_accessible_albums(request, albums)
+    albums = get_annotated_accessible_albums(request, albums)
 
     albums = albums.order_by('-date')
     paginator = Paginator(albums, 12)
@@ -65,78 +65,77 @@ def _render_album_page(request, album):
 @login_required
 def album(request, slug):
     album = get_object_or_404(Album, slug=slug)
-    if services.is_album_accessible(request, album):
+    if is_album_accessible(request, album):
         return _render_album_page(request, album)
     raise Http404("Sorry, you're not allowed to view this album")
 
 
-def _checked_shared_album(slug, token):
-    album = get_object_or_404(Album, slug=slug)
-    if token != album.access_token:
-        raise Http404("Invalid token.")
-    return album
-
-
 def shared_album(request, slug, token):
-    album = _checked_shared_album(slug, token)
+    album = get_object_or_404(Album, slug=slug)
+    check_shared_album_token(album, token)
     return _render_album_page(request, album)
 
 
-def _download(request, original_path):
-    """This function provides a layer of indirection for shared albums
-
-    Checks for some path traversal:
+def _photo_path(album, filename):
+    photoname = os.path.basename(filename)
+    albumpath = os.path.join(album.photosdir, album.dirname)
+    photopath = os.path.join(albumpath, photoname)
+    get_object_or_404(Photo.objects.filter(album=album, file=photopath))
+    return photopath
 
-    >>> from django.test import RequestFactory
-    >>> r = RequestFactory().get('/photos/download/../../../../../etc/passwd')
-    >>> _download(r, '../../../../../../../etc/passwd')  #doctest: +ELLIPSIS
-    Traceback (most recent call last):
-        ...
-    django.core.exceptions.SuspiciousFileOperation: ...
-    """
-    photopath = os.path.join(settings.MEDIA_ROOT, 'photos')
 
-    path = os.path.normpath(
-        os.path.join(photopath, *original_path.split('/')[1:]))
-
-    if not os.path.commonpath([photopath, path]) == photopath:
-        raise SuspiciousFileOperation(
-            "Path traversal detected: someone tried to download "
-            "{}, input: {}".format(path, original_path))
-    if not os.path.isfile(path):
-        raise Http404("Photo not found.")
-    return sendfile(request, path, attachment=True)
+def _download(request, album, filename):
+    """This function provides a layer of indirection for shared albums"""
+    photopath = _photo_path(album, filename)
+    photo = get_object_or_404(
+            Photo.objects.filter(album=album, file=photopath))
+    return sendfile(request, photo.file.path, attachment=True)
 
 
-def _album_download(request, slug):
+def _album_download(request, album):
     """This function provides a layer of indirection for shared albums"""
-    album = get_object_or_404(Album, slug=slug)
     albumpath = os.path.join(album.photospath, album.dirname)
-    pictures = [os.path.join(albumpath, x) for x in os.listdir(albumpath)]
     zipfilename = os.path.join(gettempdir(),
                                '{}.zip'.format(album.dirname))
     if not os.path.exists(zipfilename):
         with ZipFile(zipfilename, 'w') as f:
+            pictures = [os.path.join(albumpath, x)
+                        for x in os.listdir(albumpath)]
             for picture in pictures:
                 f.write(picture, arcname=os.path.basename(picture))
     return sendfile(request, zipfilename, attachment=True)
 
 
 @login_required
-def download(request, path):
-    return _download(request, path)
+def download(request, slug, filename):
+    album = get_object_or_404(Album, slug=slug)
+    if is_album_accessible(request, album):
+        return _download(request, album, filename)
+    raise Http404("Sorry, you're not allowed to view this album")
 
 
 @login_required
 def album_download(request, slug):
-    return _album_download(request, slug)
+    album = get_object_or_404(Album, slug=slug)
+    if is_album_accessible(request, album):
+        return _album_download(request, album)
+    raise Http404("Sorry, you're not allowed to view this album")
+
+
+def shared_download(request, slug, token, filename):
+    album = get_object_or_404(Album, slug=slug)
+    check_shared_album_token(album, token)
+    return _download(request, album, filename)
 
 
-def shared_download(request, slug, token, path):
-    _checked_shared_album(slug, token)
-    return _download(request, path)
+def shared_album_download(request, slug, token):
+    album = get_object_or_404(Album, slug=slug)
+    check_shared_album_token(album, token)
+    return _album_download(request, album)
 
 
-def shared_thumbnail(request, slug, token, size_fit, path):
-    _checked_shared_album(slug, token)
-    return _private_thumbnails_unauthed(request, size_fit, path)
+def shared_thumbnail(request, slug, size_fit, token, filename):
+    album = get_object_or_404(Album, slug=slug)
+    check_shared_album_token(album, token)
+    photopath = _photo_path(album, filename)
+    return _private_thumbnails_unauthed(request, size_fit, photopath)