Re: [security-dev] IdentityManager review - queries

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.

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> >