On Tue, 2008-10-28 at 21:07 +0100, Bernhard Herzog wrote:
One of the main problems the patch addresses is getting a list of all users that have mailboxes the logged in user can see. The patch uses a dict to cache information about which users have at least one mailbox that is visible to other users. The dict doesn't cache which other users, though. The cache entry for a given user is updated whenever the dovecot-acl-list file in the maildir root directory is updated. This ties the implementation to a specific acl backend to an extent, but that shouldn't be a problem at the moment.
What I'm worrying here is if all users have shared something. For example how does this work with global ACLs? IIRC if there's a global ACL for e.g. "Spam", Dovecot will create dovecot-acl-list with "Spam" in it for all users. Even without global ACLs the number of users sharing something might be a bit too large.
So the performance would be a lot better if the dict stored something like source_user -> dest, where dest would be all the user names and group names that are included in the user's ACLs (for any mailbox). Then you'd look only at those users' mailboxes where you or your groups are mentioned. Negative user/group rules perhaps shouldn't be in the dict.
I'm not sure the new hook is really needed. The patch could perhaps just as well extend the acl_next_hook_mail_storage_created and acl_next_hook_mailbox_list_created functions to do the namespace creation when they're called for a shared storage or mailbox list.
Perhaps hook_mail_namespaces_created?
Some other things:
v1.2 code supports multiple users per process. That means you can't really use getenv("USER") and you can't store per-user objects as static variables. Rather you should hook into hook_mail_user_created and add the per-user variables to the mail_user structure. See for example lazy-expunge plugin (struct lazy_expunge_mail_user) or quota plugin how that works.
i_info("no acl_shared_dict specified; shared namespaces will not be listed") could be written only if getenv("DEBUG") != NULL.
Is acl_shared_debug stuff only a temporary developer debugging thing, or will it be useful also for sysadmins?
All of my tests so far involved a shared namespace of the form
namespace shared { separator = / prefix = users/%%u/ location = maildir:.../var/mail/%%u:... subscriptions = no list = yes hidden = no }
Also, let's assume two users, ford and arthur with ford's "INBOX/hhgttg" available to arthur as "users/ford/INBOX/hhgttg". Arthur may not list ford's INBOX, though. In the following the current user is always arthur.
I found the following problems:
LIST response includes namespaces the user doesn't really have access to. E.g. if there's another user, zaphod who's made some mailbox available to somebody else, but not arthur, arthur still sees
- LIST (\Noselect \HasChildren) "/" "users/zaphod"
Not sure it's worth fixing this, though.
It'll expose other users' names, which isn't good. It needs to be fixed before I can make a release. Probably not too difficult though. I think the ACL plugin's mailbox listing code could detect when a shared namespace prefix is listed and not show it if it doesn't have visible mailboxes under it. And that's related to the next problem:
- List with "%" doesn't list all intermediate mailboxes.
Yes, this is actually in my TODO list already. I'll read and aswer to your next mail about this..
- The dovecot-acl-list is not always rebuilt, even when it should have been, AFAICT. In particular, if the file exists but is empty, it's never updated, even when ACL later change. Maybe this is a bug in the Kolab branch.
I haven't actually tested dovecot-acl-list all that much, so it's possible that there are bugs in it.