Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC5258] LIST return options are ignored #11

Closed
alecpl opened this issue Jul 29, 2016 · 18 comments
Closed

[RFC5258] LIST return options are ignored #11

alecpl opened this issue Jul 29, 2016 · 18 comments
Assignees
Labels
2.5 affects 2.5

Comments

@alecpl
Copy link
Contributor

alecpl commented Jul 29, 2016

Cyrus IMAP Murder 2.5.8.12-Kolab-2.5.8-13.1.el6.kolab_14

\Subscribed attribute is not returned for subscribed folders when requested. E.g.

C: A0006 LIST (SPECIAL-USE) "" "*" RETURN (SUBSCRIBED)
S: * LIST (\HasNoChildren \Drafts) "/" "Drafts"
S: * LIST (\HasNoChildren \Sent) "/" "Sent"
@elliefm elliefm added the 2.5 affects 2.5 label Aug 22, 2016
@elliefm
Copy link
Contributor

elliefm commented Sep 5, 2016

Haven't been able to reproduce this with current cyrus-imapd-2.5, nor with 2.5.8.

It might be that there's a detail particular to your setup that's triggering this, which my test case has missed?

It might also be a bug in Kolab's modifications. Have you raised this issue with Kolab?

C: 1 capability
S: * OK [CAPABILITY IMAP4rev1 LITERAL+ ID ENABLE AUTH=PLAIN AUTH=LOGIN AUTH=DIGEST-MD5 SASL-IR] debian-testing Cyrus IMAP 2.5.8 server ready
S: * CAPABILITY IMAP4rev1 LITERAL+ ID ENABLE ACL RIGHTS=kxten QUOTA MAILBOX-REFERRALS NAMESPACE UIDPLUS NO_ATOMIC_RENAME UNSELECT CHILDREN MULTIAPPEND BINARY CATENATE CONDSTORE ESEARCH SORT SORT=MODSEQ SORT=DISPLAY SORT=UID THREAD=ORDEREDSUBJECT THREAD=REFERENCES ANNOTATEMORE ANNOTATE-EXPERIMENT-1 METADATA LIST-EXTENDED LIST-STATUS LIST-MYRIGHTS WITHIN QRESYNC SCAN XLIST XMOVE MOVE SPECIAL-USE CREATE-SPECIAL-USE URLAUTH URLAUTH=BINARY AUTH=PLAIN AUTH=LOGIN AUTH=DIGEST-MD5 SASL-IR COMPRESS=DEFLATE X-QUOTA=STORAGE X-QUOTA=MESSAGE X-QUOTA=X-ANNOTATION-STORAGE X-QUOTA=X-NUM-FOLDERS IDLE
S: 1 OK Completed
C: 2 login cassandane "testpw"
S: 2 OK [CAPABILITY IMAP4rev1 LITERAL+ ID ENABLE ACL RIGHTS=kxten QUOTA MAILBOX-REFERRALS NAMESPACE UIDPLUS NO_ATOMIC_RENAME UNSELECT CHILDREN MULTIAPPEND BINARY CATENATE CONDSTORE ESEARCH SORT SORT=MODSEQ SORT=DISPLAY SORT=UID THREAD=ORDEREDSUBJECT THREAD=REFERENCES ANNOTATEMORE ANNOTATE-EXPERIMENT-1 METADATA LIST-EXTENDED LIST-STATUS LIST-MYRIGHTS WITHIN QRESYNC SCAN XLIST XMOVE MOVE SPECIAL-USE CREATE-SPECIAL-USE URLAUTH URLAUTH=BINARY LOGINDISABLED COMPRESS=DEFLATE X-QUOTA=STORAGE X-QUOTA=MESSAGE X-QUOTA=X-ANNOTATION-STORAGE X-QUOTA=X-NUM-FOLDERS IDLE] User logged in SESSIONID=<0525241-9968-1473053124-2-1054053662254863799>
C: 3 create ToDo
S: 3 OK Completed
C: 4 create Projects
S: 4 OK Completed
C: 5 create Projects/Foo
S: 5 OK Completed
C: 6 create SentMail
S: 6 OK Completed
C: 7 create MyDrafts
S: 7 OK Completed
C: 8 create Trash
S: 8 OK Completed
C: 9 subscribe SentMail
S: 9 OK Completed
C: 10 subscribe Trash
S: 10 OK Completed
C: 11 setmetadata SentMail ("/private/specialuse" "\\Sent")
S: 11 OK Completed
C: 12 setmetadata MyDrafts ("/private/specialuse" "\\Drafts")
S: 12 OK Completed
C: 13 setmetadata Trash ("/private/specialuse" "\\Trash")
S: 13 OK Completed
C: 14 list (SPECIAL-USE) "" "*" RETURN (SUBSCRIBED)
S: * LIST (\HasNoChildren \Drafts) "/" MyDrafts
S: * LIST (\Subscribed \HasNoChildren \Sent) "/" SentMail
S: * LIST (\Subscribed \Trash) "/" Trash
S: 14 OK Completed (0.001 secs 11 calls)

Cassandane test: cyrusimap/cassandane@91096e0 -- though note that this tickles the long-standing "missing child info flag on last returned folder" bug, and thus fails for this unrelated reason on 2.5 without a little extra coercion. But you can see from the session log that the \Subscribed flag is returned where expected, and not where it's not.

@alecpl
Copy link
Contributor Author

alecpl commented Sep 5, 2016

Do you test with murder? Is it possible that's a murder issue?

@alecpl
Copy link
Contributor Author

alecpl commented Sep 5, 2016

I just tested without murder on Cyrus IMAP 2.5.9.11-Kolab-2.5.9-12.2.el7.kolab_wf and still see the issue. I'll create a ticket in Kolab's bugtracker.

@elliefm
Copy link
Contributor

elliefm commented Sep 6, 2016

I didn't test with murder (start simple...), but if you've reproduced the bug without murder then I guess that wasn't the missing detail anyway.

@elliefm
Copy link
Contributor

elliefm commented Sep 27, 2016

I'm curious if you've had any feedback from Kolab on this? Are you able to link to the ticket in their tracker?

@alecpl
Copy link
Contributor Author

alecpl commented Sep 27, 2016

https://git.kolab.org/T1465, but no feedback yet.

@brong
Copy link
Member

brong commented Sep 28, 2016

Can you please paste the output of LSUB for that same user as well... just want to check the obvious really dumb question - are those folders actually subscribed?

@alecpl
Copy link
Contributor Author

alecpl commented Sep 28, 2016

They are subscribed.

C: A0005 LSUB "" "*"
S: * LSUB (\Noinferiors) "/" INBOX
S: * LSUB () "/" Drafts
S: * LSUB () "/" Sent
S: * LSUB () "/" Trash
C: A0005 LIST "" "*"
S: * LIST (\Noinferiors \HasNoChildren) "/" INBOX
S: * LIST (\HasNoChildren) "/" Drafts
S: * LIST (\HasNoChildren) "/" Sent
S: * LIST (\HasNoChildren) "/" Trash

Truncated, there was many more folders on the list. And here's my config:

configdirectory: /var/lib/imap
partition-default: /var/spool/imap
admins: cyrus-admin
sievedir: /var/lib/imap/sieve
sendmail: /usr/sbin/sendmail
sasl_pwcheck_method: saslauthd
sasl_mech_list: PLAIN LOGIN
allowplaintext: no
tls_server_cert: /etc/pki/cyrus-imapd/cyrus-imapd.pem
tls_server_key: /etc/pki/cyrus-imapd/cyrus-imapd.pem
auth_mech: pts
pts_module: ldap
# skipped ldap settings
unixhierarchysep: 1
virtdomains: userid
annotation_definitions: /etc/imapd.annotations.conf
sieve_extensions: fileinto reject envelope body vacation imapflags notify include regex subaddress relational copy date index
allowallsubscribe: 0
allowusermoves: 1
altnamespace: 1
hashimapspool: 1
nysievefolder: 1
fulldirhash: 0
sieveusehomedir: 0
sieve_allowreferrals: 0
lmtp_downcase_rcpt: 1
lmtp_fuzzy_mailbox_match: 1
username_tolower: 1
deletedprefix: DELETED
delete_mode: delayed
expunge_mode: delayed
postuser: shared

@brong
Copy link
Member

brong commented Sep 28, 2016

Yeah, so 2.5 definitely has a bug with the final item not getting a \HasNoChildren :( I've fixed that in master.

But otherwise I agree with Ellie - I can't reproduce against a current latest 2.5, so it really does look like a Kolab bug. I wonder what they changed!

@brong
Copy link
Member

brong commented Sep 28, 2016

I added VirtDomains to my test and it doesn't make any difference either. Though I'm not sure I'm testing with a user in a virtual domain... ho hum. Maybe there's something broken with the check in virtdomains specifically?

@brong
Copy link
Member

brong commented Sep 28, 2016

Hmm.... I think we're maybe testing against internal namespace in mboxlist_findsub, but that doesn't work with virtdomains and non-admin. Crap.

@elliefm - the test needs to have a user with virtdomains turned on to fail to find SUBSCRIBED in that case. I've pushed a test to Cassandane for that - it breaks on 2.5 but passes on master.

We're going to have to patch 2.5 for this one. It's not Kolab specific, it's a broken behaviour in upstream 2.5 :(

Bron.

@elliefm elliefm self-assigned this Oct 2, 2016
@elliefm
Copy link
Contributor

elliefm commented Oct 2, 2016

Oh cool, reproducability helps :)

@alecpl, I guess you can close that Kolab task, since the problem exists upstream after all

@elliefm
Copy link
Contributor

elliefm commented Oct 3, 2016

Observations so far:

  • we wind up in list_cb (and not subscribed_cb) because this is a LIST (SPECIAL-USE), not a LIST (SUBSCRIBED)
  • list_cb() calls mboxlist_findsub() to find subscription info -- even though a level above, list_data() took great pains to choose between mboxlist_findsub() vs imapd_namespace.mboxlist_findsub()
  • we can correct the above by adding findall and findsub to struct list_rock and passing them through, though this (alone) doesn't fix the problem. I haven't yet explored this avenue further, I just undid and kept examining the existing code paths
  • mboxlist_findsub() ends up constructing a domainpat like example.com!example.com!user.foo.Trash and trying to use it for NAMESPACE_USER lookups (and finds nothing)

At this point I think maybe list_cb() needs to transform name appropriately before passing it on to mboxlist_findsub (or imapd_namespace.mboxlist_findsub()), or maybe mboxlist_findsub() needs to be cleverer in its construction of domainpat. Or maybe both?

@elliefm
Copy link
Contributor

elliefm commented Oct 3, 2016

Switching list_cb() to use the findsub function that list_data() looked up, and now:

  • list_cb() calls imapd_namespace.mboxlist_findsub() which is aka mboxlist_findsub_alt() to find subscription info
  • it passes through example.com!user.foo.Drafts as the pattern, and foo@example.com as the userid
  • mboxlist_findsub_alt() detects the domainpat as being example.com!
  • then it builds a usermboxname containing example.com!user.foo
  • then it builds a patbuf containing example.com!user.foo.example.com!user.foo.Drafts -- this is a concatenation of usermboxname and pattern.
  • then it iterates through the personal namespace, looking for that prefix, and finds nothing
  • then it would iterate through the other users and shared namespaces, except that in the setup for each it decides not to

@elliefm
Copy link
Contributor

elliefm commented Oct 3, 2016

cyrus-imapd-2.5...elliefm:v25/list-specialuse-subscribed seems to fix it, at least for Bron's one test.

Need some tests to exercise it with Shared and Other Users namespaces...

@elliefm
Copy link
Contributor

elliefm commented Dec 15, 2016

Turns out from testing against shared/other users namespaces that my patch breaks these -- which is why this has stalled out.

@elliefm
Copy link
Contributor

elliefm commented Dec 16, 2016

Okay, I think it's fixed properly now:

cyrus-imapd-2.5...elliefm:v25/list-specialuse-subscribed (has been rebased)

@elliefm
Copy link
Contributor

elliefm commented Dec 16, 2016

This is now on the cyrus-imapd-2.5 branch, and there's tests in cassandane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.5 affects 2.5
Projects
None yet
Development

No branches or pull requests

3 participants