Reload files in policy_dirs on primary file changequeens-eolstable/queens

It was determined that rules from policy files located in the directory
specified in the policy_dirs option (/etc/<config_dir>/policy.d by
default) are not re-applied after the rules from the primary policy file
is re-applied due to a change.

This change introduces additional behavior to make sure the rules from
policy_dirs are reapplied if there is a change to the primary policy
file.

Change-Id: I8a6f8e971d881365c41ea409966723319d5b239a
Closes-Bug: #1880959
Related-Bug: #1880847
(cherry picked from commit 75677a31108243e0adddc89f1fbf669053f9573b)
(cherry picked from commit c8c138e69d9189345a7366a10b0779ef70b7250e)
(cherry picked from commit 5904564bf13bbac7d66e00ec6312487c507f09c4)
(cherry picked from commit a49f31d3fc7690e3d15bc2428660b537967c89cc)
(cherry picked from commit 622bcf207948174535b4d04f15ef14fb265c834d)

3 files changed, 87 insertions, 9 deletions

diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py
index 09597f2..187669b 100644
--- a/oslo_policy/policy.py
+++ b/oslo_policy/policy.py
@@ -540,6 +540,8 @@ class Enforcer(object):
         if force_reload:
             self.use_conf = force_reload
 
+        policy_file_rules_changed = False
+
         if self.use_conf:
             if not self.policy_path:
                 try:
@@ -551,18 +553,30 @@ class Enforcer(object):
                         self._informed_no_policy_file = True
 
             if self.policy_path:
-                self._load_policy_file(self.policy_path, force_reload,
-                                       overwrite=self.overwrite)
+                # If the policy file rules have changed any policy.d rules
+                # also need to be reapplied on top of that change.
+                policy_file_rules_changed = self._load_policy_file(
+                    self.policy_path,
+                    force_reload,
+                    overwrite=self.overwrite
+                )
+
+            force_reload_policy_dir = force_reload
+            if policy_file_rules_changed:
+                force_reload_policy_dir = True
+
             for path in self.conf.oslo_policy.policy_dirs:
                 try:
                     path = self._get_policy_path(path)
                 except cfg.ConfigFilesNotFoundError:
                     continue
-                if (force_reload or self._is_directory_updated(
-                        self._policy_dir_mtimes, path)):
-                    self._walk_through_policy_directory(path,
-                                                        self._load_policy_file,
-                                                        force_reload, False)
+                if (self._is_directory_updated(self._policy_dir_mtimes, path)
+                        or force_reload_policy_dir):
+                    self._walk_through_policy_directory(
+                        path,
+                        self._load_policy_file,
+                        force_reload_policy_dir, False
+                    )
 
             for default in self.registered_rules.values():
                 if default.deprecated_rule:
@@ -713,6 +727,8 @@ class Enforcer(object):
         # is in the cache
         mtime = 0
         if os.path.exists(path):
+            if not os.path.isdir(path):
+                raise ValueError('{} is not a directory'.format(path))
             # Make a list of all the files
             files = [path] + [os.path.join(path, file) for file in
                               os.listdir(path)]
@@ -751,14 +767,25 @@ class Enforcer(object):
             self.file_rules[name] = RuleDefault(name, check_str)
 
     def _load_policy_file(self, path, force_reload, overwrite=True):
+        """Load policy rules from the specified policy file.
+
+        :param str path: A path of the policy file to load rules from.
+        :param bool force_reload: Forcefully reload the policy file content.
+        :param bool overwrite: Replace policy rules instead of updating them.
+        :return: A bool indicating whether rules have been changed or not.
+        :rtype: bool
+        """
+        rules_changed = False
         reloaded, data = _cache_handler.read_cached_file(
             self._file_cache, path, force_reload=force_reload)
         if reloaded or not self.rules:
             rules = Rules.load(data, self.default_rule)
             self.set_rules(rules, overwrite=overwrite, use_conf=True)
+            rules_changed = True
             self._record_file_rules(data, overwrite)
             self._loaded_files.append(path)
             LOG.debug('Reloaded policy file: %(path)s', {'path': path})
+        return rules_changed
 
     def _get_policy_path(self, path):
         """Locate the policy YAML/JSON data file/path.
@@ -836,14 +863,14 @@ class Enforcer(object):
                 # Check the scope of the operation against the possible scope
                 # attributes provided in `creds`.
                 if creds.get('system'):
-                    token_scope = 'system'
+                    token_scope = 'system'  # nosec
                 else:
                     # If the token isn't system-scoped then we're dealing with
                     # either a domain-scoped token or a project-scoped token.
                     # From a policy perspective, both are "project" operations.
                     # Whether or not the project is a domain depends on where
                     # it sits in the hierarchy.
-                    token_scope = 'project'
+                    token_scope = 'project'  # nosec
 
                 registered_rule = self.registered_rules.get(rule)
                 if registered_rule and registered_rule.scope_types:
diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py
index 7d22001..82bf574 100644
--- a/oslo_policy/tests/test_policy.py
+++ b/oslo_policy/tests/test_policy.py
@@ -264,6 +264,48 @@ class EnforcerTest(base.PolicyBaseTestCase):
             os.path.join('policy.d', 'b.conf'),
         ])
 
+    def test_load_directory_after_file_update(self):
+        self.create_config_file(
+            os.path.join('policy.d', 'a.conf'), POLICY_A_CONTENTS)
+        self.enforcer.load_rules(True)
+        self.assertIsNotNone(self.enforcer.rules)
+        loaded_rules = jsonutils.loads(str(self.enforcer.rules))
+        self.assertEqual('role:fakeA', loaded_rules['default'])
+        self.assertEqual('is_admin:True', loaded_rules['admin'])
+        self.check_loaded_files([
+            'policy.json',
+            os.path.join('policy.d', 'a.conf'),
+        ])
+        new_policy_json_contents = jsonutils.dumps({
+            "default": "rule:admin",
+            "admin": "is_admin:True",
+            "foo": "rule:bar",
+        })
+        # Modify the policy.json file and then validate that the rules
+        # from the policy directory are re-applied on top of the
+        # new rules from the file.
+        self.create_config_file('policy.json', new_policy_json_contents)
+        policy_file_path = self.get_config_file_fullname('policy.json')
+        # Force the mtime change since the unit test may write to this file
+        # too fast for mtime to actually change.
+        stinfo = os.stat(policy_file_path)
+        os.utime(policy_file_path, (stinfo.st_atime + 42,
+                                    stinfo.st_mtime + 42))
+
+        self.enforcer.load_rules()
+
+        self.assertIsNotNone(self.enforcer.rules)
+        loaded_rules = jsonutils.loads(str(self.enforcer.rules))
+        self.assertEqual('role:fakeA', loaded_rules['default'])
+        self.assertEqual('is_admin:True', loaded_rules['admin'])
+        self.assertEqual('rule:bar', loaded_rules['foo'])
+        self.check_loaded_files([
+            'policy.json',
+            os.path.join('policy.d', 'a.conf'),
+            'policy.json',
+            os.path.join('policy.d', 'a.conf'),
+        ])
+
     def test_load_directory_opts_registered(self):
         self._test_scenario_with_opts_registered(self.test_load_directory)
 
@@ -389,6 +431,7 @@ class EnforcerTest(base.PolicyBaseTestCase):
                                [os.path.join('policy.d', 'a.conf')],
                                group='oslo_policy')
         self.assertRaises(ValueError, self.enforcer.load_rules, True)
+        self.assertRaises(ValueError, self.enforcer.load_rules, False)
 
     @mock.patch('oslo_policy.policy.Enforcer.check_rules')
     def test_load_rules_twice(self, mock_check_rules):
diff --git a/releasenotes/notes/bug-1880959-8f1370a59759d40d.yaml b/releasenotes/notes/bug-1880959-8f1370a59759d40d.yaml
new file mode 100644
index 0000000..929f509
--- /dev/null
+++ b/releasenotes/notes/bug-1880959-8f1370a59759d40d.yaml
@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    [`bug 1880959 <https://bugs.launchpad.net/keystone/+bug/1880959>`_]
+    The behavior of policy file reloading from policy directories was fixed.
+    Previously the rules from policy files located in the directories
+    specified in the ``policy_dirs`` option were not reapplied after the rules
+    from the primary policy file have been reapplied due to a change.