[dev] Kolab Share wrapup

[Jan Schneider]

Zitat von Gunnar Wrobel <wrobel at horde.org>:

> Hi Jan,
>
> thanks for the feedback. It definitely helped me further advancing  
> the Kolab version of the share driver. At least the basic mock test  
> is running through now.
>
> Zitat von Jan Schneider <jan at horde.org>:
>
>> Zitat von Gunnar Wrobel <wrobel at horde.org>:
>>
>>> Hi!
>>>
>>> the update of the Kolab share backend is complete and the testing  
>>> has been aligned with the SQL based backend. I already fixed a few  
>>> issues of the test suite in cases where I considered the tests to  
>>> be having too much internal knowledge of the SQL backend. There  
>>> are still a few issues left and I wanted to summarize my remaining  
>>> notes and questions here.
>>>
>>> There is one API change that I'd like to suggest:
>>>
>>> 1) The Kolab backend requires a "name" attribute when creating a share
>>>
>>> For the SQL share the "name" attribute is not really relevant and  
>>> you could create an unnamed share. For the Kolab backend this is  
>>> not quite the same. Let's take the calendars as an example: If a  
>>> user on a Kolab backend creates a new calendar within Kronolith he  
>>> will give it a name: "Calendar" for example. The user would expect  
>>> this calendar to pop up with exactly the same name in Outlook,  
>>> Kontact or Thunderbird. For that to work the new calendar will  
>>> have to be named "INBOX/Calendar" on the IMAP server. This is  
>>> however also the only type of ID reference there is: the folder  
>>> name on the server. That means that the "name" attribute and the  
>>> internal backend ID are automatically linked for the Kolab backend.
>>>
>>> Currently a Kolab share object is considered invalid as long as no  
>>> "name" attribute has been set. Actually this approach has worked  
>>> quite well in the past (Horde3) since our code always sets a name.  
>>> But it is currently not obvious from the API that you *must* set a  
>>> name if you use the Kolab backend.
>>>
>>> This is the only interface change I would definitely like to see  
>>> in the current API. We currently use
>>>
>>>   public function newShare($owner, $name = '')
>>>   {
>>>       $share = $this->_newShare($name);
>>>       $share->set('owner', $owner);
>>>       return $share;
>>>   }
>>>
>>> and I would like that to be
>>>
>>>   public function newShare($owner, $share_name = '', $name_attribute = '')
>>>   {
>>>       $share = $this->_newShare($share_name);
>>>       $share->set('owner', $owner);
>>>       $share->set('name', $name_attribute);
>>>       return $share;
>>>   }
>>>
>>>
>>> As we seem to be setting a share name everywhere in the code  
>>> anyway I assume this should pose no problem. Unless nobody  
>>> disagrees I would make that change.
>>
>> Fine for me.
>
> Completed.
>
>>
>>> Besides that change I would have two questions concerning the SQL  
>>> based backend:
>>>
>>> 1) Visibility of shares to the system user
>>>
>>> The "_listSharesSystem" test verifies that the system user only  
>>> sees the "systemshare". For the Kolab backend the system users  
>>> sees "myshare" as well as the default permission grants  
>>> Horde_Perms::SHOW. The SQL driver seems to verify the guest  
>>> permissions only. Is that intended or should the check also  
>>> include the default permissions?
>>
>> _listSharesSystem() doesn't test the "system user" (whatever that  
>> is, there is no such thing in Horde), but the *guest* user (false).
>
> Ah, that wasn't clear to me. I clarified this in the method  
> description of the Share handler and fixed the method name in the  
> test suite. Such a list of guest shares would require "admin mode"  
> for the Kolab driver though (see below).
>
>>
>>> 2) Removing shares twice
>>>
>>> Why does "removeShare()" not fail with the SQL backend? The Kolab  
>>> backend complains that the share does not exist anymore as it has  
>>> been already deleted before resetting the cache. Issue or  
>>> intentional?
>>
>> Good point. This was probably just a copy and paste of the  
>> resetCache() stuff that happened to not fail with SQL shares.
>> I'm not sure if removeShare() should fail if the share doesn't  
>> exist. But if this is the only possible solution in Kolab, I'm fine  
>> with doing the same in the SQL drivers too, for consistency's sake.
>
> I simply removed the second call in the test suite. The Kolab driver  
> would still throw an error in case you remove a missing share, the  
> SQL driver would not. The behavior could be changed in either of the  
> drivers but I didn't see a compelling reason to do so right now.
>
>>
>>> Besides the things above I collected a few notes that I wanted to  
>>> share. None of these require any action but I'm of course happy if  
>>> others have some input on these.
>>>
>>> 1) The Kolab backend enforces authentication and permission
>>>
>>> The SQL backend ignores authentication and the fact that not every  
>>> user can do everything with shares. This is visible from the test  
>>> suite where the SQL tests initialize the Share handler with
>>>
>>>   new Horde_Share_Sql('test', 'john', new Horde_Perms(), $group);
>>>
>>> The current user is set to 'john' but the user is used to create
>
> We talked about this on Wednesday and you were surprised that we  
> tell the share driver the current user at all and assumed  
> (correctly) that this is not necessary. $this->user was used once in  
> the Kolab driver but I was able to move that call into the  
> Kolab_Storage backend. And it is used for the listOwners() method in  
> the SQL driver. This is only used by Ansel though and Micheal R. can  
> probably comment if this method really needs $this->user or if this  
> could be avoided. I assume one could indeed fully remove the current  
> user from the Share lib.
>
>>> system shares and the shares for other users during the tests.  
>>> This is something I simply cannot do with the Kolab backend  
>>> because the IMAP server will tell me: "Oh, you are John and you  
>>> really want to create a share for Jane? Hm, ... NO!" (... unless  
>>> Jane gave John explicit permissions on her account). So I have to  
>>> really switch users during the test suite.
>>
>> That's fine I think. Permission checking is done on the
>> application-level, one of the reasons being the next paragraph.
>
> Even though I said above that the use of the current user can be  
> avoided in the Share library this is not really true for the Kolab  
> version of the driver. The Kolab_Storage backend driver used within  
> the Kolab share driver is authenticated to the IMAP backend as the  
> current user logged into Horde. So the Kolab share driver  
> automatically inherits this dependency on the current user.
>
> Let me detail the core of the problem:
>
> "Permission checking is done on application-level" is not the  
> desired mode for the Kolab backend. It would be possible in case  
> Horde would always access the database (the IMAP backend) with admin  
> permissions.
>
> We provided the IMAP admin credentials to the Horde configuration in  
> the early Horde/Kolab days but this was something the Kolab  
> community always frowned upon. I know the SQL variant basically  
> works this way but for the Kolab database the idea is somewhat  
> different. We rely on the IMAP permission model as Horde is not the  
> only client to the backend database. You can just use Outlook,  
> Kontact, or Thunderbird to access the IMAP database directly. And  
> all the user needs for that are his own credentials.
>
> I could still say: "Horde is a special client so let's use the admin  
> credentials for backend access". But of course I would throw away an  
> important layer of security. And it wouldn't be necessary in most  
> cases. The large part of the data access works just fine without  
> going into admin mode.

Yes, I agree, the Kolab model makes indeed sense (from Kolab's POV),  
and we shouldn't require Kolab access with admin permissions.

> The problem of the share API - as seen from the Kolab perspective -  
> is the fact that there is no distinction between "These are methods  
> available to the user to access data he has permission to see" and  
> "These are methods solely available in admin mode". Of course such a  
> separation in the share library would contradict "Permission  
> checking is done on application-level".

Not necessarily. It's a perfectly valid (even short-term) solution to  
add some flag or boolean method that determines which features of the  
share system are available with the current drivers. See the  
readOnly() and renameSupported() methods in Horde_Group.

> I'm not certain at the moment that this needs to be changed right  
> now. For one thing I do not yet have a clear idea of how I'd like  
> the API to be changed to achieve the separation. I think I would  
> need to take a closer look at how the applications use the "admin  
> mode". And there might just not be enough time for this before the  
> release. In addition I was able to get the test suite to run  
> completely now. So in principle I'm somewhat able to deal with the  
> discrepancies between the different share backends.
>
> In the long run (speak Horde 5) I would like to fix this specific  
> problem however. And maybe I'm even able to take another look before  
> our release next month. So I would like to understand why you said  
> that "Permission checking is done on application-level". What is the  
> reason for requiring that?

No reason, this was just a description of the current state.  
Generally, I do think that permission checking doesn't belong into a  
libraries for storage access/abstraction. You need to be able to do  
administrative tasks in a library, and we don't want to add  
depencencies on a permission system.

However, the share system is a different story, because it *is* about  
permissions, so it might indeed make sense to move permission checking  
completely inside the share system. We would still need to be able to  
check permissions in application code, e.g. to hide certain features  
etc. but that's easy to solve.

> I would also assume that it could be useful to modify the Share API  
> so that it can also deal with databases that have their own access  
> controls.

I can't think of such a system. But that's not a reason to *not*  
modify the API.

>>> 2) listAllShares()
>>>
>>> This call does not really work as expected for the Kolab backend  
>>> because it would require real IMAP manager credentials. The SQL  
>>> backend can support this call for just any user as it does not  
>>> heed the authentication (see (1)).
>>
>> Understood, though a pity from the Horde standpoint, because it  
>> renders scripts like alarms.php and agenda.php useless for Kolab  
>> backends.
>
> I implemented it now so that it would work in case you access the  
> IMAP backend as admin. Which is usually not the case when using the  
> Kolab backend but maybe I'm able to identify a method that would  
> allow me to circumvent the problems for the few cases where we  
> actually use the method in the applications.
>
>>
>>> 3) System shares
>>>
>>> Not implemented yet and I still have to think about these. The SQL  
>>> backend currently assumes that such shares use "null" as the  
>>> creator ID. I will probably implement them with a special user  
>>> account.
>>
>> We could also check if the share backend supports system shares at  
>> all, and only provide this option if it does. This decision is up  
>> to you.
>
> Same as listAllShares().
>
>>
>>> 4) Different share ID's for each user
>>>
>>> The internal ID for shares used by the backend remain the same for  
>>> the SQL backend. They switch for each user with the Kolab backend  
>>> (e.g. while "INBOX/Calendar" will be the basis for the ID for  
>>> John, Jane will see it as "user/John/Calendar"). So far I don't  
>>> see this as a problem but I might be able to find a way around it.
>>
>> That would probably be safer, because currently the ID is not  
>> really an ID with the Kolab backend.
>
> I was indeed able to fix that.

Jan.

-- 
Do you need professional PHP or Horde consulting?
http://horde.org/consulting/