From bfab6f42d050cdcf438645d0a18b253017dd2d31 Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Fri, 23 Sep 2022 13:29:46 +0200 Subject: [PATCH] use saml config interface instead of generic system configuration (#3551) --- config/services-saml.yaml | 2 +- src/Controller/Auth/SamlController.php | 16 +++++++------- .../Security/SecurityController.php | 6 +++++- src/Saml/Provider/SamlProvider.php | 6 +++--- templates/security/login.html.twig | 4 ++-- tests/Controller/Auth/SamlControllerTest.php | 21 ++++++++++--------- .../Security/SecurityControllerTest.php | 11 ++++++++-- tests/Saml/Provider/SamlProviderTest.php | 3 ++- 8 files changed, 41 insertions(+), 28 deletions(-) diff --git a/config/services-saml.yaml b/config/services-saml.yaml index ebd71d24..3b2f083d 100644 --- a/config/services-saml.yaml +++ b/config/services-saml.yaml @@ -12,7 +12,7 @@ services: - [setAuth, ['@App\Saml\SamlAuthFactory']] App\Saml\Provider\SamlProvider: - arguments: ['@App\Repository\UserRepository', '', '@App\Saml\SamlTokenFactory', '@App\Saml\User\SamlUserFactory', '@App\Configuration\SystemConfiguration'] + arguments: ['@App\Repository\UserRepository', '', '@App\Saml\SamlTokenFactory', '@App\Saml\User\SamlUserFactory', '@App\Configuration\SamlConfigurationInterface'] App\Saml\Security\SamlAuthenticationSuccessHandler: parent: security.authentication.success_handler diff --git a/src/Controller/Auth/SamlController.php b/src/Controller/Auth/SamlController.php index cdab44a9..3ecb745d 100644 --- a/src/Controller/Auth/SamlController.php +++ b/src/Controller/Auth/SamlController.php @@ -9,7 +9,7 @@ namespace App\Controller\Auth; -use App\Configuration\SystemConfiguration; +use App\Configuration\SamlConfigurationInterface; use App\Saml\SamlAuthFactory; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\Request; @@ -23,12 +23,12 @@ use Symfony\Component\Security\Core\Security; final class SamlController extends AbstractController { private $authFactory; - private $systemConfiguration; + private $samlConfiguration; - public function __construct(SamlAuthFactory $authFactory, SystemConfiguration $systemConfiguration) + public function __construct(SamlAuthFactory $authFactory, SamlConfigurationInterface $samlConfiguration) { $this->authFactory = $authFactory; - $this->systemConfiguration = $systemConfiguration; + $this->samlConfiguration = $samlConfiguration; } /** @@ -36,7 +36,7 @@ final class SamlController extends AbstractController */ public function loginAction(Request $request) { - if (!$this->systemConfiguration->isSamlActive()) { + if (!$this->samlConfiguration->isActivated()) { throw $this->createNotFoundException('SAML deactivated'); } @@ -67,7 +67,7 @@ final class SamlController extends AbstractController */ public function metadataAction() { - if (!$this->systemConfiguration->isSamlActive()) { + if (!$this->samlConfiguration->isActivated()) { throw $this->createNotFoundException('SAML deactivated'); } @@ -84,7 +84,7 @@ final class SamlController extends AbstractController */ public function assertionConsumerServiceAction() { - if (!$this->systemConfiguration->isSamlActive()) { + if (!$this->samlConfiguration->isActivated()) { throw $this->createNotFoundException('SAML deactivated'); } @@ -96,7 +96,7 @@ final class SamlController extends AbstractController */ public function logoutAction() { - if (!$this->systemConfiguration->isSamlActive()) { + if (!$this->samlConfiguration->isActivated()) { throw $this->createNotFoundException('SAML deactivated'); } diff --git a/src/Controller/Security/SecurityController.php b/src/Controller/Security/SecurityController.php index ebea51d7..c9c4aab8 100644 --- a/src/Controller/Security/SecurityController.php +++ b/src/Controller/Security/SecurityController.php @@ -9,6 +9,7 @@ namespace App\Controller\Security; +use App\Configuration\SamlConfigurationInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -21,10 +22,12 @@ use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface; final class SecurityController extends AbstractController { private $tokenManager; + private $samlConfiguration; - public function __construct(CsrfTokenManagerInterface $tokenManager) + public function __construct(CsrfTokenManagerInterface $tokenManager, SamlConfigurationInterface $samlConfiguration) { $this->tokenManager = $tokenManager; + $this->samlConfiguration = $samlConfiguration; } /** @@ -67,6 +70,7 @@ final class SecurityController extends AbstractController 'last_username' => $lastUsername, 'error' => $error, 'csrf_token' => $csrfToken, + 'saml_config' => $this->samlConfiguration, ]); } diff --git a/src/Saml/Provider/SamlProvider.php b/src/Saml/Provider/SamlProvider.php index 03299c97..88d88d44 100644 --- a/src/Saml/Provider/SamlProvider.php +++ b/src/Saml/Provider/SamlProvider.php @@ -9,7 +9,7 @@ namespace App\Saml\Provider; -use App\Configuration\SystemConfiguration; +use App\Configuration\SamlConfigurationInterface; use App\Entity\User; use App\Repository\UserRepository; use App\Saml\SamlTokenFactory; @@ -29,7 +29,7 @@ final class SamlProvider implements AuthenticationProviderInterface private $repository; private $configuration; - public function __construct(UserRepository $repository, UserProviderInterface $userProvider, SamlTokenFactory $tokenFactory, SamlUserFactory $userFactory, SystemConfiguration $configuration) + public function __construct(UserRepository $repository, UserProviderInterface $userProvider, SamlTokenFactory $tokenFactory, SamlUserFactory $userFactory, SamlConfigurationInterface $configuration) { $this->repository = $repository; $this->userProvider = $userProvider; @@ -74,7 +74,7 @@ final class SamlProvider implements AuthenticationProviderInterface public function supports(TokenInterface $token) { - if (!$this->configuration->isSamlActive()) { + if (!$this->configuration->isActivated()) { return false; } diff --git a/templates/security/login.html.twig b/templates/security/login.html.twig index 2957a93e..472382ee 100644 --- a/templates/security/login.html.twig +++ b/templates/security/login.html.twig @@ -23,13 +23,13 @@ {% endblock %} {% block login_social_auth %} - {% if kimai_config.samlActive %} + {% if saml_config.isActivated() %} {% set class = 'btn-primary' %} {% if kimai_config.loginFormActive %} {% set class = 'btn-google' %} {% endif %} - {{ kimai_config.samlTitle|trans }} + {{ saml_config.getTitle()|trans }}
{% endif %} diff --git a/tests/Controller/Auth/SamlControllerTest.php b/tests/Controller/Auth/SamlControllerTest.php index 4335d25c..47615789 100644 --- a/tests/Controller/Auth/SamlControllerTest.php +++ b/tests/Controller/Auth/SamlControllerTest.php @@ -9,6 +9,7 @@ namespace App\Tests\Controller\Auth; +use App\Configuration\SamlConfiguration; use App\Configuration\SystemConfiguration; use App\Controller\Auth\SamlController; use App\Saml\SamlAuthFactory; @@ -53,9 +54,9 @@ class SamlControllerTest extends TestCase return (new SamlAuthFactoryFactory($this))->create()->create(); } - protected function getSystemConfiguration(bool $activated = true) + protected function getSamlConfiguration(bool $activated = true): SamlConfiguration { - return $this->getSystemConfigurationMock($this->getDefaultSettings($activated), []); + return new SamlConfiguration($this->getSystemConfigurationMock($this->getDefaultSettings($activated), [])); } public function testAssertionConsumerServiceAction() @@ -65,7 +66,7 @@ class SamlControllerTest extends TestCase $factory = $this->getMockBuilder(SamlAuthFactory::class)->disableOriginalConstructor()->getMock(); - $sut = new SamlController($factory, $this->getSystemConfiguration()); + $sut = new SamlController($factory, $this->getSamlConfiguration()); $sut->assertionConsumerServiceAction(); } @@ -76,7 +77,7 @@ class SamlControllerTest extends TestCase $factory = $this->getMockBuilder(SamlAuthFactory::class)->disableOriginalConstructor()->getMock(); - $sut = new SamlController($factory, $this->getSystemConfiguration()); + $sut = new SamlController($factory, $this->getSamlConfiguration()); $sut->logoutAction(); } @@ -111,7 +112,7 @@ EOD; $factory = $this->getMockBuilder(SamlAuthFactory::class)->disableOriginalConstructor()->getMock(); $factory->expects($this->once())->method('create')->willReturn($oauth); - $sut = new SamlController($factory, $this->getSystemConfiguration()); + $sut = new SamlController($factory, $this->getSamlConfiguration()); $result = $sut->metadataAction(); self::assertInstanceOf(Response::class, $result); @@ -140,7 +141,7 @@ EOD; $factory = $this->getMockBuilder(SamlAuthFactory::class)->disableOriginalConstructor()->getMock(); - $sut = new SamlController($factory, $this->getSystemConfiguration()); + $sut = new SamlController($factory, $this->getSamlConfiguration()); $sut->loginAction($request); } @@ -151,7 +152,7 @@ EOD; $factory = $this->getMockBuilder(SamlAuthFactory::class)->disableOriginalConstructor()->getMock(); - $sut = new SamlController($factory, $this->getSystemConfiguration(false)); + $sut = new SamlController($factory, $this->getSamlConfiguration(false)); $sut->loginAction(new Request()); } @@ -162,7 +163,7 @@ EOD; $factory = $this->getMockBuilder(SamlAuthFactory::class)->disableOriginalConstructor()->getMock(); - $sut = new SamlController($factory, $this->getSystemConfiguration(false)); + $sut = new SamlController($factory, $this->getSamlConfiguration(false)); $sut->metadataAction(); } @@ -173,7 +174,7 @@ EOD; $factory = $this->getMockBuilder(SamlAuthFactory::class)->disableOriginalConstructor()->getMock(); - $sut = new SamlController($factory, $this->getSystemConfiguration(false)); + $sut = new SamlController($factory, $this->getSamlConfiguration(false)); $sut->logoutAction(); } @@ -184,7 +185,7 @@ EOD; $factory = $this->getMockBuilder(SamlAuthFactory::class)->disableOriginalConstructor()->getMock(); - $sut = new SamlController($factory, $this->getSystemConfiguration(false)); + $sut = new SamlController($factory, $this->getSamlConfiguration(false)); $sut->assertionConsumerServiceAction(); } } diff --git a/tests/Controller/Security/SecurityControllerTest.php b/tests/Controller/Security/SecurityControllerTest.php index 047bdbd7..fcc71af8 100644 --- a/tests/Controller/Security/SecurityControllerTest.php +++ b/tests/Controller/Security/SecurityControllerTest.php @@ -9,8 +9,11 @@ namespace App\Tests\Controller\Security; +use App\Configuration\SamlConfiguration; +use App\Configuration\SystemConfiguration; use App\Controller\Security\SecurityController; use App\Entity\User; +use App\Tests\Configuration\TestConfigLoader; use App\Tests\Controller\ControllerBaseTest; use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface; @@ -119,7 +122,9 @@ class SecurityControllerTest extends ControllerBaseTest $client = self::createClient(); // just to bootstrap the container $csrf = $this->createMock(CsrfTokenManagerInterface::class); - $sut = new SecurityController($csrf); + $systemConfig = new SystemConfiguration(new TestConfigLoader([]), ['saml' => ['activate' => true]]); + $samlConfig = new SamlConfiguration($systemConfig); + $sut = new SecurityController($csrf, $samlConfig); $sut->checkAction(); } @@ -130,7 +135,9 @@ class SecurityControllerTest extends ControllerBaseTest $client = self::createClient(); // just to bootstrap the container $csrf = $this->createMock(CsrfTokenManagerInterface::class); - $sut = new SecurityController($csrf); + $systemConfig = new SystemConfiguration(new TestConfigLoader([]), ['saml' => ['activate' => true]]); + $samlConfig = new SamlConfiguration($systemConfig); + $sut = new SecurityController($csrf, $samlConfig); $sut->logoutAction(); } } diff --git a/tests/Saml/Provider/SamlProviderTest.php b/tests/Saml/Provider/SamlProviderTest.php index fa9b695d..1ed2f365 100644 --- a/tests/Saml/Provider/SamlProviderTest.php +++ b/tests/Saml/Provider/SamlProviderTest.php @@ -54,13 +54,14 @@ class SamlProviderTest extends TestCase } $systemConfig = new SystemConfiguration(new TestConfigLoader([]), ['saml' => ['activate' => true]]); + $samlConfig = new SamlConfiguration($systemConfig); $repository = $this->getMockBuilder(UserRepository::class)->disableOriginalConstructor()->getMock(); if ($user !== null) { $repository->expects($this->once())->method('loadUserByUsername')->willReturn($user); } $userProvider = new ChainUserProvider([new DoctrineUserProvider($repository)]); - $provider = new SamlProvider($repository, $userProvider, new SamlTokenFactory(), $userFactory, $systemConfig); + $provider = new SamlProvider($repository, $userProvider, new SamlTokenFactory(), $userFactory, $samlConfig); return $provider; }