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.

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

Fix wrong condition so unreadable files won't be in the filecache #40480

Closed
wants to merge 1 commit into from

Conversation

jvillafanez
Copy link
Member

Description

Files without read permission won't be in the filecache. A files:scan is also expected to remove the files without read permission (as long as the scanning goes through the target file).
This is also expected to affect the clients because the target file won't be in the propfind response any longer.

Related Issue

#40460

Motivation and Context

Need to check if this is the right behavior. Up until now, permissions were stored in the filecache even if the file was unreadable. This has led to files with write permissions but not read permissions in the fliecache. This seems to be wrong accordingly to the modified condition.

Note that file metadata such as comments and tags will be either removed or disconnected because the file will be removed. If the file regains the read permission at a later point, it will be considered a new file with a new fileid.

This PR might cause side-effects.

How Has This Been Tested?

  1. Add a file in ownCloud
  2. Change the permissions in the FS so the webserver can't read the file
  3. Run a files:scan

The file is removed from the filecache table.

There are several cases not tested yet, specially with external storages.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

Notes

@pmaier1 I think we need to decide which is the right behavior.

  • If the file isn't readable, removing the file will also remove its metadata (or it will be disconnected). This might be a problem if we want to keep the metadata in case the read permission has been removed accidentally. This is what this PR is doing.
  • If we want to keep the current behavior, we should remove the condition. Note that we have to handle files with any permission combination as weird as it could be.
  • We might want to skip the file only if there is no stored data in the DB. If we have data, but the permission changed, we could try to update the permission without removing the data. Later, if the file regains the read permission, the metadata should still be present

@update-docs
Copy link

update-docs bot commented Nov 8, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants