[security-dev] IdentityManager review - queries

Shane Bryzak sbryzak at redhat.com
Tue Oct 30 19:02:23 EDT 2012


I knew I shouldn't have clicked the "Send" button so quickly. :)  After 
5 minutes of thinking about this, there would be no reason to deprecate 
the Query API.  JPA supports both a query language and a criteria API, 
so there's no reason why we couldn't do both.

On 10/31/2012 08:52 AM, Shane Bryzak wrote:
> I've made some further minor changes to the Query API, specifically in 
> the area of memberships.  By introducing a new QueryParameter 
> implementation called MembershipParameter, we are now able to support 
> queries that look like this:
>
>         List<User> users = this.<User>createQuery()
>                 .setParameter(User.MEMBER_OF.group("Head 
> Office").role("Admin"), true)
> .setParameter(User.MEMBER_OF.group("Springfield"), false)
>                 .setParameter(User.MEMBER_OF.role("Contractor"), false)
>                 .getResultList();
>
> Translation of the above code: return all users that have the "Admin" 
> role in the "Head Office" group, but aren't a member of the 
> "Springfield" group and don't have the application role "Contractor".
>
> This support for membership exclusion gives us a little bit more 
> flexibility in what we can return from the IdentityQuery.  I have to 
> say though, I'm still not 100% satisfied with the Query API - my 
> opinion is that we should mark this feature as already deprecated in 
> the 3.0 release, and in 3.1 introduce a proper query language (PLQL - 
> PicketLink Query Language) where the developer is able to provide 
> free-form queries, e.g. being able to run something like "SELECT User 
> WHERE MEMBER_OF(Group("Head Office"), Role("Office Administrator")) 
> and enabled = true and createdDate < :createdDate and not in (SELECT 
> User WHERE NOT MEMBER_OF("Admin"))".
>
> Maybe this feature would be going too far, so please let me know if 
> you think it is.
>
>
> On 10/30/2012 08:41 PM, Shane Bryzak wrote:
>> 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>
>>>>>>
>>>>
>>
>>
>>
>> _______________________________________________
>> security-dev mailing list
>> security-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/security-dev
>
>
>
> _______________________________________________
> security-dev mailing list
> security-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/security-dev

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


More information about the security-dev mailing list