Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow adding multiple ssh keys at once #1309

Closed
wants to merge 1 commit into from
Closed

Allow adding multiple ssh keys at once #1309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 32 additions & 14 deletions src/components/create-vm-dialog/createVmDialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,8 +752,7 @@ const UsersConfigurationRow = ({
);
};

// This method needs to be outside of component as re-render would create a new instance of debounce
const parseKey = debounce(500, (key, setKeyObject, setKeyInvalid) => {
const parseKey = (key, setKeyObject, setKeyInvalid) => {
if (isEmpty(key))
return;

Expand All @@ -776,22 +775,41 @@ const parseKey = debounce(500, (key, setKeyObject, setKeyInvalid) => {
setKeyInvalid(true);
console.warn("Could not validate the public key");
});
});
};

const handleSSHKeysValue = (value, idx, onChange, additem) => {
let index = idx;
for (const part of value.split("\n")) {
if (part.trim() === "") {
continue;
Comment on lines +783 to +784
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 added lines are not executed by any test. Details

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that the last character in the textarea can never be deleted.

}
if (index !== idx) {
additem();
Comment on lines +786 to +787
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 added lines are not executed by any test. Details

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true. The test with a \n between two SSH keys never creates a second item. The \n does not make it into the textrea with our set_input_text function on Chromium, one has to use \r ro produce a line break.

The test still passes since the check for expected keys is pretty sloppy and doesn't consider line breaks either.

}

onChange(index, "value", part);
index++;
}
};

const SshKeysRow = ({
id, item, onChange, idx, removeitem,
id, item, onChange, idx, removeitem, additem
}) => {
const [keyObject, setKeyObject] = useState();
const [keyInvalid, setKeyInvalid] = useState(false);
const keyValue = item.value || "";

const onChangeHelper = (value) => {
// Some users might want to input multiple keys into the same field
// While handling that in the future might be a nice user experience, now we only parse one key out of input
value = value.split(/\r?\n/)[0];

onChange(idx, "value", value);
parseKey(value, setKeyObject, setKeyInvalid);
};
useEffect(() => {
let timeoutId = null;
if (keyValue !== "" && !keyObject) {
timeoutId = setTimeout(() => parseKey(keyValue, setKeyObject, setKeyInvalid), 500);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original debounce comment still applies here -- this useEffect() re-runs on every input, so it creates multiple parallel timers and calls parseKey() for each one as well. Some debouncing still needs to happen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with a global debounce is that it is used for multiple components, so if you add two keys quickly only one will get parsed as the other call is debounced. So debouncing needs to happen per Component and we can't keep the debounce function in the component as every re-render it would be re-instantiated. So I believe the current approach with setTimeout debounces correctly, when a new as a new value would cause a re-render so clearTimeout() to be called?

If I'm wrong I'm happy to fix it if there is a better way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying that the original wrapping of parseKey() into debounce was right -- it's not, it's a hack which doesn't belong there at all. I'm saying that I dislike useEffect() here. It's too disconnected from the actual input that it wants to debounce, it duplicates debouncing logic, has setState() calls in it which are prone to recursion, is too hard to understand, and above of all, it really shouldn't be necessary here.

I mean, what is this trying to do? It's a controlled component, so onChange must immediately call setKeyValue() so that the UI doesn't lag. The expensive bit is the validation because of parseKey(), so the onChange handler should wrap that part into a debunce(). None of that requires useEffect()?

(That said: of course I may miss something here)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can not make onChange call setKeyValue simply because onChange will add a new row if multiple keys are added using additem(). That is the hard part, dynamically creating a new row component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds weird -- in JSX it's just a single TextArea. If this somehow multiplies the TextArea, this needs fixing -- either we shouldn't multiply it, or have a full-blown dynamic set of input lines where each one just has a single key, and ➕ 🗑️ buttons

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't keep the debounce function in the component as every re-render it would be re-instantiated.

We can use a hook to avoid the re-instantiation on re-render. Like this:

diff --git a/src/components/create-vm-dialog/createVmDialog.jsx b/src/components/create-vm-dialog/createVmDialog.jsx
index 8a2dd72..88ce41a 100644
--- a/src/components/create-vm-dialog/createVmDialog.jsx
+++ b/src/components/create-vm-dialog/createVmDialog.jsx
@@ -37,6 +37,7 @@ import { Spinner } from "@patternfly/react-core/dist/esm/components/Spinner";
 import { ExternalLinkAltIcon, TrashIcon } from '@patternfly/react-icons';
 
 import { DialogsContext } from 'dialogs.jsx';
+import { useInit } from "hooks.js";
 import cockpit from 'cockpit';
 import store from "../../store.js";
 import { MachinesConnectionSelector } from '../common/machinesConnectionSelector.jsx';
@@ -752,13 +753,16 @@ const UsersConfigurationRow = ({
     );
 };
 
-// This method needs to be outside of component as re-render would create a new instance of debounce
-const parseKey = debounce(500, (key, setKeyObject, setKeyInvalid) => {
-    if (isEmpty(key))
-        return;
+const useParsedKey = (value) => {
+    const [keyObject, setKeyObject] = useState();
+    const [keyInvalid, setKeyInvalid] = useState(false);
+
+    const parseKey = (key) => {
+        if (isEmpty(key))
+            return;
 
-    // Validate correctness of the key
-    return cockpit.spawn(["ssh-keygen", "-l", "-f", "-"], { err: "message" })
+        // Validate correctness of the key
+        return cockpit.spawn(["ssh-keygen", "-l", "-f", "-"], { err: "message" })
             .input(key)
             .then(() => {
                 setKeyInvalid(false);
@@ -776,21 +780,33 @@ const parseKey = debounce(500, (key, setKeyObject, setKeyInvalid) => {
                 setKeyInvalid(true);
                 console.warn("Could not validate the public key");
             });
-});
+    };
+
+    const debouncedParseKey = useInit(() => debounce(500, parseKey));
+    useInit(() => debouncedParseKey(value), [ value ]);
+
+    return [keyObject, keyInvalid];
+};
 
 const SshKeysRow = ({
-    id, item, onChange, idx, removeitem,
+    id, item, onChange, idx, removeitem, additem
 }) => {
-    const [keyObject, setKeyObject] = useState();
-    const [keyInvalid, setKeyInvalid] = useState(false);
+    const [keyObject, keyInvalid] = useParsedKey(item.value);
 
     const onChangeHelper = (value) => {
-        // Some users might want to input multiple keys into the same field
-        // While handling that in the future might be a nice user experience, now we only parse one key out of input
-        value = value.split(/\r?\n/)[0];
+        const lines = value.split(/\r?\n/);
 
+        // The first line is for us.
+        value = lines[0];
         onChange(idx, "value", value);
-        parseKey(value, setKeyObject, setKeyInvalid);
+
+        // If there are more lines, create additional entries for them.
+        for (let i = 1; i < lines.length; i++) {
+            if (lines[i].length > 0) {
+                additem();
+                onChange(idx + i, "value", lines[i]);
+            }
+        }
     };
 
     return (

}
return () => {
if (timeoutId !== null) {
clearTimeout(timeoutId);
}
};
}, [keyValue, keyObject]);

return (
<Grid id={id} key={id}>
Expand All @@ -805,9 +823,9 @@ const SshKeysRow = ({
: <FormGroup label={_("Public key")}
testdata={keyInvalid ? "key-invalid" : undefined}
fieldId='public-key'>
<TextArea value={item.value || ""}
<TextArea value={keyValue}
aria-label={_("Public SSH key")}
onChange={(_, value) => onChangeHelper(value)}
onChange={(_, value) => handleSSHKeysValue(value, idx, onChange, additem)}
rows="3" />
<FormHelper helperText={_("Keys are located in ~/.ssh/ and have a \".pub\" extension.")} />
</FormGroup>
Expand Down
2 changes: 1 addition & 1 deletion test/check-machines-create
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ class TestMachinesCreate(VirtualMachinesCase):
user_login="foo",
root_password="dogsaremybestfr13nds",
ssh_keys=[f"{rsakey} \n {dsakey}"],
expected_ssh_keys=[rsakey],
expected_ssh_keys=[rsakey, dsakey],
create_and_run=True))

# Try to create VM with invalid ssh key
Expand Down