Default behavior should restrict admins to tenant for volumes.

  * NOTE: This is a port from cinder to nova volumes
  * Now to view all volumes or volume snapshots across
    all tenants you need to include the all_tenants=1
    GET param in your api request.
  * Fixes remaining issues blocking bug #967882

Change-Id: I7fe15e792b62e59973c7faa2cf1c52929ae5864f

7 files changed, 165 insertions, 20 deletions

diff --git a/nova/api/openstack/volume/snapshots.py b/nova/api/openstack/volume/snapshots.py
index 209f78d13..91823de79 100644
--- a/nova/api/openstack/volume/snapshots.py
+++ b/nova/api/openstack/volume/snapshots.py
@@ -130,7 +130,11 @@ class SnapshotsController(object):
         """Returns a list of snapshots, transformed through entity_maker."""
         context = req.environ['nova.context']
 
-        snapshots = self.volume_api.get_all_snapshots(context)
+        search_opts = {}
+        search_opts.update(req.GET)
+
+        snapshots = self.volume_api.get_all_snapshots(context,
+                                                      search_opts=search_opts)
         limited_list = common.limited(snapshots, req)
         res = [entity_maker(context, snapshot) for snapshot in limited_list]
         return {'snapshots': res}
diff --git a/nova/api/openstack/volume/volumes.py b/nova/api/openstack/volume/volumes.py
index 83a2b2f63..4c12638b4 100644
--- a/nova/api/openstack/volume/volumes.py
+++ b/nova/api/openstack/volume/volumes.py
@@ -196,9 +196,15 @@ class VolumeController(object):
 
     def _items(self, req, entity_maker):
         """Returns a list of volumes, transformed through entity_maker."""
+
+        search_opts = {}
+        search_opts.update(req.GET)
+
         context = req.environ['nova.context']
+        remove_invalid_options(context,
+                               search_opts, self._get_volume_search_options())
 
-        volumes = self.volume_api.get_all(context)
+        volumes = self.volume_api.get_all(context, search_opts=search_opts)
         limited_list = common.limited(volumes, req)
         res = [entity_maker(context, vol) for vol in limited_list]
         return {'volumes': res}
@@ -253,6 +259,25 @@ class VolumeController(object):
 
         return wsgi.ResponseObject(result, headers=dict(location=location))
 
+    def _get_volume_search_options(self):
+        """Return volume search options allowed by non-admin."""
+        return ('name', 'status')
+
 
 def create_resource():
     return wsgi.Resource(VolumeController())
+
+
+def remove_invalid_options(context, search_options, allowed_search_options):
+    """Remove search options that are not valid for non-admin API/context."""
+    if context.is_admin:
+        # Allow all options
+        return
+    # Otherwise, strip out all unknown options
+    unknown_options = [opt for opt in search_options
+            if opt not in allowed_search_options]
+    bad_options = ", ".join(unknown_options)
+    log_msg = _("Removing options '%(bad_options)s' from query") % locals()
+    LOG.debug(log_msg)
+    for opt in unknown_options:
+        search_options.pop(opt, None)
diff --git a/nova/tests/api/ec2/test_cloud.py b/nova/tests/api/ec2/test_cloud.py
index f2684efc3..b2d552f84 100644
--- a/nova/tests/api/ec2/test_cloud.py
+++ b/nova/tests/api/ec2/test_cloud.py
@@ -616,8 +616,8 @@ class CloudTestCase(test.TestCase):
 
     def test_describe_volumes(self):
         """Makes sure describe_volumes works and filters results."""
-        vol1 = db.volume_create(self.context, {})
-        vol2 = db.volume_create(self.context, {})
+        vol1 = db.volume_create(self.context, {'project_id': self.project_id})
+        vol2 = db.volume_create(self.context, {'project_id': self.project_id})
         result = self.cloud.describe_volumes(self.context)
         self.assertEqual(len(result['volumeSet']), 2)
         volume_id = ec2utils.id_to_ec2_vol_id(vol2['id'])
@@ -653,7 +653,8 @@ class CloudTestCase(test.TestCase):
 
     def test_create_volume_from_snapshot(self):
         """Makes sure create_volume works when we specify a snapshot."""
-        vol = db.volume_create(self.context, {'size': 1})
+        vol = db.volume_create(self.context, {'size': 1,
+                                              'project_id': self.project_id})
         snap = db.snapshot_create(self.context, {'volume_id': vol['id'],
                                                  'volume_size': vol['size'],
                                                  'status': "available"})
@@ -690,8 +691,10 @@ class CloudTestCase(test.TestCase):
     def test_describe_snapshots(self):
         """Makes sure describe_snapshots works and filters results."""
         vol = db.volume_create(self.context, {})
-        snap1 = db.snapshot_create(self.context, {'volume_id': vol['id']})
-        snap2 = db.snapshot_create(self.context, {'volume_id': vol['id']})
+        snap1 = db.snapshot_create(self.context,
+                {'volume_id': vol['id'], 'project_id': self.project_id})
+        snap2 = db.snapshot_create(self.context,
+                {'volume_id': vol['id'], 'project_id': self.project_id})
         result = self.cloud.describe_snapshots(self.context)
         self.assertEqual(len(result['snapshotSet']), 2)
         snapshot_id = ec2utils.id_to_ec2_snap_id(snap2['id'])
diff --git a/nova/tests/api/openstack/fakes.py b/nova/tests/api/openstack/fakes.py
index 62722b34d..a5eb57b1b 100644
--- a/nova/tests/api/openstack/fakes.py
+++ b/nova/tests/api/openstack/fakes.py
@@ -540,5 +540,37 @@ def stub_volume_get_notfound(self, context, volume_id):
     raise exc.NotFound
 
 
-def stub_volume_get_all(self, context, search_opts=None):
+def stub_volume_get_all(context, search_opts=None):
+    return [stub_volume(100, project_id='fake'),
+            stub_volume(101, project_id='superfake'),
+            stub_volume(102, project_id='superduperfake')]
+
+
+def stub_volume_get_all_by_project(self, context, search_opts=None):
     return [stub_volume_get(self, context, '1')]
+
+
+def stub_snapshot(id, **kwargs):
+    snapshot = {
+        'id': id,
+        'volume_id': 12,
+        'status': 'available',
+        'volume_size': 100,
+        'created_at': None,
+        'display_name': 'Default name',
+        'display_description': 'Default description',
+        'project_id': 'fake'
+        }
+
+    snapshot.update(kwargs)
+    return snapshot
+
+
+def stub_snapshot_get_all(self):
+    return [stub_snapshot(100, project_id='fake'),
+            stub_snapshot(101, project_id='superfake'),
+            stub_snapshot(102, project_id='superduperfake')]
+
+
+def stub_snapshot_get_all_by_project(self, context):
+    return [stub_snapshot(1)]
diff --git a/nova/tests/api/openstack/volume/test_snapshots.py b/nova/tests/api/openstack/volume/test_snapshots.py
index ccd3a70b4..e7370c548 100644
--- a/nova/tests/api/openstack/volume/test_snapshots.py
+++ b/nova/tests/api/openstack/volume/test_snapshots.py
@@ -17,6 +17,7 @@ from lxml import etree
 import webob
 
 from nova.api.openstack.volume import snapshots
+from nova import db
 from nova import exception
 from nova import flags
 from nova.openstack.common import log as logging
@@ -63,7 +64,7 @@ def stub_snapshot_get(self, context, snapshot_id):
     return param
 
 
-def stub_snapshot_get_all(self, context):
+def stub_snapshot_get_all(self, context, search_opts=None):
     param = _get_default_snapshot_param()
     return [param]
 
@@ -73,9 +74,10 @@ class SnapshotApiTest(test.TestCase):
         super(SnapshotApiTest, self).setUp()
         self.controller = snapshots.SnapshotsController()
 
-        self.stubs.Set(volume.api.API, "get_snapshot", stub_snapshot_get)
-        self.stubs.Set(volume.api.API, "get_all_snapshots",
-            stub_snapshot_get_all)
+        self.stubs.Set(db, 'snapshot_get_all_by_project',
+                       fakes.stub_snapshot_get_all_by_project)
+        self.stubs.Set(db, 'snapshot_get_all',
+                      fakes.stub_snapshot_get_all)
 
     def test_snapshot_create(self):
         self.stubs.Set(volume.api.API, "create_snapshot", stub_snapshot_create)
@@ -113,6 +115,7 @@ class SnapshotApiTest(test.TestCase):
                         snapshot['display_description'])
 
     def test_snapshot_delete(self):
+        self.stubs.Set(volume.api.API, "get_snapshot", stub_snapshot_get)
         self.stubs.Set(volume.api.API, "delete_snapshot", stub_snapshot_delete)
 
         snapshot_id = 123
@@ -130,6 +133,7 @@ class SnapshotApiTest(test.TestCase):
                           snapshot_id)
 
     def test_snapshot_show(self):
+        self.stubs.Set(volume.api.API, "get_snapshot", stub_snapshot_get)
         req = fakes.HTTPRequest.blank('/v1/snapshots/123')
         resp_dict = self.controller.show(req, 123)
 
@@ -145,6 +149,8 @@ class SnapshotApiTest(test.TestCase):
                           snapshot_id)
 
     def test_snapshot_detail(self):
+        self.stubs.Set(volume.api.API, "get_all_snapshots",
+                      stub_snapshot_get_all)
         req = fakes.HTTPRequest.blank('/v1/snapshots/detail')
         resp_dict = self.controller.detail(req)
 
@@ -155,6 +161,33 @@ class SnapshotApiTest(test.TestCase):
         resp_snapshot = resp_snapshots.pop()
         self.assertEqual(resp_snapshot['id'], '123')
 
+    def test_admin_list_snapshots_limited_to_project(self):
+        req = fakes.HTTPRequest.blank('/v1/fake/snapshots',
+                                      use_admin_context=True)
+        res = self.controller.index(req)
+
+        self.assertTrue('snapshots' in res)
+        self.assertEqual(1, len(res['snapshots']))
+
+    def test_admin_list_snapshots_all_tenants(self):
+        req = fakes.HTTPRequest.blank('/v2/fake/snapshots?all_tenants=1',
+                                      use_admin_context=True)
+        res = self.controller.index(req)
+        self.assertTrue('snapshots' in res)
+        self.assertEqual(3, len(res['snapshots']))
+
+    def test_all_tenants_non_admin_gets_all_tenants(self):
+        req = fakes.HTTPRequest.blank('/v2/fake/snapshots?all_tenants=1')
+        res = self.controller.index(req)
+        self.assertTrue('snapshots' in res)
+        self.assertEqual(1, len(res['snapshots']))
+
+    def test_non_admin_get_by_project(self):
+        req = fakes.HTTPRequest.blank('/v2/fake/snapshots')
+        res = self.controller.index(req)
+        self.assertTrue('snapshots' in res)
+        self.assertEqual(1, len(res['snapshots']))
+
 
 class SnapshotSerializerTest(test.TestCase):
     def _verify_snapshot(self, snap, tree):
diff --git a/nova/tests/api/openstack/volume/test_volumes.py b/nova/tests/api/openstack/volume/test_volumes.py
index 672c08d88..6f9a6557b 100644
--- a/nova/tests/api/openstack/volume/test_volumes.py
+++ b/nova/tests/api/openstack/volume/test_volumes.py
@@ -19,6 +19,7 @@ from lxml import etree
 import webob
 
 from nova.api.openstack.volume import volumes
+from nova import db
 from nova import exception
 from nova import flags
 from nova.openstack.common import timeutils
@@ -35,7 +36,9 @@ class VolumeApiTest(test.TestCase):
         super(VolumeApiTest, self).setUp()
         self.controller = volumes.VolumeController()
 
-        self.stubs.Set(volume_api.API, 'get_all', fakes.stub_volume_get_all)
+        self.stubs.Set(db, 'volume_get_all', fakes.stub_volume_get_all)
+        self.stubs.Set(db, 'volume_get_all_by_project',
+                       fakes.stub_volume_get_all_by_project)
         self.stubs.Set(volume_api.API, 'get', fakes.stub_volume_get)
         self.stubs.Set(volume_api.API, 'delete', fakes.stub_volume_delete)
 
@@ -89,6 +92,9 @@ class VolumeApiTest(test.TestCase):
                           body)
 
     def test_volume_list(self):
+        self.stubs.Set(volume_api.API, 'get_all',
+                     fakes.stub_volume_get_all_by_project)
+
         req = fakes.HTTPRequest.blank('/v1/volumes')
         res_dict = self.controller.index(req)
         expected = {'volumes': [{'status': 'fakestatus',
@@ -110,6 +116,9 @@ class VolumeApiTest(test.TestCase):
         self.assertEqual(res_dict, expected)
 
     def test_volume_list_detail(self):
+        self.stubs.Set(volume_api.API, 'get_all',
+                   fakes.stub_volume_get_all_by_project)
+
         req = fakes.HTTPRequest.blank('/v1/volumes/detail')
         res_dict = self.controller.index(req)
         expected = {'volumes': [{'status': 'fakestatus',
@@ -194,6 +203,33 @@ class VolumeApiTest(test.TestCase):
                           req,
                           1)
 
+    def test_admin_list_volumes_limited_to_project(self):
+        req = fakes.HTTPRequest.blank('/v2/fake/volumes',
+                                      use_admin_context=True)
+        res = self.controller.index(req)
+
+        self.assertTrue('volumes' in res)
+        self.assertEqual(1, len(res['volumes']))
+
+    def test_admin_list_volumes_all_tenants(self):
+        req = fakes.HTTPRequest.blank('/v2/fake/volumes?all_tenants=1',
+                                      use_admin_context=True)
+        res = self.controller.index(req)
+        self.assertTrue('volumes' in res)
+        self.assertEqual(3, len(res['volumes']))
+
+    def test_all_tenants_non_admin_gets_all_tenants(self):
+        req = fakes.HTTPRequest.blank('/v2/fake/volumes?all_tenants=1')
+        res = self.controller.index(req)
+        self.assertTrue('volumes' in res)
+        self.assertEqual(1, len(res['volumes']))
+
+    def test_non_admin_get_by_project(self):
+        req = fakes.HTTPRequest.blank('/v2/fake/volumes')
+        res = self.controller.index(req)
+        self.assertTrue('volumes' in res)
+        self.assertEqual(1, len(res['volumes']))
+
 
 class VolumeSerializerTest(test.TestCase):
     def _verify_volume_attachment(self, attach, tree):
diff --git a/nova/volume/api.py b/nova/volume/api.py
index a9ebeda28..d98c8d6eb 100644
--- a/nova/volume/api.py
+++ b/nova/volume/api.py
@@ -201,14 +201,19 @@ class API(base.Base):
         check_policy(context, 'get', volume)
         return volume
 
-    def get_all(self, context, search_opts={}):
+    def get_all(self, context, search_opts=None):
         check_policy(context, 'get_all')
-        if context.is_admin:
+
+        if search_opts is None:
+            search_opts = {}
+
+        if (context.is_admin and 'all_tenants' in search_opts):
+            # Need to remove all_tenants to pass the filtering below.
+            del search_opts['all_tenants']
             volumes = self.db.volume_get_all(context)
         else:
             volumes = self.db.volume_get_all_by_project(context,
-                                    context.project_id)
-
+                                                        context.project_id)
         if search_opts:
             LOG.debug(_("Searching by: %s") % str(search_opts))
 
@@ -247,11 +252,18 @@ class API(base.Base):
         rv = self.db.snapshot_get(context, snapshot_id)
         return dict(rv.iteritems())
 
-    def get_all_snapshots(self, context):
+    def get_all_snapshots(self, context, search_opts=None):
         check_policy(context, 'get_all_snapshots')
-        if context.is_admin:
+
+        search_opts = search_opts or {}
+
+        if (context.is_admin and 'all_tenants' in search_opts):
+            # Need to remove all_tenants to pass the filtering below.
+            del search_opts['all_tenants']
             return self.db.snapshot_get_all(context)
-        return self.db.snapshot_get_all_by_project(context, context.project_id)
+        else:
+            return self.db.snapshot_get_all_by_project(context,
+                                                       context.project_id)
 
     @wrap_check_policy
     def check_attach(self, context, volume):