Permissions can accidentally block entire backend

[scottbedard]

User permissions can block access to the entire backend if a user does not have access to a plugin's default link.

Steps to reproduce

1. Create a plugin with 2 controllers, and register the navigation so that it appears in the backend header.
2. Create permissions for each of the two controllers, and create a user that has access blocked for the controller linked in the backend header.
3. Log in with the blocked user's account

Expected result

The user should be able to log in, and perhaps see the header linking to a controller he does have access to. Or possibly not seeing that link in the header at all.

Actual result

The user may not access the backend. When a request is made to /backend, a redirect occurs sending me to the controller that I am blocked from.

Demo plugin

[LukeTowers]

Closing as it has been over a month since any activity on this occurred.

[scottbedard]

@LukeTowers obviously it's been a while since I've thought about this issue. Is there anything else we need to resolve this though? It seems like a pretty thorough bug report in my opinion.

I'll try and clone down the reproduction repo sometime this week and see if this bug is still present. If it is, I think this is definitely something that should be fixed. Without a fix, user's who don't have access to the default page of a plugin will be entirely unable to use the backend.

[LukeTowers]

@scottbedard nothing against the report, just going on a closing spree to issues in order to prompt some movement on them ;)

I'm thinking the issue is because October will redirect you to the first navigation item that you have access to, which could be an incorrectly-configured plugin. This could be fixed by proper permission implementing on the plugin side of things (don't show menu items that take the user somewhere they can't go).

I'm not really sure this is an October issue that we can fix from our end, it seems more like it's up to the plugin author's to build their plugins correctly.

[scottbedard]

Ah, I see what you're saying now. When defining the plugin's navigation, the user's permissions should be checked first. Sounds good, I'm on board with that as a solution :)

[LukeTowers]

Well you can already assign permissions to navigation items when you register them, so I don't see how this was a problem in the first place. Is that what you're picking up from what I'm saying?

[scottbedard]

@LukeTowers I just have never seen a plugin that checks a user's permissions before returning the default navigation. Even plugins that are very well written like RainLab.Blog don't do this. Maybe we should add it to the docs that plugin authors should check a user's permissions before setting this piece of navigation.

[LukeTowers]

Doesn't it right here: https://github.com/rainlab/blog-plugin/blob/master/Plugin.php#L68?

[scottbedard]

I think you're misunderstanding the issue. The problem does not occur when a user doesn't have access to any part of the plugin. The problem exists where a plugin's default nav link sends the user to a place where they do not have access.

Using RainLab.Blog as an example...
In the backend nav, the plugin's default link will send the user to the posts controller, but if the user doesn't have permission to view that area, the entire backend can become unusable.

Maybe this is still a bug? It certainly could be solved by the plugin first checking the user's permission before returning the navigation array, I have just never seen this before.

[LukeTowers]

I understand that, it's still up to the plugin to solve it intelligently however. I do this in one of my client's plugins:

    /**
     * Registers back-end navigation items for this plugin.
     *
     * @return array
     */
    public function registerNavigation()
    {
        $url = $this->generateMenuUrl();
        if (empty($url)) {
            return [];
        }

        return [
            'exchanges' => [
                'label'       => 'iacea.exchanges::lang.navigation.exchanges',
                'url'         => $url,
                'iconSvg'     => 'plugins/iacea/exchanges/assets/images/icon.svg',
                'order'       => 100,
                'permissions' => ['iacea.exchanges.*'],
                'sideMenu'    => [
                    'programmes'   => [
                        'label'       => 'iacea.exchanges::lang.navigation.programmes',
                        'url'         => $this->generateMenuUrl('programmes'),
                        'icon'        => 'icon-list-alt',
                        'permissions' => ['iacea.exchanges.programmes.*'],
                    ],
                    'nations'      => [
                        'label'       => 'iacea.exchanges::lang.navigation.nations',
                        'url'         => $this->generateMenuUrl('nations'),
                        'icon'        => 'icon-flag',
                        'permissions' => ['iacea.exchanges.nations.*'],
                    ],
                    'members'      => [
                        'label'       => 'iacea.exchanges::lang.models.member.label_plural',
                        'url'         => $this->generateMenuUrl('members'),
                        'icon'        => 'icon-users',
                        'permissions' => ['iacea.exchanges.members.*'],
                    ],
                    'participants' => [
                        'label'       => 'iacea.exchanges::lang.models.participant.label_plural',
                        'url'         => $this->generateMenuUrl('participants'),
                        'icon'        => 'icon-user-secret',
                        'permissions' => ['iacea.exchanges.participants.*'],
                    ],
                ],
            ],
        ];
    }

    /**
     * Generate the URL to use for the main navigation item based on the
     * logged in user
     *
     * #return string
     */
    protected function generateMenuUrl($section = false)
    {
        if (empty($section)) {
            $user = BackendAuth::getUser();

            // Check permissions
            if ($user) {
                if ($user->hasAccess('iacea.exchanges.participants.*')) {
                    $section = 'participants';
                }

                if ($user->hasAccess('iacea.exchanges.members.*')) {
                    $section = 'members';
                }

                if ($user->hasAccess('iacea.exchanges.nations.*')) {
                    $section = 'nations';
                }

                if ($user->hasAccess('iacea.exchanges.programmes.*')) {
                    $section = 'programmes';
                }
            }

        }

        // Generate the URL
        return !empty($section) ? Backend::url("iacea/exchanges/$section") : '';
    }

[LukeTowers]

Now if this is an issue with the RainLab.Blog plugin that it needs to implement such a solution, then that would be an issue for the rainlab/blog-plugin repository

[scottbedard]

@LukeTowers sounds good, I can live with this being the plugin's responsibility :)

@daftspunk would you be open to a PR whereby October fails a bit more gracefully in situations where a plugin has not done this?

[LukeTowers]

@scottbedard I think we would be open to such a PR, what do you have in mind?

[scottbedard]

Probably something just like what you have implemented. Check if the user has access to the page being linked to, and if not, look for permission to one of the subnavs. If no acceptable path is found, do not render the link.

Or perhaps, render the link, but disable it? I'm not sure, what do you think?

[LukeTowers]

As long as you can implement the suggestion of looking at the subnavs in a simple manner that's not difficult to understand and maintain going forwards it sounds good to me!