Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,12 @@
import org.keycloak.models.UserCredentialModel;
import org.keycloak.models.UserModel;
import org.keycloak.models.credential.OTPCredentialModel;
import org.keycloak.models.credential.RecoveryAuthnCodesCredentialModel;
import org.keycloak.representations.idm.CredentialRepresentation;
import org.keycloak.util.JsonSerialization;

import java.io.IOException;
import java.util.List;
import java.util.Objects;

/**
Expand Down Expand Up @@ -69,15 +73,15 @@ public static void setOrReplaceAuthenticationRequirement(KeycloakSession session
}));
}

public static ConfigurableAuthenticatorFactory getConfigurableAuthenticatorFactory(KeycloakSession session, String providerId) {
ConfigurableAuthenticatorFactory factory = (AuthenticatorFactory)session.getKeycloakSessionFactory().getProviderFactory(Authenticator.class, providerId);
if (factory == null) {
factory = (FormActionFactory)session.getKeycloakSessionFactory().getProviderFactory(FormAction.class, providerId);
}
if (factory == null) {
factory = (ClientAuthenticatorFactory)session.getKeycloakSessionFactory().getProviderFactory(ClientAuthenticator.class, providerId);
}
return factory;
public static ConfigurableAuthenticatorFactory getConfigurableAuthenticatorFactory(KeycloakSession session, String providerId) {
ConfigurableAuthenticatorFactory factory = (AuthenticatorFactory)session.getKeycloakSessionFactory().getProviderFactory(Authenticator.class, providerId);
if (factory == null) {
factory = (FormActionFactory)session.getKeycloakSessionFactory().getProviderFactory(FormAction.class, providerId);
}
if (factory == null) {
factory = (ClientAuthenticatorFactory)session.getKeycloakSessionFactory().getProviderFactory(ClientAuthenticator.class, providerId);
}
return factory;
}

/**
Expand Down Expand Up @@ -105,6 +109,27 @@ public static boolean createOTPCredential(KeycloakSession session, RealmModel re
return user.credentialManager().isValid(credential);
}

/**
* Create RecoveryCodes credential either in userStorage or local storage (Keycloak DB)
*/
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);
}
}
Comment on lines +115 to +131

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Improve exception handling and add input validation

Several issues need to be addressed in this method:

  1. The caught IOException loses context when rethrowing
  2. Missing null checks for input parameters
  3. 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.


/**
* Create "dummy" representation of the credential. Typically used when credential is provided by userStorage and we don't know further
* details about the credential besides the type
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
package org.keycloak.models.utils;

import java.util.Optional;
import java.util.function.Supplier;
import org.keycloak.common.util.Base64;
import org.keycloak.common.util.SecretGenerator;
import org.keycloak.credential.CredentialModel;
import org.keycloak.crypto.Algorithm;
import org.keycloak.crypto.JavaAlgorithm;
import org.keycloak.jose.jws.crypto.HashUtils;
import org.keycloak.models.UserModel;
import org.keycloak.models.credential.RecoveryAuthnCodesCredentialModel;

import java.nio.charset.StandardCharsets;
import java.util.List;
Expand Down Expand Up @@ -43,4 +47,17 @@ public static List<String> generateRawCodes() {
return Stream.generate(code).limit(QUANTITY_OF_CODES_TO_GENERATE).collect(Collectors.toList());
}

/**
* 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
*/
public static Optional<CredentialModel> getCredential(UserModel user) {
return user.credentialManager()
.getFederatedCredentialsStream()
.filter(c -> RecoveryAuthnCodesCredentialModel.TYPE.equals(c.getType()))
.findFirst()
.or(() -> user.credentialManager().getStoredCredentialsByTypeStream(RecoveryAuthnCodesCredentialModel.TYPE).findFirst());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,7 @@ private boolean isRecoveryAuthnCodeInputValid(AuthenticationFlowContext authnFlo
authnFlowContext.failureChallenge(AuthenticationFlowError.INVALID_CREDENTIALS, responseChallenge);
} else {
result = true;
Optional<CredentialModel> optUserCredentialFound = authenticatedUser.credentialManager().getStoredCredentialsByTypeStream(
RecoveryAuthnCodesCredentialModel.TYPE).findFirst();
Optional<CredentialModel> optUserCredentialFound = RecoveryAuthnCodesUtils.getCredential(authenticatedUser);
RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = null;
if (optUserCredentialFound.isPresent()) {
recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.Arrays;
import java.util.List;

import org.keycloak.Config;
import org.keycloak.authentication.AuthenticatorUtil;
import org.keycloak.authentication.CredentialRegistrator;
Expand All @@ -11,8 +12,6 @@
import org.keycloak.authentication.RequiredActionProvider;
import org.keycloak.authentication.authenticators.browser.RecoveryAuthnCodesFormAuthenticator;
import org.keycloak.common.Profile;
import org.keycloak.credential.RecoveryAuthnCodesCredentialProviderFactory;
import org.keycloak.credential.CredentialProvider;
import org.keycloak.events.Details;
import org.keycloak.events.EventBuilder;
import org.keycloak.events.EventType;
Expand All @@ -26,6 +25,8 @@
import jakarta.ws.rs.core.Response;
import org.keycloak.sessions.AuthenticationSessionModel;

import static org.keycloak.utils.CredentialHelper.createRecoveryCodesCredential;

public class RecoveryAuthnCodesAction implements RequiredActionProvider, RequiredActionFactory, EnvironmentDependentProviderFactory, CredentialRegistrator {

private static final String FIELD_GENERATED_RECOVERY_AUTHN_CODES_HIDDEN = "generatedRecoveryAuthnCodes";
Expand Down Expand Up @@ -86,13 +87,8 @@ public void requiredActionChallenge(RequiredActionContext context) {
public void processAction(RequiredActionContext reqActionContext) {
EventBuilder event = reqActionContext.getEvent();
event.event(EventType.UPDATE_CREDENTIAL);

CredentialProvider recoveryCodeCredentialProvider;
MultivaluedMap<String, String> httpReqParamsMap;

recoveryCodeCredentialProvider = reqActionContext.getSession().getProvider(CredentialProvider.class,
RecoveryAuthnCodesCredentialProviderFactory.PROVIDER_ID);

event.detail(Details.CREDENTIAL_TYPE, RecoveryAuthnCodesCredentialModel.TYPE);

httpReqParamsMap = reqActionContext.getHttpRequest().getDecodedFormParameters();
Expand All @@ -117,8 +113,7 @@ public void processAction(RequiredActionContext reqActionContext) {
AuthenticatorUtil.logoutOtherSessions(reqActionContext);
}

recoveryCodeCredentialProvider.createCredential(reqActionContext.getRealm(), reqActionContext.getUser(),
credentialModel);
createRecoveryCodesCredential(reqActionContext.getSession(), reqActionContext.getRealm(), reqActionContext.getUser(), credentialModel, generatedCodes);

reqActionContext.success();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,18 @@
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserModel;
import org.keycloak.models.credential.RecoveryAuthnCodesCredentialModel;
import org.keycloak.models.utils.RecoveryAuthnCodesUtils;

import java.util.Optional;

public class RecoveryAuthnCodeInputLoginBean {

private final int codeNumber;

public RecoveryAuthnCodeInputLoginBean(KeycloakSession session, RealmModel realm, UserModel user) {
CredentialModel credentialModel = user.credentialManager().getStoredCredentialsByTypeStream(RecoveryAuthnCodesCredentialModel.TYPE)
.findFirst().get();
Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);

RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel);
RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
Comment on lines +17 to +19

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@

package org.keycloak.testsuite.federation;

import java.io.IOException;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.jboss.logging.Logger;
Expand All @@ -33,7 +34,6 @@
import org.keycloak.credential.CredentialInputValidator;
import org.keycloak.credential.CredentialModel;
import org.keycloak.credential.hash.PasswordHashProvider;
import org.keycloak.credential.hash.Pbkdf2Sha512PasswordHashProviderFactory;
import org.keycloak.models.GroupModel;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.OTPPolicy;
Expand All @@ -43,6 +43,8 @@
import org.keycloak.models.UserModel;
import org.keycloak.models.cache.UserCache;
import org.keycloak.models.credential.PasswordUserCredentialModel;
import org.keycloak.models.credential.RecoveryAuthnCodesCredentialModel;
import org.keycloak.models.utils.KeycloakModelUtils;
import org.keycloak.models.utils.TimeBasedOTP;
import org.keycloak.storage.StorageId;
import org.keycloak.storage.UserStorageProvider;
Expand All @@ -51,11 +53,12 @@
import org.keycloak.storage.user.UserLookupProvider;
import org.keycloak.storage.user.UserQueryProvider;
import org.keycloak.storage.user.UserRegistrationProvider;
import org.keycloak.util.JsonSerialization;

/**
* UserStorage implementation created in Keycloak 4.8.3. It is used for backwards compatibility testing. Future Keycloak versions
* should work fine without a need to change the code of this provider.
*
* <p>
* TODO: Have some good mechanims to make sure that source code of this provider is really compatible with Keycloak 4.8.3
*
* @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
Expand Down Expand Up @@ -89,7 +92,7 @@ public UserModel getUserById(RealmModel realm, String id) {
}

private UserModel createUser(RealmModel realm, String username) {
return new AbstractUserAdapterFederatedStorage(session, realm, model) {
return new AbstractUserAdapterFederatedStorage(session, realm, model) {
@Override
public String getUsername() {
return username;
Expand All @@ -107,7 +110,8 @@ public void setUsername(String username1) {
@Override
public boolean supportsCredentialType(String credentialType) {
if (CredentialModel.PASSWORD.equals(credentialType)
|| isOTPType(credentialType)) {
|| isOTPType(credentialType)
|| credentialType.equals(RecoveryAuthnCodesCredentialModel.TYPE)) {
return true;
} else {
log.infof("Unsupported credential type: %s", credentialType);
Expand Down Expand Up @@ -172,6 +176,7 @@ public boolean updateCredential(RealmModel realm, UserModel user, CredentialInpu
OTPPolicy otpPolicy = session.getContext().getRealm().getOTPPolicy();

CredentialModel newOTP = new CredentialModel();
newOTP.setId(KeycloakModelUtils.generateId());
newOTP.setType(input.getType());
long createdDate = Time.currentTimeMillis();
newOTP.setCreatedDate(createdDate);
Expand All @@ -184,6 +189,15 @@ public boolean updateCredential(RealmModel realm, UserModel user, CredentialInpu

users.get(translateUserName(user.getUsername())).otp = newOTP;

return true;
} 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;
Comment on lines +193 to 201

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

} else {
log.infof("Attempt to update unsupported credential of type: %s", input.getType());
Expand Down Expand Up @@ -213,6 +227,30 @@ private MyUser getMyUser(UserModel user) {
return users.get(translateUserName(user.getUsername()));
}

@Override
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();
}
Comment on lines +231 to +252

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


@Override
public Stream<String> getDisableableCredentialTypesStream(RealmModel realm, UserModel user) {
Set<String> types = new HashSet<>();
Expand All @@ -234,6 +272,8 @@ public boolean isConfiguredFor(RealmModel realm, UserModel user, String credenti

if (isOTPType(credentialType) && myUser.otp != null) {
return true;
} else if (credentialType.equals(RecoveryAuthnCodesCredentialModel.TYPE) && myUser.recoveryCodes != null) {
return true;
} else {
log.infof("Not supported credentialType '%s' for user '%s'", credentialType, user.getUsername());
return false;
Expand Down Expand Up @@ -283,7 +323,22 @@ public boolean isValid(RealmModel realm, UserModel user, CredentialInput input)
TimeBasedOTP validator = new TimeBasedOTP(storedOTPCredential.getAlgorithm(), storedOTPCredential.getDigits(),
storedOTPCredential.getPeriod(), realm.getOTPPolicy().getLookAheadWindow());
return validator.validateTOTP(otpCredential.getValue(), storedOTPCredential.getValue().getBytes());
} 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;
}
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 {
Comment on lines +326 to +341

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

log.infof("Not supported to validate credential of type '%s' for user '%s'", input.getType(), user.getUsername());
return false;
}
Expand Down Expand Up @@ -369,6 +424,7 @@ static class MyUser {
private String username;
private CredentialModel hashedPassword;
private CredentialModel otp;
private CredentialModel recoveryCodes;

private MyUser(String username) {
this.username = username;
Expand All @@ -377,6 +433,10 @@ private MyUser(String username) {
public CredentialModel getOtp() {
return otp;
}

public CredentialModel getRecoveryCodes() {
return recoveryCodes;
}
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,9 @@ public boolean hasUserOTP(String username) {
return user.getOtp() != null;
}

public boolean hasRecoveryCodes(String username) {
BackwardsCompatibilityUserStorage.MyUser user = userPasswords.get(username);
if (user == null) return false;
return user.getRecoveryCodes() != null;
}
}
Loading