Merge pull request #24364 from nextcloud/fix/sharing-mail-link-parity

Sharing link & mail parity

apps/files_sharing/lib/Controller/ShareAPIController.php

@@ -556,13 +556,13 @@ class ShareAPIController extends OCSController {
 			}
 
 			// Only share by mail have a recipient
-			if ($shareType === IShare::TYPE_EMAIL) {
+			if (is_string($shareWith) && $shareType === IShare::TYPE_EMAIL) {
 				$share->setSharedWith($shareWith);
-			} else {
-				// Only link share have a label
-				if (!empty($label)) {
-					$share->setLabel($label);
-				}
+			}
+
+			// If we have a label, use it
+			if (!empty($label)) {
+				$share->setLabel($label);
 			}
 
 			if ($sendPasswordByTalk === 'true') {
@@ -1127,8 +1127,7 @@ class ShareAPIController extends OCSController {
 				$share->setPassword($password);
 			}
 
-			// only link shares have labels
-			if ($share->getShareType() === IShare::TYPE_LINK && $label !== null) {
+			if ($label !== null) {
 				if (strlen($label) > 255) {
 					throw new OCSBadRequestException("Maxmimum label length is 255");
 				}
@@ -1592,7 +1591,6 @@ class ShareAPIController extends OCSController {
 			IShare::TYPE_GROUP,
 			IShare::TYPE_LINK,
 			IShare::TYPE_EMAIL,
-			IShare::TYPE_EMAIL,
 			IShare::TYPE_CIRCLE,
 			IShare::TYPE_ROOM,
 			IShare::TYPE_DECK

apps/files_sharing/src/components/SharingEntryLink.vue

@@ -29,6 +29,9 @@
 			<h5 :title="title">
 				{{ title }}
 			</h5>
+			<p v-if="subtitle">
+				{{ subtitle }}
+			</p>
 		</div>
 
 		<!-- clipboard -->
@@ -321,7 +324,6 @@
 
 <script>
 import { generateUrl } from '@nextcloud/router'
-import axios from '@nextcloud/axios'
 import Vue from 'vue'
 
 import ActionButton from '@nextcloud/vue/dist/Components/ActionButton'
@@ -335,11 +337,10 @@ import Actions from '@nextcloud/vue/dist/Components/Actions'
 import Avatar from '@nextcloud/vue/dist/Components/Avatar'
 import Tooltip from '@nextcloud/vue/dist/Directives/Tooltip'
 
+import GeneratePassword from '../utils/GeneratePassword'
 import Share from '../models/Share'
 import SharesMixin from '../mixins/SharesMixin'
 
-const passwordSet = 'abcdefgijkmnopqrstwxyzABCDEFGHJKLMNPQRSTWXYZ23456789'
-
 export default {
 	name: 'SharingEntryLink',
 
@@ -406,7 +407,6 @@ export default {
 
 		/**
 		 * Link share label
-		 * TODO: allow editing
 		 * @returns {string}
 		 */
 		title() {
@@ -424,6 +424,11 @@ export default {
 					})
 				}
 				if (this.share.label && this.share.label.trim() !== '') {
+					if (this.isEmailShareType) {
+						return t('files_sharing', 'Mail share ({label})', {
+							label: this.share.label.trim(),
+						})
+					}
 					return t('files_sharing', 'Share link ({label})', {
 						label: this.share.label.trim(),
 					})
@@ -435,6 +440,18 @@ export default {
 			return t('files_sharing', 'Share link')
 		},
 
+		/**
+		 * Show the email on a second line if a label is set for mail shares
+		 * @returns {string}
+		 */
+		subtitle() {
+			if (this.isEmailShareType
+				&& this.title !== this.share.shareWith) {
+				return this.share.shareWith
+			}
+			return null
+		},
+
 		/**
 		 * Does the current share have an expiration date
 		 * @returns {boolean}
@@ -472,7 +489,7 @@ export default {
 			},
 			async set(enabled) {
 				// TODO: directly save after generation to make sure the share is always protected
-				Vue.set(this.share, 'password', enabled ? await this.generatePassword() : '')
+				Vue.set(this.share, 'password', enabled ? await GeneratePassword() : '')
 				Vue.set(this.share, 'newPassword', this.share.password)
 			},
 		},
@@ -635,7 +652,7 @@ export default {
 				shareDefaults.expiration = this.config.defaultExpirationDateString
 			}
 			if (this.config.enableLinkPasswordByDefault) {
-				shareDefaults.password = await this.generatePassword()
+				shareDefaults.password = await GeneratePassword()
 			}
 
 			// do not push yet if we need a password or an expiration date: show pending menu
@@ -658,7 +675,7 @@ export default {
 				// ELSE, show the pending popovermenu
 				// if password enforced, pre-fill with random one
 				if (this.config.enforcePasswordForPublicLink) {
-					shareDefaults.password = await this.generatePassword()
+					shareDefaults.password = await GeneratePassword()
 				}
 
 				// create share & close menu
@@ -781,35 +798,6 @@ export default {
 				this.queueUpdate('label')
 			}
 		},
-
-		/**
-		 * Generate a valid policy password or
-		 * request a valid password if password_policy
-		 * is enabled
-		 *
-		 * @returns {string} a valid password
-		 */
-		async generatePassword() {
-			// password policy is enabled, let's request a pass
-			if (this.config.passwordPolicy.api && this.config.passwordPolicy.api.generate) {
-				try {
-					const request = await axios.get(this.config.passwordPolicy.api.generate)
-					if (request.data.ocs.data.password) {
-						return request.data.ocs.data.password
-					}
-				} catch (error) {
-					console.info('Error generating password from password_policy', error)
-				}
-			}
-
-			// generate password of 10 length based on passwordSet
-			return Array(10).fill(0)
-				.reduce((prev, curr) => {
-					prev += passwordSet.charAt(Math.floor(Math.random() * passwordSet.length))
-					return prev
-				}, '')
-		},
-
 		async copyLink() {
 			try {
 				await this.$copyText(this.shareLink)
@@ -933,6 +921,9 @@ export default {
 			overflow: hidden;
 			white-space: nowrap;
 		}
+		p {
+			color: var(--color-text-maxcontrast);
+		}
 	}
 
 	&:not(.sharing-entry--share) &__actions {

apps/files_sharing/src/components/SharingInput.vue

@@ -56,6 +56,7 @@ import debounce from 'debounce'
 import Multiselect from '@nextcloud/vue/dist/Components/Multiselect'
 
 import Config from '../services/ConfigService'
+import GeneratePassword from '../utils/GeneratePassword'
 import Share from '../models/Share'
 import ShareRequests from '../mixins/ShareRequests'
 import ShareTypes from '../mixins/ShareTypes'
@@ -448,9 +449,6 @@ export default {
 				return true
 			}
 
-			// TODO: reset the search string when done
-			// https://github.com/shentao/vue-multiselect/issues/633
-
 			// handle externalResults from OCA.Sharing.ShareSearch
 			if (value.handler) {
 				const share = await value.handler(this)
@@ -459,25 +457,55 @@ export default {
 			}
 
 			this.loading = true
+			console.debug('Adding a new share from the input for', value)
 			try {
+				let password = null
+
+				if (this.config.enforcePasswordForPublicLink
+					&& value.shareType === this.SHARE_TYPES.SHARE_TYPE_EMAIL) {
+					password = await GeneratePassword()
+				}
+
 				const path = (this.fileInfo.path + '/' + this.fileInfo.name).replace('//', '/')
 				const share = await this.createShare({
 					path,
 					shareType: value.shareType,
 					shareWith: value.shareWith,
+					password,
 					permissions: this.fileInfo.sharePermissions & OC.getCapabilities().files_sharing.default_permissions,
 				})
-				this.$emit('add:share', share)
 
-				this.getRecommendations()
+				// If we had a password, we need to show it to the user as it was generated
+				if (password) {
+					share.newPassword = password
+					// Wait for the newly added share
+					const component = await new Promise(resolve => {
+						this.$emit('add:share', share, resolve)
+					})
 
-			} catch (response) {
+					// open the menu on the
+					// freshly created share component
+					component.open = true
+				} else {
+					// Else we just add it normally
+					this.$emit('add:share', share)
+				}
+
+				// reset the search string when done
+				// FIXME: https://github.com/shentao/vue-multiselect/issues/633
+				if (this.$refs.multiselect?.$refs?.VueMultiselect?.search) {
+					this.$refs.multiselect.$refs.VueMultiselect.search = ''
+				}
+
+				await this.getRecommendations()
+			} catch (error) {
 				// focus back if any error
 				const input = this.$refs.multiselect.$el.querySelector('input')
 				if (input) {
 					input.focus()
 				}
 				this.query = value.shareWith
+				console.error('Error while adding new share', error)
 			} finally {
 				this.loading = false
 			}

apps/files_sharing/src/utils/GeneratePassword.js

@@ -0,0 +1,56 @@
+
+/**
+ * @copyright Copyright (c) 2020 John Molakvoæ <skjnldsv@protonmail.com>
+ *
+ * @author John Molakvoæ <skjnldsv@protonmail.com>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+import axios from '@nextcloud/axios'
+import Config from '../services/ConfigService'
+
+const config = new Config()
+const passwordSet = 'abcdefgijkmnopqrstwxyzABCDEFGHJKLMNPQRSTWXYZ23456789'
+
+/**
+ * Generate a valid policy password or
+ * request a valid password if password_policy
+ * is enabled
+ *
+ * @returns {string} a valid password
+ */
+export default async function() {
+	// password policy is enabled, let's request a pass
+	if (config.passwordPolicy.api && config.passwordPolicy.api.generate) {
+		try {
+			const request = await axios.get(config.passwordPolicy.api.generate)
+			if (request.data.ocs.data.password) {
+				return request.data.ocs.data.password
+			}
+		} catch (error) {
+			console.info('Error generating password from password_policy', error)
+		}
+	}
+
+	// generate password of 10 length based on passwordSet
+	return Array(10).fill(0)
+		.reduce((prev, curr) => {
+			prev += passwordSet.charAt(Math.floor(Math.random() * passwordSet.length))
+			return prev
+		}, '')
+}

apps/files_sharing/src/views/SharingTab.vue

@@ -52,12 +52,14 @@
 
 			<!-- link shares list -->
 			<SharingLinkList v-if="!loading"
+				ref="linkShareList"
 				:can-reshare="canReshare"
 				:file-info="fileInfo"
 				:shares="linkShares" />
 
 			<!-- other shares list -->
 			<SharingList v-if="!loading"
+				ref="shareList"
 				:shares="shares"
 				:file-info="fileInfo" />
 
@@ -295,11 +297,13 @@ export default {
 		},
 
 		/**
-		 * Insert share at top of arrays
+		 * Add a new share into the shares list
+		 * and return the newly created share component
 		 *
-		 * @param {Share} share the share to insert
+		 * @param {Share} share the share to add to the array
+		 * @param {Function} resolve a function to run after the share is added and its component initialized
 		 */
-		addShare(share) {
+		addShare(share, resolve) {
 			// only catching share type MAIL as link shares are added differently
 			// meaning: not from the ShareInput
 			if (share.type === this.SHARE_TYPES.SHARE_TYPE_EMAIL) {
@@ -307,6 +311,31 @@ export default {
 			} else {
 				this.shares.unshift(share)
 			}
+			this.awaitForShare(share, resolve)
+		},
+
+		/**
+		 * Await for next tick and render after the list updated
+		 * Then resolve with the matched vue component of the
+		 * provided share object
+		 *
+		 * @param {Share} share newly created share
+		 * @param {Function} resolve a function to execute after
+		 */
+		awaitForShare(share, resolve) {
+			let listComponent = this.$refs.shareList
+			// Only mail shares comes from the input, link shares
+			// are managed internally in the SharingLinkList component
+			if (share.type === this.SHARE_TYPES.SHARE_TYPE_EMAIL) {
+				listComponent = this.$refs.linkShareList
+			}
+
+			this.$nextTick(() => {
+				const newShare = listComponent.$children.find(component => component.share === share)
+				if (newShare) {
+					resolve(newShare)
+				}
+			})
 		},
 	},
 }

apps/settings/templates/settings/admin/sharing.php

@@ -70,7 +70,7 @@
 			   value="1" <?php if ($_['allowLinks'] === 'yes') {
 	print_unescaped('checked="checked"');
 } ?> />
-		<label for="allowLinks"><?php p($l->t('Allow users to share via link'));?></label><br/>
+		<label for="allowLinks"><?php p($l->t('Allow users to share via link and emails'));?></label><br/>
 	</p>
 
 	<p id="publicLinkSettings" class="indent <?php if ($_['allowLinks'] !== 'yes' || $_['shareAPIEnabled'] === 'no') {
@@ -96,7 +96,7 @@
 			   value="1" <?php if ($_['shareDefaultExpireDateSet'] === 'yes') {
 	print_unescaped('checked="checked"');
 } ?> />
-		<label for="shareapiDefaultExpireDate"><?php p($l->t('Set default expiration date for link shares'));?></label><br/>
+		<label for="shareapiDefaultExpireDate"><?php p($l->t('Set default expiration date'));?></label><br/>
 
 	</p>
 	<p id="setDefaultExpireDate" class="double-indent <?php if ($_['allowLinks'] !== 'yes' || $_['shareDefaultExpireDateSet'] === 'no' || $_['shareAPIEnabled'] === 'no') {

apps/sharebymail/lib/Capabilities.php

@@ -27,15 +27,15 @@ declare(strict_types=1);
 
 namespace OCA\ShareByMail;
 
-use OCA\ShareByMail\Settings\SettingsManager;
 use OCP\Capabilities\ICapability;
+use OCP\Share\IManager;
 
 class Capabilities implements ICapability {
 
-	/** @var SettingsManager */
+	/** @var IManager */
 	private $manager;
 
-	public function __construct(SettingsManager $manager) {
+	public function __construct(IManager $manager) {
 		$this->manager = $manager;
 	}
 
@@ -45,16 +45,17 @@ class Capabilities implements ICapability {
 				[
 					'sharebymail' =>
 						[
-							'enabled' => true,
+							'enabled' => $this->manager->shareApiAllowLinks(),
 							'upload_files_drop' => [
 								'enabled' => true,
 							],
 							'password' => [
 								'enabled' => true,
-								'enforced' => $this->manager->enforcePasswordProtection(),
+								'enforced' => $this->manager->shareApiLinkEnforcePassword(),
 							],
 							'expire_date' => [
 								'enabled' => true,
+								'enforced' => $this->manager->shareApiLinkDefaultExpireDateEnforced(),
 							],
 						]
 				]

apps/sharebymail/lib/Settings/Admin.php

@@ -41,7 +41,6 @@ class Admin implements ISettings {
 	public function getForm() {
 		$parameters = [
 			'sendPasswordMail' => $this->settingsManager->sendPasswordByMail(),
-			'enforcePasswordProtection' => $this->settingsManager->enforcePasswordProtection(),
 			'replyToInitiator' => $this->settingsManager->replyToInitiator()
 		];
 

apps/sharebymail/lib/Settings/SettingsManager.php

@@ -36,8 +36,6 @@ class SettingsManager {
 
 	private $sendPasswordByMailDefault = 'yes';
 
-	private $enforcePasswordProtectionDefault = 'no';
-
 	private $replyToInitiatorDefault = 'yes';
 
 	public function __construct(IConfig $config) {
@@ -54,16 +52,6 @@ class SettingsManager {
 		return $sendPasswordByMail === 'yes';
 	}
 
-	/**
-	 * do we require a share by mail to be password protected
-	 *
-	 * @return bool
-	 */
-	public function enforcePasswordProtection(): bool {
-		$enforcePassword = $this->config->getAppValue('sharebymail', 'enforcePasswordProtection', $this->enforcePasswordProtectionDefault);
-		return $enforcePassword === 'yes';
-	}
-
 	/**
 	 * should add reply to with initiator mail
 	 *

apps/sharebymail/lib/ShareByMailProvider.php

@@ -61,6 +61,7 @@ use OCP\Security\IHasher;
 use OCP\Security\ISecureRandom;
 use OCP\Share\Exceptions\GenericShareException;
 use OCP\Share\Exceptions\ShareNotFound;
+use OCP\Share\IManager as IShareManager;
 use OCP\Share\IShare;
 use OCP\Share\IShareProvider;
 
@@ -110,6 +111,9 @@ class ShareByMailProvider implements IShareProvider {
 	/** @var IEventDispatcher */
 	private $eventDispatcher;
 
+	/** @var IShareManager */
+	private $shareManager;
+
 	/**
 	 * Return the identifier of this provider.
 	 *
@@ -119,21 +123,20 @@ class ShareByMailProvider implements IShareProvider {
 		return 'ocMailShare';
 	}
 
-	public function __construct(
-		IDBConnection $connection,
-		ISecureRandom $secureRandom,
-		IUserManager $userManager,
-		IRootFolder $rootFolder,
-		IL10N $l,
-		ILogger $logger,
-		IMailer $mailer,
-		IURLGenerator $urlGenerator,
-		IManager $activityManager,
-		SettingsManager $settingsManager,
-		Defaults $defaults,
-		IHasher $hasher,
-		IEventDispatcher $eventDispatcher
-	) {
+	public function __construct(IDBConnection $connection,
+								ISecureRandom $secureRandom,
+								IUserManager $userManager,
+								IRootFolder $rootFolder,
+								IL10N $l,
+								ILogger $logger,
+								IMailer $mailer,
+								IURLGenerator $urlGenerator,
+								IManager $activityManager,
+								SettingsManager $settingsManager,
+								Defaults $defaults,
+								IHasher $hasher,
+								IEventDispatcher $eventDispatcher,
+								IShareManager $shareManager) {
 		$this->dbConnection = $connection;
 		$this->secureRandom = $secureRandom;
 		$this->userManager = $userManager;
@@ -147,6 +150,7 @@ class ShareByMailProvider implements IShareProvider {
 		$this->defaults = $defaults;
 		$this->hasher = $hasher;
 		$this->eventDispatcher = $eventDispatcher;
+		$this->shareManager = $shareManager;
 	}
 
 	/**
@@ -173,7 +177,7 @@ class ShareByMailProvider implements IShareProvider {
 		// if the admin enforces a password for all mail shares we create a
 		// random password and send it to the recipient
 		$password = $share->getPassword() ?: '';
-		$passwordEnforced = $this->settingsManager->enforcePasswordProtection();
+		$passwordEnforced = $this->shareManager->shareApiLinkEnforcePassword();
 		if ($passwordEnforced && empty($password)) {
 			$password = $this->autoGeneratePassword($share);
 		}
@@ -322,6 +326,7 @@ class ShareByMailProvider implements IShareProvider {
 			$share->getPassword(),
 			$share->getSendPasswordByTalk(),
 			$share->getHideDownload(),
+			$share->getLabel(),
 			$share->getExpirationDate()
 		);
 
@@ -659,10 +664,11 @@ class ShareByMailProvider implements IShareProvider {
 	 * @param string $password
 	 * @param bool $sendPasswordByTalk
 	 * @param bool $hideDownload
+	 * @param string $label
 	 * @param \DateTime|null $expirationTime
 	 * @return int
 	 */
-	protected function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $password, $sendPasswordByTalk, $hideDownload, $expirationTime) {
+	protected function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $password, $sendPasswordByTalk, $hideDownload, $label, $expirationTime) {
 		$qb = $this->dbConnection->getQueryBuilder();
 		$qb->insert('share')
 			->setValue('share_type', $qb->createNamedParameter(IShare::TYPE_EMAIL))
@@ -677,7 +683,8 @@ class ShareByMailProvider implements IShareProvider {
 			->setValue('password', $qb->createNamedParameter($password))
 			->setValue('password_by_talk', $qb->createNamedParameter($sendPasswordByTalk, IQueryBuilder::PARAM_BOOL))
 			->setValue('stime', $qb->createNamedParameter(time()))
-			->setValue('hide_download', $qb->createNamedParameter((int)$hideDownload, IQueryBuilder::PARAM_INT));
+			->setValue('hide_download', $qb->createNamedParameter((int)$hideDownload, IQueryBuilder::PARAM_INT))
+			->setValue('label', $qb->createNamedParameter($label));
 
 		if ($expirationTime !== null) {
 			$qb->setValue('expiration', $qb->createNamedParameter($expirationTime, IQueryBuilder::PARAM_DATE));
@@ -720,6 +727,7 @@ class ShareByMailProvider implements IShareProvider {
 			->set('uid_owner', $qb->createNamedParameter($share->getShareOwner()))
 			->set('uid_initiator', $qb->createNamedParameter($share->getSharedBy()))
 			->set('password', $qb->createNamedParameter($share->getPassword()))
+			->set('label', $qb->createNamedParameter($share->getLabel()))
 			->set('password_by_talk', $qb->createNamedParameter($share->getSendPasswordByTalk(), IQueryBuilder::PARAM_BOOL))
 			->set('expiration', $qb->createNamedParameter($share->getExpirationDate(), IQueryBuilder::PARAM_DATE))
 			->set('note', $qb->createNamedParameter($share->getNote()))
@@ -982,6 +990,7 @@ class ShareByMailProvider implements IShareProvider {
 		$share->setShareTime($shareTime);
 		$share->setSharedWith($data['share_with']);
 		$share->setPassword($data['password']);
+		$share->setLabel($data['label']);
 		$share->setSendPasswordByTalk((bool)$data['password_by_talk']);
 		$share->setHideDownload((bool)$data['hide_download']);
 

apps/sharebymail/templates/settings-admin.php

@@ -14,14 +14,11 @@ style('sharebymail', 'settings-admin');
 	p('checked');
 } ?> />
 		<label for="sendPasswordMail"><?php p($l->t('Send password by mail')); ?></label><br/>
-		<input id="enforcePasswordProtection" type="checkbox" class="checkbox" <?php if ($_['enforcePasswordProtection']) {
+
+		<input id="replyToInitiator" type="checkbox" class="checkbox" <?php if ($_['replyToInitiator']) {
 	p('checked');
 } ?> />
-		<label for="enforcePasswordProtection"><?php p($l->t('Enforce password protection')); ?></label><br/>
-        <input id="replyToInitiator" type="checkbox" class="checkbox" <?php if ($_['replyToInitiator']) {
-	p('checked');
-} ?> />
-        <label for="replyToInitiator"><?php p($l->t('Reply to initiator')); ?></label>
+		<label for="replyToInitiator"><?php p($l->t('Reply to initiator')); ?></label>
 	</p>
 
 </div>

apps/sharebymail/tests/CapabilitiesTest.php

@@ -26,26 +26,30 @@
 namespace OCA\ShareByMail\Tests;
 
 use OCA\ShareByMail\Capabilities;
-use OCA\ShareByMail\Settings\SettingsManager;
+use OCP\Share\IManager;
 use Test\TestCase;
 
 class CapabilitiesTest extends TestCase {
 	/** @var Capabilities */
 	private $capabilities;
 
-	/** @var SettingsManager */
-	private $settingsManager;
+	/** @var IManager | \PHPUnit\Framework\MockObject\MockObject */
+	private $manager;
 
 	protected function setUp(): void {
 		parent::setUp();
 
 
-		$this->settingsManager = $this::createMock(SettingsManager::class);
-		$this->capabilities = new Capabilities($this->settingsManager);
+		$this->manager = $this::createMock(IManager::class);
+		$this->capabilities = new Capabilities($this->manager);
 	}
 
 	public function testGetCapabilities() {
-		$this->settingsManager->method('enforcePasswordProtection')
+		$this->manager->method('shareApiAllowLinks')
+			->willReturn(true);
+		$this->manager->method('shareApiLinkEnforcePassword')
+			->willReturn(false);
+		$this->manager->method('shareApiLinkDefaultExpireDateEnforced')
 			->willReturn(false);
 
 		$capabilities = [
@@ -54,9 +58,17 @@ class CapabilitiesTest extends TestCase {
 					'sharebymail' =>
 						[
 							'enabled' => true,
-							'upload_files_drop' => ['enabled' => true],
-							'password' => ['enabled' => true, 'enforced' => false],
-							'expire_date' => ['enabled' => true]
+							'upload_files_drop' => [
+								'enabled' => true,
+							],
+							'password' => [
+								'enabled' => true,
+								'enforced' => false,
+							],
+							'expire_date' => [
+								'enabled' => true,
+								'enforced' => false,
+							],
 						]
 				]
 		];

apps/sharebymail/tests/ShareByMailProviderTest.php

@@ -65,7 +65,7 @@ class ShareByMailProviderTest extends TestCase {
 	/** @var  IDBConnection */
 	private $connection;
 
-	/** @var  IManager */
+	/** @var  IManager | \PHPUnit\Framework\MockObject\MockObject */
 	private $shareManager;
 
 	/** @var  IL10N | \PHPUnit\Framework\MockObject\MockObject */
@@ -110,7 +110,6 @@ class ShareByMailProviderTest extends TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 
-		$this->shareManager = \OC::$server->getShareManager();
 		$this->connection = \OC::$server->getDatabaseConnection();
 
 		$this->l = $this->getMockBuilder(IL10N::class)->getMock();
@@ -130,6 +129,7 @@ class ShareByMailProviderTest extends TestCase {
 		$this->defaults = $this->createMock(Defaults::class);
 		$this->hasher = $this->getMockBuilder(IHasher::class)->getMock();
 		$this->eventDispatcher = $this->getMockBuilder(IEventDispatcher::class)->getMock();
+		$this->shareManager = $this->getMockBuilder(IManager::class)->getMock();
 
 		$this->userManager->expects($this->any())->method('userExists')->willReturn(true);
 	}
@@ -156,7 +156,8 @@ class ShareByMailProviderTest extends TestCase {
 					$this->settingsManager,
 					$this->defaults,
 					$this->hasher,
-					$this->eventDispatcher
+					$this->eventDispatcher,
+					$this->shareManager
 				]
 			);
 
@@ -178,7 +179,8 @@ class ShareByMailProviderTest extends TestCase {
 			$this->settingsManager,
 			$this->defaults,
 			$this->hasher,
-			$this->eventDispatcher
+			$this->eventDispatcher,
+			$this->shareManager
 		);
 	}
 
@@ -204,7 +206,7 @@ class ShareByMailProviderTest extends TestCase {
 		$instance->expects($this->once())->method('createShareObject')->with('rawShare')->willReturn('shareObject');
 		$instance->expects($this->any())->method('sendPassword')->willReturn(true);
 		$share->expects($this->any())->method('getNode')->willReturn($node);
-		$this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(false);
+		$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(false);
 		$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);
 
 		$this->assertSame('shareObject',
@@ -231,7 +233,7 @@ class ShareByMailProviderTest extends TestCase {
 		$share->expects($this->any())->method('getNode')->willReturn($node);
 
 		// The autogenerated password should not be mailed.
-		$this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(false);
+		$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(false);
 		$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);
 		$instance->expects($this->never())->method('autoGeneratePassword');
 
@@ -266,7 +268,7 @@ class ShareByMailProviderTest extends TestCase {
 
 		// The given password (but not the autogenerated password) should be
 		// mailed to the receiver of the share.
-		$this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(false);
+		$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(false);
 		$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);
 		$instance->expects($this->never())->method('autoGeneratePassword');
 
@@ -318,7 +320,7 @@ class ShareByMailProviderTest extends TestCase {
 		$share->expects($this->once())->method('setPassword')->with('autogeneratedPasswordHashed');
 
 		// The autogenerated password should be mailed to the receiver of the share.
-		$this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(true);
+		$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(true);
 		$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);
 
 		$message = $this->createMock(IMessage::class);
@@ -362,7 +364,7 @@ class ShareByMailProviderTest extends TestCase {
 
 		// The given password (but not the autogenerated password) should be
 		// mailed to the receiver of the share.
-		$this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(true);
+		$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(true);
 		$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);
 		$instance->expects($this->never())->method('autoGeneratePassword');
 
@@ -406,7 +408,7 @@ class ShareByMailProviderTest extends TestCase {
 		$share->expects($this->once())->method('setPassword')->with('autogeneratedPasswordHashed');
 
 		// The autogenerated password should be mailed to the owner of the share.
-		$this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(true);
+		$this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(true);
 		$this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);
 		$instance->expects($this->once())->method('autoGeneratePassword')->with($share)->willReturn('autogeneratedPassword');
 
@@ -524,6 +526,7 @@ class ShareByMailProviderTest extends TestCase {
 		$password = 'password';
 		$sendPasswordByTalk = true;
 		$hideDownload = true;
+		$label = 'label';
 		$expiration = new \DateTime();
 
 
@@ -542,6 +545,7 @@ class ShareByMailProviderTest extends TestCase {
 				$password,
 				$sendPasswordByTalk,
 				$hideDownload,
+				$label,
 				$expiration
 			]
 		);
@@ -567,6 +571,7 @@ class ShareByMailProviderTest extends TestCase {
 		$this->assertSame($password, $result[0]['password']);
 		$this->assertSame($sendPasswordByTalk, (bool)$result[0]['password_by_talk']);
 		$this->assertSame($hideDownload, (bool)$result[0]['hide_download']);
+		$this->assertSame($label, $result[0]['label']);
 		$this->assertSame($expiration->getTimestamp(), \DateTime::createFromFormat('Y-m-d H:i:s', $result[0]['expiration'])->getTimestamp());
 	}
 
@@ -976,6 +981,10 @@ class ShareByMailProviderTest extends TestCase {
 		$userManager = \OC::$server->getUserManager();
 		$rootFolder = \OC::$server->getRootFolder();
 
+		$this->shareManager->expects($this->any())
+			->method('newShare')
+			->willReturn(new \OC\Share20\Share($rootFolder, $userManager));
+
 		$provider = $this->getInstance(['sendMailNotification', 'createShareActivity']);
 
 		$u1 = $userManager->createUser('testFed', md5(time()));
@@ -1018,6 +1027,10 @@ class ShareByMailProviderTest extends TestCase {
 		$userManager = \OC::$server->getUserManager();
 		$rootFolder = \OC::$server->getRootFolder();
 
+		$this->shareManager->expects($this->any())
+			->method('newShare')
+			->willReturn(new \OC\Share20\Share($rootFolder, $userManager));
+
 		$provider = $this->getInstance(['sendMailNotification', 'createShareActivity']);
 
 		$u1 = $userManager->createUser('testFed', md5(time()));

build/integration/features/bootstrap/SharingContext.php

@@ -42,9 +42,9 @@ 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');
-		$this->deleteServerConfig('sharebymail', 'enforcePasswordProtection');
 	}
 }

build/integration/sharing_features/sharing-v1.feature

@@ -84,7 +84,7 @@ Feature: sharing
   Scenario: Creating a new mail share with password when password protection is enforced
     Given dummy mail server is listening
     And As an "admin"
-    And parameter "enforcePasswordProtection" of app "sharebymail" is set to "yes"
+    And parameter "shareapi_enforce_links_password" of app "core" is set to "yes"
     And user "user0" exists
     And As an "user0"
     When creating a share with

lib/private/Share20/Manager.php

@@ -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;
@@ -228,9 +228,14 @@ class Manager implements IManager {
 				throw new \InvalidArgumentException('SharedWith is not a valid group');
 			}
 		} elseif ($share->getShareType() === IShare::TYPE_LINK) {
+			// No check for TYPE_EMAIL here as we have a recipient for them
 			if ($share->getSharedWith() !== null) {
 				throw new \InvalidArgumentException('SharedWith should be empty');
 			}
+		} elseif ($share->getShareType() === IShare::TYPE_EMAIL) {
+			if ($share->getSharedWith() === null) {
+				throw new \InvalidArgumentException('SharedWith should not be empty');
+			}
 		} elseif ($share->getShareType() === IShare::TYPE_REMOTE) {
 			if ($share->getSharedWith() === null) {
 				throw new \InvalidArgumentException('SharedWith should not be empty');
@@ -239,10 +244,6 @@ class Manager implements IManager {
 			if ($share->getSharedWith() === null) {
 				throw new \InvalidArgumentException('SharedWith should not be empty');
 			}
-		} elseif ($share->getShareType() === IShare::TYPE_EMAIL) {
-			if ($share->getSharedWith() === null) {
-				throw new \InvalidArgumentException('SharedWith should not be empty');
-			}
 		} elseif ($share->getShareType() === IShare::TYPE_CIRCLE) {
 			$circle = \OCA\Circles\Api\v1\Circles::detailsCircle($share->getSharedWith());
 			if ($circle === null) {
@@ -739,24 +740,23 @@ class Manager implements IManager {
 		}
 
 		try {
-			//Verify share type
+			// Verify share type
 			if ($share->getShareType() === IShare::TYPE_USER) {
 				$this->userCreateChecks($share);
 
-				//Verify the expiration date
+				// Verify the expiration date
 				$share = $this->validateExpirationDateInternal($share);
 			} elseif ($share->getShareType() === IShare::TYPE_GROUP) {
 				$this->groupCreateChecks($share);
 
-				//Verify the expiration date
+				// Verify the expiration date
 				$share = $this->validateExpirationDateInternal($share);
-			} elseif ($share->getShareType() === IShare::TYPE_LINK) {
+			} elseif ($share->getShareType() === IShare::TYPE_LINK
+				|| $share->getShareType() === IShare::TYPE_EMAIL) {
 				$this->linkCreateChecks($share);
 				$this->setLinkParent($share);
 
-				/*
-				 * For now ignore a set token.
-				 */
+				// For now ignore a set token.
 				$share->setToken(
 					$this->secureRandom->generate(
 						\OC\Share\Constants::TOKEN_LENGTH,
@@ -764,23 +764,17 @@ class Manager implements IManager {
 					)
 				);
 
-				//Verify the expiration date
+				// Verify the expiration date
 				$share = $this->validateExpirationDateLink($share);
 
-				//Verify the password
+				// 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()));
 				}
-			} elseif ($share->getShareType() === IShare::TYPE_EMAIL) {
-				$share->setToken(
-					$this->secureRandom->generate(
-						\OC\Share\Constants::TOKEN_LENGTH,
-						\OCP\Security\ISecureRandom::CHAR_HUMAN_READABLE
-					)
-				);
 			}
 
 			// Cannot share with the owner
@@ -804,7 +798,8 @@ class Manager implements IManager {
 			$oldShare = $share;
 			$provider = $this->factory->getProviderForType($share->getShareType());
 			$share = $provider->create($share);
-			//reuse the node we already have
+
+			// Reuse the node we already have
 			$share->setNode($oldShare->getNode());
 
 			// Reset the target if it is null for the new share
@@ -988,37 +983,42 @@ class Manager implements IManager {
 				$this->validateExpirationDateInternal($share);
 				$expirationDateUpdated = true;
 			}
-		} elseif ($share->getShareType() === IShare::TYPE_LINK) {
+		} elseif ($share->getShareType() === IShare::TYPE_LINK
+			|| $share->getShareType() === IShare::TYPE_EMAIL) {
 			$this->linkCreateChecks($share);
 
+			// 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);
 
-			$this->updateSharePasswordIfNeeded($share, $originalShare);
-
+			/**
+			 * 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()) {
+					throw new \InvalidArgumentException('Can’t enable sending the password by Talk without setting a new password');
+				}
+				if ($originalShare->getSendPasswordByTalk() && !$share->getSendPasswordByTalk()) {
+					throw new \InvalidArgumentException('Can’t disable sending the password by Talk without setting a new password');
+				}
+			}
 
 			if ($share->getExpirationDate() != $originalShare->getExpirationDate()) {
-				//Verify the expiration date
+				// Verify the expiration date
 				$this->validateExpirationDateLink($share);
 				$expirationDateUpdated = true;
 			}
-		} elseif ($share->getShareType() === IShare::TYPE_EMAIL) {
-			// The new password is not set again if it is the same as the old
-			// one.
-			$plainTextPassword = $share->getPassword();
-			if (!empty($plainTextPassword) && !$this->updateSharePasswordIfNeeded($share, $originalShare)) {
-				$plainTextPassword = null;
-			}
-			if (empty($plainTextPassword) && !$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');
-			} elseif (empty($plainTextPassword) && $originalShare->getSendPasswordByTalk() && !$share->getSendPasswordByTalk()) {
-				throw new \InvalidArgumentException('Can’t disable sending the password by Talk without setting a new password');
-			}
 		}
 
 		$this->pathCreateChecks($share->getNode());
@@ -1116,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 {
@@ -1218,7 +1222,8 @@ class Manager implements IManager {
 	 * @inheritdoc
 	 */
 	public function moveShare(IShare $share, $recipientId) {
-		if ($share->getShareType() === IShare::TYPE_LINK) {
+		if ($share->getShareType() === IShare::TYPE_LINK
+			|| $share->getShareType() === IShare::TYPE_EMAIL) {
 			throw new \InvalidArgumentException('Can’t change target of link share');
 		}
 
@@ -1480,10 +1485,10 @@ class Manager implements IManager {
 		$this->checkExpireDate($share);
 
 		/*
-		 * Reduce the permissions for link shares if public upload is not enabled
+		 * Reduce the permissions for link or email shares if public upload is not enabled
 		 */
-		if ($share->getShareType() === IShare::TYPE_LINK &&
-			!$this->shareApiLinkAllowPublicUpload()) {
+		if (($share->getShareType() === IShare::TYPE_LINK || $share->getShareType() === IShare::TYPE_EMAIL)
+			&& !$this->shareApiLinkAllowPublicUpload()) {
 			$share->setPermissions($share->getPermissions() & ~(\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE));
 		}
 

lib/private/Share20/ProviderFactory.php

@@ -43,6 +43,7 @@ use OCA\ShareByMail\ShareByMailProvider;
 use OCP\Defaults;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IServerContainer;
+use OCP\Share\IManager;
 use OCP\Share\IProviderFactory;
 use OCP\Share\IShare;
 use OCP\Share\IShareProvider;
@@ -195,7 +196,8 @@ class ProviderFactory implements IProviderFactory {
 				$settingsManager,
 				$this->serverContainer->query(Defaults::class),
 				$this->serverContainer->getHasher(),
-				$this->serverContainer->get(IEventDispatcher::class)
+				$this->serverContainer->get(IEventDispatcher::class),
+				$this->serverContainer->get(IManager::class)
 			);
 		}
 

lib/private/Share20/Share.php

@@ -40,7 +40,7 @@ use OCP\IUserManager;
 use OCP\Share\Exceptions\IllegalIDChangeException;
 use OCP\Share\IShare;
 
-class Share implements \OCP\Share\IShare {
+class Share implements IShare {
 
 	/** @var string */
 	private $id;

lib/public/Share/IShare.php

@@ -433,7 +433,7 @@ interface IShare {
 	 * When the share is passed to the share manager to be created
 	 * or updated the password will be hashed.
 	 *
-	 * @param string $password
+	 * @param string|null $password
 	 * @return \OCP\Share\IShare The modified object
 	 * @since 9.0.0
 	 */

tests/lib/Share20/ManagerTest.php

@@ -485,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'],
@@ -2203,17 +2203,19 @@ class ManagerTest extends \Test\TestCase {
 		$manager->expects($this->once())
 			->method('generalCreateChecks')
 			->with($share);
-		;
-		$manager->expects($this->never())
+
+		$manager->expects($this->once())
 			->method('linkCreateChecks');
 		$manager->expects($this->once())
 			->method('pathCreateChecks')
 			->with($path);
-		$manager->expects($this->never())
-			->method('validateExpirationDateLink');
-		$manager->expects($this->never())
+		$manager->expects($this->once())
+			->method('validateExpirationDateLink')
+			->with($share)
+			->willReturn($share);
+		$manager->expects($this->once())
 			->method('verifyPassword');
-		$manager->expects($this->never())
+		$manager->expects($this->once())
 			->method('setLinkParent');
 
 		$this->secureRandom->method('generate')
@@ -3203,8 +3205,8 @@ class ManagerTest extends \Test\TestCase {
 		$manager->expects($this->once())->method('generalCreateChecks')->with($share);
 		$manager->expects($this->once())->method('verifyPassword')->with('password');
 		$manager->expects($this->once())->method('pathCreateChecks')->with($file);
-		$manager->expects($this->never())->method('linkCreateChecks');
-		$manager->expects($this->never())->method('validateExpirationDateLink');
+		$manager->expects($this->once())->method('linkCreateChecks');
+		$manager->expects($this->once())->method('validateExpirationDateLink');
 
 		$this->hasher->expects($this->once())
 			->method('hash')
@@ -3218,7 +3220,12 @@ class ManagerTest extends \Test\TestCase {
 
 		$hookListener = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock();
 		\OCP\Util::connectHook('OCP\Share', 'post_set_expiration_date', $hookListener, 'post');
-		$hookListener->expects($this->never())->method('post');
+		$hookListener->expects($this->once())->method('post')->with([
+			'itemType' => 'file',
+			'itemSource' => 100,
+			'date' => $tomorrow,
+			'uidOwner' => 'owner',
+		]);
 
 		$hookListener2 = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock();
 		\OCP\Util::connectHook('OCP\Share', 'post_update_password', $hookListener2, 'post');
@@ -3281,8 +3288,8 @@ class ManagerTest extends \Test\TestCase {
 		$manager->expects($this->once())->method('generalCreateChecks')->with($share);
 		$manager->expects($this->once())->method('verifyPassword')->with('password');
 		$manager->expects($this->once())->method('pathCreateChecks')->with($file);
-		$manager->expects($this->never())->method('linkCreateChecks');
-		$manager->expects($this->never())->method('validateExpirationDateLink');
+		$manager->expects($this->once())->method('linkCreateChecks');
+		$manager->expects($this->once())->method('validateExpirationDateLink');
 
 		$this->hasher->expects($this->once())
 			->method('hash')
@@ -3296,7 +3303,12 @@ class ManagerTest extends \Test\TestCase {
 
 		$hookListener = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock();
 		\OCP\Util::connectHook('OCP\Share', 'post_set_expiration_date', $hookListener, 'post');
-		$hookListener->expects($this->never())->method('post');
+		$hookListener->expects($this->once())->method('post')->with([
+			'itemType' => 'file',
+			'itemSource' => 100,
+			'date' => $tomorrow,
+			'uidOwner' => 'owner',
+		]);
 
 		$hookListener2 = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock();
 		\OCP\Util::connectHook('OCP\Share', 'post_update_password', $hookListener2, 'post');
@@ -3359,8 +3371,8 @@ class ManagerTest extends \Test\TestCase {
 		$manager->expects($this->once())->method('generalCreateChecks')->with($share);
 		$manager->expects($this->once())->method('verifyPassword')->with('password');
 		$manager->expects($this->once())->method('pathCreateChecks')->with($file);
-		$manager->expects($this->never())->method('linkCreateChecks');
-		$manager->expects($this->never())->method('validateExpirationDateLink');
+		$manager->expects($this->once())->method('linkCreateChecks');
+		$manager->expects($this->once())->method('validateExpirationDateLink');
 
 		$this->hasher->expects($this->once())
 			->method('verify')
@@ -3379,7 +3391,12 @@ class ManagerTest extends \Test\TestCase {
 
 		$hookListener = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock();
 		\OCP\Util::connectHook('OCP\Share', 'post_set_expiration_date', $hookListener, 'post');
-		$hookListener->expects($this->never())->method('post');
+		$hookListener->expects($this->once())->method('post')->with([
+			'itemType' => 'file',
+			'itemSource' => 100,
+			'date' => $tomorrow,
+			'uidOwner' => 'owner',
+		]);
 
 		$hookListener2 = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock();
 		\OCP\Util::connectHook('OCP\Share', 'post_update_password', $hookListener2, 'post');
@@ -3400,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([
@@ -3445,9 +3462,10 @@ class ManagerTest extends \Test\TestCase {
 		$manager->expects($this->once())->method('generalCreateChecks')->with($share);
 		$manager->expects($this->never())->method('verifyPassword');
 		$manager->expects($this->never())->method('pathCreateChecks');
-		$manager->expects($this->never())->method('linkCreateChecks');
+		$manager->expects($this->once())->method('linkCreateChecks');
 		$manager->expects($this->never())->method('validateExpirationDateLink');
 
+		// If the password is empty, we have nothing to hash
 		$this->hasher->expects($this->never())
 			->method('hash');
 
@@ -3472,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([
@@ -3515,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->never())->method('linkCreateChecks');
+		$manager->expects($this->once())->method('linkCreateChecks');
 		$manager->expects($this->never())->method('validateExpirationDateLink');
 
+		// If the password is empty, we have nothing to hash
 		$this->hasher->expects($this->never())
 			->method('hash');
 
@@ -3544,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([
@@ -3587,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->never())->method('linkCreateChecks');
+		$manager->expects($this->once())->method('linkCreateChecks');
 		$manager->expects($this->never())->method('validateExpirationDateLink');
 
+		// If the password is empty, we have nothing to hash
 		$this->hasher->expects($this->never())
 			->method('hash');
 
@@ -3633,7 +3653,7 @@ class ManagerTest extends \Test\TestCase {
 		$originalShare = $this->manager->newShare();
 		$originalShare->setShareType(IShare::TYPE_EMAIL)
 			->setPermissions(\OCP\Constants::PERMISSION_ALL)
-			->setPassword('passwordHash')
+			->setPassword('password')
 			->setSendPasswordByTalk(false);
 
 		$tomorrow = new \DateTime();
@@ -3661,14 +3681,12 @@ class ManagerTest extends \Test\TestCase {
 		$manager->expects($this->once())->method('generalCreateChecks')->with($share);
 		$manager->expects($this->never())->method('verifyPassword');
 		$manager->expects($this->never())->method('pathCreateChecks');
-		$manager->expects($this->never())->method('linkCreateChecks');
+		$manager->expects($this->once())->method('linkCreateChecks');
 		$manager->expects($this->never())->method('validateExpirationDateLink');
 
-		$this->hasher->expects($this->once())
-			->method('verify')
-			->with('password', 'passwordHash')
-			->willReturn(true);
-
+		// 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');
 
@@ -3726,7 +3744,7 @@ class ManagerTest extends \Test\TestCase {
 			->setToken('token')
 			->setSharedBy('owner')
 			->setShareOwner('owner')
-			->setPassword('password')
+			->setPassword('passwordHash')
 			->setSendPasswordByTalk(false)
 			->setExpirationDate($tomorrow)
 			->setNode($file)
@@ -3737,14 +3755,12 @@ class ManagerTest extends \Test\TestCase {
 		$manager->expects($this->once())->method('generalCreateChecks')->with($share);
 		$manager->expects($this->never())->method('verifyPassword');
 		$manager->expects($this->never())->method('pathCreateChecks');
-		$manager->expects($this->never())->method('linkCreateChecks');
+		$manager->expects($this->once())->method('linkCreateChecks');
 		$manager->expects($this->never())->method('validateExpirationDateLink');
 
-		$this->hasher->expects($this->once())
-			->method('verify')
-			->with('password', 'passwordHash')
-			->willReturn(true);
-
+		// 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');
 
@@ -3813,12 +3829,12 @@ class ManagerTest extends \Test\TestCase {
 		$manager->expects($this->once())->method('generalCreateChecks')->with($share);
 		$manager->expects($this->never())->method('verifyPassword');
 		$manager->expects($this->never())->method('pathCreateChecks');
-		$manager->expects($this->never())->method('linkCreateChecks');
+		$manager->expects($this->once())->method('linkCreateChecks');
 		$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');