Merge rbac_auth with rbac_rule_validation

Currently, rbac_auth doesn't do much: It decentralizes logic
that can be easily merged into rbac_rule_validation without
doing anything authentication-related. All rbac_auth does is:

  1) Construct RbacPolicyParser and check whether a given role
     is allowed to perform a given policy action.
  2) Dump some info to LOG
  3) Catch some exceptions

Thus, there's no justification for keeping rbac_auth. It doesn't
provide a high-enough level of abstraction to warrant being used.
It should be removed and its logic inserted in rbac_rule_validation.

Change-Id: I756175ea28ec11f24150f46d5ae4c2f64499a0ea
Closes-Bug: #1681459
diff --git a/patrole_tempest_plugin/rbac_auth.py b/patrole_tempest_plugin/rbac_auth.py
deleted file mode 100644
index ae497f3..0000000
--- a/patrole_tempest_plugin/rbac_auth.py
+++ /dev/null
@@ -1,49 +0,0 @@
-# Copyright 2017 AT&T Corporation.
-# All Rights Reserved.
-#
-#    Licensed under the Apache License, Version 2.0 (the "License"); you may
-#    not use this file except in compliance with the License. You may obtain
-#    a copy of the License at
-#
-#         http://www.apache.org/licenses/LICENSE-2.0
-#
-#    Unless required by applicable law or agreed to in writing, software
-#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-#    License for the specific language governing permissions and limitations
-#    under the License.
-
-import testtools
-
-from oslo_log import log as logging
-
-from tempest import config
-
-from patrole_tempest_plugin import rbac_exceptions
-from patrole_tempest_plugin import rbac_policy_parser
-
-LOG = logging.getLogger(__name__)
-CONF = config.CONF
-
-
-class RbacAuthority(object):
-    def __init__(self, project_id, user_id, service, extra_target_data):
-        self.policy_parser = rbac_policy_parser.RbacPolicyParser(
-            project_id, user_id, service, extra_target_data=extra_target_data)
-
-    def get_permission(self, rule_name, role):
-        try:
-            is_allowed = self.policy_parser.allowed(rule_name, role)
-            if is_allowed:
-                LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule_name,
-                          role)
-            else:
-                LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
-                          rule_name, role)
-            return is_allowed
-        except rbac_exceptions.RbacParsingException as e:
-            if CONF.rbac.strict_policy_check:
-                raise e
-            else:
-                raise testtools.TestCase.skipException(str(e))
-        return False
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 6a5ed5e..5a6bc74 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -15,6 +15,7 @@
 
 import logging
 import sys
+import testtools
 
 import six
 
@@ -22,8 +23,8 @@
 from tempest.lib import exceptions
 from tempest import test
 
-from patrole_tempest_plugin import rbac_auth
 from patrole_tempest_plugin import rbac_exceptions
+from patrole_tempest_plugin import rbac_policy_parser
 
 CONF = config.CONF
 LOG = logging.getLogger(__name__)
@@ -62,18 +63,15 @@
     :raises RbacOverPermission: for bullet (3) above.
     """
     def decorator(func):
+        role = CONF.rbac.rbac_test_role
+
         def wrapper(*args, **kwargs):
-            try:
-                caller_ref = None
-                if args and isinstance(args[0], test.BaseTestCase):
-                    caller_ref = args[0]
-                project_id = caller_ref.auth_provider.credentials.project_id
-                user_id = caller_ref.auth_provider.credentials.user_id
-            except AttributeError as e:
-                msg = ("{0}: project_id/user_id not found in "
-                       "cls.auth_provider.credentials".format(e))
-                LOG.error(msg)
-                raise rbac_exceptions.RbacResourceSetupFailed(msg)
+            if args and isinstance(args[0], test.BaseTestCase):
+                test_obj = args[0]
+            else:
+                raise rbac_exceptions.RbacResourceSetupFailed(
+                    '`rbac_rule_validation` decorator can only be applied to '
+                    'an instance of `tempest.test.BaseTestCase`.')
 
             if admin_only:
                 LOG.info("As admin_only is True, only admin role should be "
@@ -81,33 +79,28 @@
                          "check for policy action {0}.".format(rule))
                 allowed = CONF.rbac.rbac_test_role == 'admin'
             else:
-                authority = rbac_auth.RbacAuthority(
-                    project_id, user_id, service,
-                    _format_extra_target_data(caller_ref, extra_target_data))
-                allowed = authority.get_permission(
-                    rule, CONF.rbac.rbac_test_role)
+                allowed = _is_authorized(test_obj, service, rule,
+                                         extra_target_data)
 
             expected_exception, irregular_msg = _get_exception_type(
                 expected_error_code)
 
             try:
-                func(*args)
+                func(*args, **kwargs)
             except rbac_exceptions.RbacInvalidService as e:
                 msg = ("%s is not a valid service." % service)
                 LOG.error(msg)
                 raise exceptions.NotFound(
-                    "%s RbacInvalidService was: %s" %
-                    (msg, e))
+                    "%s RbacInvalidService was: %s" % (msg, e))
             except (expected_exception, rbac_exceptions.RbacActionFailed) as e:
                 if irregular_msg:
                     LOG.warning(irregular_msg.format(rule, service))
                 if allowed:
                     msg = ("Role %s was not allowed to perform %s." %
-                           (CONF.rbac.rbac_test_role, rule))
+                           (role, rule))
                     LOG.error(msg)
                     raise exceptions.Forbidden(
-                        "%s exception was: %s" %
-                        (msg, e))
+                        "%s exception was: %s" % (msg, e))
             except Exception as e:
                 exc_info = sys.exc_info()
                 error_details = exc_info[1].__str__()
@@ -119,21 +112,58 @@
             else:
                 if not allowed:
                     LOG.error("Role %s was allowed to perform %s" %
-                              (CONF.rbac.rbac_test_role, rule))
+                              (role, rule))
                     raise rbac_exceptions.RbacOverPermission(
                         "OverPermission: Role %s was allowed to perform %s" %
-                        (CONF.rbac.rbac_test_role, rule))
+                        (role, rule))
             finally:
-                caller_ref.rbac_utils.switch_role(caller_ref,
-                                                  toggle_rbac_role=False)
-        return wrapper
+                test_obj.rbac_utils.switch_role(test_obj,
+                                                toggle_rbac_role=False)
+
+        _wrapper = testtools.testcase.attr(role)(wrapper)
+        return _wrapper
     return decorator
 
 
+def _is_authorized(test_obj, service, rule_name, extra_target_data):
+    try:
+        project_id = test_obj.auth_provider.credentials.project_id
+        user_id = test_obj.auth_provider.credentials.user_id
+    except AttributeError as e:
+        msg = ("{0}: project_id/user_id not found in "
+               "cls.auth_provider.credentials".format(e))
+        LOG.error(msg)
+        raise rbac_exceptions.RbacResourceSetupFailed(msg)
+
+    try:
+        role = CONF.rbac.rbac_test_role
+        formatted_target_data = _format_extra_target_data(
+            test_obj, extra_target_data)
+        policy_parser = rbac_policy_parser.RbacPolicyParser(
+            project_id, user_id, service,
+            extra_target_data=formatted_target_data)
+        is_allowed = policy_parser.allowed(rule_name, role)
+
+        if is_allowed:
+            LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule_name,
+                      role)
+        else:
+            LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
+                      rule_name, role)
+        return is_allowed
+    except rbac_exceptions.RbacParsingException as e:
+        if CONF.rbac.strict_policy_check:
+            raise e
+        else:
+            raise testtools.TestCase.skipException(str(e))
+    return False
+
+
 def _get_exception_type(expected_error_code):
     expected_exception = None
     irregular_msg = None
     supported_error_codes = [403, 404]
+
     if expected_error_code == 403:
         expected_exception = exceptions.Forbidden
     elif expected_error_code == 404:
diff --git a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
index 174945e..41af3b2 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -19,7 +19,6 @@
 from tempest import test
 from tempest.tests import base
 
-from patrole_tempest_plugin import rbac_auth
 from patrole_tempest_plugin import rbac_exceptions
 from patrole_tempest_plugin import rbac_rule_validation as rbac_rv
 
@@ -43,8 +42,9 @@
         self.addCleanup(CONF.clear_override, 'rbac_test_role', group='rbac')
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_rule_validation_have_permission_no_exc(self, mock_auth, mock_log):
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_rule_validation_have_permission_no_exc(self, mock_policy,
+                                                    mock_log):
         """Test that having permission and no exception thrown is success.
 
         Positive test case success scenario.
@@ -54,9 +54,7 @@
         mock_function = mock.Mock()
         wrapper = decorator(mock_function)
 
-        mock_permission = mock.Mock()
-        mock_permission.get_permission.return_value = True
-        mock_auth.return_value = mock_permission
+        mock_policy.RbacPolicyParser.return_value.allowed.return_value = True
 
         result = wrapper(self.mock_args)
 
@@ -65,8 +63,8 @@
         mock_log.error.assert_not_called()
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_rule_validation_lack_permission_throw_exc(self, mock_auth,
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_rule_validation_lack_permission_throw_exc(self, mock_policy,
                                                        mock_log):
         """Test that having no permission and exception thrown is success.
 
@@ -78,9 +76,7 @@
         mock_function.side_effect = exceptions.Forbidden
         wrapper = decorator(mock_function)
 
-        mock_permission = mock.Mock()
-        mock_permission.get_permission.return_value = False
-        mock_auth.return_value = mock_permission
+        mock_policy.RbacPolicyParser.return_value.allowed.return_value = False
 
         result = wrapper(self.mock_args)
 
@@ -89,8 +85,8 @@
         mock_log.error.assert_not_called()
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_rule_validation_forbidden_negative(self, mock_auth, mock_log):
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_rule_validation_forbidden_negative(self, mock_policy, mock_log):
         """Test Forbidden error is thrown and have permission fails.
 
         Negative test case: if Forbidden is thrown and the user should be
@@ -102,9 +98,7 @@
         mock_function.side_effect = exceptions.Forbidden
         wrapper = decorator(mock_function)
 
-        mock_permission = mock.Mock()
-        mock_permission.get_permission.return_value = True
-        mock_auth.return_value = mock_permission
+        mock_policy.RbacPolicyParser.return_value.allowed.return_value = True
 
         e = self.assertRaises(exceptions.Forbidden, wrapper, self.mock_args)
         self.assertIn(
@@ -114,8 +108,8 @@
                                                " perform sentinel.action.")
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_rule_validation_rbac_action_failed_positive(self, mock_auth,
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_rule_validation_rbac_action_failed_positive(self, mock_policy,
                                                          mock_log):
         """Test RbacActionFailed error is thrown without permission passes.
 
@@ -127,9 +121,7 @@
         mock_function.side_effect = rbac_exceptions.RbacActionFailed
         wrapper = decorator(mock_function)
 
-        mock_permission = mock.Mock()
-        mock_permission.get_permission.return_value = False
-        mock_auth.return_value = mock_permission
+        mock_policy.RbacPolicyParser.return_value.allowed.return_value = False
 
         result = wrapper(self.mock_args)
 
@@ -138,8 +130,8 @@
         mock_log.warning.assert_not_called()
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_rule_validation_rbac_action_failed_negative(self, mock_auth,
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_rule_validation_rbac_action_failed_negative(self, mock_policy,
                                                          mock_log):
         """Test RbacActionFailed error is thrown with permission fails.
 
@@ -151,9 +143,7 @@
         mock_function.side_effect = rbac_exceptions.RbacActionFailed
         wrapper = decorator(mock_function)
 
-        mock_permission = mock.Mock()
-        mock_permission.get_permission.return_value = True
-        mock_auth.return_value = mock_permission
+        mock_policy.RbacPolicyParser.return_value.allowed.return_value = True
 
         e = self.assertRaises(exceptions.Forbidden, wrapper, self.mock_args)
         self.assertIn(
@@ -164,8 +154,9 @@
                                                " perform sentinel.action.")
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_expect_not_found_but_raises_forbidden(self, mock_auth, mock_log):
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_expect_not_found_but_raises_forbidden(self, mock_policy,
+                                                   mock_log):
         """Test that expecting 404 but getting 403 works for all scenarios.
 
         Tests the following scenarios:
@@ -186,9 +177,8 @@
                          "NotFound, which was not thrown."
 
         for permission in [True, False]:
-            mock_permission = mock.Mock()
-            mock_permission.get_permission.return_value = permission
-            mock_auth.return_value = mock_permission
+            mock_policy.RbacPolicyParser.return_value.allowed.return_value =\
+                permission
 
             e = self.assertRaises(exceptions.Forbidden, wrapper,
                                   self.mock_args)
@@ -197,8 +187,8 @@
             mock_log.error.reset_mock()
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_expect_not_found_and_raise_not_found(self, mock_auth, mock_log):
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_expect_not_found_and_raise_not_found(self, mock_policy, mock_log):
         """Test that expecting 404 and getting 404 works for all scenarios.
 
         Tests the following scenarios:
@@ -220,9 +210,8 @@
         ]
 
         for pos, permission in enumerate([True, False]):
-            mock_permission = mock.Mock()
-            mock_permission.get_permission.return_value = permission
-            mock_auth.return_value = mock_permission
+            mock_policy.RbacPolicyParser.return_value.allowed.return_value =\
+                permission
 
             expected_error = expected_errors[pos]
 
@@ -245,8 +234,8 @@
             mock_log.error.reset_mock()
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_rule_validation_overpermission_negative(self, mock_auth,
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_rule_validation_overpermission_negative(self, mock_policy,
                                                      mock_log):
         """Test that OverPermission is correctly handled.
 
@@ -258,9 +247,7 @@
         mock_function = mock.Mock()
         wrapper = decorator(mock_function)
 
-        mock_permission = mock.Mock()
-        mock_permission.get_permission.return_value = False
-        mock_auth.return_value = mock_permission
+        mock_policy.RbacPolicyParser.return_value.allowed.return_value = False
 
         e = self.assertRaises(rbac_exceptions.RbacOverPermission, wrapper,
                               self.mock_args)
@@ -270,7 +257,7 @@
         mock_log.error.assert_called_once_with(
             "Role Member was allowed to perform sentinel.action")
 
-    @mock.patch.object(rbac_auth, 'rbac_policy_parser', autospec=True)
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
     def test_invalid_policy_rule_throws_parsing_exception(
             self, mock_rbac_policy_parser):
         """Test that invalid policy action causes test to be skipped."""
@@ -295,8 +282,8 @@
             mock.sentinel.project_id, mock.sentinel.user_id,
             mock.sentinel.service, extra_target_data={})
 
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_get_exception_type_404(self, mock_auth):
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_get_exception_type_404(self, mock_policy):
         """Test that getting a 404 exception type returns NotFound."""
         expected_exception = exceptions.NotFound
         expected_irregular_msg = ("NotFound exception was caught for policy "
@@ -309,8 +296,8 @@
         self.assertEqual(expected_exception, actual_exception)
         self.assertEqual(expected_irregular_msg, actual_irregular_msg)
 
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_get_exception_type_403(self, mock_auth):
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_get_exception_type_403(self, mock_policy):
         """Test that getting a 404 exception type returns Forbidden."""
         expected_exception = exceptions.Forbidden
         expected_irregular_msg = None
@@ -322,8 +309,9 @@
         self.assertEqual(expected_irregular_msg, actual_irregular_msg)
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_exception_thrown_when_type_is_not_int(self, mock_auth, mock_log):
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_exception_thrown_when_type_is_not_int(self, mock_policy,
+                                                   mock_log):
         """Test that non-integer exception type raises error."""
         self.assertRaises(rbac_exceptions.RbacInvalidErrorCode,
                           rbac_rv._get_exception_type, "403")
@@ -333,8 +321,8 @@
                                                "codes: [403, 404]")
 
     @mock.patch.object(rbac_rv, 'LOG', autospec=True)
-    @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
-    def test_exception_thrown_when_type_is_403_or_404(self, mock_auth,
+    @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+    def test_exception_thrown_when_type_is_403_or_404(self, mock_policy,
                                                       mock_log):
         """Test that unsupported exceptions throw error."""
         invalid_exceptions = [200, 400, 500]
diff --git a/releasenotes/notes/merge-rbac-auth-with-rbac-rule-validation-5d7c286788a95ee9.yaml b/releasenotes/notes/merge-rbac-auth-with-rbac-rule-validation-5d7c286788a95ee9.yaml
new file mode 100644
index 0000000..b96c73a
--- /dev/null
+++ b/releasenotes/notes/merge-rbac-auth-with-rbac-rule-validation-5d7c286788a95ee9.yaml
@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    Merges `rbac_auth` with `rbac_rule_validation`, because `rbac_auth`
+    decentralized logic from `rbac_rule_validation` without providing any
+    authentication-related utility. This change facilitates code maintenance
+    and code readability.