As an implementation we could say a null creation date will be set to today, otherwise we'll use what is set. This could actually be important for people migrating.<div class="gmail_extra"><br><br><div class="gmail_quote">
On Wed, Nov 7, 2012 at 1:08 PM, Shane Bryzak <span dir="ltr"><<a href="mailto:sbryzak@redhat.com" target="_blank">sbryzak@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>They're actually a fundamental part of
the identity model (see [1]). I have no real problem with the
principle of removing the String versions of createGroup() (we
would also have to do the same to createRole() for consistency)
and in fact it would provide some additional advantages. For
example, being able to set a Group to being disabled at creation
time, setting attribute values, etc. My only concern is from a
coding "correctness" point of view, and I guess is centered around
the creation date being automatically set (or potentially
overridden) on the Group instance that's passed to createGroup().
It's probably not an important concern though, and I'm happy to
concede on this one which would mean we end up with the following
methods (replacing all existing createGroup() and createRole()
methods):<br>
<br>
void createGroup(Group group);<br>
<br>
void createRole(Role role);<br>
<br>
<br>
[1]
<a href="https://github.com/picketlink/picketlink/blob/master/idm/api/src/main/java/org/picketlink/idm/model/IdentityType.java" target="_blank">https://github.com/picketlink/picketlink/blob/master/idm/api/src/main/java/org/picketlink/idm/model/IdentityType.java</a><div>
<div class="h5"><br>
<br>
On 11/08/2012 05:37 AM, Jason Porter wrote:<br>
</div></div></div><div><div class="h5">
<blockquote type="cite">Aren't those implementation details though?</blockquote>
<blockquote type="cite">
<div class="gmail_extra">
<div class="gmail_quote">On Wed, Nov 7, 2012 at 12:16 PM, Shane
Bryzak <span dir="ltr"><<a href="mailto:sbryzak@redhat.com" target="_blank">sbryzak@redhat.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>
<div>
<div>On 11/08/2012 04:08 AM, Jason Porter wrote:<br>
</div>
<blockquote type="cite">Replies inline
<div class="gmail_extra"><br>
<div class="gmail_quote">On Wed, Nov 7, 2012 at
3:54 AM, Shane Bryzak <span dir="ltr"><<a href="mailto:sbryzak@redhat.com" target="_blank">sbryzak@redhat.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey guys,<br>
<br>
I spent some time reviewing the
IdentityManager API today to identify<br>
any redundancies and also locate any holes
where we might not properly<br>
support required features, as well as a
general "sanity" check to ensure<br>
that what we're exposing via this API makes
sense and is intuitive for<br>
consumers. As Anil has pointed out already,
our short term priority for<br>
the project is to stabilize the API - this is
extremely important as<br>
PicketLink will provide the security
foundation for many other<br>
projects. With that in mind I'd like to
strongly encourage everyone<br>
with a stake in this to carefully review the
API and provide feedback,<br>
as once we release it it will be essentially
set in stone.<br>
<br>
To try and avoid a wall of text (and make this
post easier to reply to),<br>
I'm going to break this post up into
sub-sections, one for each feature<br>
group. For each section, I'll include the API
as it currently exists,<br>
followed by a brief summary of my thoughts and
any recommendations I may<br>
have - this is especially where I want to hear
any feedback indicating<br>
whether you agree or disagree with my
assessment. Let's start with the<br>
user-related methods:<br>
<br>
User management<br>
-------------------------<br>
<br>
User createUser(String name);<br>
<br>
void createUser(User user);<br>
<br>
void removeUser(User user);<br>
<br>
void removeUser(String name);<br>
<br>
User getUser(String name);<br>
<br>
My thoughts:<br>
<br>
1. I'm wondering if we should remove the
createUser(String) method and<br>
just have createUser(User). This would make
sense in a way because I<br>
can't think of many use cases where you might
want to create a User with<br>
just a username and not any of the other
typical information (such as<br>
first name, last name, e-mail address, etc).
Creating a User via<br>
createUser(String) when you actually want to
set the other details after<br>
the initial User creation is also horribly
inefficient, requiring<br>
multiple round trips to the database (or
whatever identity store backend<br>
you use). To add to that, the actual
parameter name is a little<br>
unintuitive - "name" could refer to any number
of things - is it the<br>
username or the user's actual name that you
need to provide here? My +1<br>
goes to removing createUser(String).<br>
</blockquote>
<div><br>
</div>
<div>I like removing the strings. For most of
the rest of this API we have a very type safe
idea here. Maybe I'm thinking a little
overboard on things, but I really like the
idea of having wrappers around things (simple
wrappers for our API so people could implement
an interface or use a base class or something
to be able to interact with our API but still
easily get to the information they need). </div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 2. Similar to point
1), having two removeUser() methods seems
equally<br>
redundant. The User object that needs to be
provided to the<br>
removeUser(User) method can be easily looked
up by calling getUser(), as<br>
per this example:<br>
<br>
identityManager.removeUser(identityManager.getUser("jsmith"));<br>
</blockquote>
<div><br>
</div>
<div>+1</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
3. As I also pointed out in point 1), the
"name" parameter is a little<br>
unintuitive. In regard to the getUser()
method I think we should rename<br>
the "name" parameter to "username" just so
it's perfectly clear what the<br>
method expects.<br>
</blockquote>
<div><br>
</div>
<div>+1</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 4. We don't currently
have any way to update User details. I
recommend<br>
that we add an updateUser() method as follows:<br>
<br>
void updateUser(User user);<br>
</blockquote>
<div><br>
</div>
<div>I like this idea. I've seen on a few sites
where you start with a basic username and
password, then your confirmation is to
complete their identity for yourself, so it's
not completely out there that people wouldn't
do this.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> This will allow User
details such as their first and last name,
e-mail<br>
address etc to be updated without having to
delete and re-create the<br>
User. Extending on this a little further, I'm
wondering if we should<br>
think about adding an audit API that logs
these changes. Perhaps it's<br>
something to think about for a later release.<br>
<br>
Group management<br>
--------------------------<br>
<br>
Group createGroup(String id);<br>
<br>
Group createGroup(String id, Group
parent);<br>
<br>
Group createGroup(String id, String
parent);<br>
<br>
void removeGroup(Group group);<br>
<br>
void removeGroup(String groupId);<br>
<br>
Group getGroup(String groupId);<br>
<br>
Group getGroup(String groupId, Group
parent);<br>
<br>
void addToGroup(IdentityType
identityType, Group group);<br>
<br>
void removeFromGroup(IdentityType
identityType, Group group);<br>
<br>
My thoughts:<br>
<br>
1. I think there is a little bit of ambiguity
here in regard to the<br>
differences between a Group's ID and name. To
assist in understanding<br>
the differences, here's the JavaDoc pasted
from the Group interface:<br>
<br>
/**<br>
* Groups are stored in tree hierarchy
and therefore ID represents<br>
a path. ID string always begins with "/"
element that<br>
* represents root of the tree<br>
* <p/><br>
* Example: Valid IDs are
"/acme/departments/marketing",<br>
"/security/administrator" or "/administrator".
Where "acme",<br>
* "departments", "marketing", "security"
and "administrator" are<br>
group names.<br>
*<br>
* @return Group Id in String
representation.<br>
*/<br>
String getId();<br>
<br>
/**<br>
* Group name is unique identifier in
specific group tree branch.<br>
For example group with id
"/acme/departments/marketing"<br>
* will have name "marketing" and parent
group of id<br>
"/acme/departments"<br>
*<br>
* @return name<br>
*/<br>
String getName();<br>
<br>
Taking the above JavaDoc into account, it
seems that where we are<br>
specifying "id" as a method parameter we
should be specifying "name"<br>
instead. If we are indeed providing a "fully
qualified" path to the<br>
createGroup() method, then there is no need to
also specify the parent<br>
as it can be easily derived from the path.
More on this in the<br>
following points.<br>
<br>
2. We currently have three createGroup()
methods for creating a new<br>
Group. I think we should remove the (String,
String) variant, as the<br>
parent parameter is ambiguous (is it the
parent's ID or name we need to<br>
specify here?), and rename the "id" parameter
of the (String, Group)<br>
variant to "name", leaving us with the
following:<br>
<br>
Group createGroup(String id);<br>
<br>
Group createGroup(String name, Group
parent);<br>
<br>
This then gives us two ways of creating a
group. We can either specify<br>
the fully qualified group ID:<br>
<br>
Group employees =
identityManager.createGroup("/employees");<br>
<br>
Or we can specify a subgroup name and parent
Group:<br>
<br>
Group managers =
identityManager.createGroup("managers",
employees);<br>
<br>
We can also use this second form to create a
"root" group by just<br>
specifying null for the parent:<br>
<br>
Group admins =
identityManager.createGroup("admins", null);<br>
</blockquote>
<div><br>
</div>
<div>I mentioned this earlier, but I don't see
why we need the Strings. We could simply use
Group and have a basic constructor that would
take a String for the name and possibly
another one with a String and a parent group.</div>
</div>
</div>
</blockquote>
<br>
</div>
</div>
My only reservation here is that the createGroup() method
(this is true of createRole() also) sets some additional
state during Group (or Role) creation, such as the created
date and enabled status. I'm not a big fan of mutating
the object that's passed in like this, which is why I kept
the String variants of these methods, as a Group (and Role
also) is little more than its name. Of course, User
breaks this rule but I see it as being an exception as it
requires a whole bunch of extended information beyond just
the username itself. I could make this clearer with an
example if it helps.
<div>
<div><br>
<br>
<blockquote type="cite">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
3. For the removeGroup() methods, I think what
we have is fine.<br>
<br>
4. For the getGroup() methods, I'm happy with
the first one (String<br>
groupId) but I think the second one (String
groupId, Group parent) needs<br>
to be (String name, Group parent) instead.
The first method would be<br>
used when the fully qualified group ID is
known, and the second one<br>
would be used when you already have the parent
Group and know the name<br>
of the subgroup.<br>
<br>
5. The addToGroup() and removeFromGroup()
methods seem fine to me.<br>
<br>
6. One thing we don't have is a method to test
whether an IdentityType<br>
is a member of a Group. I suggest we add
another method to support this:<br>
<br>
boolean inGroup(IdentityType identityType,
Group group);<br>
<br>
</blockquote>
<div><br>
</div>
<div>+1 for the above (with the type idea for
#4).</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Roles<br>
----------<br>
<br>
Role createRole(String name);<br>
<br>
void removeRole(Role role);<br>
<br>
void removeRole(String name);<br>
<br>
Role getRole(String name);<br>
<br>
boolean hasRole(Role role, IdentityType
identityType, Group group);<br>
<br>
void grantRole(Role role, IdentityType
identityType, Group group);<br>
<br>
void revokeRole(Role role, IdentityType
identityType, Group group);<br>
<br>
My thoughts:<br>
<br>
1. I would remove the removeRole(String)
method as the same thing can be<br>
achieved with removeRole(getRole(String)).<br>
</blockquote>
<div><br>
</div>
<div>+1</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 2. I would swap the
order of the Role and IdentityType parameters
within<br>
the hasRole(), grantRole() and revokeRole()
methods. As the<br>
IdentityType is the primary artifact in these
operations it makes sense<br>
to have it listed first.<br>
</blockquote>
<div><br>
</div>
<div>+1</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">3. We don't have any
explicit support for application roles, which
are<br>
roles where there is no Group component (for
example, an<br>
application-wide "admin" role). We could
support this by just allowing<br>
hasRole(), grantRole() etc to accept null
values for the Group<br>
parameter, however I feel it would be more
semantically correct to<br>
provide a distinct set of methods for the
purpose of supporting<br>
application roles, as follows:<br>
<br>
boolean hasApplicationRole(IdentityType
identityType, Role role);<br>
<br>
void grantApplicationRole(IdentityType
identityType, Role role);<br>
<br>
void revokeApplicationRole(IdentityType
identityType, Role role);<br>
</blockquote>
<div><br>
</div>
<div>+1</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Query API<br>
--------------<br>
<br>
<T extends IdentityType>
IdentityQuery<T> createQuery();<br>
<br>
My thoughts:<br>
<br>
1. After our recent rewrite of the Query API
I'm satisfied with what we<br>
have now.<br>
<br>
Credential management<br>
-------------------------------<br>
<br>
boolean validateCredential(User user,
Credential credential);<br>
<br>
void updateCredential(User user,
Credential credential);<br>
<br>
My thoughts:<br>
<br>
1. I'm satisfied with what we currently have,
however we still need to<br>
review the mechanism that we provide for
encoding passwords. I'm not<br>
sure that it will have any effect on these
methods though (I'm toying<br>
with the idea of integrating the password
encoding functionality via the<br>
IdentityStoreInvocationContext).<br>
<br>
Identity expiry<br>
------------------<br>
<br>
void setEnabled(IdentityType
identityType, boolean enabled);<br>
<br>
void setExpirationDate(IdentityType
identityType, Date expirationDate);<br>
<br>
My thoughts:<br>
<br>
1. I think these methods are fine the way they
are. One discrepancy<br>
that I've identified is that a User can be
created already being<br>
disabled or having an expiry date, while for a
Role or Group the<br>
equivalent status must be set via these
methods after creation. I'm not<br>
sure this is a big deal though.<br>
</blockquote>
<div><br>
</div>
<div>All good.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Attributes<br>
-------------<br>
<br>
void setAttribute(IdentityType
identityType, String attributeName,<br>
String attributeValue);<br>
<br>
String getAttribute(IdentityType
identityType, String attributeName);<br>
<br>
My thoughts:<br>
<br>
1. This area poses a slight dilemma.
Currently the API only supports<br>
simple String-based attribute values, however
I'm pretty sure that<br>
people are going to want to store all sorts of
things, ranging from<br>
boolean and Date values through to large byte
arrays that store a User's<br>
photograph or other data. If everyone is in
agreement that we need to<br>
support more than just String values, then the
first thing we probably<br>
need to do is modify these methods to work
with a Serializable instead.<br>
<br>
2. If we all agree on point 1), the next thing
we need to decide is<br>
whether we want the capability to make some
attribute values "lazy<br>
loaded". If we want to do a quick lookup of a
User object for the<br>
express purpose of assigning a Role or Group
membership (or any other<br>
type of simple operation) then we probably
don't want the performance<br>
hit of having to load bulky attribute data.<br>
<br>
3. With the above two points in mind, I'm
going to hold off on surmising<br>
any further on attributes until some of you
guys weigh in with your<br>
opinions. I do have some rough ideas, however
I'll wait until we have a<br>
consensus of exactly what we want to achieve
here before we proceed further.<br>
</blockquote>
<div><br>
</div>
<div>Serializable works for me. Do we do have a
generic Attribute<? extends
Serializable> or just forgo the generics?</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Utility methods<br>
--------------------<br>
<br>
IdentityType lookupIdentityByKey(String
key);<br>
<br>
My thoughts:<br>
<br>
1. This method is required by other features,
such as the Permissions<br>
API. I'm fine with what we have here.<br>
<br>
In summary<br>
----------------<br>
<br>
I apologise for yet another epic e-mail to the
list, however this is<br>
everyone's chance to review the API for
themselves and decide whether or<br>
not it will be suitable for addressing all of
your requirements. I'd<br>
like to get this API stable by next week so
please don't be shy about<br>
speaking up. I'm especially looking forward
to hearing your opinions<br>
about how we handle attributes (there may even
be some relevant JSR-351<br>
stuff that we should be looking at here).<br>
<br>
Thanks,<br>
Shane<br>
_______________________________________________<br>
security-dev mailing list<br>
<a href="mailto:security-dev@lists.jboss.org" target="_blank">security-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/security-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/security-dev</a><br>
</blockquote>
</div>
<br>
<br clear="all">
<div><br>
</div>
-- <br>
Jason Porter<br>
<a href="http://lightguard-jp.blogspot.com" target="_blank">http://lightguard-jp.blogspot.com</a><br>
<a 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 href="http://keyserver.net" target="_blank">keyserver.net</a>,
<a href="http://pgp.mit.edu" target="_blank">pgp.mit.edu</a><br>
</div>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
<br clear="all">
<div><br>
</div>
-- <br>
Jason Porter<br>
<a href="http://lightguard-jp.blogspot.com" target="_blank">http://lightguard-jp.blogspot.com</a><br>
<a 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 href="http://keyserver.net" target="_blank">keyserver.net</a>,
<a href="http://pgp.mit.edu" target="_blank">pgp.mit.edu</a><br>
</div>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>Jason Porter<br><a href="http://lightguard-jp.blogspot.com" target="_blank">http://lightguard-jp.blogspot.com</a><br><a 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 href="http://keyserver.net" target="_blank">keyserver.net</a>, <a href="http://pgp.mit.edu" target="_blank">pgp.mit.edu</a><br>
</div>