Skip to content

Check the permission of the underlying storage - #11041

Merged
nickvergessen merged 4 commits into
masterfrom
fix/noid/get_permission_of_storage_for_shares
Sep 19, 2018
Merged

Check the permission of the underlying storage#11041
nickvergessen merged 4 commits into
masterfrom
fix/noid/get_permission_of_storage_for_shares

Conversation

@rullzer

Copy link
Copy Markdown
Member

Else shares might expose more permissions than the storage actually
providers.

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

@nickvergessen

Copy link
Copy Markdown
Member
There were 3 errors:
1) OCA\Files_Sharing\Tests\ApiTest::testGetShareFromFolderReshares
Undefined offset: 0
/drone/src/github.com/nextcloud/server/apps/files_sharing/tests/ApiTest.php:654
2) OCA\Files_Sharing\Tests\ApiTest::testGetShareFromFileReReShares
OCP\Share\Exceptions\GenericShareException: You are not allowed to share /test-share-user2/files/subfolder_share_api_test/share-api-test.txt
/drone/src/github.com/nextcloud/server/lib/private/Share20/Manager.php:283
/drone/src/github.com/nextcloud/server/lib/private/Share20/Manager.php:592
/drone/src/github.com/nextcloud/server/apps/files_sharing/tests/ApiTest.php:859
3) OCA\Files_Sharing\Tests\ApiTest::testDeleteReshare
OCP\Share\Exceptions\GenericShareException: You are not allowed to share /test-share-user2/files/folder_share_api_test/share-api-test.txt
/drone/src/github.com/nextcloud/server/lib/private/Share20/Manager.php:283
/drone/src/github.com/nextcloud/server/lib/private/Share20/Manager.php:592
/drone/src/github.com/nextcloud/server/apps/files_sharing/tests/ApiTest.php:1109

@nickvergessennickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 4, 2018
@rullzer

Copy link
Copy Markdown
MemberAuthor

Ok so the reason it does boom is because of

$results = $this->getCache()->getFolderContentsById($fileId);

Which is the function called from the shared cache. This gives back the actual path instead of the jailed one. Making the getPermissions do 💥.

@icewind1991 do we have to fixed the shared cache for this?

@rullzer
rullzerforce-pushed the fix/noid/get_permission_of_storage_for_shares branch from 48957a7 to 74971a2CompareSeptember 5, 2018 20:48
@rullzer

Copy link
Copy Markdown
MemberAuthor

@icewind1991 please have a look if 74971a2 makes sense.

It does to me but you know more about the internal dark magic of the storages.

Comment threadapps/files_sharing/lib/Cache.php Outdated
// Not a valid action for Shared Cache
}

public function getFolderContentsById($fileId) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this shouldn't be needed as it's the same as the base method

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hehe late night hacking ;)

@rullzer
rullzerforce-pushed the fix/noid/get_permission_of_storage_for_shares branch from 74971a2 to b32b9dbCompareSeptember 6, 2018 10:07
Comment threadapps/files_sharing/lib/Cache.php

@nickvergessennickvergessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still works

@rullzer

Copy link
Copy Markdown
MemberAuthor

It seems this makes other stuff do 💥. (yay for part files).
I'll try to come up with a fix

Else shares might expose more permissions than the storage actually
providers.
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
isCreatable only works on folders
isUpdatable if the file is not there but it is a part file also has to
be checked on the folder
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Now that we actually check thepermissions properly we have to update the
tests.
* We checked an invalid path
* We checked from wrong permissions (files never have CREATE permissions
for example)
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
rullzerforce-pushed the fix/noid/get_permission_of_storage_for_shares branch from b32b9db to a2725c3CompareSeptember 6, 2018 15:10
@rullzer

Copy link
Copy Markdown
MemberAuthor

Ok all good now I think...

@nickvergessen@icewind1991 please have yet another look.

@rullzerrullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 6, 2018
@nickvergessen

Copy link
Copy Markdown
Member

@icewind1991 your +1 still counts?

@icewind1991

Copy link
Copy Markdown
Member

👍

@nickvergessen
nickvergessen merged commit 06141b2 into masterSep 19, 2018
@nickvergessen
nickvergessen deleted the fix/noid/get_permission_of_storage_for_shares branch September 19, 2018 10:15
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rullzer@nickvergessen@icewind1991@blizzz@MorrisJobke