From ca0fecfe08818ba7fa344988e418fb06b36975f8 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+klarkent@users.noreply.github.com> Date: Wed, 28 May 2025 19:28:26 +0200 Subject: [PATCH 1/3] chore: improve debug logging in RetentionService Signed-off-by: Salvatore Martire <4652631+klarkent@users.noreply.github.com> --- lib/Service/RetentionService.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Service/RetentionService.php b/lib/Service/RetentionService.php index 674eed3f..868189e1 100644 --- a/lib/Service/RetentionService.php +++ b/lib/Service/RetentionService.php @@ -157,6 +157,7 @@ public function executeRetentionPolicy(IUser $user): ?bool { $this->logger->debug('Account already disabled, continuing with potential deletion: ' . $user->getUID()); } catch (SkipUserException $e) { // Not disabling yet, continue with checking deletion + $this->logger->debug("Disable: {$e->getMessage()}", $e->getLogParameters()); } } else { $this->logger->debug('No account disabling policy enabled for account: ' . $user->getUID()); @@ -183,6 +184,7 @@ public function executeRetentionPolicy(IUser $user): ?bool { return true; } catch (SkipUserException $e) { // Not deleting yet, continue with checking reminders + $this->logger->debug("Delete: {$e->getMessage()}", $e->getLogParameters()); } } else { $this->logger->debug('No account retention policy enabled for account: ' . $user->getUID()); @@ -202,7 +204,7 @@ public function executeRetentionPolicy(IUser $user): ?bool { $this->sendReminder($user, $lastActivity, $policyDays, $policyDaysDisable); } catch (SkipUserException $e) { - $this->logger->debug($e->getMessage(), $e->getLogParameters()); + $this->logger->debug("Reminder: {$e->getMessage()}", $e->getLogParameters()); continue; } } From ab1c1262124d7b3dd9c4661998b88ff859bdeb4a Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+klarkent@users.noreply.github.com> Date: Wed, 28 May 2025 19:30:15 +0200 Subject: [PATCH 2/3] fix: avoid disabling user again when re-enabled... This commit fixes an issue where a user that has been inactive for a while, had their account disabled automatically by this app and then re-enabled by an admin would get their account disabled again, if they would not log-in again before the next run of the background-job. Signed-off-by: Salvatore Martire <4652631+klarkent@users.noreply.github.com> --- AUTHORS.md | 1 + lib/AppInfo/Application.php | 3 + lib/Listeners/UserChangedListener.php | 52 ++++++++++++++ lib/Service/RetentionService.php | 5 +- tests/Listeners/UserChangedListenerTest.php | 80 +++++++++++++++++++++ 5 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 lib/Listeners/UserChangedListener.php create mode 100644 tests/Listeners/UserChangedListenerTest.php diff --git a/AUTHORS.md b/AUTHORS.md index e2f77322..e9fd6cd0 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -15,5 +15,6 @@ - Jonas - Morris Jobke - rakekniven <2069590+rakekniven@users.noreply.github.com> +- Salvatore Martire - Sascha Wiswedel - Vincent Petry diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 62592cdb..8603ebb4 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -7,10 +7,12 @@ */ namespace OCA\UserRetention\AppInfo; +use OCA\UserRetention\Listeners\UserChangedListener; use OCP\AppFramework\App; use OCP\AppFramework\Bootstrap\IBootContext; use OCP\AppFramework\Bootstrap\IBootstrap; use OCP\AppFramework\Bootstrap\IRegistrationContext; +use OCP\User\Events\UserChangedEvent; use OCP\Util; class Application extends App implements IBootstrap { @@ -20,6 +22,7 @@ public function __construct(array $urlParams = []) { } public function register(IRegistrationContext $context): void { + $context->registerEventListener(UserChangedEvent::class, UserChangedListener::class); } public function boot(IBootContext $context): void { diff --git a/lib/Listeners/UserChangedListener.php b/lib/Listeners/UserChangedListener.php new file mode 100644 index 00000000..a13b5b4d --- /dev/null +++ b/lib/Listeners/UserChangedListener.php @@ -0,0 +1,52 @@ + + */ +class UserChangedListener implements IEventListener { + + public function __construct( + public LoggerInterface $logger, + public IConfig $config, + public ITimeFactory $timeFactory, + ) { + } + + public function handle(Event $event): void { + if (!($event instanceof UserChangedEvent)) { + return; + } + + if ($event->getFeature() === 'enabled') { + $this->handleEnabledChange($event); + } + } + + private function handleEnabledChange(UserChangedEvent $event): void { + $oldValue = $event->getOldValue(); + $newValue = $event->getValue(); + if ($oldValue === false && $newValue === true) { + $this->config->setUserValue( + $event->getUser()->getUID(), + 'user_retention', + 'user_reenabled_at', + (string)$this->timeFactory->getTime() + ); + } + } +} diff --git a/lib/Service/RetentionService.php b/lib/Service/RetentionService.php index 868189e1..88e0ee0c 100644 --- a/lib/Service/RetentionService.php +++ b/lib/Service/RetentionService.php @@ -223,11 +223,12 @@ protected function shouldPerformActionOnUser(IUser $user, int $skipIfNewerThan, $discoveryTimestamp = $this->skipUserBasedOnDiscovery($user); $lastWebLogin = $user->getLastLogin(); $authTokensLastActivity = $this->getAuthTokensLastActivity($user); + $userReenabledTimestamp = (int)$this->config->getUserValue($user->getUID(), 'user_retention', 'user_reenabled_at', 0); if ($authTokensLastActivity === null) { - $lastAction = max($discoveryTimestamp, $lastWebLogin); + $lastAction = max($discoveryTimestamp, $lastWebLogin, $userReenabledTimestamp); } else { - $lastAction = max($discoveryTimestamp, $lastWebLogin, $authTokensLastActivity); + $lastAction = max($discoveryTimestamp, $lastWebLogin, $authTokensLastActivity, $userReenabledTimestamp); } if ($this->keepUsersWithoutLogin && $lastAction === 0) { diff --git a/tests/Listeners/UserChangedListenerTest.php b/tests/Listeners/UserChangedListenerTest.php new file mode 100644 index 00000000..637c7e35 --- /dev/null +++ b/tests/Listeners/UserChangedListenerTest.php @@ -0,0 +1,80 @@ +logger = $this->createMock(LoggerInterface::class); + $this->config = $this->createMock(IConfig::class); + $this->timeFactory = $this->createMock(ITimeFactory::class); + } + + public function testUserEnabledShouldTriggerUserReenabledAtUpdate() { + $time = time(); + $uid = '100'; + $user = $this->createMock(IUser::class); + $user->expects($this->once())->method('getUID')->willReturn($uid); + $this->timeFactory->expects($this->once())->method('getTime')->willReturn($time); + $this->config->expects($this->once())->method('setUserValue')->with($uid, 'user_retention', 'user_reenabled_at', $time); + + $event = $this->createMock(UserChangedEvent::class); + $event->expects($this->once())->method('getUser')->willReturn($user); + $event->expects($this->once())->method('getFeature')->willReturn('enabled'); + $event->expects($this->once())->method('getOldValue')->willReturn(false); + $event->expects($this->once())->method('getValue')->willReturn(true); + + $listener = new UserChangedListener($this->logger, $this->config, $this->timeFactory); + $listener->handle($event); + } + + public function testHandleShouldNotHandleOtherEvents() { + $event = $this->createMock(UserCreatedEvent::class); + $this->config->expects($this->never())->method('setUserValue'); + + $listener = new UserChangedListener($this->logger, $this->config, $this->timeFactory); + $listener->handle($event); + } + + public function testHandleShouldOnlyHandleEnabledFeature() { + $event = $this->createMock(UserChangedEvent::class); + $event->expects($this->once())->method('getFeature')->willReturn('otherFeature'); + $this->config->expects($this->never())->method('setUserValue'); + + $listener = new UserChangedListener($this->logger, $this->config, $this->timeFactory); + $listener->handle($event); + } + + public function testDisabledUserShouldNotTriggerUserReenabledAtUpdate() { + $this->config->expects($this->never())->method('setUserValue'); + + $event = $this->createMock(UserChangedEvent::class); + $event->expects($this->once())->method('getFeature')->willReturn('enabled'); + $event->expects($this->once())->method('getOldValue')->willReturn(true); + $event->expects($this->once())->method('getValue')->willReturn(false); + + $listener = new UserChangedListener($this->logger, $this->config, $this->timeFactory); + $listener->handle($event); + } + +} From aed567e1d1d997129c9e91ec3beee5be112cfa72 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 5 Jun 2025 10:57:06 +0200 Subject: [PATCH 3/3] chore(cs): Apply autofixes Signed-off-by: Joas Schilling --- lib/Listeners/UserChangedListener.php | 6 +++--- lib/Service/RetentionService.php | 6 +++--- tests/Listeners/UserChangedListenerTest.php | 16 ++++++++-------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/Listeners/UserChangedListener.php b/lib/Listeners/UserChangedListener.php index a13b5b4d..244f3ee3 100644 --- a/lib/Listeners/UserChangedListener.php +++ b/lib/Listeners/UserChangedListener.php @@ -21,9 +21,9 @@ class UserChangedListener implements IEventListener { public function __construct( - public LoggerInterface $logger, - public IConfig $config, - public ITimeFactory $timeFactory, + protected LoggerInterface $logger, + protected IConfig $config, + protected ITimeFactory $timeFactory, ) { } diff --git a/lib/Service/RetentionService.php b/lib/Service/RetentionService.php index 88e0ee0c..57935b00 100644 --- a/lib/Service/RetentionService.php +++ b/lib/Service/RetentionService.php @@ -157,7 +157,7 @@ public function executeRetentionPolicy(IUser $user): ?bool { $this->logger->debug('Account already disabled, continuing with potential deletion: ' . $user->getUID()); } catch (SkipUserException $e) { // Not disabling yet, continue with checking deletion - $this->logger->debug("Disable: {$e->getMessage()}", $e->getLogParameters()); + $this->logger->debug('Disable: ' . $e->getMessage(), $e->getLogParameters()); } } else { $this->logger->debug('No account disabling policy enabled for account: ' . $user->getUID()); @@ -184,7 +184,7 @@ public function executeRetentionPolicy(IUser $user): ?bool { return true; } catch (SkipUserException $e) { // Not deleting yet, continue with checking reminders - $this->logger->debug("Delete: {$e->getMessage()}", $e->getLogParameters()); + $this->logger->debug('Delete: ' . $e->getMessage(), $e->getLogParameters()); } } else { $this->logger->debug('No account retention policy enabled for account: ' . $user->getUID()); @@ -204,7 +204,7 @@ public function executeRetentionPolicy(IUser $user): ?bool { $this->sendReminder($user, $lastActivity, $policyDays, $policyDaysDisable); } catch (SkipUserException $e) { - $this->logger->debug("Reminder: {$e->getMessage()}", $e->getLogParameters()); + $this->logger->debug('Reminder: ' . $e->getMessage(), $e->getLogParameters()); continue; } } diff --git a/tests/Listeners/UserChangedListenerTest.php b/tests/Listeners/UserChangedListenerTest.php index 637c7e35..966863ec 100644 --- a/tests/Listeners/UserChangedListenerTest.php +++ b/tests/Listeners/UserChangedListenerTest.php @@ -18,9 +18,9 @@ use Test\TestCase; class UserChangedListenerTest extends TestCase { - private MockObject&LoggerInterface $logger; - private MockObject&IConfig $config; - private MockObject&ITimeFactory $timeFactory; + private LoggerInterface&MockObject $logger; + private IConfig&MockObject $config; + private ITimeFactory&MockObject $timeFactory; protected function setUp(): void { parent::setUp(); @@ -30,9 +30,9 @@ protected function setUp(): void { $this->timeFactory = $this->createMock(ITimeFactory::class); } - public function testUserEnabledShouldTriggerUserReenabledAtUpdate() { + public function testUserEnabledShouldTriggerUserReenabledAtUpdate(): void { $time = time(); - $uid = '100'; + $uid = 'user'; $user = $this->createMock(IUser::class); $user->expects($this->once())->method('getUID')->willReturn($uid); $this->timeFactory->expects($this->once())->method('getTime')->willReturn($time); @@ -48,7 +48,7 @@ public function testUserEnabledShouldTriggerUserReenabledAtUpdate() { $listener->handle($event); } - public function testHandleShouldNotHandleOtherEvents() { + public function testHandleShouldNotHandleOtherEvents(): void { $event = $this->createMock(UserCreatedEvent::class); $this->config->expects($this->never())->method('setUserValue'); @@ -56,7 +56,7 @@ public function testHandleShouldNotHandleOtherEvents() { $listener->handle($event); } - public function testHandleShouldOnlyHandleEnabledFeature() { + public function testHandleShouldOnlyHandleEnabledFeature(): void { $event = $this->createMock(UserChangedEvent::class); $event->expects($this->once())->method('getFeature')->willReturn('otherFeature'); $this->config->expects($this->never())->method('setUserValue'); @@ -65,7 +65,7 @@ public function testHandleShouldOnlyHandleEnabledFeature() { $listener->handle($event); } - public function testDisabledUserShouldNotTriggerUserReenabledAtUpdate() { + public function testDisabledUserShouldNotTriggerUserReenabledAtUpdate(): void { $this->config->expects($this->never())->method('setUserValue'); $event = $this->createMock(UserChangedEvent::class);