From 98520dcc6abf0871dfb5f641840769f91e64947d Mon Sep 17 00:00:00 2001 From: fs Date: Thu, 19 Mar 2026 18:41:08 +0100 Subject: [PATCH] Split filter drawer contracts --- bin/module-permissions-sync.php | 3 +- .../Module/ModulePermissionSynchronizer.php | 50 +++++++- .../Access/PermissionRepository.php | 9 ++ .../Access/PermissionRepositoryInterface.php | 3 + .../ModulePermissionSynchronizerTest.php | 111 ++++++++++++++++++ 5 files changed, 174 insertions(+), 2 deletions(-) diff --git a/bin/module-permissions-sync.php b/bin/module-permissions-sync.php index b9958fe..32dbf81 100644 --- a/bin/module-permissions-sync.php +++ b/bin/module-permissions-sync.php @@ -35,10 +35,11 @@ function modulePermissionsSyncRun(): int $result = $synchronizer->sync(); fwrite(STDOUT, sprintf( - "module-permissions-sync: created=%d updated=%d unchanged=%d total=%d\n", + "module-permissions-sync: created=%d updated=%d unchanged=%d deactivated=%d total=%d\n", $result['created'], $result['updated'], $result['unchanged'], + $result['deactivated'], $result['total'] )); diff --git a/lib/App/Module/ModulePermissionSynchronizer.php b/lib/App/Module/ModulePermissionSynchronizer.php index 97cf003..1832c50 100644 --- a/lib/App/Module/ModulePermissionSynchronizer.php +++ b/lib/App/Module/ModulePermissionSynchronizer.php @@ -16,7 +16,7 @@ final class ModulePermissionSynchronizer } /** - * @return array{created: int, updated: int, unchanged: int, total: int} + * @return array{created: int, updated: int, unchanged: int, deactivated: int, total: int} */ public function sync(): array { @@ -66,14 +66,62 @@ final class ModulePermissionSynchronizer $updated++; } + $deactivated = $this->deactivateOrphaned(); + return [ 'created' => $created, 'updated' => $updated, 'unchanged' => $unchanged, + 'deactivated' => $deactivated, 'total' => $total, ]; } + /** + * Deactivate system permissions that no active module claims. + * + * Only touches is_system=1 rows (module-owned). Admin-created permissions + * (is_system=0) are never modified. Setting active=0 is reversible — if the + * module is re-enabled, the next sync will set active=1 again. + */ + private function deactivateOrphaned(): int + { + $activeKeys = $this->moduleRegistry->getPermissionKeys(); + $systemPermissions = $this->permissionRepository->listSystem(); + $deactivated = 0; + + foreach ($systemPermissions as $permission) { + $key = (string) $permission['key']; + $id = (int) $permission['id']; + $isActive = (int) $permission['active']; + + if ($key === '' || $id <= 0) { + continue; + } + + // Already inactive — nothing to do + if ($isActive === 0) { + continue; + } + + // Still claimed by an active module — keep it + if (in_array($key, $activeKeys, true)) { + continue; + } + + // Orphaned: system permission not claimed by any active module → deactivate + $this->permissionRepository->update($id, [ + 'key' => $key, + 'description' => (string) $permission['description'], + 'active' => 0, + 'is_system' => 1, + ]); + $deactivated++; + } + + return $deactivated; + } + /** * @param array $existing * @param array $desired diff --git a/lib/Repository/Access/PermissionRepository.php b/lib/Repository/Access/PermissionRepository.php index 16b8579..9c144b7 100644 --- a/lib/Repository/Access/PermissionRepository.php +++ b/lib/Repository/Access/PermissionRepository.php @@ -115,4 +115,13 @@ class PermissionRepository implements PermissionRepositoryInterface $result = DB::delete('delete from permissions where id = ?', (string) $id); return (bool) $result; } + + public function listSystem(): array + { + $rows = DB::select( + 'select id, `key`, description, active, is_system, created from permissions where is_system = 1 order by `key` asc' + ); + + return RepositoryArrayHelper::unwrapList($rows, 'permissions'); + } } diff --git a/lib/Repository/Access/PermissionRepositoryInterface.php b/lib/Repository/Access/PermissionRepositoryInterface.php index 5ac2eda..0e7c61e 100644 --- a/lib/Repository/Access/PermissionRepositoryInterface.php +++ b/lib/Repository/Access/PermissionRepositoryInterface.php @@ -20,4 +20,7 @@ interface PermissionRepositoryInterface public function update(int $id, array $data): bool; public function delete(int $id): bool; + + /** @return list */ + public function listSystem(): array; } diff --git a/tests/App/Module/ModulePermissionSynchronizerTest.php b/tests/App/Module/ModulePermissionSynchronizerTest.php index afce9fc..cd1b644 100644 --- a/tests/App/Module/ModulePermissionSynchronizerTest.php +++ b/tests/App/Module/ModulePermissionSynchronizerTest.php @@ -45,6 +45,112 @@ final class ModulePermissionSynchronizerTest extends TestCase self::assertSame(1, $second['unchanged']); } + public function testSyncDeactivatesOrphanedSystemPermissions(): void + { + // Module claims 'mod.perm_a' — 'mod.perm_b' is orphaned (no module owns it) + $registry = $this->createRegistryWithPermissions([[ + 'key' => 'mod.perm_a', + 'description' => 'Active permission', + 'active' => 1, + 'is_system' => 1, + ]]); + + $repository = new InMemoryPermissionRepository(); + $repository->create([ + 'key' => 'mod.perm_a', + 'description' => 'Active permission', + 'active' => 1, + 'is_system' => 1, + ]); + $repository->create([ + 'key' => 'mod.perm_b', + 'description' => 'Orphaned module permission', + 'active' => 1, + 'is_system' => 1, + ]); + + $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $result = $synchronizer->sync(); + + self::assertSame(1, $result['deactivated']); + self::assertSame(1, $result['unchanged']); + + $orphaned = $repository->findByKey('mod.perm_b'); + self::assertSame(0, (int) ($orphaned['active'] ?? 1), 'Orphaned permission should be deactivated'); + + $active = $repository->findByKey('mod.perm_a'); + self::assertSame(1, (int) ($active['active'] ?? 0), 'Claimed permission should stay active'); + } + + public function testSyncDoesNotDeactivateAdminCreatedPermissions(): void + { + // Empty module — no permissions declared + $registry = $this->createRegistryWithPermissions([]); + + $repository = new InMemoryPermissionRepository(); + $repository->create([ + 'key' => 'admin.custom', + 'description' => 'Admin-created permission', + 'active' => 1, + 'is_system' => 0, + ]); + + $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $result = $synchronizer->sync(); + + self::assertSame(0, $result['deactivated']); + + $adminPerm = $repository->findByKey('admin.custom'); + self::assertSame(1, (int) ($adminPerm['active'] ?? 0), 'Admin-created permission must not be touched'); + } + + public function testSyncReactivatesPermissionWhenModuleReEnabled(): void + { + $registry = $this->createRegistryWithPermissions([[ + 'key' => 'mod.perm_a', + 'description' => 'Reactivated permission', + 'active' => 1, + 'is_system' => 1, + ]]); + + $repository = new InMemoryPermissionRepository(); + // Simulate previously deactivated permission + $repository->create([ + 'key' => 'mod.perm_a', + 'description' => 'Reactivated permission', + 'active' => 0, + 'is_system' => 1, + ]); + + $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $result = $synchronizer->sync(); + + self::assertSame(0, $result['created']); + self::assertSame(1, $result['updated']); + self::assertSame(0, $result['deactivated']); + + $perm = $repository->findByKey('mod.perm_a'); + self::assertSame(1, (int) ($perm['active'] ?? 0), 'Permission should be reactivated by sync'); + } + + public function testSyncSkipsAlreadyDeactivatedOrphans(): void + { + $registry = $this->createRegistryWithPermissions([]); + + $repository = new InMemoryPermissionRepository(); + $repository->create([ + 'key' => 'mod.old_perm', + 'description' => 'Already deactivated', + 'active' => 0, + 'is_system' => 1, + ]); + + $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $result = $synchronizer->sync(); + + self::assertSame(0, $result['deactivated'], 'Already-inactive permission should not be counted'); + } + public function testSyncUpdatesExistingPermissionWhenDefinitionChanged(): void { $registry = $this->createRegistryWithPermissions([[ @@ -178,4 +284,9 @@ final class InMemoryPermissionRepository implements PermissionRepositoryInterfac unset($this->rows[$id]); return true; } + + public function listSystem(): array + { + return array_values(array_filter($this->rows, static fn (array $row): bool => (int) ($row['is_system'] ?? 0) === 1)); + } }