launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19591
[Merge] lp:~cjwatson/launchpad/bmp-webhooks into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/bmp-webhooks into lp:launchpad with lp:~cjwatson/launchpad/no-excess-aags as a prerequisite.
Commit message:
Add webhook support for merge proposals.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/bmp-webhooks/+merge/274005
Add webhook support for merge proposals. They are only dispatched if the webhook owner can see both the source and the target.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/bmp-webhooks into lp:launchpad.
=== modified file 'lib/lp/code/adapters/branch.py'
--- lib/lp/code/adapters/branch.py 2015-07-08 16:05:11 +0000
+++ lib/lp/code/adapters/branch.py 2015-10-09 16:59:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Components related to branches."""
@@ -23,6 +23,7 @@
IBranchDelta,
)
from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
+from lp.services.webhooks.payload import compose_webhook_payload
# XXX: thumper 2006-12-20: This needs to be extended
# to cover bugs and specs linked and unlinked, as
@@ -80,8 +81,11 @@
delta_values = (
'registrant',
'source_branch',
+ 'source_git_ref',
'target_branch',
+ 'target_git_ref',
'prerequisite_branch',
+ 'prerequisite_git_ref',
'queue_status',
)
new_values = (
@@ -111,6 +115,17 @@
return BranchMergeProposalDelta(**delta.changes)
@classmethod
+ def composeWebhookPayload(klass, old_merge_proposal, new_merge_proposal):
+ """Return part of a webhook payload representing the differences."""
+ return {
+ "old": compose_webhook_payload(
+ IBranchMergeProposal, old_merge_proposal, klass.delta_values),
+ "new": compose_webhook_payload(
+ IBranchMergeProposal, new_merge_proposal,
+ klass.delta_values + klass.new_values),
+ }
+
+ @classmethod
def snapshot(klass, merge_proposal):
"""Return a snapshot suitable for use with construct.
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2015-10-06 16:09:06 +0000
+++ lib/lp/code/configure.zcml 2015-10-09 16:59:27 +0000
@@ -291,6 +291,10 @@
handler="lp.code.subscribers.branchmergeproposal.merge_proposal_modified"/>
<subscriber
for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposal
+ lazr.lifecycle.interfaces.IObjectDeletedEvent"
+ handler="lp.code.subscribers.branchmergeproposal.merge_proposal_deleted"/>
+ <subscriber
+ for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposal
lazr.lifecycle.interfaces.IObjectCreatedEvent"
handler="lp.code.subscribers.karma.branch_merge_proposed"/>
<subscriber
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2015-06-12 03:48:40 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2015-10-09 16:59:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""The interface for branch merge proposals."""
@@ -7,6 +7,7 @@
__all__ = [
'BRANCH_MERGE_PROPOSAL_FINAL_STATES',
'BRANCH_MERGE_PROPOSAL_OBSOLETE_STATES',
+ 'BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG',
'IBranchMergeProposal',
'IBranchMergeProposalGetter',
'IBranchMergeProposalJob',
@@ -104,6 +105,10 @@
)
+BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG = (
+ "merge_proposals.webhooks.enabled")
+
+
class IBranchMergeProposalPublic(IPrivacy):
id = Int(
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2015-10-06 16:08:27 +0000
+++ lib/lp/code/model/branch.py 2015-10-09 16:59:27 +0000
@@ -206,6 +206,10 @@
@property
def valid_webhook_event_types(self):
+ return ["bzr:push:0.1", "merge-proposal:0.1"]
+
+ @property
+ def default_webhook_event_types(self):
return ["bzr:push:0.1"]
@property
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2015-10-06 16:09:06 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2015-10-09 16:59:27 +0000
@@ -12,7 +12,10 @@
from email.utils import make_msgid
-from lazr.lifecycle.event import ObjectCreatedEvent
+from lazr.lifecycle.event import (
+ ObjectCreatedEvent,
+ ObjectDeletedEvent,
+ )
from sqlobject import (
ForeignKey,
IntCol,
@@ -777,6 +780,7 @@
branch_merge_proposal=self.id):
job.destroySelf()
self._preview_diffs.remove()
+ notify(ObjectDeletedEvent(self))
self.destroySelf()
def getUnlandedSourceBranchRevisions(self):
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2015-10-05 11:11:47 +0000
+++ lib/lp/code/model/gitrepository.py 2015-10-09 16:59:27 +0000
@@ -236,6 +236,10 @@
@property
def valid_webhook_event_types(self):
+ return ["git:push:0.1", "merge-proposal:0.1"]
+
+ @property
+ def default_webhook_event_types(self):
return ["git:push:0.1"]
# Marker for references to Git URL layouts: ##GITNAMESPACE##
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2015-10-06 16:09:06 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2015-10-09 16:59:27 +0000
@@ -20,12 +20,18 @@
from pytz import UTC
from sqlobject import SQLObjectNotFound
from storm.locals import Store
+from testtools.matchers import (
+ Equals,
+ MatchesDict,
+ MatchesStructure,
+ )
import transaction
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
from lp.app.enums import InformationType
from lp.app.interfaces.launchpad import IPrivacy
+from lp.code.adapters.branch import BranchMergeProposalNoPreviewDiffDelta
from lp.code.enums import (
BranchMergeProposalStatus,
BranchSubscriptionDiffSize,
@@ -46,6 +52,7 @@
from lp.code.interfaces.branchmergeproposal import (
BRANCH_MERGE_PROPOSAL_FINAL_STATES as FINAL_STATES,
BRANCH_MERGE_PROPOSAL_OBSOLETE_STATES as OBSOLETE_STATES,
+ BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG,
IBranchMergeProposal,
IBranchMergeProposalGetter,
notify_modified,
@@ -67,6 +74,7 @@
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.product import IProductSet
from lp.services.database.constants import UTC_NOW
+from lp.services.features.testing import FeatureFixture
from lp.services.webapp import canonical_url
from lp.testing import (
ExpectedException,
@@ -921,6 +929,143 @@
self.assertIn(charlie, recipients)
+class TestMergeProposalWebhooksMixin:
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestMergeProposalWebhooksMixin, self).setUp()
+ self.useFixture(FeatureFixture(
+ {BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"}))
+
+ @staticmethod
+ def getURL(obj):
+ if obj is not None:
+ return canonical_url(obj, force_local_path=True)
+ else:
+ return None
+
+ @classmethod
+ def getExpectedPayload(cls, proposal, include_new_values=True):
+ payload = {
+ "registrant": "/~%s" % proposal.registrant.name,
+ "source_branch": cls.getURL(proposal.source_branch),
+ "source_git_ref": cls.getURL(proposal.source_git_ref),
+ "target_branch": cls.getURL(proposal.target_branch),
+ "target_git_ref": cls.getURL(proposal.target_git_ref),
+ "prerequisite_branch": cls.getURL(proposal.prerequisite_branch),
+ "prerequisite_git_ref": cls.getURL(proposal.prerequisite_git_ref),
+ "queue_status": proposal.queue_status.title,
+ }
+ if include_new_values:
+ payload.update({
+ "commit_message": proposal.commit_message,
+ "whiteboard": proposal.whiteboard,
+ "description": proposal.description,
+ "preview_diff": cls.getURL(proposal.preview_diff),
+ })
+ return {k: Equals(v) for k, v in payload.items()}
+
+ def assertCorrectDelivery(self, expected_payload, delivery):
+ self.assertThat(
+ delivery, MatchesStructure(
+ event_type=Equals("merge-proposal:0.1"),
+ payload=MatchesDict(expected_payload)))
+
+ def test_create_triggers_webhooks(self):
+ # When a merge proposal is created, any relevant webhooks are
+ # triggered.
+ source = self.makeBranch()
+ target = self.makeBranch(same_target_as=source)
+ registrant = self.factory.makePerson()
+ hook = self.factory.makeWebhook(
+ target=self.getWebhookTarget(target),
+ event_types=["merge-proposal:0.1"])
+ proposal = source.addLandingTarget(
+ registrant, target, needs_review=True)
+ login_person(target.owner)
+ delivery = hook.deliveries.one()
+ expected_payload = {
+ "merge_proposal": Equals(self.getURL(proposal)),
+ "action": Equals("created"),
+ }
+ expected_payload.update(self.getExpectedPayload(proposal))
+ self.assertCorrectDelivery(expected_payload, delivery)
+
+ def test_modify_triggers_webhooks(self):
+ # When an existing merge proposal is modified, any relevant webhooks
+ # are triggered.
+ source = self.makeBranch()
+ target = self.makeBranch(same_target_as=source)
+ registrant = self.factory.makePerson()
+ proposal = source.addLandingTarget(
+ registrant, target, needs_review=True)
+ hook = self.factory.makeWebhook(
+ target=self.getWebhookTarget(target),
+ event_types=["merge-proposal:0.1"])
+ login_person(target.owner)
+ expected_payload = {
+ "merge_proposal": Equals(self.getURL(proposal)),
+ "action": Equals("modified"),
+ }
+ expected_payload["old"] = MatchesDict(
+ self.getExpectedPayload(proposal, include_new_values=False))
+ with BranchMergeProposalNoPreviewDiffDelta.monitor(proposal):
+ proposal.setStatus(
+ BranchMergeProposalStatus.CODE_APPROVED, user=target.owner)
+ proposal.description = "An excellent proposal."
+ expected_payload["new"] = MatchesDict(
+ self.getExpectedPayload(proposal))
+ delivery = hook.deliveries.one()
+ self.assertCorrectDelivery(expected_payload, delivery)
+
+ def test_delete_triggers_webhooks(self):
+ # When an existing merge proposal is deleted, any relevant webhooks
+ # are triggered.
+ source = self.makeBranch()
+ target = self.makeBranch(same_target_as=source)
+ registrant = self.factory.makePerson()
+ proposal = source.addLandingTarget(
+ registrant, target, needs_review=True)
+ hook = self.factory.makeWebhook(
+ target=self.getWebhookTarget(target),
+ event_types=["merge-proposal:0.1"])
+ login_person(target.owner)
+ expected_payload = {
+ "merge_proposal": Equals(self.getURL(proposal)),
+ "action": Equals("deleted"),
+ }
+ proposal.deleteProposal()
+ delivery = hook.deliveries.one()
+ self.assertCorrectDelivery(expected_payload, delivery)
+
+
+class TestMergeProposalWebhooksBzr(
+ TestMergeProposalWebhooksMixin, TestCaseWithFactory):
+
+ def makeBranch(self, same_target_as=None):
+ kwargs = {}
+ if same_target_as is not None:
+ kwargs["product"] = same_target_as.product
+ return self.factory.makeProductBranch(**kwargs)
+
+ def getWebhookTarget(self, branch):
+ return branch
+
+
+class TestMergeProposalWebhooksGit(
+ TestMergeProposalWebhooksMixin, TestCaseWithFactory):
+
+ def makeBranch(self, same_target_as=None):
+ kwargs = {}
+ if same_target_as is not None:
+ kwargs["target"] = same_target_as.target
+ return self.factory.makeGitRefs(**kwargs)[0]
+
+ def getWebhookTarget(self, branch):
+ return branch.repository
+
+
class TestGetAddress(TestCaseWithFactory):
"""Test that the address property gives expected results."""
=== modified file 'lib/lp/code/subscribers/branchmergeproposal.py'
--- lib/lp/code/subscribers/branchmergeproposal.py 2015-10-06 15:15:45 +0000
+++ lib/lp/code/subscribers/branchmergeproposal.py 2015-10-09 16:59:27 +0000
@@ -5,20 +5,42 @@
__metaclass__ = type
-
from zope.component import getUtility
from zope.principalregistry.principalregistry import UnauthenticatedPrincipal
-from lp.code.adapters.branch import BranchMergeProposalNoPreviewDiffDelta
+from lp.code.adapters.branch import (
+ BranchMergeProposalDelta,
+ BranchMergeProposalNoPreviewDiffDelta,
+ )
from lp.code.enums import BranchMergeProposalStatus
from lp.code.interfaces.branchmergeproposal import (
+ BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG,
+ IBranchMergeProposal,
IMergeProposalNeedsReviewEmailJobSource,
IMergeProposalUpdatedEmailJobSource,
IReviewRequestedEmailJobSource,
IUpdatePreviewDiffJobSource,
)
from lp.registry.interfaces.person import IPerson
+from lp.services.features import getFeatureFlag
from lp.services.utils import text_delta
+from lp.services.webapp.publisher import canonical_url
+from lp.services.webhooks.interfaces import IWebhookSet
+from lp.services.webhooks.payload import compose_webhook_payload
+
+
+def _trigger_webhook(merge_proposal, payload):
+ payload = dict(payload)
+ payload["merge_proposal"] = canonical_url(
+ merge_proposal, force_local_path=True)
+ if merge_proposal.target_branch is not None:
+ source = merge_proposal.source_branch
+ target = merge_proposal.target_branch
+ else:
+ source = merge_proposal.source_git_repository
+ target = merge_proposal.target_git_repository
+ getUtility(IWebhookSet).trigger(
+ target, "merge-proposal:0.1", payload, source=source)
def merge_proposal_created(merge_proposal, event):
@@ -28,6 +50,13 @@
Also create a job to email the subscribers about the new proposal.
"""
getUtility(IUpdatePreviewDiffJobSource).create(merge_proposal)
+ if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
+ payload = {"action": "created"}
+ payload.update(compose_webhook_payload(
+ IBranchMergeProposal, merge_proposal,
+ BranchMergeProposalDelta.delta_values +
+ BranchMergeProposalDelta.new_values))
+ _trigger_webhook(merge_proposal, payload)
def merge_proposal_needs_review(merge_proposal, event):
@@ -49,9 +78,6 @@
from_person = None
else:
from_person = IPerson(event.user)
- # If the merge proposal was work in progress and is now needs review,
- # then we don't want to send out an email as the needs review email will
- # cover that.
old_status = event.object_before_modification.queue_status
new_status = merge_proposal.queue_status
@@ -59,20 +85,26 @@
BranchMergeProposalStatus.WORK_IN_PROGRESS,
BranchMergeProposalStatus.NEEDS_REVIEW)
- if (old_status == BranchMergeProposalStatus.WORK_IN_PROGRESS and
- new_status in in_progress_states):
- return
- # Create a delta of the changes. If there are no changes to report, then
- # we're done.
- delta = BranchMergeProposalNoPreviewDiffDelta.construct(
- event.object_before_modification, merge_proposal)
- if delta is None:
- return
- changes = text_delta(
- delta, delta.delta_values, delta.new_values, delta.interface)
- # Now create the job to send the email.
- getUtility(IMergeProposalUpdatedEmailJobSource).create(
- merge_proposal, changes, from_person)
+ # If the merge proposal was work in progress and is now needs review,
+ # then we don't want to send out an email as the needs review email will
+ # cover that.
+ if (old_status != BranchMergeProposalStatus.WORK_IN_PROGRESS or
+ new_status not in in_progress_states):
+ # Create a delta of the changes. If there are no changes to report,
+ # then we're done.
+ delta = BranchMergeProposalNoPreviewDiffDelta.construct(
+ event.object_before_modification, merge_proposal)
+ if delta is not None:
+ changes = text_delta(
+ delta, delta.delta_values, delta.new_values, delta.interface)
+ # Now create the job to send the email.
+ getUtility(IMergeProposalUpdatedEmailJobSource).create(
+ merge_proposal, changes, from_person)
+ if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
+ payload = {"action": "modified"}
+ payload.update(BranchMergeProposalDelta.composeWebhookPayload(
+ event.object_before_modification, merge_proposal))
+ _trigger_webhook(merge_proposal, payload)
def review_requested(vote_reference, event):
@@ -81,3 +113,12 @@
bmp_status = vote_reference.branch_merge_proposal.queue_status
if bmp_status != BranchMergeProposalStatus.WORK_IN_PROGRESS:
getUtility(IReviewRequestedEmailJobSource).create(vote_reference)
+
+
+def merge_proposal_deleted(merge_proposal, event):
+ """A merge proposal has been deleted."""
+ if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
+ # The merge proposal link will be invalid by the time the webhook is
+ # delivered, but this may still be useful for endpoints that might
+ # e.g. want to cancel CI jobs in flight.
+ _trigger_webhook(merge_proposal, {"action": "deleted"})
=== modified file 'lib/lp/services/webhooks/configure.zcml'
--- lib/lp/services/webhooks/configure.zcml 2015-10-01 12:24:36 +0000
+++ lib/lp/services/webhooks/configure.zcml 2015-10-09 16:59:27 +0000
@@ -61,6 +61,13 @@
factory="lp.services.webhooks.client.WebhookClient"
permission="zope.Public"/>
+ <adapter
+ for="zope.interface.Interface
+ lp.services.webhooks.interfaces.IWebhookPayloadRequest"
+ provides="zope.traversing.browser.interfaces.IAbsoluteURL"
+ factory="lp.services.webhooks.payload.WebhookAbsoluteURL"
+ />
+
<browser:url
for="lp.services.webhooks.interfaces.IWebhook"
path_expression="string:+webhook/${id}"
=== modified file 'lib/lp/services/webhooks/interfaces.py'
--- lib/lp/services/webhooks/interfaces.py 2015-10-05 11:11:47 +0000
+++ lib/lp/services/webhooks/interfaces.py 2015-10-09 16:59:27 +0000
@@ -13,6 +13,7 @@
'IWebhookDeliveryJobSource',
'IWebhookJob',
'IWebhookJobSource',
+ 'IWebhookPayloadRequest',
'IWebhookSet',
'IWebhookTarget',
'WEBHOOK_EVENT_TYPES',
@@ -65,6 +66,7 @@
IJobSource,
IRunnableJob,
)
+from lp.services.webapp.interfaces import ILaunchpadBrowserApplicationRequest
from lp.services.webservice.apihelpers import (
patch_collection_property,
patch_entry_return_type,
@@ -75,6 +77,7 @@
WEBHOOK_EVENT_TYPES = {
"bzr:push:0.1": "Bazaar push",
"git:push:0.1": "Git push",
+ "merge-proposal:0.1": "Merge proposal",
}
@@ -199,7 +202,7 @@
def findByTarget(target):
"""Find all webhooks for the given target."""
- def trigger(target, event_type, payload):
+ def trigger(target, event_type, payload, source=None):
"""Trigger subscribed webhooks to deliver a payload."""
@@ -359,6 +362,11 @@
header will be sent using HMAC-SHA1.
"""
+
+class IWebhookPayloadRequest(ILaunchpadBrowserApplicationRequest):
+ """An internal fake request used while composing webhook payloads."""
+
+
patch_collection_property(IWebhook, 'deliveries', IWebhookDeliveryJob)
patch_entry_return_type(IWebhook, 'ping', IWebhookDeliveryJob)
patch_reference_property(IWebhook, 'target', IWebhookTarget)
=== modified file 'lib/lp/services/webhooks/model.py'
--- lib/lp/services/webhooks/model.py 2015-10-05 11:11:47 +0000
+++ lib/lp/services/webhooks/model.py 2015-10-09 16:59:27 +0000
@@ -204,10 +204,47 @@
return IStore(Webhook).find(Webhook, target_filter).order_by(
Webhook.id)
- def trigger(self, target, event_type, payload):
- # XXX wgrant 2015-08-10: Two INSERTs and one celery submission
- # for each webhook, but the set should be small and we'd have to
- # defer the triggering itself to a job to fix it.
+ @classmethod
+ def _checkVisibility(cls, target, source=None):
+ """Check visibility of webhook context objects.
+
+ In order to be able to dispatch a webhook without disclosing
+ unauthorised information, the webhook owner (currently always equal
+ to the webhook target owner) must be able to see the webhook target.
+ If deliveries are being triggered due to a change to some different
+ source object, then the webhook owner must also be able to see that
+ source.
+
+ :return: True if all objects are visible to the webhook owner,
+ otherwise False.
+ """
+ from lp.code.interfaces.branch import IBranch
+ from lp.code.interfaces.gitrepository import IGitRepository
+ owner = removeSecurityProxy(target).owner
+ if IGitRepository.providedBy(target):
+ if not removeSecurityProxy(target).visibleByUser(owner):
+ return False
+ if source is not None:
+ assert IGitRepository.providedBy(source)
+ if not removeSecurityProxy(source).visibleByUser(owner):
+ return False
+ elif IBranch.providedBy(target):
+ if not removeSecurityProxy(target).visibleByUser(owner):
+ return False
+ if source is not None:
+ assert IBranch.providedBy(source)
+ if not removeSecurityProxy(source).visibleByUser(owner):
+ return False
+ else:
+ raise AssertionError("Unsupported target: %r" % (target,))
+ return True
+
+ def trigger(self, target, event_type, payload, source=None):
+ if not self._checkVisibility(target, source=source):
+ return
+ # XXX wgrant 2015-08-10: Two INSERTs and one celery submission for
+ # each webhook, but the set should be small and we'd have to defer
+ # the triggering itself to a job to fix it.
for webhook in self.findByTarget(target):
if webhook.active and event_type in webhook.event_types:
WebhookDeliveryJob.create(webhook, event_type, payload)
=== added file 'lib/lp/services/webhooks/payload.py'
--- lib/lp/services/webhooks/payload.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/webhooks/payload.py 2015-10-09 16:59:27 +0000
@@ -0,0 +1,73 @@
+# Copyright 2015 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+__all__ = [
+ 'compose_webhook_payload',
+ 'WebhookAbsoluteURL',
+ 'WebhookPayloadRequest',
+ ]
+
+import StringIO
+
+from lazr.restful.interfaces import IFieldMarshaller
+from zope.component import getMultiAdapter
+from zope.interface import implementer
+from zope.traversing.browser.interfaces import IAbsoluteURL
+
+from lp.services.webapp.publisher import canonical_url
+from lp.services.webapp.servers import LaunchpadBrowserRequest
+from lp.services.webhooks.interfaces import IWebhookPayloadRequest
+
+
+@implementer(IWebhookPayloadRequest)
+class WebhookPayloadRequest(LaunchpadBrowserRequest):
+ """An internal fake request used while composing webhook payloads."""
+
+ def __init__(self):
+ super(WebhookPayloadRequest, self).__init__(StringIO.StringIO(), {})
+
+
+@implementer(IAbsoluteURL)
+class WebhookAbsoluteURL:
+ """A variant of CanonicalAbsoluteURL that always forces a local path."""
+
+ def __init__(self, context, request):
+ """Initialize with respect to a context and request."""
+ self.context = context
+ self.request = request
+
+ def __unicode__(self):
+ """Returns the URL as a unicode string."""
+ raise NotImplementedError()
+
+ def __str__(self):
+ """Returns an ASCII string with all unicode characters url quoted."""
+ return canonical_url(self.context, force_local_path=True)
+
+ def __repr__(self):
+ """Get a string representation """
+ raise NotImplementedError()
+
+ __call__ = __str__
+
+
+def compose_webhook_payload(interface, obj, names):
+ """Compose a webhook payload dictionary from some fields of an object.
+
+ Fields are serialised in the same way that lazr.restful does for
+ webservice-exported objects, except that object paths are always local.
+
+ :param interface: The interface of the object to serialise.
+ :param obj: The object to serialise.
+ :param names: A list of fields from `obj` to serialise.
+ """
+ payload = {}
+ request = WebhookPayloadRequest()
+ for name in names:
+ field = interface[name]
+ marshaller = getMultiAdapter((field, request), IFieldMarshaller)
+ value = getattr(obj, name, None)
+ payload[name] = marshaller.unmarshall(field, value)
+ return payload
=== modified file 'lib/lp/services/webhooks/tests/test_browser.py'
--- lib/lp/services/webhooks/tests/test_browser.py 2015-10-01 12:24:36 +0000
+++ lib/lp/services/webhooks/tests/test_browser.py 2015-10-09 16:59:27 +0000
@@ -52,7 +52,10 @@
class GitRepositoryTestHelpers:
event_type = "git:push:0.1"
- expected_event_types = [("git:push:0.1", "Git push")]
+ expected_event_types = [
+ ("git:push:0.1", "Git push"),
+ ("merge-proposal:0.1", "Merge proposal"),
+ ]
def makeTarget(self):
return self.factory.makeGitRepository()
@@ -61,7 +64,10 @@
class BranchTestHelpers:
event_type = "bzr:push:0.1"
- expected_event_types = [("bzr:push:0.1", "Bazaar push")]
+ expected_event_types = [
+ ("bzr:push:0.1", "Bazaar push"),
+ ("merge-proposal:0.1", "Merge proposal"),
+ ]
def makeTarget(self):
return self.factory.makeBranch()
=== modified file 'lib/lp/services/webhooks/tests/test_model.py'
--- lib/lp/services/webhooks/tests/test_model.py 2015-09-24 13:44:02 +0000
+++ lib/lp/services/webhooks/tests/test_model.py 2015-10-09 16:59:27 +0000
@@ -13,13 +13,18 @@
from zope.event import notify
from zope.security.checker import getChecker
+from lp.app.enums import InformationType
+from lp.registry.enums import BranchSharingPolicy
from lp.services.database.interfaces import IStore
from lp.services.webapp.authorization import check_permission
from lp.services.webhooks.interfaces import (
IWebhook,
IWebhookSet,
)
-from lp.services.webhooks.model import WebhookJob
+from lp.services.webhooks.model import (
+ WebhookJob,
+ WebhookSet,
+ )
from lp.testing import (
admin_logged_in,
anonymous_logged_in,
@@ -193,6 +198,50 @@
self.assertEqual(1, IStore(WebhookJob).find(WebhookJob).count())
self.assertEqual(1, hooks[2].deliveries.count())
+ def test__checkVisibility_public_artifact(self):
+ target = self.makeTarget()
+ login_person(target.owner)
+ self.assertTrue(WebhookSet._checkVisibility(target))
+
+ def test__checkVisibility_private_artifact(self):
+ owner = self.factory.makePerson()
+ target = self.makeTarget(
+ owner=owner, information_type=InformationType.PROPRIETARY)
+ login_person(owner)
+ self.assertTrue(WebhookSet._checkVisibility(target))
+
+ def test__checkVisibility_lost_access_to_private_artifact(self):
+ # A user may lose access to a private artifact even if they own it,
+ # for example if they leave the team that has a policy grant for
+ # branches on that project; in such cases they should stop receiving
+ # webhooks.
+ project = self.factory.makeProduct(
+ branch_sharing_policy=BranchSharingPolicy.PROPRIETARY)
+ grantee_team = self.factory.makeTeam()
+ policy = self.factory.makeAccessPolicy(
+ pillar=project, check_existing=True)
+ self.factory.makeAccessPolicyGrant(
+ 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.leave(grantee_team)
+ self.assertFalse(WebhookSet._checkVisibility(target))
+
+ def test__checkVisibility_with_source(self):
+ project = self.factory.makeProduct(
+ branch_sharing_policy=BranchSharingPolicy.PUBLIC_OR_PROPRIETARY)
+ owner = self.factory.makePerson()
+ source = self.makeTarget(
+ owner=owner, project=project,
+ information_type=InformationType.PROPRIETARY)
+ target1 = self.makeTarget(owner=owner, project=project)
+ target2 = self.makeTarget(project=project)
+ login_person(owner)
+ self.assertTrue(WebhookSet._checkVisibility(target1, source=source))
+ self.assertFalse(WebhookSet._checkVisibility(target2, source=source))
+
def test_trigger(self):
owner = self.factory.makePerson()
target1 = self.makeTarget(owner=owner)
@@ -229,18 +278,71 @@
delivery = hook2a.deliveries.one()
self.assertEqual(delivery.payload, {'other': 'payload'})
+ def test_trigger_skips_invisible(self):
+ # No webhooks are dispatched if the visibility check fails.
+ project = self.factory.makeProduct(
+ branch_sharing_policy=BranchSharingPolicy.PUBLIC_OR_PROPRIETARY)
+ owner = self.factory.makePerson()
+ source = self.makeTarget(
+ owner=owner, project=project,
+ information_type=InformationType.PROPRIETARY)
+ target1 = self.makeTarget(project=project)
+ target2 = self.makeTarget(owner=owner, project=project)
+ event_type = 'merge-proposal:0.1'
+ hook1 = self.factory.makeWebhook(
+ target=target1, event_types=[event_type])
+ hook2 = self.factory.makeWebhook(
+ target=target2, event_types=[event_type])
+
+ # The owner of target1 cannot see source, so their webhook is kept
+ # in the dark too.
+ getUtility(IWebhookSet).trigger(
+ target1, event_type, {'some': 'payload'}, source=source)
+ with admin_logged_in():
+ self.assertThat(list(hook1.deliveries), HasLength(0))
+ self.assertThat(list(hook2.deliveries), HasLength(0))
+
+ # The owner of target2 can see source, so it's OK to tell their
+ # webhook about the existence of source.
+ getUtility(IWebhookSet).trigger(
+ target2, event_type, {'some': 'payload'}, source=source)
+ with admin_logged_in():
+ self.assertThat(list(hook1.deliveries), HasLength(0))
+ self.assertThat(list(hook2.deliveries), HasLength(1))
+ delivery = hook2.deliveries.one()
+ self.assertEqual(delivery.payload, {'some': 'payload'})
+
+ def test_trigger_different_source_and_target_owners(self):
+ # Only people who can edit the webhook target can view the webhook,
+ # so, for a merge proposal between two branches with different
+ # owners, the owner of the merge source will in general not be able
+ # to see a webhook attached to the target. trigger copes with this.
+ project = self.factory.makeProduct()
+ source = self.makeTarget(project=project)
+ target = self.makeTarget(project=project)
+ 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'}, source=source)
+ with admin_logged_in():
+ self.assertThat(list(hook.deliveries), HasLength(1))
+ delivery = hook.deliveries.one()
+ self.assertEqual(delivery.payload, {'some': 'payload'})
+
class TestWebhookSetGitRepository(TestWebhookSetBase, TestCaseWithFactory):
event_type = 'git:push:0.1'
- def makeTarget(self, owner=None):
- return self.factory.makeGitRepository(owner=owner)
+ def makeTarget(self, project=None, **kwargs):
+ return self.factory.makeGitRepository(target=project, **kwargs)
class TestWebhookSetBranch(TestWebhookSetBase, TestCaseWithFactory):
event_type = 'bzr:push:0.1'
- def makeTarget(self, owner=None):
- return self.factory.makeBranch(owner=owner)
+ def makeTarget(self, project=None, **kwargs):
+ return self.factory.makeBranch(product=project, **kwargs)
=== added file 'lib/lp/services/webhooks/tests/test_payload.py'
--- lib/lp/services/webhooks/tests/test_payload.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/webhooks/tests/test_payload.py 2015-10-09 16:59:27 +0000
@@ -0,0 +1,31 @@
+# Copyright 2015 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from testtools.matchers import (
+ Equals,
+ Is,
+ MatchesDict,
+ )
+
+from lp.registry.interfaces.product import IProduct
+from lp.services.webhooks.payload import compose_webhook_payload
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import DatabaseFunctionalLayer
+
+
+class TestComposeWebhookPayload(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_serialises(self):
+ project = self.factory.makeProduct()
+ self.assertThat(
+ compose_webhook_payload(
+ IProduct, project, ["display_name", "owner", "projectgroup"]),
+ MatchesDict({
+ "display_name": Equals(project.display_name),
+ "owner": Equals("/~%s" % project.owner.name),
+ "projectgroup": Is(None),
+ }))
Follow ups
