Implement recovery key support for user storage providers#9
Open
everettbu wants to merge 1 commit into
Open
Conversation
closes #38445 Signed-off-by: rtufisi <rtufisi@phasetwo.io>
WalkthroughThese changes introduce support for recovery authentication codes as a credential type in both the main application and a legacy user storage provider. New utility and helper methods are added for creating, retrieving, and managing recovery codes. The test suite is expanded to verify recovery code setup and login scenarios, ensuring backward compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Authenticator
participant CredentialHelper
participant UserStorage
participant LocalDB
User->>UI: Initiate recovery code setup
UI->>Authenticator: Request to generate recovery codes
Authenticator->>CredentialHelper: createRecoveryCodesCredential(session, realm, user, model, codes)
CredentialHelper->>UserStorage: Try updateCredential with recovery codes
alt Success
UserStorage-->>CredentialHelper: Update OK
CredentialHelper->>Authenticator: Return success
else Failure
CredentialHelper->>LocalDB: Create credential in local DB
LocalDB-->>CredentialHelper: Store credential
CredentialHelper->>Authenticator: Return success
end
Authenticator->>UI: Show recovery codes to user
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (1)
135-135: Fix typo in authenticator aliasThere's a typo in the authenticator alias name.
- config.setAlias("delayed-suthenticator-config"); + config.setAlias("delayed-authenticator-config");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java(3 hunks)server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java(2 hunks)services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java(1 hunks)services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java(3 hunks)services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java(1 hunks)testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java(12 hunks)testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorageFactory.java(1 hunks)testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java(8 hunks)
🔇 Additional comments (6)
services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java (1)
80-80: LGTM: Good refactoring to use unified credential retrieval.The change to use
RecoveryAuthnCodesUtils.getCredential(authenticatedUser)properly centralizes credential retrieval logic and supports both federated and local storage scenarios.testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorageFactory.java (1)
53-57: LGTM: Consistent implementation following established pattern.The
hasRecoveryCodesmethod correctly mirrors thehasUserOTPimplementation pattern and provides the necessary functionality for testing recovery codes in the backward compatibility user storage provider.server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java (1)
50-62: LGTM: Well-implemented utility method with proper fallback logic.The
getCredentialmethod correctly implements the federated-first, local-fallback pattern for credential retrieval. The use ofOptional.or()provides clean fallback semantics, and the Javadoc clearly explains the method's behavior.services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java (1)
28-28: LGTM: Good refactoring to centralize credential creation logic.The changes properly delegate credential creation to
CredentialHelper.createRecoveryCodesCredential, which centralizes the logic for handling both federated and local storage scenarios. The static import usage is appropriate here.Also applies to: 116-116
server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java (1)
119-119: Ensure Recovery Codes Are Secured at Rest
I didn’t find any code-level encryption or hashing in theRecoveryAuthnCodesCredentialProvider/model. The recovery codes are serialized to plaintext JSON viarecoveryCodesJson = JsonSerialization.writeValueAsString(generatedCodes);and persisted in
RecoveryAuthnCodesCredentialModel.secretDataunencrypted—just like OTP secrets. Please manually verify that:
• The credential provider or underlying storage layer (DB, vault, etc.) encrypts or otherwise protects these codes at rest.
• If not, consider hashing (e.g. HMAC or salted hash) the codes before persisting to avoid storing them in plaintext.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (1)
240-269: Well-structured test implementationThe test properly covers the recovery codes setup and login flow with appropriate cleanup in the finally block.
Comment on lines
+115
to
+131
| public static void createRecoveryCodesCredential(KeycloakSession session, RealmModel realm, UserModel user, RecoveryAuthnCodesCredentialModel credentialModel, List<String> generatedCodes) { | ||
| var recoveryCodeCredentialProvider = session.getProvider(CredentialProvider.class, "keycloak-recovery-authn-codes"); | ||
| String recoveryCodesJson; | ||
| try { | ||
| recoveryCodesJson = JsonSerialization.writeValueAsString(generatedCodes); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| UserCredentialModel recoveryCodesCredential = new UserCredentialModel("", credentialModel.getType(), recoveryCodesJson); | ||
|
|
||
| boolean userStorageCreated = user.credentialManager().updateCredential(recoveryCodesCredential); | ||
| if (userStorageCreated) { | ||
| logger.debugf("Created RecoveryCodes credential for user '%s' in the user storage", user.getUsername()); | ||
| } else { | ||
| recoveryCodeCredentialProvider.createCredential(realm, user, credentialModel); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Improve exception handling and add input validation
Several issues need to be addressed in this method:
- The caught IOException loses context when rethrowing
- Missing null checks for input parameters
- No logging when creating credentials locally (inconsistent with OTP creation)
Apply this diff to fix these issues:
public static void createRecoveryCodesCredential(KeycloakSession session, RealmModel realm, UserModel user, RecoveryAuthnCodesCredentialModel credentialModel, List<String> generatedCodes) {
+ if (session == null || realm == null || user == null || credentialModel == null || generatedCodes == null || generatedCodes.isEmpty()) {
+ throw new IllegalArgumentException("Invalid parameters for creating recovery codes credential");
+ }
var recoveryCodeCredentialProvider = session.getProvider(CredentialProvider.class, "keycloak-recovery-authn-codes");
String recoveryCodesJson;
try {
recoveryCodesJson = JsonSerialization.writeValueAsString(generatedCodes);
} catch (IOException e) {
- throw new RuntimeException(e);
+ throw new RuntimeException("Failed to serialize recovery codes", e);
}
UserCredentialModel recoveryCodesCredential = new UserCredentialModel("", credentialModel.getType(), recoveryCodesJson);
boolean userStorageCreated = user.credentialManager().updateCredential(recoveryCodesCredential);
if (userStorageCreated) {
logger.debugf("Created RecoveryCodes credential for user '%s' in the user storage", user.getUsername());
} else {
- recoveryCodeCredentialProvider.createCredential(realm, user, credentialModel);
+ CredentialModel createdCredential = recoveryCodeCredentialProvider.createCredential(realm, user, credentialModel);
+ logger.debugf("Created RecoveryCodes credential for user '%s' in local storage with id '%s'", user.getUsername(), createdCredential.getId());
}
}🤖 Prompt for AI Agents
In server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java
lines 115 to 131, improve exception handling by catching IOException and
rethrowing it with a descriptive message to preserve context, add null checks
for all input parameters at the start of the method to prevent
NullPointerExceptions, and add appropriate logging when creating credentials
locally to maintain consistency with OTP creation logging. Implement these
changes to enhance robustness and traceability.
Comment on lines
+17
to
+19
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | ||
|
|
||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | ||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); |
There was a problem hiding this comment.
Potential NoSuchElementException on Optional.get().
Line 19 calls .get() on the Optional without checking if it's present first. This will throw a NoSuchElementException if no recovery codes credential is found.
Consider adding a safety check:
-Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
-
-RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
+Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
+
+RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(
+ credentialModelOpt.orElseThrow(() -> new IllegalStateException("Recovery codes credential not found for user")));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); | |
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel( | |
| credentialModelOpt.orElseThrow(() -> new IllegalStateException("Recovery codes credential not found for user"))); |
🤖 Prompt for AI Agents
In
services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java
around lines 17 to 19, the code calls .get() on an Optional without checking if
a value is present, risking a NoSuchElementException. To fix this, add a check
to verify if the Optional contains a value before calling .get(), such as using
isPresent() or orElseThrow() with a clear exception, and handle the case where
the credential is absent appropriately.
Comment on lines
+193
to
201
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | ||
| CredentialModel recoveryCodesModel = new CredentialModel(); | ||
| recoveryCodesModel.setId(KeycloakModelUtils.generateId()); | ||
| recoveryCodesModel.setType(input.getType()); | ||
| recoveryCodesModel.setCredentialData(input.getChallengeResponse()); | ||
| long createdDate = Time.currentTimeMillis(); | ||
| recoveryCodesModel.setCreatedDate(createdDate); | ||
| users.get(translateUserName(user.getUsername())).recoveryCodes = recoveryCodesModel; | ||
| return true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for recovery codes input
The method stores recovery codes without validating the input format or content.
} else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) {
+ String credentialData = input.getChallengeResponse();
+ if (credentialData == null || credentialData.trim().isEmpty()) {
+ log.warnf("Empty recovery codes data for user %s", user.getUsername());
+ return false;
+ }
+ // Validate that the data is valid JSON array
+ try {
+ JsonSerialization.readValue(credentialData, List.class);
+ } catch (IOException e) {
+ log.warnf("Invalid recovery codes format for user %s: %s", user.getUsername(), e.getMessage());
+ return false;
+ }
CredentialModel recoveryCodesModel = new CredentialModel();
recoveryCodesModel.setId(KeycloakModelUtils.generateId());
recoveryCodesModel.setType(input.getType());
- recoveryCodesModel.setCredentialData(input.getChallengeResponse());
+ recoveryCodesModel.setCredentialData(credentialData);
long createdDate = Time.currentTimeMillis();
recoveryCodesModel.setCreatedDate(createdDate);
users.get(translateUserName(user.getUsername())).recoveryCodes = recoveryCodesModel;
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | |
| CredentialModel recoveryCodesModel = new CredentialModel(); | |
| recoveryCodesModel.setId(KeycloakModelUtils.generateId()); | |
| recoveryCodesModel.setType(input.getType()); | |
| recoveryCodesModel.setCredentialData(input.getChallengeResponse()); | |
| long createdDate = Time.currentTimeMillis(); | |
| recoveryCodesModel.setCreatedDate(createdDate); | |
| users.get(translateUserName(user.getUsername())).recoveryCodes = recoveryCodesModel; | |
| return true; | |
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | |
| String credentialData = input.getChallengeResponse(); | |
| if (credentialData == null || credentialData.trim().isEmpty()) { | |
| log.warnf("Empty recovery codes data for user %s", user.getUsername()); | |
| return false; | |
| } | |
| // Validate that the data is valid JSON array | |
| try { | |
| JsonSerialization.readValue(credentialData, List.class); | |
| } catch (IOException e) { | |
| log.warnf("Invalid recovery codes format for user %s: %s", user.getUsername(), e.getMessage()); | |
| return false; | |
| } | |
| CredentialModel recoveryCodesModel = new CredentialModel(); | |
| recoveryCodesModel.setId(KeycloakModelUtils.generateId()); | |
| recoveryCodesModel.setType(input.getType()); | |
| recoveryCodesModel.setCredentialData(credentialData); | |
| long createdDate = Time.currentTimeMillis(); | |
| recoveryCodesModel.setCreatedDate(createdDate); | |
| users.get(translateUserName(user.getUsername())).recoveryCodes = recoveryCodesModel; | |
| return true; |
🤖 Prompt for AI Agents
In
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java
around lines 193 to 201, the code stores recovery codes without validating the
input. Add validation logic to check the format and content of
input.getChallengeResponse() before creating and setting the recoveryCodesModel.
If the input is invalid, handle it appropriately by returning false or throwing
an exception to prevent storing malformed recovery codes.
Comment on lines
+231
to
+252
| public Stream<CredentialModel> getCredentials(RealmModel realm, UserModel user) { | ||
| var myUser = getMyUser(user); | ||
| RecoveryAuthnCodesCredentialModel model; | ||
| List<CredentialModel> credentialModels = new ArrayList<>(); | ||
| if (myUser.recoveryCodes != null) { | ||
| try { | ||
| model = RecoveryAuthnCodesCredentialModel.createFromValues( | ||
| JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class), | ||
| myUser.recoveryCodes.getCreatedDate(), | ||
| myUser.recoveryCodes.getUserLabel() | ||
| ); | ||
| credentialModels.add(model); | ||
| } catch (IOException e) { | ||
| log.error("Could not deserialize credential of type: recovery-codes"); | ||
| } | ||
| } | ||
| if (myUser.otp != null) { | ||
| credentialModels.add(myUser.getOtp()); | ||
| } | ||
|
|
||
| return credentialModels.stream(); | ||
| } |
There was a problem hiding this comment.
Improve error handling in getCredentials method
The method silently continues after deserialization failure and uses var keyword which might not be compatible with the target Java version.
@Override
public Stream<CredentialModel> getCredentials(RealmModel realm, UserModel user) {
- var myUser = getMyUser(user);
+ MyUser myUser = getMyUser(user);
RecoveryAuthnCodesCredentialModel model;
List<CredentialModel> credentialModels = new ArrayList<>();
if (myUser.recoveryCodes != null) {
try {
model = RecoveryAuthnCodesCredentialModel.createFromValues(
JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class),
myUser.recoveryCodes.getCreatedDate(),
myUser.recoveryCodes.getUserLabel()
);
credentialModels.add(model);
} catch (IOException e) {
- log.error("Could not deserialize credential of type: recovery-codes");
+ log.error("Could not deserialize credential of type: recovery-codes", e);
+ // Consider whether to throw an exception or return partial results
}
}
if (myUser.otp != null) {
credentialModels.add(myUser.getOtp());
}
return credentialModels.stream();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public Stream<CredentialModel> getCredentials(RealmModel realm, UserModel user) { | |
| var myUser = getMyUser(user); | |
| RecoveryAuthnCodesCredentialModel model; | |
| List<CredentialModel> credentialModels = new ArrayList<>(); | |
| if (myUser.recoveryCodes != null) { | |
| try { | |
| model = RecoveryAuthnCodesCredentialModel.createFromValues( | |
| JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class), | |
| myUser.recoveryCodes.getCreatedDate(), | |
| myUser.recoveryCodes.getUserLabel() | |
| ); | |
| credentialModels.add(model); | |
| } catch (IOException e) { | |
| log.error("Could not deserialize credential of type: recovery-codes"); | |
| } | |
| } | |
| if (myUser.otp != null) { | |
| credentialModels.add(myUser.getOtp()); | |
| } | |
| return credentialModels.stream(); | |
| } | |
| @Override | |
| public Stream<CredentialModel> getCredentials(RealmModel realm, UserModel user) { | |
| MyUser myUser = getMyUser(user); | |
| RecoveryAuthnCodesCredentialModel model; | |
| List<CredentialModel> credentialModels = new ArrayList<>(); | |
| if (myUser.recoveryCodes != null) { | |
| try { | |
| model = RecoveryAuthnCodesCredentialModel.createFromValues( | |
| JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class), | |
| myUser.recoveryCodes.getCreatedDate(), | |
| myUser.recoveryCodes.getUserLabel() | |
| ); | |
| credentialModels.add(model); | |
| } catch (IOException e) { | |
| log.error("Could not deserialize credential of type: recovery-codes", e); | |
| // Consider whether to throw an exception or return partial results | |
| } | |
| } | |
| if (myUser.otp != null) { | |
| credentialModels.add(myUser.getOtp()); | |
| } | |
| return credentialModels.stream(); | |
| } |
🤖 Prompt for AI Agents
In
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java
between lines 231 and 252, improve error handling in the getCredentials method
by explicitly declaring the type of the RecoveryAuthnCodesCredentialModel
variable instead of using 'var' for compatibility, and modify the catch block to
handle the IOException more robustly, such as rethrowing it or returning an
empty stream to avoid silent failures after deserialization errors.
Comment on lines
+326
to
+341
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | ||
| CredentialModel storedRecoveryKeys = myUser.recoveryCodes; | ||
| if (storedRecoveryKeys == null) { | ||
| log.warnf("Not found credential for the user %s", user.getUsername()); | ||
| return false; | ||
| } | ||
| List generatedKeys; | ||
| try { | ||
| generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); | ||
| } catch (IOException e) { | ||
| log.warnf("Cannot deserialize recovery keys credential for the user %s", user.getUsername()); | ||
| return false; | ||
| } | ||
|
|
||
| return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse())); | ||
| } else { |
There was a problem hiding this comment.
Critical: Recovery codes should be single-use
Recovery codes are typically single-use for security reasons, but this implementation doesn't remove used codes after successful validation. Additionally, the validation logic needs improvement.
} else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) {
CredentialModel storedRecoveryKeys = myUser.recoveryCodes;
if (storedRecoveryKeys == null) {
log.warnf("Not found credential for the user %s", user.getUsername());
return false;
}
+ String inputCode = input.getChallengeResponse();
+ if (inputCode == null || inputCode.trim().isEmpty()) {
+ return false;
+ }
List generatedKeys;
try {
generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class);
} catch (IOException e) {
log.warnf("Cannot deserialize recovery keys credential for the user %s", user.getUsername());
return false;
}
- return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse()));
+ // Check if the code matches and remove it for single-use
+ boolean found = generatedKeys.removeIf(key -> key.toString().trim().equals(inputCode.trim()));
+ if (found) {
+ // Update the stored credential with remaining codes
+ try {
+ storedRecoveryKeys.setCredentialData(JsonSerialization.writeValueAsString(generatedKeys));
+ } catch (IOException e) {
+ log.errorf(e, "Failed to update recovery codes for user %s", user.getUsername());
+ return false;
+ }
+ }
+ return found;
}Note: This is a backwards compatibility test provider, so the single-use behavior might be intentionally omitted. Please verify if this matches the expected behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | |
| CredentialModel storedRecoveryKeys = myUser.recoveryCodes; | |
| if (storedRecoveryKeys == null) { | |
| log.warnf("Not found credential for the user %s", user.getUsername()); | |
| return false; | |
| } | |
| List generatedKeys; | |
| try { | |
| generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); | |
| } catch (IOException e) { | |
| log.warnf("Cannot deserialize recovery keys credential for the user %s", user.getUsername()); | |
| return false; | |
| } | |
| return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse())); | |
| } else { | |
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | |
| CredentialModel storedRecoveryKeys = myUser.recoveryCodes; | |
| if (storedRecoveryKeys == null) { | |
| log.warnf("Not found credential for the user %s", user.getUsername()); | |
| return false; | |
| } | |
| String inputCode = input.getChallengeResponse(); | |
| if (inputCode == null || inputCode.trim().isEmpty()) { | |
| return false; | |
| } | |
| List generatedKeys; | |
| try { | |
| generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); | |
| } catch (IOException e) { | |
| log.warnf("Cannot deserialize recovery keys credential for the user %s", user.getUsername()); | |
| return false; | |
| } | |
| // Check if the code matches and remove it for single-use | |
| boolean found = generatedKeys.removeIf(key -> key.toString().trim().equals(inputCode.trim())); | |
| if (found) { | |
| // Update the stored credential with remaining codes | |
| try { | |
| storedRecoveryKeys.setCredentialData(JsonSerialization.writeValueAsString(generatedKeys)); | |
| } catch (IOException e) { | |
| log.errorf(e, "Failed to update recovery codes for user %s", user.getUsername()); | |
| return false; | |
| } | |
| } | |
| return found; | |
| } else { |
🤖 Prompt for AI Agents
In
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java
between lines 326 and 341, the recovery codes validation currently does not
remove a code after it is used, which is a security risk since recovery codes
should be single-use. To fix this, after confirming a code matches the input
challenge response, remove that code from the stored list, update the stored
credential data with the modified list, and persist the change to ensure the
code cannot be reused. Verify if this single-use behavior aligns with the
backwards compatibility requirements before implementing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 9
Summary by CodeRabbit
New Features
Bug Fixes
Tests