Added tests about permissions and public upload #26639

SergioBertolinSG · 2016-11-15T17:39:10Z

Test for owncloud/QA#301
And a current failing case. cc @PVince81

mention-bot · 2016-11-15T17:39:11Z

@SergioBertolinSG, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @PVince81 and @MorrisJobke to be potential reviewers.

PVince81 · 2016-11-21T17:40:01Z

build/integration/features/sharing-v1.feature

+    And group "new-group" exists
+    And user "user0" belongs to group "new-group"
+    And user "user1" belongs to group "new-group"
+    And Assure user "user0" is subadmin of group "new-group"


subadmin really needed ?

Perhaps not, it comes from here #25542 (comment)

PVince81 · 2016-11-21T17:44:05Z

build/integration/features/sharing-v1.feature

+      | shareType | 3 |
+      | publicUpload | false |
+    When Updating last share with
+      | publicUpload | true |


This looks legitimate. At line 1002 the share is read-write so it's acceptable that the recipient can also make the link share read-write.

Did you mean to make the local share read-only ?

Yes, you're right. I'll change it.

PVince81 · 2016-11-22T10:35:22Z

Pfff, this API thing is a real mess. Some checks are done in the share manager, others are done only in the OCS controller...

PVince81 · 2016-11-22T10:38:11Z

Basically the createShare method in the OCS controller doesn't check if permissions can be increased, but a check is done in the sharemanager.
However for updateShare it is done in the OCS controller but not in the share manager.

@SergioBertolinSG can we also have integration tests for creating link shares as reshares for a read-write and read-only received share ?

PVince81 · 2016-11-22T10:41:56Z

Debugging in the updateShare case, apparently the share manager calls $share->getNode()->getPermissions() which returns 31. These are the permissiosn from the POV of the owner, not the recipient.

But weird thing that this check is correct when creating shares...

PVince81 · 2016-11-22T10:51:06Z

Ok, got it. The problem is as follows: the Node that was set to the Share object to be modified is different depending if create or update:

for createShare: the Node is set to "/user1/files/test", the received folder
for updateShare: since the owner is "user0", the node sets itself to "/user0/files/test". And because of this, the permissions returned for the share's node is different, the ones from the owner's POV not the recipient. However when creating shares, we always need the permissions from the POV of the recipient.

So need to look into whether we need to "fix" the node in the share manager, or adjust the node in the OCS controller code. It looks like the logic would let us discard the permissions validation logic from the OCS controller and only rely on the one in the manager, which would be ideal.

PVince81 · 2016-11-22T10:58:11Z

Looks like for the share manager we need to somehow try and get the node with the POV of the user who is specified in "getSharedBy()" instead of relying on the node that was specified in the share itself as it might be inaccurate.

PVince81 · 2016-11-22T11:09:27Z

Some POC here: fe38088

Would be good to add many more integration tests to make sure everything is covered (including reshare-creation perm denied)
Will also add unit tests for the share manager change.

SergioBertolinSG · 2016-11-22T12:02:30Z

@SergioBertolinSG can we also have integration tests for creating link shares as reshares for a read-write and read-only received share ?

You mean without permission to share?

PVince81 · 2016-11-22T12:08:52Z

You mean without permission to share?

I didn't mean that. But you're right that it needs to be added as well.

PVince81 · 2016-11-22T12:34:14Z

For now I'll focus on the quick fix. The clean but more risky fix will be done for 9.2 separately: #26684 (more risky because it will affect more cases that we need to make sure we cover as well)

PVince81 · 2016-11-22T12:41:52Z

build/integration/features/sharing-v1.feature

+      | shareType | 3 |
+      | publicUpload | false |
+    When Updating last share with
+      | publicUpload | true |


Can we also have the same tests with the "permissions" attribute ? @SergioBertolinSG

SergioBertolinSG · 2016-11-22T15:22:55Z

@SergioBertolinSG can we also have integration tests for creating link shares as reshares for a read-write and read-only received share ?

With sharing permission, these cases are covered in
'Scenario: Adding public upload to a shared folder as recipient is allowed with permissions' and 'Scenario: Adding public upload to a read only shared folder as recipient is not allowed'
aren't they?

Without sharing permissions there is 'Scenario: User is not allowed to reshare file' and 'Scenario: User is not allowed to reshare file with more permissions'. They reshare like regular shares, Do we need different tests using public links shares?

PVince81 · 2016-11-22T15:30:24Z

Do we need different tests using public links shares?

Yes, that would be good as it's a slightly different code path.

…e enough permisssions

PVince81 · 2016-11-22T17:32:06Z

Seems Jenkins env is broken, I resubmit the PR with your commits squashed here: #26691

lock · 2019-08-04T08:02:15Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

SergioBertolinSG added the dev:integration-tests label Nov 15, 2016

SergioBertolinSG added this to the 9.2 milestone Nov 15, 2016

PVince81 added backport-request Type:Bug sev2-high labels Nov 18, 2016

PVince81 self-assigned this Nov 18, 2016

PVince81 reviewed Nov 21, 2016

View reviewed changes

PVince81 added sev1-critical and removed sev2-high labels Nov 22, 2016

PVince81 mentioned this pull request Nov 22, 2016

Make share permission check work correctly from share manager #26684

Closed

11 tasks

SergioBertolinSG and others added 4 commits November 22, 2016 13:38

Added tests about permissions and public upload

60451fa

Added read and share permissions instead of all (default ones)

e05c004

Added positive case

dad998c

Correctly convert OCS permissions params

2ba0f9a

PVince81 force-pushed the integration-tests-modifying-permissions branch from fe38088 to 2ba0f9a Compare November 22, 2016 12:38

PVince81 reviewed Nov 22, 2016

View reviewed changes

Check also permissions update

1b3984c

Added tests for resharing with public links when the user doesn't hav…

3358c2b

…e enough permisssions

PVince81 mentioned this pull request Nov 22, 2016

Properly convert OCS params + more tests #26691

Merged

PVince81 closed this Nov 22, 2016

PVince81 deleted the integration-tests-modifying-permissions branch November 22, 2016 17:32

PVince81 removed the backport-request label Dec 7, 2016

lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019

Added tests about permissions and public upload #26639

Added tests about permissions and public upload #26639

Uh oh!

Conversation

SergioBertolinSG commented Nov 15, 2016

Uh oh!

mention-bot commented Nov 15, 2016

Uh oh!

PVince81 Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

SergioBertolinSG Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

SergioBertolinSG Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

SergioBertolinSG commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

PVince81 Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

SergioBertolinSG Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

SergioBertolinSG commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

lock bot commented Aug 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SergioBertolinSG commented Nov 22, 2016 •

edited

Loading