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 9a91623e1a1625e060af51a507dfc97dbd599498..74c097cb78129d3be5972805a1a6a504b039e245 100644 (file)
--- 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 41763430e0a0358e050931740330d605871620c1..aa22b6706d68a10e2ea6a17cf8efd30e62a7c390 100644 (file)
--- 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 d30f8308d287e0ae50101a6fb71cbac7e01bf18f..e21ca48f6d7064c86145a88e4bb67d75b2cf25f4 100644 (file)
--- 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 12c92a12b4898932e35da6bf938242eec7b1b689..4f32d7d86e9c04b6c0507662e1f058c7ba401c13 100644 (file)
--- 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 d8043cce735245ebe235fb6ee27927ef9f1761a9..418be1950db1c45dc8bdce9a3e7a428d76beea67 100644 (file)
--- 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: