Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust settings for mail link password #31975

Merged
merged 1 commit into from
Apr 14, 2022
Merged

Adjust settings for mail link password #31975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/sharebymail/lib/ShareByMailProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ public function create(IShare $share) {

// Sends share password to receiver when it's a permanent one (otherwise she will have to request it via the showShare UI)
// or to owner when the password shall be given during a Talk session
if ($this->config->getSystemValue('allow_mail_share_permanent_password', true) || $share->getSendPasswordByTalk()) {
if ($this->config->getSystemValue('sharing.enable_mail_link_password_expiration', false) === true || $share->getSendPasswordByTalk()) {
$send = $this->sendPassword($share, $password);
if ($passwordEnforced && $send === false) {
$this->sendPasswordToOwner($share, $password);
Expand Down
10 changes: 5 additions & 5 deletions apps/sharebymail/tests/ShareByMailProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ public function testCreateSendPasswordByMailWithPasswordAndWithoutEnforcedPasswo
// The given password (but not the autogenerated password) should not be
// mailed to the receiver of the share because permanent passwords are not enforced.
$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(false);
$this->config->expects($this->once())->method('getSystemValue')->with('allow_mail_share_permanent_password')->willReturn(false);
$this->config->expects($this->once())->method('getSystemValue')->with('sharing.enable_mail_link_password_expiration')->willReturn(false);
$instance->expects($this->never())->method('autoGeneratePassword');

$this->assertSame('shareObject',
Expand Down Expand Up @@ -310,7 +310,7 @@ public function testCreateSendPasswordByMailWithPasswordAndWithoutEnforcedPasswo
// The given password (but not the autogenerated password) should be
// mailed to the receiver of the share because permanent passwords are enforced.
$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(false);
$this->config->expects($this->once())->method('getSystemValue')->with('allow_mail_share_permanent_password')->willReturn(true);
$this->config->expects($this->once())->method('getSystemValue')->with('sharing.enable_mail_link_password_expiration')->willReturn(true);
$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);
$instance->expects($this->never())->method('autoGeneratePassword');

Expand Down Expand Up @@ -363,7 +363,7 @@ public function testCreateSendPasswordByMailWithEnforcedPasswordProtectionWithPe

// The autogenerated password should be mailed to the receiver of the share because permanent passwords are enforced.
$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(true);
$this->config->expects($this->once())->method('getSystemValue')->with('allow_mail_share_permanent_password')->willReturn(true);
$this->config->expects($this->once())->method('getSystemValue')->with('sharing.enable_mail_link_password_expiration')->willReturn(true);
$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);

$message = $this->createMock(IMessage::class);
Expand Down Expand Up @@ -408,7 +408,7 @@ public function testCreateSendPasswordByMailWithPasswordAndWithEnforcedPasswordP
// The given password (but not the autogenerated password) should be
// mailed to the receiver of the share.
$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(true);
$this->config->expects($this->once())->method('getSystemValue')->with('allow_mail_share_permanent_password')->willReturn(true);
$this->config->expects($this->once())->method('getSystemValue')->with('sharing.enable_mail_link_password_expiration')->willReturn(true);
$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);
$instance->expects($this->never())->method('autoGeneratePassword');

Expand Down Expand Up @@ -453,7 +453,7 @@ public function testCreateSendPasswordByTalkWithEnforcedPasswordProtection() {

// The autogenerated password should be mailed to the owner of the share.
$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(true);
$this->config->expects($this->once())->method('getSystemValue')->with('allow_mail_share_permanent_password')->willReturn(false);
$this->config->expects($this->once())->method('getSystemValue')->with('sharing.enable_mail_link_password_expiration')->willReturn(false);
$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);
$instance->expects($this->once())->method('autoGeneratePassword')->with($share)->willReturn('autogeneratedPassword');

Expand Down
12 changes: 12 additions & 0 deletions config/config.sample.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1540,6 +1540,18 @@
*/
'sharing.managerFactory' => '\OC\Share20\ProviderFactory',

/**
* Enables expiration for link share passwords sent by email (sharebymail).
* The passwords will expire after the configured interval, the users can
* still request a new one in the public link page.
*/
'sharing.enable_mail_link_password_expiration' => false,

/**
* Expiration interval for passwords, in seconds.
*/
'sharing.mail_link_password_expiration_interval' => 3600,

/**
* Define max number of results returned by the search for auto-completion of
* users, groups, etc. The value must not be lower than 0 (for unlimited).
Expand Down
22 changes: 5 additions & 17 deletions lib/private/Share20/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1175,29 +1175,17 @@ private function updateSharePasswordIfNeeded(IShare $share, IShare $originalShar
* Set the share's password expiration time
*/
private function setSharePasswordExpirationTime(IShare $share): void {
if ($this->config->getSystemValue('allow_mail_share_permanent_password', true)) {
if (!$this->config->getSystemValue('sharing.enable_mail_link_password_expiration', false)) {
// Sets password expiration date to NULL
$share->setPasswordExpirationTime();
return;
}
// Sets password expiration date
$expirationTime = null;
try {
$now = new \DateTime();
$expirationInterval = $this->config->getSystemValue('share_temporary_password_expiration_interval');
if ($expirationInterval === '' || is_null($expirationInterval)) {
$expirationInterval = 'P0DT15M';
}
$expirationTime = $now->add(new \DateInterval($expirationInterval));
} catch (\Exception $e) {
// Catches invalid format for system value 'share_temporary_password_expiration_interval'
\OC::$server->getLogger()->logException($e, [
'message' => 'The \'share_temporary_password_expiration_interval\' system setting does not respect the DateInterval::__construct() format. Setting it to \'P0DT15M\''
]);
$expirationTime = $now->add(new \DateInterval('P0DT15M'));
} finally {
$share->setPasswordExpirationTime($expirationTime);
}
$now = new \DateTime();
$expirationInterval = $this->config->getSystemValue('sharing.mail_link_password_expiration_interval', 3600);
$expirationTime = $now->add(new \DateInterval('PT' . $expirationInterval . 'S'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A try catch with a error log would make sense in case the admin did a mistake when configuration the interval (e,g, put a broken value or negative number)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just removed the try catch because I didn't think such easy mistake could be done

the try-catch before was because of the complex interval string where a mistake is much more likely

$share->setPasswordExpirationTime($expirationTime);
Comment on lines +1185 to +1188
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$now = new \DateTime();
$expirationInterval = $this->config->getSystemValue('sharing.mail_link_password_expiration_interval', 3600);
$expirationTime = $now->add(new \DateInterval('PT' . $expirationInterval . 'S'));
$share->setPasswordExpirationTime($expirationTime);
$expirationTime = new \DateTime();
$expirationInterval = $this->config->getSystemValue('sharing.mail_link_password_expiration_interval', 3600);
$expirationTime->add(new \DateInterval('PT' . $expirationInterval . 'S'));
$share->setPasswordExpirationTime($expirationTime);

}


Expand Down