launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/product-lp-limitedview-3-1 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/product-lp-limitedview-3-1/+merge/134075

This branch changes the permission check in
LaunchpadRootNavigation.traverse() for products.

If the current request URL is related to pillar, i.e., if it does
not start with a '~', traverse() tries to find a pillar with
the given name.

If the pillar exists and if the current user has the permission
launchpad.View for the pillar, it is returned by the method.
Otherwise, None is returned.

The permission current check "may the user view this product?" does
not work correctly in the following situation:

- The URL points to a branch, bug or specification of a proprietary
  or embargoed product
- The current user has a artifact grant for the branch/bug/spec
  but does not have a policy grant for the product.

In this case, a user has the permission launchpad.Limitedview for
the product, but not the permission launchpad.View, so traverse()
should chekc if the user has the permission lp.LimitedView.
(This permission is required for attributes like IProduct.name or
IProduct.displayname, which are shown on bug/branch/spec related
pages.)

This check is similar to that for private teams, a few lines up
in the same method.

So the core change is simply
s/check_permission('launchpad.View', pillar)/check_permission('launchpad.LimitedView', pillar)

The method already had a special rule for inactive products:

-        if (pillar is not None and IProduct.providedBy(pillar) and
-            not pillar.active):

In order to keep the if (...) expressions readable, I refactored the
checks "pillar is None", "IProduct.providedBy(pillar)" and
"pillar.active".

test:

./bin/test app -vvt test_access_for_persons_with_artifact_grant

no lint

-- 
https://code.launchpad.net/~adeuring/launchpad/product-lp-limitedview-3-1/+merge/134075
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/product-lp-limitedview-3-1 into lp:launchpad.

=== modified file 'lib/lp/app/browser/launchpad.py'
--- lib/lp/app/browser/launchpad.py	2012-11-11 23:39:34 +0000
+++ lib/lp/app/browser/launchpad.py	2012-11-13 11:12:26 +0000
@@ -766,29 +766,35 @@
         pillar = getUtility(IPillarNameSet).getByName(
             name, ignore_inactive=False)
 
-        if (pillar is not None and IProduct.providedBy(pillar) and
-            not pillar.active):
-            # Emergency brake for public but inactive products:
-            # These products should not be shown to ordinary users.
-            # The root problem is that many views iterate over products,
-            # inactive products included, and access attributes like
-            # name, displayname or call canonical_url(product) --
-            # and finally throw the data away, if the product is
-            # inactive. So we cannot make these attributes inaccessible
-            # for inactive public products. On the other hand, we
-            # require the permission launchpad.View to protect private
-            # products.
-            # This means that we cannot simply check if the current
-            # user has the permission launchpad.View for an inactive
-            # product.
-            user = getUtility(ILaunchBag).user
-            if user is None:
-                return None
-            user = IPersonRoles(user)
-            if (not user.in_commercial_admin and not user.in_admin and
-                not user.in_registry_experts):
-                return None
-        if pillar is not None and check_permission('launchpad.View', pillar):
+        if pillar is None:
+            return None
+
+        if IProduct.providedBy(pillar):
+            if not pillar.active:
+                # Emergency brake for public but inactive products:
+                # These products should not be shown to ordinary users.
+                # The root problem is that many views iterate over products,
+                # inactive products included, and access attributes like
+                # name, displayname or call canonical_url(product) --
+                # and finally throw the data away, if the product is
+                # inactive. So we cannot make these attributes inaccessible
+                # for inactive public products. On the other hand, we
+                # require the permission launchpad.View to protect private
+                # products.
+                # This means that we cannot simply check if the current
+                # user has the permission launchpad.View for an inactive
+                # product.
+                user = getUtility(ILaunchBag).user
+                if user is None:
+                    return None
+                user = IPersonRoles(user)
+                if (not user.in_commercial_admin and not user.in_admin and
+                    not user.in_registry_experts):
+                    return None
+            permitted = check_permission('launchpad.LimitedView', pillar)
+        else:
+            permitted = check_permission('launchpad.View', pillar)
+        if permitted:
             if pillar.name != name:
                 # This pillar was accessed through one of its aliases, so we
                 # must redirect to its canonical URL.

=== modified file 'lib/lp/app/browser/tests/test_launchpad.py'
--- lib/lp/app/browser/tests/test_launchpad.py	2012-11-11 23:39:34 +0000
+++ lib/lp/app/browser/tests/test_launchpad.py	2012-11-13 11:12:26 +0000
@@ -554,6 +554,19 @@
             self.assertRaises(
                 NotFound, self.traverse_to_inactive_proprietary_product)
 
+    def test_access_for_persons_with_artifact_grant(self):
+        # Persons with an artifact grant related to a private product
+        # can traverse the product.
+        user = self.factory.makePerson()
+        with person_logged_in(self.proprietary_product_owner):
+            bug = self.factory.makeBug(
+                target=self.active_proprietary_product,
+                information_type=InformationType.PROPRIETARY)
+            getUtility(IService, 'sharing').ensureAccessGrants(
+                [user], self.proprietary_product_owner, bugs=[bug])
+        with person_logged_in(user):
+            self.traverse_to_active_proprietary_product()
+
     def check_admin_access(self):
         self.traverse_to_active_public_product()
         self.traverse_to_inactive_public_product()