diff --git a/app/config/errors.php b/app/config/errors.php index 6d9d29a8ea..8365e8c705 100644 --- 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 => [ diff --git a/app/controllers/api/teams.php b/app/controllers/api/teams.php index 49d9005c54..ede61115e2 100644 --- 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)) ; diff --git a/src/Appwrite/Extend/Exception.php b/src/Appwrite/Extend/Exception.php index 296434ed57..3af6d9962c 100644 --- 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'; diff --git a/tests/e2e/Services/Teams/TeamsBase.php b/tests/e2e/Services/Teams/TeamsBase.php index 80ac1621ee..fccfded1e1 100644 --- 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']); } diff --git a/tests/e2e/Services/Teams/TeamsConsoleClientTest.php b/tests/e2e/Services/Teams/TeamsConsoleClientTest.php index f096831a76..dda7f8e4ae 100644 --- 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 []; + } }