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 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">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>