Cockpit-HQ / Cockpit

Cockpit Core - Content Platform
https://getcockpit.com
Other
388 stars 47 forks source link

added missing acl settings #99

Closed raffaelj closed 1 year ago

raffaelj commented 1 year ago

'app/finder' 'app/resources/unlock' 'app/system/info' 'app/spaces'

aheinze commented 1 year ago

Thanks for the contribution! Regarding the finder permissions, I think is better to keep it only for "admins".

raffaelj commented 1 year ago

You're right. I remember asking you about the same thing for v1 a few years ago. I was just focused on the mapping between v1 and v2 permissions and scraped the code for missing pieces.

Now there is a theoretical problem. If I just remove the app.permissions.collect part, there is no GUI option to allow finder access anymore. But users with app/roles/manage permission can send a custom request to http://localhost:9090/system/users/roles/save and enable permissions, that are hidden from the GUI. It's theoretical, because users with app/roles/manage access can create admin roles anyway (If they have app/users/manage rights or if they grant them to their role with the same technique).

Maybe it would be better to introduce a warning to some permissions, that they are only cosmetic for users with backend access (also I didn't find an implementation for users without backend access in v2).

This could look like this:

$this->on('app.permissions.collect', function($permissions) {

    $permissions['Finder'] = [
        'app/finder' => [
            'label' => 'Manage files',
            'warning'  => 'If enabled, this user group can grant themself admin access.',
        ],
    ];
});

And now I realized, that the users and roles permissions are directly defined in modules/System/views/users/roles/role.php. This feels very counter-intuitive. They should be defined in modules/System/admin.php, too.

raffaelj commented 1 year ago

But users with app/roles/manage permission can send a custom request to http://localhost:9090/system/users/roles/save and enable permissions

I had a second look. This doesn't affect the Finder, because the controller is only bound, if the user is admin.

See: https://github.com/Cockpit-HQ/Cockpit/blob/develop/modules/Finder/admin.php#L7

So it still feels inconsistent, to check for an app/finder permission on the settings page and in the before method of the finder controller, but I was wrong about the theoretical security issue.

aheinze commented 1 year ago

Merci 🙏