fix(sharing): show original file owner in reshare panel#41640
fix(sharing): show original file owner in reshare panel#41640DeepDiver1975 wants to merge 1 commit into
Conversation
The share panel's reshare banner only displayed the direct sharer, not
the original file owner, even though the OCS reshare response already
carries uid_file_owner / displayname_file_owner. Surface those fields in
ShareItemModel and render "Shared with you by {owner} (on behalf of
{fileOwner})" when the file owner differs from the direct sharer, keeping
the previous single-owner wording otherwise (no regression).
Frontend-only change; the backend already exposes the data.
Fixes #30394
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
DeepDiver1975
left a comment
There was a problem hiding this comment.
🤖 Automated review by Claude Code review agent. (Note: this PR was generated by this same agent — treat this as a self-review, not independent validation.)
Because this PR was authored by the same automated fleet that is now reviewing it, the points below should be confirmed by a human maintainer and by CI rather than taken as an independent sign-off.
Overview
The change surfaces the original file owner in the reshare banner by reading the already-delivered uid_file_owner / displayname_file_owner OCS fields. It adds two model getters and branches the resharer view to render ... (on behalf of {fileOwner}) only when a distinct file owner exists. It is frontend-only with no API change, which is the right scope for #30394. The general approach is sound and the regression guard (keep old wording when owner == sharer) is the correct instinct.
Code quality / style
- The new getters mirror the existing
getReshareOwner*getters and are consistent with the file's style. Good. - The
render()branching is duplicated four-ways (group/user × distinct/not). It's readable but verbose. Computing the suffix once (e.g. anonBehalffragment) would halve the branching, though splitting a translatable sentence into fragments is itself an i18n anti-pattern — so the current full-sentence-per-branch approach is actually the more translator-correct choice. Keep it as is. - JSDoc
@propertyadditions to the typedef are a nice touch.
Correctness
hasDistinctFileOwneruses!_.isUndefined(fileOwner) && fileOwner !== getReshareOwner(). This compares uid values (uid_file_ownervsuid_owner), which is correct — uids are the stable identity, not display names. Good.- Gap —
null/ empty-string file owner. The guard only checks!_.isUndefined(...). If the backend ever sendsuid_file_owner: nullor""(distinct from the sharer's uid),hasDistinctFileOwnerbecomes true and the banner renders(on behalf of )with an empty/nulldisplay name. Recommend tightening to a truthiness check, e.g.!!fileOwner && fileOwner !== this.model.getReshareOwner(), and ideally also guard onfileOwnerDisplayNamebeing present. - Gap — current user IS the file owner.
render()already early-returns whengetReshareOwner() === OC.currentUser, but does NOT early-return when the file owner is the current user. Scenario: user A owns a file, user B reshares it back into a group that includes A. A would then see "...by B (on behalf of You/)". This is at minimum awkward UX and arguably wrong. Please confirm the intended behavior; consider suppressing the "on behalf of" clause whenfileOwner === OC.currentUser.
i18n
- Placeholder names (
{group},{owner},{fileOwner}) match the existing convention andt()substitution. Source strings are extracted by the external l10n tooling (Transifex) from thet('core', '...')calls — I confirmed the existing strings live incore/l10n/*.jsand are not hand-edited in PRs — so the two new strings will be picked up automatically. No manual l10n file changes are needed or expected here. - "(on behalf of {fileOwner})" is reasonably translator-friendly as it is a complete clause within a complete sentence (not a concatenated fragment). Acceptable.
XSS / escaping
- Confirmed
t()auto-escapes placeholder values by default (core/js/l10n.js~L144/157-158 →escapeHTML). The new calls do not passescape:false, so they escape exactly like the existing reshare strings. No new XSS surface, and the data was already delivered to this user. Good. - Note (pre-existing, not introduced here): the template interpolates
{{sharedByText}}with Handlebars double-brace, so the already-t()-escaped string is HTML-escaped a second time. The original single-owner path has the same double-escaping, so this is not a regression — but if a display name contained&, it would render as&. Worth a separate cleanup someday; not blocking.
Test quality
- The three specs assert the exact rendered text for: single-user distinct owner, group distinct owner, and owner==sharer regression guard. The group spec correctly relies on
share_with_displayname→getReshareWithDisplayName(). These are the right core assertions. - Missing coverage matching the gaps above:
uid_file_ownerabsent/undefined(the most common real case — ordinary non-reshare-of-resharer flow) → should fall back to the plain wording. This is the primary regression path and is not directly asserted.uid_file_ownerpresent butnull/""(the empty-suffix bug).displayname_file_ownermissing whileuid_file_ownerpresent.- file owner == current user.
- Critical process gap: the JS unit tests were NOT executed in the generating environment (karma/node_modules unavailable); only
node -csyntax checks were run. The assertions are therefore unverified. CI MUST run the karma suite green before merge — please do not merge on the strength of the syntax check alone.
Regression risk to ordinary shares
Low for the truly-ordinary case (no reshare → early return; reshare where owner==sharer → hasDistinctFileOwner false → unchanged text). The realistic risk is the null/empty uid_file_owner edge described above producing a malformed "(on behalf of )" string. Tightening the guard to a truthiness check closes that.
Summary / recommendation
Direction and scope are correct and the escaping is safe. Before merge: (1) tighten hasDistinctFileOwner to reject empty/null owner and missing display name; (2) decide and handle the "current user is the file owner" case; (3) add the missing spec cases; (4) ensure CI runs karma green. Request changes pending those, with (1) and (4) being the must-haves.
Summary
Fixes #30394.
The share panel's reshare banner showed only the direct sharer of a reshare, not the original file owner — even though the OCS reshare response already carries
uid_file_owner/displayname_file_owner. The frontend simply discarded those fields.This surfaces them:
core/js/shareitemmodel.js— addgetReshareFileOwner()/getReshareFileOwnerDisplayname()getters for the already-present fields.core/js/sharedialogresharerinfoview.js— when the file owner is known and differs from the direct sharer, renderShared with you by {owner} (on behalf of {fileOwner})(and the group-share equivalent). When the file owner equals the sharer or is absent, the previous wording is kept unchanged — no regression for ordinary shares.Frontend-only; no API change (the backend already exposes the data).
Tests
Added three cases to
core/js/tests/specs/sharedialogviewSpec.js:Notes
t()which auto-escapes placeholders, matching the existing escaping — no XSS regression, and no data is exposed that wasn't already delivered to this user.🤖 Generated with Claude Code