From: Michal Dulko <michal.dulko@intel.com>
Date: Wed, 26 Aug 2015 12:10:00 +0000 (+0200)
Subject: Validate filters in snapshot*, backup* in db.api
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=8ba66fc5d2672bff69c21e36918e5e1882167649;p=openstack-build%2Fcinder-build.git

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
---

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,