Two ways for setting and getting username, email, firstName, lastName properties

[mhajas]

Introduction to the issue

Currently, there are two ways how to set some chosen attributes for UserModel (namely username, email, firstName, and lastName):

1. userModel.setUsername/Email/FirstName/LastName(value)
2. or, userModel,setAttribute("username"/"email"/"firstName"/"lastName", value)

I am not sure how it was before (I guess it was number 1), but currently, when a new user is created approach 2 is used. See: https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java#L157

Problem statement

This change (from approach 1 to 2) doesn't cause any problems when legacy JPA adapter or new storage MapUserAdapter is used, however, it may cause problems with custom user storage providers.

An example of a wrong implementation can be found in our quickstarts where we are not checking the attribute when obtaining email https://github.com/keycloak/keycloak-quickstarts/blob/5719ba5b3df671df8e69da12632f3c47f1ac7421/user-storage-jpa/src/main/java/org/keycloak/quickstart/storage/user/UserAdapter.java#L65-L68

In some cases, this becomes really a mess where we sometimes use approach 1 and sometimes 2. This resulted, for example, in this issue: https://issues.redhat.com/browse/KEYCLOAK-19610 where the reporter uses something similar to our quickstart.

In this ^^ case, the email is present only when the user is obtained from the cache because the cached adapter is adding the functionality of checking attributes instead of just looking at the email.

Solution #1

This issue is mainly caused by the wrong UserAdapter implementation in the custom storage provider. However, we don't have any documentation and we have even the quickstart made wrong, so people cannot know that approaches 1 and 2 need to be equivalent. Hence the solution for this would be the addition of documentation to UserAdapter stating that the methods should work in a way that userModel.setUsername == userModel.setAttribute("username",...)

Solution #2

Refactoring the logical layer and allowing only one of the two ways for setting these attributes. Currently, it would probably make sense to allow only 2.

Solution #2.5

Take #2 even more seriously and move this to the storage layer as well. We could remove the email/username/lastName/firstName fields from the entities and leave this decision to each storage implementation whether it makes sense to have this stored separately.

WDYT?

[hmlnarik]

Caused by https://issues.redhat.com/browse/KEYCLOAK-14536

[hmlnarik]

I like the solution 2.5, though due to compatibility requirements, we can only mark the existing UserModel methods as deprecated rather than removing them.

Interesting aspect is that the username, first / last name and email are searchable fields.

The question of treating them as first-class entity members vs attributes then has to consider the following facts:

• Since they are searchable, they are distinguishable from other "ordinary" attributes and fields
• There is already support for search by attribute so it should not represent a big change to replace the searchable field with attribute search
• Should these be treated as normal attributes, the implementations might need to optimize the lookup by these specific attributes

Q: Would this change represent a performance or other issue for the storages?

[bs-matil]

I would say that for a sql store it would be harder to search by a foreign key with a one to many relationship. So I would suspect a penalty if you do a search on one or even multiple attributes, this depends also on the feature set of the database. It would also get worse by the numbers of attributes as they have to be fully scanned, if I am not wrong.

On the other hand in some nosql dbs it would be possible to index just fields individually on demand. So one could imagine to define the name of "searchable" attributes upfront and keycloak requests dynamically indices on this fields.

Or keycloak introduces a search cache which caches just those fields for the object life time (maybe also in an external service) to do searches. This is may the most expensive but fastest (query time) solution. I could imagine it would be nice to have some interface to implement external fulltext search or something. The way search works this days is often quite slow.

[mhajas]

Thank you for your responses.

These are all valid points. However, these all can be solved per storage implementation. By removing setUsername/getUsername we are not removing the column that is currently present in JPA or the field in HotRod, it can stay as is, the only thing that changes are methods setAttribute/getAttribute (setting the value in first-class entity member instead of the attributes map). Therefore, I believe this will not have any performance impact if we do it correctly.

For example, storing username in attributes instead of first-class entity member could have a performance impact as usernames are queried quite often, at least for JPA so we shouldn't do it this way.

[I am just thinking aloud, not suggesting anything]

On the other hand, this could lead to a conclusion: why only username/firstName/lastName/email fields can be set using the set/getAttribute methods, why not others? We could end up in a situation where there are only a few methods for working with the attributes and everything would be an attribute. This would be possible using something similar like *entityFields enums we already have in map module.

This would mean quite a massive refactoring though, maybe we should leave this discussion to future work on the adapters refactoring and just fix the issue of two ways for doing the same thing.

[ahus1]

I could see that username and email are first class citizens when we need to have special semantics on these attributes, like: special handling that a username can't be changed without checking it is unique, or changing an email address would require re-validation.

Putting everything into generic attributes would result in meta-programming, and checking attribute names with string comparisons. To me, using attribute names is using a weakly typed model. It would allow a more generic approach to things, but would be more difficult to maintain IMHO.

Making something accessible as an attribute and as a first class method at the same time is the worst of two worlds. It should be either-or.

I'll leave the decision about "generic attributes" vs. "semantic model" to those with more experience in Keycloak, still I would lean towards "semantic model". At the same time, I'd argue not to allow both methods for the same value as it will be confusing.

[mhajas]

Thank you for the response @ahus1, this thread is not about removing the possibility for uniqueness checks in a relational database by storing everything in the attributes table. This is just about getting rid of two ways for achieving the same thing on the logical level.

[ahus1]

Thanks for your response. My previous comment was primarily on the logical level, as on that level setting an email address usually does something more than "just" setting an email address. I see several places where email addresses are converted to lowercase, and where checks about duplicates occur. From a technical point of view both approaches can work.

[sschu]

Currently, email does have a special semantics at it is the primary means for registration. I am not sure this is justified. There are quite some countries where phone numbers are primarily used instead of email so I would rather go with the generic approach.

[vramik]

+1 for getting rid of two ways of achieving the same thing.

looking from jpa point of view ...

Currently there are methods which uses exact search by username (UserLookupProvider.getUserByUsername) and email (UserLookupProvider.getUserByEmail).

When both fields would be stored as first class citizens (and assuming index is present) the query plan could use index scan and therefore the execution time would be low (in my testing environment 3.482 ms with ~10M entries).

When both fields are stored as attributes (also index on attributes present) the query plan looks like

Nested Loop  (cost=482.14..58552.55 rows=1 width=328) (actual time=70.871..83.582 rows=1 loops=1)
  ->  Bitmap Heap Scan on user_attribute  (cost=481.85..58544.24 rows=1 width=51) (actual time=42.441..83.523 rows=2 loops=1)
        Recheck Cond: ((name)::text = 'username'::text)
        Filter: (value = 'user5774'::text)
        Rows Removed by Filter: 20712
        Heap Blocks: exact=20682
        ->  Bitmap Index Scan on user_attr_name_value  (cost=0.00..481.85 rows=19372 width=0) (actual time=5.984..5.985 rows=20714 loops=1)
              Index Cond: ((name)::text = 'username'::text)
  ->  Index Scan using user_pkey on kc_user  (cost=0.29..8.31 rows=1 width=277) (actual time=0.017..0.017 rows=0 loops=2)
        Index Cond: (id = user_attribute.fk_root)
        Filter: ((realmid)::text = '4df28e0b-f198-46b0-9ed2-4b661c1e5ba1'::text)
        Rows Removed by Filter: 0
Planning Time: 0.558 ms
Execution Time: 83.657 ms

It seems it might be beneficial to store at least username and email as first class citizens even when they would be stored in attributes at logical level considering that there is a high chance of having many entries in user table and the fact that lookup should be fast.

The other fields are used by UserQueryProvider.searchForUserStream. Recently there has been done change in default behavior in the search: #8346

[hmlnarik]

Thinking about this a bit more:

On the logical level, the fields need to be accessible also as attributes: they are treated as such in user profile code (see #7080). At the same time, often accessed attributes deserve a support in model being visible as separate methods, so this seems to converge to approach #1.

On the physical level, it is desirable to leverage constraints supported by the datastore for uniqueness checks. In case of RDBMS, there should ideally be a separate column for username / email to allow for uniqueness check, and also firstname and lastname for fast search. However this does not mean that Map*Entity will have the username and similar fields - only that the JPA storage would translate the values from the attribute to a database column (just like it does it for a few fields already). So here it is #2.5

In total - the adapters should support the getter methods for the username / email / firstname and lastname fields as transcribing the value into attributes, to be consistent with logical layer requirements. The storage implementation should consider storage capabilities and retranslate some attributes into physical column, based on analysis of uniqueness and performance similar to @vramik's above.

Technical side note: Postgres, CockroachDB, MSSQL, Oracle support partial unique indices which could be used for DB-level data integrity check. AFAIK, there is no such support in MySQL / MariaDB. This could be used for e.g. uniqueness checks for the telephone number @sschu has mentioned above.

[mhajas]

So if I understand correctly, we will go with #2.5 in the physical layer and #1 in the logical layer is it correct? Should we include this change in the 1st phase? Or is this a good candidate for trying out no-downtime migration?

[hmlnarik]

So if I understand correctly, we will go with #2.5 in the physical layer and #1 in the logical layer is it correct?

Yes, that is so.

Should we include this change in the 1st phase? Or is this a good candidate for trying out no-downtime migration?

First phase please - we should avoid introducing / continuing in the inconsistency of handling first / last name, username and email specially when they should be handled as attributes.

[mhajas]

Thanks for the clarification. Issue created: #12495