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,