From 887be709f5f0dbbc6ad7b1cc1a9793d04421c5b9 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 12 May 2015 18:49:25 +0200 Subject: [PATCH] a new approach to display the error message --- apps/encryption/appinfo/application.php | 3 +- apps/encryption/lib/crypto/encryption.php | 40 +++++++++ apps/encryption/lib/util.php | 31 ++++++- .../encryption/settings/settings-personal.php | 3 +- apps/encryption/tests/lib/UtilTest.php | 18 ++-- apps/files_sharing/lib/sharedstorage.php | 8 +- .../exceptions/decryptionfailedexception.php | 11 +++ lib/private/files.php | 90 ++++++++++--------- .../files/storage/wrapper/encryption.php | 24 +++++ .../exceptions/genericencryptionexception.php | 11 ++- lib/public/encryption/iencryptionmodule.php | 12 +++ settings/changepassword/controller.php | 3 +- .../lib/files/storage/wrapper/encryption.php | 9 +- tests/lib/files/stream/encryption.php | 3 +- 14 files changed, 209 insertions(+), 57 deletions(-) diff --git a/apps/encryption/appinfo/application.php b/apps/encryption/appinfo/application.php index 0d6f57f46e..79b2ad3aba 100644 --- a/apps/encryption/appinfo/application.php +++ b/apps/encryption/appinfo/application.php @@ -209,7 +209,8 @@ class Application extends \OCP\AppFramework\App { $c->query('Crypt'), $server->getLogger(), $server->getUserSession(), - $server->getConfig()); + $server->getConfig(), + $server->getUserManager()); }); } diff --git a/apps/encryption/lib/crypto/encryption.php b/apps/encryption/lib/crypto/encryption.php index a4abcd7dc5..0fd85fa4e9 100644 --- a/apps/encryption/lib/crypto/encryption.php +++ b/apps/encryption/lib/crypto/encryption.php @@ -270,6 +270,15 @@ class Encryption implements IEncryptionModule { * @return mixed decrypted data */ public function decrypt($data) { + if (empty($this->fileKey)) { + $msg = $this->l->t('Can not decrypt this file, probably this is a shared file. Please ask the file owner to reshare the file with you.'); + $this->logger->error('Can not decrypt this file, + probably this is a shared file. + Please ask the file owner to reshare the file with you.'); + + throw new DecryptionFailedException($msg); + } + $result = ''; if (!empty($data)) { $result = $this->crypt->symmetricDecryptFileContent($data, $this->fileKey, $this->cipher); @@ -345,6 +354,36 @@ class Encryption implements IEncryptionModule { return 6126; } + /** + * check if the encryption module is able to read the file, + * e.g. if all encryption keys exists + * + * @param string $path + * @param string $uid user for whom we want to check if he can read the file + * @return bool + * @throws DecryptionFailedException + */ + public function isReadable($path, $uid) { + $fileKey = $this->keyManager->getFileKey($path, $uid); + if (empty($fileKey)) { + $owner = $this->util->getOwner($path); + if ($owner !== $uid) { + // if it is a shared file we throw a exception with a useful + // error message because in this case it means that the file was + // shared with the user at a point where the user didn't had a + // valid private/public key + $msg = 'Encryption module "' . $this->getDisplayName() . + '" is not able to read ' . $path; + $hint = $this->l->t('Can not read this file, probably this is a shared file. Please ask the file owner to reshare the file with you.'); + $this->logger->warning($msg); + throw new DecryptionFailedException($msg, 0, null, $hint); + } + return false; + } + + return true; + } + /** * @param string $path * @return string @@ -360,4 +399,5 @@ class Encryption implements IEncryptionModule { return $realPath; } + } diff --git a/apps/encryption/lib/util.php b/apps/encryption/lib/util.php index 51d5241122..afed96aaa3 100644 --- a/apps/encryption/lib/util.php +++ b/apps/encryption/lib/util.php @@ -29,6 +29,7 @@ use OCA\Encryption\Crypto\Crypt; use OCP\IConfig; use OCP\ILogger; use OCP\IUser; +use OCP\IUserManager; use OCP\IUserSession; use OCP\PreConditionNotMetException; @@ -53,6 +54,10 @@ class Util { * @var IConfig */ private $config; + /** + * @var IUserManager + */ + private $userManager; /** * Util constructor. @@ -62,18 +67,21 @@ class Util { * @param ILogger $logger * @param IUserSession $userSession * @param IConfig $config + * @param IUserManager $userManager */ public function __construct(View $files, Crypt $crypt, ILogger $logger, IUserSession $userSession, - IConfig $config + IConfig $config, + IUserManager $userManager ) { $this->files = $files; $this->crypt = $crypt; $this->logger = $logger; $this->user = $userSession && $userSession->isLoggedIn() ? $userSession->getUser() : false; $this->config = $config; + $this->userManager = $userManager; } /** @@ -117,5 +125,26 @@ class Util { return $this->files->file_exists($uid . '/files'); } + /** + * get owner from give path, path relative to data/ expected + * + * @param string $path relative to data/ + * @return string + * @throws \BadMethodCallException + */ + public function getOwner($path) { + $owner = ''; + $parts = explode('/', $path, 3); + if (count($parts) > 1) { + $owner = $parts[1]; + if ($this->userManager->userExists($owner) === false) { + throw new \BadMethodCallException('Unknown user: ' . + 'method expects path to a user folder relative to the data folder'); + } + + } + + return $owner; + } } diff --git a/apps/encryption/settings/settings-personal.php b/apps/encryption/settings/settings-personal.php index 003a27da71..3815626ee6 100644 --- a/apps/encryption/settings/settings-personal.php +++ b/apps/encryption/settings/settings-personal.php @@ -35,7 +35,8 @@ $util = new \OCA\Encryption\Util( $crypt, \OC::$server->getLogger(), $userSession, - \OC::$server->getConfig()); + \OC::$server->getConfig(), + \OC::$server->getUserManager()); $keyManager = new \OCA\Encryption\KeyManager( \OC::$server->getEncryptionKeyStorage(), diff --git a/apps/encryption/tests/lib/UtilTest.php b/apps/encryption/tests/lib/UtilTest.php index eab912b82d..18cf038679 100644 --- a/apps/encryption/tests/lib/UtilTest.php +++ b/apps/encryption/tests/lib/UtilTest.php @@ -28,11 +28,17 @@ use Test\TestCase; class UtilTest extends TestCase { private static $tempStorage = []; + + /** @var \PHPUnit_Framework_MockObject_MockObject */ private $configMock; + + /** @var \PHPUnit_Framework_MockObject_MockObject */ private $filesMock; - /** - * @var Util - */ + + /** @var \PHPUnit_Framework_MockObject_MockObject */ + private $userManagerMock; + + /** @var Util */ private $instance; public function testSetRecoveryForUser() { @@ -40,9 +46,6 @@ class UtilTest extends TestCase { $this->assertArrayHasKey('recoveryEnabled', self::$tempStorage); } - /** - * - */ public function testIsRecoveryEnabledForUser() { $this->assertTrue($this->instance->isRecoveryEnabledForUser('admin')); @@ -62,6 +65,7 @@ class UtilTest extends TestCase { protected function setUp() { parent::setUp(); $this->filesMock = $this->getMock('OC\Files\View'); + $this->userManagerMock = $this->getMock('\OCP\IUserManager'); $cryptMock = $this->getMockBuilder('OCA\Encryption\Crypto\Crypt') ->disableOriginalConstructor() @@ -98,7 +102,7 @@ class UtilTest extends TestCase { ->method('setUserValue') ->will($this->returnCallback([$this, 'setValueTester'])); - $this->instance = new Util($this->filesMock, $cryptMock, $loggerMock, $userSessionMock, $configMock); + $this->instance = new Util($this->filesMock, $cryptMock, $loggerMock, $userSessionMock, $configMock, $this->userManagerMock); } /** diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index 33b7f887e1..10a37c7dae 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -217,7 +217,13 @@ class Shared extends \OC\Files\Storage\Common implements ISharedStorage { } public function isReadable($path) { - return $this->file_exists($path); + $isReadable = false; + if ($source = $this->getSourcePath($path)) { + list($storage, $internalPath) = \OC\Files\Filesystem::resolvePath($source); + $isReadable = $storage->isReadable($internalPath); + } + + return $isReadable && $this->file_exists($path); } public function isUpdatable($path) { diff --git a/lib/private/encryption/exceptions/decryptionfailedexception.php b/lib/private/encryption/exceptions/decryptionfailedexception.php index 406ae12968..7e9fa21eae 100644 --- a/lib/private/encryption/exceptions/decryptionfailedexception.php +++ b/lib/private/encryption/exceptions/decryptionfailedexception.php @@ -27,4 +27,15 @@ use OCP\Encryption\Exceptions\GenericEncryptionException; class DecryptionFailedException extends GenericEncryptionException { + /** + * @param string $message + * @param int $code + * @param \Exception $previous + * @param string $hint + */ + public function __construct($message = '', $code = 0, \Exception $previous = null, $hint = '') { + parent::__construct($message, $code, $previous, $hint); + +} + } diff --git a/lib/private/files.php b/lib/private/files.php index 97f9d8163b..6a739fc844 100644 --- a/lib/private/files.php +++ b/lib/private/files.php @@ -129,52 +129,60 @@ class OC_Files { $zip = new ZipStreamer(false); } OC_Util::obEnd(); - if ($zip or \OC\Files\Filesystem::isReadable($filename)) { - self::sendHeaders($filename, $name, $zip); - } elseif (!\OC\Files\Filesystem::file_exists($filename)) { - header("HTTP/1.0 404 Not Found"); - $tmpl = new OC_Template('', '404', 'guest'); - $tmpl->printPage(); - } else { - header("HTTP/1.0 403 Forbidden"); - die('403 Forbidden'); - } - if($only_header) { - return ; - } - if ($zip) { - $executionTime = intval(ini_get('max_execution_time')); - set_time_limit(0); - if ($get_type === self::ZIP_FILES) { - foreach ($files as $file) { - $file = $dir . '/' . $file; - if (\OC\Files\Filesystem::is_file($file)) { - $fh = \OC\Files\Filesystem::fopen($file, 'r'); - $zip->addFileFromStream($fh, basename($file)); - fclose($fh); - } elseif (\OC\Files\Filesystem::is_dir($file)) { - self::zipAddDir($file, $zip); - } - } - } elseif ($get_type === self::ZIP_DIR) { - $file = $dir . '/' . $files; - self::zipAddDir($file, $zip); + + try { + + if ($zip or \OC\Files\Filesystem::isReadable($filename)) { + self::sendHeaders($filename, $name, $zip); + } elseif (!\OC\Files\Filesystem::file_exists($filename)) { + header("HTTP/1.0 404 Not Found"); + $tmpl = new OC_Template('', '404', 'guest'); + $tmpl->printPage(); + } else { + header("HTTP/1.0 403 Forbidden"); + die('403 Forbidden'); } - $zip->finalize(); - set_time_limit($executionTime); - } else { - if ($xsendfile) { - $view = \OC\Files\Filesystem::getView(); - /** @var $storage \OC\Files\Storage\Storage */ - list($storage) = $view->resolvePath($filename); - if ($storage->isLocal()) { - self::addSendfileHeader($filename); + if ($only_header) { + return; + } + if ($zip) { + $executionTime = intval(ini_get('max_execution_time')); + set_time_limit(0); + if ($get_type === self::ZIP_FILES) { + foreach ($files as $file) { + $file = $dir . '/' . $file; + if (\OC\Files\Filesystem::is_file($file)) { + $fh = \OC\Files\Filesystem::fopen($file, 'r'); + $zip->addFileFromStream($fh, basename($file)); + fclose($fh); + } elseif (\OC\Files\Filesystem::is_dir($file)) { + self::zipAddDir($file, $zip); + } + } + } elseif ($get_type === self::ZIP_DIR) { + $file = $dir . '/' . $files; + self::zipAddDir($file, $zip); + } + $zip->finalize(); + set_time_limit($executionTime); + } else { + if ($xsendfile) { + $view = \OC\Files\Filesystem::getView(); + /** @var $storage \OC\Files\Storage\Storage */ + list($storage) = $view->resolvePath($filename); + if ($storage->isLocal()) { + self::addSendfileHeader($filename); + } else { + \OC\Files\Filesystem::readfile($filename); + } } else { \OC\Files\Filesystem::readfile($filename); } - } else { - \OC\Files\Filesystem::readfile($filename); } + } catch (\Exception $ex) { + $l = \OC::$server->getL10N('core'); + $hint = method_exists($ex, 'getHint') ? $ex->getHint() : ''; + \OC_Template::printErrorPage($l->t('Can\'t read file'), $hint); } } diff --git a/lib/private/files/storage/wrapper/encryption.php b/lib/private/files/storage/wrapper/encryption.php index f7759d9149..1683ff1350 100644 --- a/lib/private/files/storage/wrapper/encryption.php +++ b/lib/private/files/storage/wrapper/encryption.php @@ -252,6 +252,30 @@ class Encryption extends Wrapper { return $result; } + /** + * check if a file can be read + * + * @param string $path + * @return bool + */ + public function isReadable($path) { + + $isReadable = true; + + $metaData = $this->getMetaData($path); + if ( + !$this->is_dir($path) && + isset($metaData['encrypted']) && + $metaData['encrypted'] === true + ) { + $fullPath = $this->getFullPath($path); + $module = $this->getEncryptionModule($path); + $isReadable = $module->isReadable($fullPath, $this->uid); + } + + return $this->storage->isReadable($path) && $isReadable; + } + /** * see http://php.net/manual/en/function.copy.php * diff --git a/lib/public/encryption/exceptions/genericencryptionexception.php b/lib/public/encryption/exceptions/genericencryptionexception.php index be96450d43..e97f00c88b 100644 --- a/lib/public/encryption/exceptions/genericencryptionexception.php +++ b/lib/public/encryption/exceptions/genericencryptionexception.php @@ -30,17 +30,26 @@ namespace OCP\Encryption\Exceptions; */ class GenericEncryptionException extends \Exception { + /** @var string */ + protected $hint; + /** * @param string $message * @param int $code * @param \Exception $previous * @since 8.1.0 */ - public function __construct($message = '', $code = 0, \Exception $previous = null) { + public function __construct($message = '', $code = 0, \Exception $previous = null, $hint = '') { if (empty($message)) { $message = 'Unspecified encryption exception'; } parent::__construct($message, $code, $previous); + + $this->hint = $hint; + } + + public function getHint() { + return $this->hint; } } diff --git a/lib/public/encryption/iencryptionmodule.php b/lib/public/encryption/iencryptionmodule.php index 0dda042d75..975e57744e 100644 --- a/lib/public/encryption/iencryptionmodule.php +++ b/lib/public/encryption/iencryptionmodule.php @@ -119,4 +119,16 @@ interface IEncryptionModule { * @since 8.1.0 */ public function getUnencryptedBlockSize(); + + /** + * check if the encryption module is able to read the file, + * e.g. if all encryption keys exists + * + * @param string $path + * @param string $uid user for whom we want to check if he can read the file + * @return boolean + * @since 8.1.0 + */ + public function isReadable($path, $uid); + } diff --git a/settings/changepassword/controller.php b/settings/changepassword/controller.php index 94323fc2fc..69b7ca710a 100644 --- a/settings/changepassword/controller.php +++ b/settings/changepassword/controller.php @@ -89,7 +89,8 @@ class Controller { $crypt, \OC::$server->getLogger(), \OC::$server->getUserSession(), - \OC::$server->getConfig()); + \OC::$server->getConfig(), + \OC::$server->getUserManager()); $keyManager = new \OCA\Encryption\KeyManager( $keyStorage, $crypt, diff --git a/tests/lib/files/storage/wrapper/encryption.php b/tests/lib/files/storage/wrapper/encryption.php index 97810c9c5d..361592f6ca 100644 --- a/tests/lib/files/storage/wrapper/encryption.php +++ b/tests/lib/files/storage/wrapper/encryption.php @@ -117,7 +117,7 @@ class Encryption extends \Test\Files\Storage\Storage { $this->encryptionManager, $this->util, $logger, $file, null, $this->keyStore, $this->update ] ) - ->setMethods(['getMetaData', 'getCache']) + ->setMethods(['getMetaData', 'getCache', 'getEncryptionModule']) ->getMock(); $this->instance->expects($this->any()) @@ -127,6 +127,10 @@ class Encryption extends \Test\Files\Storage\Storage { $this->instance->expects($this->any()) ->method('getCache') ->willReturn($this->cache); + + $this->instance->expects($this->any()) + ->method('getEncryptionModule') + ->willReturn($mockModule); } /** @@ -135,7 +139,7 @@ class Encryption extends \Test\Files\Storage\Storage { protected function buildMockModule() { $this->encryptionModule = $this->getMockBuilder('\OCP\Encryption\IEncryptionModule') ->disableOriginalConstructor() - ->setMethods(['getId', 'getDisplayName', 'begin', 'end', 'encrypt', 'decrypt', 'update', 'shouldEncrypt', 'getUnencryptedBlockSize']) + ->setMethods(['getId', 'getDisplayName', 'begin', 'end', 'encrypt', 'decrypt', 'update', 'shouldEncrypt', 'getUnencryptedBlockSize', 'isReadable']) ->getMock(); $this->encryptionModule->expects($this->any())->method('getId')->willReturn('UNIT_TEST_MODULE'); @@ -147,6 +151,7 @@ class Encryption extends \Test\Files\Storage\Storage { $this->encryptionModule->expects($this->any())->method('update')->willReturn(true); $this->encryptionModule->expects($this->any())->method('shouldEncrypt')->willReturn(true); $this->encryptionModule->expects($this->any())->method('getUnencryptedBlockSize')->willReturn(8192); + $this->encryptionModule->expects($this->any())->method('isReadable')->willReturn(true); return $this->encryptionModule; } diff --git a/tests/lib/files/stream/encryption.php b/tests/lib/files/stream/encryption.php index 892491cbc3..113cd14ada 100644 --- a/tests/lib/files/stream/encryption.php +++ b/tests/lib/files/stream/encryption.php @@ -253,13 +253,14 @@ class Encryption extends \Test\TestCase { protected function buildMockModule() { $encryptionModule = $this->getMockBuilder('\OCP\Encryption\IEncryptionModule') ->disableOriginalConstructor() - ->setMethods(['getId', 'getDisplayName', 'begin', 'end', 'encrypt', 'decrypt', 'update', 'shouldEncrypt', 'getUnencryptedBlockSize']) + ->setMethods(['getId', 'getDisplayName', 'begin', 'end', 'encrypt', 'decrypt', 'update', 'shouldEncrypt', 'getUnencryptedBlockSize', 'isReadable']) ->getMock(); $encryptionModule->expects($this->any())->method('getId')->willReturn('UNIT_TEST_MODULE'); $encryptionModule->expects($this->any())->method('getDisplayName')->willReturn('Unit test module'); $encryptionModule->expects($this->any())->method('begin')->willReturn([]); $encryptionModule->expects($this->any())->method('end')->willReturn(''); + $encryptionModule->expects($this->any())->method('isReadable')->willReturn(true); $encryptionModule->expects($this->any())->method('encrypt')->willReturnCallback(function($data) { // simulate different block size by adding some padding to the data if (isset($data[6125])) {