Is this search supported by any index?

Yes, inverted index was added for the components

Is this search supported by any index?

Yes, inverted index was added for client initial accesses

UUID ids are being enforced only when the map storage is enabled as it was not a requirement for the legacy storage. We need to decide if we simply convert non-conforming ids automatically as the code above is doing, or if we want to throw a validation error.

Conversion is essential to be able to support importing legacy realms as those will come with the previously exported id, which can be a non-UUID string. We may, however, introduce a system property so that users can enforce UUIDs if they want to. Something like "keycloak.map_storage.reject_invalid_ids" which would be false by default, but can be turned on to make the above block throw an exception instead. Thoughts?

I think the configuration option would be nice. I agree with the proposed solution to have it by default set to false and throw an exception when set to true. On top of that I think we should add a warning which would alert a user that the id of realm is going to be changed.

Incorporated all these suggestions. For now I'm using a config property from the map storage, but it can be switched into something else, like a system property.

Sorry that I'm late for this discussion: For the LDAP store, all IDs are determined by the LDAP server as GUIDs, so all ids passed in by Keycloak will be replaced in the returned entity with a GUID.

The automatic conversion is therefore correct IMHO. Thinking about configuration, there having an automatic conversion is what is expected from a map store as it is allowed to treat any passed in ID as a mere suggestion.

I would even go so far to make it non-configurable and also to not log a warning unless you have a scenario in mind where the conversion could break an existing behavior.

That makes sense, @ahus1, thanks for sharing your thoughts. I've changed the code so no warning is printed and no exception is thrown, JPA store simply converts non-compliant ids into UUIDs.

I think the configuration option would be nice. I agree with the proposed solution to have it by default set to false and throw an exception when set to true. On top of that I think we should add a warning which would alert a user that the id of realm is going to be changed.

The JavaDoc states "Internal ID of the realm or null if one is to be created by the underlying store" for the id attribute. Please consider changing the JavaDoc to indicate that the storage might overwrite/change the ID at its own discretion.

Updated the javadoc.

+1 for the javadoc. Actually I don't agree with the throwing an exception, it should always create a new one.

Based on the other comments I'll remove the exception part.

Done, javadoc updated.

Just thinking a bit more about this validation/conversion of ids into UUIDs for the JPA storage.. using a config property from the mapStorage does feel a bit misplaced as other storage impls might not have this hard requirement of ids being UUIDs.

Perhaps we should have this rejectInvalidIds config in the jpa-map-storage instead and inject its value into the created transactions. The transaction's create method can then run the validateAndConvert method using the flag that signals if an exception should be thrown or if the invalid id should be simply converted into an UUID.

Thoughts?

If the UUIDs are a requirement for the map storage in general, then maybe the approach of using a config in mapStorage directly is not displaced at all.

Given the example of LDAP, the UUIDs are no requirement across all map storages. A store might decide to create IDs differently, and as long as they are a String they are fine to do so. Having a UUID might be default for Keycloak, and the only supported way for JPA, but this won't hold true for all map storages.

+1 to what @ahus1 wrote.

Also: can rejectInvalidIds ever be false? Wouldn't passing in an invalid ID result in an exception from the store later upon saving the object?

@ahus1 @hmlnarik Ok, so based on the discussion the conclusion is that the store has the freedom to convert ids at its own discretion, treating any supplied id as a suggestion. In that case, I should remove the id validation/conversion from the MapRealmProvider and MapRealmAdapter and move it to the JPA transaction create method. The reason being that converting at the logical layer is meaningless if other store, like LDAP, will have to convert it again.

In JPA, once the store is invoked to create an entity it can validate if the incoming id is a UUID and convert if needed. I'll also remove the exception and log message. Can I proceed this way?

+1

Done! All changes pushed based on the reviews. EDIT: still testing the best way to handle this, tx.create is not suitable for components as they are not created there but rather added to the realm-components relationship. Idea is to enforce the id conversion, if needed, in the JPA entities setId() method. Will update again once testing finishes.

This approach worked, got a clean testsuite run using this approach instead of adding things in the logical layer.

nitpick: formatting

Thanks!

It basically enforces that all realm map stores uses UUID format of ids, right? Do we want that? I thought individual implementations should decide what key format will be used.

Yeah, I had a few issues moving this into the JPA storage but I seem to have found a way to do it correctly. Once I finish testing I'll update.

Done

the same here, do we want to enforce UUIDs at provider level?

same thing, currently testing a different approach to put this into the JPA storage.

Why do we need to set the id to the representation?

We don't - prob the first test I've ran and I think I set it here to see if the failure would be fixed with a proper id set. Now that invalid ids are converted this is not needed, so I'll remove. Thanks for spotting this.

Shoudn't we also handle case COMPONENTS to avoid unnecessary additional query?

I think so, and the impl would very very similar to the attributes. I'll add it.

wondering if it should also be implemented for other areas as well ... if so perhaps we could move it into JpaMapKeycloakTransaction.create method, wdyt?

create wouldn't work for entities such as the ComponentEntity because its creation happens when it is added to the components relationship - i.e. components are not created via tx.create. I would indeed put this check in every entity's setId(String id) because this is always called, whether from the tx.create or when creating an entity by adding it to a relationship (such as components or auth sessions that are added to the root auth session).

Added the validation on setId to all other entities that have non-generated ids. For generated ids this method should never be called.