[security-dev] IdentityManager review - queries

Shane Bryzak sbryzak at redhat.com
Tue Oct 30 06:41:40 EDT 2012


I've managed to trim the IdentityQuery API down to just this:

public interface IdentityQuery<T extends IdentityType> {
     public enum Operator { equals, notEquals, greaterThan, lessThan };
     IdentityQuery<T> setOffset(int offset);
     IdentityQuery<T> setLimit(int limit);
     IdentityQuery<T> setParameter(QueryParameter param, Object value);
     IdentityQuery<T> setParameter(QueryParameter param, Operator 
operator, Object value);
     List<T> getResultList();
}

While I did like the concept of Pedro's method refactor, I think that 
sticking with an API that mimics that of JPA (which most developers 
should be already familiar with) will be more intuitive.

To get rid of the setAttribute() methods, I had to do a little bit of 
hackery by adding this to the IdentityType interface:

     public class AttributeParameter implements QueryParameter {
         private String name;
         public AttributeParameter(String name) {
             this.name = name;
         }
         public String getName() {
             return name;
         }
     }
     public final class PARAM_ATTRIBUTE {
         public static AttributeParameter byName(String name) {
             return new AttributeParameter(name);
         }
     }

This then allows us to simply use the setParameter() method to set 
attribute values also, like so:

         List<User> users = im.<User>createQuery()
                 .setParameter(User.PARAM_ENABLED, true)
                 .setParameter(User.PARAM_ATTRIBUTE.byName("foo"), "bar")
                 .getResultList();

I'm still trying to decide if I like this or not.  I guess it somewhat 
simplifies usage as the developer can follow the same pattern for 
setting all parameter values.  What do you guys think about it?


On 10/30/2012 01:49 PM, Jason Porter wrote:
> On Oct 29, 2012, at 21:31, Shane Bryzak <sbryzak at redhat.com 
> <mailto:sbryzak at redhat.com>> wrote:
>
>> On 10/30/2012 12:32 PM, Jason Porter wrote:
>>> On Oct 29, 2012, at 17:20, Shane Bryzak <sbryzak at redhat.com 
>>> <mailto:sbryzak at redhat.com>> wrote:
>>>
>>>> On 10/30/2012 01:20 AM, Jason Porter wrote:
>>>>> I very much like the DSL you have going there. Removing the Range 
>>>>> class is also a good idea, imo.  However, I'm at -1 for the use of 
>>>>> enums. You can't extend them, and you can't anticipate everything 
>>>>> a user would add to a class. Sure we could do it in the simple idm 
>>>>> idea where we provide the structure, but as soon as they advance 
>>>>> past that then they're forced to use a different API.
>>>>
>>>> I agree, I wasn't a fan of using an enum either but at the time I 
>>>> hashed this out I couldn't think of a better alternative.  How 
>>>> about instead of enums we introduce a marker interface like this:
>>>>
>>>> public interface QueryParameter {
>>>>
>>>> }
>>>>
>>>> Then we can simply define supported parameters directly on the core 
>>>> identity model interfaces, as Pedro has suggested:
>>>>
>>>> public interface User extends IdentityType, Serializable {
>>>>     public static final QueryParameter FIRST_NAME = new 
>>>> QueryParameter() {};
>>>>     public static final QueryParameter LAST_NAME = new 
>>>> QueryParameter() {};
>>>>     ...
>>>> }
>>>>
>>>> Then the method signature for IdentityQuery.setParameter would look 
>>>> something like this:
>>>>
>>>> IdentityQuery<T> setParameter(QueryParameter param, Object value);
>>>>
>>>> This then makes the parameter set extensible (anyone can create 
>>>> their own) and we can create an SPI (or just make the IdentityStore 
>>>> implementations extensible themselves) so that developers can 
>>>> provide support for their own custom query parameters (not to 
>>>> mention it makes it more future proof if we want to add more 
>>>> parameter types later).
>>>
>>> Any reason not to do a generic interface or abstract class? 
>>> QueryParam<TYPE> and have a void setValue(TYPE val), a <TYPE> 
>>> getValue() and a String getName() on that interface/class? That 
>>> would cut down on an argument.
>>
>> I'm not quite getting this - could you give an example of what the 
>> API would look like?
>
> I've thought about it a bit more and like your proposal better for the 
> enhanced readability.
>
>>>
>>> Though the final empty interfaces from the classes does read nicely.
>>>
>>>>>
>>>>> On Mon, Oct 29, 2012 at 7:10 AM, Pedro Igor Silva 
>>>>> <psilva at redhat.com <mailto:psilva at redhat.com>> wrote:
>>>>>
>>>>>     I liked your proposal, it is simple and clear.
>>>>>
>>>>>     Considering what you did I thought in another solution. At
>>>>>     first glance, it seems more odd. But I think it can be more
>>>>>     intuitive for users. For example, instead of having the
>>>>>     Param.memberOf, users may use query.role(Role) or
>>>>>     query.group(Group) directly.
>>>>>
>>>>>             IdentityManager identityManager = // get the identity
>>>>>     manager;
>>>>>
>>>>>             Query<User> query = identityManager.<User>createQuery();
>>>>>
>>>>>             query
>>>>>                 .limit(100)
>>>>>                 .offset(1)
>>>>>                 .where()
>>>>>                     .id("1")
>>>>>
>>>>>                     .property(User.FIRST_NAME, OPERATOR.equals,
>>>>>     "John")
>>>>>                     .property(User.EXPIRATION_DATE,
>>>>>     OPERATOR.lessThan, new Date())
>>>>>
>>>>>                     .attribute("ssn", OPERATOR.equals, "SSN-123")
>>>>>
>>>>>                     .role(new SimpleRole("Payroll Officer"))
>>>>>
>>>>>                     .group(new SimpleGroup("Superuser",
>>>>>     "Superuser", null));
>>>>>
>>>>>             List<User> list = query.list();
>>>>>
>>>>>             for (User user : list) {
>>>>>                 // handle users
>>>>>             }
>>>>>
>>>>>     I think we can avoid having a Range class/interface by using
>>>>>     some methods on the Query interface (eg. limit and offset
>>>>>     above). Th Query interface can be used only to configure how
>>>>>     the query should be executed, global configuration, etc.
>>>>>
>>>>>     The Where interface can be useful to provide some specific
>>>>>     validation depending of the IdentityType and how the
>>>>>     restrictions are configured. For example, users do not have a
>>>>>     parent relationship.
>>>>>
>>>>>     The property names (firstName, email, etc) can be a Enum for
>>>>>     each IdentityType type (user,group and role). Each
>>>>>     IdentityType type should specify which properties are
>>>>>     searchable + the common properties defined by the IdentityType
>>>>>     interface.
>>>>>
>>>>>     Not sure if we need the query.reset() method ... Why not just
>>>>>     discard the query instance and build another one ?
>>>>>
>>>>>     Regards.
>>>>>     Pedro Igor
>>>>>
>>>>>     ----- Original Message -----
>>>>>     From: "Shane Bryzak" <sbryzak at redhat.com
>>>>>     <mailto:sbryzak at redhat.com>>
>>>>>     To: security-dev at lists.jboss.org
>>>>>     <mailto:security-dev at lists.jboss.org>
>>>>>     Sent: Monday, October 29, 2012 6:38:14 AM
>>>>>     Subject: [security-dev] IdentityManager review - queries
>>>>>
>>>>>     I've started reviewing the IdentityManager interface to see
>>>>>     where we can
>>>>>     improve the API.  The first area I'd like to visit is the
>>>>>     Query API, of
>>>>>     which I've come to the conclusion that we need to do some serious
>>>>>     redesign - the current API is non-intuitive, too verbose and
>>>>>     not future
>>>>>     proof.
>>>>>
>>>>>     What I'd like to do is throw it all out and start again,
>>>>>     replacing it
>>>>>     with a new cleaner API that looks something like this:
>>>>>
>>>>>     public interface IdentityManager {
>>>>>          // <snip other methods>
>>>>>
>>>>>          <T extends IdentityType> IdentityQuery<T> createQuery();
>>>>>     }
>>>>>
>>>>>     public interface IdentityQuery<T extends IdentityType> {
>>>>>          public enum Param {id, key, created, expired, enabled,
>>>>>     firstName,
>>>>>     lastName, email, name, parent, memberOf};
>>>>>
>>>>>          public enum Operator { equals, notEquals, greaterThan,
>>>>>     lessThan };
>>>>>
>>>>>          IdentityQuery<T> reset();
>>>>>
>>>>>          IdentityQuery<T> setParameter(Param param, Object value);
>>>>>
>>>>>          IdentityQuery<T> setParameter(Param param, Operator operator,
>>>>>     Object value);
>>>>>
>>>>>          IdentityQuery<T> setAttributeParameter(String
>>>>>     attributeName, Object
>>>>>     value);
>>>>>
>>>>>          IdentityQuery<T> setAttributeParameter(String attributeName,
>>>>>     Operator operator, Object value);
>>>>>
>>>>>          IdentityQuery<T> setRange(Range range);
>>>>>
>>>>>          List<T> getResultList();
>>>>>     }
>>>>>
>>>>>     This unified API basically replaces the 4 separate existing
>>>>>     interfaces
>>>>>     we currently have; UserQuery, RoleQuery, GroupQuery and
>>>>>     MembershipQuery.  I've put together a few usage scenarios to
>>>>>     show how it
>>>>>     might work:
>>>>>
>>>>>     1) Find users with first name 'John':
>>>>>
>>>>>     List<User> users = identityManager.<User>createQuery()
>>>>>          .setParameter(Param.firstName, "John")
>>>>>          .getResultList();
>>>>>
>>>>>     2) Find all expired users:
>>>>>
>>>>>     List<User> users = identityManager.<User>createQuery()
>>>>>          .setParameter(Param.expired, Operator.lessThan, new Date())
>>>>>          .getResultList();
>>>>>
>>>>>     3) Find all users that are a member of the "Superuser" group
>>>>>
>>>>>     List<User> users = identityManager.<User>createQuery()
>>>>>          .setParameter(Param.memberOf,
>>>>>     identityManager.getGroup("Superuser"))
>>>>>          .getResultList();
>>>>>
>>>>>     4) Find all sub-groups of the "Employees" group:
>>>>>
>>>>>     List<Group> groups = identityManager.<Group>createQuery()
>>>>>          .setParameter(Param.memberOf,
>>>>>     identityManager.getGroup("Employees"))
>>>>>          .getResultList();
>>>>>
>>>>>     5) Find all disabled roles:
>>>>>
>>>>>     List<Role> roles = identityManager.<Role>createQuery()
>>>>>          .setParameter(Param.enabled, false)
>>>>>          .getResultList();
>>>>>
>>>>>     6) Find all Users, Groups and Roles that have been granted the
>>>>>     "Payroll
>>>>>     Officer" role in the "Human Resources" group:
>>>>>
>>>>>     List<IdentityType> identities =
>>>>>     identityManager.<IdentityType>createQuery()
>>>>>          .setParameter(Param.memberOf, identityManager.getGroup("Human
>>>>>     Resources"))
>>>>>          .setParameter(Param.memberOf,
>>>>>     identityManager.getRole("Payroll
>>>>>     Officer"))
>>>>>          .getResultList();
>>>>>
>>>>>     7) Find all Users that have an attribute named "Citizenship"
>>>>>     with a
>>>>>     value of "Greenland":
>>>>>
>>>>>     List<User> users = identityManager.<User>createQuery()
>>>>>          .setAttributeParameter("Citizenship", "Greenland")
>>>>>          .getResultList();
>>>>>
>>>>>     I'm *pretty* certain that this API is at least as capable as
>>>>>     what we
>>>>>     currently have, if not more so, and IMHO provides a far
>>>>>     simpler and more
>>>>>     versatile design (being able to select different IdentityTypes
>>>>>     in a
>>>>>     single query I think is a big plus).  I'd love to hear any
>>>>>     feedback on
>>>>>     whether you like it, hate it or can think of any improvements
>>>>>     to the
>>>>>     design to make it better for our developers. Also, please think
>>>>>     especially about additional usage scenarios and whether or not
>>>>>     there are
>>>>>     any particular use cases which might be problematic for this API.
>>>>>
>>>>>
>>>>>     Thanks!
>>>>>     Shane
>>>>>     _______________________________________________
>>>>>     security-dev mailing list
>>>>>     security-dev at lists.jboss.org <mailto:security-dev at lists.jboss.org>
>>>>>     https://lists.jboss.org/mailman/listinfo/security-dev
>>>>>     _______________________________________________
>>>>>     security-dev mailing list
>>>>>     security-dev at lists.jboss.org <mailto: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 <http://keyserver.net>, 
>>>>> pgp.mit.edu <http://pgp.mit.edu>
>>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/security-dev/attachments/20121030/8f9a5956/attachment-0001.html 


More information about the security-dev mailing list