Remove already-deprecated strict_policy_check option
The option ``[patrole].strict_policy_check`` was deprecated
during the last release cycle (Queens). This was because the
option could be set to False in order to skip tests which
might result in false positives.
This PS, then, removes strict_policy_check references in the code,
updates documentation, and adds a releasenote.
Change-Id: I7f7eda39c0472bd3d70892c801fc4d14db0c0426
diff --git a/patrole_tempest_plugin/config.py b/patrole_tempest_plugin/config.py
index 8ac2a20..0077d19 100644
--- a/patrole_tempest_plugin/config.py
+++ b/patrole_tempest_plugin/config.py
@@ -27,15 +27,6 @@
cfg.BoolOpt('enable_rbac',
default=True,
help="Enables RBAC tests."),
- cfg.BoolOpt('strict_policy_check',
- default=True,
- deprecated_for_removal=True,
- deprecated_reason="""This option allows for the possibility
-of false positives. As a testing framework, Patrole should fail any test that
-passes in an invalid policy.""",
- help="""If true, throws RbacParsingException for policies which
-don't exist or are not included in the service's policy file. If false, throws
-skipException."""),
# TODO(rb560u): There needs to be support for reading these JSON files from
# other hosts. It may be possible to leverage the v3 identity policy API.
cfg.ListOpt('custom_policy_files',
diff --git a/patrole_tempest_plugin/policy_authority.py b/patrole_tempest_plugin/policy_authority.py
index 6851942..99348b9 100644
--- a/patrole_tempest_plugin/policy_authority.py
+++ b/patrole_tempest_plugin/policy_authority.py
@@ -158,6 +158,8 @@
:param string rule_name: Rule to be checked using ``oslo.policy``.
:param bool is_admin: Whether admin context is used.
+ :raises RbacParsingException: If `rule_name`` does not exist in the
+ cloud (in policy file or among registered in-code policy defaults).
"""
is_admin_context = self._is_admin_context(role)
is_allowed = self._allowed(
@@ -215,9 +217,11 @@
policy_data = mgr_policy_data
else:
error_message = (
- 'Policy file for {0} service neither found in code nor at {1}.'
- .format(service, [loc % service for loc in
- CONF.patrole.custom_policy_files])
+ 'Policy file for {0} service was not found among the '
+ 'registered in-code policies or in any of the possible policy '
+ 'files: {1}.'.format(service,
+ [loc % service for loc in
+ CONF.patrole.custom_policy_files])
)
raise rbac_exceptions.RbacParsingException(error_message)
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 97d246f..853e99b 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -163,8 +163,7 @@
LOG.error(msg)
else:
if not allowed:
- LOG.error("Role %s was allowed to perform %s",
- role, rule)
+ LOG.error("Role %s was allowed to perform %s", role, rule)
raise rbac_exceptions.RbacOverPermission(
"OverPermission: Role %s was allowed to perform %s" %
(role, rule))
@@ -200,10 +199,6 @@
:raises RbacResourceSetupFailed: If `project_id` or `user_id` are missing
from the `auth_provider` attribute in `test_obj`.
- :raises RbacParsingException: if ``[patrole] strict_policy_check`` is True
- and the ``rule`` does not exist in the system.
- :raises skipException: If ``[patrole] strict_policy_check`` is False and
- the ``rule`` does not exist in the system.
"""
try:
@@ -215,33 +210,27 @@
LOG.error(msg)
raise rbac_exceptions.RbacResourceSetupFailed(msg)
- try:
- role = CONF.patrole.rbac_test_role
- # Test RBAC against custom requirements. Otherwise use oslo.policy.
- if CONF.patrole.test_custom_requirements:
- authority = requirements_authority.RequirementsAuthority(
- CONF.patrole.custom_requirements_file, service)
- else:
- formatted_target_data = _format_extra_target_data(
- test_obj, extra_target_data)
- authority = policy_authority.PolicyAuthority(
- project_id, user_id, service,
- extra_target_data=formatted_target_data)
- is_allowed = authority.allowed(rule, role)
+ role = CONF.patrole.rbac_test_role
+ # Test RBAC against custom requirements. Otherwise use oslo.policy.
+ if CONF.patrole.test_custom_requirements:
+ authority = requirements_authority.RequirementsAuthority(
+ CONF.patrole.custom_requirements_file, service)
+ else:
+ formatted_target_data = _format_extra_target_data(
+ test_obj, extra_target_data)
+ authority = policy_authority.PolicyAuthority(
+ project_id, user_id, service,
+ extra_target_data=formatted_target_data)
+ is_allowed = authority.allowed(rule, role)
- if is_allowed:
- LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule,
- role)
- else:
- LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
- rule, role)
- return is_allowed
- except rbac_exceptions.RbacParsingException as e:
- if CONF.patrole.strict_policy_check:
- raise e
- else:
- raise testtools.TestCase.skipException(str(e))
- return False
+ if is_allowed:
+ LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule,
+ role)
+ else:
+ LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
+ rule, role)
+
+ return is_allowed
def _get_exception_type(expected_error_code=403):
diff --git a/patrole_tempest_plugin/tests/unit/test_policy_authority.py b/patrole_tempest_plugin/tests/unit/test_policy_authority.py
index d2074e7..3e2cc4c 100644
--- a/patrole_tempest_plugin/tests/unit/test_policy_authority.py
+++ b/patrole_tempest_plugin/tests/unit/test_policy_authority.py
@@ -387,12 +387,11 @@
policy_authority.PolicyAuthority,
None, None, 'test_service')
- expected_error = \
- 'Policy file for {0} service neither found in code '\
- 'nor at {1}.'.format(
- 'test_service',
- [CONF.patrole.custom_policy_files[0] % 'test_service'])
-
+ expected_error = (
+ 'Policy file for {0} service was not found among the registered '
+ 'in-code policies or in any of the possible policy files: {1}.'
+ .format('test_service',
+ [CONF.patrole.custom_policy_files[0] % 'test_service']))
self.assertIn(expected_error, str(e))
@mock.patch.object(policy_authority, 'json', autospec=True)
@@ -436,7 +435,8 @@
None, None, 'tenant_rbac_policy')
expected_error = (
- 'Policy file for {0} service neither found in code nor at {1}.'
+ 'Policy file for {0} service was not found among the registered '
+ 'in-code policies or in any of the possible policy files: {1}.'
.format('tenant_rbac_policy', [CONF.patrole.custom_policy_files[0]
% 'tenant_rbac_policy']))
self.assertIn(expected_error, str(e))
@@ -485,9 +485,10 @@
policy_parser.policy_files['test_service'])
def test_discover_policy_files_with_no_valid_files(self):
- expected_error = ("Policy file for test_service service neither found "
- "in code nor at %s." %
- [self.conf_policy_path % 'test_service'])
+ expected_error = (
+ 'Policy file for {0} service was not found among the registered '
+ 'in-code policies or in any of the possible policy files: {1}.'
+ .format('test_service', [self.conf_policy_path % 'test_service']))
e = self.assertRaises(rbac_exceptions.RbacParsingException,
policy_authority.PolicyAuthority,
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 82f0428..85547e1 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -13,7 +13,6 @@
# under the License.
import mock
-import testtools
from tempest.lib import exceptions
from tempest import manager
@@ -297,12 +296,8 @@
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
def test_invalid_policy_rule_raises_parsing_exception(
self, mock_authority):
- """Test that invalid policy action causes test to be fail with
- ``[patrole] strict_policy_check`` set to True.
+ """Test that invalid policy action causes test to raise an exception.
"""
- self.useFixture(
- fixtures.ConfPatcher(strict_policy_check=True, group='patrole'))
-
mock_authority.PolicyAuthority.return_value.allowed.\
side_effect = rbac_exceptions.RbacParsingException
@@ -319,30 +314,6 @@
mock.sentinel.service, extra_target_data={})
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
- def test_invalid_policy_rule_raises_skip_exception(
- self, mock_authority):
- """Test that invalid policy action causes test to be skipped with
- ``[patrole] strict_policy_check`` set to False.
- """
- self.useFixture(
- fixtures.ConfPatcher(strict_policy_check=False, group='patrole'))
-
- mock_authority.PolicyAuthority.return_value.allowed.side_effect = (
- rbac_exceptions.RbacParsingException)
-
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
- def test_policy(*args):
- pass
-
- error_re = 'Attempted to test an invalid policy file or action'
- self.assertRaisesRegex(testtools.TestCase.skipException, error_re,
- test_policy, self.mock_test_args)
-
- mock_authority.PolicyAuthority.assert_called_once_with(
- mock.sentinel.project_id, mock.sentinel.user_id,
- mock.sentinel.service, extra_target_data={})
-
- @mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
def test_get_exception_type_404(self, _):
"""Test that getting a 404 exception type returns NotFound."""
expected_exception = exceptions.NotFound