Re: [security-dev] IdentityManager review - queries

On Oct 29, 2012, at 21:31, Shane Bryzak <sbryzak(a)redhat.com <mailto:sbryzak@redhat.com>> wrote: > On 10/30/2012 12:32 PM, Jason Porter wrote: >> On Oct 29, 2012, at 17:20, Shane Bryzak <sbryzak(a)redhat.com >> <mailto:sbryzak@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(a)redhat.com <mailto:psilva@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(a)redhat.com >>>> <mailto:sbryzak@redhat.com>> >>>> To: security-dev(a)lists.jboss.org >>>> <mailto:security-dev@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(a)lists.jboss.org <mailto:security-dev@lists.jboss.org> >>>> https://lists.jboss.org/mailman/listinfo/security-dev >>>> _______________________________________________ >>>> security-dev mailing list >>>> security-dev(a)lists.jboss.org <mailto:security-dev@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> >>> >