<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 23 June 2016 at 14:19, Marek Posolda <span dir="ltr">&lt;<a href="mailto:mposolda@redhat.com" target="_blank">mposolda@redhat.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000">
    <div>+1 on having &quot;invalidateProvider&quot;
      method.<br>
      <br>
      For the other stuff,  we already have the first 2 &quot;getProvider&quot;
      methods, so the new stuff will be the methods with &quot;String
      instanceId&quot; parameter, right?<br></div></div></blockquote><div><br></div><div>Yes, I just included the two existing methods to point out that they will still be there.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div>
      <br>
      We already discuss adding the &quot;String instanceId&quot; . Now when
      thinking more, it looks that it is not so convenient. <br>
      <br>
      When adding again UserFederation SPI as an example:<br>
      <br>
      - UserFederationProviderFactory needs UserFederationProviderModel
      to create instance of UserFederationProvider<br>
      - So factory needs to lookup model from cache/db. Hence the
      instanceId would need to be compound of something like:
      &lt;REALM-UUID&gt;::&lt;USER-FEDERATION-PROVIDER-MODEL-ID&gt;<br>
      That&#39;s because to lookup UserFederationProviderModel, you first
      need RealmModel and then find the UserFederationProviderModel by
      it&#39;s ID within the realm.<br>
      <br>
      You may admit that RealmModel is available on KeycloakContext.
      However I don&#39;t think that we can rely on it. KeycloakContext is
      available in REST requests, but in some other cases (ie.
      ExportImport, periodic tasks etc), it&#39;s not available. Caller
      usually have the RealmModel and he can manually set it to
      KeycloakContext before calling session.getProvider, however that
      doesn&#39;t look like good approach to me and should be rather
      avoided. So in shortcut, we shouldn&#39;t rely on realm being
      available in KeycloakContext IMO. <br></div></div></blockquote><div><br></div><div>Going forward we should rely on the realm being available in KeycloakContext IMO. The whole purpose of it is so we don&#39;t have to pass details around all the time, including the realm.</div><div><br></div><div>I see two options to it:</div><div><br></div><div>* Don&#39;t require the realm to load provider config. If instances ids are UUIDs this would work, but I don&#39;t think they always are right?</div><div>* Add RealmModel to the lookup, so it becomes:</div><div>  getProvider(Class&lt;T&gt; clazz, String providerId, RealModel realm, String instanceId)<br></div><div>  That would also require a <span style="color:rgb(80,0,80)">invalidateProviders(RealmModel realm) that can clear all provider instances for a specific realm</span><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div>
      <br>
      The logic for parse the &quot;instanceId&quot; and retrieve
      UserFederationProviderModel from DB would be boilerplate code same
      to all UserFederationProviderFactory impls.<br>
      <br>
      <br>
      With that in mind, it really seems to me that instead of &quot;String
      instanceId&quot;, it may work better to have some common configuration
      class like &quot;ProviderModel&quot; . Then signature will look like: <br>
      <br>
      * getProvider(Class&lt;T&gt; clazz, String providerId,
      ProviderModel  model)<br>
      <br>
      All the model subclasses (UserFederationProviderModel,
      IdentityProviderModel, PasswordPolicyModel ...) will be subclasses
      of ProviderModel<br></div></div></blockquote><div><br></div><div>I don&#39;t like that at all as it requires loading and retrieving the model to be able to get the instance. It should be the responsibility of the factory and provider framework to be able to do that, not the code that wants to use the provider.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div>
      <br>
      Marek<div><div class="h5"><br>
      <br>
      On 23/06/16 12:01, Stian Thorgersen wrote:<br>
    </div></div></div>
    <blockquote type="cite"><div><div class="h5">
      <div dir="ltr">Currently it&#39;s expected that the factory is
        application scoped, while provider instances are request scoped.
        Factories can if they want return the same instance for provider
        to make it application scoped.
        <div><br>
        </div>
        <div>This works as long as config is server-wide, but not if
          there are config per-realm or even multiple different
          instances per-realm. This applies to for example User
          Federation SPI (multiple per-realm), Password Hashing SPI (one
          per-realm), etc.</div>
        <div><br>
        </div>
        <div>Currently the User Federation SPI creates and manages
          instances outside of the session factory and session, which
          results in multiple instances created per-request, not all
          being closed properly, etc..</div>
        <div><br>
        </div>
        <div>With that in mind I&#39;d like to change the provider factories
          so that there can be multiple provider factory instances. It&#39;s
          not completely figured out, but I wanted to discuss it before
          I start a POC around it.</div>
        <div><br>
        </div>
        <div>We&#39;d have the following methods on KeycloakSession:</div>
        <div><br>
        </div>
        <div>* getProvider(Class&lt;T&gt; clazz, Provider.class) -
          returns default provider<br>
        </div>
        <div>* getProvider(Class&lt;T&gt; clazz, Provider.class, String
          providerId) - returns a specific provider, with the default
          config<br>
        </div>
        <div>* getProvider(Class&lt;T&gt; clazz, Provider.class, String
          providerId, String instanceId) - returns a specific provider,
          with the specific config<br>
        </div>
        <div><br>
        </div>
        <div>We&#39;d also add a method:</div>
        <div><br>
        </div>
        <div>* invalidateProvider(Class&lt;T&gt; clazz, Provider.class,
          String providerId, String instanceId) - this would be called
          when the config for a specific provider instance is updated</div>
        <div><br>
        </div>
        <div>Behind the covers the instances would be maintained. Each
          provider factory would internally be responsible to retrieve
          config and cache config for instances.</div>
        <div><br>
        </div>
        <div>Does this sound like an idea worth pursuing? I&#39;d like to
          try it out on the PasswordPolicy SPI first.</div>
      </div>
      <br>
      <fieldset></fieldset>
      <br>
      </div></div><pre>_______________________________________________
keycloak-dev mailing list
<a href="mailto:keycloak-dev@lists.jboss.org" target="_blank">keycloak-dev@lists.jboss.org</a>
<a href="https://lists.jboss.org/mailman/listinfo/keycloak-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/keycloak-dev</a></pre>
    </blockquote>
    <br>
  </div>

</blockquote></div><br></div></div>