[ticket/9687] Add tests for ban manager and fix some inconsistencies

PHPBB3-9687

phpBB/phpbb/ban/manager.php

@@ -76,7 +76,7 @@ class manager
 	 *
 	 * @return bool
 	 */
-	public function ban(string $mode, array $items, \DateTimeInterface $start, \DateTimeInterface $end, string $reason, string $display_reason = '')
+	public function ban(string $mode, array $items, \DateTimeInterface $start, \DateTimeInterface $end, string $reason, string $display_reason = ''): bool
 	{
 		if ($start > $end && $end->getTimestamp() !== 0)
 		{
@@ -122,11 +122,7 @@ class manager
 			return false;
 		}
 
-		$result = $this->db->sql_multi_insert($this->bans_table, $insert_array);
-		if ($result === false)
-		{
-			throw new ban_insert_failed_exception(); // TODO
-		}
+		$this->db->sql_multi_insert($this->bans_table, $insert_array);
 
 		$ban_data = [
 			'items'				=> $ban_items,
@@ -138,7 +134,7 @@ class manager
 
 		if ($ban_mode->after_ban($ban_data))
 		{
-			// TODO
+			// @todo: Add logging
 		}
 
 		$this->cache->destroy(self::CACHE_KEY_INFO);
@@ -153,7 +149,7 @@ class manager
 	 * @param string	$mode		The ban type in which the ban IDs were created
 	 * @param array		$items		An array of ban IDs which should be removed
 	 */
-	public function unban($mode, array $items)
+	public function unban(string $mode, array $items)
 	{
 		/** @var type_interface $ban_mode */
 		$ban_mode = $this->find_type($mode);
@@ -245,7 +241,7 @@ class manager
 						}
 						else
 						{
-							$regex = str_replace('\*', '.*?', preg_quote($ban_row['item'], '#'));
+							$regex = '#^' . str_replace('\*', '.*?', preg_quote($ban_row['item'], '#')) . '$#i';
 							if (preg_match($regex, $user_data[$user_column]))
 							{
 								return $ban_row + ['mode' => $mode];
@@ -279,7 +275,7 @@ class manager
 		$sql = 'SELECT ban_id, ban_item, ban_start, ban_end, ban_reason, ban_reason_display
 			FROM ' . $this->bans_table . "
 			WHERE ban_mode = '" . $this->db->sql_escape($mode) . "'
-				AND (ban_end <= 0 OR ban_end >= " . (int) time() . ')';
+				AND (ban_end = 0 OR ban_end >= " . time() . ')';
 		$result = $this->db->sql_query($sql);
 		$rowset = $this->db->sql_fetchrowset($result);
 		$this->db->sql_freeresult($result);
@@ -294,7 +290,7 @@ class manager
 	 *
 	 * @return array
 	 */
-	public function get_banned_users()
+	public function get_banned_users(): array
 	{
 		$banned_users = $this->cache->get(self::CACHE_KEY_USERS);
 		if ($banned_users === false)
@@ -375,10 +371,10 @@ class manager
 	/**
 	 * Get ban end
 	 *
-	 * @param \DateTimeInterface $ban_start
-	 * @param int $length
-	 * @param string $end_date
-	 * @return \DateTimeInterface
+	 * @param \DateTimeInterface $ban_start Ban start time
+	 * @param int $length Ban length in minutes
+	 * @param string $end_date Ban end date as YYYY-MM-DD string
+	 * @return \DateTimeInterface Ban end as DateTimeInterface instance
 	 */
 	public function get_ban_end(\DateTimeInterface $ban_start, int $length, string $end_date): \DateTimeInterface
 	{
@@ -402,7 +398,7 @@ class manager
 				}
 				else
 				{
-					trigger_error('LENGTH_BAN_INVALID', E_USER_WARNING);
+					throw new invalid_length_exception();
 				}
 			}
 		}
@@ -434,11 +430,11 @@ class manager
 	 * Finds the ban type for the given mode string.
 	 * Returns false if none was found
 	 *
-	 * @param string	$mode	The mode string
+	 * @param string $mode	The mode string
 	 *
 	 * @return bool|type\type_interface
 	 */
-	protected function find_type($mode)
+	protected function find_type(string $mode)
 	{
 		/** @var type_interface $type */
 		foreach ($this->types as $type)
@@ -461,7 +457,7 @@ class manager
 	 *
 	 * @return array
 	 */
-	protected function get_info_cache()
+	protected function get_info_cache(): array
 	{
 		$ban_info = $this->cache->get(self::CACHE_KEY_INFO);
 		if ($ban_info === false)

phpBB/phpbb/ban/type/base.php

@@ -107,7 +107,7 @@ abstract class base implements type_interface
 	 *
 	 * @return bool
 	 */
-	protected function get_excluded()
+	protected function get_excluded(): bool
 	{
 		$user_column = $this->get_user_column();
 		if (empty($user_column))
@@ -119,7 +119,7 @@ abstract class base implements type_interface
 
 		if (!empty($this->user))
 		{
-			$this->excluded[(int) $this->user->data['user_id']]	= $this->user->data[$user_column];
+			$this->excluded[$this->user->id()] = $this->user->data[$user_column];
 		}
 
 		$sql = "SELECT user_id, {$user_column}
@@ -143,15 +143,15 @@ abstract class base implements type_interface
 	 *
 	 * @param array $ban_items
 	 *
-	 * @return array
+	 * @return array Logged out users
 	 */
-	protected function logout_affected_users(array $ban_items)
+	protected function logout_affected_users(array $ban_items): array
 	{
 		$user_column = $this->get_user_column();
 
 		if (empty($user_column))
 		{
-			// TODO throw ex (it's a developer exception)
+			return [];
 		}
 
 		if ($user_column !== 'user_id')

phpBB/phpbb/ban/type/email.php

@@ -39,10 +39,8 @@ class email extends base
 	 */
 	public function prepare_for_storage(array $items): array
 	{
-		if (!$this->get_excluded())
-		{
-			throw new runtime_exception(); // TODO
-		}
+		$this->get_excluded();
+
 		$regex = '#^.*?@.*|(([a-z0-9\-]+\.)+([a-z]{2,3}))$#i'; // TODO
 
 		$ban_items = [];

phpBB/phpbb/ban/type/ip.php

@@ -18,6 +18,7 @@ use Symfony\Component\HttpFoundation\IpUtils;
 
 class ip extends base
 {
+	private const USER_IP = 'user_ip';
 
 	/**
 	 * @inheritDoc
@@ -32,7 +33,7 @@ class ip extends base
 	 */
 	public function get_user_column(): ?string
 	{
-		return 'user_ip';
+		return null;
 	}
 
 	/**
@@ -40,7 +41,20 @@ class ip extends base
 	 */
 	public function check(array $ban_rows, array $user_data)
 	{
-		return parent::check($ban_rows, $user_data); // TODO: Change the autogenerated stub
+		if (!isset($user_data[self::USER_IP]))
+		{
+			return false;
+		}
+
+		foreach ($ban_rows as $ip_ban)
+		{
+			if (IpUtils::checkIp($user_data[self::USER_IP], $ip_ban['item']))
+			{
+				return $ip_ban;
+			}
+		}
+
+		return false;
 	}
 
 	/**
@@ -61,10 +75,12 @@ class ip extends base
 
 				$ban_items[] = $ip;
 			}
+			// @codeCoverageIgnoreStart
 			catch (\RuntimeException $exception)
 			{
 				// IPv6 not supported, therefore IPv6 address will not be added
 			}
+			// @codeCoverageIgnoreEnd
 		}
 
 		if (empty($ban_items))

phpBB/phpbb/ban/type/user.php

@@ -73,10 +73,8 @@ class user extends base
 	 */
 	public function prepare_for_storage(array $items): array
 	{
-		if (!$this->get_excluded())
-		{
-			throw new runtime_exception(); // TODO
-		}
+		// Fill excluded user list
+		$this->get_excluded();
 
 		$sql_usernames = [];
 		$sql_or_like = [];