shell: Don't automatically connect to remote machines on navigation #20826

mvollmer · 2024-07-31T15:38:44Z

Shell: Extra warnings when connecting to remote hosts

Connecting to multiple hosts in a single Cockpit session allows all these hosts to access each other freely. Eventually Cockpit will not allow multiple connections, but in the mean time, we want to educate people better.

This warning can be disabled by including the following in /etc/cockpit/cockpit.conf:

[Session] 
WarnBeforeConnecting=false

Demo: https://youtu.be/Un6k3DiukOg

garrett · 2024-08-01T14:00:03Z

I was thinking of something more like this:

Then we can succinctly explain what can happen, provide a link with more information, and say this will show up once per session on the very first connection to any remote machine (instead of a "don't remind me"). If accepted, it would not show up again until the next Cockpit session.

The external page would explain it a little more in depth and mention the config file options, including:

Being able to toggle the host connection feature on and off completely
Being able to toggle off this warning message (which only matters if remote host connection is on)

Marius said this could be a three-state option, which I agree with:

Off (default for RHEL/CentOS)
On (default for non-RHEL/CentOS)
On without warnings (never default; an override to prevent the connection message that must be manually set)

This would prevent someone from connecting to a host machine without knowing the ramifications, but also allow them to work more-or-less how they currently do, with a workaround to skip the warning in some environments.

If we are indeed going to outright remove this feature, then we could even rephrase the modal and be explicit about the timeline in the external page, something like this:

These mockups are just examples of what we could do instead, and we'd want to revise the text and plan for the actual version where it would be removed.

pkg/shell/hosts_dialog.jsx

pkg/shell/indexes.jsx

mvollmer · 2024-09-03T06:35:55Z

@garrett, could you check https://www.youtube.com/watch?v=wL4VyE9tUb8 and tell me whether this goes in the right direction? Then I can work on making this nice and robust.

garrett · 2024-09-03T08:43:33Z

@mvollmer: I'm watching this video and wondering: Why is it different form the mockups and discussion we talked about? (See above comment #20826 (comment))

pkg/shell/indexes.jsx

mvollmer · 2024-09-04T08:35:31Z

I'm watching this video and wondering: Why is it different form the mockups and discussion we talked about?

@garrett, the difference I see is that the dialog is shown more often than what you described. Correct? The warning should be shown only for the very first connection in the session, right? So maximum number of times we show the warning per session is 1. (And reloading doesn't start a new session.)

What do you think of immediately opening the connection dialog when the URL already points to a remote machine when you login to the Cockpit session?

Any opinions about the "Not connected" placeholder page that you see when cancelling the connection dialog in that case? Should we keep it? Redesign it?

pkg/shell/hosts.jsx

mvollmer · 2024-09-11T10:52:53Z

There are lots more opportunities for cleanup, but I think this is fine for now. The next step would probably be to turn the whole shell into a React component and move the trigger_connection_flow thing into the top component so that it can be cleanly triggered from anywhere in the shell.

cockpituous · 2024-09-16T07:44:21Z

pkg/shell/hosts.jsx

+    componentWillUnmount() {
+        window.trigger_connection_flow = null;


These 2 added lines are not executed by any test.

cockpituous · 2024-09-16T07:44:21Z

pkg/shell/hosts.jsx

+            try {
+                await try2Connect(this.props.machines, machine.connection_string);
+                connection_string = machine.connection_string;
+            } catch (err) {


This added line is not executed by any test.

cockpituous · 2024-09-16T07:44:21Z

pkg/shell/hosts.jsx

+                // continue with troubleshooting in the dialog
+                connection_string = await this.showModal({
+                    address: machine.address,
+                    template: codes[err.problem] || "change-port",


This added line is not executed by any test.

cockpituous · 2024-09-16T07:44:21Z

pkg/shell/hosts_dialog.jsx

@@ -124,6 +196,8 @@ class AddMachine extends React.Component {
            old_machine = props.machines_ins.lookup(props.old_address);
        if (old_machine)
            color = this.rgb2Hex(old_machine.color);
+        if (old_machine && !old_machine.visible)
+            old_machine = null;


This added line is not executed by any test.

cockpituous · 2024-09-16T07:44:21Z

pkg/shell/hosts_dialog.jsx

+                    if (port_index === -1) {
+                        host_id_port = address + ":22";


These 2 added lines are not executed by any test.

cockpituous · 2024-09-16T07:44:21Z

pkg/shell/hosts_dialog.jsx

+        if (machine && machine.host_key && !machine.on_disk) {
+            conn_options['temp-session'] = false; // Compatibility option
+            conn_options.session = 'shared';
+            conn_options['host-key'] = machine.host_key;


These 4 added lines are not executed by any test.

cockpituous · 2024-09-16T07:44:21Z

pkg/shell/indexes.jsx

+    cockpit.dbus(null, { bus: "internal" }).call("/config", "cockpit.Config", "GetString",
+                                                 ["Session", "WarnBeforeConnecting"], [])
+            .then(([result]) => {
+                if (result == "false" || result == "no") {


This added line is not executed by any test.

mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Aug 1, 2024

mvollmer force-pushed the host-switcher-warnings branch 2 times, most recently from 2fcb51b to 2790704 Compare September 2, 2024 13:50

github-advanced-security bot found potential problems Sep 2, 2024

View reviewed changes

pkg/shell/hosts_dialog.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the host-switcher-warnings branch 2 times, most recently from 40ef9ad to 721f769 Compare September 2, 2024 14:18

github-advanced-security bot found potential problems Sep 2, 2024

View reviewed changes

pkg/shell/indexes.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the host-switcher-warnings branch from 721f769 to 3d83b85 Compare September 2, 2024 14:21

mvollmer requested a review from garrett September 3, 2024 06:34

mvollmer force-pushed the host-switcher-warnings branch from e63798d to d777e51 Compare September 3, 2024 07:43

mvollmer force-pushed the host-switcher-warnings branch from d777e51 to ffd92c3 Compare September 3, 2024 11:29

github-advanced-security bot found potential problems Sep 3, 2024

View reviewed changes

pkg/shell/indexes.jsx Fixed Show fixed Hide fixed

pkg/shell/indexes.jsx Fixed Show fixed Hide fixed

pkg/shell/indexes.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the host-switcher-warnings branch 7 times, most recently from e37b4cc to a943f94 Compare September 4, 2024 07:26

mvollmer force-pushed the host-switcher-warnings branch 3 times, most recently from 5751da0 to a28eb5a Compare September 4, 2024 18:10

mvollmer added the release-note label Sep 5, 2024

mvollmer force-pushed the host-switcher-warnings branch 3 times, most recently from 3499f26 to e19df5d Compare September 5, 2024 10:49

mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Sep 5, 2024

mvollmer force-pushed the host-switcher-warnings branch 4 times, most recently from b62a27f to 6e91d23 Compare September 11, 2024 09:16

mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Sep 11, 2024

github-advanced-security bot found potential problems Sep 11, 2024

View reviewed changes

pkg/shell/hosts.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the host-switcher-warnings branch from 6e91d23 to 4f90306 Compare September 11, 2024 09:50

mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Sep 11, 2024

mvollmer force-pushed the host-switcher-warnings branch from 1f04970 to 57aeb51 Compare September 11, 2024 10:50

mvollmer force-pushed the host-switcher-warnings branch 3 times, most recently from c2f658a to c35a131 Compare September 13, 2024 11:49

mvollmer marked this pull request as ready for review September 13, 2024 11:52

mvollmer mentioned this pull request Sep 13, 2024

shell: Navigate to new host after adding one #21004

Draft

1 task

shell: Require confirmation before connecting to remote machines

b13ec87

mvollmer force-pushed the host-switcher-warnings branch from c35a131 to b13ec87 Compare September 16, 2024 06:38

cockpituous reviewed Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shell: Don't automatically connect to remote machines on navigation #20826

shell: Don't automatically connect to remote machines on navigation #20826

mvollmer commented Jul 31, 2024 •

edited

Loading

garrett commented Aug 1, 2024

mvollmer commented Sep 3, 2024

garrett commented Sep 3, 2024

mvollmer commented Sep 4, 2024 •

edited

Loading

mvollmer commented Sep 11, 2024

cockpituous Sep 16, 2024

cockpituous Sep 16, 2024

cockpituous Sep 16, 2024

cockpituous Sep 16, 2024

cockpituous Sep 16, 2024

cockpituous Sep 16, 2024

cockpituous Sep 16, 2024

		componentWillUnmount() {
		window.trigger_connection_flow = null;

shell: Don't automatically connect to remote machines on navigation #20826

Are you sure you want to change the base?

shell: Don't automatically connect to remote machines on navigation #20826

Conversation

mvollmer commented Jul 31, 2024 • edited Loading

Shell: Extra warnings when connecting to remote hosts

garrett commented Aug 1, 2024

mvollmer commented Sep 3, 2024

garrett commented Sep 3, 2024

mvollmer commented Sep 4, 2024 • edited Loading

mvollmer commented Sep 11, 2024

cockpituous Sep 16, 2024

Choose a reason for hiding this comment

cockpituous Sep 16, 2024

Choose a reason for hiding this comment

cockpituous Sep 16, 2024

Choose a reason for hiding this comment

cockpituous Sep 16, 2024

Choose a reason for hiding this comment

cockpituous Sep 16, 2024

Choose a reason for hiding this comment

cockpituous Sep 16, 2024

Choose a reason for hiding this comment

cockpituous Sep 16, 2024

Choose a reason for hiding this comment

mvollmer commented Jul 31, 2024 •

edited

Loading

mvollmer commented Sep 4, 2024 •

edited

Loading