tbh/breadcrumb-the-shire
1
0
Fork 0
forked from fa/breadcrumb-the-shire
Code Pull Requests Activity

fix: permission sync was deactivating core permissions, not just module-owned ones

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Felix Schneider
2026-03-19 19:32:36 +01:00
parent e40cba23a1
commit 8cd21f3c8a
3 changed files with 151 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
bin/module-permissions-sync.php
View File

@@ -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();

82
lib/App/Module/ModulePermissionSynchronizer.php
View File

@@ -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<string>
*/
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<string, mixed> $existing
* @param array<string, mixed> $desired

132
tests/App/Module/ModulePermissionSynchronizerTest.php
View File

@@ -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<array<string, mixed>> $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',
'<?php return ' . var_export(['id' => $moduleId, 'permissions' => $permissions], true) . ';'
);
}
/**
* @param list<array{key: string, description: string, active: int, is_system: int}> $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',
'<?php return ' . var_export([