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.

Sign up for GitHub

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

Fire prehooks when uploading directly to storage #16292

Merged
merged 1 commit into from
May 15, 2015
Merged

Fire prehooks when uploading directly to storage #16292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 28 additions & 6 deletions lib/private/connector/sabre/file.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,37 @@ public function put($data) {
}

try {
$view = \OC\Files\Filesystem::getView();
$run = true;
if ($view) {
$hookPath = $view->getRelativePath($this->fileView->getAbsolutePath($this->path));

if (!$exists) {
\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_create, array(
\OC\Files\Filesystem::signal_param_path => $hookPath,
\OC\Files\Filesystem::signal_param_run => &$run,
));
} else {
\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_update, array(
\OC\Files\Filesystem::signal_param_path => $hookPath,
\OC\Files\Filesystem::signal_param_run => &$run,
));
}
\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_write, array(
\OC\Files\Filesystem::signal_param_path => $hookPath,
\OC\Files\Filesystem::signal_param_run => &$run,
));
}

if ($needsPartFile) {
// rename to correct path
try {
$renameOkay = $storage->moveFromStorage($partStorage, $internalPartPath, $internalPath);
$fileExists = $storage->file_exists($internalPath);
if ($renameOkay === false || $fileExists === false) {
\OC_Log::write('webdav', '\OC\Files\Filesystem::rename() failed', \OC_Log::ERROR);
if ($run) {
$renameOkay = $storage->moveFromStorage($partStorage, $internalPartPath, $internalPath);
$fileExists = $storage->file_exists($internalPath);
}
if (!$run || $renameOkay === false || $fileExists === false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks weird... the !$run but the comparison for the others..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only replicates the old behavior.

In the past we only called $view->rename() instead of $storage->moveFromStorage().
In the case of the view, it would internally trigger the pre and post hooks.
If the pre-hook fails, then it would return false. See https://github.com/owncloud/core/blob/master/lib/private/files/view.php#L621 and the else block.
This would cause $renameOkay to be false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually $run means "all the hooks agreed that the action can be run" or "$run is true when the operation is NOT cancelled by a hook"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it was just a tiny style thing..

You write !$run but the but the other comparisons are written in full...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes... it was out of the habit of not trusting our code to return a solid false in some cases.
I'll leave it as is for now, unless I need to retouch the code for other issues.

\OC_Log::write('webdav', 'renaming part file to final file failed', \OC_Log::ERROR);
$partStorage->unlink($internalPartPath);
throw new Exception('Could not rename part file to final file');
}
Expand All @@ -180,9 +204,7 @@ public function put($data) {
// since we skipped the view we need to scan and emit the hooks ourselves
$partStorage->getScanner()->scanFile($internalPath);

$view = \OC\Files\Filesystem::getView();
if ($view) {
$hookPath = $view->getRelativePath($this->fileView->getAbsolutePath($this->path));
$this->fileView->getUpdater()->propagate($hookPath);
if (!$exists) {
\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_create, array(
Expand Down
232 changes: 194 additions & 38 deletions tests/lib/connector/sabre/file.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,35 @@

namespace Test\Connector\Sabre;

use Test\HookHelper;
use OC\Files\Filesystem;

class File extends \Test\TestCase {

/**
* @var string
*/
private $user;

public function setUp() {
parent::setUp();

\OC_Hook::clear();

$this->user = $this->getUniqueID('user_');
$userManager = \OC::$server->getUserManager();
$userManager->createUser($this->user, 'pass');

$this->loginAsUser($this->user);
}

public function tearDown() {
$userManager = \OC::$server->getUserManager();
$userManager->get($this->user)->delete();

parent::tearDown();
}

private function getStream($string) {
$stream = fopen('php://temp', 'r+');
fwrite($stream, $string);
Expand All @@ -23,7 +50,7 @@ private function getStream($string) {
public function testSimplePutFails() {
// setup
$storage = $this->getMock('\OC\Files\Storage\Local', ['fopen'], [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]);
$view = $this->getMock('\OC\Files\View', array('file_put_contents', 'getRelativePath', 'resolvePath'), array());
$view = $this->getMock('\OC\Files\View', array('getRelativePath', 'resolvePath'), array());
$view->expects($this->any())
->method('resolvePath')
->will($this->returnValue(array($storage, '')));
Expand All @@ -45,45 +72,166 @@ public function testSimplePutFails() {
$file->put('test data');
}

public function testPutSingleFileShare() {
// setup
$stream = fopen('php://temp', 'w+');
$storage = $this->getMock('\OC\Files\Storage\Local', ['fopen'], [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]);
$view = $this->getMock('\OC\Files\View', array('file_put_contents', 'getRelativePath', 'resolvePath'), array());
$view->expects($this->any())
->method('resolvePath')
->will($this->returnValue(array($storage, '')));
$view->expects($this->any())
->method('getRelativePath')
->will($this->returnValue(''));
$view->expects($this->any())
->method('file_put_contents')
->with('')
->will($this->returnValue(true));
$storage->expects($this->once())
->method('fopen')
->will($this->returnValue($stream));

$info = new \OC\Files\FileInfo('/foo.txt', null, null, array(
'permissions' => \OCP\Constants::PERMISSION_ALL
), null);
private function doPut($path, $viewRoot = null) {
$view = \OC\Files\Filesystem::getView();
if (!is_null($viewRoot)) {
$view = new \OC\Files\View($viewRoot);
} else {
$viewRoot = '/' . $this->user . '/files';
}

$info = new \OC\Files\FileInfo(
$viewRoot . '/' . ltrim($path, '/'),
null,
null,
['permissions' => \OCP\Constants::PERMISSION_ALL],
null
);

$file = new \OC\Connector\Sabre\File($view, $info);

$this->assertNotEmpty($file->put($this->getStream('test data')));
}

/**
* Test putting a single file
*/
public function testPutSingleFile() {
$this->doPut('/foo.txt');
}

/**
* Test that putting a file triggers create hooks
*/
public function testPutSingleFileTriggersHooks() {
HookHelper::setUpHooks();

$this->doPut('/foo.txt');

$this->assertCount(4, HookHelper::$hookCalls);
$this->assertHookCall(
HookHelper::$hookCalls[0],
Filesystem::signal_create,
'/foo.txt'
);
$this->assertHookCall(
HookHelper::$hookCalls[1],
Filesystem::signal_write,
'/foo.txt'
);
$this->assertHookCall(
HookHelper::$hookCalls[2],
Filesystem::signal_post_create,
'/foo.txt'
);
$this->assertHookCall(
HookHelper::$hookCalls[3],
Filesystem::signal_post_write,
'/foo.txt'
);
}

/**
* Test that putting a file triggers update hooks
*/
public function testPutOverwriteFileTriggersHooks() {
$view = \OC\Files\Filesystem::getView();
$view->file_put_contents('/foo.txt', 'some content that will be replaced');

HookHelper::setUpHooks();

$this->doPut('/foo.txt');

$this->assertCount(4, HookHelper::$hookCalls);
$this->assertHookCall(
HookHelper::$hookCalls[0],
Filesystem::signal_update,
'/foo.txt'
);
$this->assertHookCall(
HookHelper::$hookCalls[1],
Filesystem::signal_write,
'/foo.txt'
);
$this->assertHookCall(
HookHelper::$hookCalls[2],
Filesystem::signal_post_update,
'/foo.txt'
);
$this->assertHookCall(
HookHelper::$hookCalls[3],
Filesystem::signal_post_write,
'/foo.txt'
);
}

/**
* Test that putting a file triggers hooks with the correct path
* if the passed view was chrooted (can happen with public webdav
* where the root is the share root)
*/
public function testPutSingleFileTriggersHooksDifferentRoot() {
$view = \OC\Files\Filesystem::getView();
$view->mkdir('noderoot');

HookHelper::setUpHooks();

// happens with public webdav where the view root is the share root
$this->doPut('/foo.txt', '/' . $this->user . '/files/noderoot');

$this->assertCount(4, HookHelper::$hookCalls);
$this->assertHookCall(
HookHelper::$hookCalls[0],
Filesystem::signal_create,
'/noderoot/foo.txt'
);
$this->assertHookCall(
HookHelper::$hookCalls[1],
Filesystem::signal_write,
'/noderoot/foo.txt'
);
$this->assertHookCall(
HookHelper::$hookCalls[2],
Filesystem::signal_post_create,
'/noderoot/foo.txt'
);
$this->assertHookCall(
HookHelper::$hookCalls[3],
Filesystem::signal_post_write,
'/noderoot/foo.txt'
);
}

public static function cancellingHook($params) {
self::$hookCalls[] = array(
'signal' => Filesystem::signal_post_create,
'params' => $params
);
}

/**
* Test put file with cancelled hook
*
* @expectedException \Sabre\DAV\Exception
*/
public function testPutSingleFileCancelPreHook() {
\OCP\Util::connectHook(
Filesystem::CLASSNAME,
Filesystem::signal_create,
'\Test\HookHelper',
'cancellingCallback'
);

$this->doPut('/foo.txt');
}

/**
* @expectedException \Sabre\DAV\Exception
*/
public function testSimplePutFailsOnRename() {
// setup
$view = $this->getMock('\OC\Files\View',
array('file_put_contents', 'rename', 'getRelativePath', 'filesize'));
$view->expects($this->any())
->method('file_put_contents')
->withAnyParameters()
->will($this->returnValue(true));
array('rename', 'getRelativePath', 'filesize'));
$view->expects($this->any())
->method('rename')
->withAnyParameters()
Expand Down Expand Up @@ -113,11 +261,7 @@ public function testSimplePutFailsOnRename() {
*/
public function testSimplePutInvalidChars() {
// setup
$view = $this->getMock('\OC\Files\View', array('file_put_contents', 'getRelativePath'));
$view->expects($this->any())
->method('file_put_contents')
->will($this->returnValue(false));

$view = $this->getMock('\OC\Files\View', array('getRelativePath'));
$view->expects($this->any())
->method('getRelativePath')
->will($this->returnValue('/*'));
Expand Down Expand Up @@ -157,11 +301,7 @@ public function testSetNameInvalidChars() {
public function testUploadAbort() {
// setup
$view = $this->getMock('\OC\Files\View',
array('file_put_contents', 'rename', 'getRelativePath', 'filesize'));
$view->expects($this->any())
->method('file_put_contents')
->withAnyParameters()
->will($this->returnValue(true));
array('rename', 'getRelativePath', 'filesize'));
$view->expects($this->any())
->method('rename')
->withAnyParameters()
Expand Down Expand Up @@ -248,4 +388,20 @@ public function testDeleteThrowsWhenDeletionFailed() {
// action
$file->delete();
}

/**
* Asserts hook call
*
* @param array $callData hook call data to check
* @param string $signal signal name
* @param string $hookPath hook path
*/
protected function assertHookCall($callData, $signal, $hookPath) {
$this->assertEquals($signal, $callData['signal']);
$params = $callData['params'];
$this->assertEquals(
$hookPath,
$params[Filesystem::signal_param_path]
);
}
}
Loading