Shell: Extra warnings when connecting to remote hosts #20826
Conversation
mvollmer
added
the
no-test
|
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. |
mvollmer
force-pushed
the
host-switcher-warnings
branch
2 times, most recently
from
2fcb51b
to
2790704
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mvollmer
force-pushed
the
host-switcher-warnings
branch
2 times, most recently
from
40ef9ad
to
721f769
Compare
mvollmer
force-pushed
the
host-switcher-warnings
branch
from
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. |
mvollmer
force-pushed
the
host-switcher-warnings
branch
from
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)) |
mvollmer
force-pushed
the
host-switcher-warnings
branch
from
d777e51
to
ffd92c3
Compare
mvollmer
force-pushed
the
host-switcher-warnings
branch
7 times, most recently
from
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? |
mvollmer
force-pushed
the
host-switcher-warnings
branch
3 times, most recently
from
5751da0
to
a28eb5a
Compare
mvollmer
force-pushed
the
host-switcher-warnings
branch
3 times, most recently
from
3499f26
to
e19df5d
Compare
mvollmer
force-pushed
the
host-switcher-warnings
branch
from
c35a131
to
b13ec87
Compare
mvollmer
force-pushed
the
host-switcher-warnings
branch
from
b13ec87
to
f90684d
Compare
mvollmer
added
the
blocked
There was a problem hiding this comment.
This looks mostly great. Thanks!
However: After I added a new host, I got the connect warning dialog and then the key dialog, all as expected. However, it didn't switch to that remote host. Instead, it stayed on the original host and the switcher persisted.
I would have expected it to switch to the new host and the switcher to go away at the same time.
Are you splitting that out into this other PR?
(If that's the case, I can approve this one and check out that one to make it works as expected.)
(I thought I spotted some regressions, like the host switcher not having a hover state, but it looks like that in main too, so I guess we picked up that regression elsewhere, in the past. I guess after we land the rework and reimplement the shell's UI, it'll be automatically fixed. Otherwise, I could send up a PR to fix the CSS outside of your changes.)
mvollmer
removed
the
blocked
mvollmer
force-pushed
the
host-switcher-warnings
branch
2 times, most recently
from
8020e84
to
d370165
Compare
mvollmer
changed the title
Yes, that's the current behavior of the shell. I have included #21004 here now to jump to a newly added host immediately. |
mvollmer
added
the
no-test
mvollmer
force-pushed
the
host-switcher-warnings
branch
2 times, most recently
from
6b0ae6a
to
7219b41
Compare
mvollmer
removed
the
no-test
mvollmer
force-pushed
the
host-switcher-warnings
branch
from
7219b41
to
3d812b1
Compare
| if (port_index === -1) { | ||
| host_id_port = address + ":22"; |
There was a problem hiding this comment.
These 2 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.
This added line is not executed by any test.
|
We need to create a document about this, and make sure the link is correct: https://cockpit-project.org/guide/latest/multi-host.html
Also, what do we do about RHEL? We probably don't want to link to Cockpit from RHEL. I did get the login prompt after adding, as it seems like it does try to connect now (which makes sense), but then I got an error. Since it's Debian stable on Arm and I'm connecting from Fedora 41 on x86, it's not going to beibooot anyway (except from Cockpit Client or the container). It's almost guaranteed to not be related to this PR, but we should address it in a different PR:
The bug is that the close "button" is not a button, but should be. But it's not related to this PR. I'll file an issue. Summary:
|
The document is part of this PR: https://github.com/cockpit-project/cockpit/blob/3d812b1783c63a2395d10d7a741052a9b25bbe99/doc/guide/multi-host.xml It will appear at the URL once we make a release after this PR is merged.
I don't know... Can we do this as a follow? |
mvollmer
added
the
no-test
The old behavior was to navigate always and try to login afterwards. If the login failed, the "Troubleshoot" curtain would be shown instead of the pages from the host. The new behavior will first try to connect to the host and only navigate to it when the connection has succeeded.
This requires calling connect_host() in reaction to the "connect" event of the ShellState so that the warning dialog will be shown before attempting any connection. A consequence of that is that now login attempts are always interactive, and show the password prompts immediately instead of only showing the "Troubleshoot" curtain with a button to launch those dialogs.
mvollmer
force-pushed
the
host-switcher-warnings
branch
from
3d812b1
to
f3f4219
Compare
Sure, as long as we immediately work on it. Thanks. |
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