Call getFullPath only if its defined in storage by sharidas · Pull Request #29337 · owncloud/core

sharidas · 2017-10-24T11:13:03Z

Call getFullPath only if its defined in storage.
Else use mount point to resolve the full path.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

This change adds getFullPath method to shared external storage file.

Related Issue

#29167

Motivation and Context

Setup to recreate the issue:

Create 2 instances of oC. Lets say oC1 and oC2.
oC1 is not encrypted and oC2 is encrypted ( tested with masterkey ). oC2 is added as an external storage to oC1.
Create a folder in oC2 name test
Share the folder to an oC1 user user1 using federated share
Now login as user1 in oC1 and create a file under test
Try to delete the file. The log file in oC1, can see message mentioned in the issue.
Since oC1 is non encrypted and when file under test is being deleted, the https://github.com/owncloud/core/blob/v10.0.3/lib/private/Files/Storage/Wrapper/Encryption.php#L786 code is hit. The reason is its a cross storage move. And getFullPath is defined only in Encryption storage. Its not available in other storages. So during the iteration to find the getFullPath method under storage wrappers, it couldn't find and throws the error. This change helps us to resolve that issue.

How Has This Been Tested?

Recreated the issue with the steps mentioned above. After applying this change when the file is deleted, the log file doesn't spit the error message.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

sharidas · 2017-10-24T11:14:36Z

@PVince81 Let me know if you have suggestions to improve this change.

codecov · 2017-10-24T13:41:51Z

Codecov Report

Merging #29337 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #29337      +/-   ##
============================================
+ Coverage     60.21%   60.21%   +<.01%     
- Complexity    17183    17184       +1     
============================================
  Files          1030     1030              
  Lines         57280    57281       +1     
============================================
+ Hits          34493    34494       +1     
  Misses        22787    22787

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/Storage/Wrapper/Encryption.php	`68.51% <100%> (+0.07%)`	`151 <0> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c12854c...6748fba. Read the comment docs.

PVince81 · 2017-10-25T08:49:24Z

This is just a quick workaround as it adds the method only on one implementation instead of the interface. I'm not sure if we should add this on the interface as it would affect every possible storage implementation out there.

@sharidas please also add a PHP "has_method" (or "is_method"?) check in the encryption code to check if the target storage does have the method before calling. If not, find a way to fallback.

sharidas · 2017-10-25T14:30:26Z

@PVince81 Hope the new change looks better.

PVince81 · 2017-10-25T15:10:26Z

+					if (method_exists($sourceStorage, "getFullPath")) {
+						$this->sourcePath[$targetInternalPath] = $sourceStorage->getFullPath($sourceInternalPath);
+					} else {
+						$this->sourcePath[$targetInternalPath] = $mountPoint . '/' . $sourceInternalPath;


can $mountPoint be undefined or is it always set ?

With OC instance with encryption enabled:

External Storage
a) SFTP
Delete a file from SFTP. mountPoint is not empty.
Restore the file from trashbin. mountPoint is not empty
b) ownCloud
Delete a file from another oC instance ( encryption enabled ). mountPoint is not empty

With the tests done other than fed share, I found that if condition is not getting true. The reason I guess is wrapper could be checksum and which doesn't have "getFullPath", so it moves to else.

Call getFullPath only if its defined in storage. Else use mount point to resolve the full path. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81

👍 if CI passes

sharidas · 2017-10-26T09:43:38Z

 				$source = $sourceStorage->fopen($sourceInternalPath, 'r');
-				if ($isRename) {
-					$this->sourcePath[$targetInternalPath] = $sourceStorage->getFullPath($sourceInternalPath);
+				if ($isRename && (count($mount) === 1)) {


This way $mount is not used without defining. The path obtained using this method and getFullPath in the scenario of transfer file onwership between users ( with masterkey encryption ), both path remains same.

sharidas · 2017-10-26T10:19:03Z

Backport PR : #29360

lock · 2019-08-02T05:01:38Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

sharidas self-assigned this Oct 24, 2017

sharidas requested a review from PVince81 October 24, 2017 11:13

sharidas added the 3 - To Review label Oct 24, 2017

sharidas added this to the development milestone Oct 24, 2017

sharidas mentioned this pull request Oct 24, 2017

Delete: call_user_func_array() expects parameter 1 to be a valid callback #29167

Closed

sharidas force-pushed the fix-getfullpath-sharedstorage branch from 426a3b4 to 8a0ee8d Compare October 25, 2017 13:50

sharidas changed the title ~~Add getFullPath method to shared external storage~~ Call getFullPath only if its defined in storage Oct 25, 2017

PVince81 reviewed Oct 25, 2017

View reviewed changes

sharidas force-pushed the fix-getfullpath-sharedstorage branch 2 times, most recently from e0f3acd to cd39315 Compare October 26, 2017 08:26

Call getFullPath only if its defined in storage

6748fba

Call getFullPath only if its defined in storage. Else use mount point to resolve the full path. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the fix-getfullpath-sharedstorage branch from cd39315 to 6748fba Compare October 26, 2017 08:58

PVince81 approved these changes Oct 26, 2017

View reviewed changes

PVince81 added the backport-request label Oct 26, 2017

sharidas commented Oct 26, 2017

View reviewed changes

sharidas mentioned this pull request Oct 26, 2017

[stable10] Call getFullPath only if its defined in storage #29360

Merged

9 tasks

PVince81 merged commit f4b3388 into master Oct 26, 2017

PVince81 deleted the fix-getfullpath-sharedstorage branch October 26, 2017 11:53

PVince81 removed the backport-request label Nov 3, 2017

ownclouders added the status/STALE label Dec 20, 2017

lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Call getFullPath only if its defined in storage#29337

Call getFullPath only if its defined in storage#29337
PVince81 merged 1 commit into
masterfrom
fix-getfullpath-sharedstorage

sharidas commented Oct 24, 2017 •

edited

Loading

Uh oh!

sharidas commented Oct 24, 2017

Uh oh!

codecov Bot commented Oct 24, 2017 •

edited

Loading

Uh oh!

PVince81 commented Oct 25, 2017

Uh oh!

sharidas commented Oct 25, 2017

Uh oh!

PVince81 Oct 25, 2017

Uh oh!

sharidas Oct 25, 2017

Uh oh!

sharidas Oct 25, 2017

Uh oh!

PVince81 left a comment

Uh oh!

sharidas Oct 26, 2017

Uh oh!

sharidas commented Oct 26, 2017

Uh oh!

lock Bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

sharidas commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

sharidas commented Oct 24, 2017

Uh oh!

codecov Bot commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PVince81 commented Oct 25, 2017

Uh oh!

sharidas commented Oct 25, 2017

Uh oh!

PVince81 Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

sharidas Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

sharidas Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

sharidas Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

sharidas commented Oct 26, 2017

Uh oh!

lock Bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sharidas commented Oct 24, 2017 •

edited

Loading

codecov Bot commented Oct 24, 2017 •

edited

Loading