Skip to content

Transfer ownership work with masterkey #28107

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

Merged
sharidas merged 1 commit into from
Jul 28, 2017
Merged

Transfer ownership work with masterkey #28107

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
8 changes: 8 additions & 0 deletions apps/files/lib/Command/TransferOwnership.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,14 @@ function (FileInfo $fileInfo) use ($progress, $self) {
$progress->advance();
$this->allFiles[] = $fileInfo;
if ($fileInfo->isEncrypted()) {
if (\OC::$server->getAppConfig()->getValue('encryption', 'useMasterKey', 0) !== 0) {
Copy link
Contributor

@PVince81 PVince81 Jun 21, 2017

Choose a reason for hiding this comment

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

Note that in general the transfer ownership app isn't supposed to know about specific encryption implementations.
Some people might implement their own custom encryption schemes, so would need a better abstraction.

But as we likely don't have time for better abstractions let's merge this.

/**
* We are not going to add this to encryptedFiles array.
* Because its encrypted with masterKey and hence it doesn't
* require user's specific password.
*/
return true;
}
$this->encryptedFiles[] = $fileInfo;
}
return true;
Expand Down
3 changes: 3 additions & 0 deletions lib/private/Files/Filesystem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,9 @@ public static function getDirectoryContent($directory, $mimetype_filter = '') {
* @return string
*/
public static function getPath($id) {
if (self::$defaultInstance === null) {
throw new NotFoundException("defaultInstance is null");
Copy link
Contributor Author

@sharidas sharidas Jun 11, 2017

Choose a reason for hiding this comment

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

This is one solution I found to get rid of the fatal error thrown by symfony. Any suggestions how to improve this?

Copy link
Contributor

@PVince81 PVince81 Jun 19, 2017

Choose a reason for hiding this comment

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

thrown by symfony.

when exactly is this happening ? when running CI ?

Copy link
Contributor Author

@sharidas sharidas Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

when exactly is this happening ? when running CI ?

When integration test is executed.

}
return self::$defaultInstance->getPath($id);
}

Expand Down
25 changes: 18 additions & 7 deletions lib/private/Files/Storage/Wrapper/Encryption.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ public function unlink($path) {
return $this->storage->unlink($path);
}

$encryptionModule = $this->getEncryptionModule($path);
$encryptionModule = ($this->encryptionManager->isEnabled()) ? $this->getEncryptionModule($path) : "";
if ($encryptionModule) {
$this->keyStorage->deleteAllFileKeys($this->getFullPath($path));
}
Expand Down Expand Up @@ -358,11 +358,12 @@ public function copy($path1, $path2) {
*
* @param string $path
* @param string $mode
* @param string|null $sourceFileOfRename
* @return resource|bool
* @throws GenericEncryptionException
* @throws ModuleDoesNotExistsException
*/
public function fopen($path, $mode) {
public function fopen($path, $mode, $sourceFileOfRename = null) {

// check if the file is stored in the array cache, this means that we
// copy a file over to the versions folder, in this case we don't want to
Expand All @@ -378,7 +379,7 @@ public function fopen($path, $mode) {
$header = $this->getHeader($path);
$signed = (isset($header['signed']) && $header['signed'] === 'true') ? true : false;
$fullPath = $this->getFullPath($path);
$encryptionModuleId = $this->util->getEncryptionModuleId($header);
$encryptionModuleId = ($encryptionEnabled) ? $this->util->getEncryptionModuleId($header): "";

if ($this->util->isExcluded($fullPath) === false) {

Expand Down Expand Up @@ -457,7 +458,7 @@ public function fopen($path, $mode) {
}
$handle = \OC\Files\Stream\Encryption::wrap($source, $path, $fullPath, $header,
$this->uid, $encryptionModule, $this->storage, $this, $this->util, $this->fileHelper, $mode,
$size, $unencryptedSize, $headerSize, $signed);
$size, $unencryptedSize, $headerSize, $signed, $sourceFileOfRename);
return $handle;
}

Expand Down Expand Up @@ -569,7 +570,7 @@ protected function fixUnencryptedSize($path, $size, $unencryptedSize) {
fclose($stream);

// we have to decrypt the last chunk to get it actual size
$encryptionModule->begin($this->getFullPath($path), $this->uid, 'r', $header, []);
$encryptionModule->begin($this->getFullPath($path), $this->uid, 'r', $header, [], null);
$decryptedLastChunk = $encryptionModule->decrypt($lastChunkContentEncrypted, $lastChunkNr . 'end');
$decryptedLastChunk .= $encryptionModule->end($this->getFullPath($path), $lastChunkNr . 'end');

Expand Down Expand Up @@ -672,6 +673,12 @@ private function updateEncryptedVersion(Storage $sourceStorage, $sourceInternalP
// in case of a rename we need to manipulate the source cache because
// this information will be kept for the new target
if ($isRename) {
/*
* Rename is a process of creating a new file. Here we try to use the
* incremented version of source file, for the destination file.
*/
$encryptedVersion = $sourceStorage->getCache()->get($sourceInternalPath)['encryptedVersion'];
Copy link
Contributor Author

@sharidas sharidas Jul 28, 2017

Choose a reason for hiding this comment

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

Modified this change. Trivial change. There was failure in couple of tests : apps/files_trashbin/tests/StorageTest.php and apps/files_trashbin/tests/TrashbinTest.php

$cacheInformation['encryptedVersion'] = $encryptedVersion + 1;
$sourceStorage->getCache()->put($sourceInternalPath, $cacheInformation);
} else {
$this->getCache()->put($targetInternalPath, $cacheInformation);
Expand All @@ -690,7 +697,6 @@ private function updateEncryptedVersion(Storage $sourceStorage, $sourceInternalP
* @throws \Exception
*/
private function copyBetweenStorage(Storage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime, $isRename) {

// for versions we have nothing to do, because versions should always use the
// key from the original file. Just create a 1:1 copy and done
if ($this->isVersion($targetInternalPath) ||
Expand Down Expand Up @@ -740,7 +746,12 @@ private function copyBetweenStorage(Storage $sourceStorage, $sourceInternalPath,
} else {
try {
$source = $sourceStorage->fopen($sourceInternalPath, 'r');
$target = $this->fopen($targetInternalPath, 'w');
if ($isRename) {
$absSourcePath = Filesystem::normalizePath($sourceStorage->getOwner($sourceInternalPath). '/' . $sourceInternalPath);
$target = $this->fopen($targetInternalPath, 'w', $absSourcePath);
} else {
$target = $this->fopen($targetInternalPath, 'w');
}
list(, $result) = \OC_Helper::streamCopy($source, $target);
fclose($source);
fclose($target);
Expand Down
11 changes: 7 additions & 4 deletions lib/private/Files/Stream/Encryption.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ public function __construct() {
'unencryptedSize',
'encryptionStorage',
'headerSize',
'signed'
'signed',
'sourceFileOfRename'
];
}

Expand Down Expand Up @@ -155,6 +156,7 @@ public static function wrap($source, $internalPath, $fullPath, array $header,
$unencryptedSize,
$headerSize,
$signed,
$sourceFileOfRename = null,
$wrapper = 'OC\Files\Stream\Encryption') {

$context = stream_context_create([
Expand All @@ -172,7 +174,8 @@ public static function wrap($source, $internalPath, $fullPath, array $header,
'unencryptedSize' => $unencryptedSize,
'encryptionStorage' => $encStorage,
'headerSize' => $headerSize,
'signed' => $signed
'signed' => $signed,
'sourceFileOfRename' => $sourceFileOfRename
]
]);

Expand Down Expand Up @@ -228,7 +231,7 @@ protected function loadContext($name) {
}

public function stream_open($path, $mode, $options, &$opened_path) {
$this->loadContext('ocencryption');
$context = $this->loadContext('ocencryption');

$this->position = 0;
$this->cache = '';
Expand All @@ -254,7 +257,7 @@ public function stream_open($path, $mode, $options, &$opened_path) {
}

$accessList = $this->file->getAccessList($sharePath);
$this->newHeader = $this->encryptionModule->begin($this->fullPath, $this->uid, $mode, $this->header, $accessList);
$this->newHeader = $this->encryptionModule->begin($this->fullPath, $this->uid, $mode, $this->header, $accessList, $context['sourceFileOfRename']);

if (
$mode === 'w'
Expand Down
4 changes: 3 additions & 1 deletion lib/public/Encryption/IEncryptionModule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,15 @@ public function getDisplayName();
* @param string $mode php stream open mode
* @param array $header contains the header data read from the file
* @param array $accessList who has access to the file contains the key 'users' and 'public'
* @param string|null $sourceFileOfRename contains false or the rename source file. This is required
* for version increment.
*
* $return array $header contain data as key-value pairs which should be
* written to the header, in case of a write operation
* or if no additional data is needed return a empty array
* @since 8.1.0
*/
public function begin($path, $user, $mode, array $header, array $accessList);
public function begin($path, $user, $mode, array $header, array $accessList, $sourceFileOfRename);

/**
* last chunk received. This is the place where you can perform some final
Expand Down
34 changes: 13 additions & 21 deletions tests/integration/features/transfer-ownership.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
Feature: transfer-ownership

# TODO: change to @no_default_encryption once all this works with master key
@no_encryption
Scenario: transfering ownership of a file
Given user "user0" exists
And user "user1" exists
Expand All @@ -12,7 +11,19 @@ Feature: transfer-ownership
And using received transfer folder of "user1" as dav path
Then Downloaded content when downloading file "/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership of a file after updating the file
Copy link
Contributor Author

@sharidas sharidas Jul 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The intention behind this test is:

  1. update the file 3 times ( which would increase the encrypt version )
  2. When the file is transferred it should be readable.

Copy link
Contributor

@PVince81 PVince81 Jul 5, 2017

Choose a reason for hiding this comment

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

I don't see the file updated three times. All you do is upload three separate pieces of the file. This doesn't increase the encryption version of the final file. You need to upload 3 times.
I suggest to not use chunked upload here for readability.

Copy link
Contributor Author

@sharidas sharidas Jul 5, 2017

Choose a reason for hiding this comment

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

Ok. Will upload the same file 3 times. And then check the value in the file after download.

Copy link
Contributor Author

@sharidas sharidas Jul 5, 2017

Choose a reason for hiding this comment

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

I have updated the feature by uploading same file 4 times. Before doing this, I had verified in the UI. When tried to update same file multiple times, the encrypted version was incremented.

Given user "user0" exists
And user "user1" exists
And User "user0" uploads file "data/file_to_overwrite.txt" to "/PARENT/textfile0.txt"
And user "user0" uploads chunk file "1" of "3" with "AA" to "/PARENT/textfile0.txt"
And user "user0" uploads chunk file "2" of "3" with "BB" to "/PARENT/textfile0.txt"
And user "user0" uploads chunk file "3" of "3" with "CC" to "/PARENT/textfile0.txt"
When transfering ownership from "user0" to "user1"
Then the command was successful
And As an "user1"
And using received transfer folder of "user1" as dav path
Then Downloaded content when downloading file "/PARENT/textfile0.txt" with range "bytes=0-5" should be "AABBCC"
Copy link
Contributor

@SergioBertolinSG SergioBertolinSG Jul 5, 2017

Choose a reason for hiding this comment

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

This 'Then' has to be 'And'. (there can be only one then)


Scenario: transfering ownership of a folder
Given user "user0" exists
And user "user1" exists
Expand All @@ -24,7 +35,6 @@ Feature: transfer-ownership
And using received transfer folder of "user1" as dav path
Then Downloaded content when downloading file "/test/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership of file shares
Given user "user0" exists
And user "user1" exists
Expand All @@ -36,7 +46,6 @@ Feature: transfer-ownership
And As an "user2"
Then Downloaded content when downloading file "/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership of folder shared with third user
Given user "user0" exists
And user "user1" exists
Expand All @@ -49,7 +58,6 @@ Feature: transfer-ownership
And As an "user2"
Then Downloaded content when downloading file "/test/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership of folder shared with transfer recipient
Given user "user0" exists
And user "user1" exists
Expand All @@ -63,7 +71,6 @@ Feature: transfer-ownership
And using received transfer folder of "user1" as dav path
And Downloaded content when downloading file "/test/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership of folder doubly shared with third user
Given group "group1" exists
And user "user0" exists
Expand All @@ -79,7 +86,6 @@ Feature: transfer-ownership
And As an "user2"
Then Downloaded content when downloading file "/test/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership does not transfer received shares
Given user "user0" exists
And user "user1" exists
Expand All @@ -92,7 +98,6 @@ Feature: transfer-ownership
And using received transfer folder of "user1" as dav path
Then as "user1" the folder "/test" does not exist

@no_encryption
@local_storage
Scenario: transfering ownership does not transfer external storage
Given user "user0" exists
Expand All @@ -103,7 +108,6 @@ Feature: transfer-ownership
And using received transfer folder of "user1" as dav path
Then as "user1" the folder "/local_storage" does not exist

@no_encryption
Scenario: transfering ownership does not fail with shared trashed files
Given user "user0" exists
And user "user1" exists
Expand All @@ -115,21 +119,18 @@ Feature: transfer-ownership
When transfering ownership from "user0" to "user1"
Then the command was successful

@no_encryption
Scenario: transfering ownership fails with invalid source user
Given user "user0" exists
When transfering ownership from "invalid_user" to "user0"
Then the command error output contains the text "Unknown source user"
And the command failed with exit code 1

@no_encryption
Scenario: transfering ownership fails with invalid target user
Given user "user0" exists
When transfering ownership from "user0" to "invalid_user"
Then the command error output contains the text "Unknown target user"
And the command failed with exit code 1

@no_encryption
Scenario: transfering ownership of a folder
Given user "user0" exists
And user "user1" exists
Expand All @@ -141,7 +142,6 @@ Feature: transfer-ownership
And using received transfer folder of "user1" as dav path
Then Downloaded content when downloading file "/test/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership of file shares
Given user "user0" exists
And user "user1" exists
Expand All @@ -154,7 +154,6 @@ Feature: transfer-ownership
And As an "user2"
Then Downloaded content when downloading file "/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership of folder shared with third user
Given user "user0" exists
And user "user1" exists
Expand All @@ -167,7 +166,6 @@ Feature: transfer-ownership
And As an "user2"
Then Downloaded content when downloading file "/test/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership of folder shared with transfer recipient
Given user "user0" exists
And user "user1" exists
Expand All @@ -181,7 +179,6 @@ Feature: transfer-ownership
And using received transfer folder of "user1" as dav path
And Downloaded content when downloading file "/test/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership of folder doubly shared with third user
Given group "group1" exists
And user "user0" exists
Expand All @@ -197,7 +194,6 @@ Feature: transfer-ownership
And As an "user2"
Then Downloaded content when downloading file "/test/somefile.txt" with range "bytes=0-6" should be "This is"

@no_encryption
Scenario: transfering ownership does not transfer received shares
Given user "user0" exists
And user "user1" exists
Expand All @@ -212,7 +208,6 @@ Feature: transfer-ownership
And using received transfer folder of "user1" as dav path
Then as "user1" the folder "/sub/test" does not exist

@no_encryption
@local_storage
Scenario: transfering ownership does not transfer external storage
Given user "user0" exists
Expand All @@ -224,23 +219,20 @@ Feature: transfer-ownership
And using received transfer folder of "user1" as dav path
Then as "user1" the folder "/local_storage" does not exist

@no_encryption
Scenario: transfering ownership fails with invalid source user
Given user "user0" exists
And User "user0" created a folder "/sub"
When transfering ownership of path "sub" from "invalid_user" to "user0"
Then the command error output contains the text "Unknown source user"
And the command failed with exit code 1

@no_encryption
Scenario: transfering ownership fails with invalid target user
Given user "user0" exists
And User "user0" created a folder "/sub"
When transfering ownership of path "sub" from "user0" to "invalid_user"
Then the command error output contains the text "Unknown target user"
And the command failed with exit code 1

@no_encryption
Scenario: transfering ownership fails with invalid path
Given user "user0" exists
And user "user1" exists
Expand Down