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

Validate reshare permissions against actual path that the user tries to share #27820

Closed

Conversation

juliusknorr
Copy link
Member

Otherwise this could lead to taking the wrong user mount in case there
are multiple ones with different permissions that the user could use to
reshare

Possible reproduction:

  • A creates a folder and shares it with group without reshare permissions
  • A creates a subfolder inside of the share and shares it to B with reshare permisions
  • B is member of group
  • B Tries to create a share link of the subfolder

Before:
💥 Cannot increase permissions

After:
The proper source user mount is taken for permission comparison where the user has reshare permissions and the share link creation passes.

@juliusknorr juliusknorr added 2. developing Work in progress bug labels Jul 6, 2021
@juliusknorr
Copy link
Member Author

juliusknorr commented Jul 6, 2021

Other bug found, to be filed separately:

  • B is offered resharing in the UI for the subfolder (but not children) when accessing it through the original share path, but the backend doesn't allow resharing there

@juliusknorr
Copy link
Member Author

Unit tests should be fine now and were actually using the wrong source node in case of a reshare as then the share would always be created from the resharing users filesystem instead of using the owners user folder as a base.

@juliusknorr juliusknorr added 3. to review Waiting for reviews feature: sharing and removed 2. developing Work in progress labels Sep 14, 2021
@juliusknorr juliusknorr marked this pull request as ready for review September 14, 2021 17:10
@juliusknorr juliusknorr added this to the Nextcloud 23 milestone Sep 14, 2021
@nickvergessen
Copy link
Member

We really need integrations tests for this.

@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@skjnldsv skjnldsv modified the milestones: Nextcloud 23, Nextcloud 24 Oct 15, 2021
@juliusknorr juliusknorr force-pushed the bugfix/noid/check-reshare-permission-mountpoint branch from 627640e to 09a6305 Compare October 20, 2021 14:13
@juliusknorr juliusknorr added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 20, 2021
@juliusknorr
Copy link
Member Author

Will look into some tests for that and will double check the suggestion from @PVince81

…to share

Otherwise this could lead to taking the wrong user mount in case there
are multiple ones with different permissions that the user could use to
reshare

Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Copy link
Member Author

Pushed another attempt as the share node might be the one located in the owners filesystem

if ($this->userManager->userExists($this->shareOwner)) {
$userFolder = $this->rootFolder->getUserFolder($this->shareOwner);
} else {
$userFolder = $this->rootFolder->getUserFolder($this->sharedBy);
}
compared to the previously assumed shared by users filesystem https://github.com/nextcloud/server/blob/master/lib/private/Share20/Manager.php#L271-L275

@juliusknorr juliusknorr force-pushed the bugfix/noid/check-reshare-permission-mountpoint branch from 8d8664e to 20fc6a7 Compare October 28, 2021 09:55
@juliusknorr juliusknorr force-pushed the bugfix/noid/check-reshare-permission-mountpoint branch from 20fc6a7 to a6c557c Compare October 28, 2021 10:50
@blizzz blizzz mentioned this pull request Apr 13, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@young-coder-vn

This comment was marked as abuse.

@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
@juliusknorr
Copy link
Member Author

Closing as I don't have time to properly finish this

@juliusknorr juliusknorr closed this Nov 6, 2023
@nickvergessen nickvergessen deleted the bugfix/noid/check-reshare-permission-mountpoint branch November 6, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants