[stable28] Fix status check and saving of external storages

apps/files_external/css/settings.scss

@@ -24,6 +24,8 @@
 	/* overwrite conflicting core styles */
 	display: table-cell;
 	vertical-align: middle;
+	/* ensure width does not change even if internal span is not displayed */
+	width: 43px;
 }
 
 #externalStorage td.status > span {

apps/files_external/js/settings.js

@@ -770,6 +770,8 @@ MountConfigListView.prototype = _.extend({
 		storageConfig.backend = $target.val();
 		$tr.find('.mountPoint input').val('');
 
+		$tr.find('.selectBackend').prop('selectedIndex', 0)
+
 		var onCompletion = jQuery.Deferred();
 		$tr = this.newStorage(storageConfig, onCompletion);
 		$tr.find('.applicableToAllUsers').prop('checked', false).trigger('change');
@@ -878,7 +880,7 @@ MountConfigListView.prototype = _.extend({
 			$tr.find('.applicable,.mountOptionsToggle').empty();
 			$tr.find('.save').empty();
 			if (backend.invalid) {
-				this.updateStatus($tr, false, 'Unknown backend: ' + backend.name);
+				this.updateStatus($tr, false, t('files_external', 'Unknown backend: {backendName}', {backendName: backend.name}));
 			}
 			return $tr;
 		}
@@ -981,9 +983,10 @@ MountConfigListView.prototype = _.extend({
 				data: {'testOnly' : true},
 				contentType: 'application/json',
 				success: function(result) {
+					result = Object.values(result);
 					var onCompletion = jQuery.Deferred();
 					var $rows = $();
-					Object.values(result).forEach(function(storageParams) {
+					result.forEach(function(storageParams) {
 						var storageConfig;
 						var isUserGlobal = storageParams.type === 'system' && self._isPersonal;
 						storageParams.mountPoint = storageParams.mountPoint.substr(1); // trim leading slash
@@ -1012,6 +1015,13 @@ MountConfigListView.prototype = _.extend({
 							// userglobal storages do not expose configuration data
 							$tr.find('.configuration').text(t('files_external', 'Admin defined'));
 						}
+
+						// don't recheck config automatically when there are a large number of storages
+						if (result.length < 20) {
+							self.recheckStorageConfig($tr);
+						} else {
+							self.updateStatus($tr, StorageConfig.Status.INDETERMINATE, t('files_external', 'Automatic status checking is disabled due to the large number of configured storages, click to check status'));
+						}
 						$rows = $rows.add($tr);
 					});
 					initApplicableUsersMultiselect(self.$el.find('.applicableUsers'), this._userListLimit);
@@ -1230,8 +1240,9 @@ MountConfigListView.prototype = _.extend({
 					success: function () {
 						$tr.remove();
 					},
-					error: function () {
-						self.updateStatus($tr, StorageConfig.Status.ERROR);
+					error: function (result) {
+						const statusMessage = (result && result.responseJSON) ? result.responseJSON.message : undefined;
+						self.updateStatus($tr, StorageConfig.Status.ERROR, statusMessage);
 					}
 				});
 			}
@@ -1258,19 +1269,20 @@ MountConfigListView.prototype = _.extend({
 				if (concurrentTimer === undefined
 					|| $tr.data('save-timer') === concurrentTimer
 				) {
-					self.updateStatus($tr, result.status);
+					self.updateStatus($tr, result.status, result.statusMessage);
 					$tr.data('id', result.id);
 
 					if (_.isFunction(callback)) {
 						callback(storage);
 					}
 				}
 			},
-			error: function() {
+			error: function(result) {
 				if (concurrentTimer === undefined
 					|| $tr.data('save-timer') === concurrentTimer
 				) {
-					self.updateStatus($tr, StorageConfig.Status.ERROR);
+					const statusMessage = (result && result.responseJSON) ? result.responseJSON.message : undefined;
+					self.updateStatus($tr, StorageConfig.Status.ERROR, statusMessage);
 				}
 			}
 		});
@@ -1294,8 +1306,9 @@ MountConfigListView.prototype = _.extend({
 			success: function(result) {
 				self.updateStatus($tr, result.status, result.statusMessage);
 			},
-			error: function() {
-				self.updateStatus($tr, StorageConfig.Status.ERROR);
+			error: function(result) {
+				const statusMessage = (result && result.responseJSON) ? result.responseJSON.message : undefined;
+				self.updateStatus($tr, StorageConfig.Status.ERROR, statusMessage);
 			}
 		});
 	},
@@ -1312,6 +1325,7 @@ MountConfigListView.prototype = _.extend({
 		switch (status) {
 			case null:
 				// remove status
+				$statusSpan.hide();
 				break;
 			case StorageConfig.Status.IN_PROGRESS:
 				$statusSpan.attr('class', 'icon-loading-small');
@@ -1325,12 +1339,13 @@ MountConfigListView.prototype = _.extend({
 			default:
 				$statusSpan.attr('class', 'error icon-error-white');
 		}
-		if (typeof message === 'string') {
-			$statusSpan.attr('title', message);
-			$statusSpan.tooltip();
-		} else {
-			$statusSpan.tooltip('dispose');
+		if (status !== null) {
+			$statusSpan.show();
+		}
+		if (typeof message !== 'string') {
+			message = t('files_external', 'Click to recheck the configuration');
 		}
+		$statusSpan.attr('title', message);
 	},
 
 	/**

apps/files_external/lib/Lib/Auth/Password/UserGlobalAuth.php

@@ -29,6 +29,7 @@
 namespace OCA\Files_External\Lib\Auth\Password;
 
 use OCA\Files_External\Lib\Auth\AuthMechanism;
+use OCA\Files_External\Lib\DefinitionParameter;
 use OCA\Files_External\Lib\InsufficientDataForMeaningfulAnswerException;
 use OCA\Files_External\Lib\StorageConfig;
 use OCA\Files_External\Service\BackendService;
@@ -61,6 +62,12 @@ public function saveBackendOptions(IUser $user, $id, $backendOptions) {
 		if (!isset($backendOptions['user']) && !isset($backendOptions['password'])) {
 			return;
 		}
+
+		if ($backendOptions['password'] === DefinitionParameter::UNMODIFIED_PLACEHOLDER) {
+			$oldCredentials = $this->credentialsManager->retrieve($user->getUID(), self::CREDENTIALS_IDENTIFIER);
+			$backendOptions['password'] = $oldCredentials['password'];
+		}
+
 		// make sure we're not setting any unexpected keys
 		$credentials = [
 			'user' => $backendOptions['user'],

apps/files_external/lib/Lib/Auth/Password/UserProvided.php

@@ -65,6 +65,11 @@ private function getCredentialsIdentifier($storageId) {
 	}
 
 	public function saveBackendOptions(IUser $user, $mountId, array $options) {
+		if ($options['password'] === DefinitionParameter::UNMODIFIED_PLACEHOLDER) {
+			$oldCredentials = $this->credentialsManager->retrieve($user->getUID(), $this->getCredentialsIdentifier($mountId));
+			$options['password'] = $oldCredentials['password'];
+		}
+
 		$this->credentialsManager->store($user->getUID(), $this->getCredentialsIdentifier($mountId), [
 			'user' => $options['user'], // explicitly copy the fields we want instead of just passing the entire $options array
 			'password' => $options['password'] // this way we prevent users from being able to modify any other field

apps/files_external/templates/settings.php

@@ -140,7 +140,7 @@ function writeParameterInput($parameter, $options, $classes = []) {
 			<?php endif; ?>
 			>
 				<td class="status">
-					<span data-placement="right" title="<?php p($l->t('Click to recheck the configuration')); ?>"></span>
+					<span data-placement="right" title="<?php p($l->t('Click to recheck the configuration')); ?>" style="display: none;"></span>
 				</td>
 				<td class="mountPoint"><input type="text" name="mountPoint" value=""
 					placeholder="<?php p($l->t('Folder name')); ?>">

build/integration/features/bootstrap/BasicStructure.php

@@ -354,7 +354,7 @@ public function sendingAToWithRequesttoken($method, $url, $body = null) {
 			$fd = $body->getRowsHash();
 			$options['form_params'] = $fd;
 		} elseif ($body) {
-			$options = array_merge($options, $body);
+			$options = array_merge_recursive($options, $body);
 		}
 
 		$client = new Client();

build/integration/features/bootstrap/ExternalStorage.php

@@ -0,0 +1,103 @@
+<?php
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+use Behat\Gherkin\Node\TableNode;
+use PHPUnit\Framework\Assert;
+
+require __DIR__ . '/../../vendor/autoload.php';
+
+trait ExternalStorage {
+	private array $storageIds = [];
+
+	private array $lastExternalStorageData;
+
+	/**
+	 * @AfterScenario
+	 **/
+	public function deleteCreatedStorages(): void {
+		foreach ($this->storageIds as $storageId) {
+			$this->deleteStorage($storageId);
+		}
+		$this->storageIds = [];
+	}
+
+	private function deleteStorage(string $storageId): void {
+		// Based on "runOcc" from CommandLine trait
+		$args = ['files_external:delete', '--yes', $storageId];
+		$args = array_map(function ($arg) {
+			return escapeshellarg($arg);
+		}, $args);
+		$args[] = '--no-ansi --no-warnings';
+		$args = implode(' ', $args);
+
+		$descriptor = [
+			0 => ['pipe', 'r'],
+			1 => ['pipe', 'w'],
+			2 => ['pipe', 'w'],
+		];
+		$process = proc_open('php console.php ' . $args, $descriptor, $pipes, $ocPath = '../..');
+		$lastStdOut = stream_get_contents($pipes[1]);
+		proc_close($process);
+	}
+
+	/**
+	 * @When logged in user creates external global storage
+	 *
+	 * @param TableNode $fields
+	 */
+	public function loggedInUserCreatesExternalGlobalStorage(TableNode $fields): void {
+		$this->sendJsonWithRequestToken('POST', '/index.php/apps/files_external/globalstorages', $fields);
+		$this->theHTTPStatusCodeShouldBe('201');
+
+		$this->lastExternalStorageData = json_decode($this->response->getBody(), $asAssociativeArray = true);
+
+		$this->storageIds[] = $this->lastExternalStorageData['id'];
+	}
+
+	/**
+	 * @When logged in user updates last external userglobal storage
+	 *
+	 * @param TableNode $fields
+	 */
+	public function loggedInUserUpdatesLastExternalUserglobalStorage(TableNode $fields): void {
+		$this->sendJsonWithRequestToken('PUT', '/index.php/apps/files_external/userglobalstorages/' . $this->lastExternalStorageData['id'], $fields);
+		$this->theHTTPStatusCodeShouldBe('200');
+
+		$this->lastExternalStorageData = json_decode($this->response->getBody(), $asAssociativeArray = true);
+	}
+
+	/**
+	 * @Then fields of last external storage match with
+	 *
+	 * @param TableNode $fields
+	 */
+	public function fieldsOfLastExternalStorageMatchWith(TableNode $fields): void {
+		foreach ($fields->getRowsHash() as $expectedField => $expectedValue) {
+			if (!array_key_exists($expectedField, $this->lastExternalStorageData)) {
+				Assert::fail("$expectedField was not found in response");
+			}
+
+			Assert::assertEquals($expectedValue, $this->lastExternalStorageData[$expectedField], "Field '$expectedField' does not match ({$this->lastExternalStorageData[$expectedField]})");
+		}
+	}
+
+	private function sendJsonWithRequestToken(string $method, string $url, TableNode $fields): void {
+		$isFirstField = true;
+		$fieldsAsJsonString = '{';
+		foreach ($fields->getRowsHash() as $key => $value) {
+			$fieldsAsJsonString .= ($isFirstField ? '' : ',') . '"' . $key . '":' . $value;
+			$isFirstField = false;
+		}
+		$fieldsAsJsonString .= '}';
+
+		$body = [
+			'headers' => [
+				'Content-Type' => 'application/json',
+			],
+			'body' => $fieldsAsJsonString,
+		];
+		$this->sendingAToWithRequesttoken($method, $url, $body);
+	}
+}

build/integration/features/bootstrap/FeatureContext.php

@@ -34,6 +34,7 @@
  */
 class FeatureContext implements Context, SnippetAcceptingContext {
 	use ContactsMenu;
+	use ExternalStorage;
 	use Search;
 	use WebDav;
 	use Trashbin;