[KEYCLOAK-7416] Added code to store device information from the user-agent header by abstractj · Pull Request #6127 · keycloak/keycloak

abstractj · 2019-06-26T18:18:18Z

No description provided.

abstractj · 2019-06-27T18:31:42Z

@mposolda @hmlnarik I just did a rebase (nothing has changed since Marek's approval) of #5582 and @douglaspalmer already reviewed and approved it. He also handled uap-java parser and seems we are all set.

Only to make sure we're on the same page and has been a while, could you please take and check if there are more considerations around it?

hmlnarik

Why there is a need to change the UserSessionModel? I believe the device info should be only placed there as a note, just the same as other notes, and used only when needed. This would limit the number of changes in the PR

hmlnarik · 2019-06-28T12:37:32Z

@@ -235,6 +236,14 @@ public void testGetSessions() throws IOException {
        List<SessionRepresentation> sessions = SimpleHttp.doGet(getAccountUrl("sessions"), httpClient).auth(tokenUtil.getToken()).asJson(new TypeReference<List<SessionRepresentation>>() {});

        assertEquals(1, sessions.size());


Implement DeviceInfo.equals and use assertThat(sessions, contains(expectedDeviceInfo))

hmlnarik · 2019-06-28T12:41:50Z

+        String browser = deviceInfo.getBrowser();
+        String os = deviceInfo.getOs();
+
+        assertEquals("Other", device);


Use some other device info that uses explictly set values for some device (this needs to be done by manipulating the HTTP headers before GET is performed.

hmlnarik · 2019-06-28T12:43:51Z

+    }
+
+    public String toString() {
+        StringBuilder builder = new StringBuilder();


: needs to be escaped inside values and decoded in create

Feel free to use:

builder.append(device) .append(':') .append(browser) ...

Or other alternative can be to use other very strange delimiter like @@ when there is no chance of having the delimiter in the values and hence no escaping needed.

Regardless of the actual separator shape, it needs to be escaped, since it is coming from HTTP headers, i.e. potentially malicious source. In this case any occurrence of @@ would need escaping.

@hmlnarik Good point regarding escaping. With regards to this, I don;t have any preference whether to use ":" or something like "@@" . Can be fine to stick with ":" if it's easier.

Incidentally worked on another issue that also needs multi-field string serialization, and developed a tool for fast string (de)serialization, maybe it is worth reusing: https://github.com/keycloak/keycloak/pull/6139/files#diff-de17a5622aadad5857ba5697419c1e0b

mposolda · 2019-07-01T10:09:38Z

Why there is a need to change the UserSessionModel? I believe the device info should be only placed there as a note, just the same as other notes, and used only when needed. This would limit the number of changes in the PR

+1 to this. The PR already delegates saving DeviceInfo to notes and doesn't introduce new property on infinispan UserSessionEntity, but if there is no need to change UserSessionModel and anything in infinispan models etc, it will be even better. How about having some utility like this?

    public static DeviceInfo getDeviceInfo(UserSessionModel userSession) {
        return DeviceInfo.create(userSession.getNotes().get(DeviceInfo.ID));
    }

    public static void setDeviceInfo(UserSessionModel userSession, DeviceInfo deviceInfo) {
        userSession.getNotes().put(DeviceInfo.ID, deviceInfo.toString());
    }

Or the approach similar to OIDCAdvancedConfigWrapper, which means allow to use nice java set/get methods for specified properties, but delegate to generic "attributes" at the model level, so there is no need to change models when new property is needed?

mposolda · 2019-07-01T10:12:53Z

One more comment: I probably missed when/where it was decided that ua_parser is fine from various points of view (especially there are not any blockers from the productization perspective etc). I assume this was already agreed to go with ua_parser, but rather just asking.

abstractj · 2019-07-11T18:19:58Z

Why there is a need to change the UserSessionModel? I believe the device info should be only placed there as a note, just the same as other notes, and used only when needed. This would limit the number of changes in the PR

+1 to this. The PR already delegates saving DeviceInfo to notes and doesn't introduce new property on infinispan UserSessionEntity, but if there is no need to change UserSessionModel and anything in infinispan models etc, it will be even better. How about having some utility like this?
    public static DeviceInfo getDeviceInfo(UserSessionModel userSession) {
        return DeviceInfo.create(userSession.getNotes().get(DeviceInfo.ID));
    }

    public static void setDeviceInfo(UserSessionModel userSession, DeviceInfo deviceInfo) {
        userSession.getNotes().put(DeviceInfo.ID, deviceInfo.toString());
    }

I was chatting with @douglaspalmer. Could you please help me with my ignorance about the codebase? Is model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider the right place to add these methods?

Or the approach similar to OIDCAdvancedConfigWrapper, which means allow to use nice java set/get methods for specified properties, but delegate to generic "attributes" at the model level, so there is no need to change models when new property is needed?

abstractj · 2019-07-15T16:46:19Z

Why there is a need to change the UserSessionModel? I believe the device info should be only placed there as a note, just the same as other notes, and used only when needed. This would limit the number of changes in the PR

@hmlnarik could you please point to the specific part of the code where we do what you mentioned here? I could not find exactly what you meant. Taking for example SamlService I see that we created methods inside AuthenticationSessionModel to set client notes. But it's better to confirm exactly what you mean here.

hmlnarik

@abstractj Please see inline

hmlnarik · 2019-07-15T19:43:55Z

For example here. If this is kept inside notes, there is no need to change this inner class.

hmlnarik · 2019-07-15T19:44:17Z

For example here. If this is kept inside notes, there is no need to change this class.

hmlnarik · 2019-07-15T19:46:06Z

hmlnarik · 2019-07-15T19:46:08Z

…agent header

pedroigor · 2019-07-16T20:58:59Z

@hmlnarik and @mposolda, after talking with @douglaspalmer about these changes we noticed that the changes herein are not really addressing the requirements we have in the new admin console.

Based on the wireframe (not the best source of requirements but what we have) we should store device activity independently of user sessions. In order to address the requirements, we need to actually store device activity, separately.

We also discussed the best approach to storing this data and we can go either by having a specific entity for device activity or just store it as a user attribute.

Although using user attributes provide a more simple implementation with only a few changes, I think creating a new entity is the best way to go.

abstractj · 2019-07-23T18:22:02Z

@hmlnarik @mposolda thanks for taking the time reviewing it. I'm closing this PR for now because it's just a rebase from the @douglaspalmer changes. Like @pedroigor mentioned there are some requirements that we're not addressing in this change.

We are going to revisit it and open a new PR.

abstractj requested a review from douglaspalmer June 26, 2019 18:18

abstractj mentioned this pull request Jun 26, 2019

[KEYCLOAK-7416] Added code to store device information from the user-… #5582

Closed

abstractj self-assigned this Jun 27, 2019

douglaspalmer previously approved these changes Jun 27, 2019

View reviewed changes

abstractj requested review from hmlnarik and mposolda June 27, 2019 18:28

abstractj marked this pull request as ready for review June 27, 2019 18:31

hmlnarik reviewed Jun 28, 2019

View reviewed changes

hmlnarik mentioned this pull request Jul 4, 2019

KEYCLOAK-7852 Use original NameId value in logout requests #6139

Merged

abstractj dismissed douglaspalmer’s stale review via 51fd2db July 15, 2019 16:29

abstractj requested a review from pedroigor July 15, 2019 17:04

hmlnarik reviewed Jul 15, 2019

View reviewed changes

[KEYCLOAK-7416] Added code to store device information from the user-…

cb627be

…agent header

abstractj closed this Jul 23, 2019

clemlesne mentioned this pull request Apr 12, 2026

Expose device info (OS, browser, device) in Admin API UserSessionRepresentation #47980

Open

ahus1 mentioned this pull request Aug 10, 2026

Expose device info in Admin API UserSessionRepresentation #51517

Open

		@@ -235,6 +236,14 @@ public void testGetSessions() throws IOException {
		List<SessionRepresentation> sessions = SimpleHttp.doGet(getAccountUrl("sessions"), httpClient).auth(tokenUtil.getToken()).asJson(new TypeReference<List<SessionRepresentation>>() {});

		assertEquals(1, sessions.size());

Uh oh!

Conversation

abstractj commented Jun 26, 2019

Uh oh!

abstractj commented Jun 27, 2019

Uh oh!

hmlnarik left a comment

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!

mposolda commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mposolda commented Jul 1, 2019

Uh oh!

abstractj commented Jul 11, 2019

Uh oh!

abstractj commented Jul 15, 2019

Uh oh!

hmlnarik left a comment

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!

pedroigor commented Jul 16, 2019

Uh oh!

abstractj commented Jul 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mposolda commented Jul 1, 2019 •

edited

Loading