Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harden permission management #1651

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Rate limiting for room access code authentication ([#669], [#1617])
- Logging for room authentication ([#669], [#1617])
- Command to test email configuration ([#530], [#1618])
- Permission restrictions to prevent non-superusers from editing and deleting superusers ([#1651])
- Permission restrictions to prevent non-superusers from assigning the superuser role ([#1651])
- Environment variable for configuring restricted permissions that cannot be assigned to non-superuser roles ([#1651])
- Display raw permission names in the admin interface ([#1651])

### Changed

- The recording import task is now prevented from running until the previous run has finished ([#1484], [#1604])
- Adjust frontend tests to better check the resetting of form errors ([#1679], [#1702])
- Error handling in create room dialog ([#1704])
- Real-time input validation on create superuser command ([#1651])

### Fixed

Expand Down Expand Up @@ -308,6 +313,7 @@ You can find the changelog for older versions there [here](https://github.com/TH
[#1617]: https://github.com/THM-Health/PILOS/pull/1617
[#1618]: https://github.com/THM-Health/PILOS/pull/1618
[#1679]: https://github.com/THM-Health/PILOS/issues/1679
[#1651]: https://github.com/THM-Health/PILOS/issues/1651
[#1702]: https://github.com/THM-Health/PILOS/pull/1702
[#1704]: https://github.com/THM-Health/PILOS/pull/1704
[#1721]: https://github.com/THM-Health/PILOS/issues/1721
Expand Down
45 changes: 16 additions & 29 deletions app/Console/Commands/CreateSuperuserCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@

namespace App\Console\Commands;

use App\Http\Requests\UserRequest;
use App\Models\Role;
use App\Models\User;
use App\Rules\Password;
use Illuminate\Console\Command;
use Illuminate\Support\Facades\Hash;
use Illuminate\Support\Facades\Validator;
use Illuminate\Validation\Rule;

use function Laravel\Prompts\password;
use function Laravel\Prompts\select;
use function Laravel\Prompts\text;

/**
* Command class that makes it possible to create an new superuser.
Expand Down Expand Up @@ -65,36 +69,19 @@ public function handle()
$this->info('Creating an new superuser, please notify your inputs.');

$data = [];
$data['firstname'] = $this->ask('Firstname');
$data['lastname'] = $this->ask('Lastname');
$data['email'] = $this->ask('E-Mail');
$data['user_locale'] = $this->ask('Locale (possible values: '.implode(',', array_keys(config('app.enabled_locales'))).')');
$data['password'] = $this->secret('Password');
$data['password_confirmation'] = $this->secret('Password Confirmation');
$data['generate_password'] = false;
$data['bbb_skip_check_audio'] = false;
$data['roles'] = [$superuserRole->id];
$data['timezone'] = 'UTC';

$validator = Validator::make($data, (new UserRequest)->rules());

if ($validator->fails()) {
$this->info('Something went wrong, please see the error messages below for more information.');

foreach ($validator->errors()->all() as $error) {
$this->error($error);
}

return 1;
}
$firstname = text('Firstname', validate: ['firstname' => 'required|max:255']);
$lastname = text('Lastname', validate: ['lastname' => 'required|max:255']);
$email = text('E-Mail', validate: ['email' => ['required', 'max:255', 'email', Rule::unique('users', 'email')->where('authenticator', 'local')]]);
$locale = select('Locale', array_keys(config('app.enabled_locales')));
$password = password('Password', required: true, validate: ['min:8', new Password]);

$user = new User;

$user->firstname = $data['firstname'];
$user->lastname = $data['lastname'];
$user->email = $data['email'];
$user->locale = $data['user_locale'];
$user->password = Hash::make($data['password']);
$user->firstname = $firstname;
$user->lastname = $lastname;
$user->email = $email;
$user->locale = $locale;
$user->password = Hash::make($password);
$user->email_verified_at = $user->freshTimestamp();

$user->save();
Expand Down
6 changes: 5 additions & 1 deletion app/Http/Controllers/api/v1/PermissionController.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ class PermissionController extends Controller
*/
public function index()
{
return new PermissionResourceCollection(Permission::all());
return (new PermissionResourceCollection(Permission::all()))->additional([
'meta' => [
'restrictions' => config('permissions.restrictions'),
],
]);
}
}
9 changes: 7 additions & 2 deletions app/Http/Controllers/api/v1/RoleController.php
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,10 @@ public function store(RoleRequest $request)
$role->superuser = false;

$role->save();
$role->permissions()->sync($request->permissions);

$new_permissions = $role->filterRestrictedPermissions(collect($request->permissions));

$role->permissions()->sync($new_permissions);

return (new RoleResource($role))->withPermissions();
}
Expand All @@ -98,7 +101,9 @@ public function update(RoleRequest $request, Role $role)
if (! $role->superuser) {
$old_role_permissions = $role->permissions()->pluck('permissions.id')->toArray();

$role->permissions()->sync($request->permissions);
$new_permissions = $role->filterRestrictedPermissions(collect($request->permissions));

$role->permissions()->sync($new_permissions);

$user = Auth::user();

Expand Down
5 changes: 4 additions & 1 deletion app/Http/Requests/NewUserRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace App\Http\Requests;

use App\Models\Role;
use App\Rules\Password;
use Illuminate\Foundation\Http\FormRequest;
use Illuminate\Validation\Rule;
Expand All @@ -15,14 +16,16 @@ class NewUserRequest extends FormRequest
*/
public function rules()
{
$prohibitedRoles = \Auth::user()->superuser ? [] : Role::where(['superuser' => true])->pluck('id')->toArray();

return [
'firstname' => ['required', 'string', 'max:255'],
'lastname' => ['required', 'string', 'max:255'],
'email' => ['required', 'string', 'email', 'max:255', Rule::unique('users', 'email')->where('authenticator', 'local')],
'user_locale' => ['required', 'string', Rule::in(array_keys(config('app.enabled_locales')))],
'timezone' => ['required', 'string', Rule::in(timezone_identifiers_list())],
'roles' => ['required', 'array'],
'roles.*' => ['distinct', 'integer', 'exists:App\Models\Role,id'],
'roles.*' => ['distinct', 'integer', 'exists:App\Models\Role,id', Rule::notIn($prohibitedRoles)],
'generate_password' => ['required', 'boolean'],
'new_password' => ['required_if:generate_password,false', 'string', 'min:8', 'confirmed', new Password],
];
Expand Down
5 changes: 4 additions & 1 deletion app/Http/Requests/UserRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace App\Http\Requests;

use App\Models\Role;
use Illuminate\Foundation\Http\FormRequest;
use Illuminate\Validation\Rule;

Expand All @@ -14,12 +15,14 @@ class UserRequest extends FormRequest
*/
public function rules()
{
$prohibitedRoles = \Auth::user()->superuser ? [] : Role::where(['superuser' => true])->pluck('id')->toArray();

$rules = [
'user_locale' => ['sometimes', 'required', 'string', Rule::in(array_keys(config('app.enabled_locales')))],
'bbb_skip_check_audio' => ['sometimes', 'required', 'boolean'],
'timezone' => ['sometimes', 'required', 'string', Rule::in(timezone_identifiers_list())],
'roles' => ['sometimes', 'required', 'array'],
'roles.*' => ['sometimes', 'distinct', 'integer', 'exists:App\Models\Role,id'],
'roles.*' => ['sometimes', 'distinct', 'integer', 'exists:App\Models\Role,id', Rule::notIn($prohibitedRoles)],
'image' => ['sometimes', 'nullable', 'mimes:jpg', 'dimensions:width=100,height=100'],
];

Expand Down
1 change: 1 addition & 0 deletions app/Http/Resources/RoleCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public function toArray($request)
'automatic' => $role->whenPivotLoaded('role_user', function () use ($role) {
return $role->pivot->automatic;
}),
'superuser' => $role->superuser,
];
})->toArray();
}
Expand Down
1 change: 1 addition & 0 deletions app/Http/Resources/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ public function toArray($request)
'bbb_skip_check_audio' => $this->bbb_skip_check_audio,
'initial_password_set' => $this->initial_password_set,
'timezone' => $this->timezone,
'superuser' => $this->superuser,
];
}
}
37 changes: 37 additions & 0 deletions app/Models/Role.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsToMany;
use Illuminate\Support\Collection;

/**
* Class Role
Expand Down Expand Up @@ -46,6 +47,42 @@
return $this->belongsToMany(Permission::class)->using(PermissionRole::class);
}

/**
* Filters out restricted permissions from the given collection of permission IDs.
*
* @param Collection $permissionIDs Collection of permission IDs to be filtered.
* @return Collection Filtered collection of permission IDs that are not restricted.
*/
public function filterRestrictedPermissions(Collection $permissionIDs): Collection
{
return $permissionIDs->filter(function ($permissionID) {
// Find the permission by its ID
$permission = Permission::find($permissionID);
if ($permission === null) {
return false;

Check warning on line 62 in app/Models/Role.php

View check run for this annotation

Codecov / codecov/patch

app/Models/Role.php#L62

Added line #L62 was not covered by tests
}

// Get the list of restricted permissions from the configuration
$restrictions = collect(config('permissions.restrictions'));

// Check if the permission is not in the list of restricted permissions
return $restrictions->doesntContain(function (string $restriction) use ($permission) {
// If the restriction matches the permission name, it is restricted
if ($restriction === $permission->name) {
return true;
}

// Split the restriction and permission names into groups and permissions
$restrictionGroup = explode('.', $restriction, 2)[0];
$restrictionPermission = explode('.', $restriction, 2)[1] ?? null;
$permissionGroup = explode('.', $permission->name, 2)[0];

// If the restriction applies to all permissions in the group, it is restricted
return $restrictionPermission === '*' && $restrictionGroup === $permissionGroup;
});
});
}

/**
* Types of rooms that can be used by the user of this role.
*
Expand Down
8 changes: 8 additions & 0 deletions app/Models/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -327,4 +327,12 @@ public function sendPasswordResetNotification($token)

$this->notify(new PasswordReset($token, Carbon::parse($reset->created_at)));
}

/**
* @return bool
*/
public function getSuperuserAttribute()
{
return $this->roles->where('superuser', true)->isNotEmpty();
}
}
15 changes: 15 additions & 0 deletions app/Policies/UserPolicy.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ public function create(User $user)
*/
public function update(User $user, User $model)
{
// Prevent users of the super-user role to be updated by users without the super-user role
if ($model->superuser && ! $user->superuser) {
return false;
}

return $user->can('users.update') || $model->id === $user->id;
}

Expand All @@ -57,6 +62,11 @@ public function update(User $user, User $model)
*/
public function delete(User $user, User $model)
{
// Prevent users of the super-user role to be deleted by users without the super-user role
if ($model->superuser && ! $user->superuser) {
return false;
}

return $user->can('users.delete') && $model->id !== $user->id;
}

Expand Down Expand Up @@ -102,6 +112,11 @@ public function changePassword(User $user, User $model)
*/
public function resetPassword(User $user, User $model)
{
// Prevent users of the super-user role to be deleted by users without the super-user role
if ($model->superuser && ! $user->superuser) {
return false;
}

return $model->authenticator === 'local'
&& $user->can('update', $model)
&& $model->id !== $user->id
Expand Down
5 changes: 5 additions & 0 deletions config/permissions.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php

return [
'restrictions' => explode(',', env('PERMISSION_RESTRICTIONS', '')),
];
6 changes: 3 additions & 3 deletions docs/docs/administration/03-configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ To enable external authentication methods like LDAP, please refer to the [Extern
## More config options

- [External Authentication](./08-advanced/01-external-authentication.md)
- [Recording](./08-advanced/02-recording.md)
- [Scaling](./08-advanced/03-scaling.md)
- [Greenlight Configuration](./08-advanced/04-migrate-greenlight.md)
- [Recording](./08-advanced/03-recording.md)
- [Scaling](./08-advanced/04-scaling.md)
- [Greenlight Configuration](./08-advanced/05-migrate-greenlight.md)
- [Development](../development/03-configuration.md)
2 changes: 1 addition & 1 deletion docs/docs/administration/04-pilos-cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ Build the BigBlueButton recording player with the release version you want.
docker compose exec app pilos-cli playback-player:build 5.0.2
```

See [Recording](./08-advanced/02-recording.md#update-the-bigbluebutton-recording-player) for more information.
See [Recording](./08-advanced/03-recording.md#update-the-bigbluebutton-recording-player) for more information.

### locales\:cache

Expand Down
Loading
Loading