fix(contactsMenu): Attach user cloud to each contact entry #44954

nfebe · 2024-04-22T08:44:24Z

The contact entry should carry cloud information, this can help to fix various issues including:

Displaying correct avatar links for federated users

This can also lead to UI improvements in the frontend where, it's federated contacts are shown with a marker or indicator on the UI.

Resolves: [Bug]: Broken avatar placeholders for federated users in new search #44319

Summary

TODO

...

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

lib/private/Contacts/ContactsMenu/Entry.php

+		$this->cloud = $cloudId;
+	}
+
+	public function getCloud(): CloudId {


lib/private/Contacts/ContactsMenu/Entry.php

+	}
+
+	public function getCloud(): CloudId {
+		return $this->cloud;


nfebe

To solve this issue we could simple use the isUser property of the contact along side isNouser boolean of NcAvatar to show the text generated icons.

However, here:

I instead try to link to the actual avatars on the remote server, since the links public
I am adding the cloud data to be passed to the frontend

Problems:

This code is able to load the avatars from a federated server but would like to "Content Security Policy" errors if the remote server is unsecured.

There are other places in the code base such as here where the current approach is used but we could swap if this attempt to directly load the avatars from the federated server is successful/useful.

@nickvergessen since you are familiar with section of the code, does this approach make sense?

The contact entry should carry cloud information, this can help to fix various issues including: - Displaying correct avatar links for federated users This can also lead to UI improvements in the frontend where, it's federated contacts are shown with a marker or indicator on the UI. Signed-off-by: fenn-cs <[email protected]>

lib/private/Contacts/ContactsMenu/ContactsStore.php

-			} elseif (!empty($contact['FN'])) {
-				$avatar = $this->urlGenerator->linkToRouteAbsolute('core.GuestAvatar.getAvatar', ['guestName' => str_replace('/', ' ', $contact['FN']), 'size' => 64]);
+			} elseif ($username != '') {
+				$avatar = $this->urlGenerator->linkToRemoteRouteAbsolute($remoteServer, 'core.avatar.getAvatar', ['userId' => str_replace('/', ' ', $username), 'size' => 64]);


lib/private/Contacts/ContactsMenu/ContactsStore.php

 			} else {
-				$avatar = $this->urlGenerator->linkToRouteAbsolute('core.GuestAvatar.getAvatar', ['guestName' => str_replace('/', ' ', $uid), 'size' => 64]);
+				$avatar = $this->urlGenerator->linkToRemoteRouteAbsolute($remoteServer, 'core.avatar.getAvatar', ['userId' => str_replace('/', ' ', $uid), 'size' => 64]);


lib/private/Contacts/ContactsMenu/Entry.php


 	public function setId(string $id): void {
 		$this->id = $id;
 	}

+	public function getId(): string {


lib/private/Contacts/ContactsMenu/Entry.php


 	public function setId(string $id): void {
 		$this->id = $id;
 	}

+	public function getId(): string {
+		return $this->id;


lib/private/URLGenerator.php

@@ -115,6 +115,19 @@
 		return $this->getAbsoluteURL($this->linkToRoute($routeName, $arguments));
 	}

+
+	public function linkToRemoteRouteAbsolute(string $remote, $routeName, array $arguments = []): string {


lib/private/URLGenerator.php

@@ -115,6 +115,19 @@
 		return $this->getAbsoluteURL($this->linkToRoute($routeName, $arguments));
 	}

+
+	public function linkToRemoteRouteAbsolute(string $remote, $routeName, array $arguments = []): string {
+		return $this->formatAsUrl($remote, $this->linkToRoute($routeName, $arguments));


nickvergessen · 2024-04-22T17:23:16Z

lib/private/Contacts/ContactsMenu/ContactsStore.php

+			$remoteServer = '';
+
+			if (isset($contact['CLOUD']) && is_array($contact['CLOUD']) && isset($contact['CLOUD'][0])) {
+				preg_match('/^(.*?)@(https?:\/\/.*?)$/', $contact['CLOUD'][0], $matches);


Please use the ICloudManager::resolve() method instead?

nickvergessen · 2024-04-22T17:25:46Z

lib/private/Contacts/ContactsMenu/Entry.php

 	/**
-	 * @return array{id: int|string|null, fullName: string, avatar: string|null, topAction: mixed, actions: array, lastMessage: '', emailAddresses: string[], profileTitle: string|null, profileUrl: string|null, status: string|null, statusMessage: null|string, statusMessageTimestamp: null|int, statusIcon: null|string, isUser: bool, uid: mixed}
+	 * @return array{id: int|string|null, fullName: string, avatar: string|null, topAction: mixed, actions: array, lastMessage: '', emailAddresses: string[], profileTitle: string|null, profileUrl: string|null, status: string|null, statusMessage: null|string, statusMessageTimestamp: null|int, statusIcon: null|string, isUser: bool, uid: mixed, cloud: mixed}


cloud should be called cloudId to be more clear and the type should be the array you define in the other file

nickvergessen · 2024-04-22T17:28:14Z

lib/private/Federation/CloudId.php

+	/**
+	 * @return array{id: string, user: string, remote: string, displayName: string|null}
+	 */
+	public function jsonSerialize(): array {


Should implement the interface JsonSerializable?

That being said, should be on the public interface?

nickvergessen · 2024-04-22T17:31:30Z

lib/private/Contacts/ContactsMenu/ContactsStore.php

-			} elseif (!empty($contact['FN'])) {
-				$avatar = $this->urlGenerator->linkToRouteAbsolute('core.GuestAvatar.getAvatar', ['guestName' => str_replace('/', ' ', $contact['FN']), 'size' => 64]);
+			} elseif ($username != '') {
+				$avatar = $this->urlGenerator->linkToRemoteRouteAbsolute($remoteServer, 'core.avatar.getAvatar', ['userId' => str_replace('/', ' ', $username), 'size' => 64]);


This assumes every cloud ID is a nextcloud? 🫣

nickvergessen · 2024-04-22T17:31:57Z

lib/private/URLGenerator.php

@@ -115,6 +115,19 @@ public function linkToRouteAbsolute(string $routeName, array $arguments = []): s
 		return $this->getAbsoluteURL($this->linkToRoute($routeName, $arguments));
 	}

+
+	public function linkToRemoteRouteAbsolute(string $remote, $routeName, array $arguments = []): string {


This should not be here or on the public interface

nickvergessen · 2024-04-22T17:34:30Z

does this approach make sense

From my POV it's not okay...

nfebe · 2024-04-22T19:48:36Z

Based on conversation with @nickvergessen, this needs more effort especially as the content security policy can't changed to allow all trusted servers.

So this would remain in draft in favor of : #44972

Eventually, I could copy the proxy strategy from https://github.com/nextcloud/spreed/blob/main/lib/Controller/AvatarController.php#L244 where the server downloads the image and sends in on the instance URL.

nfebe added 2. developing Work in progress feature: search feature: contacts menu labels Apr 22, 2024

github-advanced-security bot found potential problems Apr 22, 2024

View reviewed changes

lib/private/Contacts/ContactsMenu/Entry.php Fixed Show fixed Hide fixed

lib/private/Contacts/ContactsMenu/Entry.php Fixed Show fixed Hide fixed

lib/private/Contacts/ContactsMenu/Entry.php Fixed Show fixed Hide fixed

lib/private/Contacts/ContactsMenu/Entry.php Fixed Show fixed Hide fixed

nfebe force-pushed the 44319-fix-fed-share-user-avatars branch 2 times, most recently from 70f529c to fe83f91 Compare April 22, 2024 09:08

github-advanced-security bot found potential problems Apr 22, 2024

View reviewed changes

nfebe force-pushed the 44319-fix-fed-share-user-avatars branch from fe83f91 to 8640581 Compare April 22, 2024 15:34

nfebe commented Apr 22, 2024

View reviewed changes

nfebe force-pushed the 44319-fix-fed-share-user-avatars branch from 8640581 to 2b1c1ab Compare April 22, 2024 15:43

nfebe force-pushed the 44319-fix-fed-share-user-avatars branch from 2b1c1ab to 990d244 Compare April 22, 2024 15:46

github-advanced-security bot found potential problems Apr 22, 2024

View reviewed changes

nickvergessen requested changes Apr 22, 2024

View reviewed changes

nfebe added descoped and removed 2. developing Work in progress labels Apr 22, 2024

solracsf added this to the Nextcloud 30 milestone Jun 18, 2024

skjnldsv added the 2. developing Work in progress label Jul 27, 2024

skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024

susnux added stale Ticket or PR with no recent activity and removed descoped labels Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(contactsMenu): Attach user cloud to each contact entry #44954

fix(contactsMenu): Attach user cloud to each contact entry #44954

nfebe commented Apr 22, 2024

nfebe left a comment

nickvergessen Apr 22, 2024

nickvergessen Apr 22, 2024

nickvergessen Apr 22, 2024

nickvergessen Apr 22, 2024

nickvergessen Apr 22, 2024

nickvergessen Apr 22, 2024

nickvergessen commented Apr 22, 2024

nfebe commented Apr 22, 2024

fix(contactsMenu): Attach user cloud to each contact entry #44954

Are you sure you want to change the base?

fix(contactsMenu): Attach user cloud to each contact entry #44954

Conversation

nfebe commented Apr 22, 2024

Summary

TODO

Checklist

nfebe left a comment

Choose a reason for hiding this comment

nickvergessen Apr 22, 2024

Choose a reason for hiding this comment

nickvergessen Apr 22, 2024

Choose a reason for hiding this comment

nickvergessen Apr 22, 2024

Choose a reason for hiding this comment

nickvergessen Apr 22, 2024

Choose a reason for hiding this comment

nickvergessen Apr 22, 2024

Choose a reason for hiding this comment

nickvergessen Apr 22, 2024

Choose a reason for hiding this comment

nickvergessen commented Apr 22, 2024

nfebe commented Apr 22, 2024