-
Notifications
You must be signed in to change notification settings - Fork 10
Implement recovery key support for user storage providers #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature-recovery-keys-foundation
Are you sure you want to change the base?
Implement recovery key support for user storage providers #9
Conversation
closes #38445 Signed-off-by: rtufisi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Typo in config alias: 'delayed-suthenticator-config' should be 'delayed-authenticator-config'
| config.setAlias("delayed-suthenticator-config"); | |
| config.setAlias("delayed-authenticator-config"); |
Test 9