Add support for federated calls #12668

danxuliu · 2024-07-10T13:52:30Z

Requires #12604

Permissions

The default permissions of the conversation are not propagated to the federated conversation. However, that would be relevant only for moderators (and currently federated users can not be moderator) when showing or modifying the current default permissions. The actual permissions used by participants, although affected by the default permissions, are got directly from the participant data.

Moreover, even if the default permissions are not propagated, when the permissions of a participant are modified in any way the remote Nextcloud server triggers a participants->update signaling message that now also includes federated users. So, to sum up, changes in participant permissions for federated users can be handled just like for regular users.

TODO:

lib/Chat/SystemMessage/Listener.php

lib/Controller/CallController.php

nickvergessen · 2024-07-24T17:59:30Z

lib/Events/ARoomModifiedEvent.php

 use OCA\Talk\Room;

 abstract class ARoomModifiedEvent extends ARoomEvent {
+	public const PROPERTY_ACTIVE_SINCE = 'activeSince';


This is handled by other logic when inCall is toggled to or from 0
Now it would send 2 remote requests? Maybe it would be useful to create a special case like with the lobby (which has the lobbytimer in addition)

Now the ActiveSinceModifiedEvent includes the call flags too. If only the call flags are modified, without touching "active since", a RoomModifiedEvent for the in_call property is triggered instead.

nickvergessen · 2024-07-24T18:02:38Z

lib/Federation/CloudFederationProviderTalk.php

+				$this->roomService->resetActiveSince($room);
+			} else {
+				$activeSince = \DateTime::createFromFormat('U', $notification['newValue']);
+				$this->roomService->setActiveSince($room, $activeSince, $room->getCallFlag(), false);


this will not work, depending on the order the requests arrive or when they arrive in parallel

But doesn't all properties have the same problem if they are quickly modified?

Or do you mean regarding sending a request for "active since" and then another one for "in call" both for the same action of modifying the "in call" status? In that case now there is a single request when a call is started which provides the value of both "active since" and "in call".

Or do you mean regarding sending a request for "active since" and then another one for "in call" both for the same action of modifying the "in call" status?

yes

But doesn't all properties have the same problem if they are quickly modified?

well the case of activesince+callflag is the only one apart from lobby that touches multiple at the same time and they need to come in the same event request

As described above now the ActiveSinceModifiedEvent includes bot the active since and the call flags.

I noticed a corner case behaviour, though: if some notification is missed for some reason and a newer notification arrives, when the old notification is retried it could wipe the newer value. But that is unrelated to the changes here and more a general problem, so I will open an issue about that.

lib/Federation/Proxy/TalkV1/Controller/CallController.php

nickvergessen · 2024-07-24T18:10:17Z

lib/Signaling/BackendNotifier.php

-			if ($attendee->getActorType() === Attendee::ACTOR_USERS) {
+			if ($attendee->getActorType() === Attendee::ACTOR_USERS
+					|| $attendee->getActorType() === Attendee::ACTOR_FEDERATED_USERS) {
 				$data['userId'] = $attendee->getActorId();


This could be problematic as there could be a conflict?
Is it required by the HPB, or could it be either a new attribute $data['cloudId'] or dedicated actor type+id fields?

I assumed that the actorId of a federated user always includes the @remote part (based on what I saw in my test instances) 🤔

If they do not then 7056338 will need to be fixed, as that one affects how receives the signaling messages (the change here is only about the data sent and probably a different field could be used if needed).

I assumed that the actorId of a federated user always includes the @Remote part (based on what I saw in my test instances)

They do, but it can still conflict. UserA@ServerA could also be a valid email address that they received as user ID on ServerB when being invited via the guest app.

OK, something for a follow up then.

nickvergessen · 2024-07-24T18:10:36Z

lib/Signaling/BackendNotifier.php

-				if ($attendee->getActorType() === Attendee::ACTOR_USERS) {
+				if ($attendee->getActorType() === Attendee::ACTOR_USERS
+						|| $attendee->getActorType() === Attendee::ACTOR_FEDERATED_USERS) {
 					$data['userId'] = $attendee->getActorId();


same problem here

nickvergessen · 2024-07-25T09:10:07Z

lib/Activity/Listener.php

 			$this->timeFactory->getDateTime(),
-			$participant->getSession() ? $participant->getSession()->getInCall() : Participant::FLAG_DISCONNECTED,
-			$participant->getAttendee()->getActorType() !== Attendee::ACTOR_USERS
+			$participant->getSession() ? $participant->getSession()->getInCall() : Participant::FLAG_DISCONNECTED


Suggested change

$participant->getSession() ? $participant->getSession()->getInCall() : Participant::FLAG_DISCONNECTED

$participant->getSession() ? $participant->getSession()->getInCall() : Participant::FLAG_DISCONNECTED,

$participant->getAttendee()->getActorType() === Attendee::ACTOR_GUESTS

To keep writing the data for now, while we fix reading it

Guests are counted from the attendees instead of from the active_guests column for several years already, according to the PR:
Make the call summary reuse the attendee table also for guests, so we don't have "and 132 guests" when a guest rejoins with a weak internet connection.

Due to that and to either avoid propagating it for consistency, or not propagating it which would work for now if the field is used only for the call summary but could cause unexpected inconsistencies in the future if it ends used for something else, I kept it removed and did not change it.

And if it needs to be introduced back later... we can always do it anyway :-)

nickvergessen · 2024-07-25T09:40:46Z

lib/Manager.php

 			'avatar' => '',
 			'remote_server' => '',
 			'remote_token' => '',
-			'active_guests' => 0,


Kept removed as explained above.

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

Signed-off-by: Joas Schilling <[email protected]>

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

"In call" status is set from two different properties, "active since" and "call flag". Call flags can be modified independently, but "active since" is always linked to setting call flags, so the event needs to include both properties. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

In federated conversations the recording consent is got directly from the host room (through the propagated property), no matter the global configuration for recording consent in the federated instance. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The federated users are now included in the "inCall" and "participants" notfications sent from the Nextcloud server to the signaling server, so their data is also included in the "participants->update" events sent from the signaling server to the clients. Otherwise the clients would not be notified when the properties of a federated user change, not even for their own participant. Note that the "participants->update" is also received by federated clients when the default permissions of the room change, even if the default permissions themselves are not propagated to the federated room, as the change in the default permissions also cause a change in the participant permissions. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

src/components/TopBar/CallButton.vue

As federated users can now join calls the setting to show the media settings or not before joining a call is also shown for federated conversations. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

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

This should be properly fixed... but I have not found a way to do it :-( Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Antreesy

See nothing breaking here, but mostly Yolo-ing

Already addressed or will be in follow-ups

danxuliu added 2. developing enhancement feature: call 📹 Voice and video calls labels Jul 10, 2024

danxuliu added this to the 💙 Next Major (30) milestone Jul 10, 2024

danxuliu force-pushed the add-support-for-federated-calls branch 3 times, most recently from d0c9b8e to 6aff0e1 Compare July 18, 2024 15:12

nickvergessen assigned danxuliu Jul 23, 2024

nickvergessen added feature: federation 🌐 feature: api 🛠️ OCS API for conversations, chats and participants feature: frontend 🖌️ "Web UI" client client: 🤖🍏 mobile labels Jul 23, 2024

danxuliu force-pushed the add-support-for-federated-calls branch 2 times, most recently from 7a14f53 to 53ca371 Compare July 24, 2024 16:21

nickvergessen previously requested changes Jul 24, 2024

View reviewed changes

danxuliu force-pushed the add-support-for-federated-calls branch from 53ca371 to 37431a9 Compare July 25, 2024 07:47

nickvergessen reviewed Jul 25, 2024

View reviewed changes

danxuliu force-pushed the add-support-for-federated-calls branch from 7850718 to e6bca6e Compare July 25, 2024 17:56

refactor: Extract method to validate recording consent

37da3b7

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

danxuliu force-pushed the add-support-for-federated-calls branch from fc6ed23 to d4d9809 Compare July 25, 2024 21:15

nickvergessen and others added 6 commits July 25, 2024 23:32

feat: Add annotation to load a federated user from the given session

8fa7819

Signed-off-by: Joas Schilling <[email protected]>

feat: Join and leave calls from federated conversations

e3f3146

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

fix: Remove unused "activeGuests" attribute

9df96cd

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

danxuliu force-pushed the add-support-for-federated-calls branch from d4d9809 to 1b65c8b Compare July 25, 2024 21:40

danxuliu added 3. to review and removed 2. developing labels Jul 26, 2024

danxuliu marked this pull request as ready for review July 26, 2024 07:11

Antreesy reviewed Jul 26, 2024

View reviewed changes

src/components/TopBar/CallButton.vue Show resolved Hide resolved

danxuliu added 4 commits July 26, 2024 11:46

feat: Show call button to federated users

1163e73

As federated users can now join calls the setting to show the media settings or not before joining a call is also shown for federated conversations. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

feat: Update call flags by federated participants

9b526a0

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

chore: Update OpenAPI

e264e82

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

chore: Suppress Psalm issue

13f092a

This should be properly fixed... but I have not found a way to do it :-( Signed-off-by: Daniel Calviño Sánchez <[email protected]>

danxuliu force-pushed the add-support-for-federated-calls branch from 1b65c8b to 13f092a Compare July 26, 2024 09:51

danxuliu enabled auto-merge July 26, 2024 09:56

Antreesy approved these changes Jul 26, 2024

View reviewed changes

danxuliu merged commit 02eb9c2 into main Jul 26, 2024

danxuliu deleted the add-support-for-federated-calls branch July 26, 2024 10:55

danxuliu mentioned this pull request Jul 31, 2024

Fix avatars in federated calls #12863

Merged

	$participant->getSession() ? $participant->getSession()->getInCall() : Participant::FLAG_DISCONNECTED
	$participant->getSession() ? $participant->getSession()->getInCall() : Participant::FLAG_DISCONNECTED,
	$participant->getAttendee()->getActorType() === Attendee::ACTOR_GUESTS

Add support for federated calls #12668

Add support for federated calls #12668

Uh oh!

Conversation

danxuliu commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Permissions

TODO:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Antreesy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danxuliu commented Jul 10, 2024 •

edited

Loading