auth_mboxgroups: a new in-cyrus group mechanism #4895
Conversation
brong
force-pushed
the
mboxgroups
branch
2 times, most recently
from
61f4367
to
a043cf8
Compare
brong
requested review from
rsto,
ksmurchison and
elliefm
and removed request for
rsto
Did that :) |
|
damn I need to do some rebasery to make this merge correctly with other branches |
There was a problem hiding this comment.
Looks good generally, but I have a bunch of nits of varying severity, details inline.
Also, I'd especially like to see tests for the replication functionality, probably in Replication.pm. I assume this code is already minimally exercised by the existing tests just because the new key/value will be in their MAILBOX dlists. But it would be good to see some tests that tinker with the user's groups and then verify that the expected changes actually appear on the replica
|
@elliefm I've addressed the things you mentioned, and also add the Replication test (which raised that in some configs we won't send the groups due to missing RACLs, so I've made it that we always send them. |
There was a problem hiding this comment.
Looks good, couple of details in the new tests that could be streamlined (details inline), but it's not crucial.
But! This is failing the Backups.xbackup_shared and Backups.shared_mailbox tests in CI, look like the sync_client side is crashing. It might be that backupd doesn't recognise the changed protocol and rejects it, and sync_client doesn't know how to proceed after that? It's not real clear from the CI output, would need to be looked at closely in action.
I might be tempted to say just ignore these failures, since we're getting rid of all the backups code and tests soon anyway (#4896). But I notice that both of the failing tests are something to do with shared mailboxes, and we have a bad habit of accidentally breaking shared mailboxes code, so I think in this case it warrants a deeper look. If it is really broken, it would be good to get a regression test for it added to Replication, so we don't lose that coverage when we throw out the Backups stuff.
| $mastertalk->create("INBOX.Test") || die; | ||
| $mastertalk->create("INBOX.Test.Sub") || die; | ||
| $mastertalk->create("INBOX.Test Foo") || die; | ||
|
|
||
| my $ldata = $mastertalk->list("", "*"); | ||
| $self->assert_deep_equals($ldata, [ | ||
| [ | ||
| [ |
There was a problem hiding this comment.
You could use setup_mailbox_structure() and assert_mailbox_structure() here to remove a lot of clutter. See the List tests for lots of examples.
| $self->assert_deep_equals($rdata, [ | ||
| [ | ||
| [ | ||
| '\\HasChildren' | ||
| ], | ||
| '.', | ||
| 'INBOX' |
There was a problem hiding this comment.
Likewise wrt assert_mailbox_structure(). It would make it easier to see at a glance that the test expects to see brandnew's own mailboxes, plus cassandane's Inbox, without those details getting lost in all the lines of [[[]]]
|
@elliefm yes - you're right - it was broken for shared! I've added a test - Replication.shared_folder so we catch this if it happens again, and fixed it. |
Sure! Again it's copy-paste code. This is better Co-authored-by: elliefm <[email protected]>
auth_mech value changes Co-authored-by: elliefm <[email protected]>
Another fix for UNRELASED Co-authored-by: elliefm <[email protected]>
This takes groups into Cyrus. I want to add a JMAP-style management interface as well, but that's for v2.
It also adds additional pusher info to push a list of all users who can see the change, for handy pusher/notify extensions.