[14.0][FIX] password_security: Correctly look at previous password history#448
[14.0][FIX] password_security: Correctly look at previous password history#448
Conversation
If a user has previous passwords stored and the `password_history` is set to 0 to disable the check, the line of code would evaluate to: ``` rec_id.password_history_ids[0:-1] ``` The above returns all items in the list except for the first last entry. One expects the above to evaluate to: ``` rec_id.password_history_ids[0:0] ``` The above returns an empty list which would then correctly not raise a warning.
sebalix
changed the title
There was a problem hiding this comment.
I haven't tested, but I think you're correct, without this fix the number of history passwords to check is not computed properly, but I would expand it a little bit more to completely skip this check in case recent_passes == 0, and that would save us the read on password_history_ids.
if not recent_passes: # disabled
continue
elif recent_passes < 0: # unlimited
recent_passes = rec_id.password_history_ids
else:
recent_passes = rec_id.password_history_ids[:recent_passes]Unit tests to reproduce the bug you're fixing would be appreciated 🙏🏻 it's the best way to fight regressions
|
Hi @adekock11, I think the proposal of @ivantodorovich is a nice one. Are you going to implement it and complete the tests? |
There was a problem hiding this comment.
@adekock11 For password_security/tests/test_res_users.py
def test_check_password_history(self):
# Disable all password checks except for history
set_param = self.env['ir.config_parameter'].sudo().set_param
set_param('auth_password_policy.minlength', 0)
user = self.env.ref('base.user_admin')
user.company_id.update({
'password_estimate': 0,
'password_length': 0,
'password_lower': 0,
'password_history': 1,
'password_numeric': 0,
'password_special': 0,
'password_upper': 0,
})
self.assertEqual(len(user.password_history_ids), 0)
user.write({'password': 'admin'})
self.assertEqual(len(user.password_history_ids), 1)
with self.assertRaises(UserError):
user.write({'password': 'admin'})
user.write({'password': 'admit'})
self.assertEqual(len(user.password_history_ids), 2)
user.company_id.password_history = 2
with self.assertRaises(UserError):
user.write({'password': 'admin'})
with self.assertRaises(UserError):
user.write({'password': 'admit'})
user.write({'password': 'badminton'})
self.assertEqual(len(user.password_history_ids), 3)
user.company_id.password_history = 0
user.write({'password': 'badminton'})
self.assertEqual(len(user.password_history_ids), 4)Edit: Added test for password_history = 0
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
bot
added
the
stale
Syncing from upstream OCA/server-auth (16.0)
5 participants
If a user has previous passwords stored and the
password_historyis set to 0 to disable the check, the line of code would evaluate to:The above returns all items in the list except for the first last entry. One expects the above to evaluate to:
The above returns an empty list which would then correctly not raise a warning.