Pop out 'offset' and 'limit' before use for filter

In previous code of _items() from api/v{1,2}/snapshots.py,
and also the _items)_ from api/v1/volume.py.we didn't pop
out the 'offset' and 'limit' fields from HTTP get params
before we use such params for filter.This is the root cause
for bug #1205956

For non-admin user, since 'offset' and 'limit' is not in the
allowed_search_options, so the volumes.remove_invalid_options
will help to filter them out. As a result, it walks around this
bug.

But for admin user,the volumes.remove_invalid_options will not
try to filter the search_options.So for admin user, the 'limit'
will appear in search_options, then obviously get no result.

fixed bug #1205956

Change-Id: Ib1a66c9d104ac52d6eae18be7f06d02985d4c2fd

diff --git a/cinder/api/v1/snapshots.py b/cinder/api/v1/snapshots.py
index 05a7ee0fa968e5e493626bf0992de6f0eba6b2eb..d2922dc5abe78074850d93e68d6fe2b8a5038fb4 100644 (file)
--- a/cinder/api/v1/snapshots.py
+++ b/cinder/api/v1/snapshots.py
@@ -139,8 +139,12 @@ class SnapshotsController(wsgi.Controller):
         """Returns a list of snapshots, transformed through entity_maker."""
         context = req.environ['cinder.context']
 
-        search_opts = {}
-        search_opts.update(req.GET)
+        #pop out limit and offset , they are not search_opts
+        search_opts = req.GET.copy()
+        search_opts.pop('limit', None)
+        search_opts.pop('offset', None)
+
+        #filter out invalid option
         allowed_search_options = ('status', 'volume_id', 'display_name')
         volumes.remove_invalid_options(context, search_opts,
                                        allowed_search_options)

diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py
index 1f75c7105d1081930fa1b985f69aeb0bdf45ae14..61eb48c23f6b6193dc58ded80589755ceb1b4f6b 100644 (file)
--- a/cinder/api/v1/volumes.py
+++ b/cinder/api/v1/volumes.py
@@ -251,8 +251,10 @@ class VolumeController(wsgi.Controller):
     def _items(self, req, entity_maker):
         """Returns a list of volumes, transformed through entity_maker."""
 
-        search_opts = {}
-        search_opts.update(req.GET)
+        #pop out limit and offset , they are not search_opts
+        search_opts = req.GET.copy()
+        search_opts.pop('limit', None)
+        search_opts.pop('offset', None)
 
         if 'metadata' in search_opts:
             search_opts['metadata'] = ast.literal_eval(search_opts['metadata'])

diff --git a/cinder/api/v2/snapshots.py b/cinder/api/v2/snapshots.py
index 47a21f310fe659a7adc5a991a92c85bf8d7978a9..8e348c65a73b27a0c0a38589c6d8f854a5277bd8 100644 (file)
--- a/cinder/api/v2/snapshots.py
+++ b/cinder/api/v2/snapshots.py
@@ -139,8 +139,12 @@ class SnapshotsController(wsgi.Controller):
         """Returns a list of snapshots, transformed through entity_maker."""
         context = req.environ['cinder.context']
 
-        search_opts = {}
-        search_opts.update(req.GET)
+        #pop out limit and offset , they are not search_opts
+        search_opts = req.GET.copy()
+        search_opts.pop('limit', None)
+        search_opts.pop('offset', None)
+
+        #filter out invalid option
         allowed_search_options = ('status', 'volume_id', 'name')
         volumes.remove_invalid_options(context, search_opts,
                                        allowed_search_options)

diff --git a/cinder/tests/api/v1/test_snapshots.py b/cinder/tests/api/v1/test_snapshots.py
index a0c0fa0b5fbbd738a03a22ee434eb063cbce622a..10aa914d03552afae2ca4c64971371262f74cf72 100644 (file)
--- a/cinder/tests/api/v1/test_snapshots.py
+++ b/cinder/tests/api/v1/test_snapshots.py
@@ -314,6 +314,32 @@ class SnapshotApiTest(test.TestCase):
         self.assertTrue('snapshots' in res)
         self.assertEqual(1, len(res['snapshots']))
 
+    def test_list_snapshots_with_limit_and_offset(self):
+        def list_snapshots_with_limit_and_offset(is_admin):
+            def stub_snapshot_get_all_by_project(context, project_id):
+                return [
+                    stubs.stub_snapshot(1, display_name='backup1'),
+                    stubs.stub_snapshot(2, display_name='backup2'),
+                    stubs.stub_snapshot(3, display_name='backup3'),
+                ]
+
+            self.stubs.Set(db, 'snapshot_get_all_by_project',
+                           stub_snapshot_get_all_by_project)
+
+            req = fakes.HTTPRequest.blank('/v1/fake/snapshots?limit=1\
+                                          &offset=1',
+                                          use_admin_context=is_admin)
+            res = self.controller.index(req)
+
+            self.assertTrue('snapshots' in res)
+            self.assertEqual(1, len(res['snapshots']))
+            self.assertEqual(2, res['snapshots'][0]['id'])
+
+        #admin case
+        list_snapshots_with_limit_and_offset(is_admin=True)
+        #non_admin case
+        list_snapshots_with_limit_and_offset(is_admin=False)
+
     def test_admin_list_snapshots_all_tenants(self):
         req = fakes.HTTPRequest.blank('/v1/fake/snapshots?all_tenants=1',
                                       use_admin_context=True)

diff --git a/cinder/tests/api/v1/test_volumes.py b/cinder/tests/api/v1/test_volumes.py
index c64a762e4a46fa6d37cd3d31d47dc03f600f63d4..4c557efc969557cbb00de8161cc769fe5c990922 100644 (file)
--- a/cinder/tests/api/v1/test_volumes.py
+++ b/cinder/tests/api/v1/test_volumes.py
@@ -546,6 +546,30 @@ class VolumeApiTest(test.TestCase):
                           req,
                           1)
 
+    def test_volume_detail_limit_offset(self):
+        def volume_detail_limit_offset(is_admin):
+            def stub_volume_get_all_by_project(context, project_id, marker,
+                                               limit, sort_key, sort_dir):
+                return [
+                    stubs.stub_volume(1, display_name='vol1'),
+                    stubs.stub_volume(2, display_name='vol2'),
+                ]
+
+            self.stubs.Set(db, 'volume_get_all_by_project',
+                           stub_volume_get_all_by_project)
+            req = fakes.HTTPRequest.blank('/v1/volumes/detail?limit=2\
+                                          &offset=1',
+                                          use_admin_context=is_admin)
+            res_dict = self.controller.index(req)
+            volumes = res_dict['volumes']
+            self.assertEquals(len(volumes), 1)
+            self.assertEquals(volumes[0]['id'], 2)
+
+        #admin case
+        volume_detail_limit_offset(is_admin=True)
+        #non_admin case
+        volume_detail_limit_offset(is_admin=False)
+
     def test_volume_delete(self):
         req = fakes.HTTPRequest.blank('/v1/volumes/1')
         resp = self.controller.delete(req, 1)

diff --git a/cinder/tests/api/v2/test_snapshots.py b/cinder/tests/api/v2/test_snapshots.py
index 5404ab102ef905fc25b8682281e828d91a95e184..2f96f0415a4ac458b07b0a94629c27673282438e 100644 (file)
--- a/cinder/tests/api/v2/test_snapshots.py
+++ b/cinder/tests/api/v2/test_snapshots.py
@@ -325,6 +325,32 @@ class SnapshotApiTest(test.TestCase):
         self.assertTrue('snapshots' in res)
         self.assertEqual(1, len(res['snapshots']))
 
+    def test_list_snapshots_with_limit_and_offset(self):
+        def list_snapshots_with_limit_and_offset(is_admin):
+            def stub_snapshot_get_all_by_project(context, project_id):
+                return [
+                    stubs.stub_snapshot(1, display_name='backup1'),
+                    stubs.stub_snapshot(2, display_name='backup2'),
+                    stubs.stub_snapshot(3, display_name='backup3'),
+                ]
+
+            self.stubs.Set(db, 'snapshot_get_all_by_project',
+                           stub_snapshot_get_all_by_project)
+
+            req = fakes.HTTPRequest.blank('/v2/fake/snapshots?limit=1\
+                                          &offset=1',
+                                          use_admin_context=is_admin)
+            res = self.controller.index(req)
+
+            self.assertTrue('snapshots' in res)
+            self.assertEqual(1, len(res['snapshots']))
+            self.assertEqual(2, res['snapshots'][0]['id'])
+
+        #admin case
+        list_snapshots_with_limit_and_offset(is_admin=True)
+        #non_admin case
+        list_snapshots_with_limit_and_offset(is_admin=False)
+
     def test_admin_list_snapshots_all_tenants(self):
         req = fakes.HTTPRequest.blank('/v2/fake/snapshots?all_tenants=1',
                                       use_admin_context=True)

diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py
index db2e09f219b3af36e40a0a40561d23fe69d19198..828296b61194c6559cda780c288360eff0eceb3d 100644 (file)
--- a/cinder/tests/api/v2/test_volumes.py
+++ b/cinder/tests/api/v2/test_volumes.py
@@ -521,6 +521,13 @@ class VolumeApiTest(test.TestCase):
         self.assertEquals(len(volumes), 1)
         self.assertEquals(volumes[0]['id'], 2)
 
+        req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=2&offset=1',
+                                      use_admin_context=True)
+        res_dict = self.controller.index(req)
+        volumes = res_dict['volumes']
+        self.assertEquals(len(volumes), 1)
+        self.assertEquals(volumes[0]['id'], 2)
+
         req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=-1&offset=1')
         self.assertRaises(exception.InvalidInput,
                           self.controller.index,