From 8cd21f3c8a859e9836008167daa6ea11ec1db71b Mon Sep 17 00:00:00 2001 From: fs Date: Thu, 19 Mar 2026 19:32:36 +0100 Subject: [PATCH] fix: permission sync was deactivating core permissions, not just module-owned ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deactivateOrphaned() treated ALL is_system=1 permissions as module-owned. Core seed permissions (users.view, tenants.view, etc.) are also is_system=1, so they were incorrectly deactivated — breaking admin panel access. Fix: scan all module manifests on disk (enabled or not) to build the set of module-owned permission keys. Only deactivate permissions whose key appears in that set. Core seed permissions are never touched. Co-Authored-By: Claude Opus 4.6 (1M context) --- bin/module-permissions-sync.php | 3 +- .../Module/ModulePermissionSynchronizer.php | 82 ++++++++++- .../ModulePermissionSynchronizerTest.php | 132 ++++++++++-------- 3 files changed, 151 insertions(+), 66 deletions(-) diff --git a/bin/module-permissions-sync.php b/bin/module-permissions-sync.php index e6d48cc..122d527 100644 --- a/bin/module-permissions-sync.php +++ b/bin/module-permissions-sync.php @@ -30,7 +30,8 @@ function modulePermissionsSyncRun(): int static function (): int { $synchronizer = new ModulePermissionSynchronizer( app(ModuleRegistry::class), - app(PermissionRepository::class) + app(PermissionRepository::class), + moduleCliProjectRoot() . '/modules' ); $result = $synchronizer->sync(); diff --git a/lib/App/Module/ModulePermissionSynchronizer.php b/lib/App/Module/ModulePermissionSynchronizer.php index 1832c50..aafbda5 100644 --- a/lib/App/Module/ModulePermissionSynchronizer.php +++ b/lib/App/Module/ModulePermissionSynchronizer.php @@ -11,7 +11,8 @@ final class ModulePermissionSynchronizer { public function __construct( private readonly ModuleRegistry $moduleRegistry, - private readonly PermissionRepositoryInterface $permissionRepository + private readonly PermissionRepositoryInterface $permissionRepository, + private readonly string $modulesDir = '' ) { } @@ -78,15 +79,23 @@ final class ModulePermissionSynchronizer } /** - * Deactivate system permissions that no active module claims. + * Deactivate module-owned 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. + * Only touches permissions whose key is declared in at least one module + * manifest on disk (enabled or not). Core seed permissions are never touched. + * Setting active=0 is reversible — re-enabling the module and running sync + * restores active=1. */ private function deactivateOrphaned(): int { $activeKeys = $this->moduleRegistry->getPermissionKeys(); + $allModuleKeys = $this->collectAllModulePermissionKeys(); + + // Nothing to deactivate if we can't determine module-owned keys + if ($allModuleKeys === []) { + return 0; + } + $systemPermissions = $this->permissionRepository->listSystem(); $deactivated = 0; @@ -109,7 +118,12 @@ final class ModulePermissionSynchronizer continue; } - // Orphaned: system permission not claimed by any active module → deactivate + // Not a module-owned permission — never touch core seed permissions + if (!in_array($key, $allModuleKeys, true)) { + continue; + } + + // Orphaned module permission: declared by a module on disk but not by any active module $this->permissionRepository->update($id, [ 'key' => $key, 'description' => (string) $permission['description'], @@ -122,6 +136,62 @@ final class ModulePermissionSynchronizer return $deactivated; } + /** + * Scan all module manifests on disk (not just enabled) and collect their permission keys. + * This determines which permissions are module-owned vs core seed data. + * + * @return list + */ + private function collectAllModulePermissionKeys(): array + { + if ($this->modulesDir === '' || !is_dir($this->modulesDir)) { + return []; + } + + $keys = []; + $entries = scandir($this->modulesDir); + if ($entries === false) { + return []; + } + + foreach ($entries as $entry) { + if ($entry === '.' || $entry === '..') { + continue; + } + + $manifestFile = $this->modulesDir . '/' . $entry . '/module.php'; + if (!is_file($manifestFile)) { + continue; + } + + try { + $raw = require $manifestFile; + if (!is_array($raw)) { + continue; + } + + $permissions = $raw['permissions'] ?? []; + if (!is_array($permissions)) { + continue; + } + + foreach ($permissions as $permission) { + if (is_array($permission)) { + $key = trim((string) ($permission['key'] ?? '')); + if ($key !== '') { + $keys[] = $key; + } + } + } + } catch (\Throwable) { + // Skip broken manifests — don't break the sync + continue; + } + } + + return array_values(array_unique($keys)); + } + /** * @param array $existing * @param array $desired diff --git a/tests/App/Module/ModulePermissionSynchronizerTest.php b/tests/App/Module/ModulePermissionSynchronizerTest.php index cd1b644..fd9b207 100644 --- a/tests/App/Module/ModulePermissionSynchronizerTest.php +++ b/tests/App/Module/ModulePermissionSynchronizerTest.php @@ -32,7 +32,7 @@ final class ModulePermissionSynchronizerTest extends TestCase ]]); $repository = new InMemoryPermissionRepository(); - $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $synchronizer = new ModulePermissionSynchronizer($registry, $repository, $this->fixturesDir); $first = $synchronizer->sync(); self::assertSame(1, $first['created']); @@ -45,9 +45,17 @@ final class ModulePermissionSynchronizerTest extends TestCase self::assertSame(1, $second['unchanged']); } - public function testSyncDeactivatesOrphanedSystemPermissions(): void + public function testSyncDeactivatesOrphanedModulePermissions(): void { - // Module claims 'mod.perm_a' — 'mod.perm_b' is orphaned (no module owns it) + // mod-perm declares perm_a (enabled). mod-disabled declares perm_b (on disk but not enabled). + // perm_b is in DB as active — should be deactivated. + $this->writeModuleManifest('mod-disabled', [[ + 'key' => 'mod.perm_b', + 'description' => 'Orphaned module permission', + 'active' => true, + 'is_system' => true, + ]]); + $registry = $this->createRegistryWithPermissions([[ 'key' => 'mod.perm_a', 'description' => 'Active permission', @@ -56,52 +64,50 @@ final class ModulePermissionSynchronizerTest extends TestCase ]]); $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, - ]); + $repository->create(['key' => 'mod.perm_a', 'description' => 'Active', 'active' => 1, 'is_system' => 1]); + $repository->create(['key' => 'mod.perm_b', 'description' => 'Orphaned', 'active' => 1, 'is_system' => 1]); - $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $synchronizer = new ModulePermissionSynchronizer($registry, $repository, $this->fixturesDir); $result = $synchronizer->sync(); self::assertSame(1, $result['deactivated']); - self::assertSame(1, $result['unchanged']); + self::assertSame(0, (int) ($repository->findByKey('mod.perm_b')['active'] ?? 1)); + self::assertSame(1, (int) ($repository->findByKey('mod.perm_a')['active'] ?? 0)); + } - $orphaned = $repository->findByKey('mod.perm_b'); - self::assertSame(0, (int) ($orphaned['active'] ?? 1), 'Orphaned permission should be deactivated'); + public function testSyncDoesNotDeactivateCoreSystemPermissions(): void + { + // Core permission (users.view) is is_system=1 but NOT declared by any module on disk + $registry = $this->createRegistryWithPermissions([[ + 'key' => 'mod.perm_a', + 'description' => 'Module permission', + 'active' => 1, + 'is_system' => 1, + ]]); - $active = $repository->findByKey('mod.perm_a'); - self::assertSame(1, (int) ($active['active'] ?? 0), 'Claimed permission should stay active'); + $repository = new InMemoryPermissionRepository(); + $repository->create(['key' => 'mod.perm_a', 'description' => 'Module', 'active' => 1, 'is_system' => 1]); + $repository->create(['key' => 'users.view', 'description' => 'Core permission', 'active' => 1, 'is_system' => 1]); + + $synchronizer = new ModulePermissionSynchronizer($registry, $repository, $this->fixturesDir); + $result = $synchronizer->sync(); + + self::assertSame(0, $result['deactivated']); + self::assertSame(1, (int) ($repository->findByKey('users.view')['active'] ?? 0), 'Core permission must not be deactivated'); } 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, - ]); + $repository->create(['key' => 'admin.custom', 'description' => 'Admin-created', 'active' => 1, 'is_system' => 0]); - $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $synchronizer = new ModulePermissionSynchronizer($registry, $repository, $this->fixturesDir); $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'); + self::assertSame(1, (int) ($repository->findByKey('admin.custom')['active'] ?? 0)); } public function testSyncReactivatesPermissionWhenModuleReEnabled(): void @@ -114,41 +120,35 @@ final class ModulePermissionSynchronizerTest extends TestCase ]]); $repository = new InMemoryPermissionRepository(); - // Simulate previously deactivated permission - $repository->create([ - 'key' => 'mod.perm_a', - 'description' => 'Reactivated permission', - 'active' => 0, - 'is_system' => 1, - ]); + $repository->create(['key' => 'mod.perm_a', 'description' => 'Reactivated', 'active' => 0, 'is_system' => 1]); - $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $synchronizer = new ModulePermissionSynchronizer($registry, $repository, $this->fixturesDir); $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'); + self::assertSame(1, (int) ($repository->findByKey('mod.perm_a')['active'] ?? 0)); } public function testSyncSkipsAlreadyDeactivatedOrphans(): void { + $this->writeModuleManifest('mod-old', [[ + 'key' => 'mod.old_perm', + 'description' => 'Old', + 'active' => true, + 'is_system' => true, + ]]); + $registry = $this->createRegistryWithPermissions([]); $repository = new InMemoryPermissionRepository(); - $repository->create([ - 'key' => 'mod.old_perm', - 'description' => 'Already deactivated', - 'active' => 0, - 'is_system' => 1, - ]); + $repository->create(['key' => 'mod.old_perm', 'description' => 'Already deactivated', 'active' => 0, 'is_system' => 1]); - $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $synchronizer = new ModulePermissionSynchronizer($registry, $repository, $this->fixturesDir); $result = $synchronizer->sync(); - self::assertSame(0, $result['deactivated'], 'Already-inactive permission should not be counted'); + self::assertSame(0, $result['deactivated']); } public function testSyncUpdatesExistingPermissionWhenDefinitionChanged(): void @@ -161,14 +161,9 @@ final class ModulePermissionSynchronizerTest extends TestCase ]]); $repository = new InMemoryPermissionRepository(); - $repository->create([ - 'key' => 'address_book.view', - 'description' => 'Old description', - 'active' => 0, - 'is_system' => 0, - ]); + $repository->create(['key' => 'address_book.view', 'description' => 'Old description', 'active' => 0, 'is_system' => 0]); - $synchronizer = new ModulePermissionSynchronizer($registry, $repository); + $synchronizer = new ModulePermissionSynchronizer($registry, $repository, $this->fixturesDir); $result = $synchronizer->sync(); self::assertSame(0, $result['created']); @@ -181,12 +176,31 @@ final class ModulePermissionSynchronizerTest extends TestCase self::assertSame(1, (int) ($permission['is_system'] ?? 0)); } + /** + * Write a module manifest with the given permissions to a module directory. + * + * @param list> $permissions + */ + private function writeModuleManifest(string $moduleId, array $permissions): void + { + $dir = $this->fixturesDir . '/' . $moduleId; + if (!is_dir($dir)) { + mkdir($dir, 0777, true); + } + file_put_contents( + $dir . '/module.php', + ' $moduleId, 'permissions' => $permissions], true) . ';' + ); + } + /** * @param list $permissions */ private function createRegistryWithPermissions(array $permissions): ModuleRegistry { - mkdir($this->fixturesDir . '/mod-perm', 0777, true); + if (!is_dir($this->fixturesDir . '/mod-perm')) { + mkdir($this->fixturesDir . '/mod-perm', 0777, true); + } file_put_contents( $this->fixturesDir . '/mod-perm/module.php', '