From d394e51674ee8b4ec6531a9e19dd814923525d4a Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 2 Mar 2022 16:08:20 +0100 Subject: [PATCH] Introduce profile 'feature' for step-up authentication enabled by default Closes #10315 --- .../java/org/keycloak/common/Profile.java | 3 ++- .../ConditionalLoaAuthenticatorFactory.java | 9 ++++++- .../keycloak/protocol/oidc/TokenManager.java | 8 +++++- .../forms/LevelOfAssuranceFlowTest.java | 27 +++++++++++++++++++ .../resources/partials/client-detail.html | 4 +-- .../partials/realm-login-settings.html | 2 +- 6 files changed, 47 insertions(+), 6 deletions(-) diff --git a/common/src/main/java/org/keycloak/common/Profile.java b/common/src/main/java/org/keycloak/common/Profile.java index dd5dd61da7..584e98e671 100755 --- a/common/src/main/java/org/keycloak/common/Profile.java +++ b/common/src/main/java/org/keycloak/common/Profile.java @@ -65,7 +65,8 @@ public class Profile { MAP_STORAGE("New store", Type.EXPERIMENTAL), PAR("OAuth 2.0 Pushed Authorization Requests (PAR)", Type.DEFAULT), DECLARATIVE_USER_PROFILE("Configure user profiles using a declarative style", Type.PREVIEW), - DYNAMIC_SCOPES("Dynamic OAuth 2.0 scopes", Type.EXPERIMENTAL); + DYNAMIC_SCOPES("Dynamic OAuth 2.0 scopes", Type.EXPERIMENTAL), + STEP_UP_AUTHENTICATION("Step-up Authentication", Type.DEFAULT); private String label; private final Type typeProject; diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/conditional/ConditionalLoaAuthenticatorFactory.java b/services/src/main/java/org/keycloak/authentication/authenticators/conditional/ConditionalLoaAuthenticatorFactory.java index 41f55469eb..bd90702564 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/conditional/ConditionalLoaAuthenticatorFactory.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/conditional/ConditionalLoaAuthenticatorFactory.java @@ -21,13 +21,15 @@ import java.util.List; import org.keycloak.Config; import org.keycloak.authentication.AuthenticationFlowCallbackFactory; import org.keycloak.authentication.Authenticator; +import org.keycloak.common.Profile; import org.keycloak.models.AuthenticationExecutionModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; +import org.keycloak.provider.EnvironmentDependentProviderFactory; import org.keycloak.provider.ProviderConfigProperty; import org.keycloak.provider.ProviderConfigurationBuilder; -public class ConditionalLoaAuthenticatorFactory implements ConditionalAuthenticatorFactory, AuthenticationFlowCallbackFactory { +public class ConditionalLoaAuthenticatorFactory implements ConditionalAuthenticatorFactory, AuthenticationFlowCallbackFactory, EnvironmentDependentProviderFactory { public static final String PROVIDER_ID = "conditional-level-of-authentication"; private static final AuthenticationExecutionModel.Requirement[] REQUIREMENT_CHOICES = new AuthenticationExecutionModel.Requirement[]{ @@ -105,4 +107,9 @@ public class ConditionalLoaAuthenticatorFactory implements ConditionalAuthentica // NOP - instance created in create() method return null; } + + @Override + public boolean isSupported() { + return Profile.isFeatureEnabled(Profile.Feature.STEP_UP_AUTHENTICATION); + } } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java index 09c8d79928..7e458626a0 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java @@ -884,7 +884,13 @@ public class TokenManager { token.setNonce(clientSessionCtx.getAttribute(OIDCLoginProtocol.NONCE_PARAM, String.class)); token.setScope(clientSessionCtx.getScopeString()); - token.setAcr(getAcr(clientSession)); + if (Profile.isFeatureEnabled(Profile.Feature.STEP_UP_AUTHENTICATION)) { + token.setAcr(getAcr(clientSession)); + } else { + // Backwards compatibility behaviour prior step-up authentication was introduced + String acr = AuthenticationManager.isSSOAuthentication(clientSession) ? "0" : "1"; + token.setAcr(acr); + } String authTime = session.getNote(AuthenticationManager.AUTH_TIME); if (authTime != null) { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/LevelOfAssuranceFlowTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/LevelOfAssuranceFlowTest.java index 8968cb14df..6239a1c08e 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/LevelOfAssuranceFlowTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/LevelOfAssuranceFlowTest.java @@ -31,11 +31,13 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.keycloak.OAuth2Constants; import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.authentication.authenticators.browser.OTPFormAuthenticatorFactory; import org.keycloak.authentication.authenticators.browser.UsernamePasswordFormFactory; import org.keycloak.authentication.authenticators.conditional.ConditionalLoaAuthenticator; import org.keycloak.authentication.authenticators.conditional.ConditionalLoaAuthenticatorFactory; +import org.keycloak.common.Profile; import org.keycloak.events.Details; import org.keycloak.models.AuthenticationExecutionModel.Requirement; import org.keycloak.models.Constants; @@ -44,6 +46,7 @@ import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.representations.ClaimsRepresentation; import org.keycloak.representations.IDToken; +import org.keycloak.representations.idm.AuthenticationFlowRepresentation; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.EventRepresentation; import org.keycloak.representations.idm.RealmRepresentation; @@ -51,9 +54,12 @@ import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.AbstractTestRealmKeycloakTest; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.admin.ApiUtil; +import org.keycloak.testsuite.admin.authentication.AbstractAuthenticationTest; import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude; +import org.keycloak.testsuite.arquillian.annotation.DisableFeature; import org.keycloak.testsuite.authentication.PushButtonAuthenticatorFactory; import org.keycloak.testsuite.client.KeycloakTestingClient; +import org.keycloak.testsuite.pages.AppPage; import org.keycloak.testsuite.pages.ErrorPage; import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.pages.LoginTotpPage; @@ -66,6 +72,7 @@ import org.keycloak.testsuite.util.WaitUtils; import org.keycloak.util.JsonSerialization; import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertEquals; import static org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer.REMOTE; /** @@ -617,6 +624,26 @@ public class LevelOfAssuranceFlowTest extends AbstractTestRealmKeycloakTest { } + @Test + @DisableFeature(value = Profile.Feature.STEP_UP_AUTHENTICATION, skipRestart = true) + public void testDisableStepupFeatureTest() { + BrowserFlowTest.revertFlows(testRealm(), "browser - Level of Authentication FLow"); + + // Login normal way - should return 1 (backwards compatibility before step-up was introduced) + loginPage.open(); + authenticateWithUsernamePassword(); + authenticateWithTotp(); // OTP required due the user has it + assertLoggedInWithAcr("1"); + + // SSO login - should return 0 (backwards compatibility before step-up was introduced) + oauth.openLoginForm(); + assertLoggedInWithAcr("0"); + + // Flow is needed due the "after()" method + testingClient.server(TEST_REALM_NAME).run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow("browser - Level of Authentication FLow")); + } + + public void openLoginFormWithAcrClaim(boolean essential, String... acrValues) { openLoginFormWithAcrClaim(oauth, essential, acrValues); } diff --git a/themes/src/main/resources/theme/base/admin/resources/partials/client-detail.html b/themes/src/main/resources/theme/base/admin/resources/partials/client-detail.html index c9c2730493..78822fe23d 100755 --- a/themes/src/main/resources/theme/base/admin/resources/partials/client-detail.html +++ b/themes/src/main/resources/theme/base/admin/resources/partials/client-detail.html @@ -932,7 +932,7 @@ {{:: 'require-pushed-authorization-requests.tooltip' | translate}} -
+
@@ -953,7 +953,7 @@ {{:: 'acr-loa-map-client.tooltip' | translate}}
-
+
diff --git a/themes/src/main/resources/theme/base/admin/resources/partials/realm-login-settings.html b/themes/src/main/resources/theme/base/admin/resources/partials/realm-login-settings.html index ef3790108b..7f6837322f 100755 --- a/themes/src/main/resources/theme/base/admin/resources/partials/realm-login-settings.html +++ b/themes/src/main/resources/theme/base/admin/resources/partials/realm-login-settings.html @@ -72,7 +72,7 @@
{{:: 'sslRequired.tooltip' | translate}}
-
+