Conversation
Fixes Update Password Bug Fixes #10878
📝 WalkthroughWalkthroughThis pull request refactors password handling in the users API controller and adds corresponding test coverage. The controller changes involve initializing a default hash instance upfront and restructuring the password validation logic to handle three cases: hashed passwords, plaintext passwords (which are hashed), and no password provided. Type annotations are added to loop variables for clarity. A new end-to-end test validates the workflow of creating a user without a password and subsequently setting one via a PATCH request. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/api/users.php (1)
120-133: Consider simplifying the empty password branch.The logic correctly fixes the Plaintext hash issue by using
ProofsPasswordas the default, but line 132 recalculates$isHashedafter already being set on line 119. Since$defaultHash->getHash()should consistently return the same hash type, line 132 will yield the same result unlessProofsPasswordreturns aPlaintextinstance (which would defeat the purpose of the fix).Consider simplifying by removing the redundant
$isHashedrecalculation:} else { // when password is not provided, plaintext was set as the default hash causing the issue $hash = $defaultHash->getHash(); - $isHashed = !$hash instanceof Plaintext; }Alternatively, add a comment explaining why the recalculation is necessary if there's a scenario where
ProofsPassword::getHash()could returnPlaintext.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/users.php(3 hunks)tests/e2e/Services/Users/UsersConsoleClientTest.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Users/UsersConsoleClientTest.php (1)
tests/e2e/Client.php (1)
Client(8-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (4)
app/controllers/api/users.php (2)
168-170: LGTM! Password validation now correctly guards against empty passwords.The updated condition ensures password validators only run when a plaintext password is actually provided, which aligns with the fix to support creating users without initial passwords.
635-636: LGTM! Type annotations improve code clarity.The added docblock annotations for loop variables enhance IDE support and static analysis. All annotations consistently specify
Utopia\Database\Documentfor session documents.Also applies to: 856-856, 1398-1398, 2430-2430, 2636-2636
tests/e2e/Services/Users/UsersConsoleClientTest.php (2)
9-9: LGTM! Import is necessary for the new test.The added import for
IDhelper is used correctly at line 53 to generate a unique user identifier.
68-68: The test correctly validates the console API behavior. Thepasswordfield in user responses intentionally exposes the hashed password (not the plaintext), which is documented in the User response model and appropriate for admin/console endpoints. No changes needed.
✨ Benchmark results
⚡ Benchmark Comparison
|
2 participants
What does this PR do?
Test Plan
Related PRs and Issues
Checklist