diff --git a/src/Http/Controllers/CP/Users/RolesController.php b/src/Http/Controllers/CP/Users/RolesController.php index 80fa2da285d..25ba6eddccb 100644 --- a/src/Http/Controllers/CP/Users/RolesController.php +++ b/src/Http/Controllers/CP/Users/RolesController.php @@ -155,8 +155,8 @@ public function update(Request $request, $role) if ($request->super && User::current()->isSuper()) { $role->permissions(['super']); - } elseif (! in_array('super', $request->permissions ?? [])) { - $role->permissions($request->permissions); + } elseif ($request->has('permissions') && ! in_array('super', $request->permissions)) { + $role->permissions($this->preserveUnregisteredPermissions($role, $request->permissions)); } $role->save(); @@ -179,6 +179,17 @@ public function destroy($role) return response('', 204); } + private function preserveUnregisteredPermissions($role, $permissions) + { + $registered = Permission::boot()->flattened()->map->value(); + + $unregistered = $role->permissions() + ->diff($registered) + ->reject(fn ($permission) => $permission === 'super'); + + return collect($permissions)->merge($unregistered)->unique()->values()->all(); + } + protected function updateTree($tree, $role = null) { return $tree->map(function ($group) use ($role) { diff --git a/tests/Feature/Roles/UpdateRoleTest.php b/tests/Feature/Roles/UpdateRoleTest.php index f0da48eae5f..8355d745497 100644 --- a/tests/Feature/Roles/UpdateRoleTest.php +++ b/tests/Feature/Roles/UpdateRoleTest.php @@ -78,7 +78,7 @@ public function it_updates_a_role() $role = tap( Role::make('test') ->title('Test') - ->permissions(['one', 'two']) + ->permissions(['configure fields', 'manage preferences']) )->save(); $this @@ -87,7 +87,7 @@ public function it_updates_a_role() ->update($role, [ 'title' => 'Updated', 'handle' => 'changed', - 'permissions' => ['one', 'three'], + 'permissions' => ['configure fields', 'resolve duplicate ids'], ]) ->assertOk() ->assertJson(['redirect' => cp_route('roles.index')]); @@ -95,7 +95,53 @@ public function it_updates_a_role() $this->assertNull(Role::find('test')); $role = Role::find('changed'); $this->assertEquals('Updated', $role->title()); - $this->assertEquals(['one', 'three'], $role->permissions()->all()); + $this->assertEquals(['configure fields', 'resolve duplicate ids'], $role->permissions()->all()); + $this->assertFalse($role->isSuper()); + } + + #[Test] + public function it_preserves_permissions_that_are_no_longer_registered() + { + // A permission that used to be registered, but no longer is. For example, one + // belonging to an addon that's been disabled, or a deprecated core permission. + $role = tap( + Role::make('test') + ->title('Test') + ->permissions(['configure fields', 'manage preferences', 'do addon things']) + )->save(); + + $this + ->actingAsUserWithPermissions(['edit roles']) + ->withActiveElevatedSession() + ->update($role, [ + 'permissions' => ['configure fields'], + ]) + ->assertOk(); + + $role = Role::find('test'); + $this->assertEquals(['configure fields', 'do addon things'], $role->permissions()->all()); + } + + #[Test] + public function demoting_a_super_role_does_not_resurrect_the_super_permission() + { + $role = tap( + Role::make('test') + ->title('Test') + ->permissions(['super']) + )->save(); + + $this + ->actingAs(tap(User::make()->makeSuper())->save()) + ->withActiveElevatedSession() + ->update($role, [ + 'super' => false, + 'permissions' => [], + ]) + ->assertOk(); + + $role = Role::find('test'); + $this->assertEquals([], $role->permissions()->all()); $this->assertFalse($role->isSuper()); }