[RBAC] Enforce check for share updates
When a user has access to the APIs to reset status, task state or replica state but doesn't have access to the share, they must be prevented from performing those actions. This enforcement allows granular control of these actions and the resources themselves. Change-Id: Ic3be777b238a467d1b7bd1daa6aa088dedb095b0 Closes-Bug: #1955627 Signed-off-by: Goutham Pacha Ravi <gouthampravi@gmail.com>
This commit is contained in:
parent
8ed1c72d57
commit
55edb00cc1
|
@ -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)
|
||||
|
||||
|
|
|
@ -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']
|
||||
|
|
|
@ -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)
|
||||
|
||||
|
|
|
@ -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())
|
||||
|
|
|
@ -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')
|
||||
|
|
|
@ -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(
|
||||
|
|
|
@ -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'):
|
||||
|
|
|
@ -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 <https://launchpad.net/bugs/1955627>`_ for more context.
|
Loading…
Reference in New Issue