[dev] [commits] Horde branch H4-Group updated. 840392473461fdde7040004a26bceac2a3cc3dcf

[Michael Rubinsky]

Quoting Jan Schneider <jan at horde.org>:

> Zitat von Michael Rubinsky <mrubinsk at horde.org>:
>
>>
>> Quoting Jan Schneider <jan at horde.org>:
>>
>>> Zitat von Michael Rubinsky <mrubinsk at horde.org>:
>>>
>>>>
>>>> Quoting Jan Schneider <jan at horde.org>:
>>>>
>>>> <snip>
>>>>
>>>>> commit a45e4bb5980601ebeea3c3231e0f1afac9fcb944
>>>>> Author: Jan Schneider <jan at horde.org>
>>>>> Date:   Sat Mar 5 18:04:12 2011 +0100
>>>>>
>>>>> Tweaks and fixes to the contact group API.
>>>>>
>>>>> Michael, can we drop listUserGroupObjects(), because this seems  
>>>>> to provide the
>>>>> same functionality like getGroupMemberships()?
>>>>
>>>>
>>>> getGroupMemberships() returns a list group id => group names that  
>>>> the current user is a *member* of (useful for checking if a user  
>>>> has permissions on a resource to which the group has permissions  
>>>> to), while listUserGroupObjects() returns an array of turba lists  
>>>> that the current user has Horde_Perms::SHOW on, to be used e.g,  
>>>> in listing the groups the user has available for assigning  
>>>> permissions to his/her resources.  In other words, User "A" can  
>>>> be a member of User B's contact list, but not have permissions to  
>>>> User B's list (so should not be able to use User B's list to  
>>>> assign permissions to one of User A's resources).
>>>
>>> Hm, that doesn't align at all with the general group API (and how  
>>> we use it in application, if I didn't miss anything), because  
>>> there is only the destinction between listing *all* groups and  
>>> only the groups the user is a member of.
>>>
>>> With that approach we need to consider different group lists for  
>>> permission checking and permission assignment. How did that ever  
>>> work in the past? :) Ah, I guess because we had  
>>> userIsMemberOfGroup() (or some such), which has been dropped in  
>>> the rewrite.
>>
>> I'll have to take a closer look at the new code when I can, but,  
>> with the contactList driver, it doesn't make much sense to me to  
>> return groups based on contact lists that the current user doesn't  
>> even have access to. That is what the listUserGroupObjects() method  
>> call does - it only returns lists the user can actually see. When  
>> User A is assigning group permissions to User A's resources, how is  
>> he supposed to know what users belong in some group that he doesn't  
>> have permissions enough to see? Not to mention, for an installation  
>> like SAPO's, that would lead to some seriously large group lists.
>>
>> With the cursory glance I took at the new API, I don't see why the  
>> this won't work. The contactList driver could use the results of  
>> listUserGroupObjects() in Horde_Group::listAll(). This result will  
>> only contain lists the user can see. When *checking* perms we don't  
>> care about all existing groups, we only care about groups that the  
>> user is a member of (Horde_Group::listGroups()), so this check  
>> should also work as expected.
>
> Not quite, because there are switches in application code whether to  
> use listAll() or listGroups() to display the available groups. This  
> switch is configured with the $conf['share']['any_group'] setting.
> This could be solved by having the admin to always enable this  
> setting when usig the Contactlists driver. But I feel uncomfortable  
> with using such side-effects. Not to mention that this behavior is  
> still completely different from the other drivers. This could be  
> solved after the alpha release though.
>
>> As we discussed when developing this driver, it was never designed  
>> to provide access to ALL contact lists to ALL users. It was  
>> designed to allow one user to assign permissions to his resources  
>> based on that user's groups, not every user's groups.
>
> Yes, and I'm still fine with that.

Sorry, then I still don't see what the issue is.  If we only ever want  
to allow a user to use his own turba lists as groups, then  
Horde_Group::listAll() should ever only return those groups that the  
current user can see, not ALL groups for ALL users i.e., it should  
call Turba_Api::listUserGroupObjects(). This has nothing to with  
whether or not the current user is a member of any group. The way it's  
working right now, since Horde_Group::listAll() is calling  
Turba_Api::getGroups(), it will show every single contact list to  
every user, even if they don't have permission to see it.

If we *do* want to set the conf[share][any_group] setting to only  
allow sharing with groups that the user is a *member* of, then  
Horde_Group::listGroups() is called, which can provide the information  
correctly, but this still presents a problem since the user could be  
shown groups based on turba lists that they do not have enough  
permission to see in Turba, so the user has no way of knowing who he  
is sharing his resource with. This doesn't make much sense with the  
contactList driver anyway since it's unlikely that a user would be a  
member of each and every one of his own Turba contact lists.

mike

The Horde Project (www.horde.org)
mrubinsk at horde.org