Skip to content

Commit a643d3e

Browse files
o-bao-ba
committed
[BUGFIX] Fix access checks for nested modules
Module access checks in ModuleProvider were only performed up to 2 levels deep. While direct access to this modules is prevented by the middleware, this led to problems on calculation of menus and also led to wrong behaviour in combination with the new `dependsOnSubmodules` functionality (#107663). This patch introduces recursive access filtering to properly check permissions at all nesting levels. Additionally, this is now verified by a couple of new tests. Resolves: #107715 Releases: main, 13.4 Change-Id: Ia995fb99f4b3259c9f905483697dec1a3a63e015 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/91097 Reviewed-by: Oli Bartsch <bo@cedev.de> Tested-by: Benni Mack <benni@typo3.org> Tested-by: core-ci <typo3@b13.com> Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch> Tested-by: Christian Kuhn <lolli@schwarzbu.ch> Tested-by: Oli Bartsch <bo@cedev.de> Reviewed-by: Benni Mack <benni@typo3.org>
1 parent ec380cc commit a643d3e

File tree

2 files changed

+278
-57
lines changed

2 files changed

+278
-57
lines changed

‎typo3/sysext/backend/Classes/Module/ModuleProvider.php‎

Lines changed: 87 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,8 @@ public function getModule(
5050
|| $this->accessGranted($identifier, $user, $respectWorkspaceRestrictions)
5151
) {
5252
$module = $this->moduleRegistry->getModule($identifier);
53-
if ($module->hasSubModules()) {
54-
foreach ($module->getSubModules() as $subModuleIdentifier => $subModule) {
55-
if ($user !== null && !$this->accessGranted($subModuleIdentifier, $user, $respectWorkspaceRestrictions)) {
56-
$module->removeSubModule($subModuleIdentifier);
57-
}
58-
}
53+
if ($user !== null) {
54+
$this->filterInaccessibleSubModules($module, $user, $respectWorkspaceRestrictions);
5955
}
6056
return $module;
6157
}
@@ -88,10 +84,8 @@ public function getModules(
8884
unset($availableModules[$identifier]);
8985
continue;
9086
}
91-
foreach ($module->getSubModules() as $subModuleIdentifier => $subModule) {
92-
if ($user !== null && !$this->accessGranted($subModuleIdentifier, $user, $respectWorkspaceRestrictions)) {
93-
$module->removeSubModule($subModuleIdentifier);
94-
}
87+
if ($user !== null) {
88+
$this->filterInaccessibleSubModules($module, $user, $respectWorkspaceRestrictions);
9589
}
9690
}
9791

@@ -121,28 +115,7 @@ public function getModuleForMenu(
121115
if ($menuItem->isStandalone()) {
122116
return $menuItem;
123117
}
124-
foreach ($module->getSubModules() as $subModuleIdentifier => $subModule) {
125-
if (in_array($subModuleIdentifier, $hideModules, true)) {
126-
continue;
127-
}
128-
// Skip submodules that depend on their own submodules if they don't have any accessible ones
129-
if ($subModule->getDependsOnSubmodules()) {
130-
$hasAccessibleSubmodules = false;
131-
foreach ($subModule->getSubModules() as $thirdLevelIdentifier => $thirdLevelModule) {
132-
if (!in_array($thirdLevelIdentifier, $hideModules, true)
133-
&& $this->accessGranted($thirdLevelIdentifier, $user, $respectWorkspaceRestrictions)
134-
) {
135-
$hasAccessibleSubmodules = true;
136-
break;
137-
}
138-
}
139-
if (!$hasAccessibleSubmodules) {
140-
continue;
141-
}
142-
}
143-
$subMenuItem = new MenuModule(clone $subModule);
144-
$menuItem->addSubModule($subMenuItem);
145-
}
118+
$this->buildMenuModuleRecursively($menuItem, $module, $hideModules, $user, $respectWorkspaceRestrictions);
146119
if (!$menuItem->hasSubModules()) {
147120
// In case the main module does not have any submodules, unset it again
148121
return null;
@@ -181,31 +154,7 @@ public function getModulesForModuleMenu(
181154
if ($menuItem->isStandalone()) {
182155
continue;
183156
}
184-
foreach ($module->getSubModules() as $subModuleIdentifier => $subModule) {
185-
if (in_array($subModuleIdentifier, $hideModules, true)
186-
|| !($subModule->getAppearance()['renderInModuleMenu'] ?? true)
187-
) {
188-
continue;
189-
}
190-
// Skip submodules that depend on their own submodules if they don't have any accessible ones
191-
if ($subModule->getDependsOnSubmodules()) {
192-
$hasAccessibleSubmodules = false;
193-
foreach ($subModule->getSubModules() as $thirdLevelIdentifier => $thirdLevelModule) {
194-
if (!in_array($thirdLevelIdentifier, $hideModules, true)
195-
&& ($thirdLevelModule->getAppearance()['renderInModuleMenu'] ?? true)
196-
&& $this->accessGranted($thirdLevelIdentifier, $user, $respectWorkspaceRestrictions)
197-
) {
198-
$hasAccessibleSubmodules = true;
199-
break;
200-
}
201-
}
202-
if (!$hasAccessibleSubmodules) {
203-
continue;
204-
}
205-
}
206-
$subMenuItem = new MenuModule(clone $subModule);
207-
$menuItem->addSubModule($subMenuItem);
208-
}
157+
$this->buildMenuModuleRecursively($menuItem, $module, $hideModules, $user, $respectWorkspaceRestrictions, true);
209158
if (!$menuItem->hasSubModules()) {
210159
// In case the main module does not have any submodules, unset it again
211160
unset($moduleMenuItems[$identifier]);
@@ -302,6 +251,87 @@ public function getUserModules(): array
302251
return array_filter($this->moduleRegistry->getModules(), static fn(ModuleInterface $module): bool => $module->getAccess() === 'user');
303252
}
304253

254+
/**
255+
* Recursively removes inaccessible submodules from a module at any depth
256+
*/
257+
protected function filterInaccessibleSubModules(
258+
ModuleInterface $module,
259+
BackendUserAuthentication $user,
260+
bool $respectWorkspaceRestrictions
261+
): void {
262+
if (!$module->hasSubModules()) {
263+
return;
264+
}
265+
266+
foreach ($module->getSubModules() as $subModuleIdentifier => $subModule) {
267+
if (!$this->accessGranted($subModuleIdentifier, $user, $respectWorkspaceRestrictions)) {
268+
$module->removeSubModule($subModuleIdentifier);
269+
} else {
270+
$this->filterInaccessibleSubModules($subModule, $user, $respectWorkspaceRestrictions);
271+
}
272+
}
273+
}
274+
275+
/**
276+
* Recursively builds menu module structure, checking access, TSConfig hideModules,
277+
* and optionally renderInModuleMenu appearance setting at all nesting levels
278+
*/
279+
protected function buildMenuModuleRecursively(
280+
MenuModule $menuItem,
281+
ModuleInterface $module,
282+
array $hideModules,
283+
BackendUserAuthentication $user,
284+
bool $respectWorkspaceRestrictions,
285+
bool $checkRenderInModuleMenu = false
286+
): void {
287+
foreach ($module->getSubModules() as $subModuleIdentifier => $subModule) {
288+
if (in_array($subModuleIdentifier, $hideModules, true)
289+
|| ($checkRenderInModuleMenu && !($subModule->getAppearance()['renderInModuleMenu'] ?? true))
290+
) {
291+
continue;
292+
}
293+
// Skip submodules that depend on their own submodules if they don't have any accessible ones
294+
if ($subModule->getDependsOnSubmodules() && !$this->hasAccessibleSubModules($subModule, $hideModules, $user, $respectWorkspaceRestrictions, $checkRenderInModuleMenu)) {
295+
continue;
296+
}
297+
$subMenuItem = new MenuModule(clone $subModule);
298+
$menuItem->addSubModule($subMenuItem);
299+
// Recursively build deeper levels
300+
if ($subModule->hasSubModules()) {
301+
$this->buildMenuModuleRecursively($subMenuItem, $subModule, $hideModules, $user, $respectWorkspaceRestrictions, $checkRenderInModuleMenu);
302+
}
303+
}
304+
}
305+
306+
/**
307+
* Check if a module has at least one accessible submodule at any depth
308+
*/
309+
protected function hasAccessibleSubModules(
310+
ModuleInterface $module,
311+
array $hideModules,
312+
BackendUserAuthentication $user,
313+
bool $respectWorkspaceRestrictions,
314+
bool $checkRenderInModuleMenu = false
315+
): bool {
316+
foreach ($module->getSubModules() as $subModuleIdentifier => $subModule) {
317+
if (in_array($subModuleIdentifier, $hideModules, true)
318+
|| ($checkRenderInModuleMenu && !($subModule->getAppearance()['renderInModuleMenu'] ?? true))
319+
|| !$this->accessGranted($subModuleIdentifier, $user, $respectWorkspaceRestrictions)
320+
) {
321+
continue;
322+
}
323+
// If this submodule is accessible, return true
324+
if (!$subModule->getDependsOnSubmodules()) {
325+
return true;
326+
}
327+
// If it depends on submodules, check recursively
328+
if ($this->hasAccessibleSubModules($subModule, $hideModules, $user, $respectWorkspaceRestrictions, $checkRenderInModuleMenu)) {
329+
return true;
330+
}
331+
}
332+
return false;
333+
}
334+
305335
/**
306336
* Check if user has access to module based on the identifier or an alias for the identifier
307337
*/

‎typo3/sysext/backend/Tests/Functional/Module/ModuleProviderTest.php‎

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
use TYPO3\CMS\Backend\Module\ModuleProvider;
2323
use TYPO3\CMS\Backend\Module\ModuleRegistry;
2424
use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
25+
use TYPO3\CMS\Core\Database\ConnectionPool;
2526
use TYPO3\TestingFramework\Core\Functional\FunctionalTestCase;
2627

2728
final class ModuleProviderTest extends FunctionalTestCase
@@ -327,4 +328,194 @@ public function modulesWithDependsOnSubmodulesAreHiddenWhenAllSubmodulesAreDenie
327328
$moduleMenuItems = $moduleProvider->getModulesForModuleMenu($user);
328329
self::assertArrayNotHasKey('main_module', $moduleMenuItems);
329330
}
331+
332+
#[Test]
333+
public function deepNestedModulesAreFilteredRecursivelyInGetModule(): void
334+
{
335+
// Test recursive access filtering at 5 levels deep
336+
$level1 = $this->get(ModuleFactory::class)->createModule('level1', []);
337+
$level2 = $this->get(ModuleFactory::class)->createModule('level2', ['parent' => 'level1']);
338+
$level3Accessible = $this->get(ModuleFactory::class)->createModule('level3_accessible', ['parent' => 'level2']);
339+
$level3Denied = $this->get(ModuleFactory::class)->createModule('level3_denied', ['parent' => 'level2', 'access' => 'user']);
340+
$level4Accessible = $this->get(ModuleFactory::class)->createModule('level4_accessible', ['parent' => 'level3_accessible']);
341+
$level4Denied = $this->get(ModuleFactory::class)->createModule('level4_denied', ['parent' => 'level3_accessible', 'access' => 'admin']);
342+
$level5Accessible = $this->get(ModuleFactory::class)->createModule('level5_accessible', ['parent' => 'level4_accessible', 'access' => 'user']);
343+
$level5Denied = $this->get(ModuleFactory::class)->createModule('level5_denied', ['parent' => 'level4_accessible', 'access' => 'user']);
344+
345+
$moduleRegistry = new ModuleRegistry([
346+
$level1, $level2, $level3Accessible, $level3Denied,
347+
$level4Accessible, $level4Denied, $level5Accessible, $level5Denied,
348+
]);
349+
$moduleProvider = new ModuleProvider($moduleRegistry);
350+
351+
$user = new BackendUserAuthentication();
352+
$user->workspace = 0;
353+
$user->groupData['modules'] = 'level5_accessible'; // Only access to level5_accessible
354+
355+
// Get the top-level module with user context - should recursively filter all levels
356+
$module = $moduleProvider->getModule('level1', $user);
357+
self::assertNotNull($module);
358+
359+
// Level 2 should be present
360+
self::assertTrue($module->hasSubModule('level2'));
361+
$level2Module = $module->getSubModule('level2');
362+
363+
// Level 3: accessible should be present, denied should be removed
364+
self::assertTrue($level2Module->hasSubModule('level3_accessible'));
365+
self::assertFalse($level2Module->hasSubModule('level3_denied'), 'level3_denied should be removed due to user access restrictions');
366+
367+
$level3Module = $level2Module->getSubModule('level3_accessible');
368+
369+
// Level 4: accessible should be present, denied (admin only) should be removed
370+
self::assertTrue($level3Module->hasSubModule('level4_accessible'));
371+
self::assertFalse($level3Module->hasSubModule('level4_denied'), 'level4_denied should be removed due to admin access requirement');
372+
373+
$level4Module = $level3Module->getSubModule('level4_accessible');
374+
375+
// Level 5: accessible should be present, denied should be removed
376+
self::assertTrue($level4Module->hasSubModule('level5_accessible'));
377+
self::assertFalse($level4Module->hasSubModule('level5_denied'), 'level5_denied should be removed due to user access restrictions');
378+
}
379+
380+
#[Test]
381+
public function deepNestedModulesAreFilteredRecursivelyInGetModules(): void
382+
{
383+
// Test recursive filtering in getModules() with grouped=true
384+
$main1 = $this->get(ModuleFactory::class)->createModule('main1', []);
385+
$main2 = $this->get(ModuleFactory::class)->createModule('main2', []);
386+
$sub1 = $this->get(ModuleFactory::class)->createModule('sub1', ['parent' => 'main1']);
387+
$sub2 = $this->get(ModuleFactory::class)->createModule('sub2', ['parent' => 'main2', 'access' => 'admin']);
388+
$subsub1 = $this->get(ModuleFactory::class)->createModule('subsub1', ['parent' => 'sub1']);
389+
$subsub2 = $this->get(ModuleFactory::class)->createModule('subsub2', ['parent' => 'sub1', 'access' => 'user']);
390+
$subsubsub1 = $this->get(ModuleFactory::class)->createModule('subsubsub1', ['parent' => 'subsub1', 'access' => 'user']);
391+
$subsubsub2 = $this->get(ModuleFactory::class)->createModule('subsubsub2', ['parent' => 'subsub1', 'access' => 'user']);
392+
393+
$moduleRegistry = new ModuleRegistry([
394+
$main1, $main2, $sub1, $sub2, $subsub1, $subsub2, $subsubsub1, $subsubsub2,
395+
]);
396+
$moduleProvider = new ModuleProvider($moduleRegistry);
397+
398+
$user = new BackendUserAuthentication();
399+
$user->workspace = 0;
400+
$user->groupData['modules'] = 'subsubsub1'; // Only access to subsubsub1
401+
402+
$modules = $moduleProvider->getModules($user, true, true);
403+
404+
// main1 should be present
405+
self::assertArrayHasKey('main1', $modules);
406+
$main1Module = $modules['main1'];
407+
408+
// main2 should be present (even though sub2 is admin-only)
409+
self::assertArrayHasKey('main2', $modules);
410+
$main2Module = $modules['main2'];
411+
412+
// sub1 should be present, but sub2 (admin) should be removed from main2
413+
self::assertTrue($main1Module->hasSubModule('sub1'));
414+
self::assertFalse($main2Module->hasSubModule('sub2'), 'sub2 should be removed due to admin access requirement');
415+
416+
$sub1Module = $main1Module->getSubModule('sub1');
417+
418+
// subsub1 should be present, subsub2 (user without access) should be removed
419+
self::assertTrue($sub1Module->hasSubModule('subsub1'));
420+
self::assertFalse($sub1Module->hasSubModule('subsub2'), 'subsub2 should be removed due to user access restrictions');
421+
422+
$subsub1Module = $sub1Module->getSubModule('subsub1');
423+
424+
// subsubsub1 should be present (user has access), subsubsub2 should be removed
425+
self::assertTrue($subsub1Module->hasSubModule('subsubsub1'));
426+
self::assertFalse($subsub1Module->hasSubModule('subsubsub2'), 'subsubsub2 should be removed due to user access restrictions');
427+
}
428+
429+
#[Test]
430+
public function deepNestedModulesWithHideModulesInTSConfigAreFilteredInMenuMethods(): void
431+
{
432+
// Test that TSConfig hideModules works recursively at all levels
433+
$main = $this->get(ModuleFactory::class)->createModule('main', []);
434+
$level2 = $this->get(ModuleFactory::class)->createModule('level2', ['parent' => 'main']);
435+
$level3a = $this->get(ModuleFactory::class)->createModule('level3a', ['parent' => 'level2']);
436+
$level3b = $this->get(ModuleFactory::class)->createModule('level3b', ['parent' => 'level2']);
437+
$level4a = $this->get(ModuleFactory::class)->createModule('level4a', ['parent' => 'level3a']);
438+
$level4b = $this->get(ModuleFactory::class)->createModule('level4b', ['parent' => 'level3b']);
439+
440+
$moduleRegistry = new ModuleRegistry([$main, $level2, $level3a, $level3b, $level4a, $level4b]);
441+
$moduleProvider = new ModuleProvider($moduleRegistry);
442+
443+
// Set TSConfig via database
444+
$this->importCSVDataSet(__DIR__ . '/../Fixtures/be_users.csv');
445+
$this->get(ConnectionPool::class)
446+
->getConnectionForTable('be_users')
447+
->update(
448+
'be_users',
449+
['TSconfig' => 'options.hideModules = level3b,level4a'],
450+
['uid' => 1]
451+
);
452+
$user = $this->setUpBackendUser(1);
453+
$user->workspace = 0;
454+
455+
// Test getModuleForMenu
456+
$menuModule = $moduleProvider->getModuleForMenu('main', $user);
457+
self::assertNotNull($menuModule);
458+
self::assertTrue($menuModule->hasSubModule('level2'));
459+
460+
$level2Menu = $menuModule->getSubModule('level2');
461+
// level3a should be present, level3b should be hidden by TSConfig
462+
self::assertTrue($level2Menu->hasSubModule('level3a'));
463+
self::assertFalse($level2Menu->hasSubModule('level3b'), 'level3b should be hidden by TSConfig');
464+
465+
$level3aMenu = $level2Menu->getSubModule('level3a');
466+
// level4a should be hidden by TSConfig
467+
self::assertFalse($level3aMenu->hasSubModule('level4a'), 'level4a should be hidden by TSConfig');
468+
469+
// Test getModulesForModuleMenu
470+
$moduleMenuItems = $moduleProvider->getModulesForModuleMenu($user);
471+
self::assertArrayHasKey('main', $moduleMenuItems);
472+
$mainMenuItem = $moduleMenuItems['main'];
473+
self::assertTrue($mainMenuItem->hasSubModule('level2'));
474+
$level2MenuItem = $mainMenuItem->getSubModule('level2');
475+
self::assertTrue($level2MenuItem->hasSubModule('level3a'));
476+
self::assertFalse($level2MenuItem->hasSubModule('level3b'), 'level3b should be hidden by TSConfig in module menu');
477+
}
478+
479+
#[Test]
480+
public function deepNestedModulesWithDependsOnSubmodulesAtMultipleLevels(): void
481+
{
482+
// Test dependsOnSubmodules at multiple nesting levels
483+
$main = $this->get(ModuleFactory::class)->createModule('main', []);
484+
$level2 = $this->get(ModuleFactory::class)->createModule('level2', [
485+
'parent' => 'main',
486+
'dependsOnSubmodules' => true,
487+
]);
488+
$level3 = $this->get(ModuleFactory::class)->createModule('level3', [
489+
'parent' => 'level2',
490+
'dependsOnSubmodules' => true,
491+
]);
492+
$level4accessible = $this->get(ModuleFactory::class)->createModule('level4accessible', [
493+
'parent' => 'level3',
494+
'access' => 'user',
495+
]);
496+
$level4denied = $this->get(ModuleFactory::class)->createModule('level4denied', [
497+
'parent' => 'level3',
498+
'access' => 'user',
499+
]);
500+
501+
$moduleRegistry = new ModuleRegistry([$main, $level2, $level3, $level4accessible, $level4denied]);
502+
$moduleProvider = new ModuleProvider($moduleRegistry);
503+
504+
$user = new BackendUserAuthentication();
505+
$user->workspace = 0;
506+
$user->groupData['modules'] = 'level4accessible'; // Only access to level4accessible
507+
508+
// With access to level4accessible, the entire chain should be visible
509+
$menuModule = $moduleProvider->getModuleForMenu('main', $user);
510+
self::assertNotNull($menuModule);
511+
self::assertTrue($menuModule->hasSubModule('level2'), 'level2 with dependsOnSubmodules should be visible when nested accessible modules exist');
512+
513+
$level2Menu = $menuModule->getSubModule('level2');
514+
self::assertTrue($level2Menu->hasSubModule('level3'), 'level3 with dependsOnSubmodules should be visible when nested accessible modules exist');
515+
516+
// Now test with no access - the entire chain should be hidden
517+
$user->groupData['modules'] = '';
518+
$menuModuleNoAccess = $moduleProvider->getModuleForMenu('main', $user);
519+
self::assertNull($menuModuleNoAccess, 'main module should be null when nested dependsOnSubmodules modules have no accessible children');
520+
}
330521
}

0 commit comments

Comments
 (0)