Skip to content

Commit f4fcbaa

Browse files
CarlSchwanCarlSchwan
authored
Merge pull request #30643 from nextcloud/backport/30600/stable23
[stable23] Fix idn emails not working in shares
2 parents 26b5949 + 2b2d23c commit f4fcbaa

4 files changed

Lines changed: 76 additions & 16 deletions

File tree

‎apps/sharebymail/lib/ShareByMailProvider.php‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,16 @@ protected function createMailShare(IShare $share) {
333333
$share->getExpirationDate()
334334
);
335335

336+
if (!$this->mailer->validateMailAddress($share->getSharedWith())) {
337+
$this->removeShareFromTable($shareId);
338+
$e = new HintException('Failed to send share by mail. Got an invalid email address: ' . $share->getSharedWith(),
339+
$this->l->t('Failed to send share by email. Got an invalid email address'));
340+
$this->logger->error('Failed to send share by mail. Got an invalid email address ' . $share->getSharedWith(), [
341+
'app' => 'sharebymail',
342+
'exception' => $e,
343+
]);
344+
}
345+
336346
try {
337347
$link = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare',
338348
['token' => $share->getToken()]);
@@ -671,7 +681,7 @@ public function getChildren(IShare $parent) {
671681
* @param \DateTime|null $expirationTime
672682
* @return int
673683
*/
674-
protected function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $password, $sendPasswordByTalk, $hideDownload, $label, $expirationTime) {
684+
protected function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $password, $sendPasswordByTalk, $hideDownload, $label, $expirationTime): int {
675685
$qb = $this->dbConnection->getQueryBuilder();
676686
$qb->insert('share')
677687
->setValue('share_type', $qb->createNamedParameter(IShare::TYPE_EMAIL))
@@ -765,7 +775,7 @@ public function delete(IShare $share) {
765775
} catch (\Exception $e) {
766776
}
767777

768-
$this->removeShareFromTable($share->getId());
778+
$this->removeShareFromTable((int)$share->getId());
769779
}
770780

771781
/**
@@ -961,9 +971,9 @@ public function getShareByToken($token) {
961971
/**
962972
* remove share from table
963973
*
964-
* @param string $shareId
974+
* @param int $shareId
965975
*/
966-
protected function removeShareFromTable($shareId) {
976+
protected function removeShareFromTable(int $shareId): void {
967977
$qb = $this->dbConnection->getQueryBuilder();
968978
$qb->delete('share')
969979
->where($qb->expr()->eq('id', $qb->createNamedParameter($shareId)));

‎apps/sharebymail/tests/ShareByMailProviderTest.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ public function testCreate() {
217217

218218
public function testCreateSendPasswordByMailWithoutEnforcedPasswordProtection() {
219219
$share = $this->getMockBuilder(IShare::class)->getMock();
220-
$share->expects($this->any())->method('getSharedWith')->willReturn('receiver@example.com');
220+
$share->expects($this->any())->method('getSharedWith')->willReturn('receiver@examplelölöl.com');
221221
$share->expects($this->any())->method('getSendPasswordByTalk')->willReturn(false);
222222
$share->expects($this->any())->method('getSharedBy')->willReturn('owner');
223223

@@ -459,6 +459,7 @@ public function testCreateFailed() {
459459
public function testCreateMailShare() {
460460
$this->share->expects($this->any())->method('getToken')->willReturn('token');
461461
$this->share->expects($this->once())->method('setToken')->with('token');
462+
$this->share->expects($this->any())->method('getSharedWith')->willReturn('valid@valid.com');
462463
$node = $this->getMockBuilder('OCP\Files\Node')->getMock();
463464
$node->expects($this->any())->method('getName')->willReturn('fileName');
464465
$this->share->expects($this->any())->method('getNode')->willReturn($node);
@@ -483,6 +484,7 @@ public function testCreateMailShareFailed() {
483484

484485
$this->share->expects($this->any())->method('getToken')->willReturn('token');
485486
$this->share->expects($this->once())->method('setToken')->with('token');
487+
$this->share->expects($this->any())->method('getSharedWith')->willReturn('valid@valid.com');
486488
$node = $this->getMockBuilder('OCP\Files\Node')->getMock();
487489
$node->expects($this->any())->method('getName')->willReturn('fileName');
488490
$this->share->expects($this->any())->method('getNode')->willReturn($node);
@@ -987,6 +989,7 @@ public function testGetSharesInFolder() {
987989
->willReturn(new \OC\Share20\Share($rootFolder, $userManager));
988990

989991
$provider = $this->getInstance(['sendMailNotification', 'createShareActivity']);
992+
$this->mailer->expects($this->any())->method('validateMailAddress')->willReturn(true);
990993

991994
$u1 = $userManager->createUser('testFed', md5(time()));
992995
$u2 = $userManager->createUser('testFed2', md5(time()));
@@ -1033,6 +1036,7 @@ public function testGetAccessList() {
10331036
->willReturn(new \OC\Share20\Share($rootFolder, $userManager));
10341037

10351038
$provider = $this->getInstance(['sendMailNotification', 'createShareActivity']);
1039+
$this->mailer->expects($this->any())->method('validateMailAddress')->willReturn(true);
10361040

10371041
$u1 = $userManager->createUser('testFed', md5(time()));
10381042
$u2 = $userManager->createUser('testFed2', md5(time()));

‎lib/private/Collaboration/Collaborators/MailPlugin.php‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
use OCP\IUser;
3939
use OCP\IUserSession;
4040
use OCP\Share\IShare;
41+
use OCP\Mail\IMailer;
4142

4243
class MailPlugin implements ISearchPlugin {
4344
/* @var bool */
@@ -64,19 +65,23 @@ class MailPlugin implements ISearchPlugin {
6465
private $knownUserService;
6566
/** @var IUserSession */
6667
private $userSession;
68+
/** @var IMailer */
69+
private $mailer;
6770

6871
public function __construct(IManager $contactsManager,
6972
ICloudIdManager $cloudIdManager,
7073
IConfig $config,
7174
IGroupManager $groupManager,
7275
KnownUserService $knownUserService,
73-
IUserSession $userSession) {
76+
IUserSession $userSession,
77+
IMailer $mailer) {
7478
$this->contactsManager = $contactsManager;
7579
$this->cloudIdManager = $cloudIdManager;
7680
$this->config = $config;
7781
$this->groupManager = $groupManager;
7882
$this->knownUserService = $knownUserService;
7983
$this->userSession = $userSession;
84+
$this->mailer = $mailer;
8085

8186
$this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
8287
$this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes';
@@ -250,8 +255,7 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) {
250255
$userResults['wide'] = array_slice($userResults['wide'], $offset, $limit);
251256
}
252257

253-
254-
if (!$searchResult->hasExactIdMatch($emailType) && filter_var($search, FILTER_VALIDATE_EMAIL)) {
258+
if (!$searchResult->hasExactIdMatch($emailType) && $this->mailer->validateMailAddress($search)) {
255259
$result['exact'][] = [
256260
'label' => $search,
257261
'uuid' => $search,

‎tests/lib/Collaboration/Collaborators/MailPluginTest.php‎

Lines changed: 50 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
use OCP\IUserManager;
3838
use OCP\IUserSession;
3939
use OCP\Share\IShare;
40+
use OCP\Mail\IMailer;
4041
use Test\TestCase;
4142

4243
class MailPluginTest extends TestCase {
@@ -64,6 +65,9 @@ class MailPluginTest extends TestCase {
6465
/** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */
6566
protected $userSession;
6667

68+
/** @var IMailer|\PHPUnit\Framework\MockObject\MockObject */
69+
protected $mailer;
70+
6771
protected function setUp(): void {
6872
parent::setUp();
6973

@@ -72,6 +76,7 @@ protected function setUp(): void {
7276
$this->groupManager = $this->createMock(IGroupManager::class);
7377
$this->knownUserService = $this->createMock(KnownUserService::class);
7478
$this->userSession = $this->createMock(IUserSession::class);
79+
$this->mailer = $this->createMock(IMailer::class);
7580
$this->cloudIdManager = new CloudIdManager($this->contactsManager, $this->createMock(IURLGenerator::class), $this->createMock(IUserManager::class));
7681

7782
$this->searchResult = new SearchResult();
@@ -84,7 +89,8 @@ public function instantiatePlugin() {
8489
$this->config,
8590
$this->groupManager,
8691
$this->knownUserService,
87-
$this->userSession
92+
$this->userSession,
93+
$this->mailer
8894
);
8995
}
9096

@@ -97,7 +103,7 @@ public function instantiatePlugin() {
97103
* @param array $expected
98104
* @param bool $reachedEnd
99105
*/
100-
public function testSearch($searchTerm, $contacts, $shareeEnumeration, $expected, $exactIdMatch, $reachedEnd) {
106+
public function testSearch($searchTerm, $contacts, $shareeEnumeration, $expected, $exactIdMatch, $reachedEnd, $validEmail) {
101107
$this->config->expects($this->any())
102108
->method('getAppValue')
103109
->willReturnCallback(
@@ -117,6 +123,9 @@ function ($appName, $key, $default) use ($shareeEnumeration) {
117123
$this->userSession->method('getUser')
118124
->willReturn($currentUser);
119125

126+
$this->mailer->method('validateMailAddress')
127+
->willReturn($validEmail);
128+
120129
$this->contactsManager->expects($this->any())
121130
->method('search')
122131
->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
@@ -137,9 +146,9 @@ function ($appName, $key, $default) use ($shareeEnumeration) {
137146
public function dataGetEmail() {
138147
return [
139148
// data set 0
140-
['test', [], true, ['emails' => [], 'exact' => ['emails' => []]], false, false],
149+
['test', [], true, ['emails' => [], 'exact' => ['emails' => []]], false, false, false],
141150
// data set 1
142-
['test', [], false, ['emails' => [], 'exact' => ['emails' => []]], false, false],
151+
['test', [], false, ['emails' => [], 'exact' => ['emails' => []]], false, false, false],
143152
// data set 2
144153
[
145154
'test@remote.com',
@@ -148,6 +157,7 @@ public function dataGetEmail() {
148157
['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@remote.com', 'label' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
149158
false,
150159
false,
160+
true,
151161
],
152162
// data set 3
153163
[ // no valid email address
@@ -157,6 +167,7 @@ public function dataGetEmail() {
157167
['emails' => [], 'exact' => ['emails' => []]],
158168
false,
159169
false,
170+
false,
160171
],
161172
// data set 4
162173
[
@@ -166,6 +177,7 @@ public function dataGetEmail() {
166177
['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@remote.com', 'label' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
167178
false,
168179
false,
180+
true,
169181
],
170182
// data set 5
171183
[
@@ -193,6 +205,7 @@ public function dataGetEmail() {
193205
['emails' => [['uuid' => 'uid1', 'name' => 'User @ Localhost', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]], 'exact' => ['emails' => []]],
194206
false,
195207
false,
208+
false,
196209
],
197210
// data set 6
198211
[
@@ -220,6 +233,7 @@ public function dataGetEmail() {
220233
['emails' => [], 'exact' => ['emails' => []]],
221234
false,
222235
false,
236+
false,
223237
],
224238
// data set 7
225239
[
@@ -247,6 +261,7 @@ public function dataGetEmail() {
247261
['emails' => [['uuid' => 'uid1', 'name' => 'User @ Localhost', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]], 'exact' => ['emails' => [['label' => 'test@remote.com', 'uuid' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
248262
false,
249263
false,
264+
true,
250265
],
251266
// data set 8
252267
[
@@ -274,6 +289,7 @@ public function dataGetEmail() {
274289
['emails' => [], 'exact' => ['emails' => [['label' => 'test@remote.com', 'uuid' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
275290
false,
276291
false,
292+
true,
277293
],
278294
// data set 9
279295
[
@@ -301,6 +317,7 @@ public function dataGetEmail() {
301317
['emails' => [], 'exact' => ['emails' => [['name' => 'User @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]]]],
302318
true,
303319
false,
320+
false,
304321
],
305322
// data set 10
306323
[
@@ -328,6 +345,7 @@ public function dataGetEmail() {
328345
['emails' => [], 'exact' => ['emails' => [['name' => 'User @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]]]],
329346
true,
330347
false,
348+
false,
331349
],
332350
// data set 11
333351
// contact with space
@@ -356,6 +374,7 @@ public function dataGetEmail() {
356374
['emails' => [], 'exact' => ['emails' => [['name' => 'User Name @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User Name @ Localhost (user name@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'user name@localhost']]]]],
357375
true,
358376
false,
377+
false,
359378
],
360379
// data set 12
361380
// remote with space, no contact
@@ -384,6 +403,7 @@ public function dataGetEmail() {
384403
['emails' => [], 'exact' => ['emails' => []]],
385404
false,
386405
false,
406+
false,
387407
],
388408
// data set 13
389409
// Local user found by email
@@ -402,6 +422,7 @@ public function dataGetEmail() {
402422
['users' => [], 'exact' => ['users' => [['uuid' => 'uid1', 'name' => 'User', 'label' => 'User (test@example.com)','value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test'], 'shareWithDisplayNameUnique' => 'test@example.com']]]],
403423
true,
404424
false,
425+
true,
405426
],
406427
// data set 14
407428
// Current local user found by email => no result
@@ -420,6 +441,7 @@ public function dataGetEmail() {
420441
['exact' => []],
421442
false,
422443
false,
444+
true,
423445
],
424446
// data set 15
425447
// Pagination and "more results" for user matches byyyyyyy emails
@@ -462,6 +484,7 @@ public function dataGetEmail() {
462484
], 'emails' => [], 'exact' => ['users' => [], 'emails' => []]],
463485
false,
464486
true,
487+
false,
465488
],
466489
// data set 16
467490
// Pagination and "more results" for normal emails
@@ -500,6 +523,7 @@ public function dataGetEmail() {
500523
], 'exact' => ['emails' => []]],
501524
false,
502525
true,
526+
false,
503527
],
504528
// data set 17
505529
// multiple email addresses with type
@@ -533,6 +557,18 @@ public function dataGetEmail() {
533557
]]],
534558
false,
535559
false,
560+
false,
561+
],
562+
// data set 18
563+
// idn email
564+
[
565+
'test@lölölölölölölöl.com',
566+
[],
567+
true,
568+
['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@lölölölölölölöl.com', 'label' => 'test@lölölölölölölöl.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@lölölölölölölöl.com']]]]],
569+
false,
570+
false,
571+
true,
536572
],
537573
];
538574
}
@@ -547,7 +583,7 @@ public function dataGetEmail() {
547583
* @param bool $reachedEnd
548584
* @param array groups
549585
*/
550-
public function testSearchGroupsOnly($searchTerm, $contacts, $expected, $exactIdMatch, $reachedEnd, $userToGroupMapping) {
586+
public function testSearchGroupsOnly($searchTerm, $contacts, $expected, $exactIdMatch, $reachedEnd, $userToGroupMapping, $validEmail) {
551587
$this->config->expects($this->any())
552588
->method('getAppValue')
553589
->willReturnCallback(
@@ -570,6 +606,9 @@ function ($appName, $key, $default) {
570606
->method('getUID')
571607
->willReturn('currentUser');
572608

609+
$this->mailer->method('validateMailAddress')
610+
->willReturn($validEmail);
611+
573612
$this->contactsManager->expects($this->any())
574613
->method('search')
575614
->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
@@ -623,7 +662,8 @@ public function dataGetEmailGroupsOnly() {
623662
[
624663
"currentUser" => ["group1"],
625664
"User" => ["group1"]
626-
]
665+
],
666+
false,
627667
],
628668
// The user `User` cannot share with the current user
629669
[
@@ -643,7 +683,8 @@ public function dataGetEmailGroupsOnly() {
643683
[
644684
"currentUser" => ["group1"],
645685
"User" => ["group2"]
646-
]
686+
],
687+
false,
647688
],
648689
// The user `User` cannot share with the current user, but there is an exact match on the e-mail address -> share by e-mail
649690
[
@@ -663,7 +704,8 @@ public function dataGetEmailGroupsOnly() {
663704
[
664705
"currentUser" => ["group1"],
665706
"User" => ["group2"]
666-
]
707+
],
708+
true,
667709
]
668710
];
669711
}

0 commit comments

Comments
 (0)