launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/404-project-milestones-privacy into lp:launchpad.

Commit message:
Fixes ProjectMilestone generation in the milestone page to not link private products for unpriveleged users.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1079405 in Launchpad itself: "ProjectGroupMilestone:+index 403s because of proprietary data on associated products"
  https://bugs.launchpad.net/launchpad/+bug/1079405

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/404-project-milestones-privacy/+merge/135252

Summary
=======
Going to a projectmilestone index page causes a 403 when the milestone
incorporates data from private products.

This is because product and milestone data isn't sufficiently filtered. While
the initial query to fetch all milestones on a project's associated products
filters, a second query is made to group the milestones appropriated and
combine the data for product milestones with the same name. This second a
query is not filtered, and should be.

Preimp
======
Spoke with Curtis Hovey about the initial problem.

Implementation
==============
Add the privacy filter to the second query.

Tests
=====
bin/test -vvct test_mixed_product_projectgroup_milestone

QA
==
QA steps are listed in a comment by Curtis Hovey on the bug.

LoC
===
Part of private projects.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/model/projectgroup.py
  lib/lp/registry/browser/tests/test_projectgroup.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/404-project-milestones-privacy/+merge/135252
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/404-project-milestones-privacy into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_projectgroup.py'
--- lib/lp/registry/browser/tests/test_projectgroup.py	2012-10-19 14:22:36 +0000
+++ lib/lp/registry/browser/tests/test_projectgroup.py	2012-11-20 21:22:23 +0000
@@ -15,7 +15,10 @@
 from lp.app.browser.lazrjs import vocabulary_to_choice_edit_items
 from lp.app.enums import InformationType
 from lp.registry.browser.tests.test_product import make_product_form
-from lp.registry.enums import EXCLUSIVE_TEAM_POLICY
+from lp.registry.enums import (
+    EXCLUSIVE_TEAM_POLICY,
+    TeamMembershipPolicy,
+    )
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
 from lp.services.features.testing import FeatureFixture
@@ -96,6 +99,41 @@
         self.assertNotIn(product_name, browser.contents)
         self.assertIn(public_product.displayname, browser.contents)
 
+    def test_mixed_product_projectgroup_milestone(self):
+        # If a milestone is mixed between public and proprietary products,
+        # only the public is shown to people without access.
+        self.useFixture(
+            FeatureFixture({u'disclosure.private_projects.enabled': u'on'}))
+        owner = self.factory.makePerson()
+        teammember = self.factory.makePerson()
+        owning_team = self.factory.makeTeam(owner=owner,
+                membership_policy=TeamMembershipPolicy.RESTRICTED)
+        with person_logged_in(owner):
+            owning_team.addMember(teammember, owner)
+        group = self.factory.makeProject(owner=owning_team)
+        private = self.factory.makeProduct(
+            project=group, owner=owner,
+            information_type=InformationType.PROPRIETARY, name='private')
+        public = self.factory.makeProduct(
+            project=group, owner=owner, name='public')
+        private_milestone = self.factory.makeMilestone(
+            product=private, name='1.0')
+        public_milestone = self.factory.makeMilestone(
+            product=public, name='1.0')
+        with person_logged_in(owner):
+            self.factory.makeBug(
+                target=private, owner=owner, milestone=private_milestone,
+                title='This is the private bug')
+            self.factory.makeBug(
+                target=public, owner=owner, milestone=public_milestone,
+                title='This is the public bug')
+
+        group_milestone = group.milestones[0]
+        browser = self.getViewBrowser(
+            group_milestone, user=self.factory.makePerson())
+        self.assertTrue("This is the public bug" in browser.contents)
+        self.assertFalse("This is the private bug" in browser.contents)
+
 
 class TestProjectGroupEditView(TestCaseWithFactory):
     """Tests the edit view."""

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2012-11-15 22:23:08 +0000
+++ lib/lp/registry/model/projectgroup.py	2012-11-20 21:22:23 +0000
@@ -430,10 +430,11 @@
             SQL('MIN(Milestone.dateexpected)'),
             SQL('BOOL_OR(Milestone.active)'),
             )
+        privacy_filter = ProductSet.getProductPrivacyFilter(user)
         conditions = And(Milestone.product == Product.id,
                          Product.project == self,
                          Product.active == True,
-                         ProductSet.getProductPrivacyFilter(user))
+                         privacy_filter)
         result = store.find(columns, conditions)
         result.group_by(Milestone.name)
         if only_active:
@@ -452,6 +453,7 @@
                 Product.project == self,
                 Milestone.product == Product.id,
                 Product.active == True,
+                privacy_filter,
                 In(Milestone.name, milestone_names))
             for product, name in (
                 store.find((Product, Milestone.name), product_conditions)):