[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