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

shell: password strength indication #2061

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
3 participants
@fridex
Copy link
Contributor

commented Mar 30, 2015

Based on #2031 and #644

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2015

Nice work.

Could you rebase this ontop of git master. In particular, we've dropped the dependency on 'expect' once packagers informed us that it was implemented in TCL.

@fridex fridex force-pushed the fridex:password_strength branch from d1c7922 to 6718186 Mar 31, 2015

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2015

Could you rebase this ontop of git master.

Yes, sure. It should be rebased by now.

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2015

Now I see, bad rebase. Sorry for that....

EDIT: fixed

@fridex fridex force-pushed the fridex:password_strength branch from 6718186 to eb0912d Mar 31, 2015

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2015

Good start!
However, 0% is hard to read due to white text on gray background and due to being glued to the left side of the progress bar.

screenshot from 2015-04-01 11 50 05

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2015

The dialog will also need to inform that the current password is too weak to proceed and ideally give some tips on how you can strengthen it.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2015

Having it go by percentage from 0%-100% gives the notion that only 100% is acceptable. Does the backend have any notion of levels? Like: Bad password, Okay password, Good password, Excellent password? Similar to what Dropbox and the Fedora installer among others do http://blog.codinghorror.com/content/images/2015/01/dropbox-password-strength-meters.png

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2015

Mockup

@andreasn andreasn removed the needsdesign label Apr 1, 2015

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2015

The dialog will also need to inform that the current password is too weak to proceed and ideally give some tips on how you can strengthen it.

Tips could be confusing since it depends on actual configuration.

Having it go by percentage from 0%-100% gives the notion that only 100% is acceptable. Does the backend have any notion of levels? Like: Bad password, Okay password, Good password, Excellent password?

There is only 0-100 scale (more than 0 is acceptable), but levels can be easily hard-coded.

According to the design, I suppose that progress bar is going to be replaced with something else. Is it already available in patternfly? .

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2015

Tips could be confusing since it depends on actual configuration.

All right. Does the system have any kind of clue if it's too short or not, or do we need to go for something generic like "Password is not strong enough" ?

There is only 0-100 scale (more than 0 is acceptable), but levels can be easily hard-coded.

Cool. Lets try and do it in steps of 25%. 0-25%, 26-50%, 51-75%, 76-100%

According to the design, I suppose that progress bar is going to be replaced with something else. Is it already available in patternfly? .

No, unfortunately not, but I'm happy to help with the css.

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2015

All right. Does the system have any kind of clue if it's too short or not, or do we need to go for something generic like "Password is not strong enough" ?

Now I can imagine only a generic solution like yours.

No, unfortunately not, but I'm happy to help with the css.

That would be great.

andreasn added a commit to andreasn/cockpit that referenced this pull request Apr 8, 2015

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2015

so using this markup andreasn@a8a7399#diff-05e1d4cff6d7b149dc269fa20af3ebd5R201

and adding any of the classes weak/okay/good/excellent should get the desired results.
Not having any of those classes will give an empty bar, and that should be the initial state.

andreasn added a commit to andreasn/cockpit that referenced this pull request Apr 9, 2015

@fridex fridex force-pushed the fridex:password_strength branch from eb0912d to 7aa7809 Apr 10, 2015

fridex added a commit to fridex/cockpit that referenced this pull request Apr 10, 2015

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2015

Great progress!
Would it be possible to place the strengh meter in the same table as the password field, instead of in it's own table row?

screenshot from 2015-04-13 19 15 03

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2015

In order to achieve "Excellent password" I need to type a couple of more characters after the bar has been filled. Would be better to have more of a 1-to-1 match between filled bar and Excellent password.

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2015

In order to achieve "excellent password", one has to enter password with 100% score. If it is desired to have 4 bars only when "excellent password" is entered, the scale should be adjusted. Would be 0-33, 34-66, 67-99, 100 OK? Or is it better to achieve "excellent password" with score more than 75%? Personally, I would prefer the first option.

@fridex fridex force-pushed the fridex:password_strength branch from de91edd to 90c79d5 Apr 17, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2015

In order to achieve "excellent password", one has to enter password with 100% score. If it is desired to have 4 bars only when "excellent password" is entered, the scale should be adjusted. Would be 0-33, 34-66, 67-99, 100 OK?

Makes sense to me.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2015

Some layout fixes, as promised earlier: https://github.com/andreasn/cockpit/tree/strength-meter-layout

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2015

Oh, sorry. Looks like we are working on it at the same time :) I will use your patch.

@fridex fridex force-pushed the fridex:password_strength branch from cbc48dd to 305a7f9 Apr 21, 2015

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2015

It seems this needs a rebase

@fridex fridex force-pushed the fridex:password_strength branch from fc19934 to 50628e8 Apr 22, 2015

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2015

Rebased.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2015

Looks good!

<td><div id="account-set-password-message-password-mismatch" class="has-error">
<span class="help-block">The passwords do not match</span>
<td><div class="has-error">
<span id="account-set-password-meter-message" class="help-block">

This comment has been minimized.

Copy link
@stefwalter

stefwalter Apr 24, 2015

Contributor

This needs translatable="yes"

<div id="accounts-create-message-password-mismatch" class="has-error">
<span class="help-block">The passwords do not match</span>
<div class="has-error">
<span id="accounts-create-password-meter-message" class="help-block">The passwords do not match</span>

This comment has been minimized.

Copy link
@stefwalter

stefwalter Apr 24, 2015

Contributor

This needs translatable="yes"

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2015

Looks really good. I'll fix the remaining issues when merging.

@stefwalter stefwalter removed the needswork label Apr 24, 2015

@stefwalter stefwalter self-assigned this Apr 24, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2015

The integration tests fail due to password validation. Since I have to fix things anyway. I'm going to migrate this to the new validation style. https://github.com/cockpit-project/cockpit/wiki/HIG:-Dialogs

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2015

In addition the validation doesn't actually work yet. It's easy to get into a state where it'll never allow you to proceed no matter how much you change the password.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2015

  1. by just typing and tabbing around, i can get things into a state where i just have to cancel out, i never get the 'Set' button enabled
  2. the logic doesn't match the HIG for dialogs, which is mostly because it didn't work that way before you started: https://github.com/cockpit-project/cockpit/wiki/HIG:-Dialogs
  3. lastly the check-accounts integration tests fails because they need to be updated for the new validation
@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2015

  1. the logic doesn't match the HIG for dialogs
    With regards to the insensitive button and stuff? Could we do that as a followup to this issue? It feels like we got in the middle of it all and outside the control of this pull request.

Issue one sounds more severe though.

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2015

  1. by just typing and tabbing around, i can get things into a state where i just have to cancel out, i never get the 'Set' button enabled

Cannot reproduce. Could you be please more specific? Tab is working as expected on Chrome for me.

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Apr 30, 2015

shell: Password strength indication
Adds libpwquality dependency. Introduces a CSS widget for
password strength.

Fixes cockpit-project#2031
Closes cockpit-project#2061
Signed-off-by: Stef Walter <stefw@redhat.com>
 * Squashed, localization and test fixes
@stefwalter

This comment has been minimized.

Copy link
Contributor

commented May 4, 2015

Rebased, squashed, fixed tests, updated dialog behavior in #2233

@stefwalter stefwalter added blocked and removed needswork labels May 4, 2015

stefwalter added a commit to stefwalter/cockpit that referenced this pull request May 4, 2015

shell: Password strength indication
Adds libpwquality dependency. Introduces a CSS widget for
password strength.

Fixes cockpit-project#2031
Closes cockpit-project#2061
Signed-off-by: Stef Walter <stefw@redhat.com>
 * Squashed, localization and test fixes

stefwalter added a commit to stefwalter/cockpit that referenced this pull request May 6, 2015

shell: Password strength indication
Adds libpwquality dependency. Introduces a CSS widget for
password strength.

Fixes cockpit-project#2031
Closes cockpit-project#2061
Signed-off-by: Stef Walter <stefw@redhat.com>
 * Squashed, localization and test fixes

@dperpeet dperpeet closed this in d1d370f May 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.