From 52f476fb349507893b793a34cd2eb3fd67539687 Mon Sep 17 00:00:00 2001 From: Anler Hp Date: Fri, 4 Jul 2014 12:15:48 +0200 Subject: [PATCH] Check permissions when accessing attachments Attachment files dispatching is now done through `RawAttachmentView` view that checks for appropiate permissions. When using the development server this view just redirects to the real media path of the file. When using the production server the special redirection header `X-Accel-Redirect` is used instead to improve efficiency by instructing the server to dispatch the file instead of django, but you also need the following configuration (Nginx): location /attachment-files { internal; alias /path/to/taiga/media/attachment-files; } It's recommended to also restrict the direct access from outside to the `attachment-files` directory by using some configuration like this: location /media/attachment-files { deny all; } --- settings/common.py | 7 ++- settings/testing.py | 2 + taiga/base/utils/urls.py | 6 +++ taiga/projects/attachments/serializers.py | 6 +-- taiga/projects/attachments/views.py | 32 +++++++++++++ taiga/urls.py | 3 ++ tests/factories.py | 10 ++++ tests/integration/test_attachments.py | 58 +++++++++++++++++++++++ 8 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 taiga/projects/attachments/views.py create mode 100644 tests/integration/test_attachments.py diff --git a/settings/common.py b/settings/common.py index 2caacaa1..c50ad278 100644 --- a/settings/common.py +++ b/settings/common.py @@ -76,7 +76,7 @@ SITES = { SITE_ID = "api" # Session configuration (only used for admin) -SESSION_ENGINE="django.contrib.sessions.backends.db" +SESSION_ENGINE = "django.contrib.sessions.backends.db" SESSION_COOKIE_AGE = 1209600 # (2 weeks) # MAIL OPTIONS @@ -325,3 +325,8 @@ GRAVATAR_DEFAULT_OPTIONS = { 'default': DEFAULT_AVATAR_URL, # default avatar to show if there's no gravatar image 'size': DEFAULT_AVATAR_SIZE } + +try: + IN_DEVELOPMENT_SERVER = sys.argv[1] == 'runserver' +except IndexError: + IN_DEVELOPMENT_SERVER = False diff --git a/settings/testing.py b/settings/testing.py index 4448be54..13fe1999 100644 --- a/settings/testing.py +++ b/settings/testing.py @@ -20,3 +20,5 @@ SKIP_SOUTH_TESTS = True SOUTH_TESTS_MIGRATE = False CELERY_ALWAYS_EAGER = True + +MEDIA_ROOT = "/tmp" diff --git a/taiga/base/utils/urls.py b/taiga/base/utils/urls.py index 2e5d71be..0b94b5aa 100644 --- a/taiga/base/utils/urls.py +++ b/taiga/base/utils/urls.py @@ -1,4 +1,5 @@ import django_sites as sites +from django.core.urlresolvers import reverse as django_reverse URL_TEMPLATE = "{scheme}://{domain}/{path}" @@ -18,3 +19,8 @@ def get_absolute_url(path): return path site = sites.get_current() return build_url(path, scheme=site.scheme, domain=site.domain) + + +def reverse(viewname, *args, **kwargs): + """Same behavior as django's reverse but uses django_sites to compute absolute url.""" + return get_absolute_url(django_reverse(viewname, *args, **kwargs)) diff --git a/taiga/projects/attachments/serializers.py b/taiga/projects/attachments/serializers.py index eb74043d..92ecdb40 100644 --- a/taiga/projects/attachments/serializers.py +++ b/taiga/projects/attachments/serializers.py @@ -13,13 +13,13 @@ # # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . +from os import path from rest_framework import serializers +from taiga.base.utils.urls import reverse from . import models -from os import path - class AttachmentSerializer(serializers.ModelSerializer): name = serializers.SerializerMethodField("get_name") @@ -39,7 +39,7 @@ class AttachmentSerializer(serializers.ModelSerializer): return "" def get_url(self, obj): - return obj.attached_file.url if obj and obj.attached_file else "" + return reverse("attachment-url", kwargs={"pk": obj.pk}) def get_size(self, obj): if obj.attached_file: diff --git a/taiga/projects/attachments/views.py b/taiga/projects/attachments/views.py new file mode 100644 index 00000000..567944ba --- /dev/null +++ b/taiga/projects/attachments/views.py @@ -0,0 +1,32 @@ +import os + +from django.conf import settings +from django import http + +from rest_framework import generics +from rest_framework.permissions import IsAuthenticated + +from . import permissions +from . import models + + +def serve_attachment(request, attachment): + if settings.IN_DEVELOPMENT_SERVER: + return http.HttpResponseRedirect(attachment.url) + + name = attachment.name + response = http.HttpResponse() + response['X-Accel-Redirect'] = "/{filepath}".format(filepath=name) + response['Content-Disposition'] = 'attachment;filename={filename}'.format( + filename=os.path.basename(name)) + + return response + + +class RawAttachmentView(generics.RetrieveAPIView): + queryset = models.Attachment.objects.all() + permission_classes = (IsAuthenticated, permissions.AttachmentPermission,) + + def retrieve(self, request, *args, **kwargs): + self.object = self.get_object() + return serve_attachment(request, self.object.attached_file) diff --git a/taiga/urls.py b/taiga/urls.py index b8ffe02f..74daefd7 100644 --- a/taiga/urls.py +++ b/taiga/urls.py @@ -20,11 +20,14 @@ from django.contrib.staticfiles.urls import staticfiles_urlpatterns from django.contrib import admin from .routers import router +from .projects.attachments.views import RawAttachmentView + admin.autodiscover() urlpatterns = patterns('', + url(r'^attachments/(?P\d+)/$', RawAttachmentView.as_view(), name="attachment-url"), url(r'^api/v1/', include(router.urls)), url(r'^api/v1/api-auth/', include('rest_framework.urls', namespace='rest_framework')), url(r'^admin/', include(admin.site.urls)), diff --git a/tests/factories.py b/tests/factories.py index c80225fd..cd1d1361 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -258,6 +258,16 @@ class ContentTypeFactory(Factory): model = factory.LazyAttribute(lambda obj: ContentTypeFactory.FACTORY_FOR._meta.model_name) +class AttachmentFactory(Factory): + FACTORY_FOR = get_model("attachments", "Attachment") + + owner = factory.SubFactory("tests.factories.UserFactory") + project = factory.SubFactory("tests.factories.ProjectFactory") + content_type = factory.SubFactory("tests.factories.ContentTypeFactory") + object_id = factory.Sequence(lambda n: n) + attached_file = factory.django.FileField(data=b"File contents") + + def create_issue(**kwargs): "Create an issue and along with its dependencies." owner = kwargs.pop("owner", UserFactory()) diff --git a/tests/integration/test_attachments.py b/tests/integration/test_attachments.py new file mode 100644 index 00000000..77ffcf69 --- /dev/null +++ b/tests/integration/test_attachments.py @@ -0,0 +1,58 @@ +import pytest + +from django.core.urlresolvers import reverse +from django.core.files.base import File + +from .. import factories as f +from ..utils import set_settings + +pytestmark = pytest.mark.django_db + +def test_authentication(client): + "User can't access an attachment if not authenticated" + attachment = f.AttachmentFactory.create() + url = reverse("attachment-url", kwargs={"pk": attachment.pk}) + + response = client.get(url) + + assert response.status_code == 401 + + +def test_authorization(client): + "User can't access an attachment if not authorized" + attachment = f.AttachmentFactory.create() + user = f.UserFactory.create() + + url = reverse("attachment-url", kwargs={"pk": attachment.pk}) + + client.login(user) + response = client.get(url) + + assert response.status_code == 403 + + +@set_settings(IN_DEVELOPMENT_SERVER=True) +def test_attachment_redirect_in_devserver(client): + "When accessing the attachment in devserver redirect to the real attachment url" + attachment = f.AttachmentFactory.create() + + url = reverse("attachment-url", kwargs={"pk": attachment.pk}) + + client.login(attachment.owner) + response = client.get(url) + + assert response.status_code == 302 + + +@set_settings(IN_DEVELOPMENT_SERVER=False) +def test_attachment_redirect(client): + "When accessing the attachment redirect using X-Accel-Redirect header" + attachment = f.AttachmentFactory.create() + + url = reverse("attachment-url", kwargs={"pk": attachment.pk}) + + client.login(attachment.owner) + response = client.get(url) + + assert response.status_code == 200 + assert response.has_header('x-accel-redirect')