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

Webhook Custom HTTP Method + Basic Auth Support #4124

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions awx/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -4237,6 +4237,8 @@ def validate(self, attrs):
continue
if field_type == "password" and field_val == "$encrypted$" and object_actual is not None:
attrs['notification_configuration'][field] = object_actual.notification_configuration[field]
if field == "http_method" and field_val.lower() not in ['put', 'post']:
error_list.append(_("HTTP method must be either 'POST' or 'PUT'."))
if missing_fields:
error_list.append(_("Missing required fields for Notification Configuration: {}.").format(missing_fields))
if incorrect_type_fields:
Expand Down
25 changes: 25 additions & 0 deletions awx/main/migrations/0082_v360_webhook_http_method.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations


def add_webhook_notification_template_fields(apps, schema_editor):
# loop over all existing webhook notification templates and make
# sure they have the new "http_method" field filled in with "POST"
NotificationTemplate = apps.get_model('main', 'notificationtemplate')
webhooks = NotificationTemplate.objects.filter(notification_type='webhook')
for w in webhooks:
w.notification_configuration['http_method'] = 'POST'
w.save()


class Migration(migrations.Migration):

dependencies = [
('main', '0081_v360_notify_on_start'),
]

operations = [
migrations.RunPython(add_webhook_notification_template_fields, migrations.RunPython.noop),
]
7 changes: 4 additions & 3 deletions awx/main/models/notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,10 @@ def generate_notification(self, subject, message):
def send(self, subject, body):
for field in filter(lambda x: self.notification_class.init_parameters[x]['type'] == "password",
self.notification_class.init_parameters):
self.notification_configuration[field] = decrypt_field(self,
'notification_configuration',
subfield=field)
if field in self.notification_configuration:
self.notification_configuration[field] = decrypt_field(self,
'notification_configuration',
subfield=field)
recipients = self.notification_configuration.pop(self.notification_class.recipient_parameter)
if not isinstance(recipients, list):
recipients = [recipients]
Expand Down
17 changes: 15 additions & 2 deletions awx/main/notifications/webhook_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,20 @@
class WebhookBackend(AWXBaseEmailBackend):

init_parameters = {"url": {"label": "Target URL", "type": "string"},
"http_method": {"label": "HTTP Method", "type": "string", "default": "POST"},
"disable_ssl_verification": {"label": "Verify SSL", "type": "bool", "default": False},
"username": {"label": "Username", "type": "string", "default": ""},
"password": {"label": "Password", "type": "password", "default": ""},
"headers": {"label": "HTTP Headers", "type": "object"}}
recipient_parameter = "url"
sender_parameter = None

def __init__(self, headers, disable_ssl_verification=False, fail_silently=False, **kwargs):
def __init__(self, http_method, headers, disable_ssl_verification=False, fail_silently=False, username=None, password=None, **kwargs):
self.http_method = http_method
self.disable_ssl_verification = disable_ssl_verification
self.headers = headers
self.username = username
self.password = password
super(WebhookBackend, self).__init__(fail_silently=fail_silently)

def format_body(self, body):
Expand All @@ -32,8 +38,15 @@ def send_messages(self, messages):
sent_messages = 0
if 'User-Agent' not in self.headers:
self.headers['User-Agent'] = "Tower {}".format(get_awx_version())
if self.http_method.lower() not in ['put','post']:
Copy link
Contributor

Choose a reason for hiding this comment

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

Due to this change here[1] this is not necessary anymore . We know it will only be PUT or POST

[1] https://github.com/ansible/awx/pull/4124/files#diff-a81324c523b41de7296fdd5ff9063d10R4240

Copy link
Contributor

@ryanpetrello ryanpetrello Jul 23, 2019

Choose a reason for hiding this comment

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

I disagree; I'd prefer to program defensively here in case the code ever changes (perhaps in the future, the validation code in the serializer code stops working properly, or perhaps we add some additional ORM code elsewhere that sets the HTTP method without properly validating it).

This if statement doesn't cost much, and it's a nice final line of defense against an uncaught exception.

Copy link
Contributor

@Spredzy Spredzy Jul 23, 2019

Choose a reason for hiding this comment

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

Fair. The thing that bothers me a bit is the DRY principles not being followed here.

if field == "http_method" and field_val.lower() not in ['put', 'post']

and

if self.http_method.lower() not in ['put','post']:

Could we have ['put', 'post'] set somewhere and have those statement becomes something like if self.http_method.lower() not in SUPPORTED_HTTP_METHOD like statement ?

This approach - I think - would satisfy both point of views

Note: Error message should rely upon this new variables too

Copy link
Contributor

@ryanpetrello ryanpetrello Jul 23, 2019

Choose a reason for hiding this comment

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

I'm a big fan of DRY as long as the reuse actually saves you something 😄. This is a one line if statement that's succinct, easy to read, and not likely to change drastically in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit late to this conversation, but I wanted to show why I left that code in the webhook_backend.py file. IF somehow there is a nonsense HTTP method that gets past the UI and the serializer validator (admittedly this is a very unlikely scenario), without that code this is the error that shows up in the UI:

NoCatchInBackend

This doesn't really explain what exactly went wrong. The code that I left in webhook_backend.py causes this error message to show up instead, which explains what exactly the user can fix:

ErrorCaughtInBackend

raise ValueError("HTTP method must be either 'POST' or 'PUT'.")
chosen_method = getattr(requests, self.http_method.lower(), None)
for m in messages:
r = requests.post("{}".format(m.recipients()[0]),
auth = None
if self.username or self.password:
auth = (self.username, self.password)
r = chosen_method("{}".format(m.recipients()[0]),
auth=auth,
json=m.body,
headers=self.headers,
verify=(not self.disable_ssl_verification))
Expand Down
9 changes: 4 additions & 5 deletions awx/main/tests/factories/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def mk_credential(name, credential_type='ssh', persisted=True):
def mk_notification_template(name, notification_type='webhook', configuration=None, organization=None, persisted=True):
nt = NotificationTemplate(name=name)
nt.notification_type = notification_type
nt.notification_configuration = configuration or dict(url="http://localhost", headers={"Test": "Header"})
nt.notification_configuration = configuration or dict(url="http://localhost", username="", password="", headers={"Test": "Header"})

if organization is not None:
nt.organization = organization
Expand Down Expand Up @@ -216,7 +216,7 @@ def mk_workflow_job_template(name, extra_vars='', spec=None, organization=None,


def mk_workflow_job_template_node(workflow_job_template=None,
unified_job_template=None,
unified_job_template=None,
success_nodes=None,
failure_nodes=None,
always_nodes=None,
Expand All @@ -231,11 +231,11 @@ def mk_workflow_job_template_node(workflow_job_template=None,
return workflow_node


def mk_workflow_job_node(unified_job_template=None,
def mk_workflow_job_node(unified_job_template=None,
success_nodes=None,
failure_nodes=None,
always_nodes=None,
workflow_job=None,
workflow_job=None,
job=None,
persisted=True):
workflow_node = WorkflowJobNode(unified_job_template=unified_job_template,
Expand All @@ -247,4 +247,3 @@ def mk_workflow_job_node(unified_job_template=None,
if persisted:
workflow_node.save()
return workflow_node

4 changes: 3 additions & 1 deletion awx/main/tests/functional/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,9 @@ def notification_template(organization):
organization=organization,
notification_type="webhook",
notification_configuration=dict(url="http://localhost",
headers={"Test": "Header"}))
username="",
password="",
headers={"Test": "Header",}))


@pytest.fixture
Expand Down
4 changes: 2 additions & 2 deletions awx/main/tests/functional/test_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ def test_inherited_notification_templates(get, post, user, organization, project
isrc.save()
jt = JobTemplate.objects.create(name='test', inventory=i, project=project, playbook='debug.yml')
jt.save()


@pytest.mark.django_db
def test_notification_template_simple_patch(patch, notification_template, admin):
Expand Down Expand Up @@ -124,7 +124,7 @@ def test_custom_environment_injection(post, user, organization):
organization=organization.id,
notification_type="webhook",
notification_configuration=dict(url="https://example.org", disable_ssl_verification=False,
headers={"Test": "Header"})),
http_method="POST", headers={"Test": "Header"})),
u)
assert response.status_code == 201
template = NotificationTemplate.objects.get(pk=response.data['id'])
Expand Down
9 changes: 9 additions & 0 deletions awx/ui/client/src/notifications/add/add.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,15 @@ export default ['Rest', 'Wait', 'NotificationsFormObject',
element: '#notification_template_color',
multiple: false
});

$scope.httpMethodChoices = [
{'id': 'POST', 'name': i18n._('POST')},
{'id': 'PUT', 'name': i18n._('PUT')},
];
CreateSelect2({
element: '#notification_template_http_method',
multiple: false,
});
});

$scope.$watch('headers', function validate_headers(str) {
Expand Down
10 changes: 10 additions & 0 deletions awx/ui/client/src/notifications/edit/edit.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,16 @@ export default ['Rest', 'Wait',
element: '#notification_template_color',
multiple: false
});

$scope.httpMethodChoices = [
{'id': 'POST', 'name': i18n._('POST')},
{'id': 'PUT', 'name': i18n._('PUT')},
];
CreateSelect2({
element: '#notification_template_http_method',
multiple: false,
});

NotificationsTypeChange.getDetailFields($scope.notification_type.value).forEach(function(field) {
$scope[field[0]] = field[1];
});
Expand Down
8 changes: 6 additions & 2 deletions awx/ui/client/src/notifications/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,12 @@ angular.module('notifications', [
var url = getBasePath('notification_templates') + notificationTemplateId + '/';
rest.setUrl(url);
return rest.get()
.then(function(data) {
return data.data;
.then(function(res) {
if (_.get(res, ['data', 'notification_type'] === 'webhook') &&
_.get(res, ['data', 'notification_configuration', 'http_method'])) {
res.data.notification_configuration.http_method = res.data.notification_configuration.http_method.toUpperCase();
}
return res.data;
}).catch(function(response) {
ProcessErrors(null, response.data, response.status, null, {
hdr: 'Error!',
Expand Down
19 changes: 17 additions & 2 deletions awx/ui/client/src/notifications/notificationTemplates.form.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export default ['i18n', function(i18n) {
username: {
label: i18n._('Username'),
type: 'text',
ngShow: "notification_type.value == 'email' ",
ngShow: "notification_type.value == 'email' || notification_type.value == 'webhook' ",
subForm: 'typeSubForm',
ngDisabled: '!(notification_template.summary_fields.user_capabilities.edit || canAdd)'
},
Expand All @@ -75,7 +75,7 @@ export default ['i18n', function(i18n) {
reqExpression: "password_required" ,
init: "false"
},
ngShow: "notification_type.value == 'email' || notification_type.value == 'irc' ",
ngShow: "notification_type.value == 'email' || notification_type.value == 'irc' || notification_type.value == 'webhook' ",
subForm: 'typeSubForm',
ngDisabled: '!(notification_template.summary_fields.user_capabilities.edit || canAdd)'
},
Expand Down Expand Up @@ -423,6 +423,21 @@ export default ['i18n', function(i18n) {
subForm: 'typeSubForm',
ngDisabled: '!(notification_template.summary_fields.user_capabilities.edit || canAdd)'
},
http_method: {
label: i18n._('HTTP Method'),
dataTitle: i18n._('HTTP Method'),
type: 'select',
ngOptions: 'choice.id as choice.name for choice in httpMethodChoices',
default: 'post',
awPopOver: i18n._('Specify an HTTP method for the webhook. Acceptable choices are: POST or PUT'),
awRequiredWhen: {
reqExpression: "webhook_required",
init: "false"
},
ngShow: "notification_type.value == 'webhook' ",
subForm: 'typeSubForm',
ngDisabled: '!(notification_template.summary_fields.user_capabilities.edit || canAdd)'
},
mattermost_url: {
label: i18n._('Target URL'),
type: 'text',
Expand Down
2 changes: 2 additions & 0 deletions awx/ui/client/src/notifications/shared/type-change.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ function (i18n) {
break;
case 'webhook':
obj.webhook_required = true;
obj.passwordLabel = ' ' + i18n._('Basic Auth Password');
obj.password_required = false;
break;
case 'mattermost':
obj.mattermost_required = true;
Expand Down
8 changes: 8 additions & 0 deletions docs/notification_system.md
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,14 @@ https://gist.github.com/matburt/73bfbf85c2443f39d272
The link below shows how to define an endpoint and parse headers and json content. It doesn't show how to configure Flask for HTTPS, but is fairly straightforward:
http://flask.pocoo.org/snippets/111/

You can also link an `httpbin` service to the development environment for testing webhooks using:

```
docker run --network="tools_default" --name httpbin -p 8204:80 kennethreitz/httpbin
```

This will create an `httpbin` service reachable from the AWX container at `http://httpbin/post`, `http://httpbin/put`, etc. Outside of the container, you can reach the service at `http://localhost:8204`.


## Grafana

Expand Down