From 0f30b3118a3240264f4077b0aab42ae66c5bc61b Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Tue, 2 Feb 2021 16:00:48 -0300 Subject: [PATCH] [KEYCLOAK-16676] - Client attributes should not be stored if null or empty --- .../keycloak/models/jpa/ClientAdapter.java | 15 +++- .../models/map/client/MapClientAdapter.java | 7 ++ .../rest/TestApplicationResourceProvider.java | 23 +++++- ...estApplicationResourceProviderFactory.java | 5 +- .../resources/TestApplicationResource.java | 6 ++ .../keycloak/testsuite/admin/ClientTest.java | 8 +++ .../keycloak/testsuite/oauth/LogoutTest.java | 72 +++++++++++++++++++ 7 files changed, 132 insertions(+), 4 deletions(-) diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/ClientAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/ClientAdapter.java index 8ba83718f5..3737202679 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/ClientAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/ClientAdapter.java @@ -293,13 +293,26 @@ public class ClientAdapter implements ClientModel, JpaModel { @Override public void setAttribute(String name, String value) { + boolean valueUndefined = value == null || "".equals(value.trim()); + for (ClientAttributeEntity attr : entity.getAttributes()) { if (attr.getName().equals(name)) { - attr.setValue(value); + // clean up, so that attributes previously set with either a empty or null value are removed + // we should remove this in future versions so that new clients never store empty/null attributes + if (valueUndefined) { + removeAttribute(name); + } else { + attr.setValue(value); + } return; } } + // do not create attributes if empty or null + if (valueUndefined) { + return; + } + ClientAttributeEntity attr = new ClientAttributeEntity(); attr.setName(name); attr.setValue(value); diff --git a/model/map/src/main/java/org/keycloak/models/map/client/MapClientAdapter.java b/model/map/src/main/java/org/keycloak/models/map/client/MapClientAdapter.java index f0452ea339..161ceb688f 100644 --- a/model/map/src/main/java/org/keycloak/models/map/client/MapClientAdapter.java +++ b/model/map/src/main/java/org/keycloak/models/map/client/MapClientAdapter.java @@ -245,6 +245,13 @@ public abstract class MapClientAdapter extends AbstractClientModel adminLogoutActions; + private final BlockingQueue backChannelLogoutTokens; private final BlockingQueue adminPushNotBeforeActions; private final BlockingQueue adminTestAvailabilityAction; private final TestApplicationResourceProviderFactory.OIDCClientData oidcClientData; @@ -63,10 +65,13 @@ public class TestApplicationResourceProvider implements RealmResourceProvider { HttpRequest request; public TestApplicationResourceProvider(KeycloakSession session, BlockingQueue adminLogoutActions, + BlockingQueue backChannelLogoutTokens, BlockingQueue adminPushNotBeforeActions, - BlockingQueue adminTestAvailabilityAction, TestApplicationResourceProviderFactory.OIDCClientData oidcClientData) { + BlockingQueue adminTestAvailabilityAction, + TestApplicationResourceProviderFactory.OIDCClientData oidcClientData) { this.session = session; this.adminLogoutActions = adminLogoutActions; + this.backChannelLogoutTokens = backChannelLogoutTokens; this.adminPushNotBeforeActions = adminPushNotBeforeActions; this.adminTestAvailabilityAction = adminTestAvailabilityAction; this.oidcClientData = oidcClientData; @@ -79,6 +84,13 @@ public class TestApplicationResourceProvider implements RealmResourceProvider { adminLogoutActions.add(new JWSInput(data).readJsonContent(LogoutAction.class)); } + @POST + @Consumes(MediaType.APPLICATION_FORM_URLENCODED) + @Path("/admin/backchannelLogout") + public void backchannelLogout() throws JWSInputException { + backChannelLogoutTokens.add(new JWSInput(request.getDecodedFormParameters().getFirst(OAuth2Constants.LOGOUT_TOKEN)).readJsonContent(LogoutToken.class)); + } + @POST @Consumes(MediaType.TEXT_PLAIN_UTF_8) @Path("/admin/k_push_not_before") @@ -100,6 +112,13 @@ public class TestApplicationResourceProvider implements RealmResourceProvider { return adminLogoutActions.poll(10, TimeUnit.SECONDS); } + @GET + @Produces(MediaType.APPLICATION_JSON) + @Path("/poll-backchannel-logout") + public LogoutToken getBackChannelLogoutAction() throws InterruptedException { + return backChannelLogoutTokens.poll(20, TimeUnit.SECONDS); + } + @GET @Produces(MediaType.APPLICATION_JSON) @Path("/poll-admin-not-before") diff --git a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/rest/TestApplicationResourceProviderFactory.java b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/rest/TestApplicationResourceProviderFactory.java index 926f2f055f..2cc23f44c3 100644 --- a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/rest/TestApplicationResourceProviderFactory.java +++ b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/rest/TestApplicationResourceProviderFactory.java @@ -24,6 +24,7 @@ import org.keycloak.crypto.KeyType; import org.keycloak.crypto.KeyUse; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; +import org.keycloak.representations.LogoutToken; import org.keycloak.representations.adapters.action.LogoutAction; import org.keycloak.representations.adapters.action.PushNotBeforeAction; import org.keycloak.representations.adapters.action.TestAvailabilityAction; @@ -41,6 +42,7 @@ import java.util.concurrent.LinkedBlockingDeque; public class TestApplicationResourceProviderFactory implements RealmResourceProviderFactory { private BlockingQueue adminLogoutActions = new LinkedBlockingDeque<>(); + private BlockingQueue backChannelLogoutTokens = new LinkedBlockingDeque<>(); private BlockingQueue pushNotBeforeActions = new LinkedBlockingDeque<>(); private BlockingQueue testAvailabilityActions = new LinkedBlockingDeque<>(); @@ -48,7 +50,8 @@ public class TestApplicationResourceProviderFactory implements RealmResourceProv @Override public RealmResourceProvider create(KeycloakSession session) { - TestApplicationResourceProvider provider = new TestApplicationResourceProvider(session, adminLogoutActions, pushNotBeforeActions, testAvailabilityActions, oidcClientData); + TestApplicationResourceProvider provider = new TestApplicationResourceProvider(session, adminLogoutActions, + backChannelLogoutTokens, pushNotBeforeActions, testAvailabilityActions, oidcClientData); ResteasyProviderFactory.getInstance().injectProperties(provider); diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/client/resources/TestApplicationResource.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/client/resources/TestApplicationResource.java index 96c3c8df19..247a114a5c 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/client/resources/TestApplicationResource.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/client/resources/TestApplicationResource.java @@ -17,6 +17,7 @@ package org.keycloak.testsuite.client.resources; +import org.keycloak.representations.LogoutToken; import org.keycloak.representations.adapters.action.LogoutAction; import org.keycloak.representations.adapters.action.PushNotBeforeAction; import org.keycloak.representations.adapters.action.TestAvailabilityAction; @@ -39,6 +40,11 @@ public interface TestApplicationResource { @Path("/poll-admin-logout") LogoutAction getAdminLogoutAction(); + @GET + @Produces(MediaType.APPLICATION_JSON) + @Path("/poll-backchannel-logout") + LogoutToken getBackChannelLogoutToken(); + @GET @Produces(MediaType.APPLICATION_JSON) @Path("/poll-admin-not-before") diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java index 7e22311088..bcea606394 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java @@ -28,6 +28,7 @@ import org.keycloak.events.admin.ResourceType; import org.keycloak.models.AccountRoles; import org.keycloak.models.Constants; import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper; +import org.keycloak.protocol.oidc.OIDCConfigAttributes; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.OIDCLoginProtocolFactory; import org.keycloak.representations.adapters.action.GlobalRequestResult; @@ -385,6 +386,13 @@ public class ClientTest extends AbstractAdminTest { storedClient = realm.clients().get(client.getId()).toRepresentation(); assertClient(client, storedClient); + + storedClient.getAttributes().put(OIDCConfigAttributes.BACKCHANNEL_LOGOUT_URL, ""); + + realm.clients().get(storedClient.getId()).update(storedClient); + storedClient = realm.clients().get(client.getId()).toRepresentation(); + + assertFalse(storedClient.getAttributes().containsKey(OIDCConfigAttributes.BACKCHANNEL_LOGOUT_URL)); } @Test diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/LogoutTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/LogoutTest.java index 14e1293c9e..d419836ad8 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/LogoutTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/LogoutTest.java @@ -23,11 +23,15 @@ import org.junit.Test; import org.keycloak.OAuth2Constants; import org.keycloak.OAuthErrorException; +import org.keycloak.admin.client.resource.ClientResource; +import org.keycloak.admin.client.resource.ClientsResource; import org.keycloak.common.util.Time; import org.keycloak.events.Details; import org.keycloak.jose.jws.JWSHeader; import org.keycloak.jose.jws.JWSInput; +import org.keycloak.protocol.oidc.OIDCConfigAttributes; import org.keycloak.protocol.oidc.OIDCLoginProtocol; +import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.testsuite.AbstractKeycloakTest; import org.keycloak.testsuite.Assert; @@ -328,6 +332,74 @@ public class LogoutTest extends AbstractKeycloakTest { } } + @Test + public void successfulKLogoutAfterEmptyBackChannelUrl() throws Exception { + ClientsResource clients = adminClient.realm(oauth.getRealm()).clients(); + ClientRepresentation rep = clients.findByClientId(oauth.getClientId()).get(0); + + rep.getAttributes().put(OIDCConfigAttributes.BACKCHANNEL_LOGOUT_URL, ""); + + clients.get(rep.getId()).update(rep); + + oauth.doLogin("test-user@localhost", "password"); + + String code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + + oauth.clientSessionState("client-session"); + + OAuthClient.AccessTokenResponse tokenResponse = oauth.doAccessTokenRequest(code, "password"); + String idTokenString = tokenResponse.getIdToken(); + String logoutUrl = oauth.getLogoutUrl() + .idTokenHint(idTokenString) + .postLogoutRedirectUri(oauth.APP_AUTH_ROOT) + .build(); + + try (CloseableHttpClient c = HttpClientBuilder.create().disableRedirectHandling().build(); + CloseableHttpResponse response = c.execute(new HttpGet(logoutUrl))) { + assertThat(response, Matchers.statusCodeIsHC(Status.FOUND)); + assertThat(response.getFirstHeader(HttpHeaders.LOCATION).getValue(), is(oauth.APP_AUTH_ROOT)); + } + + assertNotNull(testingClient.testApp().getAdminLogoutAction()); + } + + @Test + public void backChannelPreferenceOverKLogout() throws Exception { + ClientsResource clients = adminClient.realm(oauth.getRealm()).clients(); + ClientRepresentation rep = clients.findByClientId(oauth.getClientId()).get(0); + + rep.getAttributes().put(OIDCConfigAttributes.BACKCHANNEL_LOGOUT_URL, oauth.APP_ROOT + "/admin/backchannelLogout"); + + ClientResource clientResource = clients.get(rep.getId()); + clientResource.update(rep); + + try { + oauth.doLogin("test-user@localhost", "password"); + + String code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + + oauth.clientSessionState("client-session"); + + OAuthClient.AccessTokenResponse tokenResponse = oauth.doAccessTokenRequest(code, "password"); + String idTokenString = tokenResponse.getIdToken(); + String logoutUrl = oauth.getLogoutUrl() + .idTokenHint(idTokenString) + .postLogoutRedirectUri(oauth.APP_AUTH_ROOT) + .build(); + + try (CloseableHttpClient c = HttpClientBuilder.create().disableRedirectHandling().build(); + CloseableHttpResponse response = c.execute(new HttpGet(logoutUrl))) { + assertThat(response, Matchers.statusCodeIsHC(Status.FOUND)); + assertThat(response.getFirstHeader(HttpHeaders.LOCATION).getValue(), is(oauth.APP_AUTH_ROOT)); + } + + assertNotNull(testingClient.testApp().getBackChannelLogoutToken()); + } finally { + rep.getAttributes().put(OIDCConfigAttributes.BACKCHANNEL_LOGOUT_URL, ""); + clientResource.update(rep); + } + } + private OAuthClient.AccessTokenResponse loginAndForceNewLoginPage() { oauth.doLogin("test-user@localhost", "password");