From e573cf1ad5fc90d8966f81b8bd842a1aedb13af9 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Fri, 12 Oct 2018 18:15:42 +0100 Subject: [PATCH] fix: Use superior oslo.policy authorize over enforce `oslo.policy` supports both enforce and authorize. authorize is stricter because it'll raise an exception if the policy action is not found in the list of registered rules. This means that attempting to enforce anything not found in ``armada.common.policies`` will error out with a 'Policy not registered' message and 403 status code. This problem manifests itself through such cases: [0] Please reference the oslo.policy docs on authorize [1] and enforce [2] to better understand the discrepancy between the two. [0] https://review.openstack.org/#/c/610117/1 [1] https://github.com/openstack/oslo.policy/blob/feac3dcbfeef8e1d28ff40e2c3df9bf36d88bc9f/oslo_policy/policy.py#L960 [2] https://github.com/openstack/oslo.policy/blob/feac3dcbfeef8e1d28ff40e2c3df9bf36d88bc9f/oslo_policy/policy.py#L792 Change-Id: I5b0a28a2b5fb4dff150f13a56013a7a9b694c756 --- armada/common/policy.py | 26 ++++++++++++++++++++++++- armada/tests/unit/common/__init__.py | 0 armada/tests/unit/common/test_policy.py | 26 +++++++++++++++---------- 3 files changed, 41 insertions(+), 11 deletions(-) create mode 100644 armada/tests/unit/common/__init__.py diff --git a/armada/common/policy.py b/armada/common/policy.py index 552fd427..5150543e 100644 --- a/armada/common/policy.py +++ b/armada/common/policy.py @@ -13,12 +13,15 @@ import functools from oslo_config import cfg +from oslo_log import log as logging from oslo_policy import policy +from oslo_utils import excutils from armada.common import policies from armada.exceptions import base_exception as exc CONF = cfg.CONF +LOG = logging.getLogger(__name__) _ENFORCER = None @@ -41,9 +44,30 @@ def _enforce_policy(action, target, credentials, do_raise=True): if do_raise: extras.update(exc=exc.ActionForbidden, do_raise=do_raise) - _ENFORCER.enforce(action, target, credentials.to_policy_view(), **extras) + # `oslo.policy` supports both enforce and authorize. authorize is + # stricter because it'll raise an exception if the policy action is + # not found in the list of registered rules. This means that attempting + # to enforce anything not found in ``armada.common.policies`` will error + # out with a 'Policy not registered' message and 403 status code. + try: + _ENFORCER.authorize(action, target, credentials.to_policy_view(), + **extras) + except policy.PolicyNotRegistered as e: + LOG.exception('Policy not registered for %(action)s', + {'action': action}) + raise exc.ActionForbidden() + except Exception as e: + with excutils.save_and_reraise_exception(): + LOG.debug( + 'Policy check for %(action)s failed with credentials ' + '%(credentials)s', { + 'action': action, + 'credentials': credentials + }) +# NOTE(felipemonteiro): This naming is OK. It's just kept around for legacy +# reasons. What's important is that authorize is used above. def enforce(rule): def decorator(func): diff --git a/armada/tests/unit/common/__init__.py b/armada/tests/unit/common/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/armada/tests/unit/common/test_policy.py b/armada/tests/unit/common/test_policy.py index 30514592..82219fd9 100644 --- a/armada/tests/unit/common/test_policy.py +++ b/armada/tests/unit/common/test_policy.py @@ -29,8 +29,8 @@ class PolicyTestCase(testtools.TestCase): super(PolicyTestCase, self).setUp() self.rules = { "true": [], - "example:allowed": [], - "example:disallowed": [["false:false"]] + "armada:validate_manifest": [], + "armada:create_endpoints": [["false:false"]] } self.useFixture(fixtures.RealPolicyFixture(False)) self._set_rules() @@ -41,24 +41,30 @@ class PolicyTestCase(testtools.TestCase): curr_rules = common_policy.Rules.from_dict(self.rules) policy._ENFORCER.set_rules(curr_rules) - @mock.patch('armada.api.ArmadaRequestContext') - def test_enforce_nonexistent_action(self, mock_ctx): + @mock.patch.object(policy, 'LOG', autospec=True) + @mock.patch('armada.api.ArmadaRequestContext', autospec=True) + def test_enforce_nonexistent_action(self, mock_ctx, mock_log): + """Validates that unregistered default policy throws exception.""" action = "example:nope" mock_ctx.to_policy_view.return_value = self.credentials self.assertRaises(exc.ActionForbidden, policy._enforce_policy, action, self.target, mock_ctx) + mock_log.exception.assert_called_once_with( + 'Policy not registered for %(action)s', {'action': 'example:nope'}) - @mock.patch('armada.api.ArmadaRequestContext') - def test_enforce_good_action(self, mock_ctx): - action = "example:allowed" + @mock.patch('armada.api.ArmadaRequestContext', autospec=True) + def test_enforce_allowed_action(self, mock_ctx): + """Validates that allowed policy action can be performed.""" + action = "armada:validate_manifest" mock_ctx.to_policy_view.return_value = self.credentials policy._enforce_policy(action, self.target, mock_ctx) - @mock.patch('armada.api.ArmadaRequestContext') - def test_enforce_bad_action(self, mock_ctx): - action = "example:disallowed" + @mock.patch('armada.api.ArmadaRequestContext', autospec=True) + def test_enforce_disallowed_action(self, mock_ctx): + """Validates that disallowed policy action cannot be performed.""" + action = "armada:create_endpoints" mock_ctx.to_policy_view.return_value = self.credentials self.assertRaises(exc.ActionForbidden, policy._enforce_policy, action,