[dev] Kolab Share wrapup

[Gunnar Wrobel]

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.

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".

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?

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.

>
>> 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.

Cheers,

Gunnar

>
> Jan.
>
> -- 
> Do you need professional PHP or Horde consulting?
> http://horde.org/consulting/
>
> -- 
> Horde developers mailing list
> Frequently Asked Questions: http://horde.org/faq/
> To unsubscribe, mail: dev-unsubscribe at lists.horde.org

-- 
Core Developer
The Horde Project

e: wrobel at horde.org
t: +49 700 6245 0000
w: http://www.horde.org

pgp: 9703 43BE
tweets: http://twitter.com/pardus_de
blog: http://log.pardus.de