Open Tofandel opened 5 months ago
🤔 it's definitely supposed to be there, see https://github.com/area17/twill/blob/ce0fbf153cb455514eeea2b800ab46d569e38c0a/views/users/create.blade.php#L14-L25
That seems to be the source of the issue
private function seedDefaultRoles()
{
// Default roles and their permissions
$roles = [
'Owner' => Permission::available(Permission::SCOPE_GLOBAL),
'Administrator' => array_diff(Permission::available(Permission::SCOPE_GLOBAL), ["edit-user-roles", "manage-modules"]),
'Team' => [],
'Guest' => [],
];
I'm logged in as an Administrator, which doesn't have "edit-user-roles", that's not a sensible default I think
Also in this case the whole creation should be disabled or role_id should have a default value corresponding to the current role of the person creating the user or we should show the list of roles with a position higher or equal to that of the current user's role
or we should show the list of roles with a position higher or equal to that of the current user's role
That's already the case in the logic if I just remove the @can
, so I think we should just remove it
We should also remove the "All" option from there
A user can only have one role so it makes no sense and creation doesn't work if you select "All", I checked and it's coming from this code
public function filters(): TableFilters
{
$tableFilters = TableFilters::make();
foreach ($this->indexData($this->request) as $key => $value) {
if (Str::endsWith($key, 'List')) {
$queryString = Str::beforeLast($key, 'List');
if ($filterKey = ($this->filters[$queryString] ?? false)) {
if (! $value instanceof Collection) {
$value = collect($value)->mapWithKeys(function ($valueLabel) {
return [$valueLabel['value'] => $valueLabel['label']];
});
}
$tableFilters->add(
BasicFilter::make()
->queryString($queryString)
->options($value)
->apply(function (Builder $builder, mixed $value) use ($filterKey) {
$builder->where($filterKey, '=', $value);
})
);
}
}
}
return $tableFilters;
}
It's being treated as a BasicFilter (which it isn't) and the 'All' option is prepended because of it
The all option is definitely a regression and should not end up there, that's for sure. The fact that you're logged in as administrator seems strange to me, you're supposed to be logged in as superadmin locally by default, which bypasses all checks, with or without permissions enabled. Were you logged in as a non superadmin before enabling advanced permissions?
you're supposed to be logged in as superadmin locally by default
Yes, but then you can create another user, give it the administrator role and login with it and that's when it happens, we can't just give superadmin permissions to everyone ;)
An administrator should definitely be able to create new users though
Of course not, I wasn't suggesting you need to use superadmin users, I wanted to understand why you were logged in as an administrator. Now I understand the non-sensible default you're talking about. I'm not sure if I agree though. In the defaults we provide (which are totally opinionated and not mandatory to use), admins are administrators of the content, not the larger system, which is the owner's role responsibility. Don't you agree that the ability to create new user roles needs to be gated more than creating/editing content?
Don't you agree that the ability to create new user roles needs to be gated more than creating/editing content?
The ability to create/edit roles yes absolutely, but the ability to create/edit users no, which I think is the source of confusion, this permission is used to assert two very distinct things
One is to edit the roles
And the second is to check if we can assign/edit the role of an user
The second usage of the permission is the one we need to remove or change to a different permission (I think removal is okay because we already have a list of which roles are accessible from another role)
If we don't remove it, then I'd name the first usage of the permission as manage-roles
instead of edit-user-roles
(because the naming hints more that it's made for the second usage)
When trying to create a user with the roles and permissions feature, it seems a role input is missing