LoginController Tests

simplesamlphp · Dec 2, 2024 · 38b43e5 · 38b43e5
1 parent 1072701
commit 38b43e5
Show file tree

Hide file tree

Showing 7 changed files with 353 additions and 550 deletions.
diff --git a/phpunit.xml b/phpunit.xml
@@ -4,6 +4,7 @@
          bootstrap="tests/bootstrap.php"
          xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/10.5/phpunit.xsd"
          backupGlobals="false"
+         displayDetailsOnTestsThatTriggerWarnings="true"
          cacheDirectory=".phpunit.cache">
   <coverage>
     <report>

diff --git a/psalm.xml b/psalm.xml
@@ -13,7 +13,6 @@
 >
     <projectFiles>
         <directory name="config" />
-        <directory name="public" />
         <directory name="src" />
     </projectFiles>
 

diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php
@@ -147,6 +147,11 @@ public function login(
         $serviceUrl = $service ?? $TARGET ?? null;
         $redirect = !(isset($method) && $method === 'POST');
 
+        // Set initial configurations, or fail
+        $this->handleServiceConfiguration($serviceUrl);
+        $this->handleScope($scope);
+        $this->handleLanguage($language);
+
         // Get the ticket from the session
         $session = $this->getSession();
         $sessionTicket = $this->ticketStore->getTicket($session->getSessionId());
@@ -155,10 +160,6 @@ public function login(
         // if this parameter is true, single sign-on will be bypassed and authentication will be enforced
         $requestForceAuthenticate = $forceAuthn && $sessionRenewId !== $requestRenewId;
 
-        $this->handleServiceConfiguration($serviceUrl);
-        $this->handleScope($scope);
-        $this->handleLanguage($language);
-
         if ($request->query->has(ProcessingChain::AUTHPARAM)) {
             $this->authProcId = $request->query->get(ProcessingChain::AUTHPARAM);
         }
@@ -193,12 +194,14 @@ public function login(
              *  REDIRECT TO AUTHSOURCE LOGIN
              * */
             $this->authSource->login($params);
+            // We should never get here.This is to facilitate testing.
+            return null;
         }
 
         // We are Authenticated.
 
         $sessionExpiry = $this->authSource->getAuthData('Expire');
-        // Create a new ticket if we do not have one alreday or if we are in a forced Authentitcation mode
+        // Create a new ticket if we do not have one alreday, or if we are in a forced Authentitcation mode
         if (!\is_array($sessionTicket) || $forceAuthn) {
             $sessionTicket = $this->ticketFactory->createSessionTicket($session->getSessionId(), $sessionExpiry);
             $this->ticketStore->addTicket($sessionTicket);
@@ -213,6 +216,8 @@ public function login(
                 $this->postAuthUrlParameters,
             );
             $this->httpUtils->redirectTrustedURL($urlParameters);
+            // We should never get here.This is to facilitate testing.
+            return null;
         }
 
         // Get the state.
@@ -246,9 +251,12 @@ public function login(
             $this->httpUtils->redirectTrustedURL(
                 $this->httpUtils->addURLParameters($serviceUrl, $this->postAuthUrlParameters),
             );
+            // We should never get here.This is to facilitate testing.
+            return null;
         }
         // POST
         $this->httpUtils->submitPOSTData($serviceUrl, $this->postAuthUrlParameters);
+        // We should never get here.This is to facilitate testing.
         return null;
     }
 
@@ -338,7 +346,7 @@ public function getReturnUrl(Request $request, ?array $sessionTicket): string
      * @param   string|null  $serviceUrl
      *
      * @return void
-     * @throws \Exception
+     * @throws \RuntimeException
      */
     public function handleServiceConfiguration(?string $serviceUrl): void
     {
@@ -351,7 +359,7 @@ public function handleServiceConfiguration(?string $serviceUrl): void
                 var_export($serviceUrl, true);
             Logger::debug('casserver:' . $message);
 
-            throw new \Exception($message);
+            throw new \RuntimeException($message);
         }
 
         // Override the cas configuration to use for this service
@@ -377,7 +385,7 @@ public function handleLanguage(?string $language): void
      * @param   string|null  $scope
      *
      * @return void
-     * @throws \Exception
+     * @throws \RuntimeException
      */
     public function handleScope(?string $scope): void
     {
@@ -392,10 +400,10 @@ public function handleScope(?string $scope): void
         // Fail
         if (!isset($scopes[$scope])) {
             $message = 'Scope parameter provided to CAS server is not listed as legal scope: [scope] = ' .
-                var_export($_GET['scope'], true);
+                var_export($scope, true);
             Logger::debug('casserver:' . $message);
 
-            throw new \Exception($message);
+            throw new \RuntimeException($message);
         }
 
         // Set the idplist from the scopes
@@ -408,8 +416,16 @@ public function handleScope(?string $scope): void
      * @return Session|null
      * @throws \Exception
      */
-    protected function getSession(): ?Session
+    public function getSession(): ?Session
     {
         return Session::getSessionFromRequest();
     }
+
+    /**
+     * @return mixed
+     */
+    public function getTicketStore(): mixed
+    {
+        return $this->ticketStore;
+    }
 }
diff --git a/src/Controller/Traits/UrlTrait.php b/src/Controller/Traits/UrlTrait.php
@@ -50,7 +50,7 @@ public function sanitize(string $parameter): string
     public function parseQueryParameters(Request $request, ?array $sessionTicket): array
     {
         $forceAuthn = $this->getRequestParam($request, 'renew');
-        $sessionRenewId = $sessionTicket ? $sessionTicket['renewId'] : null;
+        $sessionRenewId = !empty($sessionTicket['renewId']) ? $sessionTicket['renewId'] : null;
 
         $queryParameters = $request->query->all();
         $requestParameters = $request->request->all();