Fix failing v2 identity user tests by adding admin_only kwarg.
Currently, a number of identity v2 Patrole tests wrongly assume that policy
enforcement is executed by Keystone: for example [0]. These tests are
written just like any other Patrole tests but cannot be.
This is because Keystone does not actually perform a policy lookup for
many v2 endpoints: for example [1]. In the listed example, policy enforcement
is not done at all; instead, Keystone executes "self.assert_admin(request)"
which checks whether the request context has admin credentials. If not, a
403 is thrown, which is why many identity v2 Patrole tests are failing.
Policy enforcement is only executed when @controllers.protected() is
present above the API [2]; otherwise it is not. Since it is unlikely
that Keystone will change policy enforcement in its deprecated v2 API,
Patrole should instead compensate for this limitation with new
functionality.
Thus, Patrole's rbac_rule_validation.action decorator was
enhanced to take a new kwarg called "admin_only" whose default value
is False. When set to True, the local variable allowed in
rbac_rule_validation.action will check whether the current
rbac_test_role is admin: if it is, then the Patrole framework will
expect the test to pass; otherwise it will expect the test to fail.
[0] https://github.com/openstack/patrole/blob/master/patrole_tempest_plugin/tests/api/identity/v2/test_users_rbac.py
[1] https://github.com/openstack/keystone/blob/a3aee6ccb52d85eac1deedec31724a955d47fa96/keystone/identity/controllers.py
[2] https://github.com/openstack/keystone/blob/master/keystone/common/controller.py
Change-Id: Ie4025f45dc0b9434b0f5216bad8e441cdbe3b6f4
Closes-Bug: #1674495
Partial-Bug: #1670553
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 33ee666..5e1e816 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -26,7 +26,37 @@
LOG = logging.getLogger(__name__)
-def action(service, rule, expected_error_code=403):
+def action(service, rule, admin_only=False, expected_error_code=403):
+ """A decorator which does a policy check and matches it against test run.
+
+ A decorator which allows for positive and negative RBAC testing. Given
+ an OpenStack service and a policy action enforced by that service, an
+ oslo.policy lookup is performed by calling `authority.get_permission`.
+ The following cases are possible:
+
+ * If `allowed` is True and the test passes, this is a success.
+ * If `allowed` is True and the test fails, this is a failure.
+ * If `allowed` is False and the test passes, this is a failure.
+ * If `allowed` is False and the test fails, this is a success.
+
+ :param service: A OpenStack service: for example, "nova" or "neutron".
+ :param rule: A policy action defined in a policy.json file (or in code).
+ :param admin_only: Skips over oslo.policy check because the policy action
+ defined by `rule` is not enforced by the service's
+ policy enforcement logic. For example, Keystone v2
+ performs an admin check for most of its endpoints. If
+ True, `rule` is effectively ignored.
+ :param expected_error_code: Overrides default value of 403 (Forbidden)
+ with endpoint-specific error code. Currently
+ only supports 403 and 404. Support for 404
+ is needed because some services, like Neutron,
+ intentionally throw a 404 for security reasons.
+
+ :raises NotFound: if `service` is invalid or
+ if Tempest credentials cannot be found.
+ :raises Forbidden: for bullet (2) above.
+ :raises RbacOverPermission: for bullet (3) above.
+ """
def decorator(func):
def wrapper(*args, **kwargs):
try:
@@ -41,8 +71,17 @@
LOG.error(msg)
raise rbac_exceptions.RbacResourceSetupFailed(msg)
- authority = rbac_auth.RbacAuthority(tenant_id, user_id, service)
- allowed = authority.get_permission(rule, CONF.rbac.rbac_test_role)
+ if admin_only:
+ LOG.info("As admin_only is True, only admin role should be "
+ "allowed to perform the API. Skipping oslo.policy "
+ "check for policy action {0}.".format(rule))
+ allowed = CONF.rbac.rbac_test_role == 'admin'
+ else:
+ authority = rbac_auth.RbacAuthority(tenant_id, user_id,
+ service)
+ allowed = authority.get_permission(rule,
+ CONF.rbac.rbac_test_role)
+
expected_exception, irregular_msg = _get_exception_type(
expected_error_code)
diff --git a/patrole_tempest_plugin/tests/api/identity/v2/test_endpoints_rbac.py b/patrole_tempest_plugin/tests/api/identity/v2/test_endpoints_rbac.py
index ccde9bd..ba04d06 100644
--- a/patrole_tempest_plugin/tests/api/identity/v2/test_endpoints_rbac.py
+++ b/patrole_tempest_plugin/tests/api/identity/v2/test_endpoints_rbac.py
@@ -54,7 +54,8 @@
return endpoint
@rbac_rule_validation.action(service="keystone",
- rule="identity:create_endpoint")
+ rule="identity:create_endpoint",
+ admin_only=True)
@decorators.idempotent_id('6bdaecd4-0843-4ed6-ab64-3a57ab0cd124')
def test_create_endpoint(self):
@@ -67,7 +68,8 @@
self._create_endpoint()
@rbac_rule_validation.action(service="keystone",
- rule="identity:delete_endpoint")
+ rule="identity:delete_endpoint",
+ admin_only=True)
@decorators.idempotent_id('6bdaecd4-0843-4ed6-ab64-3a57ab0cd125')
def test_delete_endpoint(self):
@@ -81,7 +83,8 @@
self.endpoints_client.delete_endpoint(endpoint['endpoint']['id'])
@rbac_rule_validation.action(service="keystone",
- rule="identity:list_endpoints")
+ rule="identity:list_endpoints",
+ admin_only=True)
@decorators.idempotent_id('6bdaecd4-0843-4ed6-ab64-3a57ab0cd126')
def test_list_endpoints(self):
diff --git a/patrole_tempest_plugin/tests/api/identity/v2/test_projects_rbac.py b/patrole_tempest_plugin/tests/api/identity/v2/test_projects_rbac.py
index 9c4ab33..dd0af46 100644
--- a/patrole_tempest_plugin/tests/api/identity/v2/test_projects_rbac.py
+++ b/patrole_tempest_plugin/tests/api/identity/v2/test_projects_rbac.py
@@ -25,7 +25,8 @@
class IdentityProjectV2AdminRbacTest(rbac_base.BaseIdentityV2AdminRbacTest):
@rbac_rule_validation.action(service="keystone",
- rule="identity:create_project")
+ rule="identity:create_project",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-b348-080044d0d904')
def test_create_project(self):
@@ -38,7 +39,8 @@
self._create_tenant()
@rbac_rule_validation.action(service="keystone",
- rule="identity:update_project")
+ rule="identity:update_project",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-b348-080044d0d905')
def test_update_project(self):
@@ -53,7 +55,8 @@
description="Changed description")
@rbac_rule_validation.action(service="keystone",
- rule="identity:delete_project")
+ rule="identity:delete_project",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-b348-080044d0d906')
def test_delete_project(self):
@@ -67,7 +70,8 @@
self.tenants_client.delete_tenant(tenant['id'])
@rbac_rule_validation.action(service="keystone",
- rule="identity:get_project")
+ rule="identity:get_project",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-b348-080044d0d907')
def test_get_project(self):
@@ -82,7 +86,8 @@
self.tenants_client.show_tenant(tenant['id'])
@rbac_rule_validation.action(service="keystone",
- rule="identity:list_projects")
+ rule="identity:list_projects",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-b348-080044d0d908')
def test_get_all_projects(self):
@@ -94,9 +99,10 @@
self.tenants_client.list_tenants()
@rbac_rule_validation.action(service="keystone",
- rule="identity:list_user_projects")
+ rule="identity:list_user_projects",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-b348-080044d0d909')
- def test_list_users_for_tenant(self):
+ def test_list_project_users(self):
"""Get Users of a Project Test
diff --git a/patrole_tempest_plugin/tests/api/identity/v2/test_roles_rbac.py b/patrole_tempest_plugin/tests/api/identity/v2/test_roles_rbac.py
index 74a423e..aeb8d05 100644
--- a/patrole_tempest_plugin/tests/api/identity/v2/test_roles_rbac.py
+++ b/patrole_tempest_plugin/tests/api/identity/v2/test_roles_rbac.py
@@ -53,7 +53,8 @@
tenant['id'], user['id'], role['id'])
@rbac_rule_validation.action(service="keystone",
- rule="identity:create_role")
+ rule="identity:create_role",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-8674-080044d0d904')
def test_create_role(self):
@@ -66,7 +67,8 @@
self._create_role()
@rbac_rule_validation.action(service="keystone",
- rule="identity:delete_role")
+ rule="identity:delete_role",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-8674-080044d0d905')
def test_delete_role(self):
@@ -80,7 +82,8 @@
self.roles_client.delete_role(role['id'])
@rbac_rule_validation.action(service="keystone",
- rule="identity:get_role")
+ rule="identity:get_role",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-8674-080044d0d906')
def test_show_role(self):
@@ -94,7 +97,8 @@
self.roles_client.show_role(role['id'])
@rbac_rule_validation.action(service="keystone",
- rule="identity:list_roles")
+ rule="identity:list_roles",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-8674-080044d0d907')
def test_list_roles(self):
@@ -106,7 +110,8 @@
self.roles_client.list_roles()
@rbac_rule_validation.action(service="keystone",
- rule="identity:add_role_to_user")
+ rule="identity:add_role_to_user",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-8674-080044d0d908')
def test_create_role_on_project(self):
@@ -119,7 +124,8 @@
self._create_role_on_project(tenant, user, role)
@rbac_rule_validation.action(service="keystone",
- rule="identity:remove_role_from_user")
+ rule="identity:remove_role_from_user",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-8674-080044d0d909')
def test_delete_role_from_user_on_project(self):
@@ -135,7 +141,8 @@
tenant['id'], user['id'], role['id'])
@rbac_rule_validation.action(service="keystone",
- rule="identity:get_user_roles")
+ rule="identity:get_user_roles",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-8674-080044d0d90a')
def test_list_user_roles_on_project(self):
diff --git a/patrole_tempest_plugin/tests/api/identity/v2/test_services_rbac.py b/patrole_tempest_plugin/tests/api/identity/v2/test_services_rbac.py
index 6ba60fa..bc3dae8 100644
--- a/patrole_tempest_plugin/tests/api/identity/v2/test_services_rbac.py
+++ b/patrole_tempest_plugin/tests/api/identity/v2/test_services_rbac.py
@@ -30,7 +30,8 @@
cls.services_client = cls.os.identity_services_client
@rbac_rule_validation.action(service="keystone",
- rule="identity:create_service")
+ rule="identity:create_service",
+ admin_only=True)
@decorators.idempotent_id('370050f6-d271-4fb4-abc5-4de1d6dfbad2')
def test_create_service(self):
"""Create Service Test
@@ -41,7 +42,8 @@
self._create_service()
@rbac_rule_validation.action(service="keystone",
- rule="identity:delete_service")
+ rule="identity:delete_service",
+ admin_only=True)
@decorators.idempotent_id('f6c64fc3-6a1f-423e-af91-e411add3a384')
def test_delete_service(self):
"""Delete Service Test
@@ -54,7 +56,8 @@
self.services_client.delete_service(service_id)
@rbac_rule_validation.action(service="keystone",
- rule="identity:get_service")
+ rule="identity:get_service",
+ admin_only=True)
@decorators.idempotent_id('504d62bb-97d7-445e-9d6d-b1945a7c9e08')
def test_show_service(self):
"""Show Service Test
@@ -67,7 +70,8 @@
self.services_client.show_service(service_id)
@rbac_rule_validation.action(service="keystone",
- rule="identity:list_services")
+ rule="identity:list_services",
+ admin_only=True)
@decorators.idempotent_id('d7dc461d-51ad-48e0-9cd2-33add1b88de9')
def test_list_services(self):
"""List all the services
diff --git a/patrole_tempest_plugin/tests/api/identity/v2/test_users_rbac.py b/patrole_tempest_plugin/tests/api/identity/v2/test_users_rbac.py
index a94a811..da5cdce 100644
--- a/patrole_tempest_plugin/tests/api/identity/v2/test_users_rbac.py
+++ b/patrole_tempest_plugin/tests/api/identity/v2/test_users_rbac.py
@@ -23,14 +23,16 @@
class IdentityUserV2AdminRbacTest(rbac_base.BaseIdentityV2AdminRbacTest):
@rbac_rule_validation.action(service="keystone",
- rule="identity:create_user")
+ rule="identity:create_user",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-1342-080044d0d904')
def test_create_user(self):
self.rbac_utils.switch_role(self, switchToRbacRole=True)
self._create_user()
@rbac_rule_validation.action(service="keystone",
- rule="identity:update_user")
+ rule="identity:update_user",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-1342-080044d0d905')
def test_update_user(self):
user = self._create_user()
@@ -39,7 +41,8 @@
self.users_client.update_user(user['id'], email="changedUser@xyz.com")
@rbac_rule_validation.action(service="keystone",
- rule="identity:set_user_enabled")
+ rule="identity:set_user_enabled",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-1342-080044d0d9a1')
def test_update_user_enabled(self):
user = self._create_user()
@@ -48,7 +51,8 @@
self.users_client.update_user_enabled(user['id'], enabled=True)
@rbac_rule_validation.action(service="keystone",
- rule="identity:delete_user")
+ rule="identity:delete_user",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-1342-080044d0d906')
def test_delete_user(self):
user = self._create_user()
@@ -57,14 +61,16 @@
self.users_client.delete_user(user['id'])
@rbac_rule_validation.action(service="keystone",
- rule="identity:get_users")
+ rule="identity:get_users",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-1342-080044d0d907')
def test_list_users(self):
self.rbac_utils.switch_role(self, switchToRbacRole=True)
self.users_client.list_users()
@rbac_rule_validation.action(service="keystone",
- rule="identity:get_user")
+ rule="identity:get_user",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-1342-080044d0d908')
def test_show_user(self):
user = self._create_user()
@@ -73,7 +79,8 @@
self.users_client.show_user(user['id'])
@rbac_rule_validation.action(service="keystone",
- rule="identity:change_password")
+ rule="identity:change_password",
+ admin_only=True)
@decorators.idempotent_id('0f148510-63bf-11e6-1342-080044d0d909')
def test_update_user_password(self):
user = self._create_user()
diff --git a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
index 9fa0d98..705c7e7 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -71,7 +71,7 @@
def test_expect_not_found_but_raises_forbidden(self, mock_auth, mock_log):
decorator = rbac_rv.action(mock.sentinel.service,
mock.sentinel.action,
- 404)
+ expected_error_code=404)
mock_function = mock.Mock()
mock_function.side_effect = exceptions.NotFound
wrapper = decorator(mock_function)
@@ -138,7 +138,7 @@
def test_expect_not_found_and_not_allowed(self, mock_auth, mock_log):
decorator = rbac_rv.action(mock.sentinel.service,
mock.sentinel.action,
- 404)
+ expected_error_code=404)
mock_function = mock.Mock()
mock_function.side_effect = exceptions.NotFound
@@ -222,3 +222,39 @@
mock_log.error.assert_called_once_with("Please pass an expected error "
"code. Currently supported "
"codes: [403, 404]")
+
+ @mock.patch.object(rbac_rv, 'LOG', autospec=True)
+ def test_rbac_decorator_with_admin_only_and_have_permission(self,
+ mock_log):
+ CONF.set_override('rbac_test_role', 'admin', group='rbac',
+ enforce_type=True)
+ self.addCleanup(CONF.clear_override, 'rbac_test_role', group='rbac')
+
+ decorator = rbac_rv.action(mock.sentinel.service,
+ mock.sentinel.policy_rule,
+ admin_only=True)
+ wrapper = decorator(mock.Mock(side_effect=None))
+ wrapper(self.mock_args)
+
+ mock_log.info.assert_called_once_with(
+ "As admin_only is True, only admin role should be allowed to "
+ "perform the API. Skipping oslo.policy check for policy action "
+ "{0}.".format(mock.sentinel.policy_rule))
+
+ @mock.patch.object(rbac_rv, 'LOG', autospec=True)
+ def test_rbac_decorator_with_admin_only_and_lack_permission(self,
+ mock_log):
+ CONF.set_override('rbac_test_role', 'Member', group='rbac',
+ enforce_type=True)
+ self.addCleanup(CONF.clear_override, 'rbac_test_role', group='rbac')
+
+ decorator = rbac_rv.action(mock.sentinel.service,
+ mock.sentinel.policy_rule,
+ admin_only=True)
+ wrapper = decorator(mock.Mock(side_effect=exceptions.Forbidden))
+ wrapper(self.mock_args)
+
+ mock_log.info.assert_called_once_with(
+ "As admin_only is True, only admin role should be allowed to "
+ "perform the API. Skipping oslo.policy check for policy action "
+ "{0}.".format(mock.sentinel.policy_rule))