Conversation
Migrate from weak SHA-256 password hashing to secure bcrypt using PHP's password_hash() with PASSWORD_DEFAULT. Changes: - hashPassword() now uses password_hash() for bcrypt hashing - isPasswordValid() supports both legacy SHA-256 and new bcrypt formats - Legacy passwords are automatically upgraded to bcrypt on successful login - New users get bcrypt-hashed passwords immediately - UserEditor.php uses updatePassword() instead of manual hashing Migration strategy: - Writing: All new passwords use bcrypt - Reading: Supports both SHA-256 (legacy) and bcrypt (new) - Auto-upgrade: Legacy hashes converted to bcrypt on login The legacy SHA-256 support will be removed in a future version once all users have logged in and had their passwords upgraded.
Add rebuildViews() method to RestoreJob that executes rebuild_views.sql after a database restore completes. This ensures views (email_list, email_count) are always current even when restoring older backups that may not include current view definitions. Called from postRestoreCleanup() so views are rebuilt regardless of backup type (SQL, GZSQL, or full backup).
DawoudIO
requested review from
DAcodedBEAT,
MrClever,
bigtigerku,
grayeul and
respencer
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR implements a critical security enhancement by migrating password storage from weak SHA-256 hashing to secure bcrypt hashing using PHP's password_hash() function. The implementation includes a transparent migration path that automatically upgrades legacy SHA-256 passwords to bcrypt when users successfully authenticate. Additionally, the PR ensures database views are properly rebuilt after restore operations to prevent potential schema inconsistencies.
Key Changes:
- Password hashing upgraded from SHA-256 to bcrypt with automatic migration on login
- New user passwords now use secure bcrypt hashing via
User::updatePassword()method - Database views automatically rebuilt after restore operations
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ChurchCRM/model/ChurchCRM/User.php | Implements bcrypt password hashing with backward-compatible SHA-256 validation and automatic upgrade on successful login |
| src/UserEditor.php | Removes legacy SHA-256 hashing for new users; uses User::updatePassword() for secure bcrypt hashing |
| src/ChurchCRM/Backup/RestoreJob.php | Adds rebuildViews() method to restore database views from rebuild_views.sql after backup restoration |
| demo/ChurchCRM-Database.sql | Updates demo database with bcrypt password hashes for users 1 and 3; retains SHA-256 hash for user 95 to demonstrate backward compatibility |
| // Upgrade to bcrypt on successful login | ||
| $this->setPassword($this->hashPassword($password)); | ||
| $this->save(); |
There was a problem hiding this comment.
[nitpick] Consider adding logging for password hash migration events. When a legacy SHA-256 password is upgraded to bcrypt, this is a significant security event that should be logged for audit purposes. This helps track the migration progress and can alert administrators if unexpected migrations occur.
Suggested addition:
if (hash_equals($storedHash, $legacyHash)) {
// Upgrade to bcrypt on successful login
LoggerUtils::getAppLogger()->info('Upgrading password hash from SHA-256 to bcrypt', [
'user_id' => $this->getPersonId(),
'username' => $this->getUserName()
]);
$this->setPassword($this->hashPassword($password));
$this->save();
return true;
}| return $this->getPassword() == $this->hashPassword($password); | ||
| $storedHash = $this->getPassword(); | ||
|
|
||
| // Check if this is a bcrypt hash (starts with $2y$) |
There was a problem hiding this comment.
The comment says "Check if this is a bcrypt hash (starts with $2y$, $2b$, and $2a$. Update the comment to reflect all supported bcrypt variants:
// Check if this is a bcrypt hash (starts with $2y$, $2b$, or $2a$)| // Check if this is a bcrypt hash (starts with $2y$) | |
| // Check if this is a bcrypt hash (starts with $2y$, $2b$, or $2a$) |
What Changed
Type
Testing
Screenshots
Security Check
Code Quality
Pre-Merge