[KEYCLOAK-7416] Added code to store device information from the user-… - #5582
[KEYCLOAK-7416] Added code to store device information from the user-…#5582douglaspalmer wants to merge 1 commit into
Conversation
e02a7bc to
d35cfd2
Compare
mposolda
left a comment
There was a problem hiding this comment.
Few things to the PR:
-
The PR adds dependencies of "ua_parser" and some others in the common module. This module is very base module, which is used by the server, but also OIDC and SAML adapters. I guess this can cause various issues (EG. ua_parser library will be needed on all the adapter's sides etc). AFAIK the logic of device detection is needed just for the server? Can we re-structure the code somehow, so that the dependency is needed just on the server and not on adapters? For example change the "DeviceInfo" to be interface and create the impl in the server module to setup DeviceInfo in ClientConnection?
-
Is it possible to save DeviceInfo in the UserSession as a note? And make sure that when DeviceInfo is read from UserSessionEntity, there are checks for the case when UserSession doesn't contain DeviceInfo saved in it (make sure there is no NullPointerException etc)? It seems this can help with the failing tests as well.
More details: If someone is using cross-dc setup and do the scenario like:
-- Have Keycloak 4.4 running against JDG server.
-- Save some userSession from 4.4 to the JDG
-- Stop Keycloak 4.4 and start Keycloak 4.5 against same JDG server
-- Read the previously saved UserSession from JDG in Keycloak 4.5 and ensure that it still works.
In other words, will be good to make sure that UserSession serialized in Keycloak 4.4 can be readable in Keycloak 4.5 without some strange Serialization erroror, NullPointerExceptions etc.
b2825b2 to
4510772
Compare
|
We need to productize ua_parser before we can merge this PR |
4510772 to
2474be6
Compare
mposolda
left a comment
There was a problem hiding this comment.
Looks quite good to me. Just single comment regarding the read of UserSessionEntity
There was a problem hiding this comment.
Is it possible to add check for "nullity" here? And either return null or empty instance to the caller in that case? So something like:
String deviceInfoStr = this.getNotes().get(DeviceInfo.ID);
return deviceInfoStr == null ? null : new DeviceInfo(deviceInfoStr);
or second line to:
return deviceInfoStr == null ? DeviceInfo.EMPTY_INSTANCE : new DeviceInfo(deviceInfoStr);
The only reason for this is serialization when reading UserSessionEntity from the previous version (may happen in some scenarios like cross-dc).
5ee2750 to
1f4b000
Compare
|
@douglaspalmer Looks good to me. Thanks! I am not approving just because of ua_parser, which still require productization. Otherwise everything is fine IMO. |
1f4b000 to
6ef9d0d
Compare
6ef9d0d to
e7c990d
Compare
hmlnarik
left a comment
There was a problem hiding this comment.
My concerns are mostly about maintenance, see discussion on mailing list. Before continuing forward, we could consider the impact on the team. Marking as request changes until this is clear.
|
@douglaspalmer @mposolda I'm closing this PR because it was superseeded by #6127 |
…agent header.