redundant firewallNames parameter in Controllers - primary source are…

… the keys in ResourceOwnerMapLocator
hwi · Dec 22, 2021 · f34c980 · f34c980
1 parent dc8832f
commit f34c980
Show file tree

Hide file tree

Showing 9 changed files with 45 additions and 41 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,7 @@
 Changelog
 =========
 ## 2.0.0-BETA2 (202x-xx-xx)
+* possible BC Break if final controller class was copied: Removed/replaced redundant argument `$firewallNames` from controllers. Use `$resourceOwnerMapLocator->getFirewallNames()` instead.
 * Changed config files from `*.xml` to `*.php` (services and routes). Xml routing configs `connect.xml`, `login.xml` and `redirect.xml` are steel present but deprecated. Please use `*.php` variants in your includes instead.
 
 ## 2.0.0-BETA1 (2021-12-10)

diff --git a/src/Controller/ConnectController.php b/src/Controller/ConnectController.php
@@ -67,12 +67,8 @@ final class ConnectController
     private bool $failedUseReferer;
     private string $failedAuthPath;
     private bool $enableConnectConfirmation;
-    private array $firewallNames;
     private string $registrationForm;
 
-    /**
-     * @param string[] $firewallNames
-     */
     public function __construct(
         OAuthUtils $oauthUtils,
         ResourceOwnerMapLocator $resourceOwnerMapLocator,
@@ -88,7 +84,6 @@ public function __construct(
         bool $failedUseReferer,
         string $failedAuthPath,
         bool $enableConnectConfirmation,
-        array $firewallNames,
         string $registrationForm,
         ?AccountConnectorInterface $accountConnector,
         ?RegistrationFormHandlerInterface $formHandler
@@ -100,7 +95,6 @@ public function __construct(
         $this->failedUseReferer = $failedUseReferer;
         $this->failedAuthPath = $failedAuthPath;
         $this->enableConnectConfirmation = $enableConnectConfirmation;
-        $this->firewallNames = $firewallNames;
         $this->registrationForm = $registrationForm;
         $this->dispatcher = $dispatcher;
         $this->accountConnector = $accountConnector;
@@ -291,12 +285,7 @@ public function connectServiceAction(Request $request, string $service): Respons
      */
     private function getResourceOwnerByName(string $name): ResourceOwnerInterface
     {
-        foreach ($this->firewallNames as $firewall) {
-            if (!$this->resourceOwnerMapLocator->has($firewall)) {
-                continue;
-            }
-
-            $ownerMap = $this->resourceOwnerMapLocator->get($firewall);
+        foreach ($this->resourceOwnerMapLocator->getResourceOwnerMaps() as $ownerMap) {
             if ($resourceOwner = $ownerMap->getResourceOwnerByName($name)) {
                 return $resourceOwner;
             }
@@ -346,8 +335,8 @@ private function getTargetPath(?SessionInterface $session): ?string
             return null;
         }
 
-        foreach ($this->firewallNames as $providerKey) {
-            $sessionKey = '_security.'.$providerKey.'.target_path';
+        foreach ($this->resourceOwnerMapLocator->getFirewallNames() as $firewallName) {
+            $sessionKey = '_security.'.$firewallName.'.target_path';
             if ($session->has($sessionKey)) {
                 return $session->get($sessionKey);
             }

diff --git a/src/Controller/RedirectToServiceController.php b/src/Controller/RedirectToServiceController.php
@@ -11,6 +11,7 @@
 
 namespace HWI\Bundle\OAuthBundle\Controller;
 
+use HWI\Bundle\OAuthBundle\Security\Http\ResourceOwnerMapLocator;
 use HWI\Bundle\OAuthBundle\Security\OAuthUtils;
 use HWI\Bundle\OAuthBundle\Util\DomainWhitelist;
 use RuntimeException;
@@ -29,22 +30,22 @@ final class RedirectToServiceController
 {
     private OAuthUtils $oauthUtils;
     private DomainWhitelist $domainWhitelist;
-    private array $firewallNames;
+    private ResourceOwnerMapLocator $resourceOwnerMapLocator;
     private ?string $targetPathParameter = null;
     private bool $failedUseReferer;
     private bool $useReferer;
 
     public function __construct(
         OAuthUtils $oauthUtils,
         DomainWhitelist $domainWhitelist,
-        array $firewallNames,
+        ResourceOwnerMapLocator $resourceOwnerMapLocator,
         ?string $targetPathParameter,
         bool $failedUseReferer,
         bool $useReferer
     ) {
         $this->oauthUtils = $oauthUtils;
         $this->domainWhitelist = $domainWhitelist;
-        $this->firewallNames = $firewallNames;
+        $this->resourceOwnerMapLocator = $resourceOwnerMapLocator;
         $this->targetPathParameter = $targetPathParameter;
         $this->failedUseReferer = $failedUseReferer;
         $this->useReferer = $useReferer;
@@ -76,9 +77,9 @@ private function storeReturnPath(Request $request, string $authorizationUrl): vo
 
         $param = $this->targetPathParameter;
 
-        foreach ($this->firewallNames as $providerKey) {
-            $sessionKey = '_security.'.$providerKey.'.target_path';
-            $sessionKeyFailure = '_security.'.$providerKey.'.failed_target_path';
+        foreach ($this->resourceOwnerMapLocator->getFirewallNames() as $firewallName) {
+            $sessionKey = '_security.'.$firewallName.'.target_path';
+            $sessionKeyFailure = '_security.'.$firewallName.'.failed_target_path';
 
             if (!empty($param) && $targetUrl = $request->get($param)) {
                 if (!$this->domainWhitelist->isValidTargetUrl($targetUrl)) {

diff --git a/src/DependencyInjection/CompilerPass/ResourceOwnerMapCompilerPass.php b/src/DependencyInjection/CompilerPass/ResourceOwnerMapCompilerPass.php
@@ -28,7 +28,7 @@ public function process(ContainerBuilder $container)
         $def = $container->getDefinition('hwi_oauth.resource_ownermap_locator');
 
         foreach ($container->getParameter('hwi_oauth.firewall_names') as $firewallId) {
-            $def->addMethodCall('add', [$firewallId, new Reference('hwi_oauth.resource_ownermap.'.$firewallId)]);
+            $def->addMethodCall('set', [$firewallId, new Reference('hwi_oauth.resource_ownermap.'.$firewallId)]);
         }
     }
 }
diff --git a/src/Resources/config/controller.php b/src/Resources/config/controller.php
@@ -36,7 +36,6 @@
         ->arg('$failedUseReferer', '%hwi_oauth.failed_use_referer%')
         ->arg('$failedAuthPath', '%hwi_oauth.failed_auth_path%')
         ->arg('$enableConnectConfirmation', '%hwi_oauth.connect.confirmation%')
-        ->arg('$firewallNames', '%hwi_oauth.firewall_names%')
         ->arg('$registrationForm', '%hwi_oauth.connect.registration_form%')
         ->arg('$accountConnector', service('hwi_oauth.account.connector')->nullOnInvalid())
         ->arg('$formHandler', service('hwi_oauth.registration.form.handler')->nullOnInvalid());
@@ -58,7 +57,7 @@
         ->args([
             service('hwi_oauth.security.oauth_utils'),
             service('hwi_oauth.util.domain_whitelist'),
-            '%hwi_oauth.firewall_names%',
+            service('hwi_oauth.resource_ownermap_locator'),
             '%hwi_oauth.target_path_parameter%',
             '%hwi_oauth.failed_use_referer%',
             '%hwi_oauth.use_referer%',

diff --git a/src/Security/Http/ResourceOwnerMapLocator.php b/src/Security/Http/ResourceOwnerMapLocator.php
@@ -17,22 +17,35 @@
 final class ResourceOwnerMapLocator
 {
     /**
-     * @var array
+     * @var ResourceOwnerMapInterface[]
      */
-    private $resourceOwnerMaps = [];
+    private array $resourceOwnerMaps = [];
 
-    public function add(string $firewallId, ResourceOwnerMapInterface $resourceOwnerMap): void
+    public function set(string $firewallName, ResourceOwnerMapInterface $resourceOwnerMap): void
     {
-        $this->resourceOwnerMaps[$firewallId] = $resourceOwnerMap;
+        $this->resourceOwnerMaps[$firewallName] = $resourceOwnerMap;
     }
 
-    public function has(string $firewallId): bool
+    public function has(string $firewallName): bool
     {
-        return isset($this->resourceOwnerMaps[$firewallId]);
+        return isset($this->resourceOwnerMaps[$firewallName]);
     }
 
-    public function get(string $firewallId): ResourceOwnerMapInterface
+    public function get(string $firewallName): ResourceOwnerMapInterface
     {
-        return $this->resourceOwnerMaps[$firewallId];
+        return $this->resourceOwnerMaps[$firewallName];
+    }
+
+    /**
+     * @return ResourceOwnerMapInterface[]
+     */
+    public function getResourceOwnerMaps(): array
+    {
+        return $this->resourceOwnerMaps;
+    }
+
+    public function getFirewallNames(): array
+    {
+        return array_keys($this->resourceOwnerMaps);
     }
 }
diff --git a/tests/Controller/AbstractConnectControllerTest.php b/tests/Controller/AbstractConnectControllerTest.php
@@ -131,8 +131,9 @@ protected function setUp(): void
         $this->resourceOwnerMap = $this->createMock(ResourceOwnerMapInterface::class);
         $this->resourceOwnerMap->expects($this->any())
             ->method('getResourceOwnerByName')
-            ->with('facebook')
-            ->willReturn($this->resourceOwner);
+            ->willReturnCallback(function ($owner) {
+                return 'facebook' === $owner ? $this->resourceOwner : null;
+            });
 
         $this->oAuthUtils = new OAuthUtils(
             $this->createMock(HttpUtils::class),
@@ -142,7 +143,7 @@ protected function setUp(): void
         );
 
         $this->resourceOwnerMapLocator = new ResourceOwnerMapLocator();
-        $this->resourceOwnerMapLocator->add('default', $this->resourceOwnerMap);
+        $this->resourceOwnerMapLocator->set('default', $this->resourceOwnerMap);
 
         $this->formFactory = $this->createMock(FormFactoryInterface::class);
 
@@ -171,8 +172,7 @@ protected function mockAuthorizationCheck(bool $granted = true): void
 
     protected function createConnectController(
         bool $connectEnabled = true,
-        bool $confirmConnect = true,
-        array $firewallNames = ['default']
+        bool $confirmConnect = true
     ): ConnectController {
         return new ConnectController(
             $this->oAuthUtils,
@@ -189,7 +189,6 @@ protected function createConnectController(
             true,
             'fake_route',
             $confirmConnect,
-            $firewallNames,
             RegistrationFormType::class,
             $connectEnabled ? $this->accountConnector : null,
             $connectEnabled ? $this->registrationFormHandler : null

diff --git a/tests/Controller/ConnectControllerConnectServiceActionTest.php b/tests/Controller/ConnectControllerConnectServiceActionTest.php
@@ -46,7 +46,7 @@ public function testUnknownResourceOwner(): void
 
         $this->mockAuthorizationCheck();
 
-        $controller = $this->createConnectController(true, true, []);
+        $controller = $this->createConnectController(true, true);
         $controller->connectServiceAction($this->request, 'unknown');
     }
 

diff --git a/tests/Controller/RedirectToServiceControllerTest.php b/tests/Controller/RedirectToServiceControllerTest.php
@@ -16,6 +16,7 @@
 use HWI\Bundle\OAuthBundle\Controller\RedirectToServiceController;
 use HWI\Bundle\OAuthBundle\OAuth\ResourceOwnerInterface;
 use HWI\Bundle\OAuthBundle\Security\Http\ResourceOwnerMapInterface;
+use HWI\Bundle\OAuthBundle\Security\Http\ResourceOwnerMapLocator;
 use HWI\Bundle\OAuthBundle\Security\OAuthUtils;
 use HWI\Bundle\OAuthBundle\Util\DomainWhitelist;
 use PHPUnit\Framework\MockObject\MockObject;
@@ -36,8 +37,6 @@ final class RedirectToServiceControllerTest extends TestCase
 
     private Request $request;
 
-    private array $firewallNames = ['default'];
-
     private string $targetPathParameter = 'target_path';
 
     protected function setUp(): void
@@ -135,6 +134,9 @@ private function createController(bool $failedUseReferer = false, bool $useRefer
             ->withAnyParameters()
             ->willReturn('https://domain.com/oauth/v2/auth');
 
+        $resourceOwnerMapLocator = new ResourceOwnerMapLocator();
+        $resourceOwnerMapLocator->set('default', $ownerMap);
+
         $utils = new OAuthUtils(
             $this->createMock(HttpUtils::class),
             $this->createMock(AuthorizationCheckerInterface::class),
@@ -146,7 +148,7 @@ private function createController(bool $failedUseReferer = false, bool $useRefer
         return new RedirectToServiceController(
             $utils,
             new DomainWhitelist(['domain.com']),
-            $this->firewallNames,
+            $resourceOwnerMapLocator,
             $this->targetPathParameter,
             $failedUseReferer,
             $useReferer