-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Changes to encryption wrapper #28774
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.
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
Conversation
PVince81
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great progress, see comments
let's use the static approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if I understand correctly, this flag only affects write operations ?
If yes, call it $disableWriteEncryption instead and the methods setDisableWriteEncryption()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pass the flag as argument, no need to write two methods...
lib/private/Files/View.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer that the View doesn't know anything about encryption. Encryption is just like a plugin that inserts itself so it's not good that the core code like View relies on encryption specific things.
Let's make flag static in the Encryption class above instead and set it from outside. That should do for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to call isset earlier, else line 478 is going to throw a warning when not set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sharidas did you miss this comment ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I thought in a different way. wrap() expects either null or string from$this->sourcePath[$path]. So if $this->sourcePath[$path] not set then I set as null at https://github.com/owncloud/core/pull/28774/files#diff-938b56891f3079e628cfda1c331911b7R476 .
Else if its set then unset at line 484. Terrible idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use unset for maximum deletion
7e04194 to
db6b456
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to break horribly with external storage because you are doing a fopen directly on the filesystem.
Use $this->getWrapperStorage()->fopen() instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PVince81 Updated in my latest PR. Thanks for pointing it out.
533c1d7 to
313e91a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope this looks better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function must be static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
313e91a to
e440f98
Compare
|
| /** @var ArrayCache */ | ||
| private $arrayCache; | ||
|
|
||
| private $sourcePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPDoc comments please
PVince81
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 add missing PHP doc then this is good to go
Changes made to encryption wrapper so that we can get the commands like transfer-ownership or recreate masterkey work. This change doesn't alter the core functionality of copy or fopen function. The arguments passed to the function remains same as other wrappers. Signed-off-by: Sujith H <[email protected]>
e440f98 to
e0a0851
Compare
|
@sharidas please backport to stable10 |
|
Backport of this PR: #28845 |
|
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. |
Changes made to encryption wrapper so that we can
get the commands like transfer-ownership or recreate
masterkey work. This change doesn't alter the core
functionality of copy or fopen function. The arguments
passed to the function remains same as other wrappers.
Signed-off-by: Sujith H [email protected]
Description
Changes made to the ecnryption warpper and view to get the transfer-ownership command recreate master key work. Basically there was a problem which caused to change the arguments passed to the routines like fopen or copy. With this change we don't have to alter them. But we use flags to control the code flow for the commands.
Related Issue
Motivation and Context
This change would prevent the modification to common routines which were made to arguments of functions like fopen or copy etc.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: