Merge pull request #9888 from appwrite/fix-teams-deletion

Fix teams deletion
2026-05-23 00:49:02 +00:00 · 2025-05-30 09:23:02 +00:00 · 2025-05-30 09:23:02 +00:00 · b189ba669e
commit b189ba669e
parent 217e765f7b 36bc41a748
5 changed files with 155 additions and 30 deletions
--- a/app/config/errors.php
+++ b/app/config/errors.php
@ -393,6 +393,16 @@ return [
        'description' => 'Membership is already confirmed.',
        'code' => 409,
    ],
+    Exception::MEMBERSHIP_DELETION_PROHIBITED => [
+        'name' => Exception::MEMBERSHIP_DELETION_PROHIBITED,
+        'description' => 'Membership deletion is prohibited.',
+        'code' => 400,
+    ],
+    Exception::MEMBERSHIP_DOWNGRADE_PROHIBITED => [
+        'name' => Exception::MEMBERSHIP_DOWNGRADE_PROHIBITED,
+        'description' => 'Membership role downgrade is prohibited.',
+        'code' => 400,
+    ],

    /** Avatars */
    Exception::AVATAR_SET_NOT_FOUND => [
--- a/app/controllers/api/teams.php
+++ b/app/controllers/api/teams.php
@ -1097,10 +1097,13 @@ App::patch('/v1/teams/:teamId/memberships/:membershipId')
                max: 2
            );

+            // Is the role change being requested by the user on their own membership?
+            $isCurrentUserAnOwner =  $user->getInternalId() === $membership->getAttribute('userInternalId');
+
            // Prevent role change if there's only one owner left,
-            // the requester is that owner, and the new `$roles` no longer include 'owner'!
-            if ($ownersCount === 1 && $isOwner && !\in_array('owner', $roles)) {
-                throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'There must be at least one owner in the organization.');
+            // the requester is that owner, and the new `$roles` no longer include 'owner'
+            if ($ownersCount === 1 && $isOwner && $isCurrentUserAnOwner && !\in_array('owner', $roles)) {
+                throw new Exception(Exception::MEMBERSHIP_DOWNGRADE_PROHIBITED, 'There must be at least one owner in the organization.');
            }
        }

@ -1315,10 +1318,12 @@ App::delete('/v1/teams/:teamId/memberships/:membershipId')
    ))
    ->param('teamId', '', new UID(), 'Team ID.')
    ->param('membershipId', '', new UID(), 'Membership ID.')
+    ->inject('user')
+    ->inject('project')
    ->inject('response')
    ->inject('dbForProject')
    ->inject('queueForEvents')
-    ->action(function (string $teamId, string $membershipId, Response $response, Database $dbForProject, Event $queueForEvents) {
+    ->action(function (string $teamId, string $membershipId, Document $user, Document $project, Response $response, Database $dbForProject, Event $queueForEvents) {

        $membership = $dbForProject->getDocument('memberships', $membershipId);

@ -1326,9 +1331,9 @@ App::delete('/v1/teams/:teamId/memberships/:membershipId')
            throw new Exception(Exception::TEAM_INVITE_NOT_FOUND);
        }

-        $user = $dbForProject->getDocument('users', $membership->getAttribute('userId'));
+        $profile = $dbForProject->getDocument('users', $membership->getAttribute('userId'));

-        if ($user->isEmpty()) {
+        if ($profile->isEmpty()) {
            throw new Exception(Exception::USER_NOT_FOUND);
        }

@ -1342,6 +1347,27 @@ App::delete('/v1/teams/:teamId/memberships/:membershipId')
            throw new Exception(Exception::TEAM_MEMBERSHIP_MISMATCH);
        }

+        if ($project->getId() === 'console') {
+            // Quick check:
+            // fetch up to 2 owners to determine if only one exists
+            $ownersCount = $dbForProject->count(
+                collection: 'memberships',
+                queries: [
+                    Query::contains('roles', ['owner']),
+                    Query::equal('teamInternalId', [$team->getInternalId()])
+                ],
+                max: 2
+            );
+
+            // Is the deletion being requested by the user on their own membership?
+            $isCurrentUserAnOwner =  $user->getInternalId() === $membership->getAttribute('userInternalId');
+
+            if ($ownersCount === 1 && $isCurrentUserAnOwner) {
+                /* Prevent removal if the user is the only owner. */
+                throw new Exception(Exception::MEMBERSHIP_DELETION_PROHIBITED, 'There must be at least one owner in the organization.');
+            }
+        }
+
        try {
            $dbForProject->deleteDocument('memberships', $membership->getId());
        } catch (AuthorizationException $exception) {
@ -1350,15 +1376,15 @@ App::delete('/v1/teams/:teamId/memberships/:membershipId')
            throw new Exception(Exception::GENERAL_SERVER_ERROR, 'Failed to remove membership from DB');
        }

-        $dbForProject->purgeCachedDocument('users', $user->getId());
+        $dbForProject->purgeCachedDocument('users', $profile->getId());

        if ($membership->getAttribute('confirm')) { // Count only confirmed members
            Authorization::skip(fn () => $dbForProject->decreaseDocumentAttribute('teams', $team->getId(), 'total', 1, 0));
        }

        $queueForEvents
-            ->setParam('userId', $user->getId())
            ->setParam('teamId', $team->getId())
+            ->setParam('userId', $profile->getId())
            ->setParam('membershipId', $membership->getId())
            ->setPayload($response->output($membership, Response::MODEL_MEMBERSHIP))
        ;
--- a/src/Appwrite/Extend/Exception.php
+++ b/src/Appwrite/Extend/Exception.php
@ -124,6 +124,8 @@ class Exception extends \Exception
    /** Membership */
    public const MEMBERSHIP_NOT_FOUND              = 'membership_not_found';
    public const MEMBERSHIP_ALREADY_CONFIRMED      = 'membership_already_confirmed';
+    public const MEMBERSHIP_DELETION_PROHIBITED    = 'membership_deletion_prohibited';
+    public const MEMBERSHIP_DOWNGRADE_PROHIBITED   = 'membership_downgrade_prohibited';

    /** Avatars */
    public const AVATAR_SET_NOT_FOUND              = 'avatar_set_not_found';
--- a/tests/e2e/Services/Teams/TeamsBase.php
+++ b/tests/e2e/Services/Teams/TeamsBase.php
@ -64,7 +64,7 @@ trait TeamsBase

            // Step 4: Assert failure — cannot remove the only OWNER from a team
            $this->assertEquals(400, $response['headers']['status-code']);
-            $this->assertEquals('general_argument_invalid', $response['body']['type']);
+            $this->assertEquals('membership_downgrade_prohibited', $response['body']['type']);
            $this->assertEquals('There must be at least one owner in the organization.', $response['body']['message']);
        }

--- a/tests/e2e/Services/Teams/TeamsConsoleClientTest.php
+++ b/tests/e2e/Services/Teams/TeamsConsoleClientTest.php
@ -65,7 +65,7 @@ class TeamsConsoleClientTest extends Scope
        /**
         * Test for SUCCESS
         */
-        $response = $this->client->call(Client::METHOD_POST, '/teams/' . $teamUid . '/memberships', array_merge([
+        $developer = $this->client->call(Client::METHOD_POST, '/teams/' . $teamUid . '/memberships', array_merge([
            'content-type' => 'application/json',
            'x-appwrite-project' => $this->getProject()['$id'],
        ], $this->getHeaders()), [
@ -75,7 +75,8 @@ class TeamsConsoleClientTest extends Scope
            'url' => 'http://localhost:5000/join-us#title'
        ]);

-        $this->assertEquals(201, $response['headers']['status-code']);
+        $developerUserId = $developer['body']['$id'];
+        $this->assertEquals(201, $developer['headers']['status-code']);

        $response = $this->client->call(Client::METHOD_GET, '/users', array_merge([
            'content-type' => 'application/json',
@ -90,13 +91,26 @@ class TeamsConsoleClientTest extends Scope

        $this->assertEquals(200, $response['headers']['status-code']);

-        $ownerMembershipUid = $response['body']['memberships'][1]['$id'];
+        $ownerMembershipUid = $response['body']['memberships'][0]['$id'];

        $response = $this->client->call(Client::METHOD_DELETE, '/teams/' . $teamUid . '/memberships/' . $ownerMembershipUid, array_merge([
            'origin' => 'http://localhost',
            'content-type' => 'application/json',
            'x-appwrite-project' => $this->getProject()['$id'],
        ], $this->getHeaders()));
+
+        $this->assertEquals(400, $response['headers']['status-code']);
+        $this->assertEquals('membership_deletion_prohibited', $response['body']['type']);
+        $this->assertEquals('There must be at least one owner in the organization.', $response['body']['message']);
+
+        // Remove the excess developer member to reduce the membership count in `TeamsBaseClient` tests.
+        // This is necessary because the only owner cannot be removed in the console project / top level team / organization.
+        $response = $this->client->call(Client::METHOD_DELETE, '/teams/' . $teamUid . '/memberships/' . $developerUserId, array_merge([
+            'origin' => 'http://localhost',
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()));
+
        $this->assertEquals(204, $response['headers']['status-code']);

        return $data;
@ -109,21 +123,6 @@ class TeamsConsoleClientTest extends Scope
        $membershipUid = $data['membershipUid'] ?? '';
        $session = $data['session'] ?? '';

-        /**
-         * Test for FAILURE
-         */
-        $roles = ['developer'];
-        $response = $this->client->call(Client::METHOD_PATCH, '/teams/' . $teamUid . '/memberships/' . $membershipUid, array_merge([
-            'origin' => 'http://localhost',
-            'content-type' => 'application/json',
-            'x-appwrite-project' => $this->getProject()['$id'],
-        ], $this->getHeaders()), [
-            'roles' => $roles
-        ]);
-
-        $this->assertEquals(400, $response['headers']['status-code']);
-        $this->assertEquals('There must be at least one owner in the organization.', $response['body']['message']);
-
        /**
         * Test for unknown team
         */
@ -132,7 +131,7 @@ class TeamsConsoleClientTest extends Scope
            'content-type' => 'application/json',
            'x-appwrite-project' => $this->getProject()['$id'],
        ], $this->getHeaders()), [
-            'roles' => $roles
+            'roles' => ['developer']
        ]);

        $this->assertEquals(404, $response['headers']['status-code']);
@ -145,7 +144,7 @@ class TeamsConsoleClientTest extends Scope
            'content-type' => 'application/json',
            'x-appwrite-project' => $this->getProject()['$id'],
        ], $this->getHeaders()), [
-            'roles' => $roles
+            'roles' => ['developer']
        ]);

        $this->assertEquals(404, $response['headers']['status-code']);
@ -160,7 +159,7 @@ class TeamsConsoleClientTest extends Scope
            'x-appwrite-project' => $this->getProject()['$id'],
            'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session,
        ], [
-            'roles' => $roles
+            'roles' => ['developer']
        ]);

        $this->assertEquals(401, $response['headers']['status-code']);
@ -168,4 +167,92 @@ class TeamsConsoleClientTest extends Scope

        return $data;
    }
+
+    /**
+     * @depends testUpdateTeamMembershipRoles
+     */
+    public function testDeleteTeamMembership($data): array
+    {
+        $teamUid = $data['teamUid'] ?? '';
+        $membershipUid = $data['membershipUid'] ?? '';
+        $session = $data['session'] ?? '';
+
+        $response = $this->client->call(Client::METHOD_GET, '/teams/' . $teamUid . '/memberships', array_merge([
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()));
+
+        $this->assertEquals(200, $response['headers']['status-code']);
+        $this->assertEquals(3, $response['body']['total']);
+
+        $ownerMembershipUid = $response['body']['memberships'][0]['$id'];
+
+        /**
+         * Test deleting a membership that does not exists
+         */
+        $response = $this->client->call(Client::METHOD_DELETE, '/teams/' . $teamUid . '/memberships/dne', [
+            'origin' => 'http://localhost',
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+            'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session,
+        ]);
+
+        $this->assertEquals(404, $response['headers']['status-code']);
+
+        /**
+         * Test deleting another user's membership
+         */
+        $response = $this->client->call(Client::METHOD_DELETE, '/teams/' . $teamUid . '/memberships/' . $ownerMembershipUid, [
+            'origin' => 'http://localhost',
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+            'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session,
+        ]);
+
+        $this->assertEquals(401, $response['headers']['status-code']);
+        $this->assertEquals('The current user is not authorized to perform the requested action.', $response['body']['message']);
+
+        /**
+         * Test for when a user other than the owner tries to delete their membership
+         */
+        $response = $this->client->call(Client::METHOD_DELETE, '/teams/' . $teamUid . '/memberships/' . $membershipUid, [
+            'origin' => 'http://localhost',
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+            'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session,
+        ]);
+
+        $this->assertEquals(400, $response['headers']['status-code']);
+
+        $response = $this->client->call(Client::METHOD_GET, '/teams/' . $teamUid . '/memberships', array_merge([
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()));
+
+        $this->assertEquals(200, $response['headers']['status-code']);
+        $this->assertEquals(3, $response['body']['total']);
+
+        /**
+         * Test for when the owner tries to delete their membership
+         */
+        $response = $this->client->call(Client::METHOD_DELETE, '/teams/' . $teamUid . '/memberships/' . $ownerMembershipUid, array_merge([
+            'origin' => 'http://localhost',
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()));
+
+        $this->assertEquals(400, $response['headers']['status-code']);
+        $this->assertEquals('membership_deletion_prohibited', $response['body']['type']);
+        $this->assertEquals('There must be at least one owner in the organization.', $response['body']['message']);
+
+        $response = $this->client->call(Client::METHOD_GET, '/teams/' . $teamUid . '/memberships/' . $ownerMembershipUid, array_merge([
+            'origin' => 'http://localhost',
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()));
+
+        $this->assertEquals(200, $response['headers']['status-code']);
+
+        return [];
+    }
 }