From 3ab2c357e8510e3d1aa562c29d4b72e9f24e1477 Mon Sep 17 00:00:00 2001
From: Felipe Monteiro <felipe.monteiro@att.com>
Date: Wed, 5 Jul 2017 22:25:34 +0100
Subject: [PATCH] Dynamic policy file discovery

Patrole should eventually support other services like Heat
and Murano, not just the Big Tent services included in Tempest.

Patrole then should be able to dynamically discover custom
policy files. While the solution this commit implements is
not perfect, it will allow more services' policy file to
be discovered by Patrole. The policy files will still
have to be located on the same host as Patrole.

This commit removes the service-specific policy path
CONF options in favor of a new CONF option called
``[rbac] custom_policy_files`` which is a ListOpt that
includes paths for each custom policy file. Each
policy path assumes that the service name is included in
the path. The paths should be ordered by precedence, with
high-priority paths before low-priority paths. The first
path that is found to contain the service's policy file
will be used.

This commit refactors unit tests and rbac_policy_parser
as needed to work with the changes.

Change-Id: Ia929b77223b54906888af6cd324f0cfa0fafda8f
Implements blueprint: dynamic-policy-file-discovery
---
 patrole_tempest_plugin/config.py              |  50 ++++++--
 patrole_tempest_plugin/rbac_policy_parser.py  |  53 +++++---
 .../tests/unit/test_rbac_policy_parser.py     | 113 +++++++-----------
 ...olicy-file-discovery-104cbfc64b55d605.yaml |  22 ++++
 4 files changed, 136 insertions(+), 102 deletions(-)
 create mode 100644 releasenotes/notes/dynamic-policy-file-discovery-104cbfc64b55d605.yaml

diff --git a/patrole_tempest_plugin/config.py b/patrole_tempest_plugin/config.py
index 11808368..a6f30e7e 100644
--- a/patrole_tempest_plugin/config.py
+++ b/patrole_tempest_plugin/config.py
@@ -21,33 +21,61 @@ rbac_group = cfg.OptGroup(name='rbac',
 RbacGroup = [
     cfg.StrOpt('rbac_test_role',
                default='admin',
-               help="The current RBAC role against which to run"
-                    " Patrole tests."),
+               help="""The current RBAC role against which to run Patrole
+tests."""),
     cfg.BoolOpt('enable_rbac',
                 default=True,
                 help="Enables RBAC tests."),
     cfg.BoolOpt('strict_policy_check',
                 default=False,
-                help="If true, throws RbacParsingException for"
-                     " policies which don't exist. If false, "
-                     "throws skipException."),
+                help="""If true, throws RbacParsingException for policies which
+don't exist or are not included in the service's policy file. If false, throws
+skipException."""),
     # TODO(rb560u): There needs to be support for reading these JSON files from
-    # other hosts. It may be possible to leverage the v3 identity policy API
+    # other hosts. It may be possible to leverage the v3 identity policy API.
+    cfg.ListOpt('custom_policy_files',
+                default=['/etc/%s/policy.json'],
+                help="""List of the paths to search for policy files. Each
+policy path assumes that the service name is included in the path once. Also
+assumes Patrole is on the same host as the policy files. The paths should be
+ordered by precedence, with high-priority paths before low-priority paths. The
+first path that is found to contain the service's policy file will be used.
+"""),
     cfg.StrOpt('cinder_policy_file',
                default='/etc/cinder/policy.json',
-               help="Location of the neutron policy file."),
+               help="""Location of the Cinder policy file. Assumed to be on
+the same host as Patrole.""",
+               deprecated_for_removal=True,
+               deprecated_reason="It is better to use `custom_policy_files` "
+                                 "which supports any OpenStack service."),
     cfg.StrOpt('glance_policy_file',
                default='/etc/glance/policy.json',
-               help="Location of the glance policy file."),
+               help="""Location of the Glance policy file. Assumed to be on
+the same host as Patrole.""",
+               deprecated_for_removal=True,
+               deprecated_reason="It is better to use `custom_policy_files` "
+                                 "which supports any OpenStack service."),
     cfg.StrOpt('keystone_policy_file',
                default='/etc/keystone/policy.json',
-               help="Location of the keystone policy file."),
+               help="""Location of the custom Keystone policy file. Assumed to
+be on the same host as Patrole.""",
+               deprecated_for_removal=True,
+               deprecated_reason="It is better to use `custom_policy_files` "
+                                 "which supports any OpenStack service."),
     cfg.StrOpt('neutron_policy_file',
                default='/etc/neutron/policy.json',
-               help="Location of the neutron policy file."),
+               help="""Location of the Neutron policy file. Assumed to be on
+the same host as Patrole.""",
+               deprecated_for_removal=True,
+               deprecated_reason="It is better to use `custom_policy_files` "
+                                 "which supports any OpenStack service."),
     cfg.StrOpt('nova_policy_file',
                default='/etc/nova/policy.json',
-               help="Location of the nova policy file."),
+               help="""Location of the custom Nova policy file. Assumed to be
+on the same host as Patrole.""",
+               deprecated_for_removal=True,
+               deprecated_reason="It is better to use `custom_policy_files` "
+                                 "which supports any OpenStack service."),
     cfg.BoolOpt('test_custom_requirements',
                 default=False,
                 help="""
diff --git a/patrole_tempest_plugin/rbac_policy_parser.py b/patrole_tempest_plugin/rbac_policy_parser.py
index 17a626c2..41871cf4 100644
--- a/patrole_tempest_plugin/rbac_policy_parser.py
+++ b/patrole_tempest_plugin/rbac_policy_parser.py
@@ -58,26 +58,27 @@ class RbacPolicyParser(RbacAuthority):
         the custom policy file over the default policy implementation is
         prioritized.
 
-        :param project_id: type uuid
-        :param user_id: type uuid
-        :param service: type string
-        :param path: type string
+        :param uuid project_id: project_id of object performing API call
+        :param uuid user_id: user_id of object performing API call
+        :param string service: service of the policy file
+        :param dict extra_target_data: dictionary containing additional object
+            data needed by oslo.policy to validate generic checks
         """
 
         if extra_target_data is None:
             extra_target_data = {}
 
-        # First check if the service is valid.
         self.validate_service(service)
 
-        # Use default path in /etc/<service_name/policy.json if no path
-        # is provided.
-        path = getattr(CONF.rbac, '%s_policy_file' % str(service), None)
-        if not path:
-            LOG.info("No config option found for %s,"
-                     " using default path", str(service))
-            path = os.path.join('/etc', service, 'policy.json')
-        self.path = path
+        # Prioritize dynamically searching for policy files over relying on
+        # deprecated service-specific policy file locations.
+        if CONF.rbac.custom_policy_files:
+            self.discover_policy_files()
+            self.path = self.policy_files.get(service)
+        else:
+            self.path = getattr(CONF.rbac, '%s_policy_file' % str(service),
+                                None)
+
         self.rules = policy.Rules.load(self._get_policy_data(service),
                                        'default')
         self.project_id = project_id
@@ -86,7 +87,7 @@ class RbacPolicyParser(RbacAuthority):
 
     @classmethod
     def validate_service(cls, service):
-        """Validate whether the service passed to ``init`` exists."""
+        """Validate whether the service passed to ``__init__`` exists."""
         service = service.lower().strip() if service else None
 
         # Cache the list of available services in memory to avoid needlessly
@@ -102,6 +103,19 @@ class RbacPolicyParser(RbacAuthority):
             raise rbac_exceptions.RbacInvalidService(
                 "%s is NOT a valid service." % service)
 
+    @classmethod
+    def discover_policy_files(cls):
+        # Dynamically discover the policy file for each service in
+        # ``cls.available_services``. Pick the first ``candidate_path`` found
+        # out of the potential paths in ``CONF.rbac.custom_policy_files``.
+        if not hasattr(cls, 'policy_files'):
+            cls.policy_files = {}
+            for service in cls.available_services:
+                for candidate_path in CONF.rbac.custom_policy_files:
+                    if os.path.isfile(candidate_path % service):
+                        cls.policy_files.setdefault(service,
+                                                    candidate_path % service)
+
     def allowed(self, rule_name, role):
         is_admin_context = self._is_admin_context(role)
         is_allowed = self._allowed(
@@ -115,8 +129,8 @@ class RbacPolicyParser(RbacAuthority):
         mgr_policy_data = {}
         policy_data = {}
 
-        # Check whether policy file exists.
-        if os.path.isfile(self.path):
+        # Check whether policy file exists and attempt to read it.
+        if self.path and os.path.isfile(self.path):
             try:
                 with open(self.path, 'r') as policy_file:
                     file_policy_data = policy_file.read()
@@ -158,8 +172,11 @@ class RbacPolicyParser(RbacAuthority):
         elif mgr_policy_data:
             policy_data = mgr_policy_data
         else:
-            error_message = 'Policy file for {0} service neither found in '\
-                            'code nor at {1}.'.format(service, self.path)
+            error_message = (
+                'Policy file for {0} service neither found in code nor at {1}.'
+                .format(service, [loc % service for loc in
+                                  CONF.rbac.custom_policy_files])
+            )
             raise rbac_exceptions.RbacParsingException(error_message)
 
         try:
diff --git a/patrole_tempest_plugin/tests/unit/test_rbac_policy_parser.py b/patrole_tempest_plugin/tests/unit/test_rbac_policy_parser.py
index 09de6bfd..e3a44293 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_policy_parser.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_policy_parser.py
@@ -28,12 +28,21 @@ CONF = config.CONF
 
 class RbacPolicyTest(base.TestCase):
 
+    services = {
+        'services': [
+            {'name': 'custom_rbac_policy'},
+            {'name': 'admin_rbac_policy'},
+            {'name': 'alt_admin_rbac_policy'},
+            {'name': 'tenant_rbac_policy'},
+            {'name': 'test_service'}
+        ]
+    }
+
     def setUp(self):
         super(RbacPolicyTest, self).setUp()
-        self.mock_admin_mgr = mock.patch.object(
-            rbac_policy_parser, 'credentials').start()
-        self.mock_path = mock.patch.object(
-            rbac_policy_parser, 'os').start()
+        mock_admin_mgr = self.patchobject(rbac_policy_parser, 'credentials')
+        mock_admin_mgr.AdminManager().identity_services_v3_client.\
+            list_services.return_value = self.services
 
         current_directory = os.path.dirname(os.path.realpath(__file__))
         self.custom_policy_file = os.path.join(current_directory,
@@ -48,35 +57,13 @@ class RbacPolicyTest(base.TestCase):
         self.tenant_policy_file = os.path.join(current_directory,
                                                'resources',
                                                'tenant_rbac_policy.json')
-        services = {
-            'services': [
-                {'name': 'cinder', 'links': 'link', 'enabled': True,
-                 'type': 'volume', 'id': 'id',
-                 'description': 'description'},
-                {'name': 'glance', 'links': 'link', 'enabled': True,
-                 'type': 'image', 'id': 'id',
-                 'description': 'description'},
-                {'name': 'nova', 'links': 'link', 'enabled': True,
-                 'type': 'compute', 'id': 'id',
-                 'description': 'description'},
-                {'name': 'keystone', 'links': 'link', 'enabled': True,
-                 'type': 'identity', 'id': 'id',
-                 'description': 'description'},
-                {'name': 'heat', 'links': 'link', 'enabled': True,
-                 'type': 'orchestration', 'id': 'id',
-                 'description': 'description'},
-                {'name': 'neutron', 'links': 'link', 'enabled': True,
-                 'type': 'networking', 'id': 'id',
-                 'description': 'description'},
-                {'name': 'test_service', 'links': 'link', 'enabled': True,
-                 'type': 'unit_test', 'id': 'id',
-                 'description': 'description'}
-            ]
-        }
 
-        self.mock_admin_mgr.AdminManager.return_value.\
-            identity_services_v3_client.list_services.return_value = \
-            services
+        CONF.set_override(
+            'custom_policy_files',
+            [os.path.join(current_directory, 'resources', '%s.json')],
+            group='rbac')
+        self.addCleanup(CONF.clear_override, 'custom_policy_files',
+                        group='rbac')
 
     def _get_fake_policy_rule(self, name, rule):
         fake_rule = mock.Mock(check=rule)
@@ -90,9 +77,8 @@ class RbacPolicyTest(base.TestCase):
 
         test_tenant_id = mock.sentinel.tenant_id
         test_user_id = mock.sentinel.user_id
-        self.mock_path.path.join.return_value = self.custom_policy_file
         parser = rbac_policy_parser.RbacPolicyParser(
-            test_tenant_id, test_user_id, "test_service")
+            test_tenant_id, test_user_id, "custom_rbac_policy")
 
         expected = {
             'policy_action_1': ['two', 'four', 'six', 'eight'],
@@ -113,9 +99,8 @@ class RbacPolicyTest(base.TestCase):
     def test_admin_policy_file_with_admin_role(self):
         test_tenant_id = mock.sentinel.tenant_id
         test_user_id = mock.sentinel.user_id
-        self.mock_path.path.join.return_value = self.admin_policy_file
         parser = rbac_policy_parser.RbacPolicyParser(
-            test_tenant_id, test_user_id, "test_service")
+            test_tenant_id, test_user_id, "admin_rbac_policy")
 
         role = 'admin'
         allowed_rules = [
@@ -134,9 +119,8 @@ class RbacPolicyTest(base.TestCase):
     def test_admin_policy_file_with_member_role(self):
         test_tenant_id = mock.sentinel.tenant_id
         test_user_id = mock.sentinel.user_id
-        self.mock_path.path.join.return_value = self.admin_policy_file
         parser = rbac_policy_parser.RbacPolicyParser(
-            test_tenant_id, test_user_id, "test_service")
+            test_tenant_id, test_user_id, "admin_rbac_policy")
 
         role = 'Member'
         allowed_rules = [
@@ -153,12 +137,11 @@ class RbacPolicyTest(base.TestCase):
             allowed = parser.allowed(rule, role)
             self.assertFalse(allowed)
 
-    def test_admin_policy_file_with_context_is_admin(self):
+    def test_alt_admin_policy_file_with_context_is_admin(self):
         test_tenant_id = mock.sentinel.tenant_id
         test_user_id = mock.sentinel.user_id
-        self.mock_path.path.join.return_value = self.alt_admin_policy_file
         parser = rbac_policy_parser.RbacPolicyParser(
-            test_tenant_id, test_user_id, "test_service")
+            test_tenant_id, test_user_id, "alt_admin_rbac_policy")
 
         role = 'fake_admin'
         allowed_rules = ['non_admin_rule']
@@ -193,9 +176,8 @@ class RbacPolicyTest(base.TestCase):
         """
         test_tenant_id = mock.sentinel.tenant_id
         test_user_id = mock.sentinel.user_id
-        self.mock_path.path.join.return_value = self.tenant_policy_file
         parser = rbac_policy_parser.RbacPolicyParser(
-            test_tenant_id, test_user_id, "test_service")
+            test_tenant_id, test_user_id, "tenant_rbac_policy")
 
         # Check whether Member role can perform expected actions.
         allowed_rules = ['rule1', 'rule2', 'rule3', 'rule4']
@@ -254,7 +236,7 @@ class RbacPolicyTest(base.TestCase):
                           service)
 
         m_log.debug.assert_called_once_with(
-            '%s is NOT a valid service.', 'invalid_service')
+            '%s is NOT a valid service.', service)
 
     @mock.patch.object(rbac_policy_parser, 'LOG', autospec=True)
     def test_service_is_none_raises_exception(self, m_log):
@@ -274,9 +256,8 @@ class RbacPolicyTest(base.TestCase):
     def test_invalid_policy_rule_throws_rbac_parsing_exception(self, m_log):
         test_tenant_id = mock.sentinel.tenant_id
         test_user_id = mock.sentinel.user_id
-        self.mock_path.path.join.return_value = self.custom_policy_file
         parser = rbac_policy_parser.RbacPolicyParser(
-            test_tenant_id, test_user_id, "test_service")
+            test_tenant_id, test_user_id, "custom_rbac_policy")
 
         fake_rule = 'fake_rule'
         expected_message = "Policy action: {0} not found in policy file: {1}."\
@@ -291,10 +272,9 @@ class RbacPolicyTest(base.TestCase):
     def test_unknown_exception_throws_rbac_parsing_exception(self, m_log):
         test_tenant_id = mock.sentinel.tenant_id
         test_user_id = mock.sentinel.user_id
-        self.mock_path.path.join.return_value = self.custom_policy_file
 
         parser = rbac_policy_parser.RbacPolicyParser(
-            test_tenant_id, test_user_id, "test_service")
+            test_tenant_id, test_user_id, "custom_rbac_policy")
         parser.rules = mock.MagicMock(
             **{'__getitem__.return_value.side_effect': Exception(
                mock.sentinel.error)})
@@ -327,12 +307,10 @@ class RbacPolicyTest(base.TestCase):
 
         test_tenant_id = mock.sentinel.tenant_id
         test_user_id = mock.sentinel.user_id
-        self.mock_path.path.join.return_value = self.tenant_policy_file
         parser = rbac_policy_parser.RbacPolicyParser(
-            test_tenant_id, test_user_id, "test_service")
+            test_tenant_id, test_user_id, "tenant_rbac_policy")
 
         policy_data = parser._get_policy_data('fake_service')
-
         self.assertIsInstance(policy_data, str)
 
         actual_policy_data = json.loads(policy_data)
@@ -346,7 +324,6 @@ class RbacPolicyTest(base.TestCase):
             "rule4": "user_id:%(user_id)s",
             "admin_tenant_rule": "role:admin and tenant_id:%(tenant_id)s",
             "admin_user_rule": "role:admin and user_id:%(user_id)s"
-
         }
 
         self.assertEqual(expected_policy_data, actual_policy_data)
@@ -371,12 +348,10 @@ class RbacPolicyTest(base.TestCase):
 
         test_tenant_id = mock.sentinel.tenant_id
         test_user_id = mock.sentinel.user_id
-        self.mock_path.path.join.return_value = self.tenant_policy_file
+
         parser = rbac_policy_parser.RbacPolicyParser(
-            test_tenant_id, test_user_id, "test_service")
-
+            test_tenant_id, test_user_id, 'tenant_rbac_policy')
         policy_data = parser._get_policy_data('fake_service')
-
         self.assertIsInstance(policy_data, str)
 
         actual_policy_data = json.loads(policy_data)
@@ -392,23 +367,18 @@ class RbacPolicyTest(base.TestCase):
 
         self.assertEqual(expected_policy_data, actual_policy_data)
 
-    @mock.patch.object(rbac_policy_parser, 'credentials', autospec=True)
     @mock.patch.object(rbac_policy_parser, 'stevedore', autospec=True)
-    def test_get_policy_data_cannot_find_policy(self, mock_stevedore,
-                                                mock_creds):
+    def test_get_policy_data_cannot_find_policy(self, mock_stevedore):
         mock_stevedore.named.NamedExtensionManager.return_value = None
-        mock_creds.AdminManager.return_value.identity_services_v3_client.\
-            list_services.return_value = {
-                'services': [{'name': 'test_service'}]}
-        self.mock_path.path.join.return_value = '/etc/test_service/policy.json'
         e = self.assertRaises(rbac_exceptions.RbacParsingException,
                               rbac_policy_parser.RbacPolicyParser,
                               None, None, 'test_service')
 
         expected_error = \
             'Policy file for {0} service neither found in code '\
-            'nor at {1}.'.format('test_service',
-                                 '/etc/test_service/policy.json')
+            'nor at {1}.'.format(
+                'test_service',
+                [CONF.rbac.custom_policy_files[0] % 'test_service'])
 
         self.assertIn(expected_error, str(e))
 
@@ -416,8 +386,6 @@ class RbacPolicyTest(base.TestCase):
     @mock.patch.object(rbac_policy_parser, 'stevedore', autospec=True)
     def test_get_policy_data_without_valid_policy(self, mock_stevedore,
                                                   mock_json):
-        self.mock_path.path.isfile.return_value = False
-
         test_policy_action = mock.Mock(check='rule:bar')
         test_policy_action.configure_mock(name='foo')
 
@@ -450,13 +418,12 @@ class RbacPolicyTest(base.TestCase):
                                                 mock_json):
         mock_stevedore.named.NamedExtensionManager.return_value = None
         mock_json.loads.side_effect = ValueError
-        self.mock_path.path.join.return_value = self.tenant_policy_file
         e = self.assertRaises(rbac_exceptions.RbacParsingException,
                               rbac_policy_parser.RbacPolicyParser,
-                              None, None, 'test_service')
-
-        expected_error = 'Policy file for {0} service neither found in code '\
-                         'nor at {1}.'.format('test_service',
-                                              self.tenant_policy_file)
+                              None, None, 'tenant_rbac_policy')
 
+        expected_error = (
+            'Policy file for {0} service neither found in code nor at {1}.'
+            .format('tenant_rbac_policy', [CONF.rbac.custom_policy_files[0]
+                                           % 'tenant_rbac_policy']))
         self.assertIn(expected_error, str(e))
diff --git a/releasenotes/notes/dynamic-policy-file-discovery-104cbfc64b55d605.yaml b/releasenotes/notes/dynamic-policy-file-discovery-104cbfc64b55d605.yaml
new file mode 100644
index 00000000..59019cf6
--- /dev/null
+++ b/releasenotes/notes/dynamic-policy-file-discovery-104cbfc64b55d605.yaml
@@ -0,0 +1,22 @@
+---
+features:
+  - |
+    Add new configuration option ``[rbac] custom_policy_files``,
+    allowing users to specify list of the paths to search for custom
+    policy files. Each policy path assumes that the service name is
+    included in the path once. Also assumes Patrole is on the same host
+    as the policy files. The paths should be ordered by precedence, with
+    high-priority paths before low-priority paths. The first path that
+    is found to contain the service's policy file will be used.
+deprecations:
+  - |
+    Deprecate the following configuration options from ``[rbac]`` group:
+
+    * cinder_policy_file
+    * glance_policy_file
+    * keystone_policy_file
+    * neutron_policy_file
+    * nova_policy_file
+
+    It is better to use ``[rbac] custom_policy_files`` which supports
+    any OpenStack service.