<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">I like this direction very much. <div><br></div><div>For Range what I liked was the .next() method as it made pagination easy. Still API is more readable without Range I'm not going to fight for it. </div><div><br></div><div>One missing part is to get results count if you need to estimate number of pages. Still this is also perf nightmare in some cases on the implementation level and currently you can see more and more sites not exposing number of pages. For example on <a href="http://jboss.org">jboss.org</a> forums you cannot jump to the last page anymore (only next). So maybe it is good idea to not expose it for such reasons. </div><div><br></div><div>QueryParameter also looks good.</div><div><br></div><div><div><div>On Oct 30, 2012, at 11:41 AM, Shane Bryzak <<a href="mailto:sbryzak@redhat.com">sbryzak@redhat.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">I've managed to trim the IdentityQuery
API down to just this:<br>
<br>
public interface IdentityQuery<T extends IdentityType> {<br>
public enum Operator { equals, notEquals, greaterThan,
lessThan };<br>
IdentityQuery<T> setOffset(int offset);<br>
IdentityQuery<T> setLimit(int limit);<br>
IdentityQuery<T> setParameter(QueryParameter param,
Object value);<br>
IdentityQuery<T> setParameter(QueryParameter param,
Operator operator, Object value);<br>
List<T> getResultList();<br>
}<br>
<br>
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.<br>
<br>
To get rid of the setAttribute() methods, I had to do a little bit
of hackery by adding this to the IdentityType interface:<br>
<br>
public class AttributeParameter implements QueryParameter {<br>
private String name;<br>
public AttributeParameter(String name) {<br>
this.name = name;<br>
}<br>
public String getName() {<br>
return name;<br>
}<br>
}<br>
public final class PARAM_ATTRIBUTE {<br>
public static AttributeParameter byName(String name) {<br>
return new AttributeParameter(name);<br>
}<br>
}<br>
<br>
This then allows us to simply use the setParameter() method to set
attribute values also, like so:<br>
<br>
List<User> users = im.<User>createQuery()<br>
.setParameter(User.PARAM_ENABLED, true)<br>
.setParameter(User.PARAM_ATTRIBUTE.byName("foo"),
"bar")<br>
.getResultList();<br>
<br>
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?<br>
<br>
<br>
On 10/30/2012 01:49 PM, Jason Porter wrote:<br>
</div>
<blockquote cite="mid:61BF871B-C98B-4D3A-83E5-5DCD2EFD07EC@gmail.com" type="cite">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div>On Oct 29, 2012, at 21:31, Shane Bryzak <<a moz-do-not-send="true" href="mailto:sbryzak@redhat.com">sbryzak@redhat.com</a>>
wrote:</div>
<div><br>
</div>
<blockquote type="cite">
<div>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">On 10/30/2012 12:32 PM, Jason
Porter wrote:<br>
</div>
<blockquote cite="mid:342B0DF8-E14E-4440-9863-F16670BD6FCE@gmail.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<div>On Oct 29, 2012, at 17:20, Shane Bryzak <<a moz-do-not-send="true" href="mailto:sbryzak@redhat.com">sbryzak@redhat.com</a>>
wrote:</div>
<div><br>
</div>
<blockquote type="cite">
<div>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">On 10/30/2012 01:20 AM,
Jason Porter wrote:<br>
</div>
<blockquote cite="mid:CAF9TksM6DKBaAsqr+HannN-vHptPCUFN4m-_=8MNCeOW6_CLwg@mail.gmail.com" type="cite">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.<br>
</blockquote>
<br>
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:<br>
<br>
public interface QueryParameter {<br>
<br>
}<br>
<br>
Then we can simply define supported parameters directly
on the core identity model interfaces, as Pedro has
suggested:<br>
<br>
public interface User extends IdentityType, Serializable
{<br>
public static final QueryParameter FIRST_NAME = new
QueryParameter() {};<br>
public static final QueryParameter LAST_NAME = new
QueryParameter() {};<br>
...<br>
}<br>
<br>
Then the method signature for IdentityQuery.setParameter
would look something like this:<br>
<br>
IdentityQuery<T> setParameter(QueryParameter
param, Object value);<br>
<br>
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).<br>
</div>
</blockquote>
<div><br>
</div>
<div>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. <br>
</div>
</blockquote>
<br>
I'm not quite getting this - could you give an example of what
the API would look like?<br>
</div>
</blockquote>
<div><br>
</div>
<div>I've thought about it a bit more and like your proposal
better for the enhanced readability. </div>
<br>
<blockquote type="cite">
<div>
<blockquote cite="mid:342B0DF8-E14E-4440-9863-F16670BD6FCE@gmail.com" type="cite">
<div><br>
</div>
<div>Though the final empty interfaces from the classes does
read nicely. </div>
<br>
<blockquote type="cite">
<div>
<blockquote cite="mid:CAF9TksM6DKBaAsqr+HannN-vHptPCUFN4m-_=8MNCeOW6_CLwg@mail.gmail.com" type="cite"><br>
<div class="gmail_quote">On Mon, Oct 29, 2012 at 7:10
AM, Pedro Igor Silva <span dir="ltr"><<a moz-do-not-send="true" href="mailto:psilva@redhat.com" target="_blank">psilva@redhat.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
I liked your proposal, it is simple and clear.<br>
<br>
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.<br>
<br>
IdentityManager identityManager = // get
the identity manager;<br>
<br>
Query<User> query =
identityManager.<User>createQuery();<br>
<br>
query<br>
.limit(100)<br>
.offset(1)<br>
.where()<br>
.id("1")<br>
<br>
.property(User.FIRST_NAME,
OPERATOR.equals, "John")<br>
.property(User.EXPIRATION_DATE,
OPERATOR.lessThan, new Date())<br>
<br>
.attribute("ssn", OPERATOR.equals,
"SSN-123")<br>
<br>
.role(new SimpleRole("Payroll
Officer"))<br>
<br>
.group(new
SimpleGroup("Superuser", "Superuser", null));<br>
<br>
List<User> list = query.list();<br>
<br>
for (User user : list) {<br>
// handle users<br>
}<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
Not sure if we need the query.reset() method ...
Why not just discard the query instance and build
another one ?<br>
<br>
Regards.<br>
<span class="HOEnZb"><font color="#888888">Pedro
Igor<br>
</font></span>
<div class="HOEnZb">
<div class="h5"><br>
----- Original Message -----<br>
From: "Shane Bryzak" <<a moz-do-not-send="true" href="mailto:sbryzak@redhat.com">sbryzak@redhat.com</a>><br>
To: <a moz-do-not-send="true" href="mailto:security-dev@lists.jboss.org">security-dev@lists.jboss.org</a><br>
Sent: Monday, October 29, 2012 6:38:14 AM<br>
Subject: [security-dev] IdentityManager review
- queries<br>
<br>
I've started reviewing the IdentityManager
interface to see where we can<br>
improve the API. The first area I'd like to
visit is the Query API, of<br>
which I've come to the conclusion that we need
to do some serious<br>
redesign - the current API is non-intuitive,
too verbose and not future<br>
proof.<br>
<br>
What I'd like to do is throw it all out and
start again, replacing it<br>
with a new cleaner API that looks something
like this:<br>
<br>
public interface IdentityManager {<br>
// <snip other methods><br>
<br>
<T extends IdentityType>
IdentityQuery<T> createQuery();<br>
}<br>
<br>
public interface IdentityQuery<T extends
IdentityType> {<br>
public enum Param {id, key, created,
expired, enabled, firstName,<br>
lastName, email, name, parent, memberOf};<br>
<br>
public enum Operator { equals, notEquals,
greaterThan, lessThan };<br>
<br>
IdentityQuery<T> reset();<br>
<br>
IdentityQuery<T> setParameter(Param
param, Object value);<br>
<br>
IdentityQuery<T> setParameter(Param
param, Operator operator,<br>
Object value);<br>
<br>
IdentityQuery<T>
setAttributeParameter(String attributeName,
Object<br>
value);<br>
<br>
IdentityQuery<T>
setAttributeParameter(String attributeName,<br>
Operator operator, Object value);<br>
<br>
IdentityQuery<T> setRange(Range
range);<br>
<br>
List<T> getResultList();<br>
}<br>
<br>
This unified API basically replaces the 4
separate existing interfaces<br>
we currently have; UserQuery, RoleQuery,
GroupQuery and<br>
MembershipQuery. I've put together a few
usage scenarios to show how it<br>
might work:<br>
<br>
1) Find users with first name 'John':<br>
<br>
List<User> users =
identityManager.<User>createQuery()<br>
.setParameter(Param.firstName, "John")<br>
.getResultList();<br>
<br>
2) Find all expired users:<br>
<br>
List<User> users =
identityManager.<User>createQuery()<br>
.setParameter(Param.expired,
Operator.lessThan, new Date())<br>
.getResultList();<br>
<br>
3) Find all users that are a member of the
"Superuser" group<br>
<br>
List<User> users =
identityManager.<User>createQuery()<br>
.setParameter(Param.memberOf,
identityManager.getGroup("Superuser"))<br>
.getResultList();<br>
<br>
4) Find all sub-groups of the "Employees"
group:<br>
<br>
List<Group> groups =
identityManager.<Group>createQuery()<br>
.setParameter(Param.memberOf,
identityManager.getGroup("Employees"))<br>
.getResultList();<br>
<br>
5) Find all disabled roles:<br>
<br>
List<Role> roles =
identityManager.<Role>createQuery()<br>
.setParameter(Param.enabled, false)<br>
.getResultList();<br>
<br>
6) Find all Users, Groups and Roles that have
been granted the "Payroll<br>
Officer" role in the "Human Resources" group:<br>
<br>
List<IdentityType> identities =
identityManager.<IdentityType>createQuery()<br>
.setParameter(Param.memberOf,
identityManager.getGroup("Human<br>
Resources"))<br>
.setParameter(Param.memberOf,
identityManager.getRole("Payroll<br>
Officer"))<br>
.getResultList();<br>
<br>
7) Find all Users that have an attribute named
"Citizenship" with a<br>
value of "Greenland":<br>
<br>
List<User> users =
identityManager.<User>createQuery()<br>
.setAttributeParameter("Citizenship",
"Greenland")<br>
.getResultList();<br>
<br>
I'm *pretty* certain that this API is at least
as capable as what we<br>
currently have, if not more so, and IMHO
provides a far simpler and more<br>
versatile design (being able to select
different IdentityTypes in a<br>
single query I think is a big plus). I'd love
to hear any feedback on<br>
whether you like it, hate it or can think of
any improvements to the<br>
design to make it better for our developers.
Also, please think<br>
especially about additional usage scenarios
and whether or not there are<br>
any particular use cases which might be
problematic for this API.<br>
<br>
<br>
Thanks!<br>
Shane<br>
_______________________________________________<br>
security-dev mailing list<br>
<a moz-do-not-send="true" href="mailto:security-dev@lists.jboss.org">security-dev@lists.jboss.org</a><br>
<a moz-do-not-send="true" href="https://lists.jboss.org/mailman/listinfo/security-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/security-dev</a><br>
_______________________________________________<br>
security-dev mailing list<br>
<a moz-do-not-send="true" href="mailto:security-dev@lists.jboss.org">security-dev@lists.jboss.org</a><br>
<a moz-do-not-send="true" href="https://lists.jboss.org/mailman/listinfo/security-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/security-dev</a><br>
</div>
</div>
</blockquote>
</div>
<br>
<br clear="all">
<div><br>
</div>
-- <br>
Jason Porter<br>
<a moz-do-not-send="true" href="http://lightguard-jp.blogspot.com/" target="_blank">http://lightguard-jp.blogspot.com</a><br>
<a moz-do-not-send="true" href="http://twitter.com/lightguardjp" target="_blank">http://twitter.com/lightguardjp</a><br>
<br>
Software Engineer<br>
Open Source Advocate<br>
<br>
PGP key id: 926CCFF5<br>
PGP key available at: <a moz-do-not-send="true" href="http://keyserver.net/" target="_blank">keyserver.net</a>,
<a moz-do-not-send="true" href="http://pgp.mit.edu/" target="_blank">pgp.mit.edu</a><br>
</blockquote>
<br>
</div>
</blockquote>
</blockquote>
<br>
</div>
</blockquote>
</blockquote>
<br>
</div>
_______________________________________________<br>security-dev mailing list<br><a href="mailto:security-dev@lists.jboss.org">security-dev@lists.jboss.org</a><br>https://lists.jboss.org/mailman/listinfo/security-dev<br></blockquote></div><br></div></body></html>