openstack/manila
Code Issues Proposed changes

Merge "[rbac] Pull up policy checks on share/snapshot APIs"

Browse Source
This commit is contained in:
Zuul 2023-09-25 12:06:33 +00:00 committed by Gerrit Code Review
commit 8ed1c72d57
6 changed files with 77 additions and 80 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
manila/api/v1/share_snapshots.py
View File

@ -29,6 +29,7 @@ from manila.api.views import share_snapshots as snapshot_views
from manila import db
from manila import exception
from manila.i18n import _
from manila import policy
from manila import share
from manila import utils
@ -67,6 +68,7 @@ class ShareSnapshotMixin(object):
context = req.environ['manila.context']
LOG.info("Delete snapshot with id: %s", id, context=context)
policy.check_policy(context, 'share', 'delete_snapshot')
try:
snapshot = self.share_api.get_snapshot(context, id)
@ -172,6 +174,7 @@ class ShareSnapshotMixin(object):
def update(self, req, id, body):
"""Update a snapshot."""
context = req.environ['manila.context']
policy.check_policy(context, 'share', 'snapshot_update')
if not body or 'snapshot' not in body:
raise exc.HTTPUnprocessableEntity()

6
manila/api/v1/shares.py
View File

@ -52,6 +52,7 @@ class ShareMixin(object):
def _delete(self, *args, **kwargs):
return self.share_api.delete(*args, **kwargs)
@wsgi.Controller.authorize('get')
def show(self, req, id):
"""Return data about the given share."""
context = req.environ['manila.context']
@ -63,6 +64,7 @@ class ShareMixin(object):
return self._view_builder.detail(req, share)
@wsgi.Controller.authorize
def delete(self, req, id):
"""Delete a share."""
context = req.environ['manila.context']
@ -97,6 +99,7 @@ class ShareMixin(object):
return webob.Response(status_int=http_client.ACCEPTED)
@wsgi.Controller.authorize("get_all")
def index(self, req):
"""Returns a summary list of shares."""
req.GET.pop('export_location_id', None)
@ -107,6 +110,7 @@ class ShareMixin(object):
req.GET.pop('with_count', None)
return self._get_shares(req, is_detail=False)
@wsgi.Controller.authorize("get_all")
def detail(self, req):
"""Returns a detailed list of shares."""
req.GET.pop('export_location_id', None)
@ -659,6 +663,7 @@ class ShareMixin(object):
return self._access_view_builder.list_view(req, access_rules)
@wsgi.Controller.authorize("extend")
def _extend(self, req, id, body):
"""Extend size of a share."""
context = req.environ['manila.context']
@ -679,6 +684,7 @@ class ShareMixin(object):
return webob.Response(status_int=http_client.ACCEPTED)
@wsgi.Controller.authorize("shrink")
def _shrink(self, req, id, body):
"""Shrink size of a share."""
context = req.environ['manila.context']

5
manila/api/v2/shares.py
View File

@ -242,6 +242,7 @@ class ShareController(wsgi.Controller,
@wsgi.Controller.api_version('2.7')
@wsgi.action('reset_status')
@wsgi.Controller.authorize('reset_status')
def share_reset_status(self, req, id, body):
context = req.environ['manila.context']
try:
@ -266,6 +267,7 @@ class ShareController(wsgi.Controller,
@wsgi.Controller.api_version('2.69')
@wsgi.action('soft_delete')
@wsgi.Controller.authorize('soft_delete')
def share_soft_delete(self, req, id, body):
"""Soft delete a share."""
context = req.environ['manila.context']
@ -288,6 +290,7 @@ class ShareController(wsgi.Controller,
@wsgi.Controller.api_version('2.69')
@wsgi.action('restore')
@wsgi.Controller.authorize("restore")
def share_restore(self, req, id, body):
"""Restore a share from recycle bin."""
context = req.environ['manila.context']
@ -578,6 +581,7 @@ class ShareController(wsgi.Controller,
return self._revert(req, id, body)
@wsgi.Controller.api_version("2.0")
@wsgi.Controller.authorize("get_all")
def index(self, req):
"""Returns a summary list of shares."""
if req.api_version_request < api_version.APIVersionRequest("2.35"):
@ -598,6 +602,7 @@ class ShareController(wsgi.Controller,
return self._get_shares(req, is_detail=False)
@wsgi.Controller.api_version("2.0")
@wsgi.Controller.authorize("get_all")
def detail(self, req):
"""Returns a detailed list of shares."""
if req.api_version_request < api_version.APIVersionRequest("2.35"):

10
manila/share/api.py
View File

@ -1289,7 +1289,6 @@ class API(base.Base):
self.share_rpcapi.revert_to_snapshot(
context, share, snapshot, active_replica['host'], reservations)
@policy.wrap_check_policy('share')
@prevent_locked_action_on_share('delete')
def soft_delete(self, context, share):
"""Soft delete share."""
@ -1336,7 +1335,6 @@ class API(base.Base):
self._check_is_share_busy(share)
self.db.share_soft_delete(context, share_id)
@policy.wrap_check_policy('share')
def restore(self, context, share):
"""Restore share."""
share_id = share['id']
@ -2150,8 +2148,6 @@ class API(base.Base):
def _get_all(self, context, search_opts=None, sort_key='created_at',
sort_dir='desc', show_count=False):
policy.check_policy(context, 'share', 'get_all')
if search_opts is None:
search_opts = {}
@ -2521,9 +2517,9 @@ class API(base.Base):
def extend(self, context, share, new_size, force=False):
if force:
policy.check_policy(context, 'share', 'force_extend')
policy.check_policy(context, 'share', 'force_extend', share)
else:
policy.check_policy(context, 'share', 'extend')
policy.check_policy(context, 'share', 'extend', share)
if share['status'] != constants.STATUS_AVAILABLE:
msg_params = {
@ -2642,8 +2638,6 @@ class API(base.Base):
resource=share)
def shrink(self, context, share, new_size):
policy.check_policy(context, 'share', 'shrink')
status = str(share['status']).lower()
valid_statuses = (constants.STATUS_AVAILABLE,
constants.STATUS_SHRINKING_POSSIBLE_DATA_LOSS_ERROR)

32
manila/tests/api/v2/test_shares.py
View File

@ -133,6 +133,7 @@ class ShareAPITest(test.TestCase):
}
CONF.set_default("default_share_type", None)
self.mock_object(policy, 'check_policy')
def _process_expected_share_detailed_response(self, shr_dict, req_version):
"""Sets version based parameters on share dictionary."""
@ -1826,6 +1827,10 @@ class ShareAPITest(test.TestCase):
search_opts_expected['is_soft_deleted'] = (
search_opts['is_soft_deleted'])
policy.check_policy.assert_called_once_with(
req.environ['manila.context'],
'share',
'get_all')
if use_admin_context:
search_opts_expected.update({'fake_key': 'fake_value'})
search_opts_expected['host'] = search_opts['host']
@ -1870,6 +1875,10 @@ class ShareAPITest(test.TestCase):
search_opts_expected = {}
policy.check_policy.assert_called_once_with(
req.environ['manila.context'],
'share',
'get_all')
if use_admin_context:
search_opts_expected.update({'fake_key': 'fake_value'})
search_opts_expected['host'] = search_opts['host']
@ -1905,6 +1914,10 @@ class ShareAPITest(test.TestCase):
}
]
}
policy.check_policy.assert_called_once_with(
req.environ['manila.context'],
'share',
'get_all')
self.assertEqual(expected, res_dict)
@ddt.data({'use_admin_context': False, 'version': '2.4'},
@ -2002,10 +2015,14 @@ class ShareAPITest(test.TestCase):
api_version.APIVersionRequest('2.69')):
search_opts_expected['is_soft_deleted'] = (
search_opts['is_soft_deleted'])
if use_admin_context:
search_opts_expected.update({'fake_key': 'fake_value'})
search_opts_expected['host'] = search_opts['host']
policy.check_policy.assert_called_once_with(
req.environ['manila.context'],
'share',
'get_all')
mock_get_all.assert_called_once_with(
req.environ['manila.context'],
sort_key=search_opts['sort_key'],
@ -2078,7 +2095,13 @@ class ShareAPITest(test.TestCase):
def _list_detail_test_common(self, req, expected):
self.mock_object(share_api.API, 'get_all',
stubs.stub_share_get_all_by_project)
res_dict = self.controller.detail(req)
policy.check_policy.assert_called_once_with(
req.environ['manila.context'],
'share',
'get_all')
self.assertDictListMatch(expected['shares'], res_dict['shares'])
self.assertEqual(res_dict['shares'][0]['volume_type'],
res_dict['shares'][0]['share_type'])
@ -2137,7 +2160,13 @@ class ShareAPITest(test.TestCase):
req = fakes.HTTPRequest.blank(
'/v2/fake/shares/detail', environ=env,
version=share_replicas.MIN_SUPPORTED_API_VERSION)
res_dict = self.controller.detail(req)
policy.check_policy.assert_called_once_with(
req.environ['manila.context'],
'share',
'get_all')
expected = {
'shares': [
{
@ -2222,6 +2251,7 @@ class ShareActionsTest(test.TestCase):
super(ShareActionsTest, self).setUp()
self.controller = shares.ShareController()
self.mock_object(share_api.API, 'get', stubs.stub_share_get)
self.mock_object(policy, 'check_policy')
@ddt.unpack
@ddt.data(

101
manila/tests/share/test_api.py
View File

@ -245,9 +245,9 @@ class ShareAPITestCase(test.TestCase):
self.mock_object(db_api, 'share_get_all_by_project',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0]))
ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=True)
shares = self.api.get_all(ctx)
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'get_all')
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
project_id='fake_pid_1', filters={}, is_public=False
@ -258,9 +258,9 @@ class ShareAPITestCase(test.TestCase):
ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=True)
self.mock_object(db_api, 'share_get_all',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES))
shares = self.api.get_all(ctx, {'all_tenants': 1})
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'get_all')
db_api.share_get_all.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at', filters={})
self.assertEqual(_FAKE_LIST_OF_ALL_SHARES, shares)
@ -269,9 +269,9 @@ class ShareAPITestCase(test.TestCase):
ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=True)
self.mock_object(db_api, 'share_get_all',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES))
shares = self.api.get_all(ctx, {'all_tenants': ''})
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'get_all')
db_api.share_get_all.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at', filters={})
self.assertEqual(_FAKE_LIST_OF_ALL_SHARES, shares)
@ -280,9 +280,9 @@ class ShareAPITestCase(test.TestCase):
ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=True)
self.mock_object(db_api, 'share_get_all_by_project',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0]))
shares = self.api.get_all(ctx, {'all_tenants': 'false'})
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'get_all')
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
project_id='fake_pid_1', filters={}, is_public=False
@ -316,10 +316,8 @@ class ShareAPITestCase(test.TestCase):
exception.NotAuthorized,
self.api.get_all, ctx, filters)
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
mock.call(ctx, 'share', policy),
])
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', policy)
def test_get_all_admin_filter_by_share_server_and_all_tenants(self):
# NOTE(vponomaryov): if share_server_id provided, 'all_tenants' opt
@ -331,10 +329,8 @@ class ShareAPITestCase(test.TestCase):
self.mock_object(db_api, 'share_get_all_by_project')
shares = self.api.get_all(
ctx, {'share_server_id': 'fake_server_3', 'all_tenants': 1})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
mock.call(ctx, 'share', 'list_by_share_server_id'),
])
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'list_by_share_server_id')
db_api.share_get_all_by_share_server.assert_called_once_with(
ctx, 'fake_server_3', sort_dir='desc', sort_key='created_at',
filters={},
@ -349,10 +345,9 @@ class ShareAPITestCase(test.TestCase):
db_api, 'share_get_all_by_project',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[1::2]))
expected_filters = {'display_name': 'bar'}
shares = self.api.get_all(ctx, {'display_name': 'bar'})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
project_id='fake_pid_2', filters=expected_filters, is_public=False
@ -382,9 +377,6 @@ class ShareAPITestCase(test.TestCase):
expected_filters = copy.copy(search_opts)
shares = self.api.get_all(ctx, search_opts)
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
@ -399,10 +391,9 @@ class ShareAPITestCase(test.TestCase):
ctx = context.RequestContext('fake_uid', 'fake_pid_2', is_admin=True)
self.mock_object(db_api, 'share_get_all_by_project',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[1:]))
shares = self.api.get_all(ctx, {'export_location_' + type: 'test'})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
project_id='fake_pid_2',
@ -414,10 +405,9 @@ class ShareAPITestCase(test.TestCase):
ctx = context.RequestContext('fake_uid', 'fake_pid_2', is_admin=True)
self.mock_object(db_api, 'share_get_all',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[:1]))
shares = self.api.get_all(ctx, {'name': 'foo', 'all_tenants': 1})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at', filters={})
self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[:1], shares)
@ -430,9 +420,7 @@ class ShareAPITestCase(test.TestCase):
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0::2]))
shares = self.api.get_all(ctx, {'status': constants.STATUS_AVAILABLE})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
project_id='fake_pid_2', filters=expected_filter, is_public=False
@ -447,9 +435,6 @@ class ShareAPITestCase(test.TestCase):
expected_filter = {'status': constants.STATUS_ERROR}
shares = self.api.get_all(
ctx, {'status': constants.STATUS_ERROR, 'all_tenants': 1})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
filters=expected_filter)
@ -460,10 +445,9 @@ class ShareAPITestCase(test.TestCase):
ctx = context.RequestContext('fake_uid', 'fake_pid_2', is_admin=False)
self.mock_object(db_api, 'share_get_all_by_project',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[1:]))
shares = self.api.get_all(ctx, {'all_tenants': 1})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
project_id='fake_pid_2', filters={}, is_public=False
@ -477,11 +461,10 @@ class ShareAPITestCase(test.TestCase):
mock.Mock(side_effect=[
_FAKE_LIST_OF_ALL_SHARES[1::2],
_FAKE_LIST_OF_ALL_SHARES[2::4]]))
shares = self.api.get_all(
ctx, {'name': 'bar', 'status': constants.STATUS_ERROR})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
expected_filter_1 = {'status': constants.STATUS_ERROR}
expected_filter_2 = {'status': constants.STATUS_AVAILABLE}
@ -496,11 +479,8 @@ class ShareAPITestCase(test.TestCase):
# one item expected, two filtered
shares = self.api.get_all(
ctx, {'name': 'foo1', 'status': constants.STATUS_AVAILABLE})
self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[2::4], shares)
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all_by_project.assert_has_calls([
mock.call(
ctx, sort_dir='desc', sort_key='created_at',
@ -519,9 +499,6 @@ class ShareAPITestCase(test.TestCase):
self.mock_object(db_api, 'share_get_all_by_project', mock.Mock(
return_value=_FAKE_LIST_OF_ALL_SHARES[1:]))
shares = self.api.get_all(ctx, {'is_public': is_public})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
project_id='fake_pid_2', filters={}, is_public=True
@ -535,9 +512,6 @@ class ShareAPITestCase(test.TestCase):
self.mock_object(db_api, 'share_get_all_by_project', mock.Mock(
return_value=_FAKE_LIST_OF_ALL_SHARES[1:]))
shares = self.api.get_all(ctx, {'is_public': is_public})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
project_id='fake_pid_2', filters={}, is_public=False
@ -550,17 +524,14 @@ class ShareAPITestCase(test.TestCase):
is_admin=False)
self.assertRaises(ValueError, self.api.get_all,
ctx, {'is_public': is_public})
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
])
def test_get_all_with_sorting_valid(self):
self.mock_object(db_api, 'share_get_all_by_project',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0]))
ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=False)
shares = self.api.get_all(ctx, sort_key='status', sort_dir='asc')
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'get_all')
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='asc', sort_key='status',
project_id='fake_pid_1', filters={}, is_public=False
@ -577,8 +548,6 @@ class ShareAPITestCase(test.TestCase):
ctx,
sort_key=1,
)
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'get_all')
def test_get_all_sort_dir_invalid(self):
self.mock_object(db_api, 'share_get_all_by_project',
@ -590,23 +559,18 @@ class ShareAPITestCase(test.TestCase):
ctx,
sort_dir=1,
)
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'get_all')
def _get_all_filter_metadata_or_extra_specs_valid(self, key):
self.mock_object(db_api, 'share_get_all_by_project',
mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0]))
ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=False)
search_opts = {key: {'foo1': 'bar1', 'foo2': 'bar2'}}
shares = self.api.get_all(ctx, search_opts=search_opts.copy())
if key == 'extra_specs':
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
mock.call(ctx, 'share_types_extra_spec', 'index'),
])
else:
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'get_all')
ctx, 'share_types_extra_spec', 'index')
db_api.share_get_all_by_project.assert_called_once_with(
ctx, sort_dir='desc', sort_key='created_at',
project_id='fake_pid_1', filters=search_opts, is_public=False)
@ -626,13 +590,8 @@ class ShareAPITestCase(test.TestCase):
self.assertRaises(exception.InvalidInput, self.api.get_all, ctx,
search_opts=search_opts)
if key == 'extra_specs':
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
mock.call(ctx, 'share_types_extra_spec', 'index'),
])
else:
share_api.policy.check_policy.assert_called_once_with(
ctx, 'share', 'get_all')
ctx, 'share_types_extra_spec', 'index')
def test_get_all_filter_by_invalid_metadata(self):
self._get_all_filter_metadata_or_extra_specs_invalid(key='metadata')