Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Move storage constructor to specific interface #48111

Merged
merged 3 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions apps/files_external/lib/Config/ConfigAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@
use OCA\Files_External\Service\UserStoragesService;
use OCP\Files\Config\IMountProvider;
use OCP\Files\ObjectStore\IObjectStore;
use OCP\Files\Storage\IConstructableStorage;
use OCP\Files\Storage\IStorage;
use OCP\Files\Storage\IStorageFactory;
use OCP\Files\StorageNotAvailableException;
use OCP\IUser;
use Psr\Clock\ClockInterface;
use Psr\Log\LoggerInterface;

/**
* Make the old files_external config work with the new public mount config api
Expand Down Expand Up @@ -48,7 +50,7 @@
if (!is_subclass_of($objectClass, IObjectStore::class)) {
throw new \InvalidArgumentException('Invalid object store');
}
$storage->setBackendOption('objectstore', new $objectClass($objectStore));

Check failure on line 53 in apps/files_external/lib/Config/ConfigAdapter.php

View workflow job for this annotation

GitHub Actions / static-code-analysis-security

TaintedCallable

apps/files_external/lib/Config/ConfigAdapter.php:53:50: TaintedCallable: Detected tainted text (see https://psalm.dev/243)
}

$storage->getAuthMechanism()->manipulateStorageConfig($storage, $user);
Expand All @@ -62,6 +64,9 @@
*/
private function constructStorage(StorageConfig $storageConfig): IStorage {
$class = $storageConfig->getBackend()->getStorageClass();
if (!$class instanceof IConstructableStorage) {
\OCP\Server::get(LoggerInterface::class)->warning('Building a storage not implementing IConstructableStorage is deprecated since 31.0.0', ['class' => $class]);
}
$storage = new $class($storageConfig->getBackendOptions());

// auth mechanism should fire first
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,7 @@
'OCP\\Files\\StorageNotAvailableException' => $baseDir . '/lib/public/Files/StorageNotAvailableException.php',
'OCP\\Files\\StorageTimeoutException' => $baseDir . '/lib/public/Files/StorageTimeoutException.php',
'OCP\\Files\\Storage\\IChunkedFileWrite' => $baseDir . '/lib/public/Files/Storage/IChunkedFileWrite.php',
'OCP\\Files\\Storage\\IConstructableStorage' => $baseDir . '/lib/public/Files/Storage/IConstructableStorage.php',
'OCP\\Files\\Storage\\IDisableEncryptionStorage' => $baseDir . '/lib/public/Files/Storage/IDisableEncryptionStorage.php',
'OCP\\Files\\Storage\\ILockingStorage' => $baseDir . '/lib/public/Files/Storage/ILockingStorage.php',
'OCP\\Files\\Storage\\INotifyStorage' => $baseDir . '/lib/public/Files/Storage/INotifyStorage.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\Files\\StorageNotAvailableException' => __DIR__ . '/../../..' . '/lib/public/Files/StorageNotAvailableException.php',
'OCP\\Files\\StorageTimeoutException' => __DIR__ . '/../../..' . '/lib/public/Files/StorageTimeoutException.php',
'OCP\\Files\\Storage\\IChunkedFileWrite' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IChunkedFileWrite.php',
'OCP\\Files\\Storage\\IConstructableStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IConstructableStorage.php',
'OCP\\Files\\Storage\\IDisableEncryptionStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IDisableEncryptionStorage.php',
'OCP\\Files\\Storage\\ILockingStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/ILockingStorage.php',
'OCP\\Files\\Storage\\INotifyStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/INotifyStorage.php',
Expand Down
3 changes: 2 additions & 1 deletion lib/private/Files/Storage/Common.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use OCP\Files\GenericFileException;
use OCP\Files\IFilenameValidator;
use OCP\Files\InvalidPathException;
use OCP\Files\Storage\IConstructableStorage;
use OCP\Files\Storage\ILockingStorage;
use OCP\Files\Storage\IStorage;
use OCP\Files\Storage\IWriteStreamStorage;
Expand All @@ -41,7 +42,7 @@
* Some \OC\Files\Storage\Common methods call functions which are first defined
* in classes which extend it, e.g. $this->stat() .
*/
abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage, IConstructableStorage {
Fixed Show fixed Hide fixed
use LocalTempFileTrait;

protected ?Cache $cache = null;
Expand Down
5 changes: 5 additions & 0 deletions lib/private/Files/Storage/StorageFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
namespace OC\Files\Storage;

use OCP\Files\Mount\IMountPoint;
use OCP\Files\Storage\IConstructableStorage;
use OCP\Files\Storage\IStorage;
use OCP\Files\Storage\IStorageFactory;
use Psr\Log\LoggerInterface;

class StorageFactory implements IStorageFactory {
/**
Expand Down Expand Up @@ -62,6 +64,9 @@ public function removeStorageWrapper($wrapperName) {
* @return IStorage
*/
public function getInstance(IMountPoint $mountPoint, $class, $arguments) {
if (!($class instanceof IConstructableStorage)) {
Fixed Show fixed Hide fixed
\OCP\Server::get(LoggerInterface::class)->warning('Building a storage not implementing IConstructableStorage is deprecated since 31.0.0', ['class' => $class]);
}
return $this->wrap($mountPoint, new $class($arguments));
}

Expand Down
26 changes: 26 additions & 0 deletions lib/public/Files/Storage/IConstructableStorage.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-only
*/
// use OCP namespace for all classes that are considered public.
// This means that they should be used by apps instead of the internal Nextcloud classes
provokateurin marked this conversation as resolved.
Show resolved Hide resolved

namespace OCP\Files\Storage;

/**
* Marks a storage as constructable. Allows to pass the storage as a string to a mounpoint and let it build the instance.
*
* @since 31.0.0
*/
interface IConstructableStorage {
/**
* @param array $parameters is a free form array with the configuration options needed to construct the storage
*
* @since 31.0.0
*/
public function __construct(array $parameters);
}
9 changes: 1 addition & 8 deletions lib/public/Files/Storage/IStorage.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,9 @@
* All paths passed to the storage are relative to the storage and should NOT have a leading slash.
*
* @since 9.0.0
* @since 31.0.0 Moved the constructor to IConstructableStorage so that wrappers can use DI
*/
interface IStorage {
/**
* $parameters is a free form array with the configuration options needed to construct the storage
*
* @param array $parameters
* @since 9.0.0
*/
public function __construct($parameters);

/**
* Get the identifier for the storage,
* the returned id should be the same for every storage object that is created with the same parameters
Expand Down
1 change: 0 additions & 1 deletion tests/lib/Files/ViewTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1589,7 +1589,6 @@ private function createTestMovableMountPoints($mountPoints) {
foreach ($mountPoints as $mountPoint) {
$storage = $this->getMockBuilder(Storage::class)
->setMethods([])
->setConstructorArgs([[]])
->getMock();
$storage->method('getId')->willReturn('non-null-id');
$storage->method('getStorageCache')->willReturnCallback(function () use ($storage) {
Expand Down
Loading