[security-dev] IdentityManager API review
Jason Porter
lightguard.jp at gmail.com
Wed Nov 7 15:14:18 EST 2012
As an implementation we could say a null creation date will be set to
today, otherwise we'll use what is set. This could actually be important
for people migrating.
On Wed, Nov 7, 2012 at 1:08 PM, Shane Bryzak <sbryzak at redhat.com> wrote:
> They're actually a fundamental part of the identity model (see [1]). I
> have no real problem with the principle of removing the String versions of
> createGroup() (we would also have to do the same to createRole() for
> consistency) and in fact it would provide some additional advantages. For
> example, being able to set a Group to being disabled at creation time,
> setting attribute values, etc. My only concern is from a coding
> "correctness" point of view, and I guess is centered around the creation
> date being automatically set (or potentially overridden) on the Group
> instance that's passed to createGroup(). It's probably not an important
> concern though, and I'm happy to concede on this one which would mean we
> end up with the following methods (replacing all existing createGroup() and
> createRole() methods):
>
> void createGroup(Group group);
>
> void createRole(Role role);
>
>
> [1]
> https://github.com/picketlink/picketlink/blob/master/idm/api/src/main/java/org/picketlink/idm/model/IdentityType.java
>
>
> On 11/08/2012 05:37 AM, Jason Porter wrote:
>
> Aren't those implementation details though?
>
> On Wed, Nov 7, 2012 at 12:16 PM, Shane Bryzak <sbryzak at redhat.com> wrote:
>
>> On 11/08/2012 04:08 AM, Jason Porter wrote:
>>
>> 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.
>>
>>
>> My only reservation here is that the createGroup() method (this is true
>> of createRole() also) sets some additional state during Group (or Role)
>> creation, such as the created date and enabled status. I'm not a big fan
>> of mutating the object that's passed in like this, which is why I kept the
>> String variants of these methods, as a Group (and Role also) is little more
>> than its name. Of course, User breaks this rule but I see it as being an
>> exception as it requires a whole bunch of extended information beyond just
>> the username itself. I could make this clearer with an example if it
>> helps.
>>
>>
>>
>>
>>>
>>> 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
>>
>>
>>
>
>
> --
> 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
>
>
>
--
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/5620f82c/attachment-0001.html
More information about the security-dev
mailing list