[Dovecot] patch: list shared namespace
Hi,
I've been working on a patch for dovecot 1.2 from the Kolab branch (http://hg.intevation.org/kolab/dovecot-1.2_kolab-branch/) that implements listing of shared namespaces. I've got something that works in some basic way but is still missing some pieces. See the attached patch, which also contains some installation and configuration notes.
Implementation notes:
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.
Another problem is that namespaces for all those users have to be created. The patch does that in shared-storage.c when the shared storage is created. At this stage of development of the patch that works well enough, I think, but it might be better to update the namespaces whenever a list iterator is created.
To avoid unnecessary coupling between the shared namespace code and the ACL plugin, the shared namespace code has a hook that it calls when it needs a list of all the users who may have mailboxes visible to the current user. The ACL plugin sets that hook and uses the dict to produce that list. This way, the ACL plugin depends on the shared namespace code but not the other way round and all the dict handling is in the ACL plugin.
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.
Problems:
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.
List with "%" doesn't list all intermediate mailboxes.
On the one hand arthur sees this:
x LIST "" "*" ...
- LIST (\Noselect \HasChildren) "/" "users/ford"
- LIST (\HasNoChildren) "/" "users/ford/INBOX/hhgttg" x OK List completed.
OTOH, with "%" only this:
x LIST "" "users/ford/%" x OK List completed.
cyrus shows
x LIST "" "users/ford/%"
- LIST (\Noselect \HasChildren) "/" "users/ford/INBOX" x OK List completed.
At least Kontact resp. KMail rely on 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.
Cheers,
Bernhard
-- Bernhard Herzog | ++49-541-335 08 30 | http://www.intevation.de/ Intevation GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 18998 Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
On 28.10.2008, Bernhard Herzog wrote:
I've been working on a patch for dovecot 1.2 from the Kolab branch (http://hg.intevation.org/kolab/dovecot-1.2_kolab-branch/) that implements listing of shared namespaces. I've got something that works in some basic way but is still missing some pieces. See the attached patch, which also contains some installation and configuration notes.
The patch is now available in the kolab branch hg repository: http://hg.intevation.org/kolab/dovecot-1.2_kolab-branch/rev/c2396923cd2f
Bernhard
-- Bernhard Herzog | ++49-541-335 08 30 | http://www.intevation.de/ Intevation GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 18998 Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
On 28.10.2008, Bernhard Herzog wrote:
List with "%" doesn't list all intermediate mailboxes.
On the one hand arthur sees this:
x LIST "" "*" ...
- LIST (\Noselect \HasChildren) "/" "users/ford"
- LIST (\HasNoChildren) "/" "users/ford/INBOX/hhgttg" x OK List completed.
OTOH, with "%" only this:
x LIST "" "users/ford/%" x OK List completed.
I've looked into this. The problem is that acl_mailbox_list_info_is_visible in src/plugins/acl/acl-mailbox-list.c considers nonexistent mailboxes as not visible. The attached patch fixes that for me. I'm not sure it really is the right fix, though. Maybe it would cause some mailboxes to be listed even though they shouldn't.
There's one thing about acl_mailbox_list_info_is_visible and struct acl_mailbox_list_iterate_context that I don't understand. What's the member struct mailbox_info info; used for?
Regards,
Bernhard
-- Bernhard Herzog | ++49-541-335 08 30 | http://www.intevation.de/ Intevation GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 18998 Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
On Fri, 2008-10-31 at 12:52 +0100, Bernhard Herzog wrote:
On 28.10.2008, Bernhard Herzog wrote:
List with "%" doesn't list all intermediate mailboxes.
On the one hand arthur sees this:
x LIST "" "*" ...
- LIST (\Noselect \HasChildren) "/" "users/ford"
- LIST (\HasNoChildren) "/" "users/ford/INBOX/hhgttg" x OK List completed.
OTOH, with "%" only this:
x LIST "" "users/ford/%" x OK List completed.
I've looked into this. The problem is that acl_mailbox_list_info_is_visible in src/plugins/acl/acl-mailbox-list.c considers nonexistent mailboxes as not visible. The attached patch fixes that for me. I'm not sure it really is the right fix, though. Maybe it would cause some mailboxes to be listed even though they shouldn't.
Right, it could (would) cause mailboxes to be listed that aren't supposed to be listed. I think you'll also have a problem if e.g. "foo" exists but doesn't have 'l' right and "foo/bar" exists and has 'l' right. I think % will currently not list "foo". If it behaved correctly it should list it as non-existing mailbox.
The real solution would be to find out if there are any visible child mailboxes. That's kind of annoying to implement though. You'll only need to do that if the mailbox list pattern expects that mailbox to be returned. For example with the above foo and foo/bar mailboxes:
LIST % -> List "foo" as non-existing LIST foo -> List "foo" as non-existing LIST * -> List "foo/bar" only
I'm not exactly sure what's the right way to implement this. That's why it's still in my TODO list instead of actually implemented. :)
There's one thing about acl_mailbox_list_info_is_visible and struct acl_mailbox_list_iterate_context that I don't understand. What's the member struct mailbox_info info; used for?
Looks like it's a bug. Perhaps I moved some of the code to acl_mailbox_list_info_is_visible() which broke it. The idea was anyway to modify info.flags and store them to ctx->info and then return ctx->info from the function. But that's clearly not happening.
Fixed: http://hg.dovecot.org/dovecot-1.2/rev/692aac54ae1c
..and I immediately noticed that's buggy too, so..: http://hg.dovecot.org/dovecot-1.2/rev/ca4e277a6615
On 31.10.2008, Timo Sirainen wrote:
Right, it could (would) cause mailboxes to be listed that aren't supposed to be listed. I think you'll also have a problem if e.g. "foo" exists but doesn't have 'l' right and "foo/bar" exists and has 'l' right. I think % will currently not list "foo". If it behaved correctly it should list it as non-existing mailbox.
That case seems to work correctly with my patch. In my tests so far, it basically behaves exactly like you explain:
LIST % -> List "foo" as non-existing LIST foo -> List "foo" as non-existing LIST * -> List "foo/bar" only
Maybe there are circumstances that I didn't encounter yet, where it does indeed fail.
Bernhard
-- Bernhard Herzog | ++49-541-335 08 30 | http://www.intevation.de/ Intevation GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 18998 Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
On Oct 31, 2008, at 10:03 PM, Bernhard Herzog wrote:
On 31.10.2008, Timo Sirainen wrote:
Right, it could (would) cause mailboxes to be listed that aren't supposed to be listed. I think you'll also have a problem if e.g.
"foo" exists but doesn't have 'l' right and "foo/bar" exists and has 'l' right. I think % will currently not list "foo". If it behaved
correctly it should list it as non-existing mailbox.That case seems to work correctly with my patch.
Oh. Wonder why..
In my tests so far, it basically behaves exactly like you explain:
LIST % -> List "foo" as non-existing LIST foo -> List "foo" as non-existing LIST * -> List "foo/bar" only
Maybe there are circumstances that I didn't encounter yet, where it
does indeed fail.
Well, the main failure is that if the child mailboxes aren't listable,
the parent mailbox shouldn't be listed either. Like if I share a
secret/dovecot corp/contracts/google/october mailbox to someone then
other people really shouldn't be seeing secret/dovecot corp/contracts/
google :)
On Fri, 2008-10-31 at 17:51 +0200, Timo Sirainen wrote:
LIST % -> List "foo" as non-existing LIST foo -> List "foo" as non-existing LIST * -> List "foo/bar" only
There are also some truly horrible cases. For example:
1 list "" foo*
- LIST (\HasNoChildren) "." "foo.foo.foo"
- LIST (\HasNoChildren) "." "foo.bar.baz" 1 ok
2 list "" f*o.%
- LIST (\HasNoChildren) "." "foo.foo.foo"
- LIST (\Noselect \HasChildren) "." "foo.bar" 2 OK List completed.
3 list "" f*r
- LIST (\Noselect \HasChildren) "." "foo.bar" 3 OK List completed.
As you can see, the non-existing "foo.foo" isn't returned because its child "foo.foo.foo" also matches the pattern and is returned. But the non-existing "foo.bar" is returned because its children don't match the pattern. It took me forever to get all this stuff working right with Maildir++. :)
I think it would be possible to implement the same somewhat easily in ACL code:
When ACL code sees that a non-existing mailbox is to be returned, find out if there are any patterns that match the mailbox and that ends with "*" character. If yes, don't return the mailbox (because its children will be returned anyway). If not:
Start a new mailbox listing that lists children of the non-existing mailbox (mailbox/*). If you find:
a) A visible mailbox that matches the original patterns -> don't return the original non-existing mailbox (since its child will be returned later)
b) No visible mailboxes -> don't return the original non-existing mailbox
c) Fallback to returning the non-existing mailbox
The same logic should also be used when determining if shared namespace prefixes should be returned (I think ACL code can do that too?)
Also that code should work properly when mailbox names contain "*" or "%" characters. Basically it means that when generating the mailbox/* pattern replace all "*" chars with "%" chars in the mailbox name and then later when going through the results skip over everything that doesn't begin with the real mailbox name.
On 01.11.2008, Timo Sirainen wrote:
On Fri, 2008-10-31 at 17:51 +0200, Timo Sirainen wrote:
LIST % -> List "foo" as non-existing LIST foo -> List "foo" as non-existing LIST * -> List "foo/bar" only
There are also some truly horrible cases.
I tested this with my acl_mailbox_list_info_is_visible modification in a vanilla dovecot 1.2 (rev. c6482b5cdea1). User listtest2@test.hq has these mailboxes:
- LIST (\HasChildren) "/" "INBOX/foo"
- LIST (\HasChildren) "/" "INBOX/foo/foo"
- LIST (\HasNoChildren) "/" "INBOX/foo/foo/foo"
- LIST (\HasChildren) "/" "INBOX/foo/bar"
- LIST (\HasNoChildren) "/" "INBOX/foo/bar/baz"
INBOX/foo/foo/foo and INBOX/foo/bar/baz have ACLs which give listtest1@test.hq the l-permission. The other mailboxes involved have no ACLs or only ACL settings for the owner. The results for listtest1 are as follows:
1 list "" foo*
- LIST (\HasNoChildren) "." "foo.foo.foo"
- LIST (\HasNoChildren) "." "foo.bar.baz" 1 ok
1 list "" "users/listtest2@test.hq/foo*"
- LIST (\HasNoChildren) "/" "users/listtest2@test.hq/foo/foo/foo"
- LIST (\HasNoChildren) "/" "users/listtest2@test.hq/foo/bar/baz" 1 OK List completed.
2 list "" f*o.%
- LIST (\HasNoChildren) "." "foo.foo.foo"
- LIST (\Noselect \HasChildren) "." "foo.bar" 2 OK List completed.
2 list "" "users/listtest2@test.hq/f*o.%" 2 OK List completed.
The equivalent list command for the owner of the mailboxes, listtest2, doesn't return anything either:
2 list "" "INBOX/f*o.%" 2 OK List completed.
3 list "" f*r
- LIST (\Noselect \HasChildren) "." "foo.bar" 3 OK List completed.
3 list "" "users/listtest2@test.hq/f*r"
- LIST (\Noselect \HasChildren) "/" "users/listtest2@test.hq/foo/bar" 3 OK List completed.
As you can see, the non-existing "foo.foo" isn't returned because its child "foo.foo.foo" also matches the pattern and is returned. But the non-existing "foo.bar" is returned because its children don't match the pattern. It took me forever to get all this stuff working right with Maildir++. :)
I can imagine :). The reason it should work with ACLs more or less automatically is that when the mailbox list is populated by acl_mailbox_try_list_fast, it only adds the mailboxes that the user can see using mailbox_list_iter_update. mailbox_list_iter_update takes care of filling in the nonexisting parent mailboxes if necessary.
In your example, that means only foo.foo.foo and foo.bar.baz are added, regardless of whether foo, foo.foo or foo.bar actually exist. foo, foo.foo and foo.bar are added to the list as nonexisting mailboxes automatically, though. So AFAICT from the other user's point of view it really is as if only foo.foo.foo and foo.bar.baz actually existed.
Of course, assuming there's a reason acl_mailbox_try_list_fast has a "try" in its name and that it actually can fail, foo, foo.foo and foo.bar could perhaps end up in the mailbox list even if they do not have children that are visible to the user.
Bernhard
-- Bernhard Herzog | ++49-541-335 08 30 | http://www.intevation.de/ Intevation GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 18998 Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
On Mon, 2008-11-03 at 13:03 +0100, Bernhard Herzog wrote:
As you can see, the non-existing "foo.foo" isn't returned because its child "foo.foo.foo" also matches the pattern and is returned. But the non-existing "foo.bar" is returned because its children don't match the pattern. It took me forever to get all this stuff working right with Maildir++. :)
I can imagine :). The reason it should work with ACLs more or less automatically is that when the mailbox list is populated by acl_mailbox_try_list_fast, it only adds the mailboxes that the user can see using mailbox_list_iter_update. mailbox_list_iter_update takes care of filling in the nonexisting parent mailboxes if necessary.
That's not correct actually. acl_mailbox_try_list_fast adds all mailboxes that exist in dovecot-acl-list file, i.e. all mailboxes that have 'l' right set to someone (not necessarily to you). So if you have:
foo: owner <no rights> foo/bar: user=xyz l
Then "foo" should be visible as non-existing mailbox for user xyz, but no-one else. With your change it will be visible to everyone.
Of course, assuming there's a reason acl_mailbox_try_list_fast has a "try" in its name and that it actually can fail, foo, foo.foo and foo.bar could perhaps end up in the mailbox list even if they do not have children that are visible to the user.
The name implies that it could fail. But .. hmm. I'm not sure yet, have to look at the code some more. :)
All of this is now implemented. I think shared mailboxes/ACLs are now fully working. The only thing left is to avoid calling acl_lookup_dict_rebuild() after each ACL change, but rather just update the dict directly with the changes.
Hmm. Wonder how quota behaves with shared mailboxes.. That's probably broken.
On Sat, 2008-11-01 at 23:43 +0200, Timo Sirainen wrote:
On Fri, 2008-10-31 at 17:51 +0200, Timo Sirainen wrote:
LIST % -> List "foo" as non-existing LIST foo -> List "foo" as non-existing LIST * -> List "foo/bar" only
There are also some truly horrible cases. For example:
1 list "" foo*
- LIST (\HasNoChildren) "." "foo.foo.foo"
- LIST (\HasNoChildren) "." "foo.bar.baz" 1 ok
2 list "" f*o.%
- LIST (\HasNoChildren) "." "foo.foo.foo"
- LIST (\Noselect \HasChildren) "." "foo.bar" 2 OK List completed.
3 list "" f*r
- LIST (\Noselect \HasChildren) "." "foo.bar" 3 OK List completed.
As you can see, the non-existing "foo.foo" isn't returned because its child "foo.foo.foo" also matches the pattern and is returned. But the non-existing "foo.bar" is returned because its children don't match the pattern. It took me forever to get all this stuff working right with Maildir++. :)
I think it would be possible to implement the same somewhat easily in ACL code:
When ACL code sees that a non-existing mailbox is to be returned, find out if there are any patterns that match the mailbox and that ends with "*" character. If yes, don't return the mailbox (because its children will be returned anyway). If not:
Start a new mailbox listing that lists children of the non-existing mailbox (mailbox/*). If you find:
a) A visible mailbox that matches the original patterns -> don't return the original non-existing mailbox (since its child will be returned later)
b) No visible mailboxes -> don't return the original non-existing mailbox
c) Fallback to returning the non-existing mailbox
The same logic should also be used when determining if shared namespace prefixes should be returned (I think ACL code can do that too?)
Also that code should work properly when mailbox names contain "*" or "%" characters. Basically it means that when generating the mailbox/* pattern replace all "*" chars with "%" chars in the mailbox name and then later when going through the results skip over everything that doesn't begin with the real mailbox name.
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.
On 31.10.2008, Timo Sirainen wrote:
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 have done that for the metadata plugin now, which actually does use per-user
dicts if private metadata is enabled. However, the only reason my ACL plugin
changes use getenv("USER") is that dict_init requires a username. That
username is not actually needed for what the code does with the dict, though.
All entries are public entries and it doesn't matter which user reads or
modifies the dict. Maybe the dict API should have the concept of a purely
public dict which doesn't require a username.
i_info("no acl_shared_dict specified; shared namespaces will not be listed") could be written only if getenv("DEBUG") != NULL.
I did that now.
Is acl_shared_debug stuff only a temporary developer debugging thing, or will it be useful also for sysadmins?
I put that in because it was useful for me while developing. I'm not sure it's useful for admins later on. So maybe I should remove them.
Bernhard
-- Bernhard Herzog | ++49-541-335 08 30 | http://www.intevation.de/ Intevation GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 18998 Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
On 31.10.2008, Timo Sirainen wrote:
On Tue, 2008-10-28 at 21:07 +0100, Bernhard Herzog wrote:
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?
Or perhaps hook_mailbox_list_created? It would be nice to make sure that if a user makes a mailbox available to another user, that it shows up as soon as the other user issues the next LIST command.
What's the best way to determine whether the mailbox list or namespace that was created is actually a shared namespace (or list for a shared namespace)? Should I just check whether ns->storage->name equals "shared"?
Bernhard
-- Bernhard Herzog | ++49-541-335 08 30 | http://www.intevation.de/ Intevation GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 18998 Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
On Nov 4, 2008, at 8:38 PM, Bernhard Herzog wrote:
On 31.10.2008, Timo Sirainen wrote:
On Tue, 2008-10-28 at 21:07 +0100, Bernhard Herzog wrote:
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?
Or perhaps hook_mailbox_list_created? It would be nice to make sure
that if a user makes a mailbox available to another user, that it shows up as
soon as the other user issues the next LIST command.
How would hook_mailbox_list_created help with that?
What's the best way to determine whether the mailbox list or
namespace that was created is actually a shared namespace (or list for a shared
namespace)? Should I just check whether ns->storage->name equals "shared"?
ns->type == NAMESPACE_SHARED?
On Fri, 2008-10-31 at 17:33 +0200, Timo Sirainen wrote:
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.
Maybe something like:
Dict contains /acl/$dest/$source_user -> 1 where $dest is either the ACL user or ACL group (with u= and g= prefixes so they don't get mixed) and $source_user is the user whose mailbox ACL is being changed.
This allows getting a list of all possible users whose shared mailboxes we have visible. Just do a dict iteration for /acl/$user and /acl/$group for each group we belong to. And I guess also check /acl/anyone and /acl/authenticated.
As for updating the dict, it should normally be done by acl_backend_vfile_object_update(). It should also immediately update the dovecot-acl-list file. This should take care of all the ACL changes that are done via IMAP. The dict changes are single entry updates or deletions, no iterations, so the IMAP ACL commands are relatively cheap to use.
Changing dovecot-acl files by hand then requires dropping removed users/groups from the dict. Since we don't know anymore what was removed, we have to go through the entire dict and find all the entries with /acl/*/$source_user. Then add the missing entries and delete the unnecessary entries. Triggering this rescan could be done by acl_backend_vfile_acllist_verify() when it's also rebuilding the dovecot-acl-list file.
Then there's a final problem of how to rebuild the dict in case it gets broken, desynced or whatever. I think this would be solved most easily with an increasing acl-validity value (UNIX timestamp). In dict store it to /acl/validity and in each dovecot-acl-list store it to its header (which doesn't exist yet, need to add it). When reading or updating dovecot-acl-list file check if its acl-validity value is different from dict's /acl/validity value. If it is, trigger a dictionary rescan.
I think it should rescan only the one user whose dovecot-acl-list is being read (so the code would be identical to handling changed dovecot-acl files), because it might take too long to rescan all users' ACL files. This means anyway that deleting everything from dictionary would cause shared mailboxes to be lost from LIST until either the dovecot-acl-list is being read by either a) the owning user logging in and issuing a LIST command, b) the destination users having those mailboxes subscribed and issuing a LSUB command.
Any hope of getting you to implement at least parts of this? :) Also the IMAP ACL code needs to be moved to a separate imap-acl plugin. After those two I could start trying to get all of it merged.
On 28.10.2008, Bernhard Herzog wrote:
- 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.
It happens with vanilla dovecot 1.2 too. Here's my analysis so far:
The dovecot-acl-list is rebuilt by acl_backend_vfile_acllist_rebuild which called in two places, acl_backend_vfile_acllist_refresh and acl_backend_vfile_acllist_verify. The _refresh-funtion rereads the file if it has changed and rebuilds it if it cannot be read. The _verify-function checks whether any of the acl files referenced by the dovecot-acl-list file has been changed and rebuilds the dovecot-acl-list file if that's the case.
The behavior of the _verify funtion is the problem. It only rebuilds if any of the already referenced acl files has changed but not if new acl files have been created. I'm not sure yet what the best way is to determine whether new mailbox whose acllist is being verified has a new acl file.
Cheers,
Bernhard
-- Bernhard Herzog | ++49-541-335 08 30 | http://www.intevation.de/ Intevation GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 18998 Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
On Oct 31, 2008, at 9:42 PM, Bernhard Herzog wrote:
The behavior of the _verify funtion is the problem. It only
rebuilds if any of the already referenced acl files has changed but not if new acl
files have been created. I'm not sure yet what the best way is to determine
whether new mailbox whose acllist is being verified has a new acl file.
Doesn't it notice the new ACL file when that mailbox is being listed
(or selected)? That's at least how I planned that it should work.
Also your IMAP ACL plugin probably should somehow trigger the dovecot- acl-list rebuild whenever an ACL is added to a new mailbox.
participants (2)
-
Bernhard Herzog
-
Timo Sirainen