[security-dev] IdentityManager API review
Shane Bryzak
sbryzak at redhat.com
Wed Nov 7 14:29:42 EST 2012
Hey Rodney,
I assume you're referring to these methods:
void grantRole(Role role, IdentityType identityType, Group group);
void grantApplicationRole(IdentityType identityType, Role role);
The reason we use IdentityType instead of just User, is to allow roles
to be granted to an entire group of users at once. For example, let's
say that we wanted to automatically make every Red Hat employee a
moderator on some forums. We could do it like this:
Group employees = identityManager.getGroup("/employees");
identityManager.grantApplicationRole(employees,
identityManager.getRole("moderator"));
This way, when we create a new User and make them part of the
"employees" group, they automatically get moderator privileges.
The same goes for grantRole(). By allowing an IdentityType as a
parameter, we can assign a Role to either a User or a Group (both of
which extend IdentityType).
On 11/08/2012 04:29 AM, Rodney Russ wrote:
> Any reason why a role cannot be granted to just a User?
>
> ----- "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).
>>
>> 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);
>>
>> 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);
>>
>> 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);
>>
>> 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)).
>>
>> 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.
>>
>> 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);
>>
>>
>> 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.
>>
>> 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.
>>
>> 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
More information about the security-dev
mailing list