From a5a2753d1139410e66249cdd05daa85c139d11ec Mon Sep 17 00:00:00 2001 From: rmartinc Date: Mon, 26 Jun 2023 17:10:50 +0200 Subject: [PATCH] Don't allow impersonate disabled users or service accounts Closes https://github.com/keycloak/keycloak/issues/21106 --- .../resources/admin/UserResource.java | 8 ++++ .../testsuite/admin/ImpersonationTest.java | 40 +++++++++++++++---- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java index cb0d9642e4..4286888313 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java @@ -331,6 +331,14 @@ public class UserResource { ProfileHelper.requireFeature(Profile.Feature.IMPERSONATION); auth.users().requireImpersonate(user); + + if (!user.isEnabled()) { + throw ErrorResponse.error("User is disabled", Status.BAD_REQUEST); + } + if (user.getServiceAccountClientLink() != null) { + throw ErrorResponse.error("Service accounts cannot be impersonated", Status.BAD_REQUEST); + } + RealmModel authenticatedRealm = auth.adminAuth().getRealm(); // if same realm logout before impersonation boolean sameRealm = false; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ImpersonationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ImpersonationTest.java index 2932aff8a6..bb62cd26ed 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ImpersonationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ImpersonationTest.java @@ -24,6 +24,7 @@ import org.apache.http.impl.client.BasicCookieStore; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; import org.apache.http.util.EntityUtils; +import org.hamcrest.MatcherAssert; import org.jboss.arquillian.graphene.page.Page; import org.jboss.resteasy.client.jaxrs.ResteasyClient; import org.jboss.resteasy.client.jaxrs.ResteasyClientBuilder; @@ -56,7 +57,6 @@ import org.keycloak.testsuite.AbstractKeycloakTest; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.auth.page.AuthRealm; import org.keycloak.testsuite.pages.AppPage; -import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.util.*; import org.openqa.selenium.Cookie; @@ -104,9 +104,6 @@ public class ImpersonationTest extends AbstractKeycloakTest { @Page protected AppPage appPage; - @Page - protected LoginPage loginPage; - private String impersonatedUserId; @Override @@ -181,6 +178,20 @@ public class ImpersonationTest extends AbstractKeycloakTest { testForbiddenImpersonation("bad-impersonator", "test"); } + @Test + public void testImpersonationFailsForDisabledUser() { + UserResource impersonatedUserResource = adminClient.realms().realm("test").users().get(impersonatedUserId); + UserRepresentation impersonatedUserRepresentation = impersonatedUserResource.toRepresentation(); + impersonatedUserRepresentation.setEnabled(false); + impersonatedUserResource.update(impersonatedUserRepresentation); + try { + testBadRequestImpersonation("impersonator", "test", impersonatedUserId, "test", "User is disabled"); + } finally { + impersonatedUserRepresentation.setEnabled(true); + impersonatedUserResource.update(impersonatedUserRepresentation); + } + } + @Test public void testImpersonateByMastertBadImpersonator() { String userId; @@ -246,6 +257,9 @@ public class ImpersonationTest extends AbstractKeycloakTest { // Impersonation testSuccessfulServiceAccountImpersonation(user, "test"); + // test impersonation over the service account fails + testBadRequestImpersonation("impersonator", "test", user.getId(), "test", "Service accounts cannot be impersonated"); + // Remove test client ApiUtil.findClientByClientId(realm, "service-account-cl").remove(); } @@ -331,7 +345,7 @@ public class ImpersonationTest extends AbstractKeycloakTest { .collect(Collectors.toSet()); Assert.assertNotNull(cookies); - Assert.assertThat(cookies, is(not(empty()))); + MatcherAssert.assertThat(cookies, is(not(empty()))); return cookies; } catch (IOException e) { @@ -344,7 +358,19 @@ public class ImpersonationTest extends AbstractKeycloakTest { client.realms().realm("test").users().get(impersonatedUserId).impersonate(); Assert.fail("Expected ClientErrorException wasn't thrown."); } catch (ClientErrorException e) { - Assert.assertThat(e.getMessage(), containsString("403 Forbidden")); + MatcherAssert.assertThat(e.getMessage(), containsString("403 Forbidden")); + } + } + + protected void testBadRequestImpersonation(String admin, String adminRealm, String impersonatedId, + String impersonatedRealm, String errorExpected) { + try (Keycloak client = createAdminClient(adminRealm, establishClientId(adminRealm), admin)) { + client.realms().realm(impersonatedRealm).users().get(impersonatedId).impersonate(); + Assert.fail("Expected ClientErrorException wasn't thrown."); + } catch (ClientErrorException e) { + Assert.assertEquals(Response.Status.BAD_REQUEST, e.getResponse().getStatusInfo()); + ErrorRepresentation error = e.getResponse().readEntity(ErrorRepresentation.class); + Assert.assertEquals(errorExpected, error.getErrorMessage()); } } @@ -438,7 +464,7 @@ public class ImpersonationTest extends AbstractKeycloakTest { .collect(Collectors.toSet()); Assert.assertNotNull(cookies); - Assert.assertThat(cookies, is(not(empty()))); + MatcherAssert.assertThat(cookies, is(not(empty()))); return cookies; } catch (IOException e) {