-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(contactsMenu): Attach user cloud to each contact entry #44954
base: stable29
Are you sure you want to change the base?
Conversation
70f529c
to
fe83f91
Compare
$this->cloud = $cloudId; | ||
} | ||
|
||
public function getCloud(): CloudId { |
Check failure
Code scanning / Psalm
InvalidNullableReturnType Error
} | ||
|
||
public function getCloud(): CloudId { | ||
return $this->cloud; |
Check failure
Code scanning / Psalm
NullableReturnStatement Error
fe83f91
to
8640581
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
8640581
to
2b1c1ab
Compare
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 <fenn25.fn@gmail.com>
2b1c1ab
to
990d244
Compare
} 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]); |
Check failure
Code scanning / Psalm
UndefinedInterfaceMethod Error
} 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]); |
Check failure
Code scanning / Psalm
UndefinedInterfaceMethod Error
|
||
public function setId(string $id): void { | ||
$this->id = $id; | ||
} | ||
|
||
public function getId(): string { |
Check failure
Code scanning / Psalm
InvalidNullableReturnType Error
|
||
public function setId(string $id): void { | ||
$this->id = $id; | ||
} | ||
|
||
public function getId(): string { | ||
return $this->id; |
Check failure
Code scanning / Psalm
NullableReturnStatement Error
@@ -115,6 +115,19 @@ | |||
return $this->getAbsoluteURL($this->linkToRoute($routeName, $arguments)); | |||
} | |||
|
|||
|
|||
public function linkToRemoteRouteAbsolute(string $remote, $routeName, array $arguments = []): string { |
Check failure
Code scanning / Psalm
InvalidNullableReturnType Error
@@ -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)); |
Check failure
Code scanning / Psalm
NullableReturnStatement Error
$remoteServer = ''; | ||
|
||
if (isset($contact['CLOUD']) && is_array($contact['CLOUD']) && isset($contact['CLOUD'][0])) { | ||
preg_match('/^(.*?)@(https?:\/\/.*?)$/', $contact['CLOUD'][0], $matches); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the ICloudManager::resolve()
method instead?
/** | ||
* @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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cloud should be called cloudId to be more clear and the type should be the array you define in the other file
/** | ||
* @return array{id: string, user: string, remote: string, displayName: string|null} | ||
*/ | ||
public function jsonSerialize(): array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should implement the interface JsonSerializable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, should be on the public interface?
} 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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes every cloud ID is a nextcloud? 🫣
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be here or on the public interface
From my POV it's not okay... |
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. |
The contact entry should carry cloud information, this can help to fix various issues including:
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.
Summary
TODO
Checklist