From bb12f3fb82ebb288643307b11396e3bd90199807 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Mon, 12 Feb 2024 17:47:48 +0100 Subject: [PATCH] Do not require non-builtin attributes for service accounts Closes #26716 Signed-off-by: rmartinc --- .../DeclarativeUserProfileProvider.java | 15 ++++-- .../oauth/ServiceAccountUserProfileTest.java | 49 +++++++++++++++---- 2 files changed, 49 insertions(+), 15 deletions(-) diff --git a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java index eda3b02276..2e97254a06 100644 --- a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java +++ b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java @@ -105,7 +105,7 @@ public class DeclarativeUserProfileProvider implements UserProfileProvider { protected Attributes createAttributes(UserProfileContext context, Map attributes, UserModel user, UserProfileMetadata metadata) { - if (user != null && user.getServiceAccountClientLink() != null) { + if (isServiceAccountUser(user)) { return new LegacyAttributes(context, attributes, user, metadata, session); } return new DefaultAttributes(context, attributes, user, metadata, session); @@ -285,13 +285,14 @@ public class DeclarativeUserProfileProvider implements UserProfileProvider { Predicate required = AttributeMetadata.ALWAYS_FALSE; if (rc != null) { if (rc.isAlways() || context.isRoleForContext(rc.getRoles())) { - required = AttributeMetadata.ALWAYS_TRUE; + // service accounts does not require common attributes + required = c -> !isServiceAccountUser(c.getUser()); // If scopes are configured, we will use scope-based selector and require the attribute just if scope is // in current authenticationSession (either default scope or by 'scope' parameter) if (rc.getScopes() != null && !rc.getScopes().isEmpty()) { if (context.canBeAuthFlowContext()) { - required = (c) -> requestedScopePredicate(c, rc.getScopes()); + required = (c) -> !isServiceAccountUser(c.getUser()) && requestedScopePredicate(c, rc.getScopes()); } else { // Scopes not available for admin and account contexts required = AttributeMetadata.ALWAYS_FALSE; @@ -300,7 +301,7 @@ public class DeclarativeUserProfileProvider implements UserProfileProvider { } else if (context.canBeAuthFlowContext() && rc.getScopes() != null && !rc.getScopes().isEmpty()) { // for contexts executed from auth flow and with configured scopes requirement // we have to create required validation with scopes based selector - required = (c) -> requestedScopePredicate(c, rc.getScopes()); + required = (c) -> !isServiceAccountUser(c.getUser()) && requestedScopePredicate(c, rc.getScopes()); } } @@ -364,7 +365,7 @@ public class DeclarativeUserProfileProvider implements UserProfileProvider { public boolean test(AttributeContext context) { UserModel user = context.getUser(); - if (user != null && user.getServiceAccountClientLink() != null) { + if (isServiceAccountUser(user)) { return false; } @@ -441,6 +442,10 @@ public class DeclarativeUserProfileProvider implements UserProfileProvider { return ac -> ac.getContext().isRoleForContext(viewRoles) || canEdit.test(ac); } + private boolean isServiceAccountUser(UserModel user) { + return user != null && user.getServiceAccountClientLink() != null; + } + /** * Get parsed config file configured in model. Default one used if not configured. */ diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountUserProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountUserProfileTest.java index 2de9232443..26419e5aa7 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountUserProfileTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountUserProfileTest.java @@ -21,25 +21,33 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.MatcherAssert.assertThat; +import jakarta.ws.rs.BadRequestException; import java.util.List; import java.util.Map; -import org.junit.Rule; +import java.util.Set; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.admin.client.resource.UserResource; import org.keycloak.common.constants.ServiceAccountConstants; +import org.keycloak.models.UserModel; import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.protocol.oidc.OIDCConfigAttributes; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.representations.userprofile.config.UPAttribute; +import org.keycloak.representations.userprofile.config.UPAttributeRequired; +import org.keycloak.representations.userprofile.config.UPConfig; import org.keycloak.testsuite.AbstractKeycloakTest; -import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.util.ClientBuilder; import org.keycloak.testsuite.util.RealmBuilder; import org.keycloak.testsuite.util.UserBuilder; +import org.keycloak.userprofile.UserProfileConstants; +import org.keycloak.validate.validators.EmailValidator; /** * @author Marek Posolda @@ -49,13 +57,6 @@ public class ServiceAccountUserProfileTest extends AbstractKeycloakTest { private static String userId; private static String userName; - @Rule - public AssertEvents events = new AssertEvents(this); - - @Rule - public ExpectedException expectedException = ExpectedException.none(); - - @Override public void addTestRealms(List testRealms) { @@ -158,4 +159,32 @@ public class ServiceAccountUserProfileTest extends AbstractKeycloakTest { assertFalse(representation.getAttributes().isEmpty()); assertEquals("attr-1-value", representation.getAttributes().get("attr-1").get(0)); } + + @Test + public void testEmailFormatIsEnforced() { + final RealmResource realm = adminClient.realm("test"); + UserResource serviceAccount = realm.users().get(userId); + UserRepresentation rep = serviceAccount.toRepresentation(); + rep.setEmail("invalidEmail"); + BadRequestException e = assertThrows(BadRequestException.class, () -> serviceAccount.update(rep)); + assertThat(e.getResponse().readEntity(String.class), containsString(EmailValidator.MESSAGE_INVALID_EMAIL)); + } + + @Test + public void testAttributesAreNotRequired() { + final RealmResource realm = adminClient.realm("test"); + UPConfig config = realm.users().userProfile().getConfiguration(); + UPAttribute lastName = config.getAttribute(UserModel.LAST_NAME); + assertNotNull("The attribute lastName is not defined in User Profile", lastName); + UPAttributeRequired upRequired = new UPAttributeRequired(Set.of( + UserProfileConstants.ROLE_ADMIN, UserProfileConstants.ROLE_USER), null); + lastName.setRequired(upRequired); + realm.users().userProfile().update(config); + getCleanup().addCleanup(() -> realm.users().userProfile().update(null)); + + UserResource serviceAccount = realm.users().get(userId); + UserRepresentation rep = serviceAccount.toRepresentation(); + rep.setLastName(null); + serviceAccount.update(rep); + } }