Add policy granularity to the Flavors API

The same policy rule (os_compute_api:os-flavor-manage) is being used
for the create and delete actions of the flavors REST API. It is thus
impossible to provide different RBAC for the create and delete actions
based on roles. To address this, changes are made to have separate
policy rules for each action.

Most other places in nova (and OpenStack in general) have separate
policy rules for each action. This affords the ultimate flexibility
to deployers, who can obviously use the same rule if that is what they
want.

To address backwards compatibility, the new rules added to the
flavor_manage.py policy file, default to the existing rule
(os_compute_api:os-flavor-manage). That way across upgrades this
should ensure if an existing admin has customised the rule, it keeps
working, but folks that know about the new setting can override the
default rule. In addtion, a verify_deprecated_policy method is added
to see if the old policy action is being configured instead of the
new actions.

Closes-Bug: #1675147

Co-Authored-By: Felipe Monteiro <felipe.monteiro@att.com>
Change-Id: Ic67b52ebac3a47e9fb7e3c0d6c3ce8a6bc539e11

nova/api/openstack/compute/flavor_manage.py

@@ -12,6 +12,8 @@
 
 import webob
 
+from oslo_log import log as logging
+
 from nova.api.openstack.compute.schemas import flavor_manage
 from nova.api.openstack.compute.views import flavors as flavors_view
 from nova.api.openstack import extensions
@@ -21,8 +23,12 @@ from nova.compute import flavors
 from nova import exception
 from nova.i18n import _
 from nova import objects
+from nova.policies import base
 from nova.policies import flavor_manage as fm_policies
+from nova import policy
 
+
+LOG = logging.getLogger(__name__)
 ALIAS = "os-flavor-manage"
 
 
@@ -41,7 +47,15 @@ class FlavorManageController(wsgi.Controller):
     @wsgi.action("delete")
     def _delete(self, req, id):
         context = req.environ['nova.context']
-        context.can(fm_policies.BASE_POLICY_NAME)
+        # TODO(rb560u): remove this check in future release
+        using_old_action = \
+            policy.verify_deprecated_policy(fm_policies.BASE_POLICY_NAME,
+                fm_policies.POLICY_ROOT % 'delete',
+                base.RULE_ADMIN_API,
+                context)
+
+        if not using_old_action:
+            context.can(fm_policies.POLICY_ROOT % 'delete')
 
         flavor = objects.Flavor(context=context, flavorid=id)
         try:
@@ -57,7 +71,15 @@ class FlavorManageController(wsgi.Controller):
     @validation.schema(flavor_manage.create, '2.1')
     def _create(self, req, body):
         context = req.environ['nova.context']
-        context.can(fm_policies.BASE_POLICY_NAME)
+        # TODO(rb560u): remove this check in future release
+        using_old_action = \
+            policy.verify_deprecated_policy(fm_policies.BASE_POLICY_NAME,
+                fm_policies.POLICY_ROOT % 'create',
+                base.RULE_ADMIN_API,
+                context)
+
+        if not using_old_action:
+            context.can(fm_policies.POLICY_ROOT % 'create')
 
         vals = body['flavor']
 

nova/policies/flavor_manage.py

@@ -20,13 +20,17 @@ from nova.policies import base
 
 
 BASE_POLICY_NAME = 'os_compute_api:os-flavor-manage'
+POLICY_ROOT = 'os_compute_api:os-flavor-manage:%s'
+BASE_POLICY_RULE = 'rule:%s' % BASE_POLICY_NAME
 
 
 flavor_manage_policies = [
+    # TODO(rb560u): remove this rule in future release
     policy.DocumentedRuleDefault(
         BASE_POLICY_NAME,
         base.RULE_ADMIN_API,
-        "Create and delete Flavors",
+        "Create and delete Flavors. Deprecated in Pike and will be "
+        "removed in future release",
         [
             {
                 'method': 'POST',
@@ -38,6 +42,26 @@ flavor_manage_policies = [
             },
 
         ]),
+    policy.DocumentedRuleDefault(
+        POLICY_ROOT % 'create',
+        BASE_POLICY_RULE,
+        "Create a flavor",
+        [
+            {
+                'method': 'POST',
+                'path': '/flavors'
+            }
+        ]),
+    policy.DocumentedRuleDefault(
+        POLICY_ROOT % 'delete',
+        BASE_POLICY_RULE,
+        "Delete a flavor",
+        [
+            {
+                'method': 'DELETE',
+                'path': '/flavors/{flavor_id}'
+            }
+        ]),
 ]
 
 

nova/policy.py

@@ -225,3 +225,33 @@ def get_enforcer():
     cfg.CONF(conf_args, project='nova')
     init()
     return _ENFORCER
+
+
+def verify_deprecated_policy(old_policy, new_policy, default_rule, context):
+    """Check the rule of the deprecated policy action
+
+    If the current rule of the deprecated policy action is set to a non-default
+    value, then a warning message is logged stating that the new policy
+    action should be used to dictate permissions as the old policy action is
+    being deprecated.
+
+    :param old_policy: policy action that is being deprecated
+    :param new_policy: policy action that is replacing old_policy
+    :param default_rule: the old_policy action default rule value
+    :param context: the nova context
+    """
+
+    if _ENFORCER:
+        current_rule = str(_ENFORCER.rules[old_policy])
+    else:
+        current_rule = None
+
+    if current_rule != default_rule:
+        LOG.warning("Start using the new action '{0}'. The existing "
+                    "action '{1}' is being deprecated and will be "
+                    "removed in future release.".format(new_policy,
+                                                        old_policy))
+        context.can(old_policy)
+        return True
+    else:
+        return False

nova/tests/unit/api/openstack/compute/test_flavor_manage.py

@@ -23,6 +23,7 @@ from nova.api.openstack.compute import flavor_manage as flavormanage_v21
 from nova.compute import flavors
 from nova import db
 from nova import exception
+from nova import policy
 from nova import test
 from nova.tests.unit.api.openstack import fakes
 
@@ -373,19 +374,20 @@ class PrivateFlavorManageTestV21(test.TestCase):
             self.assertEqual(body["flavor"][key], self.expected["flavor"][key])
 
 
-class FlavorManagerPolicyEnforcementV21(test.NoDBTestCase):
+class FlavorManagerPolicyEnforcementV21(test.TestCase):
 
     def setUp(self):
         super(FlavorManagerPolicyEnforcementV21, self).setUp()
         self.controller = flavormanage_v21.FlavorManageController()
+        self.adm_req = fakes.HTTPRequest.blank('', use_admin_context=True)
+        self.req = fakes.HTTPRequest.blank('')
 
     def test_create_policy_failed(self):
         rule_name = "os_compute_api:os-flavor-manage"
         self.policy.set_rules({rule_name: "project:non_fake"})
-        req = fakes.HTTPRequest.blank('')
         exc = self.assertRaises(
             exception.PolicyNotAuthorized,
-            self.controller._create, req,
+            self.controller._create, self.req,
             body={"flavor": {
                 "name": "test",
                 "ram": 512,
@@ -394,6 +396,8 @@ class FlavorManagerPolicyEnforcementV21(test.NoDBTestCase):
                 "swap": 512,
                 "rxtx_factor": 1,
             }})
+        # The deprecated action is being enforced since the rule that is
+        # configured is different than the default rule
         self.assertEqual(
             "Policy doesn't allow %s to be performed." % rule_name,
             exc.format_message())
@@ -401,11 +405,172 @@ class FlavorManagerPolicyEnforcementV21(test.NoDBTestCase):
     def test_delete_policy_failed(self):
         rule_name = "os_compute_api:os-flavor-manage"
         self.policy.set_rules({rule_name: "project:non_fake"})
-        req = fakes.HTTPRequest.blank('')
         exc = self.assertRaises(
             exception.PolicyNotAuthorized,
-            self.controller._delete, req,
+            self.controller._delete, self.req,
             fakes.FAKE_UUID)
+        # The deprecated action is being enforced since the rule that is
+        # configured is different than the default rule
         self.assertEqual(
             "Policy doesn't allow %s to be performed." % rule_name,
             exc.format_message())
+
+    @mock.patch.object(policy.LOG, 'warning')
+    def test_create_policy_rbac_inherit_default(self, mock_warning):
+        """Test to verify inherited rule is working. The rule of the
+           deprecated action is not set to the default, so the deprecated
+           action is being enforced
+        """
+
+        default_flavor_policy = "os_compute_api:os-flavor-manage"
+        create_flavor_policy = "os_compute_api:os-flavor-manage:create"
+        rules = {default_flavor_policy: 'is_admin:True',
+                 create_flavor_policy: 'rule:%s' % default_flavor_policy}
+        self.policy.set_rules(rules)
+        body = {
+            "flavor": {
+                "name": "azAZ09. -_",
+                "ram": 512,
+                "vcpus": 2,
+                "disk": 1,
+                "OS-FLV-EXT-DATA:ephemeral": 1,
+                "id": six.text_type('1234'),
+                "swap": 512,
+                "rxtx_factor": 1,
+                "os-flavor-access:is_public": True,
+            }
+        }
+        # check for success as admin
+        self.controller._create(self.adm_req, body=body)
+        # check for failure as non-admin
+        exc = self.assertRaises(exception.PolicyNotAuthorized,
+                                self.controller._create, self.req,
+                                body=body)
+        # The deprecated action is being enforced since the rule that is
+        # configured is different than the default rule
+        self.assertEqual(
+            "Policy doesn't allow %s to be performed." % default_flavor_policy,
+            exc.format_message())
+        mock_warning.assert_called_with("Start using the new "
+            "action '{0}'. The existing action '{1}' is being deprecated and "
+            "will be removed in future release.".format(create_flavor_policy,
+                                                        default_flavor_policy))
+
+    @mock.patch.object(policy.LOG, 'warning')
+    def test_delete_policy_rbac_inherit_default(self, mock_warning):
+        """Test to verify inherited rule is working. The rule of the
+           deprecated action is not set to the default, so the deprecated
+           action is being enforced
+        """
+
+        default_flavor_policy = "os_compute_api:os-flavor-manage"
+        create_flavor_policy = "os_compute_api:os-flavor-manage:create"
+        delete_flavor_policy = "os_compute_api:os-flavor-manage:delete"
+        rules = {default_flavor_policy: 'is_admin:True',
+                 create_flavor_policy: 'rule:%s' % default_flavor_policy,
+                 delete_flavor_policy: 'rule:%s' % default_flavor_policy}
+        self.policy.set_rules(rules)
+        body = {
+            "flavor": {
+                "name": "azAZ09. -_",
+                "ram": 512,
+                "vcpus": 2,
+                "disk": 1,
+                "OS-FLV-EXT-DATA:ephemeral": 1,
+                "id": six.text_type('1234'),
+                "swap": 512,
+                "rxtx_factor": 1,
+                "os-flavor-access:is_public": True,
+            }
+        }
+        self.flavor = self.controller._create(self.adm_req, body=body)
+        mock_warning.assert_called_once_with("Start using the new "
+            "action '{0}'. The existing action '{1}' is being deprecated and "
+            "will be removed in future release.".format(create_flavor_policy,
+                                                        default_flavor_policy))
+        # check for success as admin
+        flavor = self.flavor
+        self.controller._delete(self.adm_req, flavor['flavor']['id'])
+        # check for failure as non-admin
+        flavor = self.flavor
+        exc = self.assertRaises(exception.PolicyNotAuthorized,
+                                self.controller._delete, self.req,
+                                flavor['flavor']['id'])
+        # The deprecated action is being enforced since the rule that is
+        # configured is different than the default rule
+        self.assertEqual(
+            "Policy doesn't allow %s to be performed." % default_flavor_policy,
+            exc.format_message())
+        mock_warning.assert_called_with("Start using the new "
+            "action '{0}'. The existing action '{1}' is being deprecated and "
+            "will be removed in future release.".format(delete_flavor_policy,
+                                                        default_flavor_policy))
+
+    def test_create_policy_rbac_no_change_to_default_action_rule(self):
+        """Test to verify the correct action is being enforced. When the
+           rule configured for the deprecated action is the same as the
+           default, the new action should be enforced.
+        """
+
+        default_flavor_policy = "os_compute_api:os-flavor-manage"
+        create_flavor_policy = "os_compute_api:os-flavor-manage:create"
+        # The default rule of the deprecated action is admin_api
+        rules = {default_flavor_policy: 'rule:admin_api',
+                 create_flavor_policy: 'rule:%s' % default_flavor_policy}
+        self.policy.set_rules(rules)
+        body = {
+            "flavor": {
+                "name": "azAZ09. -_",
+                "ram": 512,
+                "vcpus": 2,
+                "disk": 1,
+                "OS-FLV-EXT-DATA:ephemeral": 1,
+                "id": six.text_type('1234'),
+                "swap": 512,
+                "rxtx_factor": 1,
+                "os-flavor-access:is_public": True,
+            }
+        }
+        exc = self.assertRaises(exception.PolicyNotAuthorized,
+                                self.controller._create, self.req,
+                                body=body)
+        self.assertEqual(
+            "Policy doesn't allow %s to be performed." % create_flavor_policy,
+            exc.format_message())
+
+    def test_delete_policy_rbac_change_to_default_action_rule(self):
+        """Test to verify the correct action is being enforced. When the
+           rule configured for the deprecated action is the same as the
+           default, the new action should be enforced.
+        """
+
+        default_flavor_policy = "os_compute_api:os-flavor-manage"
+        create_flavor_policy = "os_compute_api:os-flavor-manage:create"
+        delete_flavor_policy = "os_compute_api:os-flavor-manage:delete"
+        # The default rule of the deprecated action is admin_api
+        # Set the rule of the create flavor action to is_admin:True so that
+        # admin context can be used to create a flavor
+        rules = {default_flavor_policy: 'rule:admin_api',
+                 create_flavor_policy: 'is_admin:True',
+                 delete_flavor_policy: 'rule:%s' % default_flavor_policy}
+        self.policy.set_rules(rules)
+        body = {
+            "flavor": {
+                "name": "azAZ09. -_",
+                "ram": 512,
+                "vcpus": 2,
+                "disk": 1,
+                "OS-FLV-EXT-DATA:ephemeral": 1,
+                "id": six.text_type('1234'),
+                "swap": 512,
+                "rxtx_factor": 1,
+                "os-flavor-access:is_public": True,
+            }
+        }
+        flavor = self.controller._create(self.adm_req, body=body)
+        exc = self.assertRaises(exception.PolicyNotAuthorized,
+                                self.controller._delete, self.req,
+                                flavor['flavor']['id'])
+        self.assertEqual(
+            "Policy doesn't allow %s to be performed." % delete_flavor_policy,
+            exc.format_message())

nova/tests/unit/fake_policy.py

@@ -51,6 +51,8 @@ policy_data = """
     "os_compute_api:os-flavor-extra-specs:index": "",
     "os_compute_api:os-flavor-extra-specs:show": "",
     "os_compute_api:os-flavor-manage": "",
+    "os_compute_api:os-flavor-manage:create": "",
+    "os_compute_api:os-flavor-manage:delete": "",
     "os_compute_api:os-floating-ip-dns": "",
     "os_compute_api:os-floating-ip-dns:domain:update": "",
     "os_compute_api:os-floating-ip-dns:domain:delete": "",

nova/tests/unit/test_policy.py

@@ -72,6 +72,9 @@ class PolicyTestCase(test.NoDBTestCase):
             oslo_policy.RuleDefault("true", '@'),
             oslo_policy.RuleDefault("example:allowed", '@'),
             oslo_policy.RuleDefault("example:denied", "!"),
+            oslo_policy.RuleDefault("old_action_not_default", "@"),
+            oslo_policy.RuleDefault("new_action", "@"),
+            oslo_policy.RuleDefault("old_action_default", "rule:admin_api"),
             oslo_policy.RuleDefault("example:get_http",
                                     "http://www.example.com"),
             oslo_policy.RuleDefault("example:my_file",
@@ -181,6 +184,32 @@ class PolicyTestCase(test.NoDBTestCase):
                 "project_id:%(project_id)s")])
         mock_warning.assert_not_called()
 
+    @mock.patch.object(policy.LOG, 'warning')
+    def test_verify_deprecated_policy_using_old_action(self, mock_warning):
+
+        old_policy = "old_action_not_default"
+        new_policy = "new_action"
+        default_rule = "rule:admin_api"
+
+        using_old_action = policy.verify_deprecated_policy(
+            old_policy, new_policy, default_rule, self.context)
+
+        mock_warning.assert_called_once_with("Start using the new "
+            "action '{0}'. The existing action '{1}' is being deprecated and "
+            "will be removed in future release.".format(new_policy,
+                                                        old_policy))
+        self.assertTrue(using_old_action)
+
+    def test_verify_deprecated_policy_using_new_action(self):
+        old_policy = "old_action_default"
+        new_policy = "new_action"
+        default_rule = "rule:admin_api"
+
+        using_old_action = policy.verify_deprecated_policy(
+            old_policy, new_policy, default_rule, self.context)
+
+        self.assertFalse(using_old_action)
+
 
 class IsAdminCheckTestCase(test.NoDBTestCase):
     def setUp(self):
@@ -283,6 +312,8 @@ class RealRolePolicyTestCase(test.NoDBTestCase):
 "os_compute_api:os-flavor-extra-specs:update",
 "os_compute_api:os-flavor-extra-specs:delete",
 "os_compute_api:os-flavor-manage",
+"os_compute_api:os-flavor-manage:create",
+"os_compute_api:os-flavor-manage:delete",
 "os_compute_api:os-floating-ips-bulk",
 "os_compute_api:os-floating-ip-dns:domain:delete",
 "os_compute_api:os-floating-ip-dns:domain:update",

releasenotes/notes/flavor-api-policy-granularity-f563d621c615fd64.yaml

@@ -0,0 +1,13 @@
+---
+features:
+  - |
+    Add granularity to the ``os_compute_api:os-flavor-manage`` policy
+    with the addition of distinct actions for create and delete:
+
+    - ``os_compute_api:os-flavor-manage:create``
+    - ``os_compute_api:os-flavor-manage:delete``
+
+    To address backwards compatibility, the new rules added to the
+    flavor_manage.py policy file, default to the existing rule,
+    ``os_compute_api:os-flavor-manage``, if it is set to a non-default
+    value.