From: Kuo-tung Kao Date: Tue, 28 Jul 2015 09:44:57 +0000 (+0800) Subject: Don't use context.elevated to get volume X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=b7c7bb8f951d6d5155f70887282ddb9e4fd2a3fb;p=openstack-build%2Fcinder-build.git Don't use context.elevated to get volume Original Problem: ================= Since the metadata(readonly and attached_mode) is stored in admin metadata, normal user need `run context.elevated` to retrieves admin metadata. The above way will bring a side effect. Normal user can also get any volume which the user shouldn't access when the user knows the UUID. Solution: ========= Use context instead of context.elevated to get volume. And add admin metadata to it. Based on cinder-meetup-summer-2015 conclusion, we use the solution. The solution will need extra database connection. Change-Id: I06f21e7578b65a59c0fe4d3afe0e882ed73c4725 Closes-Bug: #1477625 --- diff --git a/cinder/tests/unit/api/v1/test_volumes.py b/cinder/tests/unit/api/v1/test_volumes.py index 9a91623e1..74c097cb7 100644 --- a/cinder/tests/unit/api/v1/test_volumes.py +++ b/cinder/tests/unit/api/v1/test_volumes.py @@ -235,10 +235,13 @@ class VolumeApiTest(test.TestCase): req, body) - def test_volume_update(self): - self.stubs.Set(db, 'volume_get', stubs.stub_volume_get_db) - self.stubs.Set(volume_api.API, "update", stubs.stub_volume_update) - + @mock.patch.object(db, 'volume_admin_metadata_get', + return_value={'attached_mode': 'rw', + 'readonly': 'False'}) + @mock.patch.object(db, 'volume_get', side_effect=stubs.stub_volume_get_db) + @mock.patch.object(volume_api.API, 'update', + side_effect=stubs.stub_volume_update) + def test_volume_update(self, *args): updates = { "display_name": "Updated Test Name", } @@ -266,10 +269,14 @@ class VolumeApiTest(test.TestCase): self.assertEqual(res_dict, expected) self.assertEqual(len(self.notifier.notifications), 2) - def test_volume_update_metadata(self): - self.stubs.Set(db, 'volume_get', stubs.stub_volume_get_db) - self.stubs.Set(volume_api.API, "update", stubs.stub_volume_update) - + @mock.patch.object(db, 'volume_admin_metadata_get', + return_value={"qos_max_iops": 2000, + "readonly": "False", + "attached_mode": "rw"}) + @mock.patch.object(db, 'volume_get', side_effect=stubs.stub_volume_get_db) + @mock.patch.object(volume_api.API, 'update', + side_effect=stubs.stub_volume_update) + def test_volume_update_metadata(self, *args): updates = { "metadata": {"qos_max_iops": 2000} } @@ -300,6 +307,11 @@ class VolumeApiTest(test.TestCase): self.assertEqual(len(self.notifier.notifications), 2) def test_volume_update_with_admin_metadata(self): + def stubs_volume_admin_metadata_get(context, volume_id): + return {'key': 'value', + 'readonly': 'True'} + self.stubs.Set(db, 'volume_admin_metadata_get', + stubs_volume_admin_metadata_get) self.stubs.Set(volume_api.API, "update", stubs.stub_volume_update) volume = stubs.stub_volume("1") @@ -379,6 +391,11 @@ class VolumeApiTest(test.TestCase): req, '1', body) def test_volume_list(self): + def stubs_volume_admin_metadata_get(context, volume_id): + return {'attached_mode': 'rw', + 'readonly': 'False'} + self.stubs.Set(db, 'volume_admin_metadata_get', + stubs_volume_admin_metadata_get) self.stubs.Set(db, 'volume_get', stubs.stub_volume_get_db) self.stubs.Set(volume_api.API, 'get_all', stubs.stub_volume_get_all_by_project) @@ -524,9 +541,11 @@ class VolumeApiTest(test.TestCase): 'size': 1}]} self.assertEqual(res_dict, expected) - def test_volume_show(self): - self.stubs.Set(db, 'volume_get', stubs.stub_volume_get_db) - + @mock.patch.object(db, 'volume_admin_metadata_get', + return_value={'attached_mode': 'rw', + 'readonly': 'False'}) + @mock.patch.object(db, 'volume_get', side_effect=stubs.stub_volume_get_db) + def test_volume_show(self, *args): req = fakes.HTTPRequest.blank('/v1/volumes/1') res_dict = self.controller.show(req, '1') expected = {'volume': {'status': 'fakestatus', diff --git a/cinder/tests/unit/api/v2/stubs.py b/cinder/tests/unit/api/v2/stubs.py index 41763430e..aa22b6706 100644 --- a/cinder/tests/unit/api/v2/stubs.py +++ b/cinder/tests/unit/api/v2/stubs.py @@ -113,7 +113,12 @@ def stub_volume_delete(self, context, *args, **param): def stub_volume_get(self, context, volume_id, viewable_admin_meta=False): - return stub_volume(volume_id) + if viewable_admin_meta: + return stub_volume(volume_id) + else: + volume = stub_volume(volume_id) + del volume['volume_admin_metadata'] + return volume def stub_volume_get_notfound(self, context, @@ -122,7 +127,12 @@ def stub_volume_get_notfound(self, context, def stub_volume_get_db(context, volume_id): - return stub_volume(volume_id) + if context.is_admin: + return stub_volume(volume_id) + else: + volume = stub_volume(volume_id) + del volume['volume_admin_metadata'] + return volume def stub_volume_get_all(context, search_opts=None, marker=None, limit=None, @@ -138,7 +148,7 @@ def stub_volume_get_all_by_project(self, context, marker, limit, filters=None, viewable_admin_meta=False): filters = filters or {} - return [stub_volume_get(self, context, '1')] + return [stub_volume_get(self, context, '1', viewable_admin_meta=True)] def stub_snapshot(id, **kwargs): diff --git a/cinder/tests/unit/api/v2/test_volumes.py b/cinder/tests/unit/api/v2/test_volumes.py index d30f8308d..e21ca48f6 100644 --- a/cinder/tests/unit/api/v2/test_volumes.py +++ b/cinder/tests/unit/api/v2/test_volumes.py @@ -15,6 +15,7 @@ import datetime +import functools from lxml import etree import mock @@ -249,7 +250,8 @@ class VolumeApiTest(test.TestCase): @mock.patch.object(volume_api.API, 'create', autospec=True) def test_volume_creation_from_source_volume(self, create, get_volume): - get_volume.side_effect = stubs.stub_volume_get + get_volume.side_effect = functools.partial(stubs.stub_volume_get, + viewable_admin_meta=True) create.side_effect = stubs.stub_volume_create source_volid = '2f49aa3a-6aae-488d-8b99-a43271605af6' diff --git a/cinder/utils.py b/cinder/utils.py index 12c92a12b..4f32d7d86 100644 --- a/cinder/utils.py +++ b/cinder/utils.py @@ -700,9 +700,15 @@ def add_visible_admin_metadata(volume): visible_admin_meta = {} if volume.get('volume_admin_metadata'): - for item in volume['volume_admin_metadata']: - if item['key'] in _visible_admin_metadata_keys: - visible_admin_meta[item['key']] = item['value'] + if isinstance(volume['volume_admin_metadata'], dict): + volume_admin_metadata = volume['volume_admin_metadata'] + for key in volume_admin_metadata: + if key in _visible_admin_metadata_keys: + visible_admin_meta[key] = volume_admin_metadata[key] + else: + for item in volume['volume_admin_metadata']: + if item['key'] in _visible_admin_metadata_keys: + visible_admin_meta[item['key']] = item['value'] # avoid circular ref when volume is a Volume instance elif (volume.get('admin_metadata') and isinstance(volume.get('admin_metadata'), dict)): diff --git a/cinder/volume/api.py b/cinder/volume/api.py index d8043cce7..418be1950 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -400,12 +400,16 @@ class API(base.Base): LOG.info(_LI("Volume updated successfully."), resource=vref) def get(self, context, volume_id, viewable_admin_meta=False): + rv = self.db.volume_get(context, volume_id) + + volume = dict(rv) + if viewable_admin_meta: ctxt = context.elevated() - else: - ctxt = context - rv = self.db.volume_get(ctxt, volume_id) - volume = dict(rv) + admin_metadata = self.db.volume_admin_metadata_get(ctxt, + volume_id) + volume['volume_admin_metadata'] = admin_metadata + try: check_policy(context, 'get', volume) except exception.PolicyNotAuthorized: