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
Implements password validation via Django #1658
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but a few points:
- The contrib validator has no instructions for how to actually use it in Evennia code. I know this can be looked up in Django docs, but that's not good enough for a contrib.
- Does the minimum password length affect existing password validation? If so this appears inconsistent - would be bad if you can log into the game as user
#1
but not into the admin with the same account and password. I can see the point in increasing the minimum (although everything under 11 characters won't stand up well to cracking anyway I guess), but maybe it could only happen for newly created games. - Inconsistency between game and web is no good. There are not that many places where the password is checked in regular game code. It sounds like a good time to integrate this in those places?
- If the previous point is done and the Evennia validator is used by default, I guess it doesn't belong in the contrib/ folder but should be moved into the
evennia/server/
folder.
The validator should not be getting called directly though; it undermines the purpose of using the Django validation framework. If you're manually calling the contrib validator, that means you're bypassing the others. This is not a good practice to get in the habit of. I think the actual problem here is the lack of a common API call to set/change passwords that forces validation, like however the Admin interface and stock password change forms do it. That would be worth implementing and documenting; more on this below. Given the status quo, if a dev rolls their own password-change functionality (and there are many valid reasons), it's quite trivial for them to inadvertently bypass all validation by calling low-level methods on high-level objects without understanding the implications.
Nope! I agree with your concern, but the low impact I'm estimating is the only reason I felt comfortable pushing this in the first place. If your existing password is something trivial like "1234" you will be allowed to continue to use it in perpetuity whether it's on the web or in a terminal. However with these validators enabled, when the time comes for you to change it/set it to something else (or create a new account), the stock web interface will force adherence to the new requirements. The existing terminal commands (as currently implemented) will not.
CmdPassword, CmdNewPassword, and CmdUnconnectedCreate are three commands that hard-code the checks and should be corrected to have that block removed (I also need to modify the connection screen to reflect the stated policy). There are a bunch of interrelated helper functions that handle things like account creation too though, which is the more daunting rabbithole. Would it be an overreach to override the Account.set_password() method (and maybe create an Account.validate_password() method to accomplish point 1) to simply use the framework as intended? This would force consistency regardless of calling function. Then it would be transparent to everyone with no special implementation details, and entirely configurable via server.conf. The tradeoff is that a dev would no longer be able to override the password policy and manually set a password of poor complexity, but in my professional experience no good ever comes of that (and if they want to be able to set crappy passwords, they can disable the validators and deal with the consequences). Or I can set a flag to allow skipping of the validation check. |
|
Awesome, we're on the same page. I think I'll add a few hooks while I'm at it to make things like #1383 more of a possibility; password changes are a sensitive-enough event worth having the ability to yield notifications before or after the fact. |
…alidate_password() method to validate passwords.
…Django password validation before modification.
@Griatch Ready for review.
|
Brief overview of PR changes/additions
Evennia's current password validation functionality is copied and pasted across many Command objects and given the contextual impositions of Commands, are completely unavailable from the web server. This leads to unsafe and hard-coded policies such a default minimum password length of 4 characters and allowing public-known compromised passwords to be used.
This PR enables the 4 default validators that would otherwise be enabled by default in any new Django 1.9+ project, sets the default minimum password length to a more sane 8 (can be reverted in settings.py/server.conf per Django conventions) and adds a 5th validator based on the character set Evennia currently allows.
It does not attempt to integrate with or modify any of the default commands or impact users' existing passwords, but any stock Django forms (including the Admin interface) will enforce these policies on new passwords.
Motivation for adding to Evennia
Adherence to DRY principles and more consistent application of password policy going forward.