diff --git a/src/backend/controllers/share/ShareController.http.test.ts b/src/backend/controllers/share/ShareController.http.test.ts index 94b3c6be13..45f9ceba24 100644 --- a/src/backend/controllers/share/ShareController.http.test.ts +++ b/src/backend/controllers/share/ShareController.http.test.ts @@ -109,6 +109,33 @@ describe('share endpoints over HTTP', () => { } }); + it('says whether a share created access or the recipient already had it', async () => { + const owner = env.users.user; + const recipient = env.users.other; + const file = await makeFile(owner); + const share = (mode: string) => + post('/share', owner.token, { + recipients: [recipient.username], + items: [{ uid: file.uid }], + mode, + }).then((r) => r.json() as Promise<{ + results: Array<{ is_new?: boolean }>; + }>); + + expect((await share('read')).results[0].is_new).toBe(true); + // Without this the dialog cannot tell a repeat from a first share. + expect((await share('read')).results[0].is_new).toBe(false); + expect((await share('write')).results[0].is_new).toBe(false); + + // A listing describes standing access, so it says nothing about it. + const listed = await get('/share/shares', owner.token, { + uid: file.uid, + }).then((r) => r.json() as Promise<{ + items: Array>; + }>); + expect(listed.items[0]).not.toHaveProperty('is_new'); + }); + it('shares an item, lists it for the recipient, then revokes it', async () => { const owner = env.users.user; const recipient = env.users.other; diff --git a/src/backend/controllers/share/clientShare.ts b/src/backend/controllers/share/clientShare.ts index e7eb9fe589..8f0e813dfe 100644 --- a/src/backend/controllers/share/clientShare.ts +++ b/src/backend/controllers/share/clientShare.ts @@ -57,6 +57,8 @@ export async function toClientShare( ...(share.pending ? { pending: true, recipient_email: share.recipientEmail } : {}), + // Set on a share call only, so a listing stays silent about it. + ...(share.isNew === undefined ? {} : { is_new: share.isNew }), uid_entry: share.entryUid, is_dir: share.isDir, issuer: share.issuer.username, diff --git a/src/backend/services/share/ShareService.test.ts b/src/backend/services/share/ShareService.test.ts index 759278c24e..5a6089f017 100644 --- a/src/backend/services/share/ShareService.test.ts +++ b/src/backend/services/share/ShareService.test.ts @@ -806,7 +806,35 @@ describe('ShareService', () => { recipient: { email: third.email }, mode: 'manage', }), - ).rejects.toMatchObject({ statusCode: 403 }); + ).rejects.toMatchObject({ + statusCode: 403, + legacyCode: 'cannot_delegate_manage', + }); + + // What they can do is unchanged. + await expect( + share(delegate.actor, { + uid: file.uuid, + recipient: { email: third.email }, + mode: 'write', + }), + ).resolves.toMatchObject({ mode: 'write' }); + }); + + it('tells a stranger nothing when they ask to grant `manage`', async () => { + const owner = await makeUser(); + const stranger = await makeUser(); + const third = await makeUser(); + const file = await makeFile(owner.user); + + // No access at all, so the refusal must not confirm the file exists. + await expect( + share(stranger.actor, { + uid: file.uuid, + recipient: { email: third.email }, + mode: 'manage', + }), + ).rejects.not.toMatchObject({ legacyCode: 'cannot_delegate_manage' }); }); it('leaves a delegate alone when their authority survives another issuer', async () => { diff --git a/src/backend/services/share/ShareService.ts b/src/backend/services/share/ShareService.ts index 9e6ce17dab..9a3bf1cc9c 100644 --- a/src/backend/services/share/ShareService.ts +++ b/src/backend/services/share/ShareService.ts @@ -94,11 +94,9 @@ export interface ResolvedShare { issuedByApp?: string | null; modified: number; size: number | null; - /** - * Set by `share()` only, and never sent to a client: who to notify, and - * whether this call created reach that didn't exist before. - */ + /** Set by `share()` only: who to notify. Never sent to a client. */ holderId?: number; + /** Whether this call created reach that didn't exist before. */ isNew?: boolean; /** * An invite to an address with no confirmed account. No grant exists yet — @@ -1720,6 +1718,22 @@ export class ShareService extends PuterService { // given, rather than everything its user owns. if (allowed && (await this.#hasOwnReach(actor, entry, mode))) return; + // Only for someone who can already share here, so it leaks nothing. + if (mode === MANAGE_PERM_PREFIX) { + const canDelegateAccess = + await this.services.permission.canManagePermission( + userRelatedActor(actor), + entryPermissionForMode(entry.uuid, 'write'), + ); + if (canDelegateAccess) { + throw new HttpError( + 403, + 'Only the owner can grant edit & share access', + { legacyCode: 'cannot_delegate_manage' }, + ); + } + } + const safe = await this.services.acl.getSafeAclError( actor, this.#descriptorFor(entry), diff --git a/src/docs/src/FS/share.md b/src/docs/src/FS/share.md index 2ba1479b05..69a807bc6c 100644 --- a/src/docs/src/FS/share.md +++ b/src/docs/src/FS/share.md @@ -69,6 +69,7 @@ A `Promise` that resolves to an array of share objects, one per recipient/item p - `recipientEmail` (String) - Address a pending share was sent to. Only set when `pending`. - `modified` (Number) - Last-modified time of the item, in unix seconds. - `size` (Number) - Size of the item in bytes; `null` for a directory. +- `isNew` (Boolean) - Whether this call created access that did not exist before. `false` means the recipient already had it, possibly at a different mode — sharing again is not an error, so this is how you tell the two apart. Only `share()` reports it; a listing leaves it undefined. Sharing the same item with the same person again **replaces** their access rather than adding a second share, so raising someone from `read` to `write` is just another call. diff --git a/src/gui/src/UI/Dashboard/UIShareModal.js b/src/gui/src/UI/Dashboard/UIShareModal.js index 65e4217b8c..f6f768d984 100644 --- a/src/gui/src/UI/Dashboard/UIShareModal.js +++ b/src/gui/src/UI/Dashboard/UIShareModal.js @@ -19,7 +19,7 @@ import path from '../../lib/path.js'; import item_icon from '../../helpers/item_icon.js'; -import { owner_of_path } from '../../helpers/path_owner.js'; +import { is_owned_by_me, owner_of_path } from '../../helpers/path_owner.js'; import { invalidate_shared_roots } from '../../helpers/shared_access.js'; import { icons } from '../../helpers/actionIcons.js'; import { mode_label, options_for } from '../../helpers/share_modes.js'; @@ -29,6 +29,7 @@ import { has_direct_share, mark_item_shared, } from '../../helpers/sharedBadge.js'; +import { share_outcome } from '../../helpers/shareOutcome.js'; import { aggregateOwners, aggregateShares, missingPathsFor } from './shareAggregate.js'; const { html_encode } = window; @@ -39,6 +40,14 @@ const chevronIcon = ` item.path); const total = targets.length; + // Strictest item decides: one borrowed item withholds it for the rest. + const allow_manage = target_paths.every((p) => is_owned_by_me(p)); const is_multi = total > 1; // Nothing to share: an empty selection is a caller's mistake, not a dialog. if ( total === 0 ) return { close: () => {} }; @@ -159,7 +170,7 @@ export default function UIShareModal ({ items, path: item_path, name, owner, fse
- +
`; @@ -112,7 +123,11 @@ async function UIWindowShare (options) { $success.html(message).show(); }; + /** The access list as last drawn, which is what a share call changes. */ + let shown_shares = []; + const render = (shares) => { + shown_shares = Array.isArray(shares) ? shares : []; let rows = ''; // The owner's access comes from owning the item, so it can't be revoked rows += '
'; @@ -143,7 +158,7 @@ async function UIWindowShare (options) { } rows += ''; } @@ -176,13 +191,12 @@ async function UIWindowShare (options) { }); $(el_window).find('.share-recipient').val(''); $error.hide(); - // "Shared with" would claim access an invite does not grant. // `i18n()` encodes its replacements; encoding first would show the // entities to anyone whose address or username contains one. show_success( - created.some((share) => share.pending) - ? i18n('share_invited', { recipient }) - : i18n('share_shared_with', { recipient }), + i18n(SHARE_MESSAGE[share_outcome(created, shown_shares)], { + recipient, + }), ); invalidate_shared_roots(); await refresh(); diff --git a/src/gui/src/helpers/shareOutcome.js b/src/gui/src/helpers/shareOutcome.js new file mode 100644 index 0000000000..a8c30d03e0 --- /dev/null +++ b/src/gui/src/helpers/shareOutcome.js @@ -0,0 +1,47 @@ +/* + * Copyright (C) 2024-present Puter Technologies Inc. + * + * This file is part of Puter. + * + * Puter is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +/** + * What a share call actually did, so the dialog can say so. A backend that + * omits `isNew` reads as `shared`, which is what these dialogs said before it. + * + * @param {Array<{ pending?: boolean, isNew?: boolean, mode?: string, holder?: string|null }>} created + * @param {Array<{ holder?: string|null, mode?: string, inheritedFrom?: string|null }>} [before] + * @returns {'invited' | 'shared' | 'updated' | 'unchanged'} + */ +export const share_outcome = (created, before = []) => { + const list = Array.isArray(created) ? created.filter(Boolean) : []; + if ( list.some((share) => share.pending) ) return 'invited'; + if ( list.length === 0 || list.some((share) => share.isNew !== false) ) { + return 'shared'; + } + + // Matched on the resolved username rather than what was typed, so an email + // that belongs to a known account still finds their row. + const previous = (Array.isArray(before) ? before : []).find( + (share) => + share?.holder && + list.some((made) => made.holder === share.holder) && + ! share.inheritedFrom, + ); + if ( ! previous ) return 'unchanged'; + return list.some((made) => made.mode !== previous.mode) + ? 'updated' + : 'unchanged'; +}; diff --git a/src/gui/src/helpers/shareOutcome.test.js b/src/gui/src/helpers/shareOutcome.test.js new file mode 100644 index 0000000000..52913a67dc --- /dev/null +++ b/src/gui/src/helpers/shareOutcome.test.js @@ -0,0 +1,63 @@ +import { describe, expect, it } from 'vitest'; +import { share_outcome } from './shareOutcome.js'; + +describe('share_outcome', () => { + it('reports a first-time share', () => { + expect(share_outcome([{ holder: 'ann', mode: 'read', isNew: true }])) + .toBe('shared'); + }); + + it('reports an invite ahead of anything else', () => { + expect( + share_outcome([ + { recipientEmail: 'x@example.com', pending: true, isNew: true }, + ]), + ).toBe('invited'); + }); + + it('says nothing changed when the same access is granted twice', () => { + // The bug: this used to read as a fresh share. + expect( + share_outcome( + [{ holder: 'ann', mode: 'read', isNew: false }], + [{ holder: 'ann', mode: 'read' }], + ), + ).toBe('unchanged'); + }); + + it('says the level changed when the mode differs', () => { + expect( + share_outcome( + [{ holder: 'ann', mode: 'write', isNew: false }], + [{ holder: 'ann', mode: 'read' }], + ), + ).toBe('updated'); + }); + + it('matches on the resolved username, not what was typed', () => { + // The recipient was entered as an email; their row is keyed on the + // username the server resolved it to. + expect( + share_outcome( + [{ holder: 'ann', mode: 'write', isNew: false }], + [{ holder: 'ann', mode: 'read' }, { holder: 'bob', mode: 'read' }], + ), + ).toBe('updated'); + }); + + it('ignores an inherited row, which this call cannot have changed', () => { + expect( + share_outcome( + [{ holder: 'ann', mode: 'read', isNew: false }], + [{ holder: 'ann', mode: 'write', inheritedFrom: '/bob/Docs' }], + ), + ).toBe('unchanged'); + }); + + it('falls back to `shared` when the backend does not report isNew', () => { + // An older server, or the CDN SDK before this shipped. + expect(share_outcome([{ holder: 'ann', mode: 'read' }])).toBe('shared'); + expect(share_outcome([])).toBe('shared'); + expect(share_outcome(undefined)).toBe('shared'); + }); +}); diff --git a/src/gui/src/helpers/share_modes.js b/src/gui/src/helpers/share_modes.js index 8415da8dff..1c4d7f1f8e 100644 --- a/src/gui/src/helpers/share_modes.js +++ b/src/gui/src/helpers/share_modes.js @@ -48,11 +48,17 @@ export const mode_label = (mode) => { * unselectable placeholder, so a batch of mixed modes can't be read as one of * them, and picking a real mode is what levels them. * + * `allow_manage: false` drops "Can edit & share", bar a row already on it. + * * @param {string|null} current + * @param {{ allow_manage?: boolean }} [options] * @returns {string} HTML-safe markup */ -export const options_for = (current) => { - const listed = MODES +export const options_for = (current, { allow_manage = true } = {}) => { + const offered = MODES.filter( + (mode) => allow_manage || mode !== 'manage' || mode === current, + ); + const listed = offered .map( (mode) => ``, diff --git a/src/gui/src/helpers/share_modes.test.js b/src/gui/src/helpers/share_modes.test.js index a55fc554bc..b693b356f8 100644 --- a/src/gui/src/helpers/share_modes.test.js +++ b/src/gui/src/helpers/share_modes.test.js @@ -57,6 +57,19 @@ describe('options_for', () => { expect(html).not.toContain('