launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/add-webhook-secret into lp:launchpad.

Commit message:
Allow setting a secret when creating a new webhook.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #342729 in Launchpad itself: "Should support post-commit webhooks"
  https://bugs.launchpad.net/launchpad/+bug/342729

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/add-webhook-secret/+merge/275695

Allow setting a secret when creating a new webhook.

For now, this field is disabled when editing an existing webhook.  It will need some custom handling to, at minimum, not reveal the existing secret, and probably also do some clever JavaScript thing to obscure the entered secret except when you're editing the value.  Secrets for existing webhooks can still be edited using the API though.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/add-webhook-secret into lp:launchpad.

=== modified file 'lib/lp/services/webhooks/browser.py'
--- lib/lp/services/webhooks/browser.py	2015-10-05 11:11:47 +0000
+++ lib/lp/services/webhooks/browser.py	2015-10-26 12:21:56 +0000
@@ -110,8 +110,8 @@
 
 
 class WebhookEditSchema(Interface):
-    # XXX wgrant 2015-08-04: Need custom widget for secret.
-    use_template(IWebhook, include=['delivery_url', 'event_types', 'active'])
+    use_template(
+        IWebhook, include=['delivery_url', 'event_types', 'active', 'secret'])
 
 
 class WebhookAddView(LaunchpadFormView):
@@ -140,7 +140,8 @@
     def new_action(self, action, data):
         webhook = self.context.newWebhook(
             registrant=self.user, delivery_url=data['delivery_url'],
-            event_types=data['event_types'], active=data['active'])
+            event_types=data['event_types'], active=data['active'],
+            secret=data['secret'])
         self.next_url = canonical_url(webhook)
 
 
@@ -149,6 +150,8 @@
     label = "Manage webhook"
 
     schema = WebhookEditSchema
+    # XXX wgrant 2015-08-04: Need custom widget for secret.
+    field_names = ['delivery_url', 'event_types', 'active']
     custom_widget('event_types', LabeledMultiCheckBoxWidget)
 
     def initialize(self):

=== modified file 'lib/lp/services/webhooks/tests/test_browser.py'
--- lib/lp/services/webhooks/tests/test_browser.py	2015-10-19 10:48:28 +0000
+++ lib/lp/services/webhooks/tests/test_browser.py	2015-10-26 12:21:56 +0000
@@ -188,6 +188,7 @@
                 "field.delivery_url": "http://example.com/test";,
                 "field.active": "on", "field.event_types-empty-marker": "1",
                 "field.event_types": self.event_type,
+                "field.secret": "secret code",
                 "field.actions.new": "Add webhook"})
         self.assertEqual([], view.errors)
         hook = self.target.webhooks.one()
@@ -198,7 +199,8 @@
                 registrant=self.owner,
                 delivery_url="http://example.com/test";,
                 active=True,
-                event_types=[self.event_type]))
+                event_types=[self.event_type],
+                secret="secret code"))
 
     def test_rejects_bad_scheme(self):
         transaction.commit()
@@ -212,6 +214,21 @@
             ['delivery_url'], [error.field_name for error in view.errors])
         self.assertIs(None, self.target.webhooks.one())
 
+    def test_no_secret(self):
+        # If the secret field is left empty, the secret is set to None
+        # rather than to the empty string.
+        view = self.makeView(
+            "+new-webhook", method="POST",
+            form={
+                "field.delivery_url": "http://example.com/test";,
+                "field.active": "on", "field.event_types-empty-marker": "1",
+                "field.event_types": self.event_type,
+                "field.secret": "",
+                "field.actions.new": "Add webhook"})
+        self.assertEqual([], view.errors)
+        hook = self.target.webhooks.one()
+        self.assertIsNone(hook.secret)
+
     def test_event_types(self):
         # Only event types that are valid for the target are offered.
         browser = self.getUserBrowser(