Merge "Move default policy target"

Author: Zuul

nova/context.py

@@ -228,9 +228,8 @@ def can(self, action, target=None, fatal=True):
         :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.
 
@@ -240,19 +239,13 @@ def can(self, action, target=None, fatal=True):
         :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:
             if fatal:
                 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 @@ def test_can(self, mock_authorize):
 
         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 @@ def test_templatized_authorization(self):
         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)