fix: make sure handlers parameters are correctly handled (#40)

ecphp · Feb 3, 2023 · 645b88d · 645b88d
1 parent 2e1532c
commit 645b88d
Show file tree

Hide file tree

Showing 8 changed files with 97 additions and 97 deletions.
diff --git a/spec/EcPhp/CasLib/CasSpec.php b/spec/EcPhp/CasLib/CasSpec.php
@@ -433,15 +433,6 @@ public function it_can_handle_proxy_callback_request()
             ->getStatusCode()
             ->shouldReturn(200);
 
-        $request = new ServerRequest(
-            Method::GET,
-            'http://local/proxycallback?pgtId=pgtId&pgtIou=false'
-        );
-
-        $this
-            ->shouldThrow(Exception::class)
-            ->during('handleProxyCallback', [$request]);
-
         $request = new ServerRequest(
             Method::GET,
             'http://local/proxycallback?pgtId=pgtId'
@@ -509,7 +500,7 @@ public function it_can_login()
         $this
             ->login($request, $parameters)
             ->getHeader('Location')
-            ->shouldReturn(['http://local/cas/login?foo=bar&service=http%3A%2F%2Flocal%2F%3Ffoo%3Dbar']);
+            ->shouldReturn(['http://local/cas/login?foo=bar&service=http%3A%2F%2Ffoo.bar%2F']);
 
         $parameters = [
             'custom' => 'foo',
@@ -518,7 +509,7 @@ public function it_can_login()
         $this
             ->login($request, $parameters)
             ->getHeader('Location')
-            ->shouldReturn(['http://local/cas/login?custom=foo&service=http%3A%2F%2Flocal%2F%3Fcustom%3Dfoo']);
+            ->shouldReturn(['http://local/cas/login?custom=foo&service=http%3A%2F%2Flocal%2F']);
 
         $request = new ServerRequest(Method::GET, 'http://local/', ['referer' => 'http://referer/']);
 
@@ -530,7 +521,7 @@ public function it_can_login()
         $this
             ->login($request, $parameters)
             ->getHeader('Location')
-            ->shouldReturn(['http://local/cas/login?foo=bar&service=http%3A%2F%2Flocal%2F%3Ffoo%3Dbar']);
+            ->shouldReturn(['http://local/cas/login?foo=bar&service=http%3A%2F%2Ffoo.bar%2F']);
 
         $parameters = [
             'custom' => 'foo',
@@ -539,7 +530,7 @@ public function it_can_login()
         $this
             ->login($request, $parameters)
             ->getHeader('Location')
-            ->shouldReturn(['http://local/cas/login?custom=foo&service=http%3A%2F%2Flocal%2F%3Fcustom%3Dfoo']);
+            ->shouldReturn(['http://local/cas/login?custom=foo&service=http%3A%2F%2Flocal%2F']);
 
         $parameters = [
             'custom' => 'foo',
@@ -549,7 +540,7 @@ public function it_can_login()
         $this
             ->login($request, $parameters)
             ->getHeader('Location')
-            ->shouldReturn(['http://local/cas/login?custom=foo&service=http%3A%2F%2Flocal%2F%3Fcustom%3Dfoo']);
+            ->shouldReturn(['http://local/cas/login?custom=foo']);
     }
 
     public function it_can_logout()
@@ -568,7 +559,7 @@ public function it_can_logout()
         $this
             ->logout($request)
             ->getHeader('Location')
-            ->shouldReturn(['http://local/cas/logout']);
+            ->shouldReturn(['http://local/cas/logout?service=http%3A%2F%2Flocal%2F']);
 
         $parameters = [
             'custom' => 'bar',
@@ -577,7 +568,7 @@ public function it_can_logout()
         $this
             ->logout($request, $parameters)
             ->getHeader('Location')
-            ->shouldReturn(['http://local/cas/logout?custom=bar']);
+            ->shouldReturn(['http://local/cas/logout?custom=bar&service=http%3A%2F%2Flocal%2F']);
 
         $parameters = [
             'custom' => 'bar',
@@ -598,7 +589,7 @@ public function it_can_logout()
         $this
             ->logout($request, $parameters)
             ->getHeader('Location')
-            ->shouldReturn(['http://local/cas/logout?custom=bar']);
+            ->shouldReturn(['http://local/cas/logout?custom=bar&service=http%3A%2F%2Flocal%2F']);
 
         $parameters = [
             'custom' => 'bar',
@@ -916,10 +907,6 @@ public function let(CacheItemPoolInterface $cache, CacheItemInterface $cacheItem
             ->get()
             ->willReturn('pgtIou');
 
-        $cache
-            ->getItem('false')
-            ->willThrow(Exception::class);
-
         $cache
             ->hasItem('unknownPgtIou')
             ->willReturn(false);

diff --git a/spec/EcPhp/CasLib/Handler/LoginSpec.php b/spec/EcPhp/CasLib/Handler/LoginSpec.php
@@ -17,7 +17,7 @@
 use Exception;
 use loophp\psr17\Psr17;
 use Nyholm\Psr7\Factory\Psr17Factory;
-use Nyholm\Psr7\ServerRequest;
+use Nyholm\Psr7\Request;
 use Nyholm\Psr7\Uri;
 use PhpSpec\ObjectBehavior;
 use Psr\Cache\CacheItemPoolInterface;
@@ -45,7 +45,7 @@ public function it_can_deal_with_array_parameters(CacheItemPoolInterface $cache,
             $psr17
         );
 
-        $request = new ServerRequest(
+        $request = new Request(
             Method::GET,
             new Uri('http://from/it_can_deal_with_array_parameters')
         );
@@ -57,7 +57,7 @@ public function it_can_deal_with_array_parameters(CacheItemPoolInterface $cache,
         $this
             ->handle($request)
             ->getHeaderLine('Location')
-            ->shouldReturn('http://local/cas/login?custom%5B0%5D=1&custom%5B1%5D=2&custom%5B2%5D=3&custom%5B3%5D=4&custom%5B4%5D=5&service=http%3A%2F%2Ffrom%2Fit_can_deal_with_array_parameters%3Fcustom%255B0%255D%3D1%26custom%255B1%255D%3D2%26custom%255B2%255D%3D3%26custom%255B3%255D%3D4%26custom%255B4%255D%3D5');
+            ->shouldReturn('http://local/cas/login?custom%5B0%5D=1&custom%5B1%5D=2&custom%5B2%5D=3&custom%5B3%5D=4&custom%5B4%5D=5&service=http%3A%2F%2Ffrom%2Fit_can_deal_with_array_parameters');
     }
 
     public function it_can_deal_with_renew_and_gateway_parameters(CacheItemPoolInterface $cache, CasResponseBuilderInterface $casResponseBuilder)
@@ -80,7 +80,7 @@ public function it_can_deal_with_renew_and_gateway_parameters(CacheItemPoolInter
             $psr17
         );
 
-        $request = new ServerRequest(
+        $request = new Request(
             Method::GET,
             new Uri('http://from/it_can_deal_with_renew_and_gateway_parameters')
         );
@@ -109,7 +109,7 @@ public function it_can_deal_with_renew_parameter(CacheItemPoolInterface $cache,
             $psr17
         );
 
-        $request = new ServerRequest(
+        $request = new Request(
             Method::GET,
             new Uri('http://from/it_can_deal_with_renew_parameter')
         );
@@ -121,7 +121,7 @@ public function it_can_deal_with_renew_parameter(CacheItemPoolInterface $cache,
 
     public function it_can_get_a_response()
     {
-        $request = new ServerRequest(
+        $request = new Request(
             Method::GET,
             new Uri('http://from/it_can_deal_with_renew_parameter')
         );

diff --git a/src/Handler/Handler.php b/src/Handler/Handler.php
@@ -55,6 +55,29 @@ public function __construct(
         $this->psr17 = $psr17;
     }
 
+    /**
+     * This function will aggregate all the input arrays into a single array.
+     *
+     * The rule of concatenation is that the previous array will have precedence
+     * over the current array.
+     *
+     * Therefore: buildParameters([a=>1], [a=>2,b=>3]) will return [a=>1, b=>3]
+     *
+     * @param array<array-key, mixed> ...$parameters
+     *
+     * @return array<array-key, mixed>
+     */
+    protected function buildParameters(array ...$parameters): array
+    {
+        return $this->formatParameters(
+            array_reduce(
+                $parameters,
+                static fn (array $carry, array $item): array => $carry + $item,
+                []
+            )
+        );
+    }
+
     protected function buildUri(
         UriInterface $from,
         string $type,
@@ -83,32 +106,6 @@ protected function buildUri(
             ->withFragment($from->getFragment());
     }
 
-    /**
-     * @param array[]|bool[]|string[] $parameters
-     *
-     * @return string[]
-     */
-    protected function formatProtocolParameters(array $parameters): array
-    {
-        $parameters = array_map(
-            static fn ($parameter) => true === $parameter ? 'true' : $parameter,
-            array_filter($parameters)
-        );
-
-        if (true === array_key_exists('service', $parameters)) {
-            $parameters['service'] = (string) Uri::removeParams(
-                $this
-                    ->getPsr17()
-                    ->createUri($parameters['service']),
-                'ticket'
-            );
-        }
-
-        ksort($parameters);
-
-        return $parameters;
-    }
-
     protected function getCache(): CacheItemPoolInterface
     {
         return $this->cache;
@@ -138,4 +135,30 @@ protected function getPsr17(): Psr17Interface
     {
         return $this->psr17;
     }
+
+    /**
+     * @param array[]|bool[]|string[] $parameters
+     *
+     * @return string[]
+     */
+    private function formatParameters(array $parameters): array
+    {
+        $parameters = array_map(
+            static fn ($parameter) => true === $parameter ? 'true' : $parameter,
+            array_filter($parameters)
+        );
+
+        if (true === array_key_exists('service', $parameters)) {
+            $parameters['service'] = (string) Uri::removeParams(
+                $this
+                    ->getPsr17()
+                    ->createUri($parameters['service']),
+                'ticket'
+            );
+        }
+
+        ksort($parameters);
+
+        return $parameters;
+    }
 }
diff --git a/src/Handler/Login.php b/src/Handler/Login.php
@@ -24,26 +24,12 @@ final class Login extends Handler implements HandlerInterface
 {
     public function handle(RequestInterface $request): ResponseInterface
     {
-        $parameters = $this->getParameters();
-        $parameters += Uri::getParams($request->getUri());
-
-        // Add the query parameter to the service.
-        // Investigate if this is really needed.
-        // We do it here before adding the default parameters.
-        $parameters['service'] = (string) Uri::withParams(
-            $request->getUri(),
-            array_diff_key(
-                $parameters,
-                [
-                    'service' => null,
-                    'renew' => null,
-                ]
-            )
+        $parameters = $this->buildParameters(
+            $this->getParameters(),
+            $this->getProperties()['protocol'][HandlerInterface::TYPE_LOGIN]['default_parameters'] ?? [],
+            ['service' => (string) $request->getUri()],
         );
 
-        $parameters += $this->getProperties()['protocol'][HandlerInterface::TYPE_LOGIN]['default_parameters'] ?? [];
-        $parameters = $this->formatProtocolParameters($parameters);
-
         $this->validate($request, $parameters);
 
         return $this

diff --git a/src/Handler/Logout.php b/src/Handler/Logout.php
@@ -12,17 +12,18 @@
 namespace EcPhp\CasLib\Handler;
 
 use EcPhp\CasLib\Contract\Handler\HandlerInterface;
-use EcPhp\CasLib\Utils\Uri;
 use Psr\Http\Message\RequestInterface;
 use Psr\Http\Message\ResponseInterface;
 
 final class Logout extends Handler implements HandlerInterface
 {
     public function handle(RequestInterface $request): ResponseInterface
     {
-        $parameters = $this->getParameters();
-        $parameters += Uri::getParams($request->getUri());
-        $parameters += $this->getProperties()['protocol'][HandlerInterface::TYPE_LOGOUT]['default_parameters'] ?? [];
+        $parameters = $this->buildParameters(
+            $this->getParameters(),
+            $this->getProperties()['protocol'][HandlerInterface::TYPE_LOGOUT]['default_parameters'] ?? [],
+            ['service' => (string) $request->getUri()],
+        );
 
         return $this
             ->getPsr17()
@@ -33,7 +34,7 @@ public function handle(RequestInterface $request): ResponseInterface
                     ->buildUri(
                         $request->getUri(),
                         HandlerInterface::TYPE_LOGOUT,
-                        $this->formatProtocolParameters($parameters)
+                        $parameters
                     )
             );
     }

diff --git a/src/Handler/Proxy.php b/src/Handler/Proxy.php
@@ -13,7 +13,6 @@
 
 use EcPhp\CasLib\Contract\Handler\HandlerInterface;
 use EcPhp\CasLib\Exception\CasHandlerException;
-use EcPhp\CasLib\Utils\Uri;
 use Ergebnis\Http\Method;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
@@ -23,12 +22,11 @@ final class Proxy extends Handler implements HandlerInterface
 {
     public function handle(ServerRequestInterface $request): ResponseInterface
     {
-        $properties = $this->getProperties();
-
-        $parameters = $this->getParameters();
-        $parameters += Uri::getParams($request->getUri());
-        $parameters += $properties['protocol'][HandlerInterface::TYPE_PROXY]['default_parameters'] ?? [];
-        $parameters += ['service' => (string) $request->getUri()];
+        $parameters = $this->buildParameters(
+            $this->getParameters(),
+            $this->getProperties()['protocol'][HandlerInterface::TYPE_PROXY]['default_parameters'] ?? [],
+            ['service' => (string) $request->getUri()],
+        );
 
         $request = $this
             ->getPsr17()
@@ -38,7 +36,7 @@ public function handle(ServerRequestInterface $request): ResponseInterface
                     ->buildUri(
                         $request->getUri(),
                         HandlerInterface::TYPE_PROXY,
-                        $this->formatProtocolParameters($parameters)
+                        $parameters
                     )
             );
 

diff --git a/src/Handler/ProxyCallback.php b/src/Handler/ProxyCallback.php
@@ -19,31 +19,36 @@
 use Psr\Http\Message\ServerRequestInterface;
 use Throwable;
 
+use function array_key_exists;
+
 final class ProxyCallback extends Handler implements HandlerInterface
 {
     public function handle(ServerRequestInterface $request): ResponseInterface
     {
+        $parameters = $this->buildParameters(
+            $this->getParameters(),
+            Uri::getParams($request->getUri()),
+        );
+
         $response = $this
             ->getPsr17()
             ->createResponse();
 
-        $parameters = $this->getParameters();
-        $parameters += Uri::getParams($request->getUri());
-        $parameters += (array) $request->getBody();
-        $parameters += ['pgtId' => null, 'pgtIou' => null];
+        $hasPgtId = array_key_exists('pgtId', $parameters);
+        $hasPgtIou = array_key_exists('pgtIou', $parameters);
 
-        if (null === $parameters['pgtId'] && null === $parameters['pgtIou']) {
+        if (false === $hasPgtId && false === $hasPgtIou) {
             // We cannot return an exception here because prior sending the
             // PGT ID and PGTIOU, a request is made by the CAS server in order
             // to check the existence of the proxy callback endpoint.
             return $response;
         }
 
-        if (null === $parameters['pgtIou']) {
+        if (false === $hasPgtIou) {
             throw CasHandlerException::pgtIouIsNull();
         }
 
-        if (null === $parameters['pgtId']) {
+        if (false === $hasPgtId) {
             throw CasHandlerException::pgtIdIsNull();
         }