Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Updates internal-client detection to honor a configured admin realm, addressing #50796.
Changes:
- Replaces the hardcoded
"master"check withConfig.getAdminRealm(). - Preserves default behavior while supporting custom admin realms.
| @@ -415,7 +416,7 @@ private Map<String, Object> getClientCredentialsAdapterConfig(ClientModel client | |||
| private boolean isInternalClient(String realmName, String clientId) { | |||
| if (defaultClients.contains(clientId)) return true; | |||
|
|
|||
| if (!"master".equals(realmName)) { | |||
| if (!Config.getAdminRealm().equals(realmName)) { | |||
There was a problem hiding this comment.
Good catch. You're right that InternalClientManagementTest runs with the default admin realm and doesn't exercise client deletion, so it wouldn't guard this path.
The fix itself is a minimal alignment with the Config.getAdminRealm() idiom already used across the codebase (e.g. ApplianceBootstrap, MgmtPermissions, ModelToRepresentation) — behaviour is unchanged on the default master realm.
A regression test here needs the server booted with a non-default admin realm (a server-level option, not a per-test realm setup) and then asserts that removeClient refuses to delete a -realm client in that realm. I'm happy to add that as a @KeycloakIntegrationTest with a custom KeycloakServerConfig; let me know if you'd prefer it in this PR or a follow-up.
There was a problem hiding this comment.
@gaurav0107 I don't think we really need a test for this. We are using the same in a lot of places and I don't think we need a explicit test. Just checking everything is OK to me. If you really want to do the test you can file a follow-up issue and send a new PR. I rebased the PR and started the CI. If CI passes this is OK to me and we can proceed with it.
isInternalClient hardcoded the literal "master" realm name when deciding whether a realm is the admin realm. On servers configured with an alternative admin realm (Config.getAdminRealm()), the per-realm internal admin-container clients (<realm>-realm) were not recognised as internal and could be deleted. Use Config.getAdminRealm() instead, matching the admin-realm idiom used throughout the codebase. Behaviour is unchanged on the default master realm. Signed-off-by: Gaurav Dubey <gauravdubey0107@gmail.com>
1d53b97 to
ddadd13
Compare
rmartinc
left a comment
There was a problem hiding this comment.
Thanks @gaurav0107! Tests passed. As commented, if you want to do the test, just file a new issue for it.
Description
ClientManager.isInternalClient(realmName, clientId)hardcoded the literal"master"realm name when deciding whether a realm is the admin realm:On servers configured with an alternative admin realm (via
Config.getAdminRealm()/ thekeycloak.admin.realmoption), the per-realminternal admin-container clients (
<realm>-realm, which live in the adminrealm) are therefore not recognised as internal by
isInternalClient, andcan be deleted through
removeClient.This aligns the check with the admin-realm idiom used consistently across the
codebase (e.g.
ApplianceBootstrap,MgmtPermissions,ModelToRepresentation,AdminConsole), which all referenceConfig.getAdminRealm()rather than aliteral
"master".Fix
Use
Config.getAdminRealm().equals(realmName)instead of"master".equals(realmName).Config.getAdminRealm()returns a non-null value that defaults to"master",so behaviour is unchanged on default installations and there is no new NPE
surface (
realmName == nullstill yieldsfalse, as before).Testing
./mvnw spotless:check -pl services— passes../mvnw compile -pl services -am— compiles cleanly.test
InternalClientManagementTest(default admin realm), whose behaviouris unchanged. Exercising the custom-admin-realm path requires booting a
server with a non-default
keycloak.admin.realm; happy to add a dedicatedintegration test if preferred.
Closes #50796
🤖 Opened with Superhuman, an open-source contribution agent.