[ticket/9687] Add support for logging to banning system

PHPBB3-9687

phpBB/config/default/container/services_ban.yml

@@ -7,6 +7,7 @@ services:
             - '@cache.driver'
             - '@dbal.conn'
             - '@language'
+            - '@log'
             - '@user'
             - '%tables.bans%'
             - '%tables.users%'

phpBB/phpbb/ban/manager.php

@@ -20,6 +20,7 @@ use phpbb\cache\driver\driver_interface as cache_driver;
 use phpbb\db\driver\driver_interface;
 use phpbb\di\service_collection;
 use phpbb\language\language;
+use phpbb\log\log_interface;
 use phpbb\user;
 
 class manager
@@ -43,6 +44,9 @@ class manager
 	/** @var language */
 	protected $language;
 
+	/** @var log_interface */
+	protected $log;
+
 	/** @var user */
 	protected $user;
 
@@ -56,18 +60,21 @@ class manager
 	 * @param service_collection	$types					A service collection containing all ban types
 	 * @param cache_driver			$cache					A cache object
 	 * @param driver_interface		$db						A phpBB DBAL object
+	 * @param language				$language				Language object
+	 * @param log_interface			$log					Log object
 	 * @param user					$user					User object
 	 * @param string				$bans_table				The bans table
 	 * @param string				$users_table			The users table
 	 */
-	public function __construct(service_collection $types, cache_driver $cache, driver_interface $db,
+	public function __construct(service_collection $types, cache_driver $cache, driver_interface $db, language $language,
-								language $language, user $user, string $bans_table, string $users_table = '')
+								log_interface $log, user $user, string $bans_table, string $users_table = '')
 	{
 		$this->bans_table = $bans_table;
 		$this->cache = $cache;
 		$this->db = $db;
 		$this->types = $types;
 		$this->language = $language;
+		$this->log = $log;
 		$this->user = $user;
 		$this->users_table = $users_table;
 	}
@@ -141,9 +148,28 @@ class manager
 			'display_reason'	=> $display_reason,
 		];
 
-		if ($ban_mode->after_ban($ban_data))
+		// Add to admin log, moderator log and user notes
+		$ban_list_log = implode(', ', $items);
+
+		$log_operation = 'LOG_BAN_' . strtoupper($mode);
+		$this->log->add('admin', $this->user->data['user_id'], $this->user->ip, $log_operation, false, [$reason, $ban_list_log]);
+		$this->log->add('mod', $this->user->data['user_id'], $this->user->ip, $log_operation, false, [
+			'forum_id' => 0,
+			'topic_id' => 0,
+			$reason,
+			$ban_list_log
+		]);
+
+		if ($banlist_ary = $ban_mode->after_ban($ban_data))
 		{
-			// @todo: Add logging
+			foreach ($banlist_ary as $user_id)
+			{
+				$this->log->add('user', $this->user->data['user_id'], $this->user->ip, $log_operation, false, [
+					'reportee_id' => $user_id,
+					$reason,
+					$ban_list_log
+				]);
+			}
 		}
 
 		$this->cache->destroy(self::CACHE_KEY_INFO);
@@ -192,7 +218,25 @@ class manager
 				'items' => $unbanned_items,
 			];
 			$unbanned_users = $ban_mode->after_unban($unban_data);
-			// @todo: add logging for unbanned users
+
+			// Add to moderator log, admin log and user notes
+			$log_operation = 'LOG_UNBAN_' . strtoupper($mode);
+			$this->log->add('admin', $this->user->data['user_id'], $this->user->ip, $log_operation, false, [$unbanned_users]);
+			$this->log->add('mod', $this->user->data['user_id'], $this->user->ip, $log_operation, false, [
+				'forum_id' => 0,
+				'topic_id' => 0,
+				$unbanned_users
+			]);
+			if (count($unbanned_users))
+			{
+				foreach ($unbanned_users as $user_id)
+				{
+					$this->log->add('user', $this->user->data['user_id'], $this->user->ip, $log_operation, false, array(
+						'reportee_id' => $user_id,
+						$unbanned_users
+					));
+				}
+			}
 		}
 
 		$this->cache->destroy(self::CACHE_KEY_INFO);

phpBB/phpbb/ban/type/base.php

@@ -68,18 +68,17 @@ abstract class base implements type_interface
 	/**
 	 * {@inheritDoc}
 	 */
-	public function after_ban(array $data)
+	public function after_ban(array $data): array
 	{
-		$this->logout_affected_users($data['items']);
+		return $this->logout_affected_users($data['items']);
-		return $data['items'];
 	}
 
 	/**
 	 * {@inheritDoc}
 	 */
-	public function after_unban(array $data)
+	public function after_unban(array $data): array
 	{
-		return $data['items'];
+		return [];
 	}
 
 	/**

phpBB/phpbb/ban/type/type_interface.php

@@ -54,21 +54,21 @@ interface type_interface
 	 *                    the bans, like the reason or the start
 	 *                    and end of the ban
 	 *
-	 * @return mixed
+	 * @return array List of banned users
 	 */
-	public function after_ban(array $data);
+	public function after_ban(array $data): array;
 
 	/**
-	 * Gives the possiblity to do some clean up after unbanning.
+	 * Gives the possibility to do some clean up after unbanning.
 	 * The return value of this method will be passed through
 	 * to the caller.
 	 *
 	 * @param array $data An array containing information about
 	 *                    the unbans, e.g. the unbanned items.
 	 *
-	 * @return mixed
+	 * @return array List of unbanned users
 	 */
-	public function after_unban(array $data);
+	public function after_unban(array $data): array;
 
 	/**
 	 * In the case that get_user_column() returns null, this method

phpBB/phpbb/ban/type/user.php

@@ -39,7 +39,7 @@ class user extends base
 	/**
 	 * {@inheritDoc}
 	 */
-	public function after_ban(array $data)
+	public function after_ban(array $data): array
 	{
 		$this->logout_affected_users($data['items']);
 		return $this->banned_users;
@@ -48,7 +48,7 @@ class user extends base
 	/**
 	 * {@inheritDoc}
 	 */
-	public function after_unban(array $data)
+	public function after_unban(array $data): array
 	{
 		$user_ids = array_map('intval', $data['items']);
 

tests/ban/ban_manager_test.php

@@ -33,6 +33,7 @@ class ban_manager_test extends \phpbb_session_test_case
 		$user = new \phpbb\user($language, '\phpbb\datetime');
 		$user->data['user_id'] = 2;
 		$user->data['user_email'] = 'foo@bar.com';
+		$user->data['user_timezone'] = 0;
 		$config = new \phpbb\config\config([]);
 		$phpbb_dispatcher = new \phpbb_mock_event_dispatcher();
 
@@ -47,8 +48,9 @@ class ban_manager_test extends \phpbb_session_test_case
 		$collection->add('ban.type.email');
 		$collection->add('ban.type.user');
 		$collection->add('ban.type.ip');
+		$phpbb_log = new \phpbb\log\dummy();
 
-		$this->ban_manager = new \phpbb\ban\manager($collection, new \phpbb\cache\driver\dummy(), $this->db, $language, $user, 'phpbb_bans', 'phpbb_users');
+		$this->ban_manager = new \phpbb\ban\manager($collection, new \phpbb\cache\driver\dummy(), $this->db, $language, $phpbb_log, $user, 'phpbb_bans', 'phpbb_users');
 		$phpbb_container->set('ban.manager', $this->ban_manager);
 		$this->phpbb_container = $phpbb_container;
 	}
@@ -392,8 +394,9 @@ class ban_manager_test extends \phpbb_session_test_case
 
 		$language = new \phpbb\language\language(new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx));
 		$user = new \phpbb\user($language, '\phpbb\datetime');
+		$phpbb_log = new \phpbb\log\dummy();
 
-		$ban_manager = new \phpbb\ban\manager($collection, new \phpbb\cache\driver\dummy(), $this->db, $language, $user, 'phpbb_bans', 'phpbb_users');
+		$ban_manager = new \phpbb\ban\manager($collection, new \phpbb\cache\driver\dummy(), $this->db, $language, $phpbb_log, $user, 'phpbb_bans', 'phpbb_users');
 
 		$this->assertEquals(
 			[
@@ -434,8 +437,9 @@ class ban_manager_test extends \phpbb_session_test_case
 
 		$language = new \phpbb\language\language(new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx));
 		$user = new \phpbb\user($language, '\phpbb\datetime');
+		$phpbb_log = new \phpbb\log\dummy();
 
-		$ban_manager = new \phpbb\ban\manager($collection, new \phpbb\cache\driver\dummy(), $this->db, $language, $user, 'phpbb_bans', 'phpbb_users');
+		$ban_manager = new \phpbb\ban\manager($collection, new \phpbb\cache\driver\dummy(), $this->db, $language, $phpbb_log, $user, 'phpbb_bans', 'phpbb_users');
 
 		$start_time = new \DateTime();
 		$start_time->setTimestamp(1000);

tests/functions/validate_user_email_test.php

@@ -61,8 +61,9 @@ class phpbb_functions_validate_user_email_test extends phpbb_database_test_case
 		$collection->add('ban.type.email');
 		$collection->add('ban.type.user');
 		$collection->add('ban.type.ip');
+		$phpbb_log = new \phpbb\log\dummy();
 
-		$ban_manager = new \phpbb\ban\manager($collection, $cache->get_driver(), $this->db, $language, $this->user, 'phpbb_bans', 'phpbb_users');
+		$ban_manager = new \phpbb\ban\manager($collection, $cache->get_driver(), $this->db, $language, $phpbb_log, $this->user, 'phpbb_bans', 'phpbb_users');
 		$phpbb_container->set('ban.manager', $ban_manager);
 	}
 

tests/session/check_ban_test.php

@@ -86,8 +86,9 @@ class phpbb_session_check_ban_test extends phpbb_session_test_case
 		$collection->add('ban.type.email');
 		$collection->add('ban.type.user');
 		$collection->add('ban.type.ip');
+		$phpbb_log = new \phpbb\log\dummy();
 
-		$ban_manager = new \phpbb\ban\manager($collection, $cache->get_driver(), $this->db, $language, $user, 'phpbb_bans', 'phpbb_users');
+		$ban_manager = new \phpbb\ban\manager($collection, $cache->get_driver(), $this->db, $language, $phpbb_log, $user, 'phpbb_bans', 'phpbb_users');
 		$phpbb_container->set('ban.manager', $ban_manager);
 	}
 

tests/session/testable_factory.php

@@ -120,8 +120,9 @@ class phpbb_session_testable_factory
 		$collection->add('ban.type.email');
 		$collection->add('ban.type.user');
 		$collection->add('ban.type.ip');
+		$phpbb_log = new \phpbb\log\dummy();
 
-		$ban_manager = new \phpbb\ban\manager($collection, $cache, $db, $language,  $user,'phpbb_bans', 'phpbb_users');
+		$ban_manager = new \phpbb\ban\manager($collection, $cache, $db, $language, $phpbb_log, $user,'phpbb_bans', 'phpbb_users');
 		$phpbb_container->set('ban.manager', $ban_manager);
 
 		$session = new phpbb_mock_session_testable;