From a1b4bd5e86cd865869308c976c6aebf9a4ad23a8 Mon Sep 17 00:00:00 2001 From: jakedahn Date: Tue, 31 Jul 2012 16:35:26 -0700 Subject: [PATCH] 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 --- cinder/api/openstack/volume/volumes.py | 27 +++++++++++++- cinder/tests/api/openstack/fakes.py | 8 +++- .../api/openstack/volume/test_volumes.py | 37 ++++++++++++++++++- cinder/volume/api.py | 13 +++++-- 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/cinder/api/openstack/volume/volumes.py b/cinder/api/openstack/volume/volumes.py index 30efe9916..b620b3044 100644 --- 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 f1511b1a7..605d611ea 100644 --- 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 1c5463b0e..902ae7282 100644 --- 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 614b39d81..0d83d79b8 100644 --- 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)) -- 2.45.2