fix: refresh user status when changing pwd #2274
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes password management by ensuring that when a user changes their password, both the password expiration date and the change request flag are reset to 0. It also corrects a test initialization issue where user_admin was being called before the test environment was initialized.
Changes:
- Updated
change_passwordmethod to resetpassword_expiration_dateandchange_passwordfields to 0 - Added test coverage to verify password status is properly reset after password change
- Moved user_admin initialization to after init() call in security policy test
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/Ravada/Auth/SQL.pm | Modified change_password to update password_expiration_date and change_password fields, refactored method signature to use modern Perl signatures |
| t/user/50_admin.t | Added test case to verify password expiration and change request flags are reset to 0 after password change |
| t/mojo/40_security_policy.t | Moved user_admin variable initialization to after init() call to ensure proper test environment setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $sth= $$CON->dbh->prepare("UPDATE users " | ||
| ." set password=?, change_password=?, password_expiration_date=?" | ||
| ." WHERE id=?"); | ||
| $sth->execute(sha1_hex($password), ($force_change_password or 0) ,0, $self->id); |
There was a problem hiding this comment.
The change_password implementation is still storing user passwords using a plain SHA1 hash via sha1_hex($password) before writing to the users table. SHA1 without a dedicated password hashing scheme (e.g., bcrypt, scrypt, Argon2) is fast and unsalted, making it practical for attackers to crack large numbers of passwords offline if the database is ever compromised. Replace this with a modern, slow, and salted password hashing function from a well-vetted library and migrate existing hashes where possible.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <[email protected]>
|
@frankiejol I've opened a new pull request, #2275, to work on those changes. Once the pull request is ready, I'll request review from you. |
frankiejol
changed the title
When the user changes the password, expiration date and change request are reset. Also move user admin creation after init on testing security policy.