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/doc/source/configuration.rst b/doc/source/configuration.rst
index 8ec0013..ce799ad 100644
--- a/doc/source/configuration.rst
+++ b/doc/source/configuration.rst
@@ -35,21 +35,6 @@
Given the value of ``enable_rbac``, enables or disables Patrole tests. If
``enable_rbac`` is ``False``, then Patrole tests are skipped.
-Strict Policy Check
--------------------
-
-Currently, many services define their "default" rule to be "anyone allowed".
-If a policy action is not explicitly defined in a policy file, then
-``oslo.policy`` will fall back to the "default" rule. This implies that if
-there's a typo in a policy action specified in a Patrole test, ``oslo.policy``
-can report that the ``rbac_test_role`` will be able to perform the
-non-existent policy action. For a testing framework, this is undesirable
-behavior.
-
-Hence, ``strict_policy_check``, if ``True``, will throw an error in the event
-that a non-existent or bogus policy action is passed to a Patrole test. If
-``False``, however, a ``self.skipException`` will be raised.
-
Custom Policy Files
-------------------
@@ -70,4 +55,3 @@
Patrole currently does not support policy files located on a host different
than the one on which it is running.
-..
diff --git a/etc/patrole.conf.sample b/etc/patrole.conf.sample
index cafdf8a..ed2b07c 100644
--- a/etc/patrole.conf.sample
+++ b/etc/patrole.conf.sample
@@ -14,18 +14,6 @@
# Enables RBAC tests. (boolean value)
#enable_rbac = true
-# DEPRECATED: If true, throws RbacParsingException for policies which
-# don't exist or are not included in the service's policy file. If
-# false, throws
-# skipException. (boolean value)
-# This option is deprecated for removal.
-# Its value may be silently ignored in the future.
-# 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.
-#strict_policy_check = true
-
# List of the paths to search for policy files. Each
# policy path assumes that the service name is included in the path
# once. Also
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
diff --git a/releasenotes/notes/remove-strict-policy-check-480e3d664f7b2d96.yaml b/releasenotes/notes/remove-strict-policy-check-480e3d664f7b2d96.yaml
new file mode 100644
index 0000000..37c5e1e
--- /dev/null
+++ b/releasenotes/notes/remove-strict-policy-check-480e3d664f7b2d96.yaml
@@ -0,0 +1,6 @@
+---
+upgrade:
+ - |
+ The ``[patrole].strict_policy_check`` was deprecated during the Queens
+ release cycle. It is removed in this release cycle because Patrole should
+ always fail on invalid policies.