Skip to content
Jump to bottom

Automated Test: feature-recovery-keys-implementation #308

Closed
wants to merge 1 commit into from
+256 −31
Closed

Automated Test: feature-recovery-keys-implementation #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 34 additions & 9 deletions server-spi-private/src/main/java/org/keycloak/utils/CredentialHelper.java
View file
Open in desktop
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);
}
}

/**
* 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
17 changes: 17 additions & 0 deletions server-spi/src/main/java/org/keycloak/models/utils/RecoveryAuthnCodesUtils.java
View file
Open in desktop
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());
}
}
3 changes: 1 addition & 2 deletions ...g/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java
View file
Open in desktop
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
13 changes: 4 additions & 9 deletions ...s/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java
View file
Open in desktop
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
8 changes: 5 additions & 3 deletions .../main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java
View file
Open in desktop
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

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Unchecked Optional.get() will throw NoSuchElementException if credential is missing.

credentialModelOpt.get() on Line 19 is called without a presence check. While this bean should only be constructed when the user has recovery codes configured, a race condition or storage inconsistency could leave the Optional empty, resulting in a 500 error during login.

Proposed fix — fail with a clear message 
-        Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
-
-        RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
+        CredentialModel credentialModel = RecoveryAuthnCodesUtils.getCredential(user)
+                .orElseThrow(() -> new IllegalStateException("Recovery codes credential not found for user"));
+
+        RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel);
📝 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());
CredentialModel credentialModel = RecoveryAuthnCodesUtils.getCredential(user)
.orElseThrow(() -> new IllegalStateException("Recovery codes credential not found for user"));
RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel);
🤖 Prompt for AI Agents 
In
`@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java`
around lines 17 - 19, The code calls credentialModelOpt.get() without checking
presence, which can throw NoSuchElementException; in
RecoveryAuthnCodeInputLoginBean replace the direct get with a presence check or
use credentialModelOpt.orElseThrow(...) and throw a clear, specific runtime
exception (e.g. IllegalStateException) with a descriptive message like "Recovery
codes credential missing for user" before calling
RecoveryAuthnCodesCredentialModel.createFromCredentialModel; reference
RecoveryAuthnCodesUtils.getCredential, the credentialModelOpt variable, and
RecoveryAuthnCodesCredentialModel.createFromCredentialModel when making the
change.


this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber();
}
Expand Down
72 changes: 66 additions & 6 deletions ...rs/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java
View file
Open in desktop
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;
} 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 +230 to +252
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Null-pointer risk: getMyUser(user) can return null.

getMyUser returns null when the username is not in the map. Accessing myUser.recoveryCodes on Line 235 or myUser.otp on Line 247 will throw a NullPointerException. Other methods in this class (e.g., getDisableableCredentialTypesStream at Line 258) guard against this.

Proposed fix 
     public Stream<CredentialModel> getCredentials(RealmModel realm, UserModel user) {
         var myUser = getMyUser(user);
+        if (myUser == null) return Stream.empty();
         RecoveryAuthnCodesCredentialModel model;
         List<CredentialModel> credentialModels = new ArrayList<>();
🤖 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 230 - 252, getCredentials currently calls getMyUser(user) without
null-check and then dereferences myUser (accessing myUser.recoveryCodes and
myUser.otp), which can cause NPE; update the getCredentials method to guard
against a null myUser (similar to getDisableableCredentialTypesStream) by
returning an empty Stream when getMyUser(user) is null, or by early-returning
credentialModels.stream() before accessing fields, and keep existing
deserialization/error logging for recoveryCodes and adding otp only when myUser
!= null.


@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

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Recovery code validation doesn't consume the used code.

The standard RecoveryAuthnCodesCredentialProvider marks a code as used after successful validation and advances the counter. This test storage accepts any stored code without consumption, meaning a single code can be reused indefinitely. If tests rely on code exhaustion behavior (e.g., triggering CONFIGURE_RECOVERY_AUTHN_CODES required action), they may pass incorrectly.

This may be acceptable for the current test scope — flagging for awareness.

🤖 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 326 - 341, The recovery-code validation in
BackwardsCompatibilityUserStorage accepts a matching code but doesn't consume
it; modify the block handling RecoveryAuthnCodesCredentialModel.TYPE so that
when generatedKeys.stream().anyMatch(...) finds a match you remove that specific
key from the generatedKeys list, serialize the updated list back into
storedRecoveryKeys.getCredentialData() (e.g., via
JsonSerialization.writeValueAsString) and persist the updated CredentialModel
(myUser.recoveryCodes) so the used code can't be reused; ensure you still return
true for successful validation and return false/fail on
serialization/persistence errors.

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
5 changes: 5 additions & 0 deletions ...main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorageFactory.java
View file
Open in desktop
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;
}
}