The recipient of internal share should not increase permission by sharidas · Pull Request #35447 · owncloud/core

sharidas · 2019-06-06T10:48:08Z

The recipient of internal share, should not be able
to increase the permission.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

When an internal/regular share is created by say user1 to user2, the user2 should not be allowed to increase the permissions granted by user1. The UI prevents it. But by using ocs api, its possible to increase the permision. This pr addresses the issue.

Related Issue

Fixes

Motivation and Context

The user who is not the owner of the share shouldn't be allowed to increase the permission of the share.

How Has This Been Tested?

Create 3 users user1, user2 and user3
As user1 create folder testFolder/subFolder
As user1 share testFolder to user2 with permission can share and uncheck can edit ( i.e, readonly )
Now as user2 try to reshare testFolder/subFolder to user3
Execute the command

 sujith@sujith-ownCloud  ~  curl -u user2:1 'http://localhost/testing/ocs/v2.php/apps/files_sharing/api/v1/shares/share_id' --request PUT --data 'permissions=21'                           
<?xml version="1.0"?>
<ocs>
 <meta>
  <status>failure</status>
  <statuscode>404</statuscode>
  <message>Cannot change permission of /subFolder</message>
  <totalitems></totalitems>
  <itemsperpage></itemsperpage>
 </meta>
 <data/>
</ocs>
 sujith@sujith-ownCloud  ~  curl -u user2:1 'http://localhost/testing/ocs/v2.php/apps/files_sharing/api/v1/shares/share_id' --request PUT --data 'permissions=31'                           
<?xml version="1.0"?>
<ocs>
 <meta>
  <status>failure</status>
  <statuscode>404</statuscode>
  <message>Cannot change permission of /subFolder</message>
  <totalitems></totalitems>
  <itemsperpage></itemsperpage>
 </meta>
 <data/>
</ocs>
 sujith@sujith-ownCloud  ~ 

Similarly when a file is shared with can share and readonly permission by user1 to user2, and user2 reshares to user3, when the command is executed for the share of user2:

sujith@sujith-ownCloud  ~  curl -u user2:1 'http://localhost/testing/ocs/v2.php/apps/files_sharing/api/v1/shares/5' --request PUT --data 'permissions=21'
<?xml version="1.0"?>
<ocs>
 <meta>
  <status>failure</status>
  <statuscode>404</statuscode>
  <message>Cannot change permission of /a.txt</message>
  <totalitems></totalitems>
  <itemsperpage></itemsperpage>
 </meta>
 <data/>
</ocs>
 sujith@sujith-ownCloud  ~  curl -u user2:1 'http://localhost/testing/ocs/v2.php/apps/files_sharing/api/v1/shares/5' --request PUT --data 'permissions=31'                           
<?xml version="1.0"?>
<ocs>
 <meta>
  <status>failure</status>
  <statuscode>404</statuscode>
  <message>Cannot change permission of /a.txt</message>
  <totalitems></totalitems>
  <itemsperpage></itemsperpage>
 </meta>
 <data/>
</ocs>
 sujith@sujith-ownCloud  ~ 

Screenshots (if appropriate):

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:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

codecov · 2019-06-06T14:45:05Z

Codecov Report

Merging #35447 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #35447      +/-   ##
============================================
+ Coverage     65.66%   65.66%   +<.01%     
- Complexity    18666    18670       +4     
============================================
  Files          1221     1221              
  Lines         70597    70602       +5     
  Branches       1288     1288              
============================================
+ Hits          46358    46363       +5     
  Misses        23862    23862              
  Partials        377      377

Flag	Coverage Δ	Complexity Δ
#javascript	`53.69% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`67.04% <100%> (ø)`	`18670 <0> (+4)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...es_sharing/lib/Controller/Share20OcsController.php	`87.38% <100%> (+0.11%)`	`208 <0> (+4)`	⬆️

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 1b9285c...ea751b9. Read the comment docs.

codecov · 2019-06-06T14:45:19Z

Codecov Report

Merging #35447 into master will increase coverage by <.01%.
The diff coverage is 92.85%.

@@             Coverage Diff              @@
##             master   #35447      +/-   ##
============================================
+ Coverage     65.68%   65.68%   +<.01%     
- Complexity    18729    18735       +6     
============================================
  Files          1221     1221              
  Lines         70779    70793      +14     
  Branches       1288     1288              
============================================
+ Hits          46490    46503      +13     
- Misses        23912    23913       +1     
  Partials        377      377

Flag	Coverage Δ	Complexity Δ
#javascript	`53.69% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`67.06% <92.85%> (ø)`	`18735 <4> (+6)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...es_sharing/lib/Controller/Share20OcsController.php	`87.36% <92.85%> (+0.13%)`	`208 <4> (+6)`	⬆️

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 6583ab3...663e99a. Read the comment docs.

jvillafanez · 2019-06-07T10:07:18Z

 				$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
 				return new Result(null, 400, $this->l->t('Wrong or no update parameter given'));
 			} else {
+				if ($share->getShareType() === Share::SHARE_TYPE_USER) {


Shouldn't this be applied to groups as well? I think what you're trying to do here is to prevent the increase of permissions by anyone that isn't the owner of the share, but this should be applied to any kind of share I think.
Btw, the error message should be changed because anyone can reduce the permissions

jvillafanez · 2019-06-07T10:50:51Z


-				if ($share->getPermissions() & ~$maxPermissions) {
+				if (($share->getPermissions() & ~$maxPermissions) &&
+					//A condition where the user belongs to the same group which requests this update should be ignored


I think that noone other than the owner should be able to increase the permissions regardless of the share, so the additional condition should be removed (?)

@jvillafanez 👍

jvillafanez

maybe the canIncreasePermission might return null if we can't get the user from the session in order to distinguish cases, but since it's a private function it doesn't really matters.

The recipient of internal share, should not be able to increase the permission. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas · 2019-06-11T06:53:35Z

Backport PR: #35475

sharidas added the 2 - Developing label Jun 6, 2019

sharidas added this to the development milestone Jun 6, 2019

sharidas self-assigned this Jun 6, 2019

sharidas force-pushed the share-permission-fix branch from 53468b6 to ea751b9 Compare June 6, 2019 14:44

sharidas force-pushed the share-permission-fix branch from ea751b9 to aee99b6 Compare June 7, 2019 05:38

sharidas requested review from VicDeo and jvillafanez June 7, 2019 07:13

sharidas added 3 - To Review and removed 2 - Developing labels Jun 7, 2019

jvillafanez reviewed Jun 7, 2019

View reviewed changes

karakayasemi mentioned this pull request Jun 8, 2019

[WIP] let share manager handles permission increases in sharing #35399

Closed

11 tasks

sharidas force-pushed the share-permission-fix branch from aee99b6 to b9f1d41 Compare June 10, 2019 09:37

jvillafanez approved these changes Jun 10, 2019

View reviewed changes

sharidas changed the title ~~[WIP] The recipient of internal share should not increase permission~~ The recipient of internal share should not increase permission Jun 10, 2019

The recipient of internal share should not increase permission

663e99a

The recipient of internal share, should not be able to increase the permission. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the share-permission-fix branch from b9f1d41 to 663e99a Compare June 10, 2019 12:12

sharidas merged commit dc155a6 into master Jun 10, 2019

delete-merged-branch Bot deleted the share-permission-fix branch June 10, 2019 13:40

sharidas added the backport-request label Jun 10, 2019

sharidas mentioned this pull request Jun 11, 2019

[stable10] Backport of The recipient of internal share should not inc… #35475

Merged

11 tasks

phil-davis removed the backport-request label Jun 12, 2019

patrickjahns mentioned this pull request Jun 13, 2019

[10.2.1] The recipient of internal share should not increase permission #35510

Merged

6 tasks

This was referenced Jun 19, 2019

Permission of reshare cannot be increased #35528

Closed

if user has enough permission, allow to increase permission #35614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The recipient of internal share should not increase permission#35447

The recipient of internal share should not increase permission#35447
sharidas merged 1 commit intomasterfrom
share-permission-fix

sharidas commented Jun 6, 2019 •

edited

Loading

Uh oh!

codecov Bot commented Jun 6, 2019

Uh oh!

codecov Bot commented Jun 6, 2019 •

edited

Loading

Uh oh!

jvillafanez Jun 7, 2019

Uh oh!

jvillafanez Jun 7, 2019

Uh oh!

micbar Jun 7, 2019

Uh oh!

jvillafanez left a comment

Uh oh!

sharidas commented Jun 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

sharidas commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

Uh oh!

codecov Bot commented Jun 6, 2019

Codecov Report

Uh oh!

codecov Bot commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jvillafanez Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

jvillafanez Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

micbar Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

jvillafanez left a comment

Choose a reason for hiding this comment

Uh oh!

sharidas commented Jun 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sharidas commented Jun 6, 2019 •

edited

Loading

codecov Bot commented Jun 6, 2019 •

edited

Loading