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

4 files changed, 22 insertions, 14 deletions

diff --git a/nova/context.py b/nova/context.py
index 4901f1de1b..da1024efbb 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 cd5273547b..4f211872b3 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 c640477ab8..3fde2cb5c3 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 b91355da5e..2bc173825a 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)