@katebutler opened this Pull Request on March 11th 2019 Contributor

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

pw-very-weak

pw-very-strong

Fixes #13070

@diosmosis commented on March 11th 2019 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 commented on March 11th 2019 Member

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 commented on March 11th 2019 Member

@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 commented on March 11th 2019 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 commented on March 11th 2019 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 commented on March 11th 2019 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 commented on March 11th 2019 Member

@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 commented on March 11th 2019 Member

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 commented on March 11th 2019 Member

@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 commented on March 11th 2019 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 commented on March 11th 2019 Member

@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 commented on March 11th 2019 Member

Ok, I'll leave it alone then.

@fdellwing commented on March 11th 2019 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 commented on March 11th 2019 Member

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

This Pull Request was closed on March 11th 2019
Powered by GitHub Issue Mirror