upstream/nextcloud
1
0
Fork 0
mirror of https://github.com/nextcloud/server.git synced 2026-05-13 17:10:43 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 9

Fix idn emails not working in shares

Browse source 
And add check before sending email that email address is valid

Fix #30595

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
This commit is contained in:
Carl Schwan 2022-01-12 14:15:08 +01:00
parent 26b5949102
commit 2b2d23ce21
No known key found for this signature in database
GPG key ID: 06B35D38387B67BE
4 changed files with 76 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

18
apps/sharebymail/lib/ShareByMailProvider.php
View file

@ -333,6 +333,16 @@ class ShareByMailProvider implements IShareProvider {
$share->getExpirationDate()
);
if (!$this->mailer->validateMailAddress($share->getSharedWith())) {
$this->removeShareFromTable($shareId);
$e = new HintException('Failed to send share by mail. Got an invalid email address: ' . $share->getSharedWith(),
$this->l->t('Failed to send share by email. Got an invalid email address'));
$this->logger->error('Failed to send share by mail. Got an invalid email address ' . $share->getSharedWith(), [
'app' => 'sharebymail',
'exception' => $e,
]);
}
try {
$link = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare',
['token' => $share->getToken()]);
@ -671,7 +681,7 @@ class ShareByMailProvider implements IShareProvider {
* @param \DateTime|null $expirationTime
* @return int
*/
protected function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $password, $sendPasswordByTalk, $hideDownload, $label, $expirationTime) {
protected function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $password, $sendPasswordByTalk, $hideDownload, $label, $expirationTime): int {
$qb = $this->dbConnection->getQueryBuilder();
$qb->insert('share')
->setValue('share_type', $qb->createNamedParameter(IShare::TYPE_EMAIL))
@ -765,7 +775,7 @@ class ShareByMailProvider implements IShareProvider {
} catch (\Exception $e) {
}
$this->removeShareFromTable($share->getId());
$this->removeShareFromTable((int)$share->getId());
}
/**
@ -961,9 +971,9 @@ class ShareByMailProvider implements IShareProvider {
/**
* remove share from table
*
* @param string $shareId
* @param int $shareId
*/
protected function removeShareFromTable($shareId) {
protected function removeShareFromTable(int $shareId): void {
$qb = $this->dbConnection->getQueryBuilder();
$qb->delete('share')
->where($qb->expr()->eq('id', $qb->createNamedParameter($shareId)));

6
apps/sharebymail/tests/ShareByMailProviderTest.php
View file

@ -217,7 +217,7 @@ class ShareByMailProviderTest extends TestCase {
public function testCreateSendPasswordByMailWithoutEnforcedPasswordProtection() {
$share = $this->getMockBuilder(IShare::class)->getMock();
$share->expects($this->any())->method('getSharedWith')->willReturn('receiver@example.com');
$share->expects($this->any())->method('getSharedWith')->willReturn('receiver@examplelölöl.com');
$share->expects($this->any())->method('getSendPasswordByTalk')->willReturn(false);
$share->expects($this->any())->method('getSharedBy')->willReturn('owner');
@ -459,6 +459,7 @@ class ShareByMailProviderTest extends TestCase {
public function testCreateMailShare() {
$this->share->expects($this->any())->method('getToken')->willReturn('token');
$this->share->expects($this->once())->method('setToken')->with('token');
$this->share->expects($this->any())->method('getSharedWith')->willReturn('valid@valid.com');
$node = $this->getMockBuilder('OCP\Files\Node')->getMock();
$node->expects($this->any())->method('getName')->willReturn('fileName');
$this->share->expects($this->any())->method('getNode')->willReturn($node);
@ -483,6 +484,7 @@ class ShareByMailProviderTest extends TestCase {
$this->share->expects($this->any())->method('getToken')->willReturn('token');
$this->share->expects($this->once())->method('setToken')->with('token');
$this->share->expects($this->any())->method('getSharedWith')->willReturn('valid@valid.com');
$node = $this->getMockBuilder('OCP\Files\Node')->getMock();
$node->expects($this->any())->method('getName')->willReturn('fileName');
$this->share->expects($this->any())->method('getNode')->willReturn($node);
@ -987,6 +989,7 @@ class ShareByMailProviderTest extends TestCase {
->willReturn(new \OC\Share20\Share($rootFolder, $userManager));
$provider = $this->getInstance(['sendMailNotification', 'createShareActivity']);
$this->mailer->expects($this->any())->method('validateMailAddress')->willReturn(true);
$u1 = $userManager->createUser('testFed', md5(time()));
$u2 = $userManager->createUser('testFed2', md5(time()));
@ -1033,6 +1036,7 @@ class ShareByMailProviderTest extends TestCase {
->willReturn(new \OC\Share20\Share($rootFolder, $userManager));
$provider = $this->getInstance(['sendMailNotification', 'createShareActivity']);
$this->mailer->expects($this->any())->method('validateMailAddress')->willReturn(true);
$u1 = $userManager->createUser('testFed', md5(time()));
$u2 = $userManager->createUser('testFed2', md5(time()));

10
lib/private/Collaboration/Collaborators/MailPlugin.php
View file

@ -38,6 +38,7 @@ use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Share\IShare;
use OCP\Mail\IMailer;
class MailPlugin implements ISearchPlugin {
/* @var bool */
@ -64,19 +65,23 @@ class MailPlugin implements ISearchPlugin {
private $knownUserService;
/** @var IUserSession */
private $userSession;
/** @var IMailer */
private $mailer;
public function __construct(IManager $contactsManager,
ICloudIdManager $cloudIdManager,
IConfig $config,
IGroupManager $groupManager,
KnownUserService $knownUserService,
IUserSession $userSession) {
IUserSession $userSession,
IMailer $mailer) {
$this->contactsManager = $contactsManager;
$this->cloudIdManager = $cloudIdManager;
$this->config = $config;
$this->groupManager = $groupManager;
$this->knownUserService = $knownUserService;
$this->userSession = $userSession;
$this->mailer = $mailer;
$this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
$this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes';
@ -250,8 +255,7 @@ class MailPlugin implements ISearchPlugin {
$userResults['wide'] = array_slice($userResults['wide'], $offset, $limit);
}
if (!$searchResult->hasExactIdMatch($emailType) && filter_var($search, FILTER_VALIDATE_EMAIL)) {
if (!$searchResult->hasExactIdMatch($emailType) && $this->mailer->validateMailAddress($search)) {
$result['exact'][] = [
'label' => $search,
'uuid' => $search,

58
tests/lib/Collaboration/Collaborators/MailPluginTest.php
View file

@ -37,6 +37,7 @@ use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\Share\IShare;
use OCP\Mail\IMailer;
use Test\TestCase;
class MailPluginTest extends TestCase {
@ -64,6 +65,9 @@ class MailPluginTest extends TestCase {
/** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */
protected $userSession;
/** @var IMailer|\PHPUnit\Framework\MockObject\MockObject */
protected $mailer;
protected function setUp(): void {
parent::setUp();
@ -72,6 +76,7 @@ class MailPluginTest extends TestCase {
$this->groupManager = $this->createMock(IGroupManager::class);
$this->knownUserService = $this->createMock(KnownUserService::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->mailer = $this->createMock(IMailer::class);
$this->cloudIdManager = new CloudIdManager($this->contactsManager, $this->createMock(IURLGenerator::class), $this->createMock(IUserManager::class));
$this->searchResult = new SearchResult();
@ -84,7 +89,8 @@ class MailPluginTest extends TestCase {
$this->config,
$this->groupManager,
$this->knownUserService,
$this->userSession
$this->userSession,
$this->mailer
);
}
@ -97,7 +103,7 @@ class MailPluginTest extends TestCase {
* @param array $expected
* @param bool $reachedEnd
*/
public function testSearch($searchTerm, $contacts, $shareeEnumeration, $expected, $exactIdMatch, $reachedEnd) {
public function testSearch($searchTerm, $contacts, $shareeEnumeration, $expected, $exactIdMatch, $reachedEnd, $validEmail) {
$this->config->expects($this->any())
->method('getAppValue')
->willReturnCallback(
@ -117,6 +123,9 @@ class MailPluginTest extends TestCase {
$this->userSession->method('getUser')
->willReturn($currentUser);
$this->mailer->method('validateMailAddress')
->willReturn($validEmail);
$this->contactsManager->expects($this->any())
->method('search')
->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
@ -137,9 +146,9 @@ class MailPluginTest extends TestCase {
public function dataGetEmail() {
return [
// data set 0
['test', [], true, ['emails' => [], 'exact' => ['emails' => []]], false, false],
['test', [], true, ['emails' => [], 'exact' => ['emails' => []]], false, false, false],
// data set 1
['test', [], false, ['emails' => [], 'exact' => ['emails' => []]], false, false],
['test', [], false, ['emails' => [], 'exact' => ['emails' => []]], false, false, false],
// data set 2
[
'test@remote.com',
@ -148,6 +157,7 @@ class MailPluginTest extends TestCase {
['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@remote.com', 'label' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
false,
false,
true,
],
// data set 3
[ // no valid email address
@ -157,6 +167,7 @@ class MailPluginTest extends TestCase {
['emails' => [], 'exact' => ['emails' => []]],
false,
false,
false,
],
// data set 4
[
@ -166,6 +177,7 @@ class MailPluginTest extends TestCase {
['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@remote.com', 'label' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
false,
false,
true,
],
// data set 5
[
@ -193,6 +205,7 @@ class MailPluginTest extends TestCase {
['emails' => [['uuid' => 'uid1', 'name' => 'User @ Localhost', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]], 'exact' => ['emails' => []]],
false,
false,
false,
],
// data set 6
[
@ -220,6 +233,7 @@ class MailPluginTest extends TestCase {
['emails' => [], 'exact' => ['emails' => []]],
false,
false,
false,
],
// data set 7
[
@ -247,6 +261,7 @@ class MailPluginTest extends TestCase {
['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']]]]],
false,
false,
true,
],
// data set 8
[
@ -274,6 +289,7 @@ class MailPluginTest extends TestCase {
['emails' => [], 'exact' => ['emails' => [['label' => 'test@remote.com', 'uuid' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
false,
false,
true,
],
// data set 9
[
@ -301,6 +317,7 @@ class MailPluginTest extends TestCase {
['emails' => [], 'exact' => ['emails' => [['name' => 'User @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]]]],
true,
false,
false,
],
// data set 10
[
@ -328,6 +345,7 @@ class MailPluginTest extends TestCase {
['emails' => [], 'exact' => ['emails' => [['name' => 'User @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]]]],
true,
false,
false,
],
// data set 11
// contact with space
@ -356,6 +374,7 @@ class MailPluginTest extends TestCase {
['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']]]]],
true,
false,
false,
],
// data set 12
// remote with space, no contact
@ -384,6 +403,7 @@ class MailPluginTest extends TestCase {
['emails' => [], 'exact' => ['emails' => []]],
false,
false,
false,
],
// data set 13
// Local user found by email
@ -402,6 +422,7 @@ class MailPluginTest extends TestCase {
['users' => [], 'exact' => ['users' => [['uuid' => 'uid1', 'name' => 'User', 'label' => 'User (test@example.com)','value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test'], 'shareWithDisplayNameUnique' => 'test@example.com']]]],
true,
false,
true,
],
// data set 14
// Current local user found by email => no result
@ -420,6 +441,7 @@ class MailPluginTest extends TestCase {
['exact' => []],
false,
false,
true,
],
// data set 15
// Pagination and "more results" for user matches byyyyyyy emails
@ -462,6 +484,7 @@ class MailPluginTest extends TestCase {
], 'emails' => [], 'exact' => ['users' => [], 'emails' => []]],
false,
true,
false,
],
// data set 16
// Pagination and "more results" for normal emails
@ -500,6 +523,7 @@ class MailPluginTest extends TestCase {
], 'exact' => ['emails' => []]],
false,
true,
false,
],
// data set 17
// multiple email addresses with type
@ -533,6 +557,18 @@ class MailPluginTest extends TestCase {
]]],
false,
false,
false,
],
// data set 18
// idn email
[
'test@lölölölölölölöl.com',
[],
true,
['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']]]]],
false,
false,
true,
],
];
}
@ -547,7 +583,7 @@ class MailPluginTest extends TestCase {
* @param bool $reachedEnd
* @param array groups
*/
public function testSearchGroupsOnly($searchTerm, $contacts, $expected, $exactIdMatch, $reachedEnd, $userToGroupMapping) {
public function testSearchGroupsOnly($searchTerm, $contacts, $expected, $exactIdMatch, $reachedEnd, $userToGroupMapping, $validEmail) {
$this->config->expects($this->any())
->method('getAppValue')
->willReturnCallback(
@ -570,6 +606,9 @@ class MailPluginTest extends TestCase {
->method('getUID')
->willReturn('currentUser');
$this->mailer->method('validateMailAddress')
->willReturn($validEmail);
$this->contactsManager->expects($this->any())
->method('search')
->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
@ -623,7 +662,8 @@ class MailPluginTest extends TestCase {
[
"currentUser" => ["group1"],
"User" => ["group1"]
]
],
false,
],
// The user `User` cannot share with the current user
[
@ -643,7 +683,8 @@ class MailPluginTest extends TestCase {
[
"currentUser" => ["group1"],
"User" => ["group2"]
]
],
false,
],
// The user `User` cannot share with the current user, but there is an exact match on the e-mail address -> share by e-mail
[
@ -663,7 +704,8 @@ class MailPluginTest extends TestCase {
[
"currentUser" => ["group1"],
"User" => ["group2"]
]
],
true,
]
];
}