only share owner and sharer update or delete share #36120

karakayasemi · 2019-08-29T19:19:17Z

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 · 2019-08-30T07:19:46Z

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 · 2019-08-30T07:22:16Z

apps/files_sharing/lib/Controller/Share20OcsController.php

+	 */
+	protected function canChangeShare(IShare $share) {
+		// Only owner or the sharer of the file can update or delete share
+		if ($share->getShareOwner() === $this->userSession->getUser()->getUID() ||


$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.

tests/acceptance/features/apiShareManagementBasic/deleteShare.feature

codecov · 2019-08-30T08:31:48Z

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 · 2019-08-30T08:31:49Z

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 · 2019-08-30T08:32:59Z

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 · 2019-08-30T09:22:24Z

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 · 2019-08-30T10:30:16Z

"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 · 2019-08-30T11:01:04Z

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 · 2019-08-30T11:52:58Z

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

karakayasemi added this to the development milestone Aug 29, 2019

karakayasemi requested review from PVince81, phil-davis and micbar August 29, 2019 19:19

karakayasemi self-assigned this Aug 29, 2019

PVince81 requested a review from jvillafanez August 29, 2019 20:47

jvillafanez approved these changes Aug 30, 2019

View reviewed changes

phil-davis reviewed Aug 30, 2019

View reviewed changes

tests/acceptance/features/apiShareManagementBasic/deleteShare.feature Show resolved Hide resolved

phil-davis force-pushed the canChangeShare branch from 1f00e9c to e4fb548 Compare August 30, 2019 09:18

phil-davis approved these changes Aug 30, 2019

View reviewed changes

karakayasemi and others added 3 commits August 30, 2019 17:37

only share owner and sharer update or delete share

df88456

Adjust 'unshare from self' scenario so it uses 'unshare'

aaf1e64

Adjust an enhance 'delete share' tests

961b24a

phil-davis force-pushed the canChangeShare branch from e4fb548 to 961b24a Compare August 30, 2019 11:52

phil-davis added 4 - To release release:10.3.0 labels Aug 30, 2019

phil-davis merged commit 89cdee5 into master Aug 30, 2019

delete-merged-branch bot deleted the canChangeShare branch August 30, 2019 13:16

This was referenced Aug 31, 2019

Adjust unshare acceptance test owncloud/customgroups#244

Merged

Remove 'delete a share as sharee' tests owncloud/activity#773

Merged

davitol mentioned this pull request Sep 3, 2019

10.3 - Changelog test Results #36142

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

only share owner and sharer update or delete share #36120

only share owner and sharer update or delete share #36120

karakayasemi commented Aug 29, 2019

phil-davis commented Aug 30, 2019 •

edited

Loading

jvillafanez left a comment

jvillafanez Aug 30, 2019

codecov bot commented Aug 30, 2019

codecov bot commented Aug 30, 2019 •

edited

Loading

phil-davis commented Aug 30, 2019

phil-davis commented Aug 30, 2019

phil-davis left a comment

phil-davis commented Aug 30, 2019

phil-davis commented Aug 30, 2019 •

edited

Loading

phil-davis commented Aug 30, 2019

only share owner and sharer update or delete share #36120

only share owner and sharer update or delete share #36120

Conversation

karakayasemi commented Aug 29, 2019

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

phil-davis commented Aug 30, 2019 • edited Loading

jvillafanez left a comment

Choose a reason for hiding this comment

jvillafanez Aug 30, 2019

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2019

Codecov Report

codecov bot commented Aug 30, 2019 • edited Loading

Codecov Report

phil-davis commented Aug 30, 2019

phil-davis commented Aug 30, 2019

phil-davis left a comment

Choose a reason for hiding this comment

phil-davis commented Aug 30, 2019

phil-davis commented Aug 30, 2019 • edited Loading

phil-davis commented Aug 30, 2019

phil-davis commented Aug 30, 2019 •

edited

Loading

codecov bot commented Aug 30, 2019 •

edited

Loading

phil-davis commented Aug 30, 2019 •

edited

Loading