Skip to content

Commit 7fba0d1

Browse files
author
Mike Taylor
authored
Merge pull request #1566 from karlcow/1228/2
Fix #1228 - Improves webhooks to send labels at once
2 parents 39f6f6b + 39874cb commit 7fba0d1

File tree

7 files changed

+227
-88
lines changed

7 files changed

+227
-88
lines changed

config/secrets.py.example

+16-11
Original file line numberDiff line numberDiff line change
@@ -7,33 +7,38 @@
77
'''This file contains secrets and sensitive file-system locations and
88
settings that we don't want to be public.'''
99

10-
from environment import *
10+
import os
11+
import environment as env
1112

1213
# Secret key for signing cookies. Can be ignored for local testing.
1314
SECRET_KEY = '~*change me*~'
14-
HOOK_SECRET_KEY = '~*change me*~'
15+
16+
if env.LOCALHOST:
17+
HOOK_SECRET_KEY = 'SECRETS'
18+
else:
19+
HOOK_SECRET_KEY = '~*change me*~'
1520

1621
# Image uploading settings. PRODUCTION and STAGING can be ignored for
1722
# local testing.
18-
if PRODUCTION:
23+
if env.PRODUCTION:
1924
UPLOADS_DEFAULT_DEST = ''
2025
UPLOADS_DEFAULT_URL = ''
21-
elif STAGING:
26+
elif env.STAGING:
2227
UPLOADS_DEFAULT_DEST = ''
2328
UPLOADS_DEFAULT_URL = ''
24-
elif LOCALHOST:
25-
UPLOADS_DEFAULT_DEST = BASE_DIR + '/uploads/'
29+
elif env.LOCALHOST:
30+
UPLOADS_DEFAULT_DEST = env.BASE_DIR + '/uploads/'
2631
UPLOADS_DEFAULT_URL = 'http://localhost:5000/uploads/'
2732

2833

2934
# Database backup path.
30-
if LOCALHOST:
31-
BACKUP_DEFAULT_DEST = BASE_DIR + '/backups/'
35+
if env.LOCALHOST:
36+
BACKUP_DEFAULT_DEST = env.BASE_DIR + '/backups/'
3237
else:
3338
BACKUP_DEFAULT_DEST = ''
3439

3540
# Production GiHub Issues repo URI. Can be ignored for local testing.
36-
if PRODUCTION:
41+
if env.PRODUCTION:
3742
ISSUES_REPO_URI = ''
3843
# Staging and Local instances use the same test repo
3944
else:
@@ -45,12 +50,12 @@ else:
4550
# [1]<https://github.com/organizations/webcompat/settings/applications/>
4651
# PRODUCTION and STAGING can be ignored for local testing.
4752
# Production = webcompat.com
48-
if PRODUCTION:
53+
if env.PRODUCTION:
4954
GITHUB_CLIENT_ID = ''
5055
GITHUB_CLIENT_SECRET = ''
5156
GITHUB_CALLBACK_URL = ''
5257
# Staging = staging.webcompat.com
53-
elif STAGING:
58+
elif env.STAGING:
5459
GITHUB_CLIENT_ID = ''
5560
GITHUB_CLIENT_SECRET = ''
5661
GITHUB_CALLBACK_URL = ''
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"action": "closed",
3+
"issue": {
4+
"number": 600,
5+
"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/"
6+
}
7+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"action": "opened",
3+
"issue": {
4+
"number": 600,
5+
"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/"
6+
}
7+
}

tests/test_urls.py

-9
Original file line numberDiff line numberDiff line change
@@ -84,15 +84,6 @@ def test_issues_list_page(self):
8484
self.assertEqual(rv.status_code, 200)
8585
self.assertNotEqual(rv.status_code, 307)
8686

87-
def test_labeler_webhook(self):
88-
'''Webhook related tests.'''
89-
headers = {'X-GitHub-Event': 'ping'}
90-
rv = self.app.get('/webhooks/labeler', headers=headers)
91-
self.assertEqual(rv.status_code, 403)
92-
rv = self.app.post('/webhooks/labeler', headers=headers)
93-
# A random post should 401, only requests from GitHub will 200
94-
self.assertEqual(rv.status_code, 401)
95-
9687
def test_csp_report_uri(self):
9788
'''Test POST to /csp-report w/ correct content-type returns 204.'''
9889
headers = {'Content-Type': 'application/csp-report'}

tests/test_webhook.py

+122
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
#!/usr/bin/env python
2+
# -*- coding: utf-8 -*-
3+
# This Source Code Form is subject to the terms of the Mozilla Public
4+
# License, v. 2.0. If a copy of the MPL was not distributed with this
5+
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
6+
7+
"""Tests for our webhooks."""
8+
9+
import json
10+
import os
11+
import sys
12+
import unittest
13+
14+
from webcompat import app
15+
from webcompat.webhooks import helpers
16+
17+
# Add webcompat module to import path
18+
sys.path.append(os.path.realpath(os.pardir))
19+
import webcompat # nopep8
20+
21+
# The key is being used for testing and computing the signature.
22+
key = app.config['HOOK_SECRET_KEY']
23+
24+
25+
# Some machinery for opening our test files
26+
def event_data(filename):
27+
'''return a tuple with the content and its signature.'''
28+
current_root = os.path.realpath(os.curdir)
29+
events_path = 'tests/fixtures/webhooks'
30+
path = os.path.join(current_root, events_path, filename)
31+
with open(path, 'r') as f:
32+
json_event = json.dumps(json.load(f))
33+
signature = 'sha1={sig}'.format(
34+
sig=helpers.get_payload_signature(key, json_event))
35+
return json_event, signature
36+
37+
38+
class TestWebhook(unittest.TestCase):
39+
def setUp(self):
40+
webcompat.app.config['TESTING'] = True
41+
self.app = webcompat.app.test_client()
42+
self.headers = {'content-type': 'application/json'}
43+
self.test_url = '/webhooks/labeler'
44+
self.issue_body = """
45+
<!-- @browser: Firefox 55.0 -->
46+
<!-- @ua_header: Mozilla/5.0 (what) Gecko/20100101 Firefox/55.0 -->
47+
<!-- @reported_with: web -->
48+
49+
**URL**: https://www.example.com/
50+
**Browser / Version**: Firefox 55.0
51+
<!-- @browser: Chrome 48.0 -->
52+
"""
53+
self.issue_body2 = """
54+
<!-- @browser: Foobar -->
55+
"""
56+
57+
def tearDown(self):
58+
pass
59+
60+
def test_forbidden_get(self):
61+
"""GET is forbidden on labeler webhook."""
62+
rv = self.app.get(self.test_url, headers=self.headers)
63+
self.assertEqual(rv.status_code, 404)
64+
65+
def test_fail_on_missing_signature(self):
66+
"""POST without signature on labeler webhook is forbidden."""
67+
self.headers.update({'X-GitHub-Event': 'ping'})
68+
rv = self.app.post(self.test_url, headers=self.headers)
69+
self.assertEqual(rv.status_code, 401)
70+
71+
def test_fail_on_bogus_signature(self):
72+
"""POST without bogus signature on labeler webhook is forbidden."""
73+
json_event, signature = event_data('new_event_valid.json')
74+
self.headers.update({'X-GitHub-Event': 'ping',
75+
'X-Hub-Signature': 'Boo!'})
76+
rv = self.app.post(self.test_url,
77+
data=json_event,
78+
headers=self.headers)
79+
self.assertEqual(rv.status_code, 401)
80+
81+
def test_fail_on_invalid_event_type(self):
82+
"""POST with event not being 'issues' or 'ping' fails."""
83+
json_event, signature = event_data('new_event_valid.json')
84+
self.headers.update({'X-GitHub-Event': 'failme',
85+
'X-Hub-Signature': signature})
86+
rv = self.app.post(self.test_url,
87+
data=json_event,
88+
headers=self.headers)
89+
self.assertEqual(rv.status_code, 403)
90+
91+
def test_success_on_ping_event(self):
92+
"""POST with PING events just return a 200 and contains pong."""
93+
json_event, signature = event_data('new_event_valid.json')
94+
self.headers.update({'X-GitHub-Event': 'ping',
95+
'X-Hub-Signature': signature})
96+
rv = self.app.post(self.test_url,
97+
data=json_event,
98+
headers=self.headers)
99+
self.assertEqual(rv.status_code, 200)
100+
self.assertIn('pong', rv.data)
101+
102+
def test_fails_on_action_not_opened(self):
103+
"""POST with action different of opened fails."""
104+
json_event, signature = event_data('new_event_invalid.json')
105+
self.headers.update({'X-GitHub-Event': 'issues',
106+
'X-Hub-Signature': signature})
107+
rv = self.app.post(self.test_url,
108+
data=json_event,
109+
headers=self.headers)
110+
self.assertEqual(rv.status_code, 200)
111+
self.assertIn('cool story, bro.', rv.data)
112+
113+
def test_extract_browser_label(self):
114+
"""Extract browser label name."""
115+
browser_label = helpers.extract_browser_label(self.issue_body)
116+
self.assertEqual(browser_label, 'browser-firefox')
117+
browser_label_none = helpers.extract_browser_label(self.issue_body2)
118+
self.assertEqual(browser_label_none, None)
119+
120+
121+
if __name__ == '__main__':
122+
unittest.main()

webcompat/webhooks/__init__.py

+47-38
Original file line numberDiff line numberDiff line change
@@ -4,61 +4,70 @@
44
# License, v. 2.0. If a copy of the MPL was not distributed with this
55
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
66

7-
'''Flask Blueprint for our "webhooks" module, which we use to do cool things
8-
with GitHub events and actions.
7+
"""Flask Blueprint for our "webhooks" module.webhooks
98
10-
See https://developer.github.com/webhooks/ for what is possible.'''
9+
See https://developer.github.com/webhooks/ for what is possible."""
1110

1211
import json
12+
import logging
1313

1414
from flask import abort
1515
from flask import Blueprint
1616
from flask import request
1717

18-
from helpers import dump_to_db
19-
from helpers import parse_and_set_label
20-
from helpers import set_label
18+
from helpers import extract_browser_label
19+
from helpers import set_labels
2120
from helpers import signature_check
2221

2322
from webcompat import app
2423

25-
2624
webhooks = Blueprint('webhooks', __name__, url_prefix='/webhooks')
2725

2826

29-
@webhooks.route('/labeler', methods=['GET', 'POST'])
27+
@webhooks.route('/labeler', methods=['POST'])
3028
def hooklistener():
31-
'''Listen for the "issues" webhook event, parse the body
29+
"""Listen for the "issues" webhook event.
3230
33-
Method posts back labels and dumps data to a local db.
34-
Only in response to the 'opened' action, though.
35-
'''
36-
if request.method == 'GET':
37-
abort(403)
38-
elif request.method == 'POST':
39-
event_type = request.headers.get('X-GitHub-Event')
40-
post_signature = request.headers.get('X-Hub-Signature')
41-
if post_signature:
42-
key = app.config['HOOK_SECRET_KEY']
43-
payload = json.loads(request.data)
44-
if not signature_check(key, post_signature, request.data):
45-
abort(401)
46-
if event_type == 'issues':
47-
if payload.get('action') == 'opened':
48-
issue_body = payload.get('issue')['body']
49-
issue_title = payload.get('issue')['title']
50-
issue_number = payload.get('issue')['number']
51-
parse_and_set_label(issue_body, issue_number)
52-
# Setting "Needs Triage" label by default
53-
# to all the new issues raised
54-
set_label('status-needstriage', issue_number)
55-
dump_to_db(issue_title, issue_body, issue_number)
56-
return ('gracias, amigo.', 200)
31+
- Only in response to the 'opened' action (for now).
32+
- Add label needstriage to the issue
33+
- Add label for the browser name
34+
"""
35+
event_type = request.headers.get('X-GitHub-Event')
36+
post_signature = request.headers.get('X-Hub-Signature')
37+
if post_signature:
38+
key = app.config['HOOK_SECRET_KEY']
39+
payload = json.loads(request.data)
40+
if not signature_check(key, post_signature, request.data):
41+
abort(401)
42+
if event_type == 'issues':
43+
if payload.get('action') == 'opened':
44+
# Setting "Needs Triage" label by default
45+
# to all the new issues raised
46+
labels = ['status-needstriage']
47+
issue_body = payload.get('issue')['body']
48+
issue_number = payload.get('issue')['number']
49+
browser_label = extract_browser_label(issue_body)
50+
if browser_label:
51+
labels.append(browser_label)
52+
# Sending a request to set labels
53+
response = set_labels(labels, issue_number)
54+
if response.status_code == 200:
55+
return ('gracias, amigo.', 200,
56+
{'Content-Type': 'plain/text'})
5757
else:
58-
return ('cool story, bro.', 200)
59-
elif event_type == 'ping':
60-
return ('pong', 200)
58+
# Logging the ip and url for investigation
59+
log = app.logger
60+
log.setLevel(logging.INFO)
61+
msg = 'failed to set labels on issue {issue}'.format(
62+
issue=issue_number)
63+
log.info(msg)
6164
else:
62-
abort(403)
65+
return ('cool story, bro.', 200,
66+
{'Content-Type': 'plain/text'})
67+
elif event_type == 'ping':
68+
return ('pong', 200, {'Content-Type': 'plain/text'})
6369
else:
64-
abort(401)
70+
abort(403)
71+
else:
72+
# No signature.
73+
abort(401)

0 commit comments

Comments
 (0)