Change wording of Role Descriptions

kulmann commented 8 months ago

Describe the bug

OnlyOffice can write documents from a public link share with "Can upload" role.

Steps to reproduce

As "admin", create asdf/test.docx via OnlyOffice
As "admin", create a public link to the folder asdf with Can upload role (which states permissions view, download and upload)
As anonymous, receive and follow the public link
As anonymous, open asdf/test.docx via OnlyOffice
As anonymous, write some content into the file and leave the document again
As "admin", see that the changes have been written to disk (e.g. by downloading the file after the lock has been released)

Expected behavior

"anonymous" should have a read only view or at least an error message when trying to save.

Actual behavior

"anonymous" can edit and save the document.

kulmann commented 8 months ago

confirmed in both v5.0.0-rc.5 and v4.0.6

2403905 commented 8 months ago

Step 2. Is admin create a public link to this file with the Can upload role using API or UI ? The Can upload role has to be used for folders, bu not for files.

kulmann commented 8 months ago

Step 2. Is admin create a public link to this file with the Can upload role using API or UI ?

The Can upload role has to be used for folders, bu not for files.

Updated the steps to reproduce. Thank you. I tested it with a folder and thought it can be just done the same way with a file, sorry. Everything done via UI, nothing directly via API.

phil-davis commented 8 months ago

To clarify, the "upload" permission should allow "anonymous" to create new files in the folder, but not overwrite (=edit) existing files.

After "anonymous" has uploaded a file, they should not be able to change what they uploaded (can't edit it or delete it), but should be able to list the files and download files.

Correct?

individual-it commented 8 months ago

What API is OnlyOffice using for that? I'm surprised the API tests did not catch it

micbar commented 8 months ago

Wopi

kulmann commented 8 months ago

To clarify, the "upload" permission should allow "anonymous" to create new files in the folder, but not overwrite (=edit) existing files.

After "anonymous" has uploaded a file, they should not be able to change what they uploaded (can't edit it or delete it), but should be able to list the files and download files.

Correct?

Correct, thank you!

ScharfViktor commented 8 months ago

We have e2e coverage for an app provider. It is not very large, because we wanted to implement only primitive tests as a first step (create and open a docx or odt document). Maybe we should increase the coverage with e2e?

individual-it commented 8 months ago

Yes I think we should. I believe the decision to make only small coverage was because the security related code path should be the same as with webDAV, but that does not look like its true

2403905 commented 8 months ago

@kulmann I found the place in a code when we give the write permissions to OnlyOffice but it is not a root issue. Because the permissions statements (view, download and upload) do not contradict that. If a user can upload then the user can change the file, right?

2403905 commented 8 months ago

@micbar @kulmann Is the upload permission redundant in this case?

kulmann commented 8 months ago

If a user can upload then the user can change the file, right?

No... @phil-davis interpretation is what we want: The user can upload files, but not overwrite existing files.

2403905 commented 8 months ago

We should clarify the public permission definitions. FYI @tbsbdr @micbar

micbar commented 8 months ago

If a user can upload then the user can change the file, right?

No... @phil-davis interpretation is what we want: The user can upload files, but not overwrite existing files.

Hm, from a feature POV i tend to agree.

In the CS3 API there is no distinction, upload means also overwrite.

IMHO not solvable in this API version.

individual-it commented 7 months ago

How does the WebDAV API currently react in that case?

tbsbdr commented 7 months ago

Currently I see 3 options

fix the bug
Add "edit" to the role description
remove the whole role "Uploader"

I'd favour the 3rd option for now, as Uploader and Editor would then be almost the same. what do you think @kulmann @micbar ?

kulmann commented 7 months ago

Option 3 would be fine by me. UX-wise the differentiation is hard to understand anyway. So might actually be an improvement...

micbar commented 7 months ago

yes. Option 3 is the best.

tbsbdr commented 7 months ago

ok, so lets remove this role from the link 😬

micbar commented 7 months ago

Hm, that would need a migration 😱

2403905 commented 7 months ago

Is the "can upload" should be changed to "can edit" or "can view"?

micbar commented 7 months ago

Yes. We need a better description which matches our behavior

micbar commented 6 months ago

@tbsbdr @kulmann Let us fix the description.

tbsbdr commented 5 months ago

@tbsbdr propose a text

tbsbdr commented 5 months ago

🙈 adding "edit" to the description won't work, as text or md files can not be edited. @micbar @kulmann I currently see 2 options and would choose the one with least effort. do you agree?

fix the bug
~Add "edit" to the role description~
remove the whole role "Uploader"

micbar commented 5 months ago

@tbsbdr Not quite how i understand it.

The main difference is the "delete" permission. Edit is also true for txt and md files.

tbsbdr commented 5 months ago

I could Not Edit txt, can you?

micbar commented 5 months ago

you can upload a new txt file version.

micbar commented 5 months ago

The main problem here is the "edit" vs. "upload" semantics. From the ocis backend perspective, both are uploads and require the upload permission. This is different for directories. They are controlled by the create-directories permission.

phil-davis commented 5 months ago

Note: "edit-save" and "upload-overwrite" really mean the same thing. There is no such thing as "editing" a file that is sitting on a server. Actually, there is also no such thing as "editing" a file on the local filesystem on my laptop. "editing" is a thing that is done in-memory with a user-interface (graphical (LibreOffice...) or textual (VI, nano, a line-based command-line editor...)

When the person doing the "editing" asks to "save", then the editor software does an "upload-overwrite" to the file, wherever the persistent storage is. It might rewrite the whole file, it might append to the end (if the change just adds to the file), it might overwrite just some bytes of the file (if it implements some optimized way of saving just the bits that changed).

JustKiddingCode commented 5 months ago

From a linux file system perspective, which is similar to webdav: "upload" rights should mean "write permissions to directory, but not the files". Uploading a file should check if the file exists and the user has write permissions to that file.

mkdir test
echo "bar" > test/foo # works, because of write permissions to test
chmod -w > test/foo
echo "foo" > test/foo # fails, permission denied

tbsbdr commented 4 months ago

@tbsbdr adjust wording for the role

tbsbdr commented 3 months ago

Suggestion for the rewording of the roles description.

Assuming that "Only for invited ppl" will be removed
Adds edit to Can upload
Adds delete to Can edit
I'd prefer to be less verbose in the role description (option 2)
Note: In general I'd prefer to remove the Uploader role at all, as it adds little value for the user, but if I understand @micbar correctly this would be expensive due to a migration necessity.

@kulmann is option 2 - less verbose, fine for you?

1) Verbose	2) Less verbose

General overview

	Needs authentication	delete	upload/edit	view	download	upload (existing content not revealed)
Invited People	✅	(✅)	(✅)	(✅)	(✅)	❌
Can edit	❌	✅	✅	✅	✅	❌
Can upload	❌	❌	✅	✅	✅	❌
Can view	❌	❌	❌	✅	✅	❌
Secret File Drop	❌	❌	❌	❌	❌	✅

tbsbdr commented 3 months ago

@kulmann whats your opinion - go or no? 👆

kulmann commented 3 months ago

We had that in the past and changed it to being more verbose because people had difficulties understanding the implications. Now that the whole context is more verbose we may as well go back to less verbose within the dropdown. Fine by me.

rhafer commented 2 months ago

@kulmann So I guess this would be a web ticket now? On the server side we don't have any descriptive texts for the public link types AFAICS.

kulmann commented 2 months ago

@kulmann So I guess this would be a web ticket now? On the server side we don't have any descriptive texts for the public link types AFAICS.

Seems like it, yes. Transferred it to web now.

owncloud / web