Migrate the AccountRestServiceTest by gaoyikeshuer · Pull Request #49296 · keycloak/keycloak

gaoyikeshuer · 2026-05-25T13:01:34Z

This PR is aim to migrate AccountRestServiceTest. the two extending classes (Lightweight, withUserProfile) are technically not required by this issue, but I migrate them because they share AccountRestServiceTest as their parent, and moving the parent without them will create a half migrated state where they break.

Tests missing for full migration of the account package are:

AccountRestServiceCorsTest
AccountRestServiceReadOnlyAttributesTest
LinkedAccountsRestServiceTest
ResourcesRestServiceTest
SessionRestServiceTest
custom/* tests

Delete legacy AbstractRestServiceTest (only possible once all the above are migrated)

Closes #48148

vaceksimon

Great effort on this! It’s a massive PR, so to keep things manageable, let's break the review into a couple of rounds. Let's get these current points squared away first, and then proceed with the rest.

One thing that might save you some effort here: you don't need to migrate all the extending classes just because you're migrating the abstract one. You can absolutely scope this down to just what AccountRestServiceTest needs, and we can let the next developers handle the rest.

vaceksimon · 2026-05-26T14:41:52Z

+        oauth.scope(null);
+        setRequiredActionEnabled(managedRealm.admin(), UserModel.RequiredAction.VERIFY_PROFILE, false);
+        managedRealm.admin().logoutAll();
+        events.clear();


This doesn't do anything

Done, have removed them and @beforeeach. there are still two events.clear() calls in the tests but those are not dead. the events.clear() actually needed cause they drain login events before checking specific subsequent events. otherwise can't pass the ci/cd. let me know if you prefer a different pattern :)

rmartinc

Thanks @gaoyikeshuer! I added a new commit in my test branch for this PR https://github.com/rmartinc/keycloak/tree/refs/heads/pr-49296. The main idea is adding the theme and provider to run the disabled test in another class. I have added some other minor changes to not do manual modifications in the realm and always use the updateWithCleanup. If you find something more on those lines, you can follow the same idea (although I think that with those changes the PR is OK to me).

Please include the changes in the test commit from my branch in this PR. You can just squash it or amend or do whatever you like with it.

I think that with that commit, this is OK to me.

vaceksimon

Keep up the good work @gaoyikeshuer!

vaceksimon · 2026-05-29T14:16:27Z

+                            UserBuilder.create().username("test-user@localhost")
+                                    .email("test-user@localhost")
+                                    .name("Tom", "Brady")
+                                    .attribute("attr_required", "value")
+                                    .attribute("attr_required_by_role", "value")
+                                    .attribute("attr_required_by_scope", "value")
+                                    .realmRoles("user", "offline_access")
+                                    .clientRoles("account", "view-profile", "manage-account")
+                                    .password("password"),


This user is referenced a lot in the tests. In this case, we can turn it into a managed user with this config:

@InjectUser(config = TestUserConfig.class) ManagedUser testUser;

With that, you can replace registerUserCleanup(...) with a call to testUser.dirty().

Also using "test-user@localhost" and "password" directly can turn into testUser.getUsername() and testUser.getPassword()

@gaoyikeshuer @vaceksimon I think this is the only point that still stands. I leave it to you both, if you prefer to change to use the ManagedUser, OK, but it's also OK to me as it's now. 😄

Hi @vaceksimon , thanks for this suggestion! I tried to use the @InjectUser before but it fails at UserSupplier:42 with "Creating user with roles or client roles is not supported!". this user needs the account/view-profile + account/manage-account client roles plus user + offline_access realm roles to function. so i've reverted that change. I think we will need to modify framework if we want @injectUser here. which is better to be in a separate PR

Right. The role creation is unfortunately a limitation of the current admin API, which should be fixed with admin API v2. In this case, I'm ok with this

vaceksimon · 2026-05-29T14:19:36Z

        events.poll();
        events.poll();
        events.poll();


This is a nitpick, and I only mention it so you know it exists; the same thing can be done with events.skip(3);

vaceksimon · 2026-05-29T14:23:46Z

            final OAuth2ErrorRepresentation error = response.asJson(OAuth2ErrorRepresentation.class);
            assertThat(error.getError(), containsString("Invalid json representation for UserRepresentation. Unrecognized field \"invalid\" at line"));
        }
    }

    @Test
    public void testEmailWhenUpdateEmailEnabled() throws Exception {
-        reconnectAdminClient();
+        managedRealm.cleanup().add(r -> setRequiredActionEnabled(r, RequiredAction.UPDATE_EMAIL, false));
+        managedRealm.updateWithCleanup(r -> r);


This is a preferable way to achieve the same thing

Suggested change

managedRealm.updateWithCleanup(r -> r);

managedRealm.dirty();

vaceksimon · 2026-05-29T14:24:03Z

-        String accessToken = tokenUtil.getToken();
+        org.keycloak.representations.idm.ClientRepresentation originalClientRep = AdminApiUtil.findClientByClientId(managedRealm.admin(), "direct-grant").toRepresentation();
+        managedRealm.cleanup().add(r -> AdminApiUtil.findClientByClientId(r, "direct-grant").update(originalClientRep));
+        managedRealm.updateWithCleanup(r -> r);


This is a preferable way to achieve the same thing.

Suggested change

managedRealm.updateWithCleanup(r -> r);

managedRealm.dirty();

vaceksimon · 2026-05-29T14:46:40Z

    @Test
    public void testAccountAccessWithRealmNotBeforeAndClientNotBefore() throws IOException {
-        String accessToken = tokenUtil.getToken();
+        org.keycloak.representations.idm.ClientRepresentation originalClientRep = AdminApiUtil.findClientByClientId(managedRealm.admin(), "direct-grant").toRepresentation();


Since the realm will be recreated with everything in it, the cleanup for the "direct-grant" client can be removed.

rmartinc

Thanks @gaoyikeshuer! For me it's OK. Please @vaceksimon take a look when you have time to see if it's also OK for you.

vaceksimon

@gaoyikeshuer took a quick look. I will check the rest tomorrow once back in the office

…loak#48148 Signed-off-by: Yike Gao <yikegao8@gmail.com>

rmartinc

Thanks @gaoyikeshuer! LGTM!

vaceksimon

Well done @gaoyikeshuer, thank you!

rmartinc · 2026-06-02T09:49:21Z

Thanks @gaoyikeshuer and @vaceksimon!

…loak#48148 (keycloak#49296) Signed-off-by: Yike Gao <yikegao8@gmail.com>

gaoyikeshuer force-pushed the migration-test-48148 branch 2 times, most recently from b14bc3f to 68f2fdb Compare May 25, 2026 13:06

stianst added the team/core-authn label May 26, 2026

gaoyikeshuer force-pushed the migration-test-48148 branch 8 times, most recently from 7b2bf50 to 69ea8ea Compare May 26, 2026 11:43

gaoyikeshuer marked this pull request as ready for review May 26, 2026 12:26

gaoyikeshuer requested review from a team as code owners May 26, 2026 12:26

gaoyikeshuer force-pushed the migration-test-48148 branch 2 times, most recently from ca73108 to 116f5b4 Compare May 26, 2026 13:12

vaceksimon self-requested a review May 26, 2026 13:47

vaceksimon suggested changes May 26, 2026

View reviewed changes

gaoyikeshuer force-pushed the migration-test-48148 branch 2 times, most recently from a6b6534 to 315e940 Compare May 27, 2026 10:18

gaoyikeshuer requested a review from a team as a code owner May 27, 2026 10:18

gaoyikeshuer force-pushed the migration-test-48148 branch 7 times, most recently from 69fb906 to 481e4cc Compare May 28, 2026 09:15

gaoyikeshuer force-pushed the migration-test-48148 branch 3 times, most recently from 69d9b3d to ac3f585 Compare May 29, 2026 08:31

gaoyikeshuer commented May 29, 2026

View reviewed changes

Comment thread tests/base/src/test/java/org/keycloak/tests/account/AccountRestServiceTest.java Outdated

rmartinc requested changes May 29, 2026

View reviewed changes

Comment thread tests/base/src/test/java/org/keycloak/tests/account/AccountRestServiceTest.java Outdated

vaceksimon suggested changes May 29, 2026

View reviewed changes

gaoyikeshuer force-pushed the migration-test-48148 branch 3 times, most recently from 547db7b to 5e42776 Compare May 31, 2026 00:09

rmartinc previously approved these changes Jun 1, 2026

View reviewed changes

vaceksimon suggested changes Jun 1, 2026

View reviewed changes

Comment thread test-framework/builders/src/main/java/org/keycloak/testframework/realm/RealmBuilder.java Outdated

rmartinc self-assigned this Jun 1, 2026

Migrate AccountRestServiceTest to the new test framework. Closes keyc…

26e0b7e

…loak#48148 Signed-off-by: Yike Gao <yikegao8@gmail.com>

gaoyikeshuer dismissed rmartinc’s stale review via 26e0b7e June 1, 2026 23:03

gaoyikeshuer force-pushed the migration-test-48148 branch from 5e42776 to 26e0b7e Compare June 1, 2026 23:03

rmartinc approved these changes Jun 2, 2026

View reviewed changes

vaceksimon approved these changes Jun 2, 2026

View reviewed changes

rmartinc merged commit d4c89b3 into keycloak:main Jun 2, 2026
208 of 276 checks passed

This was referenced Jun 2, 2026

Complete migration of account REST tests to the new test framework #49578

Open

Migrate AccountRestServiceCorsTest #49579

Open

Migrate LinkedAccountsRestServiceTest #49582

Open

sashyo pushed a commit to tide-foundation/keycloak-IGA that referenced this pull request Jul 20, 2026

Migrate AccountRestServiceTest to the new test framework. Closes keyc…

6bc9eef

…loak#48148 (keycloak#49296) Signed-off-by: Yike Gao <yikegao8@gmail.com>

muhammedogz mentioned this pull request Aug 10, 2026

Add per-application session termination to Account Console #47922

Open

	managedRealm.updateWithCleanup(r -> r);
	managedRealm.dirty();

Uh oh!

Conversation

gaoyikeshuer commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaceksimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaoyikeshuer May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rmartinc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vaceksimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmartinc left a comment

Choose a reason for hiding this comment

Uh oh!

vaceksimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rmartinc left a comment

Choose a reason for hiding this comment

Uh oh!

vaceksimon left a comment

Choose a reason for hiding this comment

Uh oh!

rmartinc commented Jun 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaoyikeshuer commented May 25, 2026 •

edited

Loading

gaoyikeshuer May 29, 2026 •

edited

Loading

rmartinc left a comment •

edited

Loading