[FIX] password_security: update password_write_date on copy #713
Conversation
|
@moylop260 or @luisg123v Can you review? |
maneandrea
force-pushed
the
16.0-password_security-update_on_copy-andrea
branch
3 times, most recently
from
4de5b6e
to
aa457f9
Compare
Sometimes users are created from a template user via a `copy()`. This has the issue that a password is passed via the `vals` of the copy and therefore never seen by the `write()` function. As a result, the `password_write_date` field is left to the value of the template, which is either outdated or null. A concrete bug that resulted from this is that newly created users were asked to renew their password on their very first login. --- This commit reapplies the same logic of the `write()` method to the `copy()` method as well. It also changes the unit test test_03_create_user_signup to create the user at some time in the past so that ```python assertNotEqual(password_write_date, created_user.password_write_date) ``` makes sense. Finally it fixes the do_signup method to user the current user's password otherwise the password_write_date will be overwritten even when inputting invalid passwords
maneandrea
force-pushed
the
16.0-password_security-update_on_copy-andrea
branch
from
aa457f9
to
15da524
Compare
| @@ -17,7 +17,10 @@ | |||
| class PasswordSecurityHome(AuthSignupHome): | |||
| def do_signup(self, qcontext): | |||
| password = qcontext.get("password") | |||
| user = request.env.user | |||
| user = ( | |||
There was a problem hiding this comment.
the commit and the description mention the change in the copy
But this change is not mentioned
Also the title mention "...password_write_date on copy"
Could you elaborate how this search affects the purpose of this PR, please?
There was a problem hiding this comment.
I am open to separate this in two PRs. Conceptually they are different fixes but in practice they are both needed.
The case is when a user has 2FA activated. If that is the case, they get to this method as the public user, so the check in user._check_password(password) does not detect the use of an old password. This makes the write method run which in turn updates password_write_date. Only after then, a second call to _check_password rejects the change.
However, since password_write_date is not updated to today, the user is able to log in with the old (expired) password and use it for a long time after that.
BTW: I updated the PR description to match the commit message.
Sometimes users are created from a template user via a
copy().This has the issue that a password is passed via the
valsof the copyand therefore never seen by the
write()function.As a result, the
password_write_datefield is left to the value of thetemplate, which is either outdated or null.
A concrete bug that resulted from this is that newly created users were
asked to renew their password on their very first login.
This commit reapplies the same logic of the
write()method to thecopy()method as well.It also changes the unit test test_03_create_user_signup to create the
user at some time in the past so that
makes sense.
Finally it fixes the do_signup method to user the current user's
password otherwise the password_write_date will be overwritten even when
inputting invalid passwords