Skip to content

Commit a4253d1

Browse files
eiresendezeiresendez
committed
Fix permission cache invalidation and add Redis session regression coverage
1 parent e8d7b4a commit a4253d1

5 files changed

Lines changed: 163 additions & 22 deletions

File tree

‎ProcessMaker/Models/User.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -591,7 +591,7 @@ public function refresh()
591591

592592
// Clear permissions and user_permissions
593593
Cache::forget('permissions');
594-
app(\ProcessMaker\Services\PermissionCacheService::class)->forgetLegacyUserPermissions($this->id);
594+
$this->invalidatePermissionCache();
595595

596596
// return the refreshed user instance
597597
return $this;

‎ProcessMaker/Services/PermissionCacheService.php‎

Lines changed: 44 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,12 @@ class PermissionCacheService implements PermissionCacheInterface
2020

2121
private const TRACKED_PERMISSION_KEYS = 'permission_cache_keys';
2222

23+
private const TRACKED_PERMISSION_KEYS_LOCK = 'permission_cache_keys_lock';
24+
25+
private const TRACKED_PERMISSION_KEYS_LOCK_SECONDS = 10;
26+
27+
private const TRACKED_PERMISSION_KEYS_LOCK_WAIT_SECONDS = 5;
28+
2329
/**
2430
* Get cached permissions for a user
2531
*/
@@ -174,11 +180,13 @@ public function invalidateGroupPermissions(int $groupId): void
174180
public function clearAll(): void
175181
{
176182
try {
177-
foreach ($this->getTrackedPermissionKeys() as $key) {
178-
Cache::forget($key);
179-
}
183+
$this->withTrackedPermissionKeysLock(function (): void {
184+
foreach ($this->getTrackedPermissionKeysFromCache() as $key) {
185+
Cache::forget($key);
186+
}
180187

181-
Cache::forget(self::TRACKED_PERMISSION_KEYS);
188+
Cache::forget(self::TRACKED_PERMISSION_KEYS);
189+
});
182190
} catch (\Exception $e) {
183191
Log::warning('Failed to clear all permission caches: ' . $e->getMessage());
184192
}
@@ -241,39 +249,55 @@ public function getCacheStats(): array
241249
*/
242250
private function trackPermissionKey(string $key): void
243251
{
244-
$keys = $this->getTrackedPermissionKeys();
245-
if (!in_array($key, $keys, true)) {
246-
$keys[] = $key;
247-
Cache::forever(self::TRACKED_PERMISSION_KEYS, $keys);
248-
}
252+
$this->withTrackedPermissionKeysLock(function () use ($key): void {
253+
$keys = $this->getTrackedPermissionKeysFromCache();
254+
255+
if (!in_array($key, $keys, true)) {
256+
$keys[] = $key;
257+
Cache::forever(self::TRACKED_PERMISSION_KEYS, $keys);
258+
}
259+
});
249260
}
250261

251262
/**
252263
* Stop tracking a permission key after explicit invalidation.
253264
*/
254265
private function untrackPermissionKey(string $key): void
255266
{
256-
$keys = array_values(array_filter(
257-
$this->getTrackedPermissionKeys(),
258-
fn ($trackedKey) => $trackedKey !== $key
259-
));
267+
$this->withTrackedPermissionKeysLock(function () use ($key): void {
268+
$keys = array_values(array_filter(
269+
$this->getTrackedPermissionKeysFromCache(),
270+
fn ($trackedKey) => $trackedKey !== $key
271+
));
260272

261-
if (empty($keys)) {
262-
Cache::forget(self::TRACKED_PERMISSION_KEYS);
273+
if (empty($keys)) {
274+
Cache::forget(self::TRACKED_PERMISSION_KEYS);
263275

264-
return;
265-
}
276+
return;
277+
}
266278

267-
Cache::forever(self::TRACKED_PERMISSION_KEYS, $keys);
279+
Cache::forever(self::TRACKED_PERMISSION_KEYS, $keys);
280+
});
268281
}
269282

270283
/**
271-
* Read the tracked permission keys index.
284+
* Read the tracked permission keys index without locking.
272285
*/
273-
private function getTrackedPermissionKeys(): array
286+
private function getTrackedPermissionKeysFromCache(): array
274287
{
275288
$keys = Cache::get(self::TRACKED_PERMISSION_KEYS, []);
276289

277290
return is_array($keys) ? $keys : [];
278291
}
292+
293+
/**
294+
* Serialize tracked-key mutations so concurrent warmups do not drop entries.
295+
*/
296+
private function withTrackedPermissionKeysLock(callable $callback): mixed
297+
{
298+
return Cache::lock(
299+
self::TRACKED_PERMISSION_KEYS_LOCK,
300+
self::TRACKED_PERMISSION_KEYS_LOCK_SECONDS
301+
)->block(self::TRACKED_PERMISSION_KEYS_LOCK_WAIT_SECONDS, $callback);
302+
}
279303
}

‎tests/Feature/PermissionCacheInvalidationTest.php‎

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@
44

55
use Illuminate\Foundation\Testing\RefreshDatabase;
66
use Illuminate\Support\Facades\Cache;
7+
use Illuminate\Support\Facades\Hash;
8+
use Illuminate\Support\Facades\Redis;
9+
use Laravel\Passport\Passport;
10+
use ProcessMaker\Models\Group;
11+
use ProcessMaker\Models\GroupMember;
712
use ProcessMaker\Models\Permission;
813
use ProcessMaker\Models\User;
914
use ProcessMaker\Services\PermissionServiceManager;
@@ -23,7 +28,7 @@ protected function setUp(): void
2328
// Ensure the user is created by the trait
2429
if (!$this->user) {
2530
$this->user = User::factory()->create([
26-
'password' => \Illuminate\Support\Facades\Hash::make('password'),
31+
'password' => Hash::make('password'),
2732
'is_administrator' => true,
2833
]);
2934
}
@@ -114,4 +119,65 @@ public function test_permission_cache_is_invalidated_when_user_permissions_remov
114119
$this->assertContains('permission-1', $freshPermissions);
115120
$this->assertNotContains('permission-2', $freshPermissions);
116121
}
122+
123+
public function test_group_permission_update_does_not_logout_redis_backed_session()
124+
{
125+
$this->ensureRedisSessionAndCacheAreAvailable();
126+
127+
$originalPermission = Permission::factory()->create(['name' => 'redis-group-permission']);
128+
Permission::factory()->create(['name' => 'redis-group-permission-updated']);
129+
$group = Group::factory()->create(['name' => 'Redis Permission Group']);
130+
$affectedUser = User::factory()->create([
131+
'password' => Hash::make('password'),
132+
'is_administrator' => false,
133+
]);
134+
135+
GroupMember::factory()->create([
136+
'group_id' => $group->id,
137+
'member_type' => User::class,
138+
'member_id' => $affectedUser->id,
139+
]);
140+
141+
$group->permissions()->sync([$originalPermission->id]);
142+
143+
$this->permissionService->warmUpUserCache($affectedUser->id);
144+
145+
$cachedPermissions = Cache::get("user_permissions:{$affectedUser->id}");
146+
$this->assertNotNull($cachedPermissions);
147+
$this->assertContains('redis-group-permission', $cachedPermissions);
148+
149+
$this->actingAs($this->user, 'web')
150+
->post(route('keep-alive'))
151+
->assertNoContent();
152+
153+
Passport::actingAs($this->user);
154+
155+
$this->json('PUT', '/api/1.0/permissions', [
156+
'group_id' => $group->id,
157+
'permission_names' => ['redis-group-permission-updated'],
158+
])->assertNoContent();
159+
160+
$this->post(route('keep-alive'))->assertNoContent();
161+
$this->assertAuthenticatedAs($this->user, 'web');
162+
163+
$freshPermissions = $this->permissionService->getUserPermissions($affectedUser->id);
164+
$this->assertContains('redis-group-permission-updated', $freshPermissions);
165+
$this->assertNotContains('redis-group-permission', $freshPermissions);
166+
}
167+
168+
private function ensureRedisSessionAndCacheAreAvailable(): void
169+
{
170+
config()->set('cache.default', 'redis');
171+
config()->set('session.driver', 'redis');
172+
config()->set('session.connection', 'default');
173+
174+
try {
175+
Redis::connection('default')->ping();
176+
Redis::connection('cache')->ping();
177+
} catch (\Throwable $e) {
178+
$this->markTestSkipped(
179+
'Redis is not available for the permission cache invalidation regression test: ' . $e->getMessage()
180+
);
181+
}
182+
}
117183
}

‎tests/Model/UserTest.php‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,4 +130,19 @@ public function testAddCategoryViewPermissions()
130130
$testFor('screen', 'screens');
131131
$testFor('script', 'scripts');
132132
}
133+
134+
public function testRefreshInvalidatesBothPermissionCacheFamilies()
135+
{
136+
$user = User::factory()->create(['password' => Hash::make('password')]);
137+
138+
Cache::put("user_permissions:{$user->id}", ['cached'], 3600);
139+
Cache::put("user_{$user->id}_permissions", ['legacy'], 3600);
140+
Cache::put('unrelated-cache-key', 'keep-me', 3600);
141+
142+
$user->refresh();
143+
144+
$this->assertNull(Cache::get("user_permissions:{$user->id}"));
145+
$this->assertNull(Cache::get("user_{$user->id}_permissions"));
146+
$this->assertSame('keep-me', Cache::get('unrelated-cache-key'));
147+
}
133148
}

‎tests/unit/ProcessMaker/Services/PermissionCacheServiceTest.php‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,42 @@ public function test_clear_all_clears_legacy_user_permission_cache_only_when_tra
203203
$this->assertSame('keep-me', Cache::get('unrelated-cache-key'));
204204
}
205205

206+
/**
207+
* Test that tracked permission keys include every managed cache entry.
208+
*/
209+
public function test_tracked_permission_keys_include_all_managed_cache_entries()
210+
{
211+
$this->cacheService->cacheUserPermissions($this->userId, $this->userPermissions);
212+
$this->cacheService->putLegacyUserPermissions($this->userId, $this->userPermissions, 3600);
213+
$this->cacheService->cacheGroupPermissions($this->groupId, $this->groupPermissions);
214+
215+
$trackedKeys = Cache::get('permission_cache_keys');
216+
217+
$this->assertIsArray($trackedKeys);
218+
$this->assertContains("user_permissions:{$this->userId}", $trackedKeys);
219+
$this->assertContains("user_{$this->userId}_permissions", $trackedKeys);
220+
$this->assertContains("group_permissions:{$this->groupId}", $trackedKeys);
221+
}
222+
223+
/**
224+
* Test that tracked permission keys are pruned when a user cache is invalidated.
225+
*/
226+
public function test_invalidate_user_permissions_removes_only_user_keys_from_tracked_index()
227+
{
228+
$this->cacheService->cacheUserPermissions($this->userId, $this->userPermissions);
229+
$this->cacheService->putLegacyUserPermissions($this->userId, $this->userPermissions, 3600);
230+
$this->cacheService->cacheGroupPermissions($this->groupId, $this->groupPermissions);
231+
232+
$this->cacheService->invalidateUserPermissions($this->userId);
233+
234+
$trackedKeys = Cache::get('permission_cache_keys');
235+
236+
$this->assertIsArray($trackedKeys);
237+
$this->assertNotContains("user_permissions:{$this->userId}", $trackedKeys);
238+
$this->assertNotContains("user_{$this->userId}_permissions", $trackedKeys);
239+
$this->assertContains("group_permissions:{$this->groupId}", $trackedKeys);
240+
}
241+
206242
/**
207243
* Test that cache keys are generated correctly
208244
*/

0 commit comments

Comments
 (0)