Various login/beiboot preparations/cleanups #20967
Conversation
Having a random "cookie" here brings absolutely no security — it just makes it more difficult to test. Use predictable sequence numbers instead.
This lets you get access to the underlying response object, in case you want to get extra fields out of it.
These have diverged enough that splitting them out from each other substantially improves readability.
pkg/static/login.js
Outdated
| @@ -1,5 +1,10 @@ | |||
| import "./login.scss"; | |||
|
|
|||
| function debug(...args) { | |||
| if (window.debugging == 'all' || window.debugging?.includes('login')) | |||
There was a problem hiding this comment.
Also, this debugging?.includes() business is black magic :) I'd have expected .?() to be needed there as well... I guess that has to do with Javascript's strange this binding logic... TIL.
There was a problem hiding this comment.
Can do ===, that was mostly copy pasta from all the other places.
I don't understand your .?() comment though -- either window.debugging is set, then it has to be a string and has an includes() method. Or it's undefined, then debugging? short-circuits.
There was a problem hiding this comment.
The issue is that I read this parsed like so:
( (window . debugging) ?. includes ) ()and assuming that window.debugging is undefined, then I'd also expect (window.debugging)?.includes to be indefined. You can't call undefined(), which is why I'd have expected a ?.() to be required. But your code works, I tested it. I just don't understand why.
There was a problem hiding this comment.
We have a dozen copy of that code everywhere. Indeed ?. is a bit magic for function calls, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining.
I.e. foo?.method(...) is undefined (and nothing gets called) iff foo is undefined or null.
| @@ -770,15 +770,15 @@ import "./login.scss"; | |||
| function do_hostkey_verification(data) { | |||
| const key_db = get_known_hosts_db(); | |||
| const key = data["host-key"]; | |||
| const key_key = key.split(" ")[0]; | |||
| const key_host = key.split(" ")[0]; | |||
There was a problem hiding this comment.
<3. Srsly. This confused the heck out of me too.
|
...tests are angry, though :( |
|
"beiboot: support for temporary UserKnownHostsFile" causes the TestClient regressions. I made the code backwards compatible when login.js does not send the known host keys after |
|
.. but I'll look into this some more, we also wanted to change the scope of the TemporaryFile |
|
Nah, this is stupid. Let's keep that commit in #19401. |
We don't use this anywhere right now, but soon will.
The first component of the `host-key` protocol field is the host name/IP, which is used to index the known_hosts key database. It's confusing to call this `key_key`, as it's really not the key (nor fingerprint) material.
In Fedora 41, systemd ships a new file /etc/ssh/ssh_config.d/20-systemd-ssh-proxy.conf which is owned by systemd_conf_t. Allow cockpit-ws to read that, otherwise the whole `ssh` command fails with a SELinux denial as it can't read that config file.
This factorizes the current value and pins it down to a single query at the time when clicking the "Login" button. That avoids a small race condition where the user may change the field while the asynchronous login process is running.
martinpitt
force-pushed
the
login-cleanups
branch
from
bfcec78
to
971b727
Compare
| if (window.debugging === 'all' || window.debugging?.includes('login')) | ||
| console.debug('login:', ...args); |
There was a problem hiding this comment.
These 2 added lines are not executed by any test.
| @@ -552,8 +557,7 @@ import "./login.scss"; | |||
| } | |||
|
|
|||
| function host_failure(msg) { | |||
| const host = id("server-field").value; | |||
| if (!host) { | |||
| if (!login_machine) { | |||
There was a problem hiding this comment.
This added line is not executed by any test.
| const user = trim(id("login-user-input").value); | ||
| if (user === "" && !environment.is_cockpit_client) { | ||
| login_failure(_("User name cannot be empty")); | ||
| } else if (need_host() && id("server-field").value === "") { | ||
| } else if (need_host() && login_machine === "") { |
There was a problem hiding this comment.
This added line is not executed by any test.
| if (key_db[key_host] == key) { | ||
| debug("do_hostkey_verification: received key matches known_hosts database, auto-accepting fingerprint", data.default); |
There was a problem hiding this comment.
These 2 added lines are not executed by any test.
Broken out from #19401.