Merge lp:~salgado/offspring/private-projects-views into lp:offspring

[Guilherme Salgado]

Make project and build views return a 404 if the user is not allowed to see that private project/build.

[lp:~salgado/offspring/private-projects-views]

92. By Kevin McDermott on 2011-12-02

Merge add-path-independence [r=salgado] [f=897203]

This changes the default file used in Offspring, to use a file sourced from the
root of the offspring Python source files.

The path to the configuration file can still be explicitly set either through
the environment or explicitly on the command-line.

93. By Kevin McDermott on 2011-12-02

Merge slave-testing [r=jedimike] [f=898118]

This adds tests for the Offspring LexBuilderSlave functionality.

94. By Guilherme Salgado on 2011-12-02

Make project and build views return a 404 if the user is not allowed to see that private project/build.

95. By Guilherme Salgado on 2011-12-02

Update callsites that were still using the factory methods with old names

96. By Guilherme Salgado on 2011-12-02

Fix two leftover occurrences of config.web

97. By Guilherme Salgado on 2011-12-02

Add docstrings to test methods

[Kevin McDermott (bigkevmcd)]

=== added file 'lib/offspring/web/queuemanager/tests/test_views.py'
#1

+from django.contrib.auth.models import (
+ AnonymousUser,
+ Group,
+ Permission,
+ )
These all fit on one line...

+from offspring.web.queuemanager.models import Project
+ )

This too...

#2

+class Test_get_possibly_private_object(TestCase):

" Almost without exception, class names use the CapWords convention. Classes for internal use have a leading underscore in addition."

#3

+ def get_expected_page_heading(self, project):
+ raise NotImplementedError()

+
+ def grant_necessary_permission_to(self, user):
+ pass

No docstring, what is expected of subclasses?

For extra points, in the case of "grant_necessary_permission_to", you can drop the pass if you write a docstring, as the docstring counts as the body of the method ;-)

#4

+ def test_private_project_invisible(self):
+ """
+ Check that this view of a private project is not accessible to those
+ who lack the rights to see the project.
+ """
+ project = factory.make_project(is_private=True)
+ user = factory.make_user()
+ self.grant_necessary_permission_to(user)
+ self.assertTrue(
+ self.client.login(username=user.username, password=user.username))
+ response = self.client.get(
+ reverse(self.view_path, args=[project.name]))
+ self.assertEqual(404, response.status_code)

Should this really return a 404? How about a 403? There may be a good reason for returning a 404 btw :-)

#5

+class BuildViewTests(TestCase):

This should refer to BuildResult to make it clearer (and in the subsequent tests), given that there are BuildResults and BuildRequests...

#6

+def grant_permission_to_user(user, permission):

No docstring, and the parameter is misleading, it doesn't accept a permission, it accepts a Permission codename...

=== modified file 'lib/offspring/web/queuemanager/views.py'

 from django.shortcuts import (
     render_to_response,
- get_object_or_404
+ get_object_or_404,
 )

This import would fit all on one line...

#7

+def get_possibly_private_object(user, klass, *args, **kwargs):
+ """Use get_object_or_404() to return a possibly private object.
+
+ If the object does not exist or the user doesn't have permission to see
+ it, raise Http404.
+ """
+ obj = get_object_or_404(klass, *args, **kwargs)
+ if not obj.is_visible_to(user):
+ raise Http404('No matches for the given query')
+ return obj

Could this reasonably be named better? get_possibly_private_object_or_404 ?

That's also not great, but given that your function is intended to be a replacement the standard get_object_or_404, it would make reading the code easier...

How about get_protected_object_or_404 ? Not sure it's much of an improvement, but "possibly_private" sounds odd...

#8

+ user_visible_builds = BuildResult.all_objects.accessible_by_user(
+ request.user)
+ b = get_possibly_private_object(
+ request.user,
+ user_visible_builds.select_related('builder', 'project', 'requestor'),

"Use the function naming rules: lowercase with words separated by underscores ...

review: Needs Fixing

[Guilherme Salgado (salgado)]

On Wed, 2011-12-07 at 08:24 +0000, Kevin McDermott wrote:
> Review: Needs Fixing
>
> === added file 'lib/offspring/web/queuemanager/tests/test_views.py'
> #1
>
> +from django.contrib.auth.models import (
> + AnonymousUser,
> + Group,
> + Permission,
> + )
> These all fit on one line...
>
> +from offspring.web.queuemanager.models import Project
> + )
>
> This too...

Oops, fixed both.

>
> #2
>
> +class Test_get_possibly_private_object(TestCase):
>
> " Almost without exception, class names use the CapWords convention. Classes for internal use have a leading underscore in addition."

This was because I wanted to put the method name verbatin in there. Is
TestGetPossiblyPrivateObject any better?

>
> #3
>
> + def get_expected_page_heading(self, project):
> + raise NotImplementedError()
>
> +
> + def grant_necessary_permission_to(self, user):
> + pass
>
> No docstring, what is expected of subclasses?
>
> For extra points, in the case of "grant_necessary_permission_to", you can drop the pass if you write a docstring, as the docstring counts as the body of the method ;-)
>
> #4
>
> + def test_private_project_invisible(self):
> + """
> + Check that this view of a private project is not accessible to those
> + who lack the rights to see the project.
> + """
> + project = factory.make_project(is_private=True)
> + user = factory.make_user()
> + self.grant_necessary_permission_to(user)
> + self.assertTrue(
> + self.client.login(username=user.username, password=user.username))
> + response = self.client.get(
> + reverse(self.view_path, args=[project.name]))
> + self.assertEqual(404, response.status_code)
>
> Should this really return a 404? How about a 403? There may be a good reason for returning a 404 btw :-)

Yes, we return a 404 because we don't want people to know that there is a
project they're not allowed to see there.

>
> #5
>
> +class BuildViewTests(TestCase):
>
> This should refer to BuildResult to make it clearer (and in the subsequent tests), given that there are BuildResults and BuildRequests...

Right, fixed.

>
> #6
>
> +def grant_permission_to_user(user, permission):
>
> No docstring, and the parameter is misleading, it doesn't accept a permission, it accepts a Permission codename...

Fixed both.

>
> === modified file 'lib/offspring/web/queuemanager/views.py'
>
> from django.shortcuts import (
> render_to_response,
> - get_object_or_404
> + get_object_or_404,
> )
>
> This import would fit all on one line...

Changed

> #7
>
> +def get_possibly_private_object(user, klass, *args, **kwargs):
> + """Use get_object_or_404() to return a possibly private object.
> +
> + If the object does not exist or the user doesn't have permission to see
> + it, raise Http404.
> + """
> + obj = get_object_or_404(klass, *args, **kwargs)
> + if not obj.is_visible_to(user):
> + raise Http404('No matches for the given query')
> + return obj
>
> Could this reasonably be named better? get_possibly_private_object_or_404 ?
>
> That's also not great, but given that yo...

[lp:~salgado/offspring/private-projects-views]

98. By Guilherme Salgado on 2011-12-07

A few tweaks and improvements suggested by Kevin

[Kevin McDermott (bigkevmcd)]

>
> You mean that just because of the extra hack we need or because there are
> other problems with generic view?
>
> Also, do you think it's important enough to block this merge or would it be ok
> do land it this way, unblock the queue, and then file a bug to have it fixed
> later?

Sure, can you file a bug for this tho'?

My concern is that some untested-for behaviour will break due to the global being changed...maybe not today, or tomorrow... :-)

+1 from me.

review: Approve

=== modified file 'Makefile'
--- Makefile	2011-12-01 16:02:08 +0000
+++ Makefile	2011-12-07 13:26:26 +0000
@@ -40,6 +40,12 @@
 	tar --directory=lib/offspring/web/media/js/ -zxf .download-cache/SmartClientRuntime-7.0.tgz
 	make
 
+web-run: web
+	./bin/offspring-web runserver --settings=offspring.web.settings_devel
+
+web-shell: web
+	./bin/offspring-web shell --settings=offspring.web.settings_devel
+
 install-test-runner: .virtualenv .download-cache
 	$(PIP_INSTALL) nose
 

=== modified file 'lib/offspring/web/queuemanager/tests/__init__.py'
--- lib/offspring/web/queuemanager/tests/__init__.py	2011-12-01 15:09:51 +0000
+++ lib/offspring/web/queuemanager/tests/__init__.py	2011-12-07 13:26:26 +0000
@@ -1,1 +1,2 @@
 from offspring.web.queuemanager.tests.test_models import *
+from offspring.web.queuemanager.tests.test_views import *

=== modified file 'lib/offspring/web/queuemanager/tests/factory.py'
--- lib/offspring/web/queuemanager/tests/factory.py	2011-11-30 14:29:13 +0000
+++ lib/offspring/web/queuemanager/tests/factory.py	2011-12-07 13:26:26 +0000
@@ -73,14 +73,17 @@
             title = self.get_unique_string()
         return ProjectGroup.objects.create(name=name, title=title)
 
-    def make_build_result(self, project=None):
+    def make_build_result(self, project=None, name=None, result=None):
         """
         Create a BuildResult with the given project or a newly created one if
         none is given.
         """
+        if name is None:
+            name = self.get_unique_string()
         if project is None:
             project = self.make_project()
-        return BuildResult.objects.create(project=project)
+        return BuildResult.objects.create(
+            project=project, name=name, result=result)
 
     def make_release(self, build=None, name=None, creator=None):
         """

=== added file 'lib/offspring/web/queuemanager/tests/test_views.py'
--- lib/offspring/web/queuemanager/tests/test_views.py	1970-01-01 00:00:00 +0000
+++ lib/offspring/web/queuemanager/tests/test_views.py	2011-12-07 13:26:26 +0000
@@ -0,0 +1,259 @@
+from django.core.urlresolvers import reverse
+from django.contrib.auth.models import AnonymousUser, Group, Permission
+from django.http import Http404
+from django.test import TestCase
+
+from offspring.enums import ProjectBuildStates
+from offspring.web.queuemanager.models import Project
+from offspring.web.queuemanager.views import get_possibly_private_object
+from offspring.web.queuemanager.tests.factory import factory
+
+
+class TestGetPossiblyPrivateObject(TestCase):
+
+    def test_public_object(self):
+        """
+        Check that get_possibly_private_object() returns the public project
+        with the given key.
+        """
+        obj = factory.make_project(is_private=False)
+        obj2 = get_possibly_private_object(
+            AnonymousUser(), Project, pk=obj.pk)
+        self.assertEqual(obj.pk, obj2.pk)
+
+    def test_private_object_for_owner(self):
+        """
+        Check that get_possibly_private_object() returns the private project
+        with the given key when the user is the project's owner.
+        """
+        obj = factory.make_project(is_private=True)
+        obj2 = get_possibly_private_object(obj.owner, Project, pk=obj.pk)
+        self.assertEqual(obj.pk, obj2.pk)
+
+    def test_private_object_for_user_with_no_access(self):
+        """
+        Check that get_possibly_private_object() raises a 404 error when a
+        user tries to get a project they're not allowed to see.
+        """
+        obj = factory.make_project(is_private=True)
+        user = factory.make_user()
+        self.assertRaises(
+            Http404, get_possibly_private_object, user, Project, pk=obj.pk)
+
+
+class ProjectViewTestsMixin(object):
+    view_path = None
+
+    def get_expected_page_heading(self, project):
+        """
+        Return the heading that should be present on the page tested here.
+
+        Must be overwritten in subclasses.
+        """
+        raise NotImplementedError()
+
+    def grant_necessary_permission_to(self, user):
+        """
+        Grant the permission necessary for the given user to see the page
+        tested here.
+
+        Only override it in subclasses if the page you're testing requires
+        an extra permission to be seen.
+        """
+
+    def test_private_project_visible(self):
+        """
+        Check that this view of a private project is accessible to those who
+        have the rights to see the project.
+        """
+        project = factory.make_project(is_private=True)
+        user = project.owner
+        self.grant_necessary_permission_to(user)
+        self.assertTrue(
+            self.client.login(username=user.username, password=user.username))
+        response = self.client.get(
+            reverse(self.view_path, args=[project.name]))
+        self.assertContains(
+            response, self.get_expected_page_heading(project),
+            status_code=200, msg_prefix=response.content)
+
+    def test_private_project_invisible(self):
+        """
+        Check that this view of a private project is not accessible to those
+        who lack the rights to see the project.
+        """
+        project = factory.make_project(is_private=True)
+        user = factory.make_user()
+        self.grant_necessary_permission_to(user)
+        self.assertTrue(
+            self.client.login(username=user.username, password=user.username))
+        response = self.client.get(
+            reverse(self.view_path, args=[project.name]))
+        self.assertEqual(404, response.status_code)
+
+
+class ProjectDetailsViewTests(TestCase, ProjectViewTestsMixin):
+    view_path = 'offspring.web.queuemanager.views.project_details'
+
+    def get_expected_page_heading(self, project):
+        return 'Project Details for %s' % project.title.capitalize()
+
+    def test_public_project(self):
+        """
+        Check that the details view of a public project is accessible to
+        anyone.
+        """
+        project = factory.make_project(is_private=False)
+        response = self.client.get(
+            reverse(self.view_path, args=[project.name]))
+        self.assertContains(
+            response, self.get_expected_page_heading(project),
+            status_code=200, msg_prefix=response.content)
+
+
+class ProjectEditViewTests(TestCase, ProjectViewTestsMixin):
+    view_path = 'offspring.web.queuemanager.views.project_edit'
+
+    def get_expected_page_heading(self, project):
+        return 'Update details for %s' % project.title.capitalize()
+
+    def grant_necessary_permission_to(self, user):
+        grant_permission_to_user(user, 'change_project')
+
+    def test_public_project(self):
+        """
+        Check that the edit view of a public project is accessible to those
+        who have the change_project permission.
+        """
+        project = factory.make_project(is_private=False)
+        user = factory.make_user()
+        self.grant_necessary_permission_to(user)
+        self.assertTrue(
+            self.client.login(username=user.username, password=user.username))
+        response = self.client.get(
+            reverse(self.view_path, args=[project.name]))
+        self.assertContains(
+            response, self.get_expected_page_heading(project),
+            status_code=200, msg_prefix=response.content)
+
+
+class BuildResultViewTests(TestCase):
+
+    def test_public_build(self):
+        """
+        Check that the main view of a public build is accessible to anyone.
+        """
+        project = factory.make_project(is_private=False)
+        build = factory.make_build_result(project=project)
+        response = self.client.get(
+            reverse('offspring.web.queuemanager.views.build_details',
+                    args=[project.name, build.name]))
+        self.assertContains(
+            response, 'Build Details', status_code=200,
+            msg_prefix=response.content)
+
+    def test_private_build_visible(self):
+        """
+        Check that the main view of a private build is accessible to those who
+        have the rights to see the build's project.
+        """
+        project = factory.make_project(is_private=True)
+        build = factory.make_build_result(project=project)
+        user = project.owner
+        self.assertTrue(
+            self.client.login(username=user.username, password=user.username))
+        response = self.client.get(
+            reverse('offspring.web.queuemanager.views.build_details',
+                    args=[project.name, build.name]))
+        self.assertContains(
+            response, 'Build Details', status_code=200,
+            msg_prefix=response.content)
+
+    def test_private_build_invisible(self):
+        """
+        Check that the main view of a private build is not accessible to those
+        who lack the rights to see the build's project.
+        """
+        project = factory.make_project(is_private=True)
+        build = factory.make_build_result(project=project)
+        self.assertTrue(make_user_and_login(self.client))
+        response = self.client.get(
+            reverse('offspring.web.queuemanager.views.build_details',
+                    args=[project.name, build.name]))
+        self.assertEqual(404, response.status_code)
+
+
+class BuildResultPublishViewTests(TestCase):
+
+    def test_public_build(self):
+        """
+        Check that the publish view of a public build is accessible to anyone.
+        """
+        project = factory.make_project(is_private=False)
+        build = factory.make_build_result(
+            project=project, result=ProjectBuildStates.SUCCESS)
+        user = factory.make_user()
+        grant_permission_to_user(user, 'add_release')
+        self.assertTrue(
+            self.client.login(username=user.username, password=user.username))
+        response = self.client.get(
+            reverse('offspring.web.queuemanager.views.build_publish',
+                    args=[project.name, build.name]))
+        self.assertContains(
+            response, 'Release %s build' % project.title.capitalize(),
+            status_code=200, msg_prefix=response.content)
+
+    def test_private_build_visible(self):
+        """
+        Check that the publish view of a private build is accessible to those
+        who have the rights to see the build's project and the add_release
+        permission.
+        """
+        project = factory.make_project(is_private=True)
+        build = factory.make_build_result(
+            project=project, result=ProjectBuildStates.SUCCESS)
+        user = build.project.owner
+        grant_permission_to_user(user, 'add_release')
+        self.assertTrue(
+            self.client.login(username=user.username, password=user.username))
+        response = self.client.get(
+            reverse('offspring.web.queuemanager.views.build_publish',
+                    args=[project.name, build.name]))
+        self.assertContains(
+            response, 'Release %s build' % project.title.capitalize(),
+            status_code=200, msg_prefix=response.content)
+
+    def test_private_build_invisible(self):
+        """
+        Check that the publish view of a private build is not accessible to
+        those who lack the rights to see the build's project.
+        """
+        project = factory.make_project(is_private=True)
+        build = factory.make_build_result(
+            project=project, result=ProjectBuildStates.SUCCESS)
+        user = factory.make_user()
+        grant_permission_to_user(user, 'add_release')
+        self.assertTrue(
+            self.client.login(username=user.username, password=user.username))
+        response = self.client.get(
+            reverse('offspring.web.queuemanager.views.build_publish',
+                    args=[project.name, build.name]))
+        self.assertEqual(404, response.status_code)
+
+
+def make_user_and_login(client):
+    user = factory.make_user()
+    return client.login(username=user.username, password=user.username)
+
+
+def grant_permission_to_user(user, permission_codename):
+    """Grant the permission with the given codename to the given user."""
+    group = Group(name=user.username)
+    group.save()
+    group.permissions.add(
+        Permission.objects.get(codename=permission_codename))
+    group.save()
+    user.groups.add(group)
+    user.save()
+    return user
+

=== modified file 'lib/offspring/web/queuemanager/views.py'
--- lib/offspring/web/queuemanager/views.py	2011-11-18 15:45:06 +0000
+++ lib/offspring/web/queuemanager/views.py	2011-12-07 13:26:26 +0000
@@ -16,13 +16,11 @@
 from django.http import (
     HttpResponse,
     HttpResponseRedirect,
-    HttpResponseForbidden
+    HttpResponseForbidden,
+    Http404,
 )
 from django.middleware import csrf
-from django.shortcuts import (
-    render_to_response,
-    get_object_or_404
-)
+from django.shortcuts import render_to_response, get_object_or_404
 from django.template import (
     Context,
     RequestContext,
@@ -40,7 +38,7 @@
 
 from pygooglechart import PieChart3D
 
-from offspring import config
+from offspring.config import get_configuration
 from offspring.enums import ProjectBuildStates
 from offspring.web.queuemanager.forms import (
     CreateProjectForm,
@@ -57,6 +55,22 @@
     Release
 )
 
+
+config = get_configuration()
+
+
+def get_possibly_private_object(user, klass, *args, **kwargs):
+    """Use get_object_or_404() to return a possibly private object.
+
+    If the object does not exist or the user doesn't have permission to see
+    it, raise Http404.
+    """
+    obj = get_object_or_404(klass, *args, **kwargs)
+    if not obj.is_visible_to(user):
+        raise Http404('No matches for the given query')
+    return obj
+
+
 #XXX: Reduce horrible duplication that occurs in these functions (mostly done).
 #TODO: Thanks to reduced duplication, lets start using more generic views.
 
@@ -118,15 +132,18 @@
 
 
 def build_details(request, projectName, buildName):
-    b = get_object_or_404(
-        BuildResult.objects.select_related('builder', 'project', 'requestor'),
+    user_visible_builds = BuildResult.all_objects.accessible_by_user(
+        request.user)
+    build = get_possibly_private_object(
+        request.user,
+        user_visible_builds.select_related('builder', 'project', 'requestor'),
         project__pk=projectName, name=buildName)
     pageData = { 
         'pillar' : 'builds',
         'upcoming_milestones' : LaunchpadProjectMilestone.upcoming_milestones(),
         'recent_milestones' : LaunchpadProjectMilestone.recent_milestones(),
-        'project' : b.project, 
-        'build' : b
+        'project' : build.project, 
+        'build' : build
     }
     return render_to_response(
         'queuemanager/build_details.html', pageData, 
@@ -134,14 +151,18 @@
 
 @login_required
 def build_publish(request, projectName, buildName):
-    p = get_object_or_404(Project, pk=projectName)
-    b = get_object_or_404(
-        BuildResult, project=p, name=buildName, 
+    user = request.user
+    p = get_possibly_private_object(user, Project, pk=projectName)
+    # We use get_possibly_private_object() here just for consistency as it's
+    # not really needed because the above will fail if the user doesn't have
+    # the rights to see this build.
+    b = get_possibly_private_object(
+        user, BuildResult, project=p, name=buildName, 
         result=ProjectBuildStates.SUCCESS, release=None)
-    if request.user.has_perm('queuemanager.add_release'):
+    if user.has_perm('queuemanager.add_release'):
         r = Release()
         r.build = b
-        r.creator = request.user
+        r.creator = user
         if request.method == 'POST':
             f = ReleaseForm(request.POST, instance=r)
             if f.is_valid():
@@ -175,21 +196,35 @@
 project_create = permission_required('queuemanager.add_project')(project_create)
 
 def project_edit(request, projectName):
-    return update_object(
-        request,
-        form_class=EditProjectForm,
-        login_required=True,
-        template_name='queuemanager/project_edit.html',
-        template_object_name='project',
-        extra_context={ 'pillar' : 'projects' },
-        object_id=projectName,
-    )
+    # XXX: This nasty hack is because update_object uses (via lookup_object)
+    # .objects directly when it should insted use the default manager
+    # (https://code.djangoproject.com/ticket/17021).
+    from django.views.generic import create_update
+    orig_lookup = create_update.lookup_object
+    def lookup(model, obj_id, slug, slug_field):
+        return get_possibly_private_object(
+            request.user, Project, pk=projectName)
+    create_update.lookup_object = lookup
+    try:
+        response = update_object(
+            request,
+            form_class=EditProjectForm,
+            login_required=True,
+            template_name='queuemanager/project_edit.html',
+            template_object_name='project',
+            extra_context={ 'pillar' : 'projects' },
+            object_id=projectName,
+        )
+    finally:
+        create_update.lookup_object = orig_lookup
+    return response
 project_edit = permission_required('queuemanager.change_project')(project_edit)
 
 def project_details(request, projectName):
-    p = get_object_or_404(
-        Project.objects.select_related('project_group', 'launchpad_project'), 
-        pk=projectName)
+    user_visible_objects = Project.all_objects.accessible_by_user(
+        request.user)
+    p = get_possibly_private_object(
+        request.user, user_visible_objects, pk=projectName)
     project_build_results = BuildResult.objects.filter(
         project = p).exclude(result = None)
     build_stats = { 
@@ -204,15 +239,16 @@
         chart.set_pie_labels(["Failed Builds", "Successful Builds"])
         build_stats['pie_chart'] = chart.get_url()
 
-    releases_uri_base = config.web('releases_uri_base')
+    releases_uri_base = config.get('web', 'releases_uri_base')
     if not releases_uri_base.endswith('/'):
         releases_uri_base += '/'
     releases_url = urlparse.urljoin(releases_uri_base, p.name + '/images/')
 
+    archive_uri = config.get('web', 'archive_uri', None)
     pageData = {
         'build_stats' : build_stats,
         'csrf_token' : csrf.get_token(request),
-        'include_sources_list': config.web('archive_uri', None) is not None,
+        'include_sources_list': archive_uri is not None,
         'pillar' : 'projects',
         'project' : p,
         'project_build_results' : project_build_results,