Skip to content

Commit 17edcbf

Browse files
derhansenderhansen
committed
[SECURITY] Added access checks to backend module
* Event access is checked for all actions with event parameters * List view checks if current PID is in users webmount * EventRepository considers zero as constraint for storagePage restrictions
1 parent 24268e0 commit 17edcbf

File tree

6 files changed

+124
-8
lines changed

6 files changed

+124
-8
lines changed

‎Classes/Controller/AdministrationController.php

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use TYPO3\CMS\Backend\Template\Components\ButtonBar;
1919
use TYPO3\CMS\Backend\Utility\BackendUtility;
2020
use TYPO3\CMS\Backend\View\BackendTemplateView;
21+
use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
2122
use TYPO3\CMS\Core\Imaging\Icon;
2223
use TYPO3\CMS\Core\Imaging\IconFactory;
2324
use TYPO3\CMS\Core\Localization\LanguageService;
@@ -336,9 +337,14 @@ public function listAction(SearchDemand $searchDemand = null, array $overwriteDe
336337
$eventDemand->setSearchDemand($searchDemand);
337338
$eventDemand->setStoragePage($this->pid);
338339

340+
$events = [];
341+
if ($this->getBackendUser()->isInWebMount($this->pid)) {
342+
$events = $this->eventRepository->findDemanded($eventDemand);
343+
}
344+
339345
$this->view->assignMultiple([
340346
'pid' => $this->pid,
341-
'events' => $this->eventRepository->findDemanded($eventDemand),
347+
'events' => $events,
342348
'searchDemand' => $searchDemand,
343349
'orderByFields' => $this->getOrderByFields(),
344350
'orderDirections' => $this->getOrderDirections(),
@@ -353,7 +359,12 @@ public function listAction(SearchDemand $searchDemand = null, array $overwriteDe
353359
*/
354360
public function exportAction($eventUid)
355361
{
356-
$this->exportService->downloadRegistrationsCsv($eventUid, $this->settings['csvExport']);
362+
/** @var Event $event */
363+
$event = $this->eventRepository->findByUid($eventUid);
364+
if ($event) {
365+
$this->checkEventAccess($event);
366+
$this->exportService->downloadRegistrationsCsv($eventUid, $this->settings['csvExport']);
367+
}
357368
exit();
358369
}
359370

@@ -381,6 +392,7 @@ public function handleExpiredRegistrationsAction()
381392
*/
382393
public function indexNotifyAction(Event $event)
383394
{
395+
$this->checkEventAccess($event);
384396
$customNotification = GeneralUtility::makeInstance(CustomNotification::class);
385397
$customNotifications = $this->settingsService->getCustomNotifications($this->settings);
386398
$logEntries = $this->customNotificationLogRepository->findByEvent($event);
@@ -430,6 +442,7 @@ public function getNotificationRecipients(): array
430442
*/
431443
public function notifyAction(Event $event, CustomNotification $customNotification)
432444
{
445+
$this->checkEventAccess($event);
433446
$customNotifications = $this->settingsService->getCustomNotifications($this->settings);
434447
$result = $this->notificationService->sendCustomNotification($event, $customNotification, $this->settings);
435448
$this->notificationService->createCustomNotificationLogentry(
@@ -445,6 +458,26 @@ public function notifyAction(Event $event, CustomNotification $customNotificatio
445458
$this->redirect('list');
446459
}
447460

461+
/**
462+
* Checks if the current backend user has access to the PID of the event and if not, enqueue an
463+
* access denied flash message and redirect to list view
464+
*
465+
* @param Event $event
466+
* @throws \TYPO3\CMS\Extbase\Mvc\Exception\StopActionException
467+
*/
468+
public function checkEventAccess(Event $event)
469+
{
470+
if ($this->getBackendUser()->isInWebMount($event->getPid()) === null) {
471+
$this->addFlashMessage(
472+
$this->getLanguageService()->sL(self::LANG_FILE . 'administration.accessdenied.content'),
473+
$this->getLanguageService()->sL(self::LANG_FILE . 'administration.accessdenied.title'),
474+
FlashMessage::ERROR
475+
);
476+
477+
$this->redirect('list');
478+
}
479+
}
480+
448481
/**
449482
* Shows the settings error view
450483
*/
@@ -498,4 +531,13 @@ public function getOrderByFields()
498531
'enddate' => $this->getLanguageService()->sL(self::LANG_FILE . 'administration.orderBy.enddate')
499532
];
500533
}
534+
535+
/**
536+
* Returns the Backend User
537+
* @return BackendUserAuthentication
538+
*/
539+
protected function getBackendUser(): BackendUserAuthentication
540+
{
541+
return $GLOBALS['BE_USER'];
542+
}
501543
}

‎Classes/Domain/Repository/EventRepository.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ protected function setOrderingsFromDemand($query, EventDemand $eventDemand)
144144
*/
145145
protected function setStoragePageConstraint($query, $eventDemand, &$constraints)
146146
{
147-
if ($eventDemand->getStoragePage() && $eventDemand->getStoragePage() !== '') {
147+
if ($eventDemand->getStoragePage() !== null && $eventDemand->getStoragePage() !== '') {
148148
$pidList = GeneralUtility::intExplode(',', $eventDemand->getStoragePage(), true);
149149
$constraints[] = $query->in('pid', $pidList);
150150
}

‎Resources/Private/Language/locallang_be.xlf

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,12 @@
381381
<trans-unit id="administration.orderBy.enddate">
382382
<source>Enddate</source>
383383
</trans-unit>
384+
<trans-unit id="administration.accessdenied.title">
385+
<source>Access Denied</source>
386+
</trans-unit>
387+
<trans-unit id="administration.accessdenied.content">
388+
<source>Access to the given event has been denied.</source>
389+
</trans-unit>
384390
</body>
385391
</file>
386392
</xliff>

‎Tests/Functional/Repository/EventRepositoryTest.php

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -71,19 +71,47 @@ public function findRecordsByUid()
7171
self::assertSame($events->getTitle(), 'findRecordsByUid');
7272
}
7373

74+
/**
75+
* @return array
76+
*/
77+
public function findDemandedRecordsByStoragePageDataProvider(): array
78+
{
79+
return [
80+
'pid is numeric and valid' => [
81+
3,
82+
3
83+
],
84+
'pid is string and valid' => [
85+
'3',
86+
3
87+
],
88+
'pid is zero' => [
89+
0,
90+
0
91+
],
92+
'pid not set' => [
93+
null,
94+
47
95+
]
96+
];
97+
}
98+
7499
/**
75100
* Test if storagePage restriction in demand works
76-
*
101+
* @dataProvider findDemandedRecordsByStoragePageDataProvider
77102
* @test
103+
* @param $pid
104+
* @param $expected
105+
* @throws \TYPO3\CMS\Extbase\Object\Exception
78106
*/
79-
public function findDemandedRecordsByStoragePage()
107+
public function findDemandedRecordsByStoragePage($pid, $expected)
80108
{
81109
/** @var \DERHANSEN\SfEventMgt\Domain\Model\Dto\EventDemand $demand */
82110
$demand = $this->objectManager->get(EventDemand::class);
83-
$demand->setStoragePage(3);
111+
$demand->setStoragePage($pid);
84112
$events = $this->eventRepository->findDemanded($demand);
85113

86-
self::assertSame(3, $events->count());
114+
self::assertSame($expected, $events->count());
87115
}
88116

89117
/**

‎Tests/Unit/Controller/AdministrationControllerTest.php

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use DERHANSEN\SfEventMgt\Service\MaintenanceService;
2121
use DERHANSEN\SfEventMgt\Service\NotificationService;
2222
use DERHANSEN\SfEventMgt\Service\SettingsService;
23+
use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
2324
use TYPO3\CMS\Core\Utility\GeneralUtility;
2425
use TYPO3\CMS\Extbase\Mvc\Controller\Argument;
2526
use TYPO3\CMS\Extbase\Mvc\Controller\Arguments;
@@ -100,6 +101,10 @@ public function listActionDoesNotFetchEventsForStoragePidZero()
100101
$beUserSessionService->expects(self::any())->method('getSessionDataByKey');
101102
$this->inject($this->subject, 'beUserSessionService', $beUserSessionService);
102103

104+
$mockBackendUser = $this->getMockBuilder(BackendUserAuthentication::class)->getMock();
105+
$mockBackendUser->expects($this->once())->method('isInWebMount')->will($this->returnValue(1));
106+
$GLOBALS['BE_USER'] = $mockBackendUser;
107+
103108
$eventRepository = $this->getMockBuilder(EventRepository::class)
104109
->setMethods(['findDemanded'])
105110
->disableOriginalConstructor()
@@ -148,6 +153,10 @@ public function listActionDoesNotFetchEventsForStoragePidZeroAndDemand()
148153
$beUserSessionService->expects(self::once())->method('saveSessionData');
149154
$this->inject($this->subject, 'beUserSessionService', $beUserSessionService);
150155

156+
$mockBackendUser = $this->getMockBuilder(BackendUserAuthentication::class)->getMock();
157+
$mockBackendUser->expects($this->once())->method('isInWebMount')->will($this->returnValue(1));
158+
$GLOBALS['BE_USER'] = $mockBackendUser;
159+
151160
$eventRepository = $this->getMockBuilder(EventRepository::class)
152161
->setMethods(['findDemanded'])
153162
->disableOriginalConstructor()
@@ -196,6 +205,10 @@ public function listActionFetchesAllEventsForGivenStoragePidAndAssignsThemToView
196205
$beUserSessionService->expects(self::once())->method('saveSessionData');
197206
$this->inject($this->subject, 'beUserSessionService', $beUserSessionService);
198207

208+
$mockBackendUser = $this->getMockBuilder(BackendUserAuthentication::class)->getMock();
209+
$mockBackendUser->expects($this->once())->method('isInWebMount')->will($this->returnValue(1));
210+
$GLOBALS['BE_USER'] = $mockBackendUser;
211+
199212
$eventRepository = $this->getMockBuilder(EventRepository::class)
200213
->disableOriginalConstructor()
201214
->getMock();
@@ -240,6 +253,10 @@ public function listActionUsesOverwriteDemandArrayAndAssignsItToView()
240253
$beUserSessionService->expects(self::once())->method('saveSessionData');
241254
$this->inject($this->subject, 'beUserSessionService', $beUserSessionService);
242255

256+
$mockBackendUser = $this->getMockBuilder(BackendUserAuthentication::class)->getMock();
257+
$mockBackendUser->expects($this->once())->method('isInWebMount')->will($this->returnValue(1));
258+
$GLOBALS['BE_USER'] = $mockBackendUser;
259+
243260
$eventRepository = $this->getMockBuilder(EventRepository::class)
244261
->disableOriginalConstructor()
245262
->getMock();
@@ -382,6 +399,10 @@ public function indexNotifyActionAssignsExpectedObjectsToView()
382399

383400
$mockCustomNotification = GeneralUtility::makeInstance(CustomNotification::class);
384401

402+
$mockBackendUser = $this->getMockBuilder(BackendUserAuthentication::class)->getMock();
403+
$mockBackendUser->expects($this->once())->method('isInWebMount')->will($this->returnValue(1));
404+
$GLOBALS['BE_USER'] = $mockBackendUser;
405+
385406
$recipients = $this->subject->getNotificationRecipients();
386407

387408
$view = $this->getMockBuilder(ViewInterface::class)->getMock();
@@ -421,10 +442,29 @@ public function notifyActionSendsNotificationsLogsAndRedirects()
421442
$mockNotificationService->expects(self::once())->method('createCustomNotificationLogentry');
422443
$this->inject($this->subject, 'notificationService', $mockNotificationService);
423444

445+
$mockBackendUser = $this->getMockBuilder(BackendUserAuthentication::class)->getMock();
446+
$mockBackendUser->expects($this->once())->method('isInWebMount')->will($this->returnValue(1));
447+
$GLOBALS['BE_USER'] = $mockBackendUser;
448+
424449
$customNotification = new CustomNotification();
425450

426451
$this->subject->_set('settings', []);
427452
$this->subject->expects(self::once())->method('redirect');
428453
$this->subject->notifyAction($event, $customNotification);
429454
}
455+
456+
/**
457+
* @test
458+
*/
459+
public function checkEventAccessRedirectsToListViewIfNoEventAccess()
460+
{
461+
$event = new Event();
462+
463+
$mockBackendUser = $this->getMockBuilder(BackendUserAuthentication::class)->getMock();
464+
$mockBackendUser->expects($this->once())->method('isInWebMount')->will($this->returnValue(null));
465+
$GLOBALS['BE_USER'] = $mockBackendUser;
466+
467+
$this->subject->expects(self::once())->method('redirect');
468+
$this->subject->checkEventAccess($event);
469+
}
430470
}

‎ext_emconf.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
'uploadfolder' => '0',
2121
'createDirs' => '',
2222
'clearCacheOnLoad' => 1,
23-
'version' => '5.1.0',
23+
'version' => '5.1.1',
2424
'constraints' => [
2525
'depends' => [
2626
'typo3' => '10.4.2-10.4.99',

0 commit comments

Comments
 (0)