[security-dev] IdentityManager API review

[Boleslaw Dawidowicz]

On Nov 13, 2012, at 2:12 AM, Shane Bryzak <sbryzak at redhat.com> wrote:

> On 11/12/2012 09:13 PM, Boleslaw Dawidowicz wrote:
>> On Nov 12, 2012, at 4:01 AM, Shane Bryzak <sbryzak at 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.