-
Notifications
You must be signed in to change notification settings - Fork 2
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential NoSuchElementException on Optional.get(). Line 19 calls 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.infof("Attempt to update unsupported credential of type: %s", input.getType()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve error handling in getCredentials method The method silently continues after deserialization failure and uses @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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public Stream<String> getDisableableCredentialTypesStream(RealmModel realm, UserModel user) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Set<String> types = new HashSet<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.infof("Not supported to validate credential of type '%s' for user '%s'", input.getType(), user.getUsername()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -377,6 +433,10 @@ private MyUser(String username) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public CredentialModel getOtp() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return otp; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public CredentialModel getRecoveryCodes() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return recoveryCodes; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Critical: Improve exception handling and add input validation
Several issues need to be addressed in this method:
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