Skip to content

feat: perform share mount validation on share instead of on mount #57295

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.

Sign up for GitHub

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

Jump to bottom
Merged
icewind1991 merged 5 commits into from
Jan 22, 2026
Merged

feat: perform share mount validation on share instead of on mount #57295

icewind1991 merged 5 commits into from
Jan 22, 2026

Conversation

@icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Dec 30, 2025 •
edited
Loading

Extracted from #57229

Currently the share mount is validated (check that the parent exists and the share doesn't conflict with existing files for the user) when the share is mounted. This moves the check to when the share is created or a user gains access to a share.

While this is more complex then doing it at mount time, it ensures that we only pay the cost once instead of repeatedly on mount.

The existing on-mount method is also incompatible with authoritative mount points.

Requires

@icewind1991 icewind1991 added this to the Nextcloud 33 milestone Dec 30, 2025
@icewind1991 icewind1991 requested a review from a team as a code owner December 30, 2025 14:14
@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Dec 30, 2025
@icewind1991 icewind1991 requested review from Altahrim, CarlSchwan, leftybournes, salmart-dev and yemkareems and removed request for a team December 30, 2025 14:14
@icewind1991 icewind1991 force-pushed the share-mount-validation-on-share branch 11 times, most recently from bcdfc45 to eeffc0a Compare December 31, 2025 13:51
@icewind1991 icewind1991 mentioned this pull request Dec 31, 2025
Closed
3 tasks
@icewind1991
Copy link
Member Author

icewind1991 commented Jan 6, 2026 •
edited
Loading

I forgot to handle talk/circle/etc shares...

It did seem suspiciously easy compared to some earlier attempts...

@icewind1991 icewind1991 marked this pull request as draft January 6, 2026 17:53
@icewind1991 icewind1991 mentioned this pull request Jan 6, 2026
Merged
@icewind1991 icewind1991 force-pushed the share-mount-validation-on-share branch 2 times, most recently from c8e2abd to 9f2e4a7 Compare January 12, 2026 14:59
@icewind1991 icewind1991 marked this pull request as ready for review January 13, 2026 11:46
@nickvergessen nickvergessen mentioned this pull request Jan 13, 2026
Merged
@icewind1991 icewind1991 mentioned this pull request Jan 13, 2026
Draft
4 tasks
@icewind1991 icewind1991 force-pushed the share-mount-validation-on-share branch from 9f2e4a7 to 07b9b8e Compare January 13, 2026 15:38
@icewind1991 icewind1991 marked this pull request as draft January 13, 2026 15:39
@icewind1991
Copy link
Member Author

icewind1991 commented Jan 13, 2026 •
edited
Loading

Putting this on draft to make sure it doesn't get merged without considering the needed share provider work (can't review-block my own PR...)

@icewind1991 icewind1991 force-pushed the share-mount-validation-on-share branch from 07b9b8e to f92e93a Compare January 13, 2026 17:47
@icewind1991 icewind1991 mentioned this pull request Jan 13, 2026
Closed
3 tasks
@provokateurin provokateurin mentioned this pull request Jan 14, 2026
Draft
This was referenced Jan 14, 2026
Open
Closed
@icewind1991 icewind1991 force-pushed the share-mount-validation-on-share branch 5 times, most recently from 26e0887 to 85e56ab Compare January 19, 2026 14:42
@icewind1991
Copy link
Member Author

icewind1991 commented Jan 19, 2026

Still on draft to block it form being merged by accident, but otherwise ready for review

@artonge artonge mentioned this pull request Jan 21, 2026
Open
@icewind1991
feat: perform share mount validation on share instead of on mount
d71c679 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
fix: attempt to make share conflict resolution more resilient to fals…
43a9335 
…e positives

Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
test: adjust tests
94f3346 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
perf: only update shares for users once
b431506 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
fix: adjust SharesUpdatedListener to event change
34fc215 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 force-pushed the share-mount-validation-on-share branch from 7badba8 to 34fc215 Compare January 21, 2026 15:36
artonge
artonge approved these changes Jan 21, 2026
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Looks good code wise. One perf question, though, where it would be nice to double-check.

apps/files_sharing/lib/Listener/SharesUpdatedListener.php
$mountPoints = array_map(fn (ICachedMountInfo $mount) => $mount->getMountPoint(), $cachedMounts);
$mountsByPath = array_combine($mountPoints, $cachedMounts);

$shares = $this->shareMountProvider->getSuperSharesForUser($user);
Copy link
Contributor

@artonge artonge Jan 21, 2026

Choose a reason for hiding this comment

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

Am I right that getSuperSharesForUser() and verifyMountPoint() could take a lot of time?
As this been tested on next.perftesting? Like adding a user, or a group to a talk conversation for example?

I am wondering if we should find a way to put that into a background job.

Copy link
Member Author

@icewind1991 icewind1991 Jan 21, 2026

Choose a reason for hiding this comment

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

This is work that was previously done on-mount, so while it can be expensive yes, it's moving the expense from "read" (with some caching) to "write" time.

Putting it in a background job would mean that we can end up mounting the share before we did the conflict resolution. (Unless we do the work to block mounting the share until then)

@icewind1991 icewind1991 marked this pull request as ready for review January 22, 2026 12:27
@icewind1991 icewind1991 merged commit 3f9849d into master Jan 22, 2026
199 of 201 checks passed
@icewind1991 icewind1991 deleted the share-mount-validation-on-share branch January 22, 2026 12:32
@icewind1991
Copy link
Member Author

icewind1991 commented Jan 22, 2026

/backport to stable33

@backportbot backportbot bot mentioned this pull request Jan 22, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artonge artonge artonge approved these changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner automatically assigned from nextcloud/server-backend

@salmart-dev salmart-dev Awaiting requested review from salmart-dev salmart-dev is a code owner automatically assigned from nextcloud/server-backend

@yemkareems yemkareems Awaiting requested review from yemkareems yemkareems is a code owner automatically assigned from nextcloud/server-backend

@leftybournes leftybournes Awaiting requested review from leftybournes leftybournes is a code owner automatically assigned from nextcloud/server-backend

Assignees

No one assigned

Labels

3. to review Waiting for reviews performance 🚀

Projects

None yet

Milestone

Nextcloud 33

Development

Successfully merging this pull request may close these issues.

5 participants

@icewind1991 @artonge @CarlSchwan @joshtrichards