openstack
/
manila
Code Issues Proposed changes

Merge "Add policy to create/update public shares"

This commit is contained in:
Zuul 2019-02-19 11:22:21 +00:00 committed by Gerrit Code Review
parent 79504a3d7a 2289cdd9e7
commit ba43419fa6
12 changed files with 234 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
manila/api/common.py
View File

@ -22,12 +22,15 @@ import string
from oslo_config import cfg
from oslo_log import log
from oslo_utils import encodeutils
from oslo_utils import strutils
from six.moves.urllib import parse
import webob
from manila.api.openstack import api_version_request as api_version
from manila.api.openstack import versioned_method
from manila import exception
from manila.i18n import _
from manila import policy
api_common_opts = [
cfg.IntOpt(
@ -390,3 +393,35 @@ def validate_access(*args, **kwargs):
"are supported.")
raise webob.exc.HTTPBadRequest(explanation=exc_str)
def validate_public_share_policy(context, api_params, api='create'):
"""Validates if policy allows is_public parameter to be set to True.
:arg api_params - A dictionary of values that may contain 'is_public'
:returns api_params with 'is_public' item sanitized if present
:raises exception.InvalidParameterValue if is_public is set but is Invalid
exception.NotAuthorized if is_public is True but policy prevents it
"""
if 'is_public' not in api_params:
return api_params
policies = {
'create': 'create_public_share',
'update': 'set_public_share',
}
policy_to_check = policies[api]
try:
api_params['is_public'] = strutils.bool_from_string(
api_params['is_public'], strict=True)
except ValueError as e:
raise exception.InvalidParameterValue(six.text_type(e))
public_shares_allowed = policy.check_policy(
context, 'share', policy_to_check, do_raise=False)
if api_params['is_public'] and not public_shares_allowed:
message = _("User is not authorized to set 'is_public' to True in the "
"request.")
raise exception.NotAuthorized(message=message)
return api_params

2
manila/api/v1/share_manage.py
View File

@ -15,6 +15,7 @@
import six
from webob import exc
from manila.api import common
from manila.api.openstack import wsgi
from manila.api.views import shares as share_views
from manila import exception
@ -31,6 +32,7 @@ class ShareManageMixin(object):
def _manage(self, req, body):
context = req.environ['manila.context']
share_data = self._validate_manage_parameters(context, body)
share_data = common.validate_public_share_policy(context, share_data)
# NOTE(vponomaryov): compatibility actions are required between API and
# DB layers for 'name' and 'description' API params that are

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

@ -199,6 +199,7 @@ class ShareMixin(object):
'display_description'
)
@wsgi.Controller.authorize
def update(self, req, id, body):
"""Update a share."""
context = req.environ['manila.context']
@ -222,6 +223,9 @@ class ShareMixin(object):
except exception.NotFound:
raise exc.HTTPNotFound()
update_dict = common.validate_public_share_policy(
context, update_dict, api='update')
share = self.share_api.update(context, share, update_dict)
share.update(update_dict)
return self._view_builder.detail(req, share)
@ -232,6 +236,7 @@ class ShareMixin(object):
share = self._create(req, body)
return share
@wsgi.Controller.authorize('create')
def _create(self, req, body,
check_create_share_from_snapshot_support=False,
check_availability_zones_extra_spec=False):
@ -242,7 +247,7 @@ class ShareMixin(object):
raise exc.HTTPUnprocessableEntity()
share = body['share']
availability_zone_id = None
share = common.validate_public_share_policy(context, share)
# NOTE(rushiagr): Manila API allows 'name' instead of 'display_name'.
if share.get('name'):
@ -262,6 +267,7 @@ class ShareMixin(object):
{'share_proto': share_proto, 'size': size})
LOG.info(msg, context=context)
availability_zone_id = None
availability_zone = share.get('availability_zone')
if availability_zone:
try:

49
manila/policies/shares.py
View File

@ -17,6 +17,17 @@ from manila.policies import base
BASE_POLICY_NAME = 'share:%s'
# These deprecated rules can be removed in the 'Train' release.
deprecated_create_public_share_rule = policy.DeprecatedRule(
name=BASE_POLICY_NAME % 'create_public_share',
check_str=base.RULE_DEFAULT,
)
deprecated_set_public_share_rule = policy.DeprecatedRule(
name=BASE_POLICY_NAME % 'set_public_share',
check_str=base.RULE_DEFAULT,
)
shares_policies = [
policy.DocumentedRuleDefault(
@ -29,6 +40,25 @@ shares_policies = [
'path': '/shares',
}
]),
policy.DocumentedRuleDefault(
name=BASE_POLICY_NAME % 'create_public_share',
check_str=base.RULE_ADMIN_API,
description="Create shares visible across all projects in the cloud. "
"This option will default to rule:admin_api in the "
"9.0.0 (Train) release of the OpenStack Shared File "
"Systems (manila) service.",
deprecated_rule=deprecated_create_public_share_rule,
deprecated_reason="Public shares must be accessible across the "
"cloud, irrespective of project namespaces. To "
"avoid unintended consequences, rule:admin_api "
"serves as a better default for this policy.",
deprecated_since='S',
operations=[
{
'method': 'POST',
'path': '/shares',
}
]),
policy.DocumentedRuleDefault(
name=BASE_POLICY_NAME % 'get',
check_str=base.RULE_DEFAULT,
@ -63,6 +93,25 @@ shares_policies = [
'path': '/shares',
}
]),
policy.DocumentedRuleDefault(
name=BASE_POLICY_NAME % 'set_public_share',
check_str=base.RULE_ADMIN_API,
description="Update shares to be visible across all projects in the "
"cloud. This option will default to rule:admin_api in the "
"9.0.0 (Train) release of the OpenStack Shared File "
"Systems (manila) service.",
deprecated_rule=deprecated_set_public_share_rule,
deprecated_reason="Public shares must be accessible across the "
"cloud, irrespective of project namespaces. To "
"avoid unintended consequences, rule:admin_api "
"serves as a better default for this policy.",
deprecated_since='S',
operations=[
{
'method': 'PUT',
'path': '/shares',
}
]),
policy.DocumentedRuleDefault(
name=BASE_POLICY_NAME % 'delete',
check_str=base.RULE_DEFAULT,

4
manila/policy.py
View File

@ -199,11 +199,11 @@ def wrap_check_policy(resource):
return check_policy_wraper
def check_policy(context, resource, action, target_obj=None):
def check_policy(context, resource, action, target_obj=None, do_raise=True):
target = {
'project_id': context.project_id,
'user_id': context.user_id,
}
target.update(target_obj or {})
_action = '%s:%s' % (resource, action)
authorize(context, _action, target)
return authorize(context, _action, target, do_raise=do_raise)

16
manila/share/api.py
View File

@ -72,7 +72,6 @@ class API(base.Base):
share_group_id=None, share_group_snapshot_member=None,
availability_zones=None):
"""Create new share."""
policy.check_policy(context, 'share', 'create')
self._check_metadata_properties(metadata)
@ -169,11 +168,6 @@ class API(base.Base):
'd_consumed': _consumed('shares')})
raise exception.ShareLimitExceeded(allowed=quotas['shares'])
try:
is_public = strutils.bool_from_string(is_public, strict=True)
except ValueError as e:
raise exception.InvalidParameterValue(six.text_type(e))
share_group = None
if share_group_id:
try:
@ -335,8 +329,6 @@ class API(base.Base):
host=None, availability_zone=None,
share_group=None, share_group_snapshot_member=None,
share_type_id=None, availability_zones=None):
policy.check_policy(context, 'share', 'create')
request_spec, share_instance = (
self.create_share_instance_and_get_request_spec(
context, share, availability_zone=availability_zone,
@ -611,8 +603,6 @@ class API(base.Base):
self.share_rpcapi.update_share_replica(context, share_replica)
def manage(self, context, share_data, driver_options):
policy.check_policy(context, 'share', 'manage')
shares = self.get_all(context, {
'host': share_data['host'],
'export_location': share_data['export_location'],
@ -1483,12 +1473,6 @@ class API(base.Base):
@policy.wrap_check_policy('share')
def update(self, context, share, fields):
if 'is_public' in fields:
try:
fields['is_public'] = strutils.bool_from_string(
fields['is_public'], strict=True)
except ValueError as e:
raise exception.InvalidParameterValue(six.text_type(e))
return self.db.share_update(context, share['id'], fields)
@policy.wrap_check_policy('share')

60
manila/tests/api/test_common.py
View File

@ -18,10 +18,13 @@ Test suites for 'common' code used throughout the OpenStack HTTP API.
"""
import ddt
import mock
import webob
import webob.exc
from manila.api import common
from manila import exception
from manila import policy
from manila import test
from manila.tests.api import fakes
from manila.tests.db import fakes as db_fakes
@ -282,6 +285,63 @@ class MiscFunctionsTest(test.TestCase):
access_type=access_type, access_to=access_to,
enable_ceph=ceph)
def test_validate_public_share_policy_no_is_public(self):
api_params = {'foo': 'bar', 'clemson': 'tigers'}
self.mock_object(policy, 'check_policy')
actual_params = common.validate_public_share_policy(
'fake_context', api_params)
self.assertDictMatch(api_params, actual_params)
policy.check_policy.assert_not_called()
@ddt.data('foo', 123, 'all', None)
def test_validate_public_share_policy_invalid_value(self, is_public):
api_params = {'is_public': is_public}
self.mock_object(policy, 'check_policy')
self.assertRaises(exception.InvalidParameterValue,
common.validate_public_share_policy,
'fake_context',
api_params)
policy.check_policy.assert_not_called()
@ddt.data('1', True, 'true', 'yes')
def test_validate_public_share_not_authorized(self, is_public):
api_params = {'is_public': is_public, 'size': '16'}
self.mock_object(policy, 'check_policy', mock.Mock(return_value=False))
self.assertRaises(exception.NotAuthorized,
common.validate_public_share_policy,
'fake_context',
api_params)
policy.check_policy.assert_called_once_with(
'fake_context', 'share', 'create_public_share', do_raise=False)
@ddt.data('0', False, 'false', 'no')
def test_validate_public_share_is_public_False(self, is_public):
api_params = {'is_public': is_public, 'size': '16'}
self.mock_object(policy, 'check_policy', mock.Mock(return_value=False))
actual_params = common.validate_public_share_policy(
'fake_context', api_params, api='update')
self.assertDictMatch({'is_public': False, 'size': '16'}, actual_params)
policy.check_policy.assert_called_once_with(
'fake_context', 'share', 'set_public_share', do_raise=False)
@ddt.data('1', True, 'true', 'yes')
def test_validate_public_share_is_public_True(self, is_public):
api_params = {'is_public': is_public, 'size': '16'}
self.mock_object(policy, 'check_policy', mock.Mock(return_value=True))
actual_params = common.validate_public_share_policy(
'fake_context', api_params, api='update')
self.assertDictMatch({'is_public': True, 'size': '16'}, actual_params)
policy.check_policy.assert_called_once_with(
'fake_context', 'share', 'set_public_share', do_raise=False)
@ddt.ddt
class ViewBuilderTest(test.TestCase):

4
manila/tests/api/v1/test_share_manage.py
View File

@ -17,6 +17,7 @@ import ddt
import mock
import webob
from manila.api import common
from manila.api.v1 import share_manage
from manila.db import api as db_api
from manila import exception
@ -52,6 +53,9 @@ class ShareManageTest(test.TestCase):
self.context = self.request.environ['manila.context']
self.mock_policy_check = self.mock_object(
policy, 'check_policy', mock.Mock(return_value=True))
self.mock_object(
common, 'validate_public_share_policy',
mock.Mock(side_effect=lambda *args, **kwargs: args[1]))
@ddt.data({},
{'shares': {}},

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

@ -59,6 +59,11 @@ class ShareAPITest(test.TestCase):
stubs.stub_snapshot_get)
self.mock_object(share_types, 'get_share_type',
stubs.stub_share_type_get)
self.mock_object(
common, 'validate_public_share_policy',
mock.Mock(side_effect=lambda *args, **kwargs: args[1]))
self.resource_name = self.controller.resource_name
self.mock_policy_check = self.mock_object(policy, 'check_policy')
self.maxDiff = None
self.share = {
"size": 100,
@ -142,6 +147,8 @@ class ShareAPITest(test.TestCase):
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(self.share)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -154,6 +161,8 @@ class ShareAPITest(test.TestCase):
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(self.share)
self.assertEqual(expected, res_dict)
@ -167,6 +176,8 @@ class ShareAPITest(test.TestCase):
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(self.share)
expected['share'].pop('snapshot_support')
share_types.get_share_type_by_name.assert_called_once_with(
@ -181,8 +192,11 @@ class ShareAPITest(test.TestCase):
)
CONF.set_default("default_share_type", self.vt['name'])
req = fakes.HTTPRequest.blank('/shares')
self.assertRaises(exception.ShareTypeNotFoundByName,
self.controller.create, req, {'share': self.share})
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
share_types.get_default_share_type.assert_called_once_with()
def test_share_create_with_dhss_true_and_network_notexist(self):
@ -199,8 +213,11 @@ class ShareAPITest(test.TestCase):
)
CONF.set_default("default_share_type", fake_share_type['name'])
req = fakes.HTTPRequest.blank('/shares')
self.assertRaises(webob.exc.HTTPBadRequest,
self.controller.create, req, {'share': self.share})
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
share_types.get_default_share_type.assert_called_once_with()
def test_share_create_with_share_net(self):
@ -227,6 +244,8 @@ class ShareAPITest(test.TestCase):
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -254,7 +273,11 @@ class ShareAPITest(test.TestCase):
self.mock_object(share_api.API, 'create', create_mock)
body = {"share": copy.deepcopy(shr)}
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -292,7 +315,11 @@ class ShareAPITest(test.TestCase):
body = {"share": copy.deepcopy(shr)}
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -332,7 +359,11 @@ class ShareAPITest(test.TestCase):
body = {"share": copy.deepcopy(shr)}
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -352,10 +383,13 @@ class ShareAPITest(test.TestCase):
}
body = {"share": shr}
req = fakes.HTTPRequest.blank('/shares')
self.assertRaises(webob.exc.HTTPBadRequest,
self.controller.create,
req,
body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
@ddt.data("1.0", "2.0")
def test_share_create_from_snapshot_not_supported(self, microversion):
@ -397,6 +431,8 @@ class ShareAPITest(test.TestCase):
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertDictEqual(expected, res_dict)
@ -415,6 +451,8 @@ class ShareAPITest(test.TestCase):
self.controller.create,
req,
body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
def test_share_create_no_body(self):
body = {}
@ -423,6 +461,8 @@ class ShareAPITest(test.TestCase):
self.controller.create,
req,
body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
def test_share_create_invalid_availability_zone(self):
self.mock_object(
@ -482,7 +522,11 @@ class ShareAPITest(test.TestCase):
body = {"share": shr}
req = fakes.HTTPRequest.blank('/share/1')
res_dict = self.controller.update(req, 1, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'update')
self.assertEqual(shr["display_name"], res_dict['share']["name"])
self.assertEqual(shr["display_description"],
res_dict['share']["description"])
@ -491,7 +535,11 @@ class ShareAPITest(test.TestCase):
def test_share_not_updates_size(self):
req = fakes.HTTPRequest.blank('/share/1')
res_dict = self.controller.update(req, 1, {"share": self.share})
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'update')
self.assertNotEqual(res_dict['share']["size"], self.share["size"])
def test_share_delete_no_share(self):

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

@ -2808,6 +2808,9 @@ class ShareManageTest(test.TestCase):
)
self.mock_object(
share_api.API, 'manage', return_share)
self.mock_object(
common, 'validate_public_share_policy',
mock.Mock(side_effect=lambda *args, **kwargs: args[1]))
share = {
'host': data['share']['service_host'],
'export_location': data['share']['export_path'],

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

@ -662,12 +662,6 @@ class ShareAPITestCase(test.TestCase):
self.assertSubDictMatch(share_data,
db_api.share_create.call_args[0][1])
@ddt.data('', 'fake', 'Truebar', 'Bartrue')
def test_create_share_with_invalid_is_public_value(self, is_public):
self.assertRaises(exception.InvalidParameterValue,
self.api.create, self.context, 'nfs', '1',
'fakename', 'fakedesc', is_public=is_public)
@ddt.data(*constants.SUPPORTED_SHARE_PROTOCOLS)
def test_create_share_valid_protocol(self, proto):
share, share_data = self._setup_create_mocks(protocol=proto)
@ -1702,9 +1696,8 @@ class ShareAPITestCase(test.TestCase):
availability_zone=snapshot['share']['availability_zone'],
share_group=None, share_group_snapshot_member=None,
availability_zones=None)
share_api.policy.check_policy.assert_has_calls([
mock.call(self.context, 'share', 'create'),
mock.call(self.context, 'share_snapshot', 'get_snapshot')])
share_api.policy.check_policy.assert_called_once_with(
self.context, 'share_snapshot', 'get_snapshot')
quota.QUOTAS.reserve.assert_called_once_with(
self.context, share_type_id=share_type['id'],
gigabytes=1, shares=1)
@ -1885,12 +1878,6 @@ class ShareAPITestCase(test.TestCase):
instance
)
@ddt.data('', 'fake', 'Truebar', 'Bartrue')
def test_update_share_with_invalid_is_public_value(self, is_public):
self.assertRaises(exception.InvalidParameterValue,
self.api.update, self.context, 'fakeid',
{'is_public': is_public})
def test_get(self):
share = db_utils.create_share()
with mock.patch.object(db_api, 'share_get',

22
releasenotes/notes/bug-1801763-gate-public-share-creation-by-policy-a0ad84e4127a3fc3.yaml Normal file
View File

@ -0,0 +1,22 @@
---
features:
- |
New API policies (share:create_public_share and share:set_public_share)
have been introduced for the "create" (POST /shares) and "update"
(PUT /shares) APIs to validate requests to create publicly visible
shares.
deprecations:
- |
The API policies to create publicly visible shares
(share:create_public_share) or modify existing shares to become publicly
visible (share:set_public_share) have their default value changed to
rule:admin_api. This means that these APIs (POST /shares and PUT
/shares) will allow the 'is_public' parameter to be set to True in the
request body if the requester's role is set to an Administrator role.
These policies will allow their previous default behavior in the Stein
release (8.0.0) (i.e., any user can create publicly visible shares and
even non-privileged users within a project can update their shares to
become publicly visible). If the previous default behavior is always
desired, deployers *must* explicitly set "share:create_public_share"
and "share:set_public_share" to "rule:default" in their policy.json
file.