Repository navigation
Migrate Standard token exchange tests - #47516
Conversation
4905afc to
eccb10e
Compare
There was a problem hiding this comment.
@graziang Thanks for the changes and for migrating the test!
Few points:
-
Are new token-exchange executed with GH actions during new testsuite run? I did not check, just asking :-)
-
IMO it is OK to keep the
StandardTokenExchangeV2Testalso in the new testsuite as you pointed, but maybe it would be good to at least make sure that it is not executed within GH actions in the old testsuite? As otherwise, we would have same token-exchange test scenarios tested multiple times. Maybe it would be OK to updateStandardTokenExchangeV2Testin the old testsuite to theabstractclass. AFAIK this would mean that it will not be executed (would be good to doublecheck in GH actions). This may include some javadoc comment specifying on why it is abstract and why the tests are not executed. Also it can be good to deprecate that class in the old testsuite and mention in the javadoc that new tests should not be put to this class, but rather to the classes in the new testsuite. Finally, I wonder whether to remove some (or most) of tests from that class and keep just some "smoke" tests - especially since token-exchange V1 is deprecated now. But if you prefer to rather not do that in this PR, it is OK IMO. -
I will ask also "testsuite wg" team to review the PR changes in the test-framework
| public void update(ManagedClient.ClientUpdate... updates) { | ||
| ClientRepresentation rep = admin().toRepresentation(); | ||
|
|
||
| ClientConfigBuilder configBuilder = ClientConfigBuilder.update(rep); | ||
| for (ManagedClient.ClientUpdate update : updates) { | ||
| configBuilder = update.update(configBuilder); | ||
| } | ||
|
|
||
| ClientRepresentation updated = configBuilder.build(); | ||
| admin().update(updated); | ||
| } | ||
|
|
There was a problem hiding this comment.
this changes is not required, please use updateWithCleanup
| public void update(ManagedClient.ClientUpdate... updates) { | |
| ClientRepresentation rep = admin().toRepresentation(); | |
| ClientConfigBuilder configBuilder = ClientConfigBuilder.update(rep); | |
| for (ManagedClient.ClientUpdate update : updates) { | |
| configBuilder = update.update(configBuilder); | |
| } | |
| ClientRepresentation updated = configBuilder.build(); | |
| admin().update(updated); | |
| } |
| UserRepresentation john; | ||
|
|
||
| UserRepresentation mike; | ||
|
|
||
| UserRepresentation alice; | ||
|
|
||
|
|
||
| @BeforeEach | ||
| public void setup() { | ||
| john = AdminApiUtil.findUserByUsernameId(realm.admin(), "john").toRepresentation(); | ||
| mike = AdminApiUtil.findUserByUsernameId(realm.admin(), "mike").toRepresentation(); | ||
| alice = AdminApiUtil.findUserByUsernameId(realm.admin(), "alice").toRepresentation(); | ||
|
|
||
| // Assign roles to service account user | ||
| UserRepresentation serviceAccount = subjectClient.admin().getServiceAccountUser(); | ||
|
|
||
| // Assign realm role default-roles-test | ||
| RoleRepresentation defaultRolesTest = realm.admin().roles().get("default-roles-test").toRepresentation(); | ||
| realm.admin().users().get(serviceAccount.getId()).roles().realmLevel().add(List.of(defaultRolesTest)); | ||
|
|
||
| // Assign client role target-client1-role | ||
| String targetClient1Id = realm.admin().clients().findByClientId("target-client1").get(0).getId(); | ||
| RoleRepresentation targetClient1Role = realm.admin().clients().get(targetClient1Id).roles().get("target-client1-role").toRepresentation(); | ||
| realm.admin().users().get(serviceAccount.getId()).roles().clientLevel(targetClient1Id).add(List.of(targetClient1Role)); | ||
| enableEvents(realm); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please, remove these changes and use particular ManagedObjects. I suggest to have UserSupplier with attachTo option as ClientSupplier has already, but it needs to be implemented first, feel free to do it.
Please check: https://github.com/keycloak/keycloak/blob/main/test-framework/core/src/main/java/org/keycloak/testframework/realm/ClientSupplier.java#L31
| UserRepresentation john; | |
| UserRepresentation mike; | |
| UserRepresentation alice; | |
| @BeforeEach | |
| public void setup() { | |
| john = AdminApiUtil.findUserByUsernameId(realm.admin(), "john").toRepresentation(); | |
| mike = AdminApiUtil.findUserByUsernameId(realm.admin(), "mike").toRepresentation(); | |
| alice = AdminApiUtil.findUserByUsernameId(realm.admin(), "alice").toRepresentation(); | |
| // Assign roles to service account user | |
| UserRepresentation serviceAccount = subjectClient.admin().getServiceAccountUser(); | |
| // Assign realm role default-roles-test | |
| RoleRepresentation defaultRolesTest = realm.admin().roles().get("default-roles-test").toRepresentation(); | |
| realm.admin().users().get(serviceAccount.getId()).roles().realmLevel().add(List.of(defaultRolesTest)); | |
| // Assign client role target-client1-role | |
| String targetClient1Id = realm.admin().clients().findByClientId("target-client1").get(0).getId(); | |
| RoleRepresentation targetClient1Role = realm.admin().clients().get(targetClient1Id).roles().get("target-client1-role").toRepresentation(); | |
| realm.admin().users().get(serviceAccount.getId()).roles().clientLevel(targetClient1Id).add(List.of(targetClient1Role)); | |
| enableEvents(realm); | |
| } |
There was a problem hiding this comment.
done and create added attachTo to UserSupplier
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| public class BaseAbstractTokenExchangeTest { |
There was a problem hiding this comment.
| public class BaseAbstractTokenExchangeTest { | |
| public class AbstractBaseTokenExchangeTest { |
| @InjectPage | ||
| ConsentPage consentPage; | ||
|
|
||
| @InjectRealm(config = TokenExchangeRealm.class) |
There was a problem hiding this comment.
move this realm field, right after the class name, it must be first in a row, hence it will create the realm two times, once calling InjectEvents and second when recreating InjectRealm
| private void enableEvents(ManagedRealm realm) { | ||
| RealmEventsConfigRepresentation realmEventsConfig = realm.admin().getRealmEventsConfig(); | ||
| List<String> enabledEventTypes = realmEventsConfig.getEnabledEventTypes(); | ||
| if (!enabledEventTypes.contains(EventType.REFRESH_TOKEN.name())) { | ||
| enabledEventTypes.add(EventType.REFRESH_TOKEN.name()); | ||
| enabledEventTypes.add(EventType.INTROSPECT_TOKEN.name()); | ||
| realm.admin().updateRealmEventsConfig(realmEventsConfig); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
please move it into the RealmConfig implementation directly and use realm.updateWithCleanup(...) or just realm.cleanup().add(...) if you need any change in a method
| private void enableEvents(ManagedRealm realm) { | |
| RealmEventsConfigRepresentation realmEventsConfig = realm.admin().getRealmEventsConfig(); | |
| List<String> enabledEventTypes = realmEventsConfig.getEnabledEventTypes(); | |
| if (!enabledEventTypes.contains(EventType.REFRESH_TOKEN.name())) { | |
| enabledEventTypes.add(EventType.REFRESH_TOKEN.name()); | |
| enabledEventTypes.add(EventType.INTROSPECT_TOKEN.name()); | |
| realm.admin().updateRealmEventsConfig(realmEventsConfig); | |
| } | |
| } |
There was a problem hiding this comment.
this is because the realm is created with all the default events if enabledEvents is null, and I can't add specific events in addition to default ones during creation, so I need to first create realm with default events and then set additional events after the creation.
There was a problem hiding this comment.
ok, thanks for explanation
There was a problem hiding this comment.
in this case maybe TestSetup annotation would be suitable to execute it:
https://github.com/keycloak/keycloak/blob/main/test-framework/docs/WRITING_TESTS.md#setup-and-cleanup
There was a problem hiding this comment.
changed to @testsetup, setup method only set the events now
| private static final String POLICY_NAME = "MyPolicy"; | ||
|
|
||
| @Test | ||
| void testClientPolicies() throws Exception { |
There was a problem hiding this comment.
| void testClientPolicies() throws Exception { | |
| public void testClientPolicies() throws Exception { |
| @Test | ||
| void testClientPolicies() throws Exception { | ||
| // Create executor configuration | ||
| TestRaiseExceptionExecutor.Configuration executorConfig = new TestRaiseExceptionExecutor.Configuration(); |
There was a problem hiding this comment.
exception handling: https://docs.junit.org/6.0.3/writing-tests/exception-handling.html
assertThrows(() -> {...});
There was a problem hiding this comment.
Changed to RejectRequestExecutor already present in Keycloak, so removed the custom executors an conditions from the framework
| import org.keycloak.services.clientpolicy.ClientPolicyVote; | ||
| import org.keycloak.services.clientpolicy.condition.AbstractClientPolicyConditionProvider; | ||
|
|
||
| public class TestRaiseExceptionCondition extends AbstractClientPolicyConditionProvider<TestRaiseExceptionCondition.Configuration> { |
There was a problem hiding this comment.
this provider has no usage, please remove it
| import org.keycloak.services.clientpolicy.condition.ClientPolicyConditionProvider; | ||
| import org.keycloak.services.clientpolicy.condition.ClientPolicyConditionProviderFactory; | ||
|
|
||
| public class TestRaiseExceptionConditionFactory implements ClientPolicyConditionProviderFactory { |
There was a problem hiding this comment.
this factory provider has no usage, please remove it
| @@ -0,0 +1 @@ | |||
| org.keycloak.tests.providers.client.policies.condition.TestRaiseExceptionConditionFactory | |||
There was a problem hiding this comment.
this service has no usage, please remove it
| /* protected void createClientScopeForRole(RealmResource realm, String clientId, String clientRoleName, String clientScopeName) { | ||
| final ClientResource client = ApiUtil.findClientByClientId(realm, clientId); | ||
| createClientScopeForRole(realm, client, clientRoleName, clientScopeName); | ||
| } | ||
|
|
||
| protected void createClientScopeForRole(RealmResource realm, ClientResource client, String clientRoleName, String clientScopeName) { | ||
| final String clientUUID = client.toRepresentation().getId(); | ||
| final RoleRepresentation clientRole = ApiUtil.findClientRoleByName(client, clientRoleName).toRepresentation(); | ||
|
|
||
| final ClientScopeRepresentation clientScope = new ClientScopeRepresentation(); | ||
| clientScope.setName(clientScopeName); | ||
| clientScope.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL); | ||
| final String clientScopeId = ApiUtil.getCreatedId(realm.clientScopes().create(clientScope)); | ||
| getCleanup().addClientScopeId(clientScopeId); | ||
| realm.clientScopes().get(clientScopeId).getScopeMappings().clientLevel(clientUUID).add(List.of(clientRole)); | ||
| }*/ |
There was a problem hiding this comment.
please, remove unused code
| /* protected void createClientScopeForRole(RealmResource realm, String clientId, String clientRoleName, String clientScopeName) { | |
| final ClientResource client = ApiUtil.findClientByClientId(realm, clientId); | |
| createClientScopeForRole(realm, client, clientRoleName, clientScopeName); | |
| } | |
| protected void createClientScopeForRole(RealmResource realm, ClientResource client, String clientRoleName, String clientScopeName) { | |
| final String clientUUID = client.toRepresentation().getId(); | |
| final RoleRepresentation clientRole = ApiUtil.findClientRoleByName(client, clientRoleName).toRepresentation(); | |
| final ClientScopeRepresentation clientScope = new ClientScopeRepresentation(); | |
| clientScope.setName(clientScopeName); | |
| clientScope.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL); | |
| final String clientScopeId = ApiUtil.getCreatedId(realm.clientScopes().create(clientScope)); | |
| getCleanup().addClientScopeId(clientScopeId); | |
| realm.clientScopes().get(clientScopeId).getScopeMappings().clientLevel(clientUUID).add(List.of(clientRole)); | |
| }*/ |
5b959e3 to
6a704c0
Compare
|
Thanks for the review! @lhanusov I applied the suggestions, I also replied to the inline comments. @mposolda I deprectated the old class, and removed almost all the tests, I checked that the tests are executed in the new test suite and not in the old one. I added separate commits for easier review, let me know if I missed anything, Thanks! |
| } finally { | ||
| requesterClient.updateWithCleanup(client -> client | ||
| .attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
this is not needed since, the updateWithCleanup is already used on the line 145
| } finally { | |
| requesterClient.updateWithCleanup(client -> client | |
| .attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name()) | |
| ); | |
| } |
| requesterClient.updateWithCleanup(client -> client.attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name()) | ||
| ); |
There was a problem hiding this comment.
this is not needed since updateWithCleanup is already used on line 190.
| requesterClient.updateWithCleanup(client -> client.attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name()) | |
| ); |
| } finally { | ||
| requesterClient.updateWithCleanup(client -> client | ||
| .attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
not needed, cleaned already on the line 120
| } finally { | |
| requesterClient.updateWithCleanup(client -> client | |
| .attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name()) | |
| ); | |
| } |
| KeycloakUrls keycloakUrls; | ||
|
|
||
| @InjectSimpleHttp | ||
| org.keycloak.http.simple.SimpleHttp simpleHttp; |
There was a problem hiding this comment.
please add import org.keycloak.http.simple.SimpleHttp;
| org.keycloak.http.simple.SimpleHttp simpleHttp; | |
| SimpleHttp simpleHttp; |
| public UserRepresentation toRepresentation() { | ||
| return admin().toRepresentation(); | ||
| } | ||
|
|
There was a problem hiding this comment.
please, don't add unnecessary changes inside the framework
| public UserRepresentation toRepresentation() { | |
| return admin().toRepresentation(); | |
| } |
| } finally { | ||
| requesterClient.updateWithCleanup(c-> c | ||
| .attribute(OIDCConfigAttributes.USE_REFRESH_TOKEN, Boolean.TRUE.toString()) | ||
| .attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name())); | ||
| } |
There was a problem hiding this comment.
not needed, cleanup alread used on the line 101
| } finally { | |
| requesterClient.updateWithCleanup(c-> c | |
| .attribute(OIDCConfigAttributes.USE_REFRESH_TOKEN, Boolean.TRUE.toString()) | |
| .attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name())); | |
| } |
| } | ||
|
|
||
| @Test | ||
| public void testOfflineAccessNotAllowed() throws Exception { |
There was a problem hiding this comment.
| public void testOfflineAccessNotAllowed() throws Exception { | |
| public void testOfflineAccessNotAllowed() { |
| } | ||
|
|
||
| @Test | ||
| public void testOfflineAccessLoginWithRegularTokenExchange() throws Exception { |
There was a problem hiding this comment.
| public void testOfflineAccessLoginWithRegularTokenExchange() throws Exception { | |
| public void testOfflineAccessLoginWithRegularTokenExchange() { |
| } | ||
|
|
||
| @Test | ||
| public void testOfflineAccessNotAllowedAfterOfflineAccessLogin() throws Exception { |
There was a problem hiding this comment.
| public void testOfflineAccessNotAllowedAfterOfflineAccessLogin() throws Exception { | |
| public void testOfflineAccessNotAllowedAfterOfflineAccessLogin() { |
| } finally { | ||
| oauth.scope("openid"); | ||
| requesterClient.updateWithCleanup(client -> client.attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is not needed since updateWithCleanup is already used on line 190.
Is the oauth.scope("openid") required, since the default config is null and other test methods don't do any cleanup?
| } finally { | |
| oauth.scope("openid"); | |
| requesterClient.updateWithCleanup(client -> client.attribute(OIDCConfigAttributes.STANDARD_TOKEN_EXCHANGE_REFRESH_ENABLED, OIDCAdvancedConfigWrapper.TokenExchangeRefreshTokenEnabled.NO.name()) | |
| ); | |
| } |
|
Hello! What's the progress here? We might be blocked by this eventually in a short-term. Thanks! |
@mabartos this PR is also blocked by two more PRs on the test framework:
@lhanusov Do you know when we can make som progress on this? Otherwise, we'll have to find another way to migrate these tests without the improvements to the test framework. |
There was a problem hiding this comment.
please. revert these changes
There was a problem hiding this comment.
please, revert this change
| @InjectClient(ref = "subject-client", attachTo = "subject-client") | ||
| ManagedClient subjectClient; | ||
|
|
||
| @InjectClient(ref = "requester-client", attachTo = "requester-client") | ||
| ManagedClient requesterClient; | ||
|
|
||
| @InjectClient(ref = "requester-client-2", attachTo = "requester-client-2") | ||
| ManagedClient requesterClient2; | ||
|
|
||
| @InjectUser(ref = "john", attachTo = "john") | ||
| ManagedUser john; | ||
|
|
||
| @InjectUser(ref = "mike", attachTo = "mike") | ||
| ManagedUser mike; | ||
|
|
||
| @InjectUser(ref = "alice", attachTo = "alice") | ||
| ManagedUser alice; |
There was a problem hiding this comment.
instead of attachTo for clients and users, you will need to add UserConfig and ClientConfig and take it away from RealmConfig
|
@lhanusov rebased and hopefully addressed all the suggestions after our discussion. |
There was a problem hiding this comment.
thanks for updating this PR, only few changes left
| */ | ||
| public class StandardTokenExchangeV2Test extends AbstractClientPoliciesTest { | ||
| @Deprecated | ||
| public abstract class StandardTokenExchangeV2Test extends AbstractClientPoliciesTest { |
There was a problem hiding this comment.
Are there any tests left? If, no, you can remove this class. Don't worry, nobody will recreate it from scratch.
There was a problem hiding this comment.
StandardTokenExchangeV2WithLegacyTokenExchangeTest still extends StandardTokenExchangeV2Test, but it is deprecated and will be removed, the idea of removing both then.
| public static class AliceConfig implements UserConfig { | ||
| @Override | ||
| public UserBuilder configure(UserBuilder user) { | ||
| return user.username("alice") | ||
| .name("Alice", "Doe") | ||
| .email("alice@email.cz") | ||
| .password("password"); | ||
| } | ||
| } | ||
|
|
||
| public static class JohnConfig implements UserConfig { | ||
| @Override | ||
| public UserBuilder configure(UserBuilder user) { | ||
| return user.username("john") | ||
| .name("John", "Bar") | ||
| .email("john@email.cz") | ||
| .password("password"); | ||
| } | ||
| } | ||
|
|
||
| public static class MikeConfig implements UserConfig { | ||
| @Override | ||
| public UserBuilder configure(UserBuilder user) { | ||
| return user.username("mike") | ||
| .name("Mike", "Bar") | ||
| .email("mike@email.cz") | ||
| .password("password"); | ||
| } | ||
| } |
There was a problem hiding this comment.
this can be removed
| public static class AliceConfig implements UserConfig { | |
| @Override | |
| public UserBuilder configure(UserBuilder user) { | |
| return user.username("alice") | |
| .name("Alice", "Doe") | |
| .email("alice@email.cz") | |
| .password("password"); | |
| } | |
| } | |
| public static class JohnConfig implements UserConfig { | |
| @Override | |
| public UserBuilder configure(UserBuilder user) { | |
| return user.username("john") | |
| .name("John", "Bar") | |
| .email("john@email.cz") | |
| .password("password"); | |
| } | |
| } | |
| public static class MikeConfig implements UserConfig { | |
| @Override | |
| public UserBuilder configure(UserBuilder user) { | |
| return user.username("mike") | |
| .name("Mike", "Bar") | |
| .email("mike@email.cz") | |
| .password("password"); | |
| } | |
| } |
| assertUserInfoError(exchangedTokenString, "requester-client", "secret", "invalid_token", Errors.USER_SESSION_NOT_FOUND); | ||
|
|
||
| realm.admin().clients().get(subjectClient.getId()).removeOptionalClientScope(offlineAccessScopeId); | ||
| timeOffSet.set(0); |
There was a problem hiding this comment.
this can be removed, since TimeOffset has LifeCycle.Method
| realm.admin().deleteSession(exchangedToken.getSessionId(), false); | ||
| assertThrows(NotAuthorizedException.class, () -> keycloak.realm(realm.getName()).toRepresentation().getRealm()); | ||
| } | ||
| timeOffSet.set(0); |
There was a problem hiding this comment.
this can be removed, since TimeOffset has LifeCycle.Method
| fail("Error parsing response: ", e); | ||
| } | ||
| finally { | ||
| timeOffSet.set(0); |
There was a problem hiding this comment.
this can be removed, since TimeOffset has LifeCycle.Method
| assertIntrospectSuccess(exchangedToken, "requester-client", "secret", mike.getId()); | ||
| assertIntrospectSuccess(requesterClientToken, "requester-client", "secret", mike.getId()); | ||
|
|
||
| timeOffSet.set(0); |
There was a problem hiding this comment.
this can be removed, since TimeOffset has LifeCycle.Method
|
@lhanusov Thanks for the review! @mposolda now the PR should be ready, I marked @mabartos I added |
|
@graziang Thanks! Updated the PR description to cover even the related issue for dynamic scopes. |
Closes keycloak#47491 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Closes keycloak#47491 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Closes keycloak#47491 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Closes keycloak#47491 closes keycloak#48982 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Migrate
StandardTokenExchangeV2Testto the new test suite.Tests divided in 3 test classes:
StandardTokenExchangeV2TestStandardTokenExchangeClientPoliciesV2TestStandardTokenExchangeRefreshTokenV2TestI didn’t remove
StandardTokenExchangeV2Testfrom old test suite because it’s used to run V2 tests with the V1 feature enabled, and I didn’t want to add V1 tests to the new test suite. It’s probably better to remove it later with follow-up tasks on token exchange.