Merge lp:~rharding/launchpad/related_projects_1063272 into lp:launchpad

[Richard Harding]

Filter affiliatedPillars based on access grants to hide non-visible products.

= Summary =

The person getAffiliatedPillars doesn't take into account products that are
not visible to the user. This adds the same checks as
product.getProductPrivacyFilter in order to clean that data.

== Pre Implementation ==

Talked with Orange about if this could/should be converted to storm or
performed with raw sql adjustments. The storm/union/sorting issues kept it as
raw sql at this time.

Talked with Curtis and Deryck around handling the account deactivation issue
this ran smack into.

== Implementation Notes ==

Due to the nature of the query here, with unions that must be in a specific
sorted order, the product method could not be used. It's logic was turned into
raw sql and used to filter out the product part of the union.

This makes sure that commercial admins and admins maintain access/visibility
by passing in the current viewing user to the underlying methods in order to
check for permissions.

_genAffiliatedProductSql is created to contain the logic about how the product
part of the union is to be done. It uses the old query if you're an
admin/commercial admin, however it filters based on granted products
otherwise. See product.getPrivacyFilter for the logic that was copied to
construct this query.

As a side effect, we ran into the fact that account deactivation uses
getAffiliatedPillars, which we've updated.

The decision was made that accounts that are owners of non-public products
cannot be deactivated. This needed to be added to view validation. In order to
prevent duplicate code, the validation was added to the person model. It
checks for a list of non-public products owned by the user. For validation
purposes, we needed two methods. One that's just a boolean T/F, the second
however, is needed to get readable error message back to the view to be placed
in front of the user. Because of this we've added both canDeactivateUser and
canDeactivateUserWithErrors.

The validation checking in Person required a new query on Product so that we
can easily and quickly get the list of non-public products. We needed to do a
full query to get the list so we can provide the names of those products to
the user in the validation error message.

The final trick was to add a new can_deactivate kwarg so that we can prevent
the query being hit twice, once in the view and once in the model. Since the
view did the check it can let the model know it's been sanity checked already.

There's a drive by change requested by Curtis. There's no sense setting the bug supervisor or the driver to registry experts. They are nullable values. So we set them to null and only change the owner to help with registry spam.

== QA ==

Log in as a user and create a private product. Then visit the user's
+related-projects page and it should load just fine.

Log out and as an anonymous user also load the same url without issue. No
private products should be visible to the anonymous user.

Deactivating the user with the private product should not be permitted. It
must be re-assigned to another user before deactivation is permitted.

== Tests ==

registry/tests/test_person.py
registry/tests/test_product.py

[Brad Crittenden (bac)]

Hi Rick,

Some comments on your branch:

* In genAffiliatedProductSql there is a lot of repetition in the queries that could be factored out so you DRY as the first query you return is a the same as the final one minus one clause. Give a shot at refactoring if you don't mind.

* Thanks for the nice error messages when the account cannot be deactivated.

* The tests are clear and easy to follow. Thanks.

review: Approve (code)

[Richard Harding (rharding)]

> Hi Rick,
>
> Some comments on your branch:
>
> * In genAffiliatedProductSql there is a lot of repetition in the queries that
> could be factored out so you DRY as the first query you return is a the same
> as the final one minus one clause. Give a shot at refactoring if you don't
> mind.

Thanks for the catch. Was a series of iterations over that and didn't get back to cleaning it up. I've reused the base_query for the second build now.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-10-22 02:30:44 +0000
+++ lib/lp/registry/browser/person.py	2012-10-24 09:24:22 +0000
@@ -635,7 +635,8 @@
     def projects(self):
         target = '+related-projects'
         text = 'Related projects'
-        enabled = bool(self.person.getAffiliatedPillars())
+        user = getUtility(ILaunchBag).user
+        enabled = bool(self.person.getAffiliatedPillars(user))
         return Link(target, text, enabled=enabled, icon='info')
 
     def subscriptions(self):
@@ -936,12 +937,15 @@
 
     def validate(self, data):
         """See `LaunchpadFormView`."""
-        if self.context.account_status != AccountStatus.ACTIVE:
-            self.addError('This account is already deactivated.')
+        can_deactivate, errors = self.context.canDeactivateAccountWithErrors()
+        if not can_deactivate:
+            [self.addError(message) for message in errors]
 
     @action(_("Deactivate My Account"), name="deactivate")
     def deactivate_action(self, action, data):
-        self.context.deactivateAccount(data['comment'])
+        # We override the can_deactivate since validation already processed
+        # this information.
+        self.context.deactivateAccount(data['comment'], can_deactivate=True)
         logoutPerson(self.request)
         self.request.response.addInfoNotification(
             _(u'Your account has been deactivated.'))
@@ -3517,7 +3521,8 @@
     @cachedproperty
     def _related_projects(self):
         """Return all projects owned or driven by this person."""
-        return self.context.getAffiliatedPillars()
+        user = getUtility(ILaunchBag).user
+        return self.context.getAffiliatedPillars(user)
 
     def _tableHeaderMessage(self, count, label='package'):
         """Format a header message for the tables on the summary page."""

=== modified file 'lib/lp/registry/doc/person-account.txt'
--- lib/lp/registry/doc/person-account.txt	2012-09-28 14:35:35 +0000
+++ lib/lp/registry/doc/person-account.txt	2012-10-24 09:24:22 +0000
@@ -104,7 +104,7 @@
     True
 
     >>> foobar_pillars = []
-    >>> for pillar_name in foobar.getAffiliatedPillars():
+    >>> for pillar_name in foobar.getAffiliatedPillars(foobar):
     ...     pillar = pillar_name.pillar
     ...     if pillar.owner == foobar or pillar.driver == foobar:
     ...         foobar_pillars.append(pillar_name)
@@ -183,7 +183,7 @@
 
 ...no owned or driven pillars...
 
-    >>> foobar.getAffiliatedPillars().count()
+    >>> foobar.getAffiliatedPillars(foobar).count()
     0
 
 ...and, finally, to not be considered a valid person in Launchpad.
@@ -198,7 +198,7 @@
     >>> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
     >>> registry_experts = getUtility(ILaunchpadCelebrities).registry_experts
 
-    >>> registry_pillars = set(registry_experts.getAffiliatedPillars())
+    >>> registry_pillars = set(registry_experts.getAffiliatedPillars(foobar))
     >>> registry_pillars.issuperset(foobar_pillars)
     True
 

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-10-16 00:48:55 +0000
+++ lib/lp/registry/interfaces/person.py	2012-10-24 09:24:22 +0000
@@ -1132,7 +1132,7 @@
                           the icons which represent that category.
         """
 
-    def getAffiliatedPillars():
+    def getAffiliatedPillars(user):
         """Return the pillars that this person directly has a role with.
 
         Returns distributions, project groups, and projects that this person
@@ -1729,7 +1729,20 @@
 class IPersonSpecialRestricted(Interface):
     """IPerson methods that require launchpad.Special permission to use."""
 
-    def deactivateAccount(comment):
+    def canDeactivateAccount():
+        """Verify we safely deactivate this user account.
+
+        :return: True if the person can be deactivated, False otherwise.
+        """
+
+    def canDeactivateAccountWithErrors():
+        """See canDeactivateAccount with the addition of error messages for
+        why the account cannot be deactivated.
+
+        :return tuple: boolean, list of error messages.
+        """
+
+    def deactivateAccount(comment, can_deactivate=None):
         """Deactivate this person's Launchpad account.
 
         Deactivating an account means:
@@ -1740,6 +1753,9 @@
             - Changing the ownership of products/projects/teams owned by him.
 
         :param comment: An explanation of why the account status changed.
+        :param can_deactivate: Override the check if we can deactivate by
+            supplying a known value. If None, then the method will run the
+            checks.
         """
 
     def reactivate(comment, preferred_email):

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2012-10-16 13:59:06 +0000
+++ lib/lp/registry/interfaces/product.py	2012-10-24 09:24:22 +0000
@@ -942,6 +942,13 @@
     all_active = Attribute(
         "All the active products, sorted newest first.")
 
+    def get_users_private_products(user):
+        """Get users non-public products.
+
+        :param user: Which user are we searching products for.
+        :return: An iterable of IProduct
+        """
+
     def get_all_active(eager_load=True):
         """Get all active products.
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-10-22 02:30:44 +0000
+++ lib/lp/registry/model/person.py	2012-10-24 09:24:22 +0000
@@ -109,7 +109,10 @@
 
 from lp import _
 from lp.answers.model.questionsperson import QuestionsPersonMixin
-from lp.app.enums import PRIVATE_INFORMATION_TYPES
+from lp.app.enums import (
+    InformationType,
+    PRIVATE_INFORMATION_TYPES,
+    )
 from lp.app.interfaces.launchpad import (
     IHasIcon,
     IHasLogo,
@@ -202,7 +205,10 @@
 from lp.registry.interfaces.personnotification import IPersonNotificationSet
 from lp.registry.interfaces.persontransferjob import IPersonMergeJobSource
 from lp.registry.interfaces.pillar import IPillarNameSet
-from lp.registry.interfaces.product import IProduct
+from lp.registry.interfaces.product import (
+    IProduct,
+    IProductSet,
+    )
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.ssh import (
@@ -1030,19 +1036,64 @@
         cur.execute(query)
         return cur.fetchall()
 
-    def getAffiliatedPillars(self):
+    def _genAffiliatedProductSql(self, user=None):
+        """Helper to generate the product sql for getAffiliatePillars"""
+        base_query = """
+            SELECT name, 3 as kind, displayname
+            FROM product p
+            WHERE
+                p.active = True
+                AND (
+                    p.driver = %(person)s
+                    OR p.owner = %(person)s
+                    OR p.bug_supervisor = %(person)s
+                )
+        """ % sqlvalues(person=self)
+
+        if user is not None:
+            roles = IPersonRoles(user)
+            if roles.in_admin or roles.in_commercial_admin:
+                return base_query
+
+        # This is the raw sql version of model/product getProductPrivacyFilter
+        granted_products = """
+            SELECT p.id
+            FROM product p,
+                 accesspolicygrantflat apflat,
+                 teamparticipation part,
+                 accesspolicy ap
+             WHERE
+                apflat.grantee = part.team
+                AND part.person = %(user)s
+                AND apflat.policy = ap.id
+                AND ap.product = p.id
+                AND ap.type = p.information_type
+        """ % sqlvalues(user=user)
+
+        # We have to generate the sqlvalues first so that they're properly
+        # setup and escaped. Then we combine the above query which is already
+        # processed.
+        query_values = sqlvalues(information_type=InformationType.PUBLIC)
+        query_values.update(granted_sql=granted_products)
+
+        query = base_query + """
+                AND (
+                    p.information_type = %(information_type)s
+                    OR p.information_type is NULL
+                    OR p.id IN (%(granted_sql)s)
+                )
+        """ % query_values
+        return query
+
+    def getAffiliatedPillars(self, user):
         """See `IPerson`."""
         find_spec = (PillarName, SQL('kind'), SQL('displayname'))
-        origin = SQL("""
-            PillarName
-            JOIN (
-                SELECT name, 3 as kind, displayname
-                FROM product
-                WHERE
-                    active = True AND
-                    (driver = %(person)s
-                    OR owner = %(person)s
-                    OR bug_supervisor = %(person)s)
+        base = """PillarName
+                  JOIN (
+                    %s
+            """ % self._genAffiliatedProductSql(user=user)
+
+        origin = base + """
                 UNION
                 SELECT name, 2 as kind, displayname
                 FROM project
@@ -1059,8 +1110,9 @@
                     OR bug_supervisor = %(person)s
                 ) _pillar
                 ON PillarName.name = _pillar.name
-            """ % sqlvalues(person=self))
-        results = IStore(self).using(origin).find(find_spec)
+            """ % sqlvalues(person=self)
+
+        results = IStore(self).using(SQL(origin)).find(find_spec)
         results = results.order_by('kind', 'displayname')
 
         def get_pillar_name(result):
@@ -2109,15 +2161,47 @@
             clauseTables=['Person'],
             orderBy=Person.sortingColumns)
 
+    def canDeactivateAccount(self):
+        """See `IPerson`."""
+        can_deactivate, errors = self.canDeactivateAccountWithErrors()
+        return can_deactivate
+
+    def canDeactivateAccountWithErrors(self):
+        """See `IPerson`."""
+        # Users that own non-public products cannot be deactivated until the
+        # products are reassigned.
+        errors = []
+        product_set = getUtility(IProductSet)
+        non_public_products = product_set.get_users_private_products(self)
+        if non_public_products.count() != 0:
+            errors.append(('This account cannot be deactivated because it owns '
+                        'the following non-public products: ') +
+                        ','.join([p.name for p in non_public_products]))
+
+        if self.account_status != AccountStatus.ACTIVE:
+            errors.append('This account is already deactivated.')
+
+        return (not errors), errors
+
     # XXX: salgado, 2009-04-16: This should be called just deactivate(),
     # because it not only deactivates this person's account but also the
     # person.
-    def deactivateAccount(self, comment):
+    def deactivateAccount(self, comment, can_deactivate=None):
         """See `IPersonSpecialRestricted`."""
         if not self.is_valid_person:
             raise AssertionError(
                 "You can only deactivate an account of a valid person.")
 
+        if can_deactivate is None:
+            # The person can only be deactivated if they do not own any
+            # non-public products.
+            can_deactivate = self.canDeactivateAccount()
+
+        if not can_deactivate:
+            message = ("You cannot deactivate an account that owns a "
+                       "non-public product.")
+            raise AssertionError(message)
+
         for membership in self.team_memberships:
             self.leave(membership.team)
 
@@ -2146,7 +2230,7 @@
         registry_experts = getUtility(ILaunchpadCelebrities).registry_experts
         for team in Person.selectBy(teamowner=self):
             team.teamowner = registry_experts
-        for pillar_name in self.getAffiliatedPillars():
+        for pillar_name in self.getAffiliatedPillars(self):
             pillar = pillar_name.pillar
             # XXX flacoste 2007-11-26 bug=164635 The comparison using id below
             # works around a nasty intermittent failure.
@@ -2155,9 +2239,16 @@
                 pillar.owner = registry_experts
                 changed = True
             if pillar.driver is not None and pillar.driver.id == self.id:
-                pillar.driver = registry_experts
+                pillar.driver = None
                 changed = True
 
+            # Products need to change the bug supervisor as well.
+            if IProduct.providedBy(pillar):
+                if (pillar.bug_supervisor is not None and
+                    pillar.bug_supervisor.id == self.id):
+                    pillar.bug_supervisor = None
+                    changed = True
+
             if not changed:
                 # Since we removed the person from all teams, something is
                 # seriously broken here.

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-10-22 02:30:44 +0000
+++ lib/lp/registry/model/product.py	2012-10-24 09:24:22 +0000
@@ -1735,6 +1735,15 @@
                   Product.id.is_in(Select(Product.id, granted_products)))
 
     @classmethod
+    def get_users_private_products(cls, user):
+        """List the non-public products the user owns."""
+        result = IStore(Product).find(
+            Product,
+            Product._owner == user,
+            Product._information_type.is_in(PROPRIETARY_INFORMATION_TYPES))
+        return result
+
+    @classmethod
     def get_all_active(cls, user, eager_load=True):
         clause = cls.getProductPrivacyFilter(user)
         result = IStore(Product).find(Product, Product.active,

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2012-10-17 15:16:09 +0000
+++ lib/lp/registry/tests/test_person.py	2012-10-24 09:24:22 +0000
@@ -336,7 +336,7 @@
         expected_pillars = [
             distribution.name, project_group.name, project.name]
         received_pillars = [
-            pillar.name for pillar in  user.getAffiliatedPillars()]
+            pillar.name for pillar in  user.getAffiliatedPillars(user)]
         self.assertEqual(expected_pillars, received_pillars)
 
     def test_getAffiliatedPillars_roles(self):
@@ -352,7 +352,7 @@
         expected_pillars = [
             driven_project.name, owned_project.name, supervised_project.name]
         received_pillars = [
-            pillar.name for pillar in  user.getAffiliatedPillars()]
+            pillar.name for pillar in  user.getAffiliatedPillars(user)]
         self.assertEqual(expected_pillars, received_pillars)
 
     def test_getAffiliatedPillars_active_pillars(self):
@@ -364,7 +364,76 @@
             inactive_project.active = False
         expected_pillars = [active_project.name]
         received_pillars = [pillar.name for pillar in
-            user.getAffiliatedPillars()]
+            user.getAffiliatedPillars(user)]
+        self.assertEqual(expected_pillars, received_pillars)
+
+    def test_getAffiliatedPillars_minus_embargoed(self):
+        # Skip non public products if not allowed to see them.
+        owner = self.factory.makePerson()
+        user = self.factory.makePerson()
+        self.factory.makeProduct(
+            information_type=InformationType.EMBARGOED,
+            owner=owner)
+        public = self.factory.makeProduct(
+            information_type=InformationType.PUBLIC,
+            owner=owner)
+
+        expected_pillars = [public.name]
+        received_pillars = [pillar.name for pillar in
+            owner.getAffiliatedPillars(user)]
+        self.assertEqual(expected_pillars, received_pillars)
+
+    def test_getAffiliatedPillars_visible_to_self(self):
+        # Users can see their own non-public affiliated products.
+        owner = self.factory.makePerson()
+        self.factory.makeProduct(
+            name=u'embargoed',
+            information_type=InformationType.EMBARGOED,
+            owner=owner)
+        self.factory.makeProduct(
+            name=u'public',
+            information_type=InformationType.PUBLIC,
+            owner=owner)
+
+        expected_pillars = [u'embargoed', u'public']
+        received_pillars = [pillar.name for pillar in
+            owner.getAffiliatedPillars(owner)]
+        self.assertEqual(expected_pillars, received_pillars)
+
+    def test_getAffiliatedPillars_visible_to_admins(self):
+        # Users can see their own non-public affiliated products.
+        owner = self.factory.makePerson()
+        admin = self.factory.makeAdministrator()
+        self.factory.makeProduct(
+            name=u'embargoed',
+            information_type=InformationType.EMBARGOED,
+            owner=owner)
+        self.factory.makeProduct(
+            name=u'public',
+            information_type=InformationType.PUBLIC,
+            owner=owner)
+
+        expected_pillars = [u'embargoed', u'public']
+        received_pillars = [pillar.name for pillar in
+            owner.getAffiliatedPillars(admin)]
+        self.assertEqual(expected_pillars, received_pillars)
+
+    def test_getAffiliatedPillars_visible_to_commercial_admins(self):
+        # Users can see their own non-public affiliated products.
+        owner = self.factory.makePerson()
+        admin = self.factory.makeCommercialAdmin()
+        self.factory.makeProduct(
+            name=u'embargoed',
+            information_type=InformationType.EMBARGOED,
+            owner=owner)
+        self.factory.makeProduct(
+            name=u'public',
+            information_type=InformationType.PUBLIC,
+            owner=owner)
+
+        expected_pillars = [u'embargoed', u'public']
+        received_pillars = [pillar.name for pillar in
+            owner.getAffiliatedPillars(admin)]
         self.assertEqual(expected_pillars, received_pillars)
 
     def test_no_merge_pending(self):
@@ -675,6 +744,26 @@
         self.bzr = product_set.getByName('bzr')
         self.now = datetime.now(pytz.UTC)
 
+    def test_canDeactivateAccount_private_projects(self):
+        """A user owning non-public products cannot be deactivated."""
+        user = self.factory.makePerson()
+        public_product = self.factory.makeProduct(
+            information_type=InformationType.PUBLIC,
+            name="public",
+            owner=user)
+        public_product = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY,
+            name="private",
+            owner=user)
+
+        login(user.preferredemail.email)
+        can_deactivate, errors = user.canDeactivateAccountWithErrors()
+
+        self.assertFalse(can_deactivate)
+        expected_error = ('This account cannot be deactivated because it owns '
+                        'the following non-public products: private')
+        self.assertIn(expected_error, errors)
+
     def test_deactivateAccount_copes_with_names_already_in_use(self):
         """When a user deactivates his account, its name is changed.
 
@@ -710,12 +799,15 @@
         product = self.factory.makeProduct(owner=user)
         with person_logged_in(user):
             product.driver = user
+            product.bug_supervisor = user
             user.deactivateAccount("Going off the grid.")
         registry_team = getUtility(ILaunchpadCelebrities).registry_experts
         self.assertEqual(registry_team, product.owner,
                          "Owner is not registry team.")
-        self.assertEqual(registry_team, product.driver,
-                         "Driver is not registry team.")
+        self.assertEqual(None, product.driver,
+                         "Driver is not emptied.")
+        self.assertEqual(None, product.bug_supervisor,
+                         "Driver is not emptied.")
 
     def test_getDirectMemberIParticipateIn(self):
         sample_person = Person.byName('name12')

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-10-18 15:01:49 +0000
+++ lib/lp/registry/tests/test_product.py	2012-10-24 09:24:22 +0000
@@ -1866,6 +1866,22 @@
         clause = ProductSet.getProductPrivacyFilter(user)
         return IStore(Product).find(Product, clause)
 
+    def test_users_private_products(self):
+        # Ignore any public products the user may own.
+        owner = self.factory.makePerson()
+        public = self.factory.makeProduct(
+            information_type=InformationType.PUBLIC,
+            owner=owner)
+        proprietary = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY,
+            owner=owner)
+        embargoed = self.factory.makeProduct(
+            information_type=InformationType.EMBARGOED,
+            owner=owner)
+        result = ProductSet.get_users_private_products(owner)
+        self.assertIn(proprietary, result)
+        self.assertIn(embargoed, result)
+
     def test_get_all_active_omits_proprietary(self):
         # Ignore proprietary products for anonymous users
         proprietary = self.factory.makeProduct(