[keycloak-dev] Caching of identity provider links

[Bill Burke]

I had already reviewed it.  Adding another thing doesn't make the cache 
more complex as the same pattern/algorithm is pretty much repeated for 
each cached piece of data.  The risk is that we missed an edge case.

On 3/22/2016 12:50 PM, Stian Thorgersen wrote:
>
> Ok, if Bill reviews and is happy to include in 1.9.x let's do it.
>
> On 22 Mar 2016 16:29, "Marek Posolda" <mposolda at redhat.com 
> <mailto:mposolda at redhat.com>> wrote:
>
>     Yeah, there might be some risk of regression. On the other hand,
>     there are DB queries during each social login or every time when
>     you click on "federated identities" in account management.
>
>     If you think missing caching is acceptable from performance
>     perspective etc, I can merge just to master and avoid 1.9.x.
>
>     Marek
>
>     On 22/03/16 17:18, Stian Thorgersen wrote:
>>
>>     Is this really critical for 1.9.x? I'm worried about us intruding
>>     more complexity so late and I'd rather postpone to 2.x.
>>
>>     On 22 Mar 2016 15:36, "Marek Posolda" <mposolda at redhat.com
>>     <mailto:mposolda at redhat.com>> wrote:
>>
>>         On 22/03/16 15:18, Bill Burke wrote:
>>         > Some comments on code:
>>         >
>>         > * Why have a special flag in invalidateUser? Just
>>         invalidate the links
>>         > if they exist.  Aggressive eviction isn't a bad thing, is it?
>>         The only reason was performance. The flag "false" is used
>>         when you're
>>         adding new user. But this new user is not yet in cache, so
>>         calling
>>         "getFederatedIdentities" will always need to send DB query to
>>         get all
>>         links of user. But when you're adding new user, you know that
>>         this user
>>         doesn't yet have any social links. So in the end, without the
>>         flag there
>>         will be one more DB query, which will always return empty set.
>>
>>         Currently we don't support store/load of social links from
>>         federation
>>         provider. But maybe in the future, if we support it and we
>>         chain cache
>>         on top of federation storage, it could theoretically happen
>>         that new
>>         user have social links at the moment when he is registered.
>>         So I can do
>>         what you proposed as it seems to be a bit safer for the future.
>>
>>         Marek
>>         >
>>         > On a side note, cache needs more refactoring, we're reusing
>>         the same
>>         > pattern over and over.  Wonder if functions can help.
>>         >
>>         >
>>         > On 3/22/2016 5:42 AM, Marek Posolda wrote:
>>         >> Until now, we don't have support for caching of
>>         identityProvider
>>         >> (social) links. So every social login or every going to
>>         "federated
>>         >> identities" tab in account management needs to send DB
>>         queries. I was
>>         >> looking at fixing it and I've send PR
>>         >> https://github.com/keycloak/keycloak/pull/2404 . It turned
>>         to be a bit
>>         >> tricky because of:
>>         >>
>>         >> 1) Caching needs to be done on both directions. For social
>>         login, you
>>         >> need to lookup user by social link. But on the other hand,
>>         you also need
>>         >> to look all social/identityProvider links of particular
>>         user when you go
>>         >> to account management etc.
>>         >>
>>         >> 2) Because of "store token" option, the link may need to
>>         be updated in
>>         >> DB quite often (in theory even during each social login).
>>         >>
>>         >> I was thinking that storing links directly on CachedUser
>>         doesn't work
>>         >> very well, because during each update of social link
>>         (which may be often
>>         >> because of "store token") the user would need to be fully
>>         invalidated
>>         >> from cache. Having separate cache entry for each social
>>         link also has
>>         >> some downsides (many items in cache, need to have separate
>>         entry for
>>         >> store the all links of user anyway). So I ended up with
>>         having the cache
>>         >> entry, which contains list of all links of particular
>>         user. It needs to
>>         >> be updated when any social link is added,removed or
>>         updated. This seemed
>>         >> to me like good compromise. WDYT?
>>         >>
>>         >> There are also entries for lookup user by federated
>>         identity, so you
>>         >> don't need to query DB during social login.
>>         >>
>>         >> So ATM there are not DB queries during social login or
>>         during go to
>>         >> "federated identities" . I hope I handled all corner cases and
>>         >> invalidations correctly, but if someone want to take look
>>         it will be
>>         >> good. I don't want to add new regressions atm :-)
>>         >>
>>         >> Marek
>>         >>
>>         >>
>>         >>
>>         >>
>>         >>
>>         >>
>>         >> _______________________________________________
>>         >> keycloak-dev mailing list
>>         >> keycloak-dev at lists.jboss.org
>>         <mailto:keycloak-dev at lists.jboss.org>
>>         >> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>>         _______________________________________________
>>         keycloak-dev mailing list
>>         keycloak-dev at lists.jboss.org
>>         <mailto:keycloak-dev at lists.jboss.org>
>>         https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/keycloak-dev/attachments/20160322/fd7bbc55/attachment.html