Merge "fix: Use superior oslo.policy authorize over enforce"

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):

armada/tests/unit/common/test_policy.py

@@ -29,8 +29,8 @@ class PolicyTestCase(testtools.TestCase):
         super(PolicyTestCase, self).setUp()
         self.rules = {
             "true": [],
-            "example:allowed": [],
+            "armada:validate_manifest": [],
-            "example:disallowed": [["false:false"]]
+            "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')
+    @mock.patch.object(policy, 'LOG', autospec=True)
-    def test_enforce_nonexistent_action(self, mock_ctx):
+    @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')
+    @mock.patch('armada.api.ArmadaRequestContext', autospec=True)
-    def test_enforce_good_action(self, mock_ctx):
+    def test_enforce_allowed_action(self, mock_ctx):
-        action = "example:allowed"
+        """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')
+    @mock.patch('armada.api.ArmadaRequestContext', autospec=True)
-    def test_enforce_bad_action(self, mock_ctx):
+    def test_enforce_disallowed_action(self, mock_ctx):
-        action = "example:disallowed"
+        """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,