diff --git a/nova/api/openstack/compute/views/servers.py b/nova/api/openstack/compute/views/servers.py index c14eee5325..8e7b8d3019 100644 --- a/nova/api/openstack/compute/views/servers.py +++ b/nova/api/openstack/compute/views/servers.py @@ -32,7 +32,6 @@ from nova import objects from nova.objects import fields from nova.objects import virtual_interface from nova.policies import extended_server_attributes as esa_policies -from nova.policies import flavor_extra_specs as fes_policies from nova.policies import servers as servers_policies from nova import utils @@ -234,7 +233,9 @@ class ViewBuilder(common.ViewBuilder): if api_version_request.is_supported(request, min_version='2.47'): context = request.environ['nova.context'] show_extra_specs = context.can( - fes_policies.POLICY_ROOT % 'index', fatal=False) + servers_policies.SERVERS % 'show:flavor-extra-specs', + fatal=False, + target={'project_id': instance.project_id}) if cell_down_support and 'display_name' not in instance: # NOTE(tssurya): If the microversion is >= 2.69, this boolean will @@ -437,8 +438,9 @@ class ViewBuilder(common.ViewBuilder): if api_version_request.is_supported(request, min_version='2.47'): # Determine if we should show extra_specs in the inlined flavor # once before we iterate the list of instances - show_extra_specs = context.can(fes_policies.POLICY_ROOT % 'index', - fatal=False) + show_extra_specs = context.can( + servers_policies.SERVERS % 'show:flavor-extra-specs', + fatal=False) else: show_extra_specs = False show_extended_attr = context.can( diff --git a/nova/policies/base.py b/nova/policies/base.py index afa6e4c0c7..04ab8272a1 100644 --- a/nova/policies/base.py +++ b/nova/policies/base.py @@ -56,6 +56,7 @@ PROJECT_MEMBER = 'rule:project_member_api' PROJECT_READER = 'rule:project_reader_api' PROJECT_MEMBER_OR_SYSTEM_ADMIN = 'rule:system_admin_or_owner' PROJECT_READER_OR_SYSTEM_READER = 'rule:system_or_project_reader' +PROJECT_READER_OR_ADMIN = 'rule:project_reader_or_admin' ADMIN = 'rule:context_is_admin' # NOTE(gmann): Below is the mapping of new roles and scope_types @@ -139,6 +140,11 @@ rules = [ "system_or_project_reader", "rule:system_reader_api or rule:project_reader_api", "Default rule for System+Project read only APIs.", + deprecated_rule=DEPRECATED_ADMIN_OR_OWNER_POLICY), + policy.RuleDefault( + "project_reader_or_admin", + "rule:project_reader_api or rule:context_is_admin", + "Default rule for Project reader and admin APIs.", deprecated_rule=DEPRECATED_ADMIN_OR_OWNER_POLICY) ] diff --git a/nova/policies/flavor_extra_specs.py b/nova/policies/flavor_extra_specs.py index 7335763d4b..06b486bf49 100644 --- a/nova/policies/flavor_extra_specs.py +++ b/nova/policies/flavor_extra_specs.py @@ -17,14 +17,12 @@ from oslo_policy import policy from nova.policies import base - POLICY_ROOT = 'os_compute_api:os-flavor-extra-specs:%s' - flavor_extra_specs_policies = [ policy.DocumentedRuleDefault( name=POLICY_ROOT % 'show', - check_str=base.PROJECT_READER_OR_SYSTEM_READER, + check_str=base.PROJECT_READER_OR_ADMIN, description="Show an extra spec for a flavor", operations=[ { @@ -75,34 +73,15 @@ flavor_extra_specs_policies = [ ), policy.DocumentedRuleDefault( name=POLICY_ROOT % 'index', - check_str=base.PROJECT_READER, + check_str=base.PROJECT_READER_OR_ADMIN, description="List extra specs for a flavor. Starting with " - "microversion 2.47, the flavor used for a server is also returned " - "in the response when showing server details, updating a server or " - "rebuilding a server. Starting with microversion 2.61, extra specs " - "may be returned in responses for the flavor resource.", + "microversion 2.61, extra specs may be returned in responses " + "for the flavor resource.", operations=[ { 'path': '/flavors/{flavor_id}/os-extra_specs/', 'method': 'GET' }, - # Microversion 2.47 operations for servers: - { - 'path': '/servers/detail', - 'method': 'GET' - }, - { - 'path': '/servers/{server_id}', - 'method': 'GET' - }, - { - 'path': '/servers/{server_id}', - 'method': 'PUT' - }, - { - 'path': '/servers/{server_id}/action (rebuild)', - 'method': 'POST' - }, # Microversion 2.61 operations for flavors: { 'path': '/flavors', diff --git a/nova/policies/servers.py b/nova/policies/servers.py index c5b1592d8f..faa8f8d02c 100644 --- a/nova/policies/servers.py +++ b/nova/policies/servers.py @@ -22,6 +22,17 @@ ZERO_DISK_FLAVOR = SERVERS % 'create:zero_disk_flavor' REQUESTED_DESTINATION = 'compute:servers:create:requested_destination' CROSS_CELL_RESIZE = 'compute:servers:resize:cross_cell' +DEPRECATED_POLICY = policy.DeprecatedRule( + 'os_compute_api:os-flavor-extra-specs:index', + base.RULE_ADMIN_OR_OWNER, +) + +DEPRECATED_REASON = """ +Policies for showing flavor extra specs in server APIs response is +seprated as new policy. This policy is deprecated only for that but +not for list extra specs and showing it in flavor API response. +""" + rules = [ policy.DocumentedRuleDefault( name=SERVERS % 'index', @@ -95,6 +106,36 @@ rules = [ } ], scope_types=['project']), + policy.DocumentedRuleDefault( + name=SERVERS % 'show:flavor-extra-specs', + check_str=base.PROJECT_READER, + description="Starting with microversion 2.47, the flavor and its " + "extra specs used for a server is also returned in the response " + "when showing server details, updating a server or rebuilding a " + "server.", + operations=[ + # Microversion 2.47 operations for servers: + { + 'path': '/servers/detail', + 'method': 'GET' + }, + { + 'path': '/servers/{server_id}', + 'method': 'GET' + }, + { + 'path': '/servers/{server_id}', + 'method': 'PUT' + }, + { + 'path': '/servers/{server_id}/action (rebuild)', + 'method': 'POST' + }, + ], + scope_types=['project'], + deprecated_rule=DEPRECATED_POLICY, + deprecated_reason=DEPRECATED_REASON, + deprecated_since='25.0.0'), # the details in host_status are pretty sensitive, only admins # should do that by default. policy.DocumentedRuleDefault( diff --git a/nova/tests/unit/fake_policy.py b/nova/tests/unit/fake_policy.py index bfc90e119e..2f8c483554 100644 --- a/nova/tests/unit/fake_policy.py +++ b/nova/tests/unit/fake_policy.py @@ -44,6 +44,7 @@ policy_data = """ "os_compute_api:servers:trigger_crash_dump": "", "os_compute_api:servers:show:host_status": "", "os_compute_api:servers:show": "", + "os_compute_api:servers:show:flavor-extra-specs" : "", "os_compute_api:servers:show:host_status:unknown-only": "", "os_compute_api:servers:allow_all_filters": "", "os_compute_api:servers:migrations:force_complete": "", diff --git a/nova/tests/unit/policies/base.py b/nova/tests/unit/policies/base.py index 7761d99d41..1353bfc886 100644 --- a/nova/tests/unit/policies/base.py +++ b/nova/tests/unit/policies/base.py @@ -149,6 +149,8 @@ class BasePolicyTest(test.TestCase): "role:admin and system_scope:all", "system_reader_api": "role:reader and system_scope:all", + "project_reader_or_admin": + "rule:project_reader_api or rule:context_is_admin", "project_admin_api": "role:admin and project_id:%(project_id)s", "project_member_api": diff --git a/nova/tests/unit/policies/test_flavor_extra_specs.py b/nova/tests/unit/policies/test_flavor_extra_specs.py index f0b35234ed..7da297b6e1 100644 --- a/nova/tests/unit/policies/test_flavor_extra_specs.py +++ b/nova/tests/unit/policies/test_flavor_extra_specs.py @@ -10,22 +10,16 @@ # License for the specific language governing permissions and limitations # under the License. -import fixtures import mock from oslo_utils.fixture import uuidsentinel as uuids from nova.api.openstack.compute import flavor_manage from nova.api.openstack.compute import flavors from nova.api.openstack.compute import flavors_extraspecs -from nova.api.openstack.compute import servers -from nova.compute import vm_states -from nova import objects from nova.policies import flavor_extra_specs as policies from nova.policies import flavor_manage as fm_policies -from nova.policies import servers as s_policies from nova.tests.unit.api.openstack import fakes from nova.tests.unit import fake_flavor -from nova.tests.unit import fake_instance from nova.tests.unit.policies import base @@ -42,30 +36,7 @@ class FlavorExtraSpecsPolicyTest(base.BasePolicyTest): self.controller = flavors_extraspecs.FlavorExtraSpecsController() self.flavor_ctrl = flavors.FlavorsController() self.fm_ctrl = flavor_manage.FlavorManageController() - self.server_ctrl = servers.ServersController() self.req = fakes.HTTPRequest.blank('') - self.server_ctrl._view_builder._add_security_grps = mock.MagicMock() - self.server_ctrl._view_builder._get_metadata = mock.MagicMock() - self.server_ctrl._view_builder._get_addresses = mock.MagicMock() - self.server_ctrl._view_builder._get_host_id = mock.MagicMock() - self.server_ctrl._view_builder._get_fault = mock.MagicMock() - self.server_ctrl._view_builder._add_host_status = mock.MagicMock() - - self.instance = fake_instance.fake_instance_obj( - self.project_member_context, - id=1, uuid=uuids.fake_id, project_id=self.project_id, - vm_state=vm_states.ACTIVE) - - self.mock_get = self.useFixture( - fixtures.MockPatch('nova.api.openstack.common.get_instance')).mock - self.mock_get.return_value = self.instance - - fakes.stub_out_secgroup_api( - self, security_groups=[{'name': 'default'}]) - self.mock_get_all = self.useFixture(fixtures.MockPatchObject( - self.server_ctrl.compute_api, 'get_all')).mock - self.mock_get_all.return_value = objects.InstanceList( - objects=[self.instance]) def get_flavor_extra_specs(context, flavor_id): return fake_flavor.fake_flavor_obj( @@ -87,8 +58,6 @@ class FlavorExtraSpecsPolicyTest(base.BasePolicyTest): # scopes, since scope checking is disabled. self.all_system_authorized_contexts = (self.all_project_contexts | self.all_system_contexts) - self.all_project_authorized_contexts = (self.all_project_contexts | - self.all_system_contexts) # In the base/legacy case, any admin is an admin. self.admin_authorized_contexts = set([self.project_admin_context, @@ -226,85 +195,6 @@ class FlavorExtraSpecsPolicyTest(base.BasePolicyTest): for resp in unauthorize_res: self.assertNotIn('extra_specs', resp['flavor']) - def test_server_detail_with_extra_specs_policy(self): - rule = s_policies.SERVERS % 'detail' - # server 'detail' policy is checked before flavor extra specs 'index' - # policy so we have to allow it for everyone otherwise it will fail - # first for unauthorized contexts. - self.policy.set_rules({rule: "@"}, overwrite=False) - req = fakes.HTTPRequest.blank('', version='2.47') - rule_name = policies.POLICY_ROOT % 'index' - authorize_res, unauthorize_res = self.common_policy_auth( - self.all_project_authorized_contexts, - rule_name, self.server_ctrl.detail, req, - fatal=False) - for resp in authorize_res: - self.assertIn('extra_specs', resp['servers'][0]['flavor']) - for resp in unauthorize_res: - self.assertNotIn('extra_specs', resp['servers'][0]['flavor']) - - @mock.patch('nova.objects.BlockDeviceMappingList.bdms_by_instance_uuid') - @mock.patch('nova.compute.api.API.get_instance_host_status') - def test_server_show_with_extra_specs_policy(self, mock_get, mock_block): - rule = s_policies.SERVERS % 'show' - # server 'show' policy is checked before flavor extra specs 'index' - # policy so we have to allow it for everyone otherwise it will fail - # first for unauthorized contexts. - self.policy.set_rules({rule: "@"}, overwrite=False) - req = fakes.HTTPRequest.blank('', version='2.47') - rule_name = policies.POLICY_ROOT % 'index' - authorize_res, unauthorize_res = self.common_policy_auth( - self.all_project_authorized_contexts, - rule_name, self.server_ctrl.show, req, 'fake', - fatal=False) - for resp in authorize_res: - self.assertIn('extra_specs', resp['server']['flavor']) - for resp in unauthorize_res: - self.assertNotIn('extra_specs', resp['server']['flavor']) - - @mock.patch('nova.objects.BlockDeviceMappingList.bdms_by_instance_uuid') - @mock.patch('nova.compute.api.API.get_instance_host_status') - @mock.patch('nova.compute.api.API.rebuild') - def test_server_rebuild_with_extra_specs_policy(self, mock_rebuild, - mock_get, mock_bdm): - rule = s_policies.SERVERS % 'rebuild' - # server 'rebuild' policy is checked before flavor extra specs 'index' - # policy so we have to allow it for everyone otherwise it will fail - # first for unauthorized contexts. - self.policy.set_rules({rule: "@"}, overwrite=False) - req = fakes.HTTPRequest.blank('', version='2.47') - rule_name = policies.POLICY_ROOT % 'index' - authorize_res, unauthorize_res = self.common_policy_auth( - self.all_project_authorized_contexts, - rule_name, self.server_ctrl._action_rebuild, - req, self.instance.uuid, - body={'rebuild': {"imageRef": uuids.fake_id}}, - fatal=False) - for resp in authorize_res: - self.assertIn('extra_specs', resp.obj['server']['flavor']) - for resp in unauthorize_res: - self.assertNotIn('extra_specs', resp.obj['server']['flavor']) - - @mock.patch('nova.compute.api.API.update_instance') - def test_server_update_with_extra_specs_policy(self, mock_update): - rule = s_policies.SERVERS % 'update' - # server 'update' policy is checked before flavor extra specs 'index' - # policy so we have to allow it for everyone otherwise it will fail - # first for unauthorized contexts. - self.policy.set_rules({rule: "@"}, overwrite=False) - req = fakes.HTTPRequest.blank('', version='2.47') - rule_name = policies.POLICY_ROOT % 'index' - authorize_res, unauthorize_res = self.common_policy_auth( - self.all_project_authorized_contexts, - rule_name, self.server_ctrl.update, - req, self.instance.uuid, - body={'server': {'name': 'test'}}, - fatal=False) - for resp in authorize_res: - self.assertIn('extra_specs', resp['server']['flavor']) - for resp in unauthorize_res: - self.assertNotIn('extra_specs', resp['server']['flavor']) - class FlavorExtraSpecsScopeTypePolicyTest(FlavorExtraSpecsPolicyTest): """Test Flavor Extra Specs APIs policies with system scope enabled. @@ -326,17 +216,6 @@ class FlavorExtraSpecsScopeTypePolicyTest(FlavorExtraSpecsPolicyTest): # Only system_admin can do system admin things self.admin_authorized_contexts = [self.system_admin_context] - # Scope checking is in effect, so break apart project/system - # authorization. Note that even for the server tests above, we - # are technically authorizing against a server-embedded flavor - # (which has no project affiliation like the actual flavor it - # came from) and thus the other_project_* contexts are - # technically valid here. In reality, failure for - # other_project_* to get the server itself would prevent those - # projects from seeing the flavor extra_specs for it. - self.all_project_authorized_contexts = self.all_project_contexts - self.all_system_authorized_contexts = self.all_system_contexts - class FlavorExtraSpecsNoLegacyNoScopeTest(FlavorExtraSpecsPolicyTest): """Test Flavor Extra Specs API policies with deprecated rules @@ -355,15 +234,13 @@ class FlavorExtraSpecsNoLegacyNoScopeTest(FlavorExtraSpecsPolicyTest): self.system_foo_context, self.project_foo_context, ]) - self.reduce_set('all_project_authorized', everything_but_foo) self.reduce_set('all_system_authorized', everything_but_foo) self.reduce_set('all_authorized', everything_but_foo) class FlavorExtraSpecsNoLegacyPolicyTest(FlavorExtraSpecsScopeTypePolicyTest): """Test Flavor Extra Specs APIs policies with system scope enabled, - and no more deprecated rules that allow the legacy admin API to - access system_admin_or_owner APIs. + and no more deprecated rules. """ without_deprecated_rules = True @@ -373,9 +250,6 @@ class FlavorExtraSpecsNoLegacyPolicyTest(FlavorExtraSpecsScopeTypePolicyTest): # access. Same note as above, regarding other_project_* # contexts. With scope checking enabled, project and system # contexts stay separate. - self.reduce_set( - 'all_project_authorized', - self.all_project_contexts - set([self.project_foo_context])) self.reduce_set( 'all_system_authorized', self.all_system_contexts - set([self.system_foo_context])) diff --git a/nova/tests/unit/policies/test_servers.py b/nova/tests/unit/policies/test_servers.py index 53b9d78a35..3068c7e9b5 100644 --- a/nova/tests/unit/policies/test_servers.py +++ b/nova/tests/unit/policies/test_servers.py @@ -29,6 +29,7 @@ from nova.network import neutron from nova import objects from nova.objects import fields from nova.objects.instance_group import InstanceGroup +from nova.policies import base as base_policy from nova.policies import extended_server_attributes as ea_policies from nova.policies import servers as policies from nova.tests.unit.api.openstack import fakes @@ -324,6 +325,102 @@ class ServersPolicyTest(base.BasePolicyTest): self.controller.show, self.req, self.instance.uuid) + @mock.patch('nova.objects.BlockDeviceMappingList.bdms_by_instance_uuid') + @mock.patch('nova.compute.api.API.get_instance_host_status') + def test_server_show_with_extra_specs_policy(self, mock_get, mock_block): + rule = policies.SERVERS % 'show' + # server 'show' policy is checked before flavor extra specs + # policy so we have to allow it for everyone otherwise it will fail + # first for unauthorized contexts. + self.policy.set_rules({rule: "@"}, overwrite=False) + req = fakes.HTTPRequest.blank('', version='2.47') + rule_name = policies.SERVERS % 'show:flavor-extra-specs' + authorize_res, unauthorize_res = self.common_policy_auth( + self.project_reader_authorized_contexts, + rule_name, self.controller.show, req, + self.instance.uuid, fatal=False) + for resp in authorize_res: + self.assertIn('extra_specs', resp['server']['flavor']) + for resp in unauthorize_res: + self.assertNotIn('extra_specs', resp['server']['flavor']) + + @mock.patch('nova.compute.api.API.get_all') + def test_server_detail_with_extra_specs_policy(self, mock_get): + + def fake_get_all(context, search_opts=None, + limit=None, marker=None, + expected_attrs=None, sort_keys=None, sort_dirs=None, + cell_down_support=False, all_tenants=False): + self.assertIsNotNone(search_opts) + if 'project_id' in search_opts or 'user_id' in search_opts: + return objects.InstanceList(objects=self.servers) + else: + raise + + self.mock_get_all.side_effect = fake_get_all + rule = policies.SERVERS % 'detail' + # server 'detail' policy is checked before flavor extra specs + # policy so we have to allow it for everyone otherwise it will fail + # first for unauthorized contexts. + self.policy.set_rules({rule: "@"}, overwrite=False) + req = fakes.HTTPRequest.blank('', version='2.47') + rule_name = policies.SERVERS % 'show:flavor-extra-specs' + authorize_res, unauthorize_res = self.common_policy_auth( + self.everyone_authorized_contexts, + rule_name, self.controller.detail, req, + fatal=False) + for resp in authorize_res: + self.assertIn('extra_specs', resp['servers'][0]['flavor']) + for resp in unauthorize_res: + self.assertNotIn('extra_specs', resp['servers'][0]['flavor']) + + @mock.patch('nova.objects.BlockDeviceMappingList.bdms_by_instance_uuid') + @mock.patch('nova.compute.api.API.get_instance_host_status') + @mock.patch('nova.compute.api.API.rebuild') + def test_server_rebuild_with_extra_specs_policy(self, mock_rebuild, + mock_get, mock_bdm): + rule = policies.SERVERS % 'rebuild' + # server 'rebuild' policy is checked before flavor extra specs + # policy so we have to allow it for everyone otherwise it will fail + # first for unauthorized contexts. + self.policy.set_rules({rule: "@"}, overwrite=False) + req = fakes.HTTPRequest.blank('', version='2.47') + rule_name = policies.SERVERS % 'show:flavor-extra-specs' + authorize_res, unauthorize_res = self.common_policy_auth( + self.project_reader_authorized_contexts, + rule_name, self.controller._action_rebuild, + req, self.instance.uuid, + body={'rebuild': {"imageRef": uuids.fake_id}}, + fatal=False) + for resp in authorize_res: + self.assertIn('extra_specs', resp.obj['server']['flavor']) + for resp in unauthorize_res: + self.assertNotIn('extra_specs', resp.obj['server']['flavor']) + + @mock.patch('nova.objects.BlockDeviceMappingList.bdms_by_instance_uuid') + @mock.patch.object(InstanceGroup, 'get_by_instance_uuid') + @mock.patch('nova.compute.api.API.update_instance') + def test_server_update_with_extra_specs_policy(self, + mock_update, mock_group, mock_bdm): + mock_update.return_value = self.instance + rule = policies.SERVERS % 'update' + # server 'update' policy is checked before flavor extra specs + # policy so we have to allow it for everyone otherwise it will fail + # first for unauthorized contexts. + self.policy.set_rules({rule: "@"}, overwrite=False) + req = fakes.HTTPRequest.blank('', version='2.47') + rule_name = policies.SERVERS % 'show:flavor-extra-specs' + authorize_res, unauthorize_res = self.common_policy_auth( + self.project_reader_authorized_contexts, + rule_name, self.controller.update, + req, self.instance.uuid, + body={'server': {'name': 'test'}}, + fatal=False) + for resp in authorize_res: + self.assertIn('extra_specs', resp['server']['flavor']) + for resp in unauthorize_res: + self.assertNotIn('extra_specs', resp['server']['flavor']) + @mock.patch('nova.compute.api.API.create') def test_create_server_policy(self, mock_create): mock_create.return_value = ([self.instance], '') @@ -1228,6 +1325,10 @@ class ServersNoLegacyNoScopeTest(ServersPolicyTest): checking still disabled. """ without_deprecated_rules = True + rules_without_deprecation = { + policies.SERVERS % 'show:flavor-extra-specs': + base_policy.PROJECT_READER, + } def setUp(self): super(ServersNoLegacyNoScopeTest, self).setUp() @@ -1335,10 +1436,13 @@ class ServersScopeTypePolicyTest(ServersPolicyTest): class ServersNoLegacyPolicyTest(ServersScopeTypePolicyTest): """Test Servers APIs policies with system scope enabled, - and no more deprecated rules that allow the legacy admin API to - access system_admin_or_owner APIs. + and no more deprecated rules. """ without_deprecated_rules = True + rules_without_deprecation = { + policies.SERVERS % 'show:flavor-extra-specs': + base_policy.PROJECT_READER, + } def setUp(self): super(ServersNoLegacyPolicyTest, self).setUp() diff --git a/nova/tests/unit/test_policy.py b/nova/tests/unit/test_policy.py index 3318a8cd34..7c47952593 100644 --- a/nova/tests/unit/test_policy.py +++ b/nova/tests/unit/test_policy.py @@ -409,6 +409,7 @@ class RealRolePolicyTestCase(test.NoDBTestCase): "os_compute_api:servers:resize", "os_compute_api:servers:revert_resize", "os_compute_api:servers:show", +"os_compute_api:servers:show:flavor-extra-specs", "os_compute_api:servers:update", "os_compute_api:servers:create_image:allow_volume_backed", "os_compute_api:os-admin-password", @@ -560,7 +561,7 @@ class RealRolePolicyTestCase(test.NoDBTestCase): 'system_admin_api', 'system_reader_api', 'project_admin_api', 'project_member_api', 'project_reader_api', 'system_admin_or_owner', - 'system_or_project_reader') + 'system_or_project_reader', 'project_reader_or_admin') result = set(rules.keys()) - set(self.admin_only_rules + self.admin_or_owner_rules + self.allow_all_rules + self.system_reader_rules +