<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">On 29/04/16 12:42, Stian Thorgersen
      wrote:<br>
    </div>
    <blockquote
cite="mid:CAJgngAeo9_ykym7PG-yxG3gaW74fgrrLaNpx8NhR3sRCc1iDBw@mail.gmail.com"
      type="cite">
      <div dir="ltr"><br>
        <div class="gmail_extra"><br>
          <div class="gmail_quote">On 29 April 2016 at 11:48, Marek
            Posolda <span dir="ltr">&lt;<a moz-do-not-send="true"
                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"><span class="">
                  <div>On 29/04/16 11:39, Marek Posolda wrote:<br>
                  </div>
                  <blockquote type="cite">
                    <div>On 29/04/16 11:06, Stian Thorgersen wrote:<br>
                    </div>
                    <blockquote type="cite">
                      <div dir="ltr"><br>
                        <div class="gmail_extra"><br>
                          <div class="gmail_quote">On 29 April 2016 at
                            10:58, Marek Posolda <span dir="ltr">&lt;<a
                                moz-do-not-send="true"
                                href="mailto:mposolda@redhat.com"
                                target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:mposolda@redhat.com">mposolda@redhat.com</a></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"><span>
                                  <div>On 29/04/16 10:22, Stian
                                    Thorgersen wrote:<br>
                                  </div>
                                  <blockquote type="cite">
                                    <p dir="ltr">We have 3 types of
                                      providers:</p>
                                    <p dir="ltr">* Server configured -
                                      no config or config from
                                      keycloak-server<br>
                                      * Realm configured - config from
                                      realm model<br>
                                      * Instance configured - multiple
                                      instances per realm</p>
                                    <p dir="ltr">Removing master realm
                                      as we plan to do means that realm
                                      configured provider factories can
                                      get realm from KeycloakContext as
                                      there's only one realm
                                      per-session.</p>
                                  </blockquote>
                                </span> In theory yes. In practice there
                                might be still corner cases when you
                                need to deal with multiple realms inside
                                same KeycloakSession (like export/import
                                for example). But hope we can handle
                                most of the cases by assume that
                                KeycloakContext has correct realm set.</div>
                            </blockquote>
                            <div><br>
                            </div>
                            <div>Corner cases like that is easy - we'd
                              use create a KeycloakSession per-realm,
                              making sure KeycloakContext is initialized
                              properly.</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"><span><br>
                                  <blockquote type="cite">
                                    <p dir="ltr">For instance configured
                                      I propose we add getProvider(Class
                                      c, String id, String instanceId)
                                      to provider factory. The it's up
                                      to the provider factory itself to
                                      extract  the config from the realm
                                      model or another source. It also
                                      means that the session can easily
                                      keep track of these and only
                                      create one id+instanceId per
                                      session.</p>
                                  </blockquote>
                                </span> ah, ok. I somehow missed the
                                proposal. <br>
                                <br>
                                It should work fine, I think it's quite
                                similar to what I proposed. Despite I
                                proposed to send whole state to provider
                                factory (aka.
                                UserFederationProviderModel) instead of
                                just instanceId and then assume that
                                state must properly implement "hashCode"
                                to ensure that session can keep track of
                                these and return provider of already
                                used state.</div>
                            </blockquote>
                            <div><br>
                            </div>
                            <div>Yup, very similar, but I think the
                              devil is in the details. In my proposal
                              the factory itself knows how to extract
                              the state, so it's then up to the factory
                              to decide how state should be stored. A
                              custom provider may need to store config
                              in a separate custom entity, which
                              KeycloakSessionFactory wouldn't know how
                              to retrieve.</div>
                          </div>
                        </div>
                      </div>
                    </blockquote>
                    Well, for custom SPI providers, you can simply use
                    "String" as the state type. Defacto I see the only
                    difference between proposals, that yours is simpler
                    as it's just always using "String" as state type
                    instead of having type dynamic. <br>
                    <br>
                    <br>
                    I am not saying it's big issue though. For example
                    UserFederationManager now already have all
                    UserFederationProviderModel instances configured for
                    realm, so with yours, you will need to call:<br>
                    <br>
                    session.getProvider(UserFederationProvider.class,
                    "ldap", providerModel.getId());<br>
                    <br>
                    and session will need to load
                    UserFederationProviderModel again from realm as it
                    knows just id. But since it's supposed to be cached,
                    there is no additional performance penalty in
                    loading UserFederationProviderModel again.<br>
                  </blockquote>
                </span> Well <span><span> ;-)   </span></span><br>
                <br>
                But on the other hand with simpler proposal... All
                UserFederationProviderFactory implementations provided
                by people will always need to load
                UserFederationProviderModel at the beginning:<br>
                <br>
                UserFederationProviderModel providerModel =
                session.getContext().getRealm().getFederationProvider(id);<br>
                <br>
                so there is some shared logic, which can be possibly
                handled by keycloak, but with simpler proposal, people
                will always need to call this in their
                UserFederationProviderFactory implementations.</div>
            </blockquote>
            <div><br>
            </div>
            <div>Depends. Should the "caller" actually load the
              UserFederationProviderModel at all? It seems like all the
              caller needs to know is the instanceId and shouldn't need
              to deal with loading the model/config.<br>
            </div>
          </div>
        </div>
      </div>
    </blockquote>
    Yeah, we can change the "caller" ( UserFederationManager ) to load
    just id. The UserModel has just "federationLink" with the ID, so we
    don't need to load the full UserFederationProviderModel in
    UserFederationManager. During registration or lookup new user, you
    need a list of full provider models though as they need to be sorted
    by priority. But that's very minor....<br>
    <br>
    There is still also the second minor issue I mentioned above, that
    UserFederationProvider implementation always needs
    UserFederationProviderModel (besides some simple impl, which don't
    have any custom config). So in many cases the
    UserFederationProviderFactory.create implementations will always
    start with load of UserFederationProviderModel as they have only ID.
    So that's common logic, which can be theoretically handled by our
    SPI framework instead. But also quite minor though... <br>
    <br>
    I don't have strong opinion that simpler proposal with "String" is
    not enough.<br>
    <blockquote
cite="mid:CAJgngAeo9_ykym7PG-yxG3gaW74fgrrLaNpx8NhR3sRCc1iDBw@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div>Another thing, but that would require db changes for
              sure, could we have a generic configuration mechanism? So
              rather than having to create a table for each SPI we could
              have a single providers table. That would make it much
              easier to introduce new SPIs.</div>
          </div>
        </div>
      </div>
    </blockquote>
    +1<br>
    <br>
    Marek<br>
    <blockquote
cite="mid:CAJgngAeo9_ykym7PG-yxG3gaW74fgrrLaNpx8NhR3sRCc1iDBw@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <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"><span class=""><font
                    color="#888888"><br>
                    <br>
                    Marek</font></span>
                <div>
                  <div class="h5"><br>
                    <blockquote type="cite"> <br>
                      So I agree we can try to go simpler way and
                      possibly enhance just if we find another SPI
                      limitations.<br>
                      <br>
                      Marek<br>
                      <blockquote type="cite">
                        <div dir="ltr">
                          <div class="gmail_extra">
                            <div class="gmail_quote">
                              <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"><span><font
                                      color="#888888"><br>
                                      <br>
                                      Marek</font></span><span><br>
                                    <blockquote type="cite">
                                      <div class="gmail_quote">On 29 Apr
                                        2016 09:43, "Marek Posolda" &lt;<a
                                          moz-do-not-send="true"
                                          href="mailto:mposolda@redhat.com"
                                          target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:mposolda@redhat.com">mposolda@redhat.com</a></a>&gt;

                                        wrote:<br type="attribution">
                                        <blockquote 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> Yes, AFAIK we have
                                              open JIRA for this for a
                                              long time ago. <br>
                                              <br>
                                              It's the same issue for
                                              IdentityProvider (and
                                              maybe some others SPI too)
                                              that they bypass
                                              "official" way for create
                                              provider via
                                              session.getProvider(providerClazz)
                                              and hence they are not
                                              registered in
                                              KeycloakSession and
                                              "close" method is not
                                              called for them.<br>
                                              <br>
                                              The issue is that our SPI
                                              is a bit limited IMO and
                                              doesn't support "stateful"
                                              providers. The providers
                                              are created through
                                              "ProviderFactory.create(KeycloakSession)".
                                              So the only available
                                              state of provider ATM is
                                              just ProviderFactory +
                                              KeycloakSession, which is
                                              sometimes not sufficient.
                                              <br>
                                              <br>
                                              <br>
                                              I can see 2 possibilities
                                              to address:<br>
                                              <br>
                                              1) Always make the
                                              provider implementation
                                              "stateless" and ensure all
                                              the state is passed as
                                              argument to provider
                                              methods. This is what we
                                              already do for some
                                              providers (for example all
                                              methods of UserProvider
                                              has RealmModel as
                                              parameter). So if we
                                              rewrite UserFederation SPI
                                              that
                                              UserFederationProviderModel
                                              will be passed as argument
                                              to all methods of
                                              UserFederationProvider,
                                              then it can use "official"
                                              way too. <br>
                                              <br>
                                              <br>
                                              2) Improve the SPI, so it
                                              can properly support
                                              "stateful" providers. This
                                              is more flexible then (1)
                                              and I would go this way
                                              long term.<br>
                                              <br>
                                              I am thinking about
                                              something like this:<br>
                                              <br>
                                              public interface
                                              StatefulProvider&lt;S&gt;
                                              extends Provider {<br>
                                              }<br>
                                              <br>
                                              <br>
                                              public class
                                              StatefulProviderFactory&lt;T
                                              extends StatefulProvider,
                                              S&gt; {<br>
                                              <br>
                                                   <span
                                                style="color:rgb(32,153,157)">T
                                              </span>create(KeycloakSession




                                              session, S state);<br>
                                              <br>
                                                   .......    <br>
                                              }<br>
                                              <br>
                                              <br>
                                              and on KEycloakSession new
                                              method like this:<br>
                                              <br>
                                              <pre style="color:rgb(0,0,0);font-family:'DejaVu Sans Mono';font-size:9pt;background-color:rgb(255,255,255)">&lt;<span style="color:rgb(32,153,157)">S</span>, <span style="color:rgb(32,153,157)">T </span><span style="color:rgb(0,0,128);font-weight:bold">extends </span>StatefulProvider&lt;<span style="color:rgb(32,153,157)">S</span>&gt;&gt; <span style="color:rgb(32,153,157)">T </span>getProvider(Class&lt;<span style="color:rgb(32,153,157)">T</span>&gt; providerClazz, String id, <span style="color:rgb(32,153,157)">S </span>state);</pre>
                                              <br>
                                              The "state" will need to
                                              properly implement equals
                                              and hashCode, so the SPI
                                              can deal with it and not
                                              create another instance of
                                              StatefulProvider if it was
                                              called for this
                                              KeycloakSession with same
                                              state before.<br>
                                              <br>
                                              Marek
                                              <div><br>
                                                <br>
                                                <br>
                                                On 29/04/16 08:00, Stian
                                                Thorgersen wrote:<br>
                                              </div>
                                            </div>
                                            <blockquote type="cite">
                                              <div>
                                                <div dir="ltr">Looking
                                                  at the code for user
                                                  federation it looks
                                                  like user federation
                                                  provider instances
                                                  with the same
                                                  configuration can be
                                                  created multiple times
                                                  for a single session.
                                                  Also they are never
                                                  closed to resources
                                                  aren't released.</div>
                                                <br>
                                                <fieldset></fieldset>
                                                <br>
                                              </div>
                                              <pre>_______________________________________________
keycloak-dev mailing list
<a moz-do-not-send="true" href="mailto:keycloak-dev@lists.jboss.org" target="_blank">keycloak-dev@lists.jboss.org</a>
<a moz-do-not-send="true" 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>
                                    </blockquote>
                                    <br>
                                  </span></div>
                              </blockquote>
                            </div>
                            <br>
                          </div>
                        </div>
                      </blockquote>
                      <br>
                      <br>
                      <fieldset></fieldset>
                      <br>
                      <pre>_______________________________________________
keycloak-dev mailing list
<a moz-do-not-send="true" href="mailto:keycloak-dev@lists.jboss.org" target="_blank">keycloak-dev@lists.jboss.org</a>
<a moz-do-not-send="true" href="https://lists.jboss.org/mailman/listinfo/keycloak-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/keycloak-dev</a></pre>
                    </blockquote>
                    <br>
                  </div>
                </div>
              </div>
            </blockquote>
          </div>
          <br>
        </div>
      </div>
    </blockquote>
    <br>
  </body>
</html>