From 0854dedaec8220ac9cce6d96b06e11a4a73aa541 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Fri, 5 May 2017 16:30:55 +0100 Subject: [PATCH] Adds initial hacking checks to Patrole This patch: - Adds hacking check to Patrole (executed via tox -e pep8) - Corrects a few hacking errors - Adds hacking documentation to Patrole Change-Id: Id43e24060a5290df91c594df6a38ba0cb239bbaf --- HACKING.rst | 60 ++++- patrole_tempest_plugin/hacking/__init__.py | 0 patrole_tempest_plugin/hacking/checks.py | 216 ++++++++++++++++++ patrole_tempest_plugin/rbac_policy_parser.py | 6 +- .../rbac_rule_validation.py | 6 +- requirements.txt | 2 +- tox.ini | 3 + 7 files changed, 287 insertions(+), 6 deletions(-) create mode 100644 patrole_tempest_plugin/hacking/__init__.py create mode 100644 patrole_tempest_plugin/hacking/checks.py diff --git a/HACKING.rst b/HACKING.rst index bb337b0a..b1d730d0 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -1,6 +1,62 @@ Patrole Style Commandments ========================== -Read the OpenStack Style Commandments: ``__ +- Step 1: Read the OpenStack Style Commandments: ``__ +- Step 2: Review Tempest's Style Commandments: ``__ +- Step 3: Read on -Also review Tempest's Style Commandments: ``__ +Patrole Specific Commandments +------------------------------ + +Patrole borrows the following commandments from Tempest; refer to +`Tempest's Commandments `__ +for more information: + +.. note:: + + The original Tempest Commandments do not include Patrole-specific paths. + Patrole-specific paths replace the Tempest-specific paths within Patrole's + hacking checks. +.. + +- [T102] Cannot import OpenStack python clients in patrole_tempest_plugin/tests/api +- [T105] Tests cannot use setUpClass/tearDownClass +- [T106] vim configuration should not be kept in source files. +- [T107] Check that a service tag isn't in the module path +- [T108] Check no hyphen at the end of rand_name() argument +- [T109] Cannot use testtools.skip decorator; instead use + decorators.skip_because from tempest.lib +- [T113] Check that tests use data_utils.rand_uuid() instead of uuid.uuid4() +- [N322] Method's default argument shouldn't be mutable + +The following are Patrole's specific Commandments: + +- [P100] The ``rbac_rule_validation.action`` decorator must be applied to + an RBAC test (the check fails if the decorator is not one of the + two decorators directly above the function declaration) +- [P101] RBAC test filenames must end with "_rbac.py"; for example, + test_servers_rbac.py, not test_servers.py +- [P102] RBAC test class names must end in 'RbacTest' + +Role Switching +-------------- + +Correct role switching is vital to correct RBAC testing within Patrole. If a +test does not call ``rbac_utils.switch_role`` with ``toggle_rbac_role=True`` +within the RBAC test, then the test is *not* a valid RBAC test: The API +endpoint under test will be performed with admin credentials, which is always +wrong unless ``CONF.rbac_test_role`` is admin. + +.. note:: + + Switching back to the admin role for setup and clean up is automatically + performed. Toggling ``switch_role`` with ``toggle_rbac_role=False`` within + the context of a test should *never* be performed and doing so will likely + result in an error being thrown. +.. + +Patrole does not have a hacking check for role switching, but does use a +built-in mechanism for verifying that role switching is being correctly +executed across tests. If a test does not call ``switch_role`` with +``toggle_rbac_role=True``, then an ``RbacResourceSetupFailed`` exception +will be raised. diff --git a/patrole_tempest_plugin/hacking/__init__.py b/patrole_tempest_plugin/hacking/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/patrole_tempest_plugin/hacking/checks.py b/patrole_tempest_plugin/hacking/checks.py new file mode 100644 index 00000000..e7e5cb01 --- /dev/null +++ b/patrole_tempest_plugin/hacking/checks.py @@ -0,0 +1,216 @@ +# Copyright 2013 IBM Corp. +# 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 os +import re + +import pep8 + + +PYTHON_CLIENTS = ['cinder', 'glance', 'keystone', 'nova', 'swift', 'neutron', + 'ironic', 'heat', 'sahara'] + +PYTHON_CLIENT_RE = re.compile('import (%s)client' % '|'.join(PYTHON_CLIENTS)) +TEST_DEFINITION = re.compile(r'^\s*def test.*') +SETUP_TEARDOWN_CLASS_DEFINITION = re.compile(r'^\s+def (setUp|tearDown)Class') +SCENARIO_DECORATOR = re.compile(r'\s*@.*services\((.*)\)') +VI_HEADER_RE = re.compile(r"^#\s+vim?:.+") +RAND_NAME_HYPHEN_RE = re.compile(r".*rand_name\(.+[\-\_][\"\']\)") +MUTABLE_DEFAULT_ARGS = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])") +TESTTOOLS_SKIP_DECORATOR = re.compile(r'\s*@testtools\.skip\((.*)\)') +TEST_METHOD = re.compile(r"^ def test_.+") +CLASS = re.compile(r"^class .+") +RBAC_CLASS_NAME_RE = re.compile(r'class .+RbacTest') +RULE_VALIDATION_DECORATOR = re.compile( + r'\s*@.*rbac_rule_validation.action\((.*)\)') +IDEMPOTENT_ID_DECORATOR = re.compile(r'\s*@decorators\.idempotent_id\((.*)\)') + +previous_decorator = None + + +def import_no_clients_in_api_tests(physical_line, filename): + """Check for client imports from patrole_tempest_plugin/tests/api + + T102: Cannot import OpenStack python clients + """ + if "patrole_tempest_plugin/tests/api" in filename: + res = PYTHON_CLIENT_RE.match(physical_line) + if res: + return (physical_line.find(res.group(1)), + ("T102: python clients import not allowed " + "in patrole_tempest_plugin/tests/api/* or " + "patrole_tempest_plugin/tests/scenario/* tests")) + + +def no_setup_teardown_class_for_tests(physical_line, filename): + """Check that tests do not use setUpClass/tearDownClass + + T105: Tests cannot use setUpClass/tearDownClass + """ + if pep8.noqa(physical_line): + return + + if SETUP_TEARDOWN_CLASS_DEFINITION.match(physical_line): + return (physical_line.find('def'), + "T105: (setUp|tearDown)Class can not be used in tests") + + +def no_vi_headers(physical_line, line_number, lines): + """Check for vi editor configuration in source files. + + By default vi modelines can only appear in the first or + last 5 lines of a source file. + + T106 + """ + # NOTE(gilliard): line_number is 1-indexed + if line_number <= 5 or line_number > len(lines) - 5: + if VI_HEADER_RE.match(physical_line): + return 0, "T106: Don't put vi configuration in source files" + + +def service_tags_not_in_module_path(physical_line, filename): + """Check that a service tag isn't in the module path + + A service tag should only be added if the service name isn't already in + the module path. + + T107 + """ + matches = SCENARIO_DECORATOR.match(physical_line) + if matches: + services = matches.group(1).split(',') + for service in services: + service_name = service.strip().strip("'") + modulepath = os.path.split(filename)[0] + if service_name in modulepath: + return (physical_line.find(service_name), + "T107: service tag should not be in path") + + +def no_hyphen_at_end_of_rand_name(logical_line, filename): + """Check no hyphen at the end of rand_name() argument + + T108 + """ + msg = "T108: hyphen should not be specified at the end of rand_name()" + if RAND_NAME_HYPHEN_RE.match(logical_line): + return 0, msg + + +def no_mutable_default_args(logical_line): + """Check that mutable object isn't used as default argument + + N322: Method's default argument shouldn't be mutable + """ + msg = "N322: Method's default argument shouldn't be mutable!" + if MUTABLE_DEFAULT_ARGS.match(logical_line): + yield (0, msg) + + +def no_testtools_skip_decorator(logical_line): + """Check that methods do not have the testtools.skip decorator + + T109 + """ + if TESTTOOLS_SKIP_DECORATOR.match(logical_line): + yield (0, "T109: Cannot use testtools.skip decorator; instead use " + "decorators.skip_because from tempest.lib") + + +def use_rand_uuid_instead_of_uuid4(logical_line, filename): + """Check that tests use data_utils.rand_uuid() instead of uuid.uuid4() + + T113 + """ + if 'uuid.uuid4()' not in logical_line: + return + + msg = ("T113: Tests should use data_utils.rand_uuid()/rand_uuid_hex() " + "instead of uuid.uuid4()/uuid.uuid4().hex") + yield (0, msg) + + +def no_rbac_rule_validation_decorator(physical_line, filename, + previous_logical): + """Check that each test has the ``rbac_rule_validation.action`` decorator. + + Checks whether the test function has "@rbac_rule_validation.action" + above it; otherwise checks that it has "@decorators.idempotent_id" above + it and "@rbac_rule_validation.action" above that. + + Assumes that ``rbac_rule_validation.action`` decorator is either the first + or second decorator above the test function; otherwise this check fails. + + P100 + """ + global previous_decorator + + if "patrole_tempest_plugin/tests/api" in filename: + + if IDEMPOTENT_ID_DECORATOR.match(physical_line): + previous_decorator = previous_logical + return + + if TEST_METHOD.match(physical_line): + if not RULE_VALIDATION_DECORATOR.match(previous_logical) and \ + not RULE_VALIDATION_DECORATOR.match(previous_decorator): + return (0, "Must use rbac_rule_validation.action " + "decorator for API and scenario tests") + + +def no_rbac_suffix_in_test_filename(physical_line, filename, previous_logical): + """Check that RBAC filenames end with "_rbac" suffix. + + P101 + """ + if "patrole_tempest_plugin/tests/api" in filename: + + if filename.endswith('rbac_base.py'): + return + + if not filename.endswith('_rbac.py'): + return 0, "RBAC test filenames must end in _rbac suffix" + + +def no_rbac_test_suffix_in_test_class_name(physical_line, filename, + previous_logical): + """Check that RBAC class names end with "RbacTest" + + P102 + """ + if "patrole_tempest_plugin/tests/api" in filename: + + if filename.endswith('rbac_base.py'): + return + + if CLASS.match(physical_line): + if not RBAC_CLASS_NAME_RE.match(physical_line): + return 0, "RBAC test class names must end in 'RbacTest'" + + +def factory(register): + register(import_no_clients_in_api_tests) + register(no_setup_teardown_class_for_tests) + register(no_vi_headers) + register(no_hyphen_at_end_of_rand_name) + register(no_mutable_default_args) + register(no_testtools_skip_decorator) + register(use_rand_uuid_instead_of_uuid4) + register(service_tags_not_in_module_path) + register(no_rbac_rule_validation_decorator) + register(no_rbac_suffix_in_test_filename) + register(no_rbac_test_suffix_in_test_class_name) diff --git a/patrole_tempest_plugin/rbac_policy_parser.py b/patrole_tempest_plugin/rbac_policy_parser.py index 82568891..d4e989bd 100644 --- a/patrole_tempest_plugin/rbac_policy_parser.py +++ b/patrole_tempest_plugin/rbac_policy_parser.py @@ -40,8 +40,7 @@ class RbacPolicyParser(object): each role, whether a given rule is allowed using oslo policy. """ - def __init__(self, project_id, user_id, service=None, - extra_target_data={}): + def __init__(self, project_id, user_id, service, extra_target_data=None): """Initialization of Rbac Policy Parser. Parses a policy file to create a dictionary, mapping policy actions to @@ -64,6 +63,9 @@ class RbacPolicyParser(object): :param path: type string """ + if extra_target_data is None: + extra_target_data = {} + # First check if the service is valid service = service.lower().strip() if service else None self.admin_mgr = credentials.AdminManager() diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py index 8de3d970..d77b2d65 100644 --- a/patrole_tempest_plugin/rbac_rule_validation.py +++ b/patrole_tempest_plugin/rbac_rule_validation.py @@ -31,7 +31,7 @@ LOG = logging.getLogger(__name__) def action(service, rule='', admin_only=False, expected_error_code=403, - extra_target_data={}): + extra_target_data=None): """A decorator which does a policy check and matches it against test run. A decorator which allows for positive and negative RBAC testing. Given @@ -62,6 +62,10 @@ def action(service, rule='', admin_only=False, expected_error_code=403, :raises Forbidden: for bullet (2) above. :raises RbacOverPermission: for bullet (3) above. """ + + if extra_target_data is None: + extra_target_data = {} + def decorator(func): role = CONF.rbac.rbac_test_role diff --git a/requirements.txt b/requirements.txt index ffc6abe3..6d2c7f1c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,7 +1,7 @@ # The order of packages is significant, because pip processes them in the order # of appearance. Changing the order has an impact on the overall integration # process, which may cause wedges in the gate later. - +hacking!=0.13.0,<0.14,>=0.12.0 # Apache-2.0 pbr>=1.8 # Apache-2.0 urllib3>=1.15.1 # MIT oslo.log>=3.11.0 # Apache-2.0 diff --git a/tox.ini b/tox.ini index a004c6e7..be355099 100644 --- a/tox.ini +++ b/tox.ini @@ -59,3 +59,6 @@ show-source = True ignore = E123,E125 builtins = _ exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build + +[hacking] +local-check-factory = patrole_tempest_plugin.hacking.checks.factory