From 102e1c1c3ce564ef4c35850a78fec6f500cee2d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Tue, 11 Aug 2026 14:20:15 +0200 Subject: [PATCH] fix: properly gate menus by role --- config/services.yaml | 11 ++ src/Menu/AbstractMenuBuilder.php | 102 +++++++++++++++ src/Menu/AdminMenuBuilder.php | 11 +- src/Menu/ManagerMenuBuilder.php | 11 +- src/Menu/MenuBuilder.php | 51 ++++++++ templates/admin/layout.html.twig | 4 +- .../admin/teamer/availability/index.html.twig | 2 +- .../admin/teamer/crm_selections.html.twig | 2 +- .../admin/teamer/remarks_internal.html.twig | 2 +- templates/admin/teamer/skills.html.twig | 2 +- .../availability/index.html.twig | 2 +- templates/administrative/layout.html.twig | 16 +-- .../administrative/teamer/profile.html.twig | 6 +- .../teamer/recent_assignments.html.twig | 6 +- .../teamer/upcoming_assignments.html.twig | 6 +- templates/teamer/layout.html.twig | 4 +- tests/Menu/TeamerMenuBackItemTest.php | 118 ++++++++++++++++++ 17 files changed, 298 insertions(+), 58 deletions(-) create mode 100644 src/Menu/MenuBuilder.php create mode 100644 tests/Menu/TeamerMenuBackItemTest.php diff --git a/config/services.yaml b/config/services.yaml index b9d6968..5a125ff 100644 --- a/config/services.yaml +++ b/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' diff --git a/src/Menu/AbstractMenuBuilder.php b/src/Menu/AbstractMenuBuilder.php index e30eb5e..b800ed9 100644 --- a/src/Menu/AbstractMenuBuilder.php +++ b/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 $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')) { diff --git a/src/Menu/AdminMenuBuilder.php b/src/Menu/AdminMenuBuilder.php index 25a136e..a8584ed 100644 --- a/src/Menu/AdminMenuBuilder.php +++ b/src/Menu/AdminMenuBuilder.php @@ -337,16 +337,7 @@ class AdminMenuBuilder extends AbstractMenuBuilder ], ]); - $this->addDivider($menu); - - $request = $this->requestStack->getMainRequest(); - - $menu->addChild('zurück zur Übersicht', [ - 'uri' => rawurldecode($request->get('r')), - 'extras' => [ - 'icon' => 'arrow-left', - ], - ]); + $this->addBackItem($menu, $options); return $menu; } diff --git a/src/Menu/ManagerMenuBuilder.php b/src/Menu/ManagerMenuBuilder.php index c15fc90..7f6e9f1 100644 --- a/src/Menu/ManagerMenuBuilder.php +++ b/src/Menu/ManagerMenuBuilder.php @@ -204,16 +204,7 @@ class ManagerMenuBuilder extends AbstractMenuBuilder ], ]); - $this->addDivider($menu); - - $request = $this->requestStack->getMainRequest(); - - $menu->addChild('zurück zur Übersicht', [ - 'uri' => rawurldecode($request->get('r')), - 'extras' => [ - 'icon' => 'arrow-left', - ], - ]); + $this->addBackItem($menu, $options); return $menu; } diff --git a/src/Menu/MenuBuilder.php b/src/Menu/MenuBuilder.php new file mode 100644 index 0000000..56ed8c0 --- /dev/null +++ b/src/Menu/MenuBuilder.php @@ -0,0 +1,51 @@ +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(), + }; + } +} diff --git a/templates/admin/layout.html.twig b/templates/admin/layout.html.twig index 1248458..80c7426 100644 --- a/templates/admin/layout.html.twig +++ b/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 %} diff --git a/templates/admin/teamer/availability/index.html.twig b/templates/admin/teamer/availability/index.html.twig index a702bb3..6bca95a 100644 --- a/templates/admin/teamer/availability/index.html.twig +++ b/templates/admin/teamer/availability/index.html.twig @@ -5,7 +5,7 @@ {% block content %}
- {{ knp_menu_render(knp_menu_get('admin_teamer')) }} + {{ knp_menu_render(knp_menu_get('default_teamer')) }}

diff --git a/templates/admin/teamer/crm_selections.html.twig b/templates/admin/teamer/crm_selections.html.twig index 6008d69..24abef5 100644 --- a/templates/admin/teamer/crm_selections.html.twig +++ b/templates/admin/teamer/crm_selections.html.twig @@ -5,7 +5,7 @@ {% block content %}
- {{ knp_menu_render(knp_menu_get('admin_teamer')) }} + {{ knp_menu_render(knp_menu_get('default_teamer')) }}

diff --git a/templates/admin/teamer/remarks_internal.html.twig b/templates/admin/teamer/remarks_internal.html.twig index a9d1ee0..5eb11bf 100644 --- a/templates/admin/teamer/remarks_internal.html.twig +++ b/templates/admin/teamer/remarks_internal.html.twig @@ -5,7 +5,7 @@ {% block content %}
- {{ knp_menu_render(knp_menu_get('admin_teamer')) }} + {{ knp_menu_render(knp_menu_get('default_teamer')) }}

diff --git a/templates/admin/teamer/skills.html.twig b/templates/admin/teamer/skills.html.twig index 6184939..f97f827 100644 --- a/templates/admin/teamer/skills.html.twig +++ b/templates/admin/teamer/skills.html.twig @@ -5,7 +5,7 @@ {% block content %}
- {{ knp_menu_render(knp_menu_get('admin_teamer')) }} + {{ knp_menu_render(knp_menu_get('default_teamer')) }}

diff --git a/templates/administrative/availability/index.html.twig b/templates/administrative/availability/index.html.twig index 3d88682..d4cbe6b 100644 --- a/templates/administrative/availability/index.html.twig +++ b/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 %} diff --git a/templates/administrative/layout.html.twig b/templates/administrative/layout.html.twig index 990b647..80c7426 100644 --- a/templates/administrative/layout.html.twig +++ b/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('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 %} + {{ knp_menu_render(knp_menu_get('default_main')) }} {% endblock %} {% block mobile_menu %} - {% if is_granted('ROLE_MANAGER') %} - {{ 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 %} + {{ knp_menu_render(knp_menu_get('default_main')) }} {% endblock %} diff --git a/templates/administrative/teamer/profile.html.twig b/templates/administrative/teamer/profile.html.twig index 0fc42f1..7a2a0fb 100644 --- a/templates/administrative/teamer/profile.html.twig +++ b/templates/administrative/teamer/profile.html.twig @@ -5,11 +5,7 @@ {% block content %}
- {% if is_granted('ROLE_MANAGER') %} - {{ 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 %} + {{ knp_menu_render(knp_menu_get('default_teamer', [], { 'return_url': returnUrl })) }}
diff --git a/templates/administrative/teamer/recent_assignments.html.twig b/templates/administrative/teamer/recent_assignments.html.twig index bbad465..5f4f672 100644 --- a/templates/administrative/teamer/recent_assignments.html.twig +++ b/templates/administrative/teamer/recent_assignments.html.twig @@ -5,11 +5,7 @@ {% block content %}
- {% if is_granted('ROLE_MANAGER') %} - {{ knp_menu_render(knp_menu_get('manager_teamer')) }} - {% elseif is_granted('ROLE_ADMIN') %} - {{ knp_menu_render(knp_menu_get('admin_teamer')) }} - {% endif %} + {{ knp_menu_render(knp_menu_get('default_teamer')) }}

diff --git a/templates/administrative/teamer/upcoming_assignments.html.twig b/templates/administrative/teamer/upcoming_assignments.html.twig index 7f9bb65..579a9a2 100644 --- a/templates/administrative/teamer/upcoming_assignments.html.twig +++ b/templates/administrative/teamer/upcoming_assignments.html.twig @@ -5,11 +5,7 @@ {% block content %}
- {% if is_granted('ROLE_MANAGER') %} - {{ knp_menu_render(knp_menu_get('manager_teamer')) }} - {% elseif is_granted('ROLE_ADMIN') %} - {{ knp_menu_render(knp_menu_get('admin_teamer')) }} - {% endif %} + {{ knp_menu_render(knp_menu_get('default_teamer')) }}

diff --git a/templates/teamer/layout.html.twig b/templates/teamer/layout.html.twig index d8c58c8..80c7426 100644 --- a/templates/teamer/layout.html.twig +++ b/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 %} diff --git a/tests/Menu/TeamerMenuBackItemTest.php b/tests/Menu/TeamerMenuBackItemTest.php new file mode 100644 index 0000000..65b4432 --- /dev/null +++ b/tests/Menu/TeamerMenuBackItemTest.php @@ -0,0 +1,118 @@ +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 + */ + 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 $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; + } +}