From 5b759b44c28c34ab48b938711f32fb94323a1403 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 28 Feb 2019 10:26:21 +0100 Subject: [PATCH] Prevent the last moderator from leaving the room Signed-off-by: Joas Schilling --- docs/api-v1.md | 6 +- lib/Controller/RoomController.php | 40 ++++++---- lib/Room.php | 26 ++++++- .../features/conversation/delete-room.feature | 10 --- .../features/conversation/one-to-one.feature | 4 +- .../conversation/remove-participant.feature | 74 +++++++++++++++++++ 6 files changed, 131 insertions(+), 29 deletions(-) diff --git a/docs/api-v1.md b/docs/api-v1.md index cc72cb310..6e61a7da3 100644 --- a/docs/api-v1.md +++ b/docs/api-v1.md @@ -342,6 +342,7 @@ Base endpoint is: `/ocs/v2.php/apps/spreed/api/v1` * Response: - Header: + `200 OK` + + `400 Bad Request` When the participant is a moderator/owner and there are no other moderators/owners left. + `403 Forbidden` When the current user is not a moderator/owner + `403 Forbidden` When the participant to remove is an owner + `404 Not Found` When the room could not be found for the participant @@ -373,6 +374,7 @@ Base endpoint is: `/ocs/v2.php/apps/spreed/api/v1` * Response: - Header: + `200 OK` + + `400 Bad Request` When the participant is a moderator/owner and there are no other moderators/owners left. + `404 Not Found` When the room could not be found for the participant ### Join a room (available for call and chat) @@ -420,11 +422,11 @@ Base endpoint is: `/ocs/v2.php/apps/spreed/api/v1` * Response: - Header: + `200 OK` + + `400 Bad Request` When the participant to promote is not a normal user (type `3`) + `403 Forbidden` When the current user is not a moderator/owner + `403 Forbidden` When the participant to remove is an owner + `404 Not Found` When the room could not be found for the participant + `404 Not Found` When the participant to remove could not be found - + `412 Precondition Failed` When the participant to promote is not a normal user (type `3`) ### Demote a moderator to a user @@ -439,10 +441,10 @@ Base endpoint is: `/ocs/v2.php/apps/spreed/api/v1` * Response: - Header: + `200 OK` + + `400 Bad Request` When the participant to demote is not a moderator (type `2`) + `403 Forbidden` When the current user is not a moderator/owner + `404 Not Found` When the room could not be found for the participant + `404 Not Found` When the participant to demote could not be found - + `412 Precondition Failed` When the participant to demote is not a moderator (type `2`) ## Call management diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php index 24eeebb91..d97338bf2 100644 --- a/lib/Controller/RoomController.php +++ b/lib/Controller/RoomController.php @@ -801,6 +801,11 @@ class RoomController extends OCSController { return new DataResponse([], Http::STATUS_NOT_FOUND); } + if ($currentParticipant->getUser() === $participant) { + // Removing self, abusing moderator power + return $this->removeSelfFromRoomLogic($room, $currentParticipant); + } + if (!$currentParticipant->hasModeratorPermissions()) { return new DataResponse([], Http::STATUS_FORBIDDEN); } @@ -838,24 +843,33 @@ class RoomController extends OCSController { public function removeSelfFromRoom(string $token): DataResponse { try { $room = $this->manager->getRoomForParticipantByToken($token, $this->userId); - $room->getParticipant($this->userId); // Check if the participant is part of the room + $currentParticipant = $room->getParticipant($this->userId); // Check if the participant is part of the room } catch (RoomNotFoundException $e) { return new DataResponse([], Http::STATUS_NOT_FOUND); } catch (ParticipantNotFoundException $e) { return new DataResponse([], Http::STATUS_NOT_FOUND); } + return $this->removeSelfFromRoomLogic($room, $currentParticipant); + } + + protected function removeSelfFromRoomLogic(Room $room, Participant $participant): DataResponse { if ($room->getType() === Room::ONE_TO_ONE_CALL || $room->getNumberOfParticipants() === 1) { $room->deleteRoom(); - } else { - $currentUser = $this->userManager->get($this->userId); - if (!$currentUser instanceof IUser) { - return new DataResponse([], Http::STATUS_NOT_FOUND); - } - - $room->removeUser($currentUser, Room::PARTICIPANT_LEFT); + return new DataResponse([]); } + if ($participant->hasModeratorPermissions(false) && $room->getNumberOfModerators() === 1) { + return new DataResponse([], Http::STATUS_BAD_REQUEST); + } + + $currentUser = $this->userManager->get($participant->getUser()); + if (!$currentUser instanceof IUser) { + return new DataResponse([], Http::STATUS_NOT_FOUND); + } + + $room->removeUser($currentUser, Room::PARTICIPANT_LEFT); + return new DataResponse([]); } @@ -892,7 +906,7 @@ class RoomController extends OCSController { } if (!$targetParticipant->isGuest()) { - return new DataResponse([], Http::STATUS_PRECONDITION_FAILED); + return new DataResponse([], Http::STATUS_BAD_REQUEST); } if ($targetParticipant->getSessionId() === $currentParticipant->getSessionId()) { @@ -1098,7 +1112,7 @@ class RoomController extends OCSController { } if ($targetParticipant->getParticipantType() !== Participant::USER) { - return new DataResponse([], Http::STATUS_PRECONDITION_FAILED); + return new DataResponse([], Http::STATUS_BAD_REQUEST); } $room->setParticipantType($participant, Participant::MODERATOR); @@ -1114,7 +1128,7 @@ class RoomController extends OCSController { } if ($targetParticipant->getParticipantType() !== Participant::GUEST) { - return new DataResponse([], Http::STATUS_PRECONDITION_FAILED); + return new DataResponse([], Http::STATUS_BAD_REQUEST); } $room->setParticipantTypeBySession($targetParticipant, Participant::GUEST_MODERATOR); @@ -1168,7 +1182,7 @@ class RoomController extends OCSController { } if ($targetParticipant->getParticipantType() !== Participant::MODERATOR) { - return new DataResponse([], Http::STATUS_PRECONDITION_FAILED); + return new DataResponse([], Http::STATUS_BAD_REQUEST); } $room->setParticipantType($participant, Participant::USER); @@ -1188,7 +1202,7 @@ class RoomController extends OCSController { } if ($targetParticipant->getParticipantType() !== Participant::GUEST_MODERATOR) { - return new DataResponse([], Http::STATUS_PRECONDITION_FAILED); + return new DataResponse([], Http::STATUS_BAD_REQUEST); } $room->setParticipantTypeBySession($targetParticipant, Participant::GUEST); diff --git a/lib/Room.php b/lib/Room.php index 716f8d713..6a15fa51e 100644 --- a/lib/Room.php +++ b/lib/Room.php @@ -973,6 +973,28 @@ class Room { return (bool) $row; } + public function getNumberOfModerators(bool $ignoreGuests = true): int { + $types = [ + Participant::OWNER, + Participant::MODERATOR, + ]; + if (!$ignoreGuests) { + $types[] = Participant::GUEST_MODERATOR; + } + + $query = $this->db->getQueryBuilder(); + $query->select($query->func()->count('*', 'num_moderators')) + ->from('talk_participants') + ->where($query->expr()->eq('room_id', $query->createNamedParameter($this->getId(), IQueryBuilder::PARAM_INT))) + ->andWhere($query->expr()->in('participant_type', $query->createNamedParameter($types, IQueryBuilder::PARAM_INT_ARRAY))); + + $result = $query->execute(); + $row = $result->fetch(); + $result->closeCursor(); + + return (int) ($row['num_moderators'] ?? 0); + } + /** * @param bool $ignoreGuests * @param int $lastPing When the last ping is older than the given timestamp, the user is ignored @@ -980,7 +1002,7 @@ class Room { */ public function getNumberOfParticipants(bool $ignoreGuests = true, int $lastPing = 0): int { $query = $this->db->getQueryBuilder(); - $query->selectAlias($query->createFunction('COUNT(*)'), 'num_participants') + $query->select($query->func()->count('*', 'num_participants')) ->from('talk_participants') ->where($query->expr()->eq('room_id', $query->createNamedParameter($this->getId(), IQueryBuilder::PARAM_INT))); @@ -999,7 +1021,7 @@ class Room { $row = $result->fetch(); $result->closeCursor(); - return isset($row['num_participants']) ? (int) $row['num_participants'] : 0; + return (int) ($row['num_participants'] ?? 0); } public function markUsersAsMentioned(array $userIds, \DateTime $time): void { diff --git a/tests/integration/features/conversation/delete-room.feature b/tests/integration/features/conversation/delete-room.feature index 5f5a31e83..fe3539e11 100644 --- a/tests/integration/features/conversation/delete-room.feature +++ b/tests/integration/features/conversation/delete-room.feature @@ -54,13 +54,3 @@ Feature: public When user "participant2" deletes room "room" with 404 Then user "participant1" is participant of room "room" And user "participant2" is not participant of room "room" - - Scenario: User1 creates a public room and deletes themself - Given user "participant1" creates room "room" - | roomType | 3 | - | roomName | room | - And user "participant1" is participant of room "room" - And user "participant2" is not participant of room "room" - And user "participant3" is not participant of room "room" - When user "participant1" removes "participant1" from room "room" with 403 - Then user "participant1" is participant of room "room" diff --git a/tests/integration/features/conversation/one-to-one.feature b/tests/integration/features/conversation/one-to-one.feature index 8d100d575..1c4fe4507 100644 --- a/tests/integration/features/conversation/one-to-one.feature +++ b/tests/integration/features/conversation/one-to-one.feature @@ -100,7 +100,7 @@ Feature: one-to-one | invite | participant2 | And user "participant1" is participant of room "room8" And user "participant2" is participant of room "room8" - When user "participant1" promotes "participant2" in room "room8" with 412 + When user "participant1" promotes "participant2" in room "room8" with 400 Scenario: User1 invites user2 to a one2one room and demote user2 to moderator Given user "participant1" creates room "room9" @@ -108,7 +108,7 @@ Feature: one-to-one | invite | participant2 | And user "participant1" is participant of room "room9" And user "participant2" is participant of room "room9" - When user "participant1" demotes "participant2" in room "room9" with 412 + When user "participant1" demotes "participant2" in room "room9" with 400 Scenario: User1 invites user2 to a one2one room and promote non-invited user Given user "participant1" creates room "room10" diff --git a/tests/integration/features/conversation/remove-participant.feature b/tests/integration/features/conversation/remove-participant.feature index ebf74e214..c2450df4d 100644 --- a/tests/integration/features/conversation/remove-participant.feature +++ b/tests/integration/features/conversation/remove-participant.feature @@ -16,6 +16,37 @@ Feature: public When user "participant1" removes "participant3" from room "room" with 403 Then user "participant3" is participant of room "room" + Scenario: Owner removes self participant from empty public room + Given user "participant1" creates room "room" + | roomType | 3 | + | roomName | room | + And user "participant1" is participant of room "room" + When user "participant1" removes "participant1" from room "room" with 200 + Then user "participant1" is not participant of room "room" + + Scenario: Owner removes self participant from public room when there are other users in the room + Given user "participant1" creates room "room" + | roomType | 3 | + | roomName | room | + And user "participant1" adds "participant2" to room "room" with 200 + And user "participant1" is participant of room "room" + And user "participant2" is participant of room "room" + When user "participant1" removes "participant1" from room "room" with 400 + Then user "participant1" is participant of room "room" + And user "participant2" is participant of room "room" + + Scenario: Owner removes self participant from public room when there are other moderators in the room + Given user "participant1" creates room "room" + | roomType | 3 | + | roomName | room | + And user "participant1" adds "participant2" to room "room" with 200 + And user "participant1" promotes "participant2" in room "room" with 200 + And user "participant1" is participant of room "room" + And user "participant2" is participant of room "room" + When user "participant1" removes "participant1" from room "room" with 200 + Then user "participant1" is not participant of room "room" + And user "participant2" is participant of room "room" + Scenario: Moderator removes owner Given user "participant1" creates room "room" | roomType | 1 | @@ -71,6 +102,49 @@ Feature: public When user "participant2" removes "participant3" from room "room" with 200 Then user "participant3" is not participant of room "room" + Scenario: Moderator removes self participant from empty public room + Given user "participant1" creates room "room" + | roomType | 3 | + | roomName | room | + And user "participant1" adds "participant2" to room "room" with 200 + And user "participant1" promotes "participant2" in room "room" with 200 + And user "participant1" removes "participant1" from room "room" with 200 + And user "participant1" is not participant of room "room" + And user "participant2" is participant of room "room" + When user "participant2" removes "participant2" from room "room" with 200 + Then user "participant2" is not participant of room "room" + + Scenario: Moderator removes self participant from public room when there are other users in the room + Given user "participant1" creates room "room" + | roomType | 3 | + | roomName | room | + And user "participant1" adds "participant2" to room "room" with 200 + And user "participant1" promotes "participant2" in room "room" with 200 + And user "participant1" removes "participant1" from room "room" with 200 + And user "participant1" is not participant of room "room" + And user "participant2" adds "participant3" to room "room" with 200 + And user "participant2" is participant of room "room" + And user "participant3" is participant of room "room" + When user "participant2" removes "participant2" from room "room" with 400 + Then user "participant2" is participant of room "room" + And user "participant3" is participant of room "room" + + Scenario: Moderator removes self participant from public room when there are other moderators in the room + Given user "participant1" creates room "room" + | roomType | 3 | + | roomName | room | + And user "participant1" adds "participant2" to room "room" with 200 + And user "participant1" promotes "participant2" in room "room" with 200 + And user "participant1" removes "participant1" from room "room" with 200 + And user "participant1" is not participant of room "room" + And user "participant2" adds "participant3" to room "room" with 200 + And user "participant2" promotes "participant3" in room "room" with 200 + And user "participant2" is participant of room "room" + And user "participant3" is participant of room "room" + When user "participant2" removes "participant2" from room "room" with 200 + Then user "participant2" is not participant of room "room" + And user "participant3" is participant of room "room" + Scenario: User removes moderator Given user "participant1" creates room "room" | roomType | 3 |