-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support: update contact information via Front webhook #8406
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
"""Front's API client.""" | ||
|
||
import requests | ||
|
||
|
||
class FrontClient: | ||
|
||
"""Wrapper around Front's API.""" | ||
|
||
BASE_URL = 'https://api2.frontapp.com' | ||
|
||
def __init__(self, token): | ||
self.token = token | ||
|
||
@property | ||
def _headers(self): | ||
headers = { | ||
"Authorization": f"Bearer {self.token}", | ||
} | ||
return headers | ||
|
||
def _get_url(self, path): | ||
return f'{self.BASE_URL}{path}' | ||
|
||
def get(self, path, **kwargs): | ||
kwargs.setdefault('headers', {}).update(self._headers) | ||
return requests.get(self._get_url(path), **kwargs) | ||
|
||
def patch(self, path, **kwargs): | ||
kwargs.setdefault('headers', {}).update(self._headers) | ||
return requests.patch(self._get_url(path), **kwargs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
from unittest import mock | ||
|
||
import requests_mock | ||
from django.contrib.auth.models import User | ||
from django.test import TestCase, override_settings | ||
from django.urls import reverse | ||
from django_dynamic_fixture import get | ||
|
||
from readthedocs.support.views import FrontWebhookBase | ||
|
||
|
||
@override_settings( | ||
ADMIN_URL='https://readthedocs.org/admin', | ||
FRONT_API_SECRET='1234', | ||
FRONT_TOKEN='1234', | ||
) | ||
class TestFrontWebhook(TestCase): | ||
|
||
def setUp(self): | ||
self.user = get(User, email='test@example.com', username='test') | ||
self.url = reverse('front_webhook') | ||
|
||
def test_invalid_payload(self): | ||
resp = self.client.post( | ||
self.url, | ||
data={'foo': 'bar'}, | ||
content_type='application/json', | ||
) | ||
self.assertEqual(resp.status_code, 400) | ||
self.assertEqual(resp.data['detail'], 'Invalid payload') | ||
|
||
@mock.patch.object(FrontWebhookBase, '_is_payload_valid') | ||
def test_invalid_event(self, is_payload_valid): | ||
is_payload_valid.return_value = True | ||
resp = self.client.post( | ||
self.url, | ||
data={'type': 'outbound'}, | ||
content_type='application/json', | ||
) | ||
self.assertEqual(resp.status_code, 200) | ||
self.assertEqual(resp.data['detail'], 'Skipping outbound event') | ||
|
||
@requests_mock.Mocker(kw='mock_request') | ||
@mock.patch.object(FrontWebhookBase, '_is_payload_valid') | ||
def test_inbound_event(self, is_payload_valid, mock_request): | ||
is_payload_valid.return_value = True | ||
self._mock_request(mock_request) | ||
resp = self.client.post( | ||
self.url, | ||
data={ | ||
'type': 'inbound', | ||
'conversation': {'id': 'cnv_123'} | ||
}, | ||
content_type='application/json', | ||
) | ||
self.assertEqual(resp.status_code, 200) | ||
self.assertEqual(resp.data['detail'], 'User updated test@example.com') | ||
last_request = mock_request.last_request | ||
self.assertEqual(last_request.method, 'PATCH') | ||
# Existing custom fields are left unchanged. | ||
custom_fields = last_request.json()['custom_fields'] | ||
for field in ['com:dont-change', 'org:dont-change', 'ads:dont-change']: | ||
self.assertEqual(custom_fields[field], 'Do not change this') | ||
|
||
@requests_mock.Mocker(kw='mock_request') | ||
@mock.patch.object(FrontWebhookBase, '_is_payload_valid') | ||
def test_inbound_event_unknow_email(self, is_payload_valid, mock_request): | ||
self.user.email = 'unknown@example.com' | ||
self.user.save() | ||
is_payload_valid.return_value = True | ||
self._mock_request(mock_request) | ||
resp = self.client.post( | ||
self.url, | ||
data={ | ||
'type': 'inbound', | ||
'conversation': {'id': 'cnv_123'} | ||
}, | ||
content_type='application/json', | ||
) | ||
self.assertEqual(resp.status_code, 200) | ||
self.assertEqual( | ||
resp.data['detail'], | ||
'User with email test@example.com not found in our database', | ||
) | ||
|
||
def _mock_request(self, mock_request): | ||
mock_request.get( | ||
'https://api2.frontapp.com/conversations/cnv_123', | ||
json={ | ||
'recipient': { | ||
'handle': 'test@example.com', | ||
}, | ||
}, | ||
) | ||
mock_request.get( | ||
'https://api2.frontapp.com/contacts/alt:email:test@example.com', | ||
json={ | ||
'custom_fields': { | ||
'org:dont-change': 'Do not change this', | ||
'com:dont-change': 'Do not change this', | ||
'ads:dont-change': 'Do not change this', | ||
}, | ||
}, | ||
) | ||
mock_request.patch('https://api2.frontapp.com/contacts/alt:email:test@example.com') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from django.conf.urls import url | ||
|
||
from readthedocs.support.views import FrontWebhook | ||
|
||
urlpatterns = [ | ||
url(r'^front-webhook/$', FrontWebhook.as_view(), name='front_webhook'), | ||
stsewd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
] |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,144 @@ | ||||||
"""Support views.""" | ||||||
|
||||||
import base64 | ||||||
import hashlib | ||||||
import hmac | ||||||
import logging | ||||||
|
||||||
from django.conf import settings | ||||||
from django.contrib.auth.models import User | ||||||
from django.db.models import Q | ||||||
from rest_framework.response import Response | ||||||
from rest_framework.status import ( | ||||||
HTTP_400_BAD_REQUEST, | ||||||
HTTP_500_INTERNAL_SERVER_ERROR, | ||||||
) | ||||||
from rest_framework.views import APIView | ||||||
|
||||||
from readthedocs.core.utils.extend import SettingsOverrideObject | ||||||
from readthedocs.support.front import FrontClient | ||||||
|
||||||
log = logging.getLogger(__name__) | ||||||
|
||||||
|
||||||
class FrontWebhookBase(APIView): | ||||||
|
||||||
""" | ||||||
Front's webhook handler. | ||||||
|
||||||
Currently we only listen to inbound messages events. | ||||||
Contact information is updated when a new message is received. | ||||||
|
||||||
See https://dev.frontapp.com/docs/webhooks-1. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Make the link clickable 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works for me with the period (gnome-terminal and kitty) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. He, it doesn't work on GitHub 🙃 |
||||||
""" | ||||||
|
||||||
http_method_names = ['post'] | ||||||
|
||||||
def post(self, request): | ||||||
if not self._is_payload_valid(): | ||||||
return Response( | ||||||
{'detail': 'Invalid payload'}, | ||||||
status=HTTP_400_BAD_REQUEST, | ||||||
) | ||||||
|
||||||
event = request.data.get('type') | ||||||
if event == 'inbound': | ||||||
return self._update_contact_information(request.data) | ||||||
return Response({'detail': f'Skipping {event} event'}) | ||||||
|
||||||
def _update_contact_information(self, data): | ||||||
""" | ||||||
Update contact information using Front's API. | ||||||
|
||||||
The webhook event give us the conversation_id, | ||||||
we use that to | ||||||
stsewd marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
""" | ||||||
client = FrontClient(token=settings.FRONT_TOKEN) | ||||||
|
||||||
# Retrieve the user from the email from the conversation. | ||||||
conversation_id = data.get('conversation', {}).get('id') | ||||||
try: | ||||||
resp = client.get(f'/conversations/{conversation_id}').json() | ||||||
email = resp.get('recipient', {}).get('handle') | ||||||
except Exception: # noqa | ||||||
msg = f'Error while getting conversation {conversation_id}' | ||||||
log.exception(msg) | ||||||
return Response({'detail': msg}, status=HTTP_500_INTERNAL_SERVER_ERROR) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should return 500 here. It's not our own error. Maybe 400 or 404 is better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an error when connecting to the front API, not an error on how the request was done, so not a 4xx. |
||||||
|
||||||
user = ( | ||||||
User.objects | ||||||
.filter(Q(email=email) | Q(emailaddress__email=email)) | ||||||
.first() | ||||||
) | ||||||
if not user: | ||||||
return Response({'detail': f'User with email {email} not found in our database'}) | ||||||
stsewd marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
# Get current custom fields, and update them. | ||||||
try: | ||||||
resp = client.get(f'/contacts/alt:email:{email}').json() | ||||||
except Exception: # noqa | ||||||
msg = f'Error while getting contact {email}' | ||||||
log.exception(msg) | ||||||
return Response({'detail': msg}, HTTP_500_INTERNAL_SERVER_ERROR) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. 500 is not a good error IMO for external errors. Also, the exception message could be improved by showing the URL tried: |
||||||
|
||||||
new_custom_fields = self._get_custom_fields(user) | ||||||
custom_fields = resp.get('custom_fields', {}) | ||||||
custom_fields.update(new_custom_fields) | ||||||
|
||||||
try: | ||||||
client.patch( | ||||||
f'/contacts/alt:email:{email}', | ||||||
json={'custom_fields': custom_fields} | ||||||
) | ||||||
except Exception: # noqa | ||||||
msg = f'Error while updating contact information for {email}' | ||||||
log.exception(msg) | ||||||
return Response( | ||||||
{ | ||||||
'detail': msg, | ||||||
'custom_fields': new_custom_fields, | ||||||
}, | ||||||
status=HTTP_500_INTERNAL_SERVER_ERROR, | ||||||
) | ||||||
else: | ||||||
return Response({ | ||||||
'detail': f'User updated {email}', | ||||||
'custom_fields': new_custom_fields, | ||||||
}) | ||||||
|
||||||
# pylint: disable=no-self-use | ||||||
def _get_custom_fields(self, user): | ||||||
""" | ||||||
Attach custom fields for this user. | ||||||
|
||||||
These fields need to be created on Front (settings -> Contacts -> Custom Fields). | ||||||
""" | ||||||
custom_fields = {} | ||||||
custom_fields['org:username'] = user.username | ||||||
custom_fields['org:admin'] = f'{settings.ADMIN_URL}/auth/user/{user.pk}/change' | ||||||
return custom_fields | ||||||
|
||||||
def _is_payload_valid(self): | ||||||
""" | ||||||
Check if the signature and the payload from the webhook matches. | ||||||
|
||||||
https://dev.frontapp.com/docs/webhooks-1#validating-data-integrity | ||||||
""" | ||||||
digest = self._get_digest() | ||||||
signature = self.request.headers.get('X-Front-Signature', '') | ||||||
result = hmac.compare_digest(digest, signature.encode()) | ||||||
return result | ||||||
|
||||||
def _get_digest(self): | ||||||
"""Get a HMAC digest of the request using Front's API secret.""" | ||||||
secret = settings.FRONT_API_SECRET | ||||||
digest = hmac.new( | ||||||
secret.encode(), | ||||||
msg=self.request.body, | ||||||
digestmod=hashlib.sha1, | ||||||
) | ||||||
return base64.b64encode(digest.digest()) | ||||||
|
||||||
|
||||||
class FrontWebhook(SettingsOverrideObject): | ||||||
_default_class = FrontWebhookBase |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ | |
url(r'^support/error/$', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can move these to the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems reasonable 👍 |
||
TemplateView.as_view(template_name='support/error.html'), | ||
name='support_error'), | ||
url(r'^support/', include('readthedocs.support.urls')), | ||
] | ||
|
||
rtd_urls = [ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like overkill for 3 API calls. Is there a reason that we're using a wrapper instead of just calling the API directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to pass the token header on each request, felt like a lot of duplication, I'm fine moving this class to the views file. Can also merge this into the view class, but not a big fan of fat-views.