[dev] Kolab Share wrapup

[Jan Schneider]

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.

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

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

> 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  
> 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 application-level,  
one of the reasons being the next paragraph.

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

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

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

Jan.

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