mbk-lab
/
nextcloud
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix sharebymail tests 

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
This commit is contained in:
John Molakvoæ (skjnldsv) 2021-03-18 11:41:10 +01:00
parent f99876997a
commit 15767643f2
No known key found for this signature in database
GPG Key ID: 60C25B8C072916CF
3 changed files with 92 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
build/integration/features/bootstrap/SharingContext.php
View File

@ -42,6 +42,7 @@ class SharingContext implements Context, SnippetAcceptingContext {
$this->deleteServerConfig('core', 'shareapi_default_internal_expire_date');
$this->deleteServerConfig('core', 'shareapi_internal_expire_after_n_days');
$this->deleteServerConfig('core', 'internal_defaultExpDays');
$this->deleteServerConfig('core', 'shareapi_enforce_links_password');
$this->deleteServerConfig('core', 'shareapi_default_expire_date');
$this->deleteServerConfig('core', 'shareapi_expire_after_n_days');
$this->deleteServerConfig('core', 'link_defaultExpDays');

39
lib/private/Share20/Manager.php
View File

@ -193,7 +193,7 @@ class Manager implements IManager {
if ($password === null) {
// No password is set, check if this is allowed.
if ($this->shareApiLinkEnforcePassword()) {
throw new \InvalidArgumentException('Passwords are enforced for link shares');
throw new \InvalidArgumentException('Passwords are enforced for link and mail shares');
}
return;
@ -765,13 +765,14 @@ class Manager implements IManager {
);
// Verify the expiration date
$share = $this->validateExpirationDate($share);
$share = $this->validateExpirationDateLink($share);
// Verify the password
$this->verifyPassword($share->getPassword());
// If a password is set. Hash it!
if ($share->getPassword() !== null) {
if ($share->getShareType() === IShare::TYPE_LINK
&& $share->getPassword() !== null) {
$share->setPassword($this->hasher->hash($share->getPassword()));
}
}
@ -986,14 +987,26 @@ class Manager implements IManager {
|| $share->getShareType() === IShare::TYPE_EMAIL) {
$this->linkCreateChecks($share);
// The new password is not set again if it is the same as the old one.
// The new password is not set again if it is the same as the old
// one, unless when switching from sending by Talk to sending by
// mail.
$plainTextPassword = $share->getPassword();
$updatedPassword = $this->updateSharePasswordIfNeeded($share, $originalShare);
if (empty($plainTextPassword)) {
/**
* Cannot enable the getSendPasswordByTalk if there is no password set
*/
if (empty($plainTextPassword) && $share->getSendPasswordByTalk()) {
throw new \InvalidArgumentException('Can’t enable sending the password by Talk with an empty password');
}
/**
* If we're in a mail share, we need to force a password change
* as either the user is not aware of the password or is already (received by mail)
* Thus the SendPasswordByTalk feature would not make sense
*/
if (!$updatedPassword && $share->getShareType() === IShare::TYPE_EMAIL) {
if (!$originalShare->getSendPasswordByTalk() && $share->getSendPasswordByTalk()) {
// If the same password was already sent by mail the recipient
// would already have access to the share without having to call
// the sharer to verify her identity
throw new \InvalidArgumentException('Can’t enable sending the password by Talk without setting a new password');
}
if ($originalShare->getSendPasswordByTalk() && !$share->getSendPasswordByTalk()) {
@ -1001,11 +1014,9 @@ class Manager implements IManager {
}
}
$this->updateSharePasswordIfNeeded($share, $originalShare);
if ($share->getExpirationDate() != $originalShare->getExpirationDate()) {
// Verify the expiration date
$this->validateExpirationDate($share);
$this->validateExpirationDateLink($share);
$expirationDateUpdated = true;
}
}
@ -1105,9 +1116,13 @@ class Manager implements IManager {
$this->verifyPassword($share->getPassword());
// If a password is set. Hash it!
if ($share->getPassword() !== null) {
if (!empty($share->getPassword())) {
$share->setPassword($this->hasher->hash($share->getPassword()));
return true;
} else {
// Empty string and null are seen as NOT password protected
$share->setPassword(null);
return true;
}
} else {

128
tests/lib/Share20/ManagerTest.php
View File

@ -50,6 +50,7 @@ use OCP\Mail\IMailer;
use OCP\Security\Events\ValidatePasswordPolicyEvent;
use OCP\Security\IHasher;
use OCP\Security\ISecureRandom;
use OCP\Share\Exceptions\AlreadySharedException;
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IProviderFactory;
use OCP\Share\IShare;
@ -484,7 +485,7 @@ class ManagerTest extends \Test\TestCase {
public function testVerifyPasswordNullButEnforced() {
$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage('Passwords are enforced for link shares');
$this->expectExceptionMessage('Passwords are enforced for link and mail shares');
$this->config->method('getAppValue')->willReturnMap([
['core', 'shareapi_enforce_links_password', 'no', 'yes'],
@ -1067,7 +1068,7 @@ class ManagerTest extends \Test\TestCase {
$share = $this->manager->newShare();
$share->setExpirationDate($past);
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
}
public function testValidateExpirationDateEnforceButNotSet() {
@ -1083,7 +1084,7 @@ class ManagerTest extends \Test\TestCase {
['core', 'shareapi_enforce_expire_date', 'no', 'yes'],
]);
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
}
public function testValidateExpirationDateEnforceButNotEnabledAndNotSet() {
@ -1095,7 +1096,7 @@ class ManagerTest extends \Test\TestCase {
['core', 'shareapi_enforce_expire_date', 'no', 'yes'],
]);
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$this->assertNull($share->getExpirationDate());
}
@ -1115,7 +1116,7 @@ class ManagerTest extends \Test\TestCase {
$expected->setTime(0,0,0);
$expected->add(new \DateInterval('P3D'));
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$this->assertNotNull($share->getExpirationDate());
$this->assertEquals($expected, $share->getExpirationDate());
@ -1136,7 +1137,7 @@ class ManagerTest extends \Test\TestCase {
$expected->setTime(0,0,0);
$expected->add(new \DateInterval('P1D'));
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$this->assertNotNull($share->getExpirationDate());
$this->assertEquals($expected, $share->getExpirationDate());
@ -1159,7 +1160,7 @@ class ManagerTest extends \Test\TestCase {
['core', 'shareapi_default_expire_date', 'no', 'yes'],
]);
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
}
public function testValidateExpirationDateEnforceValid() {
@ -1186,7 +1187,7 @@ class ManagerTest extends \Test\TestCase {
return $data['expirationDate'] == $future;
}));
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$this->assertEquals($expected, $share->getExpirationDate());
}
@ -1208,7 +1209,7 @@ class ManagerTest extends \Test\TestCase {
return $data['expirationDate'] == $expected && $data['passwordSet'] === false;
}));
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$this->assertEquals($expected, $share->getExpirationDate());
}
@ -1223,7 +1224,7 @@ class ManagerTest extends \Test\TestCase {
$share = $this->manager->newShare();
$share->setPassword('password');
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$this->assertNull($share->getExpirationDate());
}
@ -1248,7 +1249,7 @@ class ManagerTest extends \Test\TestCase {
return $data['expirationDate'] == $expected;
}));
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$this->assertEquals($expected, $share->getExpirationDate());
}
@ -1277,7 +1278,7 @@ class ManagerTest extends \Test\TestCase {
return $data['expirationDate'] == $expected;
}));
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$this->assertEquals($expected, $share->getExpirationDate());
}
@ -1298,7 +1299,7 @@ class ManagerTest extends \Test\TestCase {
$share = $this->manager->newShare();
$share->setExpirationDate($nextWeek);
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$save->sub(new \DateInterval('P2D'));
$this->assertEquals($save, $share->getExpirationDate());
@ -1322,7 +1323,7 @@ class ManagerTest extends \Test\TestCase {
$data['message'] = 'Invalid date!';
});
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
}
public function testValidateExpirationDateExistingShareNoDefault() {
@ -1336,7 +1337,7 @@ class ManagerTest extends \Test\TestCase {
['core', 'shareapi_expire_after_n_days', '7', '6'],
]);
self::invokePrivate($this->manager, 'validateExpirationDate', [$share]);
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
$this->assertEquals(null, $share->getExpirationDate());
}
@ -2048,7 +2049,7 @@ class ManagerTest extends \Test\TestCase {
'generalCreateChecks',
'linkCreateChecks',
'pathCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
'verifyPassword',
'setLinkParent',
])
@ -2090,7 +2091,7 @@ class ManagerTest extends \Test\TestCase {
->method('pathCreateChecks')
->with($path);
$manager->expects($this->once())
->method('validateExpirationDate')
->method('validateExpirationDateLink')
->with($share)
->willReturn($share);
$manager->expects($this->once())
@ -2173,7 +2174,7 @@ class ManagerTest extends \Test\TestCase {
'generalCreateChecks',
'linkCreateChecks',
'pathCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
'verifyPassword',
'setLinkParent',
])
@ -2209,7 +2210,7 @@ class ManagerTest extends \Test\TestCase {
->method('pathCreateChecks')
->with($path);
$manager->expects($this->once())
->method('validateExpirationDate')
->method('validateExpirationDateLink')
->with($share)
->willReturn($share);
$manager->expects($this->once())
@ -3025,7 +3026,7 @@ class ManagerTest extends \Test\TestCase {
'linkCreateChecks',
'pathCreateChecks',
'verifyPassword',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3054,7 +3055,7 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->once())->method('canShare')->willReturn(true);
$manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare);
$manager->expects($this->once())->method('validateExpirationDate')->with($share);
$manager->expects($this->once())->method('validateExpirationDateLink')->with($share);
$manager->expects($this->once())->method('verifyPassword')->with('password');
$this->hasher->expects($this->once())
@ -3096,7 +3097,7 @@ class ManagerTest extends \Test\TestCase {
public function testUpdateShareLinkEnableSendPasswordByTalkWithNoPassword() {
$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage('Can’t enable sending the password by Talk without setting a new password');
$this->expectExceptionMessage('Can’t enable sending the password by Talk with an empty password');
$manager = $this->createManagerMock()
->setMethods([
@ -3106,7 +3107,7 @@ class ManagerTest extends \Test\TestCase {
'linkCreateChecks',
'pathCreateChecks',
'verifyPassword',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3140,7 +3141,7 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->once())->method('linkCreateChecks')->with($share);
$manager->expects($this->never())->method('verifyPassword');
$manager->expects($this->never())->method('pathCreateChecks');
$manager->expects($this->never())->method('validateExpirationDate');
$manager->expects($this->never())->method('validateExpirationDateLink');
$this->hasher->expects($this->never())
->method('hash');
@ -3172,7 +3173,7 @@ class ManagerTest extends \Test\TestCase {
'verifyPassword',
'pathCreateChecks',
'linkCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3205,7 +3206,7 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->once())->method('verifyPassword')->with('password');
$manager->expects($this->once())->method('pathCreateChecks')->with($file);
$manager->expects($this->once())->method('linkCreateChecks');
$manager->expects($this->once())->method('validateExpirationDate');
$manager->expects($this->once())->method('validateExpirationDateLink');
$this->hasher->expects($this->once())
->method('hash')
@ -3252,7 +3253,7 @@ class ManagerTest extends \Test\TestCase {
'verifyPassword',
'pathCreateChecks',
'linkCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3288,7 +3289,7 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->once())->method('verifyPassword')->with('password');
$manager->expects($this->once())->method('pathCreateChecks')->with($file);
$manager->expects($this->once())->method('linkCreateChecks');
$manager->expects($this->once())->method('validateExpirationDate');
$manager->expects($this->once())->method('validateExpirationDateLink');
$this->hasher->expects($this->once())
->method('hash')
@ -3335,7 +3336,7 @@ class ManagerTest extends \Test\TestCase {
'verifyPassword',
'pathCreateChecks',
'linkCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3371,7 +3372,7 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->once())->method('verifyPassword')->with('password');
$manager->expects($this->once())->method('pathCreateChecks')->with($file);
$manager->expects($this->once())->method('linkCreateChecks');
$manager->expects($this->once())->method('validateExpirationDate');
$manager->expects($this->once())->method('validateExpirationDateLink');
$this->hasher->expects($this->once())
->method('verify')
@ -3416,7 +3417,7 @@ class ManagerTest extends \Test\TestCase {
public function testUpdateShareMailEnableSendPasswordByTalkWithNoPassword() {
$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage('Can’t enable sending the password by Talk without setting a new password');
$this->expectExceptionMessage('Can’t enable sending the password by Talk with an empty password');
$manager = $this->createManagerMock()
->setMethods([
@ -3426,7 +3427,7 @@ class ManagerTest extends \Test\TestCase {
'verifyPassword',
'pathCreateChecks',
'linkCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3462,8 +3463,9 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->never())->method('verifyPassword');
$manager->expects($this->never())->method('pathCreateChecks');
$manager->expects($this->once())->method('linkCreateChecks');
$manager->expects($this->never())->method('validateExpirationDate');
$manager->expects($this->never())->method('validateExpirationDateLink');
// If the password is empty, we have nothing to hash
$this->hasher->expects($this->never())
->method('hash');
@ -3488,7 +3490,7 @@ class ManagerTest extends \Test\TestCase {
public function testUpdateShareMailEnableSendPasswordByTalkRemovingPassword() {
$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage('Can’t enable sending the password by Talk without setting a new password');
$this->expectExceptionMessage('Can’t enable sending the password by Talk with an empty password');
$manager = $this->createManagerMock()
->setMethods([
@ -3498,7 +3500,7 @@ class ManagerTest extends \Test\TestCase {
'verifyPassword',
'pathCreateChecks',
'linkCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3531,11 +3533,12 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->once())->method('canShare')->willReturn(true);
$manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare);
$manager->expects($this->once())->method('generalCreateChecks')->with($share);
$manager->expects($this->never())->method('verifyPassword');
$manager->expects($this->once())->method('verifyPassword');
$manager->expects($this->never())->method('pathCreateChecks');
$manager->expects($this->once())->method('linkCreateChecks');
$manager->expects($this->never())->method('validateExpirationDate');
$manager->expects($this->never())->method('validateExpirationDateLink');
// If the password is empty, we have nothing to hash
$this->hasher->expects($this->never())
->method('hash');
@ -3560,7 +3563,7 @@ class ManagerTest extends \Test\TestCase {
public function testUpdateShareMailEnableSendPasswordByTalkRemovingPasswordWithEmptyString() {
$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage('Can’t enable sending the password by Talk without setting a new password');
$this->expectExceptionMessage('Can’t enable sending the password by Talk with an empty password');
$manager = $this->createManagerMock()
->setMethods([
@ -3570,7 +3573,7 @@ class ManagerTest extends \Test\TestCase {
'verifyPassword',
'pathCreateChecks',
'linkCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3603,11 +3606,12 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->once())->method('canShare')->willReturn(true);
$manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare);
$manager->expects($this->once())->method('generalCreateChecks')->with($share);
$manager->expects($this->never())->method('verifyPassword');
$manager->expects($this->once())->method('verifyPassword');
$manager->expects($this->never())->method('pathCreateChecks');
$manager->expects($this->once())->method('linkCreateChecks');
$manager->expects($this->never())->method('validateExpirationDate');
$manager->expects($this->never())->method('validateExpirationDateLink');
// If the password is empty, we have nothing to hash
$this->hasher->expects($this->never())
->method('hash');
@ -3642,14 +3646,14 @@ class ManagerTest extends \Test\TestCase {
'verifyPassword',
'pathCreateChecks',
'linkCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
$originalShare = $this->manager->newShare();
$originalShare->setShareType(IShare::TYPE_EMAIL)
->setPermissions(\OCP\Constants::PERMISSION_ALL)
->setPassword('passwordHash')
->setPassword('password')
->setSendPasswordByTalk(false);
$tomorrow = new \DateTime();
@ -3678,13 +3682,11 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->never())->method('verifyPassword');
$manager->expects($this->never())->method('pathCreateChecks');
$manager->expects($this->once())->method('linkCreateChecks');
$manager->expects($this->never())->method('validateExpirationDate');
$this->hasher->expects($this->once())
->method('verify')
->with('password', 'passwordHash')
->willReturn(true);
$manager->expects($this->never())->method('validateExpirationDateLink');
// If the old & new passwords are the same, we don't do anything
$this->hasher->expects($this->never())
->method('verify');
$this->hasher->expects($this->never())
->method('hash');
@ -3718,7 +3720,7 @@ class ManagerTest extends \Test\TestCase {
'verifyPassword',
'pathCreateChecks',
'linkCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3742,7 +3744,7 @@ class ManagerTest extends \Test\TestCase {
->setToken('token')
->setSharedBy('owner')
->setShareOwner('owner')
->setPassword('password')
->setPassword('passwordHash')
->setSendPasswordByTalk(false)
->setExpirationDate($tomorrow)
->setNode($file)
@ -3751,16 +3753,14 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->once())->method('canShare')->willReturn(true);
$manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare);
$manager->expects($this->once())->method('generalCreateChecks')->with($share);
$manager->expects($this->once())->method('verifyPassword');
$manager->expects($this->once())->method('pathCreateChecks');
$manager->expects($this->never())->method('verifyPassword');
$manager->expects($this->never())->method('pathCreateChecks');
$manager->expects($this->once())->method('linkCreateChecks');
$manager->expects($this->once())->method('validateExpirationDate');
$this->hasher->expects($this->once())
->method('verify')
->with('password', 'passwordHash')
->willReturn(true);
$manager->expects($this->never())->method('validateExpirationDateLink');
// If the old & new passwords are the same, we don't do anything
$this->hasher->expects($this->never())
->method('verify');
$this->hasher->expects($this->never())
->method('hash');
@ -3794,7 +3794,7 @@ class ManagerTest extends \Test\TestCase {
'verifyPassword',
'pathCreateChecks',
'linkCreateChecks',
'validateExpirationDate',
'validateExpirationDateLink',
])
->getMock();
@ -3827,14 +3827,14 @@ class ManagerTest extends \Test\TestCase {
$manager->expects($this->once())->method('canShare')->willReturn(true);
$manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare);
$manager->expects($this->once())->method('generalCreateChecks')->with($share);
$manager->expects($this->once())->method('verifyPassword');
$manager->expects($this->once())->method('pathCreateChecks');
$manager->expects($this->never())->method('verifyPassword');
$manager->expects($this->never())->method('pathCreateChecks');
$manager->expects($this->once())->method('linkCreateChecks');
$manager->expects($this->once())->method('validateExpirationDate');
$manager->expects($this->never())->method('validateExpirationDateLink');
// If the old & new passwords are the same, we don't do anything
$this->hasher->expects($this->never())
->method('verify');
$this->hasher->expects($this->never())
->method('hash');