From cf3f05a2592e6e798384b777992ed61789cd6115 Mon Sep 17 00:00:00 2001 From: Lex Cao Date: Sat, 27 Jan 2024 01:08:47 +0800 Subject: [PATCH] Skip grant role if exists for federated storage (#26508) Closes #26507 Signed-off-by: Lex Cao --- .../AbstractUserAdapterFederatedStorage.java | 1 + .../broker/AbstractFirstBrokerLoginTest.java | 43 +++++++++++++++++++ .../federation/storage/UserStorageTest.java | 16 +++++++ 3 files changed, 60 insertions(+) diff --git a/model/storage/src/main/java/org/keycloak/storage/adapter/AbstractUserAdapterFederatedStorage.java b/model/storage/src/main/java/org/keycloak/storage/adapter/AbstractUserAdapterFederatedStorage.java index 724444e022..9606626b77 100644 --- a/model/storage/src/main/java/org/keycloak/storage/adapter/AbstractUserAdapterFederatedStorage.java +++ b/model/storage/src/main/java/org/keycloak/storage/adapter/AbstractUserAdapterFederatedStorage.java @@ -177,6 +177,7 @@ public abstract class AbstractUserAdapterFederatedStorage extends UserModelDefau @Override public void grantRole(RoleModel role) { + if (hasDirectRole(role)) return; getFederatedStorage().grantRole(realm, this.getId(), role); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java index 3d2b093514..eb7486bb4e 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java @@ -15,18 +15,22 @@ import org.keycloak.admin.client.resource.IdentityProviderResource; import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.admin.client.resource.UserResource; import org.keycloak.broker.provider.HardcodedUserSessionAttributeMapper; +import org.keycloak.common.util.MultivaluedHashMap; import org.keycloak.events.Details; import org.keycloak.events.EventType; import org.keycloak.models.IdentityProviderMapperModel; import org.keycloak.models.IdentityProviderSyncMode; import org.keycloak.models.UserModel; +import org.keycloak.representations.idm.ComponentRepresentation; import org.keycloak.representations.idm.EventRepresentation; import org.keycloak.representations.idm.IdentityProviderMapperRepresentation; import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.storage.UserStorageProvider; import org.keycloak.testsuite.Assert; import org.keycloak.testsuite.AssertEvents; +import org.keycloak.testsuite.federation.UserMapStorageFactory; import org.keycloak.testsuite.forms.VerifyProfileTest; import org.keycloak.testsuite.pages.LoginPasswordUpdatePage; import org.keycloak.testsuite.util.AccountHelper; @@ -44,8 +48,11 @@ import org.openqa.selenium.support.PageFactory; import static org.junit.Assert.assertEquals; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertTrue; +import static org.keycloak.storage.UserStorageProviderModel.IMPORT_ENABLED; +import static org.keycloak.testsuite.admin.ApiUtil.removeUserByUsername; import static org.keycloak.testsuite.broker.BrokerRunOnServerUtil.assertHardCodedSessionNote; import static org.keycloak.testsuite.broker.BrokerRunOnServerUtil.configureAutoLinkFlow; +import static org.keycloak.testsuite.broker.BrokerRunOnServerUtil.grantReadTokenRole; import static org.keycloak.testsuite.broker.BrokerTestConstants.USER_EMAIL; import static org.keycloak.testsuite.broker.BrokerTestTools.getConsumerRoot; import static org.keycloak.testsuite.broker.BrokerTestTools.waitForPage; @@ -1350,6 +1357,42 @@ public abstract class AbstractFirstBrokerLoginTest extends AbstractInitializedBa assertNumFederatedIdentities(realm.users().search(bc.getUserLogin()).get(0).getId(), 1); } + /* + * test linking the user with an existing read-token role from the federation provider + * when AddReadTokenRoleOnCreate was enabled for the IdP. + */ + @Test + public void testDuplicatedGrantReadTokenRoleWithUserFederationProvider() { + try { + // setup federation provider + ComponentRepresentation component = new ComponentRepresentation(); + component.setName("memory"); + component.setProviderId(UserMapStorageFactory.PROVIDER_ID); + component.setProviderType(UserStorageProvider.class.getName()); + component.setConfig(new MultivaluedHashMap<>()); + component.getConfig().putSingle("priority", Integer.toString(0)); + component.getConfig().putSingle(IMPORT_ENABLED, Boolean.toString(false)); + adminClient.realm(bc.consumerRealmName()).components().add(component); + + // grant read-token role first + String username = bc.getUserLogin(); + String createdId = createUser(username); + testingClient.server(bc.consumerRealmName()).run(grantReadTokenRole(username)); + + // enable read token role on create + IdentityProviderRepresentation idpRep = identityProviderResource.toRepresentation(); + idpRep.setAddReadTokenRoleOnCreate(true); + identityProviderResource.update(idpRep); + + // auto link when first broker login flow + testingClient.server(bc.consumerRealmName()).run(configureAutoLinkFlow(bc.getIDPAlias())); + logInAsUserInIDP(); + assertNumFederatedIdentities(createdId, 1); + } finally { + removeUserByUsername(adminClient.realm(bc.consumerRealmName()), bc.getUserLogin()); + } + } + private Runnable toggleRegistrationAllowed(String realmName, boolean registrationAllowed) { RealmResource consumerRealm = adminClient.realm(realmName); RealmRepresentation realmRepresentation = consumerRealm.toRepresentation(); 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 09f3f2d95b..c2bef3ba2f 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 @@ -1104,6 +1104,22 @@ public class UserStorageTest extends AbstractAuthTest { Assert.assertTrue(ObjectUtil.isEqualOrBothNull(otpCredential.getPriority(), otpCredentialLoaded.getPriority())); } + @Test + public void testGrantRoleTwice() { + RoleRepresentation role = new RoleRepresentation(); + role.setName("role"); + testRealmResource().roles().create(role); + + testingClient.server().run(session -> { + RealmModel realm = session.realms().getRealmByName("test"); + UserModel user = session.users().getUserByUsername(realm, "thor"); + + RoleModel roleModel = session.roles().getRealmRole(realm, "role"); + user.grantRole(roleModel); + user.grantRole(roleModel); + }); + } + private void assertOrder(List creds, String... expectedIds) { org.keycloak.testsuite.Assert.assertEquals(expectedIds.length, creds.size());