Refactor to use django-crossdomainmedia

docs/productionsetup.rst

@@ -39,8 +39,7 @@ Acces to Documents
 Nginx is able to serve your uploads behind authentication/authorization. Activate the following settings::
 
   # Use nginx to serve uploads authenticated
-  USE_X_ACCEL_REDIRECT = True
-  X_ACCEL_REDIRECT_PREFIX = '/protected'
+  INTERNAL_MEDIA_PREFIX = '/protected/'
 
 Nginx will forward the request to Froide which will in turn check for authentication and authorization. If everything is good Froide replies to Nginx with an internal redirect and Nginx will then serve the file to the user.
 

froide/foirequest/api_views.py

@@ -130,7 +130,7 @@ class Meta:
         )
 
     def get_file_url(self, obj):
-        return obj.get_absolute_domain_file_url(authenticated=True)
+        return obj.get_absolute_domain_file_url(authorized=True)
 
     def get_pending(self, obj):
         return obj.pending

froide/foirequest/auth.py

@@ -1,6 +1,10 @@
 from functools import lru_cache
 
 from django.utils.crypto import salted_hmac, constant_time_compare
+from django.urls import reverse
+from django.conf import settings
+
+from crossdomainmedia import CrossDomainMediaAuth
 
 from froide.helper.auth import (
     can_read_object, can_write_object,
@@ -93,3 +97,68 @@ def clear_lru_caches():
     for f in (can_write_foirequest, can_read_foirequest,
               can_read_foirequest_authenticated):
         f.cache_clear()
+
+
+def has_attachment_access(request, foirequest, attachment):
+    if not can_read_foirequest(foirequest, request):
+        return False
+    if not attachment.approved:
+        # allow only approved attachments to be read
+        # do not allow anonymous authentication here
+        allowed = can_read_foirequest_authenticated(
+            foirequest, request, allow_code=False
+        )
+        if not allowed:
+            return False
+    return True
+
+
+def get_accessible_attachment_url(foirequest, attachment):
+    needs_authorization = not is_attachment_public(foirequest, attachment)
+    return attachment.get_absolute_domain_file_url(
+        authorized=needs_authorization
+    )
+
+
+class AttachmentCrossDomainMediaAuth(CrossDomainMediaAuth):
+    '''
+    Create your own custom CrossDomainMediaAuth class
+    and implement at least these methods
+    '''
+    TOKEN_MAX_AGE_SECONDS = settings.FOI_MEDIA_TOKEN_EXPIRY
+    SITE_URL = settings.SITE_URL
+    DEBUG = False
+
+    def is_media_public(self):
+        '''
+        Determine if the media described by self.context
+        needs authentication/authorization at all
+        '''
+        ctx = self.context
+        return is_attachment_public(ctx['foirequest'], ctx['object'])
+
+    def has_perm(self, request):
+        ctx = self.context
+        obj = ctx['object']
+        foirequest = ctx['foirequest']
+        return has_attachment_access(request, foirequest, obj)
+
+    def get_auth_url(self):
+        '''
+        Give URL path to authenticating view
+        for the media described in context
+        '''
+        obj = self.context['object']
+
+        return reverse('foirequest-auth_message_attachment',
+            kwargs={
+                'message_id': obj.belongs_to_id,
+                'attachment_name': obj.name
+            })
+
+    def get_media_file_path(self):
+        '''
+        Return the URL path relative to MEDIA_ROOT for debug mode
+        '''
+        obj = self.context['object']
+        return obj.file.name

froide/foirequest/models/attachment.py

@@ -5,9 +5,6 @@
 from django.dispatch import Signal
 from django.urls import reverse
 from django.utils.translation import ugettext_lazy as _
-from django.core.signing import (
-    TimestampSigner, SignatureExpired, BadSignature
-)
 from django.utils import timezone
 
 from froide.helper.redaction import can_redact_file
@@ -31,6 +28,14 @@ def upload_to(instance, filename):
     return "%s/%s" % (settings.FOI_MEDIA_PATH, instance.name)
 
 
+class FoiAttachmentManager(models.Manager):
+    def get_for_message(self, message, name):
+        return FoiAttachment.objects.filter(
+            belongs_to=message,
+            name=name
+        ).exclude(file='').get()
+
+
 class FoiAttachment(models.Model):
     belongs_to = models.ForeignKey(
         FoiMessage, null=True,
@@ -65,6 +70,8 @@ class FoiAttachment(models.Model):
         on_delete=models.SET_NULL
     )
 
+    objects = FoiAttachmentManager()
+
     attachment_published = Signal(providing_args=[])
 
     class Meta:
@@ -83,9 +90,6 @@ def index_content(self):
     def get_html_id(self):
         return _("attachment-%(id)d") % {"id": self.id}
 
-    def get_internal_url(self):
-        return settings.FOI_MEDIA_URL + self.file.name
-
     def get_bytes(self):
         self.file.open(mode='rb')
         try:
@@ -169,53 +173,29 @@ def get_absolute_url(self):
             }
         )
 
-    def get_absolute_domain_url(self):
-        return '%s%s' % (settings.SITE_URL, self.get_absolute_url())
+    def get_crossdomain_auth(self):
+        from ..auth import AttachmentCrossDomainMediaAuth
 
-    def get_absolute_file_url(self, authenticated=False):
-        if not self.name:
-            return ''
-        url = reverse('foirequest-auth_message_attachment',
-            kwargs={
-                'message_id': self.belongs_to_id,
-                'attachment_name': self.name
-            })
-        if settings.FOI_MEDIA_TOKENS and authenticated:
-            signer = TimestampSigner()
-            value = signer.sign(url).split(':', 1)[1]
-            return '%s?token=%s' % (url, value)
-        return url
+        return AttachmentCrossDomainMediaAuth({
+            'object': self,
+        })
 
-    def get_file_url(self):
-        return self.get_absolute_domain_file_url()
+    def send_internal_file(self):
+        return self.get_crossdomain_auth().send_internal_file()
 
-    def get_file_path(self):
-        if self.file:
-            return self.file.path
-        return ''
+    def get_absolute_domain_url(self):
+        return '%s%s' % (settings.SITE_URL, self.get_absolute_url())
 
-    def get_authenticated_absolute_domain_file_url(self):
-        return self.get_absolute_domain_file_url(authenticated=True)
+    def get_absolute_file_url(self):
+        return self.get_crossdomain_auth().get_auth_url()
 
-    def get_absolute_domain_file_url(self, authenticated=False):
-        return '%s%s' % (
-            settings.FOI_MEDIA_DOMAIN,
-            self.get_absolute_file_url(authenticated=authenticated)
-        )
+    def get_authorized_absolute_domain_file_url(self):
+        return self.get_absolute_domain_file_url(authorized=True)
 
-    def check_token(self, request):
-        token = request.GET.get('token')
-        if token is None:
-            return None
-        original = '%s:%s' % (self.get_absolute_file_url(), token)
-        signer = TimestampSigner()
-        try:
-            signer.unsign(original, max_age=settings.FOI_MEDIA_TOKEN_EXPIRY)
-        except SignatureExpired:
-            return None
-        except BadSignature:
-            return False
-        return True
+    def get_absolute_domain_file_url(self, authorized=False):
+        return self.get_crossdomain_auth().get_full_media_url(
+            authorized=authorized
+        )
 
     def approve_and_save(self):
         self.approved = True

froide/foirequest/templates/foirequest/snippets/attachment_approved.html

@@ -24,7 +24,7 @@
       </span>
     {% endif %}
     {% if attachment.is_mail_decoration %}
-      <span data-toggle="tooltip" data-html="true" title="<h6>{{ attachment.name }}</h6> <img src='{{ attachment.get_authenticated_absolute_domain_file_url }}' alt='{{ attachment.name }}'/>">
+      <span data-toggle="tooltip" data-html="true" title="<h6>{{ attachment.name }}</h6> <img src='{{ attachment.get_authorized_absolute_domain_file_url }}' alt='{{ attachment.name }}'/>">
         <i class="fa fa-picture-o" aria-hidden="true"></i>
       </span>
     {% endif %}

froide/foirequest/templates/foirequest/snippets/attachment_unapproved.html

@@ -32,7 +32,7 @@
     {% blocktrans %}Not public!{% endblocktrans %}
   </span>
   {% if attachment.is_mail_decoration and object|can_write_foirequest:request %}
-    <span data-toggle="tooltip" data-html="true" title="<h6>{{ attachment.name }}</h6> <img src='{{ attachment.get_authenticated_absolute_domain_file_url }}' alt='{{ attachment.name }}'/>">
+    <span data-toggle="tooltip" data-html="true" title="<h6>{{ attachment.name }}</h6> <img src='{{ attachment.get_authorized_absolute_domain_file_url }}' alt='{{ attachment.name }}'/>">
       <i class="fa fa-picture-o" aria-hidden="true"></i>
     </span>
   {% endif %}

froide/foirequest/tests/test_web.py

@@ -1,5 +1,7 @@
 import unittest
 
+from mock import patch
+
 from django.test import TestCase
 from django.urls import reverse
 from django.conf import settings
@@ -299,34 +301,51 @@ def test_search(self):
         self.assertIn(reverse('foirequest-list'), response['Location'])
 
 
+MEDIA_DOMAIN = 'media.frag-den-staat.de'
+
+
 class MediaServingTest(TestCaseHelpers, TestCase):
     def setUp(self):
         clear_lru_caches()
         self.site = factories.make_world()
 
     @override_settings(
-        USE_X_ACCEL_REDIRECT=True
+        SITE_URL='https://fragdenstaat.de',
+        MEDIA_URL='https://' + MEDIA_DOMAIN + '/files/',
+        ALLOWED_HOSTS=('fragdenstaat.de', MEDIA_DOMAIN),
     )
+    @patch('froide.foirequest.auth.AttachmentCrossDomainMediaAuth.SITE_URL',
+           'https://fragdenstaat.de')
     def test_request_not_public(self):
         att = FoiAttachment.objects.filter(approved=True)[0]
         req = att.belongs_to.request
         req.visibility = 1
         req.save()
-        response = self.client.get(att.get_absolute_file_url())
+        response = self.client.get(
+            att.get_absolute_file_url(),
+            HTTP_HOST='fragdenstaat.de'
+        )
         self.assertForbidden(response)
         self.client.login(email='[email protected]', password='froide')
-        response = self.client.get(att.get_absolute_file_url())
+        response = self.client.get(
+            att.get_absolute_file_url(),
+            HTTP_HOST='fragdenstaat.de'
+        )
+        self.assertEqual(response.status_code, 302)
+        self.assertIn(MEDIA_DOMAIN, response['Location'])
+        response = self.client.get(
+            response['Location'],
+            HTTP_HOST=MEDIA_DOMAIN
+        )
         self.assertEqual(response.status_code, 200)
         self.assertIn('X-Accel-Redirect', response)
         self.assertEqual(response['X-Accel-Redirect'], '%s%s' % (
-            settings.X_ACCEL_REDIRECT_PREFIX, att.file.url))
+            settings.INTERNAL_MEDIA_PREFIX, att.file.name))
 
     @override_settings(
-        USE_X_ACCEL_REDIRECT=True,
-        FOI_MEDIA_TOKENS=True,
         SITE_URL='https://fragdenstaat.de',
-        FOI_MEDIA_DOMAIN='https://media.frag-den-staat.de',
-        ALLOWED_HOSTS=('fragdenstaat.de', 'media.frag-den-staat.de')
+        MEDIA_URL='https://' + MEDIA_DOMAIN + '/files/',
+        ALLOWED_HOSTS=('fragdenstaat.de', MEDIA_DOMAIN)
     )
     def test_request_media_tokens(self):
         att = FoiAttachment.objects.filter(approved=True)[0]
@@ -354,24 +373,25 @@ def test_request_media_tokens(self):
             HTTP_HOST=domain,
         )
         self.assertEqual(response.status_code, 403)
-
         response = self.client.get(
             '/' + path,
             follow=False,
             HTTP_HOST=domain,
         )
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response['X-Accel-Redirect'], '%s%s' % (
-            settings.X_ACCEL_REDIRECT_PREFIX, att.file.url))
+            settings.INTERNAL_MEDIA_PREFIX, att.file.name))
 
     @override_settings(
-        USE_X_ACCEL_REDIRECT=True,
-        FOI_MEDIA_TOKENS=True,
         SITE_URL='https://fragdenstaat.de',
-        FOI_MEDIA_DOMAIN='https://media.frag-den-staat.de',
-        ALLOWED_HOSTS=('fragdenstaat.de', 'media.frag-den-staat.de'),
+        MEDIA_URL='https://' + MEDIA_DOMAIN + '/files/',
+        ALLOWED_HOSTS=('fragdenstaat.de', MEDIA_DOMAIN),
         FOI_MEDIA_TOKEN_EXPIRY=0
     )
+    @patch('froide.foirequest.auth.AttachmentCrossDomainMediaAuth.TOKEN_MAX_AGE_SECONDS',
+           0)
+    @patch('froide.foirequest.auth.AttachmentCrossDomainMediaAuth.SITE_URL',
+           'https://fragdenstaat.de')
     def test_request_media_tokens_expired(self):
         att = FoiAttachment.objects.filter(approved=True)[0]
         req = att.belongs_to.request
@@ -416,6 +436,13 @@ def test_request_public(self):
         response = self.client.get(att.get_absolute_url() + 'a')
         self.assertEqual(response.status_code, 404)
 
+    def test_attachment_pending(self):
+        att = FoiAttachment.objects.filter(approved=True)[0]
+        att.file = ''
+        att.save()
+        response = self.client.get(att.get_absolute_file_url())
+        self.assertEqual(response.status_code, 404)
+
 
 class PerformanceTest(TestCase):
     def setUp(self):

froide/foirequest/urls/__init__.py

@@ -3,7 +3,7 @@
 from django.utils.translation import pgettext_lazy
 
 from ..views import (
-    search, auth, shortlink, auth_message_attachment,
+    search, auth, shortlink, AttachmentFileDetailView,
     project_shortlink
 )
 
@@ -42,8 +42,16 @@
         auth, name="foirequest-auth"),
 ]
 
+MEDIA_PATH = settings.MEDIA_URL
+# Split off domain and leading slash
+if MEDIA_PATH.startswith('http'):
+    MEDIA_PATH = MEDIA_PATH.split('/', 3)[-1]
+else:
+    MEDIA_PATH = MEDIA_PATH[1:]
+
+
 urlpatterns += [
     url(r'^%s%s/(?P<message_id>\d+)/(?P<attachment_name>.+)' % (
-        settings.FOI_MEDIA_URL[1:], settings.FOI_MEDIA_PATH
-    ), auth_message_attachment, name='foirequest-auth_message_attachment')
+        MEDIA_PATH, settings.FOI_MEDIA_PATH
+    ), AttachmentFileDetailView.as_view(), name='foirequest-auth_message_attachment')
 ]

froide/foirequest/views/__init__.py

@@ -5,7 +5,7 @@
 )
 from .attachment import (
     show_attachment, delete_attachment,
-    approve_attachment, auth_message_attachment, redact_attachment
+    approve_attachment, AttachmentFileDetailView, redact_attachment
 )
 from .draft import delete_draft, claim_draft
 from .list_requests import (
@@ -40,7 +40,7 @@
     MyRequestsView, DraftRequestsView, FollowingRequestsView,
     FoiProjectListView, RequestSubscriptionsView, user_calendar,
     show_attachment, delete_attachment,
-    approve_attachment, auth_message_attachment, redact_attachment,
+    approve_attachment, AttachmentFileDetailView, redact_attachment,
     delete_draft, claim_draft,
     ListRequestView, search, list_unchecked, UserRequestFeedView,
     MakeRequestView, DraftRequestView, RequestSentView,