Overview
This issue tracks post-merge recommendations from code reviews of PR #1906 (JavaDoc documentation for PasswordHash.java).
Recommendations
1. Fix PBKDF2_ALGORITHM constant bug (Critical - P0)
Problem: The constant PBKDF2_ALGORITHM at line 135 is set to "PBKDF2WithHmacSHA2" which is not a valid Java algorithm name and will cause NoSuchAlgorithmException at runtime.
Current behavior:
- Implementation actually uses PBKDF2 with HMAC-SHA1 (algorithm id "sha1")
- Hash generation at line 248 uses "sha1:" prefix
- Verification at line 322 only accepts "sha1" algorithm
Valid options:
PBKDF2WithHmacSHA1 (matches current behavior)
PBKDF2WithHmacSHA256 (recommended upgrade)
PBKDF2WithHmacSHA384
PBKDF2WithHmacSHA512
Recommendation: Fix the constant to match actual implementation or upgrade to SHA-256.
References:
- src/main/java/ca/openosp/openo/utility/PasswordHash.java:135
- Documented in JavaDoc at lines 134-139
2. Verify BCrypt policy in CLAUDE.md (High - P1)
Problem: CodeRabbit review flagged potential policy mismatch:
- CLAUDE.md may require BCrypt for password hashing
- PasswordHash.java uses PBKDF2 with HMAC-SHA1
- Need to clarify if this is an approved legacy exception or if migration is needed
Action required:
- Review CLAUDE.md and related documentation for password hashing requirements
- Determine if PasswordHash is approved legacy or needs migration
- If migration needed, create follow-up issue for BCrypt implementation
- Update documentation to clarify the policy
References:
3. Add @SInCE tags to public methods (Low - P3)
Problem: Public methods and constructors are missing @SInCE tags per CLAUDE.md documentation standards.
Action required:
- Add @SInCE tags to all public constructors in InvalidHashException
- Add @SInCE tags to all public constructors in CannotPerformOperationException
- Add @SInCE tags to createHash() methods
- Add @SInCE tags to verifyPassword() methods
- Use accurate dates from git history
References:
- CLAUDE.md documentation standards
- CodeRabbit review comment
4. Avoid line number references in JavaDoc (Low - P3)
Problem: JavaDoc currently references "line 248" and "line 322" which will drift as code changes.
Current references:
- Line 137: "at line 248"
- Line 137: "at line 322"
- Line 240: "as seen at line 248"
Recommendation: Replace with symbol references using {@link} tags:
- Reference
{@link #createHash(char[])} instead of "line 248"
- Reference
{@link #verifyPassword(char[], String)} instead of "line 322"
References:
- src/main/java/ca/openosp/openo/utility/PasswordHash.java:137, 240
Context
Priority
Generated from code review feedback on PR #1906
Overview
This issue tracks post-merge recommendations from code reviews of PR #1906 (JavaDoc documentation for PasswordHash.java).
Recommendations
1. Fix PBKDF2_ALGORITHM constant bug (Critical - P0)
Problem: The constant
PBKDF2_ALGORITHMat line 135 is set to"PBKDF2WithHmacSHA2"which is not a valid Java algorithm name and will causeNoSuchAlgorithmExceptionat runtime.Current behavior:
Valid options:
PBKDF2WithHmacSHA1(matches current behavior)PBKDF2WithHmacSHA256(recommended upgrade)PBKDF2WithHmacSHA384PBKDF2WithHmacSHA512Recommendation: Fix the constant to match actual implementation or upgrade to SHA-256.
References:
2. Verify BCrypt policy in CLAUDE.md (High - P1)
Problem: CodeRabbit review flagged potential policy mismatch:
Action required:
References:
3. Add @SInCE tags to public methods (Low - P3)
Problem: Public methods and constructors are missing @SInCE tags per CLAUDE.md documentation standards.
Action required:
References:
4. Avoid line number references in JavaDoc (Low - P3)
Problem: JavaDoc currently references "line 248" and "line 322" which will drift as code changes.
Current references:
Recommendation: Replace with symbol references using
{@link}tags:{@link #createHash(char[])}instead of "line 248"{@link #verifyPassword(char[], String)}instead of "line 322"References:
Context
Priority
Generated from code review feedback on PR #1906