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

explain in your readme why you aren't building on existing plugins rather than building a new one? #5

Open
djay opened this issue Feb 3, 2016 · 10 comments

Comments

@djay
Copy link
Member

djay commented Feb 3, 2016

Whats different from these?

@frisi
Copy link
Member

frisi commented Feb 3, 2016

good question @djay

i don't know why enfolds added a password validator to this package instead of using the already existing products.passwordstrenght for this (passwordstrenght had it's first commit in 2007, loginlockout in dec 2010 and pwexpiry in 2013).

we chose this package in favor of PasswordStrenght because we have a customer asking for an active directory password policy

  • A) 8 characters at least
  • B) 3 out of 4 requirements
    • uppercase letters
    • lowercase letters
    • numbers and
    • special characters
  • C) expire passwords after 90 days
  • D) the latest 10 passwords must not be reused

after a quick recherche it looked like collective.pwexpiry is a good fit since it solves A,B and C out of the box (passwordstrenght does not have the expiration feature)

to explain the differences i'd add something like this to the readme:

Related Addons
--------------

Products.LoginLockout
    PAS plugin that will lock a login after a predetermined number of incorrect attempts. Once locked, the user will be shown a page that tells them to contact their administrator to unlock.

    This functionality is included in collective.pwexpiry (XXX need to check if it's done better/different there)


Products.PasswordStrength
    provides a PAS plugin that allows to define password policies (based on regular expressions).

    compared to pwexpiry:

    - PasswordStrenght does not patch the description in the password form (so people are told to "provide 5 charcters at least" whereas the policy requires more)
    - has no password expiration
    - allows to change password policies through the web

unclear:

  • can users provide error messages for each validator?
  • are those messages translateable
  • which package has the more modern approach to validate passwords

@djay: as you are the maintainer of PasswordStrength i'd love to get your feedback on this comparison

there are indeed some overlappings between those two packages.

ideally every addon should serve it's very specific usecases.

so pwexpiry could do just that: expire passwords (C)

and passwordstrenght could handle A, B and D

@frisi
Copy link
Member

frisi commented Feb 8, 2016

@djay: i just noticed that pwexpiry also has a feature to lock out the user after n unsuccessful login-tries for a period of m minutes.
you can also unlock locked accounts in the control panel.
so there is definitely also an overlap with Products.LoginLogout

@djay
Copy link
Member Author

djay commented Feb 9, 2016

@frisi PasswordStrength is designed to be used TTW which I'm not sure this plugin is. It doesn't use any adapters. It does however use the existing PAS api for setting password validation rules which is built into plone.
It uses a series of regular expressions for the password rules so char limits and special chars are all possible plus a lot more other options. The advantage is regular expression syntax is all you need, not the ability to deploy or know python.

@djay
Copy link
Member Author

djay commented Feb 9, 2016

@frisi There is no need for your code to monkey patch registrationtool. Unless you are trying to support very old versions of plone. You can just use a PAS plugin which is cleaner.

@jochumdev
Copy link
Contributor

Regular expressions are nice, but IMO unable to solve the following Problem with regex only.

    # Checking it the entered password fits to the password policy scheme:
    # it must contain at least 3 from the 4 parts:
    # - Uppercase characters (A through Z)
    # - Lowercase characters (a through z)
    # - Numerals (0 through 9)
    # - Special characters such as !, $, #, %

@jochumdev
Copy link
Contributor

Think we can close this issue @frapell

@frisi
Copy link
Member

frisi commented May 3, 2016

personally i think that a small paragraph in the readme that explains the differences to the other packages (as discussed above) and explains why the monkey patch is needed makes a lot of sense.
this will help people to decide which package to use

@djay
Copy link
Member Author

djay commented May 3, 2016

@pcdummy Yes regex can handle that password rule. You would have 4 regular expressions joined into one, seperated by an OR. Each one missing one of those rules.
This probably wouldn't scale to a "must have 2 of the following 12 parts" but still.
In any case, this could have been an extension to the existing plugin so there was less confusion.

@djay
Copy link
Member Author

djay commented May 3, 2016

@frisi I possibly didn't answer all your questions. Since the configuration is TTW, you can put whatever translation you want for the error messages but there isn't a facility to have different messages for different languages. It perhaps wouldn't be hard to fix this however.

Yes you can provide a different error message for each validator. It will also combine the error messages if you want to break your rules down into seperate rules.

Which is more modern? Thats subjective. PasswordStrength works with the core API for validation which was built into Plone. This goes via PAS which is the supported method. This is preferable to monkeypatching as it will be supported going forward. Relying on adapters and filesystem coding to implement rules is perhaps more liable to result in site specific code needing to be upgraded over time.

@djay
Copy link
Member Author

djay commented May 3, 2016

"PasswordStrenght does not patch the description in the password form (so people are told to "provide 5 charcters at least" whereas the policy requires more)" is plainly untrue. Not sure where this came from.

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

No branches or pull requests

3 participants