From c71cbae6e0ae417f6696ad89bca98373287ee09f Mon Sep 17 00:00:00 2001 From: Ghanshyam Mann Date: Mon, 30 Mar 2020 23:56:58 -0500 Subject: [PATCH] Pass the actual target in migrate server policy Currently if target is not passed in context.can(), it use defauls target which is context.user_id, context.project_id. These defaults target are not useful as it pass the context's user_id and project_id only which means we tell oslo policy to verify the context data with context data. This commit pass the actual target for migrate server policies which is server project_id because policy rule is system and project scoped. Adding tests also to show that rule can be override with project roles. Partial implement blueprint policy-defaults-refresh Change-Id: I3050b7c60ccfe8b737b4dbb93f00f6d6ca82bc6d --- nova/api/openstack/compute/migrate_server.py | 25 ++++++++------ .../unit/policies/test_migrate_server.py | 34 +++++++++++++++++++ 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/nova/api/openstack/compute/migrate_server.py b/nova/api/openstack/compute/migrate_server.py index 74f04dcdd9e8..c20748ab62a1 100644 --- a/nova/api/openstack/compute/migrate_server.py +++ b/nova/api/openstack/compute/migrate_server.py @@ -48,16 +48,17 @@ class MigrateServerController(wsgi.Controller): def _migrate(self, req, id, body): """Permit admins to migrate a server to a new host.""" context = req.environ['nova.context'] - context.can(ms_policies.POLICY_ROOT % 'migrate') + + instance = common.get_instance(self.compute_api, context, id, + expected_attrs=['flavor', 'services']) + context.can(ms_policies.POLICY_ROOT % 'migrate', + target={'project_id': instance.project_id}) host_name = None if (api_version_request.is_supported(req, min_version='2.56') and body['migrate'] is not None): host_name = body['migrate'].get('host') - instance = common.get_instance(self.compute_api, context, id, - expected_attrs=['flavor', 'services']) - if common.instance_has_port_with_resource_request( instance.uuid, self.network_api): # TODO(gibi): Remove when nova only supports compute newer than @@ -99,7 +100,15 @@ class MigrateServerController(wsgi.Controller): def _migrate_live(self, req, id, body): """Permit admins to (live) migrate a server to a new host.""" context = req.environ["nova.context"] - context.can(ms_policies.POLICY_ROOT % 'migrate_live') + + # NOTE(stephenfin): we need 'numa_topology' because of the + # 'LiveMigrationTask._check_instance_has_no_numa' check in the + # conductor + instance = common.get_instance(self.compute_api, context, id, + expected_attrs=['numa_topology']) + + context.can(ms_policies.POLICY_ROOT % 'migrate_live', + target={'project_id': instance.project_id}) host = body["os-migrateLive"]["host"] block_migration = body["os-migrateLive"]["block_migration"] @@ -122,12 +131,6 @@ class MigrateServerController(wsgi.Controller): disk_over_commit = strutils.bool_from_string(disk_over_commit, strict=True) - # NOTE(stephenfin): we need 'numa_topology' because of the - # 'LiveMigrationTask._check_instance_has_no_numa' check in the - # conductor - instance = common.get_instance(self.compute_api, context, id, - expected_attrs=['numa_topology']) - # We could potentially move this check to conductor and avoid the # extra API call to neutron when we support move operations with ports # having resource requests. diff --git a/nova/tests/unit/policies/test_migrate_server.py b/nova/tests/unit/policies/test_migrate_server.py index be1aaa9972b5..9d33eb93bf5c 100644 --- a/nova/tests/unit/policies/test_migrate_server.py +++ b/nova/tests/unit/policies/test_migrate_server.py @@ -17,6 +17,7 @@ from oslo_utils import timeutils from nova.api.openstack.compute import migrate_server from nova.compute import vm_states +from nova.policies import base as base_policy from nova.policies import migrate_server as ms_policies from nova.tests.unit.api.openstack import fakes from nova.tests.unit import fake_instance @@ -123,3 +124,36 @@ class MigrateServerNoLegacyPolicyTest(MigrateServerScopeTypePolicyTest): self.project_reader_context, self.project_foo_context, self.other_project_member_context ] + + +class MigrateServerOverridePolicyTest(MigrateServerNoLegacyPolicyTest): + """Test Migrate Server APIs policies with system and project scoped + but default to system roles only are allowed for project roles + if override by operators. This test is with system scope enable + and no more deprecated rules. + """ + + def setUp(self): + super(MigrateServerOverridePolicyTest, self).setUp() + rule_migrate = ms_policies.POLICY_ROOT % 'migrate' + rule_live_migrate = ms_policies.POLICY_ROOT % 'migrate_live' + # NOTE(gmann): override the rule to project member and verify it + # work as policy is system and projct scoped. + self.policy.set_rules({ + rule_migrate: base_policy.PROJECT_MEMBER_OR_SYSTEM_ADMIN, + rule_live_migrate: base_policy.PROJECT_MEMBER_OR_SYSTEM_ADMIN}, + overwrite=False) + + # Check that system admin or project scoped role as override above + # is able to migrate the server + self.admin_authorized_contexts = [ + self.system_admin_context, + self.project_admin_context, self.project_member_context] + # Check that non-system admin or project role is not able to + # migrate the server + self.admin_unauthorized_contexts = [ + self.legacy_admin_context, self.system_member_context, + self.system_reader_context, self.system_foo_context, + self.other_project_member_context, + self.project_foo_context, self.project_reader_context + ]