Skip to content

fix(sharing): set STATUS_ACCEPTED when creating USERGROUP subshare on…#59813

Draft
miaulalala wants to merge 1 commit into
masterfrom
fix/noid/fix-owncloud-group-share-rename
Draft

fix(sharing): set STATUS_ACCEPTED when creating USERGROUP subshare on…#59813
miaulalala wants to merge 1 commit into
masterfrom
fix/noid/fix-owncloud-group-share-rename

Conversation

@miaulalala
Copy link
Copy Markdown
Contributor

@miaulalala miaulalala commented Apr 21, 2026

Root cause

When an ownCloud-migrated group share (which has no per-user USERGROUP subshare) is renamed for the first time, DefaultShareProvider::move() inserts a new USERGROUP row without setting accepted. The column defaults to 0 (STATUS_PENDING), causing MountProvider to skip the share on the next request — the file disappears for the recipient.

Native Nextcloud group shares are unaffected because the accept flow always creates the USERGROUP subshare and immediately sets accepted = STATUS_ACCEPTED before the user can rename.

A second bug compounded this: SharedMount::moveMount() was catching exceptions from updateFileTarget() (e.g. Manager::moveShare() rejecting an orphaned group share because the original group no longer exists in Nextcloud) but always returning true. This caused View::rename() to believe the rename succeeded, update \$this->path to the new location, and then fail with HTTP 500 on refreshInfo() — making the file appear to vanish with no clean error.

Fixes

  1. DefaultShareProvider::move() — set accepted = STATUS_ACCEPTED when inserting a new USERGROUP subshare on first rename, so MountProvider does not skip it on the next load.

  2. SharedMount::moveMount() — set \$result = false in the catch block so rename failures propagate cleanly instead of corrupting virtual filesystem state. updateFileTarget() widened from private to protected to allow unit testing.

  3. occ sharing:fix-owncloud-group-shares — opt-in repair command for instances that already hit the bug before this fix was deployed. Finds USERGROUP subshares stuck at STATUS_PENDING with non-zero permissions and restores them to STATUS_ACCEPTED. Supports --dry-run. Deliberately not an automatic repair step — admins should run this consciously, as shares that have been invisible for some time will reappear for recipients.

Tests

  • DefaultShareProviderTest::testMoveGroupShare() — asserts STATUS_ACCEPTED after both first and second move (INSERT and UPDATE paths).
  • SharedMountTest::testMoveOwncloudMigratedGroupShareRemainsVisibleAfterRename() — integration test simulating the full OC migration state: GROUP share with accepted = STATUS_ACCEPTED and no USERGROUP subshare; verifies the share is still visible after rename and re-login.
  • SharedMountTest::testMoveMountReturnsFalseWhenUpdateFileTargetThrows() — unit test verifying moveMount() returns false when updateFileTarget() throws.
  • FixOwncloudGroupSubshareStatusTest — covers fix, dry-run, declined shares (permissions=0), already-accepted shares, non-USERGROUP shares, and multiple rows.

Fixes: #59791

AI-Assisted-By: Claude Sonnet 4.6 noreply@anthropic.com

@miaulalala miaulalala force-pushed the fix/noid/fix-owncloud-group-share-rename branch from 630f3b2 to 1e03eff Compare April 23, 2026 11:11
@miaulalala miaulalala force-pushed the fix/noid/fix-owncloud-group-share-rename branch 5 times, most recently from c479d14 to 96fd848 Compare May 19, 2026 11:55
Copy link
Copy Markdown
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

This should only be part of the migration and not available outside.

Comment on lines +61 to +63
->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->eq('accepted', $qb->createNamedParameter(IShare::STATUS_PENDING, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->neq('permissions', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you sure this will only match those broken shares? There must not be any false positives.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wanted to give admins the option to restore already broken shares. Some admins may not want to have that.

And yes, any oC share will have a permission other than 0 if it wasn't rejected. So only oC shares that were accepted but haven't had the update for the new column will be in that state.

@miaulalala miaulalala force-pushed the fix/noid/fix-owncloud-group-share-rename branch from 96fd848 to aa49469 Compare May 19, 2026 12:10
@miaulalala
Copy link
Copy Markdown
Contributor Author

miaulalala commented May 19, 2026

@provokateurin re: "This should only be part of the migration and not available outside" - an automatic repair step would silently restore shares that recipients may not have seen for weeks or months. From their perspective, files would suddenly reappear with no explanation, which seems like the wrong default for instances that have been running in the broken state for a while.

The opt-in occ command means the admin consciously decides to restore the shares and can communicate the change to affected users beforehand. The --dry-run flag also lets them audit the scope before committing.

The actual root cause (missing accepted = STATUS_ACCEPTED on INSERT in DefaultShareProvider::move()) is fixed unconditionally, so new renames on already-migrated instances work correctly going forward without any manual intervention.

@miaulalala miaulalala force-pushed the fix/noid/fix-owncloud-group-share-rename branch from aa49469 to c1e173a Compare May 19, 2026 13:42
…shares

When an ownCloud-migrated group share (which has no per-user USERGROUP
subshare) is renamed for the first time, DefaultShareProvider::move()
inserted a new USERGROUP row without setting `accepted`. The column
defaulted to 0 (STATUS_PENDING), causing MountProvider to skip the
share on the next login — the shared file disappeared for the recipient.

Fix: set accepted = STATUS_ACCEPTED explicitly on the INSERT in
DefaultShareProvider::move() for the TYPE_GROUP branch.

Secondary fix: SharedMount::moveMount() silently returned true when
updateFileTarget() threw (e.g. group no longer exists on an OC-migrated
instance). Set $result = false in the catch block so View::rename()
propagates the failure instead of silently corrupting VFS state.

An opt-in occ command (sharing:fix-owncloud-group-shares) with --dry-run
support is included to repair existing broken instances. It targets only
TYPE_USERGROUP subshares with accepted=STATUS_PENDING and permissions!=0
(shares that were accepted but broken by the missing column default),
leaving explicitly declined shares (permissions=0) untouched.

AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala miaulalala force-pushed the fix/noid/fix-owncloud-group-share-rename branch from c1e173a to 9a4427c Compare May 19, 2026 13:44
@miaulalala miaulalala added the 2. developing Work in progress label May 19, 2026
@miaulalala miaulalala added this to the Nextcloud 35 milestone May 19, 2026
@miaulalala miaulalala self-assigned this May 19, 2026
@miaulalala miaulalala requested a review from provokateurin May 19, 2026 14:16
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.

[Bug]: owncloud migrated internal shared files cannot be renamed/changed

2 participants