From b019cf61298ce146d7282882477b06fdc33a30f7 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Wed, 15 May 2024 12:44:37 -0300 Subject: [PATCH] Support unmanaged attributes for service accounts and make sure they are only managed through the admin api Closes #29362 Signed-off-by: Pedro Igor --- .../admin/client/resource/UserResource.java | 5 + .../admin-ui/cypress/e2e/users_test.spec.ts | 2 +- .../admin-ui/src/components/users/resource.ts | 8 -- js/apps/admin-ui/src/user/EditUser.tsx | 3 +- .../src/resources/users.ts | 9 ++ .../admin/ui/rest/AdminExtResource.java | 5 - .../keycloak/admin/ui/rest/UserResource.java | 68 ---------- .../keycloak/admin/ui/rest/UsersResource.java | 45 ------- .../userprofile/DefaultAttributes.java | 31 +---- .../resources/admin/UserResource.java | 28 ++++ .../DeclarativeUserProfileProvider.java | 3 +- .../userprofile/LegacyAttributes.java | 120 ------------------ .../userprofile/ServiceAccountAttributes.java | 61 +++++++++ .../oauth/ServiceAccountUserProfileTest.java | 5 + 14 files changed, 115 insertions(+), 278 deletions(-) delete mode 100644 js/apps/admin-ui/src/components/users/resource.ts delete mode 100644 rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/UserResource.java delete mode 100644 rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/UsersResource.java delete mode 100644 services/src/main/java/org/keycloak/userprofile/LegacyAttributes.java create mode 100644 services/src/main/java/org/keycloak/userprofile/ServiceAccountAttributes.java diff --git a/integration/admin-client-jee/src/main/java/org/keycloak/admin/client/resource/UserResource.java b/integration/admin-client-jee/src/main/java/org/keycloak/admin/client/resource/UserResource.java index 1901a53aa5..f28fa1fdaf 100755 --- a/integration/admin-client-jee/src/main/java/org/keycloak/admin/client/resource/UserResource.java +++ b/integration/admin-client-jee/src/main/java/org/keycloak/admin/client/resource/UserResource.java @@ -329,4 +329,9 @@ public interface UserResource { @Path("impersonation") @Produces(MediaType.APPLICATION_JSON) Map impersonate(); + + @GET + @Path("unmanagedAttributes") + @Produces(MediaType.APPLICATION_JSON) + Map> getUnmanagedAttributes(); } diff --git a/js/apps/admin-ui/cypress/e2e/users_test.spec.ts b/js/apps/admin-ui/cypress/e2e/users_test.spec.ts index dc202e4da3..030309dc19 100644 --- a/js/apps/admin-ui/cypress/e2e/users_test.spec.ts +++ b/js/apps/admin-ui/cypress/e2e/users_test.spec.ts @@ -203,7 +203,7 @@ describe("User creation", () => { cy.wait("@save-user").should(({ request, response }) => { expect(response?.statusCode).to.equal(204); - expect(request.body.attributes, "response body").deep.equal({ + expect(request.body.attributes, "response body").deep.contains({ "key-multiple": ["other value"], }); }); diff --git a/js/apps/admin-ui/src/components/users/resource.ts b/js/apps/admin-ui/src/components/users/resource.ts deleted file mode 100644 index afc52da43e..0000000000 --- a/js/apps/admin-ui/src/components/users/resource.ts +++ /dev/null @@ -1,8 +0,0 @@ -import KeycloakAdminClient from "@keycloak/keycloak-admin-client"; -import { fetchAdminUI } from "../../context/auth/admin-ui-endpoint"; - -export const getUnmanagedAttributes = ( - adminClient: KeycloakAdminClient, - id: string, -): Promise | undefined> => - fetchAdminUI(adminClient, `ui-ext/users/${id}/unmanagedAttributes`); diff --git a/js/apps/admin-ui/src/user/EditUser.tsx b/js/apps/admin-ui/src/user/EditUser.tsx index 4963436a14..091ea1fc32 100644 --- a/js/apps/admin-ui/src/user/EditUser.tsx +++ b/js/apps/admin-ui/src/user/EditUser.tsx @@ -32,7 +32,6 @@ import { RoutableTabs, useRoutableTab, } from "../components/routable-tabs/RoutableTabs"; -import { getUnmanagedAttributes } from "../components/users/resource"; import { ViewHeader } from "../components/view-header/ViewHeader"; import { useAccess } from "../context/access/Access"; import { useRealm } from "../context/realm-context/RealmContext"; @@ -117,7 +116,7 @@ export default function EditUser() { userProfileMetadata: true, }) as UIUserRepresentation | undefined, adminClient.attackDetection.findOne({ id: id! }), - getUnmanagedAttributes(adminClient, id!), + adminClient.users.getUnmanagedAttributes({ id: id! }), adminClient.users.getProfile({ realm: realmName }), ]), ([realm, userData, attackDetection, unmanagedAttributes, upConfig]) => { diff --git a/js/libs/keycloak-admin-client/src/resources/users.ts b/js/libs/keycloak-admin-client/src/resources/users.ts index 4fd254316b..23e8034888 100644 --- a/js/libs/keycloak-admin-client/src/resources/users.ts +++ b/js/libs/keycloak-admin-client/src/resources/users.ts @@ -494,6 +494,15 @@ export class Users extends Resource<{ realm?: string }> { urlParamKeys: ["id", "clientId"], }); + public getUnmanagedAttributes = this.makeRequest< + { id: string }, + Record + >({ + method: "GET", + path: "/{id}/unmanagedAttributes", + urlParamKeys: ["id"], + }); + constructor(client: KeycloakAdminClient) { super(client, { path: "/admin/realms/{realm}/users", diff --git a/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/AdminExtResource.java b/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/AdminExtResource.java index 3a04921123..eb04586b12 100644 --- a/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/AdminExtResource.java +++ b/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/AdminExtResource.java @@ -54,9 +54,4 @@ public final class AdminExtResource { public UIRealmResource realm() { return new UIRealmResource(session, auth, adminEvent); } - - @Path("/users") - public UsersResource users() { - return new UsersResource(session, auth); - } } diff --git a/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/UserResource.java b/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/UserResource.java deleted file mode 100644 index a37dcd6b03..0000000000 --- a/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/UserResource.java +++ /dev/null @@ -1,68 +0,0 @@ -package org.keycloak.admin.ui.rest; - -import static java.util.Collections.emptyList; -import static java.util.Optional.ofNullable; -import static org.keycloak.userprofile.UserProfileContext.USER_API; - -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Map.Entry; -import java.util.stream.Collectors; - -import jakarta.ws.rs.GET; -import jakarta.ws.rs.Path; -import jakarta.ws.rs.Produces; -import jakarta.ws.rs.core.MediaType; -import org.jboss.resteasy.reactive.NoCache; -import org.keycloak.models.KeycloakSession; -import org.keycloak.models.RealmModel; -import org.keycloak.models.UserModel; -import org.keycloak.representations.userprofile.config.UPConfig; -import org.keycloak.userprofile.UserProfile; -import org.keycloak.userprofile.UserProfileProvider; -import org.keycloak.utils.StringUtil; - -/** - * @author Pedro Igor - */ -public class UserResource { - - private final KeycloakSession session; - private final UserModel user; - - public UserResource(KeycloakSession session, UserModel user) { - this.session = session; - this.user = user; - } - - @GET - @Path("unmanagedAttributes") - @NoCache - @Produces(MediaType.APPLICATION_JSON) - public Map> getUnmanagedAttributes() { - UserProfileProvider provider = session.getProvider(UserProfileProvider.class); - - UserProfile profile = provider.create(USER_API, user); - Map> managedAttributes = profile.getAttributes().getReadable(); - Map> attributes = new HashMap<>(user.getAttributes()); - UPConfig upConfig = provider.getConfiguration(); - - if (upConfig.getUnmanagedAttributePolicy() == null) { - return Collections.emptyMap(); - } - - Map> unmanagedAttributes = profile.getAttributes().getUnmanagedAttributes(); - managedAttributes.entrySet().removeAll(unmanagedAttributes.entrySet()); - attributes.entrySet().removeAll(managedAttributes.entrySet()); - - attributes.remove(UserModel.USERNAME); - attributes.remove(UserModel.EMAIL); - - return attributes.entrySet().stream() - .filter(entry -> ofNullable(entry.getValue()).orElse(emptyList()).stream().anyMatch(StringUtil::isNotBlank)) - .collect(Collectors.toMap(Entry::getKey, Entry::getValue)); - } - -} diff --git a/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/UsersResource.java b/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/UsersResource.java deleted file mode 100644 index fee417a04c..0000000000 --- a/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/UsersResource.java +++ /dev/null @@ -1,45 +0,0 @@ -package org.keycloak.admin.ui.rest; - -import jakarta.ws.rs.NotFoundException; -import jakarta.ws.rs.Path; -import jakarta.ws.rs.PathParam; -import org.keycloak.models.KeycloakSession; -import org.keycloak.models.RealmModel; -import org.keycloak.models.UserModel; -import org.keycloak.models.UserSessionModel; -import org.keycloak.models.light.LightweightUserAdapter; -import org.keycloak.services.resources.admin.permissions.AdminPermissionEvaluator; -import jakarta.ws.rs.ForbiddenException; - -public class UsersResource { - private final KeycloakSession session; - - private final AdminPermissionEvaluator auth; - - public UsersResource(KeycloakSession session, AdminPermissionEvaluator auth) { - this.session = session; - this.auth = auth; - } - - @Path("{id}") - public UserResource getUser(@PathParam("id") String id) { - RealmModel realm = session.getContext().getRealm(); - UserModel user = null; - if (LightweightUserAdapter.isLightweightUser(id)) { - UserSessionModel userSession = session.sessions().getUserSession(realm, LightweightUserAdapter.getLightweightUserId(id)); - if (userSession != null) { - user = userSession.getUser(); - } - } else { - user = session.users().getUserById(realm, id); - } - - if (user == null) { - // we do this to make sure somebody can't phish ids - if (auth.users().canQuery()) throw new NotFoundException("User not found"); - else throw new ForbiddenException(); - } - - return new UserResource(session, user); - } -} diff --git a/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java b/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java index f9cb740cda..82609bceeb 100644 --- a/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java +++ b/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java @@ -98,18 +98,6 @@ public class DefaultAttributes extends HashMap> implements return !isAllowEditUnmanagedAttribute(); } - if (UserModel.USERNAME.equals(name)) { - if (isServiceAccountUser()) { - return true; - } - } - - if (UserModel.EMAIL.equals(name)) { - if (isServiceAccountUser()) { - return false; - } - } - if (isReadOnlyFromMetadata(name) || isReadOnlyInternalAttribute(name)) { return true; } @@ -311,10 +299,6 @@ public class DefaultAttributes extends HashMap> implements return Collections.unmodifiableMap(this); } - protected boolean isServiceAccountUser() { - return user != null && user.getServiceAccountClientLink() != null; - } - private AttributeContext createAttributeContext(Entry> attribute, AttributeMetadata metadata) { return new AttributeContext(context, session, attribute, user, metadata, this); } @@ -482,7 +466,7 @@ public class DefaultAttributes extends HashMap> implements return valuesStream.collect(Collectors.toList()); } - private boolean isAllowUnmanagedAttribute() { + protected boolean isAllowUnmanagedAttribute() { UnmanagedAttributePolicy unmanagedAttributePolicy = upConfig.getUnmanagedAttributePolicy(); if (unmanagedAttributePolicy == null) { @@ -501,11 +485,8 @@ public class DefaultAttributes extends HashMap> implements return UnmanagedAttributePolicy.ENABLED.equals(unmanagedAttributePolicy); } - private void setUserName(Map> newAttributes, List lowerCaseEmailList) { - if (isServiceAccountUser()) { - return; - } - newAttributes.put(UserModel.USERNAME, lowerCaseEmailList); + protected void setUserName(Map> newAttributes, List values) { + newAttributes.put(UserModel.USERNAME, values); } protected boolean isIncludeAttributeIfNotProvided(AttributeMetadata metadata) { @@ -530,10 +511,6 @@ public class DefaultAttributes extends HashMap> implements return true; } - if (isServiceAccountUser()) { - return true; - } - return isReadOnlyInternalAttribute(name); } @@ -575,7 +552,7 @@ public class DefaultAttributes extends HashMap> implements return unmanagedAttributes; } - private AttributeMetadata createUnmanagedAttributeMetadata(String name) { + protected AttributeMetadata createUnmanagedAttributeMetadata(String name) { return new AttributeMetadata(name, Integer.MAX_VALUE) { final UnmanagedAttributePolicy unmanagedAttributePolicy = upConfig.getUnmanagedAttributePolicy(); 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 22cd2e640f..13dc46c350 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 @@ -108,6 +108,7 @@ import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.Response.Status; import jakarta.ws.rs.core.UriBuilder; +import org.keycloak.utils.StringUtil; import java.net.URI; import java.text.MessageFormat; @@ -118,6 +119,7 @@ import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Objects; import java.util.Properties; import java.util.Set; @@ -125,6 +127,8 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import java.util.stream.Stream; +import static java.util.Collections.emptyList; +import static java.util.Optional.ofNullable; import static org.keycloak.models.ImpersonationSessionNote.IMPERSONATOR_ID; import static org.keycloak.models.ImpersonationSessionNote.IMPERSONATOR_USERNAME; import static org.keycloak.userprofile.UserProfileContext.USER_API; @@ -1051,6 +1055,30 @@ public class UserResource { } } + @GET + @Path("unmanagedAttributes") + @NoCache + @Produces(MediaType.APPLICATION_JSON) + @Tag(name = KeycloakOpenAPI.Admin.Tags.USERS) + @Operation() + public Map> getUnmanagedAttributes() { + UserProfileProvider provider = session.getProvider(UserProfileProvider.class); + + UserProfile profile = provider.create(USER_API, user); + Map> managedAttributes = profile.getAttributes().getReadable(); + Map> unmanagedAttributes = profile.getAttributes().getUnmanagedAttributes(); + managedAttributes.entrySet().removeAll(unmanagedAttributes.entrySet()); + Map> attributes = new HashMap<>(user.getAttributes()); + attributes.entrySet().removeAll(managedAttributes.entrySet()); + + attributes.remove(UserModel.USERNAME); + attributes.remove(UserModel.EMAIL); + + return attributes.entrySet().stream() + .filter(entry -> ofNullable(entry.getValue()).orElse(emptyList()).stream().anyMatch(StringUtil::isNotBlank)) + .collect(Collectors.toMap(Entry::getKey, Entry::getValue)); + } + /** * Converts the specified {@link UserSessionModel} into a {@link UserSessionRepresentation}. * diff --git a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java index 65503a9fb6..60b60f8864 100644 --- a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java +++ b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java @@ -39,7 +39,6 @@ import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; -import org.keycloak.models.UserProvider; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.sessions.AuthenticationSessionModel; import org.keycloak.userprofile.config.DeclarativeUserProfileModel; @@ -107,7 +106,7 @@ public class DeclarativeUserProfileProvider implements UserProfileProvider { UserModel user, UserProfileMetadata metadata) { if (isServiceAccountUser(user)) { - return new LegacyAttributes(context, attributes, user, metadata, session); + return new ServiceAccountAttributes(context, attributes, user, metadata, session); } return new DefaultAttributes(context, attributes, user, metadata, session); } diff --git a/services/src/main/java/org/keycloak/userprofile/LegacyAttributes.java b/services/src/main/java/org/keycloak/userprofile/LegacyAttributes.java deleted file mode 100644 index 22554cd5a4..0000000000 --- a/services/src/main/java/org/keycloak/userprofile/LegacyAttributes.java +++ /dev/null @@ -1,120 +0,0 @@ -package org.keycloak.userprofile; - -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import org.keycloak.models.Constants; -import org.keycloak.models.KeycloakSession; -import org.keycloak.models.RealmModel; -import org.keycloak.models.UserModel; - -/** - * Enables legacy support when managing attributes without the declarative provider. - * - * @author Pedro Igor - */ -public class LegacyAttributes extends DefaultAttributes { - - public LegacyAttributes(UserProfileContext context, Map attributes, UserModel user, - UserProfileMetadata profileMetadata, KeycloakSession session) { - super(context, attributes, user, profileMetadata, session); - } - - @Override - protected boolean isSupportedAttribute(String name) { - if (UserProfileContext.USER_API.equals(context) || UserProfileContext.ACCOUNT.equals(context)) { - return true; - } - - if (super.isSupportedAttribute(name)) { - return true; - } - - if (name.startsWith(Constants.USER_ATTRIBUTES_PREFIX)) { - return true; - } - - return false; - } - - @Override - public boolean isReadOnly(String name) { - RealmModel realm = session.getContext().getRealm(); - - if (isReadOnlyInternalAttribute(name)) { - return true; - } - - if (user == null) { - return false; - } - - if (UserModel.USERNAME.equals(name)) { - if (isServiceAccountUser()) { - return true; - } - if (UserProfileContext.IDP_REVIEW.equals(context)) { - return false; - } - if (realm.isRegistrationEmailAsUsername()) { - return true; - } - return !realm.isEditUsernameAllowed(); - } - - if (UserModel.EMAIL.equals(name)) { - if (isServiceAccountUser()) { - return false; - } - if (UserProfileContext.IDP_REVIEW.equals(context) - || UserProfileContext.USER_API.equals(context)) { - return false; - } - if (realm.isRegistrationEmailAsUsername() && !realm.isEditUsernameAllowed()) { - return true; - } - } - - return false; - } - - @Override - public Map> getReadable() { - if(user == null || user.getAttributes() == null) { - return super.getReadable(); - } - - return new HashMap<>(user.getAttributes()); - } - - @Override - public Map> getWritable() { - Map> attributes = new HashMap<>(this); - RealmModel realm = session.getContext().getRealm(); - - for (String name : nameSet()) { - if (isReadOnly(name)) { - if (UserModel.USERNAME.equals(name) - && realm.isRegistrationEmailAsUsername()) { - continue; - } - attributes.remove(name); - } - } - - return attributes; - } - - @Override - protected boolean isIncludeAttributeIfNotProvided(AttributeMetadata metadata) { - if (UserModel.LOCALE.equals(metadata.getName())) { - // locale is an internal attribute and should be updated as a regular attribute - return false; - } - - // user api expects that attributes are not updated if not provided when in legacy mode - return UserProfileContext.USER_API.equals(context); - } -} diff --git a/services/src/main/java/org/keycloak/userprofile/ServiceAccountAttributes.java b/services/src/main/java/org/keycloak/userprofile/ServiceAccountAttributes.java new file mode 100644 index 0000000000..7eb7a89ffa --- /dev/null +++ b/services/src/main/java/org/keycloak/userprofile/ServiceAccountAttributes.java @@ -0,0 +1,61 @@ +package org.keycloak.userprofile; + +import java.util.List; +import java.util.Map; + +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.UserModel; + +/** + *

A specific {@link Attributes} implementation to handle service accounts. + * + *

Service accounts are not regular users, and it should be possible to manage unmanaged attributes but only when + * operating through the {@link UserProfileContext#USER_API}. Otherwise, administrators will be forced to enable unmanaged + * attributes by setting a {@link org.keycloak.representations.userprofile.config.UPConfig.UnmanagedAttributePolicy} or + * end up defining managed attributes that are specific for service accounts in the user profile configuration, which is + * mainly targeted for regular users. + * + * + * @author Pedro Igor + */ +public class ServiceAccountAttributes extends DefaultAttributes { + + public ServiceAccountAttributes(UserProfileContext context, Map attributes, UserModel user, + UserProfileMetadata profileMetadata, KeycloakSession session) { + super(context, attributes, user, profileMetadata, session); + } + + @Override + public boolean isReadOnly(String name) { + if (UserModel.USERNAME.equals(name)) { + return true; + } + + return !UserProfileContext.USER_API.equals(context); + } + + @Override + protected AttributeMetadata createUnmanagedAttributeMetadata(String name) { + return new AttributeMetadata(name, Integer.MAX_VALUE) { + @Override + public boolean canView(AttributeContext context) { + return UserProfileContext.USER_API.equals(context.getContext()); + } + + @Override + public boolean canEdit(AttributeContext context) { + return UserProfileContext.USER_API.equals(context.getContext()); + } + }; + } + + @Override + protected boolean isAllowUnmanagedAttribute() { + return UserProfileContext.USER_API.equals(context); + } + + @Override + protected void setUserName(Map> newAttributes, List values) { + // can not update username for service accounts + } +} 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 26419e5aa7..79b6f4b7e0 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 @@ -132,6 +132,7 @@ public class ServiceAccountUserProfileTest extends AbstractKeycloakTest { UserRepresentation representation = serviceAccount.toRepresentation(); String username = representation.getUsername(); + assertNotNull(username); assertNull(representation.getEmail()); serviceAccount.update(representation); @@ -158,6 +159,10 @@ public class ServiceAccountUserProfileTest extends AbstractKeycloakTest { representation = serviceAccount.toRepresentation(); assertFalse(representation.getAttributes().isEmpty()); assertEquals("attr-1-value", representation.getAttributes().get("attr-1").get(0)); + + Map> unmanagedAttributes = test.users().get(userId).getUnmanagedAttributes(); + + assertEquals(1, unmanagedAttributes.size()); } @Test