From 3e9852d4d2a7c73936a4cbaafd17bf13a0e412a6 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 14:56:54 +0100 Subject: [PATCH] code review, increase coverage --- fail2ban/client/jailreader.py | 24 +++++++++++++++--------- fail2ban/client/jailsreader.py | 10 ++-------- fail2ban/tests/clientreadertestcase.py | 18 ++++++++++++++---- fail2ban/tests/config/jail.conf | 8 ++++++++ 4 files changed, 39 insertions(+), 21 deletions(-) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index 74072481..b63df5f1 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -122,8 +122,8 @@ class JailReader(ConfigReader): # Read first options only needed for merge defaults ('known/...' from filter): self.__opts = ConfigReader.getOptions(self, self.__name, opts1st, shouldExist=True) - if not self.__opts: - raise ValueError("Init jail options failed") + if not self.__opts: # pragma: no cover + raise JailDefError("Init jail options failed") if self.isEnabled(): # Read filter @@ -131,7 +131,7 @@ class JailReader(ConfigReader): if flt: filterName, filterOpt = JailReader.extractOptions(flt) if not filterName: - raise ValueError("Invalid filter declaration %r" % flt) + raise JailDefError("Invalid filter definition %r" % flt) self.__filter = FilterReader( filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) ret = self.__filter.read() @@ -139,15 +139,15 @@ class JailReader(ConfigReader): self.__filter.getOptions(self.__opts) ConfigReader.merge_section(self, self.__name, self.__filter.getCombined(), 'known/') if not ret: - raise ValueError("Unable to read the filter %r" % filterName) + raise JailDefError("Unable to read the filter %r" % filterName) else: self.__filter = None logSys.warning("No filter set for jail %s" % self.__name) # Read second all options (so variables like %(known/param) can be interpolated): self.__opts = ConfigReader.getOptions(self, self.__name, opts) - if not self.__opts: - raise ValueError("Read jail options failed") + if not self.__opts: # pragma: no cover + raise JailDefError("Read jail options failed") # cumulate filter options again (ignore given in jail): if self.__filter: @@ -160,7 +160,7 @@ class JailReader(ConfigReader): continue actName, actOpt = JailReader.extractOptions(act) if not actName: - raise ValueError("Invalid action declaration %r" % act) + raise JailDefError("Invalid action definition %r" % act) if actName.endswith(".py"): self.__actions.append([ "set", @@ -180,14 +180,16 @@ class JailReader(ConfigReader): action.getOptions(self.__opts) self.__actions.append(action) else: - raise AttributeError("Unable to read action") + raise JailDefError("Unable to read action %r" % actName) + except JailDefError: + raise except Exception as e: logSys.debug("Caught exception: %s", e, exc_info=True) raise ValueError("Error in action definition %r: %r" % (act, e)) if not len(self.__actions): logSys.warning("No actions were defined for %s" % self.__name) - except ValueError as e: + except JailDefError as e: e = str(e) logSys.error(e) if not self.__opts: @@ -280,3 +282,7 @@ class JailReader(ConfigReader): val for val in optmatch.group(2,3,4) if val is not None][0] option_opts[opt.strip()] = value.strip() return option_name, option_opts + + +class JailDefError(Exception): + pass diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index ec7baca7..7d81d0a0 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -84,11 +84,8 @@ class JailsReader(ConfigReader): else: logSys.error("Errors in jail %r. Skipping..." % sec) self.__jails.append(jail) - if parse_status is None: - parse_status = False - if parse_status is None: - parse_status = True - return parse_status + if parse_status is None: parse_status = False + return True if parse_status != False else False def convert(self, allow_no_files=False): """Convert read before __opts and jails to the commands stream @@ -101,9 +98,6 @@ class JailsReader(ConfigReader): """ stream = list() - for opt in self.__opts: - if opt == "": - stream.append([]) # Convert jails for jail in self.__jails: stream.extend(jail.convert(allow_no_files=allow_no_files)) diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index 20d6ced1..33c217cd 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -193,9 +193,15 @@ class JailReaderTest(LogCaptureTestCase): self.assertTrue(jail.read()) self.assertFalse(jail.getOptions()) self.assertTrue(jail.isEnabled()) - self.assertLogged("Error in action definition 'joho[foo'") - self.assertLogged( - "Caught exception: Invalid action declaration 'joho[foo'") + self.assertLogged("Invalid action definition 'joho[foo'") + + def testJailFilterBrokenDef(self): + jail = JailReader('brokenfilterdef', basedir=IMPERFECT_CONFIG, + share_config=IMPERFECT_CONFIG_SHARE_CFG) + self.assertTrue(jail.read()) + self.assertFalse(jail.getOptions()) + self.assertTrue(jail.isEnabled()) + self.assertLogged("Invalid filter definition 'flt[test'") if STOCK: def testStockSSHJail(self): @@ -523,7 +529,11 @@ class JailsReaderTest(LogCaptureTestCase): ['start', 'brokenaction'], ['start', 'parse_to_end_of_jail.conf'], ['config-error', - 'Jail \'brokenactiondef\' skipped, because of wrong configuration: Error in action definition \'joho[foo\': ValueError("Invalid action declaration \'joho[foo\'",)'], + "Jail 'brokenactiondef' skipped, because of wrong configuration: Invalid action definition 'joho[foo'"], + ['config-error', + "Jail 'brokenfilterdef' skipped, because of wrong configuration: Invalid filter definition 'flt[test'"], + ['config-error', + "Jail 'missingaction' skipped, because of wrong configuration: Unable to read action 'noactionfileforthisaction'"], ['config-error', "Jail 'missingbitsjail' skipped, because of wrong configuration: Unable to read the filter 'catchallthebadies'"], ])) diff --git a/fail2ban/tests/config/jail.conf b/fail2ban/tests/config/jail.conf index bf1dea45..659e3fd3 100644 --- a/fail2ban/tests/config/jail.conf +++ b/fail2ban/tests/config/jail.conf @@ -27,10 +27,18 @@ logpath = /weapons/of/mass/destruction enabled = true action = joho[foo +[brokenfilterdef] +enabled = true +filter = flt[test + [brokenaction] enabled = true action = brokenaction +[missingaction] +enabled = true +action = noactionfileforthisaction + [missingbitsjail] enabled = true filter = catchallthebadies