Fix incoming payload parsing, cleanup, and add logging

Author: agjohnson

readthedocs/core/views/hooks.py

@@ -141,20 +141,24 @@ def github_build(request):
 
     This will search for projects matching either a stripped down HTTP or SSH
     URL. The search is error prone, use the API v2 webhook for new webhooks.
+
+    Old webhooks may not have specified the content type to POST with, and
+    therefore can use ``application/x-www-form-urlencoded`` to pass the JSON
+    payload. More information on the API docs here:
+    https://developer.github.com/webhooks/creating/#content-type
     """
     if request.method == 'POST':
         try:
-            data = json.loads(request.body)
-        except (ValueError, TypeError):
-            log.error('Invalid GitHub webhook payload', exc_info=True)
-            return HttpResponse('Invalid request', status=400)
-        http_url = data['repository']['url']
-        http_search_url = http_url.replace('http://', '').replace('https://', '')
-        ssh_url = data['repository']['ssh_url']
-        ssh_search_url = ssh_url.replace('git@', '').replace('.git', '')
-        try:
+            if request.META['CONTENT_TYPE'] == 'application/x-www-form-urlencoded':
+                data = json.loads(request.POST.get('payload'))
+            else:
+                data = json.loads(request.body)
+            http_url = data['repository']['url']
+            http_search_url = http_url.replace('http://', '').replace('https://', '')
+            ssh_url = data['repository']['ssh_url']
+            ssh_search_url = ssh_url.replace('git@', '').replace('.git', '')
             branches = [data['ref'].replace('refs/heads/', '')]
-        except KeyError:
+        except (ValueError, TypeError, KeyError):
             log.error('Invalid GitHub webhook payload', exc_info=True)
             return HttpResponse('Invalid request', status=400)
         try:
@@ -178,7 +182,7 @@ def github_build(request):
             log.error('Project match not found: url=%s', http_search_url)
             return HttpResponseNotFound('Project not found')
     else:
-        return HttpResponse('Method not allowed', status=405)
+        return HttpResponse('Method not allowed, POST is required', status=405)
 
 
 @csrf_exempt
@@ -191,12 +195,12 @@ def gitlab_build(request):
     if request.method == 'POST':
         try:
             data = json.loads(request.body)
-        except (ValueError, TypeError):
+            url = data['project']['http_url']
+            search_url = re.sub(r'^https?://(.*?)(?:\.git|)$', '\\1', url)
+            branches = [data['ref'].replace('refs/heads/', '')]
+        except (ValueError, TypeError, KeyError):
             log.error('Invalid GitLab webhook payload', exc_info=True)
             return HttpResponse('Invalid request', status=400)
-        url = data['project']['http_url']
-        search_url = re.sub(r'^https?://(.*?)(?:\.git|)$', '\\1', url)
-        branches = [data['ref'].replace('refs/heads/', '')]
         log.info(
             'GitLab webhook search: url=%s branches=%s',
             search_url,
@@ -209,49 +213,52 @@ def gitlab_build(request):
             log.error('Project match not found: url=%s', search_url)
             return HttpResponseNotFound('Project match not found')
     else:
-        return HttpResponse('Method not allowed', status=405)
+        return HttpResponse('Method not allowed, POST is required', status=405)
 
 
 @csrf_exempt
 def bitbucket_build(request):
     """Consume webhooks from multiple versions of Bitbucket's API
 
+    New webhooks are set up with v2, but v1 webhooks will still point to this
+    endpoint. There are also "services" that point here and submit
+    ``application/x-www-form-urlencoded`` data.
+
     API v1
         https://confluence.atlassian.com/bitbucket/events-resources-296095220.html
 
     API v2
         https://confluence.atlassian.com/bitbucket/event-payloads-740262817.html#EventPayloads-Push
+
+    Services
+        https://confluence.atlassian.com/bitbucket/post-service-management-223216518.html
     """
     if request.method == 'POST':
         try:
-            data = json.loads(request.body)
-        except (TypeError, ValueError):
-            log.error('Invalid Bitbucket webhook payload', exc_info=True)
-            return HttpResponse('Invalid request', status=400)
+            if request.META['CONTENT_TYPE'] == 'application/x-www-form-urlencoded':
+                data = json.loads(request.POST.get('payload'))
+            else:
+                data = json.loads(request.body)
 
-        version = 2 if request.META.get('HTTP_USER_AGENT') == 'Bitbucket-Webhooks/2.0' else 1
-        if version == 1:
-            try:
+            version = 2 if request.META.get('HTTP_USER_AGENT') == 'Bitbucket-Webhooks/2.0' else 1
+            if version == 1:
                 branches = [commit.get('branch', '')
                             for commit in data['commits']]
                 repository = data['repository']
                 search_url = 'bitbucket.org{0}'.format(
                     repository['absolute_url'].rstrip('/')
                 )
-            except KeyError:
-                log.error('Invalid Bitbucket webhook payload', exc_info=True)
-                return HttpResponse('Invalid request', status=400)
-        elif version == 2:
-            try:
+            elif version == 2:
                 changes = data['push']['changes']
                 branches = [change['new']['name']
                             for change in changes]
                 search_url = 'bitbucket.org/{0}'.format(
                     data['repository']['full_name']
                 )
-            except KeyError:
-                log.error('Invalid Bitbucket webhook payload', exc_info=True)
-                return HttpResponse('Invalid request', status=400)
+        except (TypeError, ValueError, KeyError):
+            log.error('Invalid Bitbucket webhook payload', exc_info=True)
+            return HttpResponse('Invalid request', status=400)
+
         log.info(
             'Bitbucket webhook search: url=%s branches=%s',
             search_url,
@@ -265,7 +272,7 @@ def bitbucket_build(request):
             log.error('Project match not found: url=%s', search_url)
             return HttpResponseNotFound('Project match not found')
     else:
-        return HttpResponse('Method not allowed', status=405)
+        return HttpResponse('Method not allowed, POST is required', status=405)
 
 
 @csrf_exempt

readthedocs/restapi/views/integrations.py

@@ -1,3 +1,5 @@
+import logging
+
 from rest_framework import permissions
 from rest_framework.views import APIView
 from rest_framework.renderers import JSONRenderer
@@ -13,6 +15,8 @@
 from readthedocs.projects.models import Project
 
 
+log = logging.getLogger(__name__)
+
 GITHUB_PUSH = 'push'
 GITLAB_PUSH = 'push'
 BITBUCKET_PUSH = 'repo:push'
@@ -27,23 +31,41 @@ def post(self, request, project_slug, format=None):
         try:
             project = Project.objects.get(slug=project_slug)
             resp = self.handle_webhook(request, project, request.data)
+            if resp is None:
+                log.info('Unhandled webhook event')
+                resp = {'detail': 'Unhandled webhook event'}
         except Project.DoesNotExist:
             raise Http404('Project does not exist')
         return Response(resp)
 
     def handle_webhook(self, request, project, data=None):
-        """Handle webhook payload
+        """Handle webhook payload"""
+        raise NotImplementedError
 
-        If a build is triggered from this method, return a JSON response with
-        the following::
+    def get_response_push(self, project, branches):
+        """Build branches on push events and return API response
+
+        Return a JSON response with the following::
 
             {
                 "build_triggered": true,
                 "project": "project_name",
                 "versions": [...]
             }
+
+        :param project: Project instance
+        :type project: Project
+        :param branches: List of branch names to build
+        :type branches: list(str)
         """
-        raise NotImplementedError
+        to_build, not_building = build_branches(project, branches)
+        if not_building:
+            log.info('Skipping project branches: project=%s branches=%s',
+                     project, branches)
+        triggered = True if to_build else False
+        return {'build_triggered': triggered,
+                'project': project.slug,
+                'versions': to_build}
 
 
 class GitHubWebhookView(WebhookMixin, APIView):
@@ -60,32 +82,19 @@ class GitHubWebhookView(WebhookMixin, APIView):
             "ref": "branch-name",
             ...
         }
-
-    If a build is triggered, this will return JSON with the following::
-
-        {
-            "build_triggered": true,
-            "project": "project_name",
-            "versions": [...]
-        }
     """
 
     def handle_webhook(self, request, project, data=None):
         # Get event and trigger other webhook events
         event = request.META.get('HTTP_X_GITHUB_EVENT', 'push')
         webhook_github.send(Project, project=project, data=data, event=event)
         # Handle push events and trigger builds
-        try:
-            branches = [request.data['ref'].replace('refs/heads/', '')]
-        except KeyError:
-            raise ParseError('Parameter "ref" is required')
         if event == GITHUB_PUSH:
-            to_build, not_building = build_branches(project, branches)
-            triggered = True if to_build else False
-            return {'build_triggered': triggered,
-                    'project': project.slug,
-                    'versions': to_build}
-        return {'build_triggered': False}
+            try:
+                branches = [request.data['ref'].replace('refs/heads/', '')]
+                return self.get_response_push(project, branches)
+            except KeyError:
+                raise ParseError('Parameter "ref" is required')
 
 
 class GitLabWebhookView(WebhookMixin, APIView):
@@ -101,32 +110,19 @@ class GitLabWebhookView(WebhookMixin, APIView):
             "ref": "branch-name",
             ...
         }
-
-    If a build is triggered, this will return JSON with the following::
-
-        {
-            "build_triggered": true,
-            "project": "project_name",
-            "versions": [...]
-        }
     """
 
     def handle_webhook(self, request, project, data=None):
         # Get event and trigger other webhook events
         event = data.get('object_kind', GITLAB_PUSH)
         webhook_gitlab.send(Project, project=project, data=data, event=event)
         # Handle push events and trigger builds
-        try:
-            branches = [request.data['ref'].replace('refs/heads/', '')]
-        except KeyError:
-            raise ParseError('Parameter "ref" is required')
         if event == GITLAB_PUSH:
-            to_build, not_building = build_branches(project, branches)
-            triggered = True if to_build else False
-            return {'build_triggered': triggered,
-                    'project': project.slug,
-                    'versions': to_build}
-        return {'build_triggered': False}
+            try:
+                branches = [request.data['ref'].replace('refs/heads/', '')]
+                return self.get_response_push(project, branches)
+            except KeyError:
+                raise ParseError('Parameter "ref" is required')
 
 
 class BitbucketWebhookView(WebhookMixin, APIView):
@@ -162,11 +158,6 @@ def handle_webhook(self, request, project, data=None):
                 changes = data['push']['changes']
                 branches = [change['new']['name']
                             for change in changes]
+                return self.get_response_push(project, branches)
             except KeyError:
                 raise ParseError('Invalid request')
-            to_build, not_building = build_branches(project, branches)
-            triggered = True if to_build else False
-            return {'build_triggered': triggered,
-                    'project': project.slug,
-                    'versions': to_build}
-        return {'build_triggered': False}

readthedocs/rtd_tests/tests/test_api.py

@@ -324,6 +324,18 @@ def test_github_webhook(self, trigger_build):
             mock.call(force=True, version=mock.ANY, project=self.project)
         ])
 
+    def test_github_invalid_webhook(self, trigger_build):
+        """GitHub webhook unhandled event"""
+        client = APIClient()
+        resp = client.post(
+            '/api/v2/webhook/github/{0}/'.format(self.project.slug),
+            {'foo': 'bar'},
+            format='json',
+            HTTP_X_GITHUB_EVENT='pull_request',
+        )
+        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(resp.data['detail'], 'Unhandled webhook event')
+
     def test_gitlab_webhook(self, trigger_build):
         """GitLab webhook API"""
         client = APIClient()
@@ -344,6 +356,17 @@ def test_gitlab_webhook(self, trigger_build):
             mock.call(force=True, version=mock.ANY, project=self.project)
         ])
 
+    def test_gitlab_invalid_webhook(self, trigger_build):
+        """GitLab webhook unhandled event"""
+        client = APIClient()
+        resp = client.post(
+            '/api/v2/webhook/gitlab/{0}/'.format(self.project.slug),
+            {'object_kind': 'pull_request'},
+            format='json',
+        )
+        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(resp.data['detail'], 'Unhandled webhook event')
+
     def test_bitbucket_webhook(self, trigger_build):
         """Bitbucket webhook API"""
         client = APIClient()
@@ -379,3 +402,15 @@ def test_bitbucket_webhook(self, trigger_build):
         trigger_build.assert_has_calls([
             mock.call(force=True, version=mock.ANY, project=self.project)
         ])
+
+    def test_bitbucket_invalid_webhook(self, trigger_build):
+        """Bitbucket webhook unhandled event"""
+        client = APIClient()
+        resp = client.post(
+            '/api/v2/webhook/bitbucket/{0}/'.format(self.project.slug),
+            {'foo': 'bar'},
+            format='json',
+            HTTP_X_EVENT_KEY='pull_request'
+        )
+        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(resp.data['detail'], 'Unhandled webhook event')

readthedocs/rtd_tests/tests/test_post_commit_hooks.py

@@ -1,9 +1,10 @@
-from django.test import TestCase
 import json
 import logging
-import mock
+from urllib import urlencode
 
+import mock
 from django_dynamic_fixture import get
+from django.test import TestCase
 
 from readthedocs.builds.models import Version
 from readthedocs.projects.models import Project
@@ -209,6 +210,17 @@ def setUp(self):
             }
         }
 
+    def test_post_types(self):
+        """Ensure various POST formats"""
+        r = self.client.post('/github/',
+                             data=json.dumps(self.payload),
+                             content_type='application/json')
+        self.assertEqual(r.status_code, 200)
+        r = self.client.post('/github/',
+                             data=urlencode({'payload': json.dumps(self.payload)}),
+                             content_type='application/x-www-form-urlencoded')
+        self.assertEqual(r.status_code, 200)
+
     def test_github_upper_case_repo(self):
         """
         Test the github post commit hook will build properly with upper case
@@ -404,6 +416,17 @@ def setUp(self):
             "user": "marcus"
         }
 
+    def test_post_types(self):
+        """Ensure various POST formats"""
+        r = self.client.post('/bitbucket/',
+                             data=json.dumps(self.hg_payload),
+                             content_type='application/json')
+        self.assertEqual(r.status_code, 200)
+        r = self.client.post('/bitbucket/',
+                             data=urlencode({'payload': json.dumps(self.hg_payload)}),
+                             content_type='application/x-www-form-urlencoded')
+        self.assertEqual(r.status_code, 200)
+
     def test_bitbucket_post_commit(self):
         r = self.client.post('/bitbucket/', data=json.dumps(self.hg_payload),
                              content_type='application/json')