Update owner of subdir on move into/out of share #31728

PVince81 · 2022-03-28T07:33:32Z

When moving a directory into or out of a received share, make sure to
also update the share owner column for shares that exist insie the
directory.

Fixes #30791

All the three scenarios listed there are fixed.

check for possible side effects
check for possible performance problems for regular rename cases
test also with reshare scenarios
add unit tests and/or integration tests

apps/files_sharing/lib/Updater.php

artonge

~~The "moving out of share" scenario is not taken into account as we exit early here: Updater.php:88~~

Handled in 7ae7ce6

artonge · 2022-03-31T10:20:41Z

~~Will look into testing this afternoon.~~

Done.

Test checks for the following for both a file and a folder:

uid_owner change when moving share in another share
→ uid_owner become the uid_owner of the destination share
permissions change when moving share on another share
→ permissions are limited by the permissions of the destination share
→ if moved_share has PERMISSION_ALL, and destination_share has PERMISSION_READ, after the move, moved_share will only have PERMISSION_READ.
uid_owner change when moving share out from another share
→ uid_owner become the current user
permissions stays the same when moving share out from another share

artonge · 2022-03-31T14:55:12Z

apps/files_sharing/lib/Updater.php

 				continue;
 			}
+
+			if ($dstMount instanceof \OCA\Files_Sharing\SharedMount) {
+				if (!($dstMount->getShare()->getPermissions() & Constants::PERMISSION_SHARE)) {


So, if the destination is a share and does not have the SHARE permission, we delete the current share?

Not sure about the rational.

PVince81 · 2022-04-01T10:47:04Z

@artonge I only realized late that you were also working on this. In parallel I wrote two integration tests.
Great to see that you fixed the "move out" scenario where my test failed 😄

lib/private/Share20/DefaultShareProvider.php

artonge · 2022-04-05T16:40:53Z

⚠️ New bug ⚠️ (will be fixed in a follow up)

With S3 bucket as primary storage, when moving a share out from a share, the file_id changes, but the item_source and file_target are not updated in oc_share.

file_id change here: lib/private/Files/ObjectStore/ObjectStoreStorage.php:577-588

Issue: #31870

The failing test has been disabled, and will be re-enabled when the new issue is fixed

artonge · 2022-06-27T09:57:10Z

videoverification tests seem broken?

It's about the implementation of IShareProvider::getSharesInFolder in Talk. Not sure how to handle this case, should we fix it in Talk first, or merge this one and fix in Talk later?

blizzz · 2022-07-05T10:50:48Z

videoverification tests seem broken?

It's about the implementation of IShareProvider::getSharesInFolder in Talk. Not sure how to handle this case, should we fix it in Talk first, or merge this one and fix in Talk later?

Ah, so we are having a breaking change here. Then it also would need to be documented in #32117

Ideally it would work without breaking the APi (via deprecations), but probably that's not feasible?

A new `$shallow` argument was introduced to `IShareProvider::getSharesInFolder` in NC25 by nextcloud/server#31728. This PR adds this new argument and the necessary changes to make it work based on this example https://github.com/nextcloud/server/pull/31728/files#diff-dee1351696d7eae959761edf66c8e5a9052e28be73319733d857a9ab2d9fc967= Signed-off-by: Louis Chemineau <louis@chmn.me>

artonge · 2022-07-26T12:14:01Z

/rebase

Signed-off-by: Louis Chemineau <louis@chmn.me>

When moving a directory into or out of a received share, make sure to also update the share owner column for shares that exist insie the directory. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Signed-off-by: Louis Chemineau <louis@chmn.me>

Added tests to verify share owner change when moving the parent of a subshare in and out of a received share. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Signed-off-by: Louis Chemineau <louis@chmn.me>

artonge · 2022-07-26T15:10:53Z

CI failure unrelated (videoverification is fixed by nextcloud/spreed#7648)

CarlSchwan · 2022-07-27T06:48:48Z

I noticed that this breaks circle due to the API break in the interface :(

A new `$shallow` argument was introduced to `IShareProvider::getSharesInFolder` in NC25 by nextcloud/server#31728. This PR adds this new argument and the necessary changes to make it work based on this example https://github.com/nextcloud/server/pull/31728/files#diff-dee1351696d7eae959761edf66c8e5a9052e28be73319733d857a9ab2d9fc967= Signed-off-by: Louis Chemineau <louis@chmn.me>

PVince81 added the 2. developing Work in progress label Mar 28, 2022

PVince81 added this to the Nextcloud 24 milestone Mar 28, 2022

PVince81 requested review from nickvergessen and icewind1991 March 28, 2022 07:33

PVince81 mentioned this pull request Mar 28, 2022

Moving share in/out of another one doesn't update uid_owner in some cases #30791

Closed

13 tasks

blizzz mentioned this pull request Mar 31, 2022

24.0.0 beta 2 #31769

Merged

artonge reviewed Mar 31, 2022

View reviewed changes

apps/files_sharing/lib/Updater.php Show resolved Hide resolved

artonge force-pushed the bugfix/30791/update-subshare-owner-on-move branch from dd5f8ce to ab7771b Compare March 31, 2022 09:38

artonge reviewed Mar 31, 2022

View reviewed changes

artonge force-pushed the bugfix/30791/update-subshare-owner-on-move branch 4 times, most recently from 0975680 to 7f43c94 Compare March 31, 2022 15:39

PVince81 assigned artonge Apr 1, 2022

nickvergessen requested review from a team, blizzz and come-nc and removed request for nickvergessen and a team April 4, 2022 11:53

nickvergessen added feature: sharing feature: filesystem bug labels Apr 4, 2022

come-nc reviewed Apr 4, 2022

View reviewed changes

lib/private/Share20/DefaultShareProvider.php Show resolved Hide resolved

artonge force-pushed the bugfix/30791/update-subshare-owner-on-move branch from 71406c6 to 8a1d8f5 Compare April 5, 2022 15:01

This comment was marked as resolved.

Sign in to view

blizzz mentioned this pull request Apr 7, 2022

24.0.0 beta 3 #31871

Merged

This was referenced Jul 26, 2022

Add $shallow arg to RoomShareProvider::getSharesInFolder nextcloud/spreed#7648

Merged

Changes for developers & admins for Nextcloud 25 #32117

Closed

artonge and others added 8 commits July 26, 2022 12:17

Test moving share in and out of another share

8967602

Signed-off-by: Louis Chemineau <louis@chmn.me>

Update owner of subdir on move into/out of share

8ca9109

When moving a directory into or out of a received share, make sure to also update the share owner column for shares that exist insie the directory. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Prevent column name conflict between tables

ce7b6ff

Signed-off-by: Louis Chemineau <louis@chmn.me>

Change share owner when moving share out of share

13e7ebb

Signed-off-by: Louis Chemineau <louis@chmn.me>

Int test for share owner change on move

b513ac5

Added tests to verify share owner change when moving the parent of a subshare in and out of a received share. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Recursively fetch subshares

0fb4eca

Signed-off-by: Louis Chemineau <louis@chmn.me>

Skip testMovedIntoShareChangeOwner

256ed34

Signed-off-by: Louis Chemineau <louis@chmn.me>

Add argument to getSharesInFolder enable deep share lookup

f525067

Signed-off-by: Louis Chemineau <louis@chmn.me>

nextcloud-command force-pushed the bugfix/30791/update-subshare-owner-on-move branch from 55fa748 to f525067 Compare July 26, 2022 12:17

artonge merged commit a32de93 into master Jul 26, 2022

artonge deleted the bugfix/30791/update-subshare-owner-on-move branch July 26, 2022 15:11

juliusknorr mentioned this pull request Jul 27, 2022

Adjust share provider to be compatible with 25 nextcloud/deck#3941

Closed

artonge mentioned this pull request Jul 27, 2022

Add $shallow arg to ShareByCircleProvider::getSharesInFolder nextcloud/circles#1117

Merged

nickvergessen mentioned this pull request Jul 29, 2022

Fix share provider for master changes nextcloud/deck#3942

Merged

skjnldsv mentioned this pull request Aug 11, 2022

25.0.0 beta 1 #33505

Merged

artonge mentioned this pull request Apr 18, 2023

Add occ maintenance:repair-share-owner to the docs nextcloud/documentation#10064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update owner of subdir on move into/out of share #31728

Update owner of subdir on move into/out of share #31728

PVince81 commented Mar 28, 2022 •

edited

Loading

artonge left a comment •

edited

Loading

artonge commented Mar 31, 2022 •

edited

Loading

artonge Mar 31, 2022 •

edited

Loading

PVince81 commented Apr 1, 2022

artonge commented Apr 5, 2022 •

edited

Loading

This comment was marked as resolved.

artonge commented Jun 27, 2022

blizzz commented Jul 5, 2022

artonge commented Jul 26, 2022

artonge commented Jul 26, 2022

CarlSchwan commented Jul 27, 2022

Update owner of subdir on move into/out of share #31728

Update owner of subdir on move into/out of share #31728

Conversation

PVince81 commented Mar 28, 2022 • edited Loading

artonge left a comment • edited Loading

Choose a reason for hiding this comment

artonge commented Mar 31, 2022 • edited Loading

artonge Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

PVince81 commented Apr 1, 2022

artonge commented Apr 5, 2022 • edited Loading

This comment was marked as resolved.

artonge commented Jun 27, 2022

blizzz commented Jul 5, 2022

artonge commented Jul 26, 2022

artonge commented Jul 26, 2022

CarlSchwan commented Jul 27, 2022

PVince81 commented Mar 28, 2022 •

edited

Loading

artonge left a comment •

edited

Loading

artonge commented Mar 31, 2022 •

edited

Loading

artonge Mar 31, 2022 •

edited

Loading

artonge commented Apr 5, 2022 •

edited

Loading