launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:webhook-team-owned-private-artifacts into launchpad:master.

Commit message:
Fix webhooks for team-owned private targets

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1872740 in Launchpad itself: "webhooks don't fire for team-owned private webhook targets"
  https://bugs.launchpad.net/launchpad/+bug/1872740

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

In order to cope with webhook targets that use DelegatedAuthorization-based security adapters, we had to switch to using iter_authorization rather than looking up the security adapter directly.  However, this breaks for private webhook targets that are owned by a team, because teams don't have an account so we can't construct a normal principal for them.  There's no appropriate principal here, because we need to know about things that are visible to the whole team in this case, not just to any particular member of the team.

However, iter_authorization only uses the principal to get hold of a corresponding IPersonRoles, and most security adapters work just fine on teams; so, to simplify all this and make it work in this case, we can just pass an IPersonRoles directly.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:webhook-team-owned-private-artifacts into launchpad:master.

diff --git a/lib/lp/services/webapp/authorization.py b/lib/lp/services/webapp/authorization.py
index 5dc4ba5..aaaa55a 100644
--- a/lib/lp/services/webapp/authorization.py
+++ b/lib/lp/services/webapp/authorization.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -285,6 +285,11 @@ def iter_authorization(objecttoauthorize, permission, principal, cache,
     if ILaunchpadPrincipal.providedBy(principal):
         check_auth = lambda authorization: (
             authorization.checkAuthenticated(IPersonRoles(principal.person)))
+    elif IPersonRoles.providedBy(principal):
+        # In some cases it's cumbersome to get hold of a proper principal,
+        # so also allow passing an IPersonRoles directly.
+        check_auth = lambda authorization: (
+            authorization.checkAuthenticated(principal))
     else:
         check_auth = lambda authorization: (
             authorization.checkUnauthenticated())
diff --git a/lib/lp/services/webhooks/model.py b/lib/lp/services/webhooks/model.py
index e60975f..a670d54 100644
--- a/lib/lp/services/webhooks/model.py
+++ b/lib/lp/services/webhooks/model.py
@@ -42,6 +42,7 @@ from zope.interface import (
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app import versioninfo
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.model.person import Person
 from lp.services.config import config
 from lp.services.database.bulk import load_related
@@ -61,7 +62,6 @@ from lp.services.job.model.job import (
 from lp.services.job.runner import BaseRunnableJob
 from lp.services.scripts import log
 from lp.services.webapp.authorization import iter_authorization
-from lp.services.webapp.interfaces import IPlacelessAuthUtility
 from lp.services.webhooks.interfaces import (
     IWebhook,
     IWebhookClient,
@@ -255,10 +255,9 @@ class WebhookSet:
         :return: True if the context is visible to the webhook owner,
             otherwise False.
         """
-        authutil = getUtility(IPlacelessAuthUtility)
         return all(iter_authorization(
             removeSecurityProxy(context), "launchpad.View",
-            authutil.getPrincipal(user.accountID), {}))
+            IPersonRoles(user), {}))
 
     def trigger(self, target, event_type, payload, context=None):
         if context is None:
diff --git a/lib/lp/services/webhooks/tests/test_model.py b/lib/lp/services/webhooks/tests/test_model.py
index 08b72fc..cc0c5a5 100644
--- a/lib/lp/services/webhooks/tests/test_model.py
+++ b/lib/lp/services/webhooks/tests/test_model.py
@@ -206,7 +206,6 @@ class TestWebhookSetBase:
 
     def test__checkVisibility_public_artifact(self):
         target = self.makeTarget()
-        login_person(target.owner)
         self.assertTrue(WebhookSet._checkVisibility(target, target.owner))
 
     def test_trigger(self):
@@ -252,7 +251,12 @@ class TestWebhookSetMergeProposalBase(TestWebhookSetBase):
         owner = self.factory.makePerson()
         target = self.makeTarget(
             owner=owner, information_type=InformationType.PROPRIETARY)
-        login_person(owner)
+        self.assertTrue(WebhookSet._checkVisibility(target, owner))
+
+    def test__checkVisibility_private_artifact_team_owned(self):
+        owner = self.factory.makeTeam()
+        target = self.makeTarget(
+            owner=owner, information_type=InformationType.PROPRIETARY)
         self.assertTrue(WebhookSet._checkVisibility(target, owner))
 
     def test__checkVisibility_lost_access_to_private_artifact(self):
@@ -269,9 +273,9 @@ class TestWebhookSetMergeProposalBase(TestWebhookSetBase):
             policy=policy, grantee=grantee_team)
         grantee_member = self.factory.makePerson(member_of=[grantee_team])
         target = self.makeTarget(owner=grantee_member, project=project)
-        login_person(grantee_member)
         self.assertTrue(WebhookSet._checkVisibility(target, grantee_member))
-        grantee_member.leave(grantee_team)
+        with person_logged_in(grantee_member):
+            grantee_member.leave(grantee_team)
         self.assertFalse(WebhookSet._checkVisibility(target, grantee_member))
 
     def test__checkVisibility_with_different_context(self):
@@ -286,7 +290,6 @@ class TestWebhookSetMergeProposalBase(TestWebhookSetBase):
             owner=owner, project=project, source=source, reviewer=reviewer)
         mp2 = self.makeMergeProposal(
             project=project, source=source, reviewer=reviewer)
-        login_person(owner)
         self.assertTrue(
             WebhookSet._checkVisibility(mp1, mp1.merge_target.owner))
         self.assertFalse(
@@ -343,7 +346,6 @@ class TestWebhookSetMergeProposalBase(TestWebhookSetBase):
         event_type = 'merge-proposal:0.1'
         hook = self.factory.makeWebhook(
             target=target, event_types=[event_type])
-        login_person(source.owner)
         getUtility(IWebhookSet).trigger(
             target, event_type, {'some': 'payload'}, context=mp)
         with admin_logged_in():