From 33ef9fe30281f74d494835a494fe9ad0e3acdb1c Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Wed, 11 Jul 2018 11:44:12 +0200 Subject: [PATCH] core: fix MultipleObjectsReturned in core.views.serve.serve.docs We get this exception while trying to serve some docs: MultipleObjectsReturned: get() returned more than one Version -- it returned 2! File "django/core/handlers/base.py", line 149, in get_response response = self.process_exception_by_middleware(e, request) File "django/core/handlers/base.py", line 147, in get_response response = wrapped_callback(request, *callback_args, **callback_kwargs) File "readthedocs/core/views/serve.py", line 96, in inner_view return view_func(request, project=project, *args, **kwargs) File "readthedocs/core/views/serve.py", line 74, in inner_view return view_func(request, subproject=subproject, *args, **kwargs) File "readthedocs/core/views/serve.py", line 156, in serve_docs version = project.versions.public(request.user).get(slug=version_slug) File "django/db/models/query.py", line 391, in get (self.model._meta.object_name, num) The view code looks legit at a first look but there's a huge side effect of the user filtering in the public() method. It does not filter the projects by user but it adds to the queryset all the other user projects which is not what we need here. Instead simplify the code to: - return 404 if the requested version does not exist - return 401 if the version is private and the user is not admin - serve the file if the version is private and the user is the admin Fix #4350 --- readthedocs/core/views/serve.py | 18 +++++++------- readthedocs/rtd_tests/tests/test_redirects.py | 24 ++++++++++++++++--- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/readthedocs/core/views/serve.py b/readthedocs/core/views/serve.py index 610abce858b..08f785757c8 100644 --- a/readthedocs/core/views/serve.py +++ b/readthedocs/core/views/serve.py @@ -201,14 +201,19 @@ def serve_docs( """Map existing proj, lang, version, filename views to the file format.""" if not version_slug: version_slug = project.get_default_version() + try: - version = project.versions.public(request.user).get(slug=version_slug) + version = project.versions.get(slug=version_slug) except Version.DoesNotExist: - # Properly raise a 404 if the version doesn't exist (or is inactive) and - # a 401 if it does - if project.versions.filter(slug=version_slug, active=True).exists(): - return _serve_401(request, project) raise Http404('Version does not exist.') + + # only project admins should see private versions + if not AdminPermission.is_member(user=request.user, obj=project): + if version.privacy_level == constants.PRIVATE: + return _serve_401(request, project) + if not version.active: + raise Http404('Version does not exist.') + filename = resolve_path( subproject or project, # Resolve the subproject if it exists version_slug=version_slug, @@ -216,9 +221,6 @@ def serve_docs( filename=filename, subdomain=True, # subdomain will make it a "full" path without a URL prefix ) - if (version.privacy_level == constants.PRIVATE and - not AdminPermission.is_member(user=request.user, obj=project)): - return _serve_401(request, project) return _serve_symlink_docs( request, filename=filename, diff --git a/readthedocs/rtd_tests/tests/test_redirects.py b/readthedocs/rtd_tests/tests/test_redirects.py index 9d0836a7516..eb99e9dfa51 100644 --- a/readthedocs/rtd_tests/tests/test_redirects.py +++ b/readthedocs/rtd_tests/tests/test_redirects.py @@ -1,6 +1,7 @@ import logging -from django.http import Http404 +from django.http import Http404, HttpResponse +from django.contrib.auth.models import User from django.test import TestCase from django.test.utils import override_settings from django_dynamic_fixture import fixture, get @@ -45,8 +46,25 @@ def test_proper_url(self): r = self.client.get('/docs/pip/') self.assertEqual(r.status_code, 302) self.assertEqual( - r['Location'], 'http://readthedocs.org/docs/pip/en/latest/', - ) + r['Location'], 'http://readthedocs.org/docs/pip/en/latest/') + + def test_owner_with_multiple_projects_can_serve_the_requested_version(self): + # GH #4350 + eric = User.objects.get(username='eric') + notpip = eric.projects.exclude(slug='pip').first() + notpip.versions.create_latest() + + self.client.login(username='eric', password='test') + with patch('readthedocs.core.views.serve._serve_symlink_docs') as _serve_docs: + _serve_docs.return_value = HttpResponse() + self.client.get('/docs/pip/en/latest/') + self.assertTrue(_serve_docs.called) + + def test_inactive_docs_should_return_404_if_the_user_is_not_the_project_admin(self): + pip = Project.objects.get(slug='pip') + pip.versions.filter(slug='latest').update(active=False) + response = self.client.get('/docs/pip/en/latest/') + self.assertEqual(response.status_code, 404) # Specific Page Redirects def test_proper_page_on_main_site(self):