[stable29] 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,9 +1339,13 @@ MountConfigListView.prototype = _.extend({
  default:
  $statusSpan.attr('class', 'error icon-error-white');
  }
- if (typeof message === 'string') {
- $statusSpan.attr('title', message);
+ 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;