From 1a83ffa2b4ce211477b2084e394ec41a546b7665 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 12:57:20 +0200 Subject: [PATCH 01/34] Allow user to specify "data_files" attribute when creating an action via the API. Files specified in this attribute are written to the disk inside the pack directory to which the action belongs to. --- st2api/st2api/controllers/v1/actions.py | 43 +++++++++++++++--- st2common/st2common/content/utils.py | 57 ++++++++++++++++++++++++ st2common/st2common/models/api/action.py | 41 +++++++++++++++-- 3 files changed, 132 insertions(+), 9 deletions(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index aeb986781d..f2945a58aa 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -13,10 +13,9 @@ # See the License for the specific language governing permissions and # limitations under the License. -from mongoengine import ValidationError - -from pecan import abort import six +from pecan import abort +from mongoengine import ValidationError # TODO: Encapsulate mongoengine errors in our persistence layer. Exceptions # that bubble up to this layer should be core Python exceptions or @@ -30,7 +29,9 @@ from st2common.models.api.base import jsexpose from st2common.persistence.action import Action from st2common.models.api.action import ActionAPI +from st2common.models.api.action import ActionCreateAPI from st2common.validators.api.misc import validate_not_part_of_system_pack +from st2common.content.utils import get_pack_resource_file_abs_path import st2common.validators.api.action as action_validator http_client = six.moves.http_client @@ -68,7 +69,7 @@ def _validate_action_parameters(action, runnertype_db): LOG.error(msg) abort(http_client.CONFLICT, msg) - @jsexpose(body_cls=ActionAPI, status_code=http_client.CREATED) + @jsexpose(body_cls=ActionCreateAPI, status_code=http_client.CREATED) def post(self, action): """ Create a new action. @@ -78,6 +79,7 @@ def post(self, action): """ if not hasattr(action, 'pack'): setattr(action, 'pack', DEFAULT_PACK_NAME) + # TODO: Dont allow manipulating system pack try: action_validator.validate_action(action) @@ -85,6 +87,11 @@ def post(self, action): abort(http_client.BAD_REQUEST, str(e)) return + # Write pack data files to disk (if any are provided) + data_files = action.data_files + if data_files: + self._handle_data_files(pack_name=action.pack, data_files=data_files) + # ActionsController._validate_action_parameters(action, runnertype_db) action_model = ActionAPI.to_model(action) @@ -92,7 +99,9 @@ def post(self, action): action_db = Action.add_or_update(action_model) LOG.debug('/actions/ POST saved ActionDB object=%s', action_db) - extra = {'action_db': action_db} + # TODO: Dispatch a trigger for each new written file + + extra = {'acion_db': action_db} LOG.audit('Action created. Action.id=%s' % (action_db.id), extra=extra) action_api = ActionAPI.from_model(action_db) @@ -163,3 +172,27 @@ def delete(self, action_ref_or_id): extra = {'action_db': action_db} LOG.audit('Action deleted. Action.id=%s' % (action_db.id), extra=extra) return None + + def _handle_data_files(self, pack_name, data_files): + """ + Method for handling action data files and writing them on disk. + """ + for data_file in data_files: + file_path = data_file['file_path'] + content = data_file['content'] + + file_path = get_pack_resource_file_abs_path(pack_name=pack_name, + resource_type='action', + file_path=file_path) + + LOG.debug('Writing data file "%s" to "%s"' % (str(data_file), file_path)) + self._write_data_file(file_path=file_path, content=content) + + return data_files + + def _write_data_file(self, file_path, content): + """ + Write data file on disk. + """ + with open(file_path, 'w') as fp: + fp.write(content) diff --git a/st2common/st2common/content/utils.py b/st2common/st2common/content/utils.py index 4c20bd3fa5..69f6251cf7 100644 --- a/st2common/st2common/content/utils.py +++ b/st2common/st2common/content/utils.py @@ -14,6 +14,7 @@ # limitations under the License. import os +import os.path from oslo_config import cfg @@ -26,6 +27,7 @@ 'get_packs_base_paths', 'get_pack_base_path', 'get_pack_directory', + 'get_pack_resource_file_abs_path', 'check_pack_directory_exists', 'check_pack_content_directory_exists' ] @@ -187,6 +189,61 @@ def get_entry_point_abs_path(pack=None, entry_point=None): return None +def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): + """ + Retrieve full absolute path to the pack resource file. + + Note: This function also takes care of sanitizing ``file_name`` argument + preventing directory traversal and similar attacks. + + :param pack_name: Pack name. + :type pack_name: ``str`` + + :param resource_type: Pack resource type (e.g. action, sensor, etc.). + :type resource_type: ``str`` + + :pack file_path: Resource file path relative to the pack directory (e.g. my_file.py or + directory/my_file.py) + :type file_path: ``str`` + + :rtype: ``str`` + """ + pack_directory = get_pack_directory(pack_name=pack_name) + + if not pack_directory: + raise ValueError('Directory for pack "%s" doesn\'t exist' % (pack_name)) + + path_components = [] + path_components.append(pack_directory) + + if resource_type == 'action': + path_components.append('actions/') + elif resource_type == 'sensor': + path_components.append('sensors/') + elif resource_type == 'rule': + path_components.append('rules/') + else: + raise ValueError('Invalid resource type: %s' % (resource_type)) + + # Normalize the path to prevent directory traversal + normalized_file_path = os.path.normpath('/' + file_path).lstrip('/') + + if normalized_file_path != file_path: + raise ValueError('Invalid file path: %s' % (file_path)) + + path_components.append(normalized_file_path) + result = os.path.join(*path_components) + + assert normalized_file_path in result + + # Final safety check for common prefix to avoid traversal attack + common_prefix = os.path.commonprefix([pack_directory, result]) + if common_prefix != pack_directory: + raise ValueError('Invalid file_path: %s' % (file_path)) + + return result + + def get_action_libs_abs_path(pack=None, entry_point=None): """ Return full absolute path of libs for an action. diff --git a/st2common/st2common/models/api/action.py b/st2common/st2common/models/api/action.py index d5bf837a37..fc92c06dc5 100644 --- a/st2common/st2common/models/api/action.py +++ b/st2common/st2common/models/api/action.py @@ -13,6 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import copy + from st2common.util import isotime from st2common.util import schema as util_schema from st2common import log as logging @@ -28,9 +30,12 @@ from st2common.models.system.common import ResourceReference -__all__ = ['ActionAPI', - 'LiveActionAPI', - 'RunnerTypeAPI'] +__all__ = [ + 'ActionAPI', + 'ActionCreateAPI', + 'LiveActionAPI', + 'RunnerTypeAPI' +] LOG = logging.getLogger(__name__) @@ -119,7 +124,9 @@ def to_model(cls, runner_type): class ActionAPI(BaseAPI): - """The system entity that represents a Stack Action/Automation in the system.""" + """ + The system entity that represents a Stack Action/Automation in the system. + """ model = ActionDB schema = { @@ -235,6 +242,32 @@ def to_model(cls, action): return model +class ActionCreateAPI(ActionAPI): + """ + API model for create action operations. + """ + schema = copy.deepcopy(ActionAPI.schema) + schema['properties']['data_files'] = { + 'description': 'Optional action script and data files which are written to the filesystem.', + 'type': 'array', + 'items': { + 'type': 'object', + 'properties': { + 'file_path': { + 'type': 'string', + 'required': True + }, + 'content': { + 'type': 'string', + 'required': True + }, + }, + 'additionalProperties': False + }, + 'default': {} + } + + class LiveActionAPI(BaseAPI): """The system entity that represents the execution of a Stack Action/Automation in the system. From 5208f22a590763bd3caa4dd259220af461c5255d Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 14:12:46 +0200 Subject: [PATCH 02/34] Add tests for get_pack_resource_file_abs_path function. --- st2common/tests/unit/test_content_utils.py | 44 +++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/st2common/tests/unit/test_content_utils.py b/st2common/tests/unit/test_content_utils.py index 586c37679f..246300e414 100644 --- a/st2common/tests/unit/test_content_utils.py +++ b/st2common/tests/unit/test_content_utils.py @@ -13,11 +13,17 @@ # See the License for the specific language governing permissions and # limitations under the License. +import os +import os.path + import unittest2 from oslo_config import cfg -from st2common.content.utils import get_packs_base_paths, get_aliases_base_paths +from st2common.content.utils import get_packs_base_paths +from st2common.content.utils import get_aliases_base_paths +from st2common.content.utils import get_pack_resource_file_abs_path from st2tests import config as tests_config +from st2tests.fixturesloader import get_fixtures_base_path class ContentUtilsTestCase(unittest2.TestCase): @@ -71,3 +77,39 @@ def test_get_aliases_base_paths(self): cfg.CONF.content.aliases_base_paths = '/opt/path1:/opt/path2:/opt/path1:/opt/path2' result = get_aliases_base_paths() self.assertEqual(result, ['/opt/path1', '/opt/path2']) + + def test_get_pack_resource_file_abs_path(self): + # Mock the packs path to point to the fixtures directory + cfg.CONF.content.packs_base_paths = get_fixtures_base_path() + + # Missing pack directory + expected_msg = 'Directory for pack "invalid-name" doesn\'t exist' + self.assertRaisesRegexp(ValueError, expected_msg, get_pack_resource_file_abs_path, + pack_name='invalid-name', + resource_type='action', + file_path='test.py') + + # Invalid resource type + expected_msg = 'Invalid resource type: fooo' + self.assertRaisesRegexp(ValueError, expected_msg, get_pack_resource_file_abs_path, + pack_name='dummy_pack_1', + resource_type='fooo', + file_path='test.py') + + # Invalid paths (directory traversal and absolute paths) + file_paths = ['/tmp/foo.py', '../foo.py', '/etc/passwd', '../../foo.py'] + for file_path in file_paths: + expected_msg = 'Invalid file path: %s' % (file_path) + self.assertRaisesRegexp(ValueError, expected_msg, get_pack_resource_file_abs_path, + pack_name='dummy_pack_1', + resource_type='action', + file_path=file_path) + + # Valid paths + file_paths = ['foo.py', 'a/foo.py', 'a/b/foo.py'] + for file_path in file_paths: + expected = os.path.join(get_fixtures_base_path(), 'dummy_pack_1/actions', file_path) + result = get_pack_resource_file_abs_path(pack_name='dummy_pack_1', + resource_type='action', + file_path=file_path) + self.assertEqual(result, expected) From d08e2af4a9f336c4a286ad64e61a7bcdf1d24f0c Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 14:13:34 +0200 Subject: [PATCH 03/34] Return written file paths so we can cleanup on exception. --- st2api/st2api/controllers/v1/actions.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index f2945a58aa..c776fedce0 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -177,6 +177,7 @@ def _handle_data_files(self, pack_name, data_files): """ Method for handling action data files and writing them on disk. """ + written_file_paths = [] for data_file in data_files: file_path = data_file['file_path'] content = data_file['content'] @@ -187,8 +188,9 @@ def _handle_data_files(self, pack_name, data_files): LOG.debug('Writing data file "%s" to "%s"' % (str(data_file), file_path)) self._write_data_file(file_path=file_path, content=content) + written_file_paths.append(file_path) - return data_files + return written_file_paths def _write_data_file(self, file_path, content): """ From 03ff7ccc54babe245f7efa163f7fa46fda6f928f Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 14:30:45 +0200 Subject: [PATCH 04/34] Dispatch an internal trigger for each written action data file. --- st2api/st2api/controllers/v1/actions.py | 29 +++++++++++++++++++++-- st2common/st2common/constants/triggers.py | 17 ++++++++++++- st2common/st2common/util/system_info.py | 16 +++++++++++-- 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index c776fedce0..4a90fc5fec 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -25,6 +25,7 @@ from st2api.controllers.v1.actionviews import ActionViewsController from st2common import log as logging from st2common.constants.pack import DEFAULT_PACK_NAME +from st2common.constants.triggers import ACTION_FILE_WRITTEN_TRIGGER from st2common.exceptions.apivalidation import ValueValidationException from st2common.models.api.base import jsexpose from st2common.persistence.action import Action @@ -32,6 +33,8 @@ from st2common.models.api.action import ActionCreateAPI from st2common.validators.api.misc import validate_not_part_of_system_pack from st2common.content.utils import get_pack_resource_file_abs_path +from st2common.transport.reactor import TriggerDispatcher +from st2common.util.system_info import get_host_info import st2common.validators.api.action as action_validator http_client = six.moves.http_client @@ -59,6 +62,10 @@ class ActionsController(resource.ContentPackResourceController): include_reference = True + def __init__(self, *args, **kwargs): + super(ActionsController, self).__init__(*args, **kwargs) + self._trigger_dispatcher = TriggerDispatcher(LOG) + @staticmethod def _validate_action_parameters(action, runnertype_db): # check if action parameters conflict with those from the supplied runner_type. @@ -89,8 +96,10 @@ def post(self, action): # Write pack data files to disk (if any are provided) data_files = action.data_files + written_data_files = [] if data_files: - self._handle_data_files(pack_name=action.pack, data_files=data_files) + written_data_files = self._handle_data_files(pack_name=action.pack, + data_files=data_files) # ActionsController._validate_action_parameters(action, runnertype_db) action_model = ActionAPI.to_model(action) @@ -99,7 +108,11 @@ def post(self, action): action_db = Action.add_or_update(action_model) LOG.debug('/actions/ POST saved ActionDB object=%s', action_db) - # TODO: Dispatch a trigger for each new written file + # Dispatch an internal trigger for each written data file. This way user + # automate comitting this files to git using StackStorm rule + if written_data_files: + self._dispatch_trigger_for_written_data_files(action_db=action_db, + written_data_files=written_data_files) extra = {'acion_db': action_db} LOG.audit('Action created. Action.id=%s' % (action_db.id), extra=extra) @@ -198,3 +211,15 @@ def _write_data_file(self, file_path, content): """ with open(file_path, 'w') as fp: fp.write(content) + + def _dispatch_trigger_for_written_data_files(self, action_db, written_data_files): + trigger = ACTION_FILE_WRITTEN_TRIGGER['name'] + host_info = get_host_info() + + for file_path in written_data_files: + payload = { + 'ref': action_db.ref, + 'file_path': file_path, + 'host_info': host_info + } + self._trigger_dispatcher.dispatch(trigger=trigger, payload=payload) diff --git a/st2common/st2common/constants/triggers.py b/st2common/st2common/constants/triggers.py index 024d160066..a1586e2e82 100644 --- a/st2common/st2common/constants/triggers.py +++ b/st2common/st2common/constants/triggers.py @@ -49,6 +49,20 @@ } } } +ACTION_FILE_WRITTEN_TRIGGER = { + 'name': 'st2.action.file_writen', + 'pack': SYSTEM_PACK_NAME, + 'description': 'Trigger encapsulating action file being written on disk.', + 'payload_schema': { + 'type': 'object', + 'properties': { + 'ref': {}, + 'file_path': {}, + 'content': {}, + 'host_info': {} + } + } +} NOTIFY_TRIGGER = { 'name': 'st2.generic.notifytrigger', @@ -123,7 +137,8 @@ INTERNAL_TRIGGER_TYPES = { 'action': [ ACTION_SENSOR_TRIGGER, - NOTIFY_TRIGGER + NOTIFY_TRIGGER, + ACTION_FILE_WRITTEN_TRIGGER ], 'sensor': [ { diff --git a/st2common/st2common/util/system_info.py b/st2common/st2common/util/system_info.py index 7a1284795f..9d20295f44 100644 --- a/st2common/st2common/util/system_info.py +++ b/st2common/st2common/util/system_info.py @@ -16,10 +16,22 @@ import os import socket +__all__ = [ + 'get_host_info', + 'get_process_info' +] + + +def get_host_info(): + host_info = { + 'hostname': socket.gethostname() + } + return host_info + def get_process_info(): - runner_info = { + process_info = { 'hostname': socket.gethostname(), 'pid': os.getpid() } - return runner_info + return process_info From 961352df390b2a71f66f9eda66b18a32be314dc6 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 28 Jul 2015 00:01:25 +0800 Subject: [PATCH 05/34] Remove unused method - we allow users to override runner parameters and make them immutable / set a default value inside action parameters. Conflicts: st2api/st2api/controllers/v1/actions.py --- st2api/st2api/controllers/v1/actions.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index 4a90fc5fec..dd8f8cb9d7 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -66,16 +66,6 @@ def __init__(self, *args, **kwargs): super(ActionsController, self).__init__(*args, **kwargs) self._trigger_dispatcher = TriggerDispatcher(LOG) - @staticmethod - def _validate_action_parameters(action, runnertype_db): - # check if action parameters conflict with those from the supplied runner_type. - conflicts = [p for p in action.parameters.keys() if p in runnertype_db.runner_parameters] - if len(conflicts) > 0: - msg = 'Parameters %s conflict with those inherited from runner_type : %s' % \ - (str(conflicts), action.runner_type) - LOG.error(msg) - abort(http_client.CONFLICT, msg) - @jsexpose(body_cls=ActionCreateAPI, status_code=http_client.CREATED) def post(self, action): """ @@ -101,7 +91,6 @@ def post(self, action): written_data_files = self._handle_data_files(pack_name=action.pack, data_files=data_files) - # ActionsController._validate_action_parameters(action, runnertype_db) action_model = ActionAPI.to_model(action) LOG.debug('/actions/ POST verified ActionAPI object=%s', action) From 86fba6dce63dd634d929b8a80975096ca871f576 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 14:36:02 +0200 Subject: [PATCH 06/34] When writing pack data file create requested pack sub-directory if it doesn't exist. --- st2api/st2api/controllers/v1/actions.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index dd8f8cb9d7..03dcedfb3b 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -13,6 +13,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +import os +import os.path + import six from pecan import abort from mongoengine import ValidationError @@ -198,6 +201,12 @@ def _write_data_file(self, file_path, content): """ Write data file on disk. """ + # Create directory if it doesn't exist + directory = os.path.dirname(file_path) + + if not os.path.isdir(directory): + os.makedirs(directory) + with open(file_path, 'w') as fp: fp.write(content) From 96867c6777d123556e3c90c53446eff0a6f73384 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 14:36:35 +0200 Subject: [PATCH 07/34] Update __all__. --- st2common/st2common/constants/triggers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/st2common/st2common/constants/triggers.py b/st2common/st2common/constants/triggers.py index a1586e2e82..ae3a27e8a1 100644 --- a/st2common/st2common/constants/triggers.py +++ b/st2common/st2common/constants/triggers.py @@ -26,6 +26,7 @@ 'ACTION_SENSOR_TRIGGER', 'NOTIFY_TRIGGER', + 'ACTION_FILE_WRITTEN_TRIGGER', 'TIMER_TRIGGER_TYPES', 'INTERNAL_TRIGGER_TYPES', From 82ed2e4d147694332427af6215df0f4e74ac3857 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 14:50:16 +0200 Subject: [PATCH 08/34] Don't throw if data_files is not provided. --- st2api/st2api/controllers/v1/actions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index 03dcedfb3b..97ede85378 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -88,7 +88,7 @@ def post(self, action): return # Write pack data files to disk (if any are provided) - data_files = action.data_files + data_files = getattr(action, 'data_files', []) written_data_files = [] if data_files: written_data_files = self._handle_data_files(pack_name=action.pack, From eb1a4545a47cb0a1d2f051813b2039de7a46e6a0 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 14:50:26 +0200 Subject: [PATCH 09/34] Fix potential security issue in get_entry_point_abs_path and EntryPointController which uses that function. Make sure the entry point file is located inside the pack directory and use get_pack_resource_file_abs_path to prevent directory traversal attacks. --- st2common/st2common/content/utils.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/st2common/st2common/content/utils.py b/st2common/st2common/content/utils.py index 69f6251cf7..83fadc6c1b 100644 --- a/st2common/st2common/content/utils.py +++ b/st2common/st2common/content/utils.py @@ -178,15 +178,23 @@ def get_entry_point_abs_path(pack=None, entry_point=None): :rtype: ``str`` """ - if entry_point is not None and len(entry_point) > 0: - if os.path.isabs(entry_point): - return entry_point + if not entry_point: + return None + if os.path.isabs(entry_point): pack_base_path = get_pack_base_path(pack_name=pack) - entry_point_abs_path = os.path.join(pack_base_path, 'actions', quote_unix(entry_point)) - return entry_point_abs_path - else: - return None + common_prefix = os.path.commonprefix([pack_base_path, entry_point]) + + if common_prefix != packs_base_paths: + raise ValueError('Entry point file "%s" is located outsite of the pack directory' % + (entry_point)) + + return entry_point + + entry_point_abs_path = get_pack_resource_file_abs_path(pack_name=pack, + resource_type='action', + file_path=entry_point) + return entry_point_abs_path def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): From cb2b8183761204aba616f750d7bfde80c5a5eb9a Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 14:54:51 +0200 Subject: [PATCH 10/34] Don't throw if directory doesn't exist, this check should be performed outside this function. Also fix a typo and update affected tests. --- st2common/st2common/content/utils.py | 13 +++++-------- st2common/tests/unit/test_content_utils.py | 7 ------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/st2common/st2common/content/utils.py b/st2common/st2common/content/utils.py index 83fadc6c1b..74117b4a0d 100644 --- a/st2common/st2common/content/utils.py +++ b/st2common/st2common/content/utils.py @@ -185,7 +185,7 @@ def get_entry_point_abs_path(pack=None, entry_point=None): pack_base_path = get_pack_base_path(pack_name=pack) common_prefix = os.path.commonprefix([pack_base_path, entry_point]) - if common_prefix != packs_base_paths: + if common_prefix != pack_base_path: raise ValueError('Entry point file "%s" is located outsite of the pack directory' % (entry_point)) @@ -216,13 +216,10 @@ def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): :rtype: ``str`` """ - pack_directory = get_pack_directory(pack_name=pack_name) - - if not pack_directory: - raise ValueError('Directory for pack "%s" doesn\'t exist' % (pack_name)) + pack_base_path = get_pack_base_path(pack_name=pack_name) path_components = [] - path_components.append(pack_directory) + path_components.append(pack_base_path) if resource_type == 'action': path_components.append('actions/') @@ -245,8 +242,8 @@ def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): assert normalized_file_path in result # Final safety check for common prefix to avoid traversal attack - common_prefix = os.path.commonprefix([pack_directory, result]) - if common_prefix != pack_directory: + common_prefix = os.path.commonprefix([pack_base_path, result]) + if common_prefix != pack_base_path: raise ValueError('Invalid file_path: %s' % (file_path)) return result diff --git a/st2common/tests/unit/test_content_utils.py b/st2common/tests/unit/test_content_utils.py index 246300e414..4314450444 100644 --- a/st2common/tests/unit/test_content_utils.py +++ b/st2common/tests/unit/test_content_utils.py @@ -82,13 +82,6 @@ def test_get_pack_resource_file_abs_path(self): # Mock the packs path to point to the fixtures directory cfg.CONF.content.packs_base_paths = get_fixtures_base_path() - # Missing pack directory - expected_msg = 'Directory for pack "invalid-name" doesn\'t exist' - self.assertRaisesRegexp(ValueError, expected_msg, get_pack_resource_file_abs_path, - pack_name='invalid-name', - resource_type='action', - file_path='test.py') - # Invalid resource type expected_msg = 'Invalid resource type: fooo' self.assertRaisesRegexp(ValueError, expected_msg, get_pack_resource_file_abs_path, From 64049f063184e0068b9ad863c649a3920e8ebe65 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 15:02:31 +0200 Subject: [PATCH 11/34] Throw inside the controller if the pack directory doesn't exist. --- st2api/st2api/controllers/v1/actions.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index 97ede85378..6eee506ee4 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -35,6 +35,7 @@ from st2common.models.api.action import ActionAPI from st2common.models.api.action import ActionCreateAPI from st2common.validators.api.misc import validate_not_part_of_system_pack +from st2common.content.utils import get_pack_base_path from st2common.content.utils import get_pack_resource_file_abs_path from st2common.transport.reactor import TriggerDispatcher from st2common.util.system_info import get_host_info @@ -192,16 +193,21 @@ def _handle_data_files(self, pack_name, data_files): file_path=file_path) LOG.debug('Writing data file "%s" to "%s"' % (str(data_file), file_path)) - self._write_data_file(file_path=file_path, content=content) + self._write_data_file(pack_name=pack_name, file_path=file_path, content=content) written_file_paths.append(file_path) return written_file_paths - def _write_data_file(self, file_path, content): + def _write_data_file(self, pack_name, file_path, content): """ Write data file on disk. """ - # Create directory if it doesn't exist + # Throw if pack directory doesn't exist + pack_base_path = get_pack_base_path(pack_name=pack_name) + if not os.path.isdir(pack_base_path): + raise ValueError('Directory for pack "%s" doesn\'t exist' % (pack_name)) + + # Create pack sub-directory tree if it doesn't exist directory = os.path.dirname(file_path) if not os.path.isdir(directory): From 135daf2ce4205832b2ebbabb0e8cc83fc1340f77 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 15:13:33 +0200 Subject: [PATCH 12/34] Update affected tests, fix typo. --- st2actions/tests/unit/test_runner_container_service.py | 10 ++++++---- st2common/st2common/content/utils.py | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/st2actions/tests/unit/test_runner_container_service.py b/st2actions/tests/unit/test_runner_container_service.py index 0f97f09468..c1ab1807c5 100644 --- a/st2actions/tests/unit/test_runner_container_service.py +++ b/st2actions/tests/unit/test_runner_container_service.py @@ -50,8 +50,9 @@ def test_get_entry_point_absolute_path(self): service = RunnerContainerService() orig_path = cfg.CONF.content.system_packs_base_path cfg.CONF.content.system_packs_base_path = '/tests/packs' - acutal_path = service.get_entry_point_abs_path(pack='foo', entry_point='/foo/bar.py') - self.assertEqual(acutal_path, '/foo/bar.py', 'Entry point path doesn\'t match.') + acutal_path = service.get_entry_point_abs_path(pack='foo', + entry_point='/tests/packs/foo/bar.py') + self.assertEqual(acutal_path, '/tests/packs/foo/bar.py', 'Entry point path doesn\'t match.') cfg.CONF.content.system_packs_base_path = orig_path def test_get_entry_point_absolute_path_empty(self): @@ -86,7 +87,8 @@ def test_get_action_libs_abs_path(self): self.assertEqual(acutal_path, expected_path, 'Action libs path doesn\'t match.') # entry point absolute. - acutal_path = service.get_action_libs_abs_path(pack='foo', entry_point='/tmp/foo.py') - expected_path = os.path.join('/tmp', ACTION_LIBS_DIR) + acutal_path = service.get_action_libs_abs_path(pack='foo', + entry_point='/tests/packs/foo/tmp/foo.py') + expected_path = os.path.join('/tests/packs/foo/tmp', ACTION_LIBS_DIR) self.assertEqual(acutal_path, expected_path, 'Action libs path doesn\'t match.') cfg.CONF.content.system_packs_base_path = orig_path diff --git a/st2common/st2common/content/utils.py b/st2common/st2common/content/utils.py index 74117b4a0d..f1da4f72e3 100644 --- a/st2common/st2common/content/utils.py +++ b/st2common/st2common/content/utils.py @@ -186,7 +186,7 @@ def get_entry_point_abs_path(pack=None, entry_point=None): common_prefix = os.path.commonprefix([pack_base_path, entry_point]) if common_prefix != pack_base_path: - raise ValueError('Entry point file "%s" is located outsite of the pack directory' % + raise ValueError('Entry point file "%s" is located outside of the pack directory' % (entry_point)) return entry_point From 94f36ccf6a8f16969e05078af195a9afde049762 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 20:12:00 +0200 Subject: [PATCH 13/34] Add new packs views controller which allows users to retrieve contents of a particular file from the provided pack. --- st2api/st2api/controllers/resource.py | 2 +- st2api/st2api/controllers/v1/packs.py | 4 ++ st2api/st2api/controllers/v1/packviews.py | 73 +++++++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 st2api/st2api/controllers/v1/packviews.py diff --git a/st2api/st2api/controllers/resource.py b/st2api/st2api/controllers/resource.py index f0d8174701..e6fe503f01 100644 --- a/st2api/st2api/controllers/resource.py +++ b/st2api/st2api/controllers/resource.py @@ -181,7 +181,7 @@ def _get_one_by_name_or_id(self, name_or_id, exclude_fields=None): from_model_kwargs = self._get_from_model_kwargs_for_request(request=pecan.request) result = self.model.from_model(instance, **from_model_kwargs) - LOG.debug('GET %s with name_or_odid=%s, client_result=%s', pecan.request.path, id, result) + LOG.debug('GET %s with name_or_id=%s, client_result=%s', pecan.request.path, id, result) return result diff --git a/st2api/st2api/controllers/v1/packs.py b/st2api/st2api/controllers/v1/packs.py index 947dcff9b2..c233840170 100644 --- a/st2api/st2api/controllers/v1/packs.py +++ b/st2api/st2api/controllers/v1/packs.py @@ -15,6 +15,7 @@ from st2common.models.api.base import jsexpose from st2api.controllers.resource import ResourceController +from st2api.controllers.v1.packviews import PackViewsController from st2common.models.api.pack import PackAPI from st2common.persistence.pack import Pack @@ -35,6 +36,9 @@ class PacksController(ResourceController): 'sort': ['ref'] } + # Nested controllers + views = PackViewsController() + @jsexpose(arg_types=[str]) def get_one(self, name_or_id): return self._get_one_by_name_or_id(name_or_id=name_or_id) diff --git a/st2api/st2api/controllers/v1/packviews.py b/st2api/st2api/controllers/v1/packviews.py new file mode 100644 index 0000000000..74d611179b --- /dev/null +++ b/st2api/st2api/controllers/v1/packviews.py @@ -0,0 +1,73 @@ +# Licensed to the StackStorm, Inc ('StackStorm') under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os + +import six +from pecan import abort +from pecan.rest import RestController + +from st2api.controllers import resource +from st2common.exceptions.db import StackStormDBObjectNotFoundError +from st2common import log as logging +from st2common.models.api.base import jsexpose +from st2common.models.api.pack import PackAPI +from st2common.persistence.pack import Pack +from st2common.content.utils import get_pack_resource_file_abs_path + +http_client = six.moves.http_client + +LOG = logging.getLogger(__name__) + + +class FilesController(resource.ResourceController): + """ + Controller which allows user to retrieve content of a particular file in the provided pack. + """ + + model = PackAPI + access = Pack + + supported_filters = {} + + @jsexpose() + def get_all(self, **kwargs): + return abort(404) + + @jsexpose(arg_types=[str, str, str], content_type='text/plain', status_code=http_client.OK) + def get_one(self, name_or_id, resource_type, file_path): + """ + Outputs the content of the requested file. + + Handles requests: + GET /packs/views/files/// + """ + pack_db = self._get_by_name_or_id(name_or_id=name_or_id) + pack_name = pack_db.name + file_path = get_pack_resource_file_abs_path(pack_name=pack_name, + resource_type=resource_type, + file_path=file_path) + + if not file_path or not os.path.isfile(file_path): + raise StackStormDBObjectNotFoundError('File "%s" not found' % (file_path)) + + with open(file_path) as fp: + content = fp.read() + + return content + + +class PackViewsController(RestController): + files = FilesController() From 2e3568d15ef178cb8b33b993fea9f5fe42677a34 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 20:16:51 +0200 Subject: [PATCH 14/34] Set packs_base_paths for tests to the fixtures directory. --- st2tests/st2tests/config.py | 1 + 1 file changed, 1 insertion(+) diff --git a/st2tests/st2tests/config.py b/st2tests/st2tests/config.py index 3257eec7c2..02e3443f04 100644 --- a/st2tests/st2tests/config.py +++ b/st2tests/st2tests/config.py @@ -66,6 +66,7 @@ def _override_db_opts(): def _override_common_opts(): packs_base_path = get_fixtures_base_path() CONF.set_override(name='system_packs_base_path', override=packs_base_path, group='content') + CONF.set_override(name='packs_base_paths', override=packs_base_path, group='content') CONF.set_override(name='api_url', override='http://localhost', group='auth') CONF.set_override(name='admin_users', override=['admin_user'], group='system') CONF.set_override(name='mask_secrets', override=True, group='log') From ce5c7629769c709b8b98cf4444c37d71a6667474 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 21:45:44 +0200 Subject: [PATCH 15/34] Also allow user to pass "exclude_fields" argument to the getter method on the ContentPackResourceController, remove method which is already implemented on the parent class. --- st2api/st2api/controllers/resource.py | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/st2api/st2api/controllers/resource.py b/st2api/st2api/controllers/resource.py index e6fe503f01..d3b7af593f 100644 --- a/st2api/st2api/controllers/resource.py +++ b/st2api/st2api/controllers/resource.py @@ -235,11 +235,11 @@ def get_one(self, ref_or_id): def get_all(self, **kwargs): return self._get_all(**kwargs) - def _get_one(self, ref_or_id): + def _get_one(self, ref_or_id, exclude_fields=None): LOG.info('GET %s with ref_or_id=%s', pecan.request.path, ref_or_id) try: - instance = self._get_by_ref_or_id(ref_or_id=ref_or_id) + instance = self._get_by_ref_or_id(ref_or_id=ref_or_id, exclude_fields=exclude_fields) except Exception as e: LOG.exception(e.message) pecan.abort(http_client.NOT_FOUND, e.message) @@ -268,7 +268,7 @@ def _get_all(self, **kwargs): return result - def _get_by_ref_or_id(self, ref_or_id): + def _get_by_ref_or_id(self, ref_or_id, exclude_fields=None): """ Retrieve resource object by an id of a reference. @@ -283,9 +283,9 @@ def _get_by_ref_or_id(self, ref_or_id): is_reference = False if is_reference: - resource_db = self._get_by_ref(resource_ref=ref_or_id) + resource_db = self._get_by_ref(resource_ref=ref_or_id, exclude_fields=exclude_fields) else: - resource_db = self._get_by_id(resource_id=ref_or_id) + resource_db = self._get_by_id(resource_id=ref_or_id, exclude_fields=exclude_fields) if not resource_db: msg = 'Resource with a reference or id "%s" not found' % (ref_or_id) @@ -293,21 +293,14 @@ def _get_by_ref_or_id(self, ref_or_id): return resource_db - def _get_by_id(self, resource_id): - try: - resource_db = self.access.get_by_id(resource_id) - except Exception: - resource_db = None - - return resource_db - - def _get_by_ref(self, resource_ref): + def _get_by_ref(self, resource_ref, exclude_fields=None): try: ref = ResourceReference.from_string_reference(ref=resource_ref) except Exception: return None - resource_db = self.access.query(name=ref.name, pack=ref.pack).first() + resource_db = self.access.query(name=ref.name, pack=ref.pack, + exclude_fields=exclude_fields).first() return resource_db def _get_filters(self, **kwargs): From 7910db5bab8c483d6d58177fa22f50e946fa5aad Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 21:55:17 +0200 Subject: [PATCH 16/34] Update chanhelog. --- CHANGELOG.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b85124182c..e5ab222a2f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,10 @@ in development * Add new OpenStack Keystone authentication backend. [Itxaka Serrano] * Information about parent workflow is now a dict in child's context field. (improvement) +* Allow user to include files which are written on disk inside the action create API payload. + (new feature) +* Allow user to retrieve content of a file inside a pack by using the new + ``/packs/views/files/`` API endpoint. (new feature) 0.12.0 - July 20, 2015 ---------------------- From ce7936e7697b50bd770498f08246c2af47fe7edb Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 3 Aug 2015 22:48:56 +0200 Subject: [PATCH 17/34] Update affected tetts so they don't violate entry_point constraint. --- st2actions/tests/unit/test_executions.py | 8 +++---- .../unit/controllers/v1/test_action_views.py | 4 ++-- .../controllers/v1/test_executions_filters.py | 8 +++---- .../st2tests/fixtures/executions/actions.yaml | 24 +++++++++---------- .../st2tests/fixtures/executions/chain.yaml | 4 ++-- .../fixtures/executions/liveactions.yaml | 6 ++--- .../st2tests/fixtures/executions/rule.yaml | 2 +- .../fixtures/history_views/filters.yaml | 4 ++-- 8 files changed, 30 insertions(+), 30 deletions(-) diff --git a/st2actions/tests/unit/test_executions.py b/st2actions/tests/unit/test_executions.py index ef42320f87..d1a8aadd71 100644 --- a/st2actions/tests/unit/test_executions.py +++ b/st2actions/tests/unit/test_executions.py @@ -72,7 +72,7 @@ def tearDown(self): super(TestActionExecutionHistoryWorker, self).tearDown() def test_basic_execution(self): - liveaction = LiveActionDB(action='core.local', parameters={'cmd': 'uname -a'}) + liveaction = LiveActionDB(action='executions.local', parameters={'cmd': 'uname -a'}) liveaction, _ = action_service.request(liveaction) liveaction = LiveAction.get_by_id(str(liveaction.id)) self.assertEqual(liveaction.status, action_constants.LIVEACTION_STATUS_FAILED) @@ -82,7 +82,7 @@ def test_basic_execution(self): self.assertDictEqual(execution.trigger_type, {}) self.assertDictEqual(execution.trigger_instance, {}) self.assertDictEqual(execution.rule, {}) - action = action_utils.get_action_by_ref('core.local') + action = action_utils.get_action_by_ref('executions.local') self.assertDictEqual(execution.action, vars(ActionAPI.from_model(action))) runner = RunnerType.get_by_name(action.runner_type['name']) self.assertDictEqual(execution.runner, vars(RunnerTypeAPI.from_model(runner))) @@ -100,13 +100,13 @@ def test_basic_execution_history_create_failed(self): self.test_basic_execution() def test_chained_executions(self): - liveaction = LiveActionDB(action='core.chain') + liveaction = LiveActionDB(action='executions.chain') liveaction, _ = action_service.request(liveaction) liveaction = LiveAction.get_by_id(str(liveaction.id)) self.assertEqual(liveaction.status, action_constants.LIVEACTION_STATUS_FAILED) execution = self._get_action_execution(liveaction__id=str(liveaction.id), raise_exception=True) - action = action_utils.get_action_by_ref('core.chain') + action = action_utils.get_action_by_ref('executions.chain') self.assertDictEqual(execution.action, vars(ActionAPI.from_model(action))) runner = RunnerType.get_by_name(action.runner_type['name']) self.assertDictEqual(execution.runner, vars(RunnerTypeAPI.from_model(runner))) diff --git a/st2api/tests/unit/controllers/v1/test_action_views.py b/st2api/tests/unit/controllers/v1/test_action_views.py index b1a378d7d8..88fe000112 100644 --- a/st2api/tests/unit/controllers/v1/test_action_views.py +++ b/st2api/tests/unit/controllers/v1/test_action_views.py @@ -25,7 +25,7 @@ 'description': 'test description', 'enabled': True, 'pack': 'wolfpack', - 'entry_point': '/tmp/test/action1.sh', + 'entry_point': 'test/action1.sh', 'runner_type': 'local-shell-script', 'parameters': { 'a': {'type': 'string', 'default': 'A1'}, @@ -39,7 +39,7 @@ 'description': 'test description', 'enabled': True, 'pack': 'wolfpack', - 'entry_point': '/tmp/test/action2.py', + 'entry_point': 'test/action2.py', 'runner_type': 'local-shell-script', 'parameters': { 'c': {'type': 'string', 'default': 'C1', 'position': 0}, diff --git a/st2api/tests/unit/controllers/v1/test_executions_filters.py b/st2api/tests/unit/controllers/v1/test_executions_filters.py index 66e7aed11e..5fb7f20aa2 100644 --- a/st2api/tests/unit/controllers/v1/test_executions_filters.py +++ b/st2api/tests/unit/controllers/v1/test_executions_filters.py @@ -99,13 +99,13 @@ def test_get_all(self): def test_get_all_exclude_attributes(self): # No attributes excluded - response = self.app.get('/v1/executions?action=core.local&limit=1') + response = self.app.get('/v1/executions?action=executions.local&limit=1') self.assertEqual(response.status_int, 200) self.assertTrue('result' in response.json[0]) # Exclude "result" attribute - path = '/v1/executions?action=core.local&limit=1&exclude_attributes=result' + path = '/v1/executions?action=executions.local&limit=1&exclude_attributes=result' response = self.app.get(path) self.assertEqual(response.status_int, 200) @@ -131,7 +131,7 @@ def test_get_one_failed(self): def test_limit(self): limit = 10 refs = [k for k, v in six.iteritems(self.refs) if v.action['name'] == 'chain'] - response = self.app.get('/v1/executions?action=core.chain&limit=%s' % + response = self.app.get('/v1/executions?action=executions.chain&limit=%s' % limit) self.assertEqual(response.status_int, 200) self.assertIsInstance(response.json, list) @@ -143,7 +143,7 @@ def test_limit(self): def test_query(self): refs = [k for k, v in six.iteritems(self.refs) if v.action['name'] == 'chain'] - response = self.app.get('/v1/executions?action=core.chain') + response = self.app.get('/v1/executions?action=executions.chain') self.assertEqual(response.status_int, 200) self.assertIsInstance(response.json, list) self.assertEqual(len(response.json), len(refs)) diff --git a/st2tests/st2tests/fixtures/executions/actions.yaml b/st2tests/st2tests/fixtures/executions/actions.yaml index 1f69d50006..842c900979 100644 --- a/st2tests/st2tests/fixtures/executions/actions.yaml +++ b/st2tests/st2tests/fixtures/executions/actions.yaml @@ -2,48 +2,48 @@ action-immutable-param-no-default: enabled: true name: action-immutable-param-no-default - pack: core + pack: executions parameters: foo: immutable: true - ref: core.action-immutable-param-no-default + ref: executions.action-immutable-param-no-default runner_type: run-local action-immutable-runner-param-no-default: enabled: true name: action-immutable-param-no-default - pack: core + pack: executions parameters: sudo: immutable: true - ref: core.action-immutable-param-no-default + ref: executions.action-immutable-param-no-default runner_type: run-local action-with-invalid-runner: enabled: true name: action-with-invalid-runner - pack: core + pack: executions parameters: hosts: immutable: false - ref: core.action-with-invalid-runner + ref: executions.action-with-invalid-runner runner_type: invalid-runner chain: enabled: true name: chain - pack: core - ref: core.chain + pack: executions + ref: executions.chain runner_type: action-chain local: enabled: true name: local - pack: core - ref: core.local + pack: executions + ref: executions.local runner_type: run-local local-override-runner-immutable: enabled: true name: local-override-runner-immutable - pack: core + pack: executions parameters: hosts: immutable: false - ref: core.local-override-runner-immutable + ref: executions.local-override-runner-immutable runner_type: run-local diff --git a/st2tests/st2tests/fixtures/executions/chain.yaml b/st2tests/st2tests/fixtures/executions/chain.yaml index c5321d32f3..9f42c099a7 100644 --- a/st2tests/st2tests/fixtures/executions/chain.yaml +++ b/st2tests/st2tests/fixtures/executions/chain.yaml @@ -4,9 +4,9 @@ chain: on-success: a2 params: cmd: uname -a - ref: core.local + ref: executions.local - name: a2 params: cmd: pwd - ref: core.local + ref: executions.local default: a1 diff --git a/st2tests/st2tests/fixtures/executions/liveactions.yaml b/st2tests/st2tests/fixtures/executions/liveactions.yaml index e021c2806f..bb21f1654a 100644 --- a/st2tests/st2tests/fixtures/executions/liveactions.yaml +++ b/st2tests/st2tests/fixtures/executions/liveactions.yaml @@ -1,6 +1,6 @@ --- task1: - action: core.local + action: executions.local callback: {} context: user: system @@ -20,7 +20,7 @@ task1: start_timestamp: '2014-09-01T00:00:02.000000Z' status: succeeded task2: - action: core.local + action: executions.local callback: {} context: user: system @@ -40,7 +40,7 @@ task2: start_timestamp: '2014-09-01T00:00:03.000000Z' status: succeeded workflow: - action: core.chain + action: executions.chain callback: {} context: user: system diff --git a/st2tests/st2tests/fixtures/executions/rule.yaml b/st2tests/st2tests/fixtures/executions/rule.yaml index bf40483a63..f3339c47bc 100644 --- a/st2tests/st2tests/fixtures/executions/rule.yaml +++ b/st2tests/st2tests/fixtures/executions/rule.yaml @@ -2,7 +2,7 @@ action: parameters: cmd: echo "{{trigger}}" >> /tmp/st2.persons.out - ref: core.local + ref: executions.local criteria: trigger.name: pattern: Joe diff --git a/st2tests/st2tests/fixtures/history_views/filters.yaml b/st2tests/st2tests/fixtures/history_views/filters.yaml index 77c34dde06..7bc5c6df20 100644 --- a/st2tests/st2tests/fixtures/history_views/filters.yaml +++ b/st2tests/st2tests/fixtures/history_views/filters.yaml @@ -1,7 +1,7 @@ --- action: -- core.local -- core.chain +- executions.local +- executions.chain rule: - st2.person.joe runner: From 389e539e605fd17a2f58846719b85480c2c808d6 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 4 Aug 2015 16:44:52 +0200 Subject: [PATCH 18/34] Add get_file_list utility functions. --- st2common/st2common/util/file_system.py | 71 +++++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 st2common/st2common/util/file_system.py diff --git a/st2common/st2common/util/file_system.py b/st2common/st2common/util/file_system.py new file mode 100644 index 0000000000..5dbcac1cad --- /dev/null +++ b/st2common/st2common/util/file_system.py @@ -0,0 +1,71 @@ +# Licensed to the StackStorm, Inc ('StackStorm') under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +""" +File system related utility functions. +""" + +import os +import os.path +import fnmatch + +__all__ = [ + 'get_file_list' +] + + +def get_file_list(directory, exclude_patterns=None): + """ + Recurisvely retrieve a list of files in the provided directory. + + :param directory: Path to directory to retrieve the file list for. + :type directory: ``str`` + + :param exclude_patterns: A list of `fnmatch` compatible patterns of files to exclude from + the result. + :type exclude_patterns: ``list`` + + :return: List of files in the provided directory. Each file path is relative + to the provided directory. + :rtype: ``list`` + """ + result = [] + if not directory.endswith('/'): + # Make sure trailing slash is present + directory = directory + '/' + + def include_file(file_path): + if not exclude_patterns: + return True + + for exclude_pattern in exclude_patterns: + if fnmatch.fnmatch(file_path, exclude_pattern): + return False + + return True + + for (dirpath, dirnames, filenames) in os.walk(directory): + base_path = dirpath.replace(directory, '') + + for filename in filenames: + if base_path: + file_path = os.path.join(base_path, filename) + else: + file_path = filename + + if include_file(file_path=file_path): + result.append(file_path) + + return result From 05fe0891018efee645fefafeeb22c21045e9a1a3 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 4 Aug 2015 16:52:41 +0200 Subject: [PATCH 19/34] Store a list of files inside a pack on the PackDB model. Note: This list is populated when running register content script when the pack is registered. --- st2common/st2common/bootstrap/base.py | 10 ++++++++++ st2common/st2common/models/api/pack.py | 8 +++++++- st2common/st2common/models/db/pack.py | 1 + 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/bootstrap/base.py b/st2common/st2common/bootstrap/base.py index dff3499884..0c54407145 100644 --- a/st2common/st2common/bootstrap/base.py +++ b/st2common/st2common/bootstrap/base.py @@ -24,6 +24,7 @@ from st2common.content.loader import ContentPackLoader from st2common.models.api.pack import PackAPI from st2common.persistence.pack import Pack +from st2common.util.file_system import get_file_list __all__ = [ 'ResourceRegistrar' @@ -37,6 +38,10 @@ # a long running process. REGISTERED_PACKS_CACHE = {} +EXCLUDE_FILE_PATTERNS = [ + '*.pyc' +] + class ResourceRegistrar(object): ALLOWED_EXTENSIONS = [] @@ -110,6 +115,11 @@ def _register_pack(self, pack_name, pack_dir): raise ValueError('Pack "%s" metadata file is empty' % (pack_name)) content['ref'] = pack_name + + # Include a list of pack files + pack_file_list = get_file_list(directory=pack_dir, exclude_patterns=EXCLUDE_FILE_PATTERNS) + content['files'] = pack_file_list + pack_api = PackAPI(**content) pack_db = PackAPI.to_model(pack_api) diff --git a/st2common/st2common/models/api/pack.py b/st2common/st2common/models/api/pack.py index 9a14b9146b..1f295f13e3 100644 --- a/st2common/st2common/models/api/pack.py +++ b/st2common/st2common/models/api/pack.py @@ -54,6 +54,11 @@ class PackAPI(BaseAPI): }, 'email': { 'type': 'string' + }, + 'files': { + 'type': 'array', + 'items': {'type': 'string'}, + 'default': [] } }, 'additionalProperties': False @@ -68,8 +73,9 @@ def to_model(cls, pack): version = str(pack.version) author = pack.author email = pack.email + files = getattr(pack, 'files', []) model = cls.model(name=name, description=description, ref=ref, keywords=keywords, - version=version, author=author, email=email) + version=version, author=author, email=email, files=files) return model diff --git a/st2common/st2common/models/db/pack.py b/st2common/st2common/models/db/pack.py index c561ddd757..3e9f08406c 100644 --- a/st2common/st2common/models/db/pack.py +++ b/st2common/st2common/models/db/pack.py @@ -34,6 +34,7 @@ class PackDB(stormbase.StormFoundationDB): version = me.StringField(required=True) # TODO: Enforce format author = me.StringField(required=True) email = me.EmailField(required=True) + files = me.ListField(field=me.StringField()) # specialized access objects pack_access = MongoDBAccess(PackDB) From e4253d0f27bc51a1a16f210aeb5a036415ce4b4d Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 4 Aug 2015 17:43:08 +0200 Subject: [PATCH 20/34] Add tests for get_file_list function. --- st2common/st2common/util/file_system.py | 2 +- st2common/tests/unit/test_util_file_system.py | 50 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 st2common/tests/unit/test_util_file_system.py diff --git a/st2common/st2common/util/file_system.py b/st2common/st2common/util/file_system.py index 5dbcac1cad..a68183bbee 100644 --- a/st2common/st2common/util/file_system.py +++ b/st2common/st2common/util/file_system.py @@ -54,7 +54,7 @@ def include_file(file_path): if fnmatch.fnmatch(file_path, exclude_pattern): return False - return True + return True for (dirpath, dirnames, filenames) in os.walk(directory): base_path = dirpath.replace(directory, '') diff --git a/st2common/tests/unit/test_util_file_system.py b/st2common/tests/unit/test_util_file_system.py new file mode 100644 index 0000000000..3aaf922f00 --- /dev/null +++ b/st2common/tests/unit/test_util_file_system.py @@ -0,0 +1,50 @@ +# Licensed to the StackStorm, Inc ('StackStorm') under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os +import os.path + +import unittest2 + +from st2common.util.file_system import get_file_list + +CURRENT_DIR = os.path.dirname(__file__) +ST2TESTS_DIR = os.path.join(CURRENT_DIR, '../../../st2tests/st2tests') + + +class FileSystemUtilsTestCase(unittest2.TestCase): + def test_get_file_list(self): + # Standard exclude pattern + directory = os.path.join(ST2TESTS_DIR, 'policies') + expected = [ + 'mock_exception.py', + 'concurrency.py', + '__init__.py', + 'meta/mock_exception.yaml', + 'meta/concurrency.yaml', + 'meta/__init__.py' + ] + result = get_file_list(directory=directory, exclude_patterns=['*.pyc']) + self.assertItemsEqual(expected, result) + + # Custom exclude pattern + expected = [ + 'mock_exception.py', + 'concurrency.py', + '__init__.py', + 'meta/__init__.py' + ] + result = get_file_list(directory=directory, exclude_patterns=['*.pyc', '*.yaml']) + self.assertItemsEqual(expected, result) From e3b9e5fad785cfbdfa21205f8976373e473b0a51 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 4 Aug 2015 18:29:10 +0200 Subject: [PATCH 21/34] Add new get_pack_file_abs_path function which is a superset of get_pack_resource_file_abs_path. Also update existing function to use this new one. --- st2common/st2common/content/utils.py | 54 ++++++++++++++++------ st2common/tests/unit/test_content_utils.py | 2 +- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/st2common/st2common/content/utils.py b/st2common/st2common/content/utils.py index f1da4f72e3..675ffb6693 100644 --- a/st2common/st2common/content/utils.py +++ b/st2common/st2common/content/utils.py @@ -27,6 +27,7 @@ 'get_packs_base_paths', 'get_pack_base_path', 'get_pack_directory', + 'get_pack_file_abs_path', 'get_pack_resource_file_abs_path', 'check_pack_directory_exists', 'check_pack_content_directory_exists' @@ -197,9 +198,9 @@ def get_entry_point_abs_path(pack=None, entry_point=None): return entry_point_abs_path -def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): +def get_pack_file_abs_path(pack_name, file_path): """ - Retrieve full absolute path to the pack resource file. + Retrieve full absolute path to the pack file. Note: This function also takes care of sanitizing ``file_name`` argument preventing directory traversal and similar attacks. @@ -207,11 +208,8 @@ def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): :param pack_name: Pack name. :type pack_name: ``str`` - :param resource_type: Pack resource type (e.g. action, sensor, etc.). - :type resource_type: ``str`` - :pack file_path: Resource file path relative to the pack directory (e.g. my_file.py or - directory/my_file.py) + actions/directory/my_file.py) :type file_path: ``str`` :rtype: ``str`` @@ -221,15 +219,6 @@ def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): path_components = [] path_components.append(pack_base_path) - if resource_type == 'action': - path_components.append('actions/') - elif resource_type == 'sensor': - path_components.append('sensors/') - elif resource_type == 'rule': - path_components.append('rules/') - else: - raise ValueError('Invalid resource type: %s' % (resource_type)) - # Normalize the path to prevent directory traversal normalized_file_path = os.path.normpath('/' + file_path).lstrip('/') @@ -249,6 +238,41 @@ def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): return result +def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): + """ + Retrieve full absolute path to the pack resource file. + + Note: This function also takes care of sanitizing ``file_name`` argument + preventing directory traversal and similar attacks. + + :param pack_name: Pack name. + :type pack_name: ``str`` + + :param resource_type: Pack resource type (e.g. action, sensor, etc.). + :type resource_type: ``str`` + + :pack file_path: Resource file path relative to the pack directory (e.g. my_file.py or + directory/my_file.py) + :type file_path: ``str`` + + :rtype: ``str`` + """ + path_components = [] + if resource_type == 'action': + path_components.append('actions/') + elif resource_type == 'sensor': + path_components.append('sensors/') + elif resource_type == 'rule': + path_components.append('rules/') + else: + raise ValueError('Invalid resource type: %s' % (resource_type)) + + path_components.append(file_path) + file_path = os.path.join(*path_components) + result = get_pack_file_abs_path(pack_name=pack_name, file_path=file_path) + return result + + def get_action_libs_abs_path(pack=None, entry_point=None): """ Return full absolute path of libs for an action. diff --git a/st2common/tests/unit/test_content_utils.py b/st2common/tests/unit/test_content_utils.py index 4314450444..3ff7b609a0 100644 --- a/st2common/tests/unit/test_content_utils.py +++ b/st2common/tests/unit/test_content_utils.py @@ -92,7 +92,7 @@ def test_get_pack_resource_file_abs_path(self): # Invalid paths (directory traversal and absolute paths) file_paths = ['/tmp/foo.py', '../foo.py', '/etc/passwd', '../../foo.py'] for file_path in file_paths: - expected_msg = 'Invalid file path: %s' % (file_path) + expected_msg = 'Invalid file path: .*%s' % (file_path) self.assertRaisesRegexp(ValueError, expected_msg, get_pack_resource_file_abs_path, pack_name='dummy_pack_1', resource_type='action', From 792490406715eb4bbc12ebb274647eec907352a3 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 4 Aug 2015 18:33:34 +0200 Subject: [PATCH 22/34] Update pack files view endpoint - allow user to retrieve a list of files with content of all the files inside a pack. Also add a new file controller which allows user to retrieve content of a single file. --- st2api/st2api/controllers/v1/packviews.py | 80 ++++++++++++++++++----- 1 file changed, 63 insertions(+), 17 deletions(-) diff --git a/st2api/st2api/controllers/v1/packviews.py b/st2api/st2api/controllers/v1/packviews.py index 74d611179b..d680fbe2f3 100644 --- a/st2api/st2api/controllers/v1/packviews.py +++ b/st2api/st2api/controllers/v1/packviews.py @@ -25,18 +25,14 @@ from st2common.models.api.base import jsexpose from st2common.models.api.pack import PackAPI from st2common.persistence.pack import Pack -from st2common.content.utils import get_pack_resource_file_abs_path +from st2common.content.utils import get_pack_file_abs_path http_client = six.moves.http_client LOG = logging.getLogger(__name__) -class FilesController(resource.ResourceController): - """ - Controller which allows user to retrieve content of a particular file in the provided pack. - """ - +class BaseFileController(resource.ResourceController): model = PackAPI access = Pack @@ -46,28 +42,78 @@ class FilesController(resource.ResourceController): def get_all(self, **kwargs): return abort(404) - @jsexpose(arg_types=[str, str, str], content_type='text/plain', status_code=http_client.OK) - def get_one(self, name_or_id, resource_type, file_path): + def _get_file_content(self, file_path): + with open(file_path, 'r') as fp: + content = fp.read() + + return content + + +class FilesController(BaseFileController): + """ + Controller which allows user to retrieve content of all the files inside the pack. + """ + + @jsexpose(arg_types=[str], status_code=http_client.OK) + def get_one(self, name_or_id): """ - Outputs the content of the requested file. + Outputs the content of all the files inside the pack. Handles requests: - GET /packs/views/files/// + GET /packs/views/files/ """ pack_db = self._get_by_name_or_id(name_or_id=name_or_id) pack_name = pack_db.name - file_path = get_pack_resource_file_abs_path(pack_name=pack_name, - resource_type=resource_type, - file_path=file_path) + pack_files = pack_db.files - if not file_path or not os.path.isfile(file_path): - raise StackStormDBObjectNotFoundError('File "%s" not found' % (file_path)) + result = [] + for file_path in pack_files: + normalized_file_path = get_pack_file_abs_path(pack_name=pack_name, file_path=file_path) - with open(file_path) as fp: - content = fp.read() + if not normalized_file_path or not os.path.isfile(normalized_file_path): + # Ignore references to files which don't exist on disk + continue + + content = self._get_file_content(file_path=normalized_file_path) + item = { + 'file_path': file_path, + 'content': content + } + result.append(item) + + return result + + +class FileController(BaseFileController): + """ + Controller which allows user to retrieve content of a specific file in a pack. + """ + @jsexpose(content_type='text/plain', status_code=http_client.OK) + def get_one(self, name_or_id, *file_path_components): + """ + Outputs the content of all the files inside the pack. + + Handles requests: + GET /packs/views/files// + """ + pack_db = self._get_by_name_or_id(name_or_id=name_or_id) + + if not file_path_components: + raise ValueError('Missing file path') + + file_path = os.path.join(*file_path_components) + pack_name = pack_db.name + + normalized_file_path = get_pack_file_abs_path(pack_name=pack_name, file_path=file_path) + + if not normalized_file_path or not os.path.isfile(normalized_file_path): + # Ignore references to files which don't exist on disk + raise StackStormDBObjectNotFoundError('File "%s" not found' % (file_path)) + content = self._get_file_content(file_path=normalized_file_path) return content class PackViewsController(RestController): files = FilesController() + file = FileController() From 1937a41adf3576788a3efbeeb5ddf74df6919d04 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Tue, 4 Aug 2015 22:00:44 +0200 Subject: [PATCH 23/34] Return 404 if user passes an invalid number of arguments aka requests an invalid path. --- st2common/st2common/models/api/base.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/models/api/base.py b/st2common/st2common/models/api/base.py index 0f831a4749..b8de82ff5f 100644 --- a/st2common/st2common/models/api/base.py +++ b/st2common/st2common/models/api/base.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import re import abc import copy import functools @@ -171,7 +172,17 @@ def callfunction(*args, **kwargs): pecan.response.status = status_code return json_encode(None) - result = f(*args, **kwargs) + try: + result = f(*args, **kwargs) + except TypeError as e: + message = str(e) + # Invalid number of arguments passed to the function meaning invalid path was + # requested + # Note: The check is hacky, but it works for now. + if re.search('takes exactly \d+ arguments \(\d+ given\)', message): + raise exc.HTTPNotFound() + else: + raise e if status_code: pecan.response.status = status_code From 08c271b21686f7e24b55d504069820f3418ba892 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 12:20:10 +0200 Subject: [PATCH 24/34] Also allow user to include files when updating an action. --- st2api/st2api/controllers/v1/actions.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index 6eee506ee4..70f83238d7 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -113,7 +113,7 @@ def post(self, action): return action_api - @jsexpose(arg_types=[str], body_cls=ActionAPI) + @jsexpose(arg_types=[str], body_cls=ActionCreateAPI) def put(self, action_ref_or_id, action): action_db = self._get_by_ref_or_id(ref_or_id=action_ref_or_id) action_id = action_db.id @@ -132,6 +132,13 @@ def put(self, action_ref_or_id, action): abort(http_client.BAD_REQUEST, str(e)) return + # Write pack data files to disk (if any are provided) + data_files = getattr(action, 'data_files', []) + written_data_files = [] + if data_files: + written_data_files = self._handle_data_files(pack_name=action.pack, + data_files=data_files) + try: action_db = ActionAPI.to_model(action) action_db.id = action_id From 292f40ae45f7aebea33890a249acd59fa7f9b441 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 12:21:07 +0200 Subject: [PATCH 25/34] Don't allow users to create actions in system packs using the API. --- st2api/st2api/controllers/v1/actions.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index 70f83238d7..5e82870a53 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -80,7 +80,11 @@ def post(self, action): """ if not hasattr(action, 'pack'): setattr(action, 'pack', DEFAULT_PACK_NAME) - # TODO: Dont allow manipulating system pack + + try: + validate_not_part_of_system_pack(action) + except ValueValidationException as e: + abort(http_client.BAD_REQUEST, str(e)) try: action_validator.validate_action(action) From b8abf258e20a917749386bd4ea2844acc6947969 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 12:27:55 +0200 Subject: [PATCH 26/34] Remove now unnecessary try / except - exception is now translated to the API error inside the JSONErrorResponseHook. --- st2api/st2api/controllers/v1/actions.py | 25 ++++++------------------- st2common/st2common/hooks.py | 3 ++- 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index 5e82870a53..a11830f37b 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -81,16 +81,9 @@ def post(self, action): if not hasattr(action, 'pack'): setattr(action, 'pack', DEFAULT_PACK_NAME) - try: - validate_not_part_of_system_pack(action) - except ValueValidationException as e: - abort(http_client.BAD_REQUEST, str(e)) - - try: - action_validator.validate_action(action) - except ValueValidationException as e: - abort(http_client.BAD_REQUEST, str(e)) - return + # Perform validation + validate_not_part_of_system_pack(action) + action_validator.validate_action(action) # Write pack data files to disk (if any are provided) data_files = getattr(action, 'data_files', []) @@ -122,19 +115,13 @@ def put(self, action_ref_or_id, action): action_db = self._get_by_ref_or_id(ref_or_id=action_ref_or_id) action_id = action_db.id - try: - validate_not_part_of_system_pack(action_db) - except ValueValidationException as e: - abort(http_client.BAD_REQUEST, str(e)) if not getattr(action, 'pack', None): action.pack = action_db.pack - try: - action_validator.validate_action(action) - except ValueValidationException as e: - abort(http_client.BAD_REQUEST, str(e)) - return + # Perform validation + validate_not_part_of_system_pack(action) + action_validator.validate_action(action) # Write pack data files to disk (if any are provided) data_files = getattr(action, 'data_files', []) diff --git a/st2common/st2common/hooks.py b/st2common/st2common/hooks.py index a23c2f7dfe..b8ddac33d8 100644 --- a/st2common/st2common/hooks.py +++ b/st2common/st2common/hooks.py @@ -26,6 +26,7 @@ from st2common.persistence.auth import User from st2common.exceptions import auth as exceptions from st2common.exceptions import db as db_exceptions +from st2common.exceptions.apivalidation import ValueValidationException from st2common.util.jsonify import json_encode from st2common.util.auth import validate_token from st2common.constants.auth import HEADER_ATTRIBUTE_NAME @@ -181,7 +182,7 @@ def on_error(self, state, e): status_code = httplib.CONFLICT message = str(e) body['conflict-id'] = e.conflict_id - elif isinstance(e, ValueError): + elif isinstance(e, (ValueValidationException, ValueError)): status_code = httplib.BAD_REQUEST message = getattr(e, 'message', str(e)) else: From 884341899aa0ade8b58c99f267e56771d9032fcd Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 15:46:10 +0200 Subject: [PATCH 27/34] Refactor _handle_data_files, also dispatch trigger when updating an action. --- st2api/st2api/controllers/v1/actions.py | 43 ++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index 91cf3468eb..eb85265d3a 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -34,6 +34,7 @@ from st2common.persistence.action import Action from st2common.models.api.action import ActionAPI from st2common.models.api.action import ActionCreateAPI +from st2common.persistence.pack import Pack from st2common.validators.api.misc import validate_not_part_of_system_pack from st2common.content.utils import get_pack_base_path from st2common.content.utils import get_pack_resource_file_abs_path @@ -138,6 +139,12 @@ def put(self, action_ref_or_id, action): abort(http_client.BAD_REQUEST, str(e)) return + # Dispatch an internal trigger for each written data file. This way user + # automate comitting this files to git using StackStorm rule + if written_data_files: + self._dispatch_trigger_for_written_data_files(action_db=action_db, + written_data_files=written_data_files) + action_api = ActionAPI.from_model(action_db) LOG.debug('PUT /actions/ client_result=%s', action_api) @@ -178,9 +185,29 @@ def delete(self, action_ref_or_id): def _handle_data_files(self, pack_name, data_files): """ - Method for handling action data files and writing them on disk. + Method for handling action data files. + + This method performs two tasks: + + 1. Writes files to disk + 2. Updates affected PackDB model + """ + # Write files to disk + written_file_paths = self._write_data_files_to_disk(pack_name=pack_name, + data_files=data_files) + + # Update affected PackDB model (update a list of files) + # Update PackDB + self._update_pack_model(pack_name=pack_name, data_files=data_files) + + return written_file_paths + + def _write_data_files_to_disk(self, pack_name, data_files): + """ + Write files to disk. """ written_file_paths = [] + for data_file in data_files: file_path = data_file['file_path'] content = data_file['content'] @@ -195,6 +222,20 @@ def _handle_data_files(self, pack_name, data_files): return written_file_paths + def _update_pack_model(self, pack_name, data_files): + """ + Update PackDB models (update files list). + """ + new_file_paths = [data['file_path'] for data in data_files] + + pack_db = Pack.get_by_ref(pack_name) + pack_db.files = set(pack_db.files) + pack_db.files.update(set(new_file_paths)) + pack_db.files = list(pack_db.files) + pack_db = Pack.add_or_update(pack_db) + + return pack_db + def _write_data_file(self, pack_name, file_path, content): """ Write data file on disk. From 751ec35e626a4ea528cffb2c3409210b94f90a0c Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 15:53:56 +0200 Subject: [PATCH 28/34] Only return 404 if TypeError refers to the controller method and not some other method. --- st2common/st2common/models/api/base.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/models/api/base.py b/st2common/st2common/models/api/base.py index b8de82ff5f..0ad317de2c 100644 --- a/st2common/st2common/models/api/base.py +++ b/st2common/st2common/models/api/base.py @@ -179,7 +179,10 @@ def callfunction(*args, **kwargs): # Invalid number of arguments passed to the function meaning invalid path was # requested # Note: The check is hacky, but it works for now. - if re.search('takes exactly \d+ arguments \(\d+ given\)', message): + func_name = f.__name__ + pattern = '%s\(\) takes exactly \d+ arguments \(\d+ given\)' % (func_name) + + if re.search(pattern, message): raise exc.HTTPNotFound() else: raise e From d47b2b427bafa46f630d9888eb258b47563be898 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 17:23:43 +0200 Subject: [PATCH 29/34] Add valid pack.yaml to packs inside the fixtures directory so they can be registered during tests. --- st2tests/st2tests/fixtures/dummy_pack_1/pack.yaml | 6 ++++++ st2tests/st2tests/fixtures/dummy_pack_2/pack.yaml | 6 ++++++ st2tests/st2tests/fixtures/dummy_pack_3/pack.yaml | 6 ++++++ 3 files changed, 18 insertions(+) diff --git a/st2tests/st2tests/fixtures/dummy_pack_1/pack.yaml b/st2tests/st2tests/fixtures/dummy_pack_1/pack.yaml index e69de29bb2..d0c7087c63 100644 --- a/st2tests/st2tests/fixtures/dummy_pack_1/pack.yaml +++ b/st2tests/st2tests/fixtures/dummy_pack_1/pack.yaml @@ -0,0 +1,6 @@ +--- +name : dummy_pack_1 +description : dummy pack +version : 0.1 +author : st2-dev +email : info@stackstorm.com diff --git a/st2tests/st2tests/fixtures/dummy_pack_2/pack.yaml b/st2tests/st2tests/fixtures/dummy_pack_2/pack.yaml index e69de29bb2..d0c7087c63 100644 --- a/st2tests/st2tests/fixtures/dummy_pack_2/pack.yaml +++ b/st2tests/st2tests/fixtures/dummy_pack_2/pack.yaml @@ -0,0 +1,6 @@ +--- +name : dummy_pack_1 +description : dummy pack +version : 0.1 +author : st2-dev +email : info@stackstorm.com diff --git a/st2tests/st2tests/fixtures/dummy_pack_3/pack.yaml b/st2tests/st2tests/fixtures/dummy_pack_3/pack.yaml index e69de29bb2..d0c7087c63 100644 --- a/st2tests/st2tests/fixtures/dummy_pack_3/pack.yaml +++ b/st2tests/st2tests/fixtures/dummy_pack_3/pack.yaml @@ -0,0 +1,6 @@ +--- +name : dummy_pack_1 +description : dummy pack +version : 0.1 +author : st2-dev +email : info@stackstorm.com From 5ffb8b2db283d97dc608692dd6123a3fd4562982 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 17:31:52 +0200 Subject: [PATCH 30/34] Register all the packs inside the fixtures directory in the DBTestCase.setUpClass method. --- st2tests/st2tests/base.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/st2tests/st2tests/base.py b/st2tests/st2tests/base.py index 204e0a429c..acb4f32889 100644 --- a/st2tests/st2tests/base.py +++ b/st2tests/st2tests/base.py @@ -29,6 +29,8 @@ from st2common.exceptions.db import StackStormDBObjectConflictError from st2common.models.db import db_setup, db_teardown +from st2common.bootstrap.base import ResourceRegistrar +from st2common.content.utils import get_packs_base_paths import st2common.models.db.rule as rule_model import st2common.models.db.sensor as sensor_model import st2common.models.db.trigger as trigger_model @@ -103,6 +105,14 @@ class BaseDbTestCase(TestCase): def setUpClass(cls): st2tests.config.parse_args() + @classmethod + def _register_packs(self): + """ + Register all the packs inside the fixtures directory. + """ + registrar = ResourceRegistrar() + registrar.register_packs(base_dirs=get_packs_base_paths()) + @classmethod def _establish_connection_and_re_create_db(cls): username = cfg.CONF.database.username if hasattr(cfg.CONF.database, 'username') else None @@ -147,6 +157,7 @@ class DbTestCase(BaseDbTestCase): def setUpClass(cls): BaseDbTestCase.setUpClass() cls._establish_connection_and_re_create_db() + cls._register_packs() @classmethod def tearDownClass(cls): From 80e4719d22cd4cd2b906e3b9f83da93d5d4bd43e Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 18:51:20 +0200 Subject: [PATCH 31/34] Add some tests for including files on action create. --- .../tests/unit/controllers/v1/test_actions.py | 50 ++++++++++++++++++- st2tests/st2tests/base.py | 15 +++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/st2api/tests/unit/controllers/v1/test_actions.py b/st2api/tests/unit/controllers/v1/test_actions.py index 96748a051b..bf83ad2394 100644 --- a/st2api/tests/unit/controllers/v1/test_actions.py +++ b/st2api/tests/unit/controllers/v1/test_actions.py @@ -13,7 +13,10 @@ # See the License for the specific language governing permissions and # limitations under the License. +import os +import os.path import copy + try: import simplejson as json except ImportError: @@ -25,6 +28,9 @@ from st2common.persistence.action import Action import st2common.validators.api.action as action_validator from st2common.constants.pack import SYSTEM_PACK_NAME +from st2common.persistence.pack import Pack +from st2tests.fixturesloader import get_fixtures_base_path +from st2tests.base import CleanFilesTestCase from tests import FunctionalTest @@ -184,8 +190,29 @@ } } +# Good action inside dummy pack +ACTION_12 = { + 'name': 'st2.dummy.action1', + 'description': 'test description', + 'enabled': True, + 'pack': 'dummy_pack_1', + 'entry_point': '/tmp/test/action1.sh', + 'runner_type': 'local-shell-script', + 'parameters': { + 'a': {'type': 'string', 'default': 'A1'}, + 'b': {'type': 'string', 'default': 'B1'} + }, + 'tags': [ + {'name': 'tag1', 'value': 'dont-care'}, + {'name': 'tag2', 'value': 'dont-care'} + ] +} + +class TestActionController(FunctionalTest, CleanFilesTestCase): + to_delete_files = [ + os.path.join(get_fixtures_base_path(), 'dummy_pack_1/actions/filea.txt') + ] -class TestActionController(FunctionalTest): @mock.patch.object(action_validator, 'validate_action', mock.MagicMock( return_value=True)) def test_get_one_using_id(self): @@ -331,6 +358,27 @@ def test_post_duplicate(self): for i in action_ids: self.__do_delete(i) + @mock.patch.object(action_validator, 'validate_action', mock.MagicMock( + return_value=True)) + def test_post_include_files(self): + # Verify initial state + pack_db = Pack.get_by_ref(ACTION_12['pack']) + self.assertTrue('actions/filea.txt' not in pack_db.files) + + action = copy.deepcopy(ACTION_12) + action['data_files'] = [ + { + 'file_path': 'filea.txt', + 'content': 'test content' + } + ] + post_resp = self.__do_post(action) + + # Verify PackDB.files has been updated + pack_db = Pack.get_by_ref(ACTION_12['pack']) + self.assertTrue('actions/filea.txt' in pack_db.files) + self.__do_delete(self.__get_action_id(post_resp)) + @mock.patch.object(action_validator, 'validate_action', mock.MagicMock( return_value=True)) def test_post_put_delete(self): diff --git a/st2tests/st2tests/base.py b/st2tests/st2tests/base.py index acb4f32889..1a697301f1 100644 --- a/st2tests/st2tests/base.py +++ b/st2tests/st2tests/base.py @@ -19,6 +19,7 @@ import json import os +import os.path import sys import shutil @@ -266,19 +267,31 @@ def setUp(self): class CleanFilesTestCase(TestCase): """ - Base test class which deletes specified files and directories on tearDown. + Base test class which deletes specified files and directories on setUp and `tearDown. """ to_delete_files = [] to_delete_directories = [] + def setUp(self): + self._delete_files() + def tearDown(self): + self._delete_files() + + def _delete_files(self): for file_path in self.to_delete_files: + if not os.path.isfile(file_path): + continue + try: os.remove(file_path) except Exception: pass for file_path in self.to_delete_directories: + if not os.path.isdir(file_path): + continue + try: shutil.rmtree(file_path) except Exception: From 349fcc55fa7f18fae04e62918a4a7a7d2e3fc1f0 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 19:14:16 +0200 Subject: [PATCH 32/34] Make sure we store relative paths. --- st2api/st2api/controllers/v1/actions.py | 13 +++++++++---- st2common/st2common/content/utils.py | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index eb85265d3a..845dfa7b7b 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -38,6 +38,7 @@ from st2common.validators.api.misc import validate_not_part_of_system_pack from st2common.content.utils import get_pack_base_path from st2common.content.utils import get_pack_resource_file_abs_path +from st2common.content.utils import get_relative_path_to_pack from st2common.transport.reactor import TriggerDispatcher from st2common.util.system_info import get_host_info import st2common.validators.api.action as action_validator @@ -198,7 +199,8 @@ def _handle_data_files(self, pack_name, data_files): # Update affected PackDB model (update a list of files) # Update PackDB - self._update_pack_model(pack_name=pack_name, data_files=data_files) + self._update_pack_model(pack_name=pack_name, data_files=data_files, + written_file_paths=written_file_paths) return written_file_paths @@ -222,15 +224,18 @@ def _write_data_files_to_disk(self, pack_name, data_files): return written_file_paths - def _update_pack_model(self, pack_name, data_files): + def _update_pack_model(self, pack_name, data_files, written_file_paths): """ Update PackDB models (update files list). """ - new_file_paths = [data['file_path'] for data in data_files] + file_paths = [] # A list of paths relative to the pack directory for new files + for file_path in written_file_paths: + file_path = get_relative_path_to_pack(pack_name=pack_name, file_path=file_path) + file_paths.append(file_path) pack_db = Pack.get_by_ref(pack_name) pack_db.files = set(pack_db.files) - pack_db.files.update(set(new_file_paths)) + pack_db.files.update(set(file_paths)) pack_db.files = list(pack_db.files) pack_db = Pack.add_or_update(pack_db) diff --git a/st2common/st2common/content/utils.py b/st2common/st2common/content/utils.py index 675ffb6693..89b4d2a3a3 100644 --- a/st2common/st2common/content/utils.py +++ b/st2common/st2common/content/utils.py @@ -29,6 +29,7 @@ 'get_pack_directory', 'get_pack_file_abs_path', 'get_pack_resource_file_abs_path', + 'get_relative_path_to_pack', 'check_pack_directory_exists', 'check_pack_content_directory_exists' ] @@ -273,6 +274,25 @@ def get_pack_resource_file_abs_path(pack_name, resource_type, file_path): return result +def get_relative_path_to_pack(pack_name, file_path): + """ + Retrieve a file path which is relative to the provided pack directory. + + :rtype: ``str`` + """ + pack_base_path = get_pack_base_path(pack_name=pack_name) + + if not os.path.isabs(file_path): + return file_path + + common_prefix = os.path.commonprefix([pack_base_path, file_path]) + if common_prefix != pack_base_path: + raise ValueError('file_path is not located inside the pack directory') + + relative_path = os.path.relpath(file_path, common_prefix) + return relative_path + + def get_action_libs_abs_path(pack=None, entry_point=None): """ Return full absolute path of libs for an action. From f4f11ecde98ec8c096b7ab65ec9d50759b433d2b Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 19:28:19 +0200 Subject: [PATCH 33/34] Also verify file has been created. --- st2api/tests/unit/controllers/v1/test_actions.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/st2api/tests/unit/controllers/v1/test_actions.py b/st2api/tests/unit/controllers/v1/test_actions.py index bf83ad2394..f55d7ce390 100644 --- a/st2api/tests/unit/controllers/v1/test_actions.py +++ b/st2api/tests/unit/controllers/v1/test_actions.py @@ -374,6 +374,10 @@ def test_post_include_files(self): ] post_resp = self.__do_post(action) + # Verify file has been written on disk + for file_path in self.to_delete_files: + self.assertTrue(os.path.exists(file_path)) + # Verify PackDB.files has been updated pack_db = Pack.get_by_ref(ACTION_12['pack']) self.assertTrue('actions/filea.txt' in pack_db.files) From 962f33ba14e4d32af534b735277ec747fd191632 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 5 Aug 2015 22:18:09 +0200 Subject: [PATCH 34/34] Fix broken test, only register packs with test_actions controller test case. --- .../tests/unit/controllers/v1/test_actions.py | 2 ++ st2common/st2common/bootstrap/base.py | 10 ++++++-- st2tests/st2tests/base.py | 23 +++++++++++-------- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/st2api/tests/unit/controllers/v1/test_actions.py b/st2api/tests/unit/controllers/v1/test_actions.py index f55d7ce390..a32717bd4f 100644 --- a/st2api/tests/unit/controllers/v1/test_actions.py +++ b/st2api/tests/unit/controllers/v1/test_actions.py @@ -208,7 +208,9 @@ ] } + class TestActionController(FunctionalTest, CleanFilesTestCase): + register_packs = True to_delete_files = [ os.path.join(get_fixtures_base_path(), 'dummy_pack_1/actions/filea.txt') ] diff --git a/st2common/st2common/bootstrap/base.py b/st2common/st2common/bootstrap/base.py index 0c54407145..442e05b084 100644 --- a/st2common/st2common/bootstrap/base.py +++ b/st2common/st2common/bootstrap/base.py @@ -46,7 +46,13 @@ class ResourceRegistrar(object): ALLOWED_EXTENSIONS = [] - def __init__(self): + def __init__(self, use_pack_cache=True): + """ + :param use_pack_cache: True to cache which packs have been registered in memory and making + sure packs are only registered once. + :type use_pack_cache: ``bool`` + """ + self._use_pack_cache = use_pack_cache self._meta_loader = MetaLoader() self._pack_loader = ContentPackLoader() @@ -83,7 +89,7 @@ def register_pack(self, pack_name, pack_dir): """ Register pack in the provided directory. """ - if pack_name in REGISTERED_PACKS_CACHE: + if self._use_pack_cache and pack_name in REGISTERED_PACKS_CACHE: # This pack has already been registered during this register content run return diff --git a/st2tests/st2tests/base.py b/st2tests/st2tests/base.py index 1a697301f1..4c227c4902 100644 --- a/st2tests/st2tests/base.py +++ b/st2tests/st2tests/base.py @@ -106,14 +106,6 @@ class BaseDbTestCase(TestCase): def setUpClass(cls): st2tests.config.parse_args() - @classmethod - def _register_packs(self): - """ - Register all the packs inside the fixtures directory. - """ - registrar = ResourceRegistrar() - registrar.register_packs(base_dirs=get_packs_base_paths()) - @classmethod def _establish_connection_and_re_create_db(cls): username = cfg.CONF.database.username if hasattr(cfg.CONF.database, 'username') else None @@ -142,6 +134,14 @@ def _drop_collections(cls): for model in ALL_MODELS: model.drop_collection() + @classmethod + def _register_packs(self): + """ + Register all the packs inside the fixtures directory. + """ + registrar = ResourceRegistrar(use_pack_cache=False) + registrar.register_packs(base_dirs=get_packs_base_paths()) + class DbTestCase(BaseDbTestCase): """ @@ -153,12 +153,15 @@ class DbTestCase(BaseDbTestCase): db_connection = None current_result = None + register_packs = False @classmethod def setUpClass(cls): BaseDbTestCase.setUpClass() cls._establish_connection_and_re_create_db() - cls._register_packs() + + if cls.register_packs: + cls._register_packs() @classmethod def tearDownClass(cls): @@ -273,9 +276,11 @@ class CleanFilesTestCase(TestCase): to_delete_directories = [] def setUp(self): + super(CleanFilesTestCase, self).setUp() self._delete_files() def tearDown(self): + super(CleanFilesTestCase, self).tearDown() self._delete_files() def _delete_files(self):