From 219d970d130613b9723381990825b05e104bfe12 Mon Sep 17 00:00:00 2001
From: Andrew Laski <andrew@lascii.com>
Date: Fri, 1 Jul 2016 13:51:48 -0400
Subject: [PATCH] Hacking check for _ENFORCER.enforce()

In order to ensure that only registered policies are used for
authorization checks _ENFORCER.authorize should be used rather than
_ENFORCER.enforce. This adds a check to look for instances of
_ENFORCER.enforce being used.

Change-Id: Iee78e93a3e1d4c6c30681b18698b7fc9cb6fa982
Implements: bp policy-in-code
---
 HACKING.rst                     |  1 +
 nova/hacking/checks.py          | 20 ++++++++++++++++++++
 nova/tests/unit/test_hacking.py | 19 +++++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/HACKING.rst b/HACKING.rst
index 632e9ad0e03a..721326db4ec6 100644
--- a/HACKING.rst
+++ b/HACKING.rst
@@ -61,6 +61,7 @@ Nova Specific Commandments
 - [N348] Deprecated library function os.popen()
 - [N349] Check for closures in tests which are not used
 - [N350] Policy registration should be in the central location ``nova/policies/``
+- [N351] Do not use the oslo_policy.policy.Enforcer.enforce() method.
 
 Creating Unit Tests
 -------------------
diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py
index f4493c4bded1..f850991b1ee3 100644
--- a/nova/hacking/checks.py
+++ b/nova/hacking/checks.py
@@ -40,6 +40,7 @@ cfg_re = re.compile(r".*\scfg\.")
 # Excludes oslo.config OptGroup objects
 cfg_opt_re = re.compile(r".*[\s\[]cfg\.[a-zA-Z]*Opt\(")
 rule_default_re = re.compile(r".*RuleDefault\(")
+policy_enforce_re = re.compile(r".*_ENFORCER\.enforce\(")
 vi_header_re = re.compile(r"^#\s+vim?:.+")
 virt_file_re = re.compile(r"\./nova/(?:tests/)?virt/(\w+)/")
 virt_import_re = re.compile(
@@ -666,6 +667,24 @@ def check_policy_registration_in_central_place(logical_line, filename):
         yield(0, msg)
 
 
+def check_policy_enforce(logical_line, filename):
+    """Look for uses of nova.policy._ENFORCER.enforce()
+
+    Now that policy defaults are registered in code the _ENFORCER.authorize
+    method should be used. That ensures that only registered policies are used.
+    Uses of _ENFORCER.enforce could allow unregistered policies to be used, so
+    this check looks for uses of that method.
+
+    N351
+    """
+
+    msg = ('N351: nova.policy._ENFORCER.enforce() should not be used. '
+           'Use the authorize() method instead.')
+
+    if policy_enforce_re.match(logical_line):
+        yield(0, msg)
+
+
 def check_doubled_words(physical_line, filename):
     """Check for the common doubled-word typos
 
@@ -813,6 +832,7 @@ def factory(register):
     register(check_greenthread_spawns)
     register(check_config_option_in_central_place)
     register(check_policy_registration_in_central_place)
+    register(check_policy_enforce)
     register(check_doubled_words)
     register(check_python3_no_iteritems)
     register(check_python3_no_iterkeys)
diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py
index 8598d08852a8..f7cc11809011 100644
--- a/nova/tests/unit/test_hacking.py
+++ b/nova/tests/unit/test_hacking.py
@@ -790,3 +790,22 @@ class HackingTestCase(test.NoDBTestCase):
             code, checks.check_policy_registration_in_central_place,
             filename="nova/api/openstack/compute/non_existent.py",
             expected_errors=errors)
+
+    def test_check_policy_enforce(self):
+        errors = [(3, 0, "N351")]
+        code = """
+        from nova import policy
+
+        policy._ENFORCER.enforce('context_is_admin', target, credentials)
+        """
+        self._assert_has_errors(code, checks.check_policy_enforce,
+                                expected_errors=errors)
+
+    def test_check_policy_enforce_does_not_catch_other_enforce(self):
+        # Simulate a different enforce method defined in Nova
+        code = """
+        from nova import foo
+
+        foo.enforce()
+        """
+        self._assert_has_no_errors(code, checks.check_policy_enforce)