Rules in policy directory files can be deleted.

Policy directory files can only add new rules or
update existing rules in cache, but cannot return back
loaded rules in memory to their default value.
This incorrect behavior was fixed in the patch.

Member "_loaded_files" of class Enforcer should keep
list of loaded policy config files paths.
In fact if the same file is changed many times
then the same file path is added many times.
If a file is deleted it's path not deleted from "_loaded_files".
The member is very misleading and is not used in code.
So this member was deleted in the patch because of
above mentioned resons.

Change-Id: I9ede38d8cf2ae968d3d8c0b1240bd6a51e6aa931
Closes-Bug: 1943584

3 files changed, 77 insertions, 62 deletions

diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py
index 4491eca..bf5556e 100644
--- a/oslo_policy/policy.py
+++ b/oslo_policy/policy.py
@@ -545,7 +545,6 @@ class Enforcer(object):
         self.use_conf = use_conf
         self._need_check_rule = True
         self.overwrite = overwrite
-        self._loaded_files = []
         self._policy_dir_mtimes = {}
         self._file_cache = {}
         self._informed_no_policy_file = False
@@ -586,7 +585,6 @@ class Enforcer(object):
         self.set_rules({})
         self.default_rule = None
         self.policy_path = None
-        self._loaded_files = []
         self._policy_dir_mtimes = {}
         self._file_cache.clear()
         self.registered_rules = {}
@@ -627,22 +625,48 @@ class Enforcer(object):
                     overwrite=self.overwrite
                 )
 
-            force_reload_policy_dir = force_reload
+            force_reload_policy_dirs = force_reload
             if policy_file_rules_changed:
-                force_reload_policy_dir = True
+                force_reload_policy_dirs = True
+            existing_policy_dirs = []
 
             for path in self.conf.oslo_policy.policy_dirs:
                 try:
-                    path = self._get_policy_path(path)
+                    absolute_path = self._get_policy_path(path)
+                    existing_policy_dirs.append(absolute_path)
                 except cfg.ConfigFilesNotFoundError:
                     continue
-                if (self._is_directory_updated(self._policy_dir_mtimes, path)
-                        or force_reload_policy_dir):
+                # If change was made in any policy directory or main policy
+                # file then all policy directories and main file are
+                # re-calculated from scratch. We don't have separate rule sets
+                # for every policy folder, we only have the only rule set in
+                # RAM for all rule configs (self.rules). So it's the only way
+                # to be consistent.
+                if self._is_directory_updated(self._policy_dir_mtimes,
+                                              absolute_path):
+                    force_reload_policy_dirs = True
+            if force_reload_policy_dirs and existing_policy_dirs:
+                # Here we realize that some policy folders or main policy file
+                # were changed and we need to recalculate all rules from
+                # scratch.
+                # If policy_file_rules_changed is True then we know:
+                # 1. all rules were already reset.
+                # 2. rules from main policy file were already applied.
+                # Otherwise main policy file was not changed and rules were not
+                # reset and. So we reset rules and force to re-calculate
+                # rules in main policy file. And after that we apply rules
+                # from every policy directory.
+                if self.policy_path:
+                    if not policy_file_rules_changed:
+                        self._load_policy_file(path=self.policy_path,
+                                               force_reload=True,
+                                               overwrite=self.overwrite
+                                               )
+                else:
+                    self.rules = Rules(default_rule=self.default_rule)
+                for path in existing_policy_dirs:
                     self._walk_through_policy_directory(
-                        path,
-                        self._load_policy_file,
-                        force_reload_policy_dir, False
-                    )
+                        path, self._load_policy_file, True, False)
 
             for default in self.registered_rules.values():
                 if default.deprecated_for_removal:
@@ -917,7 +941,6 @@ class Enforcer(object):
             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
 
diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py
index d5c86c3..5ca34a2 100644
--- a/oslo_policy/tests/test_policy.py
+++ b/oslo_policy/tests/test_policy.py
@@ -238,13 +238,6 @@ class EnforcerTest(base.PolicyBaseTestCase):
         super(EnforcerTest, self).setUp()
         self.create_config_file('policy.json', POLICY_JSON_CONTENTS)
 
-    def check_loaded_files(self, filenames):
-        self.assertEqual(
-            [self.get_config_file_fullname(n)
-             for n in filenames],
-            self.enforcer._loaded_files
-        )
-
     def _test_scenario_with_opts_registered(self, scenario, *args, **kwargs):
         # This test registers some rules, calls the scenario and then checks
         # the registered rules. The scenario should be a method which loads
@@ -291,11 +284,6 @@ class EnforcerTest(base.PolicyBaseTestCase):
         loaded_rules = jsonutils.loads(str(self.enforcer.rules))
         self.assertEqual('role:fakeB', loaded_rules['default'])
         self.assertEqual('is_admin:True', loaded_rules['admin'])
-        self.check_loaded_files([
-            'policy.json',
-            os.path.join('policy.d', 'a.conf'),
-            os.path.join('policy.d', 'b.conf'),
-        ])
 
     def test_load_directory_after_file_update(self):
         self.create_config_file(
@@ -305,10 +293,6 @@ class EnforcerTest(base.PolicyBaseTestCase):
         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",
@@ -332,12 +316,41 @@ class EnforcerTest(base.PolicyBaseTestCase):
         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_after_file_is_emptied(self):
+        def dict_rules(enforcer_rules):
+            """Converts enforcer rules to dictionary.
+
+            :param enforcer_rules: enforcer rules represented as a class Rules
+            :return: enforcer rules represented as a dictionary
+            """
+            return jsonutils.loads(str(enforcer_rules))
+
+        self.assertEqual(self.enforcer.rules, {})
+
+        self.enforcer.load_rules()
+        main_policy_file_rules = jsonutils.loads(POLICY_JSON_CONTENTS)
+        self.assertEqual(main_policy_file_rules,
+                         dict_rules(self.enforcer.rules))
+
+        folder_policy_file = os.path.join('policy.d', 'a.conf')
+        self.create_config_file(folder_policy_file, POLICY_A_CONTENTS)
+        self.enforcer.load_rules()
+        expected_rules = main_policy_file_rules.copy()
+        expected_rules.update(jsonutils.loads(POLICY_A_CONTENTS))
+        self.assertEqual(expected_rules, dict_rules(self.enforcer.rules))
+
+        self.create_config_file(folder_policy_file, '{}')
+        # Force the mtime change since the unit test may write to this file
+        # too fast for mtime to actually change.
+        absolute_folder_policy_file_path = self.get_config_file_fullname(
+            folder_policy_file)
+        stinfo = os.stat(absolute_folder_policy_file_path)
+        os.utime(absolute_folder_policy_file_path,
+                 (stinfo.st_atime + 42, stinfo.st_mtime + 42))
+        self.enforcer.load_rules()
+        self.assertEqual(main_policy_file_rules,
+                         dict_rules(self.enforcer.rules))
 
     def test_load_directory_opts_registered(self):
         self._test_scenario_with_opts_registered(self.test_load_directory)
@@ -364,11 +377,6 @@ class EnforcerTest(base.PolicyBaseTestCase):
 
         loaded_rules = jsonutils.loads(str(self.enforcer.rules))
         self.assertEqual('is_admin:True', loaded_rules['admin'])
-        self.check_loaded_files([
-            'policy.json',
-            os.path.join('policy.d', 'a.conf'),
-            os.path.join('policy.d', 'a.conf'),
-        ])
 
     def test_load_directory_caching_with_files_updated_opts_registered(self):
         self._test_scenario_with_opts_registered(
@@ -392,10 +400,6 @@ class EnforcerTest(base.PolicyBaseTestCase):
 
         loaded_rules = jsonutils.loads(str(self.enforcer.rules))
         self.assertEqual('is_admin:True', loaded_rules['admin'])
-        self.check_loaded_files([
-            'policy.json',
-            os.path.join('policy.d', 'a.conf'),
-        ])
 
     def test_load_directory_caching_with_files_same_but_overwrite_false(self):
         self.test_load_directory_caching_with_files_same(overwrite=False)
@@ -453,12 +457,6 @@ class EnforcerTest(base.PolicyBaseTestCase):
         loaded_rules = jsonutils.loads(str(self.enforcer.rules))
         self.assertEqual('role:fakeC', loaded_rules['default'])
         self.assertEqual('is_admin:True', loaded_rules['admin'])
-        self.check_loaded_files([
-            'policy.json',
-            os.path.join('policy.d', 'a.conf'),
-            os.path.join('policy.d', 'b.conf'),
-            os.path.join('policy.2.d', 'fake.conf'),
-        ])
 
     def test_load_multiple_directories_opts_registered(self):
         self._test_scenario_with_opts_registered(
@@ -474,8 +472,6 @@ class EnforcerTest(base.PolicyBaseTestCase):
         self.assertIsNotNone(self.enforcer.rules)
         self.assertIn('default', self.enforcer.rules)
         self.assertIn('admin', self.enforcer.rules)
-        self.check_loaded_files(
-            ['policy.json', os.path.join('policy.d', 'a.conf')])
 
     def test_load_non_existed_directory_opts_registered(self):
         self._test_scenario_with_opts_registered(
@@ -1011,13 +1007,6 @@ class EnforcerNoPolicyFileTest(base.PolicyBaseTestCase):
     def setUp(self):
         super(EnforcerNoPolicyFileTest, self).setUp()
 
-    def check_loaded_files(self, filenames):
-        self.assertEqual(
-            [self.get_config_file_fullname(n)
-             for n in filenames],
-            self.enforcer._loaded_files
-        )
-
     def test_load_rules(self):
         # Check that loading rules with no policy file does not error
         self.enforcer.load_rules(True)
@@ -1043,10 +1032,6 @@ class EnforcerNoPolicyFileTest(base.PolicyBaseTestCase):
         loaded_rules = jsonutils.loads(str(self.enforcer.rules))
         self.assertEqual('role:fakeB', loaded_rules['default'])
         self.assertEqual('is_admin:True', loaded_rules['admin'])
-        self.check_loaded_files([
-            'policy.d/a.conf',
-            'policy.d/b.conf',
-        ])
 
 
 class CheckFunctionTestCase(base.PolicyBaseTestCase):
diff --git a/releasenotes/notes/bug-1943584-fc74f9205039883c.yaml b/releasenotes/notes/bug-1943584-fc74f9205039883c.yaml
new file mode 100644
index 0000000..57e8d87
--- /dev/null
+++ b/releasenotes/notes/bug-1943584-fc74f9205039883c.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    [`bug 1943584 <https://bugs.launchpad.net/oslo.policy/+bug/1943584>`_]
+    If file in policy directory was emptied, rules were not re-calculated. The
+    only workaround was to restart an application. Now rules are re-calculated
+    "on the fly", without app restart.