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 remove/update share permissions when the file is locked #4534

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

2403905
Copy link

@2403905 2403905 commented Feb 21, 2024

This is a workaround that should prevent removing or changing the share permissions when the file is locked.
These limitations have to be removed after the wopi server will be able to unlock the file properly.
These limitations are not spread on the files inside the shared folder.

Related issue owncloud/ocis#8273

@2403905 2403905 marked this pull request as ready for review February 22, 2024 09:06
@2403905 2403905 requested review from a team, labkode and glpatcern as code owners February 22, 2024 09:06
Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

We need to check the PermissionSet when updating a share.

return s.checkShareLock(ctx, getShareRes.Share)
}

func (s *svc) checkShareLock(ctx context.Context, share *collaboration.Share) (*rpc.Status, error) {
Copy link
Contributor

@butonic butonic Feb 26, 2024

Choose a reason for hiding this comment

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

I think this is tooo restrictive. AFAIU we can
A) allow adding new users to a file
B) increase the permissions of a share, e.g. add edit permission for a viewer.

The current check disallows any sharing changes when a file is locked.

People often open a file in onlyoffice and then need to ADD new participants.

See owncloud/ocis#8273 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

True, we would have to send the lock in the creat/update share opaque... 🤔

Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

Nevermind, I see now that share creation is unaffected. So new users can be added when a file is locked. That is good enough for me. IMO extending share permissions can be implemented in a followup PR.

@butonic butonic merged commit 9f433ca into cs3org:edge Feb 26, 2024
9 checks passed
Status: status,
}, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I know I am a bit late to the party. But isn't this racy? The file lock can be created after this check, before updating the share. Classical TOCTOU problem, isn't it?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it could happen besides that the s.updateGrant failure can lead to an inconsistent state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, we would have to send the lock in the opaque of the sharing requests to make this (more) atomic.

}

if sRes.GetInfo().GetLock() != nil {
msg := "can not chane grants, the shared resource is locked"

Choose a reason for hiding this comment

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

here is typo: change
Screenshot 2024-02-27 at 15 03 12

@micbar micbar mentioned this pull request Mar 13, 2024
71 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants