Check trashbin perms before moving to trash #29820

PVince81 · 2017-12-12T09:50:09Z

Description

Check whether the target trashbin is writable (ex: guests).
In the case where a guest deletes a file as recipient, the owner would
get a copy in their trash. The logic here will detect that the guests'
trashbin is not writeable so it will fall back to simply moving directly
to the owner's trashbin.

Related Issue

owncloud/guests#31 approach 1

Motivation and Context

How Has This Been Tested?

Manually tested.

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.

TODOs:

add unit tests if we agree on the approach

codecov · 2017-12-12T12:30:55Z

Codecov Report

Merging #29820 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #29820      +/-   ##
============================================
+ Coverage     58.21%   58.27%   +0.05%     
- Complexity    18517    18522       +5     
============================================
  Files          1095     1095              
  Lines         63715    63722       +7     
============================================
+ Hits          37093    37135      +42     
+ Misses        26622    26587      -35

Impacted Files	Coverage Δ	Complexity Δ
drone/src/lib/private/Files/Cache/Cache.php	`96.63% <0%> (-0.58%)`	`102% <0%> (+2%)`
drone/src/lib/private/Server.php	`82.55% <0%> (-0.15%)`	`129% <0%> (ø)`
drone/src/apps/files_trashbin/lib/Trashbin.php	`76.19% <0%> (+0.1%)`	`137% <0%> (+3%)`	⬆️
drone/src/lib/private/legacy/util.php	`67.97% <0%> (+1.36%)`	`222% <0%> (ø)`	⬇️
.../src/lib/private/Files/Storage/Wrapper/DirMask.php	`48.52% <0%> (+4.41%)`	`34% <0%> (ø)`	⬇️
drone/src/lib/private/Files/Cache/CacheEntry.php	`94.73% <0%> (+5.26%)`	`20% <0%> (ø)`	⬇️
...c/lib/private/Files/Cache/Wrapper/CacheWrapper.php	`86.36% <0%> (+7.57%)`	`30% <0%> (ø)`	⬇️
.../private/Files/Storage/Wrapper/PermissionsMask.php	`93.02% <0%> (+9.3%)`	`39% <0%> (ø)`	⬇️
...lib/private/Files/Storage/Wrapper/ReadOnlyJail.php	`94.73% <0%> (+21.05%)`	`8% <0%> (ø)`	⬇️
...les/Cache/Wrapper/ReadOnlyCachePermissionsMask.php	`100% <0%> (+100%)`	`4% <0%> (ø)`	⬇️

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 81153de...c7caea2. Read the comment docs.

PVince81 · 2018-01-16T14:56:25Z

decision clarified and we agreed on the approach: need to update unit tests

PVince81 · 2018-01-16T15:46:16Z

Unit test added. Needs review.

IljaN

LGTM

PVince81 · 2018-01-17T09:56:57Z

Grrrr... Oracle

1) OCA\Files_Trashbin\Tests\StorageTest::testDeleteFolderAsReadOnlyRecipient
OCP\Files\NotPermittedException: No create permission for folder /recipient_1Ky0E38xz5c26/files

/drone/src/lib/private/Files/Node/Folder.php:162
/drone/src/lib/private/Files/Node/Root.php:369
/drone/src/lib/private/Server.php:982
/drone/src/tests/lib/TestCase.php:373
/drone/src/apps/files_trashbin/tests/StorageTest.php:276

PVince81 · 2018-01-17T10:21:18Z

I did some debugging with MariaDB and I can see that the Folder::checkPermissions call for create permissions is done on "/recipient_xyz" which allows the creation of a "files" folder for a read-only user.
Not sure if that's intended. That aside, I don't understand why Oracle would have a different result there.

However the output with Oracle seems to imply that the check is done on "/recipient_xyz/files" instead, which disallows anything else than read.

IljaN · 2018-01-17T10:50:38Z

The "ReadOnlyJail" is initialized before first-login directory structure creation, so it need to allow "/files" creation.

PVince81 · 2018-01-17T11:25:19Z

@IljaN thanks for the info.

Seems I'll need to boot up an Oracle docker to solve this mystery.

PVince81 · 2018-01-17T13:03:56Z

When running tests on Oracle: the individual test file works, but when running all the tests, some kind of side effect is causing the issue above.

PVince81 · 2018-01-17T13:47:37Z

Debug comparison: getFileInfo("/recipient_xyz") always reads the permissions from oc_filecache.
On Oracle the database contains the permissions 1 but on other DBs it contains 4. Go figure...

Maybe the read-only wrapper kicks in much earlier with Oracle, could be scan time (when?!) and reads the already masked value.

PVince81 · 2018-01-17T15:22:39Z

I checked the database and both Oracle and MariaDB have the same "permissions" value.

However the getFileInfo call still returns 1 for Oracle.

PVince81 · 2018-01-17T15:36:52Z

Remember that Oracle consider empty strings as null.
When reading the cache entry for "/recipient_xyz" this is actually the filecache entry with name === "" which is actually null on Oracle. So what happens is that $data['name'] is null instead of empty string.

I didn't yet reach the correct code but I suspect that there is a check against empty string somewhere that might not kick in when null. I remember @IljaN saying that the permissions on the root was more permissive than on "/files".

Let me fix that in the Cache and convert null to empty string... I'm sure this will somewhat solve a lot of other yet undiscovered Oracle problems...

IljaN · 2018-01-17T15:39:56Z

@PVince81 The root folder is indeed more permissive because for example cache, avatars directories needs to be created in there.

PVince81 · 2018-01-17T15:48:30Z

Confirmed, it works with this fix c30c8ec

PVince81 · 2018-01-17T16:03:16Z

Please do review my last commit. I think it looks alright.

Check whether the target trashbin is writable (ex: guests). In the case where a guest deletes a file as recipient, the owner would get a copy in their trash. The logic here will detect that the guests' trashbin is not writeable so it will fall back to simply moving directly to the owner's trashbin.

PVince81 · 2018-01-18T09:22:33Z

@ownclouders rebase

ownclouders · 2018-01-18T09:22:35Z

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

ownclouders · 2018-01-18T09:23:03Z

Automated rebase with GitMate.io was successful! 🎉

PVince81 · 2018-01-22T14:39:31Z

stable10: #30224

PVince81 · 2018-01-23T14:29:29Z

I missed the trash commit, backported here: #30240

lock · 2019-08-01T10:02:45Z

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.

PVince81 added the 3 - To Review label Dec 12, 2017

PVince81 added this to the development milestone Dec 12, 2017

PVince81 self-assigned this Dec 12, 2017

PVince81 mentioned this pull request Dec 12, 2017

Sort out deletion / trashbin issue owncloud/guests#31

Closed

ownclouders added the status/STALE label Dec 20, 2017

PVince81 modified the milestones: development, planned Jan 12, 2018

ownclouders removed the status/STALE label Jan 16, 2018

PVince81 force-pushed the trashbin-check-perms branch from 2630d03 to 8d5ad06 Compare January 16, 2018 15:45

PVince81 requested review from IljaN, VicDeo and DeepDiver1975 January 16, 2018 15:47

IljaN approved these changes Jan 16, 2018

View reviewed changes

PVince81 added 4 - To release backport-request and removed 3 - To Review labels Jan 16, 2018

IljaN approved these changes Jan 17, 2018

View reviewed changes

Vincent Petry added 2 commits January 18, 2018 09:22

Convert null to empty string for Oracle in file cache accessor

c7caea2

ownclouders force-pushed the trashbin-check-perms branch from c30c8ec to c7caea2 Compare January 18, 2018 09:23

PVince81 merged commit 0a312fc into master Jan 22, 2018

PVince81 deleted the trashbin-check-perms branch January 22, 2018 14:33

PVince81 mentioned this pull request Jan 22, 2018

[stable10] Convert null to empty string for Oracle in file cache accessor #30224

Merged

PVince81 removed the backport-request label Jan 22, 2018

PVince81 mentioned this pull request Jan 23, 2018

[stable10] Check trashbin perms before moving to trash #30240

Merged

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check trashbin perms before moving to trash #29820

Check trashbin perms before moving to trash #29820

PVince81 commented Dec 12, 2017

codecov bot commented Dec 12, 2017 •

edited

Loading

PVince81 commented Jan 16, 2018

PVince81 commented Jan 16, 2018

IljaN left a comment

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

IljaN commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

IljaN commented Jan 17, 2018 •

edited

Loading

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 18, 2018

ownclouders commented Jan 18, 2018

ownclouders commented Jan 18, 2018

PVince81 commented Jan 22, 2018

PVince81 commented Jan 23, 2018

lock bot commented Aug 1, 2019

Check trashbin perms before moving to trash #29820

Check trashbin perms before moving to trash #29820

Conversation

PVince81 commented Dec 12, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

TODOs:

codecov bot commented Dec 12, 2017 • edited Loading

Codecov Report

PVince81 commented Jan 16, 2018

PVince81 commented Jan 16, 2018

IljaN left a comment

Choose a reason for hiding this comment

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

IljaN commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

IljaN commented Jan 17, 2018 • edited Loading

PVince81 commented Jan 17, 2018

PVince81 commented Jan 17, 2018

PVince81 commented Jan 18, 2018

ownclouders commented Jan 18, 2018

ownclouders commented Jan 18, 2018

PVince81 commented Jan 22, 2018

PVince81 commented Jan 23, 2018

lock bot commented Aug 1, 2019

codecov bot commented Dec 12, 2017 •

edited

Loading

IljaN commented Jan 17, 2018 •

edited

Loading