diff --git a/manila/api/openstack/wsgi.py b/manila/api/openstack/wsgi.py index 8bd0bddabf..6257eb586c 100644 --- a/manila/api/openstack/wsgi.py +++ b/manila/api/openstack/wsgi.py @@ -1237,8 +1237,17 @@ class AdminActionsMixin(object): return update @Controller.authorize('reset_status') - def _reset_status(self, req, id, body, status_attr='status'): - """Reset the status_attr specified on the resource.""" + def _reset_status(self, req, id, body, status_attr='status', + resource=None): + """Reset the status_attr specified on the resource. + + :param req: API request object + :param id: ID of the resource + :param body: API request body + :param status_attr: Attribute on the resource denoting the status + to be reset + :param resource: Resource model or dict if we need to avoid fetching it + """ context = req.environ['manila.context'] body_attr = self.body_attributes[status_attr] update = self.validate_update( @@ -1248,9 +1257,17 @@ class AdminActionsMixin(object): LOG.debug(msg, {'resource': self.resource_name, 'id': id, 'update': update}) try: - self._update(context, id, update) + resource = resource or self._get(context, id) except exception.NotFound as e: raise webob.exc.HTTPNotFound(e.message) + try: + policy.check_policy(context, + self.resource_name, + "reset_status", + target_obj=resource) + except exception.NotAuthorized as e: + raise webob.exc.HTTPForbidden(e.message) + self._update(context, id, update) return webob.Response(status_int=http_client.ACCEPTED) @Controller.authorize('force_delete') @@ -1261,6 +1278,10 @@ class AdminActionsMixin(object): resource = self._get(context, id) except exception.NotFound as e: raise webob.exc.HTTPNotFound(e.message) + policy.check_policy(context, + self.resource_name, + "force_delete", + target_obj=resource) self._delete(context, resource, force=True) return webob.Response(status_int=http_client.ACCEPTED) diff --git a/manila/api/v2/share_networks.py b/manila/api/v2/share_networks.py index 81f1271561..1a32a158a0 100644 --- a/manila/api/v2/share_networks.py +++ b/manila/api/v2/share_networks.py @@ -59,6 +59,9 @@ class ShareNetworkController(wsgi.Controller, wsgi.AdminActionsMixin): 'status': set(constants.SHARE_NETWORK_STATUSES) } + def _get(self, *args, **kwargs): + return db_api.share_network_get(*args, **kwargs) + def show(self, req, id): """Return data about the requested network info.""" context = req.environ['manila.context'] diff --git a/manila/api/v2/share_servers.py b/manila/api/v2/share_servers.py index a477d03a18..a26f658aed 100644 --- a/manila/api/v2/share_servers.py +++ b/manila/api/v2/share_servers.py @@ -47,6 +47,9 @@ class ShareServerController(share_servers.ShareServerController, 'task_state': set(constants.SERVER_TASK_STATE_STATUSES), } + def _get(self, *args, **kwargs): + return db_api.share_server_get(*args, **kwargs) + def _update(self, context, id, update): db_api.share_server_update(context, id, update) diff --git a/manila/api/v2/share_snapshot_instances.py b/manila/api/v2/share_snapshot_instances.py index 83f0d1d345..defe527ce6 100644 --- a/manila/api/v2/share_snapshot_instances.py +++ b/manila/api/v2/share_snapshot_instances.py @@ -81,6 +81,9 @@ class ShareSnapshotInstancesController(wsgi.Controller, def _update(self, *args, **kwargs): db.share_snapshot_instance_update(*args, **kwargs) + def _get(self, *args, **kwargs): + db.share_snapshot_instance_get(*args, **kwargs) + def create_resource(): return wsgi.Resource(ShareSnapshotInstancesController()) diff --git a/manila/api/v2/shares.py b/manila/api/v2/shares.py index 5c25e91720..b5b55c04e4 100644 --- a/manila/api/v2/shares.py +++ b/manila/api/v2/shares.py @@ -233,12 +233,12 @@ class ShareController(wsgi.Controller, try: share = self.share_api.get(context, id) except exception.NotFound: - raise exception.ShareNotFound(share_id=id) + raise exc.HTTPNotFound("Share %s not found" % id) if share.get('is_soft_deleted'): msg = _("status cannot be reset for share '%s' " "since it has been soft deleted.") % id raise exc.HTTPForbidden(explanation=msg) - return self._reset_status(req, id, body) + return self._reset_status(req, id, body, resource=share) @wsgi.Controller.api_version('2.7') @wsgi.action('reset_status') @@ -248,12 +248,12 @@ class ShareController(wsgi.Controller, try: share = self.share_api.get(context, id) except exception.NotFound: - raise exception.ShareNotFound(share_id=id) + raise exc.HTTPNotFound("Share %s not found" % id) if share.get('is_soft_deleted'): msg = _("status cannot be reset for share '%s' " "since it has been soft deleted.") % id raise exc.HTTPForbidden(explanation=msg) - return self._reset_status(req, id, body) + return self._reset_status(req, id, body, resource=share) @wsgi.Controller.api_version('2.0', '2.6') @wsgi.action('os-force_delete') @@ -455,7 +455,8 @@ class ShareController(wsgi.Controller, msg = _("task state cannot be reset for share '%s' " "since it has been soft deleted.") % id raise exc.HTTPForbidden(explanation=msg) - return self._reset_status(req, id, body, status_attr='task_state') + return self._reset_status(req, id, body, status_attr='task_state', + resource=share) @wsgi.Controller.api_version('2.0', '2.6') @wsgi.action('os-allow_access') diff --git a/manila/tests/api/v2/test_share_servers.py b/manila/tests/api/v2/test_share_servers.py index 7693a76006..313227bc48 100644 --- a/manila/tests/api/v2/test_share_servers.py +++ b/manila/tests/api/v2/test_share_servers.py @@ -54,14 +54,22 @@ class ShareServerControllerTest(test.TestCase): body = {'reset_status': {'status': status}} context = req.environ['manila.context'] + self.mock_object(self.controller, '_get', mock.Mock( + return_value={'share_server': 'object'})) mock_update = self.mock_object(db_api, 'share_server_update') result = self.controller.share_server_reset_status( req, 'fake_server_id', body) self.assertEqual(202, result.status_int) - policy.check_policy.assert_called_once_with( - context, self.resource_name, 'reset_status') + policy.check_policy.assert_has_calls([ + mock.call(context, self.resource_name, 'reset_status'), + mock.call(context, + self.resource_name, + 'reset_status', + target_obj={'share_server': 'object'}), + ]) + mock_update.assert_called_once_with( context, 'fake_server_id', {'status': status}) @@ -881,16 +889,18 @@ class ShareServerControllerTest(test.TestCase): update = {'task_state': constants.TASK_STATE_MIGRATION_ERROR} body = {'reset_task_state': update} - self.mock_object(db_api, 'share_server_update', + self.mock_object(db_api, 'share_server_get', mock.Mock(side_effect=exception.ShareServerNotFound( share_server_id='fake_server_id'))) + self.mock_object(db_api, 'share_server_update') self.assertRaises(webob.exc.HTTPNotFound, self.controller.share_server_reset_task_state, req, server['id'], body) - db_api.share_server_update.assert_called_once_with(utils.IsAMatcher( - ctx_api.RequestContext), server['id'], update) + db_api.share_server_get.assert_called_once_with(utils.IsAMatcher( + ctx_api.RequestContext), server['id']) + db_api.share_server_update.assert_not_called() def test_share_server_migration_complete(self): server = db_utils.create_share_server( diff --git a/manila/tests/api/v2/test_shares.py b/manila/tests/api/v2/test_shares.py index 71c423ccc9..f13ce4e456 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -1138,15 +1138,42 @@ class ShareAPITest(test.TestCase): update = {'task_state': constants.TASK_STATE_MIGRATION_ERROR} body = {'reset_task_state': update} - self.mock_object(db, 'share_update', - mock.Mock(side_effect=exception.NotFound())) + self.mock_object(share_api.API, 'get', + mock.Mock(side_effect=exception.NotFound)) + self.mock_object(db, 'share_update') - self.assertRaises(webob.exc.HTTPNotFound, + self.assertRaises(exception.NotFound, self.controller.reset_task_state, req, share['id'], body) - db.share_update.assert_called_once_with(utils.IsAMatcher( - context.RequestContext), share['id'], update) + share_api.API.get.assert_called_once_with(utils.IsAMatcher( + context.RequestContext), share['id']) + db.share_update.assert_not_called() + + def test_reset_task_state_share_other_project_public_share(self): + share = db_utils.create_share(is_public=True) + req = fakes.HTTPRequest.blank( + '/v2/fake/shares/%s/action' % share['id'], + use_admin_context=True, version=LATEST_MICROVERSION) + req.method = 'POST' + req.headers['content-type'] = 'application/json' + req.api_version_request.experimental = True + update = {'task_state': constants.TASK_STATE_MIGRATION_ERROR} + body = {'reset_task_state': update} + + # NOTE(gouthamr): we're testing a scenario where someone has access + # to the RBAC rule share:reset_task_state, but doesn't own the share. + # Ideally we'd override the default policy, but it's a shared + # resource and we'll bleed into other tests, so we'll mock the + # policy check to return False instead + rbac_checks = [None, None, exception.NotAuthorized] + with mock.patch.object(policy, 'check_policy', + side_effect=rbac_checks): + self.mock_object(share_api.API, 'get', + mock.Mock(return_value=share)) + self.assertRaises(webob.exc.HTTPForbidden, + self.controller.reset_task_state, + req, share['id'], body) def test_reset_task_state_share_has_been_soft_deleted(self): share = self.share_in_recycle_bin @@ -2717,19 +2744,18 @@ class ShareAdminActionsAPITest(test.TestCase): req.headers['X-Openstack-Manila-Api-Version'] = version req.body = jsonutils.dumps(body).encode("utf-8") req.environ['manila.context'] = ctxt - self.mock_object(share_api.API, 'get', mock.Mock(return_value=model)) - resp = req.get_response(fakes.app()) + resp = req.get_response(fakes.app(), catch_exc_info=True) # validate response code and model status self.assertEqual(valid_code, resp.status_int) - if valid_code == 404: + if valid_code == 404 and db_access_method is not None: self.assertRaises(exception.NotFound, db_access_method, ctxt, model['id']) - else: + elif db_access_method: actual_model = db_access_method(ctxt, model['id']) self.assertEqual(valid_status, actual_model['status']) @@ -2739,6 +2765,7 @@ class ShareAdminActionsAPITest(test.TestCase): valid_status, version): share, req = self._setup_share_data(version=version) ctxt = self._get_context(role) + self.mock_object(share_api.API, 'get', mock.Mock(return_value=share)) self._reset_status(ctxt, share, req, db.share_get, valid_code, valid_status, version=version) @@ -2747,6 +2774,7 @@ class ShareAdminActionsAPITest(test.TestCase): def test_share_invalid_reset_status_body(self, body): share, req = self._setup_share_data(version='2.6') ctxt = self.admin_context + self.mock_object(share_api.API, 'get', mock.Mock(return_value=share)) self._reset_status(ctxt, share, req, db.share_get, 400, constants.STATUS_AVAILABLE, body, version='2.6') @@ -2758,7 +2786,23 @@ class ShareAdminActionsAPITest(test.TestCase): '/v2/fake/shares/%s/action' % fake_share['id'], version=version) self._reset_status(self.admin_context, fake_share, req, - db.share_snapshot_get, 404, version=version) + db.share_get, 404, version=version) + + @ddt.data('2.6', '2.7') + def test_reset_status_other_project_public_share(self, version): + # NOTE(gouthamr): we're testing a scenario where someone has access + # to the RBAC rule share:reset_status, but doesn't own the share. + # Ideally we'd override the default policy, but it's a shared + # resource and we'll bleed into other tests, so we'll mock the + # policy check to return False instead + share, req = self._setup_share_data(version=version) + share['is_public'] = True + rbac_checks = [None, exception.NotAuthorized] + with mock.patch.object(policy, 'authorize', side_effect=rbac_checks): + self.mock_object(share_api.API, 'get', + mock.Mock(return_value=share)) + self._reset_status( + self.member_context, share, req, None, 403, version=version) def _force_delete(self, ctxt, model, req, db_access_method, valid_code, check_model_in_db=False, version='2.7'): diff --git a/releasenotes/notes/bug-1955627-add-check-to-reset-status-baa126a7145a45bb.yaml b/releasenotes/notes/bug-1955627-add-check-to-reset-status-baa126a7145a45bb.yaml new file mode 100644 index 0000000000..b2547def85 --- /dev/null +++ b/releasenotes/notes/bug-1955627-add-check-to-reset-status-baa126a7145a45bb.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Role based access control is enforced on the POST /shares/{share_id}/action + API to reset status, task state, replica state and similar fields. This + prevents the situation where deployments allow some users access to + these APIs, but they don't belong to projects where the resources exist. + See `bug 1955627 `_ for more context.