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

Folder shared with Secure view can be downloaded. #9369

Open
Tracked by #8428
S-Panta opened this issue Jun 13, 2024 · 8 comments
Open
Tracked by #8428

Folder shared with Secure view can be downloaded. #9369

S-Panta opened this issue Jun 13, 2024 · 8 comments
Labels

Comments

@S-Panta
Copy link
Contributor

S-Panta commented Jun 13, 2024

Describe the bug

When a folder is shared with secure view role, it could be downloaded. However if the folder is empty, the file inside get downloaded but the content won't be available for preview.

Steps to reproduce

1.Make a new user Einstein
2. Create a new folder test
3. Admin shares test with Einstein with Secure view role.
4. Einstein download the folder.

curl -k  --location 'https://host.docker.internal:9200/archiver?id={folder-id}' -ueinstein:relativity --output -

Expected behavior

The folder download should not be possible.

Actual behavior

The folder could be downloaded.
Suppose there are files and folders inside the folder. They could be downloaded but the content couldn't be opened.

Setup

Please describe how you started the server and provide a list of relevant environment variables or configuration files.

OCIS_XXX= Infinite Scale 5.1.0-prealpha+1ef7292e21 Community 

Additional context

Add any other context about the problem here.

@saw-jan
Copy link
Member

saw-jan commented Jun 13, 2024

Similar but with project space: #9303

@phil-davis
Copy link
Contributor

Suppose there are files and folders inside the folder. They could be downloaded but the content couldn't be opened.

What is received? A "zip" of the folder? Or some other data format?

What do you mean by "the content couldn't be opened"?
I suppose that "something" is returned in response to the curl GET. Is the "something" just nonsense data? Or is there any information in the response that has metadata or content of the file(s) inside it?

@S-Panta
Copy link
Contributor Author

S-Panta commented Jun 13, 2024

@phil-davis
This is the binary response of folder when there is a text file upon sharing the parent folder via secure view role

test0000755000000000000000000000000014632541116010147 5ustar0000000000000000test/New file.txt0000644000000000000000000000001314632541114012410 0ustar0000000000000000error: permission denied: storage_id:"f76b0374-d9b4-4bf1-bee4-1b8990a50656" opaque_id:"96eeb8b3-6d43-436d-9406-85b2f07b3aa1" space_id:"bf1498ca-77ce-4ee6-b398-4c59201599da" % 

If this is the text file, the downloaded zip file upon extraction can be opened but the content in the file is read as error: perm

@phil-davis
Copy link
Contributor

the downloaded zip file upon extraction can be opened but the content in the file is read as error: perm

Interesting behavior. Anyway, the file content is not returned - that is "a good thing".

For Secure View, the resource name (folder/file path) is OK to be seen by the user. For example, in a UI the user should be able to list files in a folder that is shared with "Secure View" permissions, and select which file they want to view.

So I don't think that there is any data leak here.

But someone should give an opinion about if this response should be "tidied up".

  • if the whole folder has only Secure View permissions, then it would be nicer to just return a "no permissions" HTTP status, and don't deliver any zip file payload. (Having the "error: perm" embedded in the zip file is a bit tricky)
  • if only some files in the folder have Secure View permission and others have ordinary read permission, then maybe the zip file could completely exclude the files that do not have read permission. Having embedded "error:perm" in a zip file might be a bit painful for the receiver?

@S-Panta
Copy link
Contributor Author

S-Panta commented Jun 14, 2024

I've manually tested this with the latest build today. These are my findings.
In UI, the download option won't be available if shared via a secure role. However, it could be downloaded via API that gives weird binary responses above #9369 (comment) .
But if the folder has a view or other role but the file inside it has secure view, somehow the parent role shows dominance over the child role. The folder could be downloaded and the file be viewed.

@micbar
Copy link
Contributor

micbar commented Jun 14, 2024

We should return 403 on that api call if the folder is shared via secure view.

@phil-davis
Copy link
Contributor

But if the folder has a view or other role but the file inside it has secure view, somehow the parent role shows dominance over the child role. The folder could be downloaded and the file be viewed.

That case needs a decision about the requirements. Normally, so far, permissions have been cumulative

  • Alice shares folder with GroupA with Editor role
  • Alice shares folder/file.doc with Brian with just Read permission
  • Brian is a member of GroupA
    Brian gets full Editor access to folder/file.doc
    If Brian is removed from GroupA, then Brian will only have read access to folder/file.doc

Is the requirement the same for Secure View?
i.e. cumulative permissions

The consequence of cumulative permissions is that sharers can sometimes not realize what total permissions someone has, and they get surprised that they explicitly share to Brian with Secure View, but that Brian can still download the document because he gets Read privs via some other permission grant.

@kobergj
Copy link
Collaborator

kobergj commented Jun 24, 2024

But if the folder has a view or other role but the file inside it has secure view, somehow the parent role shows dominance over the child role. The folder could be downloaded and the file be viewed.

All permissions in ocis are cumulative except for the denial permission which is in technical preview. So this is expected behaviour, the highest permission wins.

if only some files in the folder have Secure View permission and others have ordinary read permission, then maybe the zip file could completely exclude the files that do not have read permission

This is a much more interesting case. We need to clarify: Should you be able to download a folder that is shared via secure view? It makes sense if you have read access to the children.

Having embedded "error:perm" in a zip file might be a bit painful for the receiver?

This is definitely a bug. Files should not be added to the archive when there is some sort of error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants