deltaspike-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marek Posolda <mposo...@redhat.com>
Subject Re: Early security IDM prototype feedback
Date Fri, 13 Jul 2012 10:02:23 GMT
Hi all,

some late feedback for IDM API from me.


1. On various API objects like Group, Role, GroupQuery etc. there are 
many dual methods with signatures similar to:

     GroupQuery setRelatedUser(User user);

     GroupQuery setRelatedUser(String user);

For the former one, it may be bettter to use signature like:

GroupQuery setRelatedUser(String userId);

to emphasize that expected value is Id of user. WDYT?



2. Similar case is for Role. Instead of:

     GroupQuery setRole(Role role);

     GroupQuery setRole(String role);

we can have for the second method signature like:

     GroupQuery setRole(String roleName);


3. On interface Group, I am not sure what's the purpose of methods:

     Collection<User> getUsers(User user);

     Collection<User> getUsers(String user);

Is it a mistake or am I missing something here? Instead of those, I 
think it could make sense to have method for returning all users of 
particular group with signature like:

Collection<User> getUsers();

Probably this method is not so usable because of losing information 
about role, but I think it makes sense for some usecases.


4. On Group interface, instead of method:

Map<Role, Set<User>> getMembershipsMap();

it may be good to have possibility to obtain reverse map as well. So 
having two methods instead like:

     Map<Role, Set<User>> getMembershipsMapByRole();

     Map<User, Set<Role>> getMembershipsMapByUser();

Same applies for interface User (and there is already TODO on User 
interface for this).


5. Method: IdentityStore.getGroup(String name); should be likely 
IdentityStore.getGroup(String id) I guess?



6. For Query objects, I am not sure why we have methods like:

List<User> executeQuery(UserQuery query);

and not simply:

List<User> executeQuery();

And usage of the API can simply looks like:

Collection<User> allJohnDoes = 
identityManager.createQuery(User.class).setFirstName("John").setLastName("Doe").executeQuery();



7. I like the approach proposed by Jason here 
https://github.com/LightGuard/incubator-deltaspike/commit/d5fc02d0493c2cfe6c46cd4ffc6c786968372d52

with the base Query superclass. But I think on the Query object should 
be method:

Collection<T> executeQuery();

instead of:

Collection<T> getResults(Query<T> query);

Also not sure if method "getSingleResult" makes sense here for the Query 
object.


Also if we use this approach, the method for creating query on 
IdentityManager should be like:
+ <T extends IdmObject> Query<T> createQuery(Class<T> objectType);
- <T extends IdmObject> Query<T> createQuery();


I agree with Bolek for the IdmObject part. Adding IdmObject methods to 
IdentityManager will reduce total number of needed methods on 
IdentityManager, but it adds some unecessary complexity. I think that 
adding too much generics and abstractness to the API could make it 
harder to understand and use for normal users.

Marek


Dne 9.7.2012 09:32, Boleslaw Dawidowicz napsal(a):
> Sorry for late reply - didn't have much time to look last week. Few comments:
>
> 1) I like the approach you took with Query and would like to improve it in similar direction.
>
> 2) I'm not convinced that everything should extend IdmObject. The only benefit is to
reduce number of methods however in some cases it can constrain design.
>
> 3) Methods like:
>
> <T extends IdmObject>  T get(Class<T>  objectType, String objectIdentifier);
> <T extends IdmObject>  void remove(T objectInstance);
> <T extends IdmObject>  T createBasic(Class<T>  idmType, String identifier);
>
> are I think good and suitable for SPI like IdentityStore. However I don't think they
are very user friendly for API that users will consume - like IdentityManger.
>
> Bolek
>
> On Jul 2, 2012, at 6:53 PM, Jason Porter wrote:
>
>> Due to Apache's blanket attachment scrub policy, you can find the same info
>> at my github fork:
>> https://github.com/LightGuard/incubator-deltaspike/commit/d5fc02d0493c2cfe6c46cd4ffc6c786968372d52
>>
>> On Mon, Jul 2, 2012 at 12:20 PM, Jason Porter<lightguard.jp@gmail.com>wrote:
>>
>>> Attached is my diff from what I had pulled down as of Wednesday or so.
>>> Feel free to discuss, dissect, etc
>>>
>>>
>>> On Sun, Jul 1, 2012 at 12:49 PM, Jason Porter<lightguard.jp@gmail.com>wrote:
>>>
>>>> I have some stuff I did on the plane from Summit. I'm going to go back
>>>> over it tomorrow and see if I included everything I wanted. I'll send the
>>>> diff to the list.
>>>>
>>>> Sent from my iPhone
>>>>
>>>> On Jul 1, 2012, at 9:43, Boleslaw Dawidowicz<
>>>> boleslaw.dawidowicz@gmail.com>  wrote:
>>>>
>>>>> I think it all comes down (again) to use cases [0] which we think this
>>>> API should address. Current ones are mainly around typical web application
>>>> development and proposed API prototype was focusing on easy usage. My main
>>>> fear is to go end up with too abstract and generic design that won't be
>>>> really appealing to most of developers. If you look at use cases addressed
>>>> by JSR 351 [1] it is all about dealing with attributes and scenarios closer
>>>> to healthcare - and this so far results in quite different API design. API
>>>> which I believe will be less appealing to typical web developer who doesn't
>>>> care about such use cases.
>>>>> More inline below
>>>>>
>>>>> [0]
>>>> https://cwiki.apache.org/confluence/display/DeltaSpike/Security+Module+Drafts
>>>>> [1] http://java.net/projects/identity-api-spec/pages/UseCases
>>>>>
>>>>> On Jun 29, 2012, at 4:56 PM, Jason Porter wrote:
>>>>>
>>>>>> == Security IDM prototype feedback ==
>>>>>>
>>>>>> * Fold PermissionQuery into PermissionManager to give the developers
a
>>>>>> similar model as JPA.
>>>>>>
>>>>>> * Because a Group is an IdentityType, I wonder if it would be better
to
>>>>>> just have one set of methods and pass in the IdentityType sub
>>>> interface you
>>>>>> would like to receive back
>>>>> I guess thats a bit matter of taste. We could prepare quick prototype
>>>> and compare which one looks more useful to majority of folks here.
>>>>>> * I feel the methods on UserQuery are restricting how the IDM can
be
>>>> used.
>>>>>> We can't always guarantee that there will be a first name, last name,
>>>>>> email, etc. However, being completely open and free form like what
was
>>>> done
>>>>>> in the PicketLink IDM is too much. Perhaps we can find some sort
of
>>>> balance
>>>>>> or meta model similar to JPA?
>>>>>>
>>>>>> * Maybe we could have a base no method interface for things and fold
>>>> all
>>>>>> the Query and Manager objects into one (similar to above about JPA)
>>>>>>
>>>>>> * We probably don't want to go down the route of create a query
>>>> language,
>>>>>> but we almost have a DSL with a fluent API here. Maybe we should
think
>>>>>> about about JPA the names out a bit more and actually create a fluent
>>>> API
>>>>>> for the Query objects
>>>>> Would you have time to prepare some simple prototype like this to
>>>> discuss? I'm not sure how much generic design are you proposing in fact.
>>>> Just some bare code snippets would do to back your suggestions with some
>>>> example usage.
>>>>> UserQuery is not something I'm perfectly satisfied with however it is
>>>> still an attempt to have something similar to JPA Criteria Query - which
is
>>>> very intuitive and easy to use IMO. I would love to see more feedback from
>>>> others on how to improve this part in fact.
>>>>>> * I believe we should remove some of the exception constructors and
>>>> make a
>>>>>> message required (this would probably be good to have in all of
>>>>>> DeltaSpike). Adding a cause is great, but being able to create an
>>>> exception
>>>>>> without a message is less helpful for everyone.
>>>>> Good point. I think this part is simply not really polished yet.
>>>>>
>>>>>> * Is there a point for having addObject and addObjects type methods?
>>>> Would
>>>>>> one that's simply a varag be enough? We could check the type that
>>>> comes in
>>>>>> if thy send us a collection, or simply have varags and collection
>>>>>> addObjects methods.
>>>>> Sounds good to me. Again this is probably more matter of taste and
>>>> consistency with other APIs in the project. Maybe there should be some
>>>> general project wide design guideline for choices like this.
>>>>>
>>>>>>
>>>>>> --
>>>>>> 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
>>>
>>>
>>> --
>>> 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
>>>
>>
>>
>> -- 
>> 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


Mime
View raw message