Merge "Pass the actual target in server group policy"

5 files changed, 117 insertions, 29 deletions

diff --git a/nova/api/openstack/compute/server_groups.py b/nova/api/openstack/compute/server_groups.py
index 059103017e..c1fd2634be 100644
--- a/nova/api/openstack/compute/server_groups.py
+++ b/nova/api/openstack/compute/server_groups.py
@@ -42,12 +42,6 @@ CONF = nova.conf.CONF
 GROUP_POLICY_OBJ_MICROVERSION = "2.64"
 
 
-def _authorize_context(req, action):
-    context = req.environ['nova.context']
-    context.can(sg_policies.POLICY_ROOT % action)
-    return context
-
-
 def _get_not_deleted(context, uuids):
     mappings = objects.InstanceMappingList.get_by_instance_uuids(
         context, uuids)
@@ -126,9 +120,11 @@ class ServerGroupController(wsgi.Controller):
     @wsgi.expected_errors(404)
     def show(self, req, id):
         """Return data about the given server group."""
-        context = _authorize_context(req, 'show')
+        context = req.environ['nova.context']
         try:
             sg = objects.InstanceGroup.get_by_uuid(context, id)
+            context.can(sg_policies.POLICY_ROOT % 'show',
+                        target={'project_id': sg.project_id})
         except nova.exception.InstanceGroupNotFound as e:
             raise webob.exc.HTTPNotFound(explanation=e.format_message())
         return {'server_group': self._format_server_group(context, sg, req)}
@@ -137,9 +133,11 @@ class ServerGroupController(wsgi.Controller):
     @wsgi.expected_errors(404)
     def delete(self, req, id):
         """Delete a server group."""
-        context = _authorize_context(req, 'delete')
+        context = req.environ['nova.context']
         try:
             sg = objects.InstanceGroup.get_by_uuid(context, id)
+            context.can(sg_policies.POLICY_ROOT % 'delete',
+                        target={'project_id': sg.project_id})
         except nova.exception.InstanceGroupNotFound as e:
             raise webob.exc.HTTPNotFound(explanation=e.format_message())
         try:
@@ -152,9 +150,24 @@ class ServerGroupController(wsgi.Controller):
     @validation.query_schema(schema.server_groups_query_param, '2.0', '2.74')
     def index(self, req):
         """Returns a list of server groups."""
-        context = _authorize_context(req, 'index')
+        context = req.environ['nova.context']
         project_id = context.project_id
+        # NOTE(gmann): Using context's project_id as target here so
+        # that when we remove the default target from policy class,
+        # it does not fail if user requesting operation on for their
+        # own server group.
+        context.can(sg_policies.POLICY_ROOT % 'index',
+                    target={'project_id': project_id})
         if 'all_projects' in req.GET and context.is_admin:
+            # TODO(gmann): Remove the is_admin check in the above condition
+            # so that the below policy can raise error if not allowed.
+            # In existing behavior, if non-admin users requesting
+            # all projects server groups they do not get error instead
+            # get their own server groups. Once we switch to policy
+            # new defaults completly then we can remove the above check.
+            # Until then, let's keep the old behaviour.
+            context.can(sg_policies.POLICY_ROOT % 'index:all_projects',
+                        target={})
             sgs = objects.InstanceGroupList.get_all(context)
         else:
             sgs = objects.InstanceGroupList.get_by_project_id(
@@ -171,11 +184,13 @@ class ServerGroupController(wsgi.Controller):
     @validation.schema(schema.create_v264, GROUP_POLICY_OBJ_MICROVERSION)
     def create(self, req, body):
         """Creates a new server group."""
-        context = _authorize_context(req, 'create')
-
+        context = req.environ['nova.context']
+        project_id = context.project_id
+        context.can(sg_policies.POLICY_ROOT % 'create',
+                    target={'project_id': project_id})
         try:
             objects.Quotas.check_deltas(context, {'server_groups': 1},
-                                        context.project_id, context.user_id)
+                                        project_id, context.user_id)
         except nova.exception.OverQuota:
             msg = _("Quota exceeded, too many server groups.")
             raise exc.HTTPForbidden(explanation=msg)
@@ -201,7 +216,7 @@ class ServerGroupController(wsgi.Controller):
             sg = objects.InstanceGroup(context, policy=policies[0])
         try:
             sg.name = vals.get('name')
-            sg.project_id = context.project_id
+            sg.project_id = project_id
             sg.user_id = context.user_id
             sg.create()
         except ValueError as e:
@@ -214,7 +229,7 @@ class ServerGroupController(wsgi.Controller):
         if CONF.quota.recheck_quota:
             try:
                 objects.Quotas.check_deltas(context, {'server_groups': 0},
-                                            context.project_id,
+                                            project_id,
                                             context.user_id)
             except nova.exception.OverQuota:
                 sg.destroy()
diff --git a/nova/policies/server_groups.py b/nova/policies/server_groups.py
index f678213617..e9b95d316d 100644
--- a/nova/policies/server_groups.py
+++ b/nova/policies/server_groups.py
@@ -59,6 +59,18 @@ server_groups_policies = [
         scope_types=['system', 'project']
     ),
     policy.DocumentedRuleDefault(
+        name=POLICY_ROOT % 'index:all_projects',
+        check_str=base.RULE_ADMIN_API,
+        description="List all server groups for all projects",
+        operations=[
+            {
+                'path': '/os-server-groups',
+                'method': 'GET'
+            }
+        ],
+        scope_types=['system']
+    ),
+    policy.DocumentedRuleDefault(
         name=POLICY_ROOT % 'show',
         check_str=base.RULE_ADMIN_OR_OWNER,
         description="Show details of a server group",
diff --git a/nova/tests/unit/fake_policy.py b/nova/tests/unit/fake_policy.py
index b9aeaa2882..a2ff6f7c6e 100644
--- a/nova/tests/unit/fake_policy.py
+++ b/nova/tests/unit/fake_policy.py
@@ -118,6 +118,7 @@ policy_data = """
     "os_compute_api:os-server-tags:delete_all": "",
     "os_compute_api:os-server-groups:show": "",
     "os_compute_api:os-server-groups:index": "",
+    "os_compute_api:os-server-groups:index:all_projects": "",
     "os_compute_api:os-server-groups:create": "",
     "os_compute_api:os-server-groups:delete": "",
     "os_compute_api:os-services:list": "",
diff --git a/nova/tests/unit/policies/test_server_groups.py b/nova/tests/unit/policies/test_server_groups.py
index 05e0084cdd..babdab3f75 100644
--- a/nova/tests/unit/policies/test_server_groups.py
+++ b/nova/tests/unit/policies/test_server_groups.py
@@ -10,10 +10,12 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import fixtures
 import mock
 from oslo_utils.fixture import uuidsentinel as uuids
 
 from nova.api.openstack.compute import server_groups
+from nova import objects
 from nova.policies import server_groups as policies
 from nova.tests.unit.api.openstack import fakes
 from nova.tests.unit.policies import base
@@ -32,32 +34,76 @@ class ServerGroupPolicyTest(base.BasePolicyTest):
         self.controller = server_groups.ServerGroupController()
         self.req = fakes.HTTPRequest.blank('')
 
-        # Policy is admin_or_owner but we do not pass the project id
-        # in policy enforcement to check the ownership. project id
-        # is nothing but of server group for which request is made. So
-        # let's keep it as it is and with new defaults and sceop enbled,
-        # these can be authorized to meanigful roles.
+        self.mock_get = self.useFixture(
+            fixtures.MockPatch('nova.objects.InstanceGroup.get_by_uuid')).mock
+        self.sg = [objects.InstanceGroup(
+                    uuid=uuids.fake_id, name='fake',
+                    project_id=self.project_id, user_id='u1',
+                    policies=[], members=[]),
+                   objects.InstanceGroup(
+                    uuid=uuids.fake_id, name='fake2', project_id='proj2',
+                    user_id='u2', policies=[], members=[])]
+        self.mock_get.return_value = self.sg[0]
+
+        # Check that admin or and owner is able to get and delete
+        # the server group.
         self.admin_or_owner_authorized_contexts = [
             self.legacy_admin_context, self.system_admin_context,
             self.project_admin_context, self.project_member_context,
+            self.project_reader_context, self.project_foo_context]
+        # Check that non-admin/owner is not able to get and delete
+        # the server group.
+        self.admin_or_owner_unauthorized_contexts = [
+            self.system_member_context, self.system_reader_context,
+            self.system_foo_context,
+            self.other_project_member_context
+        ]
+
+        # Check that everyone is able to list and create
+        # theie own server group.
+        self.everyone_authorized_contexts = [
+            self.legacy_admin_context, self.system_admin_context,
+            self.project_admin_context, self.project_member_context,
             self.project_reader_context, self.project_foo_context,
             self.system_member_context, self.system_reader_context,
             self.system_foo_context,
             self.other_project_member_context]
-        self.admin_or_owner_unauthorized_contexts = [
+        self.everyone_unauthorized_contexts = [
         ]
 
-    @mock.patch('nova.objects.InstanceGroupList.get_all')
+    @mock.patch('nova.objects.InstanceGroupList.get_by_project_id')
     def test_index_server_groups_policy(self, mock_get):
         rule_name = policies.POLICY_ROOT % 'index'
-        self.common_policy_check(self.admin_or_owner_authorized_contexts,
-                                 self.admin_or_owner_unauthorized_contexts,
+        self.common_policy_check(self.everyone_authorized_contexts,
+                                 self.everyone_unauthorized_contexts,
                                  rule_name,
                                  self.controller.index,
                                  self.req)
 
-    @mock.patch('nova.objects.InstanceGroup.get_by_uuid')
-    def test_show_server_groups_policy(self, mock_get):
+    @mock.patch('nova.objects.InstanceGroupList.get_all')
+    def test_index_all_project_server_groups_policy(self, mock_get_all):
+        mock_get_all.return_value = objects.InstanceGroupList(objects=self.sg)
+        # 'index' policy is checked before 'index:all_projects' so
+        # we have to allow it for everyone otherwise it will fail for
+        # unauthorized contexts here.
+        rule = policies.POLICY_ROOT % 'index'
+        self.policy.set_rules({rule: "@"}, overwrite=False)
+        admin_req = fakes.HTTPRequest.blank(
+            '/os-server-groups?all_projects=True',
+            version='2.13', use_admin_context=True)
+        # Check admin user get all projects server groups.
+        resp = self.controller.index(admin_req)
+        projs = [sg['project_id'] for sg in resp['server_groups']]
+        self.assertEqual(2, len(projs))
+        self.assertIn('proj2', projs)
+        # Check non-admin user does not get all projects server groups.
+        req = fakes.HTTPRequest.blank('/os-server-groups?all_projects=True',
+                                      version='2.13')
+        resp = self.controller.index(req)
+        projs = [sg['project_id'] for sg in resp['server_groups']]
+        self.assertNotIn('proj2', projs)
+
+    def test_show_server_groups_policy(self):
         rule_name = policies.POLICY_ROOT % 'show'
         self.common_policy_check(self.admin_or_owner_authorized_contexts,
                                  self.admin_or_owner_unauthorized_contexts,
@@ -70,14 +116,14 @@ class ServerGroupPolicyTest(base.BasePolicyTest):
         rule_name = policies.POLICY_ROOT % 'create'
         body = {'server_group': {'name': 'fake',
                                  'policies': ['affinity']}}
-        self.common_policy_check(self.admin_or_owner_authorized_contexts,
-                                 self.admin_or_owner_unauthorized_contexts,
+        self.common_policy_check(self.everyone_authorized_contexts,
+                                 self.everyone_unauthorized_contexts,
                                  rule_name,
                                  self.controller.create,
                                  self.req, body=body)
 
-    @mock.patch('nova.objects.InstanceGroup.get_by_uuid')
-    def test_delete_server_groups_policy(self, mock_get):
+    @mock.patch('nova.objects.InstanceGroup.destroy')
+    def test_delete_server_groups_policy(self, mock_destroy):
         rule_name = policies.POLICY_ROOT % 'delete'
         self.common_policy_check(self.admin_or_owner_authorized_contexts,
                                  self.admin_or_owner_unauthorized_contexts,
@@ -99,3 +145,16 @@ class ServerGroupScopeTypePolicyTest(ServerGroupPolicyTest):
     def setUp(self):
         super(ServerGroupScopeTypePolicyTest, self).setUp()
         self.flags(enforce_scope=True, group="oslo_policy")
+
+    # TODO(gmann): Test this with system scope once we remove
+    # the hardcoded admin check
+    def test_index_all_project_server_groups_policy(self):
+        pass
+
+
+class ServerGroupNoLegacyPolicyTest(ServerGroupScopeTypePolicyTest):
+    """Test Server Group APIs policies with system scope enabled,
+    and no more deprecated rules that allow the legacy admin API to
+    access system APIs.
+    """
+    without_deprecated_rules = True
diff --git a/nova/tests/unit/test_policy.py b/nova/tests/unit/test_policy.py
index 13a422daf7..ee4a4b87bc 100644
--- a/nova/tests/unit/test_policy.py
+++ b/nova/tests/unit/test_policy.py
@@ -350,6 +350,7 @@ class RealRolePolicyTestCase(test.NoDBTestCase):
 "os_compute_api:os-quota-sets:update",
 "os_compute_api:os-quota-sets:delete",
 "os_compute_api:os-server-diagnostics",
+"os_compute_api:os-server-groups:index:all_projects",
 "os_compute_api:os-services:update",
 "os_compute_api:os-services:delete",
 "os_compute_api:os-shelve:shelve_offload",