refactor: use event listener instead of user changed hook

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
nextcloud · Aug 21, 2024 · d30b988 · d30b988
1 parent cf56874
commit d30b988
Show file tree

Hide file tree

Showing 6 changed files with 142 additions and 31 deletions.
diff --git a/apps/admin_audit/composer/composer/autoload_classmap.php b/apps/admin_audit/composer/composer/autoload_classmap.php
@@ -24,4 +24,5 @@
     'OCA\\AdminAudit\\BackgroundJobs\\Rotate' => $baseDir . '/../lib/BackgroundJobs/Rotate.php',
     'OCA\\AdminAudit\\IAuditLogger' => $baseDir . '/../lib/IAuditLogger.php',
     'OCA\\AdminAudit\\Listener\\CriticalActionPerformedEventListener' => $baseDir . '/../lib/Listener/CriticalActionPerformedEventListener.php',
+    'OCA\\AdminAudit\\Listener\\UserChangedEventListener' => $baseDir . '/../lib/Listener/UserChangedEventListener.php',
 );
diff --git a/apps/admin_audit/composer/composer/autoload_static.php b/apps/admin_audit/composer/composer/autoload_static.php
@@ -39,6 +39,7 @@ class ComposerStaticInitAdminAudit
         'OCA\\AdminAudit\\BackgroundJobs\\Rotate' => __DIR__ . '/..' . '/../lib/BackgroundJobs/Rotate.php',
         'OCA\\AdminAudit\\IAuditLogger' => __DIR__ . '/..' . '/../lib/IAuditLogger.php',
         'OCA\\AdminAudit\\Listener\\CriticalActionPerformedEventListener' => __DIR__ . '/..' . '/../lib/Listener/CriticalActionPerformedEventListener.php',
+        'OCA\\AdminAudit\\Listener\\UserChangedEventListener' => __DIR__ . '/..' . '/../lib/Listener/UserChangedEventListener.php',
     );
 
     public static function getInitializer(ClassLoader $loader)

diff --git a/apps/admin_audit/lib/Actions/UserManagement.php b/apps/admin_audit/lib/Actions/UserManagement.php
@@ -71,36 +71,6 @@ public function unassign(string $uid): void {
 		);
 	}
 
-	/**
-	 * Log enabling of users
-	 *
-	 * @param array $params
-	 */
-	public function change(array $params): void {
-		switch ($params['feature']) {
-			case 'enabled':
-				$this->log(
-					$params['value'] === true
-						? 'User enabled: "%s"'
-						: 'User disabled: "%s"',
-					['user' => $params['user']->getUID()],
-					[
-						'user',
-					]
-				);
-				break;
-			case 'eMailAddress':
-				$this->log(
-					'Email address changed for user %s',
-					['user' => $params['user']->getUID()],
-					[
-						'user',
-					]
-				);
-				break;
-		}
-	}
-
 	/**
 	 * Logs changing of the user scope
 	 *

diff --git a/apps/admin_audit/lib/AppInfo/Application.php b/apps/admin_audit/lib/AppInfo/Application.php
@@ -23,6 +23,7 @@
 use OCA\AdminAudit\AuditLogger;
 use OCA\AdminAudit\IAuditLogger;
 use OCA\AdminAudit\Listener\CriticalActionPerformedEventListener;
+use OCA\AdminAudit\Listener\UserChangedEventListener;
 use OCP\App\ManagerEvent;
 use OCP\AppFramework\App;
 use OCP\AppFramework\Bootstrap\IBootContext;
@@ -47,6 +48,7 @@
 use OCP\Log\ILogFactory;
 use OCP\Preview\BeforePreviewFetchedEvent;
 use OCP\Share;
+use OCP\User\Events\UserChangedEvent;
 use OCP\Util;
 use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
@@ -65,6 +67,7 @@ public function register(IRegistrationContext $context): void {
 		});
 
 		$context->registerEventListener(CriticalActionPerformedEvent::class, CriticalActionPerformedEventListener::class);
+		$context->registerEventListener(UserChangedEvent::class, UserChangedEventListener::class);
 	}
 
 	public function boot(IBootContext $context): void {
@@ -109,7 +112,6 @@ private function userManagementHooks(IAuditLogger $logger,
 
 		Util::connectHook('OC_User', 'post_createUser', $userActions, 'create');
 		Util::connectHook('OC_User', 'post_deleteUser', $userActions, 'delete');
-		Util::connectHook('OC_User', 'changeUser', $userActions, 'change');
 
 		assert($userSession instanceof UserSession);
 		$userSession->listen('\OC\User', 'postSetPassword', [$userActions, 'setPassword']);

diff --git a/apps/admin_audit/lib/Listener/UserChangedEventListener.php b/apps/admin_audit/lib/Listener/UserChangedEventListener.php
@@ -0,0 +1,45 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\AdminAudit\Listener;
+
+use OCA\AdminAudit\Actions\Action;
+use OCP\EventDispatcher\Event;
+use OCP\EventDispatcher\IEventListener;
+use OCP\User\Events\UserChangedEvent;
+
+/** @template-implements IEventListener<UserChangedEvent> */
+class UserChangedEventListener extends Action implements IEventListener {
+	public function handle(Event $event): void {
+		if (!($event instanceof UserChangedEvent)) {
+			return;
+		}
+
+		switch ($event->getFeature()) {
+			case 'enabled':
+				$this->log(
+					$event->getValue()
+						? 'User enabled: "%s"'
+						: 'User disabled: "%s"',
+					['user' => $event->getUser()->getUID()],
+					[
+						'user',
+					]
+				);
+				break;
+			case 'eMailAddress':
+				$this->log(
+					'Email address changed for user %s',
+					['user' => $event->getUser()->getUID()],
+					[
+						'user',
+					]
+				);
+				break;
+		}
+	}
+}
diff --git a/apps/admin_audit/tests/Listener/UserChangedEventListenerTest.php b/apps/admin_audit/tests/Listener/UserChangedEventListenerTest.php
@@ -0,0 +1,92 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\AdminAudit\Tests\Actions;
+
+use OCA\AdminAudit\AuditLogger;
+use OCA\AdminAudit\Listener\UserChangedEventListener;
+use OCP\IUser;
+use OCP\User\Events\UserChangedEvent;
+use PHPUnit\Framework\MockObject\MockObject;
+use Test\TestCase;
+
+class UserChangedEventListenerTest extends TestCase {
+	private AuditLogger|MockObject $logger;
+
+	private UserChangedEventListener $listener;
+
+	private MockObject|IUser $user;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->logger = $this->createMock(AuditLogger::class);
+		$this->listener = new UserChangedEventListener($this->logger);
+
+		$this->user = $this->createMock(IUser::class);
+		$this->user->method('getUID')->willReturn('alice');
+		$this->user->method('getDisplayName')->willReturn('Alice');
+	}
+
+	public function testSkipUnsupported(): void {
+		$this->logger->expects($this->never())
+			->method('info');
+
+		$event = new UserChangedEvent(
+			$this->user,
+			'unsupported',
+			'value',
+		);
+
+		$this->listener->handle($event);
+	}
+
+	public function testUserEnabled(): void {
+		$this->logger->expects($this->once())
+			->method('info')
+			->with('User enabled: "alice"', ['app' => 'admin_audit']);
+
+		$event = new UserChangedEvent(
+			$this->user,
+			'enabled',
+			true,
+			false,
+		);
+
+		$this->listener->handle($event);
+	}
+
+	public function testUserDisabled(): void {
+		$this->logger->expects($this->once())
+			->method('info')
+			->with('User disabled: "alice"', ['app' => 'admin_audit']);
+
+		$event = new UserChangedEvent(
+			$this->user,
+			'enabled',
+			false,
+			true,
+		);
+
+		$this->listener->handle($event);
+	}
+
+	public function testEmailChanged(): void {
+		$this->logger->expects($this->once())
+			->method('info')
+			->with('Email address changed for user alice', ['app' => 'admin_audit']);
+
+		$event = new UserChangedEvent(
+			$this->user,
+			'eMailAddress',
+			'alice@alice.com',
+			'',
+		);
+
+		$this->listener->handle($event);
+	}
+}