From 08af981ffc104d7cb836c1b674e1d73724eeaf88 Mon Sep 17 00:00:00 2001 From: xiaoxi_chen Date: Mon, 29 Jul 2013 14:21:25 +0800 Subject: [PATCH] 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 --- cinder/api/v1/snapshots.py | 8 ++++++-- cinder/api/v1/volumes.py | 6 ++++-- cinder/api/v2/snapshots.py | 8 ++++++-- cinder/tests/api/v1/test_snapshots.py | 26 ++++++++++++++++++++++++++ cinder/tests/api/v1/test_volumes.py | 24 ++++++++++++++++++++++++ cinder/tests/api/v2/test_snapshots.py | 26 ++++++++++++++++++++++++++ cinder/tests/api/v2/test_volumes.py | 7 +++++++ 7 files changed, 99 insertions(+), 6 deletions(-) diff --git a/cinder/api/v1/snapshots.py b/cinder/api/v1/snapshots.py index 05a7ee0fa..d2922dc5a 100644 --- 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 1f75c7105..61eb48c23 100644 --- 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 47a21f310..8e348c65a 100644 --- 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 a0c0fa0b5..10aa914d0 100644 --- 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 c64a762e4..4c557efc9 100644 --- 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 5404ab102..2f96f0415 100644 --- 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 db2e09f21..828296b61 100644 --- 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, -- 2.45.2