diff --git a/apps/files_external/lib/Lib/Storage/SMB.php b/apps/files_external/lib/Lib/Storage/SMB.php index 0d8c10b316..5e4af8f46b 100644 --- a/apps/files_external/lib/Lib/Storage/SMB.php +++ b/apps/files_external/lib/Lib/Storage/SMB.php @@ -55,6 +55,7 @@ use OCA\Files_External\Lib\Notify\SMBNotifyHandler; use OCP\Files\Notify\IChange; use OCP\Files\Notify\IRenameChange; use OCP\Files\Storage\INotifyStorage; +use OCP\Files\StorageAuthException; use OCP\Files\StorageNotAvailableException; use OCP\ILogger; @@ -160,7 +161,7 @@ class SMB extends Common implements INotifyStorage { /** * @param string $path * @return \Icewind\SMB\IFileInfo - * @throws StorageNotAvailableException + * @throws StorageAuthException */ protected function getFileInfo($path) { try { @@ -170,11 +171,26 @@ class SMB extends Common implements INotifyStorage { } return $this->statCache[$path]; } catch (ConnectException $e) { - $this->logger->logException($e, ['message' => 'Error while getting file info']); - throw new StorageNotAvailableException($e->getMessage(), $e->getCode(), $e); + $this->throwUnavailable($e); + } catch (ForbiddenException $e) { + // with php-smbclient, this exceptions is thrown when the provided password is invalid. + // Possible is also ForbiddenException with a different error code, so we check it. + if($e->getCode() === 1) { + $this->throwUnavailable($e); + } + throw $e; } } + /** + * @param \Exception $e + * @throws StorageAuthException + */ + protected function throwUnavailable(\Exception $e) { + $this->logger->logException($e, ['message' => 'Error while getting file info']); + throw new StorageAuthException($e->getMessage(), $e); + } + /** * @param string $path * @return \Icewind\SMB\IFileInfo[] diff --git a/config/config.sample.php b/config/config.sample.php index d7f9b21706..610a8b7720 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1480,6 +1480,17 @@ $CONFIG = array( */ 'quota_include_external_storage' => false, +/** + * When an external storage is unavailable for some reasons, it will be flagged + * as such for 10 minutes. When the trigger is a failed authentication attempt + * the delay is higher and can be controlled with this option. The motivation + * is to make account lock outs at Active Directories (and compatible) more + * unlikely. + * + * Defaults to ``1800`` (seconds) + */ +'external_storage.auth_availability_delay' => 1800, + /** * Specifies how often the local filesystem (the Nextcloud data/ directory, and * NFS mounts in data/) is checked for changes made outside Nextcloud. This diff --git a/lib/private/Files/Cache/Storage.php b/lib/private/Files/Cache/Storage.php index 5b37c1f43f..72aaac11c5 100644 --- a/lib/private/Files/Cache/Storage.php +++ b/lib/private/Files/Cache/Storage.php @@ -166,11 +166,12 @@ class Storage { /** * @param bool $isAvailable + * @param int $delay amount of seconds to delay reconsidering that storage further */ - public function setAvailability($isAvailable) { + public function setAvailability($isAvailable, int $delay = 0) { $sql = 'UPDATE `*PREFIX*storages` SET `available` = ?, `last_checked` = ? WHERE `id` = ?'; $available = $isAvailable ? 1 : 0; - \OC_DB::executeAudited($sql, array($available, time(), $this->storageId)); + \OC_DB::executeAudited($sql, [$available, time() + $delay, $this->storageId]); } /** diff --git a/lib/private/Files/Storage/Wrapper/Availability.php b/lib/private/Files/Storage/Wrapper/Availability.php index 5b957ae036..77a23d26fb 100644 --- a/lib/private/Files/Storage/Wrapper/Availability.php +++ b/lib/private/Files/Storage/Wrapper/Availability.php @@ -24,6 +24,9 @@ namespace OC\Files\Storage\Wrapper; use OCP\Files\Storage\IStorage; +use OCP\Files\StorageAuthException; +use OCP\Files\StorageNotAvailableException; +use OCP\IConfig; /** * Availability checker for storages @@ -33,6 +36,14 @@ use OCP\Files\Storage\IStorage; class Availability extends Wrapper { const RECHECK_TTL_SEC = 600; // 10 minutes + /** @var IConfig */ + protected $config; + + public function __construct($parameters) { + $this->config = $parameters['config'] ?? \OC::$server->getConfig(); + parent::__construct($parameters); + } + public static function shouldRecheck($availability) { if (!$availability['available']) { // trigger a recheck if TTL reached @@ -72,11 +83,11 @@ class Availability extends Wrapper { } /** - * @throws \OCP\Files\StorageNotAvailableException + * @throws StorageNotAvailableException */ private function checkAvailability() { if (!$this->isAvailable()) { - throw new \OCP\Files\StorageNotAvailableException(); + throw new StorageNotAvailableException(); } } @@ -85,9 +96,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::mkdir($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -96,9 +106,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::rmdir($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -107,9 +116,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::opendir($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -118,9 +126,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::is_dir($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -129,9 +136,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::is_file($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -140,9 +146,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::stat($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -151,9 +156,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::filetype($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -162,9 +166,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::filesize($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -173,9 +176,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::isCreatable($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -184,9 +186,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::isReadable($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -195,9 +196,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::isUpdatable($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -206,9 +206,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::isDeletable($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -217,9 +216,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::isSharable($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -228,9 +226,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::getPermissions($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -242,9 +239,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::file_exists($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -253,9 +249,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::filemtime($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -264,9 +259,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::file_get_contents($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -275,9 +269,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::file_put_contents($path, $data); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -286,9 +279,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::unlink($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -297,9 +289,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::rename($path1, $path2); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -308,9 +299,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::copy($path1, $path2); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -319,9 +309,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::fopen($path, $mode); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -330,9 +319,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::getMimeType($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -341,9 +329,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::hash($type, $path, $raw); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -352,9 +339,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::free_space($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -363,9 +349,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::search($query); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -374,9 +359,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::touch($path, $mtime); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -385,9 +369,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::getLocalFile($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -396,9 +379,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::hasUpdated($path, $time); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -406,9 +388,8 @@ class Availability extends Wrapper { public function getOwner($path) { try { return parent::getOwner($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -417,9 +398,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::getETag($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -428,9 +408,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::getDirectDownload($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -439,9 +418,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -450,9 +428,8 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } @@ -461,9 +438,24 @@ class Availability extends Wrapper { $this->checkAvailability(); try { return parent::getMetaData($path); - } catch (\OCP\Files\StorageNotAvailableException $e) { - $this->setAvailability(false); - throw $e; + } catch (StorageNotAvailableException $e) { + $this->setUnavailable($e); } } + + /** + * @throws StorageNotAvailableException + */ + protected function setUnavailable(StorageNotAvailableException $e) { + $delay = self::RECHECK_TTL_SEC; + if($e instanceof StorageAuthException) { + $delay = max( + // 30min + $this->config->getSystemValueInt('external_storage.auth_availability_delay', 1800), + self::RECHECK_TTL_SEC + ); + } + $this->getStorageCache()->setAvailability(false, $delay); + throw $e; + } } diff --git a/tests/lib/Files/Storage/Wrapper/AvailabilityTest.php b/tests/lib/Files/Storage/Wrapper/AvailabilityTest.php index 5ea7f8403c..1672d17e36 100644 --- a/tests/lib/Files/Storage/Wrapper/AvailabilityTest.php +++ b/tests/lib/Files/Storage/Wrapper/AvailabilityTest.php @@ -21,29 +21,46 @@ namespace Test\Files\Storage\Wrapper; +use OC\Files\Cache\Storage as StorageCache; +use OC\Files\Storage\Temporary; +use OC\Files\Storage\Wrapper\Availability; +use OCP\Files\StorageNotAvailableException; + class AvailabilityTest extends \Test\TestCase { - protected function getWrapperInstance() { - $storage = $this->getMockBuilder('\OC\Files\Storage\Temporary') - ->disableOriginalConstructor() - ->getMock(); - $wrapper = new \OC\Files\Storage\Wrapper\Availability(['storage' => $storage]); - return [$storage, $wrapper]; + + /** @var \PHPUnit\Framework\MockObject\MockObject|StorageCache */ + protected $storageCache; + /** @var \PHPUnit\Framework\MockObject\MockObject|Temporary */ + protected $storage; + /** @var Availability */ + protected $wrapper; + + public function setUp() { + parent::setUp(); + + $this->storageCache = $this->createMock(StorageCache::class); + + $this->storage = $this->createMock(Temporary::class); + $this->storage->expects($this->any()) + ->method('getStorageCache') + ->willReturn($this->storageCache); + + $this->wrapper = new Availability(['storage' => $this->storage]); } /** * Storage is available */ public function testAvailable() { - list($storage, $wrapper) = $this->getWrapperInstance(); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('getAvailability') ->willReturn(['available' => true, 'last_checked' => 0]); - $storage->expects($this->never()) + $this->storage->expects($this->never()) ->method('test'); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('mkdir'); - $wrapper->mkdir('foobar'); + $this->wrapper->mkdir('foobar'); } /** @@ -52,39 +69,37 @@ class AvailabilityTest extends \Test\TestCase { * @expectedException \OCP\Files\StorageNotAvailableException */ public function testUnavailable() { - list($storage, $wrapper) = $this->getWrapperInstance(); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('getAvailability') ->willReturn(['available' => false, 'last_checked' => time()]); - $storage->expects($this->never()) + $this->storage->expects($this->never()) ->method('test'); - $storage->expects($this->never()) + $this->storage->expects($this->never()) ->method('mkdir'); - $wrapper->mkdir('foobar'); + $this->wrapper->mkdir('foobar'); } /** * Storage marked unavailable, TTL expired */ public function testUnavailableRecheck() { - list($storage, $wrapper) = $this->getWrapperInstance(); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('getAvailability') ->willReturn(['available' => false, 'last_checked' => 0]); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('test') ->willReturn(true); - $storage->expects($this->exactly(2)) + $this->storage->expects($this->exactly(2)) ->method('setAvailability') ->withConsecutive( [$this->equalTo(false)], // prevents concurrent rechecks [$this->equalTo(true)] // sets correct availability ); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('mkdir'); - $wrapper->mkdir('foobar'); + $this->wrapper->mkdir('foobar'); } /** @@ -93,20 +108,19 @@ class AvailabilityTest extends \Test\TestCase { * @expectedException \OCP\Files\StorageNotAvailableException */ public function testAvailableThrowStorageNotAvailable() { - list($storage, $wrapper) = $this->getWrapperInstance(); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('getAvailability') ->willReturn(['available' => true, 'last_checked' => 0]); - $storage->expects($this->never()) + $this->storage->expects($this->never()) ->method('test'); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('mkdir') - ->will($this->throwException(new \OCP\Files\StorageNotAvailableException())); - $storage->expects($this->once()) + ->will($this->throwException(new StorageNotAvailableException())); + $this->storageCache->expects($this->once()) ->method('setAvailability') ->with($this->equalTo(false)); - $wrapper->mkdir('foobar'); + $this->wrapper->mkdir('foobar'); } /** @@ -114,19 +128,18 @@ class AvailabilityTest extends \Test\TestCase { * Method failure does not indicate storage unavailability */ public function testAvailableFailure() { - list($storage, $wrapper) = $this->getWrapperInstance(); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('getAvailability') ->willReturn(['available' => true, 'last_checked' => 0]); - $storage->expects($this->never()) + $this->storage->expects($this->never()) ->method('test'); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('mkdir') ->willReturn(false); - $storage->expects($this->never()) + $this->storage->expects($this->never()) ->method('setAvailability'); - $wrapper->mkdir('foobar'); + $this->wrapper->mkdir('foobar'); } /** @@ -136,18 +149,17 @@ class AvailabilityTest extends \Test\TestCase { * @expectedException \Exception */ public function testAvailableThrow() { - list($storage, $wrapper) = $this->getWrapperInstance(); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('getAvailability') ->willReturn(['available' => true, 'last_checked' => 0]); - $storage->expects($this->never()) + $this->storage->expects($this->never()) ->method('test'); - $storage->expects($this->once()) + $this->storage->expects($this->once()) ->method('mkdir') ->will($this->throwException(new \Exception())); - $storage->expects($this->never()) + $this->storage->expects($this->never()) ->method('setAvailability'); - $wrapper->mkdir('foobar'); + $this->wrapper->mkdir('foobar'); } }