Username to be displayed instead of user id with clickable link #39640
Conversation
|
@edewit new PR raised for your review. Username will be displayed instead of user id as a link. On clicking the link the details will be shown as was previously with user id |
|
@mpawlus for your review, requested changes done in this PR |
server-spi-private/src/main/java/org/keycloak/events/Event.java
Outdated
Show resolved
Hide resolved
server-spi-private/src/main/java/org/keycloak/events/EventBuilder.java
Outdated
Show resolved
Hide resolved
model/jpa/src/main/java/org/keycloak/models/jpa/session/PersistentUserSessionEntity.java
Outdated
Show resolved
Hide resolved
model/jpa/src/main/java/org/keycloak/events/jpa/EventEntity.java
Outdated
Show resolved
Hide resolved
model/jpa/src/main/java/org/keycloak/events/jpa/EventEntity.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Indu John <[email protected]>
6922203 to
77da542
Compare
|
@InJoDave When looking at the failing builds, it seems like there is an issue with the migration: Maybe there should be a check in the @edewit Do you have an idea why this could be happening? |
|
@mpawlus let me try this approach and push the changes. |
811026c to
0cbf0d1
Compare
|
@mpawlus the migration checks are now ok, the ones now failing seems to be for the test cases...will try my best to solve them as well. |
|
@mpawlus Could you please check the changes now? There are some failing for which I need some help |
…s as before and propagate its value automatically in EventBuilder same like userid Signed-off-by: Indu John <[email protected]>
…s as before and propagate its value automatically in EventBuilder same like userid Signed-off-by: Indu John <[email protected]>
|
@InJoDave Would you be able to fix the failing UI test? I believe it's caused by the changes in |
...tests/base/src/test/java/org/keycloak/testsuite/actions/AppInitiatedActionTotpSetupTest.java
Show resolved
Hide resolved
|
@InJoDave - unfortunately I will not have the time to review this PR in the next two weeks. I will come back to it later. I dismissed my previous comment as the changes have been addressed to unblock this in case someone else wants to review it. |
Sure will work on it |
|
@InJoDave I think I might have a fix for one of the issues, but I can't push anymore, I keep getting 403 response - can you please double-check if I am still a contributor? |
I have added you @mpawlus as well as @ahus1 as contributor. You can merge the test changes @mpawlus. Sorry for late reply and thank you for the same. |
Signed-off-by: Maciej Pawlus <[email protected]>
655ecba to
b9de961
Compare
Signed-off-by: Maciej Pawlus <[email protected]>
28051fd to
68a352a
Compare
|
@mpawlus Could you please resolve these conflicts? |
Signed-off-by: Maciej Pawlus <[email protected]>
Signed-off-by: Maciej Pawlus <[email protected]>
46a5366 to
555aa5a
Compare
Signed-off-by: Alexander Schwartz <[email protected]>
Signed-off-by: Alexander Schwartz <[email protected]>
Signed-off-by: Alexander Schwartz <[email protected]>
ahus1
left a comment
There was a problem hiding this comment.
Thank you for the changes, and sorry for the wait.
I had a look and when I scrolled to the end of the PR, I saw that several test cases are changed as now a different username is recorded.
IMHO this would be a breaking change to those who are parsing the events, and that want to see how a user logged in: either with a username, or an email address, etc.
See below for one example where adding the username can be safely removed, and another example where to keep the username.
As a test, all changes to the test cases that relate to a change in the username should then be reverted as well.
| } | ||
|
|
||
| event.user(userSession.getUser()) | ||
| .detail(Details.USERNAME, username) |
There was a problem hiding this comment.
This is an example where the username should be stored as entered by the user, as we don't want to change the logic of existing events. It might sometimes be important to know what the user actually entered in the form.
So this line and similar ones should be restored.
| eventBuilder.event(EventType.DELETE_ACCOUNT) | ||
| .client(keycloakContext.getClient()) | ||
| .user(user) | ||
| .detail(Details.USERNAME, user.getUsername()) |
There was a problem hiding this comment.
Removing this line is fine, as here the username is picked from the user, identical to what we do when constructing the event when passing in the user.
| )} | ||
| {!event.userId && t("noUserDetails")} | ||
| </> | ||
| return userId && userName ? ( |
There was a problem hiding this comment.
This change now requires both userId and userName to be set. But there are a lot of old events in the database that have a userId, but no username. And those are connected to a user. Those should be displayed with the userID and should link to the user (as before).
Replaced userid with username as column value to be displayed in User Events tab
Fixes : #39357
Signed-off-by: Indu John [email protected]