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

Jump to bottom

When calling file_put_contents clear the checksum #24098

Merged
merged 1 commit into from
Apr 20, 2016
Merged

When calling file_put_contents clear the checksum #24098

merged 1 commit into from
Apr 20, 2016

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Apr 19, 2016 •
edited
Loading

Fixes #23782 and #23783

If a file is changed we should reset the checksum.

  • Unit test added

To test:

  • Upload a file with checksum
    curl -u admin:admin http://localhost/master/remote.php/webdav/foo.txt -X PUT -T foo.txt -H "OC-Checksum: foobar"
  • Check the file cache. This file should have the checksum foobar
  • Download the file the OC-Checksum header should be set
  • Now edit the file in the texteditor and close it
  • Check the file cache. This file should have an empty checksum
  • Download the file the OC-Checksum header should not be set

Test 2:

  • Upload a file with checksum
    curl -u admin:admin http://localhost/master/remote.php/webdav/foo.txt -X PUT -T foo.txt -H "OC-Checksum: foobar"
  • Check the file cache. This file should have the checksum foobar
  • Download the file the OC-Checksum header should be set
  • Now edit the file directly on disk
  • Run ./occ files:scan admin
  • Check the file cache. This file should have an empty checksum
  • Download the file the OC-Checksum header should not be set

CC: @PVince81 @nickvergessen @MorrisJobke @dragotin

@rullzer rullzer added this to the 9.1-current milestone Apr 19, 2016
@rullzer rullzer mentioned this pull request Apr 19, 2016
Closed
PVince81
PVince81 reviewed Apr 19, 2016
View reviewed changes
lib/private/files/cache/updater.php
@@ -126,6 +126,9 @@ public function update($path, $time = null) {
$this->cache->correctFolderSize($path, $data);
}
$this->correctParentStorageMtime($path);
if ($data['mimetype'] !== 'httpd/unix-directory') {
$this->clearChecksum($path);
Copy link
Contributor

Choose a reason for hiding this comment

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

okay, I guess there is no easy way to integrate this change in one of the existing database calls.

@icewind1991 or is there a better moment ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it gets kind of messy trying to cram this into the scan call I think

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make sense in general to clear the checksum in the scanner if it detects a change

Copy link
Contributor

Choose a reason for hiding this comment

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

@icewind1991 This will likely be needed too for the other issue #23783 but would it also cover the text editor / file_put_contents issue too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scanFile indeed might be a good place...

@rullzer
When the scanner detects a file is changed clear checksum
0dbcbc4 
Fixes #23782 and #23783

If the file scanner detects a changed file we clear the checksum while
we update the cache.

* Unit test added
@rullzer
Copy link
Contributor Author

rullzer commented Apr 19, 2016

Moved the check to the scanFile. Patch is simpler and cleaner.

@PVince81 @icewind1991 please have another look.

@rullzer rullzer mentioned this pull request Apr 19, 2016
Closed
@icewind1991
Copy link
Contributor

Code looks good 👍

@rullzer
Copy link
Contributor Author

rullzer commented Apr 20, 2016

@karlitschek we should backport this to 9.0.2

@karlitschek
Copy link
Contributor

great. fix makes sense. please backport to stable9

@PVince81
Copy link
Contributor

  • TEST: text editor save clears checksum
  • TEST: occ files:scan with unchanged file leaves checksum alone
  • TEST: occ files:scan when file was changed directly on disk properly resets checksum

@PVince81
Copy link
Contributor

  • TEST: file changed remotely on SFTP ext storage, GET the file directly => checksum properly cleared

Looks good 👍

@PVince81
Copy link
Contributor

@rullzer can you prepare the backport PR ?

@rullzer rullzer mentioned this pull request Apr 20, 2016
Merged
@rullzer
Copy link
Contributor Author

rullzer commented Apr 20, 2016

Backport in #24129

@DeepDiver1975 DeepDiver1975 merged commit cdcabbd into master Apr 20, 2016
@DeepDiver1975 DeepDiver1975 deleted the fix_23782 branch April 20, 2016 18:37
@lock
Copy link

lock bot commented Aug 6, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
4 - To release comp:filesystem sev1-critical Type:Bug
Projects
None yet
Milestone
9.1
Development

Successfully merging this pull request may close these issues.

TextEditor: Checksum not invalidated on file change
6 participants
@rullzer @icewind1991 @karlitschek @PVince81 @MorrisJobke @DeepDiver1975