Skip to content
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

Merged
merged 3 commits into from
Aug 24, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions readthedocs/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ class CommunityBaseSettings(Settings):
SERVER_EMAIL = DEFAULT_FROM_EMAIL
SUPPORT_EMAIL = None
SUPPORT_FORM_ENDPOINT = None
FRONT_TOKEN = None
FRONT_API_SECRET = None

# Sessions
SESSION_COOKIE_DOMAIN = 'readthedocs.org'
Expand Down Expand Up @@ -623,6 +625,7 @@ def DOCKER_LIMITS(self):
DEFAULT_VERSION_PRIVACY_LEVEL = 'public'
GROK_API_HOST = 'https://api.grokthedocs.com'
ALLOW_ADMIN = True
ADMIN_URL = None
RTD_ALLOW_ORGANIZATIONS = False

# Elasticsearch settings.
Expand Down
Empty file added readthedocs/support/__init__.py
Empty file.
31 changes: 31 additions & 0 deletions readthedocs/support/front.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
"""Front's API client."""

import requests


class FrontClient:
Copy link
Member

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?

Copy link
Member Author

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.


"""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)
Empty file.
105 changes: 105 additions & 0 deletions readthedocs/support/tests/test_views.py
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')
7 changes: 7 additions & 0 deletions readthedocs/support/urls.py
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
]
144 changes: 144 additions & 0 deletions readthedocs/support/views.py
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
See https://dev.frontapp.com/docs/webhooks-1.
See https://dev.frontapp.com/docs/webhooks-1

Make the link clickable 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me with the period (gnome-terminal and kitty)

Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The 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: url=...


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
8 changes: 0 additions & 8 deletions readthedocs/templates/support/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,6 @@ <h2 id="user-support">User Support</h2>
{% endif %}
</div>


{% if request.user.is_authenticated %}
<input type="hidden" name="username" value="{{ request.user.username }}">
<input type="hidden" name="pk" value="{{ request.user.pk }}">
{% else %}
<input type="hidden" name="auth" value="Unknown">
{% endif %}

<input type="hidden" name="subject" value="Community Support Request">

<input type="submit" value="Submit support request">
Expand Down
1 change: 1 addition & 0 deletions readthedocs/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
url(r'^support/error/$',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move these to the new support module in this PR if that's ok.

Copy link
Member

Choose a reason for hiding this comment

The 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 = [
Expand Down