BasicModel.removeFromGroup doesn't remove user from the parent group

[mjobanek]

Hi,

consider this scenario:

User admin = new User("Administrator");
identityManager.add(admin);

Group usersGroup = new Group("Users");
identityManager.add(usersGroup);

Group adminGroup = new Group("Administrators", usersGroup);
identityManager.add(adminGroup);

BasicModel.addToGroup(relationshipManager, admin, adminGroup);
BasicModel.isMember(relationshipManager, admin, usersGroup); //returns true

BasicModel.removeFromGroup(relationshipManager, admin, usersGroup);
BasicModel.isMember(relationshipManager, admin, usersGroup); //returns true

I've seen the implementation of the methods BasicModel.removeFromGroup and BasicModel.isMember, so I understand this behavior, but this looks weird.  Is this behaviour intentional? I would expect that when I remove the user from the parent group the user will be removed from this group and all subgroups.

[shane.bryzak]

I can understand your point, however it's hard to say which behaviour is correct.  In your example the Administrator user isn't an explicit member of the Users group, so arguably an attempt to remove it from this group will do nothing.

What if we added an overloaded version of the removeFromGroup() method that also accepted a boolean value that specifies whether the user should be removed from all subgroups as well?

public static void removeFromGroup(RelationshipManager relationshipManager, Account member,
                                   Group group, boolean removeFromSubGroups)

[mjobanek]

Yes you are right, sometimes is very difficult (or even impossible) to devise an approach which will be suitable in all cases.

I think that the overloaded version with the boolean value "removeFromSubGroups" is not a good idea - there could occur some problems if the value was "false" (not to remove from subgroups but only from the specified group).

Some javadoc explaining this behavior could be sufficient ;-).

The thing which I wanted to point out is when I use the inheritance of the groups the methods removeFromGroup() and isMember() are quite unusable. When the method isMember() says that the user is a member of some group it doesn't have to be true. And when I want to remove a user from a group with subgroups I have to know all subgroups, which the user belong to, remove him from each particular subgroup and then, finally, from the parental group.

I propose two solutions:

1) as I've already said - to document the current behavior

2) or to have one same approach in all methods of the BasicModel (and document it as well):

addToGroup() - add the user to all parent groups

isMember() - don't change

removeFromGroup() - remove from the specified group and all subgroups

[shane.bryzak]

There is a third option - shortly we'll be adding a new feature that allows you to declaratively configure privilege inheritance chains, i.e. a couple of new annotations that let you formally define how privileges are inherited between the identities of a relationship.  As part of this new feature, we'll be adding the following new method to RelationshipManager:

boolean inheritsPrivileges(IdentityType identity, IdentityType assignee);

This method will return true if the assignee has the same privileges as the specified identity.  It's basically a broader spectrum version of hasRole() or isMember(), but will work with any identity types (and actually makes these two methods redundant).  Anyway, my proposal is that we deprecate the isMember() method (as its current function will be duplicated by the inheritsPrivileges() method), and instead add a new method perhaps called isGroupMember() which will only return true if the identity is an explicit member of the specified group.  This will make the behaviour of the BasicModel class more consistent, and coupled with some improved JavaDoc should reduce any confusion.