only share owner and sharer update or delete share

[karakayasemi]

Description

Access permission, should not enough to update or delete a share. Only share-owner and sharer can update or delete the share.

Related Issue

• Fixes owncloud/enterprise#3497

Motivation and Context

Fighting with bugs.

How Has This Been Tested?

Manually with the described scenario.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

[phil-davis]

https://drone.owncloud.com/owncloud/core/20452/43/12

--- Failed scenarios:
/drone/src/tests/acceptance/features/apiShareManagementBasic/deleteShare.feature:144
/drone/src/tests/acceptance/features/apiShareManagementBasic/deleteShare.feature:145
/drone/src/tests/acceptance/features/apiShareManagementBasic/deleteShare.feature:197
/drone/src/tests/acceptance/features/apiShareManagementBasic/deleteShare.feature:198
168 scenarios (164 passed, 4 failed)
2087 steps (2071 passed, 4 failed, 12 skipped) 

Scenario Outline: unshare from self # /drone/src/tests/acceptance/features/apiShareManagementBasic/deleteShare.feature:123
Given using OCS API version "<ocs_api_version>" # FeatureContext::usingOcsApiVersion()
And group "grp1" has been created # FeatureContext::groupHasBeenCreated()
And these users have been created with default attributes and without skeleton files: # FeatureContext::theseUsersHaveBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles()
| username |
| user0 |
| user1 |
And user "user2" has been created with default attributes and skeleton files # FeatureContext::userHasBeenCreatedWithDefaultAttributes()
And user "user1" has been added to group "grp1" # FeatureContext::userHasBeenAddedToGroup()
And user "user2" has been added to group "grp1" # FeatureContext::userHasBeenAddedToGroup()
And user "user2" has shared file "/PARENT/parent.txt" with group "grp1" # FeatureContext::userHasSharedFileWithGroupUsingTheSharingApi()
And user "user2" has stored etag of element "/PARENT" # WebDavPropertiesContext::userStoresEtagOfElement()
And user "user1" has stored etag of element "/" # WebDavPropertiesContext::userStoresEtagOfElement()
When user "user1" deletes the last share using the sharing API # FeatureContext::userDeletesLastShareUsingTheSharingApi()
Then the OCS status code should be "<ocs_status_code>" # OCSContext::theOCSStatusCodeShouldBe()
And the HTTP status code should be "200" # FeatureContext::theHTTPStatusCodeShouldBe()
And the etag of element "/" of user "user1" should have changed # WebDavPropertiesContext::etagOfElementOfUserShouldHaveChanged()
And the etag of element "/PARENT" of user "user2" should not have changed # WebDavPropertiesContext::etagOfElementOfUserShouldNotHaveChanged()
Examples:
| ocs_api_version | ocs_status_code |
| 1 | 100 |
OCS status code is not the expected value
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'100'
+'404'
| 2 | 200 |
OCS status code is not the expected value
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'200'
+'404'

That existing scenario is strange. It expected that the receiving user of a group share could delete the share, and then it tests various stuff about the etags...

Edit: I pushed a commit to fixup this test scenario so that it does what seems to have been intended. (It previously worked somewhat by luck, because of the bug that is fixed in this PR)

[jvillafanez]

Just a minor optional change

[jvillafanez]

$sessionUserId = $this->userSession->getUser()->getUID();
return ($share->getShareOwner() === $sessionUserId || $share->getSharedBy() === $sessionUserId)

Not sure if this is more readable.... up to you to decide what to do.

[codecov]

Codecov Report

Merging #36120 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #36120   +/-   ##
=======================================
  Coverage      54%      54%           
=======================================
  Files          63       63           
  Lines        7403     7403           
  Branches     1308     1308           
=======================================
  Hits         3998     3998           
  Misses       3019     3019           
  Partials      386      386

Flag	Coverage Δ
#javascript	54% <ø> (ø)	⬆️

---

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 4aa17f5...1f00e9c. Read the comment docs.

[codecov]

Codecov Report

Merging #36120 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #36120   +/-   ##
=======================================
  Coverage      54%      54%           
=======================================
  Files          63       63           
  Lines        7403     7403           
  Branches     1308     1308           
=======================================
  Hits         3998     3998           
  Misses       3019     3019           
  Partials      386      386

Flag	Coverage Δ
#javascript	54% <ø> (ø)	⬆️

---

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 98922f1...961b24a. Read the comment docs.

[phil-davis]

I pushed a commit to fixup the Given steps of the acceptance test scenario added in deleteShare.feature. It should pass now (passes locally for me).

[phil-davis]

I also added and extra acceptance test scenario for the case when an individual user receives a share of a file or folder, to check that the individual receiving user cannot delete the share.

[phil-davis]

Looks good when CI passes.

[phil-davis]

"random" yarn install error in a drone job https://drone.owncloud.com/owncloud/core/20463/29/7
I restarted the whole drone CI build

[phil-davis]

more stupid drone https://drone.owncloud.com/owncloud/core/20466/6/3

stan-php7.1: Error response from daemon: Get https://registry-1.docker.io/v2/owncloudci/php/manifests/7.1: Get https://auth.docker.io/token?scope=repository%3Aowncloudci%2Fphp%3Apull&service=registry.docker.io: net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)

I restarted again.

[phil-davis]

Rebased, let's see if drone stops being stupid.