fix: disable rename action in password-protected folder view#13512
fix: disable rename action in password-protected folder view#13512ubermensch19 wants to merge 2 commits into
Conversation
Fixes owncloud#12365 - Added isLocationPublicActive check to useFileActionsRename - Prevents rename action from showing in public link context - Aligns with documented behavior that link files shouldn't be renamed from webUI
|
@LukasHirt PTAL, could you please review this and confirm whether this approach looks correct? |
|
LukasHirt
left a comment
LukasHirt
left a comment
There was a problem hiding this comment.
The rename action is now hidden in public link contexts, which aligns with the documented behavior that link files should not be manipulated from this view.
Could you please point me where is this documented?
|
Hey! The doc reference is in packages/web-app-password-protected-folders/README.md on line 59:
Though I realize it refers to the link file itself, not actions inside the modal. The main reason for this fix: renaming from inside the PPF modal (public link iframe) operates on the wrong context, causing the folder to fork instead of rename. Hiding it follows the same pattern as useFileActionsDelete (line 50) which already disables delete in public links. Let me know if you'd prefer a different approach! |
|
Alternative fix (more complex):
|
|
The main problem is with the shared context for PPF and public links in general. Since PPF "abuses" the public link functionality, simply preventing renaming breaks the regular links. Regarding backend changes, as of now, this is only frontend feature. This was done on purpose to leave backend out of this topic. If it would need to be touched, it would probably be better to do a real PPF implementation on the backend. |
DeepDiver1975
left a comment
DeepDiver1975
left a comment
There was a problem hiding this comment.
Thanks for tackling #12365, @ubermensch19 — the diagnosis (rename inside a PPF operating in the public-link context and diverging from the outer space) is correct, the changelog fragment is well-formed, and the delete-action analogy is a reasonable place to look. A few concerns before this can go in, though.
1. The condition over-disables rename in every public link, not just the PPF-from-inside case.
isLocationPublicActive(router, 'files-public-link') is true for the entire public-link file listing, for all resources and all recipients. Public links can be created with edit/write permissions, where renaming files and folders inside the link is a legitimate, working operation. This PR removes the Rename action for those users entirely. Note that the two sibling actions you'd expect to mirror — useFileActionsDelete and useFileActionsMove (cut) — deliberately keep files-public-link enabled (their guards return false only when the location is not public/spaces/...). So this change makes rename inconsistent with delete and move in public links, which is the opposite of the alignment the description claims.
2. The README citation is about the link file, not folder contents.
The README line ("Moving or renaming a link file is prevented by the webUI") refers to the .psec link file that represents the PPF — not to arbitrary files/folders viewed inside a public link. Blocking rename for the whole public-link view is a broader behavior change than that sentence supports.
3. This treats a symptom rather than the bug.
#12365 is a state-consistency bug: a rename inside the PPF succeeds on the server but the change isn't reflected in the outer space, and the UI shows a misleading state. Hiding the action avoids the misleading UI in the PPF case, but at the cost of disabling legitimate renames elsewhere, and it leaves the underlying propagation issue unaddressed. If the intent is specifically "don't allow renaming the PPF's own root folder from inside it," the guard should be scoped to that (e.g. the resource being the current/root folder of the public link), not the whole public-link context.
4. Tests — the description and the diff disagree.
The PR description says "Unit tests pass (17 tests)" and lists added unit tests, but the diff modifies only the source file and the changelog — useFileActionsRename.spec.ts is not changed. The existing isVisible test cases don't exercise a files-public-link router context, so there is no regression coverage asserting the new behavior (nor a test guarding against the over-disable above). Please add isVisible cases that mock the router location for both public-link and non-public contexts.
5. CI is red.
continuous-integration/drone/pr is currently failing and the PR is BLOCKED — needs to go green before merge regardless of the above.
Given the over-disable in editable public links (1–2), the symptom-vs-root-cause question (3), and the missing tests (4), I'm requesting changes rather than approving. Happy to re-review once the guard is scoped to the actual PPF-root case (or the underlying propagation is fixed) and tests cover it.
🤖 Automated review by Claude Code review agent (maintainer: @DeepDiver1975)
3 participants
Description
Disables the Rename action in the breadcrumb context menu when viewing a password-protected folder (public link context).
Related Issue
Fixes #12365
Motivation and Context
When renaming a folder from inside a password-protected folder view, the action operates in the public link context. This causes the rename to create a duplicate folder instead of renaming the original.
The fix aligns with:
useFileActionsDelete.tswhich already disables delete in public linksHow Has This Been Tested?
Types of changes