From 855396299771a290f2de9918a91ced4925d98238 Mon Sep 17 00:00:00 2001
From: Goutham Pacha Ravi <gouthampravi@gmail.com>
Date: Thu, 25 Feb 2021 23:41:57 -0800
Subject: [PATCH] Clean up some policy code
oslo policy handles the mapping of
credentials from a context object to values
that oslo policy cares about. This mapping
includes some deprecations and compatibility
handling code that we must take advantage of [1].
So, stop mapping context to policy values
on our end, and rely on oslo.policy handling
this for us.
enforce and authorize methods in policy.py
do the same thing, but with a subtle
difference. The "enforce" method doesn't
raise errors when unregistered policies are
invoked - this shouldn't ever be the case
for any policies written/maintained within
manila - however, we support API extensions
and don't dictate what must be done there. So
add docstrings to clarify that we shouldn't
invoke enforce, ever.
Also handle InvalidPolicyScope exceptions
and raise the oslo.policy library version
since some test enhancements have been
committed in the latest version.
[1] https://opendev.org/openstack/oslo.policy/src/commit/d3185debdbb7aa254ad4ce2acc38c9a34908e44a/oslo_policy/policy.py#L1077-L1096
Change-Id: I069bf7143d6ff66b3dcdc34c9b52d48f5808481b
Signed-off-by: Goutham Pacha Ravi <gouthampravi@gmail.com>
---
lower-constraints.txt | 2 +-
manila/api/extensions.py | 9 +++----
manila/policy.py | 50 +++++++++++++++++++++++--------------
manila/tests/test_policy.py | 35 ++++++++++++++++++++++++++
requirements.txt | 2 +-
5 files changed, 71 insertions(+), 27 deletions(-)
diff --git a/lower-constraints.txt b/lower-constraints.txt
index 57f22de724..7486b72fbd 100644
--- a/lower-constraints.txt
+++ b/lower-constraints.txt
@@ -64,7 +64,7 @@ oslo.i18n==5.0.1
oslo.log==4.4.0
oslo.messaging==12.5.0
oslo.middleware==4.1.1
-oslo.policy==3.6.0
+oslo.policy==3.6.2
oslo.reports==2.2.0
oslo.rootwrap==6.2.0
oslo.serialization==4.0.1
diff --git a/manila/api/extensions.py b/manila/api/extensions.py
index bea35bec94..6bbdca258e 100644
--- a/manila/api/extensions.py
+++ b/manila/api/extensions.py
@@ -22,10 +22,9 @@ from oslo_utils import importutils
import webob.dec
import webob.exc
-import manila.api.openstack
from manila.api.openstack import wsgi
from manila import exception
-import manila.policy
+from manila import policy
CONF = cfg.CONF
LOG = log.getLogger(__name__)
@@ -332,12 +331,10 @@ def load_standard_extensions(ext_mgr, logger, path, package, ext_list=None):
def extension_authorizer(api_name, extension_name):
def authorize(context, target=None, action=None):
- if target is None:
- target = {'project_id': context.project_id,
- 'user_id': context.user_id}
+ target = target or policy.default_target(context)
if action is None:
act = '%s_extension:%s' % (api_name, extension_name)
else:
act = '%s_extension:%s:%s' % (api_name, extension_name, action)
- manila.policy.enforce(context, act, target)
+ policy.enforce(context, act, target)
return authorize
diff --git a/manila/policy.py b/manila/policy.py
index 05ac7be479..f0501999a0 100644
--- a/manila/policy.py
+++ b/manila/policy.py
@@ -69,6 +69,10 @@ def init(rules=None, use_conf=True):
def enforce(context, action, target, do_raise=True):
"""Verifies that the action is valid on the target in this context.
+ **IMPORTANT** ONLY for use in API extensions. This method ignores
+ unregistered rules and applies a default rule on them; there should
+ be no unregistered rules in first party manila APIs.
+
:param context: manila context
:param action: string representing the action to be checked,
this should be colon separated for clarity.
@@ -88,12 +92,15 @@ def enforce(context, action, target, do_raise=True):
"""
init()
- return _ENFORCER.enforce(action,
- target,
- context.to_policy_values(),
- do_raise=do_raise,
- exc=exception.PolicyNotAuthorized,
- action=action)
+ try:
+ return _ENFORCER.enforce(action,
+ target,
+ context,
+ do_raise=do_raise,
+ exc=exception.PolicyNotAuthorized,
+ action=action)
+ except policy.InvalidScope:
+ raise exception.PolicyNotAuthorized(action=action)
def set_rules(rules, overwrite=True, use_conf=False):
@@ -165,32 +172,41 @@ def authorize(context, action, target, do_raise=True, exc=None):
do_raise is False.
"""
init()
- credentials = context.to_policy_values()
if not exc:
exc = exception.PolicyNotAuthorized
+ target = target or default_target(context)
+
try:
- result = _ENFORCER.authorize(action, target, credentials,
+ result = _ENFORCER.authorize(action, target, context,
do_raise=do_raise, exc=exc, action=action)
except policy.PolicyNotRegistered:
with excutils.save_and_reraise_exception():
LOG.exception('Policy not registered')
+ except policy.InvalidScope:
+ raise exception.PolicyNotAuthorized(action=action)
except Exception:
with excutils.save_and_reraise_exception():
+ msg_args = {
+ 'action': action,
+ 'credentials': context.to_policy_values(),
+ }
LOG.debug('Policy check for %(action)s failed with credentials '
- '%(credentials)s',
- {'action': action, 'credentials': credentials})
+ '%(credentials)s', msg_args)
return result
+def default_target(context):
+ return {'project_id': context.project_id, 'user_id': context.user_id}
+
+
def check_is_admin(context):
"""Whether or not user is admin according to policy setting.
"""
init()
-
- credentials = context.to_policy_values()
- target = credentials
- return _ENFORCER.authorize('context_is_admin', target, credentials)
+ # the target is user-self
+ target = default_target(context)
+ return _ENFORCER.authorize('context_is_admin', target, context)
def wrap_check_policy(resource):
@@ -206,10 +222,6 @@ def wrap_check_policy(resource):
def check_policy(context, resource, action, target_obj=None, do_raise=True):
- target = {
- 'project_id': context.project_id,
- 'user_id': context.user_id,
- }
- target.update(target_obj or {})
+ target = target_obj or default_target(context)
_action = '%s:%s' % (resource, action)
return authorize(context, _action, target, do_raise=do_raise)
diff --git a/manila/tests/test_policy.py b/manila/tests/test_policy.py
index a0d8bb4569..cab315f41d 100644
--- a/manila/tests/test_policy.py
+++ b/manila/tests/test_policy.py
@@ -15,6 +15,7 @@
"""Test of Policy Engine For Manila."""
+import ddt
from oslo_config import cfg
from oslo_policy import policy as common_policy
@@ -26,6 +27,7 @@ from manila import test
CONF = cfg.CONF
+@ddt.ddt
class PolicyTestCase(test.TestCase):
def setUp(self):
super(PolicyTestCase, self).setUp()
@@ -97,8 +99,41 @@ class PolicyTestCase(test.TestCase):
policy.authorize(admin_context, lowercase_action, self.target)
policy.authorize(admin_context, uppercase_action, self.target)
+ @ddt.data('enforce', 'authorize')
+ def test_authorize_properly_handles_invalid_scope_exception(self, method):
+ self.fixture.config(enforce_scope=True, group='oslo_policy')
+ project_context = context.RequestContext(project_id='fake-project-id',
+ roles=['bar'])
+ policy.reset()
+ policy.init()
+ rule = common_policy.RuleDefault('foo', 'role:bar',
+ scope_types=['system'])
+ policy._ENFORCER.register_defaults([rule])
+
+ self.assertRaises(exception.PolicyNotAuthorized,
+ getattr(policy, method),
+ project_context, 'foo', {})
+
+ @ddt.data('enforce', 'authorize')
+ def test_authorize_does_not_raise_forbidden(self, method):
+ self.fixture.config(enforce_scope=False, group='oslo_policy')
+ project_context = context.RequestContext(project_id='fake-project-id',
+ roles=['bar'])
+ policy.reset()
+ policy.init()
+ rule = common_policy.RuleDefault('foo', 'role:bar',
+ scope_types=['system'])
+ policy._ENFORCER.register_defaults([rule])
+
+ self.assertTrue(getattr(policy, method)(project_context, 'foo', {}))
+
class DefaultPolicyTestCase(test.TestCase):
+ """This test case calls into the "enforce" method in policy
+
+ enforce() in contrast with authorize() allows "default" rules to apply
+ to policies that have not been registered.
+ """
def setUp(self):
super(DefaultPolicyTestCase, self).setUp()
diff --git a/requirements.txt b/requirements.txt
index b9b71ca5e1..638607c8bc 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -17,7 +17,7 @@ oslo.i18n>=5.0.1 # Apache-2.0
oslo.log>=4.4.0 # Apache-2.0
oslo.messaging>=12.5.0 # Apache-2.0
oslo.middleware>=4.1.1 # Apache-2.0
-oslo.policy>=3.6.0 # Apache-2.0
+oslo.policy>=3.6.2 # Apache-2.0
oslo.reports>=2.2.0 # Apache-2.0
oslo.rootwrap>=6.2.0 # Apache-2.0
oslo.serialization>=4.0.1 # Apache-2.0