From 49b1dbba68af0b387d8d5101847d17321605a1cf Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Wed, 19 Feb 2020 14:27:15 -0300 Subject: [PATCH] [KEYCLOAK-11804] - Block service accounts to authenticate or manage credentials --- .../AuthenticationProcessor.java | 1 + .../UserCredentialStoreManager.java | 23 ++++++++++++++- .../testsuite/forms/ResetPasswordTest.java | 29 +++++++++++++++++++ .../testsuite/oauth/ServiceAccountTest.java | 29 +++++++++++++++++++ 4 files changed, 81 insertions(+), 1 deletion(-) diff --git a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java index b8abf25d73..6f00c23c1f 100755 --- a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java +++ b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java @@ -1061,6 +1061,7 @@ public class AuthenticationProcessor { public void validateUser(UserModel authenticatedUser) { if (authenticatedUser == null) return; if (!authenticatedUser.isEnabled()) throw new AuthenticationFlowException(AuthenticationFlowError.USER_DISABLED); + if (authenticatedUser.getServiceAccountClientLink() != null) throw new AuthenticationFlowException(AuthenticationFlowError.UNKNOWN_USER); } protected Response authenticationComplete() { diff --git a/services/src/main/java/org/keycloak/credential/UserCredentialStoreManager.java b/services/src/main/java/org/keycloak/credential/UserCredentialStoreManager.java index 6c6cb8c222..0c2318553f 100644 --- a/services/src/main/java/org/keycloak/credential/UserCredentialStoreManager.java +++ b/services/src/main/java/org/keycloak/credential/UserCredentialStoreManager.java @@ -60,16 +60,19 @@ public class UserCredentialStoreManager implements UserCredentialManager, OnUser @Override public void updateCredential(RealmModel realm, UserModel user, CredentialModel cred) { + throwExceptionIfInvalidUser(user); getStoreForUser(user).updateCredential(realm, user, cred); } @Override public CredentialModel createCredential(RealmModel realm, UserModel user, CredentialModel cred) { + throwExceptionIfInvalidUser(user); return getStoreForUser(user).createCredential(realm, user, cred); } @Override public boolean removeStoredCredential(RealmModel realm, UserModel user, String id) { + throwExceptionIfInvalidUser(user); boolean removalResult = getStoreForUser(user).removeStoredCredential(realm, user, id); session.userCache().evict(realm, user); return removalResult; @@ -97,6 +100,7 @@ public class UserCredentialStoreManager implements UserCredentialManager, OnUser @Override public boolean moveCredentialTo(RealmModel realm, UserModel user, String id, String newPreviousCredentialId){ + throwExceptionIfInvalidUser(user); return getStoreForUser(user).moveCredentialTo(realm, user, id, newPreviousCredentialId); } @@ -107,6 +111,7 @@ public class UserCredentialStoreManager implements UserCredentialManager, OnUser @Override public CredentialModel createCredentialThroughProvider(RealmModel realm, UserModel user, CredentialModel model){ + throwExceptionIfInvalidUser(user); List credentialProviders = session.getKeycloakSessionFactory().getProviderFactories(CredentialProvider.class) .stream() .map(f -> session.getProvider(CredentialProvider.class, f.getId())) @@ -121,6 +126,7 @@ public class UserCredentialStoreManager implements UserCredentialManager, OnUser @Override public void updateCredentialLabel(RealmModel realm, UserModel user, String credentialId, String userLabel){ + throwExceptionIfInvalidUser(user); CredentialModel credential = getStoredCredentialById(realm, user, credentialId); credential.setUserLabel(userLabel); getStoreForUser(user).updateCredential(realm, user, credential); @@ -132,7 +138,9 @@ public class UserCredentialStoreManager implements UserCredentialManager, OnUser @Override public boolean isValid(RealmModel realm, UserModel user, List inputs) { - + if (!isValid(user)) { + return false; + } List toValidate = new LinkedList<>(); toValidate.addAll(inputs); if (!StorageId.isLocalStorage(user)) { @@ -203,6 +211,7 @@ public class UserCredentialStoreManager implements UserCredentialManager, OnUser } } else { + throwExceptionIfInvalidUser(user); if (user.getFederationLink() != null) { UserStorageProvider provider = UserStorageManager.getStorageProvider(session, realm, user.getFederationLink()); if (provider instanceof CredentialInputUpdater) { @@ -234,6 +243,7 @@ public class UserCredentialStoreManager implements UserCredentialManager, OnUser } } else { + throwExceptionIfInvalidUser(user); if (user.getFederationLink() != null) { UserStorageProvider provider = UserStorageManager.getStorageProvider(session, realm, user.getFederationLink()); if (provider instanceof CredentialInputUpdater) { @@ -384,4 +394,15 @@ public class UserCredentialStoreManager implements UserCredentialManager, OnUser public void close() { } + + private boolean isValid(UserModel user) { + return user != null && user.getServiceAccountClientLink() == null; + } + + private void throwExceptionIfInvalidUser(UserModel user) { + if (user == null || isValid(user)) { + return; + } + throw new RuntimeException("You can not manage credentials for this user"); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java index 56d2ba2768..b9f0afe895 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java @@ -18,8 +18,10 @@ package org.keycloak.testsuite.forms; import org.hamcrest.Matchers; import org.jboss.arquillian.drone.api.annotation.Drone; +import org.keycloak.admin.client.resource.UserResource; import org.keycloak.authentication.actiontoken.resetcred.ResetCredentialsActionToken; import org.jboss.arquillian.graphene.page.Page; +import org.keycloak.common.constants.ServiceAccountConstants; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.events.EventType; @@ -44,6 +46,7 @@ import org.keycloak.testsuite.updaters.ClientAttributeUpdater; import org.keycloak.testsuite.util.GreenMailRule; import org.keycloak.testsuite.util.MailUtils; import org.keycloak.testsuite.util.OAuthClient; +import org.keycloak.testsuite.util.RealmBuilder; import org.keycloak.testsuite.util.SecondBrowser; import org.keycloak.testsuite.util.UserActionTokenBuilder; import org.keycloak.testsuite.util.UserBuilder; @@ -82,6 +85,8 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { @Override public void configureTestRealm(RealmRepresentation testRealm) { + RealmBuilder.edit(testRealm) + .client(org.keycloak.testsuite.util.ClientBuilder.create().clientId("client-user").serviceAccount()); } @Before @@ -1055,4 +1060,28 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { assertThat(driver2.getPageSource(), Matchers.containsString("Your account has been updated.")); } + @Test + public void failResetPasswordServiceAccount() { + String username = ServiceAccountConstants.SERVICE_ACCOUNT_USER_PREFIX + "client-user"; + UserRepresentation serviceAccount = testRealm().users() + .search(username).get(0); + + serviceAccount.toString(); + + UserResource serviceAccount1 = testRealm().users().get(serviceAccount.getId()); + + serviceAccount = serviceAccount1.toRepresentation(); + serviceAccount.setEmail("client-user@test.com"); + serviceAccount1.update(serviceAccount); + + String resetUri = oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials"; + driver.navigate().to(resetUri); + + resetPasswordPage.assertCurrent(); + + resetPasswordPage.changePassword(username); + + loginPage.assertCurrent(); + assertEquals("Invalid username or password.", errorPage.getError()); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountTest.java index 189ef058e3..879a2d9058 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountTest.java @@ -18,9 +18,12 @@ package org.keycloak.testsuite.oauth; import org.apache.http.HttpResponse; +import org.hamcrest.Matchers; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.keycloak.admin.client.resource.UserResource; import org.keycloak.authentication.authenticators.client.ClientIdAndSecretAuthenticator; import org.keycloak.common.constants.ServiceAccountConstants; import org.keycloak.crypto.Algorithm; @@ -32,6 +35,7 @@ import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.representations.AccessToken; import org.keycloak.representations.RefreshToken; import org.keycloak.representations.idm.ClientRepresentation; +import org.keycloak.representations.idm.CredentialRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.AbstractKeycloakTest; @@ -44,11 +48,14 @@ import org.keycloak.testsuite.util.RealmBuilder; import org.keycloak.testsuite.util.TokenSignatureUtil; import org.keycloak.testsuite.util.UserBuilder; +import java.util.Arrays; import java.util.List; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import javax.ws.rs.ClientErrorException; + /** * @author Marek Posolda */ @@ -60,6 +67,9 @@ public class ServiceAccountTest extends AbstractKeycloakTest { @Rule public AssertEvents events = new AssertEvents(this); + @Rule + public ExpectedException expectedException = ExpectedException.none(); + @Override public void beforeAbstractKeycloakTest() throws Exception { @@ -281,6 +291,25 @@ public class ServiceAccountTest extends AbstractKeycloakTest { conductClientCredentialsAuthRequest(Algorithm.HS256, Algorithm.ES256, Algorithm.PS256); } + @Test + public void failManagePassword() { + UserResource serviceAccount = adminClient.realm("test").users().get(userId); + UserRepresentation representation = serviceAccount.toRepresentation(); + + CredentialRepresentation password = new CredentialRepresentation(); + password.setValue("password"); + password.setType(CredentialRepresentation.PASSWORD); + password.setTemporary(false); + + representation.setCredentials(Arrays.asList(password)); + + this.expectedException.expect(Matchers.allOf(Matchers.instanceOf(ClientErrorException.class), + Matchers.hasProperty("response", Matchers.hasProperty("status", Matchers.is(409))))); + this.expectedException.reportMissingExceptionWithMessage("Should fail, should not be possible to manage credentials for service accounts"); + + serviceAccount.update(representation); + } + private void conductClientCredentialsAuthRequest(String expectedRefreshAlg, String expectedAccessAlg, String realmTokenAlg) throws Exception { try { /// Realm Setting is used for ID Token Signature Algorithm