Use placeholder values for password fields in external storage webui

This prevents the password from being sent to the webui.

While an admin will always be able to retrieve the passwords (as they
can do arbitrairy code execution by design) this prevents casual
password snooping

Signed-off-by: Robin Appelman <robin@icewind.nl>

8 files changed, 65 insertions, 37 deletions

diff --git a/apps/files_external/lib/Controller/GlobalStoragesController.php b/apps/files_external/lib/Controller/GlobalStoragesController.php
index cf9eb718d5d..c8fc4b00ef8 100644
--- a/apps/files_external/lib/Controller/GlobalStoragesController.php
+++ b/apps/files_external/lib/Controller/GlobalStoragesController.php
@@ -113,7 +113,7 @@ class GlobalStoragesController extends StoragesController {
 		$this->updateStorageStatus($newStorage);
 
 		return new DataResponse(
-			$newStorage,
+			$this->formatStorageForUI($newStorage),
 			Http::STATUS_CREATED
 		);
 	}
@@ -180,7 +180,7 @@ class GlobalStoragesController extends StoragesController {
 		$this->updateStorageStatus($storage, $testOnly);
 
 		return new DataResponse(
-			$storage,
+			$this->formatStorageForUI($storage),
 			Http::STATUS_OK
 		);
 
diff --git a/apps/files_external/lib/Controller/StoragesController.php b/apps/files_external/lib/Controller/StoragesController.php
index 6b3bb5d6a53..8b1b0cb7a6c 100644
--- a/apps/files_external/lib/Controller/StoragesController.php
+++ b/apps/files_external/lib/Controller/StoragesController.php
@@ -31,6 +31,7 @@ namespace OCA\Files_External\Controller;
 
 use OCA\Files_External\Lib\Auth\AuthMechanism;
 use OCA\Files_External\Lib\Backend\Backend;
+use OCA\Files_External\Lib\DefinitionParameter;
 use OCA\Files_External\Lib\InsufficientDataForMeaningfulAnswerException;
 use OCA\Files_External\Lib\StorageConfig;
 use OCA\Files_External\NotFoundException;
@@ -146,9 +147,9 @@ abstract class StoragesController extends Controller {
 		$mountPoint = $storage->getMountPoint();
 		if ($mountPoint === '') {
 			return new DataResponse(
-				array(
-					'message' => (string)$this->l10n->t('Invalid mount point')
-				),
+				[
+					'message' => (string)$this->l10n->t('Invalid mount point'),
+				],
 				Http::STATUS_UNPROCESSABLE_ENTITY
 			);
 		}
@@ -156,9 +157,9 @@ abstract class StoragesController extends Controller {
 		if ($storage->getBackendOption('objectstore')) {
 			// objectstore must not be sent from client side
 			return new DataResponse(
-				array(
-					'message' => (string)$this->l10n->t('Objectstore forbidden')
-				),
+				[
+					'message' => (string)$this->l10n->t('Objectstore forbidden'),
+				],
 				Http::STATUS_UNPROCESSABLE_ENTITY
 			);
 		}
@@ -170,11 +171,11 @@ abstract class StoragesController extends Controller {
 		if ($backend->checkDependencies()) {
 			// invalid backend
 			return new DataResponse(
-				array(
+				[
 					'message' => (string)$this->l10n->t('Invalid storage backend "%s"', [
-						$backend->getIdentifier()
-					])
-				),
+						$backend->getIdentifier(),
+					]),
+				],
 				Http::STATUS_UNPROCESSABLE_ENTITY
 			);
 		}
@@ -182,22 +183,22 @@ abstract class StoragesController extends Controller {
 		if (!$backend->isVisibleFor($this->service->getVisibilityType())) {
 			// not permitted to use backend
 			return new DataResponse(
-				array(
+				[
 					'message' => (string)$this->l10n->t('Not permitted to use backend "%s"', [
-						$backend->getIdentifier()
-					])
-				),
+						$backend->getIdentifier(),
+					]),
+				],
 				Http::STATUS_UNPROCESSABLE_ENTITY
 			);
 		}
 		if (!$authMechanism->isVisibleFor($this->service->getVisibilityType())) {
 			// not permitted to use auth mechanism
 			return new DataResponse(
-				array(
+				[
 					'message' => (string)$this->l10n->t('Not permitted to use authentication mechanism "%s"', [
-						$authMechanism->getIdentifier()
-					])
-				),
+						$authMechanism->getIdentifier(),
+					]),
+				],
 				Http::STATUS_UNPROCESSABLE_ENTITY
 			);
 		}
@@ -205,9 +206,9 @@ abstract class StoragesController extends Controller {
 		if (!$backend->validateStorage($storage)) {
 			// unsatisfied parameters
 			return new DataResponse(
-				array(
-					'message' => (string)$this->l10n->t('Unsatisfied backend parameters')
-				),
+				[
+					'message' => (string)$this->l10n->t('Unsatisfied backend parameters'),
+				],
 				Http::STATUS_UNPROCESSABLE_ENTITY
 			);
 		}
@@ -215,7 +216,7 @@ abstract class StoragesController extends Controller {
 			// unsatisfied parameters
 			return new DataResponse(
 				[
-					'message' => (string)$this->l10n->t('Unsatisfied authentication mechanism parameters')
+					'message' => (string)$this->l10n->t('Unsatisfied authentication mechanism parameters'),
 				],
 				Http::STATUS_UNPROCESSABLE_ENTITY
 			);
@@ -272,7 +273,7 @@ abstract class StoragesController extends Controller {
 			// FIXME: convert storage exceptions to StorageNotAvailableException
 			$storage->setStatus(
 				StorageNotAvailableException::STATUS_ERROR,
-				get_class($e).': '.$e->getMessage()
+				get_class($e) . ': ' . $e->getMessage()
 			);
 		}
 	}
@@ -283,7 +284,7 @@ abstract class StoragesController extends Controller {
 	 * @return DataResponse
 	 */
 	public function index() {
-		$storages = $this->service->getStorages();
+		$storages = $this->formatStoragesForUI($this->service->getStorages());
 
 		return new DataResponse(
 			$storages,
@@ -291,6 +292,29 @@ abstract class StoragesController extends Controller {
 		);
 	}
 
+	protected function formatStoragesForUI(array $storages): array {
+		return array_map(function ($storage) {
+			return $this->formatStorageForUI($storage);
+		}, $storages);
+	}
+
+	protected function formatStorageForUI(StorageConfig $storage): StorageConfig {
+		/** @var DefinitionParameter[] $parameters */
+		$parameters = array_merge($storage->getBackend()->getParameters(), $storage->getAuthMechanism()->getParameters());
+
+		$options = $storage->getBackendOptions();
+		foreach ($options as $key => $value) {
+			foreach ($parameters as $parameter) {
+				if ($parameter->getName() === $key && $parameter->getType() === DefinitionParameter::VALUE_PASSWORD) {
+					$storage->setBackendOption($key, DefinitionParameter::UNMODIFIED_PLACEHOLDER);
+					break;
+				}
+			}
+		}
+
+		return $storage;
+	}
+
 	/**
 	 * Get an external storage entry.
 	 *
@@ -307,14 +331,14 @@ abstract class StoragesController extends Controller {
 		} catch (NotFoundException $e) {
 			return new DataResponse(
 				[
-					'message' => (string)$this->l10n->t('Storage with ID "%d" not found', array($id))
+					'message' => (string)$this->l10n->t('Storage with ID "%d" not found', [$id]),
 				],
 				Http::STATUS_NOT_FOUND
 			);
 		}
 
 		return new DataResponse(
-			$storage,
+			$this->formatStorageForUI($storage),
 			Http::STATUS_OK
 		);
 	}
@@ -332,7 +356,7 @@ abstract class StoragesController extends Controller {
 		} catch (NotFoundException $e) {
 			return new DataResponse(
 				[
-					'message' => (string)$this->l10n->t('Storage with ID "%d" not found', array($id))
+					'message' => (string)$this->l10n->t('Storage with ID "%d" not found', [$id]),
 				],
 				Http::STATUS_NOT_FOUND
 			);
diff --git a/apps/files_external/lib/Controller/UserGlobalStoragesController.php b/apps/files_external/lib/Controller/UserGlobalStoragesController.php
index b564adc5811..8e0dac5a898 100644
--- a/apps/files_external/lib/Controller/UserGlobalStoragesController.php
+++ b/apps/files_external/lib/Controller/UserGlobalStoragesController.php
@@ -85,7 +85,7 @@ class UserGlobalStoragesController extends StoragesController {
 	 * @NoAdminRequired
 	 */
 	public function index() {
-		$storages = $this->service->getUniqueStorages();
+		$storages = $this->formatStoragesForUI($this->service->getUniqueStorages());
 
 		// remove configuration data, this must be kept private
 		foreach ($storages as $storage) {
@@ -133,7 +133,7 @@ class UserGlobalStoragesController extends StoragesController {
 		$this->sanitizeStorage($storage);
 
 		return new DataResponse(
-			$storage,
+			$this->formatStorageForUI($storage),
 			Http::STATUS_OK
 		);
 	}
@@ -182,7 +182,7 @@ class UserGlobalStoragesController extends StoragesController {
 		$this->sanitizeStorage($storage);
 
 		return new DataResponse(
-			$storage,
+			$this->formatStorageForUI($storage),
 			Http::STATUS_OK
 		);
 
diff --git a/apps/files_external/lib/Controller/UserStoragesController.php b/apps/files_external/lib/Controller/UserStoragesController.php
index 6f55af96d1d..46285c4758a 100644
--- a/apps/files_external/lib/Controller/UserStoragesController.php
+++ b/apps/files_external/lib/Controller/UserStoragesController.php
@@ -148,7 +148,7 @@ class UserStoragesController extends StoragesController {
 		$this->updateStorageStatus($newStorage);
 
 		return new DataResponse(
-			$newStorage,
+			$this->formatStorageForUI($newStorage),
 			Http::STATUS_CREATED
 		);
 	}
@@ -208,7 +208,7 @@ class UserStoragesController extends StoragesController {
 		$this->updateStorageStatus($storage, $testOnly);
 
 		return new DataResponse(
-			$storage,
+			$this->formatStorageForUI($storage),
 			Http::STATUS_OK
 		);
 
diff --git a/apps/files_external/lib/Lib/Auth/AuthMechanism.php b/apps/files_external/lib/Lib/Auth/AuthMechanism.php
index 891719eebb1..cd8f8242e30 100644
--- a/apps/files_external/lib/Lib/Auth/AuthMechanism.php
+++ b/apps/files_external/lib/Lib/Auth/AuthMechanism.php
@@ -51,7 +51,6 @@ use OCA\Files_External\Lib\VisibilityTrait;
  *      Object can affect storage mounting
  */
 class AuthMechanism implements \JsonSerializable {
-
 	/** Standard authentication schemes */
 	const SCHEME_NULL = 'null';
 	const SCHEME_BUILTIN = 'builtin';
diff --git a/apps/files_external/lib/Lib/DefinitionParameter.php b/apps/files_external/lib/Lib/DefinitionParameter.php
index e1f8ed733a1..7250a77e6c1 100644
--- a/apps/files_external/lib/Lib/DefinitionParameter.php
+++ b/apps/files_external/lib/Lib/DefinitionParameter.php
@@ -27,6 +27,9 @@ namespace OCA\Files_External\Lib;
  * Parameter for an external storage definition
  */
 class DefinitionParameter implements \JsonSerializable {
+	// placeholder value for password fields, when the client updates a storage configuration
+	// placeholder values are ignored and the field is left unmodified
+	const UNMODIFIED_PLACEHOLDER = '__unmodified__';
 
 	/** Value constants */
 	const VALUE_TEXT = 0;
diff --git a/apps/files_external/lib/Lib/FrontendDefinitionTrait.php b/apps/files_external/lib/Lib/FrontendDefinitionTrait.php
index c1f6e5ce1f3..58e2d5ffdcf 100644
--- a/apps/files_external/lib/Lib/FrontendDefinitionTrait.php
+++ b/apps/files_external/lib/Lib/FrontendDefinitionTrait.php
@@ -154,5 +154,4 @@ trait FrontendDefinitionTrait {
 		}
 		return true;
 	}
-
 }
diff --git a/apps/files_external/lib/Service/StoragesService.php b/apps/files_external/lib/Service/StoragesService.php
index 61c011a1517..60ccec13206 100644
--- a/apps/files_external/lib/Service/StoragesService.php
+++ b/apps/files_external/lib/Service/StoragesService.php
@@ -36,6 +36,7 @@ use OCA\Files_External\Lib\Auth\AuthMechanism;
 use OCA\Files_External\Lib\Auth\InvalidAuth;
 use OCA\Files_External\Lib\Backend\Backend;
 use OCA\Files_External\Lib\Backend\InvalidBackend;
+use OCA\Files_External\Lib\DefinitionParameter;
 use OCA\Files_External\Lib\StorageConfig;
 use OCA\Files_External\NotFoundException;
 use OCP\Files\Config\IUserMountCache;
@@ -427,7 +428,9 @@ abstract class StoragesService {
 		$changedOptions = array_diff_assoc($updatedStorage->getMountOptions(), $oldStorage->getMountOptions());
 
 		foreach ($changedConfig as $key => $value) {
-			$this->dbConfig->setConfig($id, $key, $value);
+			if ($value !== DefinitionParameter::UNMODIFIED_PLACEHOLDER) {
+				$this->dbConfig->setConfig($id, $key, $value);
+			}
 		}
 		foreach ($changedOptions as $key => $value) {
 			$this->dbConfig->setOption($id, $key, $value);