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

Add the requiredPermissions property for extra share attributes #35001

Closed
wants to merge 1 commit into from
Closed

Add the requiredPermissions property for extra share attributes #35001

wants to merge 1 commit into from

Conversation

LinneyS
Copy link
Contributor

@LinneyS LinneyS commented Apr 10, 2019

Related Issue: #33994 (comment)

When adding extra share attributes, the ability to specify not only incompatible permissions (incompatibleAttributes), but also mandatory permissions is required (requiredPermissions).

For example, a new option is available only if there are rights to edit and is hidden if there are no rights to edit.

With these changes in "shareitemmodel.js" I was able to implement the new opening modes of the ONLYOFFICE editor.
These are limited editing modes when the user has the right to save the file, but limited features are available in the editor (only reviewing, only filling forms, or only commenting).

ONLYOFFICE code:
https://github.com/ONLYOFFICE/onlyoffice-owncloud/blob/71db430e1e5b28b81cf0ef55819b7c3acca53c51/js/main.js#L281

My Video Gif(1)

@mrow4a @DeepDiver1975 @pmaier1 @ibnpetr

@CLAassistant
Copy link

CLAassistant commented Apr 10, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 10, 2019

Codecov Report

Merging #35001 into stable10 will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff               @@
##             stable10   #35001      +/-   ##
==============================================
- Coverage       64.29%   64.29%   -0.01%     
  Complexity      20040    20040              
==============================================
  Files            1285     1285              
  Lines           76824    76827       +3     
  Branches         1307     1308       +1     
==============================================
  Hits            49394    49394              
- Misses          27049    27052       +3     
  Partials          381      381
Flag Coverage Δ Complexity Δ
#javascript 52.99% <0%> (-0.03%) 0 <0> (ø)
#phpunit 65.49% <ø> (ø) 20040 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
core/js/shareitemmodel.js 67.15% <0%> (-0.61%) 0 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 443bf44...0f1b865. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Apr 10, 2019

Codecov Report

Merging #35001 into stable10 will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff               @@
##             stable10   #35001      +/-   ##
==============================================
- Coverage       64.29%   64.29%   -0.01%     
  Complexity      20040    20040              
==============================================
  Files            1285     1285              
  Lines           76824    76827       +3     
  Branches         1307     1308       +1     
==============================================
  Hits            49394    49394              
- Misses          27049    27052       +3     
  Partials          381      381
Flag Coverage Δ Complexity Δ
#javascript 52.99% <0%> (-0.03%) 0 <0> (ø)
#phpunit 65.49% <ø> (ø) 20040 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
core/js/shareitemmodel.js 67.15% <0%> (-0.61%) 0 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 443bf44...0f1b865. Read the comment docs.

Copy link
Contributor

@mrow4a mrow4a left a comment

Choose a reason for hiding this comment

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

  • "can download" should not be compatible with edit permission. Edit permission has to be unchecked to be able to disable can download.
  • please change to "can review", "can fill forms", "can comment"

When you adjust to above, it might happen we won't need this PR.

P.S. In some cases, you might impose to some user by default "edit being unchecked" etc via app admin settings

LinneyS added a commit to ONLYOFFICE/onlyoffice-owncloud that referenced this pull request Apr 10, 2019
@LinneyS
Copy link
Contributor Author

LinneyS commented Apr 10, 2019

With the boot attribute, I understood the ownCloud logic. Although in the framework of the ONLYOFFICE editor, you can prohibit the download in edit mode, I made corrections.
But this attribute is not associated with this PR.

@LinneyS
Copy link
Contributor Author

LinneyS commented Apr 10, 2019

Our goal is to add the ability to provide file permissions to the following hierarchy: Full access, Reviewing, Filling forms, Commenting. By default, rights are granted to Full Access, which allows you to perform all other actions: review, fill in the forms and comment.
The rest of the rights are restricted editing mode. But this is still editing, in which the file will have to be saved from the editor to the storage
And if there are no rights to change the file, you cannot provide the file for review, filling forms or commenting, because it will not be saved and will throw an exception.

I still need to specify required permissions to implement our logic.

@mrow4a
Copy link
Contributor

mrow4a commented Apr 10, 2019

@LinneyS Ok, I think I better understand https://github.com/ONLYOFFICE/onlyoffice-owncloud/compare/feature/extra-permissions.

So your goal is to disallow user to edit the original document, is that correct? It needs to be also restricted from sync and mobile client? In this case we would need to add restriction on PUT to be able to do that as DAV plugin, this would be core permission.

Is the comments, review and forms part of file content? Can OnlyOffice write directly to storage, or it requires DAV? I will check your code

Is it acceptable, that we disallow user to EDIT in editor, but allow him to have any combination of review/fill-forms/comment? User in this case will not be allowed to write file with e.g. sync client, and only "secure editor" will be OnlyOffice which will have permission to write to file.

Just to clarify - share with edit permission:

  • "can change document"
  • "can review" (can be only be set when "can change document" is false)
  • "can fill forms" (can be only be set when "can change document" is false)
  • "can comment" (can be only be set when "can change document" is false)

is that correct? Could user be able to review, comment but not fill forms and no edit document? or any other combination? Or can have only 1 out of 4?

@LinneyS
Copy link
Contributor Author

LinneyS commented Apr 11, 2019

So your goal is to disallow user to edit the original document, is that correct?

no

This is the edit mode.
As download mode: with or without watermark.
So it is here: with "full rights" or "only commenting" through the ONLYOFFICE editors.

Only "download modes" for some reason, you give it set up on the condition of "no editing rights" (although this is not clear to me - it is more logical under the condition of "read permissions", including editing). So, on the contrary, our editing modes are set up under the condition of "Availability of editing rights."

Do you consider the requiredPermissions property not useful? Or do not see the need for it for the ONLYOFFICE connector?

@mrow4a
Copy link
Contributor

mrow4a commented Apr 11, 2019

@LinneyS I think this change is useful, but I try to understand whether this is the only change to be done so that your "restrictions would work and that cannot be circumvented by e.g. sync client".

I still don't understand, maybe I need to try using the OnlyOffice editor for it.

There are two things:

  • edit rights from user point of view (permissions e.g. can change, can review, can comment etc)
  • edit permission on internal OC storage layer from onlyoffice perspective.

Edit rights for users are not clear for me. Can user upload a file from sync client with "review only mode" ? Can user change document in ONLYOFFICE with "review only" ? Can I both comment and review? I don't understand the use case.

And once again, permission cannot be called "only commenting", checked checkbox grants some functionality, unchecked means there is no functionality allowed for the share.

@ibnpetr
Copy link

ibnpetr commented Apr 12, 2019

@mrow4a Let me summarize the major points:
The main goal is to add few additional "editing permissions" with some limited editing features.
Please note that all these modes still remain editing modes.
In general we're just trying to avoid the situation when a user selects one of the additional modes and disables the basic "can edit" mode. (Shown as a screenshot) In that case ONLYOFFICE isn't allowed to save the file to the OC storage.

maybe I need to try using the OnlyOffice editor for it

Sure, we are ready to send you an invite and show you how it works on our test OC instance

@Galegod
Copy link

Galegod commented Apr 12, 2019

@mrow4a let me try to explain the situation with the changes needed for us to implement the new editing level rights to ownCloud.
The rights needed to download and upload the files are not related to the option here. For ONLYOFFICE there are two general modes only: "can edit" or "can not edit" (viewer).
If we choose "can not edit" - the user does not open an editor - just a viewer. And here nothing needs to be changed for now.
We can restrict the editing mode to some specific rights like

  • "review only" (the user still uses an editor, he can edit, but only add the changes will be recorded and shown along the source text underlined as suggestions in another colour) or

  • "commenting only" (the user still uses the editor, he can edit, but only add some comments without disturbing the source text) or

  • "filling forms only" (again, this is an editing option, the user an edit, but only within special forms left for this specific user available, the rest is blocked for editing with this type of rights).

All the options relate to limitations of the editing rights. As you said we need to "disallow" a user with "can edit" right to have access to specific parts of a document or levels of editing freedom in ONLYOFFICE.

So it is not possible to give a user with no access to the editing just "can comment" as he even won't open the editor, only a viewer. The same for all the other options. This is why the option should have the word "only" in the name.

Can we connect to show you what we need and how it should function? -

Probably we can do this beyond this issue as the original request is just to make the "can edit" a "requiredPermission" for any of the options below: "can review only", "can comment only" and "filling forms only")

@mrow4a
Copy link
Contributor

mrow4a commented Apr 12, 2019

@Galegod ok, I think finally I understand. Let me test this PR on my own, to check if in some other part of the code something does not need to be added. Will take care next days. Assume that requiredPermission is available in your app :)

@mrow4a
Copy link
Contributor

mrow4a commented Apr 13, 2019

@Galegod @LinneyS sorry once again for confusing. I just checked, and we cannot support attributes and permissions which dont grant permissions. So "comment only" is restricting, not adding functionality.

What I mean, your attribute "has to add functionality", not restrict it. This is due to supershare logic we use internally in the backend, and generally due to internal permission logic.

checked checkbox -> add functionality
unchecked checkbox -> remove functionality

Functionality is in a sense of "I can write, I cannot write".

Is it possible to somehow adjust to this workflow?

CC @PVince81 @DeepDiver1975

@mrow4a
Copy link
Contributor

mrow4a commented Apr 13, 2019

First we need to push the PR to master - I cherry-picked your commit to #35016

@mrow4a
Copy link
Contributor

mrow4a commented Apr 13, 2019

Please find the suggestion there - ONLYOFFICE/onlyoffice-owncloud#255. Note also, I am NOT using requiredPermissions, as I dont need it following owncloud permission logic.

@DeepDiver1975
Copy link
Member

@LinneyS @mrow4a I assume this is no longer needed? THX

@LinneyS
Copy link
Contributor Author

LinneyS commented Jul 30, 2019

Released
Thanks you

@LinneyS LinneyS closed this Jul 30, 2019
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.

6 participants