feat(sharing): allow email-based labels for lookup results#60375
Conversation
When the new system config 'shareapi_lookup_label_show_email' is set to true, the LookupPlugin composes suggestion labels with the user's email (from the lookup server response) instead of the federation ID. Default behavior is strictly preserved when the flag is absent or false. Motivation: in Global Scale deployments, federation IDs carry sharded identity that is not human-readable (e.g. 'user@node05.example.org'). Operators can opt-in to email-based labels to improve UX without affecting other deployments. Signed-off-by: niv <nicolas.varlot@ac-versailles.fr>
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in system configuration flag (shareapi_lookup_label_show_email) to change how lookup-server share suggestions are labeled in Global Scale setups: when enabled and the lookup response contains an email, the suggestion label shows Name (email) instead of Name (federationId), while keeping the federation ID as the actual share target.
Changes:
- Read new system config flag
shareapi_lookup_label_show_emailinLookupPluginand use lookup-responseemailfor the suggestion label when enabled. - Update existing
LookupPluginTestexpectations for the additional config read. - Add a new unit test covering the “email label” path when the flag is enabled and email is present.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/private/Collaboration/Collaborators/LookupPlugin.php | Adds config-driven logic to compose lookup suggestion labels using email when available. |
| tests/lib/Collaboration/Collaborators/LookupPluginTest.php | Updates mocks for the new config read and adds a new test for the email-label behavior. |
Comments suppressed due to low confidence (1)
lib/private/Collaboration/Collaborators/LookupPlugin.php:47
- This config read happens even when Global Scale is disabled (the method returns early right after), so it adds an unnecessary system-config lookup on every non-GS search call. Consider moving the
shareapi_lookup_label_show_emailread to after the!$isGlobalScaleEnabledearly-return to avoid extra work in the common non-GS case.
$isGlobalScaleEnabled = $this->config->getSystemValueBool('gs.enabled', false);
$isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes';
$hasInternetConnection = $this->config->getSystemValueBool('has_internet_connection', true);
$showFederatedEmail = $this->config->getSystemValueBool('shareapi_lookup_label_show_email', false);
// If case of Global Scale we always search the lookup server
// TODO: Reconsider using the lookup server for non-global scale
// if (!$isGlobalScaleEnabled && (!$isLookupServerEnabled || !$hasInternetConnection || $disableLookupServer)) {
if (!$isGlobalScaleEnabled) {
return false;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $isGlobalScaleEnabled = $this->config->getSystemValueBool('gs.enabled', false); | ||
| $isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes'; | ||
| $hasInternetConnection = $this->config->getSystemValueBool('has_internet_connection', true); | ||
| $showFederatedEmail = $this->config->getSystemValueBool('shareapi_lookup_label_show_email', false); |
| $email = $lookup['email']['value'] ?? ''; | ||
|
|
||
| if ($showFederatedEmail && !empty($email)) { | ||
| $label = empty($name) ? $email : $name . ' (' . $email . ')'; | ||
| } else { | ||
| $label = empty($name) ? $lookup['federationId'] : $name . ' (' . $lookup['federationId'] . ')'; | ||
| } |
| ->willReturn(json_encode($resultBody)); | ||
| $client = $this->createMock(IClient::class); | ||
| $client->expects($this->once()) | ||
| ->method('get') |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Nicolas Varlot <nicolas.varlot@ac-versailles.fr>
- rename variable $showFederatedEmail to $showEmailInLabel for clarity - add testSearchWithEmailLabelFallbackWhenEmailMissing covering the fallback path when flag is enabled but lookup response has no email - tighten HTTP expectation in testSearchWithEmailLabel to validate the requested URL Signed-off-by: niv <nicolas.varlot@ac-versailles.fr>
| if ($showEmailInLabel && !empty($email)) { | ||
| $label = empty($name) ? $email : $name . ' (' . $email . ')'; | ||
| } else { | ||
| $label = empty($name) ? $lookup['federationId'] : $name . ' (' . $lookup['federationId'] . ')'; | ||
| } |
There was a problem hiding this comment.
| if ($showEmailInLabel && !empty($email)) { | |
| $label = empty($name) ? $email : $name . ' (' . $email . ')'; | |
| } else { | |
| $label = empty($name) ? $lookup['federationId'] : $name . ' (' . $lookup['federationId'] . ')'; | |
| } | |
| if ($showEmailInLabel && $email !== '') { | |
| $id = $email; | |
| } else { | |
| $id = $lookup['federationId']; | |
| } | |
| $label = $name === '' ? $id : $name . ' (' . $id . ')'; |
| $this->assertFalse($moreResults); | ||
| } | ||
|
|
||
| public function testSearchWithEmailLabel(): void { |
There was a problem hiding this comment.
Please collapse the tests into a single one and use a data provider to change the flag value and the expected outcome.
| // System config flag: shareapi_lookup_label_show_email | ||
| // Controls whether email addresses from federated lookup results are shown in share dialog labels. | ||
| // Defaults to false to avoid exposing email addresses unnecessarily. Enabling this may reveal | ||
| // users' email addresses to people using the share dialog and should therefore be considered carefully. | ||
| $showEmailInLabel = $this->config->getSystemValueBool('shareapi_lookup_label_show_email', false); |
There was a problem hiding this comment.
Similar to the other configs we have for shareapi_ this should reside in the app config of core instead.
Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de> Signed-off-by: Nicolas Varlot <nicolas.varlot@ac-versailles.fr>
Summary
Adds an opt-in system config flag shareapi_lookup_label_show_email that, when set to true, makes the LookupPlugin compose suggestion labels with the user's email (from the lookup server response) instead of the federation ID.
Default behavior is strictly preserved when the flag is absent or
false.Motivation
In Global Scale deployments, federation IDs are sharded across nodes (e.g.
user@node05.example.org) and do not carry human-readable signal. When the sharing dialog shows two accounts of the same person who happens to be hosted on different nodes, users see:Jane Doe (jdoe@node05.example.org)Jane Doe (janedoe1@node10.example.org)The node suffix is implementation detail and does not help the user pick. The email, which the lookup server already exposes in its response, is far more discriminating.
With the flag enabled, the same suggestions become:
Jane Doe (jane.doe@site-a.example)Jane Doe (jane.doe@site-b.example)Behavior
false: identical to currentmaster.trueandemailpresent in lookup response: label usesName (email)oremailif no name.trueandemailmissing: graceful fallback toName (federationId)orfederationId.Tests