Merge "Fixed backups GET and DELETE to restrict access to the owner of backup"

trove/backup/models.py

@@ -99,7 +99,7 @@ class Backup(object):
         return query.first()
 
     @classmethod
-    def get_by_id(cls, backup_id, deleted=False):
+    def get_by_id(cls, context, backup_id, deleted=False):
         """
         get the backup for that id
         :param cls:
@@ -108,7 +108,9 @@ class Backup(object):
         :return:
         """
         try:
-            db_info = DBBackup.find_by(id=backup_id, deleted=deleted)
+            db_info = DBBackup.find_by(context=context,
+                                       id=backup_id,
+                                       deleted=deleted)
             return db_info
         except exception.NotFound:
             raise exception.NotFound(uuid=backup_id)
@@ -148,7 +150,7 @@ class Backup(object):
         """
 
         def _delete_resources():
-            backup = cls.get_by_id(backup_id)
+            backup = cls.get_by_id(context, backup_id)
             if backup.is_running:
                 msg = ("Backup %s cannot be delete because it is running." %
                        backup_id)

trove/backup/service.py

@@ -45,7 +45,8 @@ class BackupController(wsgi.Controller):
         """Return a single backup."""
         LOG.info(_("Showing a backup for tenant '%s'") % tenant_id)
         LOG.info(_("id : '%s'\n\n") % id)
-        backup = Backup.get_by_id(id)
+        context = req.environ[wsgi.CONTEXT_KEY]
+        backup = Backup.get_by_id(context, id)
         return wsgi.Result(views.BackupView(backup).data(), 200)
 
     def create(self, req, body, tenant_id):

trove/db/models.py

@@ -89,11 +89,20 @@ class DatabaseModelBase(models.ModelBase):
             self[k] = v
 
     @classmethod
-    def find_by(cls, **conditions):
+    def find_by(cls, context=None, **conditions):
         model = cls.get_by(**conditions)
+
         if model is None:
             raise exception.ModelNotFoundError(_("%s Not Found") %
                                                cls.__name__)
+
+        if ((context and not context.is_admin and hasattr(model, 'tenant_id')
+             and model.tenant_id != context.tenant)):
+            LOG.error("Tenant %s tried to access %s, owned by %s."
+                      % (context.tenant, cls.__name__, model.tenant_id))
+            raise exception.ModelNotFoundError(_("%s Not Found") %
+                                               cls.__name__)
+
         return model
 
     @classmethod

trove/instance/models.py

@@ -247,13 +247,9 @@ def get_db_info(context, id):
     elif id is None:
         raise TypeError("Argument id not defined.")
     try:
-        db_info = DBInstance.find_by(id=id, deleted=False)
+        db_info = DBInstance.find_by(context=context, id=id, deleted=False)
     except exception.NotFound:
         raise exception.NotFound(uuid=id)
-    if not context.is_admin and db_info.tenant_id != context.tenant:
-        LOG.error("Tenant %s tried to access instance %s, owned by %s."
-                  % (context.tenant, id, db_info.tenant_id))
-        raise exception.NotFound(uuid=id)
     return db_info
 
 
@@ -438,7 +434,7 @@ class Instance(BuiltInstance):
             security_groups = None
 
             if backup_id is not None:
-                backup_info = Backup.get_by_id(backup_id)
+                backup_info = Backup.get_by_id(context, backup_id)
                 if backup_info.is_running:
                     raise exception.BackupNotCompleteError(backup_id=backup_id)
 

trove/taskmanager/models.py

@@ -550,7 +550,7 @@ class BackupTasks(object):
     @classmethod
     def delete_backup(cls, context, backup_id):
         #delete backup from swift
-        backup = trove.backup.models.Backup.get_by_id(backup_id)
+        backup = trove.backup.models.Backup.get_by_id(context, backup_id)
         try:
             filename = backup.filename
             if filename:

trove/tests/api/backups.py

@@ -15,17 +15,18 @@
 from proboscis.asserts import assert_equal
 from proboscis.asserts import assert_not_equal
 from proboscis.asserts import assert_raises
-from proboscis.asserts import fail
 from proboscis import test
 from proboscis import SkipTest
 from proboscis.decorators import time_out
 from trove.tests.util import poll_until
 from trove.tests.util import test_config
+from trove.tests.util import create_dbaas_client
+from trove.tests.util.users import Requirements
+from trove.tests.config import CONFIG
 from troveclient import exceptions
 from trove.tests.api.instances import WaitForGuestInstallationToFinish
 from trove.tests.api.instances import instance_info
 from trove.tests.api.instances import assert_unprocessable
-import time
 
 GROUP = "dbaas.api.backups"
 BACKUP_NAME = 'backup_test'
@@ -145,6 +146,16 @@ class ListBackups(object):
         assert_equal(instance_info.id, backup.instance_id)
         assert_equal('COMPLETED', backup.status)
 
+        # Test to make sure that user in other tenant is not able
+        # to GET this backup
+        reqs = Requirements(is_admin=False)
+        other_user = CONFIG.users.find_user(
+            reqs,
+            black_list=[instance_info.user.auth_user])
+        other_client = create_dbaas_client(other_user)
+        assert_raises(exceptions.NotFound, other_client.backups.get,
+                      backup_info.id)
+
 
 @test(runs_after=[ListBackups],
       groups=[GROUP])
@@ -230,6 +241,17 @@ class DeleteBackups(object):
     @time_out(60 * 2)
     def test_backup_delete(self):
         """test delete"""
+
+        # Test to make sure that user in other tenant is not able
+        # to DELETE this backup
+        reqs = Requirements(is_admin=False)
+        other_user = CONFIG.users.find_user(
+            reqs,
+            black_list=[instance_info.user.auth_user])
+        other_client = create_dbaas_client(other_user)
+        assert_raises(exceptions.NotFound, other_client.backups.delete,
+                      backup_info.id)
+
         instance_info.dbaas.backups.delete(backup_info.id)
         assert_equal(202, instance_info.dbaas.last_http_code)
 

trove/tests/fakes/guestagent.py

@@ -273,7 +273,7 @@ class FakeGuest(object):
 
     def create_backup(self, backup_id):
         from trove.backup.models import Backup, BackupState
-        backup = Backup.get_by_id(backup_id)
+        backup = Backup.get_by_id(context=None, backup_id=backup_id)
 
         def finish_create_backup():
             backup.state = BackupState.COMPLETED

trove/tests/unittests/backup/test_backup_models.py

@@ -116,14 +116,14 @@ class BackupDeleteTest(testtools.TestCase):
     def test_delete_backup_is_running(self):
         backup = mock()
         backup.is_running = True
-        when(models.Backup).get_by_id(any()).thenReturn(backup)
+        when(models.Backup).get_by_id(any(), any()).thenReturn(backup)
         self.assertRaises(exception.UnprocessableEntity,
                           models.Backup.delete, self.context, 'backup_id')
 
     def test_delete_backup_swift_token_invalid(self):
         backup = mock()
         backup.is_running = False
-        when(models.Backup).get_by_id(any()).thenReturn(backup)
+        when(models.Backup).get_by_id(any(), any()).thenReturn(backup)
         when(models.Backup).verify_swift_auth_token(any()).thenRaise(
             exception.SwiftAuthError)
         self.assertRaises(exception.SwiftAuthError, models.Backup.delete,

trove/tests/unittests/taskmanager/test_models.py

@@ -38,7 +38,7 @@ class BackupTasksTest(testtools.TestCase):
                                    {'name': 'third'}])
         when(backup_models.Backup).delete(any()).thenReturn(None)
         when(backup_models.Backup).get_by_id(
-            self.backup.id).thenReturn(self.backup)
+            any(), self.backup.id).thenReturn(self.backup)
         when(self.backup).delete(any()).thenReturn(None)
         self.swift_client = mock()
         when(remote).create_swift_client(