Implement recovery key support for user storage providers #9
Conversation
closes #38445 Signed-off-by: rtufisi <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements recovery key support for user storage providers by extending Keycloak's authentication system to handle recovery codes stored in federated user storage. The implementation creates a dual storage approach where recovery codes can be stored either in external user storage providers (like LDAP) or fall back to Keycloak's local storage.
Key changes include:
- Centralized credential retrieval: A new utility method
RecoveryAuthnCodesUtils.getCredential()that checks federated credentials first, then falls back to local storage - Abstracted credential creation: Introduction of
CredentialHelper.createRecoveryCodesCredential()that handles both user storage and local database creation - Enhanced test infrastructure: Updates to the backwards compatibility test framework to support recovery codes, including comprehensive integration tests
- Updated authentication flows: Refactoring of authenticators and required actions to use the new unified credential handling approach
The implementation follows established patterns from existing OTP credential handling, ensuring consistency with Keycloak's authentication architecture. This change enables organizations using external user directories to store and manage recovery codes in their preferred storage systems while maintaining backward compatibility.
PR Description Notes:
- The PR body only contains "Test 9" which provides no meaningful description of the changes
Confidence score: 3/5
• This PR introduces some risk due to potential NullPointerException issues and incomplete error handling in critical authentication paths
• The score reflects concerns about unsafe Optional handling in RecoveryAuthnCodeInputLoginBean and lack of comprehensive error handling in the backwards compatibility provider
• Files needing attention: RecoveryAuthnCodeInputLoginBean.java for the unsafe .get() calls on Optional, and BackwardsCompatibilityUserStorage.java for error handling improvements
8 files reviewed, 7 comments
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | ||
|
|
||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | ||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); |
There was a problem hiding this comment.
logic: Calling .get() on Optional without checking if present first will throw NoSuchElementException if no recovery credential exists. Should check credentialModelOpt.isPresent() or use orElseThrow() with meaningful error message.
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.orElseThrow(() -> new IllegalStateException("No recovery authentication codes credential found for user"))); |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | ||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); | ||
|
|
||
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber(); |
There was a problem hiding this comment.
logic: Double .get() calls without null checks - both getNextRecoveryAuthnCode() and the outer get() can throw exceptions if no next code exists.
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber(); | |
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().orElseThrow(() -> new IllegalStateException("No next recovery authentication code available")).getNumber(); |
| * Checks the user storage for the credential. If not found it will look for the credential in the local storage | ||
| * | ||
| * @param user - User model | ||
| * @return - a optional credential model |
There was a problem hiding this comment.
syntax: Minor typo in comment: 'a optional' should be 'an optional'
| * @return - a optional credential model | |
| * @return - an optional credential model |
| JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class), | ||
| myUser.recoveryCodes.getCreatedDate(), | ||
| myUser.recoveryCodes.getUserLabel() |
There was a problem hiding this comment.
logic: Recovery codes are deserialized as raw List.class but should be typed as List<String> to match the expected data structure
| ); | ||
| credentialModels.add(model); | ||
| } catch (IOException e) { | ||
| log.error("Could not deserialize credential of type: recovery-codes"); |
There was a problem hiding this comment.
syntax: Error message has double space between 'deserialize' and 'credential'
| log.error("Could not deserialize credential of type: recovery-codes"); | |
| log.error("Could not deserialize credential of type: recovery-codes"); |
| List generatedKeys; | ||
| try { | ||
| generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); |
There was a problem hiding this comment.
style: Raw List type should be List<String> to properly handle recovery code validation
| .addSubFlowExecution("Recovery-Authn-Codes subflow", AuthenticationFlow.BASIC_FLOW, AuthenticationExecutionModel.Requirement.ALTERNATIVE, altSubFlow -> altSubFlow | ||
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, RecoveryAuthnCodesFormAuthenticatorFactory.PROVIDER_ID) | ||
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, "delayed-authenticator", config -> { | ||
| config.setAlias("delayed-suthenticator-config"); |
There was a problem hiding this comment.
syntax: Typo in config alias: 'delayed-suthenticator-config' should be 'delayed-authenticator-config'
| config.setAlias("delayed-suthenticator-config"); | |
| config.setAlias("delayed-authenticator-config"); |
2 participants
Test 9