Repository navigation
is-45364 migrate Client Api V2 tests to utilize adminClient - #47789
Conversation
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "realm-admin", | ||
| client = "test-client", | ||
| ref = "realmAdmin") | ||
| Keycloak realmAdminAdminClient; | ||
|
|
||
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "view-clients", | ||
| client = "test-client", | ||
| ref = "viewClients") | ||
| Keycloak viewClientsAdminClient; | ||
|
|
||
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "query-clients", | ||
| client = "test-client", | ||
| ref = "queryClients") | ||
| Keycloak queryClientsAdminClient; | ||
|
|
||
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "manage-clients", | ||
| client = "test-client", | ||
| ref = "manageClients") | ||
| Keycloak manageClientsAdminClient; | ||
|
|
||
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "no-access", | ||
| client = "test-client", | ||
| ref = "noAccess") | ||
| Keycloak noAccessAdminClient; |
There was a problem hiding this comment.
Dear @mabartos , you got any better idea for handling the user specific tests ?
There was a problem hiding this comment.
I think it's alright to have managed admin clients as this. Maybe @lhanusov or @vaceksimon have some ideas how to deal with numerous admin clients.
@robson90 For the abstract class, you could also overload the getClientsApi() like this:
public ClientsApi getClientsApi(Keycloak adminClient) {
return adminClient.clients(getRealmName()).v2();
}And instead of using the:
try (var response = realmAdminAdminClient.clients(getRealmName()).v2().getClients()) {Using:
try (var response = getClientsApi(realmAdminAdminClient).getClients()) {But it's a nitpick.
There was a problem hiding this comment.
I don't understand the PR description: Relates to: https://github.com/keycloak/keycloak/issues/45366, how exactly? Can you write better PR description or some commit message explaining changes in this PR, please? I really don't want to nitpick, but the PR title didn't help me much either.
|
Dear @michalvavrik , Ive updated the description. Hope that helps. For now I just wanted to gather some feedback for implementation in: ClientApiV2AuthorizationTest.java |
|
Thank you @robson90 . |
There was a problem hiding this comment.
I went through changes, it looks fine. I won't go into detailed review since this is still a draft and there is open question.
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "realm-admin", | ||
| client = "test-client", | ||
| ref = "realmAdmin") | ||
| Keycloak realmAdminAdminClient; | ||
|
|
||
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "view-clients", | ||
| client = "test-client", | ||
| ref = "viewClients") | ||
| Keycloak viewClientsAdminClient; | ||
|
|
||
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "query-clients", | ||
| client = "test-client", | ||
| ref = "queryClients") | ||
| Keycloak queryClientsAdminClient; | ||
|
|
||
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "manage-clients", | ||
| client = "test-client", | ||
| ref = "manageClients") | ||
| Keycloak manageClientsAdminClient; | ||
|
|
||
| @InjectAdminClient(mode = InjectAdminClient.Mode.MANAGED_REALM, | ||
| user = "no-access", | ||
| client = "test-client", | ||
| ref = "noAccess") | ||
| Keycloak noAccessAdminClient; |
There was a problem hiding this comment.
I think it's alright to have managed admin clients as this. Maybe @lhanusov or @vaceksimon have some ideas how to deal with numerous admin clients.
@robson90 For the abstract class, you could also overload the getClientsApi() like this:
public ClientsApi getClientsApi(Keycloak adminClient) {
return adminClient.clients(getRealmName()).v2();
}And instead of using the:
try (var response = realmAdminAdminClient.clients(getRealmName()).v2().getClients()) {Using:
try (var response = getClientsApi(realmAdminAdminClient).getClients()) {But it's a nitpick.
There was a problem hiding this comment.
Pull request overview
Migrates Admin API v2 client-related integration tests from raw HTTP calls to using the generated v2 adminClient APIs, aligning tests with the new Java client work tracked in #45364.
Changes:
- Introduce shared helper accessors in
AbstractClientApiV2Testto obtainClientsApi/ClientApiinstances from the injectedadminClient. - Update multiple test classes to use
adminClient.clients(realm).v2()(and related helpers) instead of manual HTTP requests. - Adjust some tests to use realm-scoped injections (
ManagedRealm) and JAX-RS exceptions (NotFoundException,ForbiddenException) rather than status-code assertions from HTTP responses.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2Test.java | Switches many client v2 tests to use ClientsApi / ClientApi helpers; introduces realm-name override. |
| rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2DisabledTest.java | Converts disabled-feature test from HTTP GET to admin client call + NotFoundException assertion. |
| rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2AuthorizationTest.java | Migrates parts of authorization tests (list/create) to use managed admin clients and v2 APIs. |
| rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AdminEventV2Test.java | Migrates admin-event tests from HTTP calls to v2 admin client calls and switches event setup to a managed test realm. |
| rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AbstractClientApiV2Test.java | Adds helper methods for constructing v2 API clients/URLs based on the test realm. |
f7e46f1 to
cd3190e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AdminEventV2Test.java:32
- After the migration to adminClient-based calls, the injected HTTP client (
@InjectHttpClient CloseableHttpClient) and themasterRealminjection appear unused in this test. If the build enforces unused imports/fields, this can fail checkstyle/compilation; consider removing the now-unused imports and corresponding injected fields.
import org.keycloak.testframework.annotations.InjectHttpClient;
import org.keycloak.testframework.annotations.InjectRealm;
import org.keycloak.testframework.annotations.KeycloakIntegrationTest;
import org.keycloak.testframework.realm.ManagedRealm;
cd3190e to
8266a49
Compare
8468fbd to
87bca49
Compare
| @@ -1006,7 +1007,7 @@ private OIDCClientRepresentation getTestingFullClientRep() { | |||
| rep.setRoles(Set.of("view-consent", "manage-account")); | |||
| rep.setLoginFlows(Set.of(OIDCClientRepresentation.Flow.SERVICE_ACCOUNT)); | |||
| // TODO when roles are not set and SA is enabled, the default role 'default-roles-master' for the SA is used for the master realm | |||
| rep.setServiceAccountRoles(Set.of("default-roles-%s".formatted(testRealm.getName()))); | |||
| rep.setServiceAccountRoles(Set.of("default-roles-%s".formatted(testRealm.getName()).toLowerCase())); | |||
There was a problem hiding this comment.
Dear @mabartos , I ran into a problem here.
My realm name is "testRealm", then this fetching of default roles failed, as it was "default-roles-testRealm", which is wrong.
What do you think about adding a helper function to the ManagedRealm, something like get defaultRoleName ?
7873982 to
a149506
Compare
There was a problem hiding this comment.
Hey @robson90, I went through the PR and noticed that ClientApiV2AuthorizationTest still carries the old AdminClientFactory setup (adminClients map, setupClients(), CURRENT_USERS, createAdminClient()) even though all tests except getRealmAdmin() already use @InjectAdminClient. That's 7 redundant client instances per run.
I migrated getRealmAdmin() to use realmAdminAdminClient directly, which allowed removing the map, factory, CloseableHttpClient, and 9 unused imports. All 13 tests pass and it's a bit faster.
As I removed bunch of code I didn't want to clutter it here with several comments and instead I opened a PR against your branch - robson90#1.
There should not be a test parity gap but please check thoroughly and feel free to do the necessary changes. Thank you very much!
There was a problem hiding this comment.
InteropTest.adminClient shadows the inherited field - getClientsApi() and test methods end up using different instances. I suggest to remove the shadowing declaration and unused CloseableHttpClient.
That should match ClientPoliciesV2Test.
There was a problem hiding this comment.
I know that both resolve identically but since all the other tests use the parameterless getClientsApi(), I would keep it consistent.
|
@Pepo48 thanks for the review, have inserted the requested changes ! |
3590639 to
1eee363
Compare
Signed-off-by: Robin Meese <39960884+robson90@users.noreply.github.com>
1eee363 to
6d196cc
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BruteForceTest#testNoFailureResetForPermanentLockout |
…t AdminClientFactory setup Signed-off-by: Peter Zaoral <pzaoral@redhat.com> is-45364 fix PR comments Signed-off-by: Robin Meese <39960884+robson90@users.noreply.github.com>
6d196cc to
916c088
Compare
…#47789) * is-45364 migrate Client Api V2 tests to utilize adminClient Signed-off-by: Robin Meese <39960884+robson90@users.noreply.github.com> * Migrate getRealmAdmin() to use @InjectAdminClient and remove redundant AdminClientFactory setup Signed-off-by: Peter Zaoral <pzaoral@redhat.com> is-45364 fix PR comments Signed-off-by: Robin Meese <39960884+robson90@users.noreply.github.com> --------- Signed-off-by: Robin Meese <39960884+robson90@users.noreply.github.com> Co-authored-by: Peter Zaoral <pzaoral@redhat.com>
Relates to: #45366
CC: @mabartos & @vmuzikar
EDIT:
Migrate Client API V2 Tests from http calls to using the Keycloak adminClient.
Original Issue: #45364
Related PR: #46973