deltaspike-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shane Bryzak <sbry...@redhat.com>
Subject Re: Security IDM API feedback
Date Wed, 25 Jul 2012 22:38:07 GMT
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.
>
>



Mime
View raw message