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.

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

login: Warn before logging into multiple hosts #20861

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented Aug 9, 2024

  • design

This is the second half of #20834.

Sorry, something went wrong.

@mvollmer mvollmer force-pushed the login-warn-multihost branch from d5b6ca1 to 3462e54 Compare August 29, 2024 11:45
redirect_to_current_machine();
if (cur_machine) {
if (!environment.page.allow_multihost)
redirect_to_current_machine();
Copy link
Contributor

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.

redirect_to_current_machine();
else {
id("multihost-message").textContent = format(_("You are already connected to '$0' in this browser session. Connecting to other hosts will allow them to execute arbitrary code on each other. Please be careful."),
cur_machine == "." ? "localhost" : cur_machine);
Copy link
Contributor

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.

@mvollmer
Copy link
Member Author

@garrett, this is the "Login Page" version of #20826.

@mvollmer
Copy link
Member Author

Old demo: https://youtu.be/lvR2youiY4M

Comment on lines -357 to +364
.login-pf #banner {
.login-pf #banner, .login-pf #multihost-warning {
margin-block: 1rem 0.5rem;
margin-inline: 0;
grid-area: banner;
inline-size: 100%;
}

#banner-message {
#banner-message, #multihost-message {
Copy link
Member

Choose a reason for hiding this comment

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

We really shouldn't target IDs in the CSS. We already do, I know, but we shouldn't add more. (The selectors should be targeting classes.)

We especially should not have IDs nested under other selectors like this.

Ideally, we'd fix the older parts by changing those too.

Anyway, I'm not expecting you to have to fix this, just pointing out that it's bad form. We're already doing bad stuff anyway in the file, so it's not a huge deal. But it's important for new code and we should (at some point) clean up instances where IDs are used in CSS. It's important everyone on the team understands this, as using IDs leads to other bad things, like CSS not working as expected, resulting in !important and other bad things.

https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/Selectors/Type_Class_and_ID_Selectors#id_selectors

The ID selector has high specificity. This means styles applied based on matching an ID selector will overrule styles applied based on other selector, including class and type selectors. Because an ID can only occur once on a page and because of the high specificity of ID selectors, it is preferable to add a class to an element instead of an ID. If using the ID is the only way to target the element — perhaps because you do not have access to the markup and cannot edit it — consider using the ID within an attribute selector, such as p[id="header"]. Learn specificity.

(Note: We shouldn't use an attribute selector as it suggests, but actually fix the HTML.)

IDs are great and to be used in JavaScript for quick specific element selecting (as there's only one unique instance of an ID allowed on a document), of course. But it's bad for CSS.

Copy link
Member

Choose a reason for hiding this comment

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

(BTW: I'm not blocking on this in this PR or nitpicking, just pointing out something that would have really bad side effects, especially elsewhere in Cockpit.)

martinpitt added a commit to martinpitt/cockpit-machines that referenced this pull request Nov 21, 2024
Adjust to cockpit-project/cockpit#20861 to keep
tests uniform across OSes.

Fixes cockpit-project#1913
martinpitt added a commit to cockpit-project/cockpit-machines that referenced this pull request Nov 22, 2024
Adjust to cockpit-project/cockpit#20861 to keep
tests uniform across OSes.

Fixes #1913
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants