-
Notifications
You must be signed in to change notification settings - Fork 294
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
Allow more advanced, custom password logic #115
base: master
Are you sure you want to change the base?
Conversation
@@ -210,7 +221,7 @@ def validate(self): | |||
user, user_email = user_manager.find_user_by_email(self.email.data) | |||
|
|||
# Handle successful authentication | |||
if user and user.password and user_manager.verify_password(self.password.data, user): | |||
if user and user_manager.verify_password(self.password.data, user): |
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.
This change minimizes the assumption that password
is a field on the user entity. In reality, verify_password
should never be returning True
if the user has no password at all.
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.
For heterogeneous authorization systems, some users do not have a local password
(such as those users relying on an external OAuth server). Without a defined password, an attribute error
is raised when trying to pass self.password.data
into verify_password
- thus mandating the extra check.
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.
self.password.data
is on the form itself. I don't know how that could fail unless you're overriding the password
input on the form, in which case you can override the validate
method as well.
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.
@pbugni I see now that you added that check. I'm confused why you needed it. With my change (and the way it was before), you can always check user.password
within the body of user_manager.verify_password
if you override that method on the UserManager
.
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.
If self.password
does not exist, then you have clearly changed the original form class and it would make sense that you need to also override the form's validate
method to accomodate your changes.
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.
@3noch - Your assertions are correct about form data - the problem is that when a user doesn't have a password and then they attempt to log in - an error is raised in the call to user_manager.verify_password. I'd forgotten the details when replying earlier today, but it's easy to reproduce - clear the db password for a user and attempt to log in - at the bottom of the error stack:
File ".../env/local/lib/python2.7/site-packages/passlib/context.py", line 1447, in identify_record
raise ExpectedStringError(hash, "hash")
TypeError: hash must be unicode or bytes, not None
The point of this check is clear - if the user doesn't have a password, don't bother trying to verify. Yes, you could alternatively patch the lower level checks, but this seemed like the clean work around to me.
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.
I can re-add that check to the default implementation of verify_password
. Do you think that would solve your case?
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.
Yes - that'd be great. I can test your branch when it's ready. Thanks.
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.
@pbugni Ok, I added that check to verify_password
.
Hi @3noch! Thank you for your contribution! During last 4 years (I'm sorry that you are waiting for so long) many things were changed in |
I needed to validate passwords based on the user's information (username not included in password, password not reused from previous password). This motivated several changes:
user
entity available to password-handling forms (and therefore their validators)password
field on theUser
entity when verifying passwords (since passwords might be in another table).I designed this change in such a way that it should have minimal effect on current users of
Flask-User
. They are likely subclassing your Form classes, so the additionaluser
information will be silently ignored in their case but won't cause any issues.