Admin users should be restricted from seeing all

volumes by default.

  * Now to view all volumes 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: Ie9d74e9c09fa0c192ab6257b5fb02d65b593cbfb

diff --git a/cinder/api/openstack/volume/volumes.py b/cinder/api/openstack/volume/volumes.py
index 30efe991641d17da63681a34e323318fa5453cf4..b620b3044050b46a16444fcd30ae025d71383b1f 100644 (file)
--- a/cinder/api/openstack/volume/volumes.py
+++ b/cinder/api/openstack/volume/volumes.py
@@ -199,9 +199,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['cinder.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}
@@ -262,6 +268,25 @@ class VolumeController(object):
 
         return {'volume': retval}
 
+    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/cinder/tests/api/openstack/fakes.py b/cinder/tests/api/openstack/fakes.py
index f1511b1a74909b69ee7da40af11687214b65dd83..605d611eacf11ee232994363c06523c000f4452c 100644 (file)
--- a/cinder/tests/api/openstack/fakes.py
+++ b/cinder/tests/api/openstack/fakes.py
@@ -231,5 +231,11 @@ 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')]

diff --git a/cinder/tests/api/openstack/volume/test_volumes.py b/cinder/tests/api/openstack/volume/test_volumes.py
index 1c5463b0ed1f0dcbfe97ce2dbfb467f6b6077a1c..902ae72828170aaa0c0f5c96a7bc7f451eddcbfa 100644 (file)
--- a/cinder/tests/api/openstack/volume/test_volumes.py
+++ b/cinder/tests/api/openstack/volume/test_volumes.py
@@ -19,6 +19,7 @@ from lxml import etree
 import webob
 
 from cinder.api.openstack.volume import volumes
+from cinder import db
 from cinder import exception
 from cinder import flags
 from cinder import test
@@ -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)
 
@@ -93,6 +96,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',
@@ -113,6 +119,8 @@ 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',
@@ -197,6 +205,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/cinder/volume/api.py b/cinder/volume/api.py
index 614b39d816cec053fc3766722b5122420e296991..0d83d79b84017aeaec70657ebf243b2dab4cf5b3 100644 (file)
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -165,14 +165,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))