Rejected join improvements

classes/login/GmsClient.php

@@ -36,7 +36,6 @@ class GmsClient {
         $info = curl_getinfo($conn);
 
         if ($info['http_code'] === 200) {
-            error_log($response);
             $joinResult = json_decode($response);
             curl_close($conn);
             if (!array_key_exists('mergedId', $joinResult)) {

classes/login/LoginHandler.php

@@ -93,6 +93,10 @@ class LoginHandler {
             $joinableUsers = $userDao->findJoinableUsersByUserId($user->id);
         }
 
+        if (count($session->getRejectedJoins()) > 0) {
+            $joinableUsers = array_values(array_diff($joinableUsers, $session->getRejectedJoins()));
+        }
+
         if (count($joinableUsers) > 0) {
             // select first user
             $userToJoin = $userDao->findUserById($joinableUsers[0]);
@@ -113,14 +117,14 @@ class LoginHandler {
             throw new \RAP\BadRequestException("Unable to find user");
         }
 
-        $session->setJoinRejected(true);
-
-        if ($session->getUser()->id === null) {
-            return $this->redirectToTOUCheck();
+        if ($user->id === null) {
+            $session->addRejectedJoin($session->getUserToJoin()->id);
         } else {
-            $this->saveRejectedJoinIfPossible();
-            return $this->getAfterLoginRedirect();
+            $this->locator->getUserDAO()
+                    ->insertRejectedJoin($user->id, $session->getUserToJoin()->id);
         }
+
+        return $this->getAfterLoginRedirect();
     }
 
     private function showConfirmJoin(User $userToJoin): string {
@@ -139,12 +143,21 @@ class LoginHandler {
      * Stores the user data into the database after he/she accepted the Terms of Use.
      */
     public function register(): string {
-        $user = $this->locator->getSession()->getUser();
+
+        $session = $this->locator->getSession();
+        $user = $session->getUser();
 
         if ($user === null) {
             throw new BadRequestException("User data not retrieved.");
         } else {
+
             $this->locator->getUserHandler()->saveUser($user);
+
+            // save rejected joins stored in session
+            foreach ($session->getRejectedJoins() as $userId) {
+                $this->locator->getUserDAO()->insertRejectedJoin($user->id, $userId);
+            }
+
             return $this->getAfterLoginRedirect();
         }
     }
@@ -155,7 +168,7 @@ class LoginHandler {
         $user = $session->getUser();
         $userToJoin = $session->getUserToJoin();
 
-        $joinedUser = $this->locator->getUserHandler()->joinUsers($userToJoin, $user);
+        $joinedUser = $this->locator->getUserHandler()->joinUsers($user, $userToJoin);
         $session->setUser($joinedUser);
 
         if ($session->getAction() === 'join') {
@@ -173,10 +186,13 @@ class LoginHandler {
         }
 
         $session = $this->locator->getSession();
+        $user = $session->getUser();
 
-        $this->saveRejectedJoinIfPossible();
+        if ($user->id === null) {
+            return $this->redirectToTOUCheck($user);
+        }
 
-        $this->locator->getAuditLogger()->info("LOGIN," . $session->getLoginIdentityType() . "," . $session->getUser()->id);
+        $this->locator->getAuditLogger()->info("LOGIN," . $session->getLoginIdentityType() . "," . $user->id);
 
         if ($session->getOAuth2RequestData() !== null) {
             // Redirect to OAuth2 client callback URL
@@ -194,18 +210,4 @@ class LoginHandler {
         throw new \Exception("Unable to find a proper redirect");
     }
 
-    private function saveRejectedJoinIfPossible(): void {
-
-        $session = $this->locator->getSession();
-
-        if ($session->isJoinRejected() && $session->getUserToJoin() !== null) {
-            $id1 = $session->getUser()->id;
-            $id2 = $session->getUserToJoin()->id;
-            if ($id1 !== null && $id2 !== null) {
-                $this->locator->getUserDAO()->insertRejectedJoin($id1, $id2);
-                $session->setJoinRejected(false);
-            }
-        }
-    }
-
 }

classes/model/SessionData.php

@@ -39,7 +39,7 @@ class SessionData {
     private $action;
     private $loginIdentityType;
     private $autojoin = false;
-    private $joinRejected = false;
+    private $rejectedJoins = [];
 
     public function setUser(?User $user): void {
         $this->user = $user;
@@ -68,13 +68,13 @@ class SessionData {
         return $this->autojoin;
     }
 
-    public function setJoinRejected(bool $joinRejected): void {
-        $this->joinRejected = $joinRejected;
+    public function addRejectedJoin(string $userId): void {
+        array_push($this->rejectedJoins, $userId);
         $this->save();
     }
 
-    public function isJoinRejected(): bool {
-        return $this->joinRejected;
+    public function getRejectedJoins(): array {
+        return $this->rejectedJoins;
     }
 
     /**

tests/LoginFlowTest.php

@@ -174,27 +174,36 @@ final class LoginFlowTest extends TestCase {
 
         $this->sessionData->setAction('account');
 
-        $this->userDaoStub->method('findJoinableUsersByEmail')->willReturn(['1']);
-        $this->userDaoStub->method('findUserById')->willReturn($this->getFakeUser1());
+        $this->userDaoStub->method('findJoinableUsersByEmail')->willReturn(['1', '2']);
+        $this->userDaoStub->method('findUserById')->will(
+                $this->returnValueMap(array(
+                    array('1', $this->getFakeUser1()),
+                    array('2', $this->getFakeUser2()))));
 
-        // Login: one joinable user detected
+        // Login: two joinable users detected
         $redirect1 = $this->loginHandler->onIdentityDataReceived($this->getFakeIdentity3());
 
         $this->assertTrue($this->sessionData->isAutojoin());
         $this->assertEquals('http://rap-ia2/confirm-join', $redirect1);
 
-        // User rejects join, redirect to TOU check
+        // User rejects first join, new confirm join is displayed
         $redirect2 = $this->loginHandler->rejectJoin();
 
-        $this->assertTrue($this->sessionData->isJoinRejected());
-        $this->assertEquals('http://rap-ia2/tou-check', $redirect2);
+        $this->assertEquals(1, count($this->sessionData->getRejectedJoins()));
+        $this->assertEquals('http://rap-ia2/confirm-join', $redirect2);
+
+        // User rejects second join, redirect to TOU check
+        $redirect3 = $this->loginHandler->rejectJoin();
+
+        $this->assertEquals(2, count($this->sessionData->getRejectedJoins()));
+        $this->assertEquals('http://rap-ia2/tou-check', $redirect3);
 
         $this->userDaoStub->method('createUser')->willReturn('5');
-        $this->userDaoStub->expects($this->once())->method('insertRejectedJoin');
+        $this->userDaoStub->expects($this->exactly(2))->method('insertRejectedJoin');
 
         // User accepts TOU
-        $redirect3 = $this->loginHandler->register();
-        $this->assertEquals('http://rap-ia2/account', $redirect3);
+        $redirect4 = $this->loginHandler->register();
+        $this->assertEquals('http://rap-ia2/account', $redirect4);
     }
 
     public function testExplicitJoin(): void {