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

Cannot share with user with more permissions when user has access through group read-only #25359

Closed
PVince81 opened this Issue Jul 5, 2016 · 6 comments

Comments

Projects
None yet
1 participant
@PVince81
Member

PVince81 commented Jul 5, 2016

Steps

  1. Create two users "user1" and "user2"
  2. Put "user2" in two groups "groupa" and "groupb"
  3. Login as "user1"
  4. Create a folder "test"
  5. Share "test" with "groupa" but remove edit permissions
  6. Share "test" with "user2" and allow editing
  7. Share "test" with "groupb" and allow editing
  8. Login as "user2"
  9. Check permissions and how many folders were received

Expected result

"user2" received a single folder "test" with edit permissions

Actual result

At step 6 (direct user share), OC doesn't allow sharing because it's already shared.
At step 7 it works however.

Versions

Observed in v9.0.3.
Regression from v8.2.

@owncloud/sharing

@PVince81 PVince81 added this to the 9.0.4 milestone Jul 5, 2016

@PVince81 PVince81 self-assigned this Jul 5, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 5, 2016

Member

A bisect shows that this problem starts to appear after we made the share dialog use the public OCS Share API instead of the old internal ajax endpoints: #21860

It is likely that the OCS Share API never allowed this configuration before, so need to find a way to make it align to the way the old API behaved.

Member

PVince81 commented Jul 5, 2016

A bisect shows that this problem starts to appear after we made the share dialog use the public OCS Share API instead of the old internal ajax endpoints: #21860

It is likely that the OCS Share API never allowed this configuration before, so need to find a way to make it align to the way the old API behaved.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 5, 2016

Member

Weiiiird, this seems to work on master. I found this check in the code: https://github.com/owncloud/core/blob/stable9/lib/private/share20/manager.php#L362

It would only show the error if someone else tried to share with that user. If the share owner is the same, it goes through.

Now to find out why it works on master and not stable9.

Member

PVince81 commented Jul 5, 2016

Weiiiird, this seems to work on master. I found this check in the code: https://github.com/owncloud/core/blob/stable9/lib/private/share20/manager.php#L362

It would only show the error if someone else tried to share with that user. If the share owner is the same, it goes through.

Now to find out why it works on master and not stable9.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 5, 2016

Member

Aha! It turns out that the share owner of the new share isn't set properly (null), so the comparison would always be true: https://github.com/owncloud/core/blob/stable9/lib/private/share20/manager.php#L362

Now to find out what commit fixed it in master

Member

PVince81 commented Jul 5, 2016

Aha! It turns out that the share owner of the new share isn't set properly (null), so the comparison would always be true: https://github.com/owncloud/core/blob/stable9/lib/private/share20/manager.php#L362

Now to find out what commit fixed it in master

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 5, 2016

Member

Found it, this is the commit that fixes it on master: afa37d3

Member

PVince81 commented Jul 5, 2016

Found it, this is the commit that fixes it on master: afa37d3

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 5, 2016

Member

Looks like it's not as easy as just backporting it. Even applying this wouldn't solve the problem.
It seems it might have been broken again later on and then fixed again.

From what I see on master the order between "setShareOwner" and "userCreateChecks" has been changed at some point. It makes sense to first set the owner before doing the checks.

I'll try and fix this manually then instead of backporting.

Member

PVince81 commented Jul 5, 2016

Looks like it's not as easy as just backporting it. Even applying this wouldn't solve the problem.
It seems it might have been broken again later on and then fixed again.

From what I see on master the order between "setShareOwner" and "userCreateChecks" has been changed at some point. It makes sense to first set the owner before doing the checks.

I'll try and fix this manually then instead of backporting.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 5, 2016

Member

Fix is here: #25360

Member

PVince81 commented Jul 5, 2016

Fix is here: #25360

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment