Always perform policy checks if enforce_new_defaults == true

When new policy default rules are enforced in the config, we shouldn't
be checking if context.is_admin is true and stop performing checks if
that is True.
As Neutron's policy rules are going to understand and use new
personas like system-admin, project-admin and e.g. reader roles, it
needs to be aware of those and simply checking if

    context.is_admin == True

in the neutron.policy.check() and neutron.policy.enforce() functions is
not enough. We need to perform all checks in such cases as well to e.g.
avoid giving list of the system resources to the user who has
project's admin role.

Such change will require a lot of changes in the code (e.g. unit tests)
and as we are close to the release point, this patch left that
context.is_admin check logic in case when
CONF.oslo_policy.enforce_new_defaults option is set to False.
In next release we need to get rid of that check if context.is_admin ==
True completly and adjust all required places in code as well.

Related-blueprint: #secure-rbac-roles
Change-Id: I403ca661dceee17aff9295caf8721c4a237a58cf

neutron/policy.py

@@ -468,7 +468,10 @@ def check(context, action, target, plugin=None, might_not_exist=False,
     """
     # If we already know the context has admin rights do not perform an
     # additional check and authorize the operation
-    if context.is_admin:
+    # TODO(slaweq): Remove that is_admin check and always perform rules checks
+    # when old, deprecated rules will be removed and only rules with new
+    # personas will be supported
+    if not cfg.CONF.oslo_policy.enforce_new_defaults and context.is_admin:
         return True
     if might_not_exist and not (_ENFORCER.rules and action in _ENFORCER.rules):
         return True
@@ -502,7 +505,10 @@ def enforce(context, action, target, plugin=None, pluralized=None):
     """
     # If we already know the context has admin rights do not perform an
     # additional check and authorize the operation
-    if context.is_admin:
+    # TODO(slaweq): Remove that is_admin check and always perform rules checks
+    # when old, deprecated rules will be removed and only rules with new
+    # personas will be supported
+    if not cfg.CONF.oslo_policy.enforce_new_defaults and context.is_admin:
         return True
     rule, target, context = _prepare_check(context, action, target, pluralized)
     try:

neutron/tests/unit/test_policy.py

@@ -78,6 +78,8 @@ class PolicyTestCase(base.BaseTestCase):
             "example:early_or_success": "@ or !",
             "example:lowercase_admin": "role:admin or role:sysadmin",
             "example:uppercase_admin": "role:ADMIN or role:sysadmin",
+            "example:only_system_admin_allowed": (
+                "role:admin and system_scope:all"),
         }
         policy.refresh()
         # NOTE(vish): then overload underlying rules
@@ -85,6 +87,60 @@ class PolicyTestCase(base.BaseTestCase):
         self.context = context.Context('fake', 'fake', roles=['member'])
         self.target = {}
 
+    def _test_check_system_admin_allowed_action(self, enforce_new_defaults):
+        action = "example:only_system_admin_allowed"
+        cfg.CONF.set_override(
+            'enforce_new_defaults', enforce_new_defaults, group='oslo_policy')
+        project_admin_ctx = context.Context(
+            user="fake", project_id="fake",
+            roles=['admin', 'member', 'reader'])
+        system_admin_ctx = context.Context(
+            user="fake", project_id="fake",
+            roles=['admin', 'member', 'reader'],
+            system_scope='all')
+        self.assertTrue(policy.check(system_admin_ctx, action, self.target))
+        if enforce_new_defaults:
+            self.assertFalse(
+                policy.check(project_admin_ctx, action, self.target))
+        else:
+            self.assertTrue(
+                policy.check(project_admin_ctx, action, self.target))
+
+    def test_check_only_system_admin_new_defaults(self):
+        self._test_check_system_admin_allowed_action(enforce_new_defaults=True)
+
+    def test_check_only_system_admin_old_defaults(self):
+        self._test_check_system_admin_allowed_action(
+            enforce_new_defaults=False)
+
+    def _test_enforce_system_admin_allowed_action(self, enforce_new_defaults):
+        action = "example:only_system_admin_allowed"
+        cfg.CONF.set_override(
+            'enforce_new_defaults', enforce_new_defaults, group='oslo_policy')
+        project_admin_ctx = context.Context(
+            user="fake", project_id="fake",
+            roles=['admin', 'member', 'reader'])
+        system_admin_ctx = context.Context(
+            user="fake", project_id="fake",
+            roles=['admin', 'member', 'reader'],
+            system_scope='all')
+        self.assertTrue(policy.enforce(system_admin_ctx, action, self.target))
+        if enforce_new_defaults:
+            self.assertRaises(
+                oslo_policy.PolicyNotAuthorized,
+                policy.enforce, project_admin_ctx, action, self.target)
+        else:
+            self.assertTrue(
+                policy.enforce(project_admin_ctx, action, self.target))
+
+    def test_enforce_only_system_admin_new_defaults(self):
+        self._test_enforce_system_admin_allowed_action(
+            enforce_new_defaults=True)
+
+    def test_enforce_only_system_admin_old_defaults(self):
+        self._test_enforce_system_admin_allowed_action(
+            enforce_new_defaults=False)
+
     def test_enforce_nonexistent_action_throws(self):
         action = "example:noexist"
         self.assertRaises(oslo_policy.PolicyNotAuthorized, policy.enforce,