8000 Reject empty credential labels in the Account API and setup flows by shuseng8 · Pull Request #54849 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Reject empty credential labels in the Account API and setup flows #54849

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.

Sign up for GitHub

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

Open
shuseng8 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reject empty credential labels in the Account API and setup flows #54849

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,14 @@ The emails are sent by the `email` executor. It has 4 threads by default and sta
Custom email template providers, including ones that extend `FreeMarkerEmailTemplateProvider`, still send the email right away, so for them the response time still depends on the mail server.
To send it in the background, a provider that extends `FreeMarkerEmailTemplateProvider` can override `supportsBackgroundDelivery()`. Other providers need to implement `EmailTemplateProvider.renderPasswordReset` and `send(RenderedEmail, int, EmailDeliveryListener)`.

=== Empty credential labels are rejected
// https://github.com/keycloak/keycloak/issues/51440

Empty or blank credential labels are now rejected in the Account REST API and in the OTP and Passkey setup flows.
The first OTP device, like every later device, requires a device name.
Clients that previously omitted a label when creating or renaming a credential must supply a non-empty value.
The Admin REST API already rejected empty labels and is unchanged.

// ------------------------ Deprecated features ------------------------ //
== Deprecated features

Expand Down
8 changes: 1 addition & 7 deletions services/src/main/java/org/keycloak/authentication/requiredactions/UpdateTotp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@
import org.keycloak.authentication.RequiredActionFactory;
import org.keycloak.authentication.RequiredActionProvider;
import org.keycloak.credential.CredentialModel;
import org.keycloak.credential.CredentialProvider;
import org.keycloak.credential.OTPCredentialProvider;
import org.keycloak.events.Details;
import org.keycloak.events.EventBuilder;
import org.keycloak.events.EventType;
Expand Down Expand Up @@ -138,11 +136,7 @@ public void processAction(RequiredActionContext context) {
context.challenge(challenge);
return;
}
OTPCredentialProvider otpCredentialProvider = (OTPCredentialProvider) context.getSession().getProvider(CredentialProvider.class, "keycloak-otp");
final Stream<CredentialModel> otpCredentials = (otpCredentialProvider.isConfiguredFor(context.getRealm(), context.getUser()))
? context.getUser().credentialManager().getStoredCredentialsByTypeStream(OTPCredentialModel.TYPE)
: Stream.empty();
if (otpCredentials.count() >= 1 && Validation.isBlank(userLabel)) {
if (Validation.isBlank(userLabel)) {
Response challenge = context.form()
.setAttribute("mode", mode)
.addError(new FormMessage(Validation.FIELD_OTP_LABEL, Messages.MISSING_TOTP_DEVICE_NAME))
Expand Down
6 changes: 6 additions & 0 deletions services/src/main/java/org/keycloak/authentication/requiredactions/WebAuthnRegister.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@
import static org.keycloak.services.messages.Messages.WEBAUTHN_ERROR_BAD_RPID;
import static org.keycloak.services.messages.Messages.WEBAUTHN_ERROR_BAD_SIGNATURE;
import static org.keycloak.services.messages.Messages.WEBAUTHN_ERROR_DUPLICATED_DEVICE;
import static org.keycloak.services.messages.Messages.WEBAUTHN_ERROR_MISSING_LABEL;
import static org.keycloak.services.messages.Messages.WEBAUTHN_ERROR_REGISTER_VERIFICATION;
import static org.keycloak.services.messages.Messages.WEBAUTHN_ERROR_REGISTRATION;
import static org.keycloak.services.messages.Messages.WEBAUTHN_ERROR_REGISTRATION_AAGUID_ATTESTATION_REQUIRED;
Expand Down Expand Up @@ -270,6 +271,10 @@ public void processAction(RequiredActionContext context) {
String rpId = policy.getRpId();
if (rpId == null || rpId.isEmpty()) rpId = context.getUriInfo().getBaseUri().getHost();
String label = params.getFirst(WebAuthnConstants.AUTHENTICATOR_LABEL);
if (StringUtil.isBlank(label)) {
setErrorResponse(context, WEBAUTHN_ERROR_MISSING_LABEL, "Credential label must not be empty", originalEventType);
return;
Comment thread
shuseng8 marked this conversation as resolved.
Resolved
}
byte[] clientDataJSON = Base64.getUrlDecoder().decode(params.getFirst(WebAuthnConstants.CLIENT_DATA_JSON));
byte[] attestationObject = Base64.getUrlDecoder().decode(params.getFirst(WebAuthnConstants.ATTESTATION_OBJECT));

Expand Down Expand Up @@ -511,6 +516,7 @@ private void setErrorResponse(RequiredActionContext context, final String errorC
context.challenge(errorResponse);
break;
case WEBAUTHN_ERROR_REGISTRATION:
case WEBAUTHN_ERROR_MISSING_LABEL:
case WEBAUTHN_ERROR_REGISTRATION_NOT_ALLOWED_AAGUID:
case WEBAUTHN_ERROR_REGISTRATION_AAGUID_ATTESTATION_REQUIRED:
case WEBAUTHN_ERROR_REGISTRATION_ATTACHMENT_MISMATCH:
Expand Down
1 change: 1 addition & 0 deletions services/src/main/java/org/keycloak/services/messages/Messages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ public class Messages {
public static final String WEBAUTHN_ERROR_BAD_RPID = "webauthn-error-bad-rpid";
public static final String WEBAUTHN_ERROR_BAD_CHALLENGE = "webauthn-error-bad-challenge";
public static final String WEBAUTHN_ERROR_BAD_SIGNATURE = "webauthn-error-bad-signature";
public static final String WEBAUTHN_ERROR_MISSING_LABEL = "webauthn-error-missing-label";

// Conditions in Conditional Flow
public static final String ACCESS_DENIED = "access-denied";
Expand Down
4 changes: 4 additions & 0 deletions ...ices/src/main/java/org/keycloak/services/resources/account/AccountCredentialResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.keycloak.representations.account.CredentialMetadataRepresentation;
import org.keycloak.representations.idm.CredentialRepresentation;
import org.keycloak.services.ErrorResponse;
import org.keycloak.services.ErrorResponseException;
import org.keycloak.services.managers.Auth;
import org.keycloak.services.messages.Messages;
import org.keycloak.util.JsonSerialization;
Expand Down Expand Up @@ -343,6 +344,9 @@ public void setLabel(final @PathParam("credentialId") String credentialId, Strin

try {
String label = JsonSerialization.readValue(userLabel, String.class);
if (label == null || label.trim().isEmpty()) {
throw new ErrorResponseException("missingCredentialLabel", "Credential label must not be empty", Response.Status.BAD_REQUEST);
}
user.credentialManager().updateCredentialLabel(credentialId, label);
} catch (IOException ioe) {
throw ErrorResponse.error(Messages.INVALID_REQUEST, Response.Status.BAD_REQUEST);
Expand Down
58 changes: 58 additions & 0 deletions tests/base/src/test/java/org/keycloak/tests/account/AccountRestServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,64 @@ public void testCRUDCredentialOfDifferentUser() throws IOException {
Assertions.assertTrue(ObjectUtil.isEqualOrBothNull(otpCredential.getUserLabel(), otpCredentialLoaded.getUserLabel()));
}

@Test
public void testSetCredentialLabelRejectsEmptyAndBlank() throws IOException {
String token = oauth.client("direct-grant", "password").doPasswordGrantRequest("test-user@localhost", "password").getAccessToken();

UserResource user = AdminApiUtil.findUserByUsernameId(managedRealm.admin(), "test-user@localhost");

org.keycloak.representations.idm.UserRepresentation userRep = UserBuilder.update(user.toRepresentation())
.totpSecret("totpSecret")
.build();
userRep.getCredentials().get(0).setUserLabel("original-label");
user.update(userRep);

CredentialRepresentation otpCredential = user.credentials().stream()
.filter(credentialRep -> OTPCredentialModel.TYPE.equals(credentialRep.getType()))
.findFirst()
.get();

try {
try (SimpleHttpResponse response = simpleHttp
.doPut(getAccountUrl("credentials/" + otpCredential.getId() + "/label"))
.auth(token)
.json("")
.asResponse()) {
Assertions.assertEquals(400, response.getStatus());
OAuth2ErrorRepresentation error = response.asJson(OAuth2ErrorRepresentation.class);
Assertions.assertEquals("missingCredentialLabel", error.getError());
Assertions.assertEquals("Credential label must not be empty", error.getErrorDescription());
}

try (SimpleHttpResponse response = simpleHttp
.doPut(getAccountUrl("credentials/" + otpCredential.getId() + "/label"))
.auth(token)
.json(" ")
.asResponse()) {
Assertions.assertEquals(400, response.getStatus());
OAuth2ErrorRepresentation error = response.asJson(OAuth2ErrorRepresentation.class);
Assertions.assertEquals("missingCredentialLabel", error.getError());
Assertions.assertEquals("Credential label must not be empty", error.getErrorDescription());
}

try (SimpleHttpResponse response = simpleHttp
.doPut(getAccountUrl("credentials/" + otpCredential.getId() + "/label"))
.auth(token)
.json("updated-label")
.asResponse()) {
Assertions.assertEquals(204, response.getStatus());
}

CredentialRepresentation updated = user.credentials().stream()
.filter(credentialRep -> otpCredential.getId().equals(credentialRep.getId()))
.findFirst()
.get();
Assertions.assertEquals("updated-label", updated.getUserLabel());
} finally {
user.removeCredential(otpCredential.getId());
}
}

@Test
public void testRemoveCredentialWithNonOtpCredentialTriggeringNoEvent() throws IOException {
String token = oauth.client("direct-grant", "password").doPasswordGrantRequest("test-user@localhost", "password").getAccessToken();
Expand Down
4 changes: 2 additions & 2 deletions ...s/base/src/test/java/org/keycloak/tests/account/custom/AbstractCustomAuthFlowOTPTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ protected void configureOTP() {
oauth.openLoginForm();
testRealmLoginPage.login(testUser.getUsername(), PASSWORD);
String totpSecret = loginConfigTotpPage.getTotpSecret();
loginConfigTotpPage.configure(totp.generateTOTP(totpSecret));
loginConfigTotpPage.configure(totp.generateTOTP(totpSecret), "otp");
AccountHelper.logout(managedRealm.admin(), testUser.getUsername());

//verify that user has OTP configured
Expand Down Expand Up @@ -123,7 +123,7 @@ protected void reuseExistingOtp(boolean allowReusingExistingOtp) {
final String generatedOtp = totp.generateTOTP(totpSecret);
assertThat(generatedOtp, notNullValue());

loginConfigTotpPage.configure(generatedOtp);
loginConfigTotpPage.configure(generatedOtp, "otp");
AccountHelper.logout(managedRealm.admin(), testUser.getUsername());

oauth.openLoginForm();
Expand Down
4 changes: 2 additions & 2 deletions tests/base/src/test/java/org/keycloak/tests/account/custom/CustomAuthFlowOTPTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ private void configureOTP() {
oauth.openLoginForm();
testRealmLoginPage.login(testUser.getUsername(), PASSWORD);
String totpSecret = loginConfigTotpPage.getTotpSecret();
loginConfigTotpPage.configure(totp.generateTOTP(totpSecret));
loginConfigTotpPage.configure(totp.generateTOTP(totpSecret), "otp");
AccountHelper.logout(managedRealm.admin(), testUser.getUsername());

//verify that user has OTP configured
Expand Down Expand Up @@ -162,7 +162,7 @@ private void reuseExistingOtp(boolean allowReusingExistingOtp) {
final String generatedOtp = totp.generateTOTP(totpSecret);
assertThat(generatedOtp, notNullValue());

loginConfigTotpPage.configure(generatedOtp);
loginConfigTotpPage.configure(generatedOtp, "otp");
AccountHelper.logout(managedRealm.admin(), testUser.getUsername());

oauth.openLoginForm();
Expand Down
40 changes: 23 additions & 17 deletions tests/base/src/test/java/org/keycloak/tests/forms/ResetCredentialsOTPFlowsTest.java
View file
Open in desktop
10BC8
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,9 @@ public void resetCredentialsVerifyCustomOtpLabelSetProperly() {
}


// KEYCLOAK-12168 Verify the 'Device Name' label is optional for the first OTP credential created
// (either via Account page or by registering new user), but required for each next created OTP credential
// Verify the 'Device Name' label is required for every OTP credential, including the first
@Test
public void deviceNameOptionalForFirstOTPCredentialButRequiredForEachNextOne() {
public void deviceNameRequiredForAllOTPCredentials() {
// Enable 'Default Action' on 'Configure OTP' RA for the 'test' realm
RequiredActionProviderRepresentation otpRequiredAction = managedRealm.admin().flows().getRequiredAction("CONFIGURE_TOTP");
otpRequiredAction.setDefaultAction(true);
Expand All @@ -129,26 +128,28 @@ public void deviceNameOptionalForFirstOTPCredentialButRequiredForEachNextOne() {
// Make a copy of the default Reset Credentials flow, but:
// * Without 'Send Reset Email' authenticator,
// * Without 'Reset Password' authenticator
final String newFlowAlias = "resetcred - KEYCLOAK-12168 - firstOTP - account - test";
final String newFlowAlias = "resetcred - firstOTP - account - test";
configureResetCredentialsRemoveExecutionsAndBindTheFlow(
newFlowAlias,
Arrays.asList("reset-credential-email", "reset-password")
);

/* Verify the 'Device Name' is optional when creating new OTP credential via the Account page */
/* Verify the 'Device Name' is required when creating a new OTP credential via the Account page */

// Login & set up the initial OTP code for the user
oauth.openLoginForm();
loginPage.fillLogin(managedUser.getUsername(), managedUser.getPassword());
loginPage.submit();

// Create OTP credential with empty label
final String emptyOtpLabel = "";

// Setup OTP
// Setup OTP with an empty label should fail
doAIA();
totpPage.assertCurrent();
totpPage.configure(totp.generateTOTP(totpPage.getTotpSecret()), emptyOtpLabel);
totpPage.configure(totp.generateTOTP(totpPage.getTotpSecret()), "");
totpPage.assertCurrent();
Assertions.assertEquals("Please specify device name.", totpPage.getInputLabelError());

final String firstOtpLabel = "My first OTP device";
totpPage.configure(totp.generateTOTP(totpPage.getTotpSecret()), firstOtpLabel);
assertKcActionStatus(SUCCESS);

Assertions.assertTrue(AccountHelper.deleteTotpAuthentication(managedRealm.admin(), managedUser.getUsername()));
Expand All @@ -158,7 +159,7 @@ public void deviceNameOptionalForFirstOTPCredentialButRequiredForEachNextOne() {
logoutConfirmPage.assertCurrent();
logoutConfirmPage.confirmLogout();

/* Verify the 'Device Name' is optional when creating the first OTP credential via the login config TOTP page */
/* Verify the 'Device Name' is required when creating the first OTP credential via the login config TOTP page */

// Register new user
oauth.openLoginForm();
Expand All @@ -170,16 +171,21 @@ public void deviceNameOptionalForFirstOTPCredentialButRequiredForEachNextOne() {
managedRealm.cleanup().add(r -> r.users().delete(bwilsonId).close());
totpPage.assertCurrent();

// Create OTP credential with empty label

// Setup OTP
// Create OTP credential with empty label should fail
totpPage.configure(totp.generateTOTP(totpPage.getTotpSecret()), "");
totpPage.assertCurrent();
Assertions.assertEquals("Please specify device name.", totpPage.getInputLabelError());
Assertions.assertFalse(AccountHelper.isTotpPresent(managedRealm.admin(), "bwilson"));

// Setup OTP with a valid Device Name
final String firstRegisteredOtpLabel = "My first OTP device";
totpPage.configure(totp.generateTOTP(totpPage.getTotpSecret()), firstRegisteredOtpLabel);

// Assert user authenticated
Assertions.assertTrue(oauth.parseLoginResponse().isSuccess());

Assertions.assertTrue(AccountHelper.isTotpPresent(managedRealm.admin(), "bwilson"));
Assertions.assertTrue(AccountHelper.totpUserLabelComparator(managedRealm.admin(), "bwilson", ""));
Assertions.assertTrue(AccountHelper.totpUserLabelComparator(managedRealm.admin(), "bwilson", firstRegisteredOtpLabel));

// Logout
oauth.openLogoutForm();
Expand All @@ -196,10 +202,10 @@ public void deviceNameOptionalForFirstOTPCredentialButRequiredForEachNextOne() {
resetPasswordPage.assertCurrent();
resetPasswordPage.changePassword("bwilson@keycloak.org");

// Try to create another OTP credential with empty label again. This
// should fail with error since OTP label is required in this case already
// Try to create another OTP credential with empty label again. This should fail
totpPage.configure(totp.generateTOTP(totpPage.getTotpSecret()), "");
Assertions.assertTrue(AccountHelper.totpCountEquals(managedRealm.admin(), "bwilson", 1));
Assertions.assertEquals("Please specify device name.", totpPage.getInputLabelError());
// Create 2nd OTP credential with valid (non-empty) Device Name label. This should pass
final String secondOtpLabel = "My 2nd OTP device";
totpPage.configure(totp.generateTOTP(totpPage.getTotpSecret()), secondOtpLabel);
Expand Down
2 changes: 1 addition & 1 deletion tests/base/src/test/java/org/keycloak/tests/oidc/AcrAuthFlowTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ private void restartAuthenticationSession() {

private void configureTOTP() {
loginConfigTotpPage.assertCurrent();
loginConfigTotpPage.configure(totp.generateTOTP(loginConfigTotpPage.getTotpSecret()));
loginConfigTotpPage.configure(totp.generateTOTP(loginConfigTotpPage.getTotpSecret()), "otp");
}

private void setAcrClientPolicy(String acr, String alias) {
Expand Down
Loading
0