diff --git a/.drone.yml b/.drone.yml index c0b07a7addd..f31bdf67d39 100644 --- a/.drone.yml +++ b/.drone.yml @@ -29,7 +29,7 @@ trigger: - master - stable* event: - - pull_request +# - pull_request - push --- @@ -64,7 +64,7 @@ trigger: - master - stable* event: - - pull_request +# - pull_request - push --- @@ -99,7 +99,7 @@ trigger: - master - stable* event: - - pull_request +# - pull_request - push --- @@ -136,7 +136,7 @@ trigger: - master - stable* event: - - pull_request +# - pull_request - push --- @@ -173,7 +173,7 @@ trigger: - master - stable* event: - - pull_request +# - pull_request - push --- @@ -208,7 +208,7 @@ trigger: - master - stable* event: - - pull_request +# - pull_request - push --- @@ -243,7 +243,7 @@ trigger: - master - stable* event: - - pull_request +# - pull_request - push --- @@ -279,7 +279,11 @@ services: MYSQL_USER: oc_autotest MYSQL_PASSWORD: owncloud MYSQL_DATABASE: oc_autotest - command: [ "--innodb_large_prefix=true", "--innodb_file_format=barracuda", "--innodb_file_per_table=true" ] + command: + - --innodb_large_prefix=true + - --innodb_file_format=barracuda + - --innodb_file_per_table=true + - --sql-mode=ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION tmpfs: - /var/lib/mysql @@ -288,7 +292,7 @@ trigger: - master - stable* event: -# - pull_request + - pull_request - push --- @@ -324,7 +328,11 @@ services: MYSQL_USER: oc_autotest MYSQL_PASSWORD: owncloud MYSQL_DATABASE: oc_autotest - command: [ "--innodb_large_prefix=true", "--innodb_file_format=barracuda", "--innodb_file_per_table=true" ] + command: + - --innodb_large_prefix=true + - --innodb_file_format=barracuda + - --innodb_file_per_table=true + - --sql-mode=ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION tmpfs: - /var/lib/mysql @@ -333,7 +341,7 @@ trigger: - master - stable* event: -# - pull_request + - pull_request - push --- @@ -369,7 +377,11 @@ services: MYSQL_USER: oc_autotest MYSQL_PASSWORD: owncloud MYSQL_DATABASE: oc_autotest - command: [ "--innodb_large_prefix=true", "--innodb_file_format=barracuda", "--innodb_file_per_table=true" ] + command: + - --innodb_large_prefix=true + - --innodb_file_format=barracuda + - --innodb_file_per_table=true + - --sql-mode=ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION tmpfs: - /var/lib/mysql @@ -378,7 +390,7 @@ trigger: - master - stable* event: -# - pull_request + - pull_request - push --- @@ -416,7 +428,11 @@ services: MYSQL_USER: oc_autotest MYSQL_PASSWORD: owncloud MYSQL_DATABASE: oc_autotest - command: [ "--innodb_large_prefix=true", "--innodb_file_format=barracuda", "--innodb_file_per_table=true" ] + command: + - --innodb_large_prefix=true + - --innodb_file_format=barracuda + - --innodb_file_per_table=true + - --sql-mode=ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION tmpfs: - /var/lib/mysql @@ -425,7 +441,7 @@ trigger: - master - stable* event: -# - pull_request + - pull_request - push --- @@ -463,7 +479,11 @@ services: MYSQL_USER: oc_autotest MYSQL_PASSWORD: owncloud MYSQL_DATABASE: oc_autotest - command: [ "--innodb_large_prefix=true", "--innodb_file_format=barracuda", "--innodb_file_per_table=true" ] + command: + - --innodb_large_prefix=true + - --innodb_file_format=barracuda + - --innodb_file_per_table=true + - --sql-mode=ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION tmpfs: - /var/lib/mysql @@ -472,7 +492,7 @@ trigger: - master - stable* event: -# - pull_request + - pull_request - push --- @@ -508,7 +528,11 @@ services: MYSQL_USER: oc_autotest MYSQL_PASSWORD: owncloud MYSQL_DATABASE: oc_autotest - command: [ "--innodb_large_prefix=true", "--innodb_file_format=barracuda", "--innodb_file_per_table=true" ] + command: + - --innodb_large_prefix=true + - --innodb_file_format=barracuda + - --innodb_file_per_table=true + - --sql-mode=ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION tmpfs: - /var/lib/mysql @@ -517,7 +541,7 @@ trigger: - master - stable* event: -# - pull_request + - pull_request - push --- @@ -553,7 +577,11 @@ services: MYSQL_USER: oc_autotest MYSQL_PASSWORD: owncloud MYSQL_DATABASE: oc_autotest - command: [ "--innodb_large_prefix=true", "--innodb_file_format=barracuda", "--innodb_file_per_table=true" ] + command: + - --innodb_large_prefix=true + - --innodb_file_format=barracuda + - --innodb_file_per_table=true + - --sql-mode=ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION tmpfs: - /var/lib/mysql @@ -562,7 +590,7 @@ trigger: - master - stable* event: -# - pull_request + - pull_request - push --- diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 731b3471fdb..6c92da58247 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -103,6 +103,11 @@ jobs: options: --health-cmd="mysqladmin ping" --health-interval 5s --health-timeout 2s --health-retries 5 steps: + - name: Enable ONLY_FULL_GROUP_BY MySQL option + run: | + echo "SET GLOBAL sql_mode=(SELECT CONCAT(@@sql_mode,',ONLY_FULL_GROUP_BY'));" | mysql -h 127.0.0.1 -P 4444 -u root -prootpassword + echo "SELECT @@sql_mode;" | mysql -h 127.0.0.1 -P 4444 -u root -prootpassword + - name: Checkout server uses: actions/checkout@v2 with: diff --git a/lib/Chat/MessageParser.php b/lib/Chat/MessageParser.php index db142107eec..5e8ae8463a1 100644 --- a/lib/Chat/MessageParser.php +++ b/lib/Chat/MessageParser.php @@ -92,7 +92,7 @@ protected function setActor(Message $message): void { $displayName = $this->guestNames[$comment->getActorId()]; } else { try { - $participant = $message->getRoom()->getParticipantByActor(Attendee::ACTOR_GUESTS, $comment->getActorId(), false); + $participant = $message->getRoom()->getParticipantByActor(Attendee::ACTOR_GUESTS, $comment->getActorId()); $displayName = $participant->getAttendee()->getDisplayName(); } catch (ParticipantNotFoundException $e) { } diff --git a/lib/Chat/Parser/SystemMessage.php b/lib/Chat/Parser/SystemMessage.php index c29da342167..40db1ae235c 100644 --- a/lib/Chat/Parser/SystemMessage.php +++ b/lib/Chat/Parser/SystemMessage.php @@ -695,7 +695,7 @@ protected function getGuest(Room $room, string $actorId): array { protected function getGuestName(Room $room, string $actorId): string { try { - $participant = $room->getParticipantByActor(Attendee::ACTOR_GUESTS, $actorId, false); + $participant = $room->getParticipantByActor(Attendee::ACTOR_GUESTS, $actorId); $name = $participant->getAttendee()->getDisplayName(); if ($name === '') { return $this->l->t('Guest'); diff --git a/lib/Chat/Parser/UserMention.php b/lib/Chat/Parser/UserMention.php index 743fb2caa35..b4bec33c718 100644 --- a/lib/Chat/Parser/UserMention.php +++ b/lib/Chat/Parser/UserMention.php @@ -132,7 +132,7 @@ public function parseMessage(Message $chatMessage): void { ]; } elseif ($mention['type'] === 'guest') { try { - $participant = $chatMessage->getRoom()->getParticipantByActor(Attendee::ACTOR_GUESTS, substr($mention['id'], strlen('guest/')), false); + $participant = $chatMessage->getRoom()->getParticipantByActor(Attendee::ACTOR_GUESTS, substr($mention['id'], strlen('guest/'))); $displayName = $participant->getAttendee()->getDisplayName() ?: $this->l->t('Guest'); } catch (ParticipantNotFoundException $e) { $displayName = $this->l->t('Guest'); diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php index 57cb0c096ca..18c62d90e23 100644 --- a/lib/Controller/RoomController.php +++ b/lib/Controller/RoomController.php @@ -1508,15 +1508,8 @@ protected function changeParticipantType(int $attendeeId, bool $promote): DataR } // Prevent users/moderators modifying themselves - if ($attendee->getActorType() === Attendee::ACTOR_USERS) { - if ($attendee->getActorId() === $this->userId) { - return new DataResponse([], Http::STATUS_FORBIDDEN); - } - } elseif ($attendee->getActorType() === Attendee::ACTOR_GUESTS) { - $session = $targetParticipant->getSession(); - $currentSessionId = $this->session->getSessionForRoom($this->room->getToken()); - - if ($session instanceof Session && $currentSessionId === $session->getSessionId()) { + if ($attendee->getActorType() === $this->participant->getAttendee()->getActorType()) { + if ($attendee->getActorId() === $this->participant->getAttendee()->getActorId()) { return new DataResponse([], Http::STATUS_FORBIDDEN); } } elseif ($attendee->getActorType() === Attendee::ACTOR_GROUPS) { diff --git a/lib/Controller/SignalingController.php b/lib/Controller/SignalingController.php index 962cc087a75..9c7e0a0e309 100644 --- a/lib/Controller/SignalingController.php +++ b/lib/Controller/SignalingController.php @@ -601,14 +601,14 @@ private function backendRoom(array $roomRequest): DataResponse { // If the user joins the session might not be known to the server yet. // In this case we load by actor information and use the session id as new session. try { - $participant = $room->getParticipantByActor($actorType, $actorId, false); + $participant = $room->getParticipantByActor($actorType, $actorId); } catch (ParticipantNotFoundException $e) { } } } } else { try { - $participant = $room->getParticipantByActor($actorType, $actorId, false); + $participant = $room->getParticipantByActor($actorType, $actorId); } catch (ParticipantNotFoundException $e) { } } diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index e42dcb29fc9..0b0072536ad 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -492,7 +492,7 @@ protected function parseChatMessage(INotification $notification, Room $room, Par * @throws ParticipantNotFoundException */ protected function getGuestParameter(Room $room, string $actorId): array { - $participant = $room->getParticipantByActor(Attendee::ACTOR_GUESTS, $actorId, false); + $participant = $room->getParticipantByActor(Attendee::ACTOR_GUESTS, $actorId); $name = $participant->getAttendee()->getDisplayName(); if (trim($name) === '') { throw new ParticipantNotFoundException('Empty name'); diff --git a/lib/Room.php b/lib/Room.php index 647462b402e..830ccd7e4b2 100644 --- a/lib/Room.php +++ b/lib/Room.php @@ -559,13 +559,10 @@ public function getParticipantByPin(string $pin): Participant { /** * @param int $attendeeId - * @param string|null|false $sessionId Set to false if you don't want to load a session (and save resources), - * string to try loading a specific session - * null to try loading "any" * @return Participant * @throws ParticipantNotFoundException When the pin is not valid (has no participant assigned) */ - public function getParticipantByAttendeeId(int $attendeeId, $sessionId = null): Participant { + public function getParticipantByAttendeeId(int $attendeeId): Participant { $query = $this->db->getQueryBuilder(); $helper = new SelectHelper(); $helper->selectAttendeesTable($query); @@ -574,20 +571,6 @@ public function getParticipantByAttendeeId(int $attendeeId, $sessionId = null): ->andWhere($query->expr()->eq('a.room_id', $query->createNamedParameter($this->getId()))) ->setMaxResults(1); - if ($sessionId !== false) { - if ($sessionId !== null) { - $helper->selectSessionsTable($query); - $query->leftJoin('a', 'talk_sessions', 's', $query->expr()->andX( - $query->expr()->eq('s.session_id', $query->createNamedParameter($sessionId)), - $query->expr()->eq('a.id', 's.attendee_id') - )); - } else { - $helper->selectSessionsTableMax($query); - $query->groupBy('a.id'); - $query->leftJoin('a', 'talk_sessions', 's', $query->expr()->eq('a.id', 's.attendee_id')); - } - } - $result = $query->executeQuery(); $row = $result->fetch(); $result->closeCursor(); @@ -602,15 +585,12 @@ public function getParticipantByAttendeeId(int $attendeeId, $sessionId = null): /** * @param string $actorType * @param string $actorId - * @param string|null|false $sessionId Set to false if you don't want to load a session (and save resources), - * string to try loading a specific session - * null to try loading "any" * @return Participant * @throws ParticipantNotFoundException When the pin is not valid (has no participant assigned) */ - public function getParticipantByActor(string $actorType, string $actorId, $sessionId = null): Participant { + public function getParticipantByActor(string $actorType, string $actorId): Participant { if ($actorType === Attendee::ACTOR_USERS) { - return $this->getParticipant($actorId, $sessionId); + return $this->getParticipant($actorId, false); } $query = $this->db->getQueryBuilder(); @@ -622,20 +602,6 @@ public function getParticipantByActor(string $actorType, string $actorId, $sessi ->andWhere($query->expr()->eq('a.room_id', $query->createNamedParameter($this->getId()))) ->setMaxResults(1); - if ($sessionId !== false) { - if ($sessionId !== null) { - $helper->selectSessionsTable($query); - $query->leftJoin('a', 'talk_sessions', 's', $query->expr()->andX( - $query->expr()->eq('s.session_id', $query->createNamedParameter($sessionId)), - $query->expr()->eq('a.id', 's.attendee_id') - )); - } else { - $helper->selectSessionsTableMax($query); - $query->groupBy('a.id'); - $query->leftJoin('a', 'talk_sessions', 's', $query->expr()->eq('a.id', 's.attendee_id')); - } - } - $result = $query->executeQuery(); $row = $result->fetch(); $result->closeCursor(); diff --git a/lib/Service/ParticipantService.php b/lib/Service/ParticipantService.php index 2bd4010fdc1..6dc318ccc8a 100644 --- a/lib/Service/ParticipantService.php +++ b/lib/Service/ParticipantService.php @@ -1188,7 +1188,7 @@ public function getParticipantsByNotificationLevel(Room $room, int $notification $helper = new SelectHelper(); $helper->selectAttendeesTable($query); - $helper->selectSessionsTableMax($query); + $helper->selectSessionsTable($query); $query->from('talk_attendees', 'a') // Currently we only care if the user has a session at all, so we can select any: #ThisIsFine ->leftJoin( @@ -1196,10 +1196,16 @@ public function getParticipantsByNotificationLevel(Room $room, int $notification $query->expr()->eq('s.attendee_id', 'a.id') ) ->where($query->expr()->eq('a.room_id', $query->createNamedParameter($room->getId(), IQueryBuilder::PARAM_INT))) - ->andWhere($query->expr()->eq('a.notification_level', $query->createNamedParameter($notificationLevel, IQueryBuilder::PARAM_INT))) - ->groupBy('a.id'); + ->andWhere($query->expr()->eq('a.notification_level', $query->createNamedParameter($notificationLevel, IQueryBuilder::PARAM_INT))); - return $this->getParticipantsFromQuery($query, $room); + $participants = $this->getParticipantsFromQuery($query, $room); + + $uniqueAttendees = []; + foreach ($participants as $participant) { + $uniqueAttendees[$participant->getAttendee()->getId()] = $participant; + } + + return array_values($uniqueAttendees); } /** diff --git a/tests/php/Service/ParticipantServiceTest.php b/tests/php/Service/ParticipantServiceTest.php new file mode 100644 index 00000000000..98f23a10e19 --- /dev/null +++ b/tests/php/Service/ParticipantServiceTest.php @@ -0,0 +1,152 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\Talk\Tests\php\Service; + +use OCA\Talk\Config; +use OCA\Talk\Federation\Notifications; +use OCA\Talk\Model\Attendee; +use OCA\Talk\Model\AttendeeMapper; +use OCA\Talk\Model\Session; +use OCA\Talk\Model\SessionMapper; +use OCA\Talk\Participant; +use OCA\Talk\Room; +use OCA\Talk\Service\MembershipService; +use OCA\Talk\Service\ParticipantService; +use OCA\Talk\Service\SessionService; +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\ICacheFactory; +use OCP\IConfig; +use OCP\IGroupManager; +use OCP\IUserManager; +use OCP\Security\ISecureRandom; +use PHPUnit\Framework\MockObject\MockObject; +use Test\TestCase; + +/** + * @group DB + */ +class ParticipantServiceTest extends TestCase { + + /** @var IConfig|MockObject */ + protected $serverConfig; + /** @var Config|MockObject */ + protected $talkConfig; + /** @var AttendeeMapper */ + protected $attendeeMapper; + /** @var SessionMapper */ + protected $sessionMapper; + /** @var SessionService|MockObject */ + protected $sessionService; + /** @var ISecureRandom|MockObject */ + protected $secureRandom; + /** @var IEventDispatcher|MockObject */ + protected $dispatcher; + /** @var IUserManager|MockObject */ + protected $userManager; + /** @var IGroupManager|MockObject */ + protected $groupManager; + /** @var MembershipService|MockObject */ + protected $membershipService; + /** @var Notifications|MockObject */ + protected $federationNotifications; + /** @var ITimeFactory|MockObject */ + protected $time; + /** @var ICacheFactory|MockObject */ + protected $cacheFactory; + /** @var ParticipantService */ + private $service; + + + public function setUp(): void { + parent::setUp(); + + $this->serverConfig = $this->createMock(IConfig::class); + $this->talkConfig = $this->createMock(Config::class); + $this->attendeeMapper = new AttendeeMapper(\OC::$server->getDatabaseConnection()); + $this->sessionMapper = new SessionMapper(\OC::$server->getDatabaseConnection()); + $this->sessionService = $this->createMock(SessionService::class); + $this->secureRandom = $this->createMock(ISecureRandom::class); + $this->dispatcher = $this->createMock(IEventDispatcher::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->membershipService = $this->createMock(MembershipService::class); + $this->federationNotifications = $this->createMock(Notifications::class); + $this->time = $this->createMock(ITimeFactory::class); + $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->service = new ParticipantService( + $this->serverConfig, + $this->talkConfig, + $this->attendeeMapper, + $this->sessionMapper, + $this->sessionService, + $this->secureRandom, + \OC::$server->getDatabaseConnection(), + $this->dispatcher, + $this->userManager, + $this->groupManager, + $this->membershipService, + $this->federationNotifications, + $this->time, + $this->cacheFactory + ); + } + + public function tearDown(): void { + try { + $attendee = $this->attendeeMapper->findByActor(123456789, Attendee::ACTOR_USERS, 'test'); + $this->sessionMapper->deleteByAttendeeId($attendee->getId()); + $this->attendeeMapper->delete($attendee); + } catch (DoesNotExistException $exception) { + } + + parent::tearDown(); + } + + public function testGetParticipantsByNotificationLevel(): void { + $attendee = new Attendee(); + $attendee->setActorType(Attendee::ACTOR_USERS); + $attendee->setActorId('test'); + $attendee->setRoomId(123456789); + $attendee->setNotificationLevel(Participant::NOTIFY_MENTION); + $this->attendeeMapper->insert($attendee); + + $session1 = new Session(); + $session1->setAttendeeId($attendee->getId()); + $session1->setSessionId(self::getUniqueID('session1')); + $this->sessionMapper->insert($session1); + + $session2 = new Session(); + $session2->setAttendeeId($attendee->getId()); + $session2->setSessionId(self::getUniqueID('session2')); + $this->sessionMapper->insert($session2); + + $room = $this->createMock(Room::class); + $room->method('getId') + ->willReturn(123456789); + $participants = $this->service->getParticipantsByNotificationLevel($room, Participant::NOTIFY_MENTION); + self::assertCount(1, $participants); + } +}