Merge pull request #304 from yarikoptic/master

RF(ENH): JailsReader.getOptions -- avoid code duplication when asking for 1 jail or all

upon @kwirk blessing ;)

client/jailsreader.py

@@ -18,7 +18,7 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 
 # Author: Cyril Jaquier
-# 
+#
 
 __author__ = "Cyril Jaquier"
 __copyright__ = "Copyright (c) 2004 Cyril Jaquier"
@@ -32,7 +32,7 @@ from jailreader import JailReader
 logSys = logging.getLogger("fail2ban.client.config")
 
 class JailsReader(ConfigReader):
-	
+
 	def __init__(self, force_enable=False, **kwargs):
 		"""
 		Parameters
@@ -44,17 +44,25 @@ class JailsReader(ConfigReader):
 		ConfigReader.__init__(self, **kwargs)
 		self.__jails = list()
 		self.__force_enable = force_enable
-	
+
 	def read(self):
 		return ConfigReader.read(self, "jail")
-	
+
-	def getOptions(self, section = None):
+	def getOptions(self, section=None):
+		"""Reads configuration for jail(s) and adds enabled jails to __jails
+		"""
 		opts = []
 		self.__opts = ConfigReader.getOptions(self, "Definition", opts)
 
-		if section:
+		if section is None:
-			# Get the options of a specific jail.
+			sections = self.sections()
-			jail = JailReader(section, basedir=self.getBaseDir(), force_enable=self.__force_enable)
+		else:
+			sections = [ section ]
+
+		# Get the options of all jails.
+		for sec in sections:
+			jail = JailReader(sec, basedir=self.getBaseDir(),
+							  force_enable=self.__force_enable)
 			jail.read()
 			ret = jail.getOptions()
 			if ret:
@@ -62,23 +70,10 @@ class JailsReader(ConfigReader):
 					# We only add enabled jails
 					self.__jails.append(jail)
 			else:
-				logSys.error("Errors in jail '%s'. Skipping..." % section)
+				logSys.error("Errors in jail %r. Skipping..." % sec)
 				return False
-		else:
-			# Get the options of all jails.
-			for sec in self.sections():
-				jail = JailReader(sec, basedir=self.getBaseDir(), force_enable=self.__force_enable)
-				jail.read()
-				ret = jail.getOptions()
-				if ret:
-					if jail.isEnabled():
-						# We only add enabled jails
-						self.__jails.append(jail)
-				else:
-					logSys.error("Errors in jail '" + sec + "'. Skipping...")
-					return False
 		return True
-	
+
 	def convert(self, allow_no_files=False):
 		"""Convert read before __opts and jails to the commands stream
 
@@ -99,6 +94,6 @@ class JailsReader(ConfigReader):
 		# Start jails
 		for jail in self.__jails:
 			stream.append(["start", jail.getName()])
-		
+
 		return stream
-		
+

server/ticket.py

@@ -47,7 +47,7 @@ class Ticket:
 
 	def __str__(self):
 		return "%s: ip=%s time=%s #attempts=%d" % \
-			   (self.__class__, self.__ip, self.__time, self.__attempt)
+			   (self.__class__.__name__.split('.')[-1], self.__ip, self.__time, self.__attempt)
 	
 
 	def setIP(self, value):
@@ -59,12 +59,6 @@ class Ticket:
 	def getIP(self):
 		return self.__ip
 	
-	def setFile(self, value):
-		self.__file = value
-	
-	def getFile(self):
-		return self.__file
-	
 	def setTime(self, value):
 		self.__time = value
 	

testcases/clientreadertestcase.py

@@ -126,6 +126,13 @@ class JailsReaderTest(unittest.TestCase):
 		# commands to communicate to the server
 		self.assertEqual(comm_commands, [])
 
+		# We should not "read" some bogus jail
+		old_comm_commands = comm_commands[:]   # make a copy
+		self.assertFalse(jails.getOptions("BOGUS"))
+		# and there should be no side-effects
+		self.assertEqual(jails.convert(), old_comm_commands)
+
+
 	def testReadStockJailConfForceEnabled(self):
 		# more of a smoke test to make sure that no obvious surprises
 		# on users' systems when enabling shipped jails

testcases/failmanagertestcase.py

@@ -78,6 +78,20 @@ class AddFailure(unittest.TestCase):
 		ticket = self.__failManager.toBan()
 		self.assertEqual(ticket.getIP(), "193.168.0.128")
 		self.assertTrue(isinstance(ticket.getIP(), str))
+
+		# finish with rudimentary tests of the ticket
+		# verify consistent str
+		ticket_str = str(ticket)
+		self.assertEqual(
+			ticket_str,
+			'FailTicket: ip=193.168.0.128 time=1167605999.0 #attempts=5')
+		# and some get/set-ers otherwise not tested
+		ticket.setTime(1000002000.0)
+		self.assertEqual(ticket.getTime(), 1000002000.0)
+		# and str() adjusted correspondingly
+		self.assertEqual(
+			str(ticket),
+			'FailTicket: ip=193.168.0.128 time=1000002000.0 #attempts=5')
 	
 	def testbanNOK(self):
 		self.__failManager.setMaxRetry(10)