Skip to content

Make share permission check work correctly from share manager#26684

Closed
PVince81 wants to merge 1 commit into
masterfrom
sharemanager-takepermsfromcorrectnode
Closed

Make share permission check work correctly from share manager#26684
PVince81 wants to merge 1 commit into
masterfrom
sharemanager-takepermsfromcorrectnode

Conversation

@PVince81

Copy link
Copy Markdown
Contributor

Description

As an API consumer, I expect the PHP Share API (share manager) to take care of all sorts of validation without having to do so myself. Currently some validation is done in the OCS API class instead.

This PR moves some of the validation to happen in the share manager instead.

Related Issue

Discovered while looking into #26639

Motivation and Context

See description

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

TODOs

  • add unit tests for share manager
  • remove tests from OCS API since the share manager is mocked there

Instead of checking for valid share permissions for reshares in the OCS
controller, do it correctly in the share manager. For this, the share
manager needs to read the permissions from the recipient's received
node instead of the node that is stored inside the share which isn't
guaranteed to be the correct one.

Instead of checking for valid share permissions for reshares in the OCS
controller, do it correctly in the share manager. For this, the share
manager needs to read the permissions from the recipient's received
node instead of the node that is stored inside the share which isn't
guaranteed to be the correct one.
@mention-bot

Copy link
Copy Markdown

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

throw new \InvalidArgumentException('A share requires permissions');
}

// Actual permissions need to be taken from the user who is created the share.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

... 'has' created the share.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function should be renamed to generalChecks, because it is also called by updateShare().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The rest looks good 👍

@PVince81 PVince81 modified the milestones: ba, 10.0, backlog Apr 11, 2017
@PVince81 PVince81 modified the milestones: backlog, triage Apr 27, 2018
@PVince81 PVince81 modified the milestones: triage, maybe some day May 22, 2018
@PVince81 PVince81 removed their assignment Jun 5, 2018
@PVince81

PVince81 commented Feb 5, 2019

Copy link
Copy Markdown
Contributor Author

risky with not much value, closing

@PVince81 PVince81 closed this Feb 5, 2019
@PVince81
PVince81 deleted the sharemanager-takepermsfromcorrectnode branch February 5, 2019 16:01
@ownclouders ownclouders mentioned this pull request Feb 6, 2019
@lock lock Bot locked as resolved and limited conversation to collaborators Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants