We just need to add few test cases to demonstrate what is supported. I
think we have a fair
number of test cases. We will need to update the group tests.
On 02/07/2013 10:09 AM, Shane Bryzak wrote:
> On 07/02/13 08:43, Marek Posolda wrote:
>> We discussed on irc with Pedro and for point 3, we choosed the approach
>> that:
>>>> IdentityManager.getGroup(String name)
>>>> will return single group only if exactly one group of this name exists.
>> If more groups with same name exists, it will throw exception.
> -1, this behaviour is inconsistent and should be avoided. We need to
> either add a "parent" parameter to the getGroup() method:
>> getGroup(String name, Group parent)
>> or pass in the fully qualified group name:
>> getGroup(String groupPath)
>> or both.
>>> It seems that this won't affect existing applications and unit tests too
>> much. There are also other approaches (return first found group, return
>> null, search only 1st level groups (groups without parent)) but these
>> seems to be more confusing or change existing behaviour.
>>>> wdyt?
>> Marek
>>>> On 07/02/13 14:09, Pedro Igor Silva wrote:
>>> Ok. I think we can add this for groups, as you pointed out.
>>>>>> The rule would be:
>>>>>> 1) You can not add two groups without parent and with the same name;
>>>>>> identityManager.add(new SimpleGroup("management"));
>>>>>> identityManager.add(new SimpleGroup("management")); // should fail
>>>>>> 2) You can add two groups with the same name but different parents;
>>>>>> identityManager.add(new SimpleGroup("management", qaGroup));
>>>>>> identityManager.add(new SimpleGroup("management"), devGroup);
>>>>>> 3) The IdentityManager.getGroup(String name) will return only one group with the specified name with no parent. Or with parent if only one exists with the given name.
>>>>>> 4) The IdentityManager.getGroup(String name, Group parent) will return only one group with the specified name and parent;
>>>>>> 5) If you need all groups with can use the Query API.
>>>>>> 6) I think we can add the method getGroupByGroupId (maybe rename it to getGroupByPath).
>>>>>> Wdyt ?
>>>>>> Regards.
>>> Pedro Igor
>>>>>>>>> ----- Original Message -----
>>> From: "Marek Posolda" <mposolda at redhat.com>
>>> To: "Pedro Igor Silva" <psilva at redhat.com>
>>> Cc: security-dev at lists.jboss.org>>> Sent: Thursday, February 7, 2013 10:51:50 AM
>>> Subject: Re: [security-dev] Group clarification
>>>>>> On 07/02/13 13:20, Pedro Igor Silva wrote:
>>>> I understood your point. Maybe you can use partitions and have something like that:
>>>>>>>> - Partition QA (Realm or Tier) -> Group Management
>>>>>>>> - Partition DEV (Realm or Tier) -> Group Management
>>>>>>>> Or you really need groups with same name ?
>>> yes, it's one of requirements. In portal we are using realms for
>>> different portal organizations (portal containers). But there is still
>>> possibility to have groups with same name withing single realm (you
>>> can't have two children groups called "management" as children of same
>>> parent group, but you can have two "management" groups if both have
>>> different parent group).
>>>>>> If I remember correctly, GateIn didn't support this in early stages few
>>> years ago, but we added it because it was feature request required by
>>> customers.
>>>>>> Regards,
>>> Marek
>>>> Regards.
>>>> Pedro Igor
>>>>>>>> ----- Original Message -----
>>>> From: "Marek Posolda" <mposolda at redhat.com>
>>>> To: security-dev at lists.jboss.org>>>> Sent: Thursday, February 7, 2013 10:02:18 AM
>>>> Subject: [security-dev] Group clarification
>>>>>>>> Hello,
>>>>>>>> One of the current requirements in GateIn is possibility to have groups
>>>> with same name and with different parents. For example: I can have
>>>> groups "/qa/management" and "/dev/management"
>>>>>>>> In other words, I have two groups called "management" but both are in
>>>> different parts of group tree, because first one has parent group "qa"
>>>> and second has parent group "dev". Currently Picketlink IDM 3 doesn't
>>>> support it (it always throws exception when it recognize that group with
>>>> same name already exists). Also I am seeing that concept of GroupID
>>>> (path to group from root group - something like "/qa/management") and
>>>> group key has been removed as well even if it was supported in IDM 3.x
>>>> couple of weeks before.
>>>>>>>> Also for read usecase, there are two methods in IdentityManager to find
>>>> groups:
>>>>>>>> Group getGroup(String groupId);
>>>>>>>> Group getGroup(String groupName, Group parent);
>>>>>>>> I think that first one has been designed to find group with argument as
>>>> groupId, so usage could looks like:
>>>>>>>> Group qaManagersGroup = identityManager.getGroup("/qa/management");
>>>>>>>> Second one has been designed with usage of plain group names like:
>>>>>>>> Group qaGroup = identityManager.getGroup("qa", null);
>>>> Group qaManagersGroup = identityManager.getGroup("management", qaGroup);
>>>>>>>>>>>> Problem is that currently we are always using first one with groupName
>>>> as an argument (not groupId), so it obviously can't work correctly if we
>>>> have two groups with same name "management" because it's unclear which
>>>> one should be result of finding...:-\
>>>>>>>>>>>> Any ideas to address this? My current proposal is:
>>>>>>>> - Return concept of groupId, which will return the path like
>>>> "/qa/management". So usage could be like:
>>>> Group qaGroup = new SimpleGroup("qa");
>>>> Group qaManagementGroup = new SimpleGroup("management", qaGroup);
>>>> assertEquals("management", qaManagementGroup.getName());
>>>> assertEquals("/qa/management", qaManagement);
>>>>>>>> - Either
>>>> -- fix all existing usages of identityManager.getGroup(String groupId),
>>>> so it really expects groupId as argument (not groupName):
>>>>>>>> -- or introduce new method on IdentityManager (and IdentityStore) like:
>>>>>>>> Group getGroupByGroupId(String groupId);
>>>>>>>> It's possible that some identityStore implementations doesn't support
>>>> groups with same name (For example current LDAPIdentityStore can't
>>>> support it because there is only one DN for access all groups, but we
>>>> discussed with Pedro that this is planned to address later)
>>>>>>>> Any thoughts?
>>>> Marek