chore: detailed hash distribution in /health + rehash metrics test #46
Conversation
Summary of ChangesHello @zensgit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's observability regarding password security by introducing more granular hash distribution metrics and tracking password rehash operations. It also includes a crucial integration test to ensure the seamless and secure upgrade of older bcrypt hashes to the more robust Argon2id algorithm during user login. Additionally, it refactors the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the health endpoint with detailed password hash distribution metrics and adds rehash tracking functionality, plus creates an integration test to verify bcrypt-to-argon2id password upgrade behavior.
- Adds detailed bcrypt variant breakdown (2a/2b/2y) and argon2id counts in health metrics
- Implements rehash metrics tracking with counter for successful password upgrades
- Creates integration test to verify bcrypt password rehashing functionality during login
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| auth_bcrypt_rehash_test.rs | New integration test verifying bcrypt-to-argon2id password upgrade on login |
| main_simple_ws.rs | Updates to use AppState with metrics instead of plain pool |
| main.rs | Enhanced health endpoint with detailed hash distribution and rehash metrics |
| handlers/auth.rs | Modified login handler to track rehash metrics and use AppState |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "argon2id": a2id | ||
| }, | ||
| "rehash": { | ||
| "enabled": std::env::var("REHASH_ON_LOGIN").map(|v| !matches!(v.as_str(), "0" | "false" | "FALSE")).unwrap_or(true), |
There was a problem hiding this comment.
The rehash enabled logic is duplicated from the auth handler. Consider extracting this into a shared utility function to maintain consistency and reduce duplication.
Copilot uses AI. Check for mistakes.
| State(state): State<crate::AppState>, | ||
| Json(req): Json<LoginRequest>, | ||
| ) -> ApiResult<Json<Value>> { | ||
| let pool = &state.pool; |
There was a problem hiding this comment.
The local variable pool is introduced but then inconsistently used throughout the function. Some database calls use pool while others use &state.pool directly. Consider using one approach consistently for better readability.
Copilot uses AI. Check for mistakes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the /health endpoint by providing detailed hash distribution metrics, including bcrypt variants and argon2id counts. It also introduces rehash metrics and an integration test to verify bcrypt to argon2id upgrades. Additionally, it aligns the simple_ws main file with AppState and metrics. The changes primarily focus on improving security monitoring and password management.
jive-api/src/handlers/auth.rs
Outdated
| .bind(user.id) | ||
| .execute(&pool) | ||
| .execute(pool) | ||
| .await |
There was a problem hiding this comment.
The execute method should use the pool from the state to ensure consistency and proper resource management. This aligns with the changes made elsewhere in this file. Using the pool directly might lead to issues if the state is not correctly propagated.
.bind(user.id)
.execute(pool)
.await| "argon2id": a2id | ||
| }, | ||
| "rehash": { | ||
| "enabled": std::env::var("REHASH_ON_LOGIN").map(|v| !matches!(v.as_str(), "0" | "false" | "FALSE")).unwrap_or(true), |
There was a problem hiding this comment.
The unwrap_or(true) could mask potential errors when reading the environment variable. It might be better to log a warning if the environment variable is not set, so that the administrator is aware of the configuration issue, and then default to true.
| "enabled": std::env::var("REHASH_ON_LOGIN").map(|v| !matches!(v.as_str(), "0" | "false" | "FALSE")).unwrap_or(true), | |
| "count": state.metrics.get_rehash_count() | |
| } | |
| }, | |
| "timestamp": chrono::Utc::now().to_rfc3339() |
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Resolves clippy::new-without-default warning by providing Default trait implementation for AppMetrics struct. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Adds:\n- Detailed bcrypt variant counts (2a/2b/2y) + argon2id in /health metrics.hash_distribution.\n- Rehash metrics block (enabled + count).\n- Integration test: auth_bcrypt_rehash_test verifies bcrypt -> argon2id upgrade.\n- Align simple_ws main with AppState & metrics.\n\nAll tests pass (24/24).