Propagate permission changes to federated servers #12979

danxuliu · 2024-08-15T06:53:35Z

Fix #12953
Requires (for the integration tests to pass) #13027

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 participant permissions.

However, note that media related permissions (audio, video and screenshare) were correctly handled (at least in the WebUI), as when the permissions are modified a participants->update signaling message is sent and the WebUI gets the media permissions directly from that signaling message rather than from the room data.

Independently of that, note that it is not enough to just propagate participant permissions, room permissions and even general default permissions need to be propagated as well (but default permissions currently are not, see below), as when there are no custom permissions for a participant it falls back to the room permissions.

TODO

Fix propagating permissions when changed in the conversation
Currently this pull request propagates participant and room permissions, but not the default permissions in the host server. Those are just read from a configuration value, and as far as I know there is no event to listen to in case it changes. I do not know how to propagate those, maybe through capabilities?
For simplicity the resource type TALK_ROOM_RESOURCE and, therefore, the CloudFederationProviderTalk were reused for participant related notifications. Should a new TALK_PARTICIPANT_RESOURCE and provider be added instead?

How to test (scenario 1)

Setup Nextcloud server A
Setup Nextcloud server B
In Nextcloud server A, create a conversation
Add a user from Nextcloud server B
Set custom permissions for the federated user and disable starting a call and sending chat messages
In a private window, log in as the federated user
Accept the invitation
Open the federated conversation

Result with this pull request

The new message area and the "Start call" button are disabled

Result without this pull request

Neither the new message area nor the "Start call" button are disabled

How to test (scenario 2)

Setup Nextcloud server A
Setup Nextcloud server B
In Nextcloud server A, create a conversation
Set custom permissions for the conversation and disable starting a call and sending chat messages
Add a user from Nextcloud server B
In a private window, log in as the federated user
Accept the invitation
Open the federated conversation

Result with this pull request

The new message area and the "Start call" button are disabled

Result without this pull request

Neither the new message area nor the "Start call" button are disabled

nickvergessen · 2024-08-15T07:23:02Z

/backport to stable30

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]>

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

When a participant does not have custom permissions the permissions are got from the call or conversation, so they need to be propagated as well to the federated servers to correctly calculate the participant permissions. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Besides propagating the permissions to federated servers when modified the existing permissions need to be set when creating the federated conversation (or if a federated user is added again to the conversation when all the previous federated users left it already). Signed-off-by: Daniel Calviño Sánchez <[email protected]>

lib/Events/BeforePermissionsModifiedEvent.php

nickvergessen · 2024-08-20T15:38:18Z

lib/Federation/BackendNotifier.php

+		$protocol['roomCallPermissions'] = $roomCallPermissions;
+		$protocol['roomDefaultPermissions'] = $roomDefaultPermissions;


And description, and call state, and, …

No this doesn't scale.
And as said, we remove call permissions, please don't add it

And description, and call state, and, …

No this doesn't scale.

Well, then some other approach is needed. But the initial state needs to be propagated somehow.

And as said, we remove call permissions, please don't add it

This code was written before it was decided that call permissions were going to be removed. Anyway, I will redo it and rebase once call permissions are removed in main and stable30.

nickvergessen · 2024-08-20T15:39:11Z

lib/Federation/CloudFederationProviderTalk.php

@@ -125,6 +125,8 @@ public function shareReceived(ICloudFederationShare $share): string {
 		$remoteId = $share->getProviderId();
 		$roomToken = $share->getResourceName();
 		$roomName = $share->getProtocol()['roomName'];
+		$roomCallPermissions = $share->getProtocol()['roomCallPermissions'];


Suggested change

$roomCallPermissions = $share->getProtocol()['roomCallPermissions'];

lib/Federation/CloudFederationProviderTalk.php

nickvergessen · 2024-08-20T15:40:18Z

lib/Federation/FederationManager.php

@@ -74,6 +77,8 @@ public function addRemoteRoom(
 		int $remoteAttendeeId,
 		int $roomType,
 		string $roomName,
+		int $roomCallPermissions,


Suggested change

int $roomCallPermissions,

nickvergessen · 2024-08-20T15:44:46Z

tests/php/Federation/FederationTest.php

@@ -288,7 +292,7 @@ public function testReceiveRemoteShare(): void {
 		// Test receiving federation expectations
 		$this->federationManager->expects($this->once())
 			->method('addRemoteRoom')
-			->with($shareWithUser, $providerId, $roomType, $roomName, $name, $remote, $token)
+			->with($shareWithUser, $providerId, $roomType, $roomName, $roomCallPermissions, $roomDefaultPermissions, $name, $remote, $token)


Suggested change

->with($shareWithUser, $providerId, $roomType, $roomName, $roomCallPermissions, $roomDefaultPermissions, $name, $remote, $token)

->with($shareWithUser, $providerId, $roomType, $roomName, $roomDefaultPermissions, $name, $remote, $token)

nickvergessen · 2024-08-20T15:46:02Z

lib/Federation/Proxy/TalkV1/Notifier/ParticipantModifiedListener.php

+		foreach ($participants as $participant) {
+			$cloudId = $this->cloudIdManager->resolveCloudId($participant->getAttendee()->getActorId());
+
+			$success = $this->notifyParticipantModified($cloudId, $participant, $event);


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?!

nickvergessen · 2024-08-20T15:46:46Z

lib/Federation/BackendNotifier.php

@@ -69,6 +69,8 @@ public function sendRemoteShare(
 		$roomName = $room->getName();
 		$roomType = $room->getType();
 		$roomToken = $room->getToken();
+		$roomCallPermissions = $room->getCallPermissions();


Suggested change

$roomCallPermissions = $room->getCallPermissions();

nickvergessen · 2024-08-20T15:46:57Z

lib/Federation/BackendNotifier.php

@@ -101,6 +103,8 @@ public function sendRemoteShare(
 		$protocol['invitedCloudId'] = $invitedCloudId->getId();
 		$protocol['roomName'] = $roomName;
 		$protocol['roomType'] = $roomType;
+		$protocol['roomCallPermissions'] = $roomCallPermissions;


Suggested change

$protocol['roomCallPermissions'] = $roomCallPermissions;

nickvergessen · 2024-08-20T15:48:29Z

lib/Federation/CloudFederationProviderTalk.php

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


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

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

danxuliu · 2024-08-23T14:49:19Z

Replaced by #13092

danxuliu added 2. developing bug feature: federation 🌐 labels Aug 15, 2024

danxuliu added this to the 💙 Next RC (30) milestone Aug 15, 2024

nickvergessen modified the milestones: 💙 Next RC (30), 🖤 Next Major (31) Aug 15, 2024

backportbot bot added the backport-request label Aug 15, 2024

danxuliu force-pushed the propagate-permission-changes-to-federated-servers branch from 1758178 to e6db057 Compare August 17, 2024 23:59

danxuliu added 3 commits August 18, 2024 02:29

refactor: Include additional context in room permission change events

12290ea

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

danxuliu force-pushed the propagate-permission-changes-to-federated-servers branch from e6db057 to f0d8036 Compare August 18, 2024 00:30

nickvergessen requested changes Aug 20, 2024

View reviewed changes

This was referenced Aug 21, 2024

fix(federation): Sync room properties on join #13072

Merged

fix(federation): Propagate permission changes to federated servers #13092

Merged

danxuliu closed this Aug 23, 2024

danxuliu deleted the propagate-permission-changes-to-federated-servers branch August 23, 2024 14:49

nickvergessen removed the backport-request label Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate permission changes to federated servers #12979

Propagate permission changes to federated servers #12979

danxuliu commented Aug 15, 2024 •

edited

Loading

nickvergessen commented Aug 15, 2024

nickvergessen Aug 20, 2024

danxuliu Aug 21, 2024

nickvergessen Aug 20, 2024

nickvergessen Aug 20, 2024

nickvergessen Aug 20, 2024

nickvergessen Aug 20, 2024

nickvergessen Aug 20, 2024

nickvergessen Aug 20, 2024

nickvergessen Aug 20, 2024

danxuliu Aug 21, 2024

danxuliu commented Aug 23, 2024

		$protocol['roomCallPermissions'] = $roomCallPermissions;
		$protocol['roomDefaultPermissions'] = $roomDefaultPermissions;

	->with($shareWithUser, $providerId, $roomType, $roomName, $roomCallPermissions, $roomDefaultPermissions, $name, $remote, $token)
	->with($shareWithUser, $providerId, $roomType, $roomName, $roomDefaultPermissions, $name, $remote, $token)

Propagate permission changes to federated servers #12979

Propagate permission changes to federated servers #12979

Conversation

danxuliu commented Aug 15, 2024 • edited Loading

TODO

How to test (scenario 1)

Result with this pull request

Result without this pull request

How to test (scenario 2)

Result with this pull request

Result without this pull request

nickvergessen commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danxuliu commented Aug 23, 2024

danxuliu commented Aug 15, 2024 •

edited

Loading