From 0d112d7901983a568f6a803f59f240afd434db61 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 12 Mar 2020 19:45:23 +0100 Subject: [PATCH] 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 --- .../Controller/GlobalStoragesController.php | 4 +- .../lib/Controller/StoragesController.php | 78 ++++++++++++------- .../UserGlobalStoragesController.php | 6 +- .../lib/Controller/UserStoragesController.php | 4 +- .../lib/Lib/Auth/AuthMechanism.php | 1 - .../lib/Lib/DefinitionParameter.php | 3 + .../lib/Lib/FrontendDefinitionTrait.php | 1 - .../lib/Service/StoragesService.php | 5 +- 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 cf9eb718d5..c8fc4b00ef 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 6b3bb5d6a5..8b1b0cb7a6 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 b564adc581..8e0dac5a89 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 6f55af96d1..46285c4758 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 891719eebb..cd8f8242e3 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 e1f8ed733a..7250a77e6c 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 c1f6e5ce1f..58e2d5ffdc 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 61c011a151..60ccec1320 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);