From d5be3eb5fa8d08e2f49f1d399983c2a781d7ebb9 Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Fri, 11 Sep 2026 10:08:43 +0200 Subject: [PATCH] fix(files_sharing): cap share presets to the permissions the resharer holds Signed-off-by: Peter Ringelmann Signed-off-by: nextcloud-command Signed-off-by: Peter Ringelmann --- .../src/views/SharingDetailsTab.spec.ts | 36 +++++++++ .../src/views/SharingDetailsTab.vue | 29 ++++++- .../reshare-permission-bundle.spec.ts | 75 +++++++++++++++++++ .../support/fixtures/files-sharing-page.ts | 7 ++ 4 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 tests/playwright/e2e/files_sharing/reshare-permission-bundle.spec.ts diff --git a/apps/files_sharing/src/views/SharingDetailsTab.spec.ts b/apps/files_sharing/src/views/SharingDetailsTab.spec.ts index 6cd649f7ae20c..ebc39c3724445 100644 --- a/apps/files_sharing/src/views/SharingDetailsTab.spec.ts +++ b/apps/files_sharing/src/views/SharingDetailsTab.spec.ts @@ -165,3 +165,39 @@ describe('SharingDetailsTab.saveShare — password guard', () => { }) }) }) + +describe('SharingDetailsTab.allPermissions — capped by what the resharer holds', () => { + /** + * @param sharePermissions what the current user received on the node + * @param isFolder whether the shared node is a folder + */ + function permissionsFor(sharePermissions: string | undefined, isFolder = true) { + const ctx = { + isFolder, + fileInfo: { sharePermissions }, + bundledPermissions: { ALL: 31, ALL_FILE: 15 }, + } as never + const grantable = SharingDetailsTab.computed.grantablePermissions.call(ctx) + return SharingDetailsTab.computed.allPermissions.call({ ...(ctx as object), grantablePermissions: grantable }) + } + + it('keeps every bit when the user owns the node', () => { + expect(permissionsFor('31')).toBe('31') + }) + + it('drops delete when the incoming share has no delete permission', () => { + expect(permissionsFor('23')).toBe('23') + }) + + it('drops create and delete on a read-only reshare', () => { + expect(permissionsFor('17')).toBe('17') + }) + + it('caps file shares against ALL_FILE', () => { + expect(permissionsFor('31', false)).toBe('15') + }) + + it('falls back to full permissions when the DAV property is missing', () => { + expect(permissionsFor(undefined)).toBe('31') + }) +}) diff --git a/apps/files_sharing/src/views/SharingDetailsTab.vue b/apps/files_sharing/src/views/SharingDetailsTab.vue index 29616d1829be1..803a27ee347c3 100644 --- a/apps/files_sharing/src/views/SharingDetailsTab.vue +++ b/apps/files_sharing/src/views/SharingDetailsTab.vue @@ -453,8 +453,21 @@ export default { return getBundledPermissions(this.config.excludeReshareFromEdit) }, + /** + * Permissions the current user is allowed to hand out. + * On a reshare this is capped by what they received themselves. + * + * @return {number} + */ + grantablePermissions() { + const received = Number(this.fileInfo.sharePermissions) + // A missing prop means we don't know, not that they hold nothing + return Number.isNaN(received) ? getBundledPermissions().ALL : received + }, + allPermissions() { - return this.isFolder ? this.bundledPermissions.ALL.toString() : this.bundledPermissions.ALL_FILE.toString() + const bundle = this.isFolder ? this.bundledPermissions.ALL : this.bundledPermissions.ALL_FILE + return (bundle & this.grantablePermissions).toString() }, /** @@ -1093,7 +1106,10 @@ export default { this.sharingPermission = basePermissions.FILE_DROP.toString() } else { this.sharingPermission = 'custom' - this.share.permissions = defaultPermissions + // The admin default can ask for more than a resharer may pass on. + // Only the value is capped: deciding the branch on the capped value + // would push every reshare into this one and expand the accordion. + this.share.permissions = defaultPermissions & this.grantablePermissions this.advancedSectionAccordionExpanded = true this.setCustomPermissions = true } @@ -1117,6 +1133,15 @@ export default { initializePermissions() { this.handleShareType() this.handleDefaultPermissions() + // A new share starts from the full permission set, so the atomic + // checkboxes would pre-check rights a resharer cannot pass on and the + // share would be rejected on save. An existing share keeps what is + // stored, so a permission the owner revoked afterwards stays revocable. + if (this.isNewShare) { + // The editor owns the share it is building, like the rest of this file. + // eslint-disable-next-line vue/no-mutating-props + this.share.permissions &= this.grantablePermissions + } this.handleCustomPermissions() }, diff --git a/tests/playwright/e2e/files_sharing/reshare-permission-bundle.spec.ts b/tests/playwright/e2e/files_sharing/reshare-permission-bundle.spec.ts new file mode 100644 index 0000000000000..96f349183fd88 --- /dev/null +++ b/tests/playwright/e2e/files_sharing/reshare-permission-bundle.spec.ts @@ -0,0 +1,75 @@ +/* + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { expect, test } from '../../support/fixtures/files-sharing-page.ts' +import { mkdir } from '../../support/utils/dav.ts' +import { ALL_PERMISSIONS, createShare, openSharingPanel, SharePermission } from '../../support/utils/sharing.ts' + +const { READ, UPDATE, CREATE, SHARE } = SharePermission + +/** What the owner hands the resharer: everything but DELETE. */ +const WITHOUT_DELETE = READ | UPDATE | CREATE | SHARE + +/** + * The editor's "Allow upload and editing" bundle asks for the full permission + * set, DELETE included. A resharer who never received DELETE cannot pass it on, + * so the backend rejected the entire share ("Cannot increase permissions of %s") + * and the bundle was unusable on a reshare. It is now capped to what the + * resharer actually holds. + */ +test.describe('files_sharing: the permissions offered on a reshare', () => { + const RESHARED = 'reshared-folder' + const OWNED = 'owned-folder' + + test.beforeEach(async ({ page, user, owner, ownerRequest, filesListPage }) => { + await mkdir(ownerRequest, owner, `/${RESHARED}`) + await createShare(ownerRequest, `/${RESHARED}`, user.userId, { permissions: WITHOUT_DELETE }) + await mkdir(page.request, user, `/${OWNED}`) + await filesListPage.open() + }) + + test('drops the delete permission the resharer never received', async ({ filesListPage, sharingTab }) => { + await openSharingPanel(filesListPage, sharingTab, RESHARED) + await sharingTab.pickRecipient('reshare@example.org', { external: true }) + await sharingTab.selectPermissionBundle('upload-edit') + + const share = await sharingTab.save() + + expect(share.permissions).toBe(WITHOUT_DELETE) + }) + + test('still grants everything on a folder the sharer owns', async ({ filesListPage, sharingTab }) => { + await openSharingPanel(filesListPage, sharingTab, OWNED) + await sharingTab.pickRecipient('owned@example.org', { external: true }) + await sharingTab.selectPermissionBundle('upload-edit') + + const share = await sharingTab.save() + + expect(share.permissions).toBe(ALL_PERMISSIONS) + }) + + /** + * The atomic checkboxes read the share's own permissions, and a new share + * starts from the full set — so "Delete" used to be offered pre-checked on a + * reshare and the share was rejected without the user touching anything. + */ + test('does not offer delete in the custom permissions of a reshare', async ({ filesListPage, sharingTab }) => { + await openSharingPanel(filesListPage, sharingTab, RESHARED) + await sharingTab.pickRecipient('custom-reshare@example.org', { external: true }) + await sharingTab.selectPermissionBundle('custom') + + await expect(sharingTab.checkbox('Delete')).not.toBeChecked() + await expect(sharingTab.checkbox('Delete')).toBeDisabled() + }) + + test('still offers delete in the custom permissions of an owned folder', async ({ filesListPage, sharingTab }) => { + await openSharingPanel(filesListPage, sharingTab, OWNED) + await sharingTab.pickRecipient('custom-owned@example.org', { external: true }) + await sharingTab.selectPermissionBundle('custom') + + await expect(sharingTab.checkbox('Delete')).toBeChecked() + await expect(sharingTab.checkbox('Delete')).toBeEnabled() + }) +}) diff --git a/tests/playwright/support/fixtures/files-sharing-page.ts b/tests/playwright/support/fixtures/files-sharing-page.ts index ebf2eed440ed5..3418183f51eec 100644 --- a/tests/playwright/support/fixtures/files-sharing-page.ts +++ b/tests/playwright/support/fixtures/files-sharing-page.ts @@ -8,6 +8,7 @@ import type { APIRequestContext } from '@playwright/test' import { runOcc } from '@nextcloud/e2e-test-server/docker' import { createRandomUser } from '@nextcloud/e2e-test-server/playwright' +import { SharingTab } from '../sections/SharingTab.ts' import { test as filesTest } from './files-page.ts' type SharingFixtures = { @@ -18,6 +19,8 @@ type SharingFixtures = { * and the seeding would run as the logged-in recipient instead. */ ownerRequest: APIRequestContext + /** The share editor in the files sidebar, driven as the share recipient. */ + sharingTab: SharingTab } /** @@ -42,6 +45,10 @@ export const test = filesTest.extend({ await use(context) await context.dispose() }, + + sharingTab: async ({ page }, use) => { + await use(new SharingTab(page)) + }, }) export { expect } from '../matchers.ts'