[security-dev] IdentityManager review - queries
Shane Bryzak
sbryzak at redhat.com
Tue Oct 30 18:52:50 EDT 2012
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/security-dev/attachments/20121031/9c533359/attachment-0001.html
More information about the security-dev
mailing list