diff --git a/config/areas/users/dialogs.php b/config/areas/users/dialogs.php index 1f1da96561..23859947b3 100644 --- a/config/areas/users/dialogs.php +++ b/config/areas/users/dialogs.php @@ -39,7 +39,7 @@ 'translation' => Field::translation([ 'required' => true ]), - 'role' => Field::role([ + 'role' => Field::role(props: [ 'required' => true ]) ], @@ -228,7 +228,7 @@ 'component' => 'k-form-dialog', 'props' => [ 'fields' => [ - 'role' => Field::role([ + 'role' => Field::role($user, [ 'label' => I18n::translate('user.changeRole.select'), 'required' => true, ]) diff --git a/src/Cms/User.php b/src/Cms/User.php index 6bc3ac4ce4..09d74d1d9d 100644 --- a/src/Cms/User.php +++ b/src/Cms/User.php @@ -574,33 +574,27 @@ public function role(): Role } /** - * Returns all available roles - * for this user, that can be selected - * by the authenticated user + * Returns all available roles for this user, + * that can be selected by the authenticated user */ public function roles(): Roles { $kirby = $this->kirby(); $roles = $kirby->roles(); - // a collection with just the one role of the user - $myRole = $roles->filter('id', $this->role()->id()); - - // if there's an authenticated user … - // admin users can select pretty much any role - if ($kirby->user()?->isAdmin() === true) { - // except if the user is the last admin - if ($this->isLastAdmin() === true) { - // in which case they have to stay admin - return $myRole; - } + // for the last admin, only their current role (admin) is available + if ($this->isLastAdmin() === true) { + // a collection with just the one role of the user + return $roles->filter('id', $this->role()->id()); + } - // return all roles for mighty admins - return $roles; + // exclude the admin role, if the user + // is not allowed to change role to admin + if ($kirby->user()?->isAdmin() !== true) { + $roles = $roles->filter(fn ($role) => $role->name() !== 'admin'); } - // any other user can only keep their role - return $myRole; + return $roles; } /** diff --git a/src/Cms/UserPermissions.php b/src/Cms/UserPermissions.php index 62cd6625cb..64fc01c7c0 100644 --- a/src/Cms/UserPermissions.php +++ b/src/Cms/UserPermissions.php @@ -26,6 +26,19 @@ public function __construct(User $model) protected function canChangeRole(): bool { + // protect admin from role changes by non-admin + if ( + $this->model->isAdmin() === true && + $this->user?->isAdmin() !== true + ) { + return false; + } + + // prevent demoting the last admin + if ($this->model->isLastAdmin() === true) { + return false; + } + return $this->model->roles()->count() > 1; } diff --git a/src/Cms/UserRules.php b/src/Cms/UserRules.php index cf326650c7..2cdd7e8cec 100644 --- a/src/Cms/UserRules.php +++ b/src/Cms/UserRules.php @@ -101,36 +101,25 @@ public static function changePassword( */ public static function changeRole(User $user, string $role): bool { - // protect admin from role changes by non-admin - if ( - $user->kirby()->user()->isAdmin() === false && - $user->isAdmin() === true - ) { - throw new PermissionException([ - 'key' => 'user.changeRole.permission', + // prevent demoting the last admin + if ($role !== 'admin' && $user->isLastAdmin() === true) { + throw new LogicException([ + 'key' => 'user.changeRole.lastAdmin', 'data' => ['name' => $user->username()] ]); } - // prevent non-admins making a user to admin + // prevent non-admins promoting a user to the admin role if ( $user->kirby()->user()->isAdmin() === false && $role === 'admin' ) { throw new PermissionException([ - 'key' => 'user.changeRole.toAdmin' - ]); - } - - static::validRole($user, $role); - - if ($role !== 'admin' && $user->isLastAdmin() === true) { - throw new LogicException([ - 'key' => 'user.changeRole.lastAdmin', - 'data' => ['name' => $user->username()] + 'key' => 'user.changeRole.toAdmin' ]); } + // check permissions if ($user->permissions()->changeRole() !== true) { throw new PermissionException([ 'key' => 'user.changeRole.permission', @@ -138,6 +127,9 @@ public static function changeRole(User $user, string $role): bool ]); } + // prevent changing to role that is not available for user + static::validRole($user, $role); + return true; } @@ -373,7 +365,7 @@ public static function validPassword( */ public static function validRole(User $user, string $role): bool { - if ($user->kirby()->roles()->find($role) instanceof Role) { + if ($user->roles()->find($role) instanceof Role) { return true; } diff --git a/src/Panel/Field.php b/src/Panel/Field.php index e89571216a..c0ca7c31b9 100644 --- a/src/Panel/Field.php +++ b/src/Panel/Field.php @@ -6,6 +6,7 @@ use Kirby\Cms\File; use Kirby\Cms\ModelWithContent; use Kirby\Cms\Page; +use Kirby\Cms\User; use Kirby\Form\Form; use Kirby\Http\Router; use Kirby\Toolkit\I18n; @@ -191,30 +192,30 @@ public static function password(array $props = []): array /** * User role radio buttons */ - public static function role(array $props = []): array - { - $kirby = App::instance(); - $isAdmin = $kirby->user()?->isAdmin() ?? false; - $roles = []; - - foreach ($kirby->roles() as $role) { - // exclude the admin role, if the user - // is not allowed to change role to admin - if ($role->name() === 'admin' && $isAdmin === false) { - continue; - } - - $roles[] = [ - 'text' => $role->title(), - 'info' => $role->description() ?? I18n::translate('role.description.placeholder'), - 'value' => $role->name() - ]; - } + public static function role( + User|null $user = null, + array $props = [] + ): array { + $kirby = App::instance(); + $roles = $user?->roles(); + + // if this role field is not for any specific user (but e.g. a new one), + // get all roles but filter out the admin role + // if the current user is no admin + $roles ??= $kirby->roles()->filter( + fn ($role) => $role->name() !== 'admin' || $kirby->user()?->isAdmin() === true + ); + + $roles = $roles->values(fn ($role) => [ + 'text' => $role->title(), + 'info' => $role->description() ?? I18n::translate('role.description.placeholder'), + 'value' => $role->name() + ]); return array_merge([ - 'label' => I18n::translate('role'), - 'type' => count($roles) <= 1 ? 'hidden' : 'radio', - 'options' => $roles + 'label' => I18n::translate('role'), + 'type' => count($roles) <= 1 ? 'hidden' : 'radio', + 'options' => $roles ], $props); } diff --git a/tests/Cms/Users/UserPermissionsTest.php b/tests/Cms/Users/UserPermissionsTest.php index 9c2f87e51f..eee8b42c2f 100644 --- a/tests/Cms/Users/UserPermissionsTest.php +++ b/tests/Cms/Users/UserPermissionsTest.php @@ -4,6 +4,9 @@ use Kirby\TestCase; +/** + * @coversDefaultClass \Kirby\Cms\UserPermissions + */ class UserPermissionsTest extends TestCase { public static function actionProvider(): array @@ -120,20 +123,131 @@ public function testWithNoAdmin($action) $this->assertFalse($perms->can($action)); } - public function testChangeSingleRole() + /** + * @covers ::canChangeRole + */ + public function testChangeRole() { - new App([ + // admin to change role of another admin + $app = new App([ 'roots' => [ 'index' => '/dev/null' ], 'roles' => [ - ['name' => 'admin'] - ] + ['name' => 'admin'], + ['name' => 'editor'] + ], + 'user' => 'another-admin@getkirby.com', + 'users' => [ + [ + 'email' => 'admin@getkirby.com', + 'role' => 'admin' + ], + [ + 'email' => 'another-admin@getkirby.com', + 'role' => 'admin' + ] + ], ]); - $user = new User(['email' => 'test@getkirby.com']); - $perms = $user->permissions(); + $user = $app->user('admin@getkirby.com'); + $this->assertTrue($user->permissions()->can('changeRole')); + + // non-admin to change role of an admin + $app = new App([ + 'roots' => [ + 'index' => '/dev/null' + ], + 'roles' => [ + ['name' => 'admin'], + ['name' => 'editor'] + ], + 'user' => 'editor@getkirby.com', + 'users' => [ + [ + 'email' => 'admin@getkirby.com', + 'role' => 'admin' + ], + [ + 'email' => 'editor@getkirby.com', + 'role' => 'editor' + ] + ], + ]); + + $user = $app->user('admin@getkirby.com'); + $this->assertFalse($user->permissions()->can('changeRole')); + + // change role of last admin + $app = new App([ + 'roots' => [ + 'index' => '/dev/null' + ], + 'roles' => [ + ['name' => 'admin'], + ['name' => 'editor'] + ], + 'user' => 'admin@getkirby.com', + 'users' => [ + [ + 'email' => 'admin@getkirby.com', + 'role' => 'admin' + ] + ], + ]); + + $user = $app->user('admin@getkirby.com'); + $this->assertFalse($user->permissions()->can('changeRole')); + + // change role if only one role is available + $app = new App([ + 'roots' => [ + 'index' => '/dev/null' + ], + 'roles' => [ + ['name' => 'admin'], + ['name' => 'editor'] + ], + 'user' => 'editor@getkirby.com', + 'users' => [ + [ + 'email' => 'admin@getkirby.com', + 'role' => 'admin' + ], + [ + 'email' => 'editor@getkirby.com', + 'role' => 'editor' + ] + ], + ]); + + $user = $app->user('editor@getkirby.com'); + $this->assertFalse($user->permissions()->can('changeRole')); + + // change role if multiple roles are available + $app = new App([ + 'roots' => [ + 'index' => '/dev/null' + ], + 'roles' => [ + ['name' => 'admin'], + ['name' => 'editor'], + ['name' => 'guest'] + ], + 'user' => 'editor@getkirby.com', + 'users' => [ + [ + 'email' => 'admin@getkirby.com', + 'role' => 'admin' + ], + [ + 'email' => 'editor@getkirby.com', + 'role' => 'editor' + ] + ], + ]); - $this->assertFalse($perms->can('changeRole')); + $user = $app->user('editor@getkirby.com'); + $this->assertTrue($user->permissions()->can('changeRole')); } } diff --git a/tests/Cms/Users/UserRulesTest.php b/tests/Cms/Users/UserRulesTest.php index 57753bfbd5..062d772168 100644 --- a/tests/Cms/Users/UserRulesTest.php +++ b/tests/Cms/Users/UserRulesTest.php @@ -8,6 +8,9 @@ use Kirby\Exception\LogicException; use Kirby\Exception\PermissionException; +/** + * @coversDefaultClass \Kirby\Cms\UserRules + */ class UserRulesTest extends TestCase { public const FIXTURES = __DIR__ . '/fixtures'; @@ -116,118 +119,131 @@ public function testChangeEmailDuplicate() UserRules::changeEmail($kirby->user('user@domain.com'), 'admin@domain.com'); } - public function testChangeRoleWithoutPermissions() + /** + * @covers ::changeRole + */ + public function testChangeRoleDemoteLastAdmin() { - $kirby = new App([ + // demoting last admin + $app = new App([ 'roots' => [ - 'site' => static::FIXTURES, + 'index' => '/dev/null' ], - 'user' => 'admin@domain.com', + 'roles' => [ + ['name' => 'admin'], + ['name' => 'editor'] + ], + 'user' => 'admin@getkirby.com', 'users' => [ - ['email' => 'editor@domain.com', 'role' => 'admin'], - ['email' => 'admin@domain.com', 'role' => 'admin'] - ] + [ + 'email' => 'admin@getkirby.com', + 'role' => 'admin' + ] + ], ]); - $kirby->impersonate('admin@domain.com'); - - $permissions = $this->createMock(UserPermissions::class); - $permissions->method('__call')->with('changeRole')->willReturn(false); - - $user = $this->createMock(User::class); - $user->method('kirby')->willReturn($kirby); - $user->method('permissions')->willReturn($permissions); - $user->method('username')->willReturn('test'); - - $this->expectException(PermissionException::class); - $this->expectExceptionMessage('You are not allowed to change the role for the user "test"'); + $this->expectException(LogicException::class); + $this->expectExceptionCode('error.user.changeRole.lastAdmin'); - UserRules::changeRole($user, 'admin'); + $user = $app->user('admin@getkirby.com'); + UserRules::changeRole($user, 'editor'); } - public function testChangeRoleFromAdminByAdmin() + /** + * @covers ::changeRole + */ + public function testChangeRolePromoteToAdmin() { - $kirby = new App([ + $app = new App([ 'roots' => [ - 'site' => static::FIXTURES, + 'index' => '/dev/null' + ], + 'roles' => [ + ['name' => 'admin'], + ['name' => 'editor'] ], - 'user' => 'admin@domain.com', 'users' => [ - ['email' => 'user@domain.com', 'role' => 'admin'], - ['email' => 'admin@domain.com', 'role' => 'admin'] - ] + [ + 'email' => 'admin@getkirby.com', + 'role' => 'admin' + ], + [ + 'email' => 'a@getkirby.com', + 'role' => 'editor' + ], + [ + 'email' => 'b@getkirby.com', + 'role' => 'editor' + ] + ], ]); - $kirby->impersonate('admin@domain.com'); - $this->assertTrue(UserRules::changeRole($kirby->user('user@domain.com'), 'editor')); - } + // by admin + $app->impersonate('admin@getkirby.com'); + $user = $app->user('b@getkirby.com'); + $this->assertTrue(UserRules::changeRole($user, 'admin')); - public function testChangeRoleFromAdminByNonAdmin() - { + // by non-admin $this->expectException(PermissionException::class); - $this->expectExceptionCode('error.user.changeRole.permission'); - - $kirby = new App([ - 'roots' => [ - 'site' => static::FIXTURES, - ], - 'user' => 'user@domain.com', - 'users' => [ - ['email' => 'user@domain.com', 'role' => 'editor'], - ['email' => 'admin@domain.com', 'role' => 'admin'] - ] - ]); - $kirby->impersonate('user@domain.com'); + $this->expectExceptionCode('error.user.changeRole.toAdmin'); - UserRules::changeRole($kirby->user('admin@domain.com'), 'editor'); + $app->impersonate('a@getkirby.com'); + $user = $app->user('b@getkirby.com'); + UserRules::changeRole($user, 'admin'); } - public function testChangeRoleToAdminByAdmin() + /** + * @covers ::changeRole + */ + public function testChangeRolePermissions() { - $kirby = new App([ + $app = new App([ 'roots' => [ - 'site' => static::FIXTURES, + 'index' => '/dev/null' ], - 'user' => 'user1@domain.com', - 'users' => [ - ['email' => 'user1@domain.com', 'role' => 'admin'], - ['email' => 'user2@domain.com', 'role' => 'editor'] - ] - ]); - $kirby->impersonate('user1@domain.com'); - - $this->assertTrue(UserRules::changeRole($kirby->user('user2@domain.com'), 'admin')); - } - - public function testChangeRoleToAdminByNonAdmin() - { - $this->expectException(PermissionException::class); - $this->expectExceptionCode('error.user.changeRole.toAdmin'); - - $kirby = new App([ - 'roots' => [ - 'site' => static::FIXTURES, + 'roles' => [ + [ + 'name' => 'manager', + 'permissions' => [ + 'users' => [ + 'changeRole' => true, + ] + ] + ], + [ + 'name' => 'editor', + 'permissions' => [ + 'users' => [ + 'changeRole' => false, + ] + ] + ] ], - 'user' => 'user1@domain.com', 'users' => [ - ['email' => 'user1@domain.com', 'role' => 'editor'], - ['email' => 'user2@domain.com', 'role' => 'editor'] - ] + [ + 'email' => 'manager@getkirby.com', + 'role' => 'manager' + ], + [ + 'email' => 'editor@getkirby.com', + 'role' => 'editor' + ] + ], ]); - $kirby->impersonate('user1@domain.com'); - UserRules::changeRole($kirby->user('user2@domain.com'), 'admin'); - } + // with permission + $app->impersonate('manager@getkirby.com'); + $user = $app->user('editor@getkirby.com'); + $this->assertTrue(UserRules::changeRole($user, 'manager')); - public function testChangeRoleLastAdmin() - { - $this->expectException(LogicException::class); - $this->expectExceptionCode('error.user.changeRole.lastAdmin'); - - $kirby = $this->appWithAdmin(); - $kirby->impersonate('admin@domain.com'); + // without permission + $this->expectException(PermissionException::class); + $this->expectExceptionCode('error.user.changeRole.permission'); + $this->expectExceptionMessage('You are not allowed to change the role for the user "manager@getkirby.com"'); - UserRules::changeRole($kirby->user('admin@domain.com'), 'editor'); + $app->impersonate('editor@getkirby.com'); + $user = $app->user('manager@getkirby.com'); + UserRules::changeRole($user, 'editor'); } public function testChangeTotp() diff --git a/tests/Cms/Users/UserTest.php b/tests/Cms/Users/UserTest.php index e57ed2382a..1bf8b957cc 100644 --- a/tests/Cms/Users/UserTest.php +++ b/tests/Cms/Users/UserTest.php @@ -341,6 +341,49 @@ public static function passwordProvider(): array ]; } + /** + * @covers ::roles + */ + public function testRoles(): void + { + $app = new App([ + 'roots' => [ + 'index' => '/dev/null' + ], + 'roles' => [ + ['name' => 'admin'], + ['name' => 'editor'], + ['name' => 'guest'] + ], + 'users' => [ + [ + 'email' => 'admin@getkirby.com', + 'role' => 'admin' + ], + [ + 'email' => 'editor@getkirby.com', + 'role' => 'editor' + ] + ], + ]); + + // last admin has only admin role as option + $user = $app->user('admin@getkirby.com'); + $roles = $user->roles()->values(fn ($role) => $role->id()); + $this->assertSame(['admin'], $roles); + + // normal user should not have admin as option + $user = $app->user('editor@getkirby.com'); + $roles = $user->roles()->values(fn ($role) => $role->id()); + $this->assertSame(['editor', 'guest'], $roles); + + // only if current user is admin, normal user can also have admin option + $app->impersonate('admin@getkirby.com'); + $user = $app->user('editor@getkirby.com'); + $roles = $user->roles()->values(fn ($role) => $role->id()); + $this->assertSame(['admin', 'editor', 'guest'], $roles); + } + public function testSecret() { $app = new App([ diff --git a/tests/Panel/FieldTest.php b/tests/Panel/FieldTest.php index 4c8f397c73..242db8249f 100644 --- a/tests/Panel/FieldTest.php +++ b/tests/Panel/FieldTest.php @@ -219,78 +219,113 @@ public function testPassword(): void */ public function testRole(): void { - $field = Field::role(); - $expected = [ - 'label' => 'Role', - 'type' => 'hidden', - 'options' => [] - ]; - - $this->assertSame($expected, $field); - - // without authenticated user $this->app = $this->app->clone([ - 'blueprints' => [ - 'users/admin' => [ - 'name' => 'admin', - 'title' => 'Admin', - 'description' => 'Admin description' - ], - 'users/editor' => [ - 'name' => 'editor', - 'title' => 'Editor', - 'description' => 'Editor description' + 'roles' => [ + ['name' => 'admin'], + ['name' => 'editor'], + ['name' => 'guest'] + ], + 'users' => [ + [ + 'email' => 'admin@getkirby.com', + 'role' => 'admin' ], - 'users/client' => [ - 'name' => 'client', - 'title' => 'Client' + [ + 'email' => 'editor@getkirby.com', + 'role' => 'editor' ] - ] + ], ]); - $field = Field::role(); + + // pass user + $this->app->impersonate('admin@getkirby.com'); + $user = $this->app->user('editor@getkirby.com'); + $field = Field::role($user); $expected = [ 'label' => 'Role', 'type' => 'radio', 'options' => [ [ - 'text' => 'Client', - 'info' => 'No description', - 'value' => 'client' + 'text' => 'Admin', + 'info' => 'No description', + 'value' => 'admin' ], [ - 'text' => 'Editor', - 'info' => 'Editor description', + 'text' => 'Editor', + 'info' => 'No description', 'value' => 'editor' ], + [ + 'text' => 'Guest', + 'info' => 'No description', + 'value' => 'guest' + ] ] ]; $this->assertSame($expected, $field); - // with authenticated admin - $this->app->impersonate('kirby'); - + // pass no user $field = Field::role(); $expected = [ 'label' => 'Role', 'type' => 'radio', 'options' => [ [ - 'text' => 'Admin', - 'info' => 'Admin description', + 'text' => 'Admin', + 'info' => 'No description', 'value' => 'admin' ], [ - 'text' => 'Client', - 'info' => 'No description', - 'value' => 'client' + 'text' => 'Editor', + 'info' => 'No description', + 'value' => 'editor' ], [ - 'text' => 'Editor', - 'info' => 'Editor description', + 'text' => 'Guest', + 'info' => 'No description', + 'value' => 'guest' + ] + ] + ]; + + $this->assertSame($expected, $field); + + // pass no user, but current user is not an admin + $this->app->impersonate('editor@getkirby.com'); + $field = Field::role(); + $expected = [ + 'label' => 'Role', + 'type' => 'radio', + 'options' => [ + [ + 'text' => 'Editor', + 'info' => 'No description', 'value' => 'editor' ], + [ + 'text' => 'Guest', + 'info' => 'No description', + 'value' => 'guest' + ] + ] + ]; + + $this->assertSame($expected, $field); + + // last admin + $user = $this->app->user('admin@getkirby.com'); + $field = Field::role($user); + $expected = [ + 'label' => 'Role', + 'type' => 'hidden', + 'options' => [ + [ + 'text' => 'Admin', + 'info' => 'No description', + 'value' => 'admin' + ] ] ];