Prefilter inaccessible shares in DefaultShareProvider::getSharedWith() #26016
Conversation
|
@PVince81, thanks for your PR! By analyzing the annotation information on this pull request, we identified @rullzer, @DeepDiver1975 and @nickvergessen to be potential reviewers |
|
Please review, I'd like to know whether this approach is acceptable before I dive into the pain of writing tests. Thanks. |
| ->selectAlias('st.id', 'storage_string_id') | ||
| ->from('share', 's') | ||
| ->leftJoin('s', 'filecache', 'f', 's.file_source = f.fileid') | ||
| ->leftJoin('f', 'storages', 'st', 'f.storage = st.numeric_id'); |
There was a problem hiding this comment.
ping DB experts @butonic @DeepDiver1975
I hope this is acceptable... Will this kill Oracle ?
Else I can make subqueries for this, not sure about perf...
There was a problem hiding this comment.
Yes, use $qb->expr()->eq()
There was a problem hiding this comment.
Assuming "Yes" is the answer for the first question "I hope this is acceptable". Thanks.
I'll change it to use $qb->expr()->eq()
There was a problem hiding this comment.
The yes was to "Will this kill Oracle ?"
97efbd9 to
afa6a99
Compare
|
Test failures unrelated. Please review @butonic @jvillafanez @DeepDiver1975 @VicDeo, needed for 9.1.1 |
| } | ||
|
|
||
| // exclude shares leading to trashbin on home storages | ||
| $pathSections = explode('/', $data['path'], 2); |
There was a problem hiding this comment.
What about external storages? Will they work properly?
There was a problem hiding this comment.
Yes, see below, the crude check for home storage
|
👍 |
afa6a99 to
472dbc7
Compare
|
Rebased for hopefully passing tests |
The DefaultShareProvider now does a DB-level check to find out whether file_source is accessible at all (deleted file) or whether it's in the trashbin of a home storage. One small corner case where the home storage id is in md5 form cannot be covered properly with this approach.
472dbc7 to
84c9613
Compare
|
stable9.1: #26050 (yes, am optimistic about the tests...) |
|
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. |
Description
See #26001 for context.
We need to prevent the shared mount provider to expose mounts where the source file is not accessible any more when in trash or completely deleted. This is important to avoid issues where the user cannot create a folder with the name of the mount point because it's still there but "ghosted".
This fix guarantees that the mount point is not present at all and all code paths for file operations won't be impaired by a ghost mount.
What this does: prefilter inaccessible shares in DefaultShareProvider::getSharedWith()
The DefaultShareProvider now does a DB-level check to find out whether file_source is accessible at all (deleted file) or whether it's in the trashbin of a home storage.
Note: One small corner case where the home storage id is in md5 form cannot be covered properly with this approach. This would only happen for user ids longer than
64 - strlen('home::')which is 58 characters. LDAP ids are usually shorter than that.Related Issue
Fixes #26001
Motivation and Context
Pfff, because of shared storage HELL, that is. See context.
How Has This Been Tested?
Types of changes
Checklist:
Backports:
Open TODOS: