Repository navigation
Conversation
1e4c58b to
f58d234
Compare
| @Override | ||
| public UserModel getUserByUsername(RealmModel realm, String username) { | ||
| logger.tracev("getUserByUsername: {0}", username); | ||
| //TODO remove following line? |
There was a problem hiding this comment.
Good question. I'd think we shouldn't change legacy behavior.
There was a problem hiding this comment.
No, we should keep it. It is the storage responsibility to deal with letter case. In the case of legacy storage, lettercase is intentionally ignored.
|
|
||
| public Scope getStorageConfigScope() { | ||
| return storageConfigScope; | ||
| } |
There was a problem hiding this comment.
I'm not sure this is the correct scope we would want to use. This only contains the storage config that is found inside the particular areas (client, group, etc). The final storage factory that is instantiated actually gets a scope that is broader than this - it combines the this area-specific config with the root config found in the mapStorage configuration (i.e. it falls back to the root config if a property is not defined in the area config). Also, if we go forward with multiple profiles for a particular storage, that would have to be taken into consideration as well.
So wouldn't we want to use the actual scope that was used to init the storage factory instead?
There was a problem hiding this comment.
@sguilhen @vramik This scope is the right one for the storage config, the options inherited from the root (and perhaps profile) are available transparently from this scope. See ComponentModelScope. However, storage maintains the capability of the storage to respect letter case. Whether the lettercase should be respected if the store supports it is however setting on the realm, so it has to be a property of the map user provider.
@vramik Factory should translate the configuration Scope values into properties and the objects created by the factory should not be given access to the Scope instance unless justified.
The reason is to keep the configuration management maintained in a single place (factory), and translated once (in the factory) rather than multiple times (in each created instance).
It seems that the only reason for this method is the eventually be searched for username-lettercase option. This by itself is not sufficient justification. Is there any other?
| @@ -0,0 +1,75 @@ | |||
| /* | |||
| * Copyright 2020 Red Hat, Inc. and/or its affiliates | |||
|
|
||
| private final Scope config; | ||
|
|
||
| public UserConcurrentHashMapStorage(Class<UserModel> modelClass, StringKeyConvertor<K> keyConvertor, DeepCloner cloner, Scope config) { |
There was a problem hiding this comment.
Perhaps we should only propagate the one property that this storage needs access to instead of the whole scope object.
| DefaultModelCriteria<UserModel> mcb = criteria(); | ||
| mcb = mcb.compare(SearchableFields.REALM_ID, Operator.EQ, realm.getId()) | ||
| .compare(SearchableFields.EMAIL, Operator.EQ, email); | ||
| .compare(SearchableFields.EMAIL, Operator.EQ, email.toLowerCase()); |
There was a problem hiding this comment.
In UserLookupProvider bellow the javadoc now says " * @PARAM email email address (case-sensitivity is controlled by storage)". This line is doing the opposite, isn't it?
There was a problem hiding this comment.
Thank you for the PR. It is a great start.
The ability to ignore the letter case in a particular realm is given by two things:
- Ability of the store to distinguish letter case (feature of the store, say LDAP might be ignoring letter-case while JPA not)
- Option on realm which instructs a particular user provider to ignore or respect letter case for name lookups
I believe that most of the changes should only be in the MapUserProvider to handle the flag, and depending on it either use username directly as is, or (say) lowercase it prior to using it in criteria.
Also please see inline.
| @Override | ||
| public UserModel getUserByUsername(RealmModel realm, String username) { | ||
| logger.tracev("getUserByUsername: {0}", username); | ||
| //TODO remove following line? |
There was a problem hiding this comment.
No, we should keep it. It is the storage responsibility to deal with letter case. In the case of legacy storage, lettercase is intentionally ignored.
|
|
||
| public Scope getStorageConfigScope() { | ||
| return storageConfigScope; | ||
| } |
There was a problem hiding this comment.
@sguilhen @vramik This scope is the right one for the storage config, the options inherited from the root (and perhaps profile) are available transparently from this scope. See ComponentModelScope. However, storage maintains the capability of the storage to respect letter case. Whether the lettercase should be respected if the store supports it is however setting on the realm, so it has to be a property of the map user provider.
@vramik Factory should translate the configuration Scope values into properties and the objects created by the factory should not be given access to the Scope instance unless justified.
The reason is to keep the configuration management maintained in a single place (factory), and translated once (in the factory) rather than multiple times (in each created instance).
It seems that the only reason for this method is the eventually be searched for username-lettercase option. This by itself is not sufficient justification. Is there any other?
Thanks @hmlnarik . I have some questions:
|
Yes
It can as long as there are no colliding usernames at the time of the switch. We do not need to support that check from the day 1, a warning to the log would suffice for the start.
It might or might not, depending on whether the username is cached in JPA or only loaded from LDAP. This is generally a problematic area, and it will be responsibility of tree store to log a warning about potential consequences of such misconfiguration. |
|
replaced by #13183 |
…ance
Closes #10245
Submitting WIP pull request. Currently it contains only implementation for
concurrenthashmap.I'd like to get opinion whether we can use this approach before going ahead with it. Few notes:
Config.Scopeis passed fromConcurrentHashMapStorageProviderFactorytoMapModelCriteriaBuilderfor querying and toUserConcurrentHashMapStoragefor persisting a user ... should it be rather different object? E.g.Map<String, Object> options?UserCacheSessionunchanged?UserStorageManagerunchanged?