Implemented validation to ensure each OTP device has a unique label

Closes #38465

Signed-off-by: Saikrishna <saikrishnap@optimeyes.ai>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Co-authored-by: Saikrishna <saikrishnap@optimeyes.ai>
Co-authored-by: Alexander Schwartz <aschwart@redhat.com>
This commit is contained in:
Pavuluri Sai Krishna 2025-06-12 15:38:05 +05:30 committed by GitHub
parent 21bd46cb18
commit 76ab8bd21d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
14 changed files with 234 additions and 30 deletions

View File

@ -21,6 +21,7 @@ import org.keycloak.common.util.Base64;
import org.keycloak.credential.CredentialModel;
import org.keycloak.credential.UserCredentialStore;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.ModelDuplicateException;
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserModel;
import org.keycloak.models.jpa.entities.CredentialEntity;
@ -62,6 +63,7 @@ public class JpaUserCredentialStore implements UserCredentialStore {
public void updateCredential(RealmModel realm, UserModel user, CredentialModel cred) {
CredentialEntity entity = em.find(CredentialEntity.class, cred.getId());
if (!checkCredentialEntity(entity, user)) return;
validateDuplicateCredential(realm, user, cred.getUserLabel(), cred.getId());
entity.setCreatedDate(cred.getCreatedDate());
entity.setUserLabel(cred.getUserLabel());
entity.setType(cred.getType());
@ -138,7 +140,21 @@ public class JpaUserCredentialStore implements UserCredentialStore {
}
private void validateDuplicateCredential(RealmModel realm, UserModel user, String userLabel, String credentialId) {
if (userLabel != null) {
boolean exists = getStoredCredentialEntities(realm, user)
.anyMatch(existing -> existing.getUserLabel() != null
&& existing.getUserLabel().equalsIgnoreCase(userLabel.trim())
&& (credentialId == null || !existing.getId().equals(credentialId))); // Exclude self in update
if (exists) {
throw new ModelDuplicateException("Device already exists with the same name", CredentialModel.USER_LABEL);
}
}
}
CredentialEntity createCredentialEntity(RealmModel realm, UserModel user, CredentialModel cred) {
validateDuplicateCredential(realm, user, cred.getUserLabel(), null);
CredentialEntity entity = new CredentialEntity();
String id = cred.getId() == null ? KeycloakModelUtils.generateId() : cred.getId();
entity.setId(id);

View File

@ -483,7 +483,7 @@ public class JpaUserFederatedStorageProvider implements
return closing(paginateQuery(query, firstResult, max).getResultStream());
}
@Override
public Stream<String> getRoleMembersStream(RealmModel realm, RoleModel role, Integer firstResult, Integer max) {
TypedQuery<String> query = em.createNamedQuery("fedRoleMembership", String.class);
@ -564,6 +564,7 @@ public class JpaUserFederatedStorageProvider implements
public void updateCredential(RealmModel realm, String userId, CredentialModel cred) {
FederatedUserCredentialEntity entity = em.find(FederatedUserCredentialEntity.class, cred.getId());
if (!checkCredentialEntity(entity, userId)) return;
validateDuplicateUserCredential(userId, cred.getUserLabel(), cred.getId());
createIndex(realm, userId);
entity.setCreatedDate(cred.getCreatedDate());
entity.setType(cred.getType());
@ -572,8 +573,26 @@ public class JpaUserFederatedStorageProvider implements
entity.setUserLabel(cred.getUserLabel());
}
/**
* Validates if a credential with the same user label already exists for the given user.
* Excludes the credential itself if updating an existing one.
*/
private void validateDuplicateUserCredential(String userId, String userLabel, String credentialId) {
if (userLabel != null) {
boolean exists = getStoredCredentialEntitiesStream(userId)
.anyMatch(existing -> existing.getUserLabel() != null
&& existing.getUserLabel().trim().equalsIgnoreCase(userLabel.trim())
&& (credentialId == null || !existing.getId().equals(credentialId)));
if (exists) {
throw new ModelDuplicateException("Device already exists with the same name", CredentialModel.USER_LABEL);
}
}
}
@Override
public CredentialModel createCredential(RealmModel realm, String userId, CredentialModel cred) {
validateDuplicateUserCredential(userId, cred.getUserLabel(), null);
createIndex(realm, userId);
FederatedUserCredentialEntity entity = new FederatedUserCredentialEntity();
String id = cred.getId() == null ? KeycloakModelUtils.generateId() : cred.getId();

View File

@ -58,6 +58,7 @@ public class CredentialModel implements Serializable {
public static final String CLIENT_CERT = "cert";
public static final String KERBEROS = "kerberos";
public static final String USER_LABEL = "userLabel";
private String id;
private String type;

View File

@ -32,6 +32,7 @@ import org.keycloak.events.EventBuilder;
import org.keycloak.events.EventType;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.KeycloakSessionFactory;
import org.keycloak.models.ModelDuplicateException;
import org.keycloak.models.OTPPolicy;
import org.keycloak.models.UserModel;
import org.keycloak.models.Constants;
@ -116,10 +117,23 @@ public class UpdateTotp implements RequiredActionProvider, RequiredActionFactory
AuthenticatorUtil.logoutOtherSessions(context);
}
if (!CredentialHelper.createOTPCredential(context.getSession(), context.getRealm(), context.getUser(), challengeResponse, credentialModel)) {
try {
if (!CredentialHelper.createOTPCredential(context.getSession(), context.getRealm(), context.getUser(), challengeResponse, credentialModel)) {
Response challenge = context.form()
.setAttribute("mode", mode)
.addError(new FormMessage(Validation.FIELD_OTP_CODE, Messages.INVALID_TOTP))
.createResponse(UserModel.RequiredAction.CONFIGURE_TOTP);
context.challenge(challenge);
return;
}
} catch (ModelDuplicateException e) {
String field = switch (e.getDuplicateFieldName()) {
case CredentialModel.USER_LABEL -> Validation.FIELD_OTP_LABEL;
default -> null;
};
Response challenge = context.form()
.setAttribute("mode", mode)
.addError(new FormMessage(Validation.FIELD_OTP_CODE, Messages.INVALID_TOTP))
.addError(new FormMessage(field, e.getMessage()))
.createResponse(UserModel.RequiredAction.CONFIGURE_TOTP);
context.challenge(challenge);
return;

View File

@ -94,6 +94,8 @@ public class KeycloakErrorHandler implements ExceptionMapper<Throwable> {
error.setError(getErrorCode(throwable));
if (throwable.getCause() instanceof ModelException) {
error.setErrorDescription(throwable.getMessage());
} if (throwable instanceof ModelDuplicateException) {
error.setErrorDescription(throwable.getMessage());
} else if (throwable instanceof JsonProcessingException || throwable.getCause() instanceof JsonProcessingException) {
error.setErrorDescription("Cannot parse the JSON");
} else if (isServerError) {
@ -153,6 +155,10 @@ public class KeycloakErrorHandler implements ExceptionMapper<Throwable> {
return OAuthErrorException.INVALID_REQUEST;
}
if (cause instanceof ModelDuplicateException || throwable instanceof ModelDuplicateException) {
return "conflict";
}
if (throwable instanceof WebApplicationException && throwable.getMessage() != null) {
return throwable.getMessage();
}

View File

@ -883,6 +883,12 @@ public class UserResource {
if (auth.users().canQuery()) throw new NotFoundException("Credential not found");
else throw new ForbiddenException();
}
if (userLabel == null || userLabel.trim().isEmpty()) {
throw new ErrorResponseException("missingCredentialLabel", "Credential label must not be empty", Status.BAD_REQUEST);
}
user.credentialManager().updateCredentialLabel(credentialId, userLabel);
}

View File

@ -95,6 +95,7 @@ public class AppInitiatedActionTotpSetupTest extends AbstractAppInitiatedActionT
registerPage.register("firstName", "lastName", "email@mail.com", "setupTotp", "password", "password");
String userId = events.expectRegister("setupTotp", "email@mail.com").assertEvent().getUserId();
getCleanup().addUserId(userId);
doAIA();
@ -120,6 +121,33 @@ public class AppInitiatedActionTotpSetupTest extends AbstractAppInitiatedActionT
events.expectLogin().user(userId).session(authSessionId2).detail(Details.USERNAME, "setuptotp").assertEvent();
}
@Test
public void setupTotpRegisterDuplicateUserLabel() {
loginPage.open();
loginPage.clickRegister();
registerPage.register("firstName", "lastName", "email@mail.com", "setupTotp", "password", "password");
String userId = events.expectRegister("setupTotp", "email@mail.com").assertEvent().getUserId();
getCleanup().addUserId(userId);
doAIA();
totpPage.assertCurrent();
totpPage.configure(totp.generateTOTP(totpPage.getTotpSecret()), "otp");
assertKcActionStatus(SUCCESS);
doAIA();
totpPage.assertCurrent();
totpPage.configure(totp.generateTOTP(totpPage.getTotpSecret()), "otp");
assertEquals("Device already exists with the same name", totpPage.getInputLabelError());
}
@Test
public void cancelSetupTotp() throws Exception {
try {
@ -382,6 +410,7 @@ public class AppInitiatedActionTotpSetupTest extends AbstractAppInitiatedActionT
registerPage.register("firstName2", "lastName2", "email2@mail.com", "setupTotp2", "password2", "password2");
String userId = events.expectRegister("setupTotp2", "email2@mail.com").assertEvent().getUserId();
getCleanup().addUserId(userId);
doAIA();

View File

@ -45,9 +45,11 @@ import org.keycloak.events.admin.OperationType;
import org.keycloak.events.admin.ResourceType;
import org.keycloak.models.Constants;
import org.keycloak.models.LDAPConstants;
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserModel;
import org.keycloak.models.credential.OTPCredentialModel;
import org.keycloak.models.credential.PasswordCredentialModel;
import org.keycloak.models.jpa.entities.CredentialEntity;
import org.keycloak.models.utils.KeycloakModelUtils;
import org.keycloak.models.utils.ModelToRepresentation;
import org.keycloak.models.utils.StripSecretsUtils;
@ -71,6 +73,7 @@ import org.keycloak.representations.idm.UserRepresentation;
import org.keycloak.representations.userprofile.config.UPAttribute;
import org.keycloak.representations.userprofile.config.UPAttributePermissions;
import org.keycloak.representations.userprofile.config.UPConfig;
import org.keycloak.services.ErrorResponseException;
import org.keycloak.storage.StorageId;
import org.keycloak.storage.UserStorageProvider;
import org.keycloak.testsuite.federation.DummyUserFederationProviderFactory;
@ -3403,6 +3406,49 @@ public class UserTest extends AbstractAdminTest {
.findFirst().orElseThrow().getUserLabel());
}
@Test
public void testShouldFailToSetCredentialUserLabelWhenLabelIsEmpty() {
UserResource user = ApiUtil.findUserByUsernameId(testRealm(), "user-with-one-configured-otp");
CredentialRepresentation otpCred = user.credentials().get(0);
BadRequestException ex = assertThrows(BadRequestException.class, () -> {
user.setCredentialUserLabel(otpCred.getId(), " ");
});
Response response = ex.getResponse();
String body = response.readEntity(String.class);
assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus());
assertTrue(body.contains("missingCredentialLabel"));
assertTrue(body.contains("Credential label must not be empty"));
}
@Test
public void testShouldFailToSetCredentialUserLabelWhenLabelAlreadyExists() {
UserResource user = ApiUtil.findUserByUsernameId(testRealm(), "user-with-two-configured-otp");
List<CredentialRepresentation> credentials = user.credentials().stream()
.filter(c -> c.getType().equals(OTPCredentialModel.TYPE))
.toList();
assertEquals(2, credentials.size());
String firstId = credentials.get(0).getId();
String secondId = credentials.get(1).getId();
user.setCredentialUserLabel(firstId, "Device");
user.setCredentialUserLabel(secondId, "Second Device");
// Attempt to update second credential to use the same label as the first
ClientErrorException ex = assertThrows(ClientErrorException.class, () -> {
user.setCredentialUserLabel(secondId, "Device");
});
Response response = ex.getResponse();
assertEquals(Response.Status.CONFLICT.getStatusCode(), response.getStatus());
String body = response.readEntity(String.class);
assertNotNull(body);
assertTrue(body.contains("Device already exists with the same name"));
}
@Test
public void testUpdateCredentialLabelForFederatedUser() {
// Create user federation

View File

@ -31,6 +31,7 @@ import org.keycloak.exportimport.dir.DirExportProviderFactory;
import org.keycloak.exportimport.singlefile.SingleFileExportProviderFactory;
import org.keycloak.models.GroupModel;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.ModelDuplicateException;
import org.keycloak.models.PasswordPolicy;
import org.keycloak.models.RealmModel;
import org.keycloak.models.RoleModel;
@ -46,6 +47,10 @@ import java.util.LinkedList;
import java.util.List;
import java.util.stream.Collectors;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
/**
* @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
* @version $Revision: 1 $
@ -161,6 +166,73 @@ public class FederatedStorageExportImportTest extends AbstractAuthTest {
});
}
@Test
public void testCreateCredentialWithDuplicateUserLabelShouldFail() {
final String userId = "f:1:testUser";
testingClient.server().run(session -> {
RealmModel realm = new RealmManager(session).createRealm(REALM_NAME);
UserStorageUtil.userFederatedStorage(session).setSingleAttribute(realm, userId, "dummy", "value");
PasswordCredentialModel cred1 = FederatedStorageExportImportTest
.getHashProvider(session, realm.getPasswordPolicy())
.encodedCredential("secret1", realm.getPasswordPolicy().getHashIterations());
cred1.setUserLabel("MyDevice");
UserStorageUtil.userFederatedStorage(session).createCredential(realm, userId, cred1);
PasswordCredentialModel cred2 = FederatedStorageExportImportTest
.getHashProvider(session, realm.getPasswordPolicy())
.encodedCredential("secret2", realm.getPasswordPolicy().getHashIterations());
cred2.setUserLabel("MyDevice");
try {
UserStorageUtil.userFederatedStorage(session).createCredential(realm, userId, cred2);
Assert.fail("Expected ModelDuplicateException was not thrown");
} catch (ModelDuplicateException ex) {
assertEquals("Device already exists with the same name", ex.getMessage());
}
});
}
@Test
public void testUpdateCredentialWithDuplicateUserLabelShouldFail() {
final String userId = "f:1:testUser";
testingClient.server().run(session -> {
RealmModel realm = new RealmManager(session).createRealm(REALM_NAME);
UserStorageUtil.userFederatedStorage(session).setSingleAttribute(realm, userId, "dummy", "value");
// Create first credential with label "DeviceOne"
PasswordCredentialModel cred1 = FederatedStorageExportImportTest
.getHashProvider(session, realm.getPasswordPolicy())
.encodedCredential("secret1", realm.getPasswordPolicy().getHashIterations());
cred1.setUserLabel("DeviceOne");
UserStorageUtil.userFederatedStorage(session).createCredential(realm, userId, cred1);
// Create second credential with label "DeviceTwo"
PasswordCredentialModel cred2 = FederatedStorageExportImportTest
.getHashProvider(session, realm.getPasswordPolicy())
.encodedCredential("secret2", realm.getPasswordPolicy().getHashIterations());
cred2.setUserLabel("DeviceTwo");
UserStorageUtil.userFederatedStorage(session).createCredential(realm, userId, cred2);
CredentialModel credentialModelUpdate = UserStorageUtil.userFederatedStorage(session)
.getStoredCredentialsStream(realm, userId)
.filter(c -> "DeviceTwo".equals(c.getUserLabel()))
.findFirst()
.orElseThrow(() -> new IllegalStateException("Credential not found"));
credentialModelUpdate.setUserLabel("DeviceOne");
try {
UserStorageUtil.userFederatedStorage(session).updateCredential(realm, userId, credentialModelUpdate);
Assert.fail("Expected ModelDuplicateException was not thrown");
} catch (ModelDuplicateException ex) {
assertEquals("Device already exists with the same name", ex.getMessage());
}
});
}
@Test
public void testDir() {
ComponentExportImportTest.clearExportImportProperties(testingClient);

View File

@ -979,7 +979,7 @@ public class UserStorageTest extends AbstractAuthTest {
@Test
@ModelTest
public void testCredentialCRUD(KeycloakSession session) throws Exception {
public void testCredentialCRUD(KeycloakSession session) {
AtomicReference<String> passwordId = new AtomicReference<>();
AtomicReference<String> otp1Id = new AtomicReference<>();
AtomicReference<String> otp2Id = new AtomicReference<>();
@ -1000,8 +1000,8 @@ public class UserStorageTest extends AbstractAuthTest {
passwordId.set(passwordCred.getId());
// Create Password and 2 OTP credentials (password was already created)
CredentialModel otp1 = OTPCredentialModel.createFromPolicy(realm, "secret1");
CredentialModel otp2 = OTPCredentialModel.createFromPolicy(realm, "secret2");
CredentialModel otp1 = OTPCredentialModel.createFromPolicy(realm, "secret1", "label1");
CredentialModel otp2 = OTPCredentialModel.createFromPolicy(realm, "secret2", "label2");
otp1 = user.credentialManager().createStoredCredential(otp1);
otp2 = user.credentialManager().createStoredCredential(otp2);
otp1Id.set(otp1.getId());

View File

@ -45,8 +45,8 @@ public class CredentialModelTest extends AbstractTestRealmKeycloakTest {
passwordId.set(list.get(0).getId());
// Create 2 OTP credentials (password was already created)
CredentialModel otp1 = OTPCredentialModel.createFromPolicy(realm, "secret1");
CredentialModel otp2 = OTPCredentialModel.createFromPolicy(realm, "secret2");
CredentialModel otp1 = OTPCredentialModel.createFromPolicy(realm, "secret1", "label1");
CredentialModel otp2 = OTPCredentialModel.createFromPolicy(realm, "secret2", "label2");
otp1 = user.credentialManager().createStoredCredential(otp1);
otp2 = user.credentialManager().createStoredCredential(otp2);
otp1Id.set(otp1.getId());

View File

@ -34,6 +34,7 @@ import org.keycloak.representations.idm.RequiredActionProviderSimpleRepresentati
import org.keycloak.testsuite.AbstractAuthTest;
import org.keycloak.testsuite.page.AbstractPatternFlyAlert;
import org.keycloak.testsuite.webauthn.pages.SigningInPage;
import org.keycloak.testsuite.webauthn.pages.WebAuthnErrorPage;
import org.keycloak.testsuite.webauthn.utils.SigningInPageUtils;
import org.keycloak.testsuite.pages.DeleteCredentialPage;
import org.keycloak.testsuite.updaters.RealmAttributeUpdater;
@ -75,6 +76,9 @@ public abstract class AbstractWebAuthnAccountTest extends AbstractAuthTest imple
@Page
private DeleteCredentialPage deleteCredentialPage;
@Page
protected WebAuthnErrorPage webAuthnErrorPage;
private VirtualAuthenticatorManager webAuthnManager;
protected SigningInPage.CredentialType webAuthnCredentialType;
protected SigningInPage.CredentialType webAuthnPwdlessCredentialType;

View File

@ -26,6 +26,7 @@ import org.keycloak.models.credential.WebAuthnCredentialModel;
import org.keycloak.representations.idm.CredentialRepresentation;
import org.keycloak.representations.idm.RequiredActionProviderRepresentation;
import org.keycloak.testsuite.arquillian.annotation.IgnoreBrowserDriver;
import org.keycloak.testsuite.page.AbstractPatternFlyAlert;
import org.keycloak.testsuite.webauthn.pages.SigningInPage;
import org.keycloak.testsuite.webauthn.pages.WebAuthnAuthenticatorsList;
import org.keycloak.testsuite.webauthn.updaters.AbstractWebAuthnRealmUpdater;
@ -94,34 +95,24 @@ public class WebAuthnSigningInTest extends AbstractWebAuthnAccountTest {
public void createWebAuthnSameUserLabel() {
final String SAME_LABEL = "key123";
// Do we really allow to have several authenticators with the same user label??
SigningInPage.UserCredential webAuthn = addWebAuthnCredential(SAME_LABEL, false);
assertThat(webAuthn, notNullValue());
SigningInPage.UserCredential passwordless = addWebAuthnCredential(SAME_LABEL, true);
assertThat(passwordless, notNullValue());
assertThat(webAuthnCredentialType.getUserCredentialsCount(), is(1));
webAuthn = webAuthnCredentialType.getUserCredential(webAuthn.getId());
assertThat(webAuthn, notNullValue());
assertThat(webAuthn.getUserLabel(), is(SAME_LABEL));
SigningInPage.CredentialType credentialType = webAuthnPwdlessCredentialType;
assertThat(webAuthnPwdlessCredentialType.getUserCredentialsCount(), is(1));
passwordless = webAuthnPwdlessCredentialType.getUserCredential(passwordless.getId());
assertThat(passwordless, notNullValue());
assertThat(passwordless.getUserLabel(), is(SAME_LABEL));
AbstractPatternFlyAlert.waitUntilHidden();
SigningInPage.UserCredential webAuthn2 = addWebAuthnCredential(SAME_LABEL, false);
assertThat(webAuthn2, notNullValue());
assertThat(webAuthn2.getUserLabel(), is(SAME_LABEL));
credentialType.clickSetUpLink();
webAuthnRegisterPage.assertCurrent();
webAuthnRegisterPage.clickRegister();
webAuthnRegisterPage.registerWebAuthnCredential(SAME_LABEL);
waitForPageToLoad();
assertThat(webAuthnCredentialType.getUserCredentialsCount(), is(2));
webAuthnErrorPage.assertCurrent();
assertThat(webAuthnErrorPage.getError(), is("Failed to register your Passkey.\nDevice already exists with the same name"));
webAuthnErrorPage.clickTryAgain();
SigningInPage.UserCredential passwordless2 = addWebAuthnCredential(SAME_LABEL, true);
assertThat(passwordless2, notNullValue());
assertThat(passwordless2.getUserLabel(), is(SAME_LABEL));
assertThat(webAuthnPwdlessCredentialType.getUserCredentialsCount(), is(2));
webAuthnRegisterPage.assertCurrent();
}
@Test

View File

@ -81,7 +81,7 @@
</label>
</div>
<div class="${properties.kcInputClass!}">
<div class="${properties.kcInputClass!} <#if messagesPerField.existsError('userLabel')>pf-m-error</#if>">
<input type="text" id="userLabel" name="userLabel" autocomplete="off"
aria-invalid="<#if messagesPerField.existsError('userLabel')>true</#if>"
/>