Implement recovery key support for user storage providers

[akshayutture-augment]

Test 9

---

Replicated from ai-code-review-evaluation/keycloak-greptile#9

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review: Implement recovery key support for user storage providers

I've completed a thorough review of this PR. The implementation adds support for recovery authentication codes in user storage providers, which is a valuable feature. Below are my findings:

---

Critical Issues

1. Potential NullPointerException in RecoveryAuthnCodeInputLoginBean

Location: services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java:19

RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());

Issue: The code calls .get() on an Optional without checking if it's present. If the credential is not found, this will throw a NoSuchElementException.

Recommendation: Add proper null/empty handling:

if (credentialModelOpt.isEmpty()) {
    throw new IllegalStateException("No recovery codes credential found for user");
}
RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = 
    RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());

2. Unsafe Exception Wrapping in CredentialHelper

Location: server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java:115-122

try {
    recoveryCodesJson = JsonSerialization.writeValueAsString(generatedCodes);
} catch (IOException e) {
    throw new RuntimeException(e);
}

Issue: Catching IOException and wrapping it in a generic RuntimeException loses important context and makes debugging harder. This could mask serialization failures.

Recommendation: Either:

• Throw a more specific exception with a descriptive message
• Let the IOException propagate (update method signature)
• Log the error before throwing

---

Security Concerns

3. Plain Text Recovery Code Comparison in User Storage

Location: testsuite/.../BackwardsCompatibilityUserStorage.java:340

return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse()));

Issue: The test user storage implementation validates recovery codes using plain string comparison. While this is in test code, it's inconsistent with the main implementation which uses hashing (RecoveryAuthnCodesUtils.hashRawCode()). This could mislead developers about proper recovery code handling.

Recommendation: Consider adding a comment explaining why this differs from production code, or better yet, use the same hashing mechanism for consistency:

// Note: This simplified validation is for testing only
// Production code should use RecoveryAuthnCodesUtils.verifyRecoveryCodeInput()

4. Missing Input Validation

Location: server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java:115

Issue: No validation that generatedCodes is non-null or non-empty before serialization.

Recommendation: Add validation:

if (generatedCodes == null || generatedCodes.isEmpty()) {
    throw new IllegalArgumentException("Generated codes cannot be null or empty");
}

---

Code Quality Issues

5. Inconsistent Error Handling in BackwardsCompatibilityUserStorage

Location: testsuite/.../BackwardsCompatibilityUserStorage.java:243-244

} catch (IOException e) {
    log.error("Could not deserialize credential of type: recovery-codes");
}

Issue: The exception is logged but then silently swallowed, and the method continues execution. The credential is not added to the list, which could lead to confusing behavior where credentials appear to not exist.

Recommendation: Either:

• Re-throw the exception or return an error indicator
• Add the exception details to the log message: log.error("Could not deserialize credential", e)

6. Code Formatting Inconsistency

Location: server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java:76-85

Issue: The PR includes reformatting of the getConfigurableAuthenticatorFactory method (changing indentation) which appears to be unrelated to the recovery codes feature. This makes the diff harder to review.

Recommendation: Consider separating formatting changes from functional changes, or avoid reformatting unrelated code.

7. Unused Import Cleanup Missing

Location: services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java

Issue: The PR removes imports for RecoveryAuthnCodesCredentialProviderFactory and CredentialProvider but adds a static import. Good cleanup, but verify all unused imports are removed.

---

Testing Observations

8. Good Test Coverage

Positive: The test testRecoveryKeysSetupAndLogin() in BackwardsCompatibilityUserStorageTest.java provides good coverage of the new functionality, including:

• Recovery key setup
• Validation that keys are stored in user storage (not DB)
• Authentication flow with recovery codes

Suggestion: Consider adding negative test cases:

• What happens when all recovery codes are exhausted?
• Invalid/tampered recovery code input
• Recovery code validation with brute force protection

---

Architecture & Design

9. Good Abstraction with RecoveryAuthnCodesUtils.getCredential()

Positive: The new getCredential() method properly abstracts the logic of checking both federated and local storage. This is used consistently across:

• RecoveryAuthnCodesFormAuthenticator.java:80
• RecoveryAuthnCodeInputLoginBean.java:17

This follows the DRY principle and makes the code more maintainable.

10. Consistent Pattern with OTP Credential Handling

Positive: The implementation of createRecoveryCodesCredential() follows the same pattern as the existing createOTPCredential() method, which provides good consistency in the codebase.

---

Minor Issues

11. Typo in Variable Name

Location: server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java:116

var recoveryCodeCredentialProvider = ...

Issue: Uses var keyword which is fine, but the naming could be more consistent with the pattern used in createOTPCredential (line 93).

---

Summary

Overall Assessment: The PR implements a useful feature with a reasonable approach. The main concerns are:

1. ⚠️ Critical: Potential NPE in RecoveryAuthnCodeInputLoginBean (must fix)
2. ⚠️ Important: Better exception handling needed in CredentialHelper
3. ℹ️ Minor: Several code quality improvements recommended

Recommended Actions:

1. Fix the NPE risk in RecoveryAuthnCodeInputLoginBean.java:19
2. Improve error handling for JSON serialization
3. Add input validation for the generatedCodes parameter
4. Consider adding more negative test cases

Strengths:

• Good test coverage for the happy path
• Consistent with existing OTP credential patterns
• Proper abstraction with the new utility method
• Works correctly with both federated and local storage

---