From 0249de1de74d899e0f4431f9914525c2a7c501c2 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 17 Jul 2017 09:53:54 +0200 Subject: [PATCH 1/7] Fix a bug - all "create" permission types are global. --- st2common/st2common/rbac/types.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/rbac/types.py b/st2common/st2common/rbac/types.py index 5b3b4f9479..896df00453 100644 --- a/st2common/st2common/rbac/types.py +++ b/st2common/st2common/rbac/types.py @@ -406,6 +406,8 @@ class SystemRole(Enum): ALL_PERMISSION_TYPES = list(itertools.chain(*ALL_PERMISSION_TYPES)) LIST_PERMISSION_TYPES = [permission_type for permission_type in ALL_PERMISSION_TYPES if permission_type.endswith('_list')] +CREATE_PERMISSION_TYPES = [permission_type for permission_type in ALL_PERMISSION_TYPES if + permission_type.endswith('_create')] # List of global permissions (ones which don't apply to a specific resource) GLOBAL_PERMISSION_TYPES = [ @@ -430,7 +432,7 @@ class SystemRole(Enum): # Execution PermissionType.EXECUTION_VIEWS_FILTERS_LIST -] + LIST_PERMISSION_TYPES +] + LIST_PERMISSION_TYPES + CREATE_PERMISSION_TYPES GLOBAL_PACK_PERMISSION_TYPES = [permission_type for permission_type in GLOBAL_PERMISSION_TYPES if permission_type.startswith('pack_')] From 5ad10092309db69c66dc0bf9d30b11e2121d7c9f Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 17 Jul 2017 10:07:10 +0200 Subject: [PATCH 2/7] Add some more debug logging. --- st2common/st2common/rbac/resolvers.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/st2common/st2common/rbac/resolvers.py b/st2common/st2common/rbac/resolvers.py index 3f86bca852..394f0fb03b 100644 --- a/st2common/st2common/rbac/resolvers.py +++ b/st2common/st2common/rbac/resolvers.py @@ -212,6 +212,7 @@ def _user_has_resource_permission(self, user_db, pack_uid, resource_uid, permiss self._log('Checking user resource permissions', extra=log_context) # First check the system role permissions + self._log('Checking grants via system role permissions', extra=log_context) has_system_role_permission = self._user_has_system_role_permission( user_db=user_db, permission_type=permission_type) @@ -235,6 +236,7 @@ def _user_has_resource_permission(self, user_db, pack_uid, resource_uid, permiss permission_types = [permission_type] # Check direct grants on the specified resource + self._log('Checking direct grans on the specified resource', extra=log_context) resource_types = [self.resource_type] permission_grants = get_all_permission_grants_for_user(user_db=user_db, resource_uid=resource_uid, @@ -245,6 +247,7 @@ def _user_has_resource_permission(self, user_db, pack_uid, resource_uid, permiss return True # Check grants on the parent pack + self._log('Checking grants on the parent resource', extra=log_context) resource_types = [ResourceType.PACK] permission_grants = get_all_permission_grants_for_user(user_db=user_db, resource_uid=pack_uid, From 7632d604c148217321e9c7dccd0b3d840ca45fba Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 17 Jul 2017 10:32:29 +0200 Subject: [PATCH 3/7] Add a test case which replicates issue described in #3572 (user can't create rules which reference actions which don't exist in the system when RBAC is enabled). --- .../unit/controllers/v1/test_rules_rbac.py | 16 ++++++++++++++ .../rules/rule_action_doesnt_exist.yaml | 21 +++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100644 st2tests/st2tests/fixtures/generic/rules/rule_action_doesnt_exist.yaml diff --git a/st2api/tests/unit/controllers/v1/test_rules_rbac.py b/st2api/tests/unit/controllers/v1/test_rules_rbac.py index af1847b11e..b6fc7f76cc 100644 --- a/st2api/tests/unit/controllers/v1/test_rules_rbac.py +++ b/st2api/tests/unit/controllers/v1/test_rules_rbac.py @@ -65,6 +65,11 @@ def setUp(self): fixtures_pack=FIXTURES_PACK, fixtures_dict={'rules': [file_name]})['rules'][file_name] + file_name = 'rule_action_doesnt_exist.yaml' + RuleControllerRBACTestCase.RULE_3 = self.fixtures_loader.load_fixtures( + fixtures_pack=FIXTURES_PACK, + fixtures_dict={'rules': [file_name]})['rules'][file_name] + # Insert mock users, roles and assignments # Users @@ -156,6 +161,17 @@ def test_post_webhook_trigger_no_trigger_and_action_permission(self): self.assertEqual(resp.status_code, httplib.FORBIDDEN) self.assertEqual(resp.json['faultstring'], expected_msg) + def test_post_user_has_no_permission_on_action_which_doesnt_exist_in_db(self): + # User has rule_create, but no action_execute on the action which doesn't exist in the db + user_db = self.users['rule_create_webhook_create'] + self.use_user(user_db) + + resp = self.__do_post(RuleControllerRBACTestCase.RULE_3) + expected_msg = ('User "rule_create_webhook_create" doesn\'t have required (action_execute)' + ' permission to use action wolfpack.action-doesnt-exist-woo') + self.assertEqual(resp.status_code, httplib.FORBIDDEN) + self.assertEqual(resp.json['faultstring'], expected_msg) + def test_post_no_webhook_trigger(self): # Test a scenario when user with only "rule_create" permission selects a non-webhook # trigger for which we don't perform any permission checking right now diff --git a/st2tests/st2tests/fixtures/generic/rules/rule_action_doesnt_exist.yaml b/st2tests/st2tests/fixtures/generic/rules/rule_action_doesnt_exist.yaml new file mode 100644 index 0000000000..a1af3cc9c0 --- /dev/null +++ b/st2tests/st2tests/fixtures/generic/rules/rule_action_doesnt_exist.yaml @@ -0,0 +1,21 @@ +--- +action: + parameters: + ip1: '{{trigger.t1_p}}' + ip2: '{{trigger}}' + ref: wolfpack.action-doesnt-exist-woo +criteria: + trigger.t1_p: + pattern: t1_p_v + type: equals +description: '' +enabled: true +name: rule_action_doesnt_exist +pack: examples +tags: +- name: tag1 + value: dont-care +- name: tag2 + value: dont-care +trigger: + type: wolfpack.triggertype-1 From 1a519cb33eb1b8fd4d4533448b340b02bf326f8b Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 17 Jul 2017 10:33:40 +0200 Subject: [PATCH 4/7] Fix a bug and allow users to create rules which reference actions which don't exist in the system when RBAC is enabled. --- st2common/st2common/rbac/utils.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/st2common/st2common/rbac/utils.py b/st2common/st2common/rbac/utils.py index ad29b3be94..b44e70c9ae 100644 --- a/st2common/st2common/rbac/utils.py +++ b/st2common/st2common/rbac/utils.py @@ -21,6 +21,8 @@ from oslo_config import cfg +from st2common.models.db.action import ActionDB +from st2common.models.system.common import ResourceReference from st2common.exceptions.rbac import AccessDeniedError from st2common.exceptions.rbac import ResourceTypeAccessDeniedError from st2common.exceptions.rbac import ResourceAccessDeniedError @@ -162,11 +164,20 @@ def user_has_rule_action_permission(user_db, action_ref): """ Check that the currently logged-in has necessary permissions on the action used / referenced inside the rule. + + Note: Rules can reference actions which don't yet exist in the system. """ if not cfg.CONF.rbac.enable: return True action_db = action_utils.get_action_by_ref(ref=action_ref) + + if not action_db: + # We allow rules to be created for actions which don't yet exist in the + # system + ref = ResourceReference.from_string_reference(ref=action_ref) + action_db = ActionDB(pack=ref.pack, name=ref.name, ref=action_ref) + action_resolver = resolvers.get_resolver_for_resource_type(ResourceType.ACTION) has_action_permission = action_resolver.user_has_resource_db_permission( user_db=user_db, resource_db=action_db, permission_type=PermissionType.ACTION_EXECUTE) From 283e0d6409bd242d54b9050891fd99f86ba1e288 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 17 Jul 2017 10:35:51 +0200 Subject: [PATCH 5/7] Update changelog. --- CHANGELOG.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4f1caaaa34..4078b077bd 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -31,6 +31,11 @@ Fixed (bug fix) Contributed by carbineneutral. #3534 #3544 +* Fix an API bug and allow users to create rules which reference actions which don't yet exist in + the system when RBAC is enabled and user doesn't have system admin permission. (bug fix) + #3572 + + Reported by sibirajal. 2.3.1 - July 07, 2017 --------------------- From 5a4c4d7d2436c70beae8ed43efd12d3fc7312b73 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 17 Jul 2017 10:39:00 +0200 Subject: [PATCH 6/7] Add PR reference to changelog entry. --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4078b077bd..7dd7ef23f3 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -33,7 +33,7 @@ Fixed Contributed by carbineneutral. #3534 #3544 * Fix an API bug and allow users to create rules which reference actions which don't yet exist in the system when RBAC is enabled and user doesn't have system admin permission. (bug fix) - #3572 + #3572 #3573 Reported by sibirajal. From f4d15ea03b806e71ce1cc033ac61584318111f13 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 17 Jul 2017 10:50:02 +0200 Subject: [PATCH 7/7] Revert "Fix a bug - all "create" permission types are global." This reverts commit 0249de1de74d899e0f4431f9914525c2a7c501c2. --- st2common/st2common/rbac/types.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/st2common/st2common/rbac/types.py b/st2common/st2common/rbac/types.py index 896df00453..5b3b4f9479 100644 --- a/st2common/st2common/rbac/types.py +++ b/st2common/st2common/rbac/types.py @@ -406,8 +406,6 @@ class SystemRole(Enum): ALL_PERMISSION_TYPES = list(itertools.chain(*ALL_PERMISSION_TYPES)) LIST_PERMISSION_TYPES = [permission_type for permission_type in ALL_PERMISSION_TYPES if permission_type.endswith('_list')] -CREATE_PERMISSION_TYPES = [permission_type for permission_type in ALL_PERMISSION_TYPES if - permission_type.endswith('_create')] # List of global permissions (ones which don't apply to a specific resource) GLOBAL_PERMISSION_TYPES = [ @@ -432,7 +430,7 @@ class SystemRole(Enum): # Execution PermissionType.EXECUTION_VIEWS_FILTERS_LIST -] + LIST_PERMISSION_TYPES + CREATE_PERMISSION_TYPES +] + LIST_PERMISSION_TYPES GLOBAL_PACK_PERMISSION_TYPES = [permission_type for permission_type in GLOBAL_PERMISSION_TYPES if permission_type.startswith('pack_')]