<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Thanks, merged to 1.9.x and will be
soon backported to master too.<br>
<br>
Marek<br>
<br>
On 22/03/16 18:17, Bill Burke wrote:<br>
</div>
<blockquote cite="mid:56F17E3D.1050705@redhat.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
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.<br>
<br>
<div class="moz-cite-prefix">On 3/22/2016 12:50 PM, Stian
Thorgersen wrote:<br>
</div>
<blockquote
cite="mid:CAJgngAeBbyWapbZAob4BhXFRzokTGoFht8qE_qzc9sbJ87RtZg@mail.gmail.com"
type="cite">
<p dir="ltr">Ok, if Bill reviews and is happy to include in
1.9.x let's do it.</p>
<div class="gmail_quote">On 22 Mar 2016 16:29, "Marek Posolda"
<<a moz-do-not-send="true"
href="mailto:mposolda@redhat.com">mposolda@redhat.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>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. <br>
<br>
If you think missing caching is acceptable from
performance perspective etc, I can merge just to master
and avoid 1.9.x.<br>
<br>
Marek<br>
<br>
On 22/03/16 17:18, Stian Thorgersen wrote:<br>
</div>
<blockquote type="cite">
<p dir="ltr">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.</p>
<div class="gmail_quote">On 22 Mar 2016 15:36, "Marek
Posolda" <<a moz-do-not-send="true"
href="mailto:mposolda@redhat.com" target="_blank">mposolda@redhat.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">On
22/03/16 15:18, Bill Burke wrote:<br>
> Some comments on code:<br>
><br>
> * Why have a special flag in invalidateUser?
Just invalidate the links<br>
> if they exist. Aggressive eviction isn't a bad
thing, is it?<br>
The only reason was performance. The flag "false" is
used when you're<br>
adding new user. But this new user is not yet in
cache, so calling<br>
"getFederatedIdentities" will always need to send DB
query to get all<br>
links of user. But when you're adding new user, you
know that this user<br>
doesn't yet have any social links. So in the end,
without the flag there<br>
will be one more DB query, which will always return
empty set.<br>
<br>
Currently we don't support store/load of social
links from federation<br>
provider. But maybe in the future, if we support it
and we chain cache<br>
on top of federation storage, it could theoretically
happen that new<br>
user have social links at the moment when he is
registered. So I can do<br>
what you proposed as it seems to be a bit safer for
the future.<br>
<br>
Marek<br>
><br>
> On a side note, cache needs more refactoring,
we're reusing the same<br>
> pattern over and over. Wonder if functions can
help.<br>
><br>
><br>
> On 3/22/2016 5:42 AM, Marek Posolda wrote:<br>
>> Until now, we don't have support for
caching of identityProvider<br>
>> (social) links. So every social login or
every going to "federated<br>
>> identities" tab in account management needs
to send DB queries. I was<br>
>> looking at fixing it and I've send PR<br>
>> <a moz-do-not-send="true"
href="https://github.com/keycloak/keycloak/pull/2404"
rel="noreferrer" target="_blank">https://github.com/keycloak/keycloak/pull/2404</a>
. It turned to be a bit<br>
>> tricky because of:<br>
>><br>
>> 1) Caching needs to be done on both
directions. For social login, you<br>
>> need to lookup user by social link. But on
the other hand, you also need<br>
>> to look all social/identityProvider links
of particular user when you go<br>
>> to account management etc.<br>
>><br>
>> 2) Because of "store token" option, the
link may need to be updated in<br>
>> DB quite often (in theory even during each
social login).<br>
>><br>
>> I was thinking that storing links directly
on CachedUser doesn't work<br>
>> very well, because during each update of
social link (which may be often<br>
>> because of "store token") the user would
need to be fully invalidated<br>
>> from cache. Having separate cache entry for
each social link also has<br>
>> some downsides (many items in cache, need
to have separate entry for<br>
>> store the all links of user anyway). So I
ended up with having the cache<br>
>> entry, which contains list of all links of
particular user. It needs to<br>
>> be updated when any social link is
added,removed or updated. This seemed<br>
>> to me like good compromise. WDYT?<br>
>><br>
>> There are also entries for lookup user by
federated identity, so you<br>
>> don't need to query DB during social login.<br>
>><br>
>> So ATM there are not DB queries during
social login or during go to<br>
>> "federated identities" . I hope I handled
all corner cases and<br>
>> invalidations correctly, but if someone
want to take look it will be<br>
>> good. I don't want to add new regressions
atm :-)<br>
>><br>
>> Marek<br>
>><br>
>><br>
>><br>
>><br>
>><br>
>><br>
>>
_______________________________________________<br>
>> keycloak-dev mailing list<br>
>> <a moz-do-not-send="true"
href="mailto:keycloak-dev@lists.jboss.org"
target="_blank">keycloak-dev@lists.jboss.org</a><br>
>> <a moz-do-not-send="true"
href="https://lists.jboss.org/mailman/listinfo/keycloak-dev"
rel="noreferrer" target="_blank">https://lists.jboss.org/mailman/listinfo/keycloak-dev</a><br>
<br>
_______________________________________________<br>
keycloak-dev mailing list<br>
<a moz-do-not-send="true"
href="mailto:keycloak-dev@lists.jboss.org"
target="_blank">keycloak-dev@lists.jboss.org</a><br>
<a moz-do-not-send="true"
href="https://lists.jboss.org/mailman/listinfo/keycloak-dev"
rel="noreferrer" target="_blank">https://lists.jboss.org/mailman/listinfo/keycloak-dev</a><br>
</blockquote>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Bill Burke
JBoss, a division of Red Hat
<a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://bill.burkecentral.com">http://bill.burkecentral.com</a></pre>
</blockquote>
<br>
</body>
</html>