diff --git a/config/routes/annotations.yaml b/config/routes/annotations.yaml index 0c3ab74f..aed75379 100644 --- a/config/routes/annotations.yaml +++ b/config/routes/annotations.yaml @@ -11,3 +11,8 @@ app.api: resource: '../src/API/' type: rest prefix: /api + +security: + resource: '../../src/Controller/Auth/' + type: annotation + prefix: /auth diff --git a/config/services.yaml b/config/services.yaml index cdc1ea0f..36bf3473 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -20,7 +20,7 @@ services: # this creates a service per class whose id is the fully-qualified class name App\: resource: '../src/*' - exclude: '../src/{DependencyInjection,Entity,Event,Migrations,Tests,Kernel.php,Constants.php}' + exclude: '../src/{DependencyInjection,Entity,Event,Migrations,Model,Tests,Kernel.php,Constants.php}' # controllers are imported separately to make sure services can be injected # as action arguments even if you don't extend any base controller class diff --git a/src/Configuration/SystemConfiguration.php b/src/Configuration/SystemConfiguration.php index 462ca8e5..dd4a5200 100644 --- a/src/Configuration/SystemConfiguration.php +++ b/src/Configuration/SystemConfiguration.php @@ -23,6 +23,13 @@ class SystemConfiguration implements SystemBundleConfiguration return $repository->getConfiguration(); } + // ========== SAML configurations ========== + + public function isSamlActive(): bool + { + return (bool) $this->find('saml.activate'); + } + // ========== Calendar configurations ========== public function getCalendarBusinessDays(): array diff --git a/src/Saml/Controller/SamlController.php b/src/Controller/Auth/SamlController.php similarity index 72% rename from src/Saml/Controller/SamlController.php rename to src/Controller/Auth/SamlController.php index 9352ca5c..efceb7e4 100644 --- a/src/Saml/Controller/SamlController.php +++ b/src/Controller/Auth/SamlController.php @@ -7,8 +7,9 @@ * file that was distributed with this source code. */ -namespace App\Saml\Controller; +namespace App\Controller\Auth; +use App\Configuration\SystemConfiguration; use App\Saml\SamlAuth; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\Request; @@ -21,14 +22,13 @@ use Symfony\Component\Security\Core\Security; */ final class SamlController extends AbstractController { - /** - * @var SamlAuth - */ private $oneLoginAuth; + private $systemConfiguration; - public function __construct(SamlAuth $oneLoginAuth) + public function __construct(SamlAuth $oneLoginAuth, SystemConfiguration $systemConfiguration) { $this->oneLoginAuth = $oneLoginAuth; + $this->systemConfiguration = $systemConfiguration; } /** @@ -36,6 +36,10 @@ final class SamlController extends AbstractController */ public function loginAction(Request $request) { + if (!$this->systemConfiguration->isSamlActive()) { + throw $this->createNotFoundException('SAML deactivated'); + } + $session = $request->getSession(); $authErrorKey = Security::AUTHENTICATION_ERROR; @@ -60,6 +64,10 @@ final class SamlController extends AbstractController */ public function metadataAction() { + if (!$this->systemConfiguration->isSamlActive()) { + throw $this->createNotFoundException('SAML deactivated'); + } + $metadata = $this->oneLoginAuth->getSettings()->getSPMetadata(); $response = new Response($metadata); @@ -73,6 +81,10 @@ final class SamlController extends AbstractController */ public function assertionConsumerServiceAction() { + if (!$this->systemConfiguration->isSamlActive()) { + throw $this->createNotFoundException('SAML deactivated'); + } + throw new \RuntimeException('You must configure the check path in your firewall.'); } @@ -81,6 +93,10 @@ final class SamlController extends AbstractController */ public function logoutAction() { + if (!$this->systemConfiguration->isSamlActive()) { + throw $this->createNotFoundException('SAML deactivated'); + } + throw new \RuntimeException('You must configure the logout path in your firewall.'); } } diff --git a/src/Kernel.php b/src/Kernel.php index edbfa62a..7f0377a7 100644 --- a/src/Kernel.php +++ b/src/Kernel.php @@ -115,15 +115,13 @@ class Kernel extends BaseKernel yield new $class(); } } - - return; - } - - // ... or we load them dynamically from the plugins directory - foreach ($this->getBundleDirectories() as $bundleDir) { - $bundleName = $bundleDir->getRelativePathname(); - $pluginClass = 'KimaiPlugin\\' . $bundleName . '\\' . $bundleName; - yield new $pluginClass(); + } else { + // ... or we load them dynamically from the plugins directory + foreach ($this->getBundleDirectories() as $bundleDir) { + $bundleName = $bundleDir->getRelativePathname(); + $pluginClass = 'KimaiPlugin\\' . $bundleName . '\\' . $bundleName; + yield new $pluginClass(); + } } } @@ -209,7 +207,6 @@ class Kernel extends BaseKernel // some routes are based on app configs and will be imported manually $this->configureFosUserRoutes($routes); - $this->configureSamlRoutes($routes); // load bundle specific route files if (is_dir($confDir . '/routes/')) { @@ -251,15 +248,4 @@ class Kernel extends BaseKernel ); } } - - protected function configureSamlRoutes(RouteCollectionBuilder $routes) - { - $saml = $this->getContainer()->getParameter('kimai.saml'); - - if (!$saml['activate']) { - return; - } - - $routes->import('../src/Saml/Controller/SamlController.php', '/auth', 'annotation'); - } } diff --git a/tests/Configuration/SystemConfigurationTest.php b/tests/Configuration/SystemConfigurationTest.php index 3f394a39..d32b7b5f 100644 --- a/tests/Configuration/SystemConfigurationTest.php +++ b/tests/Configuration/SystemConfigurationTest.php @@ -91,7 +91,11 @@ class SystemConfigurationTest extends TestCase ] ], 'weekends' => true, - ] + ], + 'saml' => [ + 'activate' => false, + 'title' => 'Fantastic OAuth login' + ], ]; } @@ -135,14 +139,17 @@ class SystemConfigurationTest extends TestCase $this->assertEquals('RUB', $sut->find('defaults.customer.currency')); $this->assertTrue($sut->find('timesheet.rules.allow_future_times')); $this->assertEquals(7, $sut->find('timesheet.active_entries.hard_limit')); + $this->assertFalse($sut->isSamlActive()); } public function testDefaultWithMixedConfigs() { $sut = $this->getSut($this->getDefaultSettings(), [ (new Configuration())->setName('timesheet.rules.allow_future_times')->setValue(''), + (new Configuration())->setName('saml.activate')->setValue(true), ]); $this->assertFalse($sut->find('timesheet.rules.allow_future_times')); + $this->assertTrue($sut->isSamlActive()); } public function testUnknownConfigs() diff --git a/tests/Saml/Controller/SamlControllerTest.php b/tests/Controller/Auth/SamlControllerTest.php similarity index 59% rename from tests/Saml/Controller/SamlControllerTest.php rename to tests/Controller/Auth/SamlControllerTest.php index 84c33387..a94b6f4d 100644 --- a/tests/Saml/Controller/SamlControllerTest.php +++ b/tests/Controller/Auth/SamlControllerTest.php @@ -7,15 +7,18 @@ * file that was distributed with this source code. */ -namespace App\Tests\Saml\Controller; +namespace App\Tests\Controller\Auth; -use App\Saml\Controller\SamlController; +use App\Configuration\SystemConfiguration; +use App\Controller\Auth\SamlController; +use App\Tests\Configuration\TestConfigLoader; use App\Tests\Mocks\Saml\SamlAuthFactory; use PHPUnit\Framework\TestCase; use PHPUnit\Util\Xml; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Security\Core\Security; /** @@ -23,18 +26,44 @@ use Symfony\Component\Security\Core\Security; */ class SamlControllerTest extends TestCase { + /** + * @param array $settings + * @param array $loaderSettings + * @return SystemConfiguration + */ + protected function getSystemConfigurationMock(array $settings, array $loaderSettings = []) + { + $loader = new TestConfigLoader($loaderSettings); + + return new SystemConfiguration($loader, $settings); + } + + protected function getDefaultSettings(bool $activated = true) + { + return [ + 'saml' => [ + 'activate' => $activated, + ] + ]; + } + protected function getAuth() { return (new SamlAuthFactory($this))->create(); } + protected function getSystemConfiguration(bool $activated = true) + { + return $this->getSystemConfigurationMock($this->getDefaultSettings($activated), []); + } + public function testAssertionConsumerServiceAction() { $this->expectException(\RuntimeException::class); $this->expectExceptionMessage('You must configure the check path in your firewall.'); $oauth = $this->getAuth(); - $sut = new SamlController($oauth); + $sut = new SamlController($oauth, $this->getSystemConfiguration()); $sut->assertionConsumerServiceAction(); } @@ -44,7 +73,7 @@ class SamlControllerTest extends TestCase $this->expectExceptionMessage('You must configure the logout path in your firewall.'); $oauth = $this->getAuth(); - $sut = new SamlController($oauth); + $sut = new SamlController($oauth, $this->getSystemConfiguration()); $sut->logoutAction(); } @@ -75,7 +104,7 @@ class SamlControllerTest extends TestCase EOD; $oauth = $this->getAuth(); - $sut = new SamlController($oauth); + $sut = new SamlController($oauth, $this->getSystemConfiguration()); $result = $sut->metadataAction(); self::assertInstanceOf(Response::class, $result); @@ -98,7 +127,43 @@ EOD; $request->attributes->set(Security::AUTHENTICATION_ERROR, new \Exception('My test error')); $oauth = $this->getAuth(); - $sut = new SamlController($oauth); + $sut = new SamlController($oauth, $this->getSystemConfiguration()); $sut->loginAction($request); } + + public function testLoginActionThrowsExceptionOnDisabledSaml() + { + $this->expectException(NotFoundHttpException::class); + $this->expectExceptionMessage('SAML deactivated'); + + $sut = new SamlController($this->getAuth(), $this->getSystemConfiguration(false)); + $sut->loginAction(new Request()); + } + + public function testMetadataActionThrowsExceptionOnDisabledSaml() + { + $this->expectException(NotFoundHttpException::class); + $this->expectExceptionMessage('SAML deactivated'); + + $sut = new SamlController($this->getAuth(), $this->getSystemConfiguration(false)); + $sut->metadataAction(); + } + + public function testLogoutActionThrowsExceptionOnDisabledSaml() + { + $this->expectException(NotFoundHttpException::class); + $this->expectExceptionMessage('SAML deactivated'); + + $sut = new SamlController($this->getAuth(), $this->getSystemConfiguration(false)); + $sut->logoutAction(); + } + + public function testAcsActionThrowsExceptionOnDisabledSaml() + { + $this->expectException(NotFoundHttpException::class); + $this->expectExceptionMessage('SAML deactivated'); + + $sut = new SamlController($this->getAuth(), $this->getSystemConfiguration(false)); + $sut->assertionConsumerServiceAction(); + } } diff --git a/tests/EventSubscriber/Actions/TimesheetSubscriberTest.php b/tests/EventSubscriber/Actions/TimesheetSubscriberTest.php index 4321f064..085a9b00 100644 --- a/tests/EventSubscriber/Actions/TimesheetSubscriberTest.php +++ b/tests/EventSubscriber/Actions/TimesheetSubscriberTest.php @@ -12,6 +12,8 @@ namespace App\Tests\EventSubscriber\Actions; use App\EventSubscriber\Actions\TimesheetSubscriber; /** + * @covers \App\EventSubscriber\Actions\AbstractActionsSubscriber + * @covers \App\EventSubscriber\Actions\AbstractTimesheetSubscriber * @covers \App\EventSubscriber\Actions\TimesheetSubscriber */ class TimesheetSubscriberTest extends AbstractActionsSubscriberTest diff --git a/tests/EventSubscriber/Actions/TimesheetTeamMultiUpdateSubscriberTest.php b/tests/EventSubscriber/Actions/TimesheetTeamMultiUpdateSubscriberTest.php index 4121bf28..72380c4e 100644 --- a/tests/EventSubscriber/Actions/TimesheetTeamMultiUpdateSubscriberTest.php +++ b/tests/EventSubscriber/Actions/TimesheetTeamMultiUpdateSubscriberTest.php @@ -12,6 +12,8 @@ namespace App\Tests\EventSubscriber\Actions; use App\EventSubscriber\Actions\TimesheetTeamMultiUpdateSubscriber; /** + * @covers \App\EventSubscriber\Actions\AbstractActionsSubscriber + * @covers \App\EventSubscriber\Actions\TimesheetTeamMultiUpdateSubscriber * @covers \App\EventSubscriber\Actions\TimesheetTeamMultiUpdateSubscriber */ class TimesheetTeamMultiUpdateSubscriberTest extends AbstractActionsSubscriberTest diff --git a/tests/EventSubscriber/Actions/TimesheetsSubscriberTest.php b/tests/EventSubscriber/Actions/TimesheetsSubscriberTest.php index 4a647c93..78bdc0c3 100644 --- a/tests/EventSubscriber/Actions/TimesheetsSubscriberTest.php +++ b/tests/EventSubscriber/Actions/TimesheetsSubscriberTest.php @@ -12,6 +12,8 @@ namespace App\Tests\EventSubscriber\Actions; use App\EventSubscriber\Actions\TimesheetsSubscriber; /** + * @covers \App\EventSubscriber\Actions\AbstractActionsSubscriber + * @covers \App\EventSubscriber\Actions\AbstractTimesheetsSubscriber * @covers \App\EventSubscriber\Actions\TimesheetsSubscriber */ class TimesheetsSubscriberTest extends AbstractActionsSubscriberTest diff --git a/tests/EventSubscriber/Actions/TimesheetsTeamSubscriberTest.php b/tests/EventSubscriber/Actions/TimesheetsTeamSubscriberTest.php index 9131167d..e76b98d0 100644 --- a/tests/EventSubscriber/Actions/TimesheetsTeamSubscriberTest.php +++ b/tests/EventSubscriber/Actions/TimesheetsTeamSubscriberTest.php @@ -12,6 +12,8 @@ namespace App\Tests\EventSubscriber\Actions; use App\EventSubscriber\Actions\TimesheetsTeamSubscriber; /** + * @covers \App\EventSubscriber\Actions\AbstractActionsSubscriber + * @covers \App\EventSubscriber\Actions\AbstractTimesheetsSubscriber * @covers \App\EventSubscriber\Actions\TimesheetsTeamSubscriber */ class TimesheetsTeamSubscriberTest extends AbstractActionsSubscriberTest