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

@@ -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

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)
 
 

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

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)