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

implement share handling for accepting and listing folder shares #929

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Jul 2, 2020

I implemented share handling functionality in the owncloud storage driver. Most of the code is similar to how eosfs handles shares. There are just some small storage specific differences.

@labkode, I'm not sure if my changes in internal/grpc/services/gateway/storageprovider.go will break other storage drivers.
I tried to test it with eos but was not able to create share references with it, even without my changes but with the master branch it didn't work.

The reason why I'm using path.Split instead of isSharedFolder or isShareName is that the latter assume the path is like /home/MyShare/somefolder/file.txt. In my understanding that would mean you couldn't share a file from the root folder because such file would have a path like /home/MyShare/file.txt.

Furthermore in line 138 we only care about if the path is a file oder a folder and we can easily distinguish that with path.Split.
What do you think?

Closes owncloud/ocis-reva#11

@labkode
Copy link
Member

labkode commented Jul 2, 2020

@C0rby the logic around the base path is wrong. When base is empty means the path is either / or empty string.
Access paths outside the share folder is already covered by:

if !s.inSharedFolder(ctx, p) {
		return s.initiateFileUpload(ctx, req)
}

so this condition is not triggered.

The method isShareChild is more semantic that the path split.

Also, with your changes you will stat resources inside a share that don't exist in the home provider, they exist in original storage provider. This assumes that the storage drivers they need to handle resources outside its own storage provider and it not the case. Furthermore, this also assumes that you can create a reference with storage id and paths mixed, also not possible.

Please revert the changes in internal/grpc/services/gateway/storageprovider.go.
The filesystem implementation should work without those changes.

Copy link
Member

@labkode labkode left a comment

Choose a reason for hiding this comment

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

Changes requested

@C0rby
Copy link
Contributor Author

C0rby commented Jul 2, 2020

@C0rby the logic around the base path is wrong. When base is empty means the path is either / or empty string.

No, when the base is empty the path can be a path to a folder.

path.Split("/home/MyShare/somefolder/") => dir == "/home/MyShare/somefolder/", base == ""
path.Split("/home/MyShare/somefile") => dir == "/home/MyShare/", base == "somefile"

But I'll try to make the owncloud storage driver work without the changes in storageprovider.go.

@labkode
Copy link
Member

labkode commented Jul 3, 2020

@C0rby, I see, however we should not assume folder paths always end in a slash.

@C0rby
Copy link
Contributor Author

C0rby commented Jul 3, 2020

Can you clarify something for me?

// /home/MyShares/photos/
func (s *svc) isShareName(ctx context.Context, p string) bool {
	return s.split(ctx, p, 3)
}

// /home/MyShares/photos/Ibiza/beach.png
func (s *svc) isShareChild(ctx context.Context, p string) bool {
	return s.split(ctx, p, 4)
}

Both of these methods assume that the path contains at least 3 folders before the actual shared file but in the usershareprovider.createReference the reference path is build to /home/MyShare/x where x is the file.
Here a comment from the createReference method:

// reference path is the home path + some name
// CreateReferene(cs3://home/MyShares/x)
// that can end up in the storage provider like:
// /eos/user/.shadow/g/gonzalhu/MyShares/x

In my understanding this doesn't fit, either isShareName and isShareChild must be changed or createReference.
Or am I missing something here?

@labkode
Copy link
Member

labkode commented Jul 7, 2020

@C0rby both of those methods assume the path contains at least two folder, /home/MyShares before the shared reference. Those methods and the create reference are correct.

In the current design we lock down to the /home/MyShares folder where shares can be mounted.

@C0rby
Copy link
Contributor Author

C0rby commented Jul 7, 2020

@labkode, I know about /home/MyShares, that part is alright.

What I mean is:
The path created by storageprovider.CreateReference is always 3 parts long i.e. /home/beach.png results in the path /home/MyShares/beach.png same with /home/photos/beach.png, this also results in /home/MyShares/beach.png.

So now when we call isShareName(ctx, "/home/MyShares/beach.png") this will return true since it consists of 3 parts even though the last part is a file not a folder.

The call to isShareChild(ctx, "/home/MyShares/beach.png") would then fail since it consists of 3 not 4 parts.

My problem is line 138-148 in internal/grpc/services/gateway/storageprovider.go

log := appctx.GetLogger(ctx)
if s.isSharedFolder(ctx, p) || s.isShareName(ctx, p) {
	log.Debug().Msgf("path:%s points to shared folder or share name", p)
	err := errtypes.PermissionDenied("gateway: cannot upload to share folder or share name: path=" + p)
	log.Err(err).Msg("gateway: error downloading")
	return &gateway.InitiateFileDownloadResponse{
		Status: status.NewInvalidArg(ctx, "path points to share folder or share name"),
	}, nil

}

if s.isShareChild(ctx, p) {

isSharedFolder returns false that is ok but isShareName returns true.
But even if isShareName would return false, isShareChild would return false, preventing to download the file.

@C0rby C0rby changed the title implement share handling for accepting and listing shares implement share handling for accepting and listing file shares Jul 7, 2020
@labkode
Copy link
Member

labkode commented Jul 8, 2020

@C0rby I understand the problem now. You're trying to mount single file shares and the current checks enforce that only folders are allowed. Can you update your code to make file sharing optional rather than enabled by default? A config option will do.

@C0rby C0rby force-pushed the feature/accept-shares branch 2 times, most recently from 67cc424 to fd56867 Compare July 8, 2020 12:02
Signed-off-by: David Christofas <dchristofas@owncloud.com>
@C0rby C0rby changed the title implement share handling for accepting and listing file shares implement share handling for accepting and listing folder shares Jul 8, 2020
@C0rby
Copy link
Contributor Author

C0rby commented Jul 8, 2020

Ok got it. My PR works with folder shares now only.
I reverted the changes in storageprovider.go.

@C0rby
Copy link
Contributor Author

C0rby commented Jul 8, 2020

Accepting and listing the shares work with this PR. I opened a new issue for downloading the shared files. owncloud/ocis-reva#359 That is not implemented here.

@labkode labkode merged commit 91eed00 into cs3org:master Jul 9, 2020
@C0rby C0rby deleted the feature/accept-shares branch July 9, 2020 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

listing received shares does not work
2 participants