diff --git a/src/app.tsx b/src/app.tsx index b37c53fd..d2fb0b50 100644 --- a/src/app.tsx +++ b/src/app.tsx @@ -46,7 +46,8 @@ interface Alert { key: string, title: string, variant: AlertVariant, - detail?: string, + detail?: string | React.ReactNode, + actionLinks?: React.ReactNode } export interface FolderFileInfo extends FileInfo { @@ -56,12 +57,15 @@ export interface FolderFileInfo extends FileInfo { } interface FilesContextType { - addAlert: (title: string, variant: AlertVariant, key: string, detail?: string) => void, + addAlert: (title: string, variant: AlertVariant, key: string, detail?: string | React.ReactNode, + actionLinks?: React.ReactNode) => void, + removeAlert: (key: string) => void, cwdInfo: FileInfo | null, } export const FilesContext = React.createContext({ addAlert: () => console.warn("FilesContext not initialized"), + removeAlert: () => console.warn("FilesContext not initialized"), cwdInfo: null, } as FilesContextType); @@ -163,14 +167,23 @@ export const Application = () => { if (loading) return ; - const addAlert = (title: string, variant: AlertVariant, key: string, detail?: string) => { - setAlerts(prevAlerts => [...prevAlerts, { title, variant, key, ...detail && { detail }, }]); + const addAlert = (title: string, variant: AlertVariant, key: string, detail?: string | React.ReactNode, + actionLinks?: React.ReactNode) => { + setAlerts(prevAlerts => [ + ...prevAlerts, { + title, + variant, + key, + ...detail && { detail }, + ...actionLinks && { actionLinks }, + } + ]); }; const removeAlert = (key: string) => setAlerts(prevAlerts => prevAlerts.filter(alert => alert.key !== key)); return ( - + {alerts.map(alert => ( @@ -184,6 +197,7 @@ export const Application = () => { onClose={() => removeAlert(alert.key)} /> } + actionLinks={alert.actionLinks} key={alert.key} > {alert.detail} diff --git a/src/upload-button.scss b/src/upload-button.scss index 8eb806be..c20950a4 100644 --- a/src/upload-button.scss +++ b/src/upload-button.scss @@ -79,3 +79,6 @@ button.cancel-button { } } +.ct-grey-text { + color: var(--pf-v5-global--Color--200); +} diff --git a/src/upload-button.tsx b/src/upload-button.tsx index 66342169..38c151f3 100644 --- a/src/upload-button.tsx +++ b/src/upload-button.tsx @@ -19,7 +19,7 @@ import React, { useState, useRef } from "react"; -import { AlertVariant } from "@patternfly/react-core/dist/esm/components/Alert"; +import { AlertVariant, AlertActionLink } from "@patternfly/react-core/dist/esm/components/Alert"; import { Button } from "@patternfly/react-core/dist/esm/components/Button"; import { Checkbox } from "@patternfly/react-core/dist/esm/components/Checkbox"; import { Divider } from "@patternfly/react-core/dist/esm/components/Divider"; @@ -29,14 +29,18 @@ import { Progress } from "@patternfly/react-core/dist/esm/components/Progress"; import { Flex, FlexItem } from "@patternfly/react-core/dist/esm/layouts/Flex"; import { TrashIcon } from "@patternfly/react-icons"; -import cockpit from "cockpit"; +import cockpit, { BasicError } from "cockpit"; import type { FileInfo } from "cockpit/fsinfo"; import { upload } from "cockpit-upload-helper"; import { DialogResult, useDialogs } from "dialogs"; +import { superuser } from "superuser"; import * as timeformat from "timeformat"; import { fmt_to_fragments } from "utils"; -import { useFilesContext } from "./app.tsx"; +import { FolderFileInfo, useFilesContext } from "./app.tsx"; +import { permissionShortStr } from "./common.ts"; +import { edit_permissions } from "./dialogs/permissions.tsx"; +import { get_owner_candidates } from "./ownership.tsx"; import "./upload-button.scss"; @@ -48,6 +52,26 @@ interface ConflictResult { applyToAll: boolean; } +const UploadedFilesList = ({ + files, + modes, + owner, +}: { + files: File[], + modes: number[], + owner: string, +}) => { + cockpit.assert(modes.length !== 0, "modes cannot be empty"); + const permission = permissionShortStr(modes[0]); + const title = files.length === 1 ? files[0].name : cockpit.format(_("$0 files"), files.length); + return ( + <> + {title} + {owner && {cockpit.format(_("Uploaded as $0, $1"), owner, permission)}} + > + ); +}; + const FileConflictDialog = ({ path, file, @@ -131,7 +155,7 @@ export const UploadButton = ({ path: string, }) => { const ref = useRef(null); - const { addAlert, cwdInfo } = useFilesContext(); + const { addAlert, removeAlert, cwdInfo } = useFilesContext(); const dialogs = useDialogs(); const [showPopover, setPopover] = React.useState(false); const [uploadedFiles, setUploadedFiles] = useState<{[name: string]: @@ -155,7 +179,16 @@ export const UploadButton = ({ cockpit.assert(event.target.files, "not an ?"); cockpit.assert(cwdInfo?.entries, "cwdInfo.entries is undefined"); let next_progress = 0; - const toUploadFiles = []; + let owner = null; + const toUploadFiles: File[] = []; + + // When we are superuser upload as the owner of the directory and allow + // the user to later change ownership if required. + const user = await cockpit.user(); + if (superuser.allowed && cwdInfo) { + const candidates = get_owner_candidates(user, cwdInfo); + owner = candidates[0]; + } const resetInput = () => { // Reset input field in the case a download was cancelled and has to be re-uploaded @@ -207,9 +240,11 @@ export const UploadButton = ({ window.addEventListener("beforeunload", beforeUnloadHandler); const cancelledUploads = []; + const fileModes: number[] = []; await Promise.allSettled(toUploadFiles.map(async (file: File) => { - const destination = path + file.name; + let destination = path + file.name; const abort = new AbortController(); + let options = { }; setUploadedFiles(oldFiles => { return { @@ -218,6 +253,49 @@ export const UploadButton = ({ }; }); + if (owner !== null) { + destination = `${path}.${file.name}.tmp`; + // The cockpit.file() API does not support setting an owner/group when uploading a new + // file with fsreplace1. This requires a re-design of the Files API: + // https://issues.redhat.com/browse/COCKPIT-1215 + // + // For now we create an empty file using fsreplace1 and give it the proper ownership and using that tag + // upload the to be uploaded file. This prevents uploading with the wrong ownership. + // + // To support changing permissions after upload with our change permissions dialog we obtain the + // file mode using `stat` as fsreplace1 does not report this back except via the `tag` which is not + // a stable interface. + try { + await cockpit.file(destination, { superuser: "try" }).replace(""); + await cockpit.spawn(["chown", owner, destination], { superuser: "try" }); + await cockpit.file(destination, { superuser: "try" }).read() + .then((((_content: string, tag: string) => { + options = { superuser: "try", tag }; + }) as any /* eslint-disable-line @typescript-eslint/no-explicit-any */)); + const stat = await cockpit.spawn(["stat", "--format", "%a", destination], { superuser: "try" }); + fileModes.push(Number.parseInt(stat.trimEnd(), 8)); + } catch (exc) { + const err = exc as BasicError; + console.warn("Cannot set initial file permissions", err.toString()); + addAlert(_("Failed"), AlertVariant.warning, "upload", err.toString()); + + try { + await cockpit.file(destination, { superuser: "require" }).replace(null); + } catch (exc) { + console.warn(`Unable to cleanup file: ${destination}, err: ${exc}`); + } + + cancelledUploads.push(file); + setUploadedFiles(oldFiles => { + const copy = { ...oldFiles }; + delete copy[file.name]; + return copy; + }); + + return; + } + } + try { await upload(destination, file, (progress) => { const now = performance.now(); @@ -231,10 +309,34 @@ export const UploadButton = ({ [file.name]: { ...oldFile, progress }, }; }); - }, abort.signal); - // TODO: pass { superuser: try } depending on directory owner + }, abort.signal, options); + + if (owner !== null) { + try { + await cockpit.spawn(["mv", destination, path + file.name], + { superuser: "require" }); + } catch (exc) { + console.warn("Unable to move file to final destination", exc); + addAlert(_("Upload error"), AlertVariant.danger, "upload-error", + _("Unable to move uploaded file to final destination")); + try { + await cockpit.file(destination, { superuser: "require" }).replace(null); + } catch (exc) { + console.warn(`Unable to cleanup file: ${destination}, err: ${exc}`); + } + } + } } catch (exc) { cockpit.assert(exc instanceof Error, "Unknown exception type"); + + // Clean up touched file + if (owner) { + try { + await cockpit.file(destination, { superuser: "require" }).replace(null); + } catch (exc) { + console.warn(`Unable to cleanup file: ${destination}, err: ${exc}`); + } + } if (exc instanceof DOMException && exc.name === 'AbortError') { addAlert(_("Cancelled"), AlertVariant.warning, "upload", cockpit.format(_("Cancelled upload of $0"), file.name)); @@ -256,7 +358,43 @@ export const UploadButton = ({ // If all uploads are cancelled, don't show an alert if (cancelledUploads.length !== toUploadFiles.length) { - addAlert(_("Upload complete"), AlertVariant.success, "upload-success", _("Successfully uploaded file(s)")); + const title = cockpit.ngettext(_("File uploaded"), _("Files uploaded"), toUploadFiles.length); + const key = window.btoa(toUploadFiles.join("")); + let description; + let action; + + if (owner !== null) { + description = ( + + ); + action = ( + { + removeAlert(key); + const [user, group] = owner.split(':'); + const uploadedFiles: FolderFileInfo[] = toUploadFiles.map((file, idx) => { + return { + name: file.name, + to: null, + category: null, + user, + group, + mode: fileModes[idx] + }; + }); + edit_permissions(dialogs, uploadedFiles, path); + }} + > + {_("Change permissions")} + + ); + } + + addAlert(title, AlertVariant.success, key, description, action); } }; diff --git a/test/check-application b/test/check-application index d9fb732f..37f330d1 100755 --- a/test/check-application +++ b/test/check-application @@ -10,6 +10,7 @@ import shlex import subprocess import tempfile from pathlib import Path +from typing import List, Optional # import Cockpit's machinery for test VMs and its browser test API import testlib @@ -2051,6 +2052,31 @@ class TestFiles(testlib.MachineCase): def dir_file_count(directory: str) -> int: return int(m.execute(f"find {directory} -mindepth 1 -maxdepth 1 | wc -l").strip()) + def assert_upload_alert(files: List[str], owner_group: Optional[str] = None, *, + close: bool = True, pixel_test: bool = False) -> None: + title = "" + description = "" + if len(files) > 1: + title = "Files uploaded" + description = f"{len(files)} files" + else: + title = "File uploaded" + description = files[0] + + if owner_group: + # Default umask should be 022 + description += f"Uploaded as {owner_group}, rw- r-- r--" + + b.wait_in_text(".pf-v5-c-alert__title", title) + b.wait_text(".pf-v5-c-alert__description", description) + + if pixel_test: + b.assert_pixels(".pf-v5-c-alert.pf-m-success", "multiple-upload-alert-success") + + if close: + b.click(".pf-v5-c-alert__action button") + b.wait_not_present(".pf-v5-c-alert__action") + with tempfile.TemporaryDirectory() as tmpdir: # Test cancelling of upload big_file = str(Path(tmpdir) / "bigfile.img") @@ -2129,9 +2155,7 @@ class TestFiles(testlib.MachineCase): with b.wait_timeout(30): b.wait(lambda: int(m.execute(f"ls {dest_dir} | wc -l").strip()) == len(files)) - b.wait_in_text(".pf-v5-c-alert__description", "Successfully uploaded file") - b.click(".pf-v5-c-alert__action button") - b.wait_not_present(".pf-v5-c-alert__action") + assert_upload_alert(files, "admin:admin", pixel_test=True) # Verify ownership filename = os.path.basename(files[0]) @@ -2164,10 +2188,7 @@ class TestFiles(testlib.MachineCase): b.wait_in_text("h1.pf-v5-c-modal-box__title", f"Replace file {filename}?") b.click("button.pf-m-warning:contains('Replace')") b.wait_not_present(".pf-v5-c-modal-box") - - b.wait_in_text(".pf-v5-c-alert__description", "Successfully uploaded file") - b.click(".pf-v5-c-alert__action button") - b.wait_not_present(".pf-v5-c-alert__action") + assert_upload_alert([os.path.basename(files[0])], "admin:admin") self.assertEqual(m.execute(f"cat {dest_dir}/{filename}"), subprocess.check_output(["cat", files[0]]).decode()) @@ -2211,10 +2232,7 @@ class TestFiles(testlib.MachineCase): b.click("button.pf-m-warning:contains('Replace')") b.wait_not_present(".pf-v5-c-modal-box") - b.wait_in_text(".pf-v5-c-alert__description", "Successfully uploaded file") - b.click(".pf-v5-c-alert__action button") - b.wait_not_present(".pf-v5-c-alert__action") - + assert_upload_alert(files, "admin:admin") verify_uploaded_files(changed=True) # Upload one file new file which is uploaded automatically @@ -2235,7 +2253,9 @@ class TestFiles(testlib.MachineCase): b.click("button.pf-m-secondary:contains('Keep original')") b.wait_not_present(".pf-v5-c-modal-box") - b.wait_in_text(".pf-v5-c-alert__description", "Successfully uploaded file") + b.wait_in_text(".pf-v5-c-alert__title", "File uploaded") + b.wait_in_text(".pf-v5-c-alert__description", "newfile.txt") + b.wait_in_text(".pf-v5-c-alert__description", "Uploaded as admin:admin") b.click(".pf-v5-c-alert__action button") b.wait_not_present(".pf-v5-c-alert__action") @@ -2259,12 +2279,59 @@ class TestFiles(testlib.MachineCase): b.click("button.pf-m-secondary:contains('Keep original')") b.wait_not_present(".pf-v5-c-modal-box") - b.wait_in_text(".pf-v5-c-alert__description", "Successfully uploaded file") - b.click(".pf-v5-c-alert__action button") - b.wait_not_present(".pf-v5-c-alert__action") + assert_upload_alert([os.path.basename(files[-1])], "admin:admin") self.assertEqual(m.execute(f"cat {dest_dir}/{filename}"), "new content") + # As administrator upload in testuser and change ownership to root:root + + m.execute("useradd --create-home testuser") + b.go('/files#/?path=/home/testuser') + b.wait_not_present('.pf-v5-c-empty-state') + + testuser_file = str(Path(tmpdir) / "testuser.txt") + test_content = "testdata" + with open(testuser_file, "w") as fp: + fp.write(test_content) + + testuser_filename = os.path.basename(testuser_file) + b.click("#upload-file-btn") + b.upload_files("#upload-file-btn + input[type='file']", [testuser_file]) + assert_upload_alert([testuser_filename], "testuser:testuser", close=False) + self.assert_owner(f'/home/testuser/{testuser_filename}', 'testuser:testuser') + contents = m.execute(f"cat /home/testuser/{testuser_filename}") + self.assertEqual(contents, test_content) + + b.click(".pf-v5-c-alert__action-group button") + b.wait_not_present(".pf-v5-c-alert__action") + b.wait_in_text(".pf-v5-c-modal-box__title-text", testuser_filename) + b.wait_val("#edit-permissions-owner", "testuser") + b.wait_val("#edit-permissions-group", "testuser") + b.wait_val("#edit-permissions-owner-access", "read-write") + b.wait_val("#edit-permissions-group-access", "read-only") + b.wait_val("#edit-permissions-other-access", "read-only") + + b.select_from_dropdown("#edit-permissions-owner", "root") + b.select_from_dropdown("#edit-permissions-group", "root") + b.click("button.pf-m-primary") + b.wait_not_present(".pf-v5-c-modal-box") + + self.assert_owner(f'/home/testuser/{testuser_filename}', 'root:root') + + # Uploading to an immutable dir fails to create a temp file, check that it is cleaned up + + m.execute("chattr +i /home/testuser") + self.addCleanup(m.execute, "chattr -i /home/testuser") + b.click("#upload-file-btn") + b.upload_files("#upload-file-btn + input[type='file']", [files[-1]]) + + b.go(f'/files#/?path={dest_dir}') + b.wait_not_present('.pf-v5-c-empty-state') + b.wait_in_text(".pf-v5-c-alert__title", "Failed") + b.wait_in_text(".pf-v5-c-alert__description", "Not permitted to perform this action") + b.click(".pf-v5-c-alert__action button") + b.wait_not_present(".pf-v5-c-alert__action") + # Non-admin session b.drop_superuser() m.execute(f"rm {dest_dir}/{filename}; touch {dest_dir}/update.txt") @@ -2275,7 +2342,7 @@ class TestFiles(testlib.MachineCase): b.click("#upload-file-btn") b.upload_files("#upload-file-btn + input[type='file']", [files[-1]]) - b.wait_in_text(".pf-v5-c-alert__description", "Successfully uploaded file") + b.wait_in_text(".pf-v5-c-alert__title", "File uploaded") b.click(".pf-v5-c-alert__action button") b.wait_not_present(".pf-v5-c-alert__action") diff --git a/test/reference b/test/reference index 706094fb..448e946b 160000 --- a/test/reference +++ b/test/reference @@ -1 +1 @@ -Subproject commit 706094fb506668547410343bc933207ee9075d93 +Subproject commit 448e946b659a233394b5825aefdcfaa08c916a0a
{title}
{cockpit.format(_("Uploaded as $0, $1"), owner, permission)}