diff --git a/manila/api/common.py b/manila/api/common.py index 67234c75a1..af9a88ca9e 100644 --- a/manila/api/common.py +++ b/manila/api/common.py @@ -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 diff --git a/manila/api/v1/share_manage.py b/manila/api/v1/share_manage.py index dc0ad7177a..90e812c8a3 100644 --- a/manila/api/v1/share_manage.py +++ b/manila/api/v1/share_manage.py @@ -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 diff --git a/manila/api/v1/shares.py b/manila/api/v1/shares.py index 2c859289f2..0e8a8b4e1e 100644 --- a/manila/api/v1/shares.py +++ b/manila/api/v1/shares.py @@ -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: diff --git a/manila/policies/shares.py b/manila/policies/shares.py index 18822215db..427b32fae5 100644 --- a/manila/policies/shares.py +++ b/manila/policies/shares.py @@ -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, diff --git a/manila/policy.py b/manila/policy.py index c4f0928451..9d767ecddd 100644 --- a/manila/policy.py +++ b/manila/policy.py @@ -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) diff --git a/manila/share/api.py b/manila/share/api.py index bb4c8076f5..9037516c3f 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -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') diff --git a/manila/tests/api/test_common.py b/manila/tests/api/test_common.py index f5255aa273..e6c70d4d6c 100644 --- a/manila/tests/api/test_common.py +++ b/manila/tests/api/test_common.py @@ -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): diff --git a/manila/tests/api/v1/test_share_manage.py b/manila/tests/api/v1/test_share_manage.py index 4183caf155..2707f0e7bb 100644 --- a/manila/tests/api/v1/test_share_manage.py +++ b/manila/tests/api/v1/test_share_manage.py @@ -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': {}}, diff --git a/manila/tests/api/v1/test_shares.py b/manila/tests/api/v1/test_shares.py index d471e94f1e..cc98089f81 100644 --- a/manila/tests/api/v1/test_shares.py +++ b/manila/tests/api/v1/test_shares.py @@ -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): diff --git a/manila/tests/api/v2/test_shares.py b/manila/tests/api/v2/test_shares.py index d827beb6ab..e6721370d2 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -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'], diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 28fb1dd5e4..643c625475 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -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', diff --git a/releasenotes/notes/bug-1801763-gate-public-share-creation-by-policy-a0ad84e4127a3fc3.yaml b/releasenotes/notes/bug-1801763-gate-public-share-creation-by-policy-a0ad84e4127a3fc3.yaml new file mode 100644 index 0000000000..605e9a0d76 --- /dev/null +++ b/releasenotes/notes/bug-1801763-gate-public-share-creation-by-policy-a0ad84e4127a3fc3.yaml @@ -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. \ No newline at end of file