[16.0][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
4 times, most recently
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 now 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.
There was a problem hiding this comment.
Hello, could you add a comment above the search to explain what we are trying to fix? That will help the maintenance in the future as it's not obvious just by reading the code.
There was a problem hiding this comment.
@sebalix Sorry for the delay. I added some explanatory comment
sebalix
changed the title
maneandrea
force-pushed
the
16.0-password_security-update_on_copy-andrea
branch
from
15da524 to
82e5c0a
Compare
| def copy(self, vals): | ||
| if vals.get("password"): | ||
| vals["password_write_date"] = fields.Datetime.now() | ||
| return super(ResUsers, self).copy(vals) |
There was a problem hiding this comment.
Hi @maneandrea thank you for proposing a fix for this bug.
I found the same issue when signing up, the template user is copied with its initialization value of password_write_date.
I am however wondering about the way you propose to fix it, from my point of view, it would be better that this field "password_write_date" is not copied at all (adding a copy=False on field definition). If no value is provided then it will use default (= fields.Datetime.now()).
There was a problem hiding this comment.
I'm taking over for Andrea so all issues in the PR get corrected. I just changed the field definition so that it is not copied over. Also added a unit test that verifies the fix is working. Can you review again please?
antonag32
force-pushed
the
16.0-password_security-update_on_copy-andrea
branch
from
82e5c0a to
c33e127
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. --- To ammend this, the field is no longer copied when cloning users. A new unnitest was added to verify this. 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
antonag32
force-pushed
the
16.0-password_security-update_on_copy-andrea
branch
from
c33e127 to
f8c4092
Compare
|
This PR has the |
|
Thanks @antonag32, tested on Runboat, it works as expected ! |
|
Great, can a maintainer merge the change? |
|
/ocabot merge patch |
|
This PR looks fantastic, let's merge it! |
|
Congratulations, your PR was merged at f75d2e3. Thanks a lot for contributing to OCA. ❤️ |
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