fix: properly gate menus by role

config/services.yaml

@@ -111,6 +111,17 @@ services:
         tags:
             - name: monolog.processor
 
+    App\Menu\MenuBuilder:
+        arguments:
+            $factory: '@knp_menu.factory'
+        tags:
+            - name: knp_menu.menu_builder
+              method: createMainMenu
+              alias: default_main
+            - name: knp_menu.menu_builder
+              method: createTeamerMenu
+              alias: default_teamer
+
     App\Menu\AdminMenuBuilder:
         arguments:
             $factory: '@knp_menu.factory'

src/Menu/AbstractMenuBuilder.php

@@ -13,6 +13,30 @@ use Symfony\Contracts\Translation\TranslatorInterface;
 
 abstract class AbstractMenuBuilder
 {
+    /**
+     * Routes of the areas that have pages of their own, mapped to the role owning the area.
+     *
+     * Note that "app_admin_" does not match the shared "app_administrative_" routes.
+     */
+    protected const AREA_ROUTE_PREFIXES = [
+        'app_admin_' => 'ROLE_ADMIN',
+        'app_manager_' => 'ROLE_MANAGER',
+        'app_house_manager_' => 'ROLE_HOUSE_MANAGER',
+        'app_teamer_' => 'ROLE_TEAMER',
+    ];
+
+    /**
+     * Priority order of the area roles, must stay in sync with User::getDefaultRoute().
+     */
+    protected const ROLE_PRIORITY = [
+        'ROLE_ADMIN',
+        'ROLE_MANAGER',
+        'ROLE_HOUSE_MANAGER',
+        'ROLE_TEAMER',
+    ];
+
+    protected const AREA_SESSION_KEY = 'menu_area';
+
     public function __construct(
         protected FactoryInterface $factory,
         protected Security $security,
@@ -26,6 +50,49 @@ abstract class AbstractMenuBuilder
         return $this->factory->createItem('root');
     }
 
+    /**
+     * Returns the role owning the area the current request belongs to.
+     *
+     * Entering an area that has pages of its own selects that area and keeps it
+     * for the rest of the session. The routes shared by all administrative roles
+     * (app_administrative_*) carry no area of their own and therefore stay in the
+     * area the user entered last, so that switching areas sticks. Without a
+     * remembered area the highest role wins.
+     */
+    protected function resolveArea(): ?string
+    {
+        $request = $this->requestStack->getMainRequest();
+        $session = null !== $this->security->getUser() && true === $request?->hasSession()
+            ? $request->getSession()
+            : null;
+        $route = $request?->attributes->get('_route');
+
+        if (is_string($route)) {
+            foreach (self::AREA_ROUTE_PREFIXES as $prefix => $role) {
+                if (str_starts_with($route, $prefix) && $this->security->isGranted($role)) {
+                    $session?->set(self::AREA_SESSION_KEY, $role);
+
+                    return $role;
+                }
+            }
+        }
+
+        $rememberedArea = $session?->get(self::AREA_SESSION_KEY);
+
+        // The remembered area is re-checked, roles can be revoked mid-session.
+        if (is_string($rememberedArea) && $this->security->isGranted($rememberedArea)) {
+            return $rememberedArea;
+        }
+
+        foreach (self::ROLE_PRIORITY as $role) {
+            if ($this->security->isGranted($role)) {
+                return $role;
+            }
+        }
+
+        return null;
+    }
+
     protected function getDefaultRouteParameters(string $parameter = 'id', string $default = '0'): array
     {
         $request = $this->requestStack->getMainRequest();
@@ -36,6 +103,41 @@ abstract class AbstractMenuBuilder
         ];
     }
 
+    /**
+     * Adds the link leading back out of a detail view.
+     *
+     * The return url is taken from the menu options first, then from the request,
+     * and falls back to the overview itself. Without that fallback the item ends up
+     * without an uri and is rendered as a dead label instead of a link.
+     *
+     * @param array<string, mixed> $options
+     */
+    protected function addBackItem(
+        ItemInterface $menu,
+        array $options,
+        string $defaultRoute = 'app_administrative_teamer_index',
+    ): void {
+        $this->addDivider($menu);
+
+        // Already decoded by ReturnUrlTrait::getReturnUrl(), must not be decoded again.
+        $returnUrl = $options['return_url'] ?? null;
+
+        if (false === is_string($returnUrl) || '' === $returnUrl) {
+            $requestReturnUrl = $this->requestStack->getMainRequest()?->get('r');
+
+            $returnUrl = is_string($requestReturnUrl) && '' !== $requestReturnUrl
+                ? rawurldecode($requestReturnUrl)
+                : null;
+        }
+
+        $menu->addChild('zurück zur Übersicht', [
+            ...(null !== $returnUrl ? ['uri' => $returnUrl] : ['route' => $defaultRoute]),
+            'extras' => [
+                'icon' => 'arrow-left',
+            ],
+        ]);
+    }
+
     protected function addAdminItem(ItemInterface $menu): void
     {
         if ($this->security->isGranted('ROLE_ADMIN')) {

src/Menu/AdminMenuBuilder.php

@@ -337,16 +337,7 @@ class AdminMenuBuilder extends AbstractMenuBuilder
             ],
         ]);
 
-        $this->addDivider($menu);
+        $this->addBackItem($menu, $options);
-
-        $request = $this->requestStack->getMainRequest();
-
-        $menu->addChild('zurück zur Übersicht', [
-            'uri' => rawurldecode($request->get('r')),
-            'extras' => [
-                'icon' => 'arrow-left',
-            ],
-        ]);
 
         return $menu;
     }

src/Menu/ManagerMenuBuilder.php

@@ -204,16 +204,7 @@ class ManagerMenuBuilder extends AbstractMenuBuilder
             ],
         ]);
 
-        $this->addDivider($menu);
+        $this->addBackItem($menu, $options);
-
-        $request = $this->requestStack->getMainRequest();
-
-        $menu->addChild('zurück zur Übersicht', [
-            'uri' => rawurldecode($request->get('r')),
-            'extras' => [
-                'icon' => 'arrow-left',
-            ],
-        ]);
 
         return $menu;
     }

src/Menu/MenuBuilder.php

@@ -0,0 +1,51 @@
+<?php
+
+namespace App\Menu;
+
+use Knp\Menu\FactoryInterface;
+use Knp\Menu\ItemInterface;
+use Symfony\Bundle\SecurityBundle\Security;
+use Symfony\Component\HttpFoundation\RequestStack;
+use Symfony\Contracts\Translation\TranslatorInterface;
+
+/**
+ * Serves the menu of the area the current user has the highest privileges for.
+ *
+ * Used by templates that are reachable by more than one role, so that the area
+ * selection follows a single priority order instead of being decided in Twig.
+ */
+class MenuBuilder extends AbstractMenuBuilder
+{
+    public function __construct(
+        FactoryInterface $factory,
+        Security $security,
+        RequestStack $requestStack,
+        TranslatorInterface $translator,
+        private readonly AdminMenuBuilder $adminMenuBuilder,
+        private readonly ManagerMenuBuilder $managerMenuBuilder,
+        private readonly HouseManagerMenuBuilder $houseManagerMenuBuilder,
+        private readonly TeamerMenuBuilder $teamerMenuBuilder,
+    ) {
+        parent::__construct($factory, $security, $requestStack, $translator);
+    }
+
+    public function createMainMenu(array $options): ItemInterface
+    {
+        return match ($this->resolveArea()) {
+            'ROLE_ADMIN' => $this->adminMenuBuilder->createMainMenu($options),
+            'ROLE_MANAGER' => $this->managerMenuBuilder->createMainMenu($options),
+            'ROLE_HOUSE_MANAGER' => $this->houseManagerMenuBuilder->createMainMenu($options),
+            'ROLE_TEAMER' => $this->teamerMenuBuilder->createMainMenu($options),
+            default => $this->createRootElement(),
+        };
+    }
+
+    public function createTeamerMenu(array $options): ItemInterface
+    {
+        return match ($this->resolveArea()) {
+            'ROLE_ADMIN' => $this->adminMenuBuilder->createTeamerMenu($options),
+            'ROLE_MANAGER' => $this->managerMenuBuilder->createTeamerMenu($options),
+            default => $this->createRootElement(),
+        };
+    }
+}

templates/admin/layout.html.twig

@@ -1,9 +1,9 @@
 {% extends 'layout.html.twig' %}
 
 {% block main_menu %}
-    {{ knp_menu_render(knp_menu_get('admin_main')) }}
+    {{ knp_menu_render(knp_menu_get('default_main')) }}
 {% endblock %}
 
 {% block mobile_menu %}
-    {{ knp_menu_render(knp_menu_get('admin_main')) }}
+    {{ knp_menu_render(knp_menu_get('default_main')) }}
 {% endblock %}

templates/admin/teamer/availability/index.html.twig

@@ -5,7 +5,7 @@
 {% block content %}
     <div class="grid lg:grid-cols-3 gap-y-8 lg:gap-y-0 lg:gap-x-8">
         <div>
-            {{ knp_menu_render(knp_menu_get('admin_teamer')) }}
+            {{ knp_menu_render(knp_menu_get('default_teamer')) }}
         </div>
         <div class="lg:col-span-2">
             <h1 class="text-2xl font-bold pb-8">

templates/admin/teamer/crm_selections.html.twig

@@ -5,7 +5,7 @@
 {% block content %}
     <div class="grid lg:grid-cols-3 gap-y-8 lg:gap-y-0 lg:gap-x-8">
         <div>
-            {{ knp_menu_render(knp_menu_get('admin_teamer')) }}
+            {{ knp_menu_render(knp_menu_get('default_teamer')) }}
         </div>
         <div class="lg:col-span-2">
             <h1 class="text-2xl font-bold pb-8">

templates/admin/teamer/remarks_internal.html.twig

@@ -5,7 +5,7 @@
 {% block content %}
     <div class="grid lg:grid-cols-3 gap-y-8 lg:gap-y-0 lg:gap-x-8">
         <div>
-            {{ knp_menu_render(knp_menu_get('admin_teamer')) }}
+            {{ knp_menu_render(knp_menu_get('default_teamer')) }}
         </div>
         <div class="lg:col-span-2">
             <h1 class="text-2xl font-bold pb-8">

templates/admin/teamer/skills.html.twig

@@ -5,7 +5,7 @@
 {% block content %}
     <div class="grid lg:grid-cols-3 gap-y-8 lg:gap-y-0 lg:gap-x-8">
         <div>
-            {{ knp_menu_render(knp_menu_get('admin_teamer')) }}
+            {{ knp_menu_render(knp_menu_get('default_teamer')) }}
         </div>
         <div class="lg:col-span-2">
             <h1 class="text-2xl font-bold pb-8">

templates/administrative/availability/index.html.twig

@@ -1,4 +1,4 @@
-{% extends 'admin/layout.html.twig' %}
+{% extends 'administrative/layout.html.twig' %}
 
 {% block title %}Eigene Verfügbarkeiten der Teamer{% endblock %}
 

templates/administrative/layout.html.twig

@@ -1,21 +1,9 @@
 {% extends 'layout.html.twig' %}
 
 {% block main_menu %}
-    {% if is_granted('ROLE_MANAGER') %}
+    {{ knp_menu_render(knp_menu_get('default_main')) }}
-        {{ knp_menu_render(knp_menu_get('manager_main')) }}
-    {% elseif is_granted('ROLE_HOUSE_MANAGER') %}
-        {{ knp_menu_render(knp_menu_get('house_manager_main')) }}
-    {% elseif is_granted('ROLE_ADMIN') %}
-        {{ knp_menu_render(knp_menu_get('admin_main')) }}
-    {% endif %}
 {% endblock %}
 
 {% block mobile_menu %}
-    {% if is_granted('ROLE_MANAGER') %}
+    {{ knp_menu_render(knp_menu_get('default_main')) }}
-        {{ knp_menu_render(knp_menu_get('manager_main')) }}
-    {% elseif is_granted('ROLE_HOUSE_MANAGER') %}
-        {{ knp_menu_render(knp_menu_get('house_manager_main')) }}
-    {% elseif is_granted('ROLE_ADMIN') %}
-        {{ knp_menu_render(knp_menu_get('admin_main')) }}
-    {% endif %}
 {% endblock %}

templates/administrative/teamer/profile.html.twig

@@ -5,11 +5,7 @@
 {% block content %}
     <div class="grid lg:grid-cols-3 gap-y-8 lg:gap-y-0 lg:gap-x-8">
         <div>
-            {% if is_granted('ROLE_MANAGER') %}
+            {{ knp_menu_render(knp_menu_get('default_teamer', [], { 'return_url': returnUrl })) }}
-                {{ knp_menu_render(knp_menu_get('manager_teamer', [], { 'return_url': returnUrl })) }}
-            {% elseif is_granted('ROLE_ADMIN') %}
-                {{ knp_menu_render(knp_menu_get('admin_teamer', [], { 'return_url': returnUrl })) }}
-            {% endif %}
 
         </div>
         <div class="lg:col-span-2">

templates/administrative/teamer/recent_assignments.html.twig

@@ -5,11 +5,7 @@
 {% block content %}
     <div class="grid lg:grid-cols-3 gap-y-8 lg:gap-y-0 lg:gap-x-8">
         <div>
-            {% if is_granted('ROLE_MANAGER') %}
+            {{ knp_menu_render(knp_menu_get('default_teamer')) }}
-                {{ knp_menu_render(knp_menu_get('manager_teamer')) }}
-            {% elseif is_granted('ROLE_ADMIN') %}
-                {{ knp_menu_render(knp_menu_get('admin_teamer')) }}
-            {% endif %}
         </div>
         <div class="lg:col-span-2">
             <h1 class="text-2xl font-bold pb-8">

templates/administrative/teamer/upcoming_assignments.html.twig

@@ -5,11 +5,7 @@
 {% block content %}
     <div class="grid lg:grid-cols-3 gap-y-8 lg:gap-y-0 lg:gap-x-8">
         <div>
-            {% if is_granted('ROLE_MANAGER') %}
+            {{ knp_menu_render(knp_menu_get('default_teamer')) }}
-                {{ knp_menu_render(knp_menu_get('manager_teamer')) }}
-            {% elseif is_granted('ROLE_ADMIN') %}
-                {{ knp_menu_render(knp_menu_get('admin_teamer')) }}
-            {% endif %}
         </div>
         <div class="lg:col-span-2">
             <h1 class="text-2xl font-bold pb-8">

templates/teamer/layout.html.twig

@@ -1,9 +1,9 @@
 {% extends 'layout.html.twig' %}
 
 {% block main_menu %}
-    {{ knp_menu_render(knp_menu_get('teamer_main')) }}
+    {{ knp_menu_render(knp_menu_get('default_main')) }}
 {% endblock %}
 
 {% block mobile_menu %}
-    {{ knp_menu_render(knp_menu_get('teamer_main')) }}
+    {{ knp_menu_render(knp_menu_get('default_main')) }}
 {% endblock %}

tests/Menu/TeamerMenuBackItemTest.php

@@ -0,0 +1,118 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Menu;
+
+use App\Menu\AdminMenuBuilder;
+use App\Menu\ManagerMenuBuilder;
+use Knp\Menu\ItemInterface;
+use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\RequestStack;
+
+/**
+ * The back item degrades silently: without an uri it is rendered as a dead label
+ * instead of a link (see templates/_partials/_menu.html.twig), so it is guarded here.
+ */
+class TeamerMenuBackItemTest extends KernelTestCase
+{
+    private const OVERVIEW_PATH = '/administrative/teamer';
+
+    private RequestStack $requestStack;
+
+    protected function setUp(): void
+    {
+        self::bootKernel();
+
+        $this->requestStack = self::getContainer()->get(RequestStack::class);
+    }
+
+    public function testTheReturnUrlOptionIsUsed(): void
+    {
+        $this->pushRequest();
+
+        $item = $this->buildBackItem(AdminMenuBuilder::class, ['return_url' => '/administrative/teamer?page=3']);
+
+        self::assertSame('/administrative/teamer?page=3', $item->getUri());
+    }
+
+    public function testTheReturnUrlOptionWinsOverTheRequestParameter(): void
+    {
+        $this->pushRequest(rawurlencode('/administrative/teamer?page=9'));
+
+        $item = $this->buildBackItem(AdminMenuBuilder::class, ['return_url' => '/administrative/teamer?page=3']);
+
+        self::assertSame('/administrative/teamer?page=3', $item->getUri());
+    }
+
+    public function testTheRequestParameterIsDecoded(): void
+    {
+        $this->pushRequest(rawurlencode('/administrative/teamer?page=3&sort=name'));
+
+        $item = $this->buildBackItem(AdminMenuBuilder::class);
+
+        self::assertSame('/administrative/teamer?page=3&sort=name', $item->getUri());
+    }
+
+    /**
+     * The regression guard: reachable e.g. after the status update in
+     * Admin\Teamer\SkillsController, which redirects without carrying "r" forward.
+     *
+     * @dataProvider builderProvider
+     */
+    public function testWithoutAnyReturnUrlTheOverviewIsLinked(string $builderClass): void
+    {
+        $this->pushRequest();
+
+        $deprecations = [];
+        set_error_handler(
+            static function (int $level, string $message) use (&$deprecations): bool {
+                $deprecations[] = $message;
+
+                return true;
+            },
+            \E_DEPRECATED
+        );
+
+        try {
+            $item = $this->buildBackItem($builderClass);
+        } finally {
+            restore_error_handler();
+        }
+
+        self::assertSame(self::OVERVIEW_PATH, $item->getUri());
+        self::assertSame([], $deprecations);
+    }
+
+    /**
+     * @return iterable<string, array{string}>
+     */
+    public static function builderProvider(): iterable
+    {
+        yield 'admin' => [AdminMenuBuilder::class];
+        yield 'manager' => [ManagerMenuBuilder::class];
+    }
+
+    private function pushRequest(?string $returnUrlParameter = null): void
+    {
+        $query = null !== $returnUrlParameter ? '?r='.$returnUrlParameter : '';
+
+        $this->requestStack->push(Request::create('/administrative/teamer/profile/0'.$query));
+    }
+
+    /**
+     * @param array<string, mixed> $options
+     */
+    private function buildBackItem(string $builderClass, array $options = []): ItemInterface
+    {
+        $builder = self::getContainer()->get($builderClass);
+        $children = $builder->createTeamerMenu($options)->getChildren();
+        $backItem = end($children);
+
+        self::assertInstanceOf(ItemInterface::class, $backItem);
+        self::assertSame('zurück zur Übersicht', $backItem->getLabel());
+
+        return $backItem;
+    }
+}