On Wed, Nov 7, 2012 at 3:54 AM, Shane Bryzak <sbryzak@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"));

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.

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.