Aren&#39;t those implementation details though?<div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Nov 7, 2012 at 12:16 PM, Shane Bryzak <span dir="ltr">&lt;<a href="mailto:sbryzak@redhat.com" target="_blank">sbryzak@redhat.com</a>&gt;</span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000"><div><div class="h5">
    <div>On 11/08/2012 04:08 AM, Jason Porter
      wrote:<br>
    </div>
    <blockquote type="cite">Replies inline
      <div class="gmail_extra"><br>
        <div class="gmail_quote">On Wed, Nov 7, 2012 at 3:54 AM, Shane
          Bryzak <span dir="ltr">&lt;<a href="mailto:sbryzak@redhat.com" target="_blank">sbryzak@redhat.com</a>&gt;</span>
          wrote:<br>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey guys,<br>
            <br>
            I spent some time reviewing the IdentityManager API today to
            identify<br>
            any redundancies and also locate any holes where we might
            not properly<br>
            support required features, as well as a general &quot;sanity&quot;
            check to ensure<br>
            that what we&#39;re exposing via this API makes sense and is
            intuitive for<br>
            consumers.  As Anil has pointed out already, our short term
            priority for<br>
            the project is to stabilize the API - this is extremely
            important as<br>
            PicketLink will provide the security foundation for many
            other<br>
            projects.  With that in mind I&#39;d like to strongly encourage
            everyone<br>
            with a stake in this to carefully review the API and provide
            feedback,<br>
            as once we release it it will be essentially set in stone.<br>
            <br>
            To try and avoid a wall of text (and make this post easier
            to reply to),<br>
            I&#39;m going to break this post up into sub-sections, one for
            each feature<br>
            group.  For each section, I&#39;ll include the API as it
            currently exists,<br>
            followed by a brief summary of my thoughts and any
            recommendations I may<br>
            have - this is especially where I want to hear any feedback
            indicating<br>
            whether you agree or disagree with my assessment.  Let&#39;s
            start with the<br>
            user-related methods:<br>
            <br>
            User management<br>
            -------------------------<br>
            <br>
                 User createUser(String name);<br>
            <br>
                 void createUser(User user);<br>
            <br>
                 void removeUser(User user);<br>
            <br>
                 void removeUser(String name);<br>
            <br>
                 User getUser(String name);<br>
            <br>
            My thoughts:<br>
            <br>
            1. I&#39;m wondering if we should remove the createUser(String)
            method and<br>
            just have createUser(User).  This would make sense in a way
            because I<br>
            can&#39;t think of many use cases where you might want to create
            a User with<br>
            just a username and not any of the other typical information
            (such as<br>
            first name, last name, e-mail address, etc). Creating a User
            via<br>
            createUser(String) when you actually want to set the other
            details after<br>
            the initial User creation is also horribly inefficient,
            requiring<br>
            multiple round trips to the database (or whatever identity
            store backend<br>
            you use).  To add to that, the actual parameter name is a
            little<br>
            unintuitive - &quot;name&quot; could refer to any number of things -
            is it the<br>
            username or the user&#39;s actual name that you need to provide
            here?  My +1<br>
            goes to removing createUser(String).<br>
          </blockquote>
          <div><br>
          </div>
          <div>I like removing the strings. For most of the rest of this
            API we have a very type safe idea here. Maybe I&#39;m thinking a
            little overboard on things, but I really like the idea of
            having wrappers around things (simple wrappers for our API
            so people could implement an interface or use a base class
            or something to be able to interact with our API but still
            easily get to the information they need). </div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            2. Similar to point 1), having two removeUser() methods
            seems equally<br>
            redundant.  The User object that needs to be provided to the<br>
            removeUser(User) method can be easily looked up by calling
            getUser(), as<br>
            per this example:<br>
            <br>
identityManager.removeUser(identityManager.getUser(&quot;jsmith&quot;));<br>
          </blockquote>
          <div><br>
          </div>
          <div>+1</div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            <br>
            3. As I also pointed out in point 1), the &quot;name&quot; parameter
            is a little<br>
            unintuitive.  In regard to the getUser() method I think we
            should rename<br>
            the &quot;name&quot; parameter to &quot;username&quot; just so it&#39;s perfectly
            clear what the<br>
            method expects.<br>
          </blockquote>
          <div><br>
          </div>
          <div>+1</div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            4. We don&#39;t currently have any way to update User details.
             I recommend<br>
            that we add an updateUser() method as follows:<br>
            <br>
            void updateUser(User user);<br>
          </blockquote>
          <div><br>
          </div>
          <div>I like this idea. I&#39;ve seen on a few sites where you
            start with a basic username and password, then your
            confirmation is to complete their identity for yourself, so
            it&#39;s not completely out there that people wouldn&#39;t do this.</div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            This will allow User details such as their first and last
            name, e-mail<br>
            address etc to be updated without having to delete and
            re-create the<br>
            User.  Extending on this a little further, I&#39;m wondering if
            we should<br>
            think about adding an audit API that logs these changes.
             Perhaps it&#39;s<br>
            something to think about for a later release.<br>
            <br>
            Group management<br>
            --------------------------<br>
            <br>
                 Group createGroup(String id);<br>
            <br>
                 Group createGroup(String id, Group parent);<br>
            <br>
                 Group createGroup(String id, String parent);<br>
            <br>
                 void removeGroup(Group group);<br>
            <br>
                 void removeGroup(String groupId);<br>
            <br>
                 Group getGroup(String groupId);<br>
            <br>
                 Group getGroup(String groupId, Group parent);<br>
            <br>
                 void addToGroup(IdentityType identityType, Group
            group);<br>
            <br>
                 void removeFromGroup(IdentityType identityType, Group
            group);<br>
            <br>
            My thoughts:<br>
            <br>
            1. I think there is a little bit of ambiguity here in regard
            to the<br>
            differences between a Group&#39;s ID and name.  To assist in
            understanding<br>
            the differences, here&#39;s the JavaDoc pasted from the Group
            interface:<br>
            <br>
                 /**<br>
                  * Groups are stored in tree hierarchy and therefore ID
            represents<br>
            a path. ID string always begins with &quot;/&quot; element that<br>
                  * represents root of the tree<br>
                  * &lt;p/&gt;<br>
                  * Example: Valid IDs are
            &quot;/acme/departments/marketing&quot;,<br>
            &quot;/security/administrator&quot; or &quot;/administrator&quot;. Where &quot;acme&quot;,<br>
                  * &quot;departments&quot;, &quot;marketing&quot;, &quot;security&quot; and
            &quot;administrator&quot; are<br>
            group names.<br>
                  *<br>
                  * @return Group Id in String representation.<br>
                  */<br>
                 String getId();<br>
            <br>
                 /**<br>
                  * Group name is unique identifier in specific group
            tree branch.<br>
            For example group with id &quot;/acme/departments/marketing&quot;<br>
                  * will have name &quot;marketing&quot; and parent group of id<br>
            &quot;/acme/departments&quot;<br>
                  *<br>
                  * @return name<br>
                  */<br>
                 String getName();<br>
            <br>
            Taking the above JavaDoc into account, it seems that where
            we are<br>
            specifying &quot;id&quot; as a method parameter we should be
            specifying &quot;name&quot;<br>
            instead.  If we are indeed providing a &quot;fully qualified&quot;
            path to the<br>
            createGroup() method, then there is no need to also specify
            the parent<br>
            as it can be easily derived from the path.  More on this in
            the<br>
            following points.<br>
            <br>
            2. We currently have three createGroup() methods for
            creating a new<br>
            Group.  I think we should remove the (String, String)
            variant, as the<br>
            parent parameter is ambiguous (is it the parent&#39;s ID or name
            we need to<br>
            specify here?), and rename the &quot;id&quot; parameter of the
            (String, Group)<br>
            variant to &quot;name&quot;, leaving us with the following:<br>
            <br>
                 Group createGroup(String id);<br>
            <br>
                 Group createGroup(String name, Group parent);<br>
            <br>
            This then gives us two ways of creating a group.  We can
            either specify<br>
            the fully qualified group ID:<br>
            <br>
            Group employees = identityManager.createGroup(&quot;/employees&quot;);<br>
            <br>
            Or we can specify a subgroup name and parent Group:<br>
            <br>
            Group managers = identityManager.createGroup(&quot;managers&quot;,
            employees);<br>
            <br>
            We can also use this second form to create a &quot;root&quot; group by
            just<br>
            specifying null for the parent:<br>
            <br>
            Group admins = identityManager.createGroup(&quot;admins&quot;, null);<br>
          </blockquote>
          <div><br>
          </div>
          <div>I mentioned this earlier, but I don&#39;t see why we need the
            Strings. We could simply use Group and have a basic
            constructor that would take a String for the name and
            possibly another one with a String and a parent group.</div>
        </div>
      </div>
    </blockquote>
    <br></div></div>
    My only reservation here is that the createGroup() method (this is
    true of createRole() also) sets some additional state during Group
    (or Role) creation, such as the created date and enabled status. 
    I&#39;m not a big fan of mutating the object that&#39;s passed in like this,
    which is why I kept the String variants of these methods, as a Group
    (and Role also) is little more than its name.  Of course, User
    breaks this rule but I see it as being an exception as it requires a
    whole bunch of extended information beyond just the username
    itself.  I could make this clearer with an example if it helps.<div><div class="h5"><br>
    <br>
    <blockquote type="cite">
      <div class="gmail_extra">
        <div class="gmail_quote">
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            <br>
            3. For the removeGroup() methods, I think what we have is
            fine.<br>
            <br>
            4. For the getGroup() methods, I&#39;m happy with the first one
            (String<br>
            groupId) but I think the second one (String groupId, Group
            parent) needs<br>
            to be (String name, Group parent) instead.  The first method
            would be<br>
            used when the fully qualified group ID is known, and the
            second one<br>
            would be used when you already have the parent Group and
            know the name<br>
            of the subgroup.<br>
            <br>
            5. The addToGroup() and removeFromGroup() methods seem fine
            to me.<br>
            <br>
            6. One thing we don&#39;t have is a method to test whether an
            IdentityType<br>
            is a member of a Group.  I suggest we add another method to
            support this:<br>
            <br>
            boolean inGroup(IdentityType identityType, Group group);<br>
            <br>
          </blockquote>
          <div><br>
          </div>
          <div>+1 for the above (with the type idea for #4).</div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            Roles<br>
            ----------<br>
            <br>
                 Role createRole(String name);<br>
            <br>
                 void removeRole(Role role);<br>
            <br>
                 void removeRole(String name);<br>
            <br>
                 Role getRole(String name);<br>
            <br>
                 boolean hasRole(Role role, IdentityType identityType,
            Group group);<br>
            <br>
                 void grantRole(Role role, IdentityType identityType,
            Group group);<br>
            <br>
                 void revokeRole(Role role, IdentityType identityType,
            Group group);<br>
            <br>
            My thoughts:<br>
            <br>
            1. I would remove the removeRole(String) method as the same
            thing can be<br>
            achieved with removeRole(getRole(String)).<br>
          </blockquote>
          <div><br>
          </div>
          <div>+1</div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            2. I would swap the order of the Role and IdentityType
            parameters within<br>
            the hasRole(), grantRole() and revokeRole() methods.  As the<br>
            IdentityType is the primary artifact in these operations it
            makes sense<br>
            to have it listed first.<br>
          </blockquote>
          <div><br>
          </div>
          <div>+1</div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">3. We
            don&#39;t have any explicit support for application roles, which
            are<br>
            roles where there is no Group component (for example, an<br>
            application-wide &quot;admin&quot; role).  We could support this by
            just allowing<br>
            hasRole(), grantRole() etc to accept null values for the
            Group<br>
            parameter, however I feel it would be more semantically
            correct to<br>
            provide a distinct set of methods for the purpose of
            supporting<br>
            application roles, as follows:<br>
            <br>
                 boolean hasApplicationRole(IdentityType identityType,
            Role role);<br>
            <br>
                 void grantApplicationRole(IdentityType identityType,
            Role role);<br>
            <br>
                 void revokeApplicationRole(IdentityType identityType,
            Role role);<br>
          </blockquote>
          <div><br>
          </div>
          <div>+1</div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            Query API<br>
            --------------<br>
            <br>
                 &lt;T extends IdentityType&gt; IdentityQuery&lt;T&gt;
            createQuery();<br>
            <br>
            My thoughts:<br>
            <br>
            1. After our recent rewrite of the Query API I&#39;m satisfied
            with what we<br>
            have now.<br>
            <br>
            Credential management<br>
            -------------------------------<br>
            <br>
                 boolean validateCredential(User user, Credential
            credential);<br>
            <br>
                 void updateCredential(User user, Credential
            credential);<br>
            <br>
            My thoughts:<br>
            <br>
            1. I&#39;m satisfied with what we currently have, however we
            still need to<br>
            review the mechanism that we provide for encoding passwords.
             I&#39;m not<br>
            sure that it will have any effect on these methods though
            (I&#39;m toying<br>
            with the idea of integrating the password encoding
            functionality via the<br>
            IdentityStoreInvocationContext).<br>
            <br>
            Identity expiry<br>
            ------------------<br>
            <br>
                 void setEnabled(IdentityType identityType, boolean
            enabled);<br>
            <br>
                 void setExpirationDate(IdentityType identityType, Date
            expirationDate);<br>
            <br>
            My thoughts:<br>
            <br>
            1. I think these methods are fine the way they are.  One
            discrepancy<br>
            that I&#39;ve identified is that a User can be created already
            being<br>
            disabled or having an expiry date, while for a Role or Group
            the<br>
            equivalent status must be set via these methods after
            creation.  I&#39;m not<br>
            sure this is a big deal though.<br>
          </blockquote>
          <div><br>
          </div>
          <div>All good.</div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            Attributes<br>
            -------------<br>
            <br>
                 void setAttribute(IdentityType identityType, String
            attributeName,<br>
            String attributeValue);<br>
            <br>
                 String getAttribute(IdentityType identityType, String
            attributeName);<br>
            <br>
            My thoughts:<br>
            <br>
            1. This area poses a slight dilemma.  Currently the API only
            supports<br>
            simple String-based attribute values, however I&#39;m pretty
            sure that<br>
            people are going to want to store all sorts of things,
            ranging from<br>
            boolean and Date values through to large byte arrays that
            store a User&#39;s<br>
            photograph or other data.  If everyone is in agreement that
            we need to<br>
            support more than just String values, then the first thing
            we probably<br>
            need to do is modify these methods to work with a
            Serializable instead.<br>
            <br>
            2. If we all agree on point 1), the next thing we need to
            decide is<br>
            whether we want the capability to make some attribute values
            &quot;lazy<br>
            loaded&quot;.  If we want to do a quick lookup of a User object
            for the<br>
            express purpose of assigning a Role or Group membership (or
            any other<br>
            type of simple operation) then we probably don&#39;t want the
            performance<br>
            hit of having to load bulky attribute data.<br>
            <br>
            3. With the above two points in mind, I&#39;m going to hold off
            on surmising<br>
            any further on attributes until some of you guys weigh in
            with your<br>
            opinions.  I do have some rough ideas, however I&#39;ll wait
            until we have a<br>
            consensus of exactly what we want to achieve here before we
            proceed further.<br>
          </blockquote>
          <div><br>
          </div>
          <div>Serializable works for me. Do we do have a generic
            Attribute&lt;? extends Serializable&gt; or just forgo the
            generics?</div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            Utility methods<br>
            --------------------<br>
            <br>
                 IdentityType lookupIdentityByKey(String key);<br>
            <br>
            My thoughts:<br>
            <br>
            1. This method is required by other features, such as the
            Permissions<br>
            API.  I&#39;m fine with what we have here.<br>
            <br>
            In summary<br>
            ----------------<br>
            <br>
            I apologise for yet another epic e-mail to the list, however
            this is<br>
            everyone&#39;s chance to review the API for themselves and
            decide whether or<br>
            not it will be suitable for addressing all of your
            requirements.  I&#39;d<br>
            like to get this API stable by next week so please don&#39;t be
            shy about<br>
            speaking up.  I&#39;m especially looking forward to hearing your
            opinions<br>
            about how we handle attributes (there may even be some
            relevant JSR-351<br>
            stuff that we should be looking at here).<br>
            <br>
            Thanks,<br>
            Shane<br>
            _______________________________________________<br>
            security-dev mailing list<br>
            <a href="mailto:security-dev@lists.jboss.org" target="_blank">security-dev@lists.jboss.org</a><br>
            <a href="https://lists.jboss.org/mailman/listinfo/security-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/security-dev</a><br>
          </blockquote>
        </div>
        <br>
        <br clear="all">
        <div><br>
        </div>
        -- <br>
        Jason Porter<br>
        <a href="http://lightguard-jp.blogspot.com" target="_blank">http://lightguard-jp.blogspot.com</a><br>
        <a href="http://twitter.com/lightguardjp" target="_blank">http://twitter.com/lightguardjp</a><br>
        <br>
        Software Engineer<br>
        Open Source Advocate<br>
        <br>
        PGP key id: 926CCFF5<br>
        PGP key available at: <a href="http://keyserver.net" target="_blank">keyserver.net</a>,
        <a href="http://pgp.mit.edu" target="_blank">pgp.mit.edu</a><br>
      </div>
    </blockquote>
    <br>
  </div></div></div>

</blockquote></div><br><br clear="all"><div><br></div>-- <br>Jason Porter<br><a href="http://lightguard-jp.blogspot.com" target="_blank">http://lightguard-jp.blogspot.com</a><br><a href="http://twitter.com/lightguardjp" target="_blank">http://twitter.com/lightguardjp</a><br>

<br>Software Engineer<br>Open Source Advocate<br><br>PGP key id: 926CCFF5<br>PGP key available at: <a href="http://keyserver.net" target="_blank">keyserver.net</a>, <a href="http://pgp.mit.edu" target="_blank">pgp.mit.edu</a><br>


</div>