[keycloak-dev] federation commited need feedback

[Marek Posolda]

On 24.7.2014 20:59, Bill Burke wrote:
>
>
> On 7/24/2014 1:51 PM, Marek Posolda wrote:
>> On 24.7.2014 15:21, Bill Burke wrote:
>>>
>>>
>>> On 7/24/2014 5:31 AM, Marek Posolda wrote:
>>>> Great stuff, some feedback inline
>>>>
>>>> On 23.7.2014 23:33, Bill Burke wrote:
>>>>> First iteration is commited.  I still have a lot to do.
>>>>>
>>>>> * AuthenticationProvider currently co-exists with Federation. I will
>>>>> delete it after the review of FederationProvider.
>>>>> * UserModel is proxied.  Some updates delegated to LDAP. Need to
>>>>> expand.
>>>>> * Still need to do admin console UI for federation
>>>>> * Still need to implement search and other queries for LDAP
>>>>> * Still need to test disjoint credential type storage.
>>>>>
>>>>> Feedback on unimplemented features for LDAP:
>>>>> * registration supported switch.
>>>>> * Importing username and email will be required. Everything else will
>>>>> be optional.  That cool?
>>>> yeah. Not directly related but I wonder if federationLink on UserModel
>>>> is sufficient? For example if user "john" is deleted in LDAP and then
>>>> user with same username "john" is added again to LDAP, it's defacto
>>>> different user but the Keycloak user "john" will be linked to the 
>>>> "new"
>>>> LDAP john. Hence AuthLinkModel had link to provider and also uuid of
>>>> user from LDAP.
>>>
>>> This is an implementation detail and shouldn't really be exposed
>>> through the Model API.
>> Ok, but how you would handle the case when user "john" is deleted in
>> LDAP and then different user with same username "john" created again?
>> Current LDAPFederationProvider will treat them in Keycloak as same user
>> even if they are not.
>
> So, you're saying we're supposed to check and verify with the LDAP 
> server every time the UserModel is referenced to make sure the uid 
> hasn't changed since the last time we accessed it? 
I would say yes if we want to do it properly? I am not sure how big 
issue is it, fact is that if someone with admin access to LDAP server 
(and without admin access to Keycloak) can delete user and then recreate 
same user in LDAP just with different password, he would then be able to 
login to Keycloak with this new password. But maybe it's extreme case? I 
am not sure...

> In your current implementation you only do this when validating 
> password or updating the credential.
yep, as AuthenticationProvider was supposed to handle just 
validating/updating credentials or just initially registering users . If 
UserModel.setFirstName etc. is called, the UserModel is updated just 
locally. It was supposed to be sync SPI to handle periodic updates 
to/from LDAP.
>
> FYI, if this happens with the AuthenticationProvider, that particular 
> user is hosed and can never log in again.  The user needs to be 
> deleted if this occurs (probably in a separate transaction).
yes, also the case when user is just deleted directly in LDAP (not 
through Keycloak) could happen IMO as for example Windows domain admins 
are supposed to edit ActiveDirectory LDAP directly IMO. In this case 
linked Keycloak user should be likely deleted? I've noticed that you are 
throwing IllegalStateException in LDAPUserModelDelegate... Maybe it's 
sufficient to have it like this and handle direct LDAP removals just 
with periodic sync?

I don't know, there are quite many corner cases here:-)
>
>
>>> I haven't actually tested or even executed searches yet.  Thanks for
>>> pointing outthe bugs.
>> I don't know if bug is correct word. IMO Federation+pagination can't
>> never properly work (without syncing all LDAP users into local storage
>> first). I would be happy if I am wrong:-)
>>
>
> You're right.  The API would have to change to note the provider that 
> was last used and how many were consumed for that provider.
>
> class Result {
>
>    List<UserModel> results;
>
>    String lastProvider;
>    int lastIndex;
>
> }
>
> then UserProvider search would need these methods:
>
> Result search(criteria..., int maxResults);  // start from beginning
> Result search(criteria..., String lastProvider, int lastIndex, int 
> maxResults);
Sorry, I still have doubts;-)

For example there are 10 users in Keycloak and just 5 of them are mapped 
to LDAP. In LDAP there are just those 5 users. Then you want to search 
for page1 with (lastIndex 0, maxResults 10) and you will retrieve those 
10 Keycloak Users. Then you want page2, so you call (lastIndex 10, 
maxResults 10) and now you retrieve those 5 users from LDAP, but those 
are same users, which were already retrieved on page1.

Marek
>
> As a side effect of this, I don't think FederationProvider extending 
> UserProvider is going to work anymore. There were already some add 
> methods that didn't make sense and would never be called.  Now 
> pagination needs a different API between UserProvider and 
> FederationProvider.
>