Fix #1228 - Improves webhooks to send labels at once

config/secrets.py.example

@@ -7,33 +7,38 @@
 '''This file contains secrets and sensitive file-system locations and
 settings that we don't want to be public.'''
 
-from environment import *
+import os
+import environment as env
 
 # Secret key for signing cookies. Can be ignored for local testing.
 SECRET_KEY = '~*change me*~'
-HOOK_SECRET_KEY = '~*change me*~'
+
+if env.LOCALHOST:
+    HOOK_SECRET_KEY = 'SECRETS'
+else:
+    HOOK_SECRET_KEY = '~*change me*~'
 
 # Image uploading settings. PRODUCTION and STAGING can be ignored for
 # local testing.
-if PRODUCTION:
+if env.PRODUCTION:
     UPLOADS_DEFAULT_DEST = ''
     UPLOADS_DEFAULT_URL = ''
-elif STAGING:
+elif env.STAGING:
     UPLOADS_DEFAULT_DEST = ''
     UPLOADS_DEFAULT_URL = ''
-elif LOCALHOST:
-    UPLOADS_DEFAULT_DEST = BASE_DIR + '/uploads/'
+elif env.LOCALHOST:
+    UPLOADS_DEFAULT_DEST = env.BASE_DIR + '/uploads/'
     UPLOADS_DEFAULT_URL = 'http://localhost:5000/uploads/'
 
 
 # Database backup path.
-if LOCALHOST:
-    BACKUP_DEFAULT_DEST = BASE_DIR + '/backups/'
+if env.LOCALHOST:
+    BACKUP_DEFAULT_DEST = env.BASE_DIR + '/backups/'
 else:
     BACKUP_DEFAULT_DEST = ''
 
 # Production GiHub Issues repo URI. Can be ignored for local testing.
-if PRODUCTION:
+if env.PRODUCTION:
     ISSUES_REPO_URI = ''
 # Staging and Local instances use the same test repo
 else:
@@ -45,12 +50,12 @@ else:
 # [1]<https://github.com/organizations/webcompat/settings/applications/>
 # PRODUCTION and STAGING can be ignored for local testing.
 # Production = webcompat.com
-if PRODUCTION:
+if env.PRODUCTION:
     GITHUB_CLIENT_ID = ''
     GITHUB_CLIENT_SECRET = ''
     GITHUB_CALLBACK_URL = ''
 # Staging = staging.webcompat.com
-elif STAGING:
+elif env.STAGING:
     GITHUB_CLIENT_ID = ''
     GITHUB_CLIENT_SECRET = ''
     GITHUB_CALLBACK_URL = ''

tests/fixtures/webhooks/new_event_invalid.json

@@ -0,0 +1,7 @@
+{
+  "action": "closed",
+  "issue": {
+    "number": 600,
+    "body": "<!-- @browser: Firefox 55.0 -->\n<!-- @ua_header: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0 -->\n<!-- @reported_with: web -->\n\n**URL**: https://www.netflix.com/"
+  }
+}

tests/fixtures/webhooks/new_event_valid.json

@@ -0,0 +1,7 @@
+{
+  "action": "opened",
+  "issue": {
+    "number": 600,
+    "body": "<!-- @browser: Firefox 55.0 -->\n<!-- @ua_header: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0 -->\n<!-- @reported_with: web -->\n\n**URL**: https://www.netflix.com/"
+  }
+}

tests/test_urls.py

@@ -84,15 +84,6 @@ def test_issues_list_page(self):
         self.assertEqual(rv.status_code, 200)
         self.assertNotEqual(rv.status_code, 307)
 
-    def test_labeler_webhook(self):
-        '''Webhook related tests.'''
-        headers = {'X-GitHub-Event': 'ping'}
-        rv = self.app.get('/webhooks/labeler', headers=headers)
-        self.assertEqual(rv.status_code, 403)
-        rv = self.app.post('/webhooks/labeler', headers=headers)
-        # A random post should 401, only requests from GitHub will 200
-        self.assertEqual(rv.status_code, 401)
-
     def test_csp_report_uri(self):
         '''Test POST to /csp-report w/ correct content-type returns 204.'''
         headers = {'Content-Type': 'application/csp-report'}

tests/test_webhook.py

@@ -0,0 +1,126 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+"""Tests for our webhooks."""
+
+import json
+import os
+import sys
+import unittest
+
+from webcompat import app
+from webcompat.webhooks import helpers
+
+# Add webcompat module to import path
+sys.path.append(os.path.realpath(os.pardir))
+import webcompat  # nopep8
+
+
+# Any request that depends on parsing HTTP Headers (basically anything
+# on the index route, will need to include the following: environ_base=headers
+headers = {'HTTP_USER_AGENT': ('Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; '
+                               'rv:31.0) Gecko/20100101 Firefox/31.0')}
+key = app.config['HOOK_SECRET_KEY']
+
+
+# Some machinery for opening our test files
+def event_data(filename):
+    '''return a tuple with the content and its signature.'''
+    current_root = os.path.realpath(os.curdir)
+    events_path = 'tests/fixtures/webhooks'
+    path = os.path.join(current_root, events_path, filename)
+    with open(path, 'r') as f:
+        json_event = json.dumps(json.load(f))
+    signature = 'sha1={sig}'.format(
+        sig=helpers.get_payload_signature(key, json_event))
+    return json_event, signature
+
+
+class TestWebhook(unittest.TestCase):
+    def setUp(self):
+        webcompat.app.config['TESTING'] = True
+        self.app = webcompat.app.test_client()
+        self.headers = {'content-type': 'application/json'}
+        self.test_url = '/webhooks/labeler'
+        self.issue_body = """
+        <!-- @browser: Firefox 55.0 -->
+        <!-- @ua_header: Mozilla/5.0 (what) Gecko/20100101 Firefox/55.0 -->
+        <!-- @reported_with: web -->
+
+        **URL**: https://www.example.com/
+        **Browser / Version**: Firefox 55.0
+        <!-- @browser: Chrome 48.0 -->
+        """
+        self.issue_body2 = """
+        <!-- @browser: Foobar -->
+        """
+
+    def tearDown(self):
+        pass
+
+    def test_forbidden_get(self):
+        """GET is forbidden on labeler webhook."""
+        rv = self.app.get(self.test_url, headers=self.headers)
+        self.assertEqual(rv.status_code, 404)
+
+    def test_fail_on_missing_signature(self):
+        """POST without signature on labeler webhook is forbidden."""
+        self.headers.update({'X-GitHub-Event': 'ping'})
+        rv = self.app.post(self.test_url, headers=self.headers)
+        self.assertEqual(rv.status_code, 401)
+
+    def test_fail_on_bogus_signature(self):
+        """POST without bogus signature on labeler webhook is forbidden."""
+        json_event, signature = event_data('new_event_valid.json')
+        self.headers.update({'X-GitHub-Event': 'ping',
+                             'X-Hub-Signature': 'Boo!'})
+        rv = self.app.post(self.test_url,
+                           data=json_event,
+                           headers=self.headers)
+        self.assertEqual(rv.status_code, 401)
+
+    def test_fail_on_invalid_event_type(self):
+        """POST with event not being 'issues' or 'ping' fails."""
+        json_event, signature = event_data('new_event_valid.json')
+        self.headers.update({'X-GitHub-Event': 'failme',
+                             'X-Hub-Signature': signature})
+        rv = self.app.post(self.test_url,
+                           data=json_event,
+                           headers=self.headers)
+        self.assertEqual(rv.status_code, 403)
+
+    def test_success_on_ping_event(self):
+        """POST with PING events just return a 200 and contains pong."""
+        json_event, signature = event_data('new_event_valid.json')
+        self.headers.update({'X-GitHub-Event': 'ping',
+                             'X-Hub-Signature': signature})
+        rv = self.app.post(self.test_url,
+                           data=json_event,
+                           headers=self.headers)
+        self.assertEqual(rv.status_code, 200)
+        self.assertIn('pong', rv.data)
+
+    def test_fails_on_action_not_opened(self):
+        """POST with action different of opened fails."""
+        json_event, signature = event_data('new_event_invalid.json')
+        self.headers.update({'X-GitHub-Event': 'issues',
+                             'X-Hub-Signature': signature})
+        rv = self.app.post(self.test_url,
+                           data=json_event,
+                           headers=self.headers)
+        self.assertEqual(rv.status_code, 200)
+        self.assertIn('cool story, bro.', rv.data)
+
+    def test_extract_browser_label(self):
+        """Extract browser label name."""
+        browser_label = helpers.extract_browser_label(self.issue_body)
+        self.assertEqual(browser_label, 'browser-firefox')
+        browser_label_none = helpers.extract_browser_label(self.issue_body2)
+        self.assertEqual(browser_label_none, None)
+
+
+if __name__ == '__main__':
+    unittest.main()

webcompat/webhooks/__init__.py

@@ -4,61 +4,70 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-'''Flask Blueprint for our "webhooks" module, which we use to do cool things
-with GitHub events and actions.
+"""Flask Blueprint for our "webhooks" module.webhooks
 
-See https://developer.github.com/webhooks/ for what is possible.'''
+See https://developer.github.com/webhooks/ for what is possible."""
 
 import json
+import logging
 
 from flask import abort
 from flask import Blueprint
 from flask import request
 
-from helpers import dump_to_db
-from helpers import parse_and_set_label
-from helpers import set_label
+from helpers import extract_browser_label
+from helpers import set_labels
 from helpers import signature_check
 
 from webcompat import app
 
-
 webhooks = Blueprint('webhooks', __name__, url_prefix='/webhooks')
 
 
-@webhooks.route('/labeler', methods=['GET', 'POST'])
+@webhooks.route('/labeler', methods=['POST'])
 def hooklistener():
-    '''Listen for the "issues" webhook event, parse the body
+    """Listen for the "issues" webhook event.
 
-       Method posts back labels and dumps data to a local db.
-       Only in response to the 'opened' action, though.
-    '''
-    if request.method == 'GET':
-        abort(403)
-    elif request.method == 'POST':
-        event_type = request.headers.get('X-GitHub-Event')
-        post_signature = request.headers.get('X-Hub-Signature')
-        if post_signature:
-            key = app.config['HOOK_SECRET_KEY']
-            payload = json.loads(request.data)
-            if not signature_check(key, post_signature, request.data):
-                abort(401)
-            if event_type == 'issues':
-                if payload.get('action') == 'opened':
-                    issue_body = payload.get('issue')['body']
-                    issue_title = payload.get('issue')['title']
-                    issue_number = payload.get('issue')['number']
-                    parse_and_set_label(issue_body, issue_number)
-                    # Setting "Needs Triage" label by default
-                    # to all the new issues raised
-                    set_label('status-needstriage', issue_number)
-                    dump_to_db(issue_title, issue_body, issue_number)
-                    return ('gracias, amigo.', 200)
+    - Only in response to the 'opened' action (for now).
+    - Add label needstriage to the issue
+    - Add label for the browser name
+    """
+    event_type = request.headers.get('X-GitHub-Event')
+    post_signature = request.headers.get('X-Hub-Signature')
+    if post_signature:
+        key = app.config['HOOK_SECRET_KEY']
+        payload = json.loads(request.data)
+        if not signature_check(key, post_signature, request.data):
+            abort(401)
+        if event_type == 'issues':
+            if payload.get('action') == 'opened':
+                # Setting "Needs Triage" label by default
+                # to all the new issues raised
+                labels = ['status-needstriage']
+                issue_body = payload.get('issue')['body']
+                issue_number = payload.get('issue')['number']
+                browser_label = extract_browser_label(issue_body)
+                if browser_label:
+                    labels.append(browser_label)
+                # Sending a request to set labels
+                response = set_labels(labels, issue_number)
+                if response.status_code == 200:
+                    return ('gracias, amigo.', 200,
+                            {'Content-Type': 'plain/text'})
                 else:
-                    return ('cool story, bro.', 200)
-            elif event_type == 'ping':
-                return ('pong', 200)
+                    # Logging the ip and url for investigation
+                    log = app.logger
+                    log.setLevel(logging.INFO)
+                    msg = 'failed to set labels on issue {issue}'.format(
+                        issue=issue_number)
+                    log.info(msg)
             else:
-                abort(403)
+                return ('cool story, bro.', 200,
+                        {'Content-Type': 'plain/text'})
+        elif event_type == 'ping':
+            return ('pong', 200, {'Content-Type': 'plain/text'})
         else:
-            abort(401)
+            abort(403)
+    else:
+        # No signature.
+        abort(401)