On 6/16/16 5:23 AM, Marek Posolda wrote:
On 15/06/16 17:33, Bill Burke wrote:
> This redesign is quite complex as you have to take a lot of variables
> into account. I also am trying to make things backward compatible and
> have the new SPI co-exist with the old. Just remember though, the
> biggest reason for this rewrite is to AVOID IMPORTING!
>
> Here's what I'm currently doing:
>
> Federated Data
> --------------
> There will be a built-in federated user data service. It exists to
> store additional data about a user, for example:
>
> public interface UserAttributeFederatedStorage {
> void setSingleAttribute(RealmModel realm, UserModel user, String
> name, String value);
> void setAttribute(RealmModel realm, UserModel user, String name,
> List<String> values);
> void removeAttribute(RealmModel realm, UserModel user, String name);
> MultivaluedHashMap<String, String> getAttributes(RealmModel realm,
> UserModel user);
> }
>
> There will be federated storage options for all data.
>
> User Storage Provider SPI
> -----------------
> Current UserProvider is going to be split up into smaller interface.
> SPI will remain the same though. There is going to be a
> UserStorageProvider SPI. Instances can implement one or more of the
> smaller UserProvider interfaces.
>
> User Storage Provider instances will be returning UserModel instances.
> This means providers will be responsible for implementing some sort of
> UserAdapter implementation. These custom UserAdapters will be
> responsible for interacting with the Federated Data Store and their
> custom storage: Merging data between i.e. LDAP and the Federated
> Storage, managing which store stores what data, managing what data
> lives where. Going to provide a helper abstract UserAdapter class
> that helps managing this data.
Ok. Am I understand correctly that "smaller" providers are not limited
to be either fully-LDAP based or fully-localDB based? For example I want
"attribute-a" of user "john" to be stored in LDAP, however
"attribute-b"
of same user will be stored in local DB. From what you wrote, I assume
it will work as UserAdapter can delegate to other store if it doesn't
find "attribute-b" in LDAP?
Adding again the scenario with requiredActions, which we currently
support and which we briefly discussed on F2F. I am adding it just to
ensure that we won't loose the support for this :)
- Password in MSAD is expired
- Keycloak reads some corresponding attribute from MSAD and it sets
UPDATE_PASSWORD requiredAction on the user based on it
- Keycloak asks user to update password and this is then propagated to MSAD
In other words, just the UPDATE_PASSWORD requiredAction is handled by
MSAD, but all the other required actions need to be handled by Keycloak
local DB.
Yes that will work. Because the provider is providing the UserAdapter,
it has control over how things are pulled together. I also intend to
eventually allow federated-federated-storage. So, you could lookup from
LDAP, but gather attributes/role-mappings/etc. from another LDAP store,
Keycloak, or other places.
>>
>> Persistent vs. Transient modes
>> ----------------------------------------
>>
>> I think we should still keep some support for "persistent" mode (the
>> case when user is imported to Keycloak local DB).
>>
>
> I want to get rid of import and only have it as an option for
> Brokering or for transient users (users that can't be looked up from a
> store out of band). The LDAP adapter will be responsible for knowing
> what data is stored in LDAP and what data is stored in the Federated
> Store, so there is no need to "import"
Ok, so LDAP data are not imported, however the non-LDAP data (or
overwritten data) are read from local store right? So we still support
the scenario with overwriting data (unsynced mode) like this?
- User from LDAP authenticates and his pasword is verified against LDAP
- User changes his password, but LDAP is read-only, so new password
needs to be written to local DB
- User authenticates again, but now his password needs to be verified
against local DB
We can provide some default behavior from the UserAdatper helper
abstract class, but the provider is accountable for merging data.
>>
>> Caching
>> -----------
>>
>> I hope new SPI will allow us more flexibility to support various
>> scenarios. It will be good to support at least those IMO:
>>
>
> See above. LDAP users will be cached in same way JPA Users currently
> are.
So if I understand correctly,we will loose the option (1), which we
currently support like:
LDAP user data are always read from "online" LDAP in every request, but
all other user data are cached, so no need to read them from local
Keycloak DB (even for many hours).
Yeah, no more on-demand. Either you cache or you don't.
In other words, we will have option to use different expiration
times
(ie. LDAP user is expired from cache every 5 minutes, when fully-local
user is expired every 2 hours), however you will need to read each 5
minutes all the data of user "john" from both LDAP and local-db, right?
However loosing this one is probably not so big issue....
The user cache will see one UserModel and load from there. So, it
caches LDAP and any other federated storage for the user that the
UserModel provides.
>
>> 3) I agree that will be cool to support different expiration time based
>> if it's LDAP user or just Keycloak-local user. Infinispan allows it
>> though with something like : cache.put("ldapuser", ldapUser, 60,
>> TimeUnit.SECONDS);
>>
>
> I think this will be an option that we will want to support.
>
>>
>>
>> Transactions and 3rd party updates
>> -----------------------------------------------
>>
>> - Will be good to improve registration of user to LDAP. Ideally during
>> registration new user to LDAP, we should allow to send all data at once.
>> (currently UserFederationProvider.register supports sending just
>> username). Also we should allow to specify if register to 3rd party
>> provider should be done *before* or *after* the registration to Keycloak
>> local DB. For details, see
https://issues.jboss.org/browse/KEYCLOAK-1075
>> and all the comments from users...
>>
>> - Also updating user should be ideally at once. For example if you call:
>> user.setFirstName("john");
>> user.setLastName("Doe");
>> user.setEmail("john(a)email.cz");
>>
>> we shouldn't have 3 update calls to LDAP, but just one. Maybe we can
>> address this with transaction abstraction? I've already did something
>> for LDAP provider (see TxAwareLDAPUserModelDelegate ), however will be
>> good to provide something more generic for user storage SPI. Then when
>> KeycloakTransactionManager.commit is called, the data are send to
>> federation storage
>>
>
> This would be an implementation detail. Similar to JPA Users, the
> LDAP UserAdapter will be remembered per session. The LDAP adapter
> could queue up updates then at commit time flush them all.
>
> Maybe you should consider writing an LDAP version of JPA? ;-)
> Probably a lot of code could be borrowed from PL IDM API.
I've wrote already something similar for Mongo ;) Also I already
addressed the multiple-updates issue for LDAP (see above). However this
one is mainly issue for 3rd party providers rather than for LDAP
provider. Especially since 3rd party provider may have some validations
for some attributes.
For example consider this scenario, which can happen in current
implementation:
UserModel john = session.users().addUser(realm, "john");
john.setEmail("john(a)email.org");
All of this happens during "addUser" call:
- User "john" is saved to local-db
- SomeUserFederationProvider.register calls the registration request to
3rd party federation storage. User "john" is saved in 3rd party storage
now.
Then john.setEmail is invoked
- There is request to 3rd party federation storage to update email.
- 3rd party storage refuse to save the email because there is already an
existing user with email "john(a)email.org" . So it sends back the
validation error. Now keycloak can rollback the transaction, however
user "john" was already registered in 3rd party federation storage from
previous "register" call -> FAIL
So now you need to manually remove the user from 3rd party storage,
which is dirty hack. There are other possible hacks to address this. For
example someone "postpone" the registration in "register" method by
adding some helper attributes and sends the registration request later
when all the attributes are set. I suggest to read the comments in
https://issues.jboss.org/browse/KEYCLOAK-1075 for details.
IMO the proper solution is to ensure that "register" request to 3rd
party is postponed until the transaction.commit when user is filled with
all the attributes. Similarly update should be done just once. IMO we
should provide something at SPI level to simplify this scenario.
We already have a KeycloakTransaction you can register with.
I understand that main motivation for you is to avoid
"imports", however
since we are refactoring anyway, we should try to address other SPI
limitations too. And IMO this one is pretty bad and worth improve ;)
So, have a method addUser(RealmModel realm, UserEntity user)?
>
>
>
>>
>> Sync users
>> --------------
>> We should still keep the option to sync users into Keycloak DB as we
>> have now. Note some persistent storages like LDAP are limited with
>> pagination. So the easiest possibility for some admins is just to sync
>> users, so they can easily search them in admin console.
>>
> Doing a full import just to support pagination is overkill. I'm
> guessing that a lot of deployments will not manage users through the
> Keycload admin console. We can offer a manual a Sync SPI that
> providers can implement.
Maybe it's overkill for 90% of deployments, but remaining 10% want to
see all LDAP users in admin console immediatelly and hence want to sync
them? IMO it's always good to have SPI flexible so it can easily support
all the possible requirements. However I understand that it's not always
possible...
This is only an issue for large query result sets where you want to do
pagination. IIRC, we couldn't figure out a way to do this in a scalable
manner without imports.