[security-dev] IdentityManager API review

Jason Porter lightguard.jp at gmail.com
Wed Nov 7 13:08:23 EST 2012


Replies inline

On Wed, Nov 7, 2012 at 3:54 AM, Shane Bryzak <sbryzak at redhat.com> wrote:

> Hey guys,
>
> I spent some time reviewing the IdentityManager API today to identify
> any redundancies and also locate any holes where we might not properly
> support required features, as well as a general "sanity" check to ensure
> that what we're exposing via this API makes sense and is intuitive for
> consumers.  As Anil has pointed out already, our short term priority for
> the project is to stabilize the API - this is extremely important as
> PicketLink will provide the security foundation for many other
> projects.  With that in mind I'd like to strongly encourage everyone
> with a stake in this to carefully review the API and provide feedback,
> as once we release it it will be essentially set in stone.
>
> To try and avoid a wall of text (and make this post easier to reply to),
> I'm going to break this post up into sub-sections, one for each feature
> group.  For each section, I'll include the API as it currently exists,
> followed by a brief summary of my thoughts and any recommendations I may
> have - this is especially where I want to hear any feedback indicating
> whether you agree or disagree with my assessment.  Let's start with the
> user-related methods:
>
> User management
> -------------------------
>
>      User createUser(String name);
>
>      void createUser(User user);
>
>      void removeUser(User user);
>
>      void removeUser(String name);
>
>      User getUser(String name);
>
> My thoughts:
>
> 1. I'm wondering if we should remove the createUser(String) method and
> just have createUser(User).  This would make sense in a way because I
> can't think of many use cases where you might want to create a User with
> just a username and not any of the other typical information (such as
> first name, last name, e-mail address, etc). Creating a User via
> createUser(String) when you actually want to set the other details after
> the initial User creation is also horribly inefficient, requiring
> multiple round trips to the database (or whatever identity store backend
> you use).  To add to that, the actual parameter name is a little
> unintuitive - "name" could refer to any number of things - is it the
> username or the user's actual name that you need to provide here?  My +1
> goes to removing createUser(String).
>

I like removing the strings. For most of the rest of this API we have a
very type safe idea here. Maybe I'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).


> 2. Similar to point 1), having two removeUser() methods seems equally
> redundant.  The User object that needs to be provided to the
> removeUser(User) method can be easily looked up by calling getUser(), as
> per this example:
>
> identityManager.removeUser(identityManager.getUser("jsmith"));
>

+1


>
> 3. As I also pointed out in point 1), the "name" parameter is a little
> unintuitive.  In regard to the getUser() method I think we should rename
> the "name" parameter to "username" just so it's perfectly clear what the
> method expects.
>

+1


> 4. We don't currently have any way to update User details.  I recommend
> that we add an updateUser() method as follows:
>
> void updateUser(User user);
>

I like this idea. I'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's not completely out there that people wouldn't do this.


> This will allow User details such as their first and last name, e-mail
> address etc to be updated without having to delete and re-create the
> User.  Extending on this a little further, I'm wondering if we should
> think about adding an audit API that logs these changes.  Perhaps it's
> something to think about for a later release.
>
> Group management
> --------------------------
>
>      Group createGroup(String id);
>
>      Group createGroup(String id, Group parent);
>
>      Group createGroup(String id, String parent);
>
>      void removeGroup(Group group);
>
>      void removeGroup(String groupId);
>
>      Group getGroup(String groupId);
>
>      Group getGroup(String groupId, Group parent);
>
>      void addToGroup(IdentityType identityType, Group group);
>
>      void removeFromGroup(IdentityType identityType, Group group);
>
> My thoughts:
>
> 1. I think there is a little bit of ambiguity here in regard to the
> differences between a Group's ID and name.  To assist in understanding
> the differences, here's the JavaDoc pasted from the Group interface:
>
>      /**
>       * Groups are stored in tree hierarchy and therefore ID represents
> a path. ID string always begins with "/" element that
>       * represents root of the tree
>       * <p/>
>       * Example: Valid IDs are "/acme/departments/marketing",
> "/security/administrator" or "/administrator". Where "acme",
>       * "departments", "marketing", "security" and "administrator" are
> group names.
>       *
>       * @return Group Id in String representation.
>       */
>      String getId();
>
>      /**
>       * Group name is unique identifier in specific group tree branch.
> For example group with id "/acme/departments/marketing"
>       * will have name "marketing" and parent group of id
> "/acme/departments"
>       *
>       * @return name
>       */
>      String getName();
>
> Taking the above JavaDoc into account, it seems that where we are
> specifying "id" as a method parameter we should be specifying "name"
> instead.  If we are indeed providing a "fully qualified" path to the
> createGroup() method, then there is no need to also specify the parent
> as it can be easily derived from the path.  More on this in the
> following points.
>
> 2. We currently have three createGroup() methods for creating a new
> Group.  I think we should remove the (String, String) variant, as the
> parent parameter is ambiguous (is it the parent's ID or name we need to
> specify here?), and rename the "id" parameter of the (String, Group)
> variant to "name", leaving us with the following:
>
>      Group createGroup(String id);
>
>      Group createGroup(String name, Group parent);
>
> This then gives us two ways of creating a group.  We can either specify
> the fully qualified group ID:
>
> Group employees = identityManager.createGroup("/employees");
>
> Or we can specify a subgroup name and parent Group:
>
> Group managers = identityManager.createGroup("managers", employees);
>
> We can also use this second form to create a "root" group by just
> specifying null for the parent:
>
> Group admins = identityManager.createGroup("admins", null);
>

I mentioned this earlier, but I don'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.


>
> 3. For the removeGroup() methods, I think what we have is fine.
>
> 4. For the getGroup() methods, I'm happy with the first one (String
> groupId) but I think the second one (String groupId, Group parent) needs
> to be (String name, Group parent) instead.  The first method would be
> used when the fully qualified group ID is known, and the second one
> would be used when you already have the parent Group and know the name
> of the subgroup.
>
> 5. The addToGroup() and removeFromGroup() methods seem fine to me.
>
> 6. One thing we don't have is a method to test whether an IdentityType
> is a member of a Group.  I suggest we add another method to support this:
>
> boolean inGroup(IdentityType identityType, Group group);
>
>
+1 for the above (with the type idea for #4).


> Roles
> ----------
>
>      Role createRole(String name);
>
>      void removeRole(Role role);
>
>      void removeRole(String name);
>
>      Role getRole(String name);
>
>      boolean hasRole(Role role, IdentityType identityType, Group group);
>
>      void grantRole(Role role, IdentityType identityType, Group group);
>
>      void revokeRole(Role role, IdentityType identityType, Group group);
>
> My thoughts:
>
> 1. I would remove the removeRole(String) method as the same thing can be
> achieved with removeRole(getRole(String)).
>

+1


> 2. I would swap the order of the Role and IdentityType parameters within
> the hasRole(), grantRole() and revokeRole() methods.  As the
> IdentityType is the primary artifact in these operations it makes sense
> to have it listed first.
>

+1


> 3. We don't have any explicit support for application roles, which are
> roles where there is no Group component (for example, an
> application-wide "admin" role).  We could support this by just allowing
> hasRole(), grantRole() etc to accept null values for the Group
> parameter, however I feel it would be more semantically correct to
> provide a distinct set of methods for the purpose of supporting
> application roles, as follows:
>
>      boolean hasApplicationRole(IdentityType identityType, Role role);
>
>      void grantApplicationRole(IdentityType identityType, Role role);
>
>      void revokeApplicationRole(IdentityType identityType, Role role);
>

+1


> Query API
> --------------
>
>      <T extends IdentityType> IdentityQuery<T> createQuery();
>
> My thoughts:
>
> 1. After our recent rewrite of the Query API I'm satisfied with what we
> have now.
>
> Credential management
> -------------------------------
>
>      boolean validateCredential(User user, Credential credential);
>
>      void updateCredential(User user, Credential credential);
>
> My thoughts:
>
> 1. I'm satisfied with what we currently have, however we still need to
> review the mechanism that we provide for encoding passwords.  I'm not
> sure that it will have any effect on these methods though (I'm toying
> with the idea of integrating the password encoding functionality via the
> IdentityStoreInvocationContext).
>
> Identity expiry
> ------------------
>
>      void setEnabled(IdentityType identityType, boolean enabled);
>
>      void setExpirationDate(IdentityType identityType, Date
> expirationDate);
>
> My thoughts:
>
> 1. I think these methods are fine the way they are.  One discrepancy
> that I've identified is that a User can be created already being
> disabled or having an expiry date, while for a Role or Group the
> equivalent status must be set via these methods after creation.  I'm not
> sure this is a big deal though.
>

All good.


> Attributes
> -------------
>
>      void setAttribute(IdentityType identityType, String attributeName,
> String attributeValue);
>
>      String getAttribute(IdentityType identityType, String attributeName);
>
> My thoughts:
>
> 1. This area poses a slight dilemma.  Currently the API only supports
> simple String-based attribute values, however I'm pretty sure that
> people are going to want to store all sorts of things, ranging from
> boolean and Date values through to large byte arrays that store a User's
> photograph or other data.  If everyone is in agreement that we need to
> support more than just String values, then the first thing we probably
> need to do is modify these methods to work with a Serializable instead.
>
> 2. If we all agree on point 1), the next thing we need to decide is
> whether we want the capability to make some attribute values "lazy
> loaded".  If we want to do a quick lookup of a User object for the
> express purpose of assigning a Role or Group membership (or any other
> type of simple operation) then we probably don't want the performance
> hit of having to load bulky attribute data.
>
> 3. With the above two points in mind, I'm going to hold off on surmising
> any further on attributes until some of you guys weigh in with your
> opinions.  I do have some rough ideas, however I'll wait until we have a
> consensus of exactly what we want to achieve here before we proceed
> further.
>

Serializable works for me. Do we do have a generic Attribute<? extends
Serializable> or just forgo the generics?


> Utility methods
> --------------------
>
>      IdentityType lookupIdentityByKey(String key);
>
> My thoughts:
>
> 1. This method is required by other features, such as the Permissions
> API.  I'm fine with what we have here.
>
> In summary
> ----------------
>
> I apologise for yet another epic e-mail to the list, however this is
> everyone's chance to review the API for themselves and decide whether or
> not it will be suitable for addressing all of your requirements.  I'd
> like to get this API stable by next week so please don't be shy about
> speaking up.  I'm especially looking forward to hearing your opinions
> about how we handle attributes (there may even be some relevant JSR-351
> stuff that we should be looking at here).
>
> Thanks,
> Shane
> _______________________________________________
> security-dev mailing list
> security-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/security-dev
>



-- 
Jason Porter
http://lightguard-jp.blogspot.com
http://twitter.com/lightguardjp

Software Engineer
Open Source Advocate

PGP key id: 926CCFF5
PGP key available at: keyserver.net, pgp.mit.edu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/security-dev/attachments/20121107/963ad1c0/attachment-0001.html 


More information about the security-dev mailing list