Make sure federationLink always map to the storage provider associated with federated users

Closes #31670

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
This commit is contained in:
Pedro Igor 2024-07-26 10:59:02 -03:00 committed by Alexander Schwartz
parent f9d3895550
commit eeae50fb43
17 changed files with 45 additions and 42 deletions

View file

@ -67,7 +67,6 @@ public class UserRepresentation extends AbstractUserRepresentation{
this.setUserProfileMetadata(rep.getUserProfileMetadata()); this.setUserProfileMetadata(rep.getUserProfileMetadata());
this.self = rep.getSelf(); this.self = rep.getSelf();
this.origin = rep.getOrigin();
this.createdTimestamp = rep.getCreatedTimestamp(); this.createdTimestamp = rep.getCreatedTimestamp();
this.enabled = rep.isEnabled(); this.enabled = rep.isEnabled();
this.totp = rep.isTotp(); this.totp = rep.isTotp();
@ -220,13 +219,21 @@ public class UserRepresentation extends AbstractUserRepresentation{
* Returns id of UserStorageProvider that loaded this user * Returns id of UserStorageProvider that loaded this user
* *
* @return NULL if user stored locally * @return NULL if user stored locally
* @deprecated Use {@link #getFederationLink()} instead
*/ */
@Deprecated
public String getOrigin() { public String getOrigin() {
return origin; return federationLink;
} }
/**
*
* @param origin the origin
* @deprecated Use {@link #setFederationLink(String)} instead
*/
@Deprecated
public void setOrigin(String origin) { public void setOrigin(String origin) {
this.origin = origin; // deprecated
} }
public Set<String> getDisableableCredentialTypes() { public Set<String> getDisableableCredentialTypes() {

View file

@ -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 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` 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. 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.

View file

@ -25,10 +25,10 @@ export const FederatedUserLink = ({ user }: FederatedUserLinkProps) => {
() => () =>
access.hasAccess("view-realm") access.hasAccess("view-realm")
? adminClient.components.findOne({ ? adminClient.components.findOne({
id: (user.federationLink || user.origin)!, id: user.federationLink!,
}) })
: adminClient.userStorageProvider.name({ : adminClient.userStorageProvider.name({
id: (user.federationLink || user.origin)!, id: user.federationLink!,
}), }),
setComponent, setComponent,
[], [],

View file

@ -354,7 +354,7 @@ export const UserCredentials = ({ user, setUser }: UserCredentialsProps) => {
toggleDeleteDialog(); toggleDeleteDialog();
}; };
const useFederatedCredentials = user.federationLink || user.origin; const useFederatedCredentials = user.federationLink;
const [credentialTypes, setCredentialTypes] = useState<string[]>([]); const [credentialTypes, setCredentialTypes] = useState<string[]>([]);
useFetch( useFetch(

View file

@ -208,7 +208,7 @@ export const UserForm = ({
label="requiredUserActions" label="requiredUserActions"
help="requiredUserActionsHelp" help="requiredUserActionsHelp"
/> />
{(user?.federationLink || user?.origin) && canViewFederationLink && ( {user?.federationLink && canViewFederationLink && (
<FormGroup <FormGroup
label={t("federationLink")} label={t("federationLink")}
labelIcon={ labelIcon={

View file

@ -27,7 +27,6 @@ export default interface UserRepresentation {
firstName?: string; firstName?: string;
groups?: string[]; groups?: string[];
lastName?: string; lastName?: string;
origin?: string;
realmRoles?: string[]; realmRoles?: string[];
self?: string; self?: string;
serviceAccountClientId?: string; serviceAccountClientId?: string;

View file

@ -350,13 +350,11 @@ public class UserCacheSession implements UserCache, OnCreateComponent, OnUpdateC
} }
} }
StorageId storageId = delegate.getFederationLink() != null ? CachedUser cached;
new StorageId(delegate.getFederationLink(), delegate.getId()) : new StorageId(delegate.getId()); UserAdapter adapter;
CachedUser cached = null;
UserAdapter adapter = null;
if (!storageId.isLocal()) { if (delegate.getFederationLink() != null) {
ComponentModel component = realm.getComponent(storageId.getProviderId()); ComponentModel component = realm.getComponent(delegate.getFederationLink());
UserStorageProviderModel model = new UserStorageProviderModel(component); UserStorageProviderModel model = new UserStorageProviderModel(component);
if (!model.isEnabled()) { if (!model.isEnabled()) {
return new ReadOnlyUserModelDelegate(delegate) { return new ReadOnlyUserModelDelegate(delegate) {

View file

@ -61,7 +61,7 @@ public class UserCredentialManager extends AbstractStorageManager<UserStoragePro
List<CredentialInput> toValidate = new LinkedList<>(inputs); List<CredentialInput> toValidate = new LinkedList<>(inputs);
String providerId = StorageId.isLocalStorage(user.getId()) ? user.getFederationLink() : StorageId.providerId(user.getId()); String providerId = user.getFederationLink();
if (providerId != null) { if (providerId != null) {
UserStorageProviderModel model = getStorageProviderModel(realm, providerId); UserStorageProviderModel model = getStorageProviderModel(realm, providerId);
if (model == null || !model.isEnabled()) return false; if (model == null || !model.isEnabled()) return false;
@ -80,8 +80,8 @@ public class UserCredentialManager extends AbstractStorageManager<UserStoragePro
@Override @Override
public boolean updateCredential(CredentialInput input) { public boolean updateCredential(CredentialInput input) {
String providerId = StorageId.isLocalStorage(user.getId()) ? user.getFederationLink() : StorageId.providerId(user.getId());
if (!StorageId.isLocalStorage(user.getId())) throwExceptionIfInvalidUser(user); if (!StorageId.isLocalStorage(user.getId())) throwExceptionIfInvalidUser(user);
String providerId = user.getFederationLink();
if (providerId != null) { if (providerId != null) {
UserStorageProviderModel model = getStorageProviderModel(realm, providerId); UserStorageProviderModel model = getStorageProviderModel(realm, providerId);
@ -152,8 +152,8 @@ public class UserCredentialManager extends AbstractStorageManager<UserStoragePro
@Override @Override
public void disableCredentialType(String credentialType) { public void disableCredentialType(String credentialType) {
String providerId = StorageId.isLocalStorage(user.getId()) ? user.getFederationLink() : StorageId.providerId(user.getId());
if (!StorageId.isLocalStorage(user.getId())) throwExceptionIfInvalidUser(user); if (!StorageId.isLocalStorage(user.getId())) throwExceptionIfInvalidUser(user);
String providerId = user.getFederationLink();
if (providerId != null) { if (providerId != null) {
UserStorageProviderModel model = getStorageProviderModel(realm, providerId); UserStorageProviderModel model = getStorageProviderModel(realm, providerId);
if (model == null || !model.isEnabled()) return; if (model == null || !model.isEnabled()) return;
@ -172,7 +172,7 @@ public class UserCredentialManager extends AbstractStorageManager<UserStoragePro
@Override @Override
public Stream<String> getDisableableCredentialTypesStream() { public Stream<String> getDisableableCredentialTypesStream() {
Stream<String> types = Stream.empty(); Stream<String> types = Stream.empty();
String providerId = StorageId.isLocalStorage(user) ? user.getFederationLink() : StorageId.resolveProviderId(user); String providerId = user.getFederationLink();
if (providerId != null) { if (providerId != null) {
UserStorageProviderModel model = getStorageProviderModel(realm, providerId); UserStorageProviderModel model = getStorageProviderModel(realm, providerId);
if (model == null || !model.isEnabled()) return types; if (model == null || !model.isEnabled()) return types;
@ -231,7 +231,7 @@ public class UserCredentialManager extends AbstractStorageManager<UserStoragePro
} }
private UserStorageCredentialConfigured isConfiguredThroughUserStorage(RealmModel realm, UserModel user, String type) { private UserStorageCredentialConfigured isConfiguredThroughUserStorage(RealmModel realm, UserModel user, String type) {
String providerId = StorageId.isLocalStorage(user) ? user.getFederationLink() : StorageId.resolveProviderId(user); String providerId = user.getFederationLink();
if (providerId != null) { if (providerId != null) {
UserStorageProviderModel model = getStorageProviderModel(realm, providerId); UserStorageProviderModel model = getStorageProviderModel(realm, providerId);
if (model == null || !model.isEnabled()) return UserStorageCredentialConfigured.USER_STORAGE_DISABLED; if (model == null || !model.isEnabled()) return UserStorageCredentialConfigured.USER_STORAGE_DISABLED;

View file

@ -240,7 +240,7 @@ public abstract class AbstractUserAdapter extends UserModelDefaultMethods {
*/ */
@Override @Override
public String getFederationLink() { public String getFederationLink() {
return null; return StorageId.providerId(getId());
} }
/** /**

View file

@ -247,7 +247,7 @@ public abstract class AbstractUserAdapterFederatedStorage extends UserModelDefau
*/ */
@Override @Override
public String getFederationLink() { public String getFederationLink() {
return null; return StorageId.providerId(getId());
} }
/** /**

View file

@ -220,8 +220,6 @@ public class ModelToRepresentation {
public static UserRepresentation toRepresentation(KeycloakSession session, RealmModel realm, UserModel user) { public static UserRepresentation toRepresentation(KeycloakSession session, RealmModel realm, UserModel user) {
UserRepresentation rep = new UserRepresentation(); UserRepresentation rep = new UserRepresentation();
rep.setId(user.getId()); rep.setId(user.getId());
String providerId = StorageId.resolveProviderId(user);
rep.setOrigin(providerId);
rep.setUsername(user.getUsername()); rep.setUsername(user.getUsername());
rep.setCreatedTimestamp(user.getCreatedTimestamp()); rep.setCreatedTimestamp(user.getCreatedTimestamp());
rep.setLastName(user.getLastName()); rep.setLastName(user.getLastName());

View file

@ -28,10 +28,12 @@ import org.keycloak.models.UserModelDefaultMethods;
import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.KeycloakModelUtils;
import org.keycloak.models.utils.RoleUtils; import org.keycloak.models.utils.RoleUtils;
import org.keycloak.storage.ReadOnlyException; import org.keycloak.storage.ReadOnlyException;
import org.keycloak.storage.StorageId;
import java.util.HashSet; import java.util.HashSet;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Optional;
import java.util.Set; import java.util.Set;
import java.util.stream.Stream; import java.util.stream.Stream;
@ -235,7 +237,7 @@ public abstract class AbstractInMemoryUserAdapter extends UserModelDefaultMethod
@Override @Override
public String getFederationLink() { public String getFederationLink() {
return federationLink; return Optional.ofNullable(federationLink).orElse(StorageId.providerId(getId()));
} }
@Override @Override

View file

@ -333,11 +333,6 @@ public class DefaultAttributes extends HashMap<String, List<String>> implements
} }
String providerId = user.getFederationLink(); String providerId = user.getFederationLink();
if (providerId == null) {
providerId = StorageId.providerId(user.getId());
}
UserProvider userProvider = session.users(); UserProvider userProvider = session.users();
if (userProvider instanceof UserProfileDecorator) { if (userProvider instanceof UserProfileDecorator) {
@ -460,7 +455,7 @@ public class DefaultAttributes extends HashMap<String, List<String>> implements
Stream<String> valuesStream = Optional.ofNullable(values).orElse(EMPTY_VALUE).stream().filter(Objects::nonNull); Stream<String> 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 // 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); valuesStream = valuesStream.map(KeycloakModelUtils::toLowerCaseSafe);
} }
@ -571,7 +566,7 @@ public class DefaultAttributes extends HashMap<String, List<String>> implements
}; };
} }
private boolean isLocalUser() { private boolean isFederated() {
return user == null || user.isLocal(); return user != null && user.isFederated();
} }
} }

View file

@ -17,11 +17,9 @@
package org.keycloak.models; 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.provider.ProviderEvent;
import org.keycloak.storage.StorageId;
import org.keycloak.utils.StringUtil;
import java.util.Comparator; import java.util.Comparator;
import java.util.List; 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 * Indicates if this {@link UserModel} maps to a local account or an account
* federated from an external user storage. * 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() { default boolean isFederated() {
return isBlank(getFederationLink()) && StorageId.isLocalStorage(getId()); return isNotBlank(getFederationLink());
} }
/** /**

View file

@ -62,7 +62,7 @@ public class ImmutableAttributeValidator implements SimpleValidator {
Stream<String> rawValues = user.getAttributeStream(inputHint).filter(Objects::nonNull); Stream<String> 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 // 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); rawValues = rawValues.map(String::toLowerCase);
} }

View file

@ -84,7 +84,7 @@ public class LDAPProvidersIntegrationNoImportTest extends LDAPProvidersIntegrati
// TODO: It should be possibly LDAP_ID (LDAP UUID) used as an externalId inside storageId... // TODO: It should be possibly LDAP_ID (LDAP UUID) used as an externalId inside storageId...
Assert.assertEquals(storageId.getExternalId(), user.getUsername()); Assert.assertEquals(storageId.getExternalId(), user.getUsername());
Assert.assertNull(user.getFederationLink()); Assert.assertNotNull(user.getFederationLink());
} }

View file

@ -421,8 +421,8 @@ public class UserStorageTest extends AbstractAuthTest {
memuser = user(uid).toRepresentation(); memuser = user(uid).toRepresentation();
assertNotNull(memuser); assertNotNull(memuser);
assertNotNull(memuser.getOrigin()); assertNotNull(memuser.getFederationLink());
ComponentRepresentation origin = testRealmResource().components().component(memuser.getOrigin()).toRepresentation(); ComponentRepresentation origin = testRealmResource().components().component(memuser.getFederationLink()).toRepresentation();
Assert.assertEquals("memory", origin.getName()); Assert.assertEquals("memory", origin.getName());
testRealmResource().users().get(memuser.getId()).remove(); testRealmResource().users().get(memuser.getId()).remove();