openstack/manila
Code Issues Proposed changes

Non-admin user can perform 'extra-specs-list'

Browse Source 
This bug, inherited from Cinder, allows a tenant to view share
extra specs using the extra-specs-list CLI command.  The Cinder
fix was to check the admin context in the DB layer and filter out
all extra specs for non-admins.  This approach doesn't work for
Manila, because some extra specs are required and are effectively
part of the Manila API (DHSS, snapshot_support).  So in Manila we
define a set of tenant-visible extra specs and restrict the extra
spec values to that set in the share type view builder. Also, we
add policies for the share type list APIs so that admins can
control access to those if desired.

The separate API to list extra specs already has adequate checking
for non-admin users; the CLI was listing the extra specs returned
by the share type API, which is now filtered as described.

Co-Authored-By: Andrew Kerr <andrew.kerr@netapp.com>
Change-Id: I9b0a8ddc064c246286f26760b703db6e3e1bcd46
Closes-Bug: #1475285
This commit is contained in:
Clinton Knight 2015-09-08 18:47:18 -04:00
parent 04bf211f8a
commit 20a0b6bce0
8 changed files with 79 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
etc/manila/policy.json
View File

@ -27,6 +27,10 @@
"share:update_share_metadata": "rule:default",
"share:migrate": "rule:admin_api",
"share_type:index": "rule:default",
"share_type:show": "rule:default",
"share_type:default": "rule:default",
"share_instance:index": "rule:admin_api",
"share_instance:show": "rule:admin_api",

8
manila/api/v1/share_types.py
View File

@ -22,8 +22,11 @@ from manila.api.openstack import wsgi
from manila.api.views import types as views_types
from manila import exception
from manila.i18n import _
from manila import policy
from manila.share import share_types
RESOURCE_NAME = 'share_type'
class ShareTypesController(wsgi.Controller):
"""The share types API controller for the OpenStack API."""
@ -32,6 +35,9 @@ class ShareTypesController(wsgi.Controller):
def index(self, req):
"""Returns the list of share types."""
context = req.environ['manila.context']
policy.check_policy(context, RESOURCE_NAME, 'index')
limited_types = self._get_share_types(req)
req.cache_db_share_types(limited_types)
return self._view_builder.index(req, limited_types)
@ -39,6 +45,7 @@ class ShareTypesController(wsgi.Controller):
def show(self, req, id):
"""Return a single share type item."""
context = req.environ['manila.context']
policy.check_policy(context, RESOURCE_NAME, 'show')
try:
share_type = share_types.get_share_type(context, id)
@ -53,6 +60,7 @@ class ShareTypesController(wsgi.Controller):
def default(self, req):
"""Return default volume type."""
context = req.environ['manila.context']
policy.check_policy(context, RESOURCE_NAME, 'default')
try:
share_type = share_types.get_default_share_type(context)

22
manila/api/views/types.py
View File

@ -14,6 +14,7 @@
# under the License.
from manila.api import common
from manila.share import share_types
class ViewBuilder(common.ViewBuilder):
@ -22,11 +23,22 @@ class ViewBuilder(common.ViewBuilder):
def show(self, request, share_type, brief=False):
"""Trim away extraneous share type attributes."""
extra_specs = share_type.get('extra_specs', {})
required_extra_specs = share_type.get('required_extra_specs', {})
# Remove non-tenant-visible extra specs in a non-admin context
if not request.environ['manila.context'].is_admin:
extra_spec_names = share_types.get_tenant_visible_extra_specs()
extra_specs = self._filter_extra_specs(extra_specs,
extra_spec_names)
required_extra_specs = self._filter_extra_specs(
required_extra_specs, extra_spec_names)
trimmed = dict(id=share_type.get('id'),
name=share_type.get('name'),
extra_specs=share_type.get('extra_specs'),
required_extra_specs=share_type.get(
'required_extra_specs'))
extra_specs=extra_specs,
required_extra_specs=required_extra_specs)
if brief:
return trimmed
else:
@ -38,3 +50,7 @@ class ViewBuilder(common.ViewBuilder):
for share_type in share_types]
return dict(volume_types=share_types_list,
share_types=share_types_list)
def _filter_extra_specs(self, extra_specs, valid_keys):
return {key: value for key, value in extra_specs.items()
if key in valid_keys}

4
manila/common/constants.py
View File

@ -118,6 +118,10 @@ class ExtraSpecs(object):
DRIVER_HANDLES_SHARE_SERVERS,
SNAPSHOT_SUPPORT,
)
# NOTE(cknight): Some extra specs are necessary parts of the Manila API and
# should be visible to non-admin users. This list matches the UNDELETABLE
# list today, but that may not always remain true.
TENANT_VISIBLE = UNDELETABLE
BOOLEAN = (
DRIVER_HANDLES_SHARE_SERVERS,
SNAPSHOT_SUPPORT,

4
manila/share/share_types.py
View File

@ -203,6 +203,10 @@ def get_undeletable_extra_specs():
return constants.ExtraSpecs.UNDELETABLE
def get_tenant_visible_extra_specs():
return constants.ExtraSpecs.TENANT_VISIBLE
def get_boolean_extra_specs():
return constants.ExtraSpecs.BOOLEAN

27
manila/tests/api/v1/test_share_types.py
View File

@ -22,6 +22,7 @@ from manila.api.v1 import share_types as types
from manila.api.views import types as views_types
from manila.common import constants
from manila import exception
from manila import policy
from manila.share import share_types
from manila import test
from manila.tests.api import fakes
@ -75,12 +76,17 @@ class ShareTypesApiTest(test.TestCase):
def setUp(self):
super(ShareTypesApiTest, self).setUp()
self.controller = types.ShareTypesController()
self.mock_object(policy, 'check_policy',
mock.Mock(return_value=True))
def test_share_types_index(self):
@ddt.data(True, False)
def test_share_types_index(self, admin):
self.mock_object(share_types, 'get_all_types',
return_share_types_get_all_types)
req = fakes.HTTPRequest.blank('/v2/fake/types')
req = fakes.HTTPRequest.blank('/v2/fake/types',
use_admin_context=admin)
res_dict = self.controller.index(req)
self.assertEqual(3, len(res_dict['share_types']))
@ -89,11 +95,16 @@ class ShareTypesApiTest(test.TestCase):
actual_names = map(lambda e: e['name'], res_dict['share_types'])
self.assertEqual(set(actual_names), set(expected_names))
for entry in res_dict['share_types']:
self.assertEqual('value1', entry['extra_specs']['key1'])
if admin:
self.assertEqual('value1', entry['extra_specs'].get('key1'))
else:
self.assertIsNone(entry['extra_specs'].get('key1'))
self.assertTrue('required_extra_specs' in entry)
required_extra_spec = entry['required_extra_specs'].get(
constants.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS, '')
self.assertEqual('true', required_extra_spec)
policy.check_policy.assert_called_once_with(
req.environ['manila.context'], types.RESOURCE_NAME, 'index')
def test_share_types_index_no_data(self):
self.mock_object(share_types, 'get_all_types',
@ -103,6 +114,8 @@ class ShareTypesApiTest(test.TestCase):
res_dict = self.controller.index(req)
self.assertEqual(0, len(res_dict['share_types']))
policy.check_policy.assert_called_once_with(
req.environ['manila.context'], types.RESOURCE_NAME, 'index')
def test_share_types_show(self):
self.mock_object(share_types, 'get_share_type',
@ -114,6 +127,8 @@ class ShareTypesApiTest(test.TestCase):
self.assertEqual(2, len(res_dict))
self.assertEqual('1', res_dict['share_type']['id'])
self.assertEqual('share_type_1', res_dict['share_type']['name'])
policy.check_policy.assert_called_once_with(
req.environ['manila.context'], types.RESOURCE_NAME, 'show')
def test_share_types_show_not_found(self):
self.mock_object(share_types, 'get_share_type',
@ -122,6 +137,8 @@ class ShareTypesApiTest(test.TestCase):
req = fakes.HTTPRequest.blank('/v2/fake/types/777')
self.assertRaises(webob.exc.HTTPNotFound, self.controller.show,
req, '777')
policy.check_policy.assert_called_once_with(
req.environ['manila.context'], types.RESOURCE_NAME, 'show')
def test_share_types_default(self):
self.mock_object(share_types, 'get_default_share_type',
@ -133,6 +150,8 @@ class ShareTypesApiTest(test.TestCase):
self.assertEqual(2, len(res_dict))
self.assertEqual('1', res_dict['share_type']['id'])
self.assertEqual('share_type_1', res_dict['share_type']['name'])
policy.check_policy.assert_called_once_with(
req.environ['manila.context'], types.RESOURCE_NAME, 'default')
def test_share_types_default_not_found(self):
self.mock_object(share_types, 'get_default_share_type',
@ -141,6 +160,8 @@ class ShareTypesApiTest(test.TestCase):
req = fakes.HTTPRequest.blank('/v2/fake/types/default')
self.assertRaises(webob.exc.HTTPNotFound, self.controller.default, req)
policy.check_policy.assert_called_once_with(
req.environ['manila.context'], types.RESOURCE_NAME, 'default')
def test_view_builder_show(self):
view_builder = views_types.ViewBuilder()

4
manila/tests/policy.json
View File

@ -18,6 +18,10 @@
"share:extend": "",
"share:shrink": "",
"share_type:index": "rule:default",
"share_type:show": "rule:default",
"share_type:default": "rule:default",
"share_instance:index": "rule:admin_api",
"share_instance:show": "rule:admin_api",

12
manila_tempest_tests/tests/api/admin/test_share_types_extra_specs_negative.py
View File

@ -66,6 +66,18 @@ class ExtraSpecsAdminNegativeTest(base.BaseSharesAdminTest):
self.member_shares_client.get_share_type_extra_specs,
st["share_type"]["id"])
@test.attr(type=["gate", "smoke", ])
def test_try_read_extra_specs_on_share_type_with_user(self):
st = self._create_share_type()
share_type = self.member_shares_client.get_share_type(
st['share_type']['id'])
# Verify a non-admin can only read the required extra-specs
expected_keys = ['driver_handles_share_servers', 'snapshot_support']
actual_keys = share_type['share_type']['extra_specs'].keys()
self.assertEqual(sorted(expected_keys), sorted(actual_keys),
'Incorrect extra specs visible to non-admin user; '
'expected %s, got %s' % (expected_keys, actual_keys))
@test.attr(type=["gate", "smoke", ])
def test_try_update_extra_spec_with_user(self):
st = self._create_share_type()