Readable realm name no longer visible in logs, but realm id is used instead

sventorben commented 8 months ago

Before reporting an issue

[X] I have read and understood the above terms for submitting issues, and I understand that my issue may be closed without action if I do not follow them.

Area

core

Describe the bug

It looks like KC behaviour with regard to logging realm ids has changed.

Here is an example log:

2024-03-04 15:06:54,694 DEBUG [org.keycloak.events] (executor-thread-4) type=LOGIN, realmId=a98c3a29-14f0-436e-92bb-288dc330be9a, clientId=security-admin-console, userId=ed7f6662-a730-49bc-be01-40596bd8a523, ipAddress=172.18.0.1, auth_method=openid-connect, auth_type=code, response_type=code, redirect_uri=http://*****/admin/test/console/, consent=no_consent_required, code_id=27089470-601b-47ed-a90f-2be66acae381, username=admin, response_mode=fragment, authSessionParentId=27089470-601b-47ed-a90f-2be66acae381, authSessionTabId=KExLqvKR4co

The realmId part in this log is the internal UUID of the realm (a98c3a29-14f0-436e-92bb-288dc330be9a) while for older realms the name is used (test). It is very cumbersome to filter for realm-specific logs now as I cannot lookup the id in the admin console.

It also does not align with the clientId which uses the name in the logs and not the internal UUID of the client.

More over

Version

23.0.7

Regression

[X] The issue is a regression

Expected behavior

Realm name should be logged instead of internal UUID to make the logs readable and align with clientId.

Actual behavior

Internal UUID of realm should be logged.

How to Reproduce?

Set up a fresh Keycloak instance with Loglevel set to org.keycloak.events:DEBUG,INFO
Try to login with wrong credentials
Check the logs

Anything else?

No response

sventorben commented 8 months ago

I checked the logging code, but it did not change recently. So, I am assuming this must be related to the code for creating new realms. The field on the "create realm" form has a label "Realm name" instead of "Realm id"

Maybe related?

jonkoops commented 8 months ago

Yeah this is wrong, we need to align these labels and be consistent about it. Feel free to submit a PR to align this.

sventorben commented 8 months ago

@jonkoops This is not only an issue with the UI and the label here. The realmId value in the logs also changed from the name (like test in this example) to the internal UUID.

It's also confusing when compared to Clients

ClientModel

    String getId(); // internal UUID
    String getClientId(); // id set by the user
    String getName(); // display name

compared to RealmModel

    String getId(); // internal UUID
    String getName(); // id as set by the user - should this be getRealmId instead to align with ClientModel?
    String getDisplayName(); // display name

and within the EventBuilder class:

    public EventBuilder client(ClientModel client) {
        event.setClientId(client == null ? null : client.getClientId());
        return this;
    }

    public EventBuilder realm(RealmModel realm) {
        event.setRealmId(realm == null ? null : realm.getId()); // should this be getName() to align with ClientModel?
        return this;
    }

jonkoops commented 8 months ago

Yeah. I'd like the core team to take a look at this as well.

mposolda commented 8 months ago

@sventorben Few things here:

The "client" is a bit specific entity as there is parameter client_id in the OAuth/OIDC specification, which does not need to be UUID, but rather refers to the name provided by administrator. But for all the other entities, the "id" points to the DB ID (which is usually UUID). So for all other entities, the realmId, userId, roleId, groupId etc typically refer to the DB ID, which is expected.
What is the old version where the thing work as expected by you? Was this working as expected when using old admin console, but changed in the new admin console? I have some suspicion that old admin console worked in a way that when creating new realm, the value provided by admin was used as both "id" and "name" of the realm. When in the new admin console, it works in a way that "name" is value provided by admin, but ID is generated. Could you confirm if this is what you are seeing?
Will it work for you if we add both "realmId" and "realmName" to the server.log when logging the events?

sventorben commented 8 months ago

@mposolda thanks for the quick reply.

I just tested with the old admin console and you are right. For realms created with the old admin console, the name is logged and not the UUID. For realms created with the new admin console, the UUID is logged and not the name. The old admin console send the id property set to the name of the realm, the new admin console does not.

Logging the "realmId" and the "realmName" would work fine for me.

mposolda commented 8 months ago

@sventorben Thanks, I am switching this to important and added to Keycloak 25 as the only thing, which we need to do, might be to add realmName to the event logged by JBossLoggingEventListenerProvider .

Specifically we don't want to change the default behaviour of admin console or anything like that and align it with old admin console as it seems the change in the behaviour was done on purpose.

sschu commented 8 months ago

I think the change was done because using the realm name as realm id (as before) led to strange behaviour when renaming realms - the realm id stays but the realm name changed. Then you could not add a new realm with the former name although in the UI, the realm did not look as if it was used. I also vaguely remember events not being deleted upon realm deletion so new realms with the same name as old realms might be associated with events from that realm.

jonkoops commented 8 months ago

See also #23496 and #28106 for the UI related changes. This would rename the 'Realm name' field to 'Realm ID' @mposolda wdyt?

MarcelSz commented 7 months ago

I was doing a research and I was about to create an issue and I'm happy to see that this was already reported.

@jonkoops, as it was mentioned earlier, it would be the best to have realmId and realmName in logs. Why so? I'm afraid that mentioned PR's are about the layout and the realmId property to be used during tenant creation, but I might be wrong. Another point, how about a production case where there are realms created in e,g, KeyCloak v19 (where realmName is expected and logged) and latest KeyCloak (where internal id is logged)? @sventorben mentioned that earlier

Mo0rBy commented 7 months ago

Hello! I came across this issue as I would also like to show the actual names of things rather than the UUID's in the logs. Reading this thread, it looks like only realmName/realmId are mentioned and I wanted to double check if this would include all objects that have a name/id. For example:

realmName/Id
clientName/Id
userName/Id

These are the 3 I am most interested in, not sure if there are others in addition to these 3.

Mo0rBy commented 7 months ago

I've done some further investigation into this and some more discussion with my team so I'm putting an update here in an effort to drive this change in the right direction.

Firstly, our requirement to print name's rather than id's to the logs is simply to make the logs very easy to follow and understand. We need to have a simple and easy audit trail events occurring with Keycloak, and dealing with UUID's and querying a database or realm file definitely does not meet this requirement. Furthermore, the resources that we require easily readable names for are:

realmName
clientName
userName

Secondly, I believe that this problem does originate from the different Admin Console versions being used in different versions of Keycloak as suggested earlier in this thread. I have the following realms (I'm unsure what version of Admin Console is used in each Keycloak version, sorry!)

Created using Keycloak v19.0 > realmId and realmName have the same string value (e.g. "myRealm")
Created using Keycloak v21.1 > realmId is a generated UUID, realmName is a string value (e.g. "myRealm")

Finally, I think there needs to be some consideration to different event types. For example, the LOGIN event type includes both the userId and username attributes in the resulting log, but the CODE_TO_TOKEN event type only includes the userId. I'm unsure if there is a reason that the username attribute is only included in only 1 of these events, as the CODE_TO_TOKEN event will always happen after a LOGIN event, so it's easy to put the 2 together and see the username attribute within the LOGIN event, but our clients log-analyzer staff are somewhat non-technical and it would be easier to just have the info available in all the log messages that it is relevant for. To summarise this, I believe it would be best to include all id and all name attributes for all the logs that they are relevant for, without needing context from a previous log in the history (realm id and name, client id and name, user id and name).

I hope this helps to give some context to this issue. Thanks!

Mo0rBy commented 6 months ago

@mposolda Thanks for working on this.

I noticed your changes only address the realmName and realmId differences. Shall I create a new issue for the other logging features I described in this thread?

keycloak / keycloak