Twill users creation and roles permissions

[Tofandel]

When trying to create a user with the roles and permissions feature, it seems a role input is missing

[ifox]

🤔 it's definitely supposed to be there, see

twill/views/users/create.blade.php

Lines 14 to 25 in ce0fbf1

	@can('edit-user-roles')
	@php $userModel = twillModel('user') @endphp
	<x-twill::select
	:name="$userModel::getRoleColumnName()"
	:label="twillTrans('twill::lang.user-management.role')"
	:native="true"
	:options="$roleList ?? []"
	:default="$roleList[0]['value'] ?? ''"
	placeholder="Select a role"
	/>
	@endcan

[Tofandel]

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

[Tofandel]

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

[ifox]

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?

[Tofandel]

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

[ifox]

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?

[Tofandel]

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)