From 8ba66fc5d2672bff69c21e36918e5e1882167649 Mon Sep 17 00:00:00 2001 From: Michal Dulko Date: Wed, 26 Aug 2015 14:10:00 +0200 Subject: [PATCH] Validate filters in snapshot*, backup* in db.api In db.sqlalchemy.api methods filters passed from c-api are applied directly to the DB query. From all snapshot_get_all* methods filter validation was done only for snapshot_get_all method. Backup methods are missing the validation completely. This is causing an exception about unknown DB column and returns 500 HTTP error when calling API with an incorrect filter from an admin context (without admin context filters are validated on an c-api level). This commit adds such validation to snapshot_get_by_host, snapshot_get_all_by_project and backup_get_all* methods to prevent such failures. Regression unit tests are also added. APIImpact Closes-Bug: 1469678 Change-Id: I3a9dc6a430f2a149073592487437721a39f0afc5 --- cinder/db/sqlalchemy/api.py | 12 +++++++ cinder/tests/unit/test_db_api.py | 62 ++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 5906fbd42..f1c5d79dc 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -2089,6 +2089,9 @@ def snapshot_get_all(context, filters=None, marker=None, limit=None, paired with corresponding item in sort_keys :returns: list of matching snapshots """ + if filters and not is_valid_model_filters(models.Snapshot, filters): + return [] + session = get_session() with session.begin(): query = _generate_paginate_query(context, session, marker, limit, @@ -2127,6 +2130,9 @@ def snapshot_get_all_for_volume(context, volume_id): @require_context def snapshot_get_by_host(context, host, filters=None): + if filters and not is_valid_model_filters(models.Snapshot, filters): + return [] + query = model_query(context, models.Snapshot, read_deleted='no', project_only=True) if filters: @@ -2169,6 +2175,9 @@ def snapshot_get_all_by_project(context, project_id, filters=None, marker=None, paired with corresponding item in sort_keys :returns: list of matching snapshots """ + if filters and not is_valid_model_filters(models.Snapshot, filters): + return [] + authorize_project_context(context, project_id) # Add project_id to filters @@ -3399,6 +3408,9 @@ def backup_get(context, backup_id): def _backup_get_all(context, filters=None): + if filters and not is_valid_model_filters(models.Backup, filters): + return [] + session = get_session() with session.begin(): # Generate the query diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index 06d68d5ed..8723a7d8c 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -1143,6 +1143,49 @@ class DBAPISnapshotTestCase(BaseTest): self.ctxt, 'host2', {'status': 'error'}), ignored_keys='volume') + self._assertEqualListsOfObjects([], + db.snapshot_get_by_host( + self.ctxt, + 'host2', {'fake_key': 'fake'}), + ignored_keys='volume') + + def test_snapshot_get_all_by_project(self): + db.volume_create(self.ctxt, {'id': 1}) + db.volume_create(self.ctxt, {'id': 2}) + snapshot1 = db.snapshot_create(self.ctxt, {'id': 1, 'volume_id': 1, + 'project_id': 'project1'}) + snapshot2 = db.snapshot_create(self.ctxt, {'id': 2, 'volume_id': 2, + 'status': 'error', + 'project_id': 'project2'}) + + self._assertEqualListsOfObjects([snapshot1], + db.snapshot_get_all_by_project( + self.ctxt, + 'project1'), + ignored_keys='volume') + self._assertEqualListsOfObjects([snapshot2], + db.snapshot_get_all_by_project( + self.ctxt, + 'project2'), + ignored_keys='volume') + self._assertEqualListsOfObjects([], + db.snapshot_get_all_by_project( + self.ctxt, + 'project2', + {'status': 'available'}), + ignored_keys='volume') + self._assertEqualListsOfObjects([snapshot2], + db.snapshot_get_all_by_project( + self.ctxt, + 'project2', + {'status': 'error'}), + ignored_keys='volume') + self._assertEqualListsOfObjects([], + db.snapshot_get_all_by_project( + self.ctxt, + 'project2', + {'fake_key': 'fake'}), + ignored_keys='volume') def test_snapshot_metadata_get(self): metadata = {'a': 'b', 'c': 'd'} @@ -1765,6 +1808,10 @@ class DBAPIBackupTestCase(BaseTest): filtered_backups = db.backup_get_all(self.ctxt, filters=filters) self._assertEqualListsOfObjects([self.created[1]], filtered_backups) + filters = {'fake_key': 'fake'} + filtered_backups = db.backup_get_all(self.ctxt, filters=filters) + self._assertEqualListsOfObjects([], filtered_backups) + def test_backup_get_all_by_host(self): byhost = db.backup_get_all_by_host(self.ctxt, self.created[1]['host']) @@ -1775,6 +1822,21 @@ class DBAPIBackupTestCase(BaseTest): self.created[1]['project_id']) self._assertEqualObjects(self.created[1], byproj[0]) + byproj = db.backup_get_all_by_project(self.ctxt, + self.created[1]['project_id'], + {'fake_key': 'fake'}) + self._assertEqualListsOfObjects([], byproj) + + def test_backup_get_all_by_volume(self): + byvol = db.backup_get_all_by_volume(self.ctxt, + self.created[1]['volume_id']) + self._assertEqualObjects(self.created[1], byvol[0]) + + byvol = db.backup_get_all_by_volume(self.ctxt, + self.created[1]['volume_id'], + {'fake_key': 'fake'}) + self._assertEqualListsOfObjects([], byvol) + def test_backup_update_nonexistent(self): self.assertRaises(exception.BackupNotFound, db.backup_update, -- 2.45.2