Merge "Remove already-deprecated strict_policy_check option"
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 1bd3c08..01a9981 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.