Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix file permissions at read time when wrong in DB #26542

Closed
wants to merge 1 commit into from

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Nov 3, 2016

Description

In some past situations it was possible to create file shares with
invalid permissions. Since such entries still exist in some databases,
the share provider now automatically masks these permissions with the
allowed ones. This will prevent some code paths like ownership transfer
to trigger problems where permissions cannot be changed.

Related Issue

Fixes #26541

Motivation and Context

See linked issue.
Note that I decided against doing this in the manager directly because the manager is doing validation and is not supposed to change input data.

How Has This Been Tested?

Unit tests and see steps in ticket.

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.

Backports

  • stable9.1
  • stable9

@DeepDiver1975 @jvillafanez @PhilippSchaffrath

In some past situations it was possible to create file shares with
invalid permissions. Since such entries still exist in some databases,
the share provider now automatically masks these permissions with the
allowed ones. This will prevent some code paths like ownership transfer
to trigger problems where permissions cannot be changed.
@PVince81 PVince81 added this to the 9.2 milestone Nov 3, 2016
@mention-bot
Copy link

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

@davitol
Copy link
Contributor

davitol commented Nov 4, 2016

👍 WFM Update from 8.2.8->9.0.5->9.1.1. 'Cannot increase permissions...' warning disappeared and the transferownership command worked fine.

oc_share permissions is set to 19

@jvillafanez
Copy link
Member

If I understood the problem correctly, I don't think this is a good solution.

The user can still create shares with wrong permissions through the ocs api and the api will return something different: if I try to create a share with permissions 75, the api should return an error, not create a share with whatever permissions it wants.

For the invalid entries that could be already in the DB, we should provide a repair step during the next upgrade. Fixing this during the transfer implies that other operations will still read wrong permissions.

There is also the problem with the permissions: web UI shows 5 kind of permissions (if I remember correctly), this PR only offers 3 so, what happens with the rest?

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2016

The user can still create shares with wrong permissions through the ocs api and the api will return something different

No, this isn't possible any more already. It's only in OC < 9.0 that it used to be possible.

A repair step, maybe yes.

  • add repair step

There is also the problem with the permissions: web UI shows 5 kind of permissions (if I remember correctly), this PR only offers 3 so, what happens with the rest?

For file shares the web UI offers only two kind of permissions: "can edit" and "can share".
For folders there's more. But this PR is only required for file shares.

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2016

if I try to create a share with permissions 75, the api should return an error, not create a share with whatever permissions it wants.

Indeed. I'd do this only for 9.2 to avoid breaking clients.

  • fix API to throw error on invalid perms instead of auto-adjusting them

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2016

Repair step here: #26562

Closing in favor of that one.

@PVince81 PVince81 closed this Nov 7, 2016
@PVince81 PVince81 deleted the shares-maskinvalidpermsonread branch November 7, 2016 11:28
@lock
Copy link

lock bot commented Aug 4, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
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.

Transfer ownership cannot increase permissions for file shares with invalid perms
4 participants