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

Why autocomplete action is mapped to list action? #113

Open
Bloodysunset opened this issue Apr 2, 2019 · 7 comments
Open

Why autocomplete action is mapped to list action? #113

Bloodysunset opened this issue Apr 2, 2019 · 7 comments

Comments

@Bloodysunset
Copy link

Bloodysunset commented Apr 2, 2019

Hi everyone. This issue is not about a bug, but to bring a discussion about the state of a functionality.

Currently, we can define a role per action in our entities to lock some users from accessing the show action for example. This is fine and great, but here is the issue...
I was trying to get results from an autocomplete field on a user with an inferior role. So I configured my entity like this :

easy_admin:
    entities:
        MyEntity:
            class: path\to\MyEntity
            role: ROLE_ADMIN # the role is applied to all actions
            autocomplete:
                role: ROLE_USER # the role attribute is overrided for this action

I could not understand why this would not work, until I find that the isActionAllowed() is overrided in the EasyAdminController.php of this bundle. Here is how it looks like :

protected function isActionAllowed($actionName)
{
    switch ($actionName) {
        // autocomplete action is mapped to list action for access permissions
        case 'autocomplete':
        // embeddedList action is mapped to list action for access permissions
        case 'embeddedList':
            $actionName = 'list';
            break;
        // newAjax action is mapped to new action for access permissions
        case 'newAjax':
            $actionName = 'new';
            break;
        default:
            break;
    }

    // Get item for edit/show or custom actions => security voters may apply
    $easyadmin = $this->request->attributes->get('easyadmin');
    $subject = $easyadmin['item'] ?? null;
    $this->get(AdminAuthorizationChecker::class)->checksUserAccess($this->entity, $actionName, $subject);

    return parent::isActionAllowed($actionName);
}

You can see that the autocomplete action is treated like the list one.
And this is where I'm lost : is there a particular reason we're not able to configure the autocomplete action to be used by a lesser role? If not, it would be great to "fix" this by removing this action from the switch case.

I'm open to discuss about it.
Have a great day/night.

@alterphp
Copy link
Owner

alterphp commented Apr 3, 2019

Hi @Bloodysunset,

This bundle brings a role-based security layer, and every action might be "protected" by requiring a specific role.
For autocomplete, I did not want to define a specific role, as it is not a true interface. This is the same for embedded_list, they are widgets. That's why I decided to map them onto the closest matching CRUD actions. Basically, autocomplete is like list or search, and embedded_list is like list.

I don't think that somebody who do not have access of Foo list should be able to list them in an autocomplete field... What do you think ?

@Bloodysunset
Copy link
Author

Hello PéCé,

While I 100% understand your point, my client doesn't. He will tell me that his collaborators should only be able to filter specifics listings (with autocomplete filters based on lists accessibles only by administrators).

So to 'fix' my problem, I've set the role: ROLE_USER on the list, AND I've added a role: ROLE_ADMIN in my menu entry to hide the link for users. Now they can use autocomplete but can't see the listing, except if they have somehow figured the URL.

My issue is now 'fixed', but I think that being able to configure the role for the autocomplete would be less 'hacky' and more secure. I get that ifrom a developer point of view, list and autocomplete are linked and therefore should be considered as the 'same action'. But from the user point of view, this makes no sense.

Not saying that you should make that change however, just wanted to share my experience with this issue/request from the client.

@gonzaloalonsod
Copy link
Contributor

gonzaloalonsod commented Apr 3, 2019

Override in AdminController:

    protected function isActionAllowed($actionName)
    {
        switch ($actionName) {
            // autocomplete action is mapped to list action for access permissions
            case 'autocomplete':
                break;
            // embeddedList action is mapped to list action for access permissions
            case 'embeddedList':
                $actionName = 'list';
                break;
...
}

@alterphp
Copy link
Owner

alterphp commented Apr 3, 2019

Hello PéCé,

While I 100% understand your point, my client doesn't. He will tell me that his collaborators should only be able to filter specifics listings (with autocomplete filters based on lists accessibles only by administrators).

So to 'fix' my problem, I've set the role: ROLE_USER on the list, AND I've added a role: ROLE_ADMIN in my menu entry to hide the link for users. Now they can use autocomplete but can't see the listing, except if they have somehow figured the URL.

My issue is now 'fixed', but I think that being able to configure the role for the autocomplete would be less 'hacky' and more secure. I get that ifrom a developer point of view, list and autocomplete are linked and therefore should be considered as the 'same action'. But from the user point of view, this makes no sense.

Not saying that you should make that change however, just wanted to share my experience with this issue/request from the client.

I share your opinion : this workaround is not a solution. I think we should define by configuration, the action name mapped onto autocomplete, embeddedList and newAjax instead of this hard coded mapping. are you interested to work on this ?

config/easy_admin_extension.yaml

easy_admin_extension:
    role_mapped_actions:
        autocomplete: list # default
        embeddedList: list # default
        newAjax: new # default

@Bloodysunset
Copy link
Author

@gonzakpo I've tried to override it, but was unsuccessful doing so (maybe something wrong on my end).

@alterphp This sounds great. I can't give you an ETA on when I'll be able to work on it, but yes!
We can close the issue until a PR is ready I think.

Thank you for taking the time to clarify things about this feature :)

@alterphp
Copy link
Owner

alterphp commented Apr 3, 2019

Keep it open, and mention it in the PR when you will be able to work on it !

I'm glad to gain a new contributor ;-)

@gonzaloalonsod
Copy link
Contributor

add break; and functioned for my

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants