From 06c0fd4fd2f1616d9b36c9d6fa934f3a45527ed7 Mon Sep 17 00:00:00 2001 From: John Date: Sun, 5 May 2019 15:27:46 +0100 Subject: [PATCH] Move default policy target Move default target from context.can() into policy.authorize() so it is easier to unit test the context behaviour. This was not done originally due to this meaning placement avoided the strange default target, but that is no longer required. Change-Id: I23c433dcd459e7f930ac2eb8a3583c857836cae2 --- nova/context.py | 11 ++--------- nova/policy.py | 18 +++++++++++++++--- nova/tests/unit/test_context.py | 3 +-- nova/tests/unit/test_policy.py | 4 ++++ 4 files changed, 22 insertions(+), 14 deletions(-) diff --git a/nova/context.py b/nova/context.py index 4901f1de1bf5..da1024efbbd2 100644 --- a/nova/context.py +++ b/nova/context.py @@ -238,9 +238,8 @@ class RequestContext(context.RequestContext): :param action: string representing the action to be checked. :param target: dictionary representing the object of the action for object creation this should be a dictionary representing the - location of the object e.g. ``{'project_id': context.project_id}``. - If None, then this default target will be considered: - {'project_id': self.project_id, 'user_id': self.user_id} + location of the object + e.g. ``{'project_id': instance.project_id}``. :param fatal: if False, will return False when an exception.Forbidden occurs. @@ -250,9 +249,6 @@ class RequestContext(context.RequestContext): :return: returns a non-False value (not necessarily "True") if authorized and False if not authorized and fatal is False. """ - if target is None: - target = self.default_target() - try: return policy.authorize(self, action, target) except exception.Forbidden: @@ -260,9 +256,6 @@ class RequestContext(context.RequestContext): raise return False - def default_target(self): - return {'project_id': self.project_id, 'user_id': self.user_id} - def to_policy_values(self): policy = super(RequestContext, self).to_policy_values() policy['is_admin'] = self.is_admin diff --git a/nova/policy.py b/nova/policy.py index cd5273547b21..4f211872b3ff 100644 --- a/nova/policy.py +++ b/nova/policy.py @@ -122,7 +122,7 @@ def set_rules(rules, overwrite=True, use_conf=False): _ENFORCER.set_rules(rules, overwrite, use_conf) -def authorize(context, action, target, do_raise=True, exc=None): +def authorize(context, action, target=None, do_raise=True, exc=None): """Verifies that the action is valid on the target in this context. :param context: nova context @@ -133,7 +133,9 @@ def authorize(context, action, target, do_raise=True, exc=None): ``volume:attach_volume`` :param target: dictionary representing the object of the action for object creation this should be a dictionary representing the - location of the object e.g. ``{'project_id': context.project_id}`` + location of the object e.g. ``{'project_id': instance.project_id}`` + If None, then this default target will be considered: + {'project_id': self.project_id, 'user_id': self.user_id} :param do_raise: if True (the default), raises PolicyNotAuthorized; if False, returns False :param exc: Class of the exception to raise if the check fails. @@ -154,6 +156,12 @@ def authorize(context, action, target, do_raise=True, exc=None): credentials = context.to_policy_values() if not exc: exc = exception.PolicyNotAuthorized + + # Legacy fallback for emtpy target from context.can() + # should be removed once we improve testing and scope checks + if target is None: + target = default_target(context) + try: result = _ENFORCER.authorize(action, target, credentials, do_raise=do_raise, exc=exc, action=action) @@ -168,6 +176,10 @@ def authorize(context, action, target, do_raise=True, exc=None): 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 roles contains 'admin' role according to policy setting. @@ -176,7 +188,7 @@ def check_is_admin(context): init() # the target is user-self credentials = context.to_policy_values() - target = context.default_target() + target = default_target(context) return _ENFORCER.authorize('context_is_admin', target, credentials) diff --git a/nova/tests/unit/test_context.py b/nova/tests/unit/test_context.py index c640477ab802..3fde2cb5c39b 100644 --- a/nova/tests/unit/test_context.py +++ b/nova/tests/unit/test_context.py @@ -216,8 +216,7 @@ class ContextTestCase(test.NoDBTestCase): self.assertTrue(result) mock_authorize.assert_called_once_with( - ctxt, mock.sentinel.rule, - {'project_id': ctxt.project_id, 'user_id': ctxt.user_id}) + ctxt, mock.sentinel.rule, None) @mock.patch.object(context.policy, 'authorize') def test_can_fatal(self, mock_authorize): diff --git a/nova/tests/unit/test_policy.py b/nova/tests/unit/test_policy.py index b91355da5efc..2bc173825ae6 100644 --- a/nova/tests/unit/test_policy.py +++ b/nova/tests/unit/test_policy.py @@ -134,6 +134,10 @@ class PolicyTestCase(test.NoDBTestCase): target_not_mine = {'project_id': 'another'} action = "example:my_file" policy.authorize(self.context, action, target_mine) + # check we fallback to context.project_id + # TODO(johngarbutt): longer term we need to remove this and make + # the target a required param. + policy.authorize(self.context, action) self.assertRaises(exception.PolicyNotAuthorized, policy.authorize, self.context, action, target_not_mine)