Security IDM API feedback

[Jason Porter <li...@gmail.com>]

I'll do a separate email for impl review / feedback

== Security Feedback ==

=== API ===

* SecurityConfigurationException has no args constructor and also one with
just a throwable, should we remove these? Same with SecurityException.

* I'm very tempted to say drop the java.util.Date and have a dependency on
joda-time. I'me sure we've all been through issues with j.u.Date.

==== Events ====
Should every event contain a user? Why do some of them have a user, but
others do not?That doesn't leave a very consistent programming model.

==== Authorization ====
* AccessDecisionVoterContext The javadoc for getSource() leaves of in
return. "the source which triggered the" -- triggered what?

* AccessDeniedException extends java.lang.SecurityException is this correct?

* We could proabably remove the constructor with just a throwable from
SecurityDefinitionException

==== Credential ====

Wouldn't LoginCredentials just be a pair of Credentials? Seems like that
would better allow flexibility.

==== IDM ====
* AbstractIdentityType returns an unmodifiable map for getAttributes, but
getAttributeValues does not return a clone of the array. If we're striving
for immutability for this class we should be doing both.

* Would it make sense to add a method sort(Comparator)?

* Not sure about the password management stuff in IdentityManager. We have
credential as a class already, wouldn't it be better to reuse that stuff
instead of tying to a string password?

* Membership, shouldn't membership return a collection of groups and roles?
This would at least allow for an empty collection instead of returning null
or a NullGroup / NullRole or similar

* I left some feedback on some of this before. Are those ideas still under
discussion or were they dropped?

* I would really think ints should be fine as a range, but do we want to
allow larger numbers just in case?


-- 
Jason Porter
http://lightguard-jp.blogspot.com
http://twitter.com/lightguardjp

Software Engineer
Open Source Advocate
Author of Seam Catch - Next Generation Java Exception Handling

PGP key id: 926CCFF5
PGP key available at: keyserver.net, pgp.mit.edu

Re: Security IDM API feedback

[Jason Porter <li...@gmail.com>]

On Jul 25, 2012, at 16:38, Shane Bryzak <sb...@redhat.com> wrote:

> On 26/07/12 06:29, Jason Porter wrote:
>> I'll do a separate email for impl review / feedback
>> 
>> == Security Feedback ==
>> 
>> === API ===
>> 
>> * SecurityConfigurationException has no args constructor and also one with
>> just a throwable, should we remove these? Same with SecurityException.
> 
> Yep, you're probably right.
>> 
>> * I'm very tempted to say drop the java.util.Date and have a dependency on
>> joda-time. I'me sure we've all been through issues with j.u.Date.
> 
> Where are you seeing this dependency?

There is no dependency. I'm suggesting adding one, but since I've thought about this more, users using jodatime can do the interop themselves. 

>> 
>> ==== Events ====
>> Should every event contain a user? Why do some of them have a user, but
>> others do not?That doesn't leave a very consistent programming model.
> 
> For the ones that don't pass the User, it can be obtained by injecting Identity.  The ones that do pass a user are related to logging out - arguably we don't need it for the PreLoggedOutEvent but it is definitely needed for PostLoggedOutEvent when the User will no longer be available from Identity.
>> 
>> ==== Authorization ====
>> * AccessDecisionVoterContext The javadoc for getSource() leaves of in
>> return. "the source which triggered the" -- triggered what?
>> 
>> * AccessDeniedException extends java.lang.SecurityException is this correct?
> 
> I'll let Gerhard respond to these two.
>> 
>> * We could proabably remove the constructor with just a throwable from
>> SecurityDefinitionException
> 
> Agreed.
>> 
>> ==== Credential ====
>> 
>> Wouldn't LoginCredentials just be a pair of Credentials? Seems like that
>> would better allow flexibility.
> 
> No, a User ID is not a credential.
>> 
>> ==== IDM ====
>> * AbstractIdentityType returns an unmodifiable map for getAttributes, but
>> getAttributeValues does not return a clone of the array. If we're striving
>> for immutability for this class we should be doing both.
> 
> Good point.
>> 
>> * Would it make sense to add a method sort(Comparator)?
> You mean for the attributes?  I'm sure if a developer wanted to sort them they could do it in their own code.

Okay that's fine. 

>> 
>> * Not sure about the password management stuff in IdentityManager. We have
>> credential as a class already, wouldn't it be better to reuse that stuff
>> instead of tying to a string password?
> For situations where you are not using a plain text password you will be very unlikely to be using Identity Management, at least to manage those alternative form of credentials.  These methods use a String for the user's convenience - for applications that use password-based authentication with IDM the passwords (or their hashes) will be stored in either a database or LDAP directory.

Alright. Users may have different feelings when they start doing more advanced stuff. If they do we'll just address it then. 

Having a common API, or at least similar when get into advanced cases I think is important and helps ease growing pains of an application over time. 

>> 
>> * Membership, shouldn't membership return a collection of groups and roles?
>> This would at least allow for an empty collection instead of returning null
>> or a NullGroup / NullRole or similar
> 
> Not quite sure what you're asking here - a Membership is a representation of a user's role within a group.  None of the aspects are optional (they cannot be null).

Maybe I missed that in the javadocs or it should be added. 

What if a user has no group (maybe the app doesn't use groups) or roles?

>> 
>> * I left some feedback on some of this before. Are those ideas still under
>> discussion or were they dropped?
> I still need to review, but it's on my to-do list ;)
>> 
>> * I would really think ints should be fine as a range, but do we want to
>> allow larger numbers just in case?
> I'm assuming you're talking about Range here?  The javax.persistence.Query interface only supports int for setFirstResult() and setMaxResults() anyway.

Okay, that's fine then. 

>> 
>> 
> 
> 

Re: Security IDM API feedback

[Shane Bryzak <sb...@redhat.com>]

On 26/07/12 06:29, Jason Porter wrote:
> I'll do a separate email for impl review / feedback
>
> == Security Feedback ==
>
> === API ===
>
> * SecurityConfigurationException has no args constructor and also one with
> just a throwable, should we remove these? Same with SecurityException.

Yep, you're probably right.
>
> * I'm very tempted to say drop the java.util.Date and have a dependency on
> joda-time. I'me sure we've all been through issues with j.u.Date.

Where are you seeing this dependency?
>
> ==== Events ====
> Should every event contain a user? Why do some of them have a user, but
> others do not?That doesn't leave a very consistent programming model.

For the ones that don't pass the User, it can be obtained by injecting 
Identity.  The ones that do pass a user are related to logging out - 
arguably we don't need it for the PreLoggedOutEvent but it is definitely 
needed for PostLoggedOutEvent when the User will no longer be available 
from Identity.
>
> ==== Authorization ====
> * AccessDecisionVoterContext The javadoc for getSource() leaves of in
> return. "the source which triggered the" -- triggered what?
>
> * AccessDeniedException extends java.lang.SecurityException is this correct?

I'll let Gerhard respond to these two.
>
> * We could proabably remove the constructor with just a throwable from
> SecurityDefinitionException

Agreed.
>
> ==== Credential ====
>
> Wouldn't LoginCredentials just be a pair of Credentials? Seems like that
> would better allow flexibility.

No, a User ID is not a credential.
>
> ==== IDM ====
> * AbstractIdentityType returns an unmodifiable map for getAttributes, but
> getAttributeValues does not return a clone of the array. If we're striving
> for immutability for this class we should be doing both.

Good point.
>
> * Would it make sense to add a method sort(Comparator)?
You mean for the attributes?  I'm sure if a developer wanted to sort 
them they could do it in their own code.
>
> * Not sure about the password management stuff in IdentityManager. We have
> credential as a class already, wouldn't it be better to reuse that stuff
> instead of tying to a string password?
For situations where you are not using a plain text password you will be 
very unlikely to be using Identity Management, at least to manage those 
alternative form of credentials.  These methods use a String for the 
user's convenience - for applications that use password-based 
authentication with IDM the passwords (or their hashes) will be stored 
in either a database or LDAP directory.
>
> * Membership, shouldn't membership return a collection of groups and roles?
> This would at least allow for an empty collection instead of returning null
> or a NullGroup / NullRole or similar

Not quite sure what you're asking here - a Membership is a 
representation of a user's role within a group.  None of the aspects are 
optional (they cannot be null).
>
> * I left some feedback on some of this before. Are those ideas still under
> discussion or were they dropped?
I still need to review, but it's on my to-do list ;)
>
> * I would really think ints should be fine as a range, but do we want to
> allow larger numbers just in case?
I'm assuming you're talking about Range here?  The 
javax.persistence.Query interface only supports int for setFirstResult() 
and setMaxResults() anyway.
>
>

Re: Security IDM API feedback

[Romain Manni-Bucau <rm...@gmail.com>]

my thoughts inline

side note: shouldn't we ping cxf for their fediz integration when the first
dev will be done (just sthg to note somewhere IMO)

- Romain


2012/7/25 Jason Porter <li...@gmail.com>

> I'll do a separate email for impl review / feedback
>
> == Security Feedback ==
>
> === API ===
>
> * SecurityConfigurationException has no args constructor and also one with
> just a throwable, should we remove these? Same with SecurityException.
>
> * I'm very tempted to say drop the java.util.Date and have a dependency on
> joda-time. I'me sure we've all been through issues with j.u.Date.
>

RMB: i'd prefer to keep j.u.D, IMO if we find issues we'll fix it


>
> ==== Events ====
> Should every event contain a user? Why do some of them have a user, but
> others do not?That doesn't leave a very consistent programming model.
>
>
RMB: couldn't we @Inject User user;? that's way event doesn't need it


> ==== Authorization ====
> * AccessDecisionVoterContext The javadoc for getSource() leaves of in
> return. "the source which triggered the" -- triggered what?
>
> * AccessDeniedException extends java.lang.SecurityException is this
> correct?
>
>
RMB: IMO we should have our own hierarchy -> good catch


> * We could proabably remove the constructor with just a throwable from
> SecurityDefinitionException
>
> ==== Credential ====
>
> Wouldn't LoginCredentials just be a pair of Credentials? Seems like that
> would better allow flexibility.
>
>
RMB: the login is not a credential (well, you'll find as much as definition
of cred and princip as users on the net but) my thought is login =
principal and pwd = credential


> ==== IDM ====
> * AbstractIdentityType returns an unmodifiable map for getAttributes, but
> getAttributeValues does not return a clone of the array. If we're striving
> for immutability for this class we should be doing both.
>
> * Would it make sense to add a method sort(Comparator)?
>
> * Not sure about the password management stuff in IdentityManager. We have
> credential as a class already, wouldn't it be better to reuse that stuff
> instead of tying to a string password?
>
> * Membership, shouldn't membership return a collection of groups and roles?
> This would at least allow for an empty collection instead of returning null
> or a NullGroup / NullRole or similar
>
> * I left some feedback on some of this before. Are those ideas still under
> discussion or were they dropped?
>
> * I would really think ints should be fine as a range, but do we want to
> allow larger numbers just in case?
>
>
> --
> Jason Porter
> http://lightguard-jp.blogspot.com
> http://twitter.com/lightguardjp
>
> Software Engineer
> Open Source Advocate
> Author of Seam Catch - Next Generation Java Exception Handling
>
> PGP key id: 926CCFF5
> PGP key available at: keyserver.net, pgp.mit.edu
>