[keycloak-dev] Progress on LDAP enhancements

Tue May 26 13:03:18 EDT 2015

On 26.5.2015 08:46, Stian Thorgersen wrote:
>
> ----- Original Message -----
>> From: "Marek Posolda" <mposolda at redhat.com>
>> To: "Stian Thorgersen" <stian at redhat.com>
>> Cc: keycloak-dev at lists.jboss.org
>> Sent: Friday, 22 May, 2015 9:43:51 PM
>> Subject: Re: [keycloak-dev] Progress on LDAP enhancements
>>
>> On 22.5.2015 13:25, Stian Thorgersen wrote:
>>> ----- Original Message -----
>>>> From: "Marek Posolda" <mposolda at redhat.com>
>>>> To: keycloak-dev at lists.jboss.org
>>>> Sent: Wednesday, 20 May, 2015 11:33:16 PM
>>>> Subject: [keycloak-dev] Progress on LDAP enhancements
>>>>
>>>> I think I am finished with step 1 prototype on LDAP enhancements. Right
>>>> now it's just in my local fork
>>>> https://github.com/mposolda/keycloak/tree/ldap , I can't send PR as
>>>> model and admin console are kind of broken in my fork (ATM I am testing
>>>> with some hardcoded configs).
>>>>
>>>> What I did so far:
>>>>
>>>> - Refactoring and simplifying of forked picketlink LDAP code and removed
>>>> some abstractions to make it easier to use
>>>>
>>>> - Added UserFederationMapper SPI. The plan is to be in line with already
>>>> existing ProtocolMappers and IdentityProviderMappers.
>>>>
>>>> - Added LDAPFederationMapper. This mapper is invoked from
>>>> LDAPFederationProvider and provides some callbacks and extension points.
>>>> For more details, see methods and javadoc:
>>>> https://github.com/mposolda/keycloak/blob/ldap/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/LDAPFederationMapper.java
>>>> . I have 3 implementations right now:
>>>> -- UserAttributeLDAPFederationMapper - This is used to map LDAP
>>>> attributes to UserModel attributes/properties .
>>>> -- FullNameLDAPFederationMapper - Many LDAP deployments store fullName
>>>> of user in single LDAP attribute (usually "cn" attribute). So this
>>>> allows to map fullName from LDAP to firstName and lastName on UserModel
>>>> -- RoleLDAPFederationMapper - allows to map LDAP roles and user role
>>>> mappings from some DN in LDAP tree to either realm roles or client roles
>>>> of some client. Of course it's not a problem to plug more mappers, so
>>>> it's not an issue to map for example LDAP roles from tree
>>>> "ou=finance,dc=myorg,dc=org" to client roles of client "finance" and
>>>> roles from tree "ou=development,dc=myorg,dc=org" to client roles of
>>>> client "development" etc.
>>>>
>>>> - Minor refactoring of some methods on UserFederationProvider interface.
>>>> I needed to add RealmModel as parameter to some methods (it was already
>>>> in most of them) because RealmModel will be used to retrieve list of
>>>> configured UserFederationMapperModels .
>>>>
>>>>
>>>> Next planned steps:
>>>> - Model - I plan to add CRUD for UserFederationMapperModel to RealmModel
>>>> (similarly like IdentityProviderMapperModel)
>>>>
>>>> - Admin console - I've been thinking about new tab "Mappers" under "User
>>>> Federation". The tab will be active when you have some Federation
>>>> provider chosen and it will allow to add/update/remove mappers for
>>>> particular provider. Again something very similar to already existing
>>>> broker mappers. WDYT?
>>> +1
>>>
>>>> - Address remaining subtasks of
>>>> https://issues.jboss.org/browse/KEYCLOAK-886 . Not everything will be
>>>> addressed by mappers. For example there is performance issue
>>>> https://issues.jboss.org/browse/KEYCLOAK-838 . I am thinking about
>>>> adding some caching at LDAPIdentityStore level, but also I am thinking
>>>> about some simple per-request caching at UserFederationManager level,
>>>> which might help with federation performance in general (not just
>>>> specific to LDAP). Maybe combination of both? But I am not sure atm...
>>>>
>>>> - DB migration
>>>>
>>>> Question: I wonder if we can put RealmModel accessible from
>>>> UserFederationProviderModel? It shouldn't be a problem from
>>>> implementation perspective as UserFederationProviderModel CRUD methods
>>>> are on RealmModel, so realm can just inject itself before return
>>>> UserFederationProviderModel. Since UserFederationProvider instances are
>>>> created by method "getInstance" on UserFederationProviderFactory, which
>>>> has UserFederationProviderModel as one of parameters, it will make
>>>> RealmModel accessible in UserFederationProvider and hence it won't be
>>>> needed to have RealmModel in signature of methods on
>>>> UserFederationProvider.
>>>>
>>>> WDYT?
>>> Realm is already available from KeycloakContext /
>>> session.getContext().getRealm()
>> Yeah, however this is not available in all cases (for example during
>> tests or admin requests). And you can't be sure that RealmModel from the
>> context is the RealmModel corresponding to your
>> UserFederationProviderModel.
> For tests it can be fixed.
How about admin requests? Currently both "realm" and "client" are null 
in session.getContext() . Should we set realm to context in 
RealmsAdminResource.getRealmAdmin ? And similarly client before 
ClientResource is invoked?

It's a bit tricky as admin can be authenticated to realm "master" but he 
is administrating realm "foo", but it seems to me that it's fine to set 
the realm, which he is administrating (sending admin requests too) 
rather than the realm where he is authenticated.
> Why can't you be sure it's the same realm?
I mean for example if you have code like this:

RealmModel masterRealm = session.realms().getRealmByName("master");
UserModel admin = session.users().getUserByUsername("admin", masterRealm);

RealmModel fooRealm = session.realms().getRealmByName("foo");
UserModel foo = session.users().getUserByUsername("foo", fooRealm);

and both realms are configured to use federation and realm "foo" is set 
in the context. Then when calling the second line:

session.users().getUserByUsername("admin", masterRealm)

the UserFederationProvider will use incorrect realm "foo" from the 
context when it should use realm "master" .

It seems the proper behaviour is, that UserFederationManager will just 
use the realm send to the "getUserByUsername" (or any other method) as 
argument and pass same realm instance to the UserFederationProvider as 
argument. Is it clearer what I mean?

Marek
>> I think I will keep it as it is for now, also due to some other possible
>> issues. After thinking more it seems that adding RealmModel as variable
>> to UserFederationProviderModel is not very good idea...
>>
>> Marek
>>>> Marek
>>>> _______________________________________________
>>>> keycloak-dev mailing list
>>>> keycloak-dev at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>>
>>