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

Add password strength indicator to user mgmt forms #14176

Closed
wants to merge 1 commit into from
Closed

Conversation

katebutler
Copy link

Uses the [jquery.pwstrength]https://github.com/matoilic/jquery.pwstrength) library.

pw-very-weak

pw-very-strong

Fixes #13070

@diosmosis
Copy link
Member

I haven't looked into how it would have to be used, but would https://github.com/dropbox/zxcvbn be usable here? Given it was created by dropbox, it might do a better job than the jquery lib.

@tsteur
Copy link
Member

tsteur commented Mar 11, 2019

We had a look at that lib but I don't think an 800KB lib should be needed for a password check ;) We found the jquery lib is lightweight but does the most important job.

@sgiehl
Copy link
Member

sgiehl commented Mar 11, 2019

@tsteur just out of curiosity, what is the plan regarding JS Frameworks? Wasn't it getting rid of jQuery and switching to AngluarJS? If so, we might better look for a small AngularJS lib for password strength.

@Findus23
Copy link
Member

I'm not sure we should add a dependency to 6 year old code just for 30 lines of JS.

I guess it should be possible to simply write the same feature in pure JS or Angular.

https://github.com/matoilic/jquery.pwstrength/blob/d6f07a13eba1479f481d0beceb612f439db406ae/jquery.pwstrength.js#L13-L44

But the main question is what excactly is a secure password (see https://xkcd.com/936/).
And when you take into account all possibilities (e.g. that Password1! is not a strong password), you either end up with a gigantic library like the one by dropbxox or start comparing it to large databases of common existing passwords which also isn't perfect.

@fdellwing
Copy link
Contributor

I'm not a fan of such password strength indicators as they mostly give a false sense of security for awful passwords. So either use zxcvbn (or similar) or nothing at all.

@diosmosis
Copy link
Member

@tsteur looks like there's a php port (linked on the github repo desc). We could invoke it through ajax, what do you think?

@Findus23
Copy link
Member

Findus23 commented Mar 11, 2019

@diosmosis It wouldn't be realtime (via ajax it would be), but I think that's not that bad as the library can even return a list of suggestions on how to improve the password.

The question is if this should be part of Matomo or a plugin.

@diosmosis
Copy link
Member

diosmosis commented Mar 11, 2019

If it's meant to increase baseline security for users, I would think it should be part of Matomo, not an optional plugin. I was addressing the issue of it being an 800kb JS library. If the PHP port is used, then only minimal JS is added to the client.

EDIT: Though maybe there's an issue of adding it to the matomo.zip package too? We're already pushing the limit in ReleaseCheckListTest.

@tsteur
Copy link
Member

tsteur commented Mar 11, 2019

@tsteur just out of curiosity, what is the plan regarding JS Frameworks? Wasn't it getting rid of jQuery and switching to AngluarJS? If so, we might better look for a small AngularJS lib for password strength.

There's not really a plan, although for sure be great. The main plan is to eventually get rid of twig templates and PHP controllers. But that might be many years away :) If there's a simple lib that works and does the job based on jQuery I would say why not. Probably before that, we need to at some point see how to upgrade from Angular 1.X to a newer angular version but this will be big project in itself.

I guess it should be possible to simply write the same feature in pure JS or Angular.

@Findus23 Can you maybe explain what the point is to rewrite it in pure JS or Angular? When there's already something that works etc?

@Findus23 @fdellwing I had a quick look on http://matoilic.github.io/jquery.pwstrength/ before we chose the lib and it was giving OK suggestions. You could of course also slightly adjust the code. A 1KB be obviously not as as good as a zxcvbn 800KB lib but it does the job and gives a better indication than it was before (which was none).

@diosmosis @Findus23 we could port it to PHP. The lib is also > 2MB though and more work for a feature that is currently not so relevant for us in terms of priority. I would say we either go with the jQuery lib here or we close the PR and move the issue back to the backlog. We only decided to implement and work on the issue cause it's a quick solution with the jQuery version that doesn't add much overhead. Happy either way.

@diosmosis
Copy link
Member

@tsteur it has already been ported to php: https://github.com/bjeavons/zxcvbn-php (the link is on the main zxcvbn github repo). Would just using this be an ok amount of work?

@tsteur
Copy link
Member

tsteur commented Mar 11, 2019

@diosmosis I know that's why I meant it's > 2MB. I meant the work to do the ajax, change wording in the UI based on result, colours, etc. The issue has just no priority currently.

@diosmosis
Copy link
Member

Ok, I'll leave it alone then.

@fdellwing
Copy link
Contributor

I had a quick look on http://matoilic.github.io/jquery.pwstrength/ before we chose the lib and it was giving OK suggestions.

@tsteur I do see that differently. The lib does not look at entropie at all, but simply checks for "complex" chars. Ignoring both length and possible common char replacement in the password. I would therefor say: The lib is worse than not having an indicator at all, because

give a false sense of security for awful passwords.

@tsteur
Copy link
Member

tsteur commented Mar 11, 2019

OK no worries. We can close the PR and move it out of the milestone for now.

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

6 participants