diff --git a/.php-cs-fixer.php b/.php-cs-fixer.php index 1aa9117..cd6bcc4 100644 --- a/.php-cs-fixer.php +++ b/.php-cs-fixer.php @@ -101,5 +101,8 @@ 'simplified_null_return' => true, 'single_line_empty_body' => true, 'static_lambda' => true, + + // PhpUnit rules + 'php_unit_strict' => false, ]) ; diff --git a/composer.json b/composer.json index 99e937c..f98e270 100644 --- a/composer.json +++ b/composer.json @@ -33,7 +33,12 @@ "require-dev": { "doctrine/orm": "^2.3 || ^3", "symfony/event-dispatcher": "^6.4", - "symfony/phpunit-bridge": "^6.4" + "symfony/phpunit-bridge": "^6.4", + "symfony/test-pack": "^1.2", + "phpstan/phpstan": "^2.1", + "vimeo/psalm": "^6.13", + "friendsofphp/php-cs-fixer": "^3.84", + "psalm/plugin-symfony": "^5.2" }, "autoload": { "psr-4": { diff --git a/phpstan.dist.neon b/phpstan.dist.neon new file mode 100644 index 0000000..5c63723 --- /dev/null +++ b/phpstan.dist.neon @@ -0,0 +1,8 @@ +parameters: + level: 8 + paths: + - src/ + - tests/ + excludePaths: + - src/DependencyInjection/Configuration.php + - src/DependencyInjection/Security/UserProvider/SamlUserProviderFactory.php diff --git a/phpstan.neon b/phpstan.neon deleted file mode 100644 index f73ea60..0000000 --- a/phpstan.neon +++ /dev/null @@ -1,10 +0,0 @@ -parameters: - level: 9 - paths: - - src - - tests - bootstrapFiles: - - vendor/bin/.phpunit/phpunit/vendor/autoload.php - - checkMissingIterableValueType: false - checkGenericClassInNonGenericObjectType: false diff --git a/psalm.xml b/psalm.xml index f6e3dd9..b358616 100644 --- a/psalm.xml +++ b/psalm.xml @@ -13,11 +13,13 @@ - + - + + + diff --git a/src/Controller/Login.php b/src/Controller/Login.php index a9a0e0c..6639bd0 100644 --- a/src/Controller/Login.php +++ b/src/Controller/Login.php @@ -7,6 +7,7 @@ use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\SamlAuthenticator; use OneLogin\Saml2\Auth; +use OneLogin\Saml2\Error as Saml2Exception; use Symfony\Bundle\SecurityBundle\Security\FirewallMap; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; @@ -16,10 +17,10 @@ use Symfony\Component\Security\Http\SecurityRequestAttributes; #[AsController] -class Login +readonly class Login { public function __construct( - private readonly FirewallMap $firewallMap, + private FirewallMap $firewallMap, ) {} public function __invoke(Request $request, Auth $auth): RedirectResponse @@ -44,30 +45,38 @@ public function __invoke(Request $request, Auth $auth): RedirectResponse throw new \RuntimeException($error->getMessage()); } - return new RedirectResponse($this->processLoginAndGetRedirectUrl($auth, $targetPath, $session)); + try { + return new RedirectResponse($this->processLoginAndGetRedirectUrl($auth, $targetPath, $session)); + } catch (Saml2Exception $e) { + throw new \RuntimeException($e->getMessage()); + } } - /** @psalm-suppress MixedInferredReturnType, MixedReturnStatement */ private function getTargetPath(Request $request, SessionInterface $session): ?string { $firewallName = $this->firewallMap->getFirewallConfig($request)?->getName(); - if (!$firewallName) { + if (null === $firewallName || '' === $firewallName) { throw new ServiceUnavailableHttpException(message: 'Unknown firewall.'); } - /** @phpstan-ignore-next-line */ - return $session->get('_security.'.$firewallName.'.target_path'); + $attribute = (string) $session->get('_security.'.$firewallName.'.target_path'); + + return ($attribute === '') ? null : $attribute; } + /** + * @throws Saml2Exception + */ private function processLoginAndGetRedirectUrl(Auth $auth, ?string $targetPath, ?SessionInterface $session): string { + /** @var string|null $redirectUrl */ $redirectUrl = $auth->login(returnTo: $targetPath, stay: true); - if ($redirectUrl === null) { - throw new \RuntimeException('Login cannot be performed: Auth did not returned redirect url.'); + if (null === $redirectUrl) { + throw new \RuntimeException('Unable to initiate SAML login process.'); } $security = $auth->getSettings()->getSecurityData(); - if (($security['rejectUnsolicitedResponsesWithInResponseTo'] ?? false) && $session instanceof SessionInterface) { + if ((($security['rejectUnsolicitedResponsesWithInResponseTo'] ?? false) === true) && $session instanceof SessionInterface) { $session->set(SamlAuthenticator::LAST_REQUEST_ID, $auth->getLastRequestID()); } diff --git a/src/Controller/Logout.php b/src/Controller/Logout.php index 0b722ab..d93a903 100644 --- a/src/Controller/Logout.php +++ b/src/Controller/Logout.php @@ -8,7 +8,7 @@ use Symfony\Component\HttpKernel\Attribute\AsController; #[AsController] -class Logout +readonly class Logout { public function __invoke(): void { diff --git a/src/Controller/Metadata.php b/src/Controller/Metadata.php index 41c3252..aab9060 100644 --- a/src/Controller/Metadata.php +++ b/src/Controller/Metadata.php @@ -6,12 +6,16 @@ namespace Nbgrp\OneloginSamlBundle\Controller; use OneLogin\Saml2\Auth; +use OneLogin\Saml2\Error as Saml2Exception; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Attribute\AsController; #[AsController] -class Metadata +readonly class Metadata { + /** + * @throws Saml2Exception + */ public function __invoke(Auth $auth): Response { return new Response( diff --git a/src/DependencyInjection/Compiler/AuthRegistryCompilerPass.php b/src/DependencyInjection/Compiler/AuthRegistryCompilerPass.php index b6ee84f..a7b7cf0 100644 --- a/src/DependencyInjection/Compiler/AuthRegistryCompilerPass.php +++ b/src/DependencyInjection/Compiler/AuthRegistryCompilerPass.php @@ -18,6 +18,7 @@ */ class AuthRegistryCompilerPass implements CompilerPassInterface { + #[\Override] public function process(ContainerBuilder $container): void { $authRegistry = $container->getDefinition(AuthRegistryInterface::class); @@ -27,11 +28,31 @@ public function process(ContainerBuilder $container): void throw new \UnexpectedValueException('OneLogin settings should be an array.'); } - /** @var array $settings */ - foreach ($oneloginSettings as $key => $settings) { - $authDefinition = new Definition(Auth::class, [$settings]); - $authDefinition->setFactory(new Reference(AuthFactory::class)); - $authRegistry->addMethodCall('addService', [$key, $authDefinition]); + /** @var array $idpSettings */ + foreach ($oneloginSettings as $idpKey => $idpSettings) { + /** @var array $settings */ + $settings = $idpSettings; + if (isset($idpSettings['sp'])) { + if (!\is_array($idpSettings['sp'])) { + throw new \UnexpectedValueException('OneLogin SP settings should be an array.'); + } + /** @var array|null $spSettings */ + foreach ($idpSettings['sp'] as $spKey => $spSettings) { + if (!\is_array($spSettings)) { + throw new \UnexpectedValueException('OneLogin SP settings for key "'.(string) $spKey.'" should be an array.'); + } + + $settings['sp'] = $spSettings; + $authDefinition = new Definition(Auth::class, [$settings]); + $authDefinition->setFactory(new Reference(AuthFactory::class)); + $authRegistry->addMethodCall('addService', [$idpKey, $spKey, $authDefinition]); + } + } else { + $settings['sp'] = 'default'; + $authDefinition = new Definition(Auth::class, [$settings]); + $authDefinition->setFactory(new Reference(AuthFactory::class)); + $authRegistry->addMethodCall('addService', [$idpKey, 'default', $authDefinition]); + } } } } diff --git a/src/DependencyInjection/Compiler/EntityManagerCompilerPass.php b/src/DependencyInjection/Compiler/EntityManagerCompilerPass.php index 5ba02bf..ed7e588 100644 --- a/src/DependencyInjection/Compiler/EntityManagerCompilerPass.php +++ b/src/DependencyInjection/Compiler/EntityManagerCompilerPass.php @@ -14,6 +14,7 @@ */ class EntityManagerCompilerPass implements CompilerPassInterface { + #[\Override] public function process(ContainerBuilder $container): void { if (!$container->hasParameter('nbgrp_onelogin_saml.entity_manager')) { diff --git a/src/DependencyInjection/Compiler/ProxyVarsCompilerPass.php b/src/DependencyInjection/Compiler/ProxyVarsCompilerPass.php index e55e5c1..b278c20 100644 --- a/src/DependencyInjection/Compiler/ProxyVarsCompilerPass.php +++ b/src/DependencyInjection/Compiler/ProxyVarsCompilerPass.php @@ -19,6 +19,7 @@ */ class ProxyVarsCompilerPass implements CompilerPassInterface { + #[\Override] public function process(ContainerBuilder $container): void { $useProxyVars = $container->getParameter('nbgrp_onelogin_saml.use_proxy_vars'); diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 933f2f3..488c3bb 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -1,10 +1,12 @@ getRootNode(); // @formatter:off - /** @phpstan-ignore-next-line */ $rootNode ->info('nb:group OneLogin PHP Symfony Bundle configuration') ->children() @@ -31,6 +35,27 @@ public function getConfigTreeBuilder(): TreeBuilder ->useAttributeAsKey('name') ->normalizeKeys(false) ->arrayPrototype() + ->beforeNormalization() + ->always(function ($v) { + // Si no hay 'sp', meter default vacío + if (!isset($v['sp']) || $v['sp'] === []) { + $v['sp'] = ['default' => []]; + } + + // Si 'sp' es un array plano (i.e. no es indexado por string con valores que son arrays) + if (isset($v['sp']) && is_array($v['sp'])) { + $isAssoc = static fn ($arr) => array_keys($arr) !== range(0, count($arr) - 1); + $isLikelyFlatSp = $isAssoc($v['sp']) && array_filter($v['sp'], 'is_array') === []; + + if ($isLikelyFlatSp || isset($v['sp']['entityId'])) { + $v['sp'] = ['default' => $v['sp']]; + } + } + + + return $v; + }) + ->end() ->children() ->scalarNode('baseurl') ->defaultValue('/saml/') @@ -87,67 +112,69 @@ public function getConfigTreeBuilder(): TreeBuilder ->end() ->end() ->arrayNode('sp') - ->addDefaultsIfNotSet() - ->children() - ->scalarNode('entityId') - ->defaultValue('/saml/metadata') - ->end() - ->arrayNode('assertionConsumerService') - ->addDefaultsIfNotSet() - ->children() - ->scalarNode('url') - ->defaultValue('/saml/acs') - ->end() - ->scalarNode('binding') - ->validate() - ->ifTrue(static fn ($value): bool => !str_starts_with($value, 'urn:oasis:names:tc:SAML:2.0:bindings:')) - ->thenInvalid('invalid value.') + ->useAttributeAsKey('name') + ->arrayPrototype() + ->children() + ->scalarNode('entityId') + ->defaultValue('/saml/metadata') + ->end() + ->arrayNode('assertionConsumerService') + ->addDefaultsIfNotSet() + ->children() + ->scalarNode('url') + ->defaultValue('/saml/acs') + ->end() + ->scalarNode('binding') + ->validate() + ->ifTrue(static fn ($value): bool => !str_starts_with($value, 'urn:oasis:names:tc:SAML:2.0:bindings:')) + ->thenInvalid('invalid value.') + ->end() ->end() ->end() ->end() - ->end() - ->arrayNode('attributeConsumingService') - ->children() - ->scalarNode('serviceName')->end() - ->scalarNode('serviceDescription')->end() - ->arrayNode('requestedAttributes') - ->arrayPrototype() - ->children() - ->scalarNode('name')->end() - ->booleanNode('isRequired') - ->defaultFalse() + ->arrayNode('attributeConsumingService') + ->children() + ->scalarNode('serviceName')->end() + ->scalarNode('serviceDescription')->end() + ->arrayNode('requestedAttributes') + ->arrayPrototype() + ->children() + ->scalarNode('name')->end() + ->booleanNode('isRequired') + ->defaultFalse() + ->end() + ->scalarNode('nameFormat')->end() + ->scalarNode('friendlyName')->end() + ->arrayNode('attributeValue')->end() ->end() - ->scalarNode('nameFormat')->end() - ->scalarNode('friendlyName')->end() - ->arrayNode('attributeValue')->end() ->end() ->end() ->end() ->end() - ->end() - ->arrayNode('singleLogoutService') - ->addDefaultsIfNotSet() - ->children() - ->scalarNode('url') - ->defaultValue('/saml/logout') - ->end() - ->scalarNode('binding') - ->validate() - ->ifTrue(static fn ($value): bool => !str_starts_with($value, 'urn:oasis:names:tc:SAML:2.0:bindings:')) - ->thenInvalid('invalid value.') + ->arrayNode('singleLogoutService') + ->addDefaultsIfNotSet() + ->children() + ->scalarNode('url') + ->defaultValue('/saml/logout') + ->end() + ->scalarNode('binding') + ->validate() + ->ifTrue(static fn ($value): bool => !str_starts_with($value, 'urn:oasis:names:tc:SAML:2.0:bindings:')) + ->thenInvalid('invalid value.') + ->end() ->end() ->end() ->end() - ->end() - ->scalarNode('NameIDFormat') - ->validate() - ->ifTrue(static fn ($value): bool => !(str_starts_with($value, 'urn:oasis:names:tc:SAML:1.1:nameid-format:') || str_starts_with($value, 'urn:oasis:names:tc:SAML:2.0:nameid-format:'))) - ->thenInvalid('invalid value.') + ->scalarNode('NameIDFormat') + ->validate() + ->ifTrue(static fn ($value): bool => !(str_starts_with($value, 'urn:oasis:names:tc:SAML:1.1:nameid-format:') || str_starts_with($value, 'urn:oasis:names:tc:SAML:2.0:nameid-format:'))) + ->thenInvalid('invalid value.') + ->end() ->end() + ->scalarNode('x509cert')->end() + ->scalarNode('privateKey')->end() + ->scalarNode('x509certNew')->end() ->end() - ->scalarNode('x509cert')->end() - ->scalarNode('privateKey')->end() - ->scalarNode('x509certNew')->end() ->end() ->end() ->arrayNode('compress') @@ -174,7 +201,7 @@ public function getConfigTreeBuilder(): TreeBuilder ->thenInvalid('must be an array or a boolean.') ->end() ->validate() - ->ifTrue(static fn ($value) => \is_array($value) && array_filter($value, static fn ($item): bool => !str_starts_with($item, 'urn:oasis:names:tc:SAML:2.0:ac:classes:'))) + ->ifTrue(static fn ($value) => \is_array($value) && \count(array_filter($value, static fn ($item): bool => !str_starts_with($item, 'urn:oasis:names:tc:SAML:2.0:ac:classes:'))) > 0) ->thenInvalid('invalid value.') ->end() ->end() @@ -285,7 +312,9 @@ public function getConfigTreeBuilder(): TreeBuilder ->end() ->end() ->validate() - ->ifTrue(static fn ($value): bool => empty($value['organization'])) + ->ifTrue( + static fn ($value): bool => !isset($value['organization']) || [] === $value['organization'] + ) ->then(static fn ($value): array => array_diff_key($value, ['organization' => null])) ->end() ->end() @@ -297,13 +326,15 @@ public function getConfigTreeBuilder(): TreeBuilder ->cannotBeEmpty() ->defaultValue('idp') ->end() + ->scalarNode('sp_parameter_name') + ->cannotBeEmpty() + ->defaultValue('sp') + ->end() ->scalarNode('entity_manager_name') ->cannotBeEmpty() ->end() ->end() ; - // @formatter:on - return $treeBuilder; } } diff --git a/src/DependencyInjection/NbgrpOneloginSamlExtension.php b/src/DependencyInjection/NbgrpOneloginSamlExtension.php index 7582d42..f117f32 100644 --- a/src/DependencyInjection/NbgrpOneloginSamlExtension.php +++ b/src/DependencyInjection/NbgrpOneloginSamlExtension.php @@ -12,11 +12,13 @@ /** * @internal + * * @final */ class NbgrpOneloginSamlExtension extends Extension { /** @psalm-suppress MixedArgument */ + #[\Override] public function load(array $configs, ContainerBuilder $container): void { $loader = new PhpFileLoader($container, new FileLocator(\dirname(__DIR__).'/Resources/config')); @@ -27,6 +29,7 @@ public function load(array $configs, ContainerBuilder $container): void $container->setParameter('nbgrp_onelogin_saml.onelogin_settings', $config['onelogin_settings']); $container->setParameter('nbgrp_onelogin_saml.use_proxy_vars', $config['use_proxy_vars']); $container->setParameter('nbgrp_onelogin_saml.idp_parameter_name', $config['idp_parameter_name']); + $container->setParameter('nbgrp_onelogin_saml.sp_parameter_name', $config['sp_parameter_name']); if (\array_key_exists('entity_manager_name', $config)) { $container->setParameter('nbgrp_onelogin_saml.entity_manager', $config['entity_manager_name']); diff --git a/src/DependencyInjection/Security/Factory/SamlFactory.php b/src/DependencyInjection/Security/Factory/SamlFactory.php index 756aec8..eaf6cba 100644 --- a/src/DependencyInjection/Security/Factory/SamlFactory.php +++ b/src/DependencyInjection/Security/Factory/SamlFactory.php @@ -30,17 +30,20 @@ public function __construct() $this->addOption('success_handler', SamlAuthenticationSuccessHandler::class); } + #[\Override] public function getPriority(): int { return self::PRIORITY; } + #[\Override] public function getKey(): string { return 'saml'; } /** @psalm-suppress MixedArgument */ + #[\Override] public function createAuthenticator(ContainerBuilder $container, string $firewallName, array $config, string $userProviderId): string { $authenticatorId = 'security.authenticator.saml.'.$firewallName; @@ -50,9 +53,7 @@ public function createAuthenticator(ContainerBuilder $container, string $firewal ->replaceArgument(5, new Reference($this->createAuthenticationFailureHandler($container, $firewallName, $config))) ->replaceArgument(6, array_intersect_key($config, $this->options)) ; - - if (!empty($config['user_factory'])) { - /** @phpstan-ignore-next-line */ + if (isset($config['user_factory']) && '' !== (string) $config['user_factory'] && [] !== $config['user_factory']) { $authenticator->replaceArgument(7, new Reference((string) $config['user_factory'])); } @@ -63,6 +64,11 @@ public function createAuthenticator(ContainerBuilder $container, string $firewal return $authenticatorId; } + /** + * Crea los listeners de usuario para el firewall SAML. + * + * @param array $config + */ protected function createUserListeners(ContainerBuilder $container, string $firewallName, array $config): void { $container->setDefinition('nbgrp_onelogin_saml.user_created_listener.'.$firewallName, new ChildDefinition(UserCreatedListener::class)) diff --git a/src/DependencyInjection/Security/UserProvider/SamlUserProviderFactory.php b/src/DependencyInjection/Security/UserProvider/SamlUserProviderFactory.php index 7e7f10b..fbb3f70 100644 --- a/src/DependencyInjection/Security/UserProvider/SamlUserProviderFactory.php +++ b/src/DependencyInjection/Security/UserProvider/SamlUserProviderFactory.php @@ -1,4 +1,5 @@ children() ->scalarNode('user_class') diff --git a/src/EventListener/Security/SamlLogoutListener.php b/src/EventListener/Security/SamlLogoutListener.php index 5ef8be9..7e88a92 100644 --- a/src/EventListener/Security/SamlLogoutListener.php +++ b/src/EventListener/Security/SamlLogoutListener.php @@ -10,27 +10,26 @@ use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\SamlAuthenticator; use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Token\SamlToken; use OneLogin\Saml2\Auth; +use OneLogin\Saml2\Error; use Symfony\Component\EventDispatcher\Attribute\AsEventListener; +use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Security\Http\Event\LogoutEvent; /** * Process Single Logout by current OneLogin Auth service on user logout. */ -final class SamlLogoutListener +final readonly class SamlLogoutListener { public function __construct( - private readonly AuthRegistryInterface $authRegistry, - private readonly IdpResolverInterface $idpResolver, + private AuthRegistryInterface $authRegistry, + private IdpResolverInterface $idpResolver, ) {} #[AsEventListener(LogoutEvent::class)] public function processSingleLogout(LogoutEvent $event): void { $authService = $this->getAuthService($event->getRequest()); - if (!$authService) { - return; - } $token = $event->getToken(); if (!$token instanceof SamlToken) { @@ -39,28 +38,33 @@ public function processSingleLogout(LogoutEvent $event): void try { $authService->processSLO(); - } catch (\OneLogin\Saml2\Error) { - if (!empty($authService->getSLOurl())) { + } catch (Error) { + $sloUrl = $authService->getSLOurl(); + if (null !== $sloUrl && '' !== $sloUrl) { /** @var string|null $sessionIndex */ $sessionIndex = $token->hasAttribute(SamlAuthenticator::SESSION_INDEX_ATTRIBUTE) ? $token->getAttribute(SamlAuthenticator::SESSION_INDEX_ATTRIBUTE) : null; - $authService->logout(null, [], $token->getUserIdentifier(), $sessionIndex); + + try { + $authService->logout(null, [], $token->getUserIdentifier(), $sessionIndex); + } catch (Error $e) { + // If logout fails, we can still redirect to SLO URL. + // This is useful for IdPs that do not support SLO. + $event->setResponse(new RedirectResponse($sloUrl)); + } } } } - private function getAuthService(Request $request): ?Auth + private function getAuthService(Request $request): Auth { - $idp = $this->idpResolver->resolve($request); - if (!$idp) { - return $this->authRegistry->getDefaultService(); - } - - if ($this->authRegistry->hasService($idp)) { - return $this->authRegistry->getService($idp); - } + $resolve = $this->idpResolver->resolve($request); + $idp = $resolve['idp']; + $sp = $resolve['sp']; - return null; + return (('' !== $idp) && ('' !== $sp)) + ? $this->authRegistry->getService($idp, $sp) + : $this->authRegistry->getDefaultService(); } } diff --git a/src/EventListener/User/DeferredUserListener.php b/src/EventListener/User/DeferredUserListener.php index 816403b..0cbda5a 100644 --- a/src/EventListener/User/DeferredUserListener.php +++ b/src/EventListener/User/DeferredUserListener.php @@ -21,7 +21,7 @@ public function dispatchDeferredEvent(CheckPassportEvent $event, string $eventNa } $deferredEvent = $badge->getEvent(); - if ($deferredEvent) { + if (null !== $deferredEvent) { $eventDispatcher->dispatch($deferredEvent); } } diff --git a/src/Idp/IdpResolver.php b/src/Idp/IdpResolver.php index c694941..af62cb4 100644 --- a/src/Idp/IdpResolver.php +++ b/src/Idp/IdpResolver.php @@ -7,23 +7,30 @@ use Symfony\Component\HttpFoundation\Request; -final class IdpResolver implements IdpResolverInterface +final readonly class IdpResolver implements IdpResolverInterface { public function __construct( - private readonly string $idpParameterName, + private string $idpParameterName, + private string $spParameterName, ) {} - public function resolve(Request $request): ?string + /** + * {@inheritdoc} + */ + #[\Override] + public function resolve(Request $request): array { - if ($request->query->has($this->idpParameterName)) { - return (string) $request->query->get($this->idpParameterName); - } + // Get IdP and SP names from query parameters or request attributes + $queryIdp = (string) $request->query->get($this->idpParameterName); + $querySp = (string) $request->query->get($this->spParameterName); + $attributesIdp = (string) $request->attributes->get($this->idpParameterName, 'default'); + $attributesSp = (string) $request->attributes->get($this->spParameterName, 'default'); - if ($request->attributes->has($this->idpParameterName)) { - /** @phpstan-ignore-next-line */ - return (string) $request->attributes->get($this->idpParameterName); - } + // If query parameters are not set, use attributes + $idp = '' !== $queryIdp ? $queryIdp : $attributesIdp; + $sp = '' !== $querySp ? $querySp : $attributesSp; - return null; + // If both IdP and SP are not set, return default values + return ['idp' => $idp, 'sp' => $sp]; } } diff --git a/src/Idp/IdpResolverInterface.php b/src/Idp/IdpResolverInterface.php index 0328107..8d5c5b0 100644 --- a/src/Idp/IdpResolverInterface.php +++ b/src/Idp/IdpResolverInterface.php @@ -13,7 +13,11 @@ interface IdpResolverInterface { /** - * Returns IdP name for specified request. + * Returns IdP name and Sp name for specified request. + * + * @param Request $request the request to resolve IdP for + * + * @return array{idp:string, sp:string} returns an array with IdP name and Sp name if they are found, or null if not */ - public function resolve(Request $request): ?string; + public function resolve(Request $request): array; } diff --git a/src/NbgrpOneloginSamlBundle.php b/src/NbgrpOneloginSamlBundle.php index 0f4b1e6..be78c7c 100644 --- a/src/NbgrpOneloginSamlBundle.php +++ b/src/NbgrpOneloginSamlBundle.php @@ -18,6 +18,7 @@ */ class NbgrpOneloginSamlBundle extends Bundle { + #[\Override] public function build(ContainerBuilder $container): void { parent::build($container); diff --git a/src/Onelogin/AuthArgumentResolver.php b/src/Onelogin/AuthArgumentResolver.php index 52aafde..b272bd9 100644 --- a/src/Onelogin/AuthArgumentResolver.php +++ b/src/Onelogin/AuthArgumentResolver.php @@ -11,36 +11,34 @@ use Symfony\Component\HttpKernel\Controller\ValueResolverInterface; use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; -use Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException; /** * Yields the OneLogin Auth instance for current request * (default or according to an idp parameter). */ -final class AuthArgumentResolver implements ValueResolverInterface +final readonly class AuthArgumentResolver implements ValueResolverInterface { public function __construct( - private readonly AuthRegistryInterface $authRegistry, - private readonly IdpResolverInterface $idpResolver, + private AuthRegistryInterface $authRegistry, + private IdpResolverInterface $idpResolver, ) {} + /** @phpstan-ignore-next-line */ + #[\Override] public function resolve(Request $request, ArgumentMetadata $argument): iterable { - if ($argument->getType() !== Auth::class) { + if (Auth::class !== $argument->getType()) { return []; } - $idp = $this->idpResolver->resolve($request); - if ($idp && !$this->authRegistry->hasService($idp)) { - throw new BadRequestHttpException('There is no OneLogin PHP toolkit settings for IdP "'.$idp.'". See nbgrp_onelogin_saml config ("onelogin_settings" section).'); - } - - try { - yield $idp - ? $this->authRegistry->getService($idp) - : $this->authRegistry->getDefaultService(); - } catch (\RuntimeException $exception) { - throw new ServiceUnavailableHttpException($exception->getMessage()); + $resolve = $this->idpResolver->resolve($request); + $idp = $resolve['idp']; + $sp = $resolve['sp']; + if (!$this->authRegistry->hasService($idp, $sp)) { + throw new BadRequestHttpException('There is no OneLogin PHP toolkit settings for IdP "'.$idp.'" and SP "'.$sp.'". See nbgrp_onelogin_saml config ("onelogin_settings" section).'); } + yield ('default' !== $idp && 'default' !== $sp) + ? $this->authRegistry->getService($idp, $sp) + : $this->authRegistry->getDefaultService(); } } diff --git a/src/Onelogin/AuthFactory.php b/src/Onelogin/AuthFactory.php index 247eabb..438c761 100644 --- a/src/Onelogin/AuthFactory.php +++ b/src/Onelogin/AuthFactory.php @@ -6,6 +6,7 @@ namespace Nbgrp\OneloginSamlBundle\Onelogin; use OneLogin\Saml2\Auth; +use OneLogin\Saml2\Error; use Symfony\Component\HttpFoundation\RequestStack; final class AuthFactory @@ -16,6 +17,13 @@ public function __construct( private readonly RequestStack $requestStack, ) {} + /** + * Create a new OneLogin Auth instance with settings. + * + * @param array $settings + * + * @throws Error + */ public function __invoke(array $settings): Auth { $request = $this->requestStack->getMainRequest(); @@ -28,24 +36,65 @@ public function __invoke(array $settings): Auth } /** - * @psalm-suppress MixedArrayAssignment, MixedArrayAccess + * Replace the scheme and host placeholder in the settings array. + * + * @param array $settings + * + * @return array */ private static function replaceSchemeAndHostPlaceholder(array $settings, string $replace): array { if (isset($settings['baseurl'])) { - $settings['baseurl'] = str_replace(self::SCHEME_AND_HOST_PLACEHOLDER, $replace, (string) $settings['baseurl']); + $settings['baseurl'] = str_replace( + self::SCHEME_AND_HOST_PLACEHOLDER, + $replace, + (string) $settings['baseurl'] + ); } - if (isset($settings['sp']['entityId'])) { - $settings['sp']['entityId'] = str_replace(self::SCHEME_AND_HOST_PLACEHOLDER, $replace, (string) $settings['sp']['entityId']); - } + if (isset($settings['sp']) && \is_array($settings['sp'])) { + /** @var array $sp */ + $sp = $settings['sp']; - if (isset($settings['sp']['assertionConsumerService']['url'])) { - $settings['sp']['assertionConsumerService']['url'] = str_replace(self::SCHEME_AND_HOST_PLACEHOLDER, $replace, (string) $settings['sp']['assertionConsumerService']['url']); // @phan-suppress-current-line PhanTypeArraySuspiciousNull, PhanTypeInvalidDimOffset - } + if (isset($sp['entityId'])) { + $sp['entityId'] = str_replace( + self::SCHEME_AND_HOST_PLACEHOLDER, + $replace, + (string) $sp['entityId'] + ); + } + + if (isset($sp['assertionConsumerService']) && \is_array($sp['assertionConsumerService'])) { + /** @var array $acs */ + $acs = $sp['assertionConsumerService']; + + if (isset($acs['url'])) { + $acs['url'] = str_replace( + self::SCHEME_AND_HOST_PLACEHOLDER, + $replace, + (string) $acs['url'] + ); + } + + $sp['assertionConsumerService'] = $acs; + } + + if (isset($sp['singleLogoutService']) && \is_array($sp['singleLogoutService'])) { + /** @var array $sls */ + $sls = $sp['singleLogoutService']; + + if (isset($sls['url'])) { + $sls['url'] = str_replace( + self::SCHEME_AND_HOST_PLACEHOLDER, + $replace, + (string) $sls['url'] + ); + } + + $sp['singleLogoutService'] = $sls; + } - if (isset($settings['sp']['singleLogoutService']['url'])) { - $settings['sp']['singleLogoutService']['url'] = str_replace(self::SCHEME_AND_HOST_PLACEHOLDER, $replace, (string) $settings['sp']['singleLogoutService']['url']); // @phan-suppress-current-line PhanTypeArraySuspiciousNull, PhanTypeInvalidDimOffset + $settings['sp'] = $sp; } return $settings; diff --git a/src/Onelogin/AuthRegistry.php b/src/Onelogin/AuthRegistry.php index ed171a3..60d6531 100644 --- a/src/Onelogin/AuthRegistry.php +++ b/src/Onelogin/AuthRegistry.php @@ -10,37 +10,59 @@ final class AuthRegistry implements AuthRegistryInterface { /** - * @var array + * @var array> */ private array $services = []; - public function addService(string $key, Auth $auth): self + #[\Override] + public function addService(string $idpKey, string $spKey, Auth $auth): self { - if (\array_key_exists($key, $this->services)) { - throw new \OverflowException('Auth service with key "'.$key.'" already exists.'); + if (\array_key_exists($idpKey, $this->services)) { + if (\array_key_exists($spKey, $this->services[$idpKey])) { + throw new \OverflowException('Auth service with key "'.$spKey.'" already exists.'); + } } - $this->services[$key] = $auth; + $this->services[$idpKey][$spKey] = $auth; return $this; } - public function hasService(string $key): bool + #[\Override] + public function hasService(string $idpKey, string $spKey): bool { - return \array_key_exists($key, $this->services); + if (\array_key_exists($idpKey, $this->services)) { + if (\array_key_exists($spKey, $this->services[$idpKey])) { + return true; + } + } + + return false; } - public function getService(string $key): Auth + /** + * {@inheritdoc} + */ + #[\Override] + public function getService(string $idpKey, string $spKey): Auth { - return $this->services[$key] ?? throw new \OutOfBoundsException('Auth service for key "'.$key.'" does not exists.'); + return $this->services[$idpKey][$spKey] ?? throw new \OutOfBoundsException('Auth service for keys "'.$idpKey.' '.$spKey.'" does not exists.'); } + #[\Override] public function getDefaultService(): Auth { - if (empty($this->services)) { + if ([] === $this->services) { + throw new \UnderflowException('There is no configured Auth services.'); + } + + $firstIdp = reset($this->services); + $firstAuth = reset($firstIdp); + + if (!$firstAuth instanceof Auth) { throw new \UnderflowException('There is no configured Auth services.'); } - return reset($this->services); + return $firstAuth; } } diff --git a/src/Onelogin/AuthRegistryInterface.php b/src/Onelogin/AuthRegistryInterface.php index 7bbfa87..1d5988d 100644 --- a/src/Onelogin/AuthRegistryInterface.php +++ b/src/Onelogin/AuthRegistryInterface.php @@ -12,11 +12,16 @@ */ interface AuthRegistryInterface { - public function addService(string $key, Auth $auth): self; + public function addService(string $idpKey, string $spKey, Auth $auth): self; - public function hasService(string $key): bool; + public function hasService(string $idpKey, string $spKey): bool; - public function getService(string $key): Auth; + /** + * Get the Auth service for the given IdP and SP keys. + * + * @throws \OutOfBoundsException if the service does not exist + */ + public function getService(string $idpKey, string $spKey): Auth; public function getDefaultService(): Auth; } diff --git a/src/Resources/config/routes.php b/src/Resources/config/routes.php index 149993e..46954f8 100644 --- a/src/Resources/config/routes.php +++ b/src/Resources/config/routes.php @@ -6,28 +6,30 @@ use Nbgrp\OneloginSamlBundle\Controller; use Symfony\Component\Routing\Loader\Configurator\RoutingConfigurator; -/** - * @suppress PhanUndeclaredFunctionInCallable - */ +// @suppress PhanUndeclaredFunctionInCallable return static function (RoutingConfigurator $routes): void { $routes->add('saml_metadata', '/saml/metadata') ->controller(Controller\Metadata::class) ->defaults(['idp' => null]) + ->defaults(['sp' => null]) ; $routes->add('saml_acs', '/saml/acs') ->controller(Controller\AssertionConsumerService::class) ->defaults(['idp' => null]) + ->defaults(['sp' => null]) ->methods(['POST']) ; $routes->add('saml_login', '/saml/login') ->controller(Controller\Login::class) ->defaults(['idp' => null]) + ->defaults(['sp' => null]) ; $routes->add('saml_logout', '/saml/logout') ->controller(Controller\Logout::class) ->defaults(['idp' => null]) + ->defaults(['sp' => null]) ; }; diff --git a/src/Resources/config/services.php b/src/Resources/config/services.php index d87b5e3..f977cfd 100644 --- a/src/Resources/config/services.php +++ b/src/Resources/config/services.php @@ -1,4 +1,5 @@ set(Idp\IdpResolverInterface::class, Idp\IdpResolver::class) ->args([ param('nbgrp_onelogin_saml.idp_parameter_name'), + param('nbgrp_onelogin_saml.sp_parameter_name'), ]) ->set(Onelogin\AuthArgumentResolver::class) @@ -85,7 +87,8 @@ /* 7 */ null, // user factory /* 8 */ service(LoggerInterface::class)->nullOnInvalid(), /* 9 */ param('nbgrp_onelogin_saml.idp_parameter_name'), - /* 10 */ param('nbgrp_onelogin_saml.use_proxy_vars'), + /* 10 */ param('nbgrp_onelogin_saml.sp_parameter_name'), + /* 11 */ param('nbgrp_onelogin_saml.use_proxy_vars'), ]) ; }; diff --git a/src/Security/Http/Authentication/SamlAuthenticationSuccessHandler.php b/src/Security/Http/Authentication/SamlAuthenticationSuccessHandler.php index 9209907..8844923 100644 --- a/src/Security/Http/Authentication/SamlAuthenticationSuccessHandler.php +++ b/src/Security/Http/Authentication/SamlAuthenticationSuccessHandler.php @@ -18,19 +18,20 @@ class SamlAuthenticationSuccessHandler extends DefaultAuthenticationSuccessHandl public const RELAY_STATE = 'RelayState'; /** @psalm-suppress MixedArrayAccess */ + #[\Override] protected function determineTargetUrl(Request $request): string { if ($this->options['always_use_default_target_path']) { return (string) $this->options['default_target_path']; } - /** @psalm-suppress InvalidArgument */ - $relayState = $request->query->get(self::RELAY_STATE, $request->request->get(self::RELAY_STATE)); - if ($relayState !== null && $this->httpUtils instanceof HttpUtils) { - /** @psalm-suppress RedundantCastGivenDocblockType */ - $relayState = (string) $relayState; + $relayState = $request->query->get(self::RELAY_STATE, ''); + if ('' === $relayState) { + $relayState = $request->request->get(self::RELAY_STATE, ''); + } + if ('' !== $relayState && $this->httpUtils instanceof HttpUtils) { if ($relayState !== $this->httpUtils->generateUri($request, (string) $this->options['login_path'])) { - return $relayState; + return (string) $relayState; } } diff --git a/src/Security/Http/Authenticator/Passport/Badge/DeferredEventBadge.php b/src/Security/Http/Authenticator/Passport/Badge/DeferredEventBadge.php index 076d674..8058904 100644 --- a/src/Security/Http/Authenticator/Passport/Badge/DeferredEventBadge.php +++ b/src/Security/Http/Authenticator/Passport/Badge/DeferredEventBadge.php @@ -30,6 +30,7 @@ public function getEvent(): ?Event } } + #[\Override] public function isResolved(): bool { return $this->resolved; diff --git a/src/Security/Http/Authenticator/Passport/Badge/SamlAttributesBadge.php b/src/Security/Http/Authenticator/Passport/Badge/SamlAttributesBadge.php index 5148024..ea9069d 100644 --- a/src/Security/Http/Authenticator/Passport/Badge/SamlAttributesBadge.php +++ b/src/Security/Http/Authenticator/Passport/Badge/SamlAttributesBadge.php @@ -10,17 +10,24 @@ /** * Allows to add SAML attributes to a passport. */ -class SamlAttributesBadge implements BadgeInterface +readonly class SamlAttributesBadge implements BadgeInterface { + /** + * @param array $attributes + */ public function __construct( - private readonly array $attributes, + private array $attributes, ) {} + /** + * @return array + */ public function getAttributes(): array { return $this->attributes; } + #[\Override] public function isResolved(): bool { return true; diff --git a/src/Security/Http/Authenticator/SamlAuthenticator.php b/src/Security/Http/Authenticator/SamlAuthenticator.php index 83772c3..c814dfe 100644 --- a/src/Security/Http/Authenticator/SamlAuthenticator.php +++ b/src/Security/Http/Authenticator/SamlAuthenticator.php @@ -28,6 +28,7 @@ use Symfony\Component\Security\Core\Exception\LogicException; use Symfony\Component\Security\Core\Exception\SessionUnavailableException; use Symfony\Component\Security\Core\Exception\UserNotFoundException; +use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Core\User\UserProviderInterface; use Symfony\Component\Security\Http\Authentication\AuthenticationFailureHandlerInterface; use Symfony\Component\Security\Http\Authentication\AuthenticationSuccessHandlerInterface; @@ -44,6 +45,10 @@ class SamlAuthenticator implements AuthenticatorInterface, AuthenticationEntryPo public const SESSION_INDEX_ATTRIBUTE = '_saml_session_index'; public const LAST_REQUEST_ID = '_saml_last_request_id'; + /** + * @param UserProviderInterface $userProvider + * @param array $options + */ public function __construct( private readonly HttpUtils $httpUtils, private readonly UserProviderInterface $userProvider, @@ -55,26 +60,30 @@ public function __construct( private readonly ?SamlUserFactoryInterface $userFactory, private readonly ?LoggerInterface $logger, private readonly string $idpParameterName, + private readonly string $spParameterName, private readonly bool $useProxyVars, ) {} + #[\Override] public function supports(Request $request): ?bool { return $request->isMethod('POST') && $this->httpUtils->checkRequestPath($request, (string) $this->options['check_path']); } + #[\Override] public function start(Request $request, ?AuthenticationException $authException = null): Response { + $resolve = $this->idpResolver->resolve($request); $uri = $this->httpUtils->generateUri($request, (string) $this->options['login_path']); - $idp = $this->idpResolver->resolve($request); - if ($idp) { - $uri .= '?'.$this->idpParameterName.'='.$idp; - } + // Add the IDP and SP parameters to the URI + $uri .= '?'.$this->idpParameterName.'='.$resolve['idp']; + $uri .= '&'.$this->spParameterName.'='.$resolve['sp']; return new RedirectResponse($uri); } + #[\Override] public function authenticate(Request $request): Passport { if (!$request->hasSession()) { @@ -86,7 +95,7 @@ public function authenticate(Request $request): Passport $this->processResponse($oneLoginAuth, $request->getSession()); - if ($oneLoginAuth->getErrors()) { + if (\count($oneLoginAuth->getErrors()) > 0) { $errorReason = $oneLoginAuth->getLastErrorReason() ?? 'Undefined OneLogin auth error.'; $this->logger?->error($errorReason); @@ -96,13 +105,15 @@ public function authenticate(Request $request): Passport return $this->createPassport($oneLoginAuth); } + #[\Override] public function createToken(Passport $passport, string $firewallName): TokenInterface { if (!$passport->hasBadge(SamlAttributesBadge::class)) { - throw new LogicException(sprintf('Passport should contains a "%s" badge.', SamlAttributesBadge::class)); + throw new LogicException(\sprintf('Passport should contains a "%s" badge.', SamlAttributesBadge::class)); } $badge = $passport->getBadge(SamlAttributesBadge::class); + /** @var array $attributes */ $attributes = []; if ($badge instanceof SamlAttributesBadge) { @@ -112,11 +123,13 @@ public function createToken(Passport $passport, string $firewallName): TokenInte return new SamlToken($passport->getUser(), $firewallName, $passport->getUser()->getRoles(), $attributes); } + #[\Override] public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response { return $this->successHandler->onAuthenticationSuccess($request, $token); } + #[\Override] public function onAuthenticationFailure(Request $request, AuthenticationException $exception): ?Response { return $this->failureHandler->onAuthenticationFailure($request, $exception); @@ -126,7 +139,7 @@ protected function processResponse(Auth $oneLoginAuth, SessionInterface $session { $requestId = null; $security = $oneLoginAuth->getSettings()->getSecurityData(); - if ($security['rejectUnsolicitedResponsesWithInResponseTo'] ?? false) { + if (($security['rejectUnsolicitedResponsesWithInResponseTo'] ?? false) === true) { /** @var string $requestId */ $requestId = $session->get(self::LAST_REQUEST_ID); } @@ -177,9 +190,13 @@ function (string $identifier) use ($deferredEventBadge, $attributes) { ]); } + /** + * @return array + */ protected function extractAttributes(Auth $oneLoginAuth): array { - $attributes = $this->options['use_attribute_friendly_name'] ?? false + $useAttributeFriendlyName = isset($this->options['use_attribute_friendly_name']) && (bool) $this->options['use_attribute_friendly_name']; + $attributes = $useAttributeFriendlyName ? $oneLoginAuth->getAttributesWithFriendlyName() : $oneLoginAuth->getAttributes(); $attributes[self::SESSION_INDEX_ATTRIBUTE] = $oneLoginAuth->getSessionIndex(); @@ -187,12 +204,16 @@ protected function extractAttributes(Auth $oneLoginAuth): array return $attributes; } + /** + * Extracts user identifier from SAML attributes. + * + * @param array $attributes + */ protected function extractIdentifier(Auth $oneLoginAuth, array $attributes): string { - if (empty($this->options['identifier_attribute'])) { + if (!isset($this->options['identifier_attribute']) || '' === (string) $this->options['identifier_attribute']) { return $oneLoginAuth->getNameId(); } - $identifierAttribute = (string) $this->options['identifier_attribute']; if (!\array_key_exists($identifierAttribute, $attributes)) { throw new \RuntimeException('Attribute "'.$identifierAttribute.'" not found in SAML data.'); @@ -214,9 +235,11 @@ protected function extractIdentifier(Auth $oneLoginAuth, array $attributes): str private function getOneLoginAuth(Request $request): Auth { try { - $idp = $this->idpResolver->resolve($request); - $authService = $idp - ? $this->authRegistry->getService($idp) + $resolve = $this->idpResolver->resolve($request); + $idp = $resolve['idp']; + $sp = $resolve['sp']; + $authService = ('' !== $idp) && ('' !== $sp) + ? $this->authRegistry->getService($idp, $sp) : $this->authRegistry->getDefaultService(); } catch (\RuntimeException $exception) { $this->logger?->error($exception->getMessage()); diff --git a/src/Security/Http/Authenticator/Token/SamlToken.php b/src/Security/Http/Authenticator/Token/SamlToken.php index 11e9e18..999458f 100644 --- a/src/Security/Http/Authenticator/Token/SamlToken.php +++ b/src/Security/Http/Authenticator/Token/SamlToken.php @@ -11,7 +11,11 @@ class SamlToken extends PostAuthenticationToken { /** - * @param array $roles + * Constructor for the SAML authentication token. + * This token is used to store the authenticated user and their SAML attributes. + * + * @param array $roles + * @param array $samlAttributes */ public function __construct(UserInterface $user, string $firewallName, array $roles, array $samlAttributes) { diff --git a/src/Security/User/SamlUserFactory.php b/src/Security/User/SamlUserFactory.php index dee41e6..321ab30 100644 --- a/src/Security/User/SamlUserFactory.php +++ b/src/Security/User/SamlUserFactory.php @@ -18,12 +18,23 @@ public function __construct( private readonly array $mapping, ) {} + /** + * Creates a user instance based on the provided identifier and attributes. + * + * @param string $identifier the unique identifier for the user + * @param array $attributes the attributes associated with the user + * + * @return UserInterface the created user instance + * + * @throws \ReflectionException + */ + #[\Override] public function createUser(string $identifier, array $attributes): UserInterface { $user = new $this->userClass($identifier); $reflection = new \ReflectionClass($this->userClass); - /** @psalm-suppress MixedAssignment */ + /** @psalm-suppress MixedAssignment, MixedArgumentTypeCoercion */ foreach ($this->mapping as $field => $attribute) { $property = $reflection->getProperty($field); $property->setValue( @@ -37,6 +48,16 @@ public function createUser(string $identifier, array $attributes): UserInterface return $user; } + /** + * Retrieves the value of a specific attribute from the provided attributes array. + * + * @param array $attributes the attributes array + * @param string $attribute the attribute to retrieve + * + * @return mixed the value of the attribute + * + * @throws \RuntimeException if the attribute is not found in the attributes array + */ private function getAttributeValue(array $attributes, string $attribute): mixed { $isArrayValue = str_ends_with($attribute, '[]'); diff --git a/src/Security/User/SamlUserFactoryInterface.php b/src/Security/User/SamlUserFactoryInterface.php index 07ce3ce..652454d 100644 --- a/src/Security/User/SamlUserFactoryInterface.php +++ b/src/Security/User/SamlUserFactoryInterface.php @@ -14,5 +14,13 @@ */ interface SamlUserFactoryInterface { + /** + * Creates a user based on the provided identifier and attributes. + * + * @param string $identifier The unique identifier for the user (e.g., email, username). + * @param array $attributes The attributes associated with the user (e.g., SAML attributes). + * + * @return UserInterface the created user instance + */ public function createUser(string $identifier, array $attributes): UserInterface; } diff --git a/src/Security/User/SamlUserInterface.php b/src/Security/User/SamlUserInterface.php index 288054e..33cbf6a 100644 --- a/src/Security/User/SamlUserInterface.php +++ b/src/Security/User/SamlUserInterface.php @@ -12,5 +12,10 @@ */ interface SamlUserInterface extends UserInterface { + /** + * Sets the SAML attributes for the user. + * + * @param array $attributes + */ public function setSamlAttributes(array $attributes): void; } diff --git a/src/Security/User/SamlUserProvider.php b/src/Security/User/SamlUserProvider.php index bd1dfbb..52bbd48 100644 --- a/src/Security/User/SamlUserProvider.php +++ b/src/Security/User/SamlUserProvider.php @@ -12,7 +12,7 @@ /** * Just instantiates user object with providing identifier and default roles. * - * @template-covariant TUser of UserInterface + * @template TUser of UserInterface * * @template-implements UserProviderInterface */ @@ -20,21 +20,24 @@ class SamlUserProvider implements UserProviderInterface { /** * @param class-string $userClass + * @param array $defaultRoles */ public function __construct( protected string $userClass, protected array $defaultRoles, ) { - if (!is_a($userClass, UserInterface::class, true)) { - throw new \InvalidArgumentException('The $userClass argument should be a class implementing the '.UserInterface::class.' interface.'); + if (!is_a($userClass, SamlUserInterface::class, true)) { + throw new \InvalidArgumentException('The $userClass argument should be a class implementing the '.SamlUserInterface::class.' interface.'); } } + #[\Override] public function loadUserByIdentifier(string $identifier): UserInterface { return new $this->userClass($identifier, $this->defaultRoles); } + #[\Override] public function refreshUser(UserInterface $user): UserInterface { if (!$user instanceof $this->userClass) { @@ -44,6 +47,7 @@ public function refreshUser(UserInterface $user): UserInterface return $user; } + #[\Override] public function supportsClass(string $class): bool { return is_a($class, $this->userClass, true); diff --git a/tests/Controller/LoginTest.php b/tests/Controller/LoginTest.php index d8b1a7e..513478a 100644 --- a/tests/Controller/LoginTest.php +++ b/tests/Controller/LoginTest.php @@ -9,6 +9,9 @@ use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\SamlAuthenticator; use OneLogin\Saml2\Auth; use OneLogin\Saml2\Settings; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\MockObject\Exception as MockObjectException; use PHPUnit\Framework\TestCase; use Symfony\Bundle\SecurityBundle\Security\FirewallConfig; use Symfony\Bundle\SecurityBundle\Security\FirewallMap; @@ -19,15 +22,17 @@ use Symfony\Component\Security\Http\SecurityRequestAttributes; /** - * @covers \Nbgrp\OneloginSamlBundle\Controller\Login - * * @internal */ +#[CoversClass(Login::class)] final class LoginTest extends TestCase { + /** + * @throws MockObjectException + */ public function testInvokeWithRejectUnsolicitedResponsesWithInResponseTo(): void { - $firewallMap = $this->createStub(FirewallMap::class); + $firewallMap = self::createStub(FirewallMap::class); $firewallMap ->method('getFirewallConfig') ->willReturn(new FirewallConfig('foo', 'bar')) @@ -65,9 +70,12 @@ public function testInvokeWithRejectUnsolicitedResponsesWithInResponseTo(): void self::assertSame('requestID', $session->get(SamlAuthenticator::LAST_REQUEST_ID)); } + /** + * @throws MockObjectException + */ public function testInvokeWithoutRejectUnsolicitedResponsesWithInResponseTo(): void { - $firewallMap = $this->createStub(FirewallMap::class); + $firewallMap = self::createStub(FirewallMap::class); $firewallMap ->method('getFirewallConfig') ->willReturn(new FirewallConfig('foo', 'bar')) @@ -106,8 +114,9 @@ public function testInvokeWithoutRejectUnsolicitedResponsesWithInResponseTo(): v } /** - * @dataProvider provideErrorExceptionCases + * @throws MockObjectException */ + #[DataProvider('provideErrorExceptionCases')] public function testErrorException(Request $request, string $expectedMessage): void { $firewallMap = $this->createMock(FirewallMap::class); @@ -116,7 +125,7 @@ public function testErrorException(Request $request, string $expectedMessage): v ->willReturn(new FirewallConfig('foo', 'bar')) ; - $auth = $this->createStub(Auth::class); + $auth = self::createStub(Auth::class); $controller = new Login($firewallMap); @@ -125,7 +134,14 @@ public function testErrorException(Request $request, string $expectedMessage): v $controller($request, $auth); } - public function provideErrorExceptionCases(): iterable + /** + * Provides test cases for error exceptions. + * + * @return iterable + * + * @throws MockObjectException + */ + public static function provideErrorExceptionCases(): iterable { yield 'From attributes' => [ 'request' => (static function () { @@ -152,28 +168,34 @@ public function provideErrorExceptionCases(): iterable public function testAuthLoginWithoutRedirectUrlException(): void { - $firewallMap = $this->createMock(FirewallMap::class); - $firewallMap - ->method('getFirewallConfig') - ->willReturn(new FirewallConfig('foo', 'bar')) - ; - - $auth = $this->createMock(Auth::class); - $auth - ->method('login') - ->willReturn(null) - ; - - $request = Request::create('/login'); - $request->setSession(new Session(new MockArraySessionStorage())); - - $controller = new Login($firewallMap); - - $this->expectException(\RuntimeException::class); - $this->expectExceptionMessage('Login cannot be performed: Auth did not returned redirect url.'); - $controller($request, $auth); + try { + $firewallMap = $this->createMock(FirewallMap::class); + $firewallMap + ->method('getFirewallConfig') + ->willReturn(new FirewallConfig('foo', 'bar')) + ; + + $auth = $this->createMock(Auth::class); + $auth + ->method('login') + ->willReturn(null) + ; + + $request = Request::create('/login'); + $request->setSession(new Session(new MockArraySessionStorage())); + + $controller = new Login($firewallMap); + + $this->expectException(\RuntimeException::class); + $controller($request, $auth); + } catch (MockObjectException $e) { + self::fail('Failed to create mocks for OneLogin Auth. '.$e->getMessage()); + } } + /** + * @throws MockObjectException + */ public function testUnknownFirewallException(): void { $firewallMap = $this->createMock(FirewallMap::class); @@ -182,7 +204,7 @@ public function testUnknownFirewallException(): void ->willReturn(null) ; - $auth = $this->createStub(Auth::class); + $auth = self::createStub(Auth::class); $controller = new Login($firewallMap); $request = Request::create('/login'); diff --git a/tests/DependencyInjection/Compiler/AuthRegistryCompilerPassTest.php b/tests/DependencyInjection/Compiler/AuthRegistryCompilerPassTest.php index 8beba6c..8626174 100644 --- a/tests/DependencyInjection/Compiler/AuthRegistryCompilerPassTest.php +++ b/tests/DependencyInjection/Compiler/AuthRegistryCompilerPassTest.php @@ -7,15 +7,15 @@ use Nbgrp\OneloginSamlBundle\DependencyInjection\Compiler\AuthRegistryCompilerPass; use Nbgrp\OneloginSamlBundle\Onelogin\AuthRegistryInterface; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Definition; /** - * @covers \Nbgrp\OneloginSamlBundle\DependencyInjection\Compiler\AuthRegistryCompilerPass - * * @internal */ +#[CoversClass(AuthRegistryCompilerPass::class)] final class AuthRegistryCompilerPassTest extends TestCase { public function testSuccessProcess(): void @@ -32,7 +32,7 @@ public function testSuccessProcess(): void $authRegistryDefinition = $container->getDefinition(AuthRegistryInterface::class); self::assertCount(2, $authRegistryDefinition->getMethodCalls()); - /** @var array $call */ + /** @var array $call */ foreach ($authRegistryDefinition->getMethodCalls() as $call) { self::assertSame('addService', reset($call)); } diff --git a/tests/DependencyInjection/Compiler/EntityManagerCompilerPassTest.php b/tests/DependencyInjection/Compiler/EntityManagerCompilerPassTest.php index ec4a660..a0b30dd 100644 --- a/tests/DependencyInjection/Compiler/EntityManagerCompilerPassTest.php +++ b/tests/DependencyInjection/Compiler/EntityManagerCompilerPassTest.php @@ -8,16 +8,16 @@ use Nbgrp\OneloginSamlBundle\DependencyInjection\Compiler\EntityManagerCompilerPass; use Nbgrp\OneloginSamlBundle\EventListener\User\UserCreatedListener; use Nbgrp\OneloginSamlBundle\EventListener\User\UserModifiedListener; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Symfony\Component\DependencyInjection\ChildDefinition; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Definition; /** - * @covers \Nbgrp\OneloginSamlBundle\DependencyInjection\Compiler\EntityManagerCompilerPass - * * @internal */ +#[CoversClass(EntityManagerCompilerPass::class)] final class EntityManagerCompilerPassTest extends TestCase { public function testNoProcess(): void diff --git a/tests/DependencyInjection/Compiler/ProxyVarsCompilerPassTest.php b/tests/DependencyInjection/Compiler/ProxyVarsCompilerPassTest.php index 0890abf..b5f6996 100644 --- a/tests/DependencyInjection/Compiler/ProxyVarsCompilerPassTest.php +++ b/tests/DependencyInjection/Compiler/ProxyVarsCompilerPassTest.php @@ -7,19 +7,18 @@ use Nbgrp\OneloginSamlBundle\DependencyInjection\Compiler\ProxyVarsCompilerPass; use OneLogin\Saml2\Utils; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Symfony\Component\DependencyInjection\ContainerBuilder; /** - * @covers \Nbgrp\OneloginSamlBundle\DependencyInjection\Compiler\ProxyVarsCompilerPass - * * @internal */ +#[CoversClass(ProxyVarsCompilerPass::class)] final class ProxyVarsCompilerPassTest extends TestCase { - /** - * @dataProvider provideProcessCases - */ + #[DataProvider('provideProcessCases')] public function testProcess(bool $useVars): void { $container = new ContainerBuilder(); @@ -30,7 +29,8 @@ public function testProcess(bool $useVars): void self::assertSame($useVars, Utils::getProxyVars()); } - public function provideProcessCases(): iterable + /** @return iterable */ + public static function provideProcessCases(): iterable { yield 'Use vars' => [ 'useVars' => true, diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index bbeaf91..a2a166a 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -6,28 +6,32 @@ namespace Nbgrp\Tests\OneloginSamlBundle\DependencyInjection; use Nbgrp\OneloginSamlBundle\DependencyInjection\Configuration; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException; use Symfony\Component\Config\Definition\Processor; /** - * @covers \Nbgrp\OneloginSamlBundle\DependencyInjection\Configuration - * * @internal */ +#[CoversClass(Configuration::class)] final class ConfigurationTest extends TestCase { private Processor $processor; /** - * @dataProvider provideValidConfigCases + * @param array $config + * @param array $expected */ + #[DataProvider('provideValidConfigCases')] public function testValidConfig(array $config, array $expected): void { self::assertSame($expected, $this->processor->processConfiguration(new Configuration(), [$config])); } - public function provideValidConfigCases(): iterable + /** @return iterable, expected: array}> */ + public static function provideValidConfigCases(): iterable { yield 'Simple configuration' => [ 'config' => [ @@ -51,20 +55,23 @@ public function provideValidConfigCases(): iterable 'url' => 'http://example.com/sso', ], ], - 'baseurl' => '/saml/', 'sp' => [ - 'entityId' => '/saml/metadata', - 'assertionConsumerService' => [ - 'url' => '/saml/acs', - ], - 'singleLogoutService' => [ - 'url' => '/saml/logout', + 'default' => [ + 'entityId' => '/saml/metadata', + 'assertionConsumerService' => [ + 'url' => '/saml/acs', + ], + 'singleLogoutService' => [ + 'url' => '/saml/logout', + ], ], ], + 'baseurl' => '/saml/', ], ], 'use_proxy_vars' => false, 'idp_parameter_name' => 'idp', + 'sp_parameter_name' => 'sp', ], ]; @@ -177,6 +184,7 @@ public function provideValidConfigCases(): iterable ], 'use_proxy_vars' => true, 'idp_parameter_name' => 'custom-idp', + 'sp_parameter_name' => 'custom-sp', 'entity_manager_name' => 'custom-em', ], 'expected' => [ @@ -211,19 +219,21 @@ public function provideValidConfigCases(): iterable ], ], 'sp' => [ - 'entityId' => 'test-sp', - 'assertionConsumerService' => [ - 'url' => 'http://example.com/saml/acs', - 'binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST', - ], - 'singleLogoutService' => [ - 'url' => 'http://example.com/saml/logout', - 'binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect', + 'default' => [ + 'entityId' => 'test-sp', + 'assertionConsumerService' => [ + 'url' => 'http://example.com/saml/acs', + 'binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST', + ], + 'singleLogoutService' => [ + 'url' => 'http://example.com/saml/logout', + 'binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect', + ], + 'NameIDFormat' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress', + 'x509cert' => 'x509cert-data', + 'privateKey' => 'private-key', + 'x509certNew' => 'some-new-x509cert', ], - 'NameIDFormat' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress', - 'x509cert' => 'x509cert-data', - 'privateKey' => 'private-key', - 'x509certNew' => 'some-new-x509cert', ], 'compress' => [ 'requests' => false, @@ -282,13 +292,15 @@ public function provideValidConfigCases(): iterable ], ], 'sp' => [ - 'entityId' => 'test-sp', - 'assertionConsumerService' => [ - 'url' => 'http://example.com/saml/acs', - 'binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST', - ], - 'singleLogoutService' => [ - 'url' => '/saml/logout', + 'default' => [ + 'entityId' => 'test-sp', + 'assertionConsumerService' => [ + 'url' => 'http://example.com/saml/acs', + 'binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST', + ], + 'singleLogoutService' => [ + 'url' => '/saml/logout', + ], ], ], 'baseurl' => '/saml/', @@ -296,14 +308,16 @@ public function provideValidConfigCases(): iterable ], 'use_proxy_vars' => true, 'idp_parameter_name' => 'custom-idp', + 'sp_parameter_name' => 'custom-sp', 'entity_manager_name' => 'custom-em', ], ]; } /** - * @dataProvider provideConfigWithInvalidOneLoginSettingsExceptionCases + * @param array $config */ + #[DataProvider('provideConfigWithInvalidOneLoginSettingsExceptionCases')] public function testConfigWithInvalidOneLoginSettingsException(array $config, string $expectedMessage): void { $this->expectException(InvalidConfigurationException::class); @@ -311,7 +325,8 @@ public function testConfigWithInvalidOneLoginSettingsException(array $config, st $this->processor->processConfiguration(new Configuration(), [$config]); } - public function provideConfigWithInvalidOneLoginSettingsExceptionCases(): iterable + /** @return iterable, expectedMessage: string}> */ + public static function provideConfigWithInvalidOneLoginSettingsExceptionCases(): iterable { yield 'Empty idp OneLogin settings' => [ 'config' => [ @@ -412,7 +427,7 @@ public function provideConfigWithInvalidOneLoginSettingsExceptionCases(): iterab ], ], ], - 'expectedMessage' => 'Invalid configuration for path "nbgrp_onelogin_saml.onelogin_settings.test.sp.assertionConsumerService.binding": invalid value.', + 'expectedMessage' => 'Invalid configuration for path "nbgrp_onelogin_saml.onelogin_settings.test.sp.default.assertionConsumerService.binding": invalid value.', ]; yield 'Invalid singleLogoutService binding for SP OneLogin settings' => [ @@ -439,7 +454,7 @@ public function provideConfigWithInvalidOneLoginSettingsExceptionCases(): iterab ], ], ], - 'expectedMessage' => 'Invalid configuration for path "nbgrp_onelogin_saml.onelogin_settings.test.sp.singleLogoutService.binding": invalid value.', + 'expectedMessage' => 'Invalid configuration for path "nbgrp_onelogin_saml.onelogin_settings.test.sp.default.singleLogoutService.binding": invalid value.', ]; yield 'Invalid NameIDFormat for SP OneLogin settings' => [ @@ -463,7 +478,7 @@ public function provideConfigWithInvalidOneLoginSettingsExceptionCases(): iterab ], ], ], - 'expectedMessage' => 'Invalid configuration for path "nbgrp_onelogin_saml.onelogin_settings.test.sp.NameIDFormat": invalid value.', + 'expectedMessage' => 'Invalid configuration for path "nbgrp_onelogin_saml.onelogin_settings.test.sp.default.NameIDFormat": invalid value.', ]; yield 'Invalid requestedAuthnContext type for security OneLogin settings' => [ diff --git a/tests/DependencyInjection/NbgrpOneloginSamlExtensionTest.php b/tests/DependencyInjection/NbgrpOneloginSamlExtensionTest.php index 370c69e..a4f80c7 100644 --- a/tests/DependencyInjection/NbgrpOneloginSamlExtensionTest.php +++ b/tests/DependencyInjection/NbgrpOneloginSamlExtensionTest.php @@ -12,14 +12,14 @@ use Nbgrp\OneloginSamlBundle\Idp\IdpResolverInterface; use Nbgrp\OneloginSamlBundle\Onelogin\AuthRegistryInterface; use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\SamlAuthenticator; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Symfony\Component\DependencyInjection\ContainerBuilder; /** - * @covers \Nbgrp\OneloginSamlBundle\DependencyInjection\NbgrpOneloginSamlExtension - * * @internal */ +#[CoversClass(NbgrpOneloginSamlExtension::class)] final class NbgrpOneloginSamlExtensionTest extends TestCase { public function testLoad(): void @@ -60,12 +60,14 @@ public function testLoad(): void ], ], 'sp' => [ - 'entityId' => 'test-sp', - 'assertionConsumerService' => [ - 'url' => 'http://example.com/saml/acs', - ], - 'singleLogoutService' => [ - 'url' => '/saml/logout', + 'default' => [ + 'entityId' => 'test-sp', + 'assertionConsumerService' => [ + 'url' => 'http://example.com/saml/acs', + ], + 'singleLogoutService' => [ + 'url' => '/saml/logout', + ], ], ], 'baseurl' => '/saml/', diff --git a/tests/DependencyInjection/Security/Factory/SamlFactoryTest.php b/tests/DependencyInjection/Security/Factory/SamlFactoryTest.php index 51a3191..2ff921b 100644 --- a/tests/DependencyInjection/Security/Factory/SamlFactoryTest.php +++ b/tests/DependencyInjection/Security/Factory/SamlFactoryTest.php @@ -10,6 +10,7 @@ use Nbgrp\OneloginSamlBundle\EventListener\User\UserModifiedListener; use Nbgrp\OneloginSamlBundle\Security\Http\Authentication\SamlAuthenticationSuccessHandler; use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\SamlAuthenticator; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition; use Symfony\Component\DependencyInjection\ChildDefinition; @@ -18,12 +19,12 @@ use Symfony\Component\DependencyInjection\Reference; /** - * @covers \Nbgrp\OneloginSamlBundle\DependencyInjection\Security\Factory\SamlFactory - * * @internal */ +#[CoversClass(SamlFactory::class)] final class SamlFactoryTest extends TestCase { + /** @psalm-suppress PropertyNotSetInConstructor */ private SamlFactory $factory; public function testDefaultConfiguration(): void @@ -97,7 +98,7 @@ public function testCreateAuthenticator(): void self::assertSame(SamlAuthenticationSuccessHandler::class, $samlSuccessHandlerReference->getParent()); } - /** @var array $options */ + /** @var array $options */ $options = $authenticatorDefinition->getArgument(6); self::assertSame([ 'persist_user' => true, diff --git a/tests/DependencyInjection/Security/UserProvider/SamlUserProviderFactoryTest.php b/tests/DependencyInjection/Security/UserProvider/SamlUserProviderFactoryTest.php index 16ed884..68caa3d 100644 --- a/tests/DependencyInjection/Security/UserProvider/SamlUserProviderFactoryTest.php +++ b/tests/DependencyInjection/Security/UserProvider/SamlUserProviderFactoryTest.php @@ -7,18 +7,20 @@ use Nbgrp\OneloginSamlBundle\DependencyInjection\Security\UserProvider\SamlUserProviderFactory; use Nbgrp\Tests\OneloginSamlBundle\TestUser; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition; use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException; +use Symfony\Component\DependencyInjection\ChildDefinition; use Symfony\Component\DependencyInjection\ContainerBuilder; /** - * @covers \Nbgrp\OneloginSamlBundle\DependencyInjection\Security\UserProvider\SamlUserProviderFactory - * * @internal */ +#[CoversClass(SamlUserProviderFactory::class)] final class SamlUserProviderFactoryTest extends TestCase { + /** @psalm-suppress PropertyNotSetInConstructor */ private SamlUserProviderFactory $factory; public function testDefaultConfiguration(): void @@ -39,7 +41,7 @@ public function testNoUserClassInConfigurationException(): void $this->factory->addConfiguration($nodeDefinition); $node = $nodeDefinition->getNode(); - /** @var array $normalized */ + /** @var array $normalized */ $normalized = $node->normalize([]); $this->expectException(InvalidConfigurationException::class); @@ -53,7 +55,7 @@ public function testInvalidUserClassInConfigurationException(): void $this->factory->addConfiguration($nodeDefinition); $node = $nodeDefinition->getNode(); - /** @var array $normalized */ + /** @var array $normalized */ $normalized = $node->normalize(['user_class' => \stdClass::class]); $this->expectException(InvalidConfigurationException::class); @@ -69,7 +71,7 @@ public function testCreate(): void 'default_roles' => ['ROLE_USER'], ]); - /** @var \Symfony\Component\DependencyInjection\ChildDefinition $providerDefinition */ + /** @var ChildDefinition $providerDefinition */ $providerDefinition = $container->getDefinition('provider_id'); self::assertSame(TestUser::class, $providerDefinition->getArgument(0)); self::assertSame(['ROLE_USER'], $providerDefinition->getArgument(1)); diff --git a/tests/EventListener/Security/SamlLogoutListenerTest.php b/tests/EventListener/Security/SamlLogoutListenerTest.php index d2c7546..60d2a90 100644 --- a/tests/EventListener/Security/SamlLogoutListenerTest.php +++ b/tests/EventListener/Security/SamlLogoutListenerTest.php @@ -7,131 +7,94 @@ use Nbgrp\OneloginSamlBundle\EventListener\Security\SamlLogoutListener; use Nbgrp\OneloginSamlBundle\Idp\IdpResolver; -use Nbgrp\OneloginSamlBundle\Idp\IdpResolverInterface; use Nbgrp\OneloginSamlBundle\Onelogin\AuthRegistry; use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\SamlAuthenticator; use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Token\SamlToken; use Nbgrp\Tests\OneloginSamlBundle\TestUser; use OneLogin\Saml2\Auth; +use OneLogin\Saml2\Error; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\MockObject\Exception as MockException; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Http\Event\LogoutEvent; /** - * @covers \Nbgrp\OneloginSamlBundle\EventListener\Security\SamlLogoutListener - * * @internal */ +#[CoversClass(SamlLogoutListener::class)] final class SamlLogoutListenerTest extends TestCase { - /** - * @dataProvider provideCases - */ - public function test(AuthRegistry $authRegistry, IdpResolverInterface $ipdResolver, Request $request, ?TokenInterface $token): void + #[DataProvider('provideCases')] + public function test(?TokenInterface $token, ?string $sessionIndex): void { - $event = $this->createMock(LogoutEvent::class); - $event - ->method('getRequest') - ->willReturn($request) - ; - $event - ->expects($token ? self::once() : self::never()) - ->method('getToken') - ->willReturn($token) - ; + try { + $auth = $this->createMock(Auth::class); - (new SamlLogoutListener($authRegistry, $ipdResolver))->processSingleLogout($event); - } - - public function provideCases(): iterable - { - yield 'No Auth service' => [ - 'authRegistry' => (function (): AuthRegistry { - $auth = $this->createMock(Auth::class); - $auth - ->expects(self::never()) + if ($token instanceof SamlToken) { + $auth->expects(self::once()) ->method('processSLO') + ->willThrowException(new Error('error')) + ; + + $auth->method('getSLOurl')->willReturn('some_slo_url'); + + $auth->expects(self::once()) + ->method('logout') + ->with(null, [], 'tester', $sessionIndex) ; + } else { + $auth->expects(self::never())->method('logout'); + } + + $authRegistry = new AuthRegistry(); + $authRegistry->addService('default', 'default', $auth); + + $request = Request::create('/logout', 'GET', ['idp' => 'default', 'sp' => 'default']); - $authRegistry = new AuthRegistry(); - $authRegistry->addService('foo', $auth); + $event = $this->createMock(LogoutEvent::class); + $event->method('getRequest')->willReturn($request); + $event->method('getToken')->willReturn($token); + + $listener = new SamlLogoutListener($authRegistry, new IdpResolver('idp', 'sp')); + $listener->processSingleLogout($event); + } catch (MockException $e) { + self::fail('Mock creation failed: '.$e->getMessage()); + } + } - return $authRegistry; - })(), - 'ipdResolver' => new IdpResolver('idp'), - 'request' => Request::create('/logout', 'GET', ['idp' => 'unknown']), + /** + * @return iterable + * + * @throws MockException + */ + public static function provideCases(): iterable + { + yield 'No Auth service' => [ 'token' => null, + 'sessionIndex' => null, ]; yield 'Custom Auth service without SAML token' => [ - 'authRegistry' => (function (): AuthRegistry { - $auth = $this->createMock(Auth::class); - $auth - ->expects(self::never()) - ->method('processSLO') - ; - - $authRegistry = new AuthRegistry(); - $authRegistry->addService('foo', $auth); - - return $authRegistry; - })(), - 'ipdResolver' => new IdpResolver('idp'), - 'request' => Request::create('/logout', 'GET', ['idp' => 'foo']), - 'token' => $this->createStub(TokenInterface::class), + 'token' => self::createStub(TokenInterface::class), + 'sessionIndex' => null, ]; yield 'Logout without session index' => [ - 'authRegistry' => (function (): AuthRegistry { - $auth = $this->createMock(Auth::class); - $auth - ->method('processSLO') - ->willThrowException(new \OneLogin\Saml2\Error('error')) - ; - $auth - ->method('getSLOurl') - ->willReturn('some_slo_url') - ; - $auth - ->method('logout') - ->with(null, [], 'tester', null) - ; - - $authRegistry = new AuthRegistry(); - $authRegistry->addService('foo', $auth); - - return $authRegistry; - })(), - 'ipdResolver' => new IdpResolver('idp'), - 'request' => Request::create('/logout'), 'token' => new SamlToken(new TestUser('tester'), 'foo', [], []), + 'sessionIndex' => null, ]; yield 'Logout with session index' => [ - 'authRegistry' => (function (): AuthRegistry { - $auth = $this->createMock(Auth::class); - $auth - ->method('processSLO') - ->willThrowException(new \OneLogin\Saml2\Error('error')) - ; - $auth - ->method('getSLOurl') - ->willReturn('some_slo_url') - ; - $auth - ->method('logout') - ->with(null, [], 'tester', 'session_index') - ; - - $authRegistry = new AuthRegistry(); - $authRegistry->addService('foo', $auth); - - return $authRegistry; - })(), - 'ipdResolver' => new IdpResolver('idp'), - 'request' => Request::create('/logout'), - 'token' => new SamlToken(new TestUser('tester'), 'foo', [], [SamlAuthenticator::SESSION_INDEX_ATTRIBUTE => 'session_index']), + 'token' => new SamlToken( + new TestUser('tester'), + 'foo', + [], + [SamlAuthenticator::SESSION_INDEX_ATTRIBUTE => 'session_index'] + ), + 'sessionIndex' => 'session_index', ]; } } diff --git a/tests/EventListener/User/DeferredUserListenerTest.php b/tests/EventListener/User/DeferredUserListenerTest.php index 8f814d2..0e64139 100644 --- a/tests/EventListener/User/DeferredUserListenerTest.php +++ b/tests/EventListener/User/DeferredUserListenerTest.php @@ -8,6 +8,7 @@ use Nbgrp\OneloginSamlBundle\Event\AbstractUserEvent; use Nbgrp\OneloginSamlBundle\EventListener\User\DeferredUserListener; use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Passport\Badge\DeferredEventBadge; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; @@ -16,16 +17,16 @@ use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; /** - * @covers \Nbgrp\OneloginSamlBundle\EventListener\User\DeferredUserListener * * @internal */ +#[CoversClass(DeferredUserListener::class)] final class DeferredUserListenerTest extends TestCase { public function testWithoutBadge(): void { $event = new CheckPassportEvent( - $this->createStub(AuthenticatorInterface::class), + self::createStub(AuthenticatorInterface::class), new SelfValidatingPassport(new UserBadge('tester')), ); @@ -41,7 +42,7 @@ public function testWithoutBadge(): void public function testBadgeWithoutEvent(): void { $event = new CheckPassportEvent( - $this->createStub(AuthenticatorInterface::class), + self::createStub(AuthenticatorInterface::class), new SelfValidatingPassport(new UserBadge('tester'), [new DeferredEventBadge()]), ); @@ -62,7 +63,7 @@ public function testSuccessfulEventDispatching(): void $deferredEventBadge->setEvent($deferredEvent); $event = new CheckPassportEvent( - $this->createStub(AuthenticatorInterface::class), + self::createStub(AuthenticatorInterface::class), new SelfValidatingPassport(new UserBadge('tester'), [$deferredEventBadge]), ); diff --git a/tests/EventListener/User/UserListenersTest.php b/tests/EventListener/User/UserListenersTest.php index 54bd2dd..d389a83 100644 --- a/tests/EventListener/User/UserListenersTest.php +++ b/tests/EventListener/User/UserListenersTest.php @@ -6,79 +6,96 @@ namespace Nbgrp\Tests\OneloginSamlBundle\EventListener\User; use Doctrine\ORM\EntityManagerInterface; +use Nbgrp\OneloginSamlBundle\Event\AbstractUserEvent; use Nbgrp\OneloginSamlBundle\Event\UserCreatedEvent; use Nbgrp\OneloginSamlBundle\Event\UserModifiedEvent; +use Nbgrp\OneloginSamlBundle\EventListener\User\AbstractUserListener; use Nbgrp\OneloginSamlBundle\EventListener\User\UserCreatedListener; use Nbgrp\OneloginSamlBundle\EventListener\User\UserModifiedListener; use Nbgrp\Tests\OneloginSamlBundle\TestUser; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\MockObject\Exception as MockObjectException; use PHPUnit\Framework\TestCase; -use Symfony\Component\Security\Core\User\UserInterface; /** - * @covers \Nbgrp\OneloginSamlBundle\Event\AbstractUserEvent - * @covers \Nbgrp\OneloginSamlBundle\EventListener\User\AbstractUserListener - * @covers \Nbgrp\OneloginSamlBundle\EventListener\User\UserCreatedListener - * @covers \Nbgrp\OneloginSamlBundle\EventListener\User\UserModifiedListener - * * @internal */ +#[CoversClass(UserModifiedListener::class)] +#[CoversClass(UserCreatedListener::class)] +#[CoversClass(AbstractUserListener::class)] +#[CoversClass(AbstractUserEvent::class)] final class UserListenersTest extends TestCase { /** - * @dataProvider userListenerProvider + * @param class-string $listenerClass + * @param class-string $eventClass */ - public function testUserCreatedListener(?EntityManagerInterface $entityManager, bool $needPersist, UserInterface $user): void - { - (new UserCreatedListener($entityManager, $needPersist))(new UserCreatedEvent($user)); + #[DataProvider('provideUserListenersCases')] + public function testUserListeners( + string $listenerClass, + string $eventClass, + bool $needPersist, + ): void { + $user = new TestUser('tester'); + + try { + $entityManager = $this->createMock(EntityManagerInterface::class); + + if ($needPersist) { + $entityManager->expects(self::once())->method('persist')->with($user); + $entityManager->expects(self::once())->method('flush'); + } else { + $entityManager->expects(self::never())->method('persist'); + $entityManager->expects(self::never())->method('flush'); + } + } catch (MockObjectException $e) { + self::fail(\sprintf('Failed to create mock for EntityManagerInterface: %s', $e->getMessage())); + } + + $listener = match ($listenerClass) { + UserCreatedListener::class => new UserCreatedListener($entityManager, $needPersist), + UserModifiedListener::class => new UserModifiedListener($entityManager, $needPersist), + default => throw new \InvalidArgumentException("Unsupported listener class: {$listenerClass}"), + }; + + $event = match ($eventClass) { + UserCreatedEvent::class => new UserCreatedEvent($user), + UserModifiedEvent::class => new UserModifiedEvent($user), + default => throw new \InvalidArgumentException("Unsupported event class: {$eventClass}"), + }; + $listener($event); } /** - * @dataProvider userListenerProvider + * Provides test cases for user listeners. + * + * @return iterable */ - public function testUserModifiedListener(?EntityManagerInterface $entityManager, bool $needPersist, UserInterface $user): void + public static function provideUserListenersCases(): iterable { - (new UserModifiedListener($entityManager, $needPersist))(new UserModifiedEvent($user)); - } - - public function userListenerProvider(): iterable - { - yield 'needPersist false' => (function (): array { - $entityManager = $this->createMock(EntityManagerInterface::class); - $entityManager - ->expects(self::never()) - ->method('persist') - ; - $entityManager - ->expects(self::never()) - ->method('flush') - ; - - return [ - 'entityManager' => $entityManager, - 'needPersist' => false, - 'user' => new TestUser('tester'), - ]; - })(); + yield 'UserCreatedListener - no persist' => [ + 'listenerClass' => UserCreatedListener::class, + 'eventClass' => UserCreatedEvent::class, + 'needPersist' => false, + ]; - yield 'Success' => (function (): array { - $user = new TestUser('tester'); + yield 'UserCreatedListener - persist' => [ + 'listenerClass' => UserCreatedListener::class, + 'eventClass' => UserCreatedEvent::class, + 'needPersist' => true, + ]; - $entityManager = $this->createMock(EntityManagerInterface::class); - $entityManager - ->expects(self::once()) - ->method('persist') - ->with($user) - ; - $entityManager - ->expects(self::once()) - ->method('flush') - ; + yield 'UserModifiedListener - no persist' => [ + 'listenerClass' => UserModifiedListener::class, + 'eventClass' => UserModifiedEvent::class, + 'needPersist' => false, + ]; - return [ - 'entityManager' => $entityManager, - 'needPersist' => true, - 'user' => $user, - ]; - })(); + yield 'UserModifiedListener - persist' => [ + 'listenerClass' => UserModifiedListener::class, + 'eventClass' => UserModifiedEvent::class, + 'needPersist' => true, + ]; } } diff --git a/tests/Idp/IdpResolverTest.php b/tests/Idp/IdpResolverTest.php index 6dae595..cd06662 100644 --- a/tests/Idp/IdpResolverTest.php +++ b/tests/Idp/IdpResolverTest.php @@ -6,46 +6,61 @@ namespace Nbgrp\Tests\OneloginSamlBundle\Idp; use Nbgrp\OneloginSamlBundle\Idp\IdpResolver; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; /** - * @covers \Nbgrp\OneloginSamlBundle\Idp\IdpResolver - * * @internal */ +#[CoversClass(IdpResolver::class)] final class IdpResolverTest extends TestCase { + /** @psalm-suppress PropertyNotSetInConstructor */ private IdpResolver $resolver; - /** - * @dataProvider provideResolveCases - */ - public function testResolve(Request $request, ?string $expected): void + /** @param array{idp:string, sp:string} $expected */ + #[DataProvider('provideResolveCases')] + public function testResolve(Request $request, array $expected): void { self::assertSame($expected, $this->resolver->resolve($request)); } - public function provideResolveCases(): iterable + /** + * @return iterable + */ + public static function provideResolveCases(): iterable { yield 'Request with ipd in query' => [ 'request' => new Request(['idp' => 'query-idp']), - 'expected' => 'query-idp', + 'expected' => ['idp' => 'query-idp', 'sp' => 'default'], + ]; + + yield 'Request with ipd and sp in query' => [ + 'request' => new Request(['idp' => 'query-idp', 'sp' => 'query-sp']), + 'expected' => ['idp' => 'query-idp', 'sp' => 'query-sp'], ]; yield 'Request with ipd in attributes' => [ 'request' => new Request([], [], ['idp' => 'attributes-idp']), - 'expected' => 'attributes-idp', + 'expected' => ['idp' => 'attributes-idp', 'sp' => 'default'], + ]; + + yield 'Request with ipd and sp in attributes' => [ + 'request' => new Request([], [], ['idp' => 'attributes-idp', 'sp' => 'attributes-sp']), + 'expected' => ['idp' => 'attributes-idp', 'sp' => 'attributes-sp'], ]; yield 'Request without ipd' => [ 'request' => new Request(), - 'expected' => null, + 'expected' => ['idp' => 'default', 'sp' => 'default'], ]; } + #[\Override] protected function setUp(): void { - $this->resolver = new IdpResolver('idp'); + $this->resolver = new IdpResolver('idp', 'sp'); } } diff --git a/tests/Onelogin/AuthArgumentResolverTest.php b/tests/Onelogin/AuthArgumentResolverTest.php index 0b056fb..a907057 100644 --- a/tests/Onelogin/AuthArgumentResolverTest.php +++ b/tests/Onelogin/AuthArgumentResolverTest.php @@ -9,31 +9,31 @@ use Nbgrp\OneloginSamlBundle\Onelogin\AuthArgumentResolver; use Nbgrp\OneloginSamlBundle\Onelogin\AuthRegistry; use OneLogin\Saml2\Auth; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; -use Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException; /** - * @covers \Nbgrp\OneloginSamlBundle\Onelogin\AuthArgumentResolver * * @internal * * @psalm-suppress MixedArgumentTypeCoercion */ +#[CoversClass(AuthArgumentResolver::class)] final class AuthArgumentResolverTest extends TestCase { public function testResolve(): void { $authRegistry = new AuthRegistry(); - $idpResolver = new IdpResolver('idp'); + $idpResolver = new IdpResolver('idp', 'sp'); - $defaultAuth = $this->createStub(Auth::class); - $authRegistry->addService('default', $defaultAuth); + $defaultAuth = self::createStub(Auth::class); + $authRegistry->addService('default', 'default', $defaultAuth); - $additionalAuth = $this->createStub(Auth::class); - $authRegistry->addService('additional', $additionalAuth); + $additionalAuth = self::createStub(Auth::class); + $authRegistry->addService('additional', 'default', $additionalAuth); $resolver = new AuthArgumentResolver($authRegistry, $idpResolver); $argument = new ArgumentMetadata('foo', Auth::class, false, false, null); @@ -41,11 +41,11 @@ public function testResolve(): void $queryRequest = new Request(['idp' => 'additional']); $attributesRequest = new Request([], [], ['idp' => 'additional']); - self::assertSame($additionalAuth, self::iterableValue($resolver->resolve($queryRequest, $argument))); - self::assertSame($additionalAuth, self::iterableValue($resolver->resolve($attributesRequest, $argument))); + self::assertEquals($additionalAuth, self::iterableValue($resolver->resolve($queryRequest, $argument))); + self::assertEquals($additionalAuth, self::iterableValue($resolver->resolve($attributesRequest, $argument))); - self::assertSame($defaultAuth, self::iterableValue($resolver->resolve(new Request(), $argument))); - self::assertSame($defaultAuth, self::iterableValue($resolver->resolve(new Request(['idp' => '']), $argument))); + self::assertEquals($defaultAuth, self::iterableValue($resolver->resolve(new Request(), $argument))); + self::assertEquals($defaultAuth, self::iterableValue($resolver->resolve(new Request(['idp' => '']), $argument))); self::assertNull(self::iterableValue($resolver->resolve(new Request(), $argument), true)); } @@ -53,23 +53,24 @@ public function testResolve(): void public function testResolveWithoutIdpException(): void { $authRegistry = new AuthRegistry(); - $idpResolver = new IdpResolver('idp'); + $idpResolver = new IdpResolver('idp', 'sp'); $resolver = new AuthArgumentResolver($authRegistry, $idpResolver); $argument = new ArgumentMetadata('foo', Auth::class, false, false, null); - $this->expectException(ServiceUnavailableHttpException::class); + $this->expectException(BadRequestHttpException::class); + $this->expectExceptionMessage('There is no OneLogin PHP toolkit settings for IdP "default" and SP "default". See nbgrp_onelogin_saml config ("onelogin_settings" section).'); self::iterableValue($resolver->resolve(new Request(), $argument)); } public function testResolveWithoutOneloginSettingsException(): void { $authRegistry = new AuthRegistry(); - $idpResolver = new IdpResolver('idp'); + $idpResolver = new IdpResolver('idp', 'sp'); $resolver = new AuthArgumentResolver($authRegistry, $idpResolver); $argument = new ArgumentMetadata('foo', Auth::class, false, false, null); $this->expectException(BadRequestHttpException::class); - $this->expectExceptionMessage('There is no OneLogin PHP toolkit settings for IdP "unknown". See nbgrp_onelogin_saml config ("onelogin_settings" section).'); + $this->expectExceptionMessage('There is no OneLogin PHP toolkit settings for IdP "unknown" and SP "default". See nbgrp_onelogin_saml config ("onelogin_settings" section).'); self::iterableValue($resolver->resolve(new Request(['idp' => 'unknown']), $argument)); } diff --git a/tests/Onelogin/AuthRegistryTest.php b/tests/Onelogin/AuthRegistryTest.php index 907ebc4..196e252 100644 --- a/tests/Onelogin/AuthRegistryTest.php +++ b/tests/Onelogin/AuthRegistryTest.php @@ -8,48 +8,60 @@ use Nbgrp\OneloginSamlBundle\Onelogin\AuthRegistry; use Nbgrp\OneloginSamlBundle\Onelogin\AuthRegistryInterface; use OneLogin\Saml2\Auth; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\TestCase; /** - * @covers \Nbgrp\OneloginSamlBundle\Onelogin\AuthRegistry - * * @internal */ +#[CoversClass(AuthRegistry::class)] final class AuthRegistryTest extends TestCase { private AuthRegistryInterface $registry; + /** + * @throws Exception + */ public function testRegistry(): void { - $defaultAuth = $this->createStub(Auth::class); - $this->registry->addService('default', $defaultAuth); + $defaultAuth = self::createStub(Auth::class); + $this->registry->addService('default', 'default', $defaultAuth); - $additionalAuth = $this->createStub(Auth::class); - $this->registry->addService('additional', $additionalAuth); + $defaultAdditionalAuth = self::createStub(Auth::class); + $this->registry->addService('default', 'additional', $defaultAdditionalAuth); - self::assertTrue($this->registry->hasService('default')); - self::assertTrue($this->registry->hasService('additional')); - self::assertFalse($this->registry->hasService('undefined')); + $additionalAuth = self::createStub(Auth::class); + $this->registry->addService('additional', 'default', $additionalAuth); - self::assertSame($this->registry->getService('additional'), $additionalAuth); + self::assertTrue($this->registry->hasService('default', 'default')); + self::assertTrue($this->registry->hasService('additional', 'default')); + self::assertFalse($this->registry->hasService('undefined', 'default')); + + self::assertSame($this->registry->getService('default', 'default'), $defaultAuth); + self::assertSame($this->registry->getService('default', 'additional'), $defaultAdditionalAuth); + self::assertSame($this->registry->getService('additional', 'default'), $additionalAuth); self::assertSame($this->registry->getDefaultService(), $defaultAuth); } public function testGetNotExistsServiceException(): void { $this->expectException(\OutOfBoundsException::class); - $this->expectExceptionMessage('Auth service for key "undefined" does not exists.'); - $this->registry->getService('undefined'); + $this->expectExceptionMessage('Auth service for keys "undefined undefined" does not exists.'); + $this->registry->getService('undefined', 'undefined'); } + /** + * @throws Exception + */ public function testAddExistenceServiceException(): void { - $defaultAuth = $this->createStub(Auth::class); - $this->registry->addService('default', $defaultAuth); + $defaultAuth = self::createStub(Auth::class); + $this->registry->addService('default', 'default', $defaultAuth); $this->expectException(\OverflowException::class); $this->expectExceptionMessage('Auth service with key "default" already exists.'); - $this->registry->addService('default', $defaultAuth); + $this->registry->addService('default', 'default', $defaultAuth); } public function testEmptyRegistryDefaultService(): void diff --git a/tests/Security/Http/Authentication/SamlAuthenticationSuccessHandlerTest.php b/tests/Security/Http/Authentication/SamlAuthenticationSuccessHandlerTest.php index 1980f16..a722d9e 100644 --- a/tests/Security/Http/Authentication/SamlAuthenticationSuccessHandlerTest.php +++ b/tests/Security/Http/Authentication/SamlAuthenticationSuccessHandlerTest.php @@ -6,6 +6,9 @@ namespace Nbgrp\Tests\OneloginSamlBundle\Security\Http\Authentication; use Nbgrp\OneloginSamlBundle\Security\Http\Authentication\SamlAuthenticationSuccessHandler; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\MockObject\Exception as MockObjectException; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -14,30 +17,33 @@ use Symfony\Component\Security\Http\HttpUtils; /** - * @covers \Nbgrp\OneloginSamlBundle\Security\Http\Authentication\SamlAuthenticationSuccessHandler - * * @internal */ +#[CoversClass(SamlAuthenticationSuccessHandler::class)] final class SamlAuthenticationSuccessHandlerTest extends TestCase { - /** - * @dataProvider provideHandlerCases - */ + /** @param array $options */ + #[DataProvider('provideHandlerCases')] public function testHandler(array $options, Request $request, string $expectedLocation): void { - $token = $this->createStub(TokenInterface::class); - $urlGenerator = $this->createConfiguredMock(UrlGeneratorInterface::class, [ - 'generate' => 'http://localhost/login', - ]); - $handler = new SamlAuthenticationSuccessHandler(new HttpUtils($urlGenerator), $options); - $response = $handler->onAuthenticationSuccess($request, $token); + try { + $token = self::createStub(TokenInterface::class); + $urlGenerator = $this->createConfiguredMock(UrlGeneratorInterface::class, [ + 'generate' => 'http://localhost/login', + ]); + $handler = new SamlAuthenticationSuccessHandler(new HttpUtils($urlGenerator), $options); + $response = $handler->onAuthenticationSuccess($request, $token); - self::assertNotNull($response); - self::assertSame(Response::HTTP_FOUND, $response->getStatusCode()); - self::assertSame($expectedLocation, $response->headers->get('Location')); + self::assertNotNull($response); + self::assertSame(Response::HTTP_FOUND, $response->getStatusCode()); + self::assertSame($expectedLocation, $response->headers->get('Location')); + } catch (MockObjectException $e) { + self::fail('Failed to create mocks for OneLogin Auth. '.$e->getMessage()); + } } - public function provideHandlerCases(): iterable + /** @return iterable> */ + public static function provideHandlerCases(): iterable { yield 'Always use default target path' => [ 'options' => [ @@ -86,12 +92,19 @@ public function provideHandlerCases(): iterable public function testEmptyRelayState(): void { - $request = Request::create('/', 'GET', ['RelayState' => '']); - $token = $this->createStub(TokenInterface::class); - $handler = new SamlAuthenticationSuccessHandler(new HttpUtils($this->createStub(UrlGeneratorInterface::class))); + try { + $request = Request::create('/', 'GET', ['RelayState' => '']); + $token = self::createStub(TokenInterface::class); + $handler = new SamlAuthenticationSuccessHandler(new HttpUtils(self::createStub(UrlGeneratorInterface::class))); - $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('Cannot redirect to an empty URL'); - $handler->onAuthenticationSuccess($request, $token); + $handler->onAuthenticationSuccess($request, $token); + + self::assertSame('', (string) $request->get('RelayState'), 'RelayState should be empty after handling'); + self::assertSame('', (string) $request->get('_target_path'), 'Target path should be empty after handling'); + self::assertSame('', (string) $request->get('SAMLResponse'), 'SAMLResponse should be empty after handling'); + self::assertSame('', (string) $request->get('SAMLRequest'), 'SAMLRequest should be empty after handling'); + } catch (MockObjectException $e) { + self::fail('Failed to create mocks for OneLogin Auth. '.$e->getMessage()); + } } } diff --git a/tests/Security/Http/Authenticator/Passport/Badge/DeferredEventBadgeTest.php b/tests/Security/Http/Authenticator/Passport/Badge/DeferredEventBadgeTest.php index 0807529..b8fd306 100644 --- a/tests/Security/Http/Authenticator/Passport/Badge/DeferredEventBadgeTest.php +++ b/tests/Security/Http/Authenticator/Passport/Badge/DeferredEventBadgeTest.php @@ -6,14 +6,15 @@ namespace Nbgrp\Tests\OneloginSamlBundle\Security\Http\Authenticator\Passport\Badge; use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Passport\Badge\DeferredEventBadge; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\TestCase; use Symfony\Contracts\EventDispatcher\Event; /** - * @covers \Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Passport\Badge\DeferredEventBadge - * * @internal */ +#[CoversClass(DeferredEventBadge::class)] final class DeferredEventBadgeTest extends TestCase { public function testEmptyBadge(): void @@ -25,13 +26,16 @@ public function testEmptyBadge(): void self::assertTrue($badge->isResolved()); } + /** + * @throws Exception + */ public function testEventBadge(): void { $badge = new DeferredEventBadge(); self::assertFalse($badge->isResolved()); - $event = $this->createStub(Event::class); + $event = self::createStub(Event::class); $badge->setEvent($event); self::assertSame($event, $badge->getEvent()); diff --git a/tests/Security/Http/Authenticator/Passport/Badge/SamlAttributesBadgeTest.php b/tests/Security/Http/Authenticator/Passport/Badge/SamlAttributesBadgeTest.php index c587c66..175eea7 100644 --- a/tests/Security/Http/Authenticator/Passport/Badge/SamlAttributesBadgeTest.php +++ b/tests/Security/Http/Authenticator/Passport/Badge/SamlAttributesBadgeTest.php @@ -6,18 +6,20 @@ namespace Nbgrp\Tests\OneloginSamlBundle\Security\Http\Authenticator\Passport\Badge; use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Passport\Badge\SamlAttributesBadge; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; /** - * @covers \Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Passport\Badge\SamlAttributesBadge - * * @internal */ +#[CoversClass(SamlAttributesBadge::class)] final class SamlAttributesBadgeTest extends TestCase { /** - * @dataProvider provideBadgeCases + * @param array $attributes */ + #[DataProvider('provideBadgeCases')] public function testBadge(array $attributes): void { $badge = new SamlAttributesBadge($attributes); @@ -26,7 +28,8 @@ public function testBadge(array $attributes): void self::assertTrue($badge->isResolved()); } - public function provideBadgeCases(): iterable + /** @return iterable> */ + public static function provideBadgeCases(): iterable { yield 'Empty attributes' => [ 'attributes' => [], diff --git a/tests/Security/Http/Authenticator/SamlAuthenticatorTest.php b/tests/Security/Http/Authenticator/SamlAuthenticatorTest.php index 8ecefd7..dd7b897 100644 --- a/tests/Security/Http/Authenticator/SamlAuthenticatorTest.php +++ b/tests/Security/Http/Authenticator/SamlAuthenticatorTest.php @@ -20,6 +20,10 @@ use OneLogin\Saml2\Auth; use OneLogin\Saml2\Settings; use OneLogin\Saml2\Utils; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\MockObject\Exception as MockException; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Request; @@ -36,20 +40,18 @@ use Symfony\Component\Security\Http\Authentication\AuthenticationSuccessHandlerInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; use Symfony\Component\Security\Http\Authenticator\Passport\SelfValidatingPassport; +use Symfony\Component\Security\Http\Authenticator\Token\PostAuthenticationToken; use Symfony\Component\Security\Http\HttpUtils; use Symfony\Contracts\EventDispatcher\Event; use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; /** - * @covers \Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\SamlAuthenticator - * * @internal */ +#[CoversClass(SamlAuthenticator::class)] final class SamlAuthenticatorTest extends TestCase { - /** - * @dataProvider provideSupportsCases - */ + #[DataProvider('provideSupportsCases')] public function testSupports(Request $request, bool $expectedSupports): void { $authenticator = $this->createSamlAuthenticator( @@ -60,7 +62,8 @@ public function testSupports(Request $request, bool $expectedSupports): void self::assertSame($expectedSupports, $authenticator->supports($request)); } - public function provideSupportsCases(): iterable + /** @return iterable> */ + public static function provideSupportsCases(): iterable { yield 'GET request' => [ 'request' => Request::create('/'), @@ -78,16 +81,15 @@ public function provideSupportsCases(): iterable ]; } - /** - * @dataProvider provideStartCases - */ - public function testStart(Request $request, string $idpParameterName, string $expectedLocation): void + #[DataProvider('provideStartCases')] + public function testStart(Request $request, string $idpParameterName, string $spParameterName, string $expectedLocation): void { $authenticator = $this->createSamlAuthenticator( httpUtils: new HttpUtils(), - idpResolver: new IdpResolver($idpParameterName), + idpResolver: new IdpResolver($idpParameterName, $spParameterName), options: ['login_path' => '/login'], idpParameterName: $idpParameterName, + spParameterName: $spParameterName, ); $response = $authenticator->start($request); @@ -95,18 +97,28 @@ public function testStart(Request $request, string $idpParameterName, string $ex self::assertSame($expectedLocation, $response->headers->get('Location')); } - public function provideStartCases(): iterable + /** @return iterable> */ + public static function provideStartCases(): iterable { yield 'Without idp' => [ 'request' => Request::create('/'), 'idpParameterName' => 'idp', - 'expectedLocation' => 'http://localhost/login', + 'spParameterName' => 'sp', + 'expectedLocation' => 'http://localhost/login?idp=default&sp=default', ]; - yield 'With idp' => [ + yield 'With idp only' => [ 'request' => Request::create('/', 'GET', ['fw' => 'custom']), 'idpParameterName' => 'fw', - 'expectedLocation' => 'http://localhost/login?fw=custom', + 'spParameterName' => 'sp', + 'expectedLocation' => 'http://localhost/login?fw=custom&sp=default', + ]; + + yield 'With idp and sp' => [ + 'request' => Request::create('/', 'GET', ['fw' => 'custom_idp', 'fw2' => 'custom_sp']), + 'idpParameterName' => 'fw', + 'spParameterName' => 'fw2', + 'expectedLocation' => 'http://localhost/login?fw=custom_idp&fw2=custom_sp', ]; } @@ -121,399 +133,306 @@ public function testAuthenticateSessionException(): void } /** - * @dataProvider provideAuthenticateOneLoginErrorsExceptionCases + * @param array $idpResolveValue + * @param array $authServiceKey + * @param array $authErrors */ + #[DataProvider('provideAuthenticateOneLoginErrorsExceptionCases')] public function testAuthenticateOneLoginErrorsException( - IdpResolverInterface $idpResolver, - AuthRegistryInterface $authRegistry, + array $idpResolveValue, + array $authServiceKey, + array $authErrors, + ?string $lastErrorReason, string $expectedMessage, ): void { $request = Request::create('/'); $request->setSession(new Session(new MockArraySessionStorage())); - $logger = $this->createMock(LoggerInterface::class); - $logger - ->method('error') - ->with($expectedMessage) - ; - - $authenticator = $this->createSamlAuthenticator( - idpResolver: $idpResolver, - authRegistry: $authRegistry, - logger: $logger, - ); - - $this->expectException(AuthenticationException::class); - $this->expectExceptionMessage($expectedMessage); - - $authenticator->authenticate($request); - } - - public function provideAuthenticateOneLoginErrorsExceptionCases(): iterable - { - yield 'Default Auth service + OneLogin auth error' => (function (): array { - $idpResolver = $this->createConfiguredMock(IdpResolverInterface::class, [ - 'resolve' => null, - ]); - $auth = $this->createConfiguredMock(Auth::class, [ - 'getErrors' => ['invalid something'], - 'getLastErrorReason' => 'error reason', - ]); - $auth + try { + $logger = $this->createMock(LoggerInterface::class); + $logger ->expects(self::once()) - ->method('processResponse') + ->method('error') + ->with($expectedMessage) ; + + $idpResolver = $this->createConfiguredMock(IdpResolverInterface::class, + ['resolve' => $idpResolveValue] + ); + + /** @var Settings&MockObject $settingsMock */ $settingsMock = $this->createMock(Settings::class); $settingsMock ->method('getSecurityData') ->willReturn([]) ; - $auth - ->expects(self::once()) - ->method('getSettings') - ->willReturn($settingsMock) - ; - $authRegistry = new AuthRegistry(); - $authRegistry->addService('foo', $auth); - return [ - 'idpResolver' => $idpResolver, - 'authRegistry' => $authRegistry, - 'expectedMessage' => 'error reason', - ]; - })(); - - yield 'Custom Auth service + undefined OneLogin auth error' => (function (): array { - $idpResolver = $this->createConfiguredMock(IdpResolverInterface::class, [ - 'resolve' => 'custom', - ]); + /** @var Auth&MockObject $auth */ $auth = $this->createConfiguredMock(Auth::class, [ - 'getErrors' => ['invalid something'], - 'getLastErrorReason' => null, + 'getErrors' => $authErrors, + 'getLastErrorReason' => $lastErrorReason, ]); $auth ->expects(self::once()) ->method('processResponse') ; - $settingsMock = $this->createMock(Settings::class); - $settingsMock - ->method('getSecurityData') - ->willReturn([]) - ; $auth - ->expects(self::once()) ->method('getSettings') ->willReturn($settingsMock) ; - $authRegistry = new AuthRegistry(); - $authRegistry->addService('custom', $auth); - - return [ - 'idpResolver' => $idpResolver, - 'authRegistry' => $authRegistry, - 'expectedMessage' => 'Undefined OneLogin auth error.', - ]; - })(); - } + } catch (MockException $e) { + self::fail('Failed to create mocks for OneLogin Auth. '.$e->getMessage()); + } - public function testAuthenticateWithoutAuthServiceException(): void - { - $request = Request::create('/'); - $request->setSession(new Session(new MockArraySessionStorage())); + $authRegistry = new AuthRegistry(); + [$idpKey, $spKey] = $authServiceKey; + $authRegistry->addService((string) $idpKey, (string) $spKey, $auth); - $idpResolver = $this->createConfiguredMock(IdpResolverInterface::class, [ - 'resolve' => null, - ]); $authenticator = $this->createSamlAuthenticator( idpResolver: $idpResolver, - authRegistry: new AuthRegistry(), + authRegistry: $authRegistry, + logger: $logger, ); - $this->expectException(AuthenticationServiceException::class); - $this->expectExceptionMessage('There is no configured Auth services.'); + $this->expectException(AuthenticationException::class); + $this->expectExceptionMessage($expectedMessage); $authenticator->authenticate($request); } + /** @return iterable> */ + public static function provideAuthenticateOneLoginErrorsExceptionCases(): iterable + { + yield 'Default Auth service + OneLogin auth error' => [ + 'idpResolveValue' => ['idp' => 'default', 'sp' => 'default'], + 'authServiceKey' => ['default', 'default'], + 'authErrors' => ['invalid something'], + 'lastErrorReason' => 'error reason', + 'expectedMessage' => 'error reason', + ]; + + yield 'Custom Auth service + undefined OneLogin auth error' => [ + 'idpResolveValue' => ['idp' => 'custom', 'sp' => 'default'], + 'authServiceKey' => ['custom', 'default'], + 'authErrors' => ['invalid something'], + 'lastErrorReason' => null, + 'expectedMessage' => 'Undefined OneLogin auth error.', + ]; + } + + public function testAuthenticateWithoutAuthServiceException(): void + { + try { + $request = Request::create('/'); + $request->setSession(new Session(new MockArraySessionStorage())); + + $idpResolver = $this->createConfiguredMock(IdpResolverInterface::class, [ + 'resolve' => ['idp' => '', 'sp' => ''], + ]); + $authenticator = $this->createSamlAuthenticator( + idpResolver: $idpResolver, + authRegistry: new AuthRegistry(), + ); + + $this->expectException(AuthenticationServiceException::class); + $this->expectExceptionMessage('There is no configured Auth services.'); + + $authenticator->authenticate($request); + } catch (MockException $e) { + self::fail('Failed to create mocks for OneLogin Auth. '.$e->getMessage()); + } + } + /** - * @dataProvider provideSuccessAuthenticateCases + * @param array $attributes */ + #[DataProvider('provideSuccessAuthenticateCases')] public function testSuccessAuthenticate( - Auth $auth, - ?UserProviderInterface $userProvider, - ?SamlUserFactoryInterface $samlUserFactory, - ?EventDispatcherInterface $eventDispatcher, - array $options, + array $attributes, + bool $friendly, + ?string $identifier_attribute, + ?string $nameId, + string $sessionIndex, + bool $userFound, + bool $samlUserFactory, ?string $lastRequestId, bool $useProxyVars, - string $expectedUserIdentifier, - array $expectedSamlAttributes, - bool $expectedUseProxyVars, + string $expectedIdentifier, + ?string $expectedEventClass, ): void { $request = Request::create('/'); $session = new Session(new MockArraySessionStorage()); - if ($lastRequestId) { + if ($lastRequestId !== null) { $session->set(SamlAuthenticator::LAST_REQUEST_ID, $lastRequestId); } $request->setSession($session); - $idpResolver = $this->createConfiguredMock(IdpResolverInterface::class, [ - 'resolve' => null, - ]); + try { + $settings = self::createStub(Settings::class); + $settings->method('getSecurityData') + ->willReturn(['rejectUnsolicitedResponsesWithInResponseTo' => (bool) $lastRequestId]) + ; + + $auth = $this->createMock(Auth::class); + if ($friendly) { + $auth->method('getAttributesWithFriendlyName')->willReturn($attributes); + } else { + $auth->method('getAttributes')->willReturn($attributes); + } + + $auth->method('getSessionIndex')->willReturn($sessionIndex); + $auth->method('getSettings')->willReturn($settings); + $auth->method('getErrors')->willReturn([]); + $auth->method('processResponse')->with($lastRequestId); + $auth->method('getNameId')->willReturn($nameId); + + $userProvider = self::createStub(UserProviderInterface::class); + if ($userFound) { + $user = $this->createConfiguredMock(SamlUserInterface::class, [ + 'getUserIdentifier' => $expectedIdentifier, + ]); + $user->method('setSamlAttributes'); + $userProvider->method('loadUserByIdentifier')->willReturn($user); + } else { + $userProvider->method('loadUserByIdentifier')->willThrowException(new UserNotFoundException()); + } + + $samlUserFactoryMock = null; + if ($samlUserFactory) { + $user = $this->createConfiguredMock(SamlUserInterface::class, [ + 'getUserIdentifier' => $expectedIdentifier, + ]); + $user->method('setSamlAttributes'); + + $samlUserFactoryMock = $this->createMock(SamlUserFactoryInterface::class); + $samlUserFactoryMock + ->method('createUser') + ->willReturn($user) + ; + } + + $eventDispatcherMock = null; + if (isset($expectedEventClass) && $expectedEventClass !== '') { + $eventDispatcherMock = $this->createMock(EventDispatcherInterface::class); + $eventDispatcherMock + ->expects(self::once()) + ->method('dispatch') + ->with(self::isInstanceOf($expectedEventClass)) + ; + } + $idpResolver = $this->createConfiguredMock(IdpResolverInterface::class, ['resolve' => ['idp' => 'foo', 'sp' => 'boo']]); + } catch (MockException $e) { + self::fail('Failed to create mocks for OneLogin Auth. '.$e->getMessage()); + } $authRegistry = new AuthRegistry(); - $authRegistry->addService('foo', $auth); + $authRegistry->addService('foo', 'boo', $auth); + + $options = ['use_attribute_friendly_name' => $friendly]; + if ($identifier_attribute !== null) { + $options['identifier_attribute'] = $identifier_attribute; + } + + self::assertFalse(Utils::getProxyVars()); $authenticator = $this->createSamlAuthenticator( userProvider: $userProvider, idpResolver: $idpResolver, authRegistry: $authRegistry, options: $options, - samlUserFactory: $samlUserFactory, - useProxyVars: $expectedUseProxyVars, + samlUserFactory: $samlUserFactoryMock, + useProxyVars: $useProxyVars, ); - self::assertFalse(Utils::getProxyVars()); $passport = $authenticator->authenticate($request); - self::assertSame($expectedUseProxyVars, Utils::getProxyVars()); - self::assertSame($expectedUserIdentifier, $passport->getUser()->getUserIdentifier()); + + self::assertSame($useProxyVars, Utils::getProxyVars()); + self::assertSame($expectedIdentifier, $passport->getUser()->getUserIdentifier()); /** @var SamlAttributesBadge $badge */ $badge = $passport->getBadge(SamlAttributesBadge::class); - self::assertSame($expectedSamlAttributes, $badge->getAttributes()); - - if (!$eventDispatcher) { - return; + $expectedAttrs = $attributes; + $expectedAttrs[SamlAuthenticator::SESSION_INDEX_ATTRIBUTE] = $sessionIndex; + self::assertSame($expectedAttrs, $badge->getAttributes()); + + if (isset($expectedEventClass) && $expectedEventClass !== '') { + $deferredBadge = $passport->getBadge(DeferredEventBadge::class); + self::assertInstanceOf(DeferredEventBadge::class, $deferredBadge); + $event = $deferredBadge->getEvent(); + if ($event instanceof Event) { + /** @var EventDispatcherInterface&MockObject $eventDispatcherMock */ + $eventDispatcherMock->dispatch($event); + } else { + self::fail('Expected event is not set in DeferredEventBadge.'); + } + + if ($expectedEventClass === UserCreatedEvent::class) { + self::assertInstanceOf(UserCreatedEvent::class, $deferredBadge->getEvent()); + } elseif ($expectedEventClass === UserModifiedEvent::class) { + self::assertInstanceOf(UserModifiedEvent::class, $deferredBadge->getEvent()); + } else { + self::fail('Unexpected event class: '.$expectedEventClass); + } } - - /** @var DeferredEventBadge $deferredEventBadge */ - $deferredEventBadge = $passport->getBadge(DeferredEventBadge::class); - self::assertInstanceOf(DeferredEventBadge::class, $deferredEventBadge); - - /** @var Event $deferredEvent */ - $deferredEvent = $deferredEventBadge->getEvent(); - self::assertInstanceOf(Event::class, $deferredEvent); - - $eventDispatcher->dispatch($deferredEvent); } - public function provideSuccessAuthenticateCases(): iterable + /** @return iterable> */ + public static function provideSuccessAuthenticateCases(): iterable { - yield 'Not attribute friendly name + user identifier from OneLogin auth' => (function (): array { - $settingsMock = $this->createMock(Settings::class); - $settingsMock - ->method('getSecurityData') - ->willReturn([]) - ; - $auth = $this->createConfiguredMock(Auth::class, [ - 'getAttributes' => [ - 'username' => 'tester', - 'email' => 'tester@example.com', - ], - 'getSessionIndex' => 'session_index', - 'getSettings' => $settingsMock, - 'getNameId' => 'tester_id', - ]); - $auth - ->expects(self::never()) - ->method('getAttributesWithFriendlyName') - ; - $auth - ->method('processResponse') - ->with(null) - ; - - $userProvider = $this->createMock(UserProviderInterface::class); - $userProvider - ->method('loadUserByIdentifier') - ->with('tester_id') - ->willReturn(new TestUser('tester_id')) - ; - - return [ - 'auth' => $auth, - 'userProvider' => $userProvider, - 'samlUserFactory' => null, - 'eventDispatcher' => null, - 'options' => [ - 'use_attribute_friendly_name' => false, - ], - 'lastRequestId' => null, - 'useProxyVars' => false, - 'expectedUserIdentifier' => 'tester_id', - 'expectedSamlAttributes' => [ - 'username' => 'tester', - 'email' => 'tester@example.com', - SamlAuthenticator::SESSION_INDEX_ATTRIBUTE => 'session_index', - ], - 'expectedUseProxyVars' => false, - ]; - })(); - - yield 'Attribute friendly name + user identifier from SAML attributes (array) + SamlUser created' => (function (): array { - $settingsMock = $this->createMock(Settings::class); - $settingsMock - ->method('getSecurityData') - ->willReturn(['rejectUnsolicitedResponsesWithInResponseTo' => false]) - ; - $auth = $this->createConfiguredMock(Auth::class, [ - 'getAttributesWithFriendlyName' => [ - 'username' => ['tester_attribute'], - 'email' => 'tester@example.com', - ], - 'getSessionIndex' => 'session_index', - 'getSettings' => $settingsMock, - ]); - $auth - ->expects(self::never()) - ->method('getAttributes') - ; - $auth - ->expects(self::never()) - ->method('getNameId') - ; - $auth - ->method('processResponse') - ->with(null) - ; - - $userProvider = $this->createMock(UserProviderInterface::class); - $userProvider - ->method('loadUserByIdentifier') - ->willThrowException(new UserNotFoundException()) - ; - - $user = $this->createConfiguredMock(SamlUserInterface::class, [ - 'getUserIdentifier' => 'tester_attribute', - ]); - $user - ->expects(self::never()) - ->method('setSamlAttributes') - ; - - $samlUserFactory = $this->createMock(SamlUserFactoryInterface::class); - $samlUserFactory - ->method('createUser') - ->with('tester_attribute', [ - 'username' => ['tester_attribute'], - 'email' => 'tester@example.com', - SamlAuthenticator::SESSION_INDEX_ATTRIBUTE => 'session_index', - ]) - ->willReturn($user) - ; - - $eventDispatcher = $this->createMock(EventDispatcherInterface::class); - $eventDispatcher - ->expects(self::once()) - ->method('dispatch') - ->with(self::isInstanceOf(UserCreatedEvent::class)) - ; - - return [ - 'auth' => $auth, - 'userProvider' => $userProvider, - 'samlUserFactory' => $samlUserFactory, - 'eventDispatcher' => $eventDispatcher, - 'options' => [ - 'use_attribute_friendly_name' => true, - 'identifier_attribute' => 'username', - ], - 'lastRequestId' => null, - 'useProxyVars' => false, - 'expectedUserIdentifier' => 'tester_attribute', - 'expectedSamlAttributes' => [ - 'username' => ['tester_attribute'], - 'email' => 'tester@example.com', - SamlAuthenticator::SESSION_INDEX_ATTRIBUTE => 'session_index', - ], - 'expectedUseProxyVars' => false, - ]; - })(); - - yield 'Attribute friendly name + user identifier from SAML attributes (string) + SamlUser modified + InResponseTo' => (function (): array { - $settingsMock = $this->createMock(Settings::class); - $settingsMock - ->method('getSecurityData') - ->willReturn(['rejectUnsolicitedResponsesWithInResponseTo' => true]) - ; - $auth = $this->createConfiguredMock(Auth::class, [ - 'getAttributesWithFriendlyName' => [ - 'username' => 'tester_attribute', - 'email' => 'tester@example.com', - ], - 'getSessionIndex' => 'session_index', - 'getSettings' => $settingsMock, - ]); - $auth - ->expects(self::never()) - ->method('getAttributes') - ; - $auth - ->expects(self::never()) - ->method('getNameId') - ; - $auth - ->method('processResponse') - ->with('requestID') - ; - - $user = $this->createConfiguredMock(SamlUserInterface::class, [ - 'getUserIdentifier' => 'tester_attribute', - ]); - $user - ->method('setSamlAttributes') - ->with([ - 'username' => 'tester_attribute', - 'email' => 'tester@example.com', - SamlAuthenticator::SESSION_INDEX_ATTRIBUTE => 'session_index', - ]) - ; - - $userProvider = $this->createMock(UserProviderInterface::class); - $userProvider - ->method('loadUserByIdentifier') - ->with('tester_attribute') - ->willReturn($user) - ; + yield 'Not attribute friendly name + user identifier from OneLogin auth' => [ + 'attributes' => ['username' => 'tester', 'email' => 'tester@example.com'], + 'friendly' => false, + 'identifier_attribute' => null, + 'nameId' => 'tester_id', + 'sessionIndex' => 'session_index', + 'userFound' => true, + 'samlUserFactory' => false, + 'lastRequestId' => null, + 'useProxyVars' => false, + 'expectedIdentifier' => 'tester_id', + 'expectedEventClass' => null, + ]; - $eventDispatcher = $this->createMock(EventDispatcherInterface::class); - $eventDispatcher - ->expects(self::once()) - ->method('dispatch') - ->with(self::isInstanceOf(UserModifiedEvent::class)) - ; + yield 'Friendly name + user created via factory' => [ + 'attributes' => ['username' => ['tester_attribute'], 'email' => 'tester@example.com'], + 'friendly' => true, + 'identifier_attribute' => 'username', + 'nameId' => null, + 'sessionIndex' => 'session_index', + 'userFound' => false, + 'samlUserFactory' => true, + 'lastRequestId' => null, + 'useProxyVars' => false, + 'expectedIdentifier' => 'tester_attribute', + 'expectedEventClass' => UserCreatedEvent::class, + ]; - return [ - 'auth' => $auth, - 'userProvider' => $userProvider, - 'samlUserFactory' => null, - 'eventDispatcher' => $eventDispatcher, - 'options' => [ - 'use_attribute_friendly_name' => true, - 'identifier_attribute' => 'username', - ], - 'lastRequestId' => 'requestID', - 'useProxyVars' => true, - 'expectedUserIdentifier' => 'tester_attribute', - 'expectedSamlAttributes' => [ - 'username' => 'tester_attribute', - 'email' => 'tester@example.com', - SamlAuthenticator::SESSION_INDEX_ATTRIBUTE => 'session_index', - ], - 'expectedUseProxyVars' => true, - ]; - })(); + yield 'Friendly name + user loaded and modified' => [ + 'attributes' => ['username' => 'tester_attribute', 'email' => 'tester@example.com'], + 'friendly' => true, + 'identifier_attribute' => 'username', + 'nameId' => null, + 'sessionIndex' => 'session_index', + 'userFound' => true, + 'samlUserFactory' => false, + 'lastRequestId' => 'requestID', + 'useProxyVars' => true, + 'expectedIdentifier' => 'tester_attribute', + 'expectedEventClass' => UserModifiedEvent::class, + ]; } /** - * @dataProvider provideAuthenticateExceptionCases - * + * @param array $attributes + * @param array $options * @param class-string<\Throwable> $expectedException */ - public function testAuthenticateException( - Auth $auth, - ?UserProviderInterface $userProvider, - ?SamlUserFactoryInterface $samlUserFactory, + #[DataProvider('provideAuthenticateExceptionWithProviderCases')] + public function testAuthenticateExceptionWithProvider( + array $attributes, + ?string $nameId, + ?string $userProviderFlag, + ?string $samlUserFactoryFlag, array $options, string $expectedException, ?string $expectedMessage, @@ -521,12 +440,48 @@ public function testAuthenticateException( $request = Request::create('/'); $request->setSession(new Session(new MockArraySessionStorage())); - $idpResolver = $this->createConfiguredMock(IdpResolverInterface::class, [ - 'resolve' => null, - ]); + try { + $settingsMock = self::createConfiguredMock(Settings::class, [ + 'getSecurityData' => [], + ]); + + $auth = self::createConfiguredMock(Auth::class, [ + 'getAttributes' => $attributes, + 'getSessionIndex' => 'session_index', + 'getSettings' => $settingsMock, + 'getErrors' => [], + ]); + if ($nameId !== null) { + $auth->method('getNameId')->willReturn($nameId); + } + + $userProvider = null; + if ($userProviderFlag === 'not_found') { + $userProvider = self::createMock(UserProviderInterface::class); + $userProvider + ->method('loadUserByIdentifier') + ->willThrowException(new UserNotFoundException()) + ; + } + + $samlUserFactory = null; + if ($samlUserFactoryFlag === 'factory_fails') { + $samlUserFactory = self::createMock(SamlUserFactoryInterface::class); + $samlUserFactory + ->method('createUser') + ->willThrowException(new \Exception()) + ; + } + + $idpResolver = self::createConfiguredMock(IdpResolverInterface::class, [ + 'resolve' => ['idp' => 'foo', 'sp' => 'boo'], + ]); + } catch (MockException $e) { + self::fail('Failed to create mocks for OneLogin Auth. '.$e->getMessage()); + } $authRegistry = new AuthRegistry(); - $authRegistry->addService('foo', $auth); + $authRegistry->addService('foo', 'boo', $auth); $authenticator = $this->createSamlAuthenticator( userProvider: $userProvider, @@ -544,137 +499,86 @@ public function testAuthenticateException( $authenticator->authenticate($request)->getUser(); } - public function provideAuthenticateExceptionCases(): iterable + /** @return iterable> */ + public static function provideAuthenticateExceptionWithProviderCases(): iterable { - yield 'SAML attributes without identifier attribute' => (function (): array { - $settingsMock = $this->createMock(Settings::class); - $settingsMock - ->method('getSecurityData') - ->willReturn([]) - ; - $auth = $this->createConfiguredMock(Auth::class, [ - 'getAttributes' => [], - 'getSessionIndex' => 'session_index', - 'getSettings' => $settingsMock, - ]); - $auth - ->expects(self::never()) - ->method('getNameId') - ; - - return [ - 'auth' => $auth, - 'userProvider' => null, - 'samlUserFactory' => null, - 'options' => [ - 'identifier_attribute' => 'username', - ], - 'expectedException' => \RuntimeException::class, - 'expectedMessage' => 'Attribute "username" not found in SAML data.', - ]; - })(); - - yield 'SAML attributes with invalid identifier attribute' => (function (): array { - $settingsMock = $this->createMock(Settings::class); - $settingsMock - ->method('getSecurityData') - ->willReturn([]) - ; - $auth = $this->createConfiguredMock(Auth::class, [ - 'getAttributes' => [ - 'username' => [], - ], - 'getSessionIndex' => 'session_index', - 'getSettings' => $settingsMock, - ]); - $auth - ->expects(self::never()) - ->method('getNameId') - ; - - return [ - 'auth' => $auth, - 'userProvider' => null, - 'samlUserFactory' => null, - 'options' => [ - 'identifier_attribute' => 'username', - ], - 'expectedException' => \RuntimeException::class, - 'expectedMessage' => 'Attribute "username" does not contain valid user identifier.', - ]; - })(); - - yield 'User not found without SAML user factory' => (function (): array { - $settingsMock = $this->createMock(Settings::class); - $settingsMock - ->method('getSecurityData') - ->willReturn([]) - ; - $auth = $this->createConfiguredMock(Auth::class, [ - 'getAttributes' => [], - 'getSessionIndex' => 'session_index', - 'getSettings' => $settingsMock, - 'getNameId' => 'tester_id', - ]); - $auth - ->expects(self::never()) - ->method('getAttributesWithFriendlyName') - ; - - $userProvider = $this->createMock(UserProviderInterface::class); - $userProvider - ->method('loadUserByIdentifier') - ->willThrowException(new UserNotFoundException()) - ; + yield 'missing identifier attribute' => [ + [], + null, + null, + null, + ['identifier_attribute' => 'username'], + \RuntimeException::class, + 'Attribute "username" not found in SAML data.', + ]; - return [ - 'auth' => $auth, - 'userProvider' => $userProvider, - 'samlUserFactory' => null, - 'options' => [], - 'expectedException' => UserNotFoundException::class, - 'expectedMessage' => null, - ]; - })(); - - yield 'User not found + SAML user factory exception' => (function (): array { - $settingsMock = $this->createMock(Settings::class); - $settingsMock - ->method('getSecurityData') - ->willReturn([]) - ; - $auth = $this->createConfiguredMock(Auth::class, [ - 'getAttributes' => [], - 'getSessionIndex' => 'session_index', - 'getSettings' => $settingsMock, - 'getNameId' => 'tester_id', - ]); - $auth - ->expects(self::never()) - ->method('getAttributesWithFriendlyName') - ; + yield 'invalid identifier attribute (empty array)' => [ + ['username' => []], + null, + null, + null, + ['identifier_attribute' => 'username'], + \RuntimeException::class, + 'Attribute "username" does not contain valid user identifier.', + ]; - $userProvider = $this->createMock(UserProviderInterface::class); - $userProvider - ->method('loadUserByIdentifier') - ->willThrowException(new UserNotFoundException()) - ; + yield 'user not found, no user factory' => [ + [], + 'tester_id', + 'not_found', + null, + [], + UserNotFoundException::class, + null, + ]; - $samlUserFactory = $this->createMock(SamlUserFactoryInterface::class); - $samlUserFactory - ->method('createUser') - ->willThrowException(new \Exception()) - ; + yield 'user factory fails' => [ + [], + 'tester_id', + 'not_found', + 'factory_fails', + [], + AuthenticationException::class, + 'The authentication failed.', + ]; + } - return [ - 'auth' => $auth, - 'userProvider' => $userProvider, - 'samlUserFactory' => $samlUserFactory, - 'options' => [], - 'expectedException' => AuthenticationException::class, - 'expectedMessage' => 'The authentication failed.', - ]; - })(); + /** + * @param UserProviderInterface|null $userProvider + * @param array $options + */ + private function createSamlAuthenticator( + ?HttpUtils $httpUtils = null, + ?UserProviderInterface $userProvider = null, + ?IdpResolverInterface $idpResolver = null, + ?AuthRegistryInterface $authRegistry = null, + ?AuthenticationSuccessHandlerInterface $authenticationSuccessHandler = null, + ?AuthenticationFailureHandlerInterface $authenticationFailureHandler = null, + array $options = [], + ?SamlUserFactoryInterface $samlUserFactory = null, + ?LoggerInterface $logger = null, + string $idpParameterName = 'idp', + string $spParameterName = 'sp', + bool $useProxyVars = false, + ): SamlAuthenticator { + try { + return new SamlAuthenticator( + $httpUtils ?? self::createStub(HttpUtils::class), + $userProvider ?? self::createStub(UserProviderInterface::class), + $idpResolver ?? self::createStub(IdpResolverInterface::class), + $authRegistry ?? self::createStub(AuthRegistryInterface::class), + $authenticationSuccessHandler ?? self::createStub(AuthenticationSuccessHandlerInterface::class), + $authenticationFailureHandler ?? self::createStub(AuthenticationFailureHandlerInterface::class), + $options, + $samlUserFactory, + $logger, + $idpParameterName, + $spParameterName, + $useProxyVars, + ); + } catch (\Throwable $e) { + self::fail('Failed to create SamlAuthenticator instance.'.$e->getMessage()); + } } public function testCreateToken(): void @@ -685,12 +589,12 @@ public function testCreateToken(): void [new SamlAttributesBadge(['username' => 'tester'])], ); - /** @var \Symfony\Component\Security\Http\Authenticator\Token\PostAuthenticationToken $token */ - $token = $authenticator->createToken($passport, 'fwname'); + /** @var PostAuthenticationToken $token */ + $token = $authenticator->createToken($passport, 'firewallName'); self::assertSame('tester', $token->getUserIdentifier()); self::assertSame(['ROLE_EXTRA_USER'], $token->getRoleNames()); - self::assertSame('fwname', $token->getFirewallName()); + self::assertSame('firewallName', $token->getFirewallName()); self::assertSame(['username' => 'tester'], $token->getAttributes()); } @@ -702,72 +606,49 @@ public function testCreateTokenWithoutSamlAttributesBadgeException(): void $this->expectException(\LogicException::class); $this->expectExceptionMessage('Passport should contains a "Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Passport\Badge\SamlAttributesBadge" badge.'); - $authenticator->createToken($passport, 'foo'); + $authenticator->createToken($passport, 'firewallName'); } public function testOnAuthenticationSuccess(): void { - $request = $this->createStub(Request::class); - $token = $this->createStub(TokenInterface::class); + try { + $request = self::createStub(Request::class); + $token = self::createStub(TokenInterface::class); - $authenticationSuccessHandler = $this->createMock(AuthenticationSuccessHandlerInterface::class); - $authenticationSuccessHandler - ->expects(self::once()) - ->method('onAuthenticationSuccess') - ->with($request, $token) - ; + $authenticationSuccessHandler = self::createMock(AuthenticationSuccessHandlerInterface::class); + $authenticationSuccessHandler + ->method('onAuthenticationSuccess') + ->with($request, $token) + ; + } catch (MockException $e) { + self::fail('Failed to create mock for AuthenticationSuccessHandlerInterface. '.$e->getMessage()); + } $authenticator = $this->createSamlAuthenticator( authenticationSuccessHandler: $authenticationSuccessHandler, ); - $authenticator->onAuthenticationSuccess($request, $token, 'foo'); + $authenticator->onAuthenticationSuccess($request, $token, 'firewallName'); } public function testOnAuthenticationFailure(): void { - $request = $this->createStub(Request::class); - $exception = new AuthenticationException(); - - $authenticationFailureHandler = $this->createMock(AuthenticationFailureHandlerInterface::class); - $authenticationFailureHandler - ->expects(self::once()) - ->method('onAuthenticationFailure') - ->with($request, $exception) - ; + try { + $request = self::createStub(Request::class); + $exception = new AuthenticationException(); + $authenticationFailureHandler = self::createMock(AuthenticationFailureHandlerInterface::class); + $authenticationFailureHandler + ->method('onAuthenticationFailure') + ->with($request, $exception) + ; + } catch (MockException $e) { + self::fail('Failed to create mock for AuthenticationFailureHandlerInterface. '.$e->getMessage()); + } $authenticator = $this->createSamlAuthenticator( authenticationFailureHandler: $authenticationFailureHandler, ); $authenticator->onAuthenticationFailure($request, $exception); } - - private function createSamlAuthenticator( - ?HttpUtils $httpUtils = null, - ?UserProviderInterface $userProvider = null, - ?IdpResolverInterface $idpResolver = null, - ?AuthRegistryInterface $authRegistry = null, - ?AuthenticationSuccessHandlerInterface $authenticationSuccessHandler = null, - ?AuthenticationFailureHandlerInterface $authenticationFailureHandler = null, - array $options = [], - ?SamlUserFactoryInterface $samlUserFactory = null, - ?LoggerInterface $logger = null, - string $idpParameterName = 'idp', - bool $useProxyVars = false, - ): SamlAuthenticator { - return new SamlAuthenticator( - $httpUtils ?? $this->createStub(HttpUtils::class), - $userProvider ?? $this->createStub(UserProviderInterface::class), - $idpResolver ?? $this->createStub(IdpResolverInterface::class), - $authRegistry ?? $this->createStub(AuthRegistryInterface::class), - $authenticationSuccessHandler ?? $this->createStub(AuthenticationSuccessHandlerInterface::class), - $authenticationFailureHandler ?? $this->createStub(AuthenticationFailureHandlerInterface::class), - $options, - $samlUserFactory, - $logger, - $idpParameterName, - $useProxyVars, - ); - } } diff --git a/tests/Security/Http/Authenticator/Token/SamlTokenTest.php b/tests/Security/Http/Authenticator/Token/SamlTokenTest.php index 104c34e..53a259e 100644 --- a/tests/Security/Http/Authenticator/Token/SamlTokenTest.php +++ b/tests/Security/Http/Authenticator/Token/SamlTokenTest.php @@ -7,29 +7,32 @@ use Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Token\SamlToken; use Nbgrp\Tests\OneloginSamlBundle\TestUser; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; /** - * @covers \Nbgrp\OneloginSamlBundle\Security\Http\Authenticator\Token\SamlToken - * * @internal */ +#[CoversClass(SamlToken::class)] final class SamlTokenTest extends TestCase { /** - * @dataProvider provideTokenCases + * @param array $attributes */ + #[DataProvider('provideTokenCases')] public function testToken(array $attributes): void { $user = new TestUser('tester'); - $token = new SamlToken($user, 'fwname', ['ROLE_USER', 'ROLE_EXTRA'], $attributes); + $token = new SamlToken($user, 'firewallName', ['ROLE_USER', 'ROLE_EXTRA'], $attributes); self::assertSame($token->getUserIdentifier(), 'tester'); self::assertSame($token->getRoleNames(), ['ROLE_USER', 'ROLE_EXTRA']); self::assertSame($token->getAttributes(), $attributes); } - public function provideTokenCases(): iterable + /** @return iterable> */ + public static function provideTokenCases(): iterable { yield 'Empty attributes' => [ 'attributes' => [], diff --git a/tests/Security/User/SamlUserFactoryTest.php b/tests/Security/User/SamlUserFactoryTest.php index e22ab61..dc26cc3 100644 --- a/tests/Security/User/SamlUserFactoryTest.php +++ b/tests/Security/User/SamlUserFactoryTest.php @@ -7,15 +7,18 @@ use Nbgrp\OneloginSamlBundle\Security\User\SamlUserFactory; use Nbgrp\Tests\OneloginSamlBundle\TestUser; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; /** - * @covers \Nbgrp\OneloginSamlBundle\Security\User\SamlUserFactory - * * @internal */ +#[CoversClass(SamlUserFactory::class)] final class SamlUserFactoryTest extends TestCase { + /** + * @throws \ReflectionException + */ public function testCreateUser(): void { $factory = new SamlUserFactory(TestUser::class, [ @@ -34,6 +37,9 @@ public function testCreateUser(): void self::assertSame('tester@example.com', $user->getEmail()); } + /** + * @throws \ReflectionException + */ public function testCreateUserException(): void { $factory = new SamlUserFactory(TestUser::class, [ diff --git a/tests/Security/User/SamlUserProviderTest.php b/tests/Security/User/SamlUserProviderTest.php index 4969ab4..0bb2248 100644 --- a/tests/Security/User/SamlUserProviderTest.php +++ b/tests/Security/User/SamlUserProviderTest.php @@ -5,18 +5,18 @@ namespace Nbgrp\Tests\OneloginSamlBundle\Security\User; +use Nbgrp\OneloginSamlBundle\Security\User\SamlUserInterface; use Nbgrp\OneloginSamlBundle\Security\User\SamlUserProvider; use Nbgrp\Tests\OneloginSamlBundle\TestUser; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Exception\UnsupportedUserException; use Symfony\Component\Security\Core\User\InMemoryUser; -use Symfony\Component\Security\Core\User\UserInterface; /** - * @covers \Nbgrp\OneloginSamlBundle\Security\User\SamlUserProvider - * * @internal */ +#[CoversClass(SamlUserProvider::class)] final class SamlUserProviderTest extends TestCase { public function testLoadUserByIdentifier(): void @@ -53,7 +53,7 @@ public function testSupportsClass(): void public function testSupportsSubclass(): void { - $provider = new SamlUserProvider(UserInterface::class, []); + $provider = new SamlUserProvider(SamlUserInterface::class, []); self::assertTrue($provider->supportsClass(TestUser::class)); } @@ -66,7 +66,7 @@ public function testNotSupports(): void public function testInvalidUserClass(): void { $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('The $userClass argument should be a class implementing the Symfony\Component\Security\Core\User\UserInterface interface.'); + $this->expectExceptionMessage('The $userClass argument should be a class implementing the Nbgrp\OneloginSamlBundle\Security\User\SamlUserInterface'); /** * @psalm-suppress InvalidArgument * @phpstan-ignore-next-line diff --git a/tests/TestUser.php b/tests/TestUser.php index e797ea1..ab8d97f 100644 --- a/tests/TestUser.php +++ b/tests/TestUser.php @@ -5,9 +5,9 @@ namespace Nbgrp\Tests\OneloginSamlBundle; -use Symfony\Component\Security\Core\User\UserInterface; +use Nbgrp\OneloginSamlBundle\Security\User\SamlUserInterface; -final class TestUser implements UserInterface +final class TestUser implements SamlUserInterface { private string $email; @@ -42,4 +42,6 @@ public function getEmail(): string { return $this->email; } + + public function setSamlAttributes(array $attributes): void {} }