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

Implement ownerless shares #49073

Merged
merged 10 commits into from
Nov 25, 2024
Merged

Implement ownerless shares #49073

merged 10 commits into from
Nov 25, 2024

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Nov 4, 2024

Summary

Can be tested with nextcloud/groupfolders#3401

Tests still need to be written, but other than that this is ready.
It doesn't work with files_external yet, but I think that is due to permissions (UI says there is no reshare permission).

I'm surprised how little lines had to be changed in the end, but the challenging part was exploring how this can be implemented and where things needs to be changed and fixed to make it work.

Behavior:

  • If the owner is deleted the shares will be deleted
  • If the owner is disabled the shares continue to exist
  • If the owner loses access (e.g. removed from group that had access to the Groupfolder) the shares will be deleted hidden

Checklist

$shares = [];
foreach ($providers as $provider) {
if ($isCoOwner) {
foreach ($node->getDirectoryListing() as $childNode) {
Copy link
Member

Choose a reason for hiding this comment

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

This makes it a lot more expensive, can we tweak getSharesInFolder to make the $user filter optional instead?

Copy link
Member Author

@provokateurin provokateurin Nov 11, 2024

Choose a reason for hiding this comment

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

We can't do this without a breaking change, as it would break provider implementations that don't expect to receive null.
Maybe that is fine though, as long as the providers only do database SELECTs where a null value is fine (but that would still result in unexpected results as nothing would be returned instead of everything).

Choose a reason for hiding this comment

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

Istead of null it could receive a keyword.

@provokateurin provokateurin force-pushed the feat/files_sharing/co-owner branch from dbee0fb to 0ee7f19 Compare November 13, 2024 13:45
@provokateurin provokateurin requested review from a team, ArtificialOwl, icewind1991 and come-nc and removed request for a team November 13, 2024 13:45
@provokateurin provokateurin marked this pull request as ready for review November 13, 2024 13:45
@provokateurin
Copy link
Member Author

Added tests for everything 🎉

@provokateurin provokateurin added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 13, 2024
@provokateurin provokateurin force-pushed the feat/files_sharing/co-owner branch 3 times, most recently from 4c63eba to cd4dc54 Compare November 14, 2024 07:16
@ArtificialOwl
Copy link
Member

maybe Ownerless instead of CoOwner ?

@provokateurin
Copy link
Member Author

Sure I will go with that, even though it is still inaccurate :/

Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin provokateurin force-pushed the feat/files_sharing/co-owner branch from cd4dc54 to 5ec57e4 Compare November 25, 2024 08:56
@provokateurin provokateurin changed the title Implement share co-owning Implement ownerless shares Nov 25, 2024
Signed-off-by: provokateurin <kate@provokateurin.de>
…ath()

Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
…es on IShareOwnerlessMount

Signed-off-by: provokateurin <kate@provokateurin.de>
…hare

Signed-off-by: provokateurin <kate@provokateurin.de>
… owner

Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin provokateurin force-pushed the feat/files_sharing/co-owner branch from 5ec57e4 to c0db746 Compare November 25, 2024 09:27
@nextcloud nextcloud locked and limited conversation to collaborators Nov 25, 2024
@nextcloud nextcloud unlocked this conversation Nov 25, 2024
*
* @since 31.0.0
*/
interface IShareOwnerlessMount {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
interface IShareOwnerlessMount {
interface IShareOpenMount {
// interface IShareCollaborativeMount

Might be too far and too large a refactor at this point but just throwing in the idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm IShareCollaborativeMount doesn't sound too bad. @ArtificialOwl what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also already did a rename of everything this morning, so doing it once more is not much work 🙈

Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

Cool to the kwargs in action here.

@provokateurin provokateurin added the pending documentation This pull request needs an associated documentation update label Nov 25, 2024
@provokateurin
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@provokateurin provokateurin merged commit 235b7d7 into master Nov 25, 2024
185 of 186 checks passed
@provokateurin provokateurin deleted the feat/files_sharing/co-owner branch November 25, 2024 13:41
@provokateurin provokateurin removed the pending documentation This pull request needs an associated documentation update label Nov 25, 2024
@narcisgarcia
Copy link

Will this enhancement reach the expected behavior asked in
nextcloud/groupfolders#3456
and
#49619
?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants