menu-control icon indicating copy to clipboard operation
menu-control copied to clipboard

Authorizator have no effect on whether the item is displayed or not

Open patrickkusebauch opened this issue 5 years ago • 12 comments

I have just implemented authorizator, where the public function isMenuItemAllowed(IMenuItem $item): bool returns false in all cases, but it has no effect what so ever on whether the item is displayed or not. The only effect it has is on whether the item can be active or not. Did I misunderstand something or is it a bug?

patrickkusebauch avatar Apr 13 '20 01:04 patrickkusebauch

@patrickkusebauch This is mostly solved in menu template. Have you custom template? If yes, can you post it?

foxycode avatar Apr 15 '20 14:04 foxycode

{varType Carrooi\Menu\IMenuItemsContainer $menu}
<nav class="mt-2">
    <ul class="nav nav-pills nav-sidebar flex-column" data-widget="treeview" role="menu" data-accordion="false">
        {foreach $menu->getVisibleItemsOnMenu() as $itemsParent}
            {varType Carrooi\Menu\Menu|Carrooi\Menu\MenuItem $itemsParent}
            {continueIf $itemsParent->isAllowed() === false}
            <hr n:if="$itemsParent->getData('section', false)">
            {if $itemsParent->hasVisibleItemsOnMenu() === false}
                <li class="nav-item">
                    <a n:class="$itemsParent->isActive() ? 'nav-link active' : 'nav-link'"
                            href="{$itemsParent->getRealLink()}">
                        <i class="nav-icon {$itemsParent->getData('icon') ?? 'fas fa-th'}"></i>
                        <p>{$itemsParent->getRealTitle()}</p>
                    </a>
                </li>
            {else}
                <li n:class="$itemsParent->isActive() ? 'nav-item has-treeview menu-open' : 'nav-item has-treeview'">
                    <a n:class="$itemsParent->isActive() ? 'nav-link active' : 'nav-link'" href="#">
                        <i class="nav-icon {$itemsParent->getData('icon') ?? 'fas fa-th'}"></i>
                        <p>{$itemsParent->getRealTitle()}
                            <i class="right fas fa-angle-left"></i>
                        </p>
                    </a>
                    <ul class="nav nav-treeview">
                        {foreach $itemsParent->getVisibleItemsOnMenu() as $item}
                            {varType Carrooi\Menu\MenuItem $item}
                            <li class="nav-item">
                                <a n:class="$item->isActive() ? 'nav-link active' : 'nav-link'"
                                        href="{$item->getRealLink()}">
                                    <i class="nav-icon {$item->getData('icon') ?? 'fas fa-th'}"></i>
                                    <p>{$item->getRealTitle()}</p>
                                </a>
                            </li>
                        {/foreach}
                    </ul>
                </li>
            {/if}
        {/foreach}
    </ul>
</nav>

You can notice the continueIf macro I had to add, but I don't think should be neccessary.

patrickkusebauch avatar Apr 15 '20 14:04 patrickkusebauch

The menu.latte serving as a template for user implementation has no mention of needing to filter in the template. It does not even make sense from the domain logic perspective. You generally should have no knowledge about things you are not allowed to access.

patrickkusebauch avatar Apr 15 '20 14:04 patrickkusebauch

You are right, but I only took over this repository, didn't design it. I will need to look into it deeper. Would be nice if you could send PR with failing test.

foxycode avatar Apr 15 '20 14:04 foxycode

@patrickkusebauch I see test is already present. All tests are passing.

In template, you need to call isAllowed() and that will call isMenuItemAllowed() from you authorizator.

Didn't saying this is good implementation, but for now, it will function.

foxycode avatar Apr 15 '20 16:04 foxycode

Hi, how to connect the MenuAuthorizator to Nette Authorizator that I have already set-up? I can ask isAllowed or isInRole on User in Presenters, but how can I get that to menu-control?

Ok, so I found how to get user there to use isAllowed($resource) , but not sure how to hide top category if all sub-cats are not not allowed to user?

darkWolf-PR avatar Apr 15 '20 18:04 darkWolf-PR

@darkWolf-PR You can inject nette User or better IUserStorage to menu authorizator using contructor.

You can use this condition I believe:

if (count($menuItem->getVisibleItemsOnMenu())) {
    // show menu
}

foxycode avatar Apr 15 '20 19:04 foxycode

@darkWolf-PR the answer about hiding the top category has 2 parts.

The first is kind of about authorization - are you authorized to top category if you are not authorized to any subcategories? In the authorizator, you have the instance of IMenuItem that can get its children with getItems(). You can loop them and test whether you are authorized to at least one of them.

So maybe you find one sub-category you are allowed to access, so you are allowed to access to the top category. But here comes the second part - maybe you don/t display your subcategory on the menu, but only in the breadcrumb.

Therefor in the menu template, you have to do pretty much the same thing as in the Authorizator. Loop over all visible children tests whether there is at least one allowed and if not, do not display the top category.

Pseudocode follows (I do not want to bother with latte styling)

displayTopCategory = false
foreach TopCategory->GetVisibleItemsOnMenu() as subCategory
    if subcategory->isAllowed(): displayTopCategory = true
/foreach
<li n:if="displayTopCategory && TopCategory->isAllowed()">TopCategory->getRealName()</li>

patrickkusebauch avatar Apr 16 '20 00:04 patrickkusebauch

@foxycode Yeah I know it functions like that. That is why I have {continueIf $itemsParent->isAllowed() === false} in the template I posted earlier. My problem is that it is not intuitive, it is not mentioned in the documentation and it is not used in the default template that serves at e guide to how to do your own template. So nowhere you will find any mention about this unexpected behaviour and it will bite you in the ass.

patrickkusebauch avatar Apr 16 '20 00:04 patrickkusebauch

I have to say I´m yet to understand whole this connection between menu and breadcrumb...I´ve only used this control for menu in admin, where all links are known and I only need to hide those not accessible for logged role. Also there is never a top category link with subcats, either it´s top category link, or it´s a blank header for opening subcats.

Breadcrumb have to show sublinks like /users/new, /users/edit, I would have to define those in menu config too to have the breadcrumb working?

darkWolf-PR avatar Apr 17 '20 09:04 darkWolf-PR

@darkWolf-PR well the issue in your case is that you have one specific use case. That use case (admin navigation) usually uses the tree structure to group some links together, that thematically go together. So you usually do not have a link for the top category. Because it is really not a top category. It is a group identifier with no meaning on its own.

But you can also imagine another use case like an e-shop catalog. In that case, having a link for the top category makes sense. For example:

  • Your top category is Furniture
  • Your subcategories might be Kitchen, Living room, etc.

In this use case, a link for the top category Furniture would lead you to a page with all the available furniture for all possible rooms.

As for the second question - yes, that would be the case. Again in your case, it is tedious and does not make much sense.

But if you come back to the e-shop example, the last level for the menu/breadcrumb would be the actual item detail page. You want to display the item detail page in the breadcrumb, but for sure you do not want to display them in the menu.

I think this library is trying to be very general and for many possible use cases, which is good, but the issue I see is that each use case has some caveats that would benefit from proper documentation. Documentation is generally a sore sport form Contributte components IMHO.

For example in the e-shop case, you probably do not really care about Authorization at all.

patrickkusebauch avatar Apr 17 '20 10:04 patrickkusebauch

@darkWolf-PR I'm adding complete example of what is working for me. Hope it will help you. It could definitelly work better and docs should be updated too. I will look into it when I have more time. Hope others will help too (@patrickkusebauch ?)

menu.neon

extensions:
    menu: Contributte\MenuControl\DI\MenuExtension

services:
    - Billing\Components\Menu\DefaultAuthorizator

menu:
    default:
        templates:
            menu: %appDir%/Components/Menu/menu.latte
            breadcrumb: %appDir%/Components/Menu/breadcrumbs.latte
        authorizator: @Billing\Components\Menu\DefaultAuthorizator
        items:
            overview:
                title: Přehled
                action: Overview:default
            invoicing:
                title: Fakturace
                link: '#'
                items:
                    actuarialList:
                        title: Doklady
                        action: Actuarial:ActuarialList:default
                        include:
                            - ^Actuarial\:ActuarialDetail\:.+$
                            - ^Actuarial\:Phone\:.+$
                    actuarialItems:
                        title: Položky dokladů
                        action: Actuarial:ActuarialItems:default
                    payList:
                        title: Platby
                        action: Actuarial:PayList:default
                        include: ^Actuarial\:PayList\:.+$

DefaultAuthorizator.php

final class DefaultAuthorizator implements IAuthorizator
{
    /**
     * @var User
     */
    private $user;

    public function __construct(User $user)
    {
        $this->user = $user;
    }

    public function isMenuItemAllowed(IMenuItem $item): bool
    {
        return $this->user->isLoggedIn();
    }
}

menu.latte

<ul class="nav navbar-nav" n:if="$menu->hasVisibleItemsOnMenu()">
    {foreach $menu->getVisibleItemsOnMenu() as $item}
        {if $item->hasVisibleItemsOnMenu()}
            <li n:if="$item->isAllowed()" n:class="dropdown, $item->isActive() ? active">
                <a href="#" class="dropdown-toggle" data-toggle="dropdown">{$item->getRealTitle()} <span class="caret"></span></a>
                <ul class="dropdown-menu">
                    <li n:foreach="$item->getVisibleItemsOnMenu() as $subitem" n:class="$subitem->isActive() ? active">
                        <a href="{$subitem->getRealLink()}">{$subitem->getRealTitle()}</a>
                    </li>
                </ul>
            </li>
        {else}
            <li n:if="$item->isAllowed()" n:class="$item->isActive() ? active">
                <a href="{$item->getRealLink()}">{$item->getRealTitle()}</a>
            </li>
        {/if}
    {/foreach}
</ul>

breadcrumbs.latte

<ol class="breadcrumb">
    <li n:foreach="$menu->getPath() as $item">
        <a href="{$item->getRealLink()}">{$item->getRealTitle()}</a>
    </li>
</ol>

BasePresenter.php

abstract class BasePresenter extends Presenter
{
    /**
     * @var IMenuComponentFactory
     */
    private $menuFactory;

    /**
     * @var MenuContainer
     */
    private $menuContainer;

    public function injectBasePresenter(
        IMenuComponentFactory $menuFactory,
        MenuContainer $menuContainer
    ): void {
        $this->menuFactory = $menuFactory;
        $this->menuContainer = $menuContainer;
    }

    protected function createComponentMenu(): MenuComponent
    {
        return $this->menuFactory->create('default');
    }

    protected function getMenu(): IMenu
    {
        return $this->menuContainer->getMenu('default');
    }
}

ActuarialDetailPresenter.php

final class ActuarialDetailPresenter extends BasePresenter
{
    /**
     * @var Actuarial
     */
    private $actuarial;

    /**
     * @throws EntityNotFoundException
     */
    public function actionDefault(int $id): void
    {
        $this->actuarial = $this->actuarialFacade->get($id);
    }

    public function actionNew(): void
    {
        $this->setView('default');
    }

    public function renderDefault(int $id = NULL): void
    {
        $this->template->actuarial = $this->actuarial;

        $this->getMenu()
            ->getItem('invoicing')
            ->getItem('actuarialList')
            ->addItem('detail', "Doklad $id", function (IMenuItem $item) use ($id): void {
                $item->setAction('ActuarialDetail:', ['id' => $id]);
                $item->setMenuVisibility(FALSE);
            });
    }
}

foxycode avatar Apr 20 '20 15:04 foxycode