URL: https://github.com/freeipa/freeipa/pull/1429 Author: tiran Title: #1429: Fix warnings and errors found by LGTM static code analyzer Action: opened
PR body: """ https://pagure.io/freeipa/issue/7344 """ To pull the PR as Git branch: git remote add ghfreeipa https://github.com/freeipa/freeipa git fetch ghfreeipa pull/1429/head:pr1429 git checkout pr1429
From afb8cf6808f1e56cca1b2298a113041e774d4d2a Mon Sep 17 00:00:00 2001 From: Christian Heimes <chei...@redhat.com> Date: Wed, 3 Jan 2018 09:59:37 +0100 Subject: [PATCH 1/7] Add LGTM config LGTM is a static code analyzer for Python, JS, and more, https://lgtm.com/projects/g/freeipa/freeipa. Ignore unmatchable dollar in regular expression. https://pagure.io/freeipa/issue/7344 Signed-off-by: Christian Heimes <chei...@redhat.com> --- .lgtm.yml | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .lgtm.yml diff --git a/.lgtm.yml b/.lgtm.yml new file mode 100644 index 0000000000..d344d6790a --- /dev/null +++ b/.lgtm.yml @@ -0,0 +1,7 @@ +# https://lgtm.com/projects/g/freeipa/freeipa + +queries: + # False positive unmatchable dollar in regular expression, e.g. + # installutils.update_file(certmap_filename, '$ISSUER_DN', str(ca_subject)) + - exclude: py/regex/unmatchable-dollar + From c95dd7818d2cfa936922c0e8d570e5093ac201b9 Mon Sep 17 00:00:00 2001 From: Christian Heimes <chei...@redhat.com> Date: Wed, 3 Jan 2018 10:08:50 +0100 Subject: [PATCH 2/7] LGTM: Use of exit() or quit() Replace exit() with sys.exit(). exit() or quit() may fail if the interpreter is run with the -S option. https://pagure.io/freeipa/issue/7344 Signed-off-by: Christian Heimes <chei...@redhat.com> --- ipalib/cli.py | 6 +++--- ipaserver/install/cainstance.py | 2 +- ipatests/ipa-test-config | 26 ++++++++++++++++---------- ipatests/ipa-test-task | 2 +- makeaci | 2 +- 5 files changed, 22 insertions(+), 16 deletions(-) diff --git a/ipalib/cli.py b/ipalib/cli.py index 625096a383..2da641046f 100644 --- a/ipalib/cli.py +++ b/ipalib/cli.py @@ -956,13 +956,13 @@ def run(self, filename=None, **options): try: script = open(filename) except IOError as e: - exit("%s: %s" % (e.filename, e.strerror)) + sys.exit("%s: %s" % (e.filename, e.strerror)) try: with script: exec(script, globals(), local) except Exception: traceback.print_exc() - exit(1) + sys.exit(1) else: code.interact( '(Custom IPA interactive Python console)', @@ -1121,7 +1121,7 @@ def get_command(self, argv): self.Command.help(outfile=sys.stderr) print(file=sys.stderr) print('Error: Command not specified', file=sys.stderr) - exit(2) + sys.exit(2) (key, argv) = (argv[0], argv[1:]) name = from_cli(key) if name not in self.Command and len(argv) == 0: diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py index 68cc77b9ff..20bca078c0 100644 --- a/ipaserver/install/cainstance.py +++ b/ipaserver/install/cainstance.py @@ -1370,7 +1370,7 @@ def replica_ca_install_check(config, promote): 'Please run copy-schema-to-ca.py on all CA masters.\n' 'If you are certain that this is a false positive, use ' '--skip-schema-check.') - exit('IPA schema missing on master CA directory server') + sys.exit('IPA schema missing on master CA directory server') def __update_entry_from_cert(make_filter, make_entry, cert): diff --git a/ipatests/ipa-test-config b/ipatests/ipa-test-config index 3d1ff757b8..b03a2bb7dc 100755 --- a/ipatests/ipa-test-config +++ b/ipatests/ipa-test-config @@ -105,7 +105,7 @@ def get_object(conf, args): try: return conf.host_by_name(args.host) except LookupError: - exit('Host %s not found in config. Try --global' % args.host) + sys.exit('Host %s not found in config. Try --global' % args.host) else: if args.domain: try: @@ -113,18 +113,18 @@ def get_object(conf, args): except ValueError: domains = [d for d in conf.domains if d.name == args.domain] if not domains: - exit('Domain %s not found' % args.domain) + sys.exit('Domain %s not found' % args.domain) domain = domains[0] else: try: domain = conf.domains[num] except LookupError: - exit('Domain %s not found.' % args.domain) + sys.exit('Domain %s not found.' % args.domain) else: try: domain = conf.domains[0] except IndexError: - exit('No domains are configured.') + sys.exit('No domains are configured.') if args.master: return domain.master @@ -133,23 +133,29 @@ def get_object(conf, args): try: return domain.replicas[args.replica] except LookupError: - exit('Domain %s not found in domain %s' % (args.replica, - domain.name)) + sys.exit( + 'Domain %s not found in domain %s' + % (args.replica, domain.name) + ) elif args.client: num = int(args.client) - 1 try: return domain.replicas[args.client] except LookupError: - exit('Client %s not found in domain %s' % (args.client, - domain.name)) + sys.exit( + 'Client %s not found in domain %s' + % (args.client, domain.name) + ) elif args.role: try: return domain.get_host_by_role(args.role) except LookupError: - exit('No host with role %s not found in domain %s' - % (args.role, domain.name)) + sys.exit( + 'No host with role %s not found in domain %s' + % (args.role, domain.name) + ) else: return domain diff --git a/ipatests/ipa-test-task b/ipatests/ipa-test-task index 3342233894..987149609f 100755 --- a/ipatests/ipa-test-task +++ b/ipatests/ipa-test-task @@ -453,4 +453,4 @@ class TaskRunner(object): tasks.add_a_record(master, host) if __name__ == '__main__': - exit(TaskRunner().main(sys.argv[1:])) + sys.exit(TaskRunner().main(sys.argv[1:])) diff --git a/makeaci b/makeaci index ff46f1ec09..4d2c75bcbc 100755 --- a/makeaci +++ b/makeaci @@ -121,7 +121,7 @@ def main(options): print(file=sys.stderr) print('Managed permission ACI validation failed.', file=sys.stderr) print('Re-check permission changes and run `makeaci`.', file=sys.stderr) - exit('%s validation failed' % options.filename) + sys.exit('%s validation failed' % options.filename) else: with open(options.filename, 'w') as file: file.writelines(output_lines) From 1bd54ae27d0b3f39e8db73351b7b0ca01c2538c7 Mon Sep 17 00:00:00 2001 From: Christian Heimes <chei...@redhat.com> Date: Wed, 3 Jan 2018 10:14:03 +0100 Subject: [PATCH 3/7] LGTM: Name unused variable in loop For loop variable '_nothing' is not used in the loop body. The name 'unused' is used to indicate that a variable is unused. https://pagure.io/freeipa/issue/7344 Signed-off-by: Christian Heimes <chei...@redhat.com> --- ipapython/install/common.py | 4 ++-- pylintrc | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ipapython/install/common.py b/ipapython/install/common.py index 3ec14141ae..05dfadfcb1 100644 --- a/ipapython/install/common.py +++ b/ipapython/install/common.py @@ -63,7 +63,7 @@ def parent(self): raise AttributeError('parent') def _install(self): - for _nothing in self._installer(self.parent): + for unused in self._installer(self.parent): yield from_(super(Step, self)._install()) @staticmethod @@ -71,7 +71,7 @@ def _installer(obj): yield def _uninstall(self): - for _nothing in self._uninstaller(self.parent): + for unused in self._uninstaller(self.parent): yield from_(super(Step, self)._uninstall()) @staticmethod diff --git a/pylintrc b/pylintrc index 8cd7c870bf..9fad4d9472 100644 --- a/pylintrc +++ b/pylintrc @@ -110,7 +110,7 @@ msg-template='{path}:{line}: [{msg_id}({symbol}), {obj}] {msg})' [VARIABLES] -dummy-variables-rgx=_.+ +dummy-variables-rgx=(_.+|unused) [IPA] From f66cfbaf6963a965ecaed3bf30defad92fd08947 Mon Sep 17 00:00:00 2001 From: Christian Heimes <chei...@redhat.com> Date: Wed, 3 Jan 2018 10:17:40 +0100 Subject: [PATCH 4/7] LGTM: Membership test with a non-container Silence false positive. Value is always a dict here. https://pagure.io/freeipa/issue/7344 Signed-off-by: Christian Heimes <chei...@redhat.com> --- ipalib/rpc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipalib/rpc.py b/ipalib/rpc.py index d868518aff..8ed1f513b2 100644 --- a/ipalib/rpc.py +++ b/ipalib/rpc.py @@ -226,7 +226,7 @@ def xml_unwrap(value, encoding='UTF-8'): if type(value) in (list, tuple): return tuple(xml_unwrap(v, encoding) for v in value) if type(value) is dict: - if '__dns_name__' in value: + if '__dns_name__' in value: # lgtm [py/member-test-non-container] return DNSName(value['__dns_name__']) else: return dict( From 687f8ad2e1533e96efca3aee38c40cc16b029862 Mon Sep 17 00:00:00 2001 From: Christian Heimes <chei...@redhat.com> Date: Wed, 3 Jan 2018 10:21:46 +0100 Subject: [PATCH 5/7] LGTM: Fix exception in permission_del Instantiating an exception, but not raising it, has no effect. https://pagure.io/freeipa/issue/7344 Signed-off-by: Christian Heimes <chei...@redhat.com> --- ipaserver/plugins/permission.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py index a91426afd2..49ac7cfcad 100644 --- a/ipaserver/plugins/permission.py +++ b/ipaserver/plugins/permission.py @@ -1072,7 +1072,7 @@ def pre_callback(self, ldap, dn, *keys, **options): try: self.obj.remove_aci(entry) except errors.NotFound: - errors.NotFound( + raise errors.NotFound( reason=_('ACI of permission %s was not found') % keys[0]) return dn From 8a75a038f04a98b3c4ae027571384eecfdad75c7 Mon Sep 17 00:00:00 2001 From: Christian Heimes <chei...@redhat.com> Date: Wed, 3 Jan 2018 10:22:56 +0100 Subject: [PATCH 6/7] LGTM: Remove redundant assignment https://pagure.io/freeipa/issue/7344 Signed-off-by: Christian Heimes <chei...@redhat.com> --- ipapython/certdb.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/ipapython/certdb.py b/ipapython/certdb.py index 4b93e77d26..e7fe17beb1 100644 --- a/ipapython/certdb.py +++ b/ipapython/certdb.py @@ -225,8 +225,6 @@ def __init__(self, nssdir=None, dbtype='auto'): self._is_temporary = True if dbtype == 'auto': dbtype = constants.NSS_DEFAULT_DBTYPE - else: - dbtype = dbtype else: self.secdir = nssdir self._is_temporary = False From b79a5fa4f27fb580ebb5fd17febee53b3c939aa5 Mon Sep 17 00:00:00 2001 From: Christian Heimes <chei...@redhat.com> Date: Wed, 3 Jan 2018 11:09:41 +0100 Subject: [PATCH 7/7] LGTM: Fix multiple use before assignment - Move assignment before try/finally block - Add return to indicate control flow change - Add default value https://pagure.io/freeipa/issue/7344 Signed-off-by: Christian Heimes <chei...@redhat.com> --- install/tools/ipa-custodia-check | 26 ++++++++++++++++++-------- ipaclient/install/client.py | 16 +++++++++------- ipaserver/install/ipa_cacert_manage.py | 6 ++---- ipaserver/install/ipa_replica_prepare.py | 2 ++ ipaserver/install/server/install.py | 2 ++ ipaserver/install/server/replicainstall.py | 3 +++ ipaserver/plugins/trust.py | 16 +++++++++------- ipaserver/secrets/store.py | 4 ++-- ipatests/pytest_plugins/integration/tasks.py | 3 ++- 9 files changed, 49 insertions(+), 29 deletions(-) diff --git a/install/tools/ipa-custodia-check b/install/tools/ipa-custodia-check index 4e46b7dc2c..3e965a328b 100755 --- a/install/tools/ipa-custodia-check +++ b/install/tools/ipa-custodia-check @@ -191,14 +191,16 @@ class IPACustodiaTester(object): pkey = JWK(**dictkeys[usage_id]) local_pubkey = json_decode(pkey.export_public()) except Exception: - self.error("Failed to load and parse local JWK.", fatal=True) + return self.error( + "Failed to load and parse local JWK.", fatal=True + ) else: self.info("Loaded key for usage '{}' from '{}'.".format( usage, IPA_CUSTODIA_KEYFILE )) if pkey.key_id != self.host_spn: - self.error( + return self.error( "KID '{}' != host service principal name '{}' " "(usage: {})".format(pkey.key_id, self.host_spn, usage), fatal=True @@ -215,8 +217,11 @@ class IPACustodiaTester(object): try: host_pubkey = json_decode(find_key(self.host_spn, usage_id)) except Exception: - self.error("Fetching host keys {} (usage: {}) failed.".format( - self.host_spn, usage), fatal=True) + return self.error( + "Fetching host keys {} (usage: {}) failed.".format( + self.host_spn, usage), + fatal=True + ) else: self.info("Checked host LDAP keys '{}' for usage {}.".format( self.host_spn, usage @@ -225,8 +230,10 @@ class IPACustodiaTester(object): if host_pubkey != local_pubkey: self.debug("LDAP: '{}'".format(host_pubkey)) self.debug("Local: '{}'".format(local_pubkey)) - self.error( - "Host key in LDAP does not match local key.", fatal=True) + return self.error( + "Host key in LDAP does not match local key.", + fatal=True + ) else: self.info( "Local key for usage '{}' matches key in LDAP.".format(usage) @@ -235,8 +242,11 @@ class IPACustodiaTester(object): try: server_pubkey = json_decode(find_key(self.server_spn, usage_id)) except Exception: - self.error("Fetching server keys {} (usage: {}) failed.".format( - self.server_spn, usage), fatal=True) + return self.error( + "Fetching server keys {} (usage: {}) failed.".format( + self.server_spn, usage), + fatal=True + ) else: self.info("Checked server LDAP keys '{}' for usage {}.".format( self.server_spn, usage diff --git a/ipaclient/install/client.py b/ipaclient/install/client.py index 6fa42e3665..c565871b9c 100644 --- a/ipaclient/install/client.py +++ b/ipaclient/install/client.py @@ -1275,9 +1275,11 @@ def update_dns(server, hostname, options): ips = get_local_ipaddresses() except CalledProcessError as e: logger.error("Cannot update DNS records. %s", e) - logger.debug("Unable to get local IP addresses.") + ips = None if options.all_ip_addresses: + if ips is None: + raise RuntimeError("Unable to get local IP addresses.") update_ips = ips elif options.ip_addresses: update_ips = [] @@ -1777,8 +1779,8 @@ def http_url(): override) else: # Auth with user credentials + url = ldap_url() try: - url = ldap_url() ca_certs = get_ca_certs_from_ldap(server, basedn, realm) validate_new_ca_certs(existing_ca_certs, ca_certs, interactive) except errors.FileError as e: @@ -1821,7 +1823,7 @@ def http_url(): if ca_certs is None and existing_ca_certs is None: raise errors.InternalError(u"expected CA cert file '%s' to " - u"exist, but it's absent" % (ca_file)) + u"exist, but it's absent" % ca_file) if ca_certs is not None: try: @@ -2427,9 +2429,10 @@ def _install(options): if not options.on_master: nolog = tuple() # First test out the kerberos configuration + fd, krb_name = tempfile.mkstemp() + os.close(fd) + ccache_dir = tempfile.mkdtemp(prefix='krbcc') try: - (krb_fd, krb_name) = tempfile.mkstemp() - os.close(krb_fd) configure_krb5_conf( cli_realm=cli_realm, cli_domain=cli_domain, @@ -2442,7 +2445,6 @@ def _install(options): configure_sssd=options.sssd, force=options.force) env['KRB5_CONFIG'] = krb_name - ccache_dir = tempfile.mkdtemp(prefix='krbcc') ccache_name = os.path.join(ccache_dir, 'ccache') join_args = [paths.SBIN_IPA_JOIN, "-s", cli_server[0], @@ -2799,7 +2801,7 @@ def _install(options): nscd = services.knownservices.nscd if nscd.is_installed(): save_state(nscd, statestore) - + nscd_service_action = None try: if options.sssd: nscd_service_action = 'stop' diff --git a/ipaserver/install/ipa_cacert_manage.py b/ipaserver/install/ipa_cacert_manage.py index 0ac0c5c0a1..c87e8048af 100644 --- a/ipaserver/install/ipa_cacert_manage.py +++ b/ipaserver/install/ipa_cacert_manage.py @@ -123,14 +123,12 @@ def run(self): try: if command == 'renew': - rc = self.renew() + return self.renew() elif command == 'install': - rc = self.install() + return self.install() finally: api.Backend.ldap2.disconnect() - return rc - def ldap_connect(self): password = self.options.password if not password: diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py index 0be0302ddc..6872cefec1 100644 --- a/ipaserver/install/ipa_replica_prepare.py +++ b/ipaserver/install/ipa_replica_prepare.py @@ -197,6 +197,8 @@ def load_pkcs12(self, cert_files, key_password, key_nickname): def ask_for_options(self): options = self.options super(ReplicaPrepare, self).ask_for_options() + http_ca_cert = None + dirsrv_ca_cert = None # get the directory manager password self.dirman_password = options.password diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py index cca25d329a..192b30db49 100644 --- a/ipaserver/install/server/install.py +++ b/ipaserver/install/server/install.py @@ -305,6 +305,8 @@ def install_check(installer): external_cert_file = installer._external_cert_file external_ca_file = installer._external_ca_file http_ca_cert = installer._ca_cert + dirsrv_ca_cert = None + pkinit_ca_cert = None tasks.check_ipv6_stack_enabled() tasks.check_selinux_status() diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py index fb1c2155bd..362e3f1b7c 100644 --- a/ipaserver/install/server/replicainstall.py +++ b/ipaserver/install/server/replicainstall.py @@ -1021,10 +1021,13 @@ def promote_check(installer): http_pkcs12_file = None http_pkcs12_info = None + http_ca_cert = None dirsrv_pkcs12_file = None dirsrv_pkcs12_info = None + dirsrv_ca_cert = None pkinit_pkcs12_file = None pkinit_pkcs12_info = None + pkinit_ca_cert = None if options.http_cert_files: if options.http_pin is None: diff --git a/ipaserver/plugins/trust.py b/ipaserver/plugins/trust.py index 835c6405b4..db194003cb 100644 --- a/ipaserver/plugins/trust.py +++ b/ipaserver/plugins/trust.py @@ -590,7 +590,7 @@ def get_dn(self, *keys, **kwargs): ldap.SCOPE_SUBTREE, trustfilter, [''] ) except errors.NotFound: - self.handle_not_found(keys[-1]) + return self.handle_not_found(keys[-1]) if len(result) > 1: raise errors.OnlyOneValueAllowed(attr='trust domain') @@ -1645,7 +1645,7 @@ def execute(self, *keys, **options): name='domain', error=_("cannot delete root domain of the trust, " "use trust-del to delete the trust itself")) - self.obj.handle_not_found(keys[0], domain) + return self.obj.handle_not_found(keys[0], domain) try: self.api.Command.trustdomain_enable(keys[0], domain) @@ -1808,7 +1808,8 @@ def execute(self, *keys, **options): trust_dn = self.obj.get_dn(keys[0], trust_type=u'ad') trust_entry = ldap.get_entry(trust_dn) except errors.NotFound: - self.api.Object[self.obj.parent_object].handle_not_found(keys[0]) + return self.api.Object[self.obj.parent_object].handle_not_found( + keys[0]) dn = self.obj.get_dn(keys[0], keys[1], trust_type=u'ad') try: @@ -1820,7 +1821,7 @@ def execute(self, *keys, **options): else: raise errors.AlreadyActive() except errors.NotFound: - self.obj.handle_not_found(*keys) + return self.obj.handle_not_found(*keys) return dict( result=True, @@ -1849,19 +1850,20 @@ def execute(self, *keys, **options): trust_dn = self.obj.get_dn(keys[0], trust_type=u'ad') trust_entry = ldap.get_entry(trust_dn) except errors.NotFound: - self.api.Object[self.obj.parent_object].handle_not_found(keys[0]) + return self.api.Object[self.obj.parent_object].handle_not_found( + keys[0]) dn = self.obj.get_dn(keys[0], keys[1], trust_type=u'ad') try: entry = ldap.get_entry(dn) sid = entry.single_value.get('ipanttrusteddomainsid', None) - if not (sid in trust_entry['ipantsidblacklistincoming']): + if sid not in trust_entry['ipantsidblacklistincoming']: trust_entry['ipantsidblacklistincoming'].append(sid) ldap.update_entry(trust_entry) else: raise errors.AlreadyInactive() except errors.NotFound: - self.obj.handle_not_found(*keys) + return self.obj.handle_not_found(*keys) return dict( result=True, diff --git a/ipaserver/secrets/store.py b/ipaserver/secrets/store.py index f23a0c3966..8da6817228 100644 --- a/ipaserver/secrets/store.py +++ b/ipaserver/secrets/store.py @@ -204,9 +204,9 @@ def import_key(self, value): v = json_decode(value) data = b64decode(v['pkcs12 data']) password = v['export password'] + fd, tmpdata = tempfile.mkstemp(dir=paths.TMP) + os.close(fd) try: - _fd, tmpdata = tempfile.mkstemp(dir=paths.TMP) - os.close(_fd) with open(tmpdata, 'wb') as f: f.write(data) diff --git a/ipatests/pytest_plugins/integration/tasks.py b/ipatests/pytest_plugins/integration/tasks.py index b407145ace..a6fdca39e6 100644 --- a/ipatests/pytest_plugins/integration/tasks.py +++ b/ipatests/pytest_plugins/integration/tasks.py @@ -600,8 +600,9 @@ def modify_sssd_conf(host, domain, mod_dict, provider='ipa', :param provider_subtype: backend subtype (e.g. id or sudo), will be added to the domain config if not present """ + fd, temp_config_file = tempfile.mkstemp() + os.close(fd) try: - temp_config_file = tempfile.mkstemp()[1] current_config = host.transport.get_file_contents(paths.SSSD_CONF) with open(temp_config_file, 'wb') as f:
_______________________________________________ FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org
