[keycloak-dev] Caching of identity provider links
Marek Posolda
mposolda at redhat.com
Tue Mar 22 16:43:10 EDT 2016
Thanks, merged to 1.9.x and will be soon backported to master too.
Marek
On 22/03/16 18:17, Bill Burke wrote:
> 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/cfdc1040/attachment-0001.html
More information about the keycloak-dev
mailing list