-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
shell: Don't automatically connect to remote machines on navigation #20826
base: main
Are you sure you want to change the base?
Conversation
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:
Marius said this could be a three-state option, which I agree with:
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. |
2fcb51b
to
2790704
Compare
40ef9ad
to
721f769
Compare
721f769
to
3d83b85
Compare
@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. |
e63798d
to
d777e51
Compare
@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)) |
d777e51
to
ffd92c3
Compare
e37b4cc
to
a943f94
Compare
@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? |
5751da0
to
a28eb5a
Compare
3499f26
to
e19df5d
Compare
b62a27f
to
6e91d23
Compare
6e91d23
to
4f90306
Compare
1f04970
to
57aeb51
Compare
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 |
c2f658a
to
c35a131
Compare
c35a131
to
b13ec87
Compare
componentWillUnmount() { | ||
window.trigger_connection_flow = null; |
There was a problem hiding this comment.
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.
try { | ||
await try2Connect(this.props.machines, machine.connection_string); | ||
connection_string = machine.connection_string; | ||
} catch (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
// continue with troubleshooting in the dialog | ||
connection_string = await this.showModal({ | ||
address: machine.address, | ||
template: codes[err.problem] || "change-port", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
if (port_index === -1) { | ||
host_id_port = address + ":22"; |
There was a problem hiding this comment.
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.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 4 added lines are not executed by any test.
cockpit.dbus(null, { bus: "internal" }).call("/config", "cockpit.Config", "GetString", | ||
["Session", "WarnBeforeConnecting"], []) | ||
.then(([result]) => { | ||
if (result == "false" || result == "no") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
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
:Demo: https://youtu.be/Un6k3DiukOg