Merge pull request #7616 from appwrite/refactor-disallow-new-session-with-existing

Disallow creating a session if one already exists

app/config/errors.php

@@ -178,7 +178,7 @@ return [
     ],
     Exception::USER_SESSION_ALREADY_EXISTS => [
         'name' => Exception::USER_SESSION_ALREADY_EXISTS,
-        'description' => 'Creation of anonymous users is prohibited when a session is active.',
+        'description' => 'Creation of a session is prohibited when a session is active.',
         'code' => 401,
     ],
     Exception::USER_NOT_FOUND => [

app/controllers/api/account.php

@@ -228,7 +228,6 @@ App::post('/v1/account/sessions/email')
     ->inject('queueForEvents')
     ->inject('hooks')
     ->action(function (string $email, string $password, Request $request, Response $response, Document $user, Database $dbForProject, Document $project, Locale $locale, Reader $geodb, Event $queueForEvents, Hooks $hooks) {
-
         $email = \strtolower($email);
         $protocol = $request->getProtocol();
 
@@ -553,7 +552,6 @@ App::get('/v1/account/sessions/oauth2/:provider/redirect')
     ->inject('geodb')
     ->inject('queueForEvents')
     ->action(function (string $provider, string $code, string $state, string $error, string $error_description, Request $request, Response $response, Document $project, Document $user, Database $dbForProject, Reader $geodb, Event $queueForEvents) use ($oauthDefaultSuccess) {
-
         $protocol = $request->getProtocol();
         $callback = $protocol . '://' . $request->getHostname() . '/v1/account/sessions/oauth2/callback/' . $provider . '/' . $project->getId();
         $defaultState = ['success' => $project->getAttribute('url', ''), 'failure' => ''];
@@ -675,6 +673,8 @@ App::get('/v1/account/sessions/oauth2/:provider/redirect')
             if (!empty($userWithMatchingEmail)) {
                 throw new Exception(Exception::USER_ALREADY_EXISTS);
             }
+
+            $sessionUpgrade = true;
         }
 
         $sessions = $user->getAttribute('sessions', []);
@@ -704,7 +704,7 @@ App::get('/v1/account/sessions/oauth2/:provider/redirect')
             }
 
             /**
-             * Is verified is not used yet, since we don't know after an accout is created anymore if it was verified or not.
+             * Is verified is not used yet, since we don't know after an account is created anymore if it was verified or not.
              */
             $isVerified = $oauth2->isEmailVerified($accessToken);
 
@@ -947,6 +947,20 @@ App::get('/v1/account/sessions/oauth2/:provider/redirect')
                 ->addCookie(Auth::$cookieName, Auth::encodeSession($user->getId(), $secret), (new \DateTime($expire))->getTimestamp(), '/', Config::getParam('cookieDomain'), ('https' == $protocol), true, Config::getParam('cookieSamesite'));
         }
 
+        if (isset($sessionUpgrade) && $sessionUpgrade) {
+            foreach ($user->getAttribute('targets', []) as $target) {
+                if ($target->getAttribute('providerType') !== MESSAGE_TYPE_PUSH) {
+                    continue;
+                }
+
+                $target
+                    ->setAttribute('sessionId', $session->getId())
+                    ->setAttrubte('sessionInternalId', $session->getInternalId());
+
+                $dbForProject->updateDocument('targets', $target->getId(), $target);
+            }
+        }
+
         $dbForProject->purgeCachedDocument('users', $user->getId());
 
         $state['success']['query'] = URLParser::unparseQuery($query);
@@ -1636,7 +1650,7 @@ $createSession = function (string $userId, string $secret, Request $request, Res
 App::put('/v1/account/sessions/magic-url')
     ->desc('Update magic URL session')
     ->label('event', 'users.[userId].sessions.[sessionId].create')
-    ->groups(['api', 'account'])
+    ->groups(['api', 'account', 'session'])
     ->label('scope', 'sessions.write')
     ->label('audits.event', 'session.create')
     ->label('audits.resource', 'user/{response.userId}')
@@ -1666,7 +1680,7 @@ App::put('/v1/account/sessions/magic-url')
 App::put('/v1/account/sessions/phone')
     ->desc('Update phone session')
     ->label('event', 'users.[userId].sessions.[sessionId].create')
-    ->groups(['api', 'account'])
+    ->groups(['api', 'account', 'session'])
     ->label('scope', 'sessions.write')
     ->label('audits.event', 'session.create')
     ->label('audits.resource', 'user/{response.userId}')
@@ -1696,7 +1710,7 @@ App::put('/v1/account/sessions/phone')
 App::post('/v1/account/sessions/token')
     ->desc('Create session')
     ->label('event', 'users.[userId].sessions.[sessionId].create')
-    ->groups(['api', 'account'])
+    ->groups(['api', 'account', 'session'])
     ->label('scope', 'sessions.write')
     ->label('audits.event', 'session.create')
     ->label('audits.resource', 'user/{response.userId}')
@@ -1919,7 +1933,6 @@ App::post('/v1/account/sessions/anonymous')
     ->inject('geodb')
     ->inject('queueForEvents')
     ->action(function (Request $request, Response $response, Locale $locale, Document $user, Document $project, Database $dbForProject, Reader $geodb, Event $queueForEvents) {
-
         $protocol = $request->getProtocol();
         $roles = Authorization::getRoles();
         $isPrivilegedUser = Auth::isPrivilegedUser($roles);
@@ -1929,10 +1942,6 @@ App::post('/v1/account/sessions/anonymous')
             throw new Exception(Exception::USER_ANONYMOUS_CONSOLE_PROHIBITED, 'Failed to create anonymous user');
         }
 
-        if (!$user->isEmpty()) {
-            throw new Exception(Exception::USER_SESSION_ALREADY_EXISTS, 'Cannot create an anonymous user when logged in');
-        }
-
         $limit = $project->getAttribute('auths', [])['limit'] ?? 0;
 
         if ($limit !== 0) {

app/controllers/shared/api.php

@@ -461,6 +461,20 @@ App::init()
         }
     });
 
+App::init()
+    ->groups(['session'])
+    ->inject('user')
+    ->inject('request')
+    ->action(function (Document $user, Request $request) {
+        if (\str_contains($request->getURI(), 'oauth2')) {
+            return;
+        }
+
+        if (!$user->isEmpty()) {
+            throw new Exception(Exception::USER_SESSION_ALREADY_EXISTS);
+        }
+    });
+
 /**
  * Limit user session
  *
@@ -497,6 +511,7 @@ App::shutdown()
             $session = array_shift($sessions);
             $dbForProject->deleteDocument('sessions', $session->getId());
         }
+
         $dbForProject->purgeCachedDocument('users', $userId);
     });
 

tests/e2e/Services/Account/AccountCustomClientTest.php

@@ -74,7 +74,9 @@ class AccountCustomClientTest extends Scope
         $this->assertEmpty($response['body']['secret']);
         $this->assertNotFalse(\DateTime::createFromFormat('Y-m-d\TH:i:s.uP', $response['body']['expire']));
 
-        // already logged in
+        /**
+         * Test for FAILURE
+         */
         $response = $this->client->call(Client::METHOD_POST, '/account/sessions/email', array_merge([
             'origin' => 'http://localhost',
             'content-type' => 'application/json',
@@ -85,11 +87,8 @@ class AccountCustomClientTest extends Scope
             'password' => $password,
         ]);
 
-        $this->assertEquals(201, $response['headers']['status-code']);
+        $this->assertEquals(401, $response['headers']['status-code']);
 
-        /**
-         * Test for FAILURE
-         */
         $response = $this->client->call(Client::METHOD_POST, '/account/sessions/email', array_merge([
             'origin' => 'http://localhost',
             'content-type' => 'application/json',
@@ -233,10 +232,7 @@ class AccountCustomClientTest extends Scope
         ]));
 
         $this->assertEquals(200, $response['headers']['status-code']);
-        $this->assertIsArray($response['body']);
-        $this->assertNotEmpty($response['body']);
-        $this->assertCount(2, $response['body']);
-        $this->assertEquals(3, $response['body']['total']);
+        $this->assertEquals(2, $response['body']['total']);
         $this->assertEquals($sessionId, $response['body']['sessions'][0]['$id']);
 
         $this->assertEquals('Windows', $response['body']['sessions'][0]['osName']);
@@ -293,9 +289,9 @@ class AccountCustomClientTest extends Scope
         $this->assertEquals(200, $response['headers']['status-code']);
         $this->assertIsArray($response['body']['logs']);
         $this->assertNotEmpty($response['body']['logs']);
-        $this->assertCount(4, $response['body']['logs']);
+        $this->assertCount(3, $response['body']['logs']);
         $this->assertIsNumeric($response['body']['total']);
-        $this->assertEquals("session.create", $response['body']['logs'][2]['event']);
+        $this->assertEquals("user.create", $response['body']['logs'][2]['event']);
         $this->assertEquals(filter_var($response['body']['logs'][2]['ip'], FILTER_VALIDATE_IP), $response['body']['logs'][2]['ip']);
         $this->assertEquals(true, (new DatetimeValidator())->isValid($response['body']['logs'][2]['time']));
 
@@ -317,10 +313,6 @@ class AccountCustomClientTest extends Scope
         $this->assertEquals('--', $response['body']['logs'][1]['countryCode']);
         $this->assertEquals('Unknown', $response['body']['logs'][1]['countryName']);
 
-        $this->assertEquals("user.create", $response['body']['logs'][3]['event']);
-        $this->assertEquals(filter_var($response['body']['logs'][3]['ip'], FILTER_VALIDATE_IP), $response['body']['logs'][3]['ip']);
-        $this->assertEquals(true, (new DatetimeValidator())->isValid($response['body']['logs'][2]['time']));
-
         $this->assertEquals('Windows', $response['body']['logs'][2]['osName']);
         $this->assertEquals('WIN', $response['body']['logs'][2]['osCode']);
         $this->assertEquals('10', $response['body']['logs'][2]['osVersion']);
@@ -372,7 +364,7 @@ class AccountCustomClientTest extends Scope
         $this->assertEquals($responseOffset['headers']['status-code'], 200);
         $this->assertIsArray($responseOffset['body']['logs']);
         $this->assertNotEmpty($responseOffset['body']['logs']);
-        $this->assertCount(3, $responseOffset['body']['logs']);
+        $this->assertCount(2, $responseOffset['body']['logs']);
         $this->assertIsNumeric($responseOffset['body']['total']);
 
         $this->assertEquals($response['body']['logs'][1], $responseOffset['body']['logs'][0]);
@@ -2239,7 +2231,6 @@ class AccountCustomClientTest extends Scope
             'content-type' => 'application/json',
             'x-appwrite-project' => $this->getProject()['$id'],
             'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session,
-
         ]));
 
         $this->assertEquals(201, $response['headers']['status-code']);
@@ -2249,6 +2240,9 @@ class AccountCustomClientTest extends Scope
 
         $smsRequest = $this->getLastRequest();
 
+        $message = $smsRequest['data']['message'];
+        $token = substr($message, 0, 6);
+
         return \array_merge($data, [
             'token' => \substr($smsRequest['data']['message'], 0, 6)
         ]);

tests/e2e/Services/Account/AccountCustomServerTest.php

@@ -62,7 +62,9 @@ class AccountCustomServerTest extends Scope
         $this->assertNotEmpty($response['body']['secret']);
         $this->assertNotFalse(\DateTime::createFromFormat('Y-m-d\TH:i:s.uP', $response['body']['expire']));
 
-        // already logged in
+        /**
+         * Test for FAILURE
+         */
         $response = $this->client->call(Client::METHOD_POST, '/account/sessions/email', array_merge([
             'content-type' => 'application/json',
             'x-appwrite-project' => $this->getProject()['$id'],
@@ -72,11 +74,8 @@ class AccountCustomServerTest extends Scope
             'password' => $password,
         ]);
 
-        $this->assertEquals(201, $response['headers']['status-code']);
+        $this->assertEquals(401, $response['headers']['status-code']);
 
-        /**
-         * Test for FAILURE
-         */
         $response = $this->client->call(Client::METHOD_POST, '/account/sessions/email', array_merge([
             'content-type' => 'application/json',
             'x-appwrite-project' => $this->getProject()['$id'],

tests/e2e/Services/GraphQL/BatchTest.php

@@ -281,19 +281,22 @@ class BatchTest extends Scope
     public function testQueryBatchedMutations()
     {
         $projectId = $this->getProject()['$id'];
-        $email = 'tester' . \uniqid() . '@example.com';
+        $email1 = 'tester' . \uniqid() . '@example.com';
+        $email2 = 'tester' . \uniqid() . '@example.com';
         $graphQLPayload = [
-            'query' => 'mutation CreateAndLogin($userId: String!, $email: String!, $password: String!, $name: String) {
-                accountCreate(userId: $userId, email: $email, password: $password, name: $name) {
-                    name
+            'query' => 'mutation CreateAndLogin($user1Id: String!, $user2Id: String!, $email1: String!, $email2: String!, $password: String!, $name: String) {
+                account1: accountCreate(userId: $user1Id, email: $email1, password: $password, name: $name) {
+                    email
                 }
-                accountCreateEmailPasswordSession(email: $email, password: $password) {
-                    expire
+                account2: accountCreate(userId: $user2Id, email: $email2, password: $password, name: $name) {
+                    email
                 }
             }',
             'variables' => [
-                'userId' => ID::unique(),
-                'email' => $email,
+                'user1Id' => ID::unique(),
+                'user2Id' => ID::unique(),
+                'email1' => $email1,
+                'email2' => $email2,
                 'password' => 'password',
                 'name' => 'Tester',
             ],
@@ -304,12 +307,12 @@ class BatchTest extends Scope
             'x-appwrite-project' => $projectId,
         ], $this->getHeaders()), $graphQLPayload);
 
-
         $this->assertIsArray($response['body']['data']);
         $this->assertArrayNotHasKey('errors', $response['body']);
-        $this->assertArrayHasKey('accountCreate', $response['body']['data']);
-        $this->assertArrayHasKey('accountCreateEmailPasswordSession', $response['body']['data']);
-        $this->assertEquals('Tester', $response['body']['data']['accountCreate']['name']);
+        $this->assertArrayHasKey('account1', $response['body']['data']);
+        $this->assertArrayHasKey('account2', $response['body']['data']);
+        $this->assertEquals($email1, $response['body']['data']['account1']['email']);
+        $this->assertEquals($email2, $response['body']['data']['account2']['email']);
     }
 
     public function testQueryBatchedMutationsOfSameType()