code review, increase coverage

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

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))

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'"],
 			 ]))

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