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.

> * Modes for federation will be: READ_ONLY, SYNCED, and UNSYNCED. > SYNCED will update LDAP on demand. UNSYNCED will store changes locally > and require the user to handle synchronization themselves. Is it some difference between READ_ONLY and UNSYNCED? Wonder if 2 modes are sufficient for federation and users are free to choose if they handle synchronization or not? Am I understand correctly that this is just for sync from Keycloak back to LDAP?

> * Going to have an import-attributes on/off switch. A keycloak->ldap > attribute map will be required to be configured. If this switch is off, > UserModel proxy will load attributes on demand. > > > Questions: > * Is ExternalModelAuthProvider actually a feature requested by users? > I'd like to not have to do this. At least for 1.0. Probably not needed AFAIK. Sorry to repeat, but I still have some doubts especially about bulk search methods. Example of some corner cases: * If there are 2 federation providers and firstResult==0, maxResults==10 and each provider has 10 users, then due to this https://github.com/keycloak/keycloak/blob/master/model/api/src/main/java/... it may be 20 users in final list (even if maxResults is 10) * If there are 10 users "locally" and 10 completely different users in LDAP, and firstResult ==0, maxResults==10 then it will return 10 "local" users. Now you want next page so next query will be firstResult==10, maxResults==10 . This should return 10 users from LDAP, but actually it won't return any user as LDAP query will have firstResult==10, maxResults==10, but there are just 10 users in LDAP

* In some cases it may happen the opposite . Like you have user "john" in both LDAP and local store. Page1 will return 10 local users with "john" between them. Page2 will query LDAP and it may also return "john" between them (the case when "john" is on page1 in "local" store but on page2 on LDAP store) Right now, I am looking at changelog stuff, which is supported by ActiveDirectory but not by some other LDAP servers. Right now I am working with Keycloak on latest Picketlink master, which helped to remove ActiveDirectory workarounds needed in LDAPKeycloakCredentialHandler and KeycloakLDAPIdentityStore . I will also possibly send more PRs to picketlink with missing features regarding changelog and pagination .

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.

> 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.

Just to have things a bit more complicated, the LDAP doesn't support "classic" pagination model with offset/limit like JPA, but the pagination model with cookie/limit (each search returns "cookie", which can be used to search for next page). Some details here http://www.ietf.org/rfc/rfc2696.txt... It is not a problem for the Sync usecase though, as sync process will have full control and can process all the pages gradually. However it looks that it will affect FederationProvider though? I guess "Result" object you proposed would need to store cookie in some property as well. For picketlink I've added paginationContext to Picketlink IdentityQuery to handle this https://github.com/picketlink/picketlink/blob/master/modules/idm/api/src/... . Marek On 25.7.2014 15:43, Bill Burke wrote: > > > On 7/25/2014 3:20 AM, Marek Posolda wrote: >>> 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. >> > > Solved by searching for local users where federation link is null > only? The side effect is that the federation provider would also have > to check the database to make sure the user hasn't already been > imported. This could suck as 1 pagination query could turn into > MAX_RESULTS local storage queries

I am already on picketlink master and trying to address the issues we have. Right now mapping of dynamic attributes aka https://issues.jboss.org/browse/PLINK-533 . I don't know when exactly is picketlink release planned, but maybe we can ask Pedro to do at least timestamped release somewhen next week? I wonder that random searches with FederationProvider might work for implementations, which supports offset/limit (like external database provided by customer) with your idea of omit local users with federation link? There are more tricky things though, like if user "john" available in in both local and LDAP store, and then directly deleted in LDAP, won't be returned if queries even if he is still in Keycloak database. Not sure if it's good or bad as he may not be able to login anyway...

However for LDAP it's tricky... I've tested with ActiveDirectory that I can query with same cookie to random LDAP pages (next page or any page which I already previously visited), but looks that just next page is always guaranteed to work but previous pages are not guaranteed according to http://www.ietf.org/rfc/rfc2696.txt sentence: "Returning cookies from previous searchResultDone responses besides the last one is undefined, as the server implementation may restrict cookies from being reused."

On 25.7.2014 21:28, Bill Burke wrote: > > > On 7/25/2014 2:19 PM, Marek Posolda wrote: >> I am already on picketlink master and trying to address the issues we >> have. Right now mapping of dynamic attributes aka >> https://issues.jboss.org/browse/PLINK-533 . I don't know when exactly is >> picketlink release planned, but maybe we can ask Pedro to do at least >> timestamped release somewhen next week? >> >> I wonder that random searches with FederationProvider might work for >> implementations, which supports offset/limit (like external database >> provided by customer) with your idea of omit local users with federation >> link? There are more tricky things though, like if user "john" available >> in in both local and LDAP store, and then directly deleted in LDAP, >> won't be returned if queries even if he is still in Keycloak database. >> Not sure if it's good or bad as he may not be able to login anyway... >> > > Was going to change it in that if a returned UserModel is linked, to > check that it is still valid for the federation and delete it if it is > not. +1 > >> However for LDAP it's tricky... I've tested with ActiveDirectory that I >> can query with same cookie to random LDAP pages (next page or any page >> which I already previously visited), but looks that just next page is >> always guaranteed to work but previous pages are not guaranteed >> according to http://www.ietf.org/rfc/rfc2696.txt sentence: >> "Returning cookies from previous searchResultDone responses besides the >> last one is undefined, as the server implementation may restrict cookies >> from being reused." >> > > for now the federation interface for queries will be: > > UserModel getUserByUsername(RealmModel realm, String username) > UserModel getUserByEmail(RealmModel realm, String email) > List<UserModel> getUserByLastName((RealmModel realm, String last) > List<UserModel> getUserByFullName((RealmModel realm, String first, > String last); > > No pagination. No paginated getAllUsers() and thus, no sync API. If > you can figure out something reliable in the next week *AND* get us a > new Picketlink release, we can use it in 1.0.Final. Otherwise, it is > just going to have to wait I guess. +1 to skip pagination from FederationProvider. But I think that at least one way sync from LDAP to Keycloak database should be possible. We already have support for ScheduledTask, which allow to do periodic syncs. IMO the tricky parts are: * pagination --- It might be needed as for example with 1 million users in LDAP, you will need multiple transaction to sync them all into Keycloak database. Some LDAP servers supports it and some not (generally said some Sync providers may support but some may not. It must be possible to configure if particular Sync Provider supports it). Regarding pagination, note that it's quite an issue for federationProvider, but not for sync as sync process have full control when to begin/commit transactions and also have access to all needed info (like cookies from LDAP) * Changelogs --- More tricky, but at least Active Directory (which seems to be major LDAP vendor) has support for it. The tricky part (also in AD) seems to be tracking of removed users (those which were directly removed in LDAP). So from time to time, it may be always good to do full sync * Configuration in admin console - It should be possible to choose Sync provider and then to configure: - if pagination will be used or not - if changelogs (partial sync) will be used or not - if to use periodic sync - how often to do full sync, how often to do partial sync (if supported) etc... - Possibility to trigger full or partial sync directly from admin console etc.

Looks good. Only two comments from me: 1. FederationManager.getFederationProvider uses factories directly Why is this? This will cause the provider instance not to be registered with the session, so won't be closed automatically when the session is closed, nor will it be able to attach to the transaction.

2. TOTP SPI (just related) Once I've finished access code work I was going to start on TOTP SPI. I think a UserProvider should only be able to verify password credentials, and TOTP providers should be used to verify TOTP. I'll send a separate email about this tomorrow so we can discuss it in more detail, just a heads up.

----- Original Message ----- > From: "Bill Burke" <bburke(a)redhat.com&gt; > To: "Stian Thorgersen" <stian(a)redhat.com&gt; > Cc: keycloak-dev(a)lists.jboss.org > Sent: Thursday, 24 July, 2014 2:47:57 PM > Subject: Re: [keycloak-dev] federation commited need feedback > > > > On 7/24/2014 9:25 AM, Stian Thorgersen wrote: >> Looks good. Only two comments from me: >> >> 1. FederationManager.getFederationProvider uses factories directly >> >> Why is this? This will cause the provider instance not to be registered >> with the session, so won't be closed automatically when the session is >> closed, nor will it be able to attach to the transaction. >> > > FederationProviders are not singletons within KeycloakSession like > RealmProvider, UserProvider, and UserSessionProvider are. You can have > multiple FederationProvider instances per realm. > > > public interface FederationProviderFactory extends > ProviderFactory<FederationProvider> { > FederationProvider getInstance(KeycloakSession session, > FederationProviderModel model); > } > > > FederationProviders implementations have the option to enlist themselves > with the KeycloakSession transaction manager. LDAP (really Picketlink) > doesn't have the concept of a session or transaction, so it doesn't > enlist itself. > > I guess we do need a enlistForClose() method on KeycloakSession though. As long as the lifespan of a FederationProvider is the same as the KeycloakSession they should be created through the KeycloakSession IMO. We just need to add an additional createProvider that can create a instance tied to a specific realm.

----- Original Message ----- > From: "Bill Burke" <bburke(a)redhat.com&gt; > To: "Stian Thorgersen" <stian(a)redhat.com&gt; > Cc: keycloak-dev(a)lists.jboss.org > Sent: Thursday, 24 July, 2014 3:56:35 PM > Subject: Re: [keycloak-dev] federation commited need feedback > > > > On 7/24/2014 10:32 AM, Stian Thorgersen wrote: >> >> >> ----- Original Message ----- >>> From: "Bill Burke" <bburke(a)redhat.com&gt; >>> To: "Stian Thorgersen" <stian(a)redhat.com&gt; >>> Cc: keycloak-dev(a)lists.jboss.org >>> Sent: Thursday, 24 July, 2014 2:47:57 PM >>> Subject: Re: [keycloak-dev] federation commited need feedback >>> >>> >>> >>> On 7/24/2014 9:25 AM, Stian Thorgersen wrote: >>>> Looks good. Only two comments from me: >>>> >>>> 1. FederationManager.getFederationProvider uses factories directly >>>> >>>> Why is this? This will cause the provider instance not to be registered >>>> with the session, so won't be closed automatically when the session is >>>> closed, nor will it be able to attach to the transaction. >>>> >>> >>> FederationProviders are not singletons within KeycloakSession like >>> RealmProvider, UserProvider, and UserSessionProvider are. You can have >>> multiple FederationProvider instances per realm. >>> >>> >>> public interface FederationProviderFactory extends >>> ProviderFactory<FederationProvider> { >>> FederationProvider getInstance(KeycloakSession session, >>> FederationProviderModel model); >>> } >>> >>> >>> FederationProviders implementations have the option to enlist themselves >>> with the KeycloakSession transaction manager. LDAP (really Picketlink) >>> doesn't have the concept of a session or transaction, so it doesn't >>> enlist itself. >>> >>> I guess we do need a enlistForClose() method on KeycloakSession though. >> >> As long as the lifespan of a FederationProvider is the same as the >> KeycloakSession they should be created through the KeycloakSession IMO. We >> just need to add an additional createProvider that can create a instance >> tied to a specific realm. >> > > I disagree. Its really up to the implementation on whether the provider > is managed by KeycloakSession or not or whether its lifespan is tied to > KeycloakSession or not. > Why would a provider not be tied to a session? IMO there should always be a session.

> We already manage transactions in this manner. For example > JpaUserProvider does not enlist its own KeycloakTransaction and its > close() method is a no-op. CacheUserProvider and JpaConnectionProvider > enlist their own transaction objects manually. JpaConnectionProvider's > close() method actually does something. Commit transactions and closing providers are tied to the session, that's the whole point. With your code you're creating providers that are never closed.

> > -- > Bill Burke > JBoss, a division of Red Hat > http://bill.burkecentral.com >

>> Why would a provider not be tied to a session? IMO there should always be a >> > >session. >> > > > > > >It might be expensive to create the provider and/or the provider may > >have no concept of a session. For example, Picketlink PartitionManagers > >are not tied to a KeycloakSession That's exactly why a ProviderFactory has application scope. Anything that is expensive to create should be stored in the ProviderFactory and reused in Provider instances. For example JpaConnectionProviderFactory creates one EntityManagerFactory (which is expensive) and reuses it to create a EntityManager for each JpaConnectionProvider.

On 7/24/2014 1:42 PM, Marek Posolda wrote: > Right now what I am doing for export/import is creating session just for > retrieving ExportProvider or ImportProvider and then particular > ExportProvider or ImportProvider is starting it's own > session/transaction lifecycles whenever it needs them - > https://github.com/keycloak/keycloak/blob/master/export-import/export-imp... > > . It's kind of a hack IMO... > KeycloakSessionFactory now has: <T extends Provider> ProviderFactory<T> getProviderFactory(Class<T> clazz); <T extends Provider> ProviderFactory<T> getProviderFactory(Class<T> clazz, String id); So you could use them instead of creating a session.

Maybe there should be a separate FactoryFinder interface? If you want to pull those methods into a FactoryFinder interface I guess that could be done. Honestly, I don't understand why we need all these component layers for export/import. Seems a bit over-engineered in that regards, IMO, and now you want to add even more component layers to manage something that should be pretty straightforward.

On 7/23/2014 5:33 PM, Bill Burke wrote: > * Going to have an import-attributes on/off switch. A keycloak->ldap > attribute map will be required to be configured. If this switch is off, > UserModel proxy will load attributes on demand. I'm not going to do anything with attributes that doesn't already exist. Picketlink requires property mappings to actual properties on an actual class (User). Our LDAP federation will be a bit limited :( Hopefully what we have is good enough. We can look at improving this after 1.0.Final. Honestly I'd just like to write our own LDAP abstraction. Once users start wanting to deal with claims, there's going to be some stored in LDAP some stored in our store. Picketlink just can handle this scenario dynamically. Everything must be statically defined in a Java class and mapped with annotations.