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

Twill users creation and roles permissions #2548

Open
Tofandel opened this issue Mar 18, 2024 · 7 comments · May be fixed by #2550
Open

Twill users creation and roles permissions #2548

Tofandel opened this issue Mar 18, 2024 · 7 comments · May be fixed by #2550

Comments

@Tofandel
Copy link
Contributor

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

image

@ifox
Copy link
Member

ifox commented Mar 18, 2024

🤔 it's definitely supposed to be there, see

@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
Copy link
Contributor Author

Tofandel commented Mar 18, 2024

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
Copy link
Contributor Author

Tofandel commented Mar 18, 2024

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
image

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
Copy link
Member

ifox commented Mar 18, 2024

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
Copy link
Contributor Author

Tofandel commented Mar 18, 2024

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
Copy link
Member

ifox commented Mar 18, 2024

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
Copy link
Contributor Author

Tofandel commented Mar 18, 2024

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
image

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)

@ifox ifox changed the title Cannot create twill users Twill users creation and roles permissions May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants