Skip to content

Conversation

@provokateurin
Copy link
Member

@provokateurin provokateurin commented Jun 3, 2025

Summary

Another attempt at #51988.
The fix is the same, but I've dug deeper into the issue and found that the integration tests don't transfer incoming shares when doing the ownership transfer. Just enabling incoming share transfer would have been enough, but this actually pointed to problem that we currently break reshares when doing ownership transfer without incoming shares.
The original user will still have the incoming share, while the outgoing (re-)share is transferred to the new user. My fix for hiding own reshares assumed, that one would always have access to the original share when you have a reshare for it.
With the partial ownership transfer this is not true and I believe this is wrong as we must always transfer all shares to guarantee that reshares are still working as expected.

This will require some changes in Talk integration tests because paths have changed:

diff --git a/tests/integration/features/sharing-4/transfer-ownership.feature b/tests/integration/features/sharing-4/transfer-ownership.feature
index cbf596344..12f547f0f 100644
--- a/tests/integration/features/sharing-4/transfer-ownership.feature
+++ b/tests/integration/features/sharing-4/transfer-ownership.feature
@@ -65,11 +65,11 @@ Feature: transfer-ownership
       | displayname_owner      | participant2-displayname |
       | uid_file_owner         | participant3 |
       | displayname_file_owner | participant3-displayname |
-      | path                   | /welcome (2).txt |
+      | path                   | /Talk/welcome (2).txt |
       | item_type              | file |
       | mimetype               | text/plain |
-      | storage_id             | shared::/welcome (2).txt |
-      | file_target            | /welcome (2).txt |
+      | storage_id             | shared::/Talk/welcome (2).txt |
+      | file_target            | /Talk/welcome (2).txt |
       | share_with             | group room |
       | share_with_displayname | Group room |
     And user "participant2" gets last share
@@ -78,10 +78,10 @@ Feature: transfer-ownership
       | displayname_owner      | participant2-displayname |
       | uid_file_owner         | participant3 |
       | displayname_file_owner | participant3-displayname |
-      | path                   | /Talk/welcome (2).txt |
+      | path                   | REGEXP /^\/Transferred from participant1-displayname on [0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{2}-[0-9]{2}-[0-9]{2}\/welcome \(2\)\.txt$/ |
       | item_type              | file |
       | mimetype               | text/plain |
-      | storage_id             | shared::/Talk/welcome (2).txt |
+      | storage_id             | REGEXP /^shared::\/Transferred from participant1-displayname on [0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{2}-[0-9]{2}-[0-9]{2}\/welcome \(2\)\.txt$/ |
       | file_target            | /Talk/welcome (2).txt |
       | share_with             | group room |
       | share_with_displayname | Group room |

Checklist

@provokateurin provokateurin added this to the Nextcloud 32 milestone Jun 3, 2025
@provokateurin provokateurin requested a review from a team as a code owner June 3, 2025 15:49
@provokateurin provokateurin requested review from icewind1991 and removed request for a team June 3, 2025 15:49
@provokateurin provokateurin added the 3. to review Waiting for reviews label Jun 3, 2025
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Code-wise this is okay, but I’m not sure about the design decision.
There might be no reshares, do we really want to remove the possibility to transfer ownership without incoming shares?

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Also, transfer takes a path to transfer ownership only of a folder, but with this PR all incoming shares including outside of this path will be included I think?

@provokateurin
Copy link
Member Author

provokateurin commented Jun 23, 2025

There might be no reshares, do we really want to remove the possibility to transfer ownership without incoming shares?

TBH I don't see why we have it as an option in the first place. The PR that added it has no explanation or review comments that give a reason for the option to exist.

Also, transfer takes a path to transfer ownership only of a folder, but with this PR all incoming shares including outside of this path will be included I think?

I didn't look into it because I thought this was already done correctly, so I suppose this is another bug with the existing implementation outside of the bug I'm trying to fix (but of course I will fix it as well).

@provokateurin provokateurin force-pushed the fix/files_sharing/hide-own-reshares branch from 10d81ca to 482c774 Compare June 23, 2025 06:59
@provokateurin provokateurin requested a review from come-nc June 23, 2025 06:59
@provokateurin
Copy link
Member Author

/backport to stable31

@provokateurin
Copy link
Member Author

/backport to stable30

@provokateurin provokateurin force-pushed the fix/files_sharing/hide-own-reshares branch from 482c774 to 3c31322 Compare June 30, 2025 07:42
@provokateurin provokateurin marked this pull request as draft June 30, 2025 09:39
@provokateurin
Copy link
Member Author

Marking as draft for now, as I still want to address @nickvergessen's comment.

@provokateurin provokateurin force-pushed the fix/files_sharing/hide-own-reshares branch from 3c31322 to 8e580f8 Compare July 1, 2025 06:24
@provokateurin provokateurin marked this pull request as ready for review July 1, 2025 06:24
@provokateurin provokateurin enabled auto-merge July 1, 2025 06:47
@sorbaugh sorbaugh disabled auto-merge July 1, 2025 10:27
@sorbaugh sorbaugh merged commit f36100e into master Jul 1, 2025
244 of 258 checks passed
@sorbaugh sorbaugh deleted the fix/files_sharing/hide-own-reshares branch July 1, 2025 10:27
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 bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Reshare with (own) group ends up as double share for sharee

7 participants