Support copy&pasting files with administrator privileges #880

tomasmatus · 2024-12-16T12:47:03Z

Allows administrator to paste files into directories owned by a different user.

Files now allow administrators to copy&paste files into directories owned by different users and select ownership of pasted files.

tomasmatus · 2024-12-16T12:50:29Z

Adjusted for design mockups from Garrett:

default selected value is the same as destination directory owner, other options include the current logged-in user, root, and if permissions matches on all files there also is an option to keep original permission (should it be different from any of the three mentioned earlier)

martinpitt

Thanks! I had an initial look for things which are independent of the visual design.

Wrt. that, I still don't understand why uploading and pasting look so dramatically different -- IMHO either both should use an Alert or a Modal. It's not clear to me how #551 handled that, but it surely must have the same problem of dropping a dragged file into a non-user directory? But that's primarily @garrett 's decision.

Thanks!

src/dialogs/copyPasteOwnership.tsx

src/menu.tsx

test/check-application

garrett · 2025-01-08T13:58:10Z

Issues:

Why is this a warning?
There is no explanation why this is showing up.

I was thinking something like this:

"Files being pasted have a different owner. By default, ownership will be changed to match the destination directory."

Also: The dropdown should have the "username:username" -> "username" collapsing, like the user:group being the same does elsewhere, to be consistent.

Anyway, this is so that there's a explanation of what's going on, and that the default is almost always what is wanted, so someone can just click "Paste" to continue.

If the ownership matches between the files and destination, then we can skip this dialog, provided there's some kind of visual change to show that files have been pasted (which should happen after this too). Nautilus has the file(s) highlighted afterward, for example.

garrett

Requested changes are above. (I really wish GitHub would fix their broken UI and allow for reviews to take place without having to look at the code. Not every review needs to look at the code.)

garrett · 2025-01-08T14:10:27Z

Here's an updated set of mockups with the various states.

It shows the collapsing when user and group are the same and it shows that the original owner has a special note in the dropdown options. It still defaults to the directory being pasted into, but it also quickly allows someone to select the original owner without having to think about it.

martinpitt

Thanks!

src/dialogs/copyPasteOwnership.tsx

martinpitt · 2025-01-16T07:01:05Z

src/dialogs/copyPasteOwnership.tsx

+    // also add current ownership if it is same for all files (shallow check)
+    const firstFile = clipboard.files[0];
+    const firstOwnerStr = `${firstFile.user}:${firstFile.group}`;
+    if (clipboard.files.every(file => file.user === firstFile.user && file.group === firstFile.group)) {
+        candidates.add(firstOwnerStr);
+    }


Hmm -- why the restriction to "same for all files"? It's harder to present in the UI (we can't give a definitive owner), but keeping the restrictions is especially useful if there are different owners -- i.e. you want to copy / move an entire hierarchy someplace else.

It shouldn't be too hard to add a ClipboardInfo.mixed_owners: boolean and showing that in the UI as "first owner (and others)" or so. Fine as follow-up material of course.

src/dialogs/copyPasteOwnership.tsx

test/check-application

martinpitt · 2025-01-28T11:55:20Z

@tomasmatus FYI, to fix the pixel tests, please push a rebase. (This has been on the pilot board for ages, but it's difficult)

src/app.tsx

martinpitt · 2025-01-29T09:19:22Z

Thanks for #921! This needs a rebase now.

martinpitt

Thanks! Next round. This gets close enough that @garrett can do the final UI review in parallel.

src/dialogs/copyPasteOwnership.tsx

test/check-application

Allows administrator to paste files into directories owned by a different user.

martinpitt

Sorry for the bad useInit() advice in my previous round. There's a little cleanup, but it's harmless. Thanks for bearing with me -- this is non-trivial file handling with tons of corner cases, and it's genuinely hard.

Looks good to me now! @garrett can you please do the final design review?

martinpitt · 2025-02-04T07:39:38Z

src/dialogs/copyPasteOwnership.tsx

+    // @ts-expect-error superuser.js is not typed
+    useEvent(superuser, "changed");


I just realized that I misread that the previous time -- this modal is a short-lived thing, it's unlikely that the superuser status changes underneath it. However, modals don't actually block the superuser widget.

However, this is still a no-op -- with that, the modal gets re-rendered on superuser changes, but useInit() doesn't re-run. I'd say let's not make this overly complicated and just drop this useEvent().

garrett · 2025-02-04T13:44:11Z

This seems mostly fine to me. I did spot a few issues:

It gives me the option to paste in a directory where I do not have access. It does result in an error, but it would be nice if it could catch it first, either by removing the option for pasting as a specific owner or by at least showing an error helper text when the option is selected.
The dropdown styling has a regression (there's no padding to the left), but this is not related to the PR.

I think this is good to land as-is, and we should consider # 1 as a possible followup and look to our PF overrides for fixing # 2 again.

garrett · 2025-02-04T13:55:29Z

Oh, it looks like PF6 had yet another regression on form select again:

https://v5-archive.patternfly.org/components/forms/form-select

While custom selects are not balanced correctly (they have questionable design issues), they at least have some space in front:

https://v5-archive.patternfly.org/components/menus/select

Form selects are just wrong and inconsistent. Again.

Zoomed in comparison to make it more obvious; the top is form select, the bottom is custom select:

But this is irrelevant for this PR; it's not a blocker. It's something that should be addressed again in an overide to fix the left padding on form-select specifically, to be consistent with the custom select (and input, and other form related elements, etc.)

The review comments are follow up or unrelated PF regressions.

martinpitt · 2025-02-07T13:21:36Z

It gives me the option to paste in a directory where I do not have access. It does result in an error, but it would be nice if it could catch it first

This is the whole raison d'être of this PR. It tries to do exactly that (with test -w and such), so if that is broken, this needs to be looked at. I retract my +1 until that happens. Thanks for spotting! @tomasmatus can you reproduce that? If not, @garrett do you have some more precise steps?

see above

jelly · 2025-02-07T13:34:32Z

src/menu.tsx

+
+    try {
+        // check for write access to destination directory
+        await cockpit.spawn(["test", "-w", path]);


This gives an empty error dialog when it fails, because there is an empty Exception. So ideally this is moved to a separate try/catch or you make the catch else aware of that

Reproducer:

As limited user, copy a file from your home and paste it in /home or /

Thanks! so that's the part which we need to fix in this PR.

jelly · 2025-02-07T15:22:20Z

It gives me the option to paste in a directory where I do not have access. It does result in an error, but it would be nice if it could catch it first

This is the whole raison d'être of this PR. It tries to do exactly that (with test -w and such), so if that is broken, this needs to be looked at. I retract my +1 until that happens. Thanks for spotting! @tomasmatus can you reproduce that? If not, @garrett do you have some more precise steps?

I believe the step is:

Log in as limited user
Copy a file from your home dir
Paste it in /
The menu entry is not disabled

This was already the case before the PR. We discussed in standup to have the access bits available as fsinfo but that takes some time. Created cockpit-project/cockpit#21596 for discussion there.

martinpitt · 2025-02-07T16:04:33Z

OK, so the error as non-admin user isn't a regression. Let's just fix the empty error message then.

tomasmatus · 2025-02-10T10:19:46Z

I don't remember why I needed the test -w call. It might have been because I did both test -r for source files and test -w for destination directory where time-of-check vs time-of-use error was brought up. I feel like it's fine to have cp fail and then open the admin modal if it does and user has admin perms.

cockpituous · 2025-02-10T10:27:29Z

src/common.ts

+    if (window.debugging === "all" || window.debugging?.includes("files")) {
+        console.debug("files:", ...args);


These 2 added lines are not executed by any test.

cockpituous · 2025-02-10T10:27:29Z

src/dialogs/copyPasteOwnership.tsx

+        } catch (ex) {
+            console.warn(`Failed to clean up copied files in ${dstPath}`, ex);


These 2 added lines are not executed by any test.

cockpituous · 2025-02-10T10:27:30Z

src/dialogs/copyPasteOwnership.tsx

+
+    // add option to keep the same permissions on mixed permissions files
+    if (uniqueOwners.length > 1) {
+        const dots = uniqueOwners.length > 2 ? ", ..." : "";


This added line is not executed by any test.

cockpituous · 2025-02-10T10:27:30Z

src/dialogs/copyPasteOwnership.tsx

+          position="top"
+          title={_("Paste as owner")}
+          isOpen
+          onClose={() => dialogResult.resolve()}


This added line is not executed by any test.

martinpitt

Thanks for fixing this!

martinpitt requested changes Dec 19, 2024

View reviewed changes

tomasmatus force-pushed the copy-paste-nonowned branch from 4cf4968 to 5323e45 Compare January 3, 2025 11:05

garrett previously requested changes Jan 8, 2025

View reviewed changes

tomasmatus force-pushed the copy-paste-nonowned branch 5 times, most recently from 367e694 to 6633d7c Compare January 14, 2025 12:32

tomasmatus requested review from martinpitt and garrett January 14, 2025 12:35

martinpitt previously approved these changes Jan 16, 2025

View reviewed changes

tomasmatus dismissed martinpitt’s stale review via 957f0bb January 22, 2025 11:39

tomasmatus force-pushed the copy-paste-nonowned branch from 957f0bb to 51e5eda Compare January 27, 2025 14:00

tomasmatus requested a review from martinpitt January 27, 2025 14:33

martinpitt reviewed Jan 28, 2025

View reviewed changes

src/app.tsx Outdated Show resolved Hide resolved

jelly added the release-note label Jan 29, 2025

tomasmatus force-pushed the copy-paste-nonowned branch from 51e5eda to d8bd6c3 Compare January 29, 2025 09:50

martinpitt requested changes Jan 30, 2025

View reviewed changes

tomasmatus force-pushed the copy-paste-nonowned branch from d8bd6c3 to dc5296d Compare February 3, 2025 13:39

Support copy&pasting files with administrator privileges

0dfd742

Allows administrator to paste files into directories owned by a different user.

tomasmatus force-pushed the copy-paste-nonowned branch from dc5296d to 0dfd742 Compare February 3, 2025 13:45

tomasmatus requested a review from martinpitt February 3, 2025 16:57

martinpitt previously approved these changes Feb 4, 2025

View reviewed changes

jelly reviewed Feb 7, 2025

View reviewed changes

jelly mentioned this pull request Feb 7, 2025

Rename "Open text file" to "Open file in editor" #729

Open

do not show alert with empty error message

2b17a99

tomasmatus force-pushed the copy-paste-nonowned branch from 1169c4f to 2b17a99 Compare February 10, 2025 10:19

cockpituous reviewed Feb 10, 2025

View reviewed changes

tomasmatus requested a review from martinpitt February 10, 2025 12:40

martinpitt approved these changes Feb 10, 2025

View reviewed changes

tomasmatus merged commit 5497760 into cockpit-project:main Feb 10, 2025
28 checks passed

martinpitt mentioned this pull request Feb 12, 2025

Fix races in TestFiles.testCopyPaste #947

Merged

allisonkarlitskaya removed the release-note label Feb 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support copy&pasting files with administrator privileges #880

Support copy&pasting files with administrator privileges #880

tomasmatus commented Dec 16, 2024 •

edited

Loading

tomasmatus commented Dec 16, 2024 •

edited

Loading

martinpitt left a comment

garrett commented Jan 8, 2025

garrett left a comment

garrett commented Jan 8, 2025

martinpitt left a comment

martinpitt Jan 16, 2025

martinpitt commented Jan 28, 2025

martinpitt commented Jan 29, 2025

martinpitt left a comment

martinpitt left a comment

martinpitt Feb 4, 2025

garrett commented Feb 4, 2025

garrett commented Feb 4, 2025

martinpitt commented Feb 7, 2025 •

edited

Loading

jelly Feb 7, 2025

martinpitt Feb 7, 2025

jelly commented Feb 7, 2025 •

edited

Loading

martinpitt commented Feb 7, 2025

tomasmatus commented Feb 10, 2025

cockpituous Feb 10, 2025

cockpituous Feb 10, 2025

cockpituous Feb 10, 2025

cockpituous Feb 10, 2025

martinpitt left a comment

		// @ts-expect-error superuser.js is not typed
		useEvent(superuser, "changed");

		if (window.debugging === "all" \|\| window.debugging?.includes("files")) {
		console.debug("files:", ...args);

		} catch (ex) {
		console.warn(`Failed to clean up copied files in ${dstPath}`, ex);

Support copy&pasting files with administrator privileges #880

Support copy&pasting files with administrator privileges #880

Conversation

tomasmatus commented Dec 16, 2024 • edited Loading

tomasmatus commented Dec 16, 2024 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

garrett commented Jan 8, 2025

garrett left a comment

Choose a reason for hiding this comment

garrett commented Jan 8, 2025

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Jan 28, 2025

martinpitt commented Jan 29, 2025

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrett commented Feb 4, 2025

garrett commented Feb 4, 2025

martinpitt commented Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Feb 7, 2025 • edited Loading

martinpitt commented Feb 7, 2025

tomasmatus commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

tomasmatus commented Dec 16, 2024 •

edited

Loading

tomasmatus commented Dec 16, 2024 •

edited

Loading

martinpitt commented Feb 7, 2025 •

edited

Loading

jelly commented Feb 7, 2025 •

edited

Loading