Fix webhook trigger V query param to be required

* Raise webob.exc.HTTPBadRequest if webhook trigger URL is missing V
query param
* Update API docs to show V query parameter as required

Closes-Bug: 1759663

Change-Id: I586df9689585dad8fe461b256ae6a1de084f67f6
Signed-off-by: Duc Truong <dtruong@blizzard.com>

api-ref/source/parameters.yaml

@@ -281,6 +281,7 @@ webhook_params:
 webhook_version:
   type: string
   in: query
+  required: True
   description: |
     The webhook implementation version requested.
 

api-ref/source/webhooks.inc

@@ -20,6 +20,7 @@ Response Codes
 
 .. rest_status_code:: error status.yaml
 
+   - 400
    - 403
    - 404
    - 503

senlin/api/middleware/webhook.py

@@ -13,6 +13,7 @@
 from oslo_log import log as logging
 import six
 from six.moves.urllib import parse as urlparse
+import webob
 
 from senlin.api.common import util
 from senlin.api.common import wsgi
@@ -75,6 +76,7 @@ class WebhookMiddleware(wsgi.Middleware):
         parts = urlparse.urlparse(url)
         p = parts.path.split('/')
 
+        # check if URL is a webhook trigger request
         # expected: ['', 'v1', 'webhooks', 'webhook-id', 'trigger']
         if len(p) != 5:
             return None
@@ -82,10 +84,16 @@ class WebhookMiddleware(wsgi.Middleware):
         if any((p[0] != '', p[2] != 'webhooks', p[4] != 'trigger')):
             return None
 
+        # at this point it has been determined that the URL is a webhook
+        # trigger request
         qs = urlparse.parse_qs(parts.query)
-        if 'V' not in qs:
+        if 'V' in qs:
-            return None
+            qs.pop('V')
-        qs.pop('V')
+        else:
+            raise webob.exc.HTTPBadRequest(
+                explanation=_('V query parameter is required in webhook '
+                              'trigger URL'))
+
         params = dict((k, v[0]) for k, v in qs.items())
         return p[3], params
 

senlin/tests/unit/api/middleware/test_webhook.py

@@ -13,6 +13,8 @@
 import mock
 
 from oslo_config import cfg
+import six
+import webob
 
 from senlin.api.common import util as common_util
 from senlin.api.common import version_request as vr
@@ -55,6 +57,40 @@ class TestWebhookMiddleware(base.SenlinTestCase):
         res = self.middleware._parse_url(self._generate_url())
         self.assertEqual(('WEBHOOK_ID', {'key': 'TEST_KEY'}), res)
 
+    def test_parse_url_version_provided_no_key(self):
+        # The structure /<webhook_id>/trigger?V=1 should be valid
+        self.url_slices.pop('05_params')
+
+        res = self.middleware._parse_url(self._generate_url())
+        self.assertEqual(('WEBHOOK_ID', {}), res)
+
+    def test_parse_url_no_version_provided_no_key_provided(self):
+        # The structure /<webhook_id>/trigger should be invalid
+        # because version is missing
+        self.url_slices.pop('04_version')
+        self.url_slices.pop('05_params')
+
+        ex = self.assertRaises(webob.exc.HTTPBadRequest,
+                               self.middleware._parse_url,
+                               self._generate_url())
+
+        self.assertEqual("V query parameter is required in webhook trigger "
+                         "URL", six.text_type(ex))
+
+    def test_parse_url_no_version_provided_key_provided(self):
+        # The structure /<webhook_id>/trigger?key=value should be invalid
+        # because version is missing
+        self.url_slices.pop('04_version')
+        self.url_slices.pop('05_params')
+        self.url_slices['05_params'] = 'key=TEST_KEY'
+
+        ex = self.assertRaises(webob.exc.HTTPBadRequest,
+                               self.middleware._parse_url,
+                               self._generate_url())
+
+        self.assertEqual("V query parameter is required in webhook trigger "
+                         "URL", six.text_type(ex))
+
     def test_parse_url_webhooks_not_found(self):
         # String 'webhooks' is not found in url
         self.url_slices['01_webhook_str'] = '/foo/'
@@ -79,12 +115,6 @@ class TestWebhookMiddleware(base.SenlinTestCase):
         res = self.middleware._parse_url(self._generate_url())
         self.assertIsNone(res)
 
-    def test_parse_url_no_key_provided(self):
-        # Bottom string of the url does not start with 'trigger'
-        self.url_slices['04_key_str'] = 'version=1'
-        res = self.middleware._parse_url(self._generate_url())
-        self.assertIsNone(res)
-
     @mock.patch.object(driver_base, 'SenlinDriver')
     def test_get_token_succeeded(self, mock_senlindriver):
         class FakeAccessInfo(object):