4:54 a.m.
Hey guys,
I spent some time reviewing the IdentityManager API today to identify
any redundancies and also locate any holes where we might not properly
support required features, as well as a general "sanity" check to ensure
that what we're exposing via this API makes sense and is intuitive for
consumers. As Anil has pointed out already, our short term priority for
the project is to stabilize the API - this is extremely important as
PicketLink will provide the security foundation for many other
projects. With that in mind I'd like to strongly encourage everyone
with a stake in this to carefully review the API and provide feedback,
as once we release it it will be essentially set in stone.
To try and avoid a wall of text (and make this post easier to reply to),
I'm going to break this post up into sub-sections, one for each feature
group. For each section, I'll include the API as it currently exists,
followed by a brief summary of my thoughts and any recommendations I may
have - this is especially where I want to hear any feedback indicating
whether you agree or disagree with my assessment. Let's start with the
user-related methods:
User management
-------------------------
User createUser(String name);
void createUser(User user);
void removeUser(User user);
void removeUser(String name);
User getUser(String name);
My thoughts:
1. I'm wondering if we should remove the createUser(String) method and
just have createUser(User). This would make sense in a way because I
can't think of many use cases where you might want to create a User with
just a username and not any of the other typical information (such as
first name, last name, e-mail address, etc). Creating a User via
createUser(String) when you actually want to set the other details after
the initial User creation is also horribly inefficient, requiring
multiple round trips to the database (or whatever identity store backend
you use). To add to that, the actual parameter name is a little
unintuitive - "name" could refer to any number of things - is it the
username or the user's actual name that you need to provide here? My +1
goes to removing createUser(String).
2. Similar to point 1), having two removeUser() methods seems equally
redundant. The User object that needs to be provided to the
removeUser(User) method can be easily looked up by calling getUser(), as
per this example:
identityManager.removeUser(identityManager.getUser("jsmith"));
3. As I also pointed out in point 1), the "name" parameter is a little
unintuitive. In regard to the getUser() method I think we should rename
the "name" parameter to "username" just so it's perfectly clear
what the
method expects.
4. We don't currently have any way to update User details. I recommend
that we add an updateUser() method as follows:
void updateUser(User user);
This will allow User details such as their first and last name, e-mail
address etc to be updated without having to delete and re-create the
User. Extending on this a little further, I'm wondering if we should
think about adding an audit API that logs these changes. Perhaps it's
something to think about for a later release.
Group management
--------------------------
Group createGroup(String id);
Group createGroup(String id, Group parent);
Group createGroup(String id, String parent);
void removeGroup(Group group);
void removeGroup(String groupId);
Group getGroup(String groupId);
Group getGroup(String groupId, Group parent);
void addToGroup(IdentityType identityType, Group group);
void removeFromGroup(IdentityType identityType, Group group);
My thoughts:
1. I think there is a little bit of ambiguity here in regard to the
differences between a Group's ID and name. To assist in understanding
the differences, here's the JavaDoc pasted from the Group interface:
/**
* Groups are stored in tree hierarchy and therefore ID represents
a path. ID string always begins with "/" element that
* represents root of the tree
* <p/>
* Example: Valid IDs are "/acme/departments/marketing",
"/security/administrator" or "/administrator". Where
"acme",
* "departments", "marketing", "security" and
"administrator" are
group names.
*
* @return Group Id in String representation.
*/
String getId();
/**
* Group name is unique identifier in specific group tree branch.
For example group with id "/acme/departments/marketing"
* will have name "marketing" and parent group of id
"/acme/departments"
*
* @return name
*/
String getName();
Taking the above JavaDoc into account, it seems that where we are
specifying "id" as a method parameter we should be specifying "name"
instead. If we are indeed providing a "fully qualified" path to the
createGroup() method, then there is no need to also specify the parent
as it can be easily derived from the path. More on this in the
following points.
2. We currently have three createGroup() methods for creating a new
Group. I think we should remove the (String, String) variant, as the
parent parameter is ambiguous (is it the parent's ID or name we need to
specify here?), and rename the "id" parameter of the (String, Group)
variant to "name", leaving us with the following:
Group createGroup(String id);
Group createGroup(String name, Group parent);
This then gives us two ways of creating a group. We can either specify
the fully qualified group ID:
Group employees = identityManager.createGroup("/employees");
Or we can specify a subgroup name and parent Group:
Group managers = identityManager.createGroup("managers", employees);
We can also use this second form to create a "root" group by just
specifying null for the parent:
Group admins = identityManager.createGroup("admins", null);
3. For the removeGroup() methods, I think what we have is fine.
4. For the getGroup() methods, I'm happy with the first one (String
groupId) but I think the second one (String groupId, Group parent) needs
to be (String name, Group parent) instead. The first method would be
used when the fully qualified group ID is known, and the second one
would be used when you already have the parent Group and know the name
of the subgroup.
5. The addToGroup() and removeFromGroup() methods seem fine to me.
6. One thing we don't have is a method to test whether an IdentityType
is a member of a Group. I suggest we add another method to support this:
boolean inGroup(IdentityType identityType, Group group);
Roles
----------
Role createRole(String name);
void removeRole(Role role);
void removeRole(String name);
Role getRole(String name);
boolean hasRole(Role role, IdentityType identityType, Group group);
void grantRole(Role role, IdentityType identityType, Group group);
void revokeRole(Role role, IdentityType identityType, Group group);
My thoughts:
1. I would remove the removeRole(String) method as the same thing can be
achieved with removeRole(getRole(String)).
2. I would swap the order of the Role and IdentityType parameters within
the hasRole(), grantRole() and revokeRole() methods. As the
IdentityType is the primary artifact in these operations it makes sense
to have it listed first.
3. We don't have any explicit support for application roles, which are
roles where there is no Group component (for example, an
application-wide "admin" role). We could support this by just allowing
hasRole(), grantRole() etc to accept null values for the Group
parameter, however I feel it would be more semantically correct to
provide a distinct set of methods for the purpose of supporting
application roles, as follows:
boolean hasApplicationRole(IdentityType identityType, Role role);
void grantApplicationRole(IdentityType identityType, Role role);
void revokeApplicationRole(IdentityType identityType, Role role);
Query API
--------------
<T extends IdentityType> IdentityQuery<T> createQuery();
My thoughts:
1. After our recent rewrite of the Query API I'm satisfied with what we
have now.
Credential management
-------------------------------
boolean validateCredential(User user, Credential credential);
void updateCredential(User user, Credential credential);
My thoughts:
1. I'm satisfied with what we currently have, however we still need to
review the mechanism that we provide for encoding passwords. I'm not
sure that it will have any effect on these methods though (I'm toying
with the idea of integrating the password encoding functionality via the
IdentityStoreInvocationContext).
Identity expiry
------------------
void setEnabled(IdentityType identityType, boolean enabled);
void setExpirationDate(IdentityType identityType, Date expirationDate);
My thoughts:
1. I think these methods are fine the way they are. One discrepancy
that I've identified is that a User can be created already being
disabled or having an expiry date, while for a Role or Group the
equivalent status must be set via these methods after creation. I'm not
sure this is a big deal though.
Attributes
-------------
void setAttribute(IdentityType identityType, String attributeName,
String attributeValue);
String getAttribute(IdentityType identityType, String attributeName);
My thoughts:
1. This area poses a slight dilemma. Currently the API only supports
simple String-based attribute values, however I'm pretty sure that
people are going to want to store all sorts of things, ranging from
boolean and Date values through to large byte arrays that store a User's
photograph or other data. If everyone is in agreement that we need to
support more than just String values, then the first thing we probably
need to do is modify these methods to work with a Serializable instead.
2. If we all agree on point 1), the next thing we need to decide is
whether we want the capability to make some attribute values "lazy
loaded". If we want to do a quick lookup of a User object for the
express purpose of assigning a Role or Group membership (or any other
type of simple operation) then we probably don't want the performance
hit of having to load bulky attribute data.
3. With the above two points in mind, I'm going to hold off on surmising
any further on attributes until some of you guys weigh in with your
opinions. I do have some rough ideas, however I'll wait until we have a
consensus of exactly what we want to achieve here before we proceed further.
Utility methods
--------------------
IdentityType lookupIdentityByKey(String key);
My thoughts:
1. This method is required by other features, such as the Permissions
API. I'm fine with what we have here.
In summary
----------------
I apologise for yet another epic e-mail to the list, however this is
everyone's chance to review the API for themselves and decide whether or
not it will be suitable for addressing all of your requirements. I'd
like to get this API stable by next week so please don't be shy about
speaking up. I'm especially looking forward to hearing your opinions
about how we handle attributes (there may even be some relevant JSR-351
stuff that we should be looking at here).
Thanks,
Shane
12:08 p.m.
Replies inline
On Wed, Nov 7, 2012 at 3:54 AM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
Hey guys,
I spent some time reviewing the IdentityManager API today to identify
any redundancies and also locate any holes where we might not properly
support required features, as well as a general "sanity" check to ensure
that what we're exposing via this API makes sense and is intuitive for
consumers. As Anil has pointed out already, our short term priority for
the project is to stabilize the API - this is extremely important as
PicketLink will provide the security foundation for many other
projects. With that in mind I'd like to strongly encourage everyone
with a stake in this to carefully review the API and provide feedback,
as once we release it it will be essentially set in stone.
To try and avoid a wall of text (and make this post easier to reply to),
I'm going to break this post up into sub-sections, one for each feature
group. For each section, I'll include the API as it currently exists,
followed by a brief summary of my thoughts and any recommendations I may
have - this is especially where I want to hear any feedback indicating
whether you agree or disagree with my assessment. Let's start with the
user-related methods:
User management
-------------------------
User createUser(String name);
void createUser(User user);
void removeUser(User user);
void removeUser(String name);
User getUser(String name);
My thoughts:
1. I'm wondering if we should remove the createUser(String) method and
just have createUser(User). This would make sense in a way because I
can't think of many use cases where you might want to create a User with
just a username and not any of the other typical information (such as
first name, last name, e-mail address, etc). Creating a User via
createUser(String) when you actually want to set the other details after
the initial User creation is also horribly inefficient, requiring
multiple round trips to the database (or whatever identity store backend
you use). To add to that, the actual parameter name is a little
unintuitive - "name" could refer to any number of things - is it the
username or the user's actual name that you need to provide here? My +1
goes to removing createUser(String).
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).
2. Similar to point 1), having two removeUser() methods seems
equally
redundant. The User object that needs to be provided to the
removeUser(User) method can be easily looked up by calling getUser(), as
per this example:
identityManager.removeUser(identityManager.getUser("jsmith"));
+1
3. As I also pointed out in point 1), the "name" parameter is a little
unintuitive. In regard to the getUser() method I think we should rename
the "name" parameter to "username" just so it's perfectly clear
what the
method expects.
+1
4. We don't currently have any way to update User details. I
recommend
that we add an updateUser() method as follows:
void updateUser(User user);
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.
This will allow User details such as their first and last name,
e-mail
address etc to be updated without having to delete and re-create the
User. Extending on this a little further, I'm wondering if we should
think about adding an audit API that logs these changes. Perhaps it's
something to think about for a later release.
Group management
--------------------------
Group createGroup(String id);
Group createGroup(String id, Group parent);
Group createGroup(String id, String parent);
void removeGroup(Group group);
void removeGroup(String groupId);
Group getGroup(String groupId);
Group getGroup(String groupId, Group parent);
void addToGroup(IdentityType identityType, Group group);
void removeFromGroup(IdentityType identityType, Group group);
My thoughts:
1. I think there is a little bit of ambiguity here in regard to the
differences between a Group's ID and name. To assist in understanding
the differences, here's the JavaDoc pasted from the Group interface:
/**
* Groups are stored in tree hierarchy and therefore ID represents
a path. ID string always begins with "/" element that
* represents root of the tree
* <p/>
* Example: Valid IDs are "/acme/departments/marketing",
"/security/administrator" or "/administrator". Where
"acme",
* "departments", "marketing", "security" and
"administrator" are
group names.
*
* @return Group Id in String representation.
*/
String getId();
/**
* Group name is unique identifier in specific group tree branch.
For example group with id "/acme/departments/marketing"
* will have name "marketing" and parent group of id
"/acme/departments"
*
* @return name
*/
String getName();
Taking the above JavaDoc into account, it seems that where we are
specifying "id" as a method parameter we should be specifying "name"
instead. If we are indeed providing a "fully qualified" path to the
createGroup() method, then there is no need to also specify the parent
as it can be easily derived from the path. More on this in the
following points.
2. We currently have three createGroup() methods for creating a new
Group. I think we should remove the (String, String) variant, as the
parent parameter is ambiguous (is it the parent's ID or name we need to
specify here?), and rename the "id" parameter of the (String, Group)
variant to "name", leaving us with the following:
Group createGroup(String id);
Group createGroup(String name, Group parent);
This then gives us two ways of creating a group. We can either specify
the fully qualified group ID:
Group employees = identityManager.createGroup("/employees");
Or we can specify a subgroup name and parent Group:
Group managers = identityManager.createGroup("managers", employees);
We can also use this second form to create a "root" group by just
specifying null for the parent:
Group admins = identityManager.createGroup("admins", null);
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.
3. For the removeGroup() methods, I think what we have is fine.
4. For the getGroup() methods, I'm happy with the first one (String
groupId) but I think the second one (String groupId, Group parent) needs
to be (String name, Group parent) instead. The first method would be
used when the fully qualified group ID is known, and the second one
would be used when you already have the parent Group and know the name
of the subgroup.
5. The addToGroup() and removeFromGroup() methods seem fine to me.
6. One thing we don't have is a method to test whether an IdentityType
is a member of a Group. I suggest we add another method to support this:
boolean inGroup(IdentityType identityType, Group group);
+1 for the above (with the type idea for #4).
Roles
----------
Role createRole(String name);
void removeRole(Role role);
void removeRole(String name);
Role getRole(String name);
boolean hasRole(Role role, IdentityType identityType, Group group);
void grantRole(Role role, IdentityType identityType, Group group);
void revokeRole(Role role, IdentityType identityType, Group group);
My thoughts:
1. I would remove the removeRole(String) method as the same thing can be
achieved with removeRole(getRole(String)).
+1
2. I would swap the order of the Role and IdentityType parameters
within
the hasRole(), grantRole() and revokeRole() methods. As the
IdentityType is the primary artifact in these operations it makes sense
to have it listed first.
+1
3. We don't have any explicit support for application roles,
which are
roles where there is no Group component (for example, an
application-wide "admin" role). We could support this by just allowing
hasRole(), grantRole() etc to accept null values for the Group
parameter, however I feel it would be more semantically correct to
provide a distinct set of methods for the purpose of supporting
application roles, as follows:
boolean hasApplicationRole(IdentityType identityType, Role role);
void grantApplicationRole(IdentityType identityType, Role role);
void revokeApplicationRole(IdentityType identityType, Role role);
+1
Query API
--------------
<T extends IdentityType> IdentityQuery<T> createQuery();
My thoughts:
1. After our recent rewrite of the Query API I'm satisfied with what we
have now.
Credential management
-------------------------------
boolean validateCredential(User user, Credential credential);
void updateCredential(User user, Credential credential);
My thoughts:
1. I'm satisfied with what we currently have, however we still need to
review the mechanism that we provide for encoding passwords. I'm not
sure that it will have any effect on these methods though (I'm toying
with the idea of integrating the password encoding functionality via the
IdentityStoreInvocationContext).
Identity expiry
------------------
void setEnabled(IdentityType identityType, boolean enabled);
void setExpirationDate(IdentityType identityType, Date
expirationDate);
My thoughts:
1. I think these methods are fine the way they are. One discrepancy
that I've identified is that a User can be created already being
disabled or having an expiry date, while for a Role or Group the
equivalent status must be set via these methods after creation. I'm not
sure this is a big deal though.
All good.
Attributes
-------------
void setAttribute(IdentityType identityType, String attributeName,
String attributeValue);
String getAttribute(IdentityType identityType, String attributeName);
My thoughts:
1. This area poses a slight dilemma. Currently the API only supports
simple String-based attribute values, however I'm pretty sure that
people are going to want to store all sorts of things, ranging from
boolean and Date values through to large byte arrays that store a User's
photograph or other data. If everyone is in agreement that we need to
support more than just String values, then the first thing we probably
need to do is modify these methods to work with a Serializable instead.
2. If we all agree on point 1), the next thing we need to decide is
whether we want the capability to make some attribute values "lazy
loaded". If we want to do a quick lookup of a User object for the
express purpose of assigning a Role or Group membership (or any other
type of simple operation) then we probably don't want the performance
hit of having to load bulky attribute data.
3. With the above two points in mind, I'm going to hold off on surmising
any further on attributes until some of you guys weigh in with your
opinions. I do have some rough ideas, however I'll wait until we have a
consensus of exactly what we want to achieve here before we proceed
further.
Serializable works for me. Do we do have a generic Attribute<? extends
Serializable> or just forgo the generics?
Utility methods
--------------------
IdentityType lookupIdentityByKey(String key);
My thoughts:
1. This method is required by other features, such as the Permissions
API. I'm fine with what we have here.
In summary
----------------
I apologise for yet another epic e-mail to the list, however this is
everyone's chance to review the API for themselves and decide whether or
not it will be suitable for addressing all of your requirements. I'd
like to get this API stable by next week so please don't be shy about
speaking up. I'm especially looking forward to hearing your opinions
about how we handle attributes (there may even be some relevant JSR-351
stuff that we should be looking at here).
Thanks,
Shane
_______________________________________________
security-dev mailing list
security-dev(a)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, pgp.mit.edu
1:16 p.m.
On 11/08/2012 04:08 AM, Jason Porter wrote:
Replies inline
On Wed, Nov 7, 2012 at 3:54 AM, Shane Bryzak <sbryzak(a)redhat.com
<mailto:sbryzak@redhat.com>> wrote:
Hey guys,
I spent some time reviewing the IdentityManager API today to identify
any redundancies and also locate any holes where we might not properly
support required features, as well as a general "sanity" check to
ensure
that what we're exposing via this API makes sense and is intuitive for
consumers. As Anil has pointed out already, our short term
priority for
the project is to stabilize the API - this is extremely important as
PicketLink will provide the security foundation for many other
projects. With that in mind I'd like to strongly encourage everyone
with a stake in this to carefully review the API and provide feedback,
as once we release it it will be essentially set in stone.
To try and avoid a wall of text (and make this post easier to
reply to),
I'm going to break this post up into sub-sections, one for each
feature
group. For each section, I'll include the API as it currently exists,
followed by a brief summary of my thoughts and any recommendations
I may
have - this is especially where I want to hear any feedback indicating
whether you agree or disagree with my assessment. Let's start
with the
user-related methods:
User management
-------------------------
User createUser(String name);
void createUser(User user);
void removeUser(User user);
void removeUser(String name);
User getUser(String name);
My thoughts:
1. I'm wondering if we should remove the createUser(String) method and
just have createUser(User). This would make sense in a way because I
can't think of many use cases where you might want to create a
User with
just a username and not any of the other typical information (such as
first name, last name, e-mail address, etc). Creating a User via
createUser(String) when you actually want to set the other details
after
the initial User creation is also horribly inefficient, requiring
multiple round trips to the database (or whatever identity store
backend
you use). To add to that, the actual parameter name is a little
unintuitive - "name" could refer to any number of things - is it the
username or the user's actual name that you need to provide here?
My +1
goes to removing createUser(String).
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).
2. Similar to point 1), having two removeUser() methods seems equally
redundant. The User object that needs to be provided to the
removeUser(User) method can be easily looked up by calling
getUser(), as
per this example:
identityManager.removeUser(identityManager.getUser("jsmith"));
+1
3. As I also pointed out in point 1), the "name" parameter is a little
unintuitive. In regard to the getUser() method I think we should
rename
the "name" parameter to "username" just so it's perfectly
clear
what the
method expects.
+1
4. We don't currently have any way to update User details. I
recommend
that we add an updateUser() method as follows:
void updateUser(User user);
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.
This will allow User details such as their first and last name, e-mail
address etc to be updated without having to delete and re-create the
User. Extending on this a little further, I'm wondering if we should
think about adding an audit API that logs these changes. Perhaps it's
something to think about for a later release.
Group management
--------------------------
Group createGroup(String id);
Group createGroup(String id, Group parent);
Group createGroup(String id, String parent);
void removeGroup(Group group);
void removeGroup(String groupId);
Group getGroup(String groupId);
Group getGroup(String groupId, Group parent);
void addToGroup(IdentityType identityType, Group group);
void removeFromGroup(IdentityType identityType, Group group);
My thoughts:
1. I think there is a little bit of ambiguity here in regard to the
differences between a Group's ID and name. To assist in understanding
the differences, here's the JavaDoc pasted from the Group interface:
/**
* Groups are stored in tree hierarchy and therefore ID
represents
a path. ID string always begins with "/" element that
* represents root of the tree
* <p/>
* Example: Valid IDs are "/acme/departments/marketing",
"/security/administrator" or "/administrator". Where
"acme",
* "departments", "marketing", "security" and
"administrator" are
group names.
*
* @return Group Id in String representation.
*/
String getId();
/**
* Group name is unique identifier in specific group tree branch.
For example group with id "/acme/departments/marketing"
* will have name "marketing" and parent group of id
"/acme/departments"
*
* @return name
*/
String getName();
Taking the above JavaDoc into account, it seems that where we are
specifying "id" as a method parameter we should be specifying
"name"
instead. If we are indeed providing a "fully qualified" path to the
createGroup() method, then there is no need to also specify the parent
as it can be easily derived from the path. More on this in the
following points.
2. We currently have three createGroup() methods for creating a new
Group. I think we should remove the (String, String) variant, as the
parent parameter is ambiguous (is it the parent's ID or name we
need to
specify here?), and rename the "id" parameter of the (String, Group)
variant to "name", leaving us with the following:
Group createGroup(String id);
Group createGroup(String name, Group parent);
This then gives us two ways of creating a group. We can either
specify
the fully qualified group ID:
Group employees = identityManager.createGroup("/employees");
Or we can specify a subgroup name and parent Group:
Group managers = identityManager.createGroup("managers", employees);
We can also use this second form to create a "root" group by just
specifying null for the parent:
Group admins = identityManager.createGroup("admins", null);
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.
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.
3. For the removeGroup() methods, I think what we have is fine.
4. For the getGroup() methods, I'm happy with the first one (String
groupId) but I think the second one (String groupId, Group parent)
needs
to be (String name, Group parent) instead. The first method would be
used when the fully qualified group ID is known, and the second one
would be used when you already have the parent Group and know the name
of the subgroup.
5. The addToGroup() and removeFromGroup() methods seem fine to me.
6. One thing we don't have is a method to test whether an IdentityType
is a member of a Group. I suggest we add another method to
support this:
boolean inGroup(IdentityType identityType, Group group);
+1 for the above (with the type idea for #4).
Roles
----------
Role createRole(String name);
void removeRole(Role role);
void removeRole(String name);
Role getRole(String name);
boolean hasRole(Role role, IdentityType identityType, Group
group);
void grantRole(Role role, IdentityType identityType, Group
group);
void revokeRole(Role role, IdentityType identityType, Group
group);
My thoughts:
1. I would remove the removeRole(String) method as the same thing
can be
achieved with removeRole(getRole(String)).
+1
2. I would swap the order of the Role and IdentityType parameters
within
the hasRole(), grantRole() and revokeRole() methods. As the
IdentityType is the primary artifact in these operations it makes
sense
to have it listed first.
+1
3. We don't have any explicit support for application roles, which are
roles where there is no Group component (for example, an
application-wide "admin" role). We could support this by just
allowing
hasRole(), grantRole() etc to accept null values for the Group
parameter, however I feel it would be more semantically correct to
provide a distinct set of methods for the purpose of supporting
application roles, as follows:
boolean hasApplicationRole(IdentityType identityType, Role role);
void grantApplicationRole(IdentityType identityType, Role role);
void revokeApplicationRole(IdentityType identityType, Role role);
+1
Query API
--------------
<T extends IdentityType> IdentityQuery<T> createQuery();
My thoughts:
1. After our recent rewrite of the Query API I'm satisfied with
what we
have now.
Credential management
-------------------------------
boolean validateCredential(User user, Credential credential);
void updateCredential(User user, Credential credential);
My thoughts:
1. I'm satisfied with what we currently have, however we still need to
review the mechanism that we provide for encoding passwords. I'm not
sure that it will have any effect on these methods though (I'm toying
with the idea of integrating the password encoding functionality
via the
IdentityStoreInvocationContext).
Identity expiry
------------------
void setEnabled(IdentityType identityType, boolean enabled);
void setExpirationDate(IdentityType identityType, Date
expirationDate);
My thoughts:
1. I think these methods are fine the way they are. One discrepancy
that I've identified is that a User can be created already being
disabled or having an expiry date, while for a Role or Group the
equivalent status must be set via these methods after creation.
I'm not
sure this is a big deal though.
All good.
Attributes
-------------
void setAttribute(IdentityType identityType, String
attributeName,
String attributeValue);
String getAttribute(IdentityType identityType, String
attributeName);
My thoughts:
1. This area poses a slight dilemma. Currently the API only supports
simple String-based attribute values, however I'm pretty sure that
people are going to want to store all sorts of things, ranging from
boolean and Date values through to large byte arrays that store a
User's
photograph or other data. If everyone is in agreement that we need to
support more than just String values, then the first thing we probably
need to do is modify these methods to work with a Serializable
instead.
2. If we all agree on point 1), the next thing we need to decide is
whether we want the capability to make some attribute values "lazy
loaded". If we want to do a quick lookup of a User object for the
express purpose of assigning a Role or Group membership (or any other
type of simple operation) then we probably don't want the performance
hit of having to load bulky attribute data.
3. With the above two points in mind, I'm going to hold off on
surmising
any further on attributes until some of you guys weigh in with your
opinions. I do have some rough ideas, however I'll wait until we
have a
consensus of exactly what we want to achieve here before we
proceed further.
Serializable works for me. Do we do have a generic Attribute<? extends
Serializable> or just forgo the generics?
Utility methods
--------------------
IdentityType lookupIdentityByKey(String key);
My thoughts:
1. This method is required by other features, such as the Permissions
API. I'm fine with what we have here.
In summary
----------------
I apologise for yet another epic e-mail to the list, however this is
everyone's chance to review the API for themselves and decide
whether or
not it will be suitable for addressing all of your requirements. I'd
like to get this API stable by next week so please don't be shy about
speaking up. I'm especially looking forward to hearing your opinions
about how we handle attributes (there may even be some relevant
JSR-351
stuff that we should be looking at here).
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
--
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>
1:37 p.m.
Aren't those implementation details though?
On Wed, Nov 7, 2012 at 12:16 PM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
On 11/08/2012 04:08 AM, Jason Porter wrote:
Replies inline
On Wed, Nov 7, 2012 at 3:54 AM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
> Hey guys,
>
> I spent some time reviewing the IdentityManager API today to identify
> any redundancies and also locate any holes where we might not properly
> support required features, as well as a general "sanity" check to ensure
> that what we're exposing via this API makes sense and is intuitive for
> consumers. As Anil has pointed out already, our short term priority for
> the project is to stabilize the API - this is extremely important as
> PicketLink will provide the security foundation for many other
> projects. With that in mind I'd like to strongly encourage everyone
> with a stake in this to carefully review the API and provide feedback,
> as once we release it it will be essentially set in stone.
>
> To try and avoid a wall of text (and make this post easier to reply to),
> I'm going to break this post up into sub-sections, one for each feature
> group. For each section, I'll include the API as it currently exists,
> followed by a brief summary of my thoughts and any recommendations I may
> have - this is especially where I want to hear any feedback indicating
> whether you agree or disagree with my assessment. Let's start with the
> user-related methods:
>
> User management
> -------------------------
>
> User createUser(String name);
>
> void createUser(User user);
>
> void removeUser(User user);
>
> void removeUser(String name);
>
> User getUser(String name);
>
> My thoughts:
>
> 1. I'm wondering if we should remove the createUser(String) method and
> just have createUser(User). This would make sense in a way because I
> can't think of many use cases where you might want to create a User with
> just a username and not any of the other typical information (such as
> first name, last name, e-mail address, etc). Creating a User via
> createUser(String) when you actually want to set the other details after
> the initial User creation is also horribly inefficient, requiring
> multiple round trips to the database (or whatever identity store backend
> you use). To add to that, the actual parameter name is a little
> unintuitive - "name" could refer to any number of things - is it the
> username or the user's actual name that you need to provide here? My +1
> goes to removing createUser(String).
>
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).
> 2. Similar to point 1), having two removeUser() methods seems equally
> redundant. The User object that needs to be provided to the
> removeUser(User) method can be easily looked up by calling getUser(), as
> per this example:
>
> identityManager.removeUser(identityManager.getUser("jsmith"));
>
+1
>
> 3. As I also pointed out in point 1), the "name" parameter is a little
> unintuitive. In regard to the getUser() method I think we should rename
> the "name" parameter to "username" just so it's perfectly
clear what the
> method expects.
>
+1
> 4. We don't currently have any way to update User details. I recommend
> that we add an updateUser() method as follows:
>
> void updateUser(User user);
>
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.
> This will allow User details such as their first and last name, e-mail
> address etc to be updated without having to delete and re-create the
> User. Extending on this a little further, I'm wondering if we should
> think about adding an audit API that logs these changes. Perhaps it's
> something to think about for a later release.
>
> Group management
> --------------------------
>
> Group createGroup(String id);
>
> Group createGroup(String id, Group parent);
>
> Group createGroup(String id, String parent);
>
> void removeGroup(Group group);
>
> void removeGroup(String groupId);
>
> Group getGroup(String groupId);
>
> Group getGroup(String groupId, Group parent);
>
> void addToGroup(IdentityType identityType, Group group);
>
> void removeFromGroup(IdentityType identityType, Group group);
>
> My thoughts:
>
> 1. I think there is a little bit of ambiguity here in regard to the
> differences between a Group's ID and name. To assist in understanding
> the differences, here's the JavaDoc pasted from the Group interface:
>
> /**
> * Groups are stored in tree hierarchy and therefore ID represents
> a path. ID string always begins with "/" element that
> * represents root of the tree
> * <p/>
> * Example: Valid IDs are "/acme/departments/marketing",
> "/security/administrator" or "/administrator". Where
"acme",
> * "departments", "marketing", "security" and
"administrator" are
> group names.
> *
> * @return Group Id in String representation.
> */
> String getId();
>
> /**
> * Group name is unique identifier in specific group tree branch.
> For example group with id "/acme/departments/marketing"
> * will have name "marketing" and parent group of id
> "/acme/departments"
> *
> * @return name
> */
> String getName();
>
> Taking the above JavaDoc into account, it seems that where we are
> specifying "id" as a method parameter we should be specifying
"name"
> instead. If we are indeed providing a "fully qualified" path to the
> createGroup() method, then there is no need to also specify the parent
> as it can be easily derived from the path. More on this in the
> following points.
>
> 2. We currently have three createGroup() methods for creating a new
> Group. I think we should remove the (String, String) variant, as the
> parent parameter is ambiguous (is it the parent's ID or name we need to
> specify here?), and rename the "id" parameter of the (String, Group)
> variant to "name", leaving us with the following:
>
> Group createGroup(String id);
>
> Group createGroup(String name, Group parent);
>
> This then gives us two ways of creating a group. We can either specify
> the fully qualified group ID:
>
> Group employees = identityManager.createGroup("/employees");
>
> Or we can specify a subgroup name and parent Group:
>
> Group managers = identityManager.createGroup("managers", employees);
>
> We can also use this second form to create a "root" group by just
> specifying null for the parent:
>
> Group admins = identityManager.createGroup("admins", null);
>
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.
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.
>
> 3. For the removeGroup() methods, I think what we have is fine.
>
> 4. For the getGroup() methods, I'm happy with the first one (String
> groupId) but I think the second one (String groupId, Group parent) needs
> to be (String name, Group parent) instead. The first method would be
> used when the fully qualified group ID is known, and the second one
> would be used when you already have the parent Group and know the name
> of the subgroup.
>
> 5. The addToGroup() and removeFromGroup() methods seem fine to me.
>
> 6. One thing we don't have is a method to test whether an IdentityType
> is a member of a Group. I suggest we add another method to support this:
>
> boolean inGroup(IdentityType identityType, Group group);
>
>
+1 for the above (with the type idea for #4).
> Roles
> ----------
>
> Role createRole(String name);
>
> void removeRole(Role role);
>
> void removeRole(String name);
>
> Role getRole(String name);
>
> boolean hasRole(Role role, IdentityType identityType, Group group);
>
> void grantRole(Role role, IdentityType identityType, Group group);
>
> void revokeRole(Role role, IdentityType identityType, Group group);
>
> My thoughts:
>
> 1. I would remove the removeRole(String) method as the same thing can be
> achieved with removeRole(getRole(String)).
>
+1
> 2. I would swap the order of the Role and IdentityType parameters within
> the hasRole(), grantRole() and revokeRole() methods. As the
> IdentityType is the primary artifact in these operations it makes sense
> to have it listed first.
>
+1
> 3. We don't have any explicit support for application roles, which are
> roles where there is no Group component (for example, an
> application-wide "admin" role). We could support this by just allowing
> hasRole(), grantRole() etc to accept null values for the Group
> parameter, however I feel it would be more semantically correct to
> provide a distinct set of methods for the purpose of supporting
> application roles, as follows:
>
> boolean hasApplicationRole(IdentityType identityType, Role role);
>
> void grantApplicationRole(IdentityType identityType, Role role);
>
> void revokeApplicationRole(IdentityType identityType, Role role);
>
+1
> Query API
> --------------
>
> <T extends IdentityType> IdentityQuery<T> createQuery();
>
> My thoughts:
>
> 1. After our recent rewrite of the Query API I'm satisfied with what we
> have now.
>
> Credential management
> -------------------------------
>
> boolean validateCredential(User user, Credential credential);
>
> void updateCredential(User user, Credential credential);
>
> My thoughts:
>
> 1. I'm satisfied with what we currently have, however we still need to
> review the mechanism that we provide for encoding passwords. I'm not
> sure that it will have any effect on these methods though (I'm toying
> with the idea of integrating the password encoding functionality via the
> IdentityStoreInvocationContext).
>
> Identity expiry
> ------------------
>
> void setEnabled(IdentityType identityType, boolean enabled);
>
> void setExpirationDate(IdentityType identityType, Date
> expirationDate);
>
> My thoughts:
>
> 1. I think these methods are fine the way they are. One discrepancy
> that I've identified is that a User can be created already being
> disabled or having an expiry date, while for a Role or Group the
> equivalent status must be set via these methods after creation. I'm not
> sure this is a big deal though.
>
All good.
> Attributes
> -------------
>
> void setAttribute(IdentityType identityType, String attributeName,
> String attributeValue);
>
> String getAttribute(IdentityType identityType, String attributeName);
>
> My thoughts:
>
> 1. This area poses a slight dilemma. Currently the API only supports
> simple String-based attribute values, however I'm pretty sure that
> people are going to want to store all sorts of things, ranging from
> boolean and Date values through to large byte arrays that store a User's
> photograph or other data. If everyone is in agreement that we need to
> support more than just String values, then the first thing we probably
> need to do is modify these methods to work with a Serializable instead.
>
> 2. If we all agree on point 1), the next thing we need to decide is
> whether we want the capability to make some attribute values "lazy
> loaded". If we want to do a quick lookup of a User object for the
> express purpose of assigning a Role or Group membership (or any other
> type of simple operation) then we probably don't want the performance
> hit of having to load bulky attribute data.
>
> 3. With the above two points in mind, I'm going to hold off on surmising
> any further on attributes until some of you guys weigh in with your
> opinions. I do have some rough ideas, however I'll wait until we have a
> consensus of exactly what we want to achieve here before we proceed
> further.
>
Serializable works for me. Do we do have a generic Attribute<? extends
Serializable> or just forgo the generics?
> Utility methods
> --------------------
>
> IdentityType lookupIdentityByKey(String key);
>
> My thoughts:
>
> 1. This method is required by other features, such as the Permissions
> API. I'm fine with what we have here.
>
> In summary
> ----------------
>
> I apologise for yet another epic e-mail to the list, however this is
> everyone's chance to review the API for themselves and decide whether or
> not it will be suitable for addressing all of your requirements. I'd
> like to get this API stable by next week so please don't be shy about
> speaking up. I'm especially looking forward to hearing your opinions
> about how we handle attributes (there may even be some relevant JSR-351
> stuff that we should be looking at here).
>
> Thanks,
> Shane
> _______________________________________________
> security-dev mailing list
> security-dev(a)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, pgp.mit.edu
--
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, pgp.mit.edu
2:08 p.m.
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):
void createGroup(Group group);
void createRole(Role role);
[1]
https://github.com/picketlink/picketlink/blob/master/idm/api/src/main/jav...
On 11/08/2012 05:37 AM, Jason Porter wrote:
Aren't those implementation details though?
On Wed, Nov 7, 2012 at 12:16 PM, Shane Bryzak <sbryzak(a)redhat.com
<mailto:sbryzak@redhat.com>> wrote:
On 11/08/2012 04:08 AM, Jason Porter wrote:
> Replies inline
>
> On Wed, Nov 7, 2012 at 3:54 AM, Shane Bryzak <sbryzak(a)redhat.com
> <mailto:sbryzak@redhat.com>> wrote:
>
> Hey guys,
>
> I spent some time reviewing the IdentityManager API today to
> identify
> any redundancies and also locate any holes where we might not
> properly
> support required features, as well as a general "sanity"
> check to ensure
> that what we're exposing via this API makes sense and is
> intuitive for
> consumers. As Anil has pointed out already, our short term
> priority for
> the project is to stabilize the API - this is extremely
> important as
> PicketLink will provide the security foundation for many other
> projects. With that in mind I'd like to strongly encourage
> everyone
> with a stake in this to carefully review the API and provide
> feedback,
> as once we release it it will be essentially set in stone.
>
> To try and avoid a wall of text (and make this post easier to
> reply to),
> I'm going to break this post up into sub-sections, one for
> each feature
> group. For each section, I'll include the API as it
> currently exists,
> followed by a brief summary of my thoughts and any
> recommendations I may
> have - this is especially where I want to hear any feedback
> indicating
> whether you agree or disagree with my assessment. Let's
> start with the
> user-related methods:
>
> User management
> -------------------------
>
> User createUser(String name);
>
> void createUser(User user);
>
> void removeUser(User user);
>
> void removeUser(String name);
>
> User getUser(String name);
>
> My thoughts:
>
> 1. I'm wondering if we should remove the createUser(String)
> method and
> just have createUser(User). This would make sense in a way
> because I
> can't think of many use cases where you might want to create
> a User with
> just a username and not any of the other typical information
> (such as
> first name, last name, e-mail address, etc). Creating a User via
> createUser(String) when you actually want to set the other
> details after
> the initial User creation is also horribly inefficient, requiring
> multiple round trips to the database (or whatever identity
> store backend
> you use). To add to that, the actual parameter name is a little
> unintuitive - "name" could refer to any number of things - is
> it the
> username or the user's actual name that you need to provide
> here? My +1
> goes to removing createUser(String).
>
>
> 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).
>
> 2. Similar to point 1), having two removeUser() methods seems
> equally
> redundant. The User object that needs to be provided to the
> removeUser(User) method can be easily looked up by calling
> getUser(), as
> per this example:
>
> identityManager.removeUser(identityManager.getUser("jsmith"));
>
>
> +1
>
>
> 3. As I also pointed out in point 1), the "name" parameter is
> a little
> unintuitive. In regard to the getUser() method I think we
> should rename
> the "name" parameter to "username" just so it's
perfectly
> clear what the
> method expects.
>
>
> +1
>
> 4. We don't currently have any way to update User details. I
> recommend
> that we add an updateUser() method as follows:
>
> void updateUser(User user);
>
>
> 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.
>
> This will allow User details such as their first and last
> name, e-mail
> address etc to be updated without having to delete and
> re-create the
> User. Extending on this a little further, I'm wondering if
> we should
> think about adding an audit API that logs these changes.
> Perhaps it's
> something to think about for a later release.
>
> Group management
> --------------------------
>
> Group createGroup(String id);
>
> Group createGroup(String id, Group parent);
>
> Group createGroup(String id, String parent);
>
> void removeGroup(Group group);
>
> void removeGroup(String groupId);
>
> Group getGroup(String groupId);
>
> Group getGroup(String groupId, Group parent);
>
> void addToGroup(IdentityType identityType, Group group);
>
> void removeFromGroup(IdentityType identityType, Group
> group);
>
> My thoughts:
>
> 1. I think there is a little bit of ambiguity here in regard
> to the
> differences between a Group's ID and name. To assist in
> understanding
> the differences, here's the JavaDoc pasted from the Group
> interface:
>
> /**
> * Groups are stored in tree hierarchy and therefore ID
> represents
> a path. ID string always begins with "/" element that
> * represents root of the tree
> * <p/>
> * Example: Valid IDs are "/acme/departments/marketing",
> "/security/administrator" or "/administrator". Where
"acme",
> * "departments", "marketing", "security"
and
> "administrator" are
> group names.
> *
> * @return Group Id in String representation.
> */
> String getId();
>
> /**
> * Group name is unique identifier in specific group
> tree branch.
> For example group with id "/acme/departments/marketing"
> * will have name "marketing" and parent group of id
> "/acme/departments"
> *
> * @return name
> */
> String getName();
>
> Taking the above JavaDoc into account, it seems that where we are
> specifying "id" as a method parameter we should be specifying
> "name"
> instead. If we are indeed providing a "fully qualified" path
> to the
> createGroup() method, then there is no need to also specify
> the parent
> as it can be easily derived from the path. More on this in the
> following points.
>
> 2. We currently have three createGroup() methods for creating
> a new
> Group. I think we should remove the (String, String)
> variant, as the
> parent parameter is ambiguous (is it the parent's ID or name
> we need to
> specify here?), and rename the "id" parameter of the (String,
> Group)
> variant to "name", leaving us with the following:
>
> Group createGroup(String id);
>
> Group createGroup(String name, Group parent);
>
> This then gives us two ways of creating a group. We can
> either specify
> the fully qualified group ID:
>
> Group employees = identityManager.createGroup("/employees");
>
> Or we can specify a subgroup name and parent Group:
>
> Group managers = identityManager.createGroup("managers",
> employees);
>
> We can also use this second form to create a "root" group by just
> specifying null for the parent:
>
> Group admins = identityManager.createGroup("admins", null);
>
>
> 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.
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.
>
> 3. For the removeGroup() methods, I think what we have is fine.
>
> 4. For the getGroup() methods, I'm happy with the first one
> (String
> groupId) but I think the second one (String groupId, Group
> parent) needs
> to be (String name, Group parent) instead. The first method
> would be
> used when the fully qualified group ID is known, and the
> second one
> would be used when you already have the parent Group and know
> the name
> of the subgroup.
>
> 5. The addToGroup() and removeFromGroup() methods seem fine
> to me.
>
> 6. One thing we don't have is a method to test whether an
> IdentityType
> is a member of a Group. I suggest we add another method to
> support this:
>
> boolean inGroup(IdentityType identityType, Group group);
>
>
> +1 for the above (with the type idea for #4).
>
> Roles
> ----------
>
> Role createRole(String name);
>
> void removeRole(Role role);
>
> void removeRole(String name);
>
> Role getRole(String name);
>
> boolean hasRole(Role role, IdentityType identityType,
> Group group);
>
> void grantRole(Role role, IdentityType identityType,
> Group group);
>
> void revokeRole(Role role, IdentityType identityType,
> Group group);
>
> My thoughts:
>
> 1. I would remove the removeRole(String) method as the same
> thing can be
> achieved with removeRole(getRole(String)).
>
>
> +1
>
> 2. I would swap the order of the Role and IdentityType
> parameters within
> the hasRole(), grantRole() and revokeRole() methods. As the
> IdentityType is the primary artifact in these operations it
> makes sense
> to have it listed first.
>
>
> +1
>
> 3. We don't have any explicit support for application roles,
> which are
> roles where there is no Group component (for example, an
> application-wide "admin" role). We could support this by
> just allowing
> hasRole(), grantRole() etc to accept null values for the Group
> parameter, however I feel it would be more semantically
> correct to
> provide a distinct set of methods for the purpose of supporting
> application roles, as follows:
>
> boolean hasApplicationRole(IdentityType identityType,
> Role role);
>
> void grantApplicationRole(IdentityType identityType,
> Role role);
>
> void revokeApplicationRole(IdentityType identityType,
> Role role);
>
>
> +1
>
> Query API
> --------------
>
> <T extends IdentityType> IdentityQuery<T> createQuery();
>
> My thoughts:
>
> 1. After our recent rewrite of the Query API I'm satisfied
> with what we
> have now.
>
> Credential management
> -------------------------------
>
> boolean validateCredential(User user, Credential
> credential);
>
> void updateCredential(User user, Credential credential);
>
> My thoughts:
>
> 1. I'm satisfied with what we currently have, however we
> still need to
> review the mechanism that we provide for encoding passwords.
> I'm not
> sure that it will have any effect on these methods though
> (I'm toying
> with the idea of integrating the password encoding
> functionality via the
> IdentityStoreInvocationContext).
>
> Identity expiry
> ------------------
>
> void setEnabled(IdentityType identityType, boolean enabled);
>
> void setExpirationDate(IdentityType identityType, Date
> expirationDate);
>
> My thoughts:
>
> 1. I think these methods are fine the way they are. One
> discrepancy
> that I've identified is that a User can be created already being
> disabled or having an expiry date, while for a Role or Group the
> equivalent status must be set via these methods after
> creation. I'm not
> sure this is a big deal though.
>
>
> All good.
>
> Attributes
> -------------
>
> void setAttribute(IdentityType identityType, String
> attributeName,
> String attributeValue);
>
> String getAttribute(IdentityType identityType, String
> attributeName);
>
> My thoughts:
>
> 1. This area poses a slight dilemma. Currently the API only
> supports
> simple String-based attribute values, however I'm pretty sure
> that
> people are going to want to store all sorts of things,
> ranging from
> boolean and Date values through to large byte arrays that
> store a User's
> photograph or other data. If everyone is in agreement that
> we need to
> support more than just String values, then the first thing we
> probably
> need to do is modify these methods to work with a
> Serializable instead.
>
> 2. If we all agree on point 1), the next thing we need to
> decide is
> whether we want the capability to make some attribute values
> "lazy
> loaded". If we want to do a quick lookup of a User object
> for the
> express purpose of assigning a Role or Group membership (or
> any other
> type of simple operation) then we probably don't want the
> performance
> hit of having to load bulky attribute data.
>
> 3. With the above two points in mind, I'm going to hold off
> on surmising
> any further on attributes until some of you guys weigh in
> with your
> opinions. I do have some rough ideas, however I'll wait
> until we have a
> consensus of exactly what we want to achieve here before we
> proceed further.
>
>
> Serializable works for me. Do we do have a generic Attribute<?
> extends Serializable> or just forgo the generics?
>
> Utility methods
> --------------------
>
> IdentityType lookupIdentityByKey(String key);
>
> My thoughts:
>
> 1. This method is required by other features, such as the
> Permissions
> API. I'm fine with what we have here.
>
> In summary
> ----------------
>
> I apologise for yet another epic e-mail to the list, however
> this is
> everyone's chance to review the API for themselves and decide
> whether or
> not it will be suitable for addressing all of your
> requirements. I'd
> like to get this API stable by next week so please don't be
> shy about
> speaking up. I'm especially looking forward to hearing your
> opinions
> about how we handle attributes (there may even be some
> relevant JSR-351
> stuff that we should be looking at here).
>
> 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
>
>
>
>
> --
> 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>
--
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>
2:11 p.m.
On a somewhat related note, we should probably fix the constructor for
SimpleGroup also, which currently looks like this:
public SimpleGroup(String id, String name, Group parentGroup)
Having both id and parentGroup parameters is redundant, so I suggest
removing the id parameter (and removing the id field altogether) and
instead have the getId() method return a calculated id.
On 11/08/2012 06:08 AM, Shane Bryzak wrote:
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):
void createGroup(Group group);
void createRole(Role role);
[1]
https://github.com/picketlink/picketlink/blob/master/idm/api/src/main/jav...
On 11/08/2012 05:37 AM, Jason Porter wrote:
> Aren't those implementation details though?
2:14 p.m.
On Wed, Nov 7, 2012 at 1:11 PM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
On a somewhat related note, we should probably fix the constructor
for
SimpleGroup also, which currently looks like this:
public SimpleGroup(String id, String name, Group parentGroup)
Having both id and parentGroup parameters is redundant, so I suggest
removing the id parameter (and removing the id field altogether) and
instead have the getId() method return a calculated id.
+1
On 11/08/2012 06:08 AM, Shane Bryzak wrote:
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):
void createGroup(Group group);
void createRole(Role role);
[1]
https://github.com/picketlink/picketlink/blob/master/idm/api/src/main/jav...
On 11/08/2012 05:37 AM, Jason Porter wrote:
Aren't those implementation details though?
_______________________________________________
security-dev mailing list
security-dev(a)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, pgp.mit.edu
7:08 a.m.
I think I'm in agreement with outcome of previous comments about API.
Debate around way of creating groups is more a matter of taste in the
end. I'm more concern at this point about implementation and explicitly
storing group ids as I know from experience that calculating this data
can be performance killer in case of any more sophisticated query.
On 11/07/2012 09:14 PM, Jason Porter wrote:
On Wed, Nov 7, 2012 at 1:11 PM, Shane Bryzak <sbryzak(a)redhat.com
<mailto:sbryzak@redhat.com>> wrote:
On a somewhat related note, we should probably fix the constructor
for SimpleGroup also, which currently looks like this:
public SimpleGroup(String id, String name, Group parentGroup)
Having both id and parentGroup parameters is redundant, so I suggest
removing the id parameter (and removing the id field altogether) and
instead have the getId() method return a calculated id.
+1
On 11/08/2012 06:08 AM, Shane Bryzak wrote:
> 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):
>
> void createGroup(Group group);
>
> void createRole(Role role);
>
>
> [1]
>
https://github.com/picketlink/picketlink/blob/master/idm/api/src/main/jav...
>
> On 11/08/2012 05:37 AM, Jason Porter wrote:
>> Aren't those implementation details though?
>
_______________________________________________
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>
_______________________________________________
security-dev mailing list
security-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/security-dev
9:03 p.m.
On 11/08/2012 11:08 PM, Bolesław Dawidowicz wrote:
I think I'm in agreement with outcome of previous comments about
API.
Debate around way of creating groups is more a matter of taste in the
end. I'm more concern at this point about implementation and explicitly
storing group ids as I know from experience that calculating this data
can be performance killer in case of any more sophisticated query.
We can probably implement this as "calculate once and store", and if we
cache Groups and Roles (like I intend to) then the performance hit will
be insignificant.
2:14 p.m.
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.
On Wed, Nov 7, 2012 at 1:08 PM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
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):
void createGroup(Group group);
void createRole(Role role);
[1]
https://github.com/picketlink/picketlink/blob/master/idm/api/src/main/jav...
On 11/08/2012 05:37 AM, Jason Porter wrote:
Aren't those implementation details though?
On Wed, Nov 7, 2012 at 12:16 PM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
> On 11/08/2012 04:08 AM, Jason Porter wrote:
>
> Replies inline
>
> On Wed, Nov 7, 2012 at 3:54 AM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
>
>> Hey guys,
>>
>> I spent some time reviewing the IdentityManager API today to identify
>> any redundancies and also locate any holes where we might not properly
>> support required features, as well as a general "sanity" check to
ensure
>> that what we're exposing via this API makes sense and is intuitive for
>> consumers. As Anil has pointed out already, our short term priority for
>> the project is to stabilize the API - this is extremely important as
>> PicketLink will provide the security foundation for many other
>> projects. With that in mind I'd like to strongly encourage everyone
>> with a stake in this to carefully review the API and provide feedback,
>> as once we release it it will be essentially set in stone.
>>
>> To try and avoid a wall of text (and make this post easier to reply to),
>> I'm going to break this post up into sub-sections, one for each feature
>> group. For each section, I'll include the API as it currently exists,
>> followed by a brief summary of my thoughts and any recommendations I may
>> have - this is especially where I want to hear any feedback indicating
>> whether you agree or disagree with my assessment. Let's start with the
>> user-related methods:
>>
>> User management
>> -------------------------
>>
>> User createUser(String name);
>>
>> void createUser(User user);
>>
>> void removeUser(User user);
>>
>> void removeUser(String name);
>>
>> User getUser(String name);
>>
>> My thoughts:
>>
>> 1. I'm wondering if we should remove the createUser(String) method and
>> just have createUser(User). This would make sense in a way because I
>> can't think of many use cases where you might want to create a User with
>> just a username and not any of the other typical information (such as
>> first name, last name, e-mail address, etc). Creating a User via
>> createUser(String) when you actually want to set the other details after
>> the initial User creation is also horribly inefficient, requiring
>> multiple round trips to the database (or whatever identity store backend
>> you use). To add to that, the actual parameter name is a little
>> unintuitive - "name" could refer to any number of things - is it the
>> username or the user's actual name that you need to provide here? My +1
>> goes to removing createUser(String).
>>
>
> 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).
>
>
>> 2. Similar to point 1), having two removeUser() methods seems equally
>> redundant. The User object that needs to be provided to the
>> removeUser(User) method can be easily looked up by calling getUser(), as
>> per this example:
>>
>> identityManager.removeUser(identityManager.getUser("jsmith"));
>>
>
> +1
>
>
>>
>> 3. As I also pointed out in point 1), the "name" parameter is a little
>> unintuitive. In regard to the getUser() method I think we should rename
>> the "name" parameter to "username" just so it's perfectly
clear what the
>> method expects.
>>
>
> +1
>
>
>> 4. We don't currently have any way to update User details. I recommend
>> that we add an updateUser() method as follows:
>>
>> void updateUser(User user);
>>
>
> 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.
>
>
>> This will allow User details such as their first and last name, e-mail
>> address etc to be updated without having to delete and re-create the
>> User. Extending on this a little further, I'm wondering if we should
>> think about adding an audit API that logs these changes. Perhaps it's
>> something to think about for a later release.
>>
>> Group management
>> --------------------------
>>
>> Group createGroup(String id);
>>
>> Group createGroup(String id, Group parent);
>>
>> Group createGroup(String id, String parent);
>>
>> void removeGroup(Group group);
>>
>> void removeGroup(String groupId);
>>
>> Group getGroup(String groupId);
>>
>> Group getGroup(String groupId, Group parent);
>>
>> void addToGroup(IdentityType identityType, Group group);
>>
>> void removeFromGroup(IdentityType identityType, Group group);
>>
>> My thoughts:
>>
>> 1. I think there is a little bit of ambiguity here in regard to the
>> differences between a Group's ID and name. To assist in understanding
>> the differences, here's the JavaDoc pasted from the Group interface:
>>
>> /**
>> * Groups are stored in tree hierarchy and therefore ID represents
>> a path. ID string always begins with "/" element that
>> * represents root of the tree
>> * <p/>
>> * Example: Valid IDs are "/acme/departments/marketing",
>> "/security/administrator" or "/administrator". Where
"acme",
>> * "departments", "marketing", "security" and
"administrator" are
>> group names.
>> *
>> * @return Group Id in String representation.
>> */
>> String getId();
>>
>> /**
>> * Group name is unique identifier in specific group tree branch.
>> For example group with id "/acme/departments/marketing"
>> * will have name "marketing" and parent group of id
>> "/acme/departments"
>> *
>> * @return name
>> */
>> String getName();
>>
>> Taking the above JavaDoc into account, it seems that where we are
>> specifying "id" as a method parameter we should be specifying
"name"
>> instead. If we are indeed providing a "fully qualified" path to the
>> createGroup() method, then there is no need to also specify the parent
>> as it can be easily derived from the path. More on this in the
>> following points.
>>
>> 2. We currently have three createGroup() methods for creating a new
>> Group. I think we should remove the (String, String) variant, as the
>> parent parameter is ambiguous (is it the parent's ID or name we need to
>> specify here?), and rename the "id" parameter of the (String, Group)
>> variant to "name", leaving us with the following:
>>
>> Group createGroup(String id);
>>
>> Group createGroup(String name, Group parent);
>>
>> This then gives us two ways of creating a group. We can either specify
>> the fully qualified group ID:
>>
>> Group employees = identityManager.createGroup("/employees");
>>
>> Or we can specify a subgroup name and parent Group:
>>
>> Group managers = identityManager.createGroup("managers", employees);
>>
>> We can also use this second form to create a "root" group by just
>> specifying null for the parent:
>>
>> Group admins = identityManager.createGroup("admins", null);
>>
>
> 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.
>
>
> 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.
>
>
>
>
>>
>> 3. For the removeGroup() methods, I think what we have is fine.
>>
>> 4. For the getGroup() methods, I'm happy with the first one (String
>> groupId) but I think the second one (String groupId, Group parent) needs
>> to be (String name, Group parent) instead. The first method would be
>> used when the fully qualified group ID is known, and the second one
>> would be used when you already have the parent Group and know the name
>> of the subgroup.
>>
>> 5. The addToGroup() and removeFromGroup() methods seem fine to me.
>>
>> 6. One thing we don't have is a method to test whether an IdentityType
>> is a member of a Group. I suggest we add another method to support this:
>>
>> boolean inGroup(IdentityType identityType, Group group);
>>
>>
> +1 for the above (with the type idea for #4).
>
>
>> Roles
>> ----------
>>
>> Role createRole(String name);
>>
>> void removeRole(Role role);
>>
>> void removeRole(String name);
>>
>> Role getRole(String name);
>>
>> boolean hasRole(Role role, IdentityType identityType, Group group);
>>
>> void grantRole(Role role, IdentityType identityType, Group group);
>>
>> void revokeRole(Role role, IdentityType identityType, Group group);
>>
>> My thoughts:
>>
>> 1. I would remove the removeRole(String) method as the same thing can be
>> achieved with removeRole(getRole(String)).
>>
>
> +1
>
>
>> 2. I would swap the order of the Role and IdentityType parameters within
>> the hasRole(), grantRole() and revokeRole() methods. As the
>> IdentityType is the primary artifact in these operations it makes sense
>> to have it listed first.
>>
>
> +1
>
>
>> 3. We don't have any explicit support for application roles, which are
>> roles where there is no Group component (for example, an
>> application-wide "admin" role). We could support this by just
allowing
>> hasRole(), grantRole() etc to accept null values for the Group
>> parameter, however I feel it would be more semantically correct to
>> provide a distinct set of methods for the purpose of supporting
>> application roles, as follows:
>>
>> boolean hasApplicationRole(IdentityType identityType, Role role);
>>
>> void grantApplicationRole(IdentityType identityType, Role role);
>>
>> void revokeApplicationRole(IdentityType identityType, Role role);
>>
>
> +1
>
>
>> Query API
>> --------------
>>
>> <T extends IdentityType> IdentityQuery<T> createQuery();
>>
>> My thoughts:
>>
>> 1. After our recent rewrite of the Query API I'm satisfied with what we
>> have now.
>>
>> Credential management
>> -------------------------------
>>
>> boolean validateCredential(User user, Credential credential);
>>
>> void updateCredential(User user, Credential credential);
>>
>> My thoughts:
>>
>> 1. I'm satisfied with what we currently have, however we still need to
>> review the mechanism that we provide for encoding passwords. I'm not
>> sure that it will have any effect on these methods though (I'm toying
>> with the idea of integrating the password encoding functionality via the
>> IdentityStoreInvocationContext).
>>
>> Identity expiry
>> ------------------
>>
>> void setEnabled(IdentityType identityType, boolean enabled);
>>
>> void setExpirationDate(IdentityType identityType, Date
>> expirationDate);
>>
>> My thoughts:
>>
>> 1. I think these methods are fine the way they are. One discrepancy
>> that I've identified is that a User can be created already being
>> disabled or having an expiry date, while for a Role or Group the
>> equivalent status must be set via these methods after creation. I'm not
>> sure this is a big deal though.
>>
>
> All good.
>
>
>> Attributes
>> -------------
>>
>> void setAttribute(IdentityType identityType, String attributeName,
>> String attributeValue);
>>
>> String getAttribute(IdentityType identityType, String
>> attributeName);
>>
>> My thoughts:
>>
>> 1. This area poses a slight dilemma. Currently the API only supports
>> simple String-based attribute values, however I'm pretty sure that
>> people are going to want to store all sorts of things, ranging from
>> boolean and Date values through to large byte arrays that store a User's
>> photograph or other data. If everyone is in agreement that we need to
>> support more than just String values, then the first thing we probably
>> need to do is modify these methods to work with a Serializable instead.
>>
>> 2. If we all agree on point 1), the next thing we need to decide is
>> whether we want the capability to make some attribute values "lazy
>> loaded". If we want to do a quick lookup of a User object for the
>> express purpose of assigning a Role or Group membership (or any other
>> type of simple operation) then we probably don't want the performance
>> hit of having to load bulky attribute data.
>>
>> 3. With the above two points in mind, I'm going to hold off on surmising
>> any further on attributes until some of you guys weigh in with your
>> opinions. I do have some rough ideas, however I'll wait until we have a
>> consensus of exactly what we want to achieve here before we proceed
>> further.
>>
>
> Serializable works for me. Do we do have a generic Attribute<? extends
> Serializable> or just forgo the generics?
>
>
>> Utility methods
>> --------------------
>>
>> IdentityType lookupIdentityByKey(String key);
>>
>> My thoughts:
>>
>> 1. This method is required by other features, such as the Permissions
>> API. I'm fine with what we have here.
>>
>> In summary
>> ----------------
>>
>> I apologise for yet another epic e-mail to the list, however this is
>> everyone's chance to review the API for themselves and decide whether or
>> not it will be suitable for addressing all of your requirements. I'd
>> like to get this API stable by next week so please don't be shy about
>> speaking up. I'm especially looking forward to hearing your opinions
>> about how we handle attributes (there may even be some relevant JSR-351
>> stuff that we should be looking at here).
>>
>> Thanks,
>> Shane
>> _______________________________________________
>> security-dev mailing list
>> security-dev(a)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, pgp.mit.edu
>
>
>
--
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, pgp.mit.edu
--
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, pgp.mit.edu
7:32 a.m.
On 11/07/2012 07:08 PM, Jason Porter wrote:
Attributes
-------------
void setAttribute(IdentityType identityType, String attributeName,
String attributeValue);
String getAttribute(IdentityType identityType, String
attributeName);
My thoughts:
1. This area poses a slight dilemma. Currently the API only supports
simple String-based attribute values, however I'm pretty sure that
people are going to want to store all sorts of things, ranging from
boolean and Date values through to large byte arrays that store a User's
photograph or other data. If everyone is in agreement that we need to
support more than just String values, then the first thing we probably
need to do is modify these methods to work with a Serializable instead.
2. If we all agree on point 1), the next thing we need to decide is
whether we want the capability to make some attribute values "lazy
loaded". If we want to do a quick lookup of a User object for the
express purpose of assigning a Role or Group membership (or any other
type of simple operation) then we probably don't want the performance
hit of having to load bulky attribute data.
3. With the above two points in mind, I'm going to hold off on surmising
any further on attributes until some of you guys weigh in with your
opinions. I do have some rough ideas, however I'll wait until we have a
consensus of exactly what we want to achieve here before we proceed
further.
Serializable works for me. Do we do have a generic Attribute<? extends
Serializable> or just forgo the generics?
Serializable could be fine. To be honest I'm fairly biased in this
topic. Mainly becuase for IDM 1.x I lost quite significant amount of
time to implement and properly test binary attributes support for like 7
different databases. It was never really used. I would start small in
this area. It is always possible to extend this part of the api later
however we may find out that there is no need for this.
We should think what are our usecases for attributes. Maybe Bill should
jump in and express his usecases and ideas. Most security tokens can be
serialized into string I guess.
I also agree that we should spend some time to check compatibility with
JSR351 API here. It will be so flexible that we rather won't be able to
implement it fully however it would be good to reduce limitations for
JSR351IdentityStore.
Another obvious usecase is storing avatar or other binary content. My
take is that application should rather implement separate binary store
and just keep basic reference as user attribute. I would simply veto any
binary attribute support idea until we have clear agreement that it is
really necessary.
Are we addressing any attribute metadata so far? This is probably more
identity store concern however part of attributes may be marked
readonly. Also in LDAP scenario multi value support is attribute
specific and this info may be worth exposing in the API directly.
Another one that is quite huge is to support unique attributes. Common
requirement is to have unique email attribute value. It is also common
to search user by unique attribute - either email or token. This needs
to be easy and efficient on the implementation side.
Utility methods
--------------------
IdentityType lookupIdentityByKey(String key);
My thoughts:
1. This method is required by other features, such as the Permissions
API. I'm fine with what we have here.
Side question - how do you want to express difference between User,
Group and Role here? Are we going to have some clear key format specified?
B.
9:01 p.m.
On 11/08/2012 11:32 PM, Bolesław Dawidowicz wrote:
On 11/07/2012 07:08 PM, Jason Porter wrote:
> Attributes
> -------------
>
> void setAttribute(IdentityType identityType, String attributeName,
> String attributeValue);
>
> String getAttribute(IdentityType identityType, String
> attributeName);
>
> My thoughts:
>
> 1. This area poses a slight dilemma. Currently the API only supports
> simple String-based attribute values, however I'm pretty sure that
> people are going to want to store all sorts of things, ranging from
> boolean and Date values through to large byte arrays that store a User's
> photograph or other data. If everyone is in agreement that we need to
> support more than just String values, then the first thing we probably
> need to do is modify these methods to work with a Serializable instead.
>
> 2. If we all agree on point 1), the next thing we need to decide is
> whether we want the capability to make some attribute values "lazy
> loaded". If we want to do a quick lookup of a User object for the
> express purpose of assigning a Role or Group membership (or any other
> type of simple operation) then we probably don't want the performance
> hit of having to load bulky attribute data.
>
> 3. With the above two points in mind, I'm going to hold off on surmising
> any further on attributes until some of you guys weigh in with your
> opinions. I do have some rough ideas, however I'll wait until we have a
> consensus of exactly what we want to achieve here before we proceed
> further.
>
>
> Serializable works for me. Do we do have a generic Attribute<? extends
> Serializable> or just forgo the generics?
Serializable could be fine. To be honest I'm fairly biased in this
topic. Mainly becuase for IDM 1.x I lost quite significant amount of
time to implement and properly test binary attributes support for like 7
different databases. It was never really used. I would start small in
this area. It is always possible to extend this part of the api later
however we may find out that there is no need for this.
We should think what are our usecases for attributes. Maybe Bill should
jump in and express his usecases and ideas. Most security tokens can be
serialized into string I guess.
I also agree that we should spend some time to check compatibility with
JSR351 API here. It will be so flexible that we rather won't be able to
implement it fully however it would be good to reduce limitations for
JSR351IdentityStore.
Another obvious usecase is storing avatar or other binary content. My
take is that application should rather implement separate binary store
and just keep basic reference as user attribute. I would simply veto any
binary attribute support idea until we have clear agreement that it is
really necessary.
Are we addressing any attribute metadata so far? This is probably more
identity store concern however part of attributes may be marked
readonly. Also in LDAP scenario multi value support is attribute
specific and this info may be worth exposing in the API directly.
Another one that is quite huge is to support unique attributes. Common
requirement is to have unique email attribute value. It is also common
to search user by unique attribute - either email or token. This needs
to be easy and efficient on the implementation side.
Ok, so let's just keep it simple for now and support String valued
attributes. If we ever decide to support more complex types, backwards
compatibility shouldn't be an issue as String is Serializable anyway.
> Utility methods
> --------------------
>
> IdentityType lookupIdentityByKey(String key);
>
> My thoughts:
>
> 1. This method is required by other features, such as the Permissions
> API. I'm fine with what we have here.
Side question - how do you want to express difference between User,
Group and Role here? Are we going to have some clear key format specified?
Yes, the key is a unique identifier and can be used to identify a
specific User, Group or Role. At present the format I was going with
looks similar to a URL, i.e. USER://sbryzak, GROUP://employees/managers,
ROLE://admin etc.
B.
_______________________________________________
security-dev mailing list
security-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/security-dev
5:13 a.m.
On Nov 12, 2012, at 4:01 AM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
On 11/08/2012 11:32 PM, Bolesław Dawidowicz wrote:
> On 11/07/2012 07:08 PM, Jason Porter wrote:
>> Attributes
>> -------------
>>
>> void setAttribute(IdentityType identityType, String attributeName,
>> String attributeValue);
>>
>> String getAttribute(IdentityType identityType, String
>> attributeName);
>>
>> My thoughts:
>>
>> 1. This area poses a slight dilemma. Currently the API only supports
>> simple String-based attribute values, however I'm pretty sure that
>> people are going to want to store all sorts of things, ranging from
>> boolean and Date values through to large byte arrays that store a User's
>> photograph or other data. If everyone is in agreement that we need to
>> support more than just String values, then the first thing we probably
>> need to do is modify these methods to work with a Serializable instead.
>>
>> 2. If we all agree on point 1), the next thing we need to decide is
>> whether we want the capability to make some attribute values "lazy
>> loaded". If we want to do a quick lookup of a User object for the
>> express purpose of assigning a Role or Group membership (or any other
>> type of simple operation) then we probably don't want the performance
>> hit of having to load bulky attribute data.
>>
>> 3. With the above two points in mind, I'm going to hold off on surmising
>> any further on attributes until some of you guys weigh in with your
>> opinions. I do have some rough ideas, however I'll wait until we have a
>> consensus of exactly what we want to achieve here before we proceed
>> further.
>>
>>
>> Serializable works for me. Do we do have a generic Attribute<? extends
>> Serializable> or just forgo the generics?
> Serializable could be fine. To be honest I'm fairly biased in this
> topic. Mainly becuase for IDM 1.x I lost quite significant amount of
> time to implement and properly test binary attributes support for like 7
> different databases. It was never really used. I would start small in
> this area. It is always possible to extend this part of the api later
> however we may find out that there is no need for this.
>
> We should think what are our usecases for attributes. Maybe Bill should
> jump in and express his usecases and ideas. Most security tokens can be
> serialized into string I guess.
>
> I also agree that we should spend some time to check compatibility with
> JSR351 API here. It will be so flexible that we rather won't be able to
> implement it fully however it would be good to reduce limitations for
> JSR351IdentityStore.
>
> Another obvious usecase is storing avatar or other binary content. My
> take is that application should rather implement separate binary store
> and just keep basic reference as user attribute. I would simply veto any
> binary attribute support idea until we have clear agreement that it is
> really necessary.
>
> Are we addressing any attribute metadata so far? This is probably more
> identity store concern however part of attributes may be marked
> readonly. Also in LDAP scenario multi value support is attribute
> specific and this info may be worth exposing in the API directly.
>
> Another one that is quite huge is to support unique attributes. Common
> requirement is to have unique email attribute value. It is also common
> to search user by unique attribute - either email or token. This needs
> to be easy and efficient on the implementation side.
>
>
>
Ok, so let's just keep it simple for now and support String valued
attributes. If we ever decide to support more complex types, backwards
compatibility shouldn't be an issue as String is Serializable anyway.
What do you think about adding some attributes metadata?
>> Utility methods
>> --------------------
>>
>> IdentityType lookupIdentityByKey(String key);
>>
>> My thoughts:
>>
>> 1. This method is required by other features, such as the Permissions
>> API. I'm fine with what we have here.
> Side question - how do you want to express difference between User,
> Group and Role here? Are we going to have some clear key format specified?
Yes, the key is a unique identifier and can be used to identify a
specific User, Group or Role. At present the format I was going with
looks similar to a URL, i.e. USER://sbryzak, GROUP://employees/managers,
ROLE://admin etc.
>
> B.
> _______________________________________________
> security-dev mailing list
> security-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/security-dev
_______________________________________________
security-dev mailing list
security-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/security-dev
7:12 p.m.
On 11/12/2012 09:13 PM, Boleslaw Dawidowicz wrote:
On Nov 12, 2012, at 4:01 AM, Shane Bryzak <sbryzak(a)redhat.com>
wrote:
> On 11/08/2012 11:32 PM, Bolesław Dawidowicz wrote:
>> On 11/07/2012 07:08 PM, Jason Porter wrote:
>>> Serializable works for me. Do we do have a generic Attribute<? extends
>>> Serializable> or just forgo the generics?
>> Serializable could be fine. To be honest I'm fairly biased in this
>> topic. Mainly becuase for IDM 1.x I lost quite significant amount of
>> time to implement and properly test binary attributes support for like 7
>> different databases. It was never really used. I would start small in
>> this area. It is always possible to extend this part of the api later
>> however we may find out that there is no need for this.
>>
>> We should think what are our usecases for attributes. Maybe Bill should
>> jump in and express his usecases and ideas. Most security tokens can be
>> serialized into string I guess.
>>
>> I also agree that we should spend some time to check compatibility with
>> JSR351 API here. It will be so flexible that we rather won't be able to
>> implement it fully however it would be good to reduce limitations for
>> JSR351IdentityStore.
>>
>> Another obvious usecase is storing avatar or other binary content. My
>> take is that application should rather implement separate binary store
>> and just keep basic reference as user attribute. I would simply veto any
>> binary attribute support idea until we have clear agreement that it is
>> really necessary.
>>
>> Are we addressing any attribute metadata so far? This is probably more
>> identity store concern however part of attributes may be marked
>> readonly. Also in LDAP scenario multi value support is attribute
>> specific and this info may be worth exposing in the API directly.
>>
>> Another one that is quite huge is to support unique attributes. Common
>> requirement is to have unique email attribute value. It is also common
>> to search user by unique attribute - either email or token. This needs
>> to be easy and efficient on the implementation side.
>>
>>
>>
> Ok, so let's just keep it simple for now and support String valued
> attributes. If we ever decide to support more complex types, backwards
> compatibility shouldn't be an issue as String is Serializable anyway.
What do you think about adding some attributes metadata?
Sure, we can do that - which specific metadata were you thinking of in
relation to attributes? I've thought about Jason's suggestion to create
a generic Attribute value, and I think it's a good idea. We can leave
it up to the IdentityStore implementation whether it supports certain
types of attributes - this way we can document the supported types and
throw an UnsupportedOperationException (or something like that) if an
unsupported attribute value is passed. We also leave it up to the
developer if they wish to extend the IdentityStore SPI to provide
support for attribute types that we don't support out of the box.
Here's what I'm proposing for the Attribute class:
public class Attribute<T extends Serializable> {
private String name;
private T value;
public Attribute(String name, T value) {
this.name = name;
this.value = value;
}
public String getName() {
return name;
}
public T getValue() {
return value;
}
}
This would mean the following changes to IdentityManager (including the
corresponding changes to IdentityStore):
Replace:
void setAttribute(IdentityType identityType, String attributeName,
String attributeValue);
String getAttribute(IdentityType identityType, String attributeName);
With:
void setAttribute(IdentityType identityType, Attribute attribute);
<T extends Serializable> Attribute<T> getAttribute(IdentityType
identityType, String attributeName);
And the following changes to IdentityType:
Replace:
void setAttribute(String name, String value);
void setAttribute(String name, String[] values);
void removeAttribute(String name);
String getAttribute(String name);
String[] getAttributeValues(String name);
Map<String, String[]> getAttributes();
With:
void setAttribute(String name, Attribute value);
void removeAttribute(String attributeName);
<T extends Serializable> Attribute<T> getAttribute(String
attributeName);
7:15 a.m.
On Nov 13, 2012, at 2:12 AM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
On 11/12/2012 09:13 PM, Boleslaw Dawidowicz wrote:
> On Nov 12, 2012, at 4:01 AM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
>
>> On 11/08/2012 11:32 PM, Bolesław Dawidowicz wrote:
>>> On 11/07/2012 07:08 PM, Jason Porter wrote:
>>>> Serializable works for me. Do we do have a generic Attribute<?
extends
>>>> Serializable> or just forgo the generics?
>>> Serializable could be fine. To be honest I'm fairly biased in this
>>> topic. Mainly becuase for IDM 1.x I lost quite significant amount of
>>> time to implement and properly test binary attributes support for like 7
>>> different databases. It was never really used. I would start small in
>>> this area. It is always possible to extend this part of the api later
>>> however we may find out that there is no need for this.
>>>
>>> We should think what are our usecases for attributes. Maybe Bill should
>>> jump in and express his usecases and ideas. Most security tokens can be
>>> serialized into string I guess.
>>>
>>> I also agree that we should spend some time to check compatibility with
>>> JSR351 API here. It will be so flexible that we rather won't be able to
>>> implement it fully however it would be good to reduce limitations for
>>> JSR351IdentityStore.
>>>
>>> Another obvious usecase is storing avatar or other binary content. My
>>> take is that application should rather implement separate binary store
>>> and just keep basic reference as user attribute. I would simply veto any
>>> binary attribute support idea until we have clear agreement that it is
>>> really necessary.
>>>
>>> Are we addressing any attribute metadata so far? This is probably more
>>> identity store concern however part of attributes may be marked
>>> readonly. Also in LDAP scenario multi value support is attribute
>>> specific and this info may be worth exposing in the API directly.
>>>
>>> Another one that is quite huge is to support unique attributes. Common
>>> requirement is to have unique email attribute value. It is also common
>>> to search user by unique attribute - either email or token. This needs
>>> to be easy and efficient on the implementation side.
>>>
>>>
>>>
>> Ok, so let's just keep it simple for now and support String valued
>> attributes. If we ever decide to support more complex types, backwards
>> compatibility shouldn't be an issue as String is Serializable anyway.
> What do you think about adding some attributes metadata?
>
Sure, we can do that - which specific metadata were you thinking of in
relation to attributes?
- read only
- multi/single value
- unique
I've thought about Jason's suggestion to create
a generic Attribute value, and I think it's a good idea. We can leave
it up to the IdentityStore implementation whether it supports certain
types of attributes - this way we can document the supported types and
throw an UnsupportedOperationException (or something like that) if an
unsupported attribute value is passed. We also leave it up to the
developer if they wish to extend the IdentityStore SPI to provide
support for attribute types that we don't support out of the box.
Here's what I'm proposing for the Attribute class:
public class Attribute<T extends Serializable> {
private String name;
private T value;
public Attribute(String name, T value) {
this.name = name;
this.value = value;
}
public String getName() {
return name;
}
public T getValue() {
return value;
}
}
This would mean the following changes to IdentityManager (including the
corresponding changes to IdentityStore):
Replace:
void setAttribute(IdentityType identityType, String attributeName,
String attributeValue);
String getAttribute(IdentityType identityType, String attributeName);
With:
void setAttribute(IdentityType identityType, Attribute attribute);
<T extends Serializable> Attribute<T> getAttribute(IdentityType
identityType, String attributeName);
And the following changes to IdentityType:
Replace:
void setAttribute(String name, String value);
void setAttribute(String name, String[] values);
void removeAttribute(String name);
String getAttribute(String name);
String[] getAttributeValues(String name);
Map<String, String[]> getAttributes();
With:
void setAttribute(String name, Attribute value);
void removeAttribute(String attributeName);
<T extends Serializable> Attribute<T> getAttribute(String
attributeName);
Looks good to me. Just that you have single value inside of Attribute and many in methods
:)
B.
5:16 a.m.
On Nov 12, 2012, at 4:01 AM, Shane Bryzak <sbryzak(a)redhat.com> wrote:
On 11/08/2012 11:32 PM, Bolesław Dawidowicz wrote:
> On 11/07/2012 07:08 PM, Jason Porter wrote:
>> Attributes
>> -------------
>>
>> void setAttribute(IdentityType identityType, String attributeName,
>> String attributeValue);
>>
>> String getAttribute(IdentityType identityType, String
>> attributeName);
>>
>> My thoughts:
>>
>> 1. This area poses a slight dilemma. Currently the API only supports
>> simple String-based attribute values, however I'm pretty sure that
>> people are going to want to store all sorts of things, ranging from
>> boolean and Date values through to large byte arrays that store a User's
>> photograph or other data. If everyone is in agreement that we need to
>> support more than just String values, then the first thing we probably
>> need to do is modify these methods to work with a Serializable instead.
>>
>> 2. If we all agree on point 1), the next thing we need to decide is
>> whether we want the capability to make some attribute values "lazy
>> loaded". If we want to do a quick lookup of a User object for the
>> express purpose of assigning a Role or Group membership (or any other
>> type of simple operation) then we probably don't want the performance
>> hit of having to load bulky attribute data.
>>
>> 3. With the above two points in mind, I'm going to hold off on surmising
>> any further on attributes until some of you guys weigh in with your
>> opinions. I do have some rough ideas, however I'll wait until we have a
>> consensus of exactly what we want to achieve here before we proceed
>> further.
>>
>>
>> Serializable works for me. Do we do have a generic Attribute<? extends
>> Serializable> or just forgo the generics?
> Serializable could be fine. To be honest I'm fairly biased in this
> topic. Mainly becuase for IDM 1.x I lost quite significant amount of
> time to implement and properly test binary attributes support for like 7
> different databases. It was never really used. I would start small in
> this area. It is always possible to extend this part of the api later
> however we may find out that there is no need for this.
>
> We should think what are our usecases for attributes. Maybe Bill should
> jump in and express his usecases and ideas. Most security tokens can be
> serialized into string I guess.
>
> I also agree that we should spend some time to check compatibility with
> JSR351 API here. It will be so flexible that we rather won't be able to
> implement it fully however it would be good to reduce limitations for
> JSR351IdentityStore.
>
> Another obvious usecase is storing avatar or other binary content. My
> take is that application should rather implement separate binary store
> and just keep basic reference as user attribute. I would simply veto any
> binary attribute support idea until we have clear agreement that it is
> really necessary.
>
> Are we addressing any attribute metadata so far? This is probably more
> identity store concern however part of attributes may be marked
> readonly. Also in LDAP scenario multi value support is attribute
> specific and this info may be worth exposing in the API directly.
>
> Another one that is quite huge is to support unique attributes. Common
> requirement is to have unique email attribute value. It is also common
> to search user by unique attribute - either email or token. This needs
> to be easy and efficient on the implementation side.
>
>
>
Ok, so let's just keep it simple for now and support String valued
attributes. If we ever decide to support more complex types, backwards
compatibility shouldn't be an issue as String is Serializable anyway.
>> Utility methods
>> --------------------
>>
>> IdentityType lookupIdentityByKey(String key);
>>
>> My thoughts:
>>
>> 1. This method is required by other features, such as the Permissions
>> API. I'm fine with what we have here.
> Side question - how do you want to express difference between User,
> Group and Role here? Are we going to have some clear key format specified?
Yes, the key is a unique identifier and can be used to identify a
specific User, Group or Role. At present the format I was going with
looks similar to a URL, i.e. USER://sbryzak, GROUP://employees/managers,
ROLE://admin etc.
You'll need a double check if there is no issue to pass those as url params or in REST
context.
>
> B.
> _______________________________________________
> security-dev mailing list
> security-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/security-dev
_______________________________________________
security-dev mailing list
security-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/security-dev
12:29 p.m.
Any reason why a role cannot be granted to just a User?
----- "Shane Bryzak" <sbryzak(a)redhat.com> wrote:
Hey guys,
I spent some time reviewing the IdentityManager API today to identify
any redundancies and also locate any holes where we might not properly
support required features, as well as a general "sanity" check to
ensure
that what we're exposing via this API makes sense and is intuitive for
consumers. As Anil has pointed out already, our short term priority
for
the project is to stabilize the API - this is extremely important as
PicketLink will provide the security foundation for many other
projects. With that in mind I'd like to strongly encourage everyone
with a stake in this to carefully review the API and provide feedback,
as once we release it it will be essentially set in stone.
To try and avoid a wall of text (and make this post easier to reply
to),
I'm going to break this post up into sub-sections, one for each
feature
group. For each section, I'll include the API as it currently exists,
followed by a brief summary of my thoughts and any recommendations I
may
have - this is especially where I want to hear any feedback indicating
whether you agree or disagree with my assessment. Let's start with
the
user-related methods:
User management
-------------------------
User createUser(String name);
void createUser(User user);
void removeUser(User user);
void removeUser(String name);
User getUser(String name);
My thoughts:
1. I'm wondering if we should remove the createUser(String) method and
just have createUser(User). This would make sense in a way because I
can't think of many use cases where you might want to create a User
with
just a username and not any of the other typical information (such as
first name, last name, e-mail address, etc). Creating a User via
createUser(String) when you actually want to set the other details
after
the initial User creation is also horribly inefficient, requiring
multiple round trips to the database (or whatever identity store
backend
you use). To add to that, the actual parameter name is a little
unintuitive - "name" could refer to any number of things - is it the
username or the user's actual name that you need to provide here? My
+1
goes to removing createUser(String).
2. Similar to point 1), having two removeUser() methods seems equally
redundant. The User object that needs to be provided to the
removeUser(User) method can be easily looked up by calling getUser(),
as
per this example:
identityManager.removeUser(identityManager.getUser("jsmith"));
3. As I also pointed out in point 1), the "name" parameter is a little
unintuitive. In regard to the getUser() method I think we should
rename
the "name" parameter to "username" just so it's perfectly clear
what
the
method expects.
4. We don't currently have any way to update User details. I
recommend
that we add an updateUser() method as follows:
void updateUser(User user);
This will allow User details such as their first and last name, e-mail
address etc to be updated without having to delete and re-create the
User. Extending on this a little further, I'm wondering if we should
think about adding an audit API that logs these changes. Perhaps it's
something to think about for a later release.
Group management
--------------------------
Group createGroup(String id);
Group createGroup(String id, Group parent);
Group createGroup(String id, String parent);
void removeGroup(Group group);
void removeGroup(String groupId);
Group getGroup(String groupId);
Group getGroup(String groupId, Group parent);
void addToGroup(IdentityType identityType, Group group);
void removeFromGroup(IdentityType identityType, Group group);
My thoughts:
1. I think there is a little bit of ambiguity here in regard to the
differences between a Group's ID and name. To assist in understanding
the differences, here's the JavaDoc pasted from the Group interface:
/**
* Groups are stored in tree hierarchy and therefore ID
represents
a path. ID string always begins with "/" element that
* represents root of the tree
* <p/>
* Example: Valid IDs are "/acme/departments/marketing",
"/security/administrator" or "/administrator". Where
"acme",
* "departments", "marketing", "security" and
"administrator" are
group names.
*
* @return Group Id in String representation.
*/
String getId();
/**
* Group name is unique identifier in specific group tree branch.
For example group with id "/acme/departments/marketing"
* will have name "marketing" and parent group of id
"/acme/departments"
*
* @return name
*/
String getName();
Taking the above JavaDoc into account, it seems that where we are
specifying "id" as a method parameter we should be specifying "name"
instead. If we are indeed providing a "fully qualified" path to the
createGroup() method, then there is no need to also specify the parent
as it can be easily derived from the path. More on this in the
following points.
2. We currently have three createGroup() methods for creating a new
Group. I think we should remove the (String, String) variant, as the
parent parameter is ambiguous (is it the parent's ID or name we need
to
specify here?), and rename the "id" parameter of the (String, Group)
variant to "name", leaving us with the following:
Group createGroup(String id);
Group createGroup(String name, Group parent);
This then gives us two ways of creating a group. We can either
specify
the fully qualified group ID:
Group employees = identityManager.createGroup("/employees");
Or we can specify a subgroup name and parent Group:
Group managers = identityManager.createGroup("managers", employees);
We can also use this second form to create a "root" group by just
specifying null for the parent:
Group admins = identityManager.createGroup("admins", null);
3. For the removeGroup() methods, I think what we have is fine.
4. For the getGroup() methods, I'm happy with the first one (String
groupId) but I think the second one (String groupId, Group parent)
needs
to be (String name, Group parent) instead. The first method would be
used when the fully qualified group ID is known, and the second one
would be used when you already have the parent Group and know the name
of the subgroup.
5. The addToGroup() and removeFromGroup() methods seem fine to me.
6. One thing we don't have is a method to test whether an IdentityType
is a member of a Group. I suggest we add another method to support
this:
boolean inGroup(IdentityType identityType, Group group);
Roles
----------
Role createRole(String name);
void removeRole(Role role);
void removeRole(String name);
Role getRole(String name);
boolean hasRole(Role role, IdentityType identityType, Group
group);
void grantRole(Role role, IdentityType identityType, Group
group);
void revokeRole(Role role, IdentityType identityType, Group
group);
My thoughts:
1. I would remove the removeRole(String) method as the same thing can
be
achieved with removeRole(getRole(String)).
2. I would swap the order of the Role and IdentityType parameters
within
the hasRole(), grantRole() and revokeRole() methods. As the
IdentityType is the primary artifact in these operations it makes
sense
to have it listed first.
3. We don't have any explicit support for application roles, which are
roles where there is no Group component (for example, an
application-wide "admin" role). We could support this by just
allowing
hasRole(), grantRole() etc to accept null values for the Group
parameter, however I feel it would be more semantically correct to
provide a distinct set of methods for the purpose of supporting
application roles, as follows:
boolean hasApplicationRole(IdentityType identityType, Role
role);
void grantApplicationRole(IdentityType identityType, Role role);
void revokeApplicationRole(IdentityType identityType, Role
role);
Query API
--------------
<T extends IdentityType> IdentityQuery<T> createQuery();
My thoughts:
1. After our recent rewrite of the Query API I'm satisfied with what
we
have now.
Credential management
-------------------------------
boolean validateCredential(User user, Credential credential);
void updateCredential(User user, Credential credential);
My thoughts:
1. I'm satisfied with what we currently have, however we still need to
review the mechanism that we provide for encoding passwords. I'm not
sure that it will have any effect on these methods though (I'm toying
with the idea of integrating the password encoding functionality via
the
IdentityStoreInvocationContext).
Identity expiry
------------------
void setEnabled(IdentityType identityType, boolean enabled);
void setExpirationDate(IdentityType identityType, Date
expirationDate);
My thoughts:
1. I think these methods are fine the way they are. One discrepancy
that I've identified is that a User can be created already being
disabled or having an expiry date, while for a Role or Group the
equivalent status must be set via these methods after creation. I'm
not
sure this is a big deal though.
Attributes
-------------
void setAttribute(IdentityType identityType, String
attributeName,
String attributeValue);
String getAttribute(IdentityType identityType, String
attributeName);
My thoughts:
1. This area poses a slight dilemma. Currently the API only supports
simple String-based attribute values, however I'm pretty sure that
people are going to want to store all sorts of things, ranging from
boolean and Date values through to large byte arrays that store a
User's
photograph or other data. If everyone is in agreement that we need to
support more than just String values, then the first thing we probably
need to do is modify these methods to work with a Serializable
instead.
2. If we all agree on point 1), the next thing we need to decide is
whether we want the capability to make some attribute values "lazy
loaded". If we want to do a quick lookup of a User object for the
express purpose of assigning a Role or Group membership (or any other
type of simple operation) then we probably don't want the performance
hit of having to load bulky attribute data.
3. With the above two points in mind, I'm going to hold off on
surmising
any further on attributes until some of you guys weigh in with your
opinions. I do have some rough ideas, however I'll wait until we have
a
consensus of exactly what we want to achieve here before we proceed
further.
Utility methods
--------------------
IdentityType lookupIdentityByKey(String key);
My thoughts:
1. This method is required by other features, such as the Permissions
API. I'm fine with what we have here.
In summary
----------------
I apologise for yet another epic e-mail to the list, however this is
everyone's chance to review the API for themselves and decide whether
or
not it will be suitable for addressing all of your requirements. I'd
like to get this API stable by next week so please don't be shy about
speaking up. I'm especially looking forward to hearing your opinions
about how we handle attributes (there may even be some relevant
JSR-351
stuff that we should be looking at here).
Thanks,
Shane
_______________________________________________
security-dev mailing list
security-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/security-dev
1:29 p.m.
Hey Rodney,
I assume you're referring to these methods:
void grantRole(Role role, IdentityType identityType, Group group);
void grantApplicationRole(IdentityType identityType, Role role);
The reason we use IdentityType instead of just User, is to allow roles
to be granted to an entire group of users at once. For example, let's
say that we wanted to automatically make every Red Hat employee a
moderator on some forums. We could do it like this:
Group employees = identityManager.getGroup("/employees");
identityManager.grantApplicationRole(employees,
identityManager.getRole("moderator"));
This way, when we create a new User and make them part of the
"employees" group, they automatically get moderator privileges.
The same goes for grantRole(). By allowing an IdentityType as a
parameter, we can assign a Role to either a User or a Group (both of
which extend IdentityType).
On 11/08/2012 04:29 AM, Rodney Russ wrote:
Any reason why a role cannot be granted to just a User?
----- "Shane Bryzak" <sbryzak(a)redhat.com> wrote:
> Hey guys,
>
> I spent some time reviewing the IdentityManager API today to identify
>
> any redundancies and also locate any holes where we might not properly
>
> support required features, as well as a general "sanity" check to
> ensure
> that what we're exposing via this API makes sense and is intuitive for
>
> consumers. As Anil has pointed out already, our short term priority
> for
> the project is to stabilize the API - this is extremely important as
> PicketLink will provide the security foundation for many other
> projects. With that in mind I'd like to strongly encourage everyone
> with a stake in this to carefully review the API and provide feedback,
>
> as once we release it it will be essentially set in stone.
>
> To try and avoid a wall of text (and make this post easier to reply
> to),
> I'm going to break this post up into sub-sections, one for each
> feature
> group. For each section, I'll include the API as it currently exists,
>
> followed by a brief summary of my thoughts and any recommendations I
> may
> have - this is especially where I want to hear any feedback indicating
>
> whether you agree or disagree with my assessment. Let's start with
> the
> user-related methods:
>
> User management
> -------------------------
>
> User createUser(String name);
>
> void createUser(User user);
>
> void removeUser(User user);
>
> void removeUser(String name);
>
> User getUser(String name);
>
> My thoughts:
>
> 1. I'm wondering if we should remove the createUser(String) method and
>
> just have createUser(User). This would make sense in a way because I
>
> can't think of many use cases where you might want to create a User
> with
> just a username and not any of the other typical information (such as
>
> first name, last name, e-mail address, etc). Creating a User via
> createUser(String) when you actually want to set the other details
> after
> the initial User creation is also horribly inefficient, requiring
> multiple round trips to the database (or whatever identity store
> backend
> you use). To add to that, the actual parameter name is a little
> unintuitive - "name" could refer to any number of things - is it the
> username or the user's actual name that you need to provide here? My
> +1
> goes to removing createUser(String).
>
> 2. Similar to point 1), having two removeUser() methods seems equally
>
> redundant. The User object that needs to be provided to the
> removeUser(User) method can be easily looked up by calling getUser(),
> as
> per this example:
>
> identityManager.removeUser(identityManager.getUser("jsmith"));
>
> 3. As I also pointed out in point 1), the "name" parameter is a little
>
> unintuitive. In regard to the getUser() method I think we should
> rename
> the "name" parameter to "username" just so it's perfectly
clear what
> the
> method expects.
>
> 4. We don't currently have any way to update User details. I
> recommend
> that we add an updateUser() method as follows:
>
> void updateUser(User user);
>
> This will allow User details such as their first and last name, e-mail
>
> address etc to be updated without having to delete and re-create the
> User. Extending on this a little further, I'm wondering if we should
>
> think about adding an audit API that logs these changes. Perhaps it's
>
> something to think about for a later release.
>
> Group management
> --------------------------
>
> Group createGroup(String id);
>
> Group createGroup(String id, Group parent);
>
> Group createGroup(String id, String parent);
>
> void removeGroup(Group group);
>
> void removeGroup(String groupId);
>
> Group getGroup(String groupId);
>
> Group getGroup(String groupId, Group parent);
>
> void addToGroup(IdentityType identityType, Group group);
>
> void removeFromGroup(IdentityType identityType, Group group);
>
> My thoughts:
>
> 1. I think there is a little bit of ambiguity here in regard to the
> differences between a Group's ID and name. To assist in understanding
>
> the differences, here's the JavaDoc pasted from the Group interface:
>
> /**
> * Groups are stored in tree hierarchy and therefore ID
> represents
> a path. ID string always begins with "/" element that
> * represents root of the tree
> * <p/>
> * Example: Valid IDs are "/acme/departments/marketing",
> "/security/administrator" or "/administrator". Where
"acme",
> * "departments", "marketing", "security" and
"administrator" are
>
> group names.
> *
> * @return Group Id in String representation.
> */
> String getId();
>
> /**
> * Group name is unique identifier in specific group tree branch.
>
> For example group with id "/acme/departments/marketing"
> * will have name "marketing" and parent group of id
> "/acme/departments"
> *
> * @return name
> */
> String getName();
>
> Taking the above JavaDoc into account, it seems that where we are
> specifying "id" as a method parameter we should be specifying
"name"
> instead. If we are indeed providing a "fully qualified" path to the
> createGroup() method, then there is no need to also specify the parent
>
> as it can be easily derived from the path. More on this in the
> following points.
>
> 2. We currently have three createGroup() methods for creating a new
> Group. I think we should remove the (String, String) variant, as the
>
> parent parameter is ambiguous (is it the parent's ID or name we need
> to
> specify here?), and rename the "id" parameter of the (String, Group)
> variant to "name", leaving us with the following:
>
> Group createGroup(String id);
>
> Group createGroup(String name, Group parent);
>
> This then gives us two ways of creating a group. We can either
> specify
> the fully qualified group ID:
>
> Group employees = identityManager.createGroup("/employees");
>
> Or we can specify a subgroup name and parent Group:
>
> Group managers = identityManager.createGroup("managers", employees);
>
> We can also use this second form to create a "root" group by just
> specifying null for the parent:
>
> Group admins = identityManager.createGroup("admins", null);
>
> 3. For the removeGroup() methods, I think what we have is fine.
>
> 4. For the getGroup() methods, I'm happy with the first one (String
> groupId) but I think the second one (String groupId, Group parent)
> needs
> to be (String name, Group parent) instead. The first method would be
>
> used when the fully qualified group ID is known, and the second one
> would be used when you already have the parent Group and know the name
>
> of the subgroup.
>
> 5. The addToGroup() and removeFromGroup() methods seem fine to me.
>
> 6. One thing we don't have is a method to test whether an IdentityType
>
> is a member of a Group. I suggest we add another method to support
> this:
>
> boolean inGroup(IdentityType identityType, Group group);
>
> Roles
> ----------
>
> Role createRole(String name);
>
> void removeRole(Role role);
>
> void removeRole(String name);
>
> Role getRole(String name);
>
> boolean hasRole(Role role, IdentityType identityType, Group
> group);
>
> void grantRole(Role role, IdentityType identityType, Group
> group);
>
> void revokeRole(Role role, IdentityType identityType, Group
> group);
>
> My thoughts:
>
> 1. I would remove the removeRole(String) method as the same thing can
> be
> achieved with removeRole(getRole(String)).
>
> 2. I would swap the order of the Role and IdentityType parameters
> within
> the hasRole(), grantRole() and revokeRole() methods. As the
> IdentityType is the primary artifact in these operations it makes
> sense
> to have it listed first.
>
> 3. We don't have any explicit support for application roles, which are
>
> roles where there is no Group component (for example, an
> application-wide "admin" role). We could support this by just
> allowing
> hasRole(), grantRole() etc to accept null values for the Group
> parameter, however I feel it would be more semantically correct to
> provide a distinct set of methods for the purpose of supporting
> application roles, as follows:
>
> boolean hasApplicationRole(IdentityType identityType, Role
> role);
>
> void grantApplicationRole(IdentityType identityType, Role role);
>
> void revokeApplicationRole(IdentityType identityType, Role
> role);
>
>
> Query API
> --------------
>
> <T extends IdentityType> IdentityQuery<T> createQuery();
>
> My thoughts:
>
> 1. After our recent rewrite of the Query API I'm satisfied with what
> we
> have now.
>
> Credential management
> -------------------------------
>
> boolean validateCredential(User user, Credential credential);
>
> void updateCredential(User user, Credential credential);
>
> My thoughts:
>
> 1. I'm satisfied with what we currently have, however we still need to
>
> review the mechanism that we provide for encoding passwords. I'm not
>
> sure that it will have any effect on these methods though (I'm toying
>
> with the idea of integrating the password encoding functionality via
> the
> IdentityStoreInvocationContext).
>
> Identity expiry
> ------------------
>
> void setEnabled(IdentityType identityType, boolean enabled);
>
> void setExpirationDate(IdentityType identityType, Date
> expirationDate);
>
> My thoughts:
>
> 1. I think these methods are fine the way they are. One discrepancy
> that I've identified is that a User can be created already being
> disabled or having an expiry date, while for a Role or Group the
> equivalent status must be set via these methods after creation. I'm
> not
> sure this is a big deal though.
>
> Attributes
> -------------
>
> void setAttribute(IdentityType identityType, String
> attributeName,
> String attributeValue);
>
> String getAttribute(IdentityType identityType, String
> attributeName);
>
> My thoughts:
>
> 1. This area poses a slight dilemma. Currently the API only supports
>
> simple String-based attribute values, however I'm pretty sure that
> people are going to want to store all sorts of things, ranging from
> boolean and Date values through to large byte arrays that store a
> User's
> photograph or other data. If everyone is in agreement that we need to
>
> support more than just String values, then the first thing we probably
>
> need to do is modify these methods to work with a Serializable
> instead.
>
> 2. If we all agree on point 1), the next thing we need to decide is
> whether we want the capability to make some attribute values "lazy
> loaded". If we want to do a quick lookup of a User object for the
> express purpose of assigning a Role or Group membership (or any other
>
> type of simple operation) then we probably don't want the performance
>
> hit of having to load bulky attribute data.
>
> 3. With the above two points in mind, I'm going to hold off on
> surmising
> any further on attributes until some of you guys weigh in with your
> opinions. I do have some rough ideas, however I'll wait until we have
> a
> consensus of exactly what we want to achieve here before we proceed
> further.
>
> Utility methods
> --------------------
>
> IdentityType lookupIdentityByKey(String key);
>
> My thoughts:
>
> 1. This method is required by other features, such as the Permissions
>
> API. I'm fine with what we have here.
>
> In summary
> ----------------
>
> I apologise for yet another epic e-mail to the list, however this is
> everyone's chance to review the API for themselves and decide whether
> or
> not it will be suitable for addressing all of your requirements. I'd
>
> like to get this API stable by next week so please don't be shy about
>
> speaking up. I'm especially looking forward to hearing your opinions
>
> about how we handle attributes (there may even be some relevant
> JSR-351
> stuff that we should be looking at here).
>
> Thanks,
> Shane
> _______________________________________________
> security-dev mailing list
> security-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/security-dev
1:48 p.m.
Ah, understood. I didn't realize that IdentityType was an abstraction of Group and
User.
----- "Shane Bryzak" <sbryzak(a)redhat.com> wrote:
Hey Rodney,
I assume you're referring to these methods:
void grantRole(Role role, IdentityType identityType, Group group);
void grantApplicationRole(IdentityType identityType, Role role);
The reason we use IdentityType instead of just User, is to allow roles
to be granted to an entire group of users at once. For example, let's
say that we wanted to automatically make every Red Hat employee a
moderator on some forums. We could do it like this:
Group employees = identityManager.getGroup("/employees");
identityManager.grantApplicationRole(employees,
identityManager.getRole("moderator"));
This way, when we create a new User and make them part of the
"employees" group, they automatically get moderator privileges.
The same goes for grantRole(). By allowing an IdentityType as a
parameter, we can assign a Role to either a User or a Group (both of
which extend IdentityType).
On 11/08/2012 04:29 AM, Rodney Russ wrote:
> Any reason why a role cannot be granted to just a User?
>
> ----- "Shane Bryzak" <sbryzak(a)redhat.com> wrote:
>
>> Hey guys,
>>
>> I spent some time reviewing the IdentityManager API today to
identify
>>
>> any redundancies and also locate any holes where we might not
properly
>>
>> support required features, as well as a general "sanity" check to
>> ensure
>> that what we're exposing via this API makes sense and is intuitive
for
>>
>> consumers. As Anil has pointed out already, our short term
priority
>> for
>> the project is to stabilize the API - this is extremely important
as
>> PicketLink will provide the security foundation for many other
>> projects. With that in mind I'd like to strongly encourage
everyone
>> with a stake in this to carefully review the API and provide
feedback,
>>
>> as once we release it it will be essentially set in stone.
>>
>> To try and avoid a wall of text (and make this post easier to
reply
>> to),
>> I'm going to break this post up into sub-sections, one for each
>> feature
>> group. For each section, I'll include the API as it currently
exists,
>>
>> followed by a brief summary of my thoughts and any recommendations
I
>> may
>> have - this is especially where I want to hear any feedback
indicating
>>
>> whether you agree or disagree with my assessment. Let's start
with
>> the
>> user-related methods:
>>
>> User management
>> -------------------------
>>
>> User createUser(String name);
>>
>> void createUser(User user);
>>
>> void removeUser(User user);
>>
>> void removeUser(String name);
>>
>> User getUser(String name);
>>
>> My thoughts:
>>
>> 1. I'm wondering if we should remove the createUser(String) method
and
>>
>> just have createUser(User). This would make sense in a way because
I
>>
>> can't think of many use cases where you might want to create a
User
>> with
>> just a username and not any of the other typical information (such
as
>>
>> first name, last name, e-mail address, etc). Creating a User via
>> createUser(String) when you actually want to set the other details
>> after
>> the initial User creation is also horribly inefficient, requiring
>> multiple round trips to the database (or whatever identity store
>> backend
>> you use). To add to that, the actual parameter name is a little
>> unintuitive - "name" could refer to any number of things - is it
the
>> username or the user's actual name that you need to provide here?
My
>> +1
>> goes to removing createUser(String).
>>
>> 2. Similar to point 1), having two removeUser() methods seems
equally
>>
>> redundant. The User object that needs to be provided to the
>> removeUser(User) method can be easily looked up by calling
getUser(),
>> as
>> per this example:
>>
>> identityManager.removeUser(identityManager.getUser("jsmith"));
>>
>> 3. As I also pointed out in point 1), the "name" parameter is a
little
>>
>> unintuitive. In regard to the getUser() method I think we should
>> rename
>> the "name" parameter to "username" just so it's
perfectly clear
what
>> the
>> method expects.
>>
>> 4. We don't currently have any way to update User details. I
>> recommend
>> that we add an updateUser() method as follows:
>>
>> void updateUser(User user);
>>
>> This will allow User details such as their first and last name,
e-mail
>>
>> address etc to be updated without having to delete and re-create
the
>> User. Extending on this a little further, I'm wondering if we
should
>>
>> think about adding an audit API that logs these changes. Perhaps
it's
>>
>> something to think about for a later release.
>>
>> Group management
>> --------------------------
>>
>> Group createGroup(String id);
>>
>> Group createGroup(String id, Group parent);
>>
>> Group createGroup(String id, String parent);
>>
>> void removeGroup(Group group);
>>
>> void removeGroup(String groupId);
>>
>> Group getGroup(String groupId);
>>
>> Group getGroup(String groupId, Group parent);
>>
>> void addToGroup(IdentityType identityType, Group group);
>>
>> void removeFromGroup(IdentityType identityType, Group
group);
>>
>> My thoughts:
>>
>> 1. I think there is a little bit of ambiguity here in regard to
the
>> differences between a Group's ID and name. To assist in
understanding
>>
>> the differences, here's the JavaDoc pasted from the Group
interface:
>>
>> /**
>> * Groups are stored in tree hierarchy and therefore ID
>> represents
>> a path. ID string always begins with "/" element that
>> * represents root of the tree
>> * <p/>
>> * Example: Valid IDs are "/acme/departments/marketing",
>> "/security/administrator" or "/administrator". Where
"acme",
>> * "departments", "marketing", "security"
and "administrator"
are
>>
>> group names.
>> *
>> * @return Group Id in String representation.
>> */
>> String getId();
>>
>> /**
>> * Group name is unique identifier in specific group tree
branch.
>>
>> For example group with id "/acme/departments/marketing"
>> * will have name "marketing" and parent group of id
>> "/acme/departments"
>> *
>> * @return name
>> */
>> String getName();
>>
>> Taking the above JavaDoc into account, it seems that where we are
>> specifying "id" as a method parameter we should be specifying
"name"
>> instead. If we are indeed providing a "fully qualified" path to
the
>> createGroup() method, then there is no need to also specify the
parent
>>
>> as it can be easily derived from the path. More on this in the
>> following points.
>>
>> 2. We currently have three createGroup() methods for creating a
new
>> Group. I think we should remove the (String, String) variant, as
the
>>
>> parent parameter is ambiguous (is it the parent's ID or name we
need
>> to
>> specify here?), and rename the "id" parameter of the (String,
Group)
>> variant to "name", leaving us with the following:
>>
>> Group createGroup(String id);
>>
>> Group createGroup(String name, Group parent);
>>
>> This then gives us two ways of creating a group. We can either
>> specify
>> the fully qualified group ID:
>>
>> Group employees = identityManager.createGroup("/employees");
>>
>> Or we can specify a subgroup name and parent Group:
>>
>> Group managers = identityManager.createGroup("managers",
employees);
>>
>> We can also use this second form to create a "root" group by just
>> specifying null for the parent:
>>
>> Group admins = identityManager.createGroup("admins", null);
>>
>> 3. For the removeGroup() methods, I think what we have is fine.
>>
>> 4. For the getGroup() methods, I'm happy with the first one
(String
>> groupId) but I think the second one (String groupId, Group parent)
>> needs
>> to be (String name, Group parent) instead. The first method would
be
>>
>> used when the fully qualified group ID is known, and the second
one
>> would be used when you already have the parent Group and know the
name
>>
>> of the subgroup.
>>
>> 5. The addToGroup() and removeFromGroup() methods seem fine to me.
>>
>> 6. One thing we don't have is a method to test whether an
IdentityType
>>
>> is a member of a Group. I suggest we add another method to
support
>> this:
>>
>> boolean inGroup(IdentityType identityType, Group group);
>>
>> Roles
>> ----------
>>
>> Role createRole(String name);
>>
>> void removeRole(Role role);
>>
>> void removeRole(String name);
>>
>> Role getRole(String name);
>>
>> boolean hasRole(Role role, IdentityType identityType, Group
>> group);
>>
>> void grantRole(Role role, IdentityType identityType, Group
>> group);
>>
>> void revokeRole(Role role, IdentityType identityType, Group
>> group);
>>
>> My thoughts:
>>
>> 1. I would remove the removeRole(String) method as the same thing
can
>> be
>> achieved with removeRole(getRole(String)).
>>
>> 2. I would swap the order of the Role and IdentityType parameters
>> within
>> the hasRole(), grantRole() and revokeRole() methods. As the
>> IdentityType is the primary artifact in these operations it makes
>> sense
>> to have it listed first.
>>
>> 3. We don't have any explicit support for application roles, which
are
>>
>> roles where there is no Group component (for example, an
>> application-wide "admin" role). We could support this by just
>> allowing
>> hasRole(), grantRole() etc to accept null values for the Group
>> parameter, however I feel it would be more semantically correct to
>> provide a distinct set of methods for the purpose of supporting
>> application roles, as follows:
>>
>> boolean hasApplicationRole(IdentityType identityType, Role
>> role);
>>
>> void grantApplicationRole(IdentityType identityType, Role
role);
>>
>> void revokeApplicationRole(IdentityType identityType, Role
>> role);
>>
>>
>> Query API
>> --------------
>>
>> <T extends IdentityType> IdentityQuery<T> createQuery();
>>
>> My thoughts:
>>
>> 1. After our recent rewrite of the Query API I'm satisfied with
what
>> we
>> have now.
>>
>> Credential management
>> -------------------------------
>>
>> boolean validateCredential(User user, Credential
credential);
>>
>> void updateCredential(User user, Credential credential);
>>
>> My thoughts:
>>
>> 1. I'm satisfied with what we currently have, however we still need
to
>>
>> review the mechanism that we provide for encoding passwords. I'm
not
>>
>> sure that it will have any effect on these methods though (I'm
toying
>>
>> with the idea of integrating the password encoding functionality
via
>> the
>> IdentityStoreInvocationContext).
>>
>> Identity expiry
>> ------------------
>>
>> void setEnabled(IdentityType identityType, boolean enabled);
>>
>> void setExpirationDate(IdentityType identityType, Date
>> expirationDate);
>>
>> My thoughts:
>>
>> 1. I think these methods are fine the way they are. One
discrepancy
>> that I've identified is that a User can be created already being
>> disabled or having an expiry date, while for a Role or Group the
>> equivalent status must be set via these methods after creation.
I'm
>> not
>> sure this is a big deal though.
>>
>> Attributes
>> -------------
>>
>> void setAttribute(IdentityType identityType, String
>> attributeName,
>> String attributeValue);
>>
>> String getAttribute(IdentityType identityType, String
>> attributeName);
>>
>> My thoughts:
>>
>> 1. This area poses a slight dilemma. Currently the API only
supports
>>
>> simple String-based attribute values, however I'm pretty sure that
>> people are going to want to store all sorts of things, ranging
from
>> boolean and Date values through to large byte arrays that store a
>> User's
>> photograph or other data. If everyone is in agreement that we need
to
>>
>> support more than just String values, then the first thing we
probably
>>
>> need to do is modify these methods to work with a Serializable
>> instead.
>>
>> 2. If we all agree on point 1), the next thing we need to decide
is
>> whether we want the capability to make some attribute values "lazy
>> loaded". If we want to do a quick lookup of a User object for the
>> express purpose of assigning a Role or Group membership (or any
other
>>
>> type of simple operation) then we probably don't want the
performance
>>
>> hit of having to load bulky attribute data.
>>
>> 3. With the above two points in mind, I'm going to hold off on
>> surmising
>> any further on attributes until some of you guys weigh in with
your
>> opinions. I do have some rough ideas, however I'll wait until we
have
>> a
>> consensus of exactly what we want to achieve here before we
proceed
>> further.
>>
>> Utility methods
>> --------------------
>>
>> IdentityType lookupIdentityByKey(String key);
>>
>> My thoughts:
>>
>> 1. This method is required by other features, such as the
Permissions
>>
>> API. I'm fine with what we have here.
>>
>> In summary
>> ----------------
>>
>> I apologise for yet another epic e-mail to the list, however this
is
>> everyone's chance to review the API for themselves and decide
whether
>> or
>> not it will be suitable for addressing all of your requirements.
I'd
>>
>> like to get this API stable by next week so please don't be shy
about
>>
>> speaking up. I'm especially looking forward to hearing your
opinions
>>
>> about how we handle attributes (there may even be some relevant
>> JSR-351
>> stuff that we should be looking at here).
>>
>> Thanks,
>> Shane
>> _______________________________________________
>> security-dev mailing list
>> security-dev(a)lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/security-dev
10:22 a.m.
One area that is missing from the design is error reporting and
exceptions in general. How should we pass the information about failure
of specific operation and reasons behind it?
One of limitations we have now in GateIn is lack of proper way to pass
info about nature of failure back into UI. In case of JPAIdentityStore
problem is a bit smaller because it is easier to predict possible
reasons of failure in advance - like non unique email passed during user
creation. So fixed set of exceptions and error codes can be enough. With
other custom IdentityStore it gets more tricky. With LDAP updating entry
can fail for number of reasons specific to server configuration or
schema requirements. It is important to be able to propagate meaningful
reason from IdentityStore implementation back to the API level.
On 11/07/2012 11:54 AM, Shane Bryzak wrote:
Hey guys,
I spent some time reviewing the IdentityManager API today to identify
any redundancies and also locate any holes where we might not properly
support required features, as well as a general "sanity" check to ensure
that what we're exposing via this API makes sense and is intuitive for
consumers. As Anil has pointed out already, our short term priority for
the project is to stabilize the API - this is extremely important as
PicketLink will provide the security foundation for many other
projects. With that in mind I'd like to strongly encourage everyone
with a stake in this to carefully review the API and provide feedback,
as once we release it it will be essentially set in stone.
To try and avoid a wall of text (and make this post easier to reply to),
I'm going to break this post up into sub-sections, one for each feature
group. For each section, I'll include the API as it currently exists,
followed by a brief summary of my thoughts and any recommendations I may
have - this is especially where I want to hear any feedback indicating
whether you agree or disagree with my assessment. Let's start with the
user-related methods:
User management
-------------------------
User createUser(String name);
void createUser(User user);
void removeUser(User user);
void removeUser(String name);
User getUser(String name);
My thoughts:
1. I'm wondering if we should remove the createUser(String) method and
just have createUser(User). This would make sense in a way because I
can't think of many use cases where you might want to create a User with
just a username and not any of the other typical information (such as
first name, last name, e-mail address, etc). Creating a User via
createUser(String) when you actually want to set the other details after
the initial User creation is also horribly inefficient, requiring
multiple round trips to the database (or whatever identity store backend
you use). To add to that, the actual parameter name is a little
unintuitive - "name" could refer to any number of things - is it the
username or the user's actual name that you need to provide here? My +1
goes to removing createUser(String).
2. Similar to point 1), having two removeUser() methods seems equally
redundant. The User object that needs to be provided to the
removeUser(User) method can be easily looked up by calling getUser(), as
per this example:
identityManager.removeUser(identityManager.getUser("jsmith"));
3. As I also pointed out in point 1), the "name" parameter is a little
unintuitive. In regard to the getUser() method I think we should rename
the "name" parameter to "username" just so it's perfectly clear
what the
method expects.
4. We don't currently have any way to update User details. I recommend
that we add an updateUser() method as follows:
void updateUser(User user);
This will allow User details such as their first and last name, e-mail
address etc to be updated without having to delete and re-create the
User. Extending on this a little further, I'm wondering if we should
think about adding an audit API that logs these changes. Perhaps it's
something to think about for a later release.
Group management
--------------------------
Group createGroup(String id);
Group createGroup(String id, Group parent);
Group createGroup(String id, String parent);
void removeGroup(Group group);
void removeGroup(String groupId);
Group getGroup(String groupId);
Group getGroup(String groupId, Group parent);
void addToGroup(IdentityType identityType, Group group);
void removeFromGroup(IdentityType identityType, Group group);
My thoughts:
1. I think there is a little bit of ambiguity here in regard to the
differences between a Group's ID and name. To assist in understanding
the differences, here's the JavaDoc pasted from the Group interface:
/**
* Groups are stored in tree hierarchy and therefore ID represents
a path. ID string always begins with "/" element that
* represents root of the tree
* <p/>
* Example: Valid IDs are "/acme/departments/marketing",
"/security/administrator" or "/administrator". Where
"acme",
* "departments", "marketing", "security" and
"administrator" are
group names.
*
* @return Group Id in String representation.
*/
String getId();
/**
* Group name is unique identifier in specific group tree branch.
For example group with id "/acme/departments/marketing"
* will have name "marketing" and parent group of id
"/acme/departments"
*
* @return name
*/
String getName();
Taking the above JavaDoc into account, it seems that where we are
specifying "id" as a method parameter we should be specifying "name"
instead. If we are indeed providing a "fully qualified" path to the
createGroup() method, then there is no need to also specify the parent
as it can be easily derived from the path. More on this in the
following points.
2. We currently have three createGroup() methods for creating a new
Group. I think we should remove the (String, String) variant, as the
parent parameter is ambiguous (is it the parent's ID or name we need to
specify here?), and rename the "id" parameter of the (String, Group)
variant to "name", leaving us with the following:
Group createGroup(String id);
Group createGroup(String name, Group parent);
This then gives us two ways of creating a group. We can either specify
the fully qualified group ID:
Group employees = identityManager.createGroup("/employees");
Or we can specify a subgroup name and parent Group:
Group managers = identityManager.createGroup("managers", employees);
We can also use this second form to create a "root" group by just
specifying null for the parent:
Group admins = identityManager.createGroup("admins", null);
3. For the removeGroup() methods, I think what we have is fine.
4. For the getGroup() methods, I'm happy with the first one (String
groupId) but I think the second one (String groupId, Group parent) needs
to be (String name, Group parent) instead. The first method would be
used when the fully qualified group ID is known, and the second one
would be used when you already have the parent Group and know the name
of the subgroup.
5. The addToGroup() and removeFromGroup() methods seem fine to me.
6. One thing we don't have is a method to test whether an IdentityType
is a member of a Group. I suggest we add another method to support this:
boolean inGroup(IdentityType identityType, Group group);
Roles
----------
Role createRole(String name);
void removeRole(Role role);
void removeRole(String name);
Role getRole(String name);
boolean hasRole(Role role, IdentityType identityType, Group group);
void grantRole(Role role, IdentityType identityType, Group group);
void revokeRole(Role role, IdentityType identityType, Group group);
My thoughts:
1. I would remove the removeRole(String) method as the same thing can be
achieved with removeRole(getRole(String)).
2. I would swap the order of the Role and IdentityType parameters within
the hasRole(), grantRole() and revokeRole() methods. As the
IdentityType is the primary artifact in these operations it makes sense
to have it listed first.
3. We don't have any explicit support for application roles, which are
roles where there is no Group component (for example, an
application-wide "admin" role). We could support this by just allowing
hasRole(), grantRole() etc to accept null values for the Group
parameter, however I feel it would be more semantically correct to
provide a distinct set of methods for the purpose of supporting
application roles, as follows:
boolean hasApplicationRole(IdentityType identityType, Role role);
void grantApplicationRole(IdentityType identityType, Role role);
void revokeApplicationRole(IdentityType identityType, Role role);
Query API
--------------
<T extends IdentityType> IdentityQuery<T> createQuery();
My thoughts:
1. After our recent rewrite of the Query API I'm satisfied with what we
have now.
Credential management
-------------------------------
boolean validateCredential(User user, Credential credential);
void updateCredential(User user, Credential credential);
My thoughts:
1. I'm satisfied with what we currently have, however we still need to
review the mechanism that we provide for encoding passwords. I'm not
sure that it will have any effect on these methods though (I'm toying
with the idea of integrating the password encoding functionality via the
IdentityStoreInvocationContext).
Identity expiry
------------------
void setEnabled(IdentityType identityType, boolean enabled);
void setExpirationDate(IdentityType identityType, Date expirationDate);
My thoughts:
1. I think these methods are fine the way they are. One discrepancy
that I've identified is that a User can be created already being
disabled or having an expiry date, while for a Role or Group the
equivalent status must be set via these methods after creation. I'm not
sure this is a big deal though.
Attributes
-------------
void setAttribute(IdentityType identityType, String attributeName,
String attributeValue);
String getAttribute(IdentityType identityType, String attributeName);
My thoughts:
1. This area poses a slight dilemma. Currently the API only supports
simple String-based attribute values, however I'm pretty sure that
people are going to want to store all sorts of things, ranging from
boolean and Date values through to large byte arrays that store a User's
photograph or other data. If everyone is in agreement that we need to
support more than just String values, then the first thing we probably
need to do is modify these methods to work with a Serializable instead.
2. If we all agree on point 1), the next thing we need to decide is
whether we want the capability to make some attribute values "lazy
loaded". If we want to do a quick lookup of a User object for the
express purpose of assigning a Role or Group membership (or any other
type of simple operation) then we probably don't want the performance
hit of having to load bulky attribute data.
3. With the above two points in mind, I'm going to hold off on surmising
any further on attributes until some of you guys weigh in with your
opinions. I do have some rough ideas, however I'll wait until we have a
consensus of exactly what we want to achieve here before we proceed further.
Utility methods
--------------------
IdentityType lookupIdentityByKey(String key);
My thoughts:
1. This method is required by other features, such as the Permissions
API. I'm fine with what we have here.
In summary
----------------
I apologise for yet another epic e-mail to the list, however this is
everyone's chance to review the API for themselves and decide whether or
not it will be suitable for addressing all of your requirements. I'd
like to get this API stable by next week so please don't be shy about
speaking up. I'm especially looking forward to hearing your opinions
about how we handle attributes (there may even be some relevant JSR-351
stuff that we should be looking at here).
Thanks,
Shane
_______________________________________________
security-dev mailing list
security-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/security-dev
10:54 a.m.
Bolek,
error reporting is valid requirement but we need to be careful.
We cannot send too much revealing information about the server side
set up via the errors (such as table does not exist).
IMO applications that are meant for IDM administration
may need deeper error information.
Regards,
Anil
On 11/08/2012 10:22 AM, Bolesław Dawidowicz wrote:
One area that is missing from the design is error reporting and
exceptions in general. How should we pass the information about failure
of specific operation and reasons behind it?
One of limitations we have now in GateIn is lack of proper way to pass
info about nature of failure back into UI. In case of JPAIdentityStore
problem is a bit smaller because it is easier to predict possible
reasons of failure in advance - like non unique email passed during user
creation. So fixed set of exceptions and error codes can be enough. With
other custom IdentityStore it gets more tricky. With LDAP updating entry
can fail for number of reasons specific to server configuration or
schema requirements. It is important to be able to propagate meaningful
reason from IdentityStore implementation back to the API level.
On 11/07/2012 11:54 AM, Shane Bryzak wrote:
> Hey guys,
>
> I spent some time reviewing the IdentityManager API today to identify
> any redundancies and also locate any holes where we might not properly
> support required features, as well as a general "sanity" check to ensure
> that what we're exposing via this API makes sense and is intuitive for
> consumers. As Anil has pointed out already, our short term priority for
> the project is to stabilize the API - this is extremely important as
> PicketLink will provide the security foundation for many other
> projects. With that in mind I'd like to strongly encourage everyone
> with a stake in this to carefully review the API and provide feedback,
> as once we release it it will be essentially set in stone.
>
> To try and avoid a wall of text (and make this post easier to reply to),
> I'm going to break this post up into sub-sections, one for each feature
> group. For each section, I'll include the API as it currently exists,
> followed by a brief summary of my thoughts and any recommendations I may
> have - this is especially where I want to hear any feedback indicating
> whether you agree or disagree with my assessment. Let's start with the
> user-related methods:
>
> User management
> -------------------------
>
> User createUser(String name);
>
> void createUser(User user);
>
> void removeUser(User user);
>
> void removeUser(String name);
>
> User getUser(String name);
>
> My thoughts:
>
> 1. I'm wondering if we should remove the createUser(String) method and
> just have createUser(User). This would make sense in a way because I
> can't think of many use cases where you might want to create a User with
> just a username and not any of the other typical information (such as
> first name, last name, e-mail address, etc). Creating a User via
> createUser(String) when you actually want to set the other details after
> the initial User creation is also horribly inefficient, requiring
> multiple round trips to the database (or whatever identity store backend
> you use). To add to that, the actual parameter name is a little
> unintuitive - "name" could refer to any number of things - is it the
> username or the user's actual name that you need to provide here? My +1
> goes to removing createUser(String).
>
> 2. Similar to point 1), having two removeUser() methods seems equally
> redundant. The User object that needs to be provided to the
> removeUser(User) method can be easily looked up by calling getUser(), as
> per this example:
>
> identityManager.removeUser(identityManager.getUser("jsmith"));
>
> 3. As I also pointed out in point 1), the "name" parameter is a little
> unintuitive. In regard to the getUser() method I think we should rename
> the "name" parameter to "username" just so it's perfectly
clear what the
> method expects.
>
> 4. We don't currently have any way to update User details. I recommend
> that we add an updateUser() method as follows:
>
> void updateUser(User user);
>
> This will allow User details such as their first and last name, e-mail
> address etc to be updated without having to delete and re-create the
> User. Extending on this a little further, I'm wondering if we should
> think about adding an audit API that logs these changes. Perhaps it's
> something to think about for a later release.
>
> Group management
> --------------------------
>
> Group createGroup(String id);
>
> Group createGroup(String id, Group parent);
>
> Group createGroup(String id, String parent);
>
> void removeGroup(Group group);
>
> void removeGroup(String groupId);
>
> Group getGroup(String groupId);
>
> Group getGroup(String groupId, Group parent);
>
> void addToGroup(IdentityType identityType, Group group);
>
> void removeFromGroup(IdentityType identityType, Group group);
>
> My thoughts:
>
> 1. I think there is a little bit of ambiguity here in regard to the
> differences between a Group's ID and name. To assist in understanding
> the differences, here's the JavaDoc pasted from the Group interface:
>
> /**
> * Groups are stored in tree hierarchy and therefore ID represents
> a path. ID string always begins with "/" element that
> * represents root of the tree
> * <p/>
> * Example: Valid IDs are "/acme/departments/marketing",
> "/security/administrator" or "/administrator". Where
"acme",
> * "departments", "marketing", "security" and
"administrator" are
> group names.
> *
> * @return Group Id in String representation.
> */
> String getId();
>
> /**
> * Group name is unique identifier in specific group tree branch.
> For example group with id "/acme/departments/marketing"
> * will have name "marketing" and parent group of id
> "/acme/departments"
> *
> * @return name
> */
> String getName();
>
> Taking the above JavaDoc into account, it seems that where we are
> specifying "id" as a method parameter we should be specifying
"name"
> instead. If we are indeed providing a "fully qualified" path to the
> createGroup() method, then there is no need to also specify the parent
> as it can be easily derived from the path. More on this in the
> following points.
>
> 2. We currently have three createGroup() methods for creating a new
> Group. I think we should remove the (String, String) variant, as the
> parent parameter is ambiguous (is it the parent's ID or name we need to
> specify here?), and rename the "id" parameter of the (String, Group)
> variant to "name", leaving us with the following:
>
> Group createGroup(String id);
>
> Group createGroup(String name, Group parent);
>
> This then gives us two ways of creating a group. We can either specify
> the fully qualified group ID:
>
> Group employees = identityManager.createGroup("/employees");
>
> Or we can specify a subgroup name and parent Group:
>
> Group managers = identityManager.createGroup("managers", employees);
>
> We can also use this second form to create a "root" group by just
> specifying null for the parent:
>
> Group admins = identityManager.createGroup("admins", null);
>
> 3. For the removeGroup() methods, I think what we have is fine.
>
> 4. For the getGroup() methods, I'm happy with the first one (String
> groupId) but I think the second one (String groupId, Group parent) needs
> to be (String name, Group parent) instead. The first method would be
> used when the fully qualified group ID is known, and the second one
> would be used when you already have the parent Group and know the name
> of the subgroup.
>
> 5. The addToGroup() and removeFromGroup() methods seem fine to me.
>
> 6. One thing we don't have is a method to test whether an IdentityType
> is a member of a Group. I suggest we add another method to support this:
>
> boolean inGroup(IdentityType identityType, Group group);
>
> Roles
> ----------
>
> Role createRole(String name);
>
> void removeRole(Role role);
>
> void removeRole(String name);
>
> Role getRole(String name);
>
> boolean hasRole(Role role, IdentityType identityType, Group group);
>
> void grantRole(Role role, IdentityType identityType, Group group);
>
> void revokeRole(Role role, IdentityType identityType, Group group);
>
> My thoughts:
>
> 1. I would remove the removeRole(String) method as the same thing can be
> achieved with removeRole(getRole(String)).
>
> 2. I would swap the order of the Role and IdentityType parameters within
> the hasRole(), grantRole() and revokeRole() methods. As the
> IdentityType is the primary artifact in these operations it makes sense
> to have it listed first.
>
> 3. We don't have any explicit support for application roles, which are
> roles where there is no Group component (for example, an
> application-wide "admin" role). We could support this by just allowing
> hasRole(), grantRole() etc to accept null values for the Group
> parameter, however I feel it would be more semantically correct to
> provide a distinct set of methods for the purpose of supporting
> application roles, as follows:
>
> boolean hasApplicationRole(IdentityType identityType, Role role);
>
> void grantApplicationRole(IdentityType identityType, Role role);
>
> void revokeApplicationRole(IdentityType identityType, Role role);
>
>
> Query API
> --------------
>
> <T extends IdentityType> IdentityQuery<T> createQuery();
>
> My thoughts:
>
> 1. After our recent rewrite of the Query API I'm satisfied with what we
> have now.
>
> Credential management
> -------------------------------
>
> boolean validateCredential(User user, Credential credential);
>
> void updateCredential(User user, Credential credential);
>
> My thoughts:
>
> 1. I'm satisfied with what we currently have, however we still need to
> review the mechanism that we provide for encoding passwords. I'm not
> sure that it will have any effect on these methods though (I'm toying
> with the idea of integrating the password encoding functionality via the
> IdentityStoreInvocationContext).
>
> Identity expiry
> ------------------
>
> void setEnabled(IdentityType identityType, boolean enabled);
>
> void setExpirationDate(IdentityType identityType, Date expirationDate);
>
> My thoughts:
>
> 1. I think these methods are fine the way they are. One discrepancy
> that I've identified is that a User can be created already being
> disabled or having an expiry date, while for a Role or Group the
> equivalent status must be set via these methods after creation. I'm not
> sure this is a big deal though.
>
> Attributes
> -------------
>
> void setAttribute(IdentityType identityType, String attributeName,
> String attributeValue);
>
> String getAttribute(IdentityType identityType, String attributeName);
>
> My thoughts:
>
> 1. This area poses a slight dilemma. Currently the API only supports
> simple String-based attribute values, however I'm pretty sure that
> people are going to want to store all sorts of things, ranging from
> boolean and Date values through to large byte arrays that store a User's
> photograph or other data. If everyone is in agreement that we need to
> support more than just String values, then the first thing we probably
> need to do is modify these methods to work with a Serializable instead.
>
> 2. If we all agree on point 1), the next thing we need to decide is
> whether we want the capability to make some attribute values "lazy
> loaded". If we want to do a quick lookup of a User object for the
> express purpose of assigning a Role or Group membership (or any other
> type of simple operation) then we probably don't want the performance
> hit of having to load bulky attribute data.
>
> 3. With the above two points in mind, I'm going to hold off on surmising
> any further on attributes until some of you guys weigh in with your
> opinions. I do have some rough ideas, however I'll wait until we have a
> consensus of exactly what we want to achieve here before we proceed further.
>
> Utility methods
> --------------------
>
> IdentityType lookupIdentityByKey(String key);
>
> My thoughts:
>
> 1. This method is required by other features, such as the Permissions
> API. I'm fine with what we have here.
>
> In summary
> ----------------
>
> I apologise for yet another epic e-mail to the list, however this is
> everyone's chance to review the API for themselves and decide whether or
> not it will be suitable for addressing all of your requirements. I'd
> like to get this API stable by next week so please don't be shy about
> speaking up. I'm especially looking forward to hearing your opinions
> about how we handle attributes (there may even be some relevant JSR-351
> stuff that we should be looking at here).
>
> Thanks,
> Shane
11:55 a.m.
I fully agree. Default set of errors need to be well thought and
narrowed down. It still needs to be possible for specific SPI
implementation to propagate additional info to the API layer. Even
simple string code can be enough as this will be responsibility of
someone who implemented custom IdentityStore to decode it in the
application.
On 11/08/2012 05:54 PM, Anil Saldhana wrote:
Bolek,
error reporting is valid requirement but we need to be careful.
We cannot send too much revealing information about the server side
set up via the errors (such as table does not exist).
IMO applications that are meant for IDM administration
may need deeper error information.
Regards,
Anil
On 11/08/2012 10:22 AM, Bolesław Dawidowicz wrote:
> One area that is missing from the design is error reporting and
> exceptions in general. How should we pass the information about failure
> of specific operation and reasons behind it?
>
> One of limitations we have now in GateIn is lack of proper way to pass
> info about nature of failure back into UI. In case of JPAIdentityStore
> problem is a bit smaller because it is easier to predict possible
> reasons of failure in advance - like non unique email passed during user
> creation. So fixed set of exceptions and error codes can be enough. With
> other custom IdentityStore it gets more tricky. With LDAP updating entry
> can fail for number of reasons specific to server configuration or
> schema requirements. It is important to be able to propagate meaningful
> reason from IdentityStore implementation back to the API level.
>
> On 11/07/2012 11:54 AM, Shane Bryzak wrote:
>> Hey guys,
>>
>> I spent some time reviewing the IdentityManager API today to identify
>> any redundancies and also locate any holes where we might not properly
>> support required features, as well as a general "sanity" check to
ensure
>> that what we're exposing via this API makes sense and is intuitive for
>> consumers. As Anil has pointed out already, our short term priority for
>> the project is to stabilize the API - this is extremely important as
>> PicketLink will provide the security foundation for many other
>> projects. With that in mind I'd like to strongly encourage everyone
>> with a stake in this to carefully review the API and provide feedback,
>> as once we release it it will be essentially set in stone.
>>
>> To try and avoid a wall of text (and make this post easier to reply to),
>> I'm going to break this post up into sub-sections, one for each feature
>> group. For each section, I'll include the API as it currently exists,
>> followed by a brief summary of my thoughts and any recommendations I may
>> have - this is especially where I want to hear any feedback indicating
>> whether you agree or disagree with my assessment. Let's start with the
>> user-related methods:
>>
>> User management
>> -------------------------
>>
>> User createUser(String name);
>>
>> void createUser(User user);
>>
>> void removeUser(User user);
>>
>> void removeUser(String name);
>>
>> User getUser(String name);
>>
>> My thoughts:
>>
>> 1. I'm wondering if we should remove the createUser(String) method and
>> just have createUser(User). This would make sense in a way because I
>> can't think of many use cases where you might want to create a User with
>> just a username and not any of the other typical information (such as
>> first name, last name, e-mail address, etc). Creating a User via
>> createUser(String) when you actually want to set the other details after
>> the initial User creation is also horribly inefficient, requiring
>> multiple round trips to the database (or whatever identity store backend
>> you use). To add to that, the actual parameter name is a little
>> unintuitive - "name" could refer to any number of things - is it the
>> username or the user's actual name that you need to provide here? My +1
>> goes to removing createUser(String).
>>
>> 2. Similar to point 1), having two removeUser() methods seems equally
>> redundant. The User object that needs to be provided to the
>> removeUser(User) method can be easily looked up by calling getUser(), as
>> per this example:
>>
>> identityManager.removeUser(identityManager.getUser("jsmith"));
>>
>> 3. As I also pointed out in point 1), the "name" parameter is a little
>> unintuitive. In regard to the getUser() method I think we should rename
>> the "name" parameter to "username" just so it's perfectly
clear what the
>> method expects.
>>
>> 4. We don't currently have any way to update User details. I recommend
>> that we add an updateUser() method as follows:
>>
>> void updateUser(User user);
>>
>> This will allow User details such as their first and last name, e-mail
>> address etc to be updated without having to delete and re-create the
>> User. Extending on this a little further, I'm wondering if we should
>> think about adding an audit API that logs these changes. Perhaps it's
>> something to think about for a later release.
>>
>> Group management
>> --------------------------
>>
>> Group createGroup(String id);
>>
>> Group createGroup(String id, Group parent);
>>
>> Group createGroup(String id, String parent);
>>
>> void removeGroup(Group group);
>>
>> void removeGroup(String groupId);
>>
>> Group getGroup(String groupId);
>>
>> Group getGroup(String groupId, Group parent);
>>
>> void addToGroup(IdentityType identityType, Group group);
>>
>> void removeFromGroup(IdentityType identityType, Group group);
>>
>> My thoughts:
>>
>> 1. I think there is a little bit of ambiguity here in regard to the
>> differences between a Group's ID and name. To assist in understanding
>> the differences, here's the JavaDoc pasted from the Group interface:
>>
>> /**
>> * Groups are stored in tree hierarchy and therefore ID represents
>> a path. ID string always begins with "/" element that
>> * represents root of the tree
>> * <p/>
>> * Example: Valid IDs are "/acme/departments/marketing",
>> "/security/administrator" or "/administrator". Where
"acme",
>> * "departments", "marketing", "security"
and "administrator" are
>> group names.
>> *
>> * @return Group Id in String representation.
>> */
>> String getId();
>>
>> /**
>> * Group name is unique identifier in specific group tree branch.
>> For example group with id "/acme/departments/marketing"
>> * will have name "marketing" and parent group of id
>> "/acme/departments"
>> *
>> * @return name
>> */
>> String getName();
>>
>> Taking the above JavaDoc into account, it seems that where we are
>> specifying "id" as a method parameter we should be specifying
"name"
>> instead. If we are indeed providing a "fully qualified" path to the
>> createGroup() method, then there is no need to also specify the parent
>> as it can be easily derived from the path. More on this in the
>> following points.
>>
>> 2. We currently have three createGroup() methods for creating a new
>> Group. I think we should remove the (String, String) variant, as the
>> parent parameter is ambiguous (is it the parent's ID or name we need to
>> specify here?), and rename the "id" parameter of the (String, Group)
>> variant to "name", leaving us with the following:
>>
>> Group createGroup(String id);
>>
>> Group createGroup(String name, Group parent);
>>
>> This then gives us two ways of creating a group. We can either specify
>> the fully qualified group ID:
>>
>> Group employees = identityManager.createGroup("/employees");
>>
>> Or we can specify a subgroup name and parent Group:
>>
>> Group managers = identityManager.createGroup("managers", employees);
>>
>> We can also use this second form to create a "root" group by just
>> specifying null for the parent:
>>
>> Group admins = identityManager.createGroup("admins", null);
>>
>> 3. For the removeGroup() methods, I think what we have is fine.
>>
>> 4. For the getGroup() methods, I'm happy with the first one (String
>> groupId) but I think the second one (String groupId, Group parent) needs
>> to be (String name, Group parent) instead. The first method would be
>> used when the fully qualified group ID is known, and the second one
>> would be used when you already have the parent Group and know the name
>> of the subgroup.
>>
>> 5. The addToGroup() and removeFromGroup() methods seem fine to me.
>>
>> 6. One thing we don't have is a method to test whether an IdentityType
>> is a member of a Group. I suggest we add another method to support this:
>>
>> boolean inGroup(IdentityType identityType, Group group);
>>
>> Roles
>> ----------
>>
>> Role createRole(String name);
>>
>> void removeRole(Role role);
>>
>> void removeRole(String name);
>>
>> Role getRole(String name);
>>
>> boolean hasRole(Role role, IdentityType identityType, Group group);
>>
>> void grantRole(Role role, IdentityType identityType, Group group);
>>
>> void revokeRole(Role role, IdentityType identityType, Group group);
>>
>> My thoughts:
>>
>> 1. I would remove the removeRole(String) method as the same thing can be
>> achieved with removeRole(getRole(String)).
>>
>> 2. I would swap the order of the Role and IdentityType parameters within
>> the hasRole(), grantRole() and revokeRole() methods. As the
>> IdentityType is the primary artifact in these operations it makes sense
>> to have it listed first.
>>
>> 3. We don't have any explicit support for application roles, which are
>> roles where there is no Group component (for example, an
>> application-wide "admin" role). We could support this by just
allowing
>> hasRole(), grantRole() etc to accept null values for the Group
>> parameter, however I feel it would be more semantically correct to
>> provide a distinct set of methods for the purpose of supporting
>> application roles, as follows:
>>
>> boolean hasApplicationRole(IdentityType identityType, Role role);
>>
>> void grantApplicationRole(IdentityType identityType, Role role);
>>
>> void revokeApplicationRole(IdentityType identityType, Role role);
>>
>>
>> Query API
>> --------------
>>
>> <T extends IdentityType> IdentityQuery<T> createQuery();
>>
>> My thoughts:
>>
>> 1. After our recent rewrite of the Query API I'm satisfied with what we
>> have now.
>>
>> Credential management
>> -------------------------------
>>
>> boolean validateCredential(User user, Credential credential);
>>
>> void updateCredential(User user, Credential credential);
>>
>> My thoughts:
>>
>> 1. I'm satisfied with what we currently have, however we still need to
>> review the mechanism that we provide for encoding passwords. I'm not
>> sure that it will have any effect on these methods though (I'm toying
>> with the idea of integrating the password encoding functionality via the
>> IdentityStoreInvocationContext).
>>
>> Identity expiry
>> ------------------
>>
>> void setEnabled(IdentityType identityType, boolean enabled);
>>
>> void setExpirationDate(IdentityType identityType, Date expirationDate);
>>
>> My thoughts:
>>
>> 1. I think these methods are fine the way they are. One discrepancy
>> that I've identified is that a User can be created already being
>> disabled or having an expiry date, while for a Role or Group the
>> equivalent status must be set via these methods after creation. I'm not
>> sure this is a big deal though.
>>
>> Attributes
>> -------------
>>
>> void setAttribute(IdentityType identityType, String attributeName,
>> String attributeValue);
>>
>> String getAttribute(IdentityType identityType, String attributeName);
>>
>> My thoughts:
>>
>> 1. This area poses a slight dilemma. Currently the API only supports
>> simple String-based attribute values, however I'm pretty sure that
>> people are going to want to store all sorts of things, ranging from
>> boolean and Date values through to large byte arrays that store a User's
>> photograph or other data. If everyone is in agreement that we need to
>> support more than just String values, then the first thing we probably
>> need to do is modify these methods to work with a Serializable instead.
>>
>> 2. If we all agree on point 1), the next thing we need to decide is
>> whether we want the capability to make some attribute values "lazy
>> loaded". If we want to do a quick lookup of a User object for the
>> express purpose of assigning a Role or Group membership (or any other
>> type of simple operation) then we probably don't want the performance
>> hit of having to load bulky attribute data.
>>
>> 3. With the above two points in mind, I'm going to hold off on surmising
>> any further on attributes until some of you guys weigh in with your
>> opinions. I do have some rough ideas, however I'll wait until we have a
>> consensus of exactly what we want to achieve here before we proceed further.
>>
>> Utility methods
>> --------------------
>>
>> IdentityType lookupIdentityByKey(String key);
>>
>> My thoughts:
>>
>> 1. This method is required by other features, such as the Permissions
>> API. I'm fine with what we have here.
>>
>> In summary
>> ----------------
>>
>> I apologise for yet another epic e-mail to the list, however this is
>> everyone's chance to review the API for themselves and decide whether or
>> not it will be suitable for addressing all of your requirements. I'd
>> like to get this API stable by next week so please don't be shy about
>> speaking up. I'm especially looking forward to hearing your opinions
>> about how we handle attributes (there may even be some relevant JSR-351
>> stuff that we should be looking at here).
>>
>> Thanks,
>> Shane
_______________________________________________
security-dev mailing list
security-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/security-dev
4404
days inactive
4410
days old
21 comments
6 participants
participants (6)
-
Anil Saldhana -
Boleslaw Dawidowicz -
Bolesław Dawidowicz
-
Jason Porter -
Rodney Russ -
Shane Bryzak