openstack/manila
Code Issues Proposed changes

Merge "Only return share host for admins using shares API"

Browse Source
This commit is contained in:
Jenkins 2017-02-16 23:42:13 +00:00 committed by Gerrit Code Review
commit 7ef9ccb688
16 changed files with 130 additions and 116 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
etc/manila/policy.json
View File

@ -22,6 +22,7 @@
"share:get": "rule:default",
"share:get_all": "rule:default",
"share:list_by_share_server_id": "rule:admin_api",
"share:list_by_host": "rule:admin_api",
"share:update": "rule:default",
"share:access_get": "rule:default",
"share:access_get_all": "rule:default",

2
manila/api/views/share_replicas.py
View File

@ -54,7 +54,6 @@ class ReplicationViewBuilder(common.ViewBuilder):
'share_id': replica.get('share_id'),
'availability_zone': replica.get('availability_zone'),
'created_at': replica.get('created_at'),
'host': replica.get('host'),
'status': replica.get('status'),
'share_network_id': replica.get('share_network_id'),
'replica_state': replica.get('replica_state'),
@ -63,6 +62,7 @@ class ReplicationViewBuilder(common.ViewBuilder):
if context.is_admin:
replica_dict['share_server_id'] = replica.get('share_server_id')
replica_dict['host'] = replica.get('host')
self.update_versioned_resource_dict(request, replica_dict, replica)
return {'share_replica': replica_dict}

3
manila/api/views/shares.py
View File

@ -80,7 +80,6 @@ class ViewBuilder(common.ViewBuilder):
'name': share.get('display_name'),
'description': share.get('display_description'),
'project_id': share.get('project_id'),
'host': share_instance.get('host'),
'snapshot_id': share.get('snapshot_id'),
'share_network_id': share_instance.get('share_network_id'),
'share_proto': share.get('share_proto'),
@ -92,12 +91,12 @@ class ViewBuilder(common.ViewBuilder):
'is_public': share.get('is_public'),
'export_locations': export_locations,
}
self.update_versioned_resource_dict(request, share_dict, share)
if context.is_admin:
share_dict['share_server_id'] = share_instance.get(
'share_server_id')
share_dict['host'] = share_instance.get('host')
return {'share': share_dict}
@common.ViewBuilder.versioned_method("2.2")

2
manila/share/api.py
View File

@ -1518,6 +1518,8 @@ class API(base.Base):
is_public = strutils.bool_from_string(is_public, strict=True)
# Get filtered list of shares
if 'host' in search_opts:
policy.check_policy(context, 'share', 'list_by_host')
if 'share_server_id' in search_opts:
# NOTE(vponomaryov): this is project_id independent
policy.check_policy(context, 'share', 'list_by_share_server_id')

82
manila/tests/api/v1/test_shares.py
View File

@ -90,7 +90,6 @@ class ShareAPITest(test.TestCase):
'export_location': 'fake_location',
'export_locations': ['fake_location', 'fake_location2'],
'project_id': 'fakeproject',
'host': 'fakehost',
'created_at': datetime.datetime(1, 1, 1, 1, 1, 1),
'share_proto': 'FAKEPROTO',
'metadata': {},
@ -123,6 +122,7 @@ class ShareAPITest(test.TestCase):
share['share_proto'] = share['share_proto'].upper()
if admin:
share['share_server_id'] = 'fake_share_server_id'
share['host'] = 'fakehost'
return {'share': share}
@ddt.data("1.0", "2.0", "2.1")
@ -501,7 +501,6 @@ class ShareAPITest(test.TestCase):
'share_server_id': 'fake_share_server_id',
'share_type_id': 'fake_share_type_id',
'snapshot_id': 'fake_snapshot_id',
'host': 'fake_host',
'share_network_id': 'fake_share_network_id',
'metadata': '%7B%27k1%27%3A+%27v1%27%7D', # serialized k1=v1
'extra_specs': '%7B%27k2%27%3A+%27v2%27%7D', # serialized k2=v2
@ -511,6 +510,8 @@ class ShareAPITest(test.TestCase):
'offset': '1',
'is_public': 'False',
}
if use_admin_context:
search_opts['host'] = 'fake_host'
# fake_key should be filtered for non-admin
url = '/shares?fake_key=fake_value'
for k, v in search_opts.items():
@ -533,7 +534,6 @@ class ShareAPITest(test.TestCase):
'share_server_id': search_opts['share_server_id'],
'share_type_id': search_opts['share_type_id'],
'snapshot_id': search_opts['snapshot_id'],
'host': search_opts['host'],
'share_network_id': search_opts['share_network_id'],
'metadata': {'k1': 'v1'},
'extra_specs': {'k2': 'v2'},
@ -541,6 +541,7 @@ class ShareAPITest(test.TestCase):
}
if use_admin_context:
search_opts_expected.update({'fake_key': 'fake_value'})
search_opts_expected['host'] = search_opts['host']
share_api.API.get_all.assert_called_once_with(
req.environ['manila.context'],
sort_key=search_opts['sort_key'],
@ -590,7 +591,6 @@ class ShareAPITest(test.TestCase):
'share_server_id': 'fake_share_server_id',
'share_type_id': 'fake_share_type_id',
'snapshot_id': 'fake_snapshot_id',
'host': 'fake_host',
'share_network_id': 'fake_share_network_id',
'metadata': '%7B%27k1%27%3A+%27v1%27%7D', # serialized k1=v1
'extra_specs': '%7B%27k2%27%3A+%27v2%27%7D', # serialized k2=v2
@ -600,6 +600,8 @@ class ShareAPITest(test.TestCase):
'offset': '1',
'is_public': 'False',
}
if use_admin_context:
search_opts['host'] = 'fake_host'
# fake_key should be filtered for non-admin
url = '/shares/detail?fake_key=fake_value'
for k, v in search_opts.items():
@ -630,7 +632,6 @@ class ShareAPITest(test.TestCase):
'share_server_id': search_opts['share_server_id'],
'share_type_id': search_opts['share_type_id'],
'snapshot_id': search_opts['snapshot_id'],
'host': search_opts['host'],
'share_network_id': search_opts['share_network_id'],
'metadata': {'k1': 'v1'},
'extra_specs': {'k2': 'v2'},
@ -638,6 +639,7 @@ class ShareAPITest(test.TestCase):
}
if use_admin_context:
search_opts_expected.update({'fake_key': 'fake_value'})
search_opts_expected['host'] = search_opts['host']
share_api.API.get_all.assert_called_once_with(
req.environ['manila.context'],
sort_key=search_opts['sort_key'],
@ -656,8 +658,9 @@ class ShareAPITest(test.TestCase):
shares[1]['share_type_id'], result['shares'][0]['share_type'])
self.assertEqual(
shares[1]['snapshot_id'], result['shares'][0]['snapshot_id'])
self.assertEqual(
shares[1]['instance']['host'], result['shares'][0]['host'])
if use_admin_context:
self.assertEqual(
shares[1]['instance']['host'], result['shares'][0]['host'])
self.assertEqual(
shares[1]['instance']['share_network_id'],
result['shares'][0]['share_network_id'])
@ -668,42 +671,41 @@ class ShareAPITest(test.TestCase):
def test_share_list_detail_with_search_opts_by_admin(self):
self._share_list_detail_with_search_opts(use_admin_context=True)
def _list_detail_common_expected(self):
return {
'shares': [
def _list_detail_common_expected(self, admin=False):
share_dict = {
'status': 'fakestatus',
'description': 'displaydesc',
'export_location': 'fake_location',
'export_locations': ['fake_location', 'fake_location2'],
'availability_zone': 'fakeaz',
'name': 'displayname',
'share_proto': 'FAKEPROTO',
'metadata': {},
'project_id': 'fakeproject',
'id': '1',
'snapshot_id': '2',
'snapshot_support': True,
'share_network_id': None,
'created_at': datetime.datetime(1, 1, 1, 1, 1, 1),
'size': 1,
'share_type': '1',
'volume_type': '1',
'is_public': False,
'links': [
{
'status': 'fakestatus',
'description': 'displaydesc',
'export_location': 'fake_location',
'export_locations': ['fake_location', 'fake_location2'],
'availability_zone': 'fakeaz',
'name': 'displayname',
'share_proto': 'FAKEPROTO',
'metadata': {},
'project_id': 'fakeproject',
'host': 'fakehost',
'id': '1',
'snapshot_id': '2',
'snapshot_support': True,
'share_network_id': None,
'created_at': datetime.datetime(1, 1, 1, 1, 1, 1),
'size': 1,
'share_type': '1',
'volume_type': '1',
'is_public': False,
'links': [
{
'href': 'http://localhost/v1/fake/shares/1',
'rel': 'self'
},
{
'href': 'http://localhost/fake/shares/1',
'rel': 'bookmark'
}
],
'href': 'http://localhost/v1/fake/shares/1',
'rel': 'self'
},
{
'href': 'http://localhost/fake/shares/1',
'rel': 'bookmark'
}
]
],
}
if admin:
share_dict['host'] = 'fakehost'
return {'shares': [share_dict]}
def _list_detail_test_common(self, req, expected):
self.mock_object(share_api.API, 'get_all',

2
manila/tests/api/v2/test_share_replicas.py
View File

@ -81,7 +81,6 @@ class ShareReplicasApiTest(test.TestCase):
if not summary:
expected_replica.update({
'host': replica['host'],
'availability_zone': None,
'created_at': None,
'share_network_id': replica['share_network_id'],
@ -90,6 +89,7 @@ class ShareReplicasApiTest(test.TestCase):
if admin:
expected_replica['share_server_id'] = replica['share_server_id']
expected_replica['host'] = replica['host']
return replica, expected_replica

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

@ -105,7 +105,6 @@ class ShareAPITest(test.TestCase):
'export_location': 'fake_location',
'export_locations': ['fake_location', 'fake_location2'],
'project_id': 'fakeproject',
'host': 'fakehost',
'created_at': datetime.datetime(1, 1, 1, 1, 1, 1),
'share_proto': 'FAKEPROTO',
'metadata': {},
@ -140,6 +139,7 @@ class ShareAPITest(test.TestCase):
share['share_proto'] = share['share_proto'].upper()
if admin:
share['share_server_id'] = 'fake_share_server_id'
share['host'] = 'fakehost'
return {'share': share}
def test__revert(self):
@ -1414,7 +1414,6 @@ class ShareAPITest(test.TestCase):
'share_server_id': 'fake_share_server_id',
'share_type_id': 'fake_share_type_id',
'snapshot_id': 'fake_snapshot_id',
'host': 'fake_host',
'share_network_id': 'fake_share_network_id',
'metadata': '%7B%27k1%27%3A+%27v1%27%7D', # serialized k1=v1
'extra_specs': '%7B%27k2%27%3A+%27v2%27%7D', # serialized k2=v2
@ -1424,6 +1423,8 @@ class ShareAPITest(test.TestCase):
'offset': '1',
'is_public': 'False',
}
if use_admin_context:
search_opts['host'] = 'fake_host'
# fake_key should be filtered for non-admin
url = '/shares?fake_key=fake_value'
for k, v in search_opts.items():
@ -1446,7 +1447,7 @@ class ShareAPITest(test.TestCase):
'share_server_id': search_opts['share_server_id'],
'share_type_id': search_opts['share_type_id'],
'snapshot_id': search_opts['snapshot_id'],
'host': search_opts['host'],
'share_network_id': search_opts['share_network_id'],
'metadata': {'k1': 'v1'},
'extra_specs': {'k2': 'v2'},
@ -1454,6 +1455,7 @@ class ShareAPITest(test.TestCase):
}
if use_admin_context:
search_opts_expected.update({'fake_key': 'fake_value'})
search_opts_expected['host'] = search_opts['host']
share_api.API.get_all.assert_called_once_with(
req.environ['manila.context'],
sort_key=search_opts['sort_key'],
@ -1503,7 +1505,6 @@ class ShareAPITest(test.TestCase):
'share_server_id': 'fake_share_server_id',
'share_type_id': 'fake_share_type_id',
'snapshot_id': 'fake_snapshot_id',
'host': 'fake_host',
'share_network_id': 'fake_share_network_id',
'metadata': '%7B%27k1%27%3A+%27v1%27%7D', # serialized k1=v1
'extra_specs': '%7B%27k2%27%3A+%27v2%27%7D', # serialized k2=v2
@ -1513,6 +1514,8 @@ class ShareAPITest(test.TestCase):
'offset': '1',
'is_public': 'False',
}
if use_admin_context:
search_opts['host'] = 'fake_host'
# fake_key should be filtered for non-admin
url = '/shares/detail?fake_key=fake_value'
for k, v in search_opts.items():
@ -1545,7 +1548,6 @@ class ShareAPITest(test.TestCase):
'share_server_id': search_opts['share_server_id'],
'share_type_id': search_opts['share_type_id'],
'snapshot_id': search_opts['snapshot_id'],
'host': search_opts['host'],
'share_network_id': search_opts['share_network_id'],
'metadata': {'k1': 'v1'},
'extra_specs': {'k2': 'v2'},
@ -1553,6 +1555,7 @@ class ShareAPITest(test.TestCase):
}
if use_admin_context:
search_opts_expected.update({'fake_key': 'fake_value'})
search_opts_expected['host'] = search_opts['host']
share_api.API.get_all.assert_called_once_with(
req.environ['manila.context'],
sort_key=search_opts['sort_key'],
@ -1571,8 +1574,9 @@ class ShareAPITest(test.TestCase):
shares[1]['share_type_id'], result['shares'][0]['share_type'])
self.assertEqual(
shares[1]['snapshot_id'], result['shares'][0]['snapshot_id'])
self.assertEqual(
shares[1]['instance']['host'], result['shares'][0]['host'])
if use_admin_context:
self.assertEqual(
shares[1]['instance']['host'], result['shares'][0]['host'])
self.assertEqual(
shares[1]['instance']['share_network_id'],
result['shares'][0]['share_network_id'])
@ -1583,42 +1587,40 @@ class ShareAPITest(test.TestCase):
def test_share_list_detail_with_search_opts_by_admin(self):
self._share_list_detail_with_search_opts(use_admin_context=True)
def _list_detail_common_expected(self):
return {
'shares': [
def _list_detail_common_expected(self, admin=False):
share_dict = {
'status': 'fakestatus',
'description': 'displaydesc',
'export_location': 'fake_location',
'export_locations': ['fake_location', 'fake_location2'],
'availability_zone': 'fakeaz',
'name': 'displayname',
'share_proto': 'FAKEPROTO',
'metadata': {},
'project_id': 'fakeproject',
'id': '1',
'snapshot_id': '2',
'snapshot_support': True,
'share_network_id': None,
'created_at': datetime.datetime(1, 1, 1, 1, 1, 1),
'size': 1,
'share_type': '1',
'volume_type': '1',
'is_public': False,
'links': [
{
'status': 'fakestatus',
'description': 'displaydesc',
'export_location': 'fake_location',
'export_locations': ['fake_location', 'fake_location2'],
'availability_zone': 'fakeaz',
'name': 'displayname',
'share_proto': 'FAKEPROTO',
'metadata': {},
'project_id': 'fakeproject',
'host': 'fakehost',
'id': '1',
'snapshot_id': '2',
'snapshot_support': True,
'share_network_id': None,
'created_at': datetime.datetime(1, 1, 1, 1, 1, 1),
'size': 1,
'share_type': '1',
'volume_type': '1',
'is_public': False,
'links': [
{
'href': 'http://localhost/v1/fake/shares/1',
'rel': 'self'
},
{
'href': 'http://localhost/fake/shares/1',
'rel': 'bookmark'
}
],
'href': 'http://localhost/v1/fake/shares/1',
'rel': 'self'
},
{
'href': 'http://localhost/fake/shares/1',
'rel': 'bookmark'
}
]
],
}
if admin:
share_dict['host'] = 'fakehost'
return {'shares': [share_dict]}
def _list_detail_test_common(self, req, expected):
self.mock_object(share_api.API, 'get_all',
@ -1692,7 +1694,6 @@ class ShareAPITest(test.TestCase):
'metadata': {},
'project_id': 'fakeproject',
'access_rules_status': 'active',
'host': 'fakehost',
'id': '1',
'snapshot_id': '2',
'share_network_id': None,

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

@ -249,24 +249,29 @@ class ShareAPITestCase(test.TestCase):
ctx, sort_dir='desc', sort_key='created_at', filters={})
self.assertEqual(_FAKE_LIST_OF_ALL_SHARES, shares)
def test_get_all_non_admin_filter_by_share_server(self):
@ddt.data(
({'share_server_id': 'fake_share_server'}, 'list_by_share_server_id'),
({'host': 'fake_host'}, 'list_by_host'),
)
@ddt.unpack
def test_get_all_by_non_admin_using_admin_filter(self, filters, policy):
def fake_policy_checker(*args, **kwargs):
if 'list_by_share_server_id' == args[2] and not args[0].is_admin:
if policy == args[2] and not args[0].is_admin:
raise exception.NotAuthorized
ctx = context.RequestContext('fake_uid', 'fake_pid_1', is_admin=False)
self.mock_object(share_api.policy, 'check_policy',
mock.Mock(side_effect=fake_policy_checker))
self.mock_object(
share_api.policy, 'check_policy',
mock.Mock(side_effect=fake_policy_checker))
self.assertRaises(
exception.NotAuthorized,
self.api.get_all,
ctx,
{'share_server_id': 'fake'},
)
self.api.get_all, ctx, filters)
share_api.policy.check_policy.assert_has_calls([
mock.call(ctx, 'share', 'get_all'),
mock.call(ctx, 'share', 'list_by_share_server_id'),
mock.call(ctx, 'share', policy),
])
def test_get_all_admin_filter_by_share_server_and_all_tenants(self):

3
manila_tempest_tests/tests/api/base.py
View File

@ -671,7 +671,8 @@ class BaseSharesTest(test.BaseTestCase):
def get_pools_for_replication_domain(self):
# Get the list of pools for the replication domain
pools = self.admin_client.list_pools(detail=True)['pools']
instance_host = self.shares[0]['host']
instance_host = self.admin_client.get_share(
self.shares[0]['id'])['host']
host_pool = [p for p in pools if p['name'] == instance_host][0]
rep_domain = host_pool['capabilities']['replication_domain']
pools_in_rep_domain = [p for p in pools if p['capabilities'][

4
manila_tempest_tests/tests/api/test_replication.py
View File

@ -25,7 +25,7 @@ from manila_tempest_tests.tests.api import base
CONF = config.CONF
_MIN_SUPPORTED_MICROVERSION = '2.11'
SUMMARY_KEYS = ['share_id', 'id', 'replica_state', 'status']
DETAIL_KEYS = SUMMARY_KEYS + ['availability_zone', 'host', 'updated_at',
DETAIL_KEYS = SUMMARY_KEYS + ['availability_zone', 'updated_at',
'share_network_id', 'created_at']
@ -201,7 +201,7 @@ class ReplicationTest(base.BaseSharesMixedTest):
cleanup_in_class=False)
self.shares_v2_client.get_share_replica(share_replica2['id'])
share_replicas = self.shares_v2_client.list_share_replicas(
share_replicas = self.admin_client.list_share_replicas(
share_id=self.shares[0]["id"])
replica_host_set = {r['host'] for r in share_replicas}

2
manila_tempest_tests/tests/api/test_replication_negative.py
View File

@ -185,7 +185,7 @@ class ReplicationNegativeTest(base.BaseSharesMixedTest):
hosts = [p['name'] for p in pools]
self.create_share_replica(self.share1["id"], self.replica_zone,
cleanup_in_class=False)
share_host = self.share1['host']
share_host = self.admin_client.get_share(self.share1['id'])['host']
for host in hosts:
if host != share_host:

2
manila_tempest_tests/tests/api/test_shares.py
View File

@ -42,7 +42,7 @@ class SharesNFSTest(base.BaseSharesTest):
share = self.create_share(self.protocol)
detailed_elements = {'name', 'id', 'availability_zone',
'description', 'project_id',
'host', 'created_at', 'share_proto', 'metadata',
'created_at', 'share_proto', 'metadata',
'size', 'snapshot_id', 'share_network_id',
'status', 'share_type', 'volume_type', 'links',
'is_public'}

19
manila_tempest_tests/tests/api/test_shares_actions.py
View File

@ -88,7 +88,7 @@ class SharesActionsTest(base.BaseSharesTest):
"status", "description", "links", "availability_zone",
"created_at", "project_id", "volume_type", "share_proto", "name",
"snapshot_id", "id", "size", "share_network_id", "metadata",
"host", "snapshot_id", "is_public",
"snapshot_id", "is_public",
]
if utils.is_microversion_lt(version, '2.9'):
expected_keys.extend(["export_location", "export_locations"])
@ -196,7 +196,7 @@ class SharesActionsTest(base.BaseSharesTest):
"status", "description", "links", "availability_zone",
"created_at", "project_id", "volume_type", "share_proto", "name",
"snapshot_id", "id", "size", "share_network_id", "metadata",
"host", "snapshot_id", "is_public", "share_type",
"snapshot_id", "is_public", "share_type",
]
if utils.is_microversion_lt(version, '2.9'):
keys.extend(["export_location", "export_locations"])
@ -283,19 +283,6 @@ class SharesActionsTest(base.BaseSharesTest):
if CONF.share.capability_create_share_from_snapshot_support:
self.assertFalse(self.shares[1]['id'] in [s['id'] for s in shares])
@tc.attr(base.TAG_POSITIVE, base.TAG_API_WITH_BACKEND)
def test_list_shares_with_detail_filter_by_host(self):
base_share = self.shares_client.get_share(self.shares[0]['id'])
filters = {'host': base_share['host']}
# list shares
shares = self.shares_client.list_shares_with_detail(params=filters)
# verify response
self.assertGreater(len(shares), 0)
for share in shares:
self.assertEqual(filters['host'], share['host'])
@tc.attr(base.TAG_POSITIVE, base.TAG_API_WITH_BACKEND)
@testtools.skipIf(
not CONF.share.multitenancy_enabled, "Only for multitenancy.")
@ -407,7 +394,7 @@ class SharesActionsTest(base.BaseSharesTest):
keys = [
"status", "description", "links", "availability_zone",
"created_at", "export_location", "share_proto", "host",
"created_at", "export_location", "share_proto",
"name", "snapshot_id", "id", "size", "project_id", "is_public",
]
[self.assertIn(key, sh.keys()) for sh in shares for key in keys]

6
manila_tempest_tests/tests/api/test_shares_negative.py
View File

@ -193,6 +193,12 @@ class SharesAPIOnlyNegativeTest(base.BaseSharesTest):
'fake-host', 'nfs', '/export/path',
'fake-type')
@tc.attr(base.TAG_NEGATIVE, base.TAG_API)
def test_list_by_user_with_host_filter(self):
self.assertRaises(lib_exc.Forbidden,
self.shares_v2_client.list_shares,
params={'host': 'fake_host'})
@tc.attr(base.TAG_NEGATIVE, base.TAG_API)
def test_list_by_share_server_by_user(self):
self.assertRaises(lib_exc.Forbidden,

2
manila_tempest_tests/tests/scenario/test_share_basic_ops.py
View File

@ -312,7 +312,7 @@ class ShareBasicOpsBase(manager.ShareScenarioTest):
instance = self.boot_instance(wait_until="BUILD")
self.create_share()
instance = self.wait_for_active_instance(instance["id"])
self.share = self.shares_client.get_share(self.share['id'])
self.share = self.shares_admin_v2_client.get_share(self.share['id'])
default_type = self.shares_v2_client.list_share_types(
default=True)['share_type']

10
releasenotes/notes/remove-host-field-from-shares-and-replicas-a087f85bc4a4ba45.yaml Normal file
View File

@ -0,0 +1,10 @@
---
critical:
- The "host" field is no longer returned in the JSON response of the /shares
and /share-replicas APIs when these APIs are invoked with non-admin
privileges. Applications that depend on this field must be updated as
necessary. The value of this field is privileged information and the
request context must specify administrator privileges when using these
APIs for the "host" field to be present. The use of "host" as a filter
key in the GET /shares API is controlled with the policy "list_by_host".
This policy defaults to "rule:admin_api".