launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:sharing-limited-view into launchpad:master.

Commit message:
Ensure that private teams are visible on +sharing

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1913334 in Launchpad itself: "403 accessing sharing with private teams that I cannot view"
  https://bugs.launchpad.net/launchpad/+bug/1913334

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/397003

Product:+sharing and Distribution:+sharing are only visible to pillar drivers, who should be able to see who has access to their pillar.  However, it's possible to end up with a pillar having a grant to a private team that not all the drivers can see.  In some ways it might be best to grant LimitedView in this case, but doing that in the security adapter would be risky for performance as that adapter is already very complex and often slow.  Instead, precache LimitedView permissions for all grantees just for PillarSharingView.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:sharing-limited-view into launchpad:master.

diff --git a/lib/lp/registry/browser/pillar.py b/lib/lp/registry/browser/pillar.py
index 791b9a9..dd83648 100644
--- a/lib/lp/registry/browser/pillar.py
+++ b/lib/lp/registry/browser/pillar.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Common views for objects that implement `IPillar`."""
@@ -57,7 +57,10 @@ from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.registry.model.pillar import PillarPerson
 from lp.services.config import config
 from lp.services.propertycache import cachedproperty
-from lp.services.webapp.authorization import check_permission
+from lp.services.webapp.authorization import (
+    check_permission,
+    precache_permission_for_objects,
+    )
 from lp.services.webapp.batching import (
     BatchNavigator,
     get_batch_properties_for_json_cache,
@@ -376,6 +379,14 @@ class PillarSharingView(LaunchpadView):
         cache.objects['has_edit_permission'] = check_permission(
             "launchpad.Edit", self.context)
         batch_navigator = self.grantees()
+        # Precache LimitedView for all the grantees, partly for performance
+        # but mainly because it's possible that the user won't strictly have
+        # LimitedView on all of them and they should nevertheless be able to
+        # see who has access to pillars they drive.  Fixing this in
+        # PublicOrPrivateTeamsExistence would very likely be too expensive.
+        precache_permission_for_objects(
+            None, 'launchpad.LimitedView',
+            [grantee for grantee, _, _ in batch_navigator.batch])
         cache.objects['grantee_data'] = (
             self._getSharingService().jsonGranteeData(batch_navigator.batch))
         cache.objects.update(
diff --git a/lib/lp/registry/browser/tests/test_pillar_sharing.py b/lib/lp/registry/browser/tests/test_pillar_sharing.py
index 0fc83ef..493b42b 100644
--- a/lib/lp/registry/browser/tests/test_pillar_sharing.py
+++ b/lib/lp/registry/browser/tests/test_pillar_sharing.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2012-2021 Canonical Ltd. This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test views that manage sharing."""
@@ -23,6 +23,7 @@ from lp.app.interfaces.services import IService
 from lp.registry.enums import (
     BranchSharingPolicy,
     BugSharingPolicy,
+    PersonVisibility,
     )
 from lp.registry.interfaces.accesspolicy import IAccessPolicyGrantFlatSource
 from lp.registry.model.pillar import PillarPerson
@@ -31,6 +32,7 @@ from lp.services.config import config
 from lp.services.webapp.interfaces import StormRangeFactoryError
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
+    admin_logged_in,
     login_person,
     logout,
     normalize_whitespace,
@@ -361,6 +363,22 @@ class PillarSharingViewTestMixin:
         self.run_sharing_message_test(
             self.pillar, self.pillar.owner, public=True)
 
+    def test_shared_with_normally_invisible_private_team(self):
+        # If a pillar is shared with a private team, then we disclose
+        # information about the share to users who can see +sharing even if
+        # they can't normally see that private team.
+        self.pushConfig('launchpad', default_batch_size=75)
+        with admin_logged_in():
+            team = self.factory.makeTeam(visibility=PersonVisibility.PRIVATE)
+            team_name = team.name
+            self.factory.makeAccessPolicyGrant(self.access_policy, team)
+        with person_logged_in(self.pillar.owner):
+            view = create_initialized_view(self.pillar, name='+sharing')
+            cache = IJSONRequestCache(view.request)
+            self.assertIn(
+                team_name,
+                [grantee['name'] for grantee in cache.objects['grantee_data']])
+
 
 class TestProductSharingView(PillarSharingViewTestMixin,
                                  SharingBaseTestCase):