From f7c1a52ced2e4e5eee803b49b724ee9ba6bf00af Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Thu, 10 Dec 2020 08:22:05 -0500 Subject: [PATCH 01/33] base webhook integration --- env/magpie.env.example | 2 ++ magpie/api/management/user/user_utils.py | 30 ++++++++++++++-- requirements-dev.txt | 1 + tests/runner.py | 2 +- tests/test_magpie_api.py | 31 +++++++++++++++- tests/utils.py | 45 +++++++++++++++++++++++- 6 files changed, 106 insertions(+), 5 deletions(-) diff --git a/env/magpie.env.example b/env/magpie.env.example index ee1a7139b..4cd9b10d6 100644 --- a/env/magpie.env.example +++ b/env/magpie.env.example @@ -15,6 +15,7 @@ MAGPIE_LOG_REQUEST=true MAGPIE_LOG_EXCEPTION=true MAGPIE_UI_ENABLED=true MAGPIE_CONFIG_DIR=../config +MAGPIE_WEBHOOK_PRE_USER_CREATION_URL= PHOENIX_USER=phoenix PHOENIX_PASSWORD=qwerty PHOENIX_PORT=8443 @@ -25,6 +26,7 @@ TWITCHER_PROTECTED_PATH=/ows/proxy MAGPIE_TEST_ADMIN_USERNAME=admin MAGPIE_TEST_ADMIN_PASSWORD=qwerty MAGPIE_TEST_REMOTE_SERVER_URL=http://localhost:2001 +MAGPIE_TEST_WEBHOOK_PRE_USER_CREATION_URL=http://localhost:8080 MAGPIE_TEST_LOCAL=true MAGPIE_TEST_REMOTE=true MAGPIE_TEST_API=true diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index e93d98fa7..0897df0f0 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -1,5 +1,6 @@ from typing import TYPE_CHECKING +import requests import six from pyramid.httpexceptions import ( HTTPBadRequest, @@ -8,6 +9,7 @@ HTTPForbidden, HTTPInternalServerError, HTTPNotFound, + HTTPFailedDependency, HTTPOk ) from ziggurat_foundations.models.services.group import GroupService @@ -18,6 +20,7 @@ from magpie import models from magpie.api import exception as ax from magpie.api import schemas as s +from magpie.api.exception import raise_http from magpie.api.management.resource import resource_utils as ru from magpie.api.management.service.service_formats import format_service from magpie.api.management.user import user_formats as uf @@ -106,8 +109,31 @@ def _add_to_group(usr, grp): _add_to_group(new_user, _get_group(anonym_grp_name)) new_user_groups.append(anonym_grp_name) - return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, - content={"user": uf.format_user(new_user, new_user_groups)}) + # user_name_list = ax.evaluate_call(lambda: [user.user_name for user in + # UserService.all(models.User, db_session=db_session)], + # fallback=lambda: db_session.rollback(), http_error=HTTPForbidden, + # msg_on_fail=s.Users_GET_ForbiddenResponseSchema.description) + # print(user_name_list) + + valid_http = ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, + content={"user": uf.format_user(new_user, new_user_groups)}) + + webhook_url = get_constant("MAGPIE_WEBHOOK_PRE_USER_CREATION_URL") + + # FIXME: add try, in case url is not available + if webhook_url is not None and webhook_url != "": + try: + # TODO: set status to 0 + resp = requests.post(webhook_url + "/create", data={'user_name': user_name}) + print(resp.status_code) + print(resp.text) + except Exception as e: + # TODO: clean up, remove pending user + raise_http(http_error=HTTPFailedDependency, + detail="Error occurred during parameter verification") + # TODO: reset status to finished + + return valid_http def create_user_resource_permission_response(user, resource, permission, db_session, overwrite=False): diff --git a/requirements-dev.txt b/requirements-dev.txt index 35ff90715..ab81f4f03 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -18,3 +18,4 @@ pyramid-twitcher==0.5.3 pytest tox>=3.0 webtest +waitress diff --git a/tests/runner.py b/tests/runner.py index 241339d8d..4beb41e8f 100644 --- a/tests/runner.py +++ b/tests/runner.py @@ -33,7 +33,7 @@ def filter_test_files(root, filename): MAGPIE_TEST_RESOURCES = RunOptionDecorator("MAGPIE_TEST_RESOURCES", "magpie resources operations") MAGPIE_TEST_PERMISSIONS = RunOptionDecorator("MAGPIE_TEST_PERMISSIONS", "magpie permissions operations") MAGPIE_TEST_GROUPS = RunOptionDecorator("MAGPIE_TEST_GROUPS", "magpie groups operations") -MAGPIE_TEST_USERS = RunOptionDecorator("MAGPIE_TEST_USERS", "magpie groups operations") +MAGPIE_TEST_USERS = RunOptionDecorator("MAGPIE_TEST_USERS", "magpie users operations") MAGPIE_TEST_LOGGED = RunOptionDecorator("MAGPIE_TEST_LOGGED", "validate 'Logged User' views (i.e.: current)") MAGPIE_TEST_STATUS = RunOptionDecorator("MAGPIE_TEST_STATUS", "validate views found/displayed as per permissions") MAGPIE_TEST_PUBLIC = RunOptionDecorator("MAGPIE_TEST_PUBLIC", "validate evaluated permission on 'Public' resources") diff --git a/tests/test_magpie_api.py b/tests/test_magpie_api.py index c63ec7091..0c00f4d01 100644 --- a/tests/test_magpie_api.py +++ b/tests/test_magpie_api.py @@ -7,7 +7,7 @@ Tests for :mod:`magpie.api` module. """ - +import os import unittest import mock @@ -107,6 +107,35 @@ def setUpClass(cls): cls.check_requirements() cls.setup_test_values() + def test_CreateUser_Webhook(self): + """ + Test creating a user using a webhook. + """ + with mock.patch.dict(os.environ, {'MAGPIE_WEBHOOK_PRE_USER_CREATION_URL': + get_constant('MAGPIE_TEST_WEBHOOK_PRE_USER_CREATION_URL')}): + utils.get_test_webhook_app() + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + assert 0 == 1 + + def test_CreateUser_FailingWebhook(self): + """ + Test creating a user using a failing webhook url. + """ + with mock.patch.dict(os.environ, {'MAGPIE_WEBHOOK_PRE_USER_CREATION_URL': + get_constant('MAGPIE_TEST_WEBHOOK_PRE_USER_CREATION_URL')}): + utils.get_test_webhook_app() + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + assert 0 == 1 + + def test_CreateUser_EmptyWebhook(self): + """ + Test creating a user with an empty webhook url. + """ + with mock.patch.dict(os.environ, {'MAGPIE_WEBHOOK_PRE_USER_CREATION_URL': ""}): + utils.get_test_webhook_app() + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + assert 0 == 1 + @runner.MAGPIE_TEST_API @runner.MAGPIE_TEST_REMOTE diff --git a/tests/utils.py b/tests/utils.py index 89fff6da5..e3bdb9dc1 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,6 +1,11 @@ +import os +from errno import EADDRINUSE +import threading +from cgi import FieldStorage import functools import itertools import json as json_pkg # avoid conflict name with json argument employed for some function +from time import sleep import unittest import warnings from distutils.version import LooseVersion @@ -11,10 +16,13 @@ import requests import requests.exceptions import six -from pyramid.httpexceptions import HTTPException +from pyramid.config import Configurator +from pyramid.response import Response +from pyramid.httpexceptions import HTTPException, HTTPInternalServerError from pyramid.settings import asbool from pyramid.testing import DummyRequest, setUp as PyramidSetUp from six.moves.urllib.parse import urlparse +from waitress import serve from webtest.app import AppError, TestApp # noqa from webtest.forms import Form from webtest.response import TestResponse @@ -241,6 +249,41 @@ def get_app_or_url(test_item): return app_or_url +def get_test_webhook_app(): + """ + Instantiate a local test application used for the prehook for user creation and deletion. + """ + def webhook_create(request): + user = request.POST['user_name'] + if user == 'failure_user': + # Simulates an error in the webhook process + return HTTPInternalServerError() + sleep(1) + return Response(user + " webhook") + + def webhook_app(): + with Configurator() as config: + config.add_route('create', '/create') + config.add_view(webhook_create, route_name='create', request_method="POST", request_param="user_name") + app = config.make_wsgi_app() + try: + webhook_url_info = urlparse(get_constant("MAGPIE_WEBHOOK_PRE_USER_CREATION_URL")) + serve(app, host=webhook_url_info.hostname, port=webhook_url_info.port) + except OSError as e: + if e.errno == EADDRINUSE: + # The app is already running, nothing to do. + return + else: + raise + x = threading.Thread(target=webhook_app, daemon=True) + x.start() + + # FIXME: sometimes an error for first request, with BadGateway + # (probably the app didn't had the time to startup completely) + + return + + def get_hostname(test_item): # type: (AnyMagpieTestItemType) -> Str """ From fa0ab137bd54e3f136914c834e66acab870e3f18 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Thu, 17 Dec 2020 08:40:08 -0500 Subject: [PATCH 02/33] change webhook feature and add all webhook tests --- config/config.yml | 11 ++ config/magpie.ini | 2 +- env/magpie.env.example | 1 - magpie/api/management/user/user_formats.py | 1 + magpie/api/management/user/user_utils.py | 62 ++++---- magpie/api/management/user/user_views.py | 16 +++ tests/test_magpie_api.py | 160 ++++++++++++++++++--- tests/utils.py | 52 ++++--- 8 files changed, 241 insertions(+), 64 deletions(-) diff --git a/config/config.yml b/config/config.yml index a23d2c117..4d341a81d 100644 --- a/config/config.yml +++ b/config/config.yml @@ -34,3 +34,14 @@ groups: - name: # required if entry provided description: # optional discoverable: True # optional + +# Definitions of all the webhooks urls that will be called when creating or deleting a users. +webhooks: + create : + - + - + - ... + delete : + - + - + - ... diff --git a/config/magpie.ini b/config/magpie.ini index aed1ae3a2..a0832783b 100644 --- a/config/magpie.ini +++ b/config/magpie.ini @@ -32,7 +32,7 @@ magpie.max_restart = 5 # This secret should be the same in Twitcher ! magpie.secret = magpie.push_phoenix = true -magpie.config_path = +# magpie.config_path = # caching settings refer to the Performance section in the documentation cache.regions = adapter, acl diff --git a/env/magpie.env.example b/env/magpie.env.example index 4cd9b10d6..9c74eb727 100644 --- a/env/magpie.env.example +++ b/env/magpie.env.example @@ -15,7 +15,6 @@ MAGPIE_LOG_REQUEST=true MAGPIE_LOG_EXCEPTION=true MAGPIE_UI_ENABLED=true MAGPIE_CONFIG_DIR=../config -MAGPIE_WEBHOOK_PRE_USER_CREATION_URL= PHOENIX_USER=phoenix PHOENIX_PASSWORD=qwerty PHOENIX_PORT=8443 diff --git a/magpie/api/management/user/user_formats.py b/magpie/api/management/user/user_formats.py index f95e91110..33e9510ac 100644 --- a/magpie/api/management/user/user_formats.py +++ b/magpie/api/management/user/user_formats.py @@ -10,6 +10,7 @@ def fmt_usr(usr, grp_names): "user_name": str(usr.user_name), "email": str(usr.email), "group_names": sorted(list(grp_names) if grp_names else [grp.group_name for grp in user.groups]), + "status": int(user.status) } if user.user_name != get_constant("MAGPIE_ANONYMOUS_USER"): user_info["user_id"] = int(user.id) diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index 0897df0f0..c85c6db52 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -1,3 +1,4 @@ +import multiprocessing from typing import TYPE_CHECKING import requests @@ -26,7 +27,11 @@ from magpie.api.management.user import user_formats as uf from magpie.constants import get_constant from magpie.permissions import PermissionSet, PermissionType, format_permissions +from magpie.register import get_all_configs from magpie.services import service_factory +from magpie.utils import get_logger + +LOGGER = get_logger(__name__) if TYPE_CHECKING: # pylint: disable=W0611,unused-import @@ -109,31 +114,38 @@ def _add_to_group(usr, grp): _add_to_group(new_user, _get_group(anonym_grp_name)) new_user_groups.append(anonym_grp_name) - # user_name_list = ax.evaluate_call(lambda: [user.user_name for user in - # UserService.all(models.User, db_session=db_session)], - # fallback=lambda: db_session.rollback(), http_error=HTTPForbidden, - # msg_on_fail=s.Users_GET_ForbiddenResponseSchema.description) - # print(user_name_list) - - valid_http = ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, - content={"user": uf.format_user(new_user, new_user_groups)}) - - webhook_url = get_constant("MAGPIE_WEBHOOK_PRE_USER_CREATION_URL") - - # FIXME: add try, in case url is not available - if webhook_url is not None and webhook_url != "": - try: - # TODO: set status to 0 - resp = requests.post(webhook_url + "/create", data={'user_name': user_name}) - print(resp.status_code) - print(resp.text) - except Exception as e: - # TODO: clean up, remove pending user - raise_http(http_error=HTTPFailedDependency, - detail="Error occurred during parameter verification") - # TODO: reset status to finished - - return valid_http + # Check for webhook requests + config_path = get_constant("MAGPIE_CONFIG_PATH", default_value=None, + raise_missing=False, raise_not_set=False, print_missing=True) + if config_path: + webhook_configs = get_all_configs(config_path, 'webhooks', allow_missing=True) + for cfg in webhook_configs: + if 'create' in cfg.keys() and len(cfg['create']) > 0: + # Execute all webhook requests + pool = multiprocessing.Pool(processes=len(cfg['create'])) + args = [(url, user_name) for url in cfg['create']] + result = pool.starmap_async(webhook_request, args, error_callback=webhook_error_callback) + + return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, + content={"user": uf.format_user(new_user, new_user_groups)}) + + +def webhook_request(webhook_url, user_name): + """ + Sends a webhook request using the url parameter + """ + # TODO: create a real temp_url that will be called if the webhook service has an error + # this would also set the user's status to 0 + resp = requests.post(webhook_url, data={'user_name': user_name, 'temp_url': 'temp_url:80/todo'}) + + +def webhook_error_callback(r): + """ + Error callback function called if an error occurs in the webhook_call function + """ + # TODO : (related to TODO in webhook_call function) handle errors occuring in the thread for the webhook_call + # change user's status to 0? + LOGGER.error(str(r)) def create_user_resource_permission_response(user, resource, permission, db_session, overwrite=False): diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index 6bbbc6d45..a1142ac4e 100644 --- a/magpie/api/management/user/user_views.py +++ b/magpie/api/management/user/user_views.py @@ -1,6 +1,8 @@ """ User Views, both for specific user-name provided as request path variable and special keyword for logged session user. """ +import multiprocessing + from pyramid.httpexceptions import HTTPBadRequest, HTTPConflict, HTTPCreated, HTTPForbidden, HTTPNotFound, HTTPOk from pyramid.settings import asbool from pyramid.view import view_config @@ -17,6 +19,7 @@ from magpie.api.management.user import user_utils as uu from magpie.constants import MAGPIE_CONTEXT_PERMISSION, MAGPIE_LOGGED_PERMISSION, get_constant from magpie.permissions import PermissionType, format_permissions +from magpie.register import get_all_configs from magpie.services import SERVICE_TYPE_DICT from magpie.utils import get_logger @@ -135,6 +138,19 @@ def delete_user_view(request): http_error=HTTPForbidden, msg_on_fail=s.User_DELETE_ForbiddenResponseSchema.description) ax.evaluate_call(lambda: request.db.delete(user), fallback=lambda: request.db.rollback(), http_error=HTTPForbidden, msg_on_fail=s.User_DELETE_ForbiddenResponseSchema.description) + + # Check for webhook requests + config_path = get_constant("MAGPIE_CONFIG_PATH", default_value=None, + raise_missing=False, raise_not_set=False, print_missing=True) + if config_path: + webhook_configs = get_all_configs(config_path, 'webhooks', allow_missing=True) + for cfg in webhook_configs: + if 'delete' in cfg.keys() and len(cfg['delete']) > 0: + # Execute all webhook requests + pool = multiprocessing.Pool(processes=len(cfg['delete'])) + args = [(url, user.user_name) for url in cfg['delete']] + result = pool.starmap_async(uu.webhook_request, args, error_callback=uu.webhook_error_callback) + return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description) diff --git a/tests/test_magpie_api.py b/tests/test_magpie_api.py index 0c00f4d01..3641b19be 100644 --- a/tests/test_magpie_api.py +++ b/tests/test_magpie_api.py @@ -8,9 +8,13 @@ Tests for :mod:`magpie.api` module. """ import os +import tempfile +from time import sleep import unittest +import yaml import mock +import requests # NOTE: must be imported without 'from', otherwise the interface's test cases are also executed import tests.interfaces as ti @@ -107,34 +111,154 @@ def setUpClass(cls): cls.check_requirements() cls.setup_test_values() - def test_CreateUser_Webhook(self): + def test_Webhook_CreateUser(self): """ - Test creating a user using a webhook. + Test creating a user using multiple webhooks. """ - with mock.patch.dict(os.environ, {'MAGPIE_WEBHOOK_PRE_USER_CREATION_URL': - get_constant('MAGPIE_TEST_WEBHOOK_PRE_USER_CREATION_URL')}): - utils.get_test_webhook_app() - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - assert 0 == 1 + # Create temporary config for testing webhooks + with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: + base_webhook_url = get_constant('MAGPIE_TEST_USER_WEBHOOK_URL') + create_webhook_url = base_webhook_url + '/webhook' + data = { + 'webhooks': + # Use two identical urls to simulate having multiple webhook urls + {'create': [create_webhook_url, create_webhook_url]}} + yaml.dump(data, tmp_config, default_flow_style=False) - def test_CreateUser_FailingWebhook(self): + with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): + app = utils.get_test_webhook_app() + + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Wait for the webhook requests to complete + sleep(1) + + # Check if both webhook requests have completed successfully + resp = requests.get(base_webhook_url + '/get_status') + assert resp.text == '2' + + # Check if user creation was successful + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + + def test_Webhook_CreateUser_FailingUrl(self): """ Test creating a user using a failing webhook url. """ - with mock.patch.dict(os.environ, {'MAGPIE_WEBHOOK_PRE_USER_CREATION_URL': - get_constant('MAGPIE_TEST_WEBHOOK_PRE_USER_CREATION_URL')}): - utils.get_test_webhook_app() - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - assert 0 == 1 + # Create temporary config for testing webhooks + with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: + create_webhook_url = "failing_url" + data = {'webhooks': {'create': [create_webhook_url]}} + yaml.dump(data, tmp_config, default_flow_style=False) - def test_CreateUser_EmptyWebhook(self): + with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): + resp = utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Wait for the webhook requests to complete + sleep(1) + + # Check if user creation was successful even if the webhook failed + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + + def test_Webhook_CreateUser_EmptyUrl(self): """ Test creating a user with an empty webhook url. """ - with mock.patch.dict(os.environ, {'MAGPIE_WEBHOOK_PRE_USER_CREATION_URL': ""}): - utils.get_test_webhook_app() - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - assert 0 == 1 + # Create temporary config for testing webhooks + with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: + data = {'webhooks': {'create': []}} + yaml.dump(data, tmp_config, default_flow_style=False) + + with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): + resp = utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Wait for the webhook requests to complete + sleep(1) + + # Check if user creation was successful even if no webhook were defined in the config + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + + def test_Webhook_DeleteUser(self): + """ + Test deleting a user using multiple webhooks. + """ + # Create temporary config for testing webhooks + with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: + base_webhook_url = get_constant('MAGPIE_TEST_USER_WEBHOOK_URL') + delete_webhook_url = base_webhook_url + '/webhook' + data = { + 'webhooks': + # Use two identical urls to simulate having multiple webhook urls + {'delete': [delete_webhook_url, delete_webhook_url]}} + yaml.dump(data, tmp_config, default_flow_style=False) + + with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): + app = utils.get_test_webhook_app() + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Webhooks shouldn't have been called during the user creation + sleep(1) + resp = requests.get(base_webhook_url + '/get_status') + assert resp.text == '0' + + # delete the test user, webhooks should be called during this delete request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) + + # Wait for the webhook requests to complete and check their success + sleep(1) + resp = requests.get(base_webhook_url + '/get_status') + assert resp.text == '2' + + def test_Webhook_DeleteUser_FailingUrl(self): + """ + Test deleting a user using a failing webhook url. + """ + # Create temporary config for testing webhooks + with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: + delete_webhook_url = "failing_url" + data = {'webhooks': {'delete': [delete_webhook_url]}} + yaml.dump(data, tmp_config, default_flow_style=False) + + with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # delete the test user, webhooks should be called during the request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + + # Check if user deletion was successful even if the webhook failed + sleep(1) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) + + def test_Webhook_DeleteUser_EmptyUrl(self): + """ + Test deleting a user with an empty webhook url. + """ + # Create temporary config for testing webhooks + with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: + data = {'webhooks': {'delete': []}} + yaml.dump(data, tmp_config, default_flow_style=False) + + with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # delete the test user, webhooks should be called during the request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + + # Check if user deletion was successful even if no webhooks were defined in the config + sleep(1) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) @runner.MAGPIE_TEST_API diff --git a/tests/utils.py b/tests/utils.py index e3bdb9dc1..3c706a8d4 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,11 +1,8 @@ -import os from errno import EADDRINUSE import threading -from cgi import FieldStorage import functools import itertools import json as json_pkg # avoid conflict name with json argument employed for some function -from time import sleep import unittest import warnings from distutils.version import LooseVersion @@ -253,35 +250,52 @@ def get_test_webhook_app(): """ Instantiate a local test application used for the prehook for user creation and deletion. """ - def webhook_create(request): + def webhook_request(request): + # Simulates a webhook url call user = request.POST['user_name'] - if user == 'failure_user': - # Simulates an error in the webhook process - return HTTPInternalServerError() - sleep(1) - return Response(user + " webhook") + temp_url = request.POST['temp_url'] + + # Status is incremented to count the number of successful test webhooks + settings["webhook_status"] += 1 + return Response("Successful webhook url for user " + user) + + def get_status(request): + # Returns the status number + return Response(str(settings["webhook_status"])) + + def reset_status(request): + settings["webhook_status"] = 0 + return Response("Webhook status has been reset to 0.") + + with Configurator() as config: + settings = config.registry.settings + # Initialize status + settings["webhook_status"] = 0 + config.add_route('webhook', '/webhook') + config.add_route('get_status', '/get_status') + config.add_route('reset_status', '/reset_status') + config.add_view(webhook_request, route_name='webhook', request_method="POST", request_param="user_name") + config.add_view(get_status, route_name='get_status', request_method="GET") + config.add_view(reset_status, route_name='reset_status', request_method="POST") + app = config.make_wsgi_app() def webhook_app(): - with Configurator() as config: - config.add_route('create', '/create') - config.add_view(webhook_create, route_name='create', request_method="POST", request_param="user_name") - app = config.make_wsgi_app() try: - webhook_url_info = urlparse(get_constant("MAGPIE_WEBHOOK_PRE_USER_CREATION_URL")) + webhook_url_info = urlparse(get_constant("MAGPIE_TEST_USER_WEBHOOK_URL")) serve(app, host=webhook_url_info.hostname, port=webhook_url_info.port) except OSError as e: if e.errno == EADDRINUSE: - # The app is already running, nothing to do. + # The app is already running, we just need to reset the webhook status for a new test. + resp = requests.post(get_constant('MAGPIE_TEST_USER_WEBHOOK_URL') + '/reset_status') + check_response_basic_info(resp, 200, expected_method="POST") return else: raise + x = threading.Thread(target=webhook_app, daemon=True) x.start() - # FIXME: sometimes an error for first request, with BadGateway - # (probably the app didn't had the time to startup completely) - - return + return app def get_hostname(test_item): From 66fc1c136509813ca8cecd30dca6a85b159c5f47 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Thu, 17 Dec 2020 09:01:31 -0500 Subject: [PATCH 03/33] updated docs with webhooks info --- docs/configuration.rst | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/configuration.rst b/docs/configuration.rst index ad159c1ad..cf4b086df 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -145,6 +145,17 @@ a combined configuration as follows. group: my-group # will reference above group action: create + webhooks: + create : + - + - + - ... + delete : + - + - + - ... + + For backward compatibility reasons, `Magpie` will first look for separate files to load each section individually. To enforce using a combined file as above, either provide ``MAGPIE_CONFIG_PATH = /config.yml`` or ensure that each @@ -160,6 +171,10 @@ creation. Otherwise defaults are assumed and only the specified user or group na `providers.cfg`_ and `permissions.cfg`_ for further details about specific formatting and behaviour of each available field. +A section for ``webhooks`` has also been added to the combined configuration file. This section defines a list of +urls that should be called when either creating or deleting a user. The webhooks urls are responsible for any extra +steps that should be taken on external services during the user creation/deletion. + Settings and Constants ---------------------- From af5b2579cb576e8ec9c2ab8b71c844a7eb2520ed Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Thu, 17 Dec 2020 09:47:07 -0500 Subject: [PATCH 04/33] fix linter --- CHANGES.rst | 8 ++- magpie/api/management/user/user_utils.py | 18 +++---- magpie/api/management/user/user_views.py | 10 ++-- tests/test_magpie_api.py | 68 ++++++++++++------------ tests/utils.py | 35 ++++++------ 5 files changed, 71 insertions(+), 68 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index f4c002101..d1cd9ffc5 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -7,7 +7,13 @@ Changes `Unreleased `_ (latest) ------------------------------------------------------------------------------------ -* Nothing yet. +Features / Changes +~~~~~~~~~~~~~~~~~~~~~ +* Add a list of webhook urls, defined in the configuration, that will be called when creating or deleting a user. + +Bug Fixes +~~~~~~~~~~~~~~~~~~~~~ +* N/A `3.4.0 `_ (2020-12-09) ------------------------------------------------------------------------------------ diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index c85c6db52..f3f6c254a 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -10,7 +10,6 @@ HTTPForbidden, HTTPInternalServerError, HTTPNotFound, - HTTPFailedDependency, HTTPOk ) from ziggurat_foundations.models.services.group import GroupService @@ -21,7 +20,6 @@ from magpie import models from magpie.api import exception as ax from magpie.api import schemas as s -from magpie.api.exception import raise_http from magpie.api.management.resource import resource_utils as ru from magpie.api.management.service.service_formats import format_service from magpie.api.management.user import user_formats as uf @@ -118,13 +116,13 @@ def _add_to_group(usr, grp): config_path = get_constant("MAGPIE_CONFIG_PATH", default_value=None, raise_missing=False, raise_not_set=False, print_missing=True) if config_path: - webhook_configs = get_all_configs(config_path, 'webhooks', allow_missing=True) + webhook_configs = get_all_configs(config_path, "webhooks", allow_missing=True) for cfg in webhook_configs: - if 'create' in cfg.keys() and len(cfg['create']) > 0: + if "create" in cfg.keys() and len(cfg["create"]) > 0: # Execute all webhook requests - pool = multiprocessing.Pool(processes=len(cfg['create'])) - args = [(url, user_name) for url in cfg['create']] - result = pool.starmap_async(webhook_request, args, error_callback=webhook_error_callback) + pool = multiprocessing.Pool(processes=len(cfg["create"])) + args = [(url, user_name) for url in cfg["create"]] + pool.starmap_async(webhook_request, args, error_callback=webhook_error_callback) return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, content={"user": uf.format_user(new_user, new_user_groups)}) @@ -136,16 +134,16 @@ def webhook_request(webhook_url, user_name): """ # TODO: create a real temp_url that will be called if the webhook service has an error # this would also set the user's status to 0 - resp = requests.post(webhook_url, data={'user_name': user_name, 'temp_url': 'temp_url:80/todo'}) + requests.post(webhook_url, data={"user_name": user_name, "temp_url": "temp_url:80/todo"}) -def webhook_error_callback(r): +def webhook_error_callback(exception): """ Error callback function called if an error occurs in the webhook_call function """ # TODO : (related to TODO in webhook_call function) handle errors occuring in the thread for the webhook_call # change user's status to 0? - LOGGER.error(str(r)) + LOGGER.error(str(exception)) def create_user_resource_permission_response(user, resource, permission, db_session, overwrite=False): diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index a1142ac4e..0a1b99918 100644 --- a/magpie/api/management/user/user_views.py +++ b/magpie/api/management/user/user_views.py @@ -143,13 +143,13 @@ def delete_user_view(request): config_path = get_constant("MAGPIE_CONFIG_PATH", default_value=None, raise_missing=False, raise_not_set=False, print_missing=True) if config_path: - webhook_configs = get_all_configs(config_path, 'webhooks', allow_missing=True) + webhook_configs = get_all_configs(config_path, "webhooks", allow_missing=True) for cfg in webhook_configs: - if 'delete' in cfg.keys() and len(cfg['delete']) > 0: + if "delete" in cfg.keys() and len(cfg["delete"]) > 0: # Execute all webhook requests - pool = multiprocessing.Pool(processes=len(cfg['delete'])) - args = [(url, user.user_name) for url in cfg['delete']] - result = pool.starmap_async(uu.webhook_request, args, error_callback=uu.webhook_error_callback) + pool = multiprocessing.Pool(processes=len(cfg["delete"])) + args = [(url, user.user_name) for url in cfg["delete"]] + pool.starmap_async(uu.webhook_request, args, error_callback=uu.webhook_error_callback) return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description) diff --git a/tests/test_magpie_api.py b/tests/test_magpie_api.py index ee0aa3f2f..5801e6a9d 100644 --- a/tests/test_magpie_api.py +++ b/tests/test_magpie_api.py @@ -116,17 +116,17 @@ def test_Webhook_CreateUser(self): Test creating a user using multiple webhooks. """ # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: - base_webhook_url = get_constant('MAGPIE_TEST_USER_WEBHOOK_URL') - create_webhook_url = base_webhook_url + '/webhook' + with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: + base_webhook_url = get_constant("MAGPIE_TEST_USER_WEBHOOK_URL") + create_webhook_url = base_webhook_url + "/webhook" data = { - 'webhooks': + "webhooks": # Use two identical urls to simulate having multiple webhook urls - {'create': [create_webhook_url, create_webhook_url]}} + {"create": [create_webhook_url, create_webhook_url]}} yaml.dump(data, tmp_config, default_flow_style=False) - with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): - app = utils.get_test_webhook_app() + with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): + utils.get_test_webhook_app() utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) @@ -134,8 +134,8 @@ def test_Webhook_CreateUser(self): sleep(1) # Check if both webhook requests have completed successfully - resp = requests.get(base_webhook_url + '/get_status') - assert resp.text == '2' + resp = requests.get(base_webhook_url + "/get_status") + assert resp.text == "2" # Check if user creation was successful users = utils.TestSetup.get_RegisteredUsersList(self) @@ -146,13 +146,13 @@ def test_Webhook_CreateUser_FailingUrl(self): Test creating a user using a failing webhook url. """ # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: + with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: create_webhook_url = "failing_url" - data = {'webhooks': {'create': [create_webhook_url]}} + data = {"webhooks": {"create": [create_webhook_url]}} yaml.dump(data, tmp_config, default_flow_style=False) - with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): - resp = utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) # Wait for the webhook requests to complete sleep(1) @@ -166,12 +166,12 @@ def test_Webhook_CreateUser_EmptyUrl(self): Test creating a user with an empty webhook url. """ # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: - data = {'webhooks': {'create': []}} + with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: + data = {"webhooks": {"create": []}} yaml.dump(data, tmp_config, default_flow_style=False) - with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): - resp = utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) # Wait for the webhook requests to complete sleep(1) @@ -185,24 +185,24 @@ def test_Webhook_DeleteUser(self): Test deleting a user using multiple webhooks. """ # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: - base_webhook_url = get_constant('MAGPIE_TEST_USER_WEBHOOK_URL') - delete_webhook_url = base_webhook_url + '/webhook' + with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: + base_webhook_url = get_constant("MAGPIE_TEST_USER_WEBHOOK_URL") + delete_webhook_url = base_webhook_url + "/webhook" data = { - 'webhooks': + "webhooks": # Use two identical urls to simulate having multiple webhook urls - {'delete': [delete_webhook_url, delete_webhook_url]}} + {"delete": [delete_webhook_url, delete_webhook_url]}} yaml.dump(data, tmp_config, default_flow_style=False) - with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): - app = utils.get_test_webhook_app() + with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): + utils.get_test_webhook_app() # create the test user first utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) # Webhooks shouldn't have been called during the user creation sleep(1) - resp = requests.get(base_webhook_url + '/get_status') - assert resp.text == '0' + resp = requests.get(base_webhook_url + "/get_status") + assert resp.text == "0" # delete the test user, webhooks should be called during this delete request path = "/users/{usr}".format(usr=self.test_user_name) @@ -212,20 +212,20 @@ def test_Webhook_DeleteUser(self): # Wait for the webhook requests to complete and check their success sleep(1) - resp = requests.get(base_webhook_url + '/get_status') - assert resp.text == '2' + resp = requests.get(base_webhook_url + "/get_status") + assert resp.text == "2" def test_Webhook_DeleteUser_FailingUrl(self): """ Test deleting a user using a failing webhook url. """ # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: + with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: delete_webhook_url = "failing_url" - data = {'webhooks': {'delete': [delete_webhook_url]}} + data = {"webhooks": {"delete": [delete_webhook_url]}} yaml.dump(data, tmp_config, default_flow_style=False) - with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): + with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): # create the test user first utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) @@ -243,11 +243,11 @@ def test_Webhook_DeleteUser_EmptyUrl(self): Test deleting a user with an empty webhook url. """ # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode='wt') as tmp_config: - data = {'webhooks': {'delete': []}} + with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: + data = {"webhooks": {"delete": []}} yaml.dump(data, tmp_config, default_flow_style=False) - with mock.patch.dict(os.environ, {'MAGPIE_CONFIG_PATH': tmp_config.name}): + with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): # create the test user first utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) diff --git a/tests/utils.py b/tests/utils.py index 6045339eb..7716d26f9 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -15,7 +15,7 @@ import six from pyramid.config import Configurator from pyramid.response import Response -from pyramid.httpexceptions import HTTPException, HTTPInternalServerError +from pyramid.httpexceptions import HTTPException from pyramid.settings import asbool from pyramid.testing import DummyRequest, setUp as PyramidSetUp from six.moves.urllib.parse import urlparse @@ -281,12 +281,12 @@ def get_test_webhook_app(): """ def webhook_request(request): # Simulates a webhook url call - user = request.POST['user_name'] - temp_url = request.POST['temp_url'] + user = request.POST["user_name"] + temp_url = request.POST["temp_url"] # Status is incremented to count the number of successful test webhooks settings["webhook_status"] += 1 - return Response("Successful webhook url for user " + user) + return Response("Successful webhook url with user " + user + " and temp_url " + temp_url) def get_status(request): # Returns the status number @@ -300,31 +300,30 @@ def reset_status(request): settings = config.registry.settings # Initialize status settings["webhook_status"] = 0 - config.add_route('webhook', '/webhook') - config.add_route('get_status', '/get_status') - config.add_route('reset_status', '/reset_status') - config.add_view(webhook_request, route_name='webhook', request_method="POST", request_param="user_name") - config.add_view(get_status, route_name='get_status', request_method="GET") - config.add_view(reset_status, route_name='reset_status', request_method="POST") - app = config.make_wsgi_app() + config.add_route("webhook", "/webhook") + config.add_route("get_status", "/get_status") + config.add_route("reset_status", "/reset_status") + config.add_view(webhook_request, route_name="webhook", request_method="POST", request_param="user_name") + config.add_view(get_status, route_name="get_status", request_method="GET") + config.add_view(reset_status, route_name="reset_status", request_method="POST") + webhook_app_instance = config.make_wsgi_app() def webhook_app(): try: webhook_url_info = urlparse(get_constant("MAGPIE_TEST_USER_WEBHOOK_URL")) - serve(app, host=webhook_url_info.hostname, port=webhook_url_info.port) - except OSError as e: - if e.errno == EADDRINUSE: + serve(webhook_app_instance, host=webhook_url_info.hostname, port=webhook_url_info.port) + except OSError as exception: + if exception.errno == EADDRINUSE: # The app is already running, we just need to reset the webhook status for a new test. - resp = requests.post(get_constant('MAGPIE_TEST_USER_WEBHOOK_URL') + '/reset_status') + resp = requests.post(get_constant("MAGPIE_TEST_USER_WEBHOOK_URL") + "/reset_status") check_response_basic_info(resp, 200, expected_method="POST") return - else: - raise + raise x = threading.Thread(target=webhook_app, daemon=True) x.start() - return app + return webhook_app_instance def get_hostname(test_item): From d3937036c09b5609cfa39c87ccc1a2b1e94538aa Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Thu, 17 Dec 2020 10:31:26 -0500 Subject: [PATCH 05/33] fix doc --- docs/configuration.rst | 3 +-- magpie/api/management/user/user_utils.py | 6 ++++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index cf4b086df..46adb8d4a 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -156,7 +156,6 @@ a combined configuration as follows. - ... - For backward compatibility reasons, `Magpie` will first look for separate files to load each section individually. To enforce using a combined file as above, either provide ``MAGPIE_CONFIG_PATH = /config.yml`` or ensure that each specific environment variable ``MAGPIE_PROVIDERS_CONFIG_PATH`` and ``MAGPIE_PERMISSIONS_CONFIG_PATH`` point to the same @@ -171,7 +170,7 @@ creation. Otherwise defaults are assumed and only the specified user or group na `providers.cfg`_ and `permissions.cfg`_ for further details about specific formatting and behaviour of each available field. -A section for ``webhooks`` has also been added to the combined configuration file. This section defines a list of +A section for webhooks has also been added to the combined configuration file. This section defines a list of urls that should be called when either creating or deleting a user. The webhooks urls are responsible for any extra steps that should be taken on external services during the user creation/deletion. diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index f3f6c254a..0a50fe47c 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -129,8 +129,9 @@ def _add_to_group(usr, grp): def webhook_request(webhook_url, user_name): + # type: (Str, Str) -> None """ - Sends a webhook request using the url parameter + Sends a webhook request using the input url. """ # TODO: create a real temp_url that will be called if the webhook service has an error # this would also set the user's status to 0 @@ -138,8 +139,9 @@ def webhook_request(webhook_url, user_name): def webhook_error_callback(exception): + # type: (requests.exceptions) -> None """ - Error callback function called if an error occurs in the webhook_call function + Error callback function called if an error occurs in the webhook_call function. """ # TODO : (related to TODO in webhook_call function) handle errors occuring in the thread for the webhook_call # change user's status to 0? From bc7b78a20e715c44848d00a49a8a38bf14a54117 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Mon, 4 Jan 2021 10:59:34 -0500 Subject: [PATCH 06/33] move webhook operations --- magpie/api/management/user/user_utils.py | 24 ++---------------------- magpie/api/management/user/user_views.py | 2 +- magpie/api/requests.py | 21 +++++++++++++++++++++ tests/utils.py | 2 +- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index 0a50fe47c..1552a6acd 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -1,7 +1,6 @@ import multiprocessing from typing import TYPE_CHECKING -import requests import six from pyramid.httpexceptions import ( HTTPBadRequest, @@ -19,6 +18,7 @@ from magpie import models from magpie.api import exception as ax +from magpie.api import requests as ar from magpie.api import schemas as s from magpie.api.management.resource import resource_utils as ru from magpie.api.management.service.service_formats import format_service @@ -122,32 +122,12 @@ def _add_to_group(usr, grp): # Execute all webhook requests pool = multiprocessing.Pool(processes=len(cfg["create"])) args = [(url, user_name) for url in cfg["create"]] - pool.starmap_async(webhook_request, args, error_callback=webhook_error_callback) + pool.starmap_async(ar.webhook_request, args, error_callback=ar.webhook_error_callback) return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, content={"user": uf.format_user(new_user, new_user_groups)}) -def webhook_request(webhook_url, user_name): - # type: (Str, Str) -> None - """ - Sends a webhook request using the input url. - """ - # TODO: create a real temp_url that will be called if the webhook service has an error - # this would also set the user's status to 0 - requests.post(webhook_url, data={"user_name": user_name, "temp_url": "temp_url:80/todo"}) - - -def webhook_error_callback(exception): - # type: (requests.exceptions) -> None - """ - Error callback function called if an error occurs in the webhook_call function. - """ - # TODO : (related to TODO in webhook_call function) handle errors occuring in the thread for the webhook_call - # change user's status to 0? - LOGGER.error(str(exception)) - - def create_user_resource_permission_response(user, resource, permission, db_session, overwrite=False): # type: (models.User, ServiceOrResourceType, PermissionSet, Session, bool) -> HTTPException """ diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index 0a1b99918..a443d6c4e 100644 --- a/magpie/api/management/user/user_views.py +++ b/magpie/api/management/user/user_views.py @@ -149,7 +149,7 @@ def delete_user_view(request): # Execute all webhook requests pool = multiprocessing.Pool(processes=len(cfg["delete"])) args = [(url, user.user_name) for url in cfg["delete"]] - pool.starmap_async(uu.webhook_request, args, error_callback=uu.webhook_error_callback) + pool.starmap_async(ar.webhook_request, args, error_callback=ar.webhook_error_callback) return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description) diff --git a/magpie/api/requests.py b/magpie/api/requests.py index e21a6e727..ef13e5360 100644 --- a/magpie/api/requests.py +++ b/magpie/api/requests.py @@ -1,5 +1,6 @@ from typing import TYPE_CHECKING +import requests import six from pyramid.authentication import Authenticated, IAuthenticationPolicy from pyramid.httpexceptions import ( @@ -344,3 +345,23 @@ def get_query_param(request, case_insensitive_key, default=None): if param.lower() == case_insensitive_key: return request.params.get(param) return default + + +def webhook_request(webhook_url, user_name): + # type: (Str, Str) -> None + """ + Sends a webhook request using the input url. + """ + # TODO: create a real temp_url that will be called if the webhook service has an error + # this would also set the user's status to 0 + requests.post(webhook_url, data={"user_name": user_name, "temp_url": "temp_url:80/todo"}) + + +def webhook_error_callback(exception): + # type: (requests.exceptions) -> None + """ + Error callback function called if an error occurs in the webhook_call function. + """ + # TODO : (related to TODO in webhook_call function) handle errors occuring in the thread for the webhook_call + # change user's status to 0? + LOGGER.error(str(exception)) diff --git a/tests/utils.py b/tests/utils.py index 7716d26f9..fe1c8749b 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -316,7 +316,7 @@ def webhook_app(): if exception.errno == EADDRINUSE: # The app is already running, we just need to reset the webhook status for a new test. resp = requests.post(get_constant("MAGPIE_TEST_USER_WEBHOOK_URL") + "/reset_status") - check_response_basic_info(resp, 200, expected_method="POST") + check_response_basic_info(resp, 200, expected_type=CONTENT_TYPE_HTML, expected_method="POST") return raise From 39dcb2ece81d7e4774a088b33dc587471075226c Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Mon, 4 Jan 2021 16:53:55 -0500 Subject: [PATCH 07/33] remove webhook env variable, and fix broken tests if env variable overwritten --- env/magpie.env.example | 2 - tests/test_magpie_api.py | 219 ++++++++++++++++++++------------------- tests/utils.py | 6 +- 3 files changed, 116 insertions(+), 111 deletions(-) diff --git a/env/magpie.env.example b/env/magpie.env.example index 23bec2a85..a3ecbd575 100644 --- a/env/magpie.env.example +++ b/env/magpie.env.example @@ -32,8 +32,6 @@ MAGPIE_TEST_VERSION=latest # this means you must have a separately running Magpie instance reachable at that endpoint to run 'remote' tests # to ignore this, consider setting an empty value or running 'make test-local' instead MAGPIE_TEST_REMOTE_SERVER_URL=http://localhost:2001 -# This webhook URL is used to create a test application and to test the webhook requests -MAGPIE_TEST_USER_WEBHOOK_URL=http://localhost:8080 # below are the credentials employed to run tests (especially if running on non-default remote server) MAGPIE_TEST_ADMIN_USERNAME=admin MAGPIE_TEST_ADMIN_PASSWORD=qwerty diff --git a/tests/test_magpie_api.py b/tests/test_magpie_api.py index 5801e6a9d..d020427fb 100644 --- a/tests/test_magpie_api.py +++ b/tests/test_magpie_api.py @@ -99,7 +99,16 @@ class TestCase_MagpieAPI_AdminAuth_Local(ti.Interface_MagpieAPI_AdminAuth, unitt @classmethod def setUpClass(cls): - cls.app = utils.get_test_magpie_app() + cls.base_webhook_url = "http://localhost:8080" + cls.webhook_tmp_config = tempfile.NamedTemporaryFile(mode="w") + # write default config with empty values, webhooks will be overwritten for each specific test setup + data = { + "webhooks": {}, + "providers": "", + "permissions": "" + } + yaml.dump(data, cls.webhook_tmp_config, default_flow_style=False) + cls.app = utils.get_test_magpie_app({"magpie.config_path": cls.webhook_tmp_config.name}) cls.grp = get_constant("MAGPIE_ADMIN_GROUP") cls.usr = get_constant("MAGPIE_TEST_ADMIN_USERNAME") cls.pwd = get_constant("MAGPIE_TEST_ADMIN_PASSWORD") @@ -111,154 +120,152 @@ def setUpClass(cls): cls.check_requirements() cls.setup_test_values() + @classmethod + def tearDownClass(cls): + cls.webhook_tmp_config.close() + def test_Webhook_CreateUser(self): """ Test creating a user using multiple webhooks. """ - # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: - base_webhook_url = get_constant("MAGPIE_TEST_USER_WEBHOOK_URL") - create_webhook_url = base_webhook_url + "/webhook" - data = { - "webhooks": - # Use two identical urls to simulate having multiple webhook urls - {"create": [create_webhook_url, create_webhook_url]}} - yaml.dump(data, tmp_config, default_flow_style=False) + # Write temporary config for testing webhooks + create_webhook_url = self.base_webhook_url + "/webhook" + data = { + "webhooks": + # Use two identical urls to simulate having multiple webhook urls + {"create": [create_webhook_url, create_webhook_url]} + } + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.safe_dump(data, stream, default_flow_style=False) - with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): - utils.get_test_webhook_app() + utils.get_test_webhook_app(self.base_webhook_url) - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - # Wait for the webhook requests to complete - sleep(1) + # Wait for the webhook requests to complete + sleep(1) - # Check if both webhook requests have completed successfully - resp = requests.get(base_webhook_url + "/get_status") - assert resp.text == "2" + # Check if both webhook requests have completed successfully + resp = requests.get(self.base_webhook_url + "/get_status") + assert resp.text == "2" - # Check if user creation was successful - users = utils.TestSetup.get_RegisteredUsersList(self) - utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + # Check if user creation was successful + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") def test_Webhook_CreateUser_FailingUrl(self): """ Test creating a user using a failing webhook url. """ - # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: - create_webhook_url = "failing_url" - data = {"webhooks": {"create": [create_webhook_url]}} - yaml.dump(data, tmp_config, default_flow_style=False) + # Write temporary config for testing webhooks + create_webhook_url = "failing_url" + data = {"webhooks": {"create": [create_webhook_url]}} + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) - with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - # Wait for the webhook requests to complete - sleep(1) + # Wait for the webhook requests to complete + sleep(1) - # Check if user creation was successful even if the webhook failed - users = utils.TestSetup.get_RegisteredUsersList(self) - utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + # Check if user creation was successful even if the webhook failed + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") def test_Webhook_CreateUser_EmptyUrl(self): """ Test creating a user with an empty webhook url. """ - # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: - data = {"webhooks": {"create": []}} - yaml.dump(data, tmp_config, default_flow_style=False) + # Write temporary config for testing webhooks + data = {"webhooks": {"create": []}} + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) - with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - # Wait for the webhook requests to complete - sleep(1) + # Wait for the webhook requests to complete + sleep(1) - # Check if user creation was successful even if no webhook were defined in the config - users = utils.TestSetup.get_RegisteredUsersList(self) - utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + # Check if user creation was successful even if no webhook were defined in the config + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") def test_Webhook_DeleteUser(self): """ Test deleting a user using multiple webhooks. """ - # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: - base_webhook_url = get_constant("MAGPIE_TEST_USER_WEBHOOK_URL") - delete_webhook_url = base_webhook_url + "/webhook" - data = { - "webhooks": - # Use two identical urls to simulate having multiple webhook urls - {"delete": [delete_webhook_url, delete_webhook_url]}} - yaml.dump(data, tmp_config, default_flow_style=False) - - with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): - utils.get_test_webhook_app() - # create the test user first - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - - # Webhooks shouldn't have been called during the user creation - sleep(1) - resp = requests.get(base_webhook_url + "/get_status") - assert resp.text == "0" - - # delete the test user, webhooks should be called during this delete request - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) - utils.check_response_basic_info(resp, 200, expected_method="GET") - utils.TestSetup.check_NonExistingTestUser(self) - - # Wait for the webhook requests to complete and check their success - sleep(1) - resp = requests.get(base_webhook_url + "/get_status") - assert resp.text == "2" + # Write temporary config for testing webhooks + delete_webhook_url = self.base_webhook_url + "/webhook" + data = { + "webhooks": + # Use two identical urls to simulate having multiple webhook urls + {"delete": [delete_webhook_url, delete_webhook_url]}} + + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) + + utils.get_test_webhook_app(self.base_webhook_url) + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Webhooks shouldn't have been called during the user creation + sleep(1) + resp = requests.get(self.base_webhook_url + "/get_status") + assert resp.text == "0" + + # delete the test user, webhooks should be called during this delete request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) + + # Wait for the webhook requests to complete and check their success + sleep(1) + resp = requests.get(self.base_webhook_url + "/get_status") + assert resp.text == "2" def test_Webhook_DeleteUser_FailingUrl(self): """ Test deleting a user using a failing webhook url. """ - # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: - delete_webhook_url = "failing_url" - data = {"webhooks": {"delete": [delete_webhook_url]}} - yaml.dump(data, tmp_config, default_flow_style=False) + # Write temporary config for testing webhooks + delete_webhook_url = "failing_url" + data = {"webhooks": {"delete": [delete_webhook_url]}} + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) - with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): - # create the test user first - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - # delete the test user, webhooks should be called during the request - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + # delete the test user, webhooks should be called during the request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) - # Check if user deletion was successful even if the webhook failed - sleep(1) - utils.check_response_basic_info(resp, 200, expected_method="GET") - utils.TestSetup.check_NonExistingTestUser(self) + # Check if user deletion was successful even if the webhook failed + sleep(1) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) def test_Webhook_DeleteUser_EmptyUrl(self): """ Test deleting a user with an empty webhook url. """ - # Create temporary config for testing webhooks - with tempfile.NamedTemporaryFile(mode="wt") as tmp_config: - data = {"webhooks": {"delete": []}} - yaml.dump(data, tmp_config, default_flow_style=False) - - with mock.patch.dict(os.environ, {"MAGPIE_CONFIG_PATH": tmp_config.name}): - # create the test user first - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - - # delete the test user, webhooks should be called during the request - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) - - # Check if user deletion was successful even if no webhooks were defined in the config - sleep(1) - utils.check_response_basic_info(resp, 200, expected_method="GET") - utils.TestSetup.check_NonExistingTestUser(self) + # Write temporary config for testing webhooks + data = {"webhooks": {"delete": []}} + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) + + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # delete the test user, webhooks should be called during the request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + + # Check if user deletion was successful even if no webhooks were defined in the config + sleep(1) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) @runner.MAGPIE_TEST_API diff --git a/tests/utils.py b/tests/utils.py index fe1c8749b..98c293823 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -275,7 +275,7 @@ def get_app_or_url(test_item): return app_or_url -def get_test_webhook_app(): +def get_test_webhook_app(webhook_url): """ Instantiate a local test application used for the prehook for user creation and deletion. """ @@ -310,12 +310,12 @@ def reset_status(request): def webhook_app(): try: - webhook_url_info = urlparse(get_constant("MAGPIE_TEST_USER_WEBHOOK_URL")) + webhook_url_info = urlparse(webhook_url) serve(webhook_app_instance, host=webhook_url_info.hostname, port=webhook_url_info.port) except OSError as exception: if exception.errno == EADDRINUSE: # The app is already running, we just need to reset the webhook status for a new test. - resp = requests.post(get_constant("MAGPIE_TEST_USER_WEBHOOK_URL") + "/reset_status") + resp = requests.post(webhook_url + "/reset_status") check_response_basic_info(resp, 200, expected_type=CONTENT_TYPE_HTML, expected_method="POST") return raise From c704e5d4de8920bc63c57a40b3ffcc696fc68f93 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Tue, 5 Jan 2021 10:15:52 -0500 Subject: [PATCH 08/33] move webhooks tests --- env/magpie.env.example | 1 + setup.cfg | 1 + tests/runner.py | 1 + tests/test_magpie_api.py | 158 +----------------------------- tests/test_webhooks.py | 204 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 208 insertions(+), 157 deletions(-) create mode 100644 tests/test_webhooks.py diff --git a/env/magpie.env.example b/env/magpie.env.example index a3ecbd575..70a428072 100644 --- a/env/magpie.env.example +++ b/env/magpie.env.example @@ -50,3 +50,4 @@ MAGPIE_TEST_STATUS=true MAGPIE_TEST_UTILS=true MAGPIE_TEST_REGISTER=true MAGPIE_TEST_FUNCTIONAL=true +MAGPIE_TEST_WEBHOOKS=true diff --git a/setup.cfg b/setup.cfg index 5c2ea5edc..086181e23 100644 --- a/setup.cfg +++ b/setup.cfg @@ -100,6 +100,7 @@ markers = api: magpie API operations cli: magpie CLI helper operations ui: magpie UI operations + webhooks: magpie webhooks operations utils: magpie utility functions functional: magpie functional operations auth_admin: magpie operations that require admin-level access diff --git a/tests/runner.py b/tests/runner.py index 251531bc3..7c4f1375c 100644 --- a/tests/runner.py +++ b/tests/runner.py @@ -42,6 +42,7 @@ def filter_test_files(root, filename): MAGPIE_TEST_API = RunOptionDecorator("MAGPIE_TEST_API", "magpie API operations") MAGPIE_TEST_CLI = RunOptionDecorator("MAGPIE_TEST_CLI", "magpie CLI operations") MAGPIE_TEST_UI = RunOptionDecorator("MAGPIE_TEST_UI", "magpie UI operations") +MAGPIE_TEST_WEBHOOKS = RunOptionDecorator("MAGPIE_TEST_WEBHOOKS", "magpie webhooks operations") MAGPIE_TEST_UTILS = RunOptionDecorator("MAGPIE_TEST_UTILS", "magpie utility functions") MAGPIE_TEST_FUNCTIONAL = RunOptionDecorator("MAGPIE_TEST_FUNCTIONAL", "functional operations sequence") MAGPIE_TEST_AUTH_ADMIN = RunOptionDecorator("MAGPIE_TEST_AUTH_ADMIN", "operations that require admin-level access") diff --git a/tests/test_magpie_api.py b/tests/test_magpie_api.py index d020427fb..b6aad9b5b 100644 --- a/tests/test_magpie_api.py +++ b/tests/test_magpie_api.py @@ -99,16 +99,7 @@ class TestCase_MagpieAPI_AdminAuth_Local(ti.Interface_MagpieAPI_AdminAuth, unitt @classmethod def setUpClass(cls): - cls.base_webhook_url = "http://localhost:8080" - cls.webhook_tmp_config = tempfile.NamedTemporaryFile(mode="w") - # write default config with empty values, webhooks will be overwritten for each specific test setup - data = { - "webhooks": {}, - "providers": "", - "permissions": "" - } - yaml.dump(data, cls.webhook_tmp_config, default_flow_style=False) - cls.app = utils.get_test_magpie_app({"magpie.config_path": cls.webhook_tmp_config.name}) + cls.app = utils.get_test_magpie_app() cls.grp = get_constant("MAGPIE_ADMIN_GROUP") cls.usr = get_constant("MAGPIE_TEST_ADMIN_USERNAME") cls.pwd = get_constant("MAGPIE_TEST_ADMIN_PASSWORD") @@ -120,153 +111,6 @@ def setUpClass(cls): cls.check_requirements() cls.setup_test_values() - @classmethod - def tearDownClass(cls): - cls.webhook_tmp_config.close() - - def test_Webhook_CreateUser(self): - """ - Test creating a user using multiple webhooks. - """ - # Write temporary config for testing webhooks - create_webhook_url = self.base_webhook_url + "/webhook" - data = { - "webhooks": - # Use two identical urls to simulate having multiple webhook urls - {"create": [create_webhook_url, create_webhook_url]} - } - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.safe_dump(data, stream, default_flow_style=False) - - utils.get_test_webhook_app(self.base_webhook_url) - - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - - # Wait for the webhook requests to complete - sleep(1) - - # Check if both webhook requests have completed successfully - resp = requests.get(self.base_webhook_url + "/get_status") - assert resp.text == "2" - - # Check if user creation was successful - users = utils.TestSetup.get_RegisteredUsersList(self) - utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") - - def test_Webhook_CreateUser_FailingUrl(self): - """ - Test creating a user using a failing webhook url. - """ - # Write temporary config for testing webhooks - create_webhook_url = "failing_url" - data = {"webhooks": {"create": [create_webhook_url]}} - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) - - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - - # Wait for the webhook requests to complete - sleep(1) - - # Check if user creation was successful even if the webhook failed - users = utils.TestSetup.get_RegisteredUsersList(self) - utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") - - def test_Webhook_CreateUser_EmptyUrl(self): - """ - Test creating a user with an empty webhook url. - """ - # Write temporary config for testing webhooks - data = {"webhooks": {"create": []}} - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) - - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - - # Wait for the webhook requests to complete - sleep(1) - - # Check if user creation was successful even if no webhook were defined in the config - users = utils.TestSetup.get_RegisteredUsersList(self) - utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") - - def test_Webhook_DeleteUser(self): - """ - Test deleting a user using multiple webhooks. - """ - # Write temporary config for testing webhooks - delete_webhook_url = self.base_webhook_url + "/webhook" - data = { - "webhooks": - # Use two identical urls to simulate having multiple webhook urls - {"delete": [delete_webhook_url, delete_webhook_url]}} - - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) - - utils.get_test_webhook_app(self.base_webhook_url) - # create the test user first - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - - # Webhooks shouldn't have been called during the user creation - sleep(1) - resp = requests.get(self.base_webhook_url + "/get_status") - assert resp.text == "0" - - # delete the test user, webhooks should be called during this delete request - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) - utils.check_response_basic_info(resp, 200, expected_method="GET") - utils.TestSetup.check_NonExistingTestUser(self) - - # Wait for the webhook requests to complete and check their success - sleep(1) - resp = requests.get(self.base_webhook_url + "/get_status") - assert resp.text == "2" - - def test_Webhook_DeleteUser_FailingUrl(self): - """ - Test deleting a user using a failing webhook url. - """ - # Write temporary config for testing webhooks - delete_webhook_url = "failing_url" - data = {"webhooks": {"delete": [delete_webhook_url]}} - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) - - # create the test user first - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - - # delete the test user, webhooks should be called during the request - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) - - # Check if user deletion was successful even if the webhook failed - sleep(1) - utils.check_response_basic_info(resp, 200, expected_method="GET") - utils.TestSetup.check_NonExistingTestUser(self) - - def test_Webhook_DeleteUser_EmptyUrl(self): - """ - Test deleting a user with an empty webhook url. - """ - # Write temporary config for testing webhooks - data = {"webhooks": {"delete": []}} - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) - - # create the test user first - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - - # delete the test user, webhooks should be called during the request - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) - - # Check if user deletion was successful even if no webhooks were defined in the config - sleep(1) - utils.check_response_basic_info(resp, 200, expected_method="GET") - utils.TestSetup.check_NonExistingTestUser(self) - @runner.MAGPIE_TEST_API @runner.MAGPIE_TEST_REMOTE diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py new file mode 100644 index 000000000..df666baed --- /dev/null +++ b/tests/test_webhooks.py @@ -0,0 +1,204 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +""" +test_webhooks +---------------------------------- + +Tests for the webhooks implementation +""" +import tempfile +from time import sleep +import yaml + +import requests + +# NOTE: must be imported without 'from', otherwise the interface's test cases are also executed +import tests.interfaces as ti +from magpie.constants import get_constant +from tests import runner, utils + + +@runner.MAGPIE_TEST_WEBHOOKS +@runner.MAGPIE_TEST_LOCAL +class TestWebhooks(ti.AdminTestCase): + # pylint: disable=C0103,invalid-name + """ + Test any operation that uses webhooks. + + Use a local Magpie test application. + """ + + __test__ = True + + @classmethod + def setUpClass(cls): + cls.base_webhook_url = "http://localhost:8080" + cls.webhook_tmp_config = tempfile.NamedTemporaryFile(mode="w") + # write default config with empty values, webhooks will be overwritten for each specific test setup + data = { + "webhooks": {}, + "providers": "", + "permissions": "" + } + yaml.dump(data, cls.webhook_tmp_config, default_flow_style=False) + cls.app = utils.get_test_magpie_app({"magpie.config_path": cls.webhook_tmp_config.name}) + cls.grp = get_constant("MAGPIE_ADMIN_GROUP") + cls.usr = get_constant("MAGPIE_TEST_ADMIN_USERNAME") + cls.pwd = get_constant("MAGPIE_TEST_ADMIN_PASSWORD") + cls.cookies = None + cls.version = utils.TestSetup.get_Version(cls) + cls.setup_admin() + cls.headers, cls.cookies = utils.check_or_try_login_user(cls.app, cls.usr, cls.pwd, use_ui_form_submit=True) + cls.require = "cannot run tests without logged in user with '{}' permissions".format(cls.grp) + cls.check_requirements() + + cls.test_group_name = "magpie-unittest-dummy-group" + cls.test_user_name = "magpie-unittest-toto" + + @classmethod + def tearDownClass(cls): + cls.webhook_tmp_config.close() + + def test_Webhook_CreateUser(self): + """ + Test creating a user using multiple webhooks. + """ + # Write temporary config for testing webhooks + create_webhook_url = self.base_webhook_url + "/webhook" + data = { + "webhooks": + # Use two identical urls to simulate having multiple webhook urls + {"create": [create_webhook_url, create_webhook_url]} + } + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.safe_dump(data, stream, default_flow_style=False) + + utils.get_test_webhook_app(self.base_webhook_url) + + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Wait for the webhook requests to complete + sleep(1) + + # Check if both webhook requests have completed successfully + resp = requests.get(self.base_webhook_url + "/get_status") + assert resp.text == "2" + + # Check if user creation was successful + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + + def test_Webhook_CreateUser_FailingUrl(self): + """ + Test creating a user using a failing webhook url. + """ + # Write temporary config for testing webhooks + create_webhook_url = "failing_url" + data = {"webhooks": {"create": [create_webhook_url]}} + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) + + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Wait for the webhook requests to complete + sleep(1) + + # Check if user creation was successful even if the webhook failed + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + + def test_Webhook_CreateUser_EmptyUrl(self): + """ + Test creating a user with an empty webhook url. + """ + # Write temporary config for testing webhooks + data = {"webhooks": {"create": []}} + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) + + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Wait for the webhook requests to complete + sleep(1) + + # Check if user creation was successful even if no webhook were defined in the config + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + + def test_Webhook_DeleteUser(self): + """ + Test deleting a user using multiple webhooks. + """ + # Write temporary config for testing webhooks + delete_webhook_url = self.base_webhook_url + "/webhook" + data = { + "webhooks": + # Use two identical urls to simulate having multiple webhook urls + {"delete": [delete_webhook_url, delete_webhook_url]}} + + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) + + utils.get_test_webhook_app(self.base_webhook_url) + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Webhooks shouldn't have been called during the user creation + sleep(1) + resp = requests.get(self.base_webhook_url + "/get_status") + assert resp.text == "0" + + # delete the test user, webhooks should be called during this delete request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) + + # Wait for the webhook requests to complete and check their success + sleep(1) + resp = requests.get(self.base_webhook_url + "/get_status") + assert resp.text == "2" + + def test_Webhook_DeleteUser_FailingUrl(self): + """ + Test deleting a user using a failing webhook url. + """ + # Write temporary config for testing webhooks + delete_webhook_url = "failing_url" + data = {"webhooks": {"delete": [delete_webhook_url]}} + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) + + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # delete the test user, webhooks should be called during the request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + + # Check if user deletion was successful even if the webhook failed + sleep(1) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) + + def test_Webhook_DeleteUser_EmptyUrl(self): + """ + Test deleting a user with an empty webhook url. + """ + # Write temporary config for testing webhooks + data = {"webhooks": {"delete": []}} + with open(self.webhook_tmp_config.name, 'w') as stream: + yaml.dump(data, stream, default_flow_style=False) + + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # delete the test user, webhooks should be called during the request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + + # Check if user deletion was successful even if no webhooks were defined in the config + sleep(1) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) From 8a59b17cd7b94aa705ebc05c818e6a7daa11e5e8 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Thu, 7 Jan 2021 14:29:33 -0500 Subject: [PATCH 09/33] webhook tests refactoring and checking webhook config --- magpie/api/management/user/user_utils.py | 21 +- magpie/api/management/user/user_views.py | 19 +- magpie/api/requests.py | 17 +- magpie/app.py | 47 +++- tests/test_webhooks.py | 289 +++++++++++++---------- tests/utils.py | 4 +- 6 files changed, 249 insertions(+), 148 deletions(-) diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index 1552a6acd..5baad2dcd 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -11,6 +11,7 @@ HTTPNotFound, HTTPOk ) +from pyramid.threadlocal import get_current_registry from ziggurat_foundations.models.services.group import GroupService from ziggurat_foundations.models.services.resource import ResourceService from ziggurat_foundations.models.services.user import UserService @@ -25,9 +26,8 @@ from magpie.api.management.user import user_formats as uf from magpie.constants import get_constant from magpie.permissions import PermissionSet, PermissionType, format_permissions -from magpie.register import get_all_configs from magpie.services import service_factory -from magpie.utils import get_logger +from magpie.utils import get_logger, get_settings LOGGER = get_logger(__name__) @@ -113,16 +113,13 @@ def _add_to_group(usr, grp): new_user_groups.append(anonym_grp_name) # Check for webhook requests - config_path = get_constant("MAGPIE_CONFIG_PATH", default_value=None, - raise_missing=False, raise_not_set=False, print_missing=True) - if config_path: - webhook_configs = get_all_configs(config_path, "webhooks", allow_missing=True) - for cfg in webhook_configs: - if "create" in cfg.keys() and len(cfg["create"]) > 0: - # Execute all webhook requests - pool = multiprocessing.Pool(processes=len(cfg["create"])) - args = [(url, user_name) for url in cfg["create"]] - pool.starmap_async(ar.webhook_request, args, error_callback=ar.webhook_error_callback) + webhooks = get_settings(get_current_registry())["webhooks"]["create_user"] + if len(webhooks) > 0: + # Execute all webhook requests + pool = multiprocessing.Pool(processes=len(webhooks)) + args = [(webhook["url"], webhook["payload"], {"user_name": user_name, "temp_url": "temp_url:80/todo"}) + for webhook in webhooks] + pool.starmap_async(ar.webhook_request, args, error_callback=ar.webhook_error_callback) return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, content={"user": uf.format_user(new_user, new_user_groups)}) diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index a443d6c4e..133f070d2 100644 --- a/magpie/api/management/user/user_views.py +++ b/magpie/api/management/user/user_views.py @@ -5,6 +5,7 @@ from pyramid.httpexceptions import HTTPBadRequest, HTTPConflict, HTTPCreated, HTTPForbidden, HTTPNotFound, HTTPOk from pyramid.settings import asbool +from pyramid.threadlocal import get_current_registry from pyramid.view import view_config from ziggurat_foundations.models.services.group import GroupService from ziggurat_foundations.models.services.resource import ResourceService @@ -21,7 +22,7 @@ from magpie.permissions import PermissionType, format_permissions from magpie.register import get_all_configs from magpie.services import SERVICE_TYPE_DICT -from magpie.utils import get_logger +from magpie.utils import get_logger, get_settings LOGGER = get_logger(__name__) @@ -140,16 +141,12 @@ def delete_user_view(request): http_error=HTTPForbidden, msg_on_fail=s.User_DELETE_ForbiddenResponseSchema.description) # Check for webhook requests - config_path = get_constant("MAGPIE_CONFIG_PATH", default_value=None, - raise_missing=False, raise_not_set=False, print_missing=True) - if config_path: - webhook_configs = get_all_configs(config_path, "webhooks", allow_missing=True) - for cfg in webhook_configs: - if "delete" in cfg.keys() and len(cfg["delete"]) > 0: - # Execute all webhook requests - pool = multiprocessing.Pool(processes=len(cfg["delete"])) - args = [(url, user.user_name) for url in cfg["delete"]] - pool.starmap_async(ar.webhook_request, args, error_callback=ar.webhook_error_callback) + webhooks = get_settings(get_current_registry())["webhooks"]["delete_user"] + if len(webhooks) > 0: + # Execute all webhook requests + pool = multiprocessing.Pool(processes=len(webhooks)) + args = [(webhook["url"], webhook["payload"], {"user_name": user.user_name}) for webhook in webhooks] + pool.starmap_async(ar.webhook_request, args, error_callback=ar.webhook_error_callback) return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description) diff --git a/magpie/api/requests.py b/magpie/api/requests.py index ef13e5360..829c54959 100644 --- a/magpie/api/requests.py +++ b/magpie/api/requests.py @@ -347,14 +347,27 @@ def get_query_param(request, case_insensitive_key, default=None): return default -def webhook_request(webhook_url, user_name): +def webhook_request(webhook_url, webhook_payload, params): # type: (Str, Str) -> None """ Sends a webhook request using the input url. """ + # Use default temp_url if it is not defined in the input parameters + # TODO + # if "temp_url" not in params: + # params["temp_url"] = "temp_url:80/todo" + + # These are the parameters permitted to use the template form in the webhook payload. + webhook_template_params = ["user_name", "temp_url"] + + for template_param in webhook_template_params: + if template_param in params: + for k,v in webhook_payload.items(): + webhook_payload[k] = v.replace("{" + template_param + "}", params[template_param]) + # TODO: create a real temp_url that will be called if the webhook service has an error # this would also set the user's status to 0 - requests.post(webhook_url, data={"user_name": user_name, "temp_url": "temp_url:80/todo"}) + requests.post(webhook_url, data=webhook_payload) def webhook_error_callback(exception): diff --git a/magpie/app.py b/magpie/app.py index 042c9f1f8..e8caa8a6c 100644 --- a/magpie/app.py +++ b/magpie/app.py @@ -4,6 +4,8 @@ """ Magpie is a service for AuthN and AuthZ based on Ziggurat-Foundations. """ +from collections import defaultdict +from urllib.parse import urlparse from pyramid.settings import asbool from pyramid_beaker import set_cache_regions_from_settings @@ -11,12 +13,26 @@ from magpie.cli.register_defaults import register_defaults from magpie.constants import get_constant from magpie.db import get_db_session_from_config_ini, run_database_migration_when_ready, set_sqlalchemy_log_level -from magpie.register import magpie_register_permissions_from_config, magpie_register_services_from_config +from magpie.register import magpie_register_permissions_from_config, magpie_register_services_from_config, \ + get_all_configs from magpie.security import get_auth_config from magpie.utils import get_logger, patch_magpie_url, print_log LOGGER = get_logger(__name__) +WEBHOOK_KEYS = { + "name", + "action", + "method", + "url", + "payload" +} +WEBHOOK_ACTIONS = [ + "create_user", + "delete_user" +] +HTTP_METHODS = ["GET", "OPTIONS", "HEAD", "POST", "PUT", "PATCH", "DELETE"] + def main(global_config=None, **settings): # noqa: F811 """ @@ -74,6 +90,35 @@ def main(global_config=None, **settings): # noqa: F811 raise_missing=False, raise_not_set=False, print_missing=True) magpie_register_permissions_from_config(perm_cfg, db_session=db_session) + print_log("Register webhook configurations...", LOGGER) + settings["webhooks"] = [] + if combined_config: + webhook_configs = get_all_configs(combined_config, "webhooks", allow_missing=True) + webhook_configs_by_action = defaultdict(lambda: []) + + for cfg in webhook_configs: + for webhook in cfg: + # Validate the webhook config + if webhook.keys() != WEBHOOK_KEYS: + raise ValueError(f"Missing or invalid key found in a webhook config " + f"from the config file {combined_config}") + if webhook["action"] not in WEBHOOK_ACTIONS: + raise ValueError(f"Invalid action {webhook['action']} found in a webhook config " + f"from the config file {combined_config}") + if webhook["method"] not in HTTP_METHODS: + raise ValueError(f"Invalid method {webhook['method']} found in a webhook config " + f"from the config file {combined_config}") + url_parsed = urlparse(webhook["url"]) + if not all([url_parsed.scheme, url_parsed.netloc, url_parsed.path]): + raise ValueError(f"Invalid url {webhook['url']} found in a webhook config " + f"from the config file {combined_config}") + + # Regroup webhooks by action key + webhook_sub_config = {k: webhook[k] for k in set(list(webhook.keys())) - {"action"}} + webhook_configs_by_action[webhook["action"]].append(webhook_sub_config) + + settings["webhooks"] = webhook_configs_by_action + print_log("Running configurations setup...", LOGGER) patch_magpie_url(settings) diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index df666baed..36991797d 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -8,20 +8,20 @@ Tests for the webhooks implementation """ import tempfile +import unittest from time import sleep import yaml import requests -# NOTE: must be imported without 'from', otherwise the interface's test cases are also executed -import tests.interfaces as ti from magpie.constants import get_constant +from magpie.utils import CONTENT_TYPE_JSON from tests import runner, utils @runner.MAGPIE_TEST_WEBHOOKS @runner.MAGPIE_TEST_LOCAL -class TestWebhooks(ti.AdminTestCase): +class TestWebhooks(unittest.TestCase): # pylint: disable=C0103,invalid-name """ Test any operation that uses webhooks. @@ -34,171 +34,218 @@ class TestWebhooks(ti.AdminTestCase): @classmethod def setUpClass(cls): cls.base_webhook_url = "http://localhost:8080" - cls.webhook_tmp_config = tempfile.NamedTemporaryFile(mode="w") - # write default config with empty values, webhooks will be overwritten for each specific test setup - data = { - "webhooks": {}, - "providers": "", - "permissions": "" - } - yaml.dump(data, cls.webhook_tmp_config, default_flow_style=False) - cls.app = utils.get_test_magpie_app({"magpie.config_path": cls.webhook_tmp_config.name}) cls.grp = get_constant("MAGPIE_ADMIN_GROUP") cls.usr = get_constant("MAGPIE_TEST_ADMIN_USERNAME") cls.pwd = get_constant("MAGPIE_TEST_ADMIN_PASSWORD") - cls.cookies = None cls.version = utils.TestSetup.get_Version(cls) - cls.setup_admin() - cls.headers, cls.cookies = utils.check_or_try_login_user(cls.app, cls.usr, cls.pwd, use_ui_form_submit=True) - cls.require = "cannot run tests without logged in user with '{}' permissions".format(cls.grp) - cls.check_requirements() - cls.test_group_name = "magpie-unittest-dummy-group" cls.test_user_name = "magpie-unittest-toto" - @classmethod - def tearDownClass(cls): - cls.webhook_tmp_config.close() + cls.json_headers = {"Accept": CONTENT_TYPE_JSON, "Content-Type": CONTENT_TYPE_JSON} + cls.extra_user_names = set() + + def tearDown(self): + """ + Cleans up the test user after a test + """ + utils.check_or_try_logout_user(self) + self.headers, self.cookies = utils.check_or_try_login_user(self.app, username=self.usr, password=self.pwd) + utils.TestSetup.delete_TestUser(self) + utils.TestSetup.delete_TestGroup(self) + + def setup_webhook_test(self, config_path): + """ + Prepares a Magpie app using a specific testing config for webhooks + :param config_path: path to the config file containing the webhook configs + """ + self.app = utils.get_test_magpie_app({"magpie.config_path": config_path}) + self.headers, self.cookies = utils.check_or_try_login_user(self.app, self.usr, self.pwd, use_ui_form_submit=True) def test_Webhook_CreateUser(self): """ - Test creating a user using multiple webhooks. + Test creating a user using webhooks. """ # Write temporary config for testing webhooks create_webhook_url = self.base_webhook_url + "/webhook" data = { - "webhooks": - # Use two identical urls to simulate having multiple webhook urls - {"create": [create_webhook_url, create_webhook_url]} + "webhooks": [ + { + "name": "test_webhook", + "action": "create_user", + "method": "POST", + "url": create_webhook_url, + "payload": {"user_name": "{user_name}", "temp_url": "{temp_url}"} + }, + { + "name": "test_webhook_2", + "action": "create_user", + "method": "POST", + "url": create_webhook_url, + "payload": {"user_name": "{user_name}", "temp_url": "{temp_url}"} + } + ], + "providers": "", + "permissions": "" } - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.safe_dump(data, stream, default_flow_style=False) - utils.get_test_webhook_app(self.base_webhook_url) + with tempfile.NamedTemporaryFile(mode="w") as webhook_tmp_config: + yaml.safe_dump(data, webhook_tmp_config, default_flow_style=False) - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + # create the magpie app with the test webhook config + self.setup_webhook_test(webhook_tmp_config.name) - # Wait for the webhook requests to complete - sleep(1) + utils.get_test_webhook_app(self.base_webhook_url) - # Check if both webhook requests have completed successfully - resp = requests.get(self.base_webhook_url + "/get_status") - assert resp.text == "2" + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - # Check if user creation was successful - users = utils.TestSetup.get_RegisteredUsersList(self) - utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + # Wait for the webhook requests to complete + sleep(1) - def test_Webhook_CreateUser_FailingUrl(self): - """ - Test creating a user using a failing webhook url. - """ - # Write temporary config for testing webhooks - create_webhook_url = "failing_url" - data = {"webhooks": {"create": [create_webhook_url]}} - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) - - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + # Check if both webhook requests have completed successfully + resp = requests.get(self.base_webhook_url + "/get_status") + assert resp.text == "2" - # Wait for the webhook requests to complete - sleep(1) - - # Check if user creation was successful even if the webhook failed - users = utils.TestSetup.get_RegisteredUsersList(self) - utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + # Check if user creation was successful + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") def test_Webhook_CreateUser_EmptyUrl(self): """ Test creating a user with an empty webhook url. """ # Write temporary config for testing webhooks - data = {"webhooks": {"create": []}} - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) + data = { + "webhooks": [], + "providers": "", + "permissions": "" + } - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + with tempfile.NamedTemporaryFile(mode="w") as webhook_tmp_config: + yaml.safe_dump(data, webhook_tmp_config, default_flow_style=False) + # create the magpie app with the test webhook config + self.setup_webhook_test(webhook_tmp_config.name) - # Wait for the webhook requests to complete - sleep(1) + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - # Check if user creation was successful even if no webhook were defined in the config - users = utils.TestSetup.get_RegisteredUsersList(self) - utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + # Wait for the webhook requests to complete + sleep(1) + + # Check if user creation was successful even if no webhook were defined in the config + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") def test_Webhook_DeleteUser(self): """ - Test deleting a user using multiple webhooks. + Test deleting a user using webhooks. """ # Write temporary config for testing webhooks delete_webhook_url = self.base_webhook_url + "/webhook" data = { - "webhooks": - # Use two identical urls to simulate having multiple webhook urls - {"delete": [delete_webhook_url, delete_webhook_url]}} - - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) - - utils.get_test_webhook_app(self.base_webhook_url) - # create the test user first - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - - # Webhooks shouldn't have been called during the user creation - sleep(1) - resp = requests.get(self.base_webhook_url + "/get_status") - assert resp.text == "0" - - # delete the test user, webhooks should be called during this delete request - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) - utils.check_response_basic_info(resp, 200, expected_method="GET") - utils.TestSetup.check_NonExistingTestUser(self) - - # Wait for the webhook requests to complete and check their success - sleep(1) - resp = requests.get(self.base_webhook_url + "/get_status") - assert resp.text == "2" - - def test_Webhook_DeleteUser_FailingUrl(self): - """ - Test deleting a user using a failing webhook url. - """ - # Write temporary config for testing webhooks - delete_webhook_url = "failing_url" - data = {"webhooks": {"delete": [delete_webhook_url]}} - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) + "webhooks": [ + { + "name": "test_webhook", + "action": "delete_user", + "method": "POST", + "url": delete_webhook_url, + "payload": {"user_name": "{user_name}"} + }, + { + "name": "test_webhook_2", + "action": "delete_user", + "method": "POST", + "url": delete_webhook_url, + "payload": {"user_name": "{user_name}"} + } + ], + "providers": "", + "permissions": "" + } + + with tempfile.NamedTemporaryFile(mode="w") as webhook_tmp_config: + yaml.dump(data, webhook_tmp_config, default_flow_style=False) + # create the magpie app with the test webhook config + self.setup_webhook_test(webhook_tmp_config.name) + + utils.get_test_webhook_app(self.base_webhook_url) + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - # create the test user first - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + # Webhooks shouldn't have been called during the user creation + sleep(1) + resp = requests.get(self.base_webhook_url + "/get_status") + assert resp.text == "0" - # delete the test user, webhooks should be called during the request - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + # delete the test user, webhooks should be called during this delete request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) - # Check if user deletion was successful even if the webhook failed - sleep(1) - utils.check_response_basic_info(resp, 200, expected_method="GET") - utils.TestSetup.check_NonExistingTestUser(self) + # Wait for the webhook requests to complete and check their success + sleep(1) + resp = requests.get(self.base_webhook_url + "/get_status") + assert resp.text == "2" def test_Webhook_DeleteUser_EmptyUrl(self): """ Test deleting a user with an empty webhook url. """ # Write temporary config for testing webhooks - data = {"webhooks": {"delete": []}} - with open(self.webhook_tmp_config.name, 'w') as stream: - yaml.dump(data, stream, default_flow_style=False) + data = { + "webhooks": [], + "providers": "", + "permissions": "" + } + + with tempfile.NamedTemporaryFile(mode="w") as webhook_tmp_config: + yaml.safe_dump(data, webhook_tmp_config, default_flow_style=False) + # create the magpie app with the test webhook config + self.setup_webhook_test(webhook_tmp_config.name) + + # create the test user first + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # delete the test user, webhooks should be called during the request + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + + # Check if user deletion was successful even if no webhooks were defined in the config + sleep(1) + utils.check_response_basic_info(resp, 200, expected_method="GET") + utils.TestSetup.check_NonExistingTestUser(self) + + +@runner.MAGPIE_TEST_WEBHOOKS +@runner.MAGPIE_TEST_LOCAL +class TestFailingWebhooks(unittest.TestCase): + # pylint: disable=C0103,invalid-name + """ + Test any operation that uses an incorrect webhook config. + """ - # create the test user first - utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + __test__ = True - # delete the test user, webhooks should be called during the request - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "DELETE", path, headers=self.json_headers, cookies=self.cookies) + def test_Webhook_Config_FailingUrl(self): + """ + Test using a config with a failing webhook url. + """ + # Write temporary config for testing webhooks + create_webhook_url = "failing_url" + data = { + "webhooks": [ + { + "name": "test_webhook_app", + "action": "create_user", + "method": "POST", + "url": create_webhook_url, + "payload": {"user_name": "{user_name}", "temp_url": "{temp_url}"} + } + ], + "providers": "", + "permissions": "" + } - # Check if user deletion was successful even if no webhooks were defined in the config - sleep(1) - utils.check_response_basic_info(resp, 200, expected_method="GET") - utils.TestSetup.check_NonExistingTestUser(self) + with tempfile.NamedTemporaryFile(mode="w") as webhook_tmp_config: + yaml.safe_dump(data, webhook_tmp_config, default_flow_style=False) + # create the magpie app with the test webhook config + self.assertRaises(ValueError, utils.get_test_magpie_app, {"magpie.config_path": webhook_tmp_config.name}) diff --git a/tests/utils.py b/tests/utils.py index 98c293823..0f498fa04 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -282,7 +282,9 @@ def get_test_webhook_app(webhook_url): def webhook_request(request): # Simulates a webhook url call user = request.POST["user_name"] - temp_url = request.POST["temp_url"] + + #TODO: fix temp_url, should be optional? delete has no temp_url + #temp_url = request.POST["temp_url"] # Status is incremented to count the number of successful test webhooks settings["webhook_status"] += 1 From 18ed180ff64674779c08f3b9c5d8586c2bc7660a Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Tue, 12 Jan 2021 13:46:02 -0500 Subject: [PATCH 10/33] change user status on error, test for non existent url --- magpie/api/management/user/user_utils.py | 12 ++- magpie/api/management/user/user_views.py | 5 +- magpie/api/requests.py | 42 ++++---- tests/test_webhooks.py | 126 ++++++++++++++++++++--- tests/utils.py | 35 +++++-- 5 files changed, 172 insertions(+), 48 deletions(-) diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index 5baad2dcd..eefadd08c 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -12,6 +12,7 @@ HTTPOk ) from pyramid.threadlocal import get_current_registry +import transaction from ziggurat_foundations.models.services.group import GroupService from ziggurat_foundations.models.services.resource import ResourceService from ziggurat_foundations.models.services.user import UserService @@ -112,17 +113,22 @@ def _add_to_group(usr, grp): _add_to_group(new_user, _get_group(anonym_grp_name)) new_user_groups.append(anonym_grp_name) + user_content = uf.format_user(new_user, new_user_groups) + + # Force commit before sending the webhook requests, so that the user's status is editable if a webhook error occurs + transaction.commit() + # Check for webhook requests webhooks = get_settings(get_current_registry())["webhooks"]["create_user"] if len(webhooks) > 0: # Execute all webhook requests pool = multiprocessing.Pool(processes=len(webhooks)) - args = [(webhook["url"], webhook["payload"], {"user_name": user_name, "temp_url": "temp_url:80/todo"}) + args = [(webhook, {"user_name": user_name, "tmp_url": "tmp_url:80/todo"}, True) for webhook in webhooks] - pool.starmap_async(ar.webhook_request, args, error_callback=ar.webhook_error_callback) + pool.starmap_async(ar.webhook_request, args) return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, - content={"user": uf.format_user(new_user, new_user_groups)}) + content={"user": user_content}) def create_user_resource_permission_response(user, resource, permission, db_session, overwrite=False): diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index 133f070d2..a0a569a29 100644 --- a/magpie/api/management/user/user_views.py +++ b/magpie/api/management/user/user_views.py @@ -20,7 +20,6 @@ from magpie.api.management.user import user_utils as uu from magpie.constants import MAGPIE_CONTEXT_PERMISSION, MAGPIE_LOGGED_PERMISSION, get_constant from magpie.permissions import PermissionType, format_permissions -from magpie.register import get_all_configs from magpie.services import SERVICE_TYPE_DICT from magpie.utils import get_logger, get_settings @@ -145,8 +144,8 @@ def delete_user_view(request): if len(webhooks) > 0: # Execute all webhook requests pool = multiprocessing.Pool(processes=len(webhooks)) - args = [(webhook["url"], webhook["payload"], {"user_name": user.user_name}) for webhook in webhooks] - pool.starmap_async(ar.webhook_request, args, error_callback=ar.webhook_error_callback) + args = [(webhook, {"user_name": user.user_name}) for webhook in webhooks] + pool.starmap_async(ar.webhook_request, args) return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description) diff --git a/magpie/api/requests.py b/magpie/api/requests.py index 829c54959..77cc4d9c8 100644 --- a/magpie/api/requests.py +++ b/magpie/api/requests.py @@ -10,6 +10,7 @@ HTTPNotFound, HTTPUnprocessableEntity ) +import transaction from ziggurat_foundations.models.services.group import GroupService from ziggurat_foundations.models.services.resource import ResourceService from ziggurat_foundations.models.services.user import UserService @@ -17,7 +18,8 @@ from magpie import models from magpie.api import exception as ax from magpie.api import schemas as s -from magpie.constants import get_constant +from magpie.constants import get_constant, MAGPIE_INI_FILE_PATH +from magpie.db import get_db_session_from_config_ini from magpie.permissions import PermissionSet from magpie.utils import CONTENT_TYPE_JSON, get_logger @@ -347,34 +349,34 @@ def get_query_param(request, case_insensitive_key, default=None): return default -def webhook_request(webhook_url, webhook_payload, params): - # type: (Str, Str) -> None +def webhook_request(webhook_config, params, update_user_status_on_error=False): + # type: (Dict, Dict, bool) -> None """ Sends a webhook request using the input url. """ - # Use default temp_url if it is not defined in the input parameters - # TODO - # if "temp_url" not in params: - # params["temp_url"] = "temp_url:80/todo" - # These are the parameters permitted to use the template form in the webhook payload. - webhook_template_params = ["user_name", "temp_url"] + webhook_template_params = ["user_name", "tmp_url"] + # Replace each instance of template parameters if a corresponding value was defined in input for template_param in webhook_template_params: if template_param in params: - for k,v in webhook_payload.items(): - webhook_payload[k] = v.replace("{" + template_param + "}", params[template_param]) + for k,v in webhook_config["payload"].items(): + webhook_config["payload"][k] = v.replace("{" + template_param + "}", params[template_param]) - # TODO: create a real temp_url that will be called if the webhook service has an error - # this would also set the user's status to 0 - requests.post(webhook_url, data=webhook_payload) + try: + resp = requests.request(webhook_config["method"], webhook_config["url"], data=webhook_config["payload"]) + resp.raise_for_status() + except requests.exceptions.RequestException as e: + LOGGER.error(str(e)) + if "user_name" in params.keys() and update_user_status_on_error: + webhook_update_error_status(params["user_name"]) -def webhook_error_callback(exception): - # type: (requests.exceptions) -> None +def webhook_update_error_status(user_name): """ - Error callback function called if an error occurs in the webhook_call function. + Updates the user's status to indicate an error occured with the webhook requests """ - # TODO : (related to TODO in webhook_call function) handle errors occuring in the thread for the webhook_call - # change user's status to 0? - LOGGER.error(str(exception)) + # find user and change its status to 0 to indicate a webhook error happened + db_session = get_db_session_from_config_ini(MAGPIE_INI_FILE_PATH) + db_session.query(models.User).filter(models.User.user_name == user_name).update({"status": 0}) + transaction.commit() diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index 36991797d..bedfa8847 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -18,6 +18,8 @@ from magpie.utils import CONTENT_TYPE_JSON from tests import runner, utils +BASE_WEBHOOK_URL = "http://localhost:8080" + @runner.MAGPIE_TEST_WEBHOOKS @runner.MAGPIE_TEST_LOCAL @@ -33,7 +35,6 @@ class TestWebhooks(unittest.TestCase): @classmethod def setUpClass(cls): - cls.base_webhook_url = "http://localhost:8080" cls.grp = get_constant("MAGPIE_ADMIN_GROUP") cls.usr = get_constant("MAGPIE_TEST_ADMIN_USERNAME") cls.pwd = get_constant("MAGPIE_TEST_ADMIN_PASSWORD") @@ -49,7 +50,8 @@ def tearDown(self): Cleans up the test user after a test """ utils.check_or_try_logout_user(self) - self.headers, self.cookies = utils.check_or_try_login_user(self.app, username=self.usr, password=self.pwd) + self.headers, self.cookies = utils.check_or_try_login_user(self.app, self.usr, self.pwd, + use_ui_form_submit=True) utils.TestSetup.delete_TestUser(self) utils.TestSetup.delete_TestGroup(self) @@ -59,14 +61,19 @@ def setup_webhook_test(self, config_path): :param config_path: path to the config file containing the webhook configs """ self.app = utils.get_test_magpie_app({"magpie.config_path": config_path}) - self.headers, self.cookies = utils.check_or_try_login_user(self.app, self.usr, self.pwd, use_ui_form_submit=True) + self.headers, self.cookies = utils.check_or_try_login_user(self.app, self.usr, self.pwd, + use_ui_form_submit=True) + + # Make sure the test user doesn't already exists from a previous test + utils.TestSetup.delete_TestUser(self) + utils.TestSetup.delete_TestGroup(self) def test_Webhook_CreateUser(self): """ Test creating a user using webhooks. """ # Write temporary config for testing webhooks - create_webhook_url = self.base_webhook_url + "/webhook" + create_webhook_url = BASE_WEBHOOK_URL + "/webhook_create" data = { "webhooks": [ { @@ -74,14 +81,14 @@ def test_Webhook_CreateUser(self): "action": "create_user", "method": "POST", "url": create_webhook_url, - "payload": {"user_name": "{user_name}", "temp_url": "{temp_url}"} + "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} }, { "name": "test_webhook_2", "action": "create_user", "method": "POST", "url": create_webhook_url, - "payload": {"user_name": "{user_name}", "temp_url": "{temp_url}"} + "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} } ], "providers": "", @@ -94,7 +101,7 @@ def test_Webhook_CreateUser(self): # create the magpie app with the test webhook config self.setup_webhook_test(webhook_tmp_config.name) - utils.get_test_webhook_app(self.base_webhook_url) + utils.get_test_webhook_app(BASE_WEBHOOK_URL) utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) @@ -102,7 +109,7 @@ def test_Webhook_CreateUser(self): sleep(1) # Check if both webhook requests have completed successfully - resp = requests.get(self.base_webhook_url + "/get_status") + resp = requests.get(BASE_WEBHOOK_URL + "/get_status") assert resp.text == "2" # Check if user creation was successful @@ -134,12 +141,101 @@ def test_Webhook_CreateUser_EmptyUrl(self): users = utils.TestSetup.get_RegisteredUsersList(self) utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + # Skip this test, until tmp_url is implemented + @unittest.skip("implement tmp_url") + def test_Webhook_CreateUser_FailingWebhook(self): + """ + Test creating a user where the webhook receives an internal error. + This should trigger a callback to Magpie using the tmp_url. + """ + # Write temporary config for testing webhooks + webhook_fail_url = BASE_WEBHOOK_URL + "/webhook_fail" + data = { + "webhooks": [ + { + "name": "test_webhook", + "action": "create_user", + "method": "POST", + "url": webhook_fail_url, + "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} + } + + ], + "providers": "", + "permissions": "" + } + + with tempfile.NamedTemporaryFile(mode="w") as webhook_tmp_config: + yaml.safe_dump(data, webhook_tmp_config, default_flow_style=False) + # create the magpie app with the test webhook config + self.setup_webhook_test(webhook_tmp_config.name) + + utils.get_test_webhook_app(BASE_WEBHOOK_URL) + + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Wait for the webhook requests to complete + sleep(1) + + # Check if user creation was successful even if the webhook resulted in failure + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + + # Check if the user's status is set to 0 + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "GET", path, headers=self.json_headers, cookies=self.cookies) + body = utils.check_response_basic_info(resp, 200, expected_method="GET") + info = utils.TestSetup.get_UserInfo(self, override_body=body) + utils.check_val_equal(info["status"], 0) + + def test_Webhook_CreateUser_NonExistentWebhookUrl(self): + """ + Test creating a user where the webhook config has a non existent url + """ + # Write temporary config for testing webhooks + webhook_url = BASE_WEBHOOK_URL + "/non_existent" + data = { + "webhooks": [ + { + "name": "test_webhook", + "action": "create_user", + "method": "POST", + "url": webhook_url, + "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} + } + + ], + "providers": "", + "permissions": "" + } + + with tempfile.NamedTemporaryFile(mode="w") as webhook_tmp_config: + yaml.safe_dump(data, webhook_tmp_config, default_flow_style=False) + # create the magpie app with the test webhook config + self.setup_webhook_test(webhook_tmp_config.name) + + utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) + + # Wait for the webhook requests to complete + sleep(1) + + # Check if user creation was successful even if the webhook resulted in failure + users = utils.TestSetup.get_RegisteredUsersList(self) + utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + + # Check if the user's status is set to 0 + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "GET", path, headers=self.json_headers, cookies=self.cookies) + body = utils.check_response_basic_info(resp, 200, expected_method="GET") + info = utils.TestSetup.get_UserInfo(self, override_body=body) + utils.check_val_equal(info["status"], 0) + def test_Webhook_DeleteUser(self): """ Test deleting a user using webhooks. """ # Write temporary config for testing webhooks - delete_webhook_url = self.base_webhook_url + "/webhook" + delete_webhook_url = BASE_WEBHOOK_URL + "/webhook_delete" data = { "webhooks": [ { @@ -166,13 +262,13 @@ def test_Webhook_DeleteUser(self): # create the magpie app with the test webhook config self.setup_webhook_test(webhook_tmp_config.name) - utils.get_test_webhook_app(self.base_webhook_url) + utils.get_test_webhook_app(BASE_WEBHOOK_URL) # create the test user first utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) # Webhooks shouldn't have been called during the user creation sleep(1) - resp = requests.get(self.base_webhook_url + "/get_status") + resp = requests.get(BASE_WEBHOOK_URL + "/get_status") assert resp.text == "0" # delete the test user, webhooks should be called during this delete request @@ -183,7 +279,7 @@ def test_Webhook_DeleteUser(self): # Wait for the webhook requests to complete and check their success sleep(1) - resp = requests.get(self.base_webhook_url + "/get_status") + resp = requests.get(BASE_WEBHOOK_URL + "/get_status") assert resp.text == "2" def test_Webhook_DeleteUser_EmptyUrl(self): @@ -225,9 +321,9 @@ class TestFailingWebhooks(unittest.TestCase): __test__ = True - def test_Webhook_Config_FailingUrl(self): + def test_Webhook_IncorrectConfig(self): """ - Test using a config with a failing webhook url. + Test using a config with a badly formatted url. """ # Write temporary config for testing webhooks create_webhook_url = "failing_url" @@ -238,7 +334,7 @@ def test_Webhook_Config_FailingUrl(self): "action": "create_user", "method": "POST", "url": create_webhook_url, - "payload": {"user_name": "{user_name}", "temp_url": "{temp_url}"} + "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} } ], "providers": "", diff --git a/tests/utils.py b/tests/utils.py index 0f498fa04..143ed94a8 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -279,16 +279,30 @@ def get_test_webhook_app(webhook_url): """ Instantiate a local test application used for the prehook for user creation and deletion. """ - def webhook_request(request): - # Simulates a webhook url call + def webhook_create_request(request): + # Simulates a webhook url call during user creation user = request.POST["user_name"] + tmp_url = request.POST["tmp_url"] - #TODO: fix temp_url, should be optional? delete has no temp_url - #temp_url = request.POST["temp_url"] + # Status is incremented to count the number of successful test webhooks + settings["webhook_status"] += 1 + return Response("Successful webhook url with user " + user + " and tmp_url " + tmp_url) + + def webhook_delete_request(request): + # Simulates a webhook url call during user deletion + user = request.POST["user_name"] # Status is incremented to count the number of successful test webhooks settings["webhook_status"] += 1 - return Response("Successful webhook url with user " + user + " and temp_url " + temp_url) + return Response("Successful webhook url with user " + user) + + def webhook_fail_request(request): + # Simulates a webhook url call during user creation + user = request.POST["user_name"] + tmp_url = request.POST["tmp_url"] + + #TODO: call tmp_url + return Response() def get_status(request): # Returns the status number @@ -302,10 +316,17 @@ def reset_status(request): settings = config.registry.settings # Initialize status settings["webhook_status"] = 0 - config.add_route("webhook", "/webhook") + config.add_route("webhook_create", "/webhook_create") + config.add_route("webhook_delete", "/webhook_delete") + config.add_route("webhook_fail", "/webhook_fail") config.add_route("get_status", "/get_status") config.add_route("reset_status", "/reset_status") - config.add_view(webhook_request, route_name="webhook", request_method="POST", request_param="user_name") + config.add_view(webhook_create_request, route_name="webhook_create", + request_method="POST", request_param=("user_name", "tmp_url")) + config.add_view(webhook_delete_request, route_name="webhook_delete", + request_method="POST", request_param="user_name") + config.add_view(webhook_fail_request, route_name="webhook_fail", + request_method="POST", request_param=("user_name", "tmp_url")) config.add_view(get_status, route_name="get_status", request_method="GET") config.add_view(reset_status, route_name="reset_status", request_method="POST") webhook_app_instance = config.make_wsgi_app() From a50fbf5c074c43bec0f6675eb4f87c112b4dbe59 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Tue, 12 Jan 2021 14:53:44 -0500 Subject: [PATCH 11/33] move webhooks code --- magpie/api/management/user/user_utils.py | 3 +- magpie/api/management/user/user_views.py | 3 +- magpie/api/requests.py | 38 +--------------- magpie/api/webhooks.py | 55 ++++++++++++++++++++++++ magpie/app.py | 14 +----- 5 files changed, 61 insertions(+), 52 deletions(-) create mode 100644 magpie/api/webhooks.py diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index eefadd08c..1e28a60cf 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -25,6 +25,7 @@ from magpie.api.management.resource import resource_utils as ru from magpie.api.management.service.service_formats import format_service from magpie.api.management.user import user_formats as uf +from magpie.api.webhooks import webhook_request from magpie.constants import get_constant from magpie.permissions import PermissionSet, PermissionType, format_permissions from magpie.services import service_factory @@ -125,7 +126,7 @@ def _add_to_group(usr, grp): pool = multiprocessing.Pool(processes=len(webhooks)) args = [(webhook, {"user_name": user_name, "tmp_url": "tmp_url:80/todo"}, True) for webhook in webhooks] - pool.starmap_async(ar.webhook_request, args) + pool.starmap_async(webhook_request, args) return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, content={"user": user_content}) diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index a0a569a29..ebad1d825 100644 --- a/magpie/api/management/user/user_views.py +++ b/magpie/api/management/user/user_views.py @@ -18,6 +18,7 @@ from magpie.api.management.service.service_formats import format_service_resources from magpie.api.management.user import user_formats as uf from magpie.api.management.user import user_utils as uu +from magpie.api.webhooks import webhook_request from magpie.constants import MAGPIE_CONTEXT_PERMISSION, MAGPIE_LOGGED_PERMISSION, get_constant from magpie.permissions import PermissionType, format_permissions from magpie.services import SERVICE_TYPE_DICT @@ -145,7 +146,7 @@ def delete_user_view(request): # Execute all webhook requests pool = multiprocessing.Pool(processes=len(webhooks)) args = [(webhook, {"user_name": user.user_name}) for webhook in webhooks] - pool.starmap_async(ar.webhook_request, args) + pool.starmap_async(webhook_request, args) return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description) diff --git a/magpie/api/requests.py b/magpie/api/requests.py index 77cc4d9c8..e21a6e727 100644 --- a/magpie/api/requests.py +++ b/magpie/api/requests.py @@ -1,6 +1,5 @@ from typing import TYPE_CHECKING -import requests import six from pyramid.authentication import Authenticated, IAuthenticationPolicy from pyramid.httpexceptions import ( @@ -10,7 +9,6 @@ HTTPNotFound, HTTPUnprocessableEntity ) -import transaction from ziggurat_foundations.models.services.group import GroupService from ziggurat_foundations.models.services.resource import ResourceService from ziggurat_foundations.models.services.user import UserService @@ -18,8 +16,7 @@ from magpie import models from magpie.api import exception as ax from magpie.api import schemas as s -from magpie.constants import get_constant, MAGPIE_INI_FILE_PATH -from magpie.db import get_db_session_from_config_ini +from magpie.constants import get_constant from magpie.permissions import PermissionSet from magpie.utils import CONTENT_TYPE_JSON, get_logger @@ -347,36 +344,3 @@ def get_query_param(request, case_insensitive_key, default=None): if param.lower() == case_insensitive_key: return request.params.get(param) return default - - -def webhook_request(webhook_config, params, update_user_status_on_error=False): - # type: (Dict, Dict, bool) -> None - """ - Sends a webhook request using the input url. - """ - # These are the parameters permitted to use the template form in the webhook payload. - webhook_template_params = ["user_name", "tmp_url"] - - # Replace each instance of template parameters if a corresponding value was defined in input - for template_param in webhook_template_params: - if template_param in params: - for k,v in webhook_config["payload"].items(): - webhook_config["payload"][k] = v.replace("{" + template_param + "}", params[template_param]) - - try: - resp = requests.request(webhook_config["method"], webhook_config["url"], data=webhook_config["payload"]) - resp.raise_for_status() - except requests.exceptions.RequestException as e: - LOGGER.error(str(e)) - if "user_name" in params.keys() and update_user_status_on_error: - webhook_update_error_status(params["user_name"]) - - -def webhook_update_error_status(user_name): - """ - Updates the user's status to indicate an error occured with the webhook requests - """ - # find user and change its status to 0 to indicate a webhook error happened - db_session = get_db_session_from_config_ini(MAGPIE_INI_FILE_PATH) - db_session.query(models.User).filter(models.User.user_name == user_name).update({"status": 0}) - transaction.commit() diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py new file mode 100644 index 000000000..b84858e7b --- /dev/null +++ b/magpie/api/webhooks.py @@ -0,0 +1,55 @@ +import requests +import transaction + +from magpie.constants import MAGPIE_INI_FILE_PATH +from magpie.db import get_db_session_from_config_ini +from magpie import models +from magpie.utils import get_logger + +WEBHOOK_KEYS = { + "name", + "action", + "method", + "url", + "payload" +} +WEBHOOK_ACTIONS = [ + "create_user", + "delete_user" +] +HTTP_METHODS = ["GET", "OPTIONS", "HEAD", "POST", "PUT", "PATCH", "DELETE"] + +LOGGER = get_logger(__name__) + + +def webhook_request(webhook_config, params, update_user_status_on_error=False): + # type: (Dict, Dict, bool) -> None + """ + Sends a webhook request using the input url. + """ + # These are the parameters permitted to use the template form in the webhook payload. + webhook_template_params = ["user_name", "tmp_url"] + + # Replace each instance of template parameters if a corresponding value was defined in input + for template_param in webhook_template_params: + if template_param in params: + for k,v in webhook_config["payload"].items(): + webhook_config["payload"][k] = v.replace("{" + template_param + "}", params[template_param]) + + try: + resp = requests.request(webhook_config["method"], webhook_config["url"], data=webhook_config["payload"]) + resp.raise_for_status() + except requests.exceptions.RequestException as e: + LOGGER.error(str(e)) + if "user_name" in params.keys() and update_user_status_on_error: + webhook_update_error_status(params["user_name"]) + + +def webhook_update_error_status(user_name): + """ + Updates the user's status to indicate an error occured with the webhook requests + """ + # find user and change its status to 0 to indicate a webhook error happened + db_session = get_db_session_from_config_ini(MAGPIE_INI_FILE_PATH) + db_session.query(models.User).filter(models.User.user_name == user_name).update({"status": 0}) + transaction.commit() diff --git a/magpie/app.py b/magpie/app.py index e8caa8a6c..74bab41b9 100644 --- a/magpie/app.py +++ b/magpie/app.py @@ -10,6 +10,7 @@ from pyramid.settings import asbool from pyramid_beaker import set_cache_regions_from_settings +from magpie.api.webhooks import WEBHOOK_KEYS, WEBHOOK_ACTIONS, HTTP_METHODS from magpie.cli.register_defaults import register_defaults from magpie.constants import get_constant from magpie.db import get_db_session_from_config_ini, run_database_migration_when_ready, set_sqlalchemy_log_level @@ -20,19 +21,6 @@ LOGGER = get_logger(__name__) -WEBHOOK_KEYS = { - "name", - "action", - "method", - "url", - "payload" -} -WEBHOOK_ACTIONS = [ - "create_user", - "delete_user" -] -HTTP_METHODS = ["GET", "OPTIONS", "HEAD", "POST", "PUT", "PATCH", "DELETE"] - def main(global_config=None, **settings): # noqa: F811 """ From 0099a0d9b527f2c52a7c67318132874220f4c95f Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Tue, 12 Jan 2021 15:43:29 -0500 Subject: [PATCH 12/33] refactor webhook code --- magpie/api/management/user/user_utils.py | 17 +++------ magpie/api/management/user/user_views.py | 15 +++----- magpie/api/schemas.py | 4 +++ magpie/api/webhooks.py | 46 ++++++++++++++++++------ tests/test_webhooks.py | 20 ++++++----- 5 files changed, 59 insertions(+), 43 deletions(-) diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index 1e28a60cf..9ddf5e4ad 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -1,4 +1,3 @@ -import multiprocessing from typing import TYPE_CHECKING import six @@ -11,7 +10,6 @@ HTTPNotFound, HTTPOk ) -from pyramid.threadlocal import get_current_registry import transaction from ziggurat_foundations.models.services.group import GroupService from ziggurat_foundations.models.services.resource import ResourceService @@ -20,16 +18,15 @@ from magpie import models from magpie.api import exception as ax -from magpie.api import requests as ar from magpie.api import schemas as s from magpie.api.management.resource import resource_utils as ru from magpie.api.management.service.service_formats import format_service from magpie.api.management.user import user_formats as uf -from magpie.api.webhooks import webhook_request +from magpie.api.webhooks import process_webhook_requests, WEBHOOK_CREATE_USER_ACTION from magpie.constants import get_constant from magpie.permissions import PermissionSet, PermissionType, format_permissions from magpie.services import service_factory -from magpie.utils import get_logger, get_settings +from magpie.utils import get_logger LOGGER = get_logger(__name__) @@ -119,14 +116,8 @@ def _add_to_group(usr, grp): # Force commit before sending the webhook requests, so that the user's status is editable if a webhook error occurs transaction.commit() - # Check for webhook requests - webhooks = get_settings(get_current_registry())["webhooks"]["create_user"] - if len(webhooks) > 0: - # Execute all webhook requests - pool = multiprocessing.Pool(processes=len(webhooks)) - args = [(webhook, {"user_name": user_name, "tmp_url": "tmp_url:80/todo"}, True) - for webhook in webhooks] - pool.starmap_async(webhook_request, args) + # Process any webhook requests + process_webhook_requests(WEBHOOK_CREATE_USER_ACTION, {"user_name": user_name, "tmp_url": "tmp_url:80/todo"}, True) return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, content={"user": user_content}) diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index ebad1d825..2d4075353 100644 --- a/magpie/api/management/user/user_views.py +++ b/magpie/api/management/user/user_views.py @@ -1,11 +1,9 @@ """ User Views, both for specific user-name provided as request path variable and special keyword for logged session user. """ -import multiprocessing from pyramid.httpexceptions import HTTPBadRequest, HTTPConflict, HTTPCreated, HTTPForbidden, HTTPNotFound, HTTPOk from pyramid.settings import asbool -from pyramid.threadlocal import get_current_registry from pyramid.view import view_config from ziggurat_foundations.models.services.group import GroupService from ziggurat_foundations.models.services.resource import ResourceService @@ -18,11 +16,11 @@ from magpie.api.management.service.service_formats import format_service_resources from magpie.api.management.user import user_formats as uf from magpie.api.management.user import user_utils as uu -from magpie.api.webhooks import webhook_request +from magpie.api.webhooks import process_webhook_requests, WEBHOOK_DELETE_USER_ACTION from magpie.constants import MAGPIE_CONTEXT_PERMISSION, MAGPIE_LOGGED_PERMISSION, get_constant from magpie.permissions import PermissionType, format_permissions from magpie.services import SERVICE_TYPE_DICT -from magpie.utils import get_logger, get_settings +from magpie.utils import get_logger LOGGER = get_logger(__name__) @@ -140,13 +138,8 @@ def delete_user_view(request): ax.evaluate_call(lambda: request.db.delete(user), fallback=lambda: request.db.rollback(), http_error=HTTPForbidden, msg_on_fail=s.User_DELETE_ForbiddenResponseSchema.description) - # Check for webhook requests - webhooks = get_settings(get_current_registry())["webhooks"]["delete_user"] - if len(webhooks) > 0: - # Execute all webhook requests - pool = multiprocessing.Pool(processes=len(webhooks)) - args = [(webhook, {"user_name": user.user_name}) for webhook in webhooks] - pool.starmap_async(webhook_request, args) + # Process any webhook requests + process_webhook_requests(WEBHOOK_DELETE_USER_ACTION, {"user_name": user.user_name}) return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description) diff --git a/magpie/api/schemas.py b/magpie/api/schemas.py index 69a31544c..c9103a15e 100644 --- a/magpie/api/schemas.py +++ b/magpie/api/schemas.py @@ -98,6 +98,10 @@ def service_api_route_info(service_api, **kwargs): _LOGGED_USER_VALUE = get_constant("MAGPIE_LOGGED_USER") LoggedUserBase = "/users/{}".format(_LOGGED_USER_VALUE) +# Values for the 'status' field in the Users database +UserOKStatus = 1 +UserWebhookErrorStatus = 0 + SwaggerGenerator = Service( path="/json", name="swagger_schema_json") diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index b84858e7b..064ca8add 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -1,11 +1,16 @@ +import multiprocessing + +from pyramid.threadlocal import get_current_registry import requests import transaction +from magpie.api.schemas import UserWebhookErrorStatus from magpie.constants import MAGPIE_INI_FILE_PATH from magpie.db import get_db_session_from_config_ini from magpie import models -from magpie.utils import get_logger +from magpie.utils import get_logger, get_settings +# List of keys that should be found for a single webhook item in the config WEBHOOK_KEYS = { "name", "action", @@ -13,25 +18,46 @@ "url", "payload" } + +# List of possible actions associated with a webhook +WEBHOOK_CREATE_USER_ACTION = "create_user" +WEBHOOK_DELETE_USER_ACTION = "delete_user" WEBHOOK_ACTIONS = [ - "create_user", - "delete_user" + WEBHOOK_CREATE_USER_ACTION, + WEBHOOK_DELETE_USER_ACTION ] + +# These are the parameters permitted to use the template form in the webhook payload. +WEBHOOK_TEMPLATE_PARAMS = ["user_name", "tmp_url"] + HTTP_METHODS = ["GET", "OPTIONS", "HEAD", "POST", "PUT", "PATCH", "DELETE"] LOGGER = get_logger(__name__) -def webhook_request(webhook_config, params, update_user_status_on_error=False): - # type: (Dict, Dict, bool) -> None +def process_webhook_requests(action, payload, update_user_status_on_error=False): """ - Sends a webhook request using the input url. + Checks the config for any webhooks that correspond to the input action, and prepares corresponding requests + :param action: tag identifying which webhooks to use in the config + :param payload: dictionary containing the parameters used for the request + :param update_user_status_on_error: update the user status or not in case of a webhook error """ - # These are the parameters permitted to use the template form in the webhook payload. - webhook_template_params = ["user_name", "tmp_url"] + # Check for webhook requests + webhooks = get_settings(get_current_registry())["webhooks"][action] + if len(webhooks) > 0: + # Execute all webhook requests + pool = multiprocessing.Pool(processes=len(webhooks)) + args = [(webhook, payload, update_user_status_on_error) for webhook in webhooks] + pool.starmap_async(send_webhook_request, args) + +def send_webhook_request(webhook_config, params, update_user_status_on_error=False): + # type: (Dict, Dict, bool) -> None + """ + Sends a single webhook request using the input config. + """ # Replace each instance of template parameters if a corresponding value was defined in input - for template_param in webhook_template_params: + for template_param in WEBHOOK_TEMPLATE_PARAMS: if template_param in params: for k,v in webhook_config["payload"].items(): webhook_config["payload"][k] = v.replace("{" + template_param + "}", params[template_param]) @@ -51,5 +77,5 @@ def webhook_update_error_status(user_name): """ # find user and change its status to 0 to indicate a webhook error happened db_session = get_db_session_from_config_ini(MAGPIE_INI_FILE_PATH) - db_session.query(models.User).filter(models.User.user_name == user_name).update({"status": 0}) + db_session.query(models.User).filter(models.User.user_name == user_name).update({"status": UserWebhookErrorStatus}) transaction.commit() diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index bedfa8847..f3afdece4 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -14,6 +14,8 @@ import requests +from magpie.api.schemas import UserWebhookErrorStatus +from magpie.api.webhooks import WEBHOOK_CREATE_USER_ACTION, WEBHOOK_DELETE_USER_ACTION from magpie.constants import get_constant from magpie.utils import CONTENT_TYPE_JSON from tests import runner, utils @@ -78,14 +80,14 @@ def test_Webhook_CreateUser(self): "webhooks": [ { "name": "test_webhook", - "action": "create_user", + "action": WEBHOOK_CREATE_USER_ACTION, "method": "POST", "url": create_webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} }, { "name": "test_webhook_2", - "action": "create_user", + "action": WEBHOOK_CREATE_USER_ACTION, "method": "POST", "url": create_webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} @@ -154,7 +156,7 @@ def test_Webhook_CreateUser_FailingWebhook(self): "webhooks": [ { "name": "test_webhook", - "action": "create_user", + "action": WEBHOOK_CREATE_USER_ACTION, "method": "POST", "url": webhook_fail_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} @@ -186,7 +188,7 @@ def test_Webhook_CreateUser_FailingWebhook(self): resp = utils.test_request(self, "GET", path, headers=self.json_headers, cookies=self.cookies) body = utils.check_response_basic_info(resp, 200, expected_method="GET") info = utils.TestSetup.get_UserInfo(self, override_body=body) - utils.check_val_equal(info["status"], 0) + utils.check_val_equal(info["status"], UserWebhookErrorStatus) def test_Webhook_CreateUser_NonExistentWebhookUrl(self): """ @@ -198,7 +200,7 @@ def test_Webhook_CreateUser_NonExistentWebhookUrl(self): "webhooks": [ { "name": "test_webhook", - "action": "create_user", + "action": WEBHOOK_CREATE_USER_ACTION, "method": "POST", "url": webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} @@ -228,7 +230,7 @@ def test_Webhook_CreateUser_NonExistentWebhookUrl(self): resp = utils.test_request(self, "GET", path, headers=self.json_headers, cookies=self.cookies) body = utils.check_response_basic_info(resp, 200, expected_method="GET") info = utils.TestSetup.get_UserInfo(self, override_body=body) - utils.check_val_equal(info["status"], 0) + utils.check_val_equal(info["status"], UserWebhookErrorStatus) def test_Webhook_DeleteUser(self): """ @@ -240,14 +242,14 @@ def test_Webhook_DeleteUser(self): "webhooks": [ { "name": "test_webhook", - "action": "delete_user", + "action": WEBHOOK_DELETE_USER_ACTION, "method": "POST", "url": delete_webhook_url, "payload": {"user_name": "{user_name}"} }, { "name": "test_webhook_2", - "action": "delete_user", + "action": WEBHOOK_DELETE_USER_ACTION, "method": "POST", "url": delete_webhook_url, "payload": {"user_name": "{user_name}"} @@ -331,7 +333,7 @@ def test_Webhook_IncorrectConfig(self): "webhooks": [ { "name": "test_webhook_app", - "action": "create_user", + "action": WEBHOOK_CREATE_USER_ACTION, "method": "POST", "url": create_webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} From d5d0d1a1c1d4fbab971c2e57b7b6dd04fca389f0 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Tue, 12 Jan 2021 17:37:39 -0500 Subject: [PATCH 13/33] updated doc --- config/config.yml | 16 ++++++------- docs/configuration.rst | 53 ++++++++++++++++++++++++++++++++++-------- magpie/api/webhooks.py | 3 ++- tests/test_webhooks.py | 3 ++- 4 files changed, 55 insertions(+), 20 deletions(-) diff --git a/config/config.yml b/config/config.yml index 4d341a81d..9b1c56c0c 100644 --- a/config/config.yml +++ b/config/config.yml @@ -37,11 +37,11 @@ groups: # Definitions of all the webhooks urls that will be called when creating or deleting a users. webhooks: - create : - - - - - - ... - delete : - - - - - - ... + - name: + action: create_user | remove_user + method: GET | HEAD | POST | PUT | PATCH | DELETE + url: + payload: # add any parameters required with the webhook url here + : + ... + diff --git a/docs/configuration.rst b/docs/configuration.rst index 46adb8d4a..b3258f813 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -146,14 +146,13 @@ a combined configuration as follows. action: create webhooks: - create : - - - - - - ... - delete : - - - - - - ... + - name: + action: create_user | remove_user + method: GET | HEAD | POST | PUT | PATCH | DELETE + url: + payload: # add any parameters required with the webhook url here + : + ... For backward compatibility reasons, `Magpie` will first look for separate files to load each section individually. @@ -170,9 +169,43 @@ creation. Otherwise defaults are assumed and only the specified user or group na `providers.cfg`_ and `permissions.cfg`_ for further details about specific formatting and behaviour of each available field. + .. versionadded:: 3.6 + A section for webhooks has also been added to the combined configuration file. This section defines a list of -urls that should be called when either creating or deleting a user. The webhooks urls are responsible for any extra -steps that should be taken on external services during the user creation/deletion. +urls that should be called after creating or deleting a user. The webhooks urls are responsible for any extra +steps that should be taken on external services after the user creation/deletion. Note that the webhook requests are +asynchronous, so Magpie might execute other requests before the webhooks requests are fully done. + +Each webhook should define the following parameters : + +- | ``name`` + + The webhook's name + +- | ``action`` + | (Values: ``create_user | delete_user``) + + The action where the webhook will be executed, either during the user's creation (create_user), + or the user's deletion (delete_user) + +- | ``method`` + + The http method used for the webhook request + +- | ``url`` + + A valid http url that should be called for the webhook request + +- | ``payload`` + + A list of parameters that will be sent with the request. Note that those parameters can contain + template variables using the braces characters {}. + The only permitted template variables for now are ``user_name`` and ``tmp_url``. These variables will get replaced + by the corresponding value at the time of the request. + + For example, the parameter "user_name_param" could be defined in the config using a template variable ``user_name``: + + ``"user_name_param": "{user_name}"`` Settings and Constants ---------------------- diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index 064ca8add..a7edda08b 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -30,7 +30,7 @@ # These are the parameters permitted to use the template form in the webhook payload. WEBHOOK_TEMPLATE_PARAMS = ["user_name", "tmp_url"] -HTTP_METHODS = ["GET", "OPTIONS", "HEAD", "POST", "PUT", "PATCH", "DELETE"] +HTTP_METHODS = ["GET", "HEAD", "POST", "PUT", "PATCH", "DELETE"] LOGGER = get_logger(__name__) @@ -66,6 +66,7 @@ def send_webhook_request(webhook_config, params, update_user_status_on_error=Fal resp = requests.request(webhook_config["method"], webhook_config["url"], data=webhook_config["payload"]) resp.raise_for_status() except requests.exceptions.RequestException as e: + LOGGER.error(f"An exception has occured with the webhook : {webhook_config['name']}") LOGGER.error(str(e)) if "user_name" in params.keys() and update_user_status_on_error: webhook_update_error_status(params["user_name"]) diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index f3afdece4..c06ebb5d1 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -136,7 +136,8 @@ def test_Webhook_CreateUser_EmptyUrl(self): utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP")) - # Wait for the webhook requests to complete + # Wait for the potential webhook requests to complete + # In this case, there should be no webhook request to execute sleep(1) # Check if user creation was successful even if no webhook were defined in the config From b1692c36cd539c237013977cb755b2519e45442c Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Wed, 13 Jan 2021 09:17:50 -0500 Subject: [PATCH 14/33] fix webhook config and complete doc (#343) --- CHANGES.rst | 1 + magpie/app.py | 9 +++------ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index d1cd9ffc5..e4581312b 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -10,6 +10,7 @@ Changes Features / Changes ~~~~~~~~~~~~~~~~~~~~~ * Add a list of webhook urls, defined in the configuration, that will be called when creating or deleting a user. + (resolves `#343 `_). Bug Fixes ~~~~~~~~~~~~~~~~~~~~~ diff --git a/magpie/app.py b/magpie/app.py index 74bab41b9..b10ba980b 100644 --- a/magpie/app.py +++ b/magpie/app.py @@ -10,7 +10,7 @@ from pyramid.settings import asbool from pyramid_beaker import set_cache_regions_from_settings -from magpie.api.webhooks import WEBHOOK_KEYS, WEBHOOK_ACTIONS, HTTP_METHODS +from magpie.api.webhooks import HTTP_METHODS, WEBHOOK_ACTIONS, WEBHOOK_KEYS from magpie.cli.register_defaults import register_defaults from magpie.constants import get_constant from magpie.db import get_db_session_from_config_ini, run_database_migration_when_ready, set_sqlalchemy_log_level @@ -79,10 +79,9 @@ def main(global_config=None, **settings): # noqa: F811 magpie_register_permissions_from_config(perm_cfg, db_session=db_session) print_log("Register webhook configurations...", LOGGER) - settings["webhooks"] = [] + settings["webhooks"] = defaultdict(lambda: []) if combined_config: webhook_configs = get_all_configs(combined_config, "webhooks", allow_missing=True) - webhook_configs_by_action = defaultdict(lambda: []) for cfg in webhook_configs: for webhook in cfg: @@ -103,9 +102,7 @@ def main(global_config=None, **settings): # noqa: F811 # Regroup webhooks by action key webhook_sub_config = {k: webhook[k] for k in set(list(webhook.keys())) - {"action"}} - webhook_configs_by_action[webhook["action"]].append(webhook_sub_config) - - settings["webhooks"] = webhook_configs_by_action + settings["webhooks"][webhook["action"]].append(webhook_sub_config) print_log("Running configurations setup...", LOGGER) patch_magpie_url(settings) From b058990b0435f053dd80f52338205123337bd570 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Wed, 13 Jan 2021 10:19:11 -0500 Subject: [PATCH 15/33] fix lint --- magpie/api/webhooks.py | 27 ++++++++++++++++----------- tests/test_magpie_api.py | 5 ----- tests/test_webhooks.py | 11 +++++++---- tests/utils.py | 4 ++-- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index a7edda08b..6223a4cbf 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -35,11 +35,12 @@ LOGGER = get_logger(__name__) -def process_webhook_requests(action, payload, update_user_status_on_error=False): +def process_webhook_requests(action, params, update_user_status_on_error=False): """ - Checks the config for any webhooks that correspond to the input action, and prepares corresponding requests + Checks the config for any webhooks that correspond to the input action, and prepares corresponding requests. :param action: tag identifying which webhooks to use in the config - :param payload: dictionary containing the parameters used for the request + :param params: dictionary containing the required parameters for the request, they will replace templates + found in the payload :param update_user_status_on_error: update the user status or not in case of a webhook error """ # Check for webhook requests @@ -47,36 +48,40 @@ def process_webhook_requests(action, payload, update_user_status_on_error=False) if len(webhooks) > 0: # Execute all webhook requests pool = multiprocessing.Pool(processes=len(webhooks)) - args = [(webhook, payload, update_user_status_on_error) for webhook in webhooks] + args = [(webhook, params, update_user_status_on_error) for webhook in webhooks] pool.starmap_async(send_webhook_request, args) def send_webhook_request(webhook_config, params, update_user_status_on_error=False): - # type: (Dict, Dict, bool) -> None """ Sends a single webhook request using the input config. + :param webhook_config: dictionary containing the config data of a single webhook + :param params: dictionary containing the required parameters for the request, they will replace templates + found in the payload + :param update_user_status_on_error: update the user status or not in case of a webhook error """ # Replace each instance of template parameters if a corresponding value was defined in input for template_param in WEBHOOK_TEMPLATE_PARAMS: if template_param in params: - for k,v in webhook_config["payload"].items(): + for k, v in webhook_config["payload"].items(): webhook_config["payload"][k] = v.replace("{" + template_param + "}", params[template_param]) try: resp = requests.request(webhook_config["method"], webhook_config["url"], data=webhook_config["payload"]) resp.raise_for_status() - except requests.exceptions.RequestException as e: - LOGGER.error(f"An exception has occured with the webhook : {webhook_config['name']}") - LOGGER.error(str(e)) + except requests.exceptions.RequestException as exception: + LOGGER.error("An exception has occured with the webhook : %s", webhook_config["name"]) + LOGGER.error(str(exception)) if "user_name" in params.keys() and update_user_status_on_error: webhook_update_error_status(params["user_name"]) def webhook_update_error_status(user_name): """ - Updates the user's status to indicate an error occured with the webhook requests + Updates the user's status to indicate an error occured with the webhook requests. """ # find user and change its status to 0 to indicate a webhook error happened db_session = get_db_session_from_config_ini(MAGPIE_INI_FILE_PATH) - db_session.query(models.User).filter(models.User.user_name == user_name).update({"status": UserWebhookErrorStatus}) + user = db_session.query(models.User).filter(models.User.user_name == user_name) # pylint: disable=E1101,no-member + user.update({"status": UserWebhookErrorStatus}) transaction.commit() diff --git a/tests/test_magpie_api.py b/tests/test_magpie_api.py index b6aad9b5b..d46a7663d 100644 --- a/tests/test_magpie_api.py +++ b/tests/test_magpie_api.py @@ -7,14 +7,9 @@ Tests for :mod:`magpie.api` module. """ -import os -import tempfile -from time import sleep import unittest -import yaml import mock -import requests # NOTE: must be imported without 'from', otherwise the interface's test cases are also executed import tests.interfaces as ti diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index c06ebb5d1..2c2f54ae8 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -43,13 +43,16 @@ def setUpClass(cls): cls.version = utils.TestSetup.get_Version(cls) cls.test_group_name = "magpie-unittest-dummy-group" cls.test_user_name = "magpie-unittest-toto" + cls.headers = None + cls.cookies = None + cls.app = None cls.json_headers = {"Accept": CONTENT_TYPE_JSON, "Content-Type": CONTENT_TYPE_JSON} cls.extra_user_names = set() def tearDown(self): """ - Cleans up the test user after a test + Cleans up the test user after a test. """ utils.check_or_try_logout_user(self) self.headers, self.cookies = utils.check_or_try_login_user(self.app, self.usr, self.pwd, @@ -59,7 +62,7 @@ def tearDown(self): def setup_webhook_test(self, config_path): """ - Prepares a Magpie app using a specific testing config for webhooks + Prepares a Magpie app using a specific testing config for webhooks. :param config_path: path to the config file containing the webhook configs """ self.app = utils.get_test_magpie_app({"magpie.config_path": config_path}) @@ -193,7 +196,7 @@ def test_Webhook_CreateUser_FailingWebhook(self): def test_Webhook_CreateUser_NonExistentWebhookUrl(self): """ - Test creating a user where the webhook config has a non existent url + Test creating a user where the webhook config has a non existent url. """ # Write temporary config for testing webhooks webhook_url = BASE_WEBHOOK_URL + "/non_existent" @@ -347,4 +350,4 @@ def test_Webhook_IncorrectConfig(self): with tempfile.NamedTemporaryFile(mode="w") as webhook_tmp_config: yaml.safe_dump(data, webhook_tmp_config, default_flow_style=False) # create the magpie app with the test webhook config - self.assertRaises(ValueError, utils.get_test_magpie_app, {"magpie.config_path": webhook_tmp_config.name}) + self.assertRaises(ValueError, utils.get_test_magpie_app, {"magpie.config_path": webhook_tmp_config.name}) diff --git a/tests/utils.py b/tests/utils.py index 143ed94a8..c81bcfb0e 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -301,8 +301,8 @@ def webhook_fail_request(request): user = request.POST["user_name"] tmp_url = request.POST["tmp_url"] - #TODO: call tmp_url - return Response() + # TODO: call tmp_url + return Response("Failing webhook url with user " + user + " and tmp_url " + tmp_url) def get_status(request): # Returns the status number From dc69fc9453edde3bafc020385fc7cd0f876fbfc6 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Wed, 13 Jan 2021 10:28:14 -0500 Subject: [PATCH 16/33] fix docf lint --- magpie/api/webhooks.py | 2 ++ tests/test_webhooks.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index 6223a4cbf..0b35e3807 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -38,6 +38,7 @@ def process_webhook_requests(action, params, update_user_status_on_error=False): """ Checks the config for any webhooks that correspond to the input action, and prepares corresponding requests. + :param action: tag identifying which webhooks to use in the config :param params: dictionary containing the required parameters for the request, they will replace templates found in the payload @@ -55,6 +56,7 @@ def process_webhook_requests(action, params, update_user_status_on_error=False): def send_webhook_request(webhook_config, params, update_user_status_on_error=False): """ Sends a single webhook request using the input config. + :param webhook_config: dictionary containing the config data of a single webhook :param params: dictionary containing the required parameters for the request, they will replace templates found in the payload diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index 2c2f54ae8..b7f80df54 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -63,6 +63,7 @@ def tearDown(self): def setup_webhook_test(self, config_path): """ Prepares a Magpie app using a specific testing config for webhooks. + :param config_path: path to the config file containing the webhook configs """ self.app = utils.get_test_magpie_app({"magpie.config_path": config_path}) @@ -152,6 +153,7 @@ def test_Webhook_CreateUser_EmptyUrl(self): def test_Webhook_CreateUser_FailingWebhook(self): """ Test creating a user where the webhook receives an internal error. + This should trigger a callback to Magpie using the tmp_url. """ # Write temporary config for testing webhooks From 7fbc03902bac8dae937b31d11361e23423d2a656 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Thu, 14 Jan 2021 08:24:46 -0500 Subject: [PATCH 17/33] check if status is ok in webhook tests --- tests/test_webhooks.py | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index b7f80df54..c103c1456 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -14,7 +14,7 @@ import requests -from magpie.api.schemas import UserWebhookErrorStatus +from magpie.api.schemas import UserOKStatus, UserWebhookErrorStatus from magpie.api.webhooks import WEBHOOK_CREATE_USER_ACTION, WEBHOOK_DELETE_USER_ACTION from magpie.constants import get_constant from magpie.utils import CONTENT_TYPE_JSON @@ -74,6 +74,18 @@ def setup_webhook_test(self, config_path): utils.TestSetup.delete_TestUser(self) utils.TestSetup.delete_TestGroup(self) + def checkTestUserStatus(self, status): + """ + Checks if the test user has the expected status value. + + :param status: Status value that should be found for the test user + """ + path = "/users/{usr}".format(usr=self.test_user_name) + resp = utils.test_request(self, "GET", path, headers=self.json_headers, cookies=self.cookies) + body = utils.check_response_basic_info(resp, 200, expected_method="GET") + info = utils.TestSetup.get_UserInfo(self, override_body=body) + utils.check_val_equal(info["status"], status) + def test_Webhook_CreateUser(self): """ Test creating a user using webhooks. @@ -121,6 +133,7 @@ def test_Webhook_CreateUser(self): # Check if user creation was successful users = utils.TestSetup.get_RegisteredUsersList(self) utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + self.checkTestUserStatus(UserOKStatus) def test_Webhook_CreateUser_EmptyUrl(self): """ @@ -147,6 +160,7 @@ def test_Webhook_CreateUser_EmptyUrl(self): # Check if user creation was successful even if no webhook were defined in the config users = utils.TestSetup.get_RegisteredUsersList(self) utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + self.checkTestUserStatus(UserOKStatus) # Skip this test, until tmp_url is implemented @unittest.skip("implement tmp_url") @@ -190,11 +204,7 @@ def test_Webhook_CreateUser_FailingWebhook(self): utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") # Check if the user's status is set to 0 - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "GET", path, headers=self.json_headers, cookies=self.cookies) - body = utils.check_response_basic_info(resp, 200, expected_method="GET") - info = utils.TestSetup.get_UserInfo(self, override_body=body) - utils.check_val_equal(info["status"], UserWebhookErrorStatus) + self.checkTestUserStatus(UserWebhookErrorStatus) def test_Webhook_CreateUser_NonExistentWebhookUrl(self): """ @@ -232,11 +242,7 @@ def test_Webhook_CreateUser_NonExistentWebhookUrl(self): utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") # Check if the user's status is set to 0 - path = "/users/{usr}".format(usr=self.test_user_name) - resp = utils.test_request(self, "GET", path, headers=self.json_headers, cookies=self.cookies) - body = utils.check_response_basic_info(resp, 200, expected_method="GET") - info = utils.TestSetup.get_UserInfo(self, override_body=body) - utils.check_val_equal(info["status"], UserWebhookErrorStatus) + self.checkTestUserStatus(UserWebhookErrorStatus) def test_Webhook_DeleteUser(self): """ From 36c7897fb43e3eaba7a9e7507c465a3d5057709c Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Fri, 15 Jan 2021 12:40:07 -0500 Subject: [PATCH 18/33] add more flexible webhook payload and related test --- magpie/api/webhooks.py | 48 +++++++++++++++++++++++++++++++++++------- tests/test_webhooks.py | 18 ++++++++++++++-- tests/utils.py | 31 ++++++++++++++++----------- 3 files changed, 75 insertions(+), 22 deletions(-) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index 0b35e3807..14c74f90c 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -53,6 +53,34 @@ def process_webhook_requests(action, params, update_user_status_on_error=False): pool.starmap_async(send_webhook_request, args) +def replace_template(param_name, param_value, payload): + """ + Replace each instances of a template parameter by its corresponding value + :param param_name: name of a template parameter + :param param_value: value of a template parameter + :param payload: structure containing the data to be processed by the template replacement + :return: structure containing the data with the replaced template parameters + """ + if isinstance(payload, dict): + replace_dict = payload.copy() + for key, value in payload.items(): + # replace templates in the dictionary value + replace_dict[key] = replace_template(param_name, param_value, value) + + # replace templates in the dictionary key + new_key = replace_template(param_name, param_value, key) + if new_key != key: + replace_dict[new_key] = replace_dict[key] + del replace_dict[key] + return replace_dict + elif isinstance(payload, list): + return [replace_template(param_name, param_value, value) for value in payload] + elif isinstance(payload, str): + return payload.replace("{" + param_name + "}", param_value) + # For any other type, no replacing to do + return payload + + def send_webhook_request(webhook_config, params, update_user_status_on_error=False): """ Sends a single webhook request using the input config. @@ -63,16 +91,20 @@ def send_webhook_request(webhook_config, params, update_user_status_on_error=Fal :param update_user_status_on_error: update the user status or not in case of a webhook error """ # Replace each instance of template parameters if a corresponding value was defined in input - for template_param in WEBHOOK_TEMPLATE_PARAMS: - if template_param in params: - for k, v in webhook_config["payload"].items(): - webhook_config["payload"][k] = v.replace("{" + template_param + "}", params[template_param]) - try: - resp = requests.request(webhook_config["method"], webhook_config["url"], data=webhook_config["payload"]) + for template_param in WEBHOOK_TEMPLATE_PARAMS: + if template_param in params: + webhook_config["payload"] = replace_template(template_param, + params[template_param], + webhook_config["payload"]) + except Exception as exception: + print("An exception has occured while processing the template parameters in a webhook payload : " + + str(exception)) + try: + resp = requests.request(webhook_config["method"], webhook_config["url"], json=webhook_config["payload"]) resp.raise_for_status() - except requests.exceptions.RequestException as exception: - LOGGER.error("An exception has occured with the webhook : %s", webhook_config["name"]) + except Exception as exception: + LOGGER.error("An exception has occured with the webhook request : %s", webhook_config["name"]) LOGGER.error(str(exception)) if "user_name" in params.keys() and update_user_status_on_error: webhook_update_error_status(params["user_name"]) diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index c103c1456..e70addb78 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -17,7 +17,7 @@ from magpie.api.schemas import UserOKStatus, UserWebhookErrorStatus from magpie.api.webhooks import WEBHOOK_CREATE_USER_ACTION, WEBHOOK_DELETE_USER_ACTION from magpie.constants import get_constant -from magpie.utils import CONTENT_TYPE_JSON +from magpie.utils import CONTENT_TYPE_JSON, CONTENT_TYPE_HTML from tests import runner, utils BASE_WEBHOOK_URL = "http://localhost:8080" @@ -106,7 +106,14 @@ def test_Webhook_CreateUser(self): "action": WEBHOOK_CREATE_USER_ACTION, "method": "POST", "url": create_webhook_url, - "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} + # Test with a more complex payload, that includes different types and nested arrays / dicts + "payload": [ + {"user_name": ["{user_name}", "other_param"], + "nested_dict": { + "{user_name}": "{user_name} {user_name}", + }}, + "{user_name}", False, 1 + ] } ], "providers": "", @@ -126,6 +133,13 @@ def test_Webhook_CreateUser(self): # Wait for the webhook requests to complete sleep(1) + # Check if the webhook received the more complex payload, with the right template replacements + expected_payload = [{"nested_dict": {self.test_user_name: f"{self.test_user_name} {self.test_user_name}"}, + "user_name": [self.test_user_name, "other_param"]}, + self.test_user_name, False, 1] + resp = requests.post(BASE_WEBHOOK_URL + "/check_payload", json=expected_payload) + utils.check_response_basic_info(resp, 200, expected_method="POST", expected_type=CONTENT_TYPE_HTML) + # Check if both webhook requests have completed successfully resp = requests.get(BASE_WEBHOOK_URL + "/get_status") assert resp.text == "2" diff --git a/tests/utils.py b/tests/utils.py index c81bcfb0e..f1e248823 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -280,13 +280,11 @@ def get_test_webhook_app(webhook_url): Instantiate a local test application used for the prehook for user creation and deletion. """ def webhook_create_request(request): - # Simulates a webhook url call during user creation - user = request.POST["user_name"] - tmp_url = request.POST["tmp_url"] - # Status is incremented to count the number of successful test webhooks settings["webhook_status"] += 1 - return Response("Successful webhook url with user " + user + " and tmp_url " + tmp_url) + # Save the request's payload + settings["payload"].append(request.body) + return Response("Successful webhook url") def webhook_delete_request(request): # Simulates a webhook url call during user deletion @@ -308,27 +306,36 @@ def get_status(request): # Returns the status number return Response(str(settings["webhook_status"])) - def reset_status(request): + def check_payload(request): + # Check if the input payload is present in the webhook app saved payload + assert request.body in settings["payload"] + return Response("Content is correct") + + def reset(request): settings["webhook_status"] = 0 - return Response("Webhook status has been reset to 0.") + settings["payload"] = [] + return Response("Webhook app has been reset.") with Configurator() as config: settings = config.registry.settings # Initialize status settings["webhook_status"] = 0 + settings["payload"] = [] config.add_route("webhook_create", "/webhook_create") config.add_route("webhook_delete", "/webhook_delete") config.add_route("webhook_fail", "/webhook_fail") config.add_route("get_status", "/get_status") - config.add_route("reset_status", "/reset_status") + config.add_route("check_payload", "/check_payload") + config.add_route("reset", "/reset") config.add_view(webhook_create_request, route_name="webhook_create", - request_method="POST", request_param=("user_name", "tmp_url")) + request_method="POST") config.add_view(webhook_delete_request, route_name="webhook_delete", request_method="POST", request_param="user_name") config.add_view(webhook_fail_request, route_name="webhook_fail", request_method="POST", request_param=("user_name", "tmp_url")) config.add_view(get_status, route_name="get_status", request_method="GET") - config.add_view(reset_status, route_name="reset_status", request_method="POST") + config.add_view(check_payload, route_name="check_payload", request_method="POST") + config.add_view(reset, route_name="reset", request_method="POST") webhook_app_instance = config.make_wsgi_app() def webhook_app(): @@ -337,8 +344,8 @@ def webhook_app(): serve(webhook_app_instance, host=webhook_url_info.hostname, port=webhook_url_info.port) except OSError as exception: if exception.errno == EADDRINUSE: - # The app is already running, we just need to reset the webhook status for a new test. - resp = requests.post(webhook_url + "/reset_status") + # The app is already running, we just need to reset the webhook status and saved payload for a new test. + resp = requests.post(webhook_url + "/reset") check_response_basic_info(resp, 200, expected_type=CONTENT_TYPE_HTML, expected_method="POST") return raise From 7ff6b6e4e33fa47f9ed27ea0cff46181e262a15c Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Mon, 18 Jan 2021 11:14:58 -0500 Subject: [PATCH 19/33] fix webhook delete params reading --- magpie/api/webhooks.py | 7 ++++--- tests/utils.py | 11 ++++++----- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index 14c74f90c..ed3016175 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -55,7 +55,8 @@ def process_webhook_requests(action, params, update_user_status_on_error=False): def replace_template(param_name, param_value, payload): """ - Replace each instances of a template parameter by its corresponding value + Replace each instances of a template parameter by its corresponding value. + :param param_name: name of a template parameter :param param_value: value of a template parameter :param payload: structure containing the data to be processed by the template replacement @@ -73,9 +74,9 @@ def replace_template(param_name, param_value, payload): replace_dict[new_key] = replace_dict[key] del replace_dict[key] return replace_dict - elif isinstance(payload, list): + if isinstance(payload, list): return [replace_template(param_name, param_value, value) for value in payload] - elif isinstance(payload, str): + if isinstance(payload, str): return payload.replace("{" + param_name + "}", param_value) # For any other type, no replacing to do return payload diff --git a/tests/utils.py b/tests/utils.py index f1e248823..1c6595754 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -288,7 +288,7 @@ def webhook_create_request(request): def webhook_delete_request(request): # Simulates a webhook url call during user deletion - user = request.POST["user_name"] + user = json_pkg.loads(request.body)["user_name"] # Status is incremented to count the number of successful test webhooks settings["webhook_status"] += 1 @@ -296,8 +296,9 @@ def webhook_delete_request(request): def webhook_fail_request(request): # Simulates a webhook url call during user creation - user = request.POST["user_name"] - tmp_url = request.POST["tmp_url"] + body = json_pkg.loads(request.body) + user = body["user_name"] + tmp_url = body["tmp_url"] # TODO: call tmp_url return Response("Failing webhook url with user " + user + " and tmp_url " + tmp_url) @@ -330,9 +331,9 @@ def reset(request): config.add_view(webhook_create_request, route_name="webhook_create", request_method="POST") config.add_view(webhook_delete_request, route_name="webhook_delete", - request_method="POST", request_param="user_name") + request_method="POST") config.add_view(webhook_fail_request, route_name="webhook_fail", - request_method="POST", request_param=("user_name", "tmp_url")) + request_method="POST") config.add_view(get_status, route_name="get_status", request_method="GET") config.add_view(check_payload, route_name="check_payload", request_method="POST") config.add_view(reset, route_name="reset", request_method="POST") From 053539c00caf5e45b2609b58dd98222e1ed1bd36 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Mon, 18 Jan 2021 11:21:53 -0500 Subject: [PATCH 20/33] fix print --- magpie/api/webhooks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index ed3016175..3e5888d0d 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -99,8 +99,8 @@ def send_webhook_request(webhook_config, params, update_user_status_on_error=Fal params[template_param], webhook_config["payload"]) except Exception as exception: - print("An exception has occured while processing the template parameters in a webhook payload : " - + str(exception)) + LOGGER.error("An exception has occured while processing the template parameters in a webhook payload : " + + str(exception)) try: resp = requests.request(webhook_config["method"], webhook_config["url"], json=webhook_config["payload"]) resp.raise_for_status() From b3e03b5220a2cbb6864c919606cd9cd6a357e5f5 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Mon, 18 Jan 2021 11:35:28 -0500 Subject: [PATCH 21/33] fix lint --- magpie/api/webhooks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index 3e5888d0d..c6095e250 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -99,8 +99,8 @@ def send_webhook_request(webhook_config, params, update_user_status_on_error=Fal params[template_param], webhook_config["payload"]) except Exception as exception: - LOGGER.error("An exception has occured while processing the template parameters in a webhook payload : " - + str(exception)) + LOGGER.error("An exception has occured while processing the template parameters in a webhook payload : %s", + str(exception)) try: resp = requests.request(webhook_config["method"], webhook_config["url"], json=webhook_config["payload"]) resp.raise_for_status() From ce026673f429a99749dd86371b42f70e552ac0c1 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Mon, 18 Jan 2021 12:58:49 -0500 Subject: [PATCH 22/33] simplified dict template replacing --- magpie/api/webhooks.py | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index c6095e250..762917cb0 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -63,17 +63,8 @@ def replace_template(param_name, param_value, payload): :return: structure containing the data with the replaced template parameters """ if isinstance(payload, dict): - replace_dict = payload.copy() - for key, value in payload.items(): - # replace templates in the dictionary value - replace_dict[key] = replace_template(param_name, param_value, value) - - # replace templates in the dictionary key - new_key = replace_template(param_name, param_value, key) - if new_key != key: - replace_dict[new_key] = replace_dict[key] - del replace_dict[key] - return replace_dict + return {replace_template(param_name, param_value, key): replace_template(param_name, param_value, value) + for key, value in payload.items()} if isinstance(payload, list): return [replace_template(param_name, param_value, value) for value in payload] if isinstance(payload, str): From 5c8eb6951b39f2cda151b80fb340decccdde464a Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Fri, 22 Jan 2021 16:24:05 -0500 Subject: [PATCH 23/33] add url templating and reoganize replace_template function --- magpie/api/webhooks.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index 762917cb0..a6d313e3b 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -1,3 +1,4 @@ +import copy import multiprocessing from pyramid.threadlocal import get_current_registry @@ -53,22 +54,24 @@ def process_webhook_requests(action, params, update_user_status_on_error=False): pool.starmap_async(send_webhook_request, args) -def replace_template(param_name, param_value, payload): +def replace_template(params, payload): """ Replace each instances of a template parameter by its corresponding value. - :param param_name: name of a template parameter - :param param_value: value of a template parameter + :param params: the values of the template parameters :param payload: structure containing the data to be processed by the template replacement :return: structure containing the data with the replaced template parameters """ if isinstance(payload, dict): - return {replace_template(param_name, param_value, key): replace_template(param_name, param_value, value) + return {replace_template(params, key): replace_template(params, value) for key, value in payload.items()} if isinstance(payload, list): - return [replace_template(param_name, param_value, value) for value in payload] + return [replace_template(params, value) for value in payload] if isinstance(payload, str): - return payload.replace("{" + param_name + "}", param_value) + for template_param in WEBHOOK_TEMPLATE_PARAMS: + if template_param in params: + payload = payload.replace("{" + template_param + "}", params[template_param]) + return payload # For any other type, no replacing to do return payload @@ -83,20 +86,18 @@ def send_webhook_request(webhook_config, params, update_user_status_on_error=Fal :param update_user_status_on_error: update the user status or not in case of a webhook error """ # Replace each instance of template parameters if a corresponding value was defined in input + processed_config = copy.deepcopy(webhook_config) try: - for template_param in WEBHOOK_TEMPLATE_PARAMS: - if template_param in params: - webhook_config["payload"] = replace_template(template_param, - params[template_param], - webhook_config["payload"]) + processed_config["payload"] = replace_template(params, processed_config["payload"]) + processed_config["url"] = replace_template(params, processed_config["url"]) except Exception as exception: LOGGER.error("An exception has occured while processing the template parameters in a webhook payload : %s", str(exception)) try: - resp = requests.request(webhook_config["method"], webhook_config["url"], json=webhook_config["payload"]) + resp = requests.request(processed_config["method"], processed_config["url"], json=processed_config["payload"]) resp.raise_for_status() except Exception as exception: - LOGGER.error("An exception has occured with the webhook request : %s", webhook_config["name"]) + LOGGER.error("An exception has occured with the webhook request : %s", processed_config["name"]) LOGGER.error(str(exception)) if "user_name" in params.keys() and update_user_status_on_error: webhook_update_error_status(params["user_name"]) From c31b9ae89bf75426ced35355570c2a2d7b8b2e12 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Mon, 25 Jan 2021 09:05:00 -0500 Subject: [PATCH 24/33] optimize template webhook request --- magpie/api/webhooks.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index a6d313e3b..24bdf66a2 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -1,4 +1,3 @@ -import copy import multiprocessing from pyramid.threadlocal import get_current_registry @@ -56,7 +55,7 @@ def process_webhook_requests(action, params, update_user_status_on_error=False): def replace_template(params, payload): """ - Replace each instances of a template parameter by its corresponding value. + Replace each template parameter from the payload by its corresponding value. :param params: the values of the template parameters :param payload: structure containing the data to be processed by the template replacement @@ -85,19 +84,14 @@ def send_webhook_request(webhook_config, params, update_user_status_on_error=Fal found in the payload :param update_user_status_on_error: update the user status or not in case of a webhook error """ - # Replace each instance of template parameters if a corresponding value was defined in input - processed_config = copy.deepcopy(webhook_config) try: - processed_config["payload"] = replace_template(params, processed_config["payload"]) - processed_config["url"] = replace_template(params, processed_config["url"]) - except Exception as exception: - LOGGER.error("An exception has occured while processing the template parameters in a webhook payload : %s", - str(exception)) - try: - resp = requests.request(processed_config["method"], processed_config["url"], json=processed_config["payload"]) + # Replace template parameters if a corresponding value was defined in input and send the webhook request + resp = requests.request(webhook_config["method"], + replace_template(params, webhook_config["url"]), + json=replace_template(params, webhook_config["payload"])) resp.raise_for_status() except Exception as exception: - LOGGER.error("An exception has occured with the webhook request : %s", processed_config["name"]) + LOGGER.error("An exception has occured with the webhook request : %s", webhook_config["name"]) LOGGER.error(str(exception)) if "user_name" in params.keys() and update_user_status_on_error: webhook_update_error_status(params["user_name"]) From 2190641a1fa36867af7f8c56148053db72221ac1 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Tue, 26 Jan 2021 10:59:00 -0500 Subject: [PATCH 25/33] fix fstrings --- magpie/app.py | 16 ++++++++-------- tests/test_webhooks.py | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/magpie/app.py b/magpie/app.py index b10ba980b..06bf68334 100644 --- a/magpie/app.py +++ b/magpie/app.py @@ -87,18 +87,18 @@ def main(global_config=None, **settings): # noqa: F811 for webhook in cfg: # Validate the webhook config if webhook.keys() != WEBHOOK_KEYS: - raise ValueError(f"Missing or invalid key found in a webhook config " - f"from the config file {combined_config}") + raise ValueError("Missing or invalid key found in a webhook config " + + "from the config file {}".format(combined_config)) if webhook["action"] not in WEBHOOK_ACTIONS: - raise ValueError(f"Invalid action {webhook['action']} found in a webhook config " - f"from the config file {combined_config}") + raise ValueError("Invalid action {} found in a webhook config ".format(webhook["action"]) + + "from the config file {}".format(combined_config)) if webhook["method"] not in HTTP_METHODS: - raise ValueError(f"Invalid method {webhook['method']} found in a webhook config " - f"from the config file {combined_config}") + raise ValueError("Invalid method {} found in a webhook config ".format(webhook["method"]) + + "from the config file {}".format(combined_config)) url_parsed = urlparse(webhook["url"]) if not all([url_parsed.scheme, url_parsed.netloc, url_parsed.path]): - raise ValueError(f"Invalid url {webhook['url']} found in a webhook config " - f"from the config file {combined_config}") + raise ValueError("Invalid url {} found in a webhook config ".format(webhook["url"]) + + "from the config file {}".format(combined_config)) # Regroup webhooks by action key webhook_sub_config = {k: webhook[k] for k in set(list(webhook.keys())) - {"action"}} diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index e70addb78..5f45d74ab 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -134,7 +134,7 @@ def test_Webhook_CreateUser(self): sleep(1) # Check if the webhook received the more complex payload, with the right template replacements - expected_payload = [{"nested_dict": {self.test_user_name: f"{self.test_user_name} {self.test_user_name}"}, + expected_payload = [{"nested_dict": {self.test_user_name: self.test_user_name + " " + self.test_user_name}, "user_name": [self.test_user_name, "other_param"]}, self.test_user_name, False, 1] resp = requests.post(BASE_WEBHOOK_URL + "/check_payload", json=expected_payload) From 9445253116197b72f42215cff77057958ed0305c Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Wed, 27 Jan 2021 12:39:32 -0500 Subject: [PATCH 26/33] implement webhook_error case for the tmp_url --- magpie/api/management/register/register_utils.py | 10 ++++++++-- magpie/api/management/user/user_utils.py | 16 ++++++++++++++-- magpie/api/schemas.py | 5 +++++ magpie/models.py | 2 +- tests/test_webhooks.py | 12 ++++++++++-- tests/utils.py | 16 ++++++++++++---- 6 files changed, 50 insertions(+), 11 deletions(-) diff --git a/magpie/api/management/register/register_utils.py b/magpie/api/management/register/register_utils.py index c7baacd77..362955a59 100644 --- a/magpie/api/management/register/register_utils.py +++ b/magpie/api/management/register/register_utils.py @@ -7,6 +7,7 @@ from magpie.api import schemas as s from magpie.api.management.user import user_utils as uu from magpie import models +from magpie.api.webhooks import webhook_update_error_status from magpie.utils import CONTENT_TYPE_JSON, ExtendedEnum if TYPE_CHECKING: @@ -23,6 +24,7 @@ class TokenOperation(ExtendedEnum): """ GROUP_ACCEPT_TERMS = "group-accept-terms" USER_PASSWORD_RESET = "user-password-reset" # nosec: B105 + WEBHOOK_ERROR = "webhook-error" def handle_temporary_token(tmp_token, db_session): @@ -36,17 +38,21 @@ def handle_temporary_token(tmp_token, db_session): ax.raise_http(HTTPGone, content={"token": str_token}, detail=s.TemporaryURL_GET_GoneResponseSchema.description) ax.verify_param(tmp_token.operation, is_in=True, param_compare=TokenOperation.values(), param_name="token", http_error=HTTPInternalServerError, msg_on_fail="Invalid token.") - if tmp_token.operation == TokenOperation.GROUP_ACCEPT_TERMS: + if tmp_token.operation == TokenOperation.GROUP_ACCEPT_TERMS.value: ax.verify_param(tmp_token.group, not_none=True, http_error=HTTPInternalServerError, msg_on_fail="Invalid token.") ax.verify_param(tmp_token.user, not_none=True, http_error=HTTPInternalServerError, msg_on_fail="Invalid token.") uu.assign_user_group(tmp_token.user, tmp_token.group, db_session) - if tmp_token.operation == TokenOperation.USER_PASSWORD_RESET: + if tmp_token.operation == TokenOperation.USER_PASSWORD_RESET.value: ax.verify_param(tmp_token.user, not_none=True, http_error=HTTPInternalServerError, msg_on_fail="Invalid token.") # TODO: reset procedure ax.raise_http(HTTPNotImplemented, detail="Not Implemented") + if tmp_token.operation == TokenOperation.WEBHOOK_ERROR.value: + ax.verify_param(tmp_token.user, not_none=True, + http_error=HTTPInternalServerError, msg_on_fail="Invalid token.") + webhook_update_error_status(tmp_token.user.user_name) db_session.delete(tmp_token) diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index caa557e82..1e2d446f2 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -1,3 +1,4 @@ +import uuid from typing import TYPE_CHECKING import six @@ -19,6 +20,7 @@ from magpie import models from magpie.api import exception as ax from magpie.api import schemas as s +from magpie.api.management.register.register_utils import TokenOperation from magpie.api.management.resource import resource_utils as ru from magpie.api.management.service.service_formats import format_service from magpie.api.management.user import user_formats as uf @@ -119,11 +121,21 @@ def _add_to_group(usr, grp): user_content = uf.format_user(new_user, new_user_groups) + # Create a token for the tmp_url, in case an error happens in the webhook services + token_id = uuid.uuid4() + webhook_token = models.TemporaryToken( + token=token_id, + operation=TokenOperation.WEBHOOK_ERROR.value, + user_id=new_user.id, + group_id=_get_group(group_name).id) + ax.evaluate_call(lambda: db_session.add(webhook_token), fallback=lambda: db_session.rollback(), + http_error=HTTPForbidden, msg_on_fail=s.TemporaryToken_POST_ForbiddenResponseSchema.description) + tmp_url = webhook_token.url() + # Force commit before sending the webhook requests, so that the user's status is editable if a webhook error occurs transaction.commit() - # Process any webhook requests - process_webhook_requests(WEBHOOK_CREATE_USER_ACTION, {"user_name": user_name, "tmp_url": "tmp_url:80/todo"}, True) + process_webhook_requests(WEBHOOK_CREATE_USER_ACTION, {"user_name": user_name, "tmp_url": tmp_url}, True) return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, content={"user": user_content}) diff --git a/magpie/api/schemas.py b/magpie/api/schemas.py index 1a0a4d44b..08393de6e 100644 --- a/magpie/api/schemas.py +++ b/magpie/api/schemas.py @@ -2745,6 +2745,11 @@ class TemporaryURL_GET_GoneResponseSchema(BaseResponseSchemaAPI): body = BaseResponseBodySchema(code=HTTPGone.code, description=description) +class TemporaryToken_POST_ForbiddenResponseSchema(BaseResponseSchemaAPI): + description = "Failed to add token to db." + body = ErrorResponseBodySchema(code=HTTPForbidden.code, description=description) + + class Session_GET_ResponseBodySchema(BaseResponseBodySchema): user = UserBodySchema(missing=colander.drop) authenticated = colander.SchemaNode( diff --git a/magpie/models.py b/magpie/models.py index d33e01431..757c20ead 100644 --- a/magpie/models.py +++ b/magpie/models.py @@ -443,7 +443,7 @@ def url(self, settings=None): def expired(self): expire = get_constant("MAGPIE_TOKEN_EXPIRE", raise_missing=False, raise_not_set=False, default_value=86400) - return (datetime.datetime.utcnow() - self.created) <= datetime.timedelta(seconds=expire) + return (datetime.datetime.utcnow() - self.created) > datetime.timedelta(seconds=expire) @staticmethod def by_token(token, db_session=None): diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index 5f45d74ab..18075495f 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -10,6 +10,8 @@ import tempfile import unittest from time import sleep +from urllib.parse import urlparse + import yaml import requests @@ -176,8 +178,6 @@ def test_Webhook_CreateUser_EmptyUrl(self): utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") self.checkTestUserStatus(UserOKStatus) - # Skip this test, until tmp_url is implemented - @unittest.skip("implement tmp_url") def test_Webhook_CreateUser_FailingWebhook(self): """ Test creating a user where the webhook receives an internal error. @@ -217,6 +217,14 @@ def test_Webhook_CreateUser_FailingWebhook(self): users = utils.TestSetup.get_RegisteredUsersList(self) utils.check_val_is_in(self.test_user_name, users, msg="Test user should exist.") + # Check if the user's status is still set to 1, since the tmp_url has not been called yet + self.checkTestUserStatus(UserOKStatus) + + # Retrieve the tmp_url and send the request to the magpie app + resp = requests.get(BASE_WEBHOOK_URL + "/get_tmp_url") + assert resp.text + utils.test_request(self, "GET", urlparse(resp.text).path) + # Check if the user's status is set to 0 self.checkTestUserStatus(UserWebhookErrorStatus) diff --git a/tests/utils.py b/tests/utils.py index 0c2df63a3..b4455576c 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -301,15 +301,19 @@ def webhook_fail_request(request): # Simulates a webhook url call during user creation body = json_pkg.loads(request.body) user = body["user_name"] - tmp_url = body["tmp_url"] - - # TODO: call tmp_url - return Response("Failing webhook url with user " + user + " and tmp_url " + tmp_url) + # Since we can't call a local magpie app directly here, we save the tmp_url here, + # and retrieve it in the test case + settings["tmp_url"] = body["tmp_url"] + return Response("Failing webhook url with user " + user + " and tmp_url " + settings["tmp_url"]) def get_status(request): # Returns the status number return Response(str(settings["webhook_status"])) + def get_tmp_url(request): + # Returns the tmp_url + return Response(str(settings["tmp_url"])) + def check_payload(request): # Check if the input payload is present in the webhook app saved payload assert request.body in settings["payload"] @@ -318,6 +322,7 @@ def check_payload(request): def reset(request): settings["webhook_status"] = 0 settings["payload"] = [] + settings["tmp_url"] = "" return Response("Webhook app has been reset.") with Configurator() as config: @@ -325,10 +330,12 @@ def reset(request): # Initialize status settings["webhook_status"] = 0 settings["payload"] = [] + settings["tmp_url"] = "" config.add_route("webhook_create", "/webhook_create") config.add_route("webhook_delete", "/webhook_delete") config.add_route("webhook_fail", "/webhook_fail") config.add_route("get_status", "/get_status") + config.add_route("get_tmp_url", "/get_tmp_url") config.add_route("check_payload", "/check_payload") config.add_route("reset", "/reset") config.add_view(webhook_create_request, route_name="webhook_create", @@ -338,6 +345,7 @@ def reset(request): config.add_view(webhook_fail_request, route_name="webhook_fail", request_method="POST") config.add_view(get_status, route_name="get_status", request_method="GET") + config.add_view(get_tmp_url, route_name="get_tmp_url", request_method="GET") config.add_view(check_payload, route_name="check_payload", request_method="POST") config.add_view(reset, route_name="reset", request_method="POST") webhook_app_instance = config.make_wsgi_app() From 3d0a2b489716c5648020226be0d5361baa4c3b6b Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Fri, 29 Jan 2021 10:06:04 -0500 Subject: [PATCH 27/33] change webhook action enum --- magpie/api/management/user/user_utils.py | 5 +++-- magpie/api/management/user/user_views.py | 5 +++-- magpie/api/webhooks.py | 20 ++++++++++---------- magpie/app.py | 4 ++-- tests/test_webhooks.py | 16 ++++++++-------- 5 files changed, 26 insertions(+), 24 deletions(-) diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index 1e2d446f2..9a1870b1f 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -24,7 +24,7 @@ from magpie.api.management.resource import resource_utils as ru from magpie.api.management.service.service_formats import format_service from magpie.api.management.user import user_formats as uf -from magpie.api.webhooks import process_webhook_requests, WEBHOOK_CREATE_USER_ACTION +from magpie.api.webhooks import process_webhook_requests, WebhookAction from magpie.constants import get_constant from magpie.permissions import PermissionSet, PermissionType, format_permissions from magpie.services import service_factory @@ -135,7 +135,8 @@ def _add_to_group(usr, grp): # Force commit before sending the webhook requests, so that the user's status is editable if a webhook error occurs transaction.commit() - process_webhook_requests(WEBHOOK_CREATE_USER_ACTION, {"user_name": user_name, "tmp_url": tmp_url}, True) + process_webhook_requests(WebhookAction.WEBHOOK_CREATE_USER_ACTION, + {"user_name": user_name, "tmp_url": tmp_url}, True) return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, content={"user": user_content}) diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index 0dc3a6ea1..dbaddddfc 100644 --- a/magpie/api/management/user/user_views.py +++ b/magpie/api/management/user/user_views.py @@ -16,7 +16,7 @@ from magpie.api.management.service.service_formats import format_service_resources from magpie.api.management.user import user_formats as uf from magpie.api.management.user import user_utils as uu -from magpie.api.webhooks import process_webhook_requests, WEBHOOK_DELETE_USER_ACTION +from magpie.api.webhooks import process_webhook_requests, WebhookAction from magpie.constants import MAGPIE_CONTEXT_PERMISSION, MAGPIE_LOGGED_PERMISSION, get_constant from magpie.permissions import PermissionType, format_permissions from magpie.services import SERVICE_TYPE_DICT @@ -140,7 +140,8 @@ def delete_user_view(request): http_error=HTTPForbidden, msg_on_fail=s.User_DELETE_ForbiddenResponseSchema.description) # Process any webhook requests - process_webhook_requests(WEBHOOK_DELETE_USER_ACTION, {"user_name": user.user_name}) + process_webhook_requests(WebhookAction.WEBHOOK_DELETE_USER_ACTION, + {"user_name": user.user_name}) return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index 24bdf66a2..019e10562 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -8,7 +8,7 @@ from magpie.constants import MAGPIE_INI_FILE_PATH from magpie.db import get_db_session_from_config_ini from magpie import models -from magpie.utils import get_logger, get_settings +from magpie.utils import get_logger, get_settings, ExtendedEnum # List of keys that should be found for a single webhook item in the config WEBHOOK_KEYS = { @@ -19,14 +19,6 @@ "payload" } -# List of possible actions associated with a webhook -WEBHOOK_CREATE_USER_ACTION = "create_user" -WEBHOOK_DELETE_USER_ACTION = "delete_user" -WEBHOOK_ACTIONS = [ - WEBHOOK_CREATE_USER_ACTION, - WEBHOOK_DELETE_USER_ACTION -] - # These are the parameters permitted to use the template form in the webhook payload. WEBHOOK_TEMPLATE_PARAMS = ["user_name", "tmp_url"] @@ -35,6 +27,14 @@ LOGGER = get_logger(__name__) +class WebhookAction(ExtendedEnum): + """ + Actions supported by webhooks + """ + WEBHOOK_CREATE_USER_ACTION = "create_user" + WEBHOOK_DELETE_USER_ACTION = "delete_user" + + def process_webhook_requests(action, params, update_user_status_on_error=False): """ Checks the config for any webhooks that correspond to the input action, and prepares corresponding requests. @@ -45,7 +45,7 @@ def process_webhook_requests(action, params, update_user_status_on_error=False): :param update_user_status_on_error: update the user status or not in case of a webhook error """ # Check for webhook requests - webhooks = get_settings(get_current_registry())["webhooks"][action] + webhooks = get_settings(get_current_registry())["webhooks"][action.value] if len(webhooks) > 0: # Execute all webhook requests pool = multiprocessing.Pool(processes=len(webhooks)) diff --git a/magpie/app.py b/magpie/app.py index 06bf68334..1781044d2 100644 --- a/magpie/app.py +++ b/magpie/app.py @@ -10,7 +10,7 @@ from pyramid.settings import asbool from pyramid_beaker import set_cache_regions_from_settings -from magpie.api.webhooks import HTTP_METHODS, WEBHOOK_ACTIONS, WEBHOOK_KEYS +from magpie.api.webhooks import HTTP_METHODS, WebhookAction, WEBHOOK_KEYS from magpie.cli.register_defaults import register_defaults from magpie.constants import get_constant from magpie.db import get_db_session_from_config_ini, run_database_migration_when_ready, set_sqlalchemy_log_level @@ -89,7 +89,7 @@ def main(global_config=None, **settings): # noqa: F811 if webhook.keys() != WEBHOOK_KEYS: raise ValueError("Missing or invalid key found in a webhook config " + "from the config file {}".format(combined_config)) - if webhook["action"] not in WEBHOOK_ACTIONS: + if webhook["action"] not in WebhookAction.values(): raise ValueError("Invalid action {} found in a webhook config ".format(webhook["action"]) + "from the config file {}".format(combined_config)) if webhook["method"] not in HTTP_METHODS: diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index 18075495f..9982650c2 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -17,7 +17,7 @@ import requests from magpie.api.schemas import UserOKStatus, UserWebhookErrorStatus -from magpie.api.webhooks import WEBHOOK_CREATE_USER_ACTION, WEBHOOK_DELETE_USER_ACTION +from magpie.api.webhooks import WebhookAction from magpie.constants import get_constant from magpie.utils import CONTENT_TYPE_JSON, CONTENT_TYPE_HTML from tests import runner, utils @@ -98,14 +98,14 @@ def test_Webhook_CreateUser(self): "webhooks": [ { "name": "test_webhook", - "action": WEBHOOK_CREATE_USER_ACTION, + "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, "method": "POST", "url": create_webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} }, { "name": "test_webhook_2", - "action": WEBHOOK_CREATE_USER_ACTION, + "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, "method": "POST", "url": create_webhook_url, # Test with a more complex payload, that includes different types and nested arrays / dicts @@ -190,7 +190,7 @@ def test_Webhook_CreateUser_FailingWebhook(self): "webhooks": [ { "name": "test_webhook", - "action": WEBHOOK_CREATE_USER_ACTION, + "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, "method": "POST", "url": webhook_fail_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} @@ -238,7 +238,7 @@ def test_Webhook_CreateUser_NonExistentWebhookUrl(self): "webhooks": [ { "name": "test_webhook", - "action": WEBHOOK_CREATE_USER_ACTION, + "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, "method": "POST", "url": webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} @@ -276,14 +276,14 @@ def test_Webhook_DeleteUser(self): "webhooks": [ { "name": "test_webhook", - "action": WEBHOOK_DELETE_USER_ACTION, + "action": WebhookAction.WEBHOOK_DELETE_USER_ACTION.value, "method": "POST", "url": delete_webhook_url, "payload": {"user_name": "{user_name}"} }, { "name": "test_webhook_2", - "action": WEBHOOK_DELETE_USER_ACTION, + "action": WebhookAction.WEBHOOK_DELETE_USER_ACTION.value, "method": "POST", "url": delete_webhook_url, "payload": {"user_name": "{user_name}"} @@ -367,7 +367,7 @@ def test_Webhook_IncorrectConfig(self): "webhooks": [ { "name": "test_webhook_app", - "action": WEBHOOK_CREATE_USER_ACTION, + "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, "method": "POST", "url": create_webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} From dd448555a843348c3d01fc750444ab2fe4688bd1 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Fri, 29 Jan 2021 10:22:34 -0500 Subject: [PATCH 28/33] small fixes after pr update --- magpie/api/management/register/register_utils.py | 4 ++-- magpie/api/management/user/user_utils.py | 2 +- magpie/app.py | 2 +- tests/test_webhooks.py | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/magpie/api/management/register/register_utils.py b/magpie/api/management/register/register_utils.py index 362955a59..70354577d 100644 --- a/magpie/api/management/register/register_utils.py +++ b/magpie/api/management/register/register_utils.py @@ -24,7 +24,7 @@ class TokenOperation(ExtendedEnum): """ GROUP_ACCEPT_TERMS = "group-accept-terms" USER_PASSWORD_RESET = "user-password-reset" # nosec: B105 - WEBHOOK_ERROR = "webhook-error" + WEBHOOK_CREATE_USER_ERROR = "webhook-create-user-error" def handle_temporary_token(tmp_token, db_session): @@ -49,7 +49,7 @@ def handle_temporary_token(tmp_token, db_session): http_error=HTTPInternalServerError, msg_on_fail="Invalid token.") # TODO: reset procedure ax.raise_http(HTTPNotImplemented, detail="Not Implemented") - if tmp_token.operation == TokenOperation.WEBHOOK_ERROR.value: + if tmp_token.operation == TokenOperation.WEBHOOK_CREATE_USER_ERROR.value: ax.verify_param(tmp_token.user, not_none=True, http_error=HTTPInternalServerError, msg_on_fail="Invalid token.") webhook_update_error_status(tmp_token.user.user_name) diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index 9a1870b1f..98d4e753c 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -125,7 +125,7 @@ def _add_to_group(usr, grp): token_id = uuid.uuid4() webhook_token = models.TemporaryToken( token=token_id, - operation=TokenOperation.WEBHOOK_ERROR.value, + operation=TokenOperation.WEBHOOK_CREATE_USER_ERROR.value, user_id=new_user.id, group_id=_get_group(group_name).id) ax.evaluate_call(lambda: db_session.add(webhook_token), fallback=lambda: db_session.rollback(), diff --git a/magpie/app.py b/magpie/app.py index 1781044d2..6e7ebc792 100644 --- a/magpie/app.py +++ b/magpie/app.py @@ -5,7 +5,7 @@ Magpie is a service for AuthN and AuthZ based on Ziggurat-Foundations. """ from collections import defaultdict -from urllib.parse import urlparse +from six.moves.urllib.parse import urlparse from pyramid.settings import asbool from pyramid_beaker import set_cache_regions_from_settings diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index 9982650c2..456e8414d 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -10,7 +10,7 @@ import tempfile import unittest from time import sleep -from urllib.parse import urlparse +from six.moves.urllib.parse import urlparse import yaml @@ -222,7 +222,7 @@ def test_Webhook_CreateUser_FailingWebhook(self): # Retrieve the tmp_url and send the request to the magpie app resp = requests.get(BASE_WEBHOOK_URL + "/get_tmp_url") - assert resp.text + utils.check_response_basic_info(resp, 200, expected_method="GET", expected_type=CONTENT_TYPE_HTML) utils.test_request(self, "GET", urlparse(resp.text).path) # Check if the user's status is set to 0 From 2cd96ec4330521500f6ae32d4cec8f24297b861d Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Fri, 29 Jan 2021 10:55:34 -0500 Subject: [PATCH 29/33] fix lint --- magpie/api/webhooks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index 019e10562..5196d74ad 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -29,7 +29,7 @@ class WebhookAction(ExtendedEnum): """ - Actions supported by webhooks + Actions supported by webhooks. """ WEBHOOK_CREATE_USER_ACTION = "create_user" WEBHOOK_DELETE_USER_ACTION = "delete_user" From 1d801d0f7e60de950bbd2a7d147cb5c74c2d4a14 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Mon, 8 Feb 2021 10:11:10 -0500 Subject: [PATCH 30/33] fix webhook action names --- magpie/api/management/user/user_utils.py | 2 +- magpie/api/management/user/user_views.py | 2 +- magpie/api/webhooks.py | 4 ++-- tests/test_webhooks.py | 14 +++++++------- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/magpie/api/management/user/user_utils.py b/magpie/api/management/user/user_utils.py index 98d4e753c..a87ad1257 100644 --- a/magpie/api/management/user/user_utils.py +++ b/magpie/api/management/user/user_utils.py @@ -135,7 +135,7 @@ def _add_to_group(usr, grp): # Force commit before sending the webhook requests, so that the user's status is editable if a webhook error occurs transaction.commit() - process_webhook_requests(WebhookAction.WEBHOOK_CREATE_USER_ACTION, + process_webhook_requests(WebhookAction.CREATE_USER, {"user_name": user_name, "tmp_url": tmp_url}, True) return ax.valid_http(http_success=HTTPCreated, detail=s.Users_POST_CreatedResponseSchema.description, diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index dbaddddfc..267d742e6 100644 --- a/magpie/api/management/user/user_views.py +++ b/magpie/api/management/user/user_views.py @@ -140,7 +140,7 @@ def delete_user_view(request): http_error=HTTPForbidden, msg_on_fail=s.User_DELETE_ForbiddenResponseSchema.description) # Process any webhook requests - process_webhook_requests(WebhookAction.WEBHOOK_DELETE_USER_ACTION, + process_webhook_requests(WebhookAction.DELETE_USER, {"user_name": user.user_name}) return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description) diff --git a/magpie/api/webhooks.py b/magpie/api/webhooks.py index 5196d74ad..26253dcdb 100644 --- a/magpie/api/webhooks.py +++ b/magpie/api/webhooks.py @@ -31,8 +31,8 @@ class WebhookAction(ExtendedEnum): """ Actions supported by webhooks. """ - WEBHOOK_CREATE_USER_ACTION = "create_user" - WEBHOOK_DELETE_USER_ACTION = "delete_user" + CREATE_USER = "create_user" + DELETE_USER = "delete_user" def process_webhook_requests(action, params, update_user_status_on_error=False): diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index 456e8414d..13c62eab8 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -98,14 +98,14 @@ def test_Webhook_CreateUser(self): "webhooks": [ { "name": "test_webhook", - "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, + "action": WebhookAction.CREATE_USER.value, "method": "POST", "url": create_webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} }, { "name": "test_webhook_2", - "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, + "action": WebhookAction.CREATE_USER.value, "method": "POST", "url": create_webhook_url, # Test with a more complex payload, that includes different types and nested arrays / dicts @@ -190,7 +190,7 @@ def test_Webhook_CreateUser_FailingWebhook(self): "webhooks": [ { "name": "test_webhook", - "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, + "action": WebhookAction.CREATE_USER.value, "method": "POST", "url": webhook_fail_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} @@ -238,7 +238,7 @@ def test_Webhook_CreateUser_NonExistentWebhookUrl(self): "webhooks": [ { "name": "test_webhook", - "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, + "action": WebhookAction.CREATE_USER.value, "method": "POST", "url": webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} @@ -276,14 +276,14 @@ def test_Webhook_DeleteUser(self): "webhooks": [ { "name": "test_webhook", - "action": WebhookAction.WEBHOOK_DELETE_USER_ACTION.value, + "action": WebhookAction.DELETE_USER.value, "method": "POST", "url": delete_webhook_url, "payload": {"user_name": "{user_name}"} }, { "name": "test_webhook_2", - "action": WebhookAction.WEBHOOK_DELETE_USER_ACTION.value, + "action": WebhookAction.DELETE_USER.value, "method": "POST", "url": delete_webhook_url, "payload": {"user_name": "{user_name}"} @@ -367,7 +367,7 @@ def test_Webhook_IncorrectConfig(self): "webhooks": [ { "name": "test_webhook_app", - "action": WebhookAction.WEBHOOK_CREATE_USER_ACTION.value, + "action": WebhookAction.CREATE_USER.value, "method": "POST", "url": create_webhook_url, "payload": {"user_name": "{user_name}", "tmp_url": "{tmp_url}"} From f012170714a08b9945512939caa3fb1aa5000f44 Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Tue, 9 Feb 2021 13:07:40 -0500 Subject: [PATCH 31/33] fix test admin user --- tests/test_webhooks.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py index 13c62eab8..36a914242 100644 --- a/tests/test_webhooks.py +++ b/tests/test_webhooks.py @@ -40,8 +40,8 @@ class TestWebhooks(unittest.TestCase): @classmethod def setUpClass(cls): cls.grp = get_constant("MAGPIE_ADMIN_GROUP") - cls.usr = get_constant("MAGPIE_TEST_ADMIN_USERNAME") - cls.pwd = get_constant("MAGPIE_TEST_ADMIN_PASSWORD") + cls.usr = get_constant("MAGPIE_ADMIN_USER") + cls.pwd = get_constant("MAGPIE_ADMIN_PASSWORD") cls.version = utils.TestSetup.get_Version(cls) cls.test_group_name = "magpie-unittest-dummy-group" cls.test_user_name = "magpie-unittest-toto" @@ -71,7 +71,6 @@ def setup_webhook_test(self, config_path): self.app = utils.get_test_magpie_app({"magpie.config_path": config_path}) self.headers, self.cookies = utils.check_or_try_login_user(self.app, self.usr, self.pwd, use_ui_form_submit=True) - # Make sure the test user doesn't already exists from a previous test utils.TestSetup.delete_TestUser(self) utils.TestSetup.delete_TestGroup(self) From 20c4380d0e950aff0a18ad1d9131b8028071ca8a Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Tue, 9 Feb 2021 16:12:47 -0500 Subject: [PATCH 32/33] fix for py3.5 test --- tests/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/utils.py b/tests/utils.py index b4455576c..9a40febaf 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -291,7 +291,7 @@ def webhook_create_request(request): def webhook_delete_request(request): # Simulates a webhook url call during user deletion - user = json_pkg.loads(request.body)["user_name"] + user = json_pkg.loads(request.body.decode('utf-8'))["user_name"] # Status is incremented to count the number of successful test webhooks settings["webhook_status"] += 1 @@ -299,7 +299,7 @@ def webhook_delete_request(request): def webhook_fail_request(request): # Simulates a webhook url call during user creation - body = json_pkg.loads(request.body) + body = json_pkg.loads(request.body.decode('utf-8')) user = body["user_name"] # Since we can't call a local magpie app directly here, we save the tmp_url here, # and retrieve it in the test case From 89209fce7751aedddd367efef8260ae567b83ebc Mon Sep 17 00:00:00 2001 From: Charles-William Cummings Date: Tue, 9 Feb 2021 16:18:49 -0500 Subject: [PATCH 33/33] fix lint --- tests/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/utils.py b/tests/utils.py index 9a40febaf..db7673e2e 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -291,7 +291,7 @@ def webhook_create_request(request): def webhook_delete_request(request): # Simulates a webhook url call during user deletion - user = json_pkg.loads(request.body.decode('utf-8'))["user_name"] + user = json_pkg.loads(request.body.decode("utf-8"))["user_name"] # Status is incremented to count the number of successful test webhooks settings["webhook_status"] += 1 @@ -299,7 +299,7 @@ def webhook_delete_request(request): def webhook_fail_request(request): # Simulates a webhook url call during user creation - body = json_pkg.loads(request.body.decode('utf-8')) + body = json_pkg.loads(request.body.decode("utf-8")) user = body["user_name"] # Since we can't call a local magpie app directly here, we save the tmp_url here, # and retrieve it in the test case