Skip to content

Commit 78bcd6d

Browse files
skjnldsvskjnldsv
committed
Fix sharebymail tests
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
1 parent e313f3c commit 78bcd6d

File tree

3 files changed

+47
-32
lines changed

3 files changed

+47
-32
lines changed

‎build/integration/features/bootstrap/SharingContext.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ protected function resetAppConfigs() {
4242
$this->deleteServerConfig('core', 'shareapi_default_internal_expire_date');
4343
$this->deleteServerConfig('core', 'shareapi_internal_expire_after_n_days');
4444
$this->deleteServerConfig('core', 'internal_defaultExpDays');
45+
$this->deleteServerConfig('core', 'shareapi_enforce_links_password');
4546
$this->deleteServerConfig('core', 'shareapi_default_expire_date');
4647
$this->deleteServerConfig('core', 'shareapi_expire_after_n_days');
4748
$this->deleteServerConfig('core', 'link_defaultExpDays');

‎lib/private/Share20/Manager.php‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ protected function verifyPassword($password) {
193193
if ($password === null) {
194194
// No password is set, check if this is allowed.
195195
if ($this->shareApiLinkEnforcePassword()) {
196-
throw new \InvalidArgumentException('Passwords are enforced for link shares');
196+
throw new \InvalidArgumentException('Passwords are enforced for link and mail shares');
197197
}
198198

199199
return;
@@ -771,7 +771,8 @@ public function createShare(IShare $share) {
771771
$this->verifyPassword($share->getPassword());
772772

773773
// If a password is set. Hash it!
774-
if ($share->getPassword() !== null) {
774+
if ($share->getShareType() === IShare::TYPE_LINK
775+
&& $share->getPassword() !== null) {
775776
$share->setPassword($this->hasher->hash($share->getPassword()));
776777
}
777778
}
@@ -986,23 +987,33 @@ public function updateShare(IShare $share) {
986987
|| $share->getShareType() === IShare::TYPE_EMAIL) {
987988
$this->linkCreateChecks($share);
988989

989-
// The new password is not set again if it is the same as the old one.
990+
// The new password is not set again if it is the same as the old
991+
// one, unless when switching from sending by Talk to sending by
992+
// mail.
990993
$plainTextPassword = $share->getPassword();
994+
$updatedPassword = $this->updateSharePasswordIfNeeded($share, $originalShare);
991995

992-
if (empty($plainTextPassword)) {
996+
/**
997+
* Cannot enable the getSendPasswordByTalk if there is no password set
998+
*/
999+
if (empty($plainTextPassword) && $share->getSendPasswordByTalk()) {
1000+
throw new \InvalidArgumentException('Can’t enable sending the password by Talk with an empty password');
1001+
}
1002+
1003+
/**
1004+
* If we're in a mail share, we need to force a password change
1005+
* as either the user is not aware of the password or is already (received by mail)
1006+
* Thus the SendPasswordByTalk feature would not make sense
1007+
*/
1008+
if (!$updatedPassword && $share->getShareType() === IShare::TYPE_EMAIL) {
9931009
if (!$originalShare->getSendPasswordByTalk() && $share->getSendPasswordByTalk()) {
994-
// If the same password was already sent by mail the recipient
995-
// would already have access to the share without having to call
996-
// the sharer to verify her identity
9971010
throw new \InvalidArgumentException('Can’t enable sending the password by Talk without setting a new password');
9981011
}
9991012
if ($originalShare->getSendPasswordByTalk() && !$share->getSendPasswordByTalk()) {
10001013
throw new \InvalidArgumentException('Can’t disable sending the password by Talk without setting a new password');
10011014
}
10021015
}
10031016

1004-
$this->updateSharePasswordIfNeeded($share, $originalShare);
1005-
10061017
if ($share->getExpirationDate() != $originalShare->getExpirationDate()) {
10071018
// Verify the expiration date
10081019
$this->validateExpirationDate($share);
@@ -1105,9 +1116,13 @@ private function updateSharePasswordIfNeeded(IShare $share, IShare $originalShar
11051116
$this->verifyPassword($share->getPassword());
11061117

11071118
// If a password is set. Hash it!
1108-
if ($share->getPassword() !== null) {
1119+
if (!empty($share->getPassword())) {
11091120
$share->setPassword($this->hasher->hash($share->getPassword()));
11101121

1122+
return true;
1123+
} else {
1124+
// Empty string and null are seen as NOT password protected
1125+
$share->setPassword(null);
11111126
return true;
11121127
}
11131128
} else {

‎tests/lib/Share20/ManagerTest.php‎

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,7 @@ public function testGetExpiredShareById() {
484484

485485
public function testVerifyPasswordNullButEnforced() {
486486
$this->expectException(\InvalidArgumentException::class);
487-
$this->expectExceptionMessage('Passwords are enforced for link shares');
487+
$this->expectExceptionMessage('Passwords are enforced for link and mail shares');
488488

489489
$this->config->method('getAppValue')->willReturnMap([
490490
['core', 'shareapi_enforce_links_password', 'no', 'yes'],
@@ -3464,6 +3464,7 @@ public function testUpdateShareMailEnableSendPasswordByTalkWithNoPassword() {
34643464
$manager->expects($this->once())->method('linkCreateChecks');
34653465
$manager->expects($this->never())->method('validateExpirationDate');
34663466

3467+
// If the password is empty, we have nothing to hash
34673468
$this->hasher->expects($this->never())
34683469
->method('hash');
34693470

@@ -3531,11 +3532,12 @@ public function testUpdateShareMailEnableSendPasswordByTalkRemovingPassword() {
35313532
$manager->expects($this->once())->method('canShare')->willReturn(true);
35323533
$manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare);
35333534
$manager->expects($this->once())->method('generalCreateChecks')->with($share);
3534-
$manager->expects($this->never())->method('verifyPassword');
3535+
$manager->expects($this->once())->method('verifyPassword');
35353536
$manager->expects($this->never())->method('pathCreateChecks');
35363537
$manager->expects($this->once())->method('linkCreateChecks');
35373538
$manager->expects($this->never())->method('validateExpirationDate');
35383539

3540+
// If the password is empty, we have nothing to hash
35393541
$this->hasher->expects($this->never())
35403542
->method('hash');
35413543

@@ -3603,11 +3605,12 @@ public function testUpdateShareMailEnableSendPasswordByTalkRemovingPasswordWithE
36033605
$manager->expects($this->once())->method('canShare')->willReturn(true);
36043606
$manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare);
36053607
$manager->expects($this->once())->method('generalCreateChecks')->with($share);
3606-
$manager->expects($this->never())->method('verifyPassword');
3608+
$manager->expects($this->once())->method('verifyPassword');
36073609
$manager->expects($this->never())->method('pathCreateChecks');
36083610
$manager->expects($this->once())->method('linkCreateChecks');
36093611
$manager->expects($this->never())->method('validateExpirationDate');
36103612

3613+
// If the password is empty, we have nothing to hash
36113614
$this->hasher->expects($this->never())
36123615
->method('hash');
36133616

@@ -3649,7 +3652,7 @@ public function testUpdateShareMailEnableSendPasswordByTalkWithPreviousPassword(
36493652
$originalShare = $this->manager->newShare();
36503653
$originalShare->setShareType(IShare::TYPE_EMAIL)
36513654
->setPermissions(\OCP\Constants::PERMISSION_ALL)
3652-
->setPassword('passwordHash')
3655+
->setPassword('password')
36533656
->setSendPasswordByTalk(false);
36543657

36553658
$tomorrow = new \DateTime();
@@ -3680,11 +3683,9 @@ public function testUpdateShareMailEnableSendPasswordByTalkWithPreviousPassword(
36803683
$manager->expects($this->once())->method('linkCreateChecks');
36813684
$manager->expects($this->never())->method('validateExpirationDate');
36823685

3683-
$this->hasher->expects($this->once())
3684-
->method('verify')
3685-
->with('password', 'passwordHash')
3686-
->willReturn(true);
3687-
3686+
// If the old & new passwords are the same, we don't do anything
3687+
$this->hasher->expects($this->never())
3688+
->method('verify');
36883689
$this->hasher->expects($this->never())
36893690
->method('hash');
36903691

@@ -3742,7 +3743,7 @@ public function testUpdateShareMailDisableSendPasswordByTalkWithPreviousPassword
37423743
->setToken('token')
37433744
->setSharedBy('owner')
37443745
->setShareOwner('owner')
3745-
->setPassword('password')
3746+
->setPassword('passwordHash')
37463747
->setSendPasswordByTalk(false)
37473748
->setExpirationDate($tomorrow)
37483749
->setNode($file)
@@ -3751,16 +3752,14 @@ public function testUpdateShareMailDisableSendPasswordByTalkWithPreviousPassword
37513752
$manager->expects($this->once())->method('canShare')->willReturn(true);
37523753
$manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare);
37533754
$manager->expects($this->once())->method('generalCreateChecks')->with($share);
3754-
$manager->expects($this->once())->method('verifyPassword');
3755-
$manager->expects($this->once())->method('pathCreateChecks');
3755+
$manager->expects($this->never())->method('verifyPassword');
3756+
$manager->expects($this->never())->method('pathCreateChecks');
37563757
$manager->expects($this->once())->method('linkCreateChecks');
3757-
$manager->expects($this->once())->method('validateExpirationDate');
3758-
3759-
$this->hasher->expects($this->once())
3760-
->method('verify')
3761-
->with('password', 'passwordHash')
3762-
->willReturn(true);
3758+
$manager->expects($this->never())->method('validateExpirationDate');
37633759

3760+
// If the old & new passwords are the same, we don't do anything
3761+
$this->hasher->expects($this->never())
3762+
->method('verify');
37643763
$this->hasher->expects($this->never())
37653764
->method('hash');
37663765

@@ -3827,14 +3826,14 @@ public function testUpdateShareMailDisableSendPasswordByTalkWithoutChangingPassw
38273826
$manager->expects($this->once())->method('canShare')->willReturn(true);
38283827
$manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare);
38293828
$manager->expects($this->once())->method('generalCreateChecks')->with($share);
3830-
$manager->expects($this->once())->method('verifyPassword');
3831-
$manager->expects($this->once())->method('pathCreateChecks');
3829+
$manager->expects($this->never())->method('verifyPassword');
3830+
$manager->expects($this->never())->method('pathCreateChecks');
38323831
$manager->expects($this->once())->method('linkCreateChecks');
3833-
$manager->expects($this->once())->method('validateExpirationDate');
3832+
$manager->expects($this->never())->method('validateExpirationDate');
38343833

3834+
// If the old & new passwords are the same, we don't do anything
38353835
$this->hasher->expects($this->never())
38363836
->method('verify');
3837-
38383837
$this->hasher->expects($this->never())
38393838
->method('hash');
38403839

0 commit comments

Comments
 (0)