Repository navigation
Replies: 7 comments 14 replies
|
To keep the promise of the method signature, attributes should be stored so that order is kept and duplicates are allowed. This means adding the guaranteed ordering. This makes it easier for consumers of the attributes. They don't have to sort to get stable lists. Receivers of a String representation (e.g. comma separated) could more easily detect that the values are unchanged. |
|
Here is a contrived example: Assume we have a group called "snowboarders chapter", it can have the following attributes 1) "allowed_snowboard_materials": ["wood", "fiber glass", "carbon fiber"], 2) "team color voting":["red", "red", "red", "green", "green"], 3) "members_accepted_from_countries": ["usa", "uk", 'fiji", ...]. There certainly are other ways to store this info that better conforms with the normal forms but if we ignore normal forms '''Map<String, List> getAttributes();''' seems to be the right signature that fits all three usages and for the "countries" attribute sort order certainly looks important. |
|
While it is possible to think of many artificial examples, we are interested in real-world uses. Are there any? If not, I'm in favor of changing attributes into a |
|
@hmlnarik How to you intend to handle existing duplicate values? I guess you would not add a unique constraint to the db but filter out duplicate values in code? |
Yes, that's correct for the current store |
|
@hmlnarik - I think there are three options to choose from: lists (ordered + duplicates allowed), bag (not ordered + duplicates allowed), and set (not ordered + no duplicates). Looking at the performance characteristics of a JPA store, both set and bag would behave the same IMHO. Any other store that can't implement a bag can implement a list instead; this will preserve the order and that feature might "leak" to the callers, still a list is a valid implementation for a bag. The old JPA store implements a bag. As @vramik mentioned, Hibernate chooses a List as the Java type to represent a bag (and also an idbag). This makes it a bit strange from a Java's point of view, and that's a trap I've been fallen into before. |
|
There's been a question on keycloak-dev regarding multiple identical values, invited the person to join the discussion: https://groups.google.com/g/keycloak-dev/c/Vmr0x6_5cHE/m/-_F2X8V0AwAJ |
|
I contacted you through the keycloak-dev mailing list to report a potential bug. As invited by @ahus1 to report my case, this is how we were using attributes... We are managing certified digital identities and each user is tagged with an attribute "rights" telling which user identity has been approved or not. In the browser login flow, we have a specific authenticator which checks if the "rights" attribute is valid for a type of right. |
|
Hi @fperot74 - thanks for detailing your scenario. The example helps a lot to understand what's going on. You wrote:
As this a discussion for an API change, this might implemented in some mid to long term future. Until then, I the behavior you described on the dev-mailinglist is still a bug, and therefore should be reported and fixed. Even if you have a workaround, it might safe someone else from experiencing this issue. Therefore, please go ahead a report a bug and propose a code change. |
|
Hi @fperot74, in your mailing list post you suggested that sorting of streams could fix the bug. Just as an option, I don't know performance impacts. |
|
Using containsAll() twice does not fully solve the issue : if you have [ A, A, B ] and [ A, B, B ], current sizes are equal, list1.containsAll(list2) and list2.containsAll(list1) are true so your algorithm will suppose that both lists are equal. Complexity for containsAll() is O(n²) and a solution with Map is only O(n) |
|
So far it seems that there are no real-world examples of the use of an ordered list in the attributes, and also no use case for a bag (a.k.a. multiset). Given the confusion around the public interface UserModel {
// [...]
void setAttribute(String name, Collection<String> values);
Set<String> getAttribute(String name);
Map<String, Set<String>> getAttributes();
// [...]This discussion is still open for the moment. If you object this change, please speak up. |
|
I don't have any use-case in mind for guaranteed ordering of attribute values and I don't recall any such use-case from anyone in the past. Also don't have any use-case for multiset. The proposed change is fine to me. |
|
From my POV, attributes don't have to be ordered in the REST API, but it makes sense to have them ordered in the UI - by attribute key. I don't know whether it is necessary that attribute values are ordered. There is an issue regarding ordering by attribute key in a slightly different, but similar context: realm localization texts. See #9418 |
|
I also don't see a reason to maintain an explicit order for attribute values. However I want to point out that changing the signature of How about leaving the attributes as is and create a new dedicated abstraction for storing user provided meta-data? |
|
I agree with the need for visible communication re upcoming changes. It will be strongly needed also because the new storage will replace user federations with user map storages, thus there will be refactorings to user federations needed anyway. Hence I incline more towards replacing the methods rather than introducing new ones. As for the metadata - whatever fields are in model are also visible to the logical layer, e.g. services. For fields that have no meaning to logical layer (various metadata, e.g. original LDAP guid in JPA federation link), these should be stored and leveraged in the storage entities, but must be invisible to the model that builds on these entities. |
|
I only see this discussion now, but I think we are affected by the change from ordered to unordered. We have multivalued attributes on groups where the two lists depended on each other. One attribute contains numbers and the other names, and the numbers and names belong to each other and were mapped based on order. For example, these are the groups where foo was mapped to 2, bar to 1, baz to 3 and zzz to 0: but using multivalued user attribute mappers for name and number, we get the following order in the JWT: So they are no longer sorted as intended, but foo is mapped to 1, bar to 0, baz to 2 and zzz to 3. Since the JWT can only contain an array data type, and not a set, that array interface suggests the order is maintained. It might not have been the best design to have two multivalued lists that depended on each other. What would be a workaround for this use case? We are now leaning to the use of a script mapper that contains the names that belong to the numbers and map them into the JWT like that, but there might be a better solution. |
|
We are now using the script mapper to maintain backward compatibility with clients. Another solution is attributes and claims like this: name:number -> "foo:2". |
Uh oh!
There was an error while loading. Please reload this page.
Keycloak currently supports multi-valued attributes for groups, roles and users. The interfaces describes following signature:
Key of the map is attribute name and value of the map should be list of the attribute values.
Looking at current jpa implementation, e.g.
RoleEntity. It contains list of attributes but no order column. Therefore the order of attribute values is not guarantied and it means the bag data-type is used. It seems it should be the preferred way when duplicates are allowed.There are couple options how we should handle attributes in new store and I'd like to know what are use-cases of it in current deployments.
Is there any deployments what'd require
All reactions