pylint_plugins: add forbidden import checker

Add new pylint AST checker plugin which implements a check for imports
forbidden in IPA. Which imports are forbidden is configurable in pylintrc.

Provide default forbidden import configuration and disable the check for
existing forbidden imports in our code base.

Reviewed-By: Martin Basti <mbasti@redhat.com>

Makefile.am

@@ -180,7 +180,9 @@ pylint: $(top_builddir)/ipapython/version.py ipasetup.py
 		-type f -exec grep -qsm1 '^#!.*\bpython' '{}' \; -print`; \
 	echo "Pylint is running, please wait ..."; \
 	PYTHONPATH=$(top_srcdir) $(PYTHON) -m pylint \
-		--rcfile=$(top_srcdir)/pylintrc $${FILES}
+		--rcfile=$(top_srcdir)/pylintrc \
+		--load-plugins pylint_plugins \
+		$${FILES}
 
 .PHONY: jslint jslint-ui jslint-ui-test jslint-html \
 	$(top_builddir)/install/ui/src/libs/loader.js

ipaclient/install/ipa_certupdate.py

@@ -100,9 +100,9 @@ class CertUpdate(admintool.AdminTool):
         if server_fstore.has_files():
             self.update_server(certs)
             try:
-                # pylint: disable=import-error
+                # pylint: disable=import-error,ipa-forbidden-import
                 from ipaserver.install import cainstance
-                # pylint: enable=import-error
+                # pylint: enable=import-error,ipa-forbidden-import
                 cainstance.add_lightweight_ca_tracking_requests(
                     self.log, lwcas)
             except Exception:

ipaclient/remote_plugins/__init__.py

@@ -109,7 +109,9 @@ class ServerInfo(collections.MutableMapping):
 
 def get_package(api):
     if api.env.in_tree:
-        from ipaserver import plugins  # pylint: disable=import-error
+        # pylint: disable=import-error,ipa-forbidden-import
+        from ipaserver import plugins
+        # pylint: enable=import-error,ipa-forbidden-import
     else:
         try:
             plugins = api._remote_plugins

ipalib/__init__.py

@@ -935,7 +935,9 @@ class API(plugable.API):
     @property
     def packages(self):
         if self.env.in_server:
-            import ipaserver.plugins  # pylint: disable=import-error
+            # pylint: disable=import-error,ipa-forbidden-import
+            import ipaserver.plugins
+            # pylint: enable=import-error,ipa-forbidden-import
             result = (
                 ipaserver.plugins,
             )
@@ -948,7 +950,9 @@ class API(plugable.API):
             )
 
         if self.env.context in ('installer', 'updates'):
-            import ipaserver.install.plugins  # pylint: disable=import-error
+            # pylint: disable=import-error,ipa-forbidden-import
+            import ipaserver.install.plugins
+            # pylint: enable=import-error,ipa-forbidden-import
             result += (ipaserver.install.plugins,)
 
         return result

ipalib/config.py

@@ -48,7 +48,9 @@ from ipalib.constants import (
 )
 from ipalib import errors
 try:
+    # pylint: disable=ipa-forbidden-import
     from ipaplatform.tasks import tasks
+    # pylint: enable=ipa-forbidden-import
 except ImportError:
     tasks = None
 

ipaplatform/base/services.py

@@ -93,11 +93,13 @@ class PlatformService(object):
     """
 
     def __init__(self, service_name, api=None):
+        # pylint: disable=ipa-forbidden-import
+        import ipalib  # FixMe: break import cycle
+        # pylint: enable=ipa-forbidden-import
         self.service_name = service_name
         if api is not None:
             self.api = api
         else:
-            import ipalib  # FixMe: break import cycle
             self.api = ipalib.api
             warnings.warn(
                 "{s.__class__.__name__}('{s.service_name}', api=None) "

ipaplatform/debian/services.py

@@ -166,7 +166,9 @@ def debian_service_class_factory(name, api=None):
 
 class DebianServices(base_services.KnownServices):
     def __init__(self):
+        # pylint: disable=ipa-forbidden-import
         import ipalib  # FixMe: break import cycle
+        # pylint: enable=ipa-forbidden-import
         services = dict()
         for s in base_services.wellknownservices:
             services[s] = self.service_class_factory(s, ipalib.api)

ipaplatform/redhat/services.py

@@ -253,7 +253,9 @@ def redhat_service_class_factory(name, api=None):
 
 class RedHatServices(base_services.KnownServices):
     def __init__(self):
+        # pylint: disable=ipa-forbidden-import
         import ipalib  # FixMe: break import cycle
+        # pylint: enable=ipa-forbidden-import
         services = dict()
         for s in base_services.wellknownservices:
             services[s] = self.service_class_factory(s, ipalib.api)

ipaplatform/redhat/tasks.py

@@ -50,7 +50,9 @@ from ipaplatform.paths import paths
 from ipaplatform.redhat.authconfig import RedHatAuthConfig
 from ipaplatform.base.tasks import BaseTaskNamespace
 
+# pylint: disable=ipa-forbidden-import
 from ipalib.constants import IPAAPI_USER
+# pylint: enable=ipa-forbidden-import
 
 _ffi = FFI()
 _ffi.cdef("""
@@ -235,8 +237,10 @@ class RedHatTaskNamespace(BaseTaskNamespace):
             return True
 
     def insert_ca_certs_into_systemwide_ca_store(self, ca_certs):
+        # pylint: disable=ipa-forbidden-import
         from ipalib import x509  # FixMe: break import cycle
         from ipalib.errors import CertificateError
+        # pylint: enable=ipa-forbidden-import
 
         new_cacert_path = paths.SYSTEMWIDE_IPA_CA_CRT
 

ipapython/certdb.py

@@ -32,10 +32,12 @@ from nss.error import NSPRError
 from ipapython.dn import DN
 from ipapython.ipa_log_manager import root_logger
 from ipapython import ipautil
-from ipalib import x509
+from ipalib import x509     # pylint: disable=ipa-forbidden-import
 
 try:
-    from ipaplatform.paths import paths  # pylint: disable=import-error
+    # pylint: disable=import-error,ipa-forbidden-import
+    from ipaplatform.paths import paths
+    # pylint: enable=import-error,ipa-forbidden-import
 except ImportError:
     CERTUTIL = '/usr/bin/certutil'
     PK12UTIL = '/usr/bin/pk12util'

ipapython/config.py

@@ -35,7 +35,9 @@ from six.moves.urllib.parse import urlsplit
 from ipapython.dn import DN
 
 try:
+    # pylint: disable=ipa-forbidden-import
     from ipaplatform.paths import paths
+    # pylint: enable=ipa-forbidden-import
 except ImportError:
     IPA_DEFAULT_CONF = '/etc/ipa/default.conf'
 else:

ipapython/cookie.py

@@ -599,7 +599,9 @@ class Cookie(object):
             # FIXME: At the moment we can't import from ipalib at the
             # module level because of a dependency loop (cycle) in the
             # import. Our module layout needs to be refactored.
+            # pylint: disable=ipa-forbidden-import
             from ipalib.util import validate_domain_name
+            # pylint: enable=ipa-forbidden-import
             try:
                 validate_domain_name(url_domain)
             except Exception:

ipapython/dogtag.py

@@ -25,10 +25,12 @@ import six
 from six.moves.urllib.parse import urlencode
 # pylint: enable=import-error
 
+# pylint: disable=ipa-forbidden-import
 from ipalib import api, errors
 from ipalib.util import create_https_connection
 from ipalib.errors import NetworkError
 from ipalib.text import _
+# pylint: enable=ipa-forbidden-import
 from ipapython import ipautil
 from ipapython.ipa_log_manager import root_logger
 

ipapython/ipaldap.py

@@ -39,8 +39,10 @@ import ldap.filter
 from ldap.controls import SimplePagedResultsControl
 import six
 
+# pylint: disable=ipa-forbidden-import
 from ipalib import errors, _
 from ipalib.constants import LDAP_GENERALIZED_TIME_FORMAT
+# pylint: enable=ipa-forbidden-import
 from ipapython.ipautil import format_netloc, CIDict
 from ipapython.ipa_log_manager import log_mgr
 from ipapython.dn import DN

pylint_plugins.py

@@ -5,14 +5,18 @@
 from __future__ import print_function
 
 import copy
+import os.path
 import sys
 
 from astroid import MANAGER
 from astroid import scoped_nodes
+from pylint.checkers import BaseChecker
+from pylint.checkers.utils import check_messages
+from pylint.interfaces import IAstroidChecker
 
 
 def register(linter):
-    pass
+    linter.register_checker(IPAChecker(linter))
 
 
 def _warning_already_exists(cls, member):
@@ -252,3 +256,80 @@ def fix_ipa_classes(cls):
         fake_class(cls, ipa_class_members[class_name_with_module])
 
 MANAGER.register_transform(scoped_nodes.Class, fix_ipa_classes)
+
+
+class IPAChecker(BaseChecker):
+    __implements__ = IAstroidChecker
+
+    name = 'ipa'
+    msgs = {
+        'W9901': (
+            'Forbidden import %s (can\'t import from %s in %s)',
+            'ipa-forbidden-import',
+            'Used when an forbidden import is detected.',
+        ),
+    }
+    options = (
+        (
+            'forbidden-imports',
+            {
+                'default': '',
+                'type': 'csv',
+                'metavar': '<path>[:<module>[:<module>...]][,<path>...]',
+                'help': 'Modules which are forbidden to be imported in the '
+                        'given paths',
+            },
+        ),
+    )
+    priority = -1
+
+    def open(self):
+        self._dir = os.path.abspath(os.path.dirname(__file__))
+
+        self._forbidden_imports = {self._dir: []}
+        for forbidden_import in self.config.forbidden_imports:
+            forbidden_import = forbidden_import.split(':')
+            path = os.path.join(self._dir, forbidden_import[0])
+            path = os.path.abspath(path)
+            modules = forbidden_import[1:]
+            self._forbidden_imports[path] = modules
+
+        self._forbidden_imports_stack = []
+
+    def _get_forbidden_import_rule(self, node):
+        path = node.source_file
+        if path:
+            path = os.path.abspath(path)
+            while path.startswith(self._dir):
+                if path in self._forbidden_imports:
+                    return path
+                path = os.path.dirname(path)
+        return self._dir
+
+    def visit_module(self, node):
+        self._forbidden_imports_stack.append(
+            self._get_forbidden_import_rule(node))
+
+    def leave_module(self, node):
+        self._forbidden_imports_stack.pop()
+
+    def _check_forbidden_imports(self, node, names):
+        path = self._forbidden_imports_stack[-1]
+        relpath = os.path.relpath(path, self._dir)
+        modules = self._forbidden_imports[path]
+        for module in modules:
+            module_prefix = module + '.'
+            for name in names:
+                if name == module or name.startswith(module_prefix):
+                    self.add_message('ipa-forbidden-import',
+                                     args=(name, module, relpath), node=node)
+
+    @check_messages('ipa-forbidden-import')
+    def visit_import(self, node):
+        names = [n[0] for n in node.names]
+        self._check_forbidden_imports(node, names)
+
+    @check_messages('ipa-forbidden-import')
+    def visit_importfrom(self, node):
+        names = ['{}.{}'.format(node.modname, n[0]) for n in node.names]
+        self._check_forbidden_imports(node, names)

pylintrc

@@ -4,7 +4,9 @@ persistent=no
 
 # List of plugins (as comma separated values of python modules names) to load,
 # usually to register additional checkers.
-load-plugins=pylint_plugins
+# FIXME: has to be specified on the command line otherwise pylint fails with
+#        DuplicateSectionError for the IPA section
+#load-plugins=pylint_plugins
 
 # Use multiple processes to speed up Pylint.
 jobs=0
@@ -103,3 +105,14 @@ msg-template='{path}:{line}: [{msg_id}({symbol}), {obj}] {msg})'
 
 [VARIABLES]
 dummy-variables-rgx=_.+
+
+
+[IPA]
+forbidden-imports=
+    client/:ipaserver,
+    ipaclient/:ipaclient.install:ipalib.install:ipaplatform:ipaserver,
+    ipaclient/install/:ipaserver,
+    ipalib/:ipaclient.install:ipalib.install:ipaplatform:ipaserver,
+    ipalib/install/:ipaserver,
+    ipaplatform/:ipaclient:ipalib:ipaserver,
+    ipapython/:ipaclient:ipalib:ipaplatform:ipaserver