[security-dev] IdentityManager API review
Rodney Russ
rruss at redhat.com
Wed Nov 7 14:48:10 EST 2012
Ah, understood. I didn't realize that IdentityType was an abstraction of Group and User.
----- "Shane Bryzak" <sbryzak at redhat.com> wrote:
> 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