diff options
author | jiaopengju <jiaopengju@cmss.chinamobile.com> | 2018-11-25 21:04:25 +0800 |
---|---|---|
committer | jiaopengju <jiaopengju@cmss.chinamobile.com> | 2018-11-25 21:09:11 +0800 |
commit | 37c197851a6a099d8c274920be62703c627b8a3d (patch) | |
tree | f493c9a6decf7bbea4cd32ae5ea5faa65779e16f | |
parent | 2df8d3400faa5c9a88d084714ed12dbf2cb4bcc7 (diff) |
Add permission check when creating restore
Currently, general users have rights to restore other users'
checkpoints. We should add permission check to avoid the security
risk.
Closes-Bug: #1805004
Change-Id: If0f957a3aa8f25778833d7611342fab6b8efa388
Notes
Notes (review):
Code-Review+2: Jiao Pengju <jiaopengju@cmss.chinamobile.com>
Workflow+1: Jiao Pengju <jiaopengju@cmss.chinamobile.com>
Verified+2: Zuul
Submitted-by: Zuul
Submitted-at: Mon, 26 Nov 2018 01:16:12 +0000
Reviewed-on: https://review.openstack.org/619926
Project: openstack/karbor
Branch: refs/heads/master
-rw-r--r-- | karbor/api/v1/restores.py | 2 | ||||
-rw-r--r-- | karbor/services/protection/manager.py | 8 | ||||
-rw-r--r-- | karbor/tests/unit/api/v1/test_restores.py | 10 | ||||
-rw-r--r-- | karbor/tests/unit/protection/test_manager.py | 14 |
4 files changed, 33 insertions, 1 deletions
diff --git a/karbor/api/v1/restores.py b/karbor/api/v1/restores.py index fca30da..3dea082 100644 --- a/karbor/api/v1/restores.py +++ b/karbor/api/v1/restores.py | |||
@@ -232,6 +232,8 @@ class RestoresController(wsgi.Controller): | |||
232 | # call restore rpc API of protection service | 232 | # call restore rpc API of protection service |
233 | try: | 233 | try: |
234 | self.protection_api.restore(context, restoreobj, restore_auth) | 234 | self.protection_api.restore(context, restoreobj, restore_auth) |
235 | except exception.AccessCheckpointNotAllowed as error: | ||
236 | raise exc.HTTPForbidden(explanation=error.msg) | ||
235 | except Exception: | 237 | except Exception: |
236 | # update the status of restore | 238 | # update the status of restore |
237 | update_dict = { | 239 | update_dict = { |
diff --git a/karbor/services/protection/manager.py b/karbor/services/protection/manager.py index c3db38c..a2de22e 100644 --- a/karbor/services/protection/manager.py +++ b/karbor/services/protection/manager.py | |||
@@ -182,7 +182,8 @@ class ProtectionManager(manager.Manager): | |||
182 | exception.CheckpointNotFound, | 182 | exception.CheckpointNotFound, |
183 | exception.CheckpointNotAvailable, | 183 | exception.CheckpointNotAvailable, |
184 | exception.FlowError, | 184 | exception.FlowError, |
185 | exception.InvalidInput) | 185 | exception.InvalidInput, |
186 | exception.AccessCheckpointNotAllowed) | ||
186 | def restore(self, context, restore, restore_auth): | 187 | def restore(self, context, restore, restore_auth): |
187 | LOG.info("Starting restore service:restore action") | 188 | LOG.info("Starting restore service:restore action") |
188 | 189 | ||
@@ -197,6 +198,11 @@ class ProtectionManager(manager.Manager): | |||
197 | checkpoint_collection = provider.get_checkpoint_collection() | 198 | checkpoint_collection = provider.get_checkpoint_collection() |
198 | checkpoint = checkpoint_collection.get(checkpoint_id) | 199 | checkpoint = checkpoint_collection.get(checkpoint_id) |
199 | 200 | ||
201 | if not context.is_admin and ( | ||
202 | checkpoint.project_id != context.project_id): | ||
203 | raise exception.AccessCheckpointNotAllowed( | ||
204 | checkpoint_id=checkpoint_id) | ||
205 | |||
200 | if checkpoint.status != constants.CHECKPOINT_STATUS_AVAILABLE: | 206 | if checkpoint.status != constants.CHECKPOINT_STATUS_AVAILABLE: |
201 | raise exception.CheckpointNotAvailable( | 207 | raise exception.CheckpointNotAvailable( |
202 | checkpoint_id=checkpoint_id) | 208 | checkpoint_id=checkpoint_id) |
diff --git a/karbor/tests/unit/api/v1/test_restores.py b/karbor/tests/unit/api/v1/test_restores.py index 8d4b50c..79fb177 100644 --- a/karbor/tests/unit/api/v1/test_restores.py +++ b/karbor/tests/unit/api/v1/test_restores.py | |||
@@ -75,6 +75,16 @@ class RestoreApiTest(base.TestCase): | |||
75 | self.assertRaises(exception.ValidationError, self.controller.create, | 75 | self.assertRaises(exception.ValidationError, self.controller.create, |
76 | req, body=body) | 76 | req, body=body) |
77 | 77 | ||
78 | @mock.patch('karbor.services.protection.api.API.restore') | ||
79 | def test_restore_create_with_checkpoint_not_allowed_exception( | ||
80 | self, mock_restore): | ||
81 | mock_restore.side_effect = exception.AccessCheckpointNotAllowed | ||
82 | restore = self._restore_in_request_body() | ||
83 | body = {"restore": restore} | ||
84 | req = fakes.HTTPRequest.blank('/v1/restores') | ||
85 | self.assertRaises(exc.HTTPForbidden, self.controller.create, | ||
86 | req, body=body) | ||
87 | |||
78 | @mock.patch( | 88 | @mock.patch( |
79 | 'karbor.api.v1.restores.RestoresController._get_all') | 89 | 'karbor.api.v1.restores.RestoresController._get_all') |
80 | def test_restore_list_detail(self, moak_get_all): | 90 | def test_restore_list_detail(self, moak_get_all): |
diff --git a/karbor/tests/unit/protection/test_manager.py b/karbor/tests/unit/protection/test_manager.py index 7528f02..6ecf263 100644 --- a/karbor/tests/unit/protection/test_manager.py +++ b/karbor/tests/unit/protection/test_manager.py | |||
@@ -147,6 +147,20 @@ class ProtectionServiceTest(base.TestCase): | |||
147 | fakes.fake_protection_plan()) | 147 | fakes.fake_protection_plan()) |
148 | 148 | ||
149 | @mock.patch.object(provider.ProviderRegistry, 'show_provider') | 149 | @mock.patch.object(provider.ProviderRegistry, 'show_provider') |
150 | def test_restore_with_project_id_not_same(self, mock_provider): | ||
151 | mock_provider.return_value = fakes.FakeProvider() | ||
152 | context = mock.MagicMock(project_id='fake_project_id_1', | ||
153 | is_admin=False) | ||
154 | fake_restore = { | ||
155 | 'checkpoint_id': 'fake_checkpoint', | ||
156 | 'provider_id': 'fake_provider_id', | ||
157 | 'parameters': None | ||
158 | } | ||
159 | self.assertRaises( | ||
160 | oslo_messaging.ExpectedException, self.pro_manager.restore, | ||
161 | context, fake_restore, None) | ||
162 | |||
163 | @mock.patch.object(provider.ProviderRegistry, 'show_provider') | ||
150 | def test_list_checkpoints(self, mock_provider): | 164 | def test_list_checkpoints(self, mock_provider): |
151 | fake_provider = fakes.FakeProvider() | 165 | fake_provider = fakes.FakeProvider() |
152 | fake_provider.list_checkpoints = mock.MagicMock() | 166 | fake_provider.list_checkpoints = mock.MagicMock() |