From eeae50fb43390172db1b15dc609d8cdf3f0041a5 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Fri, 26 Jul 2024 10:59:02 -0300 Subject: [PATCH] Make sure federationLink always map to the storage provider associated with federated users Closes #31670 Signed-off-by: Pedro Igor --- .../representations/idm/UserRepresentation.java | 13 ++++++++++--- docs/documentation/release_notes/topics/26_0_0.adoc | 6 ++++++ js/apps/admin-ui/src/user/FederatedUserLink.tsx | 4 ++-- js/apps/admin-ui/src/user/UserCredentials.tsx | 2 +- js/apps/admin-ui/src/user/UserForm.tsx | 2 +- .../src/defs/userRepresentation.ts | 1 - .../models/cache/infinispan/UserCacheSession.java | 10 ++++------ .../keycloak/credential/UserCredentialManager.java | 10 +++++----- .../storage/adapter/AbstractUserAdapter.java | 2 +- .../AbstractUserAdapterFederatedStorage.java | 2 +- .../models/utils/ModelToRepresentation.java | 2 -- .../adapter/AbstractInMemoryUserAdapter.java | 4 +++- .../org/keycloak/userprofile/DefaultAttributes.java | 11 +++-------- .../main/java/org/keycloak/models/UserModel.java | 10 ++++------ .../validator/ImmutableAttributeValidator.java | 2 +- .../LDAPProvidersIntegrationNoImportTest.java | 2 +- .../federation/storage/UserStorageTest.java | 4 ++-- 17 files changed, 45 insertions(+), 42 deletions(-) diff --git a/core/src/main/java/org/keycloak/representations/idm/UserRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/UserRepresentation.java index a4e5ed0af9..69c1ee0613 100755 --- a/core/src/main/java/org/keycloak/representations/idm/UserRepresentation.java +++ b/core/src/main/java/org/keycloak/representations/idm/UserRepresentation.java @@ -67,7 +67,6 @@ public class UserRepresentation extends AbstractUserRepresentation{ this.setUserProfileMetadata(rep.getUserProfileMetadata()); this.self = rep.getSelf(); - this.origin = rep.getOrigin(); this.createdTimestamp = rep.getCreatedTimestamp(); this.enabled = rep.isEnabled(); this.totp = rep.isTotp(); @@ -220,13 +219,21 @@ public class UserRepresentation extends AbstractUserRepresentation{ * Returns id of UserStorageProvider that loaded this user * * @return NULL if user stored locally + * @deprecated Use {@link #getFederationLink()} instead */ + @Deprecated public String getOrigin() { - return origin; + return federationLink; } + /** + * + * @param origin the origin + * @deprecated Use {@link #setFederationLink(String)} instead + */ + @Deprecated public void setOrigin(String origin) { - this.origin = origin; + // deprecated } public Set getDisableableCredentialTypes() { diff --git a/docs/documentation/release_notes/topics/26_0_0.adoc b/docs/documentation/release_notes/topics/26_0_0.adoc index de97964adf..b4d0fa55f2 100644 --- a/docs/documentation/release_notes/topics/26_0_0.adoc +++ b/docs/documentation/release_notes/topics/26_0_0.adoc @@ -129,3 +129,9 @@ The `_LEGACY` cookies also served another purpose, which was to allow login from not recommended at all in production deployments of Keycloak, it is fairly frequent to access Keycloak over `http` outside of `localhost`. As an alternative to the `_LEGACY` cookies Keycloak now doesn't set the `secure` flag and sets `SameSite=Lax` instead of `SameSite=None` when it detects an insecure context is used. + += Property `origin` in the `UserRepresentation` is deprecated + +The `origin` property in the `UserRepresentation` is deprecated and planned to be removed in future releases. + +Instead, prefer using the `federationLink` property to obtain the provider to which a user is linked with. diff --git a/js/apps/admin-ui/src/user/FederatedUserLink.tsx b/js/apps/admin-ui/src/user/FederatedUserLink.tsx index a3c1541283..0af4743e5c 100644 --- a/js/apps/admin-ui/src/user/FederatedUserLink.tsx +++ b/js/apps/admin-ui/src/user/FederatedUserLink.tsx @@ -25,10 +25,10 @@ export const FederatedUserLink = ({ user }: FederatedUserLinkProps) => { () => access.hasAccess("view-realm") ? adminClient.components.findOne({ - id: (user.federationLink || user.origin)!, + id: user.federationLink!, }) : adminClient.userStorageProvider.name({ - id: (user.federationLink || user.origin)!, + id: user.federationLink!, }), setComponent, [], diff --git a/js/apps/admin-ui/src/user/UserCredentials.tsx b/js/apps/admin-ui/src/user/UserCredentials.tsx index ce84afd838..66c0ec84eb 100644 --- a/js/apps/admin-ui/src/user/UserCredentials.tsx +++ b/js/apps/admin-ui/src/user/UserCredentials.tsx @@ -354,7 +354,7 @@ export const UserCredentials = ({ user, setUser }: UserCredentialsProps) => { toggleDeleteDialog(); }; - const useFederatedCredentials = user.federationLink || user.origin; + const useFederatedCredentials = user.federationLink; const [credentialTypes, setCredentialTypes] = useState([]); useFetch( diff --git a/js/apps/admin-ui/src/user/UserForm.tsx b/js/apps/admin-ui/src/user/UserForm.tsx index 6bd73e1d54..94733c09e3 100644 --- a/js/apps/admin-ui/src/user/UserForm.tsx +++ b/js/apps/admin-ui/src/user/UserForm.tsx @@ -208,7 +208,7 @@ export const UserForm = ({ label="requiredUserActions" help="requiredUserActionsHelp" /> - {(user?.federationLink || user?.origin) && canViewFederationLink && ( + {user?.federationLink && canViewFederationLink && ( toValidate = new LinkedList<>(inputs); - String providerId = StorageId.isLocalStorage(user.getId()) ? user.getFederationLink() : StorageId.providerId(user.getId()); + String providerId = user.getFederationLink(); if (providerId != null) { UserStorageProviderModel model = getStorageProviderModel(realm, providerId); if (model == null || !model.isEnabled()) return false; @@ -80,8 +80,8 @@ public class UserCredentialManager extends AbstractStorageManager getDisableableCredentialTypesStream() { Stream types = Stream.empty(); - String providerId = StorageId.isLocalStorage(user) ? user.getFederationLink() : StorageId.resolveProviderId(user); + String providerId = user.getFederationLink(); if (providerId != null) { UserStorageProviderModel model = getStorageProviderModel(realm, providerId); if (model == null || !model.isEnabled()) return types; @@ -231,7 +231,7 @@ public class UserCredentialManager extends AbstractStorageManager> implements } String providerId = user.getFederationLink(); - - if (providerId == null) { - providerId = StorageId.providerId(user.getId()); - } - UserProvider userProvider = session.users(); if (userProvider instanceof UserProfileDecorator) { @@ -460,7 +455,7 @@ public class DefaultAttributes extends HashMap> implements Stream valuesStream = Optional.ofNullable(values).orElse(EMPTY_VALUE).stream().filter(Objects::nonNull); // do not normalize the username if a federated user because we need to respect the format from the external identity store - if ((UserModel.USERNAME.equals(name) && isLocalUser()) || UserModel.EMAIL.equals(name)) { + if ((UserModel.USERNAME.equals(name) && !isFederated()) || UserModel.EMAIL.equals(name)) { valuesStream = valuesStream.map(KeycloakModelUtils::toLowerCaseSafe); } @@ -571,7 +566,7 @@ public class DefaultAttributes extends HashMap> implements }; } - private boolean isLocalUser() { - return user == null || user.isLocal(); + private boolean isFederated() { + return user != null && user.isFederated(); } } diff --git a/server-spi/src/main/java/org/keycloak/models/UserModel.java b/server-spi/src/main/java/org/keycloak/models/UserModel.java index fb0fcc5681..211e06f884 100755 --- a/server-spi/src/main/java/org/keycloak/models/UserModel.java +++ b/server-spi/src/main/java/org/keycloak/models/UserModel.java @@ -17,11 +17,9 @@ package org.keycloak.models; -import static org.keycloak.utils.StringUtil.isBlank; +import static org.keycloak.utils.StringUtil.isNotBlank; import org.keycloak.provider.ProviderEvent; -import org.keycloak.storage.StorageId; -import org.keycloak.utils.StringUtil; import java.util.Comparator; import java.util.List; @@ -221,10 +219,10 @@ public interface UserModel extends RoleMapperModel { * Indicates if this {@link UserModel} maps to a local account or an account * federated from an external user storage. * - * @return {@code true} if a local account. Otherwise, {@code false}. + * @return {@code true} if a federated account. Otherwise, {@code false}. */ - default boolean isLocal() { - return isBlank(getFederationLink()) && StorageId.isLocalStorage(getId()); + default boolean isFederated() { + return isNotBlank(getFederationLink()); } /** diff --git a/services/src/main/java/org/keycloak/userprofile/validator/ImmutableAttributeValidator.java b/services/src/main/java/org/keycloak/userprofile/validator/ImmutableAttributeValidator.java index 7a61218fc7..fb5cb1ec57 100644 --- a/services/src/main/java/org/keycloak/userprofile/validator/ImmutableAttributeValidator.java +++ b/services/src/main/java/org/keycloak/userprofile/validator/ImmutableAttributeValidator.java @@ -62,7 +62,7 @@ public class ImmutableAttributeValidator implements SimpleValidator { Stream rawValues = user.getAttributeStream(inputHint).filter(Objects::nonNull); // force usernames to lower-case to avoid validation errors if the external storage is using a different format - if (user.isLocal() && UserModel.USERNAME.equals(inputHint)) { + if (!user.isFederated() && UserModel.USERNAME.equals(inputHint)) { rawValues = rawValues.map(String::toLowerCase); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java index 0ddcd67f7e..874c558353 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java @@ -84,7 +84,7 @@ public class LDAPProvidersIntegrationNoImportTest extends LDAPProvidersIntegrati // TODO: It should be possibly LDAP_ID (LDAP UUID) used as an externalId inside storageId... Assert.assertEquals(storageId.getExternalId(), user.getUsername()); - Assert.assertNull(user.getFederationLink()); + Assert.assertNotNull(user.getFederationLink()); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java index 967c3e01cd..2828334617 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java @@ -421,8 +421,8 @@ public class UserStorageTest extends AbstractAuthTest { memuser = user(uid).toRepresentation(); assertNotNull(memuser); - assertNotNull(memuser.getOrigin()); - ComponentRepresentation origin = testRealmResource().components().component(memuser.getOrigin()).toRepresentation(); + assertNotNull(memuser.getFederationLink()); + ComponentRepresentation origin = testRealmResource().components().component(memuser.getFederationLink()).toRepresentation(); Assert.assertEquals("memory", origin.getName()); testRealmResource().users().get(memuser.getId()).remove();