Skip to content

Commit

Permalink
Fix changeRole permission handling
Browse files Browse the repository at this point in the history 
Fixes #5146
  • Loading branch information
@distantnative
distantnative committed Aug 31, 2024
1 parent 812be25 commit abce099
Show file tree
Hide file tree
Showing 9 changed files with 399 additions and 191 deletions.
4 changes: 2 additions & 2 deletions config/areas/users/dialogs.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
'translation' => Field::translation([
'required' => true
]),
'role' => Field::role([
'role' => Field::role(props: [
'required' => true
])
],
Expand Down Expand Up @@ -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,
])
Expand Down
30 changes: 12 additions & 18 deletions src/Cms/User.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down
13 changes: 13 additions & 0 deletions src/Cms/UserPermissions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
30 changes: 11 additions & 19 deletions src/Cms/UserRules.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,43 +101,35 @@ 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',
'data' => ['name' => $user->username()]
]);
}

// prevent changing to role that is not available for user
static::validRole($user, $role);

return true;
}

Expand Down Expand Up @@ -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;
}

Expand Down
45 changes: 23 additions & 22 deletions src/Panel/Field.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down
128 changes: 121 additions & 7 deletions tests/Cms/Users/UserPermissionsTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

use Kirby\TestCase;

/**
* @coversDefaultClass \Kirby\Cms\UserPermissions
*/
class UserPermissionsTest extends TestCase
{
public static function actionProvider(): array
Expand Down Expand Up @@ -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' => '[email protected]',
'users' => [
[
'email' => '[email protected]',
'role' => 'admin'
],
[
'email' => '[email protected]',
'role' => 'admin'
]
],
]);

$user = new User(['email' => '[email protected]']);
$perms = $user->permissions();
$user = $app->user('[email protected]');
$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' => '[email protected]',
'users' => [
[
'email' => '[email protected]',
'role' => 'admin'
],
[
'email' => '[email protected]',
'role' => 'editor'
]
],
]);

$user = $app->user('[email protected]');
$this->assertFalse($user->permissions()->can('changeRole'));

// change role of last admin
$app = new App([
'roots' => [
'index' => '/dev/null'
],
'roles' => [
['name' => 'admin'],
['name' => 'editor']
],
'user' => '[email protected]',
'users' => [
[
'email' => '[email protected]',
'role' => 'admin'
]
],
]);

$user = $app->user('[email protected]');
$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' => '[email protected]',
'users' => [
[
'email' => '[email protected]',
'role' => 'admin'
],
[
'email' => '[email protected]',
'role' => 'editor'
]
],
]);

$user = $app->user('[email protected]');
$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' => '[email protected]',
'users' => [
[
'email' => '[email protected]',
'role' => 'admin'
],
[
'email' => '[email protected]',
'role' => 'editor'
]
],
]);

$this->assertFalse($perms->can('changeRole'));
$user = $app->user('[email protected]');
$this->assertTrue($user->permissions()->can('changeRole'));
}
}
Loading

0 comments on commit abce099

Please sign in to comment.