[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