From b5325707470e30b099d30a47ca5c2e26031e12d2 Mon Sep 17 00:00:00 2001 From: Jan Lieskovsky Date: Mon, 3 Feb 2020 19:34:28 +0100 Subject: [PATCH] [KEYCLOAK-12168] Various setup TOTP screen usability improvements (#6709) On both the TOTP account and TOTP login screens perform the following: * Make the "Device name" label optional if user registers the first TOTP credential. Make it mandatory otherwise, * Denote the "Authenticator code" with asterisk, so it's clear it's required field (always), * Add sentence to Step 3 of configuring TOTP credential explaining the user to provide device name label, Also perform other CSS & locale / messages file changes, so the UX is identical when creating OTP credentials on both of these pages Add a corresponding testcase Also address issues pointed out by mposolda's review. Thanks, Marek! Signed-off-by: Jan Lieskovsky --- .../requiredactions/UpdateTotp.java | 14 ++ .../login/freemarker/model/TotpBean.java | 13 +- .../keycloak/services/messages/Messages.java | 2 + .../keycloak/testsuite/cli/KcinitTest.java | 4 +- .../ResetCredentialsAlternativeFlowsTest.java | 173 ++++++++++++++++++ .../account/messages/messages_en.properties | 13 +- .../resources/theme/base/account/totp.ftl | 25 ++- .../theme/base/login/login-config-totp.ftl | 28 ++- .../login/messages/messages_en.properties | 15 +- .../resources/theme/base/login/template.ftl | 67 +++++-- .../keycloak/login/resources/css/login.css | 10 + 11 files changed, 321 insertions(+), 43 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateTotp.java b/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateTotp.java index 837279d0ad5..ea2d6de4971 100644 --- a/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateTotp.java +++ b/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateTotp.java @@ -38,6 +38,9 @@ import org.keycloak.services.validation.Validation; import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; +import java.util.Collections; +import java.util.List; + /** * @author Bill Burke * @version $Revision: 1 $ @@ -88,6 +91,17 @@ public class UpdateTotp implements RequiredActionProvider, RequiredActionFactory return; } OTPCredentialProvider otpCredentialProvider = (OTPCredentialProvider) context.getSession().getProvider(CredentialProvider.class, "keycloak-otp"); + final List otpCredentials = (otpCredentialProvider.isConfiguredFor(context.getRealm(), context.getUser())) + ? context.getSession().userCredentialManager().getStoredCredentialsByType(context.getRealm(), context.getUser(), OTPCredentialModel.TYPE) + : Collections.EMPTY_LIST; + if (otpCredentials.size() >= 1 && Validation.isBlank(userLabel)) { + Response challenge = context.form() + .setAttribute("mode", mode) + .setError(Messages.MISSING_TOTP_DEVICE_NAME) + .createResponse(UserModel.RequiredAction.CONFIGURE_TOTP); + context.challenge(challenge); + return; + } CredentialModel createdCredential = otpCredentialProvider.createCredential(context.getRealm(), context.getUser(), credentialModel); UserCredentialModel credential = new UserCredentialModel(createdCredential.getId(), otpCredentialProvider.getType(), challengeResponse); //If the type is HOTP, call verify once to consume the OTP used for registration and increase the counter. diff --git a/services/src/main/java/org/keycloak/forms/login/freemarker/model/TotpBean.java b/services/src/main/java/org/keycloak/forms/login/freemarker/model/TotpBean.java index 534f74f84b0..82c8eabd1d0 100755 --- a/services/src/main/java/org/keycloak/forms/login/freemarker/model/TotpBean.java +++ b/services/src/main/java/org/keycloak/forms/login/freemarker/model/TotpBean.java @@ -16,6 +16,7 @@ */ package org.keycloak.forms.login.freemarker.model; +import org.keycloak.credential.CredentialModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.OTPPolicy; import org.keycloak.models.RealmModel; @@ -25,6 +26,8 @@ import org.keycloak.models.utils.HmacOTP; import org.keycloak.utils.TotpUtils; import javax.ws.rs.core.UriBuilder; +import java.util.Collections; +import java.util.List; /** * Used for UpdateTotp required action @@ -39,11 +42,17 @@ public class TotpBean { private final String totpSecretQrCode; private final boolean enabled; private UriBuilder uriBuilder; + private final List otpCredentials; public TotpBean(KeycloakSession session, RealmModel realm, UserModel user, UriBuilder uriBuilder) { this.realm = realm; this.uriBuilder = uriBuilder; this.enabled = session.userCredentialManager().isConfiguredFor(realm, user, OTPCredentialModel.TYPE); + if (enabled) { + otpCredentials = session.userCredentialManager().getStoredCredentialsByType(realm, user, OTPCredentialModel.TYPE); + } else { + otpCredentials = Collections.EMPTY_LIST; + } this.totpSecret = HmacOTP.generateSecret(20); this.totpSecretEncoded = TotpUtils.encode(totpSecret); this.totpSecretQrCode = TotpUtils.qrCode(totpSecret, realm, user); @@ -77,6 +86,8 @@ public class TotpBean { return realm.getOTPPolicy(); } + public List getOtpCredentials() { + return otpCredentials; + } } - diff --git a/services/src/main/java/org/keycloak/services/messages/Messages.java b/services/src/main/java/org/keycloak/services/messages/Messages.java index 85514d5c518..80657c86873 100755 --- a/services/src/main/java/org/keycloak/services/messages/Messages.java +++ b/services/src/main/java/org/keycloak/services/messages/Messages.java @@ -58,6 +58,8 @@ public class Messages { public static final String MISSING_TOTP = "missingTotpMessage"; + public static final String MISSING_TOTP_DEVICE_NAME = "missingTotpDeviceNameMessage"; + public static final String NOTMATCH_PASSWORD = "notMatchPasswordMessage"; public static final String INVALID_PASSWORD_EXISTING = "invalidPasswordExistingMessage"; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cli/KcinitTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cli/KcinitTest.java index a3c10edba77..033e97d4675 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cli/KcinitTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cli/KcinitTest.java @@ -569,8 +569,8 @@ public class KcinitTest extends AbstractTestRealmKeycloakTest { exe.sendLine("password"); exe.waitForStderr("One Time Password:"); - Pattern p = Pattern.compile("Open the application and enter the key\\s+(.+)\\s+Use the following configuration values"); - //Pattern p = Pattern.compile("Open the application and enter the key"); + Pattern p = Pattern.compile("Open the application and enter the key:\\s+(.+)\\s+Use the following configuration values"); + //Pattern p = Pattern.compile("Open the application and enter the key:"); String stderr = exe.stderrString(); //System.out.println("***************"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetCredentialsAlternativeFlowsTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetCredentialsAlternativeFlowsTest.java index abc76b55ae6..dbb4b510469 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetCredentialsAlternativeFlowsTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetCredentialsAlternativeFlowsTest.java @@ -24,10 +24,13 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.keycloak.OAuth2Constants; +import org.keycloak.models.UserManager; +import org.keycloak.models.UserModel; import org.keycloak.models.utils.DefaultAuthenticationFlows; import org.keycloak.models.utils.TimeBasedOTP; import org.keycloak.representations.idm.AuthenticationFlowRepresentation; import org.keycloak.representations.idm.RealmRepresentation; +import org.keycloak.representations.idm.RequiredActionProviderRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.AbstractTestRealmKeycloakTest; import org.keycloak.testsuite.admin.ApiUtil; @@ -43,6 +46,7 @@ import org.keycloak.testsuite.pages.LoginPasswordUpdatePage; import org.keycloak.testsuite.pages.LoginTotpPage; import org.keycloak.testsuite.pages.LoginUsernameOnlyPage; import org.keycloak.testsuite.pages.PasswordPage; +import org.keycloak.testsuite.pages.RegisterPage; import org.keycloak.testsuite.util.FlowUtil; import org.keycloak.testsuite.util.GreenMailRule; import org.keycloak.testsuite.util.MailUtils; @@ -52,6 +56,7 @@ import org.keycloak.testsuite.util.UserBuilder; import javax.mail.internet.MimeMessage; import java.util.Arrays; import java.util.List; +import java.util.regex.Pattern; import static org.junit.Assert.assertEquals; import static org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer.REMOTE; @@ -79,6 +84,9 @@ public class ResetCredentialsAlternativeFlowsTest extends AbstractTestRealmKeycl @Page protected PasswordPage passwordPage; + @Page + protected RegisterPage registerPage; + @Page protected LoginPasswordResetPage resetPasswordPage; @@ -380,4 +388,169 @@ public class ResetCredentialsAlternativeFlowsTest extends AbstractTestRealmKeycl revertFlows(); } } + + + // 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 + @Test + public void deviceNameOptionalForFirstOTPCredentialButRequiredForEachNextOne() { + // Enable 'Default Action' on 'Configure OTP' RA for the 'test' realm + RequiredActionProviderRepresentation otpRequiredAction = testRealm().flows().getRequiredAction("CONFIGURE_TOTP"); + otpRequiredAction.setDefaultAction(true); + testRealm().flows().updateRequiredAction("CONFIGURE_TOTP", otpRequiredAction); + + try { + // 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"; + configureResetCredentialsRemoveExecutionsAndBindTheFlow( + newFlowAlias, + Arrays.asList("reset-credential-email", "reset-password") + ); + + /* Verify the 'Device Name' is optional when creating new OTP credential via the Account page */ + + // Login & set up the initial OTP code for the user + loginPage.open(); + loginPage.login("login@test.com", "password"); + accountTotpPage.open(); + Assert.assertTrue(accountTotpPage.isCurrent()); + + String pageSource = driver.getPageSource(); + // Check the One-time code label is followed by asterisk character (since always required) + final String oneTimeCodeLabelFollowedByAsterisk = "(?s)