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

Added a Passwort Strength Component for User Registration #19355

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

Csaern
Copy link

@Csaern Csaern commented Dec 19, 2024

As a project I made a password strength component to visualize the strength of a password used to create an account for the galaxy server. The user gets feedback at the registration site, for the strength of his password and also an estimation time, how long it would take to get cracked.
I also added

  • the eye functionality, to show and hide the input for the password
  • a button for password guidelines, to show users, what components they need to get a strong password, which includes an reference link for an external website.

Password_weak
Password_medium
Password_Modal

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@nsoranzo
Copy link
Member

Thanks for working on this feature! Two suggestions:

  • Add the "eye" functionality to the password field to allow the uses to view what they are typing if it's safe to do so
  • Replace the Password Guidelines button with a simple help text containing the few rules we actually enforce and a link to general-purpose guidelines like https://www.cisa.gov/secure-our-world/use-strong-passwords

@guerler
Copy link
Contributor

guerler commented Dec 19, 2024

I agree with @nsoranzo, and would additionally suggest that you could add a checkmark to each fulfilled criteria listed in the help section. Thank you for working on this!

@Csaern Csaern force-pushed the password branch 2 times, most recently from ccfe6d5 to 44f4e95 Compare January 14, 2025 11:25
@guerler
Copy link
Contributor

guerler commented Jan 21, 2025

I think this might work better without the guidelines button and modal, since similar information is already conveyed through the strength indicator and the list of criteria.

@ElectronicBlueberry
Copy link
Member

since similar information is already conveyed through the strength indicator and the list of criteria

the list of criteria is from an old screenshot. They were removed, since the only criteria we enforce is a length of at least 6 characters.

@guerler
Copy link
Contributor

guerler commented Jan 21, 2025

Ah interesting. Thanks. I still think that the modal should be removed. Having a button and a modal just to display criteria we don't apply seems to be unnecessary. In my opinion, we should either extend the required criteria list or just show the strength bar with some help text under it. The crack time also seems to be misleading and not informative since we can not look 10-20 years into the future.

@ElectronicBlueberry
Copy link
Member

The time estimation is to give people another metric for the strength bar. Seeing that your password can be cracked within a about minute of computing time can be more effective means to motivate people to use a stronger password, than simply displaying "weak".

Regarding the foresight: The strength estimation could also be off in 10-20 years, but I'm trusting we will update our dependencies before it deviates a significant amount.

@Csaern Csaern force-pushed the password branch 2 times, most recently from 8ff5481 to 8b3cc39 Compare January 27, 2025 14:43
@Csaern Csaern marked this pull request as ready for review January 27, 2025 19:27
@github-actions github-actions bot added this to the 25.0 milestone Jan 27, 2025
height: 8px;
border-radius: 4px;
overflow: hidden;
margin-top: 5px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend using Bootstrap utility classes instead of pixel and rem values. For example, use mt-2 directly in the HTML class attribute or apply it in CSS with @extend .mt-2;.

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 need to have a talk about bootstrap and bootstrap utility classes, since they are hurting our code quality and maintainability. I've discussed this with some people already, and am currently working on an outline of why and how we can proceed with this issue with my findings. I will post this under discussions once ready, and would ask you to wait for that, since this review is not the right place for this discussion.

@guerler
Copy link
Contributor

guerler commented Feb 1, 2025

I see the intention behind the crack-time metric, but I feel there’s a slight disconnect—it represents a future projection, whereas the weak label describes the password’s strength today. Because of this, it might not be the most intuitive way to communicate security risks. Also, I haven’t come across crack-time used elsewhere, and the modal feels like a bit more than what’s necessary. That said, I really appreciate the effort put into this! If the hard-coded margins, paddings, and other CSS elements are replaced with Bootstrap utility classes, I think this PR is in good shape unless anyone else has additional comments or suggestions. Thanks again for working on this!

@ElectronicBlueberry
Copy link
Member

it represents a future projection, whereas the weak label describes the password’s strength today.

Both represent the same metric in different terms. This is about compute time today, there is no future projection involved. I encourage you to have a look at how this works here: https://dropbox.tech/security/zxcvbn-realistic-password-strength-estimation

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

Successfully merging this pull request may close these issues.

4 participants