Security IDM API feedback

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Security IDM API feedback

Jason Porter
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
Reply | Threaded
Open this post in threaded view
|

Re: Security IDM API feedback

Romain Manni-Bucau
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 <[hidden email]>

> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: Security IDM API feedback

Shane Bryzak-2
In reply to this post by Jason Porter
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.
>
>


Reply | Threaded
Open this post in threaded view
|

Re: Security IDM API feedback

Jason Porter
On Jul 25, 2012, at 16:38, Shane Bryzak <[hidden email]> 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.

>>
>>
>
>