Skip to content

Commit e8f6e06

Browse files
CarlSchwanCarlSchwan
committed
Fix idn emails not working in shares
And add check before sending email that email address is valid Fix #30595 Signed-off-by: Carl Schwan <carl@carlschwan.eu>
1 parent b62f879 commit e8f6e06

File tree

4 files changed

+76
-16
lines changed

4 files changed

+76
-16
lines changed

‎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
@@ -35,6 +35,7 @@
3535
use OCP\IUser;
3636
use OCP\IUserSession;
3737
use OCP\Share\IShare;
38+
use OCP\Mail\IMailer;
3839
use Test\TestCase;
3940

4041
class MailPluginTest extends TestCase {
@@ -62,6 +63,9 @@ class MailPluginTest extends TestCase {
6263
/** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */
6364
protected $userSession;
6465

66+
/** @var IMailer|\PHPUnit\Framework\MockObject\MockObject */
67+
protected $mailer;
68+
6569
protected function setUp(): void {
6670
parent::setUp();
6771

@@ -70,6 +74,7 @@ protected function setUp(): void {
7074
$this->groupManager = $this->createMock(IGroupManager::class);
7175
$this->knownUserService = $this->createMock(KnownUserService::class);
7276
$this->userSession = $this->createMock(IUserSession::class);
77+
$this->mailer = $this->createMock(IMailer::class);
7378
$this->cloudIdManager = new CloudIdManager($this->contactsManager);
7479

7580
$this->searchResult = new SearchResult();
@@ -82,7 +87,8 @@ public function instantiatePlugin() {
8287
$this->config,
8388
$this->groupManager,
8489
$this->knownUserService,
85-
$this->userSession
90+
$this->userSession,
91+
$this->mailer
8692
);
8793
}
8894

@@ -95,7 +101,7 @@ public function instantiatePlugin() {
95101
* @param array $expected
96102
* @param bool $reachedEnd
97103
*/
98-
public function testSearch($searchTerm, $contacts, $shareeEnumeration, $expected, $exactIdMatch, $reachedEnd) {
104+
public function testSearch($searchTerm, $contacts, $shareeEnumeration, $expected, $exactIdMatch, $reachedEnd, $validEmail) {
99105
$this->config->expects($this->any())
100106
->method('getAppValue')
101107
->willReturnCallback(
@@ -115,6 +121,9 @@ function ($appName, $key, $default) use ($shareeEnumeration) {
115121
$this->userSession->method('getUser')
116122
->willReturn($currentUser);
117123

124+
$this->mailer->method('validateMailAddress')
125+
->willReturn($validEmail);
126+
118127
$this->contactsManager->expects($this->any())
119128
->method('search')
120129
->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
@@ -135,9 +144,9 @@ function ($appName, $key, $default) use ($shareeEnumeration) {
135144
public function dataGetEmail() {
136145
return [
137146
// data set 0
138-
['test', [], true, ['emails' => [], 'exact' => ['emails' => []]], false, false],
147+
['test', [], true, ['emails' => [], 'exact' => ['emails' => []]], false, false, false],
139148
// data set 1
140-
['test', [], false, ['emails' => [], 'exact' => ['emails' => []]], false, false],
149+
['test', [], false, ['emails' => [], 'exact' => ['emails' => []]], false, false, false],
141150
// data set 2
142151
[
143152
'test@remote.com',
@@ -146,6 +155,7 @@ public function dataGetEmail() {
146155
['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@remote.com', 'label' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
147156
false,
148157
false,
158+
true,
149159
],
150160
// data set 3
151161
[ // no valid email address
@@ -155,6 +165,7 @@ public function dataGetEmail() {
155165
['emails' => [], 'exact' => ['emails' => []]],
156166
false,
157167
false,
168+
false,
158169
],
159170
// data set 4
160171
[
@@ -164,6 +175,7 @@ public function dataGetEmail() {
164175
['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@remote.com', 'label' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
165176
false,
166177
false,
178+
true,
167179
],
168180
// data set 5
169181
[
@@ -191,6 +203,7 @@ public function dataGetEmail() {
191203
['emails' => [['uuid' => 'uid1', 'name' => 'User @ Localhost', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]], 'exact' => ['emails' => []]],
192204
false,
193205
false,
206+
false,
194207
],
195208
// data set 6
196209
[
@@ -218,6 +231,7 @@ public function dataGetEmail() {
218231
['emails' => [], 'exact' => ['emails' => []]],
219232
false,
220233
false,
234+
false,
221235
],
222236
// data set 7
223237
[
@@ -245,6 +259,7 @@ public function dataGetEmail() {
245259
['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']]]]],
246260
false,
247261
false,
262+
true,
248263
],
249264
// data set 8
250265
[
@@ -272,6 +287,7 @@ public function dataGetEmail() {
272287
['emails' => [], 'exact' => ['emails' => [['label' => 'test@remote.com', 'uuid' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
273288
false,
274289
false,
290+
true,
275291
],
276292
// data set 9
277293
[
@@ -299,6 +315,7 @@ public function dataGetEmail() {
299315
['emails' => [], 'exact' => ['emails' => [['name' => 'User @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]]]],
300316
true,
301317
false,
318+
false,
302319
],
303320
// data set 10
304321
[
@@ -326,6 +343,7 @@ public function dataGetEmail() {
326343
['emails' => [], 'exact' => ['emails' => [['name' => 'User @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]]]],
327344
true,
328345
false,
346+
false,
329347
],
330348
// data set 11
331349
// contact with space
@@ -354,6 +372,7 @@ public function dataGetEmail() {
354372
['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']]]]],
355373
true,
356374
false,
375+
false,
357376
],
358377
// data set 12
359378
// remote with space, no contact
@@ -382,6 +401,7 @@ public function dataGetEmail() {
382401
['emails' => [], 'exact' => ['emails' => []]],
383402
false,
384403
false,
404+
false,
385405
],
386406
// data set 13
387407
// Local user found by email
@@ -400,6 +420,7 @@ public function dataGetEmail() {
400420
['users' => [], 'exact' => ['users' => [['uuid' => 'uid1', 'name' => 'User', 'label' => 'User (test@example.com)','value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test'], 'shareWithDisplayNameUnique' => 'test@example.com']]]],
401421
true,
402422
false,
423+
true,
403424
],
404425
// data set 14
405426
// Current local user found by email => no result
@@ -418,6 +439,7 @@ public function dataGetEmail() {
418439
['exact' => []],
419440
false,
420441
false,
442+
true,
421443
],
422444
// data set 15
423445
// Pagination and "more results" for user matches byyyyyyy emails
@@ -460,6 +482,7 @@ public function dataGetEmail() {
460482
], 'emails' => [], 'exact' => ['users' => [], 'emails' => []]],
461483
false,
462484
true,
485+
false,
463486
],
464487
// data set 16
465488
// Pagination and "more results" for normal emails
@@ -498,6 +521,7 @@ public function dataGetEmail() {
498521
], 'exact' => ['emails' => []]],
499522
false,
500523
true,
524+
false,
501525
],
502526
// data set 17
503527
// multiple email addresses with type
@@ -531,6 +555,18 @@ public function dataGetEmail() {
531555
]]],
532556
false,
533557
false,
558+
false,
559+
],
560+
// data set 18
561+
// idn email
562+
[
563+
'test@lölölölölölölöl.com',
564+
[],
565+
true,
566+
['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']]]]],
567+
false,
568+
false,
569+
true,
534570
],
535571
];
536572
}
@@ -545,7 +581,7 @@ public function dataGetEmail() {
545581
* @param bool $reachedEnd
546582
* @param array groups
547583
*/
548-
public function testSearchGroupsOnly($searchTerm, $contacts, $expected, $exactIdMatch, $reachedEnd, $userToGroupMapping) {
584+
public function testSearchGroupsOnly($searchTerm, $contacts, $expected, $exactIdMatch, $reachedEnd, $userToGroupMapping, $validEmail) {
549585
$this->config->expects($this->any())
550586
->method('getAppValue')
551587
->willReturnCallback(
@@ -568,6 +604,9 @@ function ($appName, $key, $default) {
568604
->method('getUID')
569605
->willReturn('currentUser');
570606

607+
$this->mailer->method('validateMailAddress')
608+
->willReturn($validEmail);
609+
571610
$this->contactsManager->expects($this->any())
572611
->method('search')
573612
->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
@@ -621,7 +660,8 @@ public function dataGetEmailGroupsOnly() {
621660
[
622661
"currentUser" => ["group1"],
623662
"User" => ["group1"]
624-
]
663+
],
664+
false,
625665
],
626666
// The user `User` cannot share with the current user
627667
[
@@ -641,7 +681,8 @@ public function dataGetEmailGroupsOnly() {
641681
[
642682
"currentUser" => ["group1"],
643683
"User" => ["group2"]
644-
]
684+
],
685+
false,
645686
],
646687
// The user `User` cannot share with the current user, but there is an exact match on the e-mail address -> share by e-mail
647688
[
@@ -661,7 +702,8 @@ public function dataGetEmailGroupsOnly() {
661702
[
662703
"currentUser" => ["group1"],
663704
"User" => ["group2"]
664-
]
705+
],
706+
true,
665707
]
666708
];
667709
}

0 commit comments

Comments
 (0)