launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === 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
> @@ -80,8 +81,11 @@
>      delta_values = (
>          'registrant',
>          'source_branch',
> +        'source_git_ref',
>          'target_branch',
> +        'target_git_ref',
>          'prerequisite_branch',
> +        'prerequisite_git_ref',

*_git_ref aren't exported; *_git_path and *_git_repository are.

>          '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),
> +            }

It's a bit weird that this includes a subset of the old and new state, but not just the state that has changed. They're also wrapped in "old" and "new" for the modified case, unwrapped in the created case, and non-existent in the deleted case. How would a comment or vote look?

Also, emails don't state the old values of new_values, but is there any reason for webhooks to not?

> +
> +    @classmethod
>      def snapshot(klass, merge_proposal):
>          """Return a snapshot suitable for use with construct.
>  
> 
> === 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
> @@ -104,6 +105,10 @@
>      )
>  
>  
> +BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG = (
> +    "merge_proposals.webhooks.enabled")

Might as well keep this scoped under "code.".

> +
> +
>  class IBranchMergeProposalPublic(IPrivacy):
>  
>      id = Int(
> 
> === 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
> @@ -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",

Is a single "merge-proposal" event enough? GitHub has an all-encompassing "issues", but it is the exception rather than the rule. I'm not sure how far we want to split them.

>      }
>  
>  
> @@ -359,6 +362,11 @@
>          header will be sent using HMAC-SHA1.
>          """
>  
> +
> +class IWebhookPayloadRequest(ILaunchpadBrowserApplicationRequest):
> +    """An internal fake request used while composing webhook payloads."""

I can't see this being a circular imports problem, so would it be cleaner to move this to payload.py to isolate it?

> +
> +
>  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

This could do with a comment outlining when it might not be the case.

> +            if source is not None:
> +                assert IGitRepository.providedBy(source)
> +                if not removeSecurityProxy(source).visibleByUser(owner):
> +                    return False

BMP visibility additionally requires access to the prerequisite branch. Can we avoid reimplementing the security checkers here?

> +        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)


-- 
https://code.launchpad.net/~cjwatson/launchpad/bmp-webhooks/+merge/274005
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.