Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix: Propagate permission changes of federated users to their servers
The frontend gets the permission for the current participant from the
room data (as guest users can not get it from the participant list).
However, permission changes were not propagated to federated servers, so
the frontend of federated users always assumed that the user had the
default permissions.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
  • Loading branch information
danxuliu committed Aug 17, 2024
commit 883998e2443d754732cb445e62f389ee74d26786
2 changes: 2 additions & 0 deletions lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
use OCA\Talk\Federation\Proxy\TalkV1\Notifier\BeforeRoomDeletedListener as TalkV1BeforeRoomDeletedListener;
use OCA\Talk\Federation\Proxy\TalkV1\Notifier\CancelRetryOCMListener as TalkV1CancelRetryOCMListener;
use OCA\Talk\Federation\Proxy\TalkV1\Notifier\MessageSentListener as TalkV1MessageSentListener;
use OCA\Talk\Federation\Proxy\TalkV1\Notifier\ParticipantModifiedListener as TalkV1ParticipantModifiedListener;
use OCA\Talk\Federation\Proxy\TalkV1\Notifier\RoomModifiedListener as TalkV1RoomModifiedListener;
use OCA\Talk\Files\Listener as FilesListener;
use OCA\Talk\Files\TemplateLoader as FilesTemplateLoader;
Expand Down Expand Up @@ -266,6 +267,7 @@ public function register(IRegistrationContext $context): void {

// Federation listeners
$context->registerEventListener(BeforeRoomDeletedEvent::class, TalkV1BeforeRoomDeletedListener::class);
$context->registerEventListener(ParticipantModifiedEvent::class, TalkV1ParticipantModifiedListener::class);
$context->registerEventListener(CallEndedEvent::class, TalkV1RoomModifiedListener::class);
$context->registerEventListener(CallEndedForEveryoneEvent::class, TalkV1RoomModifiedListener::class);
$context->registerEventListener(CallStartedEvent::class, TalkV1RoomModifiedListener::class);
Expand Down
34 changes: 34 additions & 0 deletions lib/Federation/BackendNotifier.php
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,40 @@ public function sendRoomModifiedUpdate(
return $this->sendUpdateToRemote($remote, $notification);
}

/**
* Send information to remote participants that the participant meta info updated
* Sent from Host server to Remote participant server
*/
public function sendParticipantModifiedUpdate(
string $remoteServer,
int $localAttendeeId,
#[SensitiveParameter]
string $accessToken,
string $localToken,
string $changedProperty,
string|int $newValue,
string|int|null $oldValue,
): ?bool {
$remote = $this->prepareRemoteUrl($remoteServer);

$notification = $this->cloudFederationFactory->getCloudFederationNotification();
$notification->setMessage(
FederationManager::NOTIFICATION_PARTICIPANT_MODIFIED,
FederationManager::TALK_ROOM_RESOURCE,
(string) $localAttendeeId,
[
'remoteServerUrl' => $this->getServerRemoteUrl(),
'sharedSecret' => $accessToken,
'remoteToken' => $localToken,
'changedProperty' => $changedProperty,
'newValue' => $newValue,
'oldValue' => $oldValue,
],
);

return $this->sendUpdateToRemote($remote, $notification);
}

/**
* Send information to remote participants that "active since" was updated
* Sent from Host server to Remote participant server
Expand Down
38 changes: 38 additions & 0 deletions lib/Federation/CloudFederationProviderTalk.php
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,8 @@ public function notificationReceived($notificationType, $providerId, array $noti
return $this->shareDeclined((int) $providerId, $notification);
case FederationManager::NOTIFICATION_SHARE_UNSHARED:
return $this->shareUnshared((int) $providerId, $notification);
case FederationManager::NOTIFICATION_PARTICIPANT_MODIFIED:
return $this->participantModified((int) $providerId, $notification);
case FederationManager::NOTIFICATION_ROOM_MODIFIED:
return $this->roomModified((int) $providerId, $notification);
case FederationManager::NOTIFICATION_MESSAGE_POSTED:
Expand Down Expand Up @@ -292,6 +294,42 @@ private function shareUnshared(int $remoteAttendeeId, array $notification): arra
return [];
}

/**
* @param int $remoteAttendeeId
* @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, changedProperty: string, newValue: string|int, oldValue: string|int|null} $notification
* @return array
* @throws ActionNotSupportedException
* @throws AuthenticationFailedException
* @throws ShareNotFound
*/
private function participantModified(int $remoteAttendeeId, array $notification): array {
$invite = $this->getByRemoteAttendeeAndValidate($notification['remoteServerUrl'], $remoteAttendeeId, $notification['sharedSecret']);
try {
$room = $this->manager->getRoomById($invite->getLocalRoomId());
} catch (RoomNotFoundException) {
throw new ShareNotFound(FederationManager::OCM_RESOURCE_NOT_FOUND);
}

// Sanity check to make sure the room is a remote room
if (!$room->isFederatedConversation()) {
throw new ShareNotFound(FederationManager::OCM_RESOURCE_NOT_FOUND);
}

try {
$participant = $this->participantService->getParticipant($room, $invite->getUserId());
} catch (ParticipantNotFoundException $e) {
throw new ShareNotFound(FederationManager::OCM_RESOURCE_NOT_FOUND);
}

if ($notification['changedProperty'] === AParticipantModifiedEvent::PROPERTY_PERMISSIONS) {
$this->participantService->updatePermissions($room, $participant, Attendee::PERMISSIONS_MODIFY_SET, $notification['newValue']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this always updates the permissions of the recipient, independent of which users permissions were actually modified? :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event includes the final permissions, no matter if they were set, added or removed, so the effect should be the same, no? 🤔

} else {
$this->logger->debug('Update of participant property "' . $notification['changedProperty'] . '" is not handled and should not be send via federation');
}

return [];
}

/**
* @param int $remoteAttendeeId
* @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, changedProperty: string, newValue: string|int|bool|null, oldValue: string|int|bool|null, callFlag?: int, dateTime?: string, timerReached?: bool, details?: array<AParticipantModifiedEvent::DETAIL_*, bool>} $notification
Expand Down
1 change: 1 addition & 0 deletions lib/Federation/FederationManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class FederationManager {
public const NOTIFICATION_SHARE_ACCEPTED = 'SHARE_ACCEPTED';
public const NOTIFICATION_SHARE_DECLINED = 'SHARE_DECLINED';
public const NOTIFICATION_SHARE_UNSHARED = 'SHARE_UNSHARED';
public const NOTIFICATION_PARTICIPANT_MODIFIED = 'PARTICIPANT_MODIFIED';
public const NOTIFICATION_ROOM_MODIFIED = 'ROOM_MODIFIED';
public const NOTIFICATION_MESSAGE_POSTED = 'MESSAGE_POSTED';
public const TOKEN_LENGTH = 64;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

declare(strict_types=1);
/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Talk\Federation\Proxy\TalkV1\Notifier;

use OCA\Talk\Events\AAttendeeRemovedEvent;
use OCA\Talk\Events\AParticipantModifiedEvent;
use OCA\Talk\Events\ParticipantModifiedEvent;
use OCA\Talk\Federation\BackendNotifier;
use OCA\Talk\Model\Attendee;
use OCA\Talk\Participant;
use OCA\Talk\Service\ParticipantService;
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventListener;
use OCP\Federation\ICloudId;
use OCP\Federation\ICloudIdManager;

/**
* @template-implements IEventListener<Event>
*/
class ParticipantModifiedListener implements IEventListener {
public function __construct(
protected BackendNotifier $backendNotifier,
protected ParticipantService $participantService,
protected ICloudIdManager $cloudIdManager,
) {
}

public function handle(Event $event): void {
if (!$event instanceof ParticipantModifiedEvent) {
return;
}

if (!in_array($event->getProperty(), [
AParticipantModifiedEvent::PROPERTY_PERMISSIONS,
], true)) {
return;
}

$participants = $this->participantService->getParticipantsByActorType($event->getRoom(), Attendee::ACTOR_FEDERATED_USERS);
foreach ($participants as $participant) {
$cloudId = $this->cloudIdManager->resolveCloudId($participant->getAttendee()->getActorId());

$success = $this->notifyParticipantModified($cloudId, $participant, $event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be sent to the instance of the user that is modified.
We do not tell non-moderators the permissions of other users?!


if ($success === null) {
$this->participantService->removeAttendee($event->getRoom(), $participant, AAttendeeRemovedEvent::REASON_LEFT);
}
}
}

private function notifyParticipantModified(ICloudId $cloudId, Participant $participant, AParticipantModifiedEvent $event) {
return $this->backendNotifier->sendParticipantModifiedUpdate(
$cloudId->getRemote(),
$participant->getAttendee()->getId(),
$participant->getAttendee()->getAccessToken(),
$event->getRoom()->getToken(),
$event->getProperty(),
$event->getNewValue(),
$event->getOldValue(),
);
}
}
9 changes: 9 additions & 0 deletions tests/integration/features/bootstrap/FeatureContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,9 @@ private function assertRooms(array $rooms, TableNode $formData, bool $shouldOrde
if (isset($expectedRoom['recordingConsent'])) {
$data['recordingConsent'] = (int) $room['recordingConsent'];
}
if (isset($expectedRoom['permissions'])) {
$data['permissions'] = $this->mapPermissionsAPIOutput($room['permissions']);
}
if (isset($expectedRoom['participants'])) {
throw new \Exception('participants key needs to be checked via participants endpoint');
}
Expand Down Expand Up @@ -2004,6 +2007,12 @@ public function userSetsPermissionsForInRoomTo(string $user, string $participant
} elseif (strpos($participant, 'guest') === 0) {
$sessionId = self::$userToSessionId[$participant];
$attendeeId = $this->getAttendeeId('guests', sha1($sessionId), $identifier, $statusCode === 200 ? $user : null);
} elseif (str_ends_with($participant, '@{$LOCAL_REMOTE_URL}') ||
str_ends_with($participant, '@{$REMOTE_URL}')) {
$participant = str_replace('{$LOCAL_REMOTE_URL}', rtrim($this->localRemoteServerUrl, '/'), $participant);
$participant = str_replace('{$REMOTE_URL}', rtrim($this->remoteServerUrl, '/'), $participant);

$attendeeId = $this->getAttendeeId('federated_users', $participant, $identifier, $statusCode === 200 ? $user : null);
} else {
$attendeeId = $this->getAttendeeId('users', $participant, $identifier, $statusCode === 200 ? $user : null);
}
Expand Down
23 changes: 23 additions & 0 deletions tests/integration/features/federation/permissions.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
Feature: federation/permissions

Background:
Given user "participant1" exists
And user "participant2" exists
And the following "spreed" app config is set
| federation_enabled | yes |

Scenario: set participant permissions
Given user "participant1" creates room "room" (v4)
| roomType | 2 |
| roomName | room name |
And user "participant1" adds federated_user "participant2" to room "room" with 200 (v4)
And user "participant2" has the following invitations (v1)
| remoteServerUrl | remoteToken | state | inviterCloudId | inviterDisplayName |
| LOCAL | room | 0 | participant1@http://localhost:8080 | participant1-displayname |
And user "participant2" accepts invite to room "room" of server "LOCAL" with 200 (v1)
| id | name | type | remoteServer | remoteToken |
| LOCAL::room | room name | 2 | LOCAL | room |
When user "participant1" sets permissions for "participant2@{$LOCAL_REMOTE_URL}" in room "room" to "S" with 200 (v4)
Then user "participant2" is participant of room "LOCAL::room" (v4)
| permissions |
| CS |