Verify that a file exists before we share it

[icewind1991]

Prevents invalid entries from getting into the share table and folder

cc @karlitschek @DeepDiver1975 @LukasReschke

[LukasReschke]

I think patching it here is the wrong approach as there may be other apps which are using this function wrong. We should - at least in master - do this check within OCP\shareItem() and there already plenty of other checks, is there any reason not to implement it there?

(

core/lib/public/share.php

Line 475 in b2757e6

public static function shareItem($itemType, $itemSource, $shareType, $shareWith, $permissions, $itemSourceName = null) {

)

[icewind1991]

You're right, that's a better approach

[DeepDiver1975]

@icewind will Filesystem::getPath () return false for files the user has no access to - or only for invalid file ids?
I just want to make sure.

[icewind1991]

@icewind will Filesystem::getPath () return false for files the user has no access to - or only for invalid file ids?

It will return false if the id is not found within any of the storages the user has access to.

@LukasReschke updated

[DeepDiver1975]

@icewind1991 unit test? 😉

[ghost]

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3765/

[PVince81]

It works 👍 (I've tested before and after the fix with curl)

[icewind1991]

Failing unit tests are related to resharing, adding support for "regular" resharing is easy but I dont know how to add support for resharing a file from a shared folder.
From what I can tell there is no proper way of telling if a file is within a folder to the user, especially since we only have the fileid and dont know the path

[PVince81]

Hmm, I forgot to test resharing... trying out now.

[PVince81]

Manual test also fails for reshare.
@schiesbn is off this week, we'll need to figure this out ourselves

[PVince81]

Another idea would be that if the check fails, find out whether the current user has access to that file. There should be another entry in the sharing table where "share_with = 'current_user' AND item_source = ? and share_type = ?".

Not sure how to use that convoluted internal API to request this. Maybe looking at how the OCS Share API does it might help.

[icewind1991]

I already got that far, it's a simple one line addition.

The problem is files from shared folders, if the folder foo is shared than there wont be an entry for foo/bar making it no possible for us to detect if the user has access to foo/bar

[PVince81]

If I remember well there might be a way (in the OCS Share API) to detect if a file in a subfolder is shared / what share it belongs to.

[PVince81]

Just tried it, and it doesn't work. It looks like it's not possible to use the OCS Share API to get a share id when being the receiver of the share.

In the example above, I tried calling /ocs/v1.php/apps/files_sharing/api/v1/shares?path=/Shared/foo and it doesn't return the share id. Same for /Shared/foo/bar.
It probably only works if the current user is the "sharer", not the "share receiver".

@purigarcia can you confirm that the above is correct ?

[PVince81]

@icewind1991 Just found this method: \OCP\Share::getUsersSharingFile() which might bring us a bit closer.

[icewind1991]

@PVince81 unfortunately that requires knowing the owner of the file and the path

[schiessle]

@PVince81 the result of the OCS call is correct. You should only get the share ID for files you shared.

I think the problem we try to fix here doesn't exists for the ocs api because the ocs api works with the path instead of the file id and if you try to a share a path which doesn't exists you will get an error.

@icewind1991 Is there no way to get the owner and the owners path for a file if we know the file id?

If this really isn't possible than I think we need to solve the problem at a higher level. In this case the ajax call should contain the path to the file for the current user, similar to the ocs api. This way we can easily do all the checks we want. The only problem, it looks like the methods for the ajax calls are generic ones which get also called for other shares.

[icewind1991]

Passing the path in the ajax call seems like the best solution, then the check becomes a simple file_exists and the shared storage will handle cases such as resharing

[PVince81]

Any update here ?
Needs rebase it seems.

[icewind1991]

Waiting for #7935

[icewind1991]

rebased and squashed.

@PVince81 please retest (also with reshares)

[ghost]

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4007/

[PVince81]

Tests:

• Prevent sharing file with known id from another user
• Normal sharing still work
• Reshares still work

[PVince81]

👍

[PVince81]

@icewind1991 can you add a unit test for this case ?

[icewind1991]

@PVince81 done

[ghost]

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4029/

[scrutinizer-notifier]

The inspection completed: 1 new issues, 3 updated code elements

[PVince81]

@icewind1991 awesome!

@schiesbn can you please review this ?

[icewind1991]

@schiesbn @DeepDiver1975 can you have a look at this

[schiessle]

looks good to me! 👍

[LukasReschke]

This needs a backport. #7935 has been already backported. What do you think @karlitschek?

@icewind1991 Can you take care of this?

[karlitschek]

yes. agreed. Backport please

[icewind1991]

Stable6: d4be12a
Stable5: 2264615