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

JMAP Email/import setting hasAttachment when it shouldn't? #2389

Closed
wolfsage opened this issue Jun 12, 2018 · 6 comments
Closed

JMAP Email/import setting hasAttachment when it shouldn't? #2389

wolfsage opened this issue Jun 12, 2018 · 6 comments
Assignees
Labels
jmap affects the jmap implementation

Comments

@wolfsage
Copy link
Contributor

wolfsage commented Jun 12, 2018

Email/import of file without attachments results in an Email object with hasAttachment true (but no attachments):

=== BEGIN UPLOAD REQUEST 3455.14 ===
POST http://localhost/jmap/upload/jt-70578ae4-6e66-11e8-b448-533ca35aa5ac@localhost/
Authorization: Basic ...
User-Agent: libwww-perl/6.33
Content-Type: message/rfc822

From: example@example.com
To: example@example.biz
Subject: This is a test
Message-Id: <15288246899.CBDb71cE.3455@cyrus-dev>
Date: Tue, 12 Jun 2018 13:31:29 -0400
MIME-Version: 1.0

This is a very simple message.
=== END UPLOAD REQUEST 3455.14 ===
=== BEGIN UPLOAD RESPONSE 3455.14 ===
HTTP/1.1 201 Created
Connection: close
Date: Tue, 12 Jun 2018 17:31:29 GMT
Vary: Accept-Encoding
Content-Length: 186
Content-Type: application/json; charset=utf-8
Client-Date: Tue, 12 Jun 2018 17:31:29 GMT
Client-Peer: ::1:80
Client-Response-Num: 1

{"accountId":"jt-70578ae4-6e66-11e8-b448-533ca35aa5ac@localhost","blobId":"Gd77fc7cfb6c00cd94b21da64e827fab35913e2c2","size":218,"expires":"2018-06-13T17:31:29Z","type":"message/rfc822"}
=== END UPLOAD RESPONSE 3455.14 ===
=== BEGIN JMAP REQUEST 3455.48 ===
POST http://localhost/jmap/
Authorization: Basic ...
User-Agent: libwww-perl/6.33
Content-Type: application/json

{"methodCalls":[["Email/import",{"emails":{"msg":{"isDraft":false,"isUnread":false,"mailboxIds":{"d9dde7eb-7a53-4812-8ef7-c4ebe9a010e7":true},"isAnswered":false,"isFlagged":false,"blobId":"Gd77fc7cfb6c00cd94b21da64e827fab35913e2c2"}}},"a
w"]],"using":["ietf:jmapmail"]}
=== END JMAP REQUEST 3455.48 ===
=== BEGIN JMAP RESPONSE 3455.48 ===
HTTP/1.1 200 OK
Connection: close
Date: Tue, 12 Jun 2018 17:31:29 GMT
Vary: Accept-Encoding
Content-Length: 263
Content-Type: application/json; charset=utf-8
Client-Date: Tue, 12 Jun 2018 17:31:29 GMT
Client-Peer: ::1:80
Client-Response-Num: 1

{"methodResponses":[["Email/import",{"accountId":"jt-70578ae4-6e66-11e8-b448-533ca35aa5ac","created":{"msg":{"id":"Md77fc7cfb6c00cd94b21da64","blobId":"Gd77fc7cfb6c00cd94b21da64e827fab35913e2c2","threadId":"T468c1e2ebda907cb","size":218}
},"notCreated":{}},"aw"]]}
=== END JMAP RESPONSE 3455.48 ===
=== BEGIN JMAP REQUEST 3455.49 ===
POST http://localhost/jmap/
Authorization: Basic ...
User-Agent: libwww-perl/6.33
Content-Type: application/json

{"using":["ietf:jmapmail"],"methodCalls":[["Email/get",{"ids":["Md77fc7cfb6c00cd94b21da64"]},"ax"]]}
=== END JMAP REQUEST 3455.49 ===
=== BEGIN JMAP RESPONSE 3455.49 ===
HTTP/1.1 200 OK
Connection: close
Date: Tue, 12 Jun 2018 17:31:29 GMT
Vary: Accept-Encoding
Content-Type: application/json; charset=utf-8
Client-Date: Tue, 12 Jun 2018 17:31:29 GMT
Client-Peer: ::1:80
Client-Response-Num: 1
Client-Transfer-Encoding: gzip, chunked

{"methodResponses":[["Email/get",{"state":"74","list":[{"id":"Md77fc7cfb6c00cd94b21da64","blobId":"Gd77fc7cfb6c00cd94b21da64e827fab35913e2c2","threadId":"T468c1e2ebda907cb","mailboxIds":{"d9dde7eb-7a53-4812-8ef7-c4ebe9a010e7":true},"keywords":{"$hasattachment":true},"size":218,"receivedAt":"2018-06-12T17:31:29Z","messageId":["15288246899.CBDb71cE.3455@cyrus-dev"],"inReplyTo":null,"from":[{"name":null,"email":"example@example.com"}],"to":[{"name":null,"email":"example@example.biz"}],"cc":null,"bcc":null,"subject":"This is a test","sentAt":"2018-06-12T17:31:29Z","references":null,"sender":null,"replyTo":null,"bodyValues":{},"textBody":[{"partId":"1","blobId":"Ge9dd999849d79fa314a428a3b8972d4270d2ab5f","size":30,"name":null,"type":"text/plain","charset":"us-ascii","disposition":null,"cid":null,"language":[],"location":null}],"htmlBody":[{"partId":"1","blobId":"Ge9dd999849d79fa314a428a3b8972d4270d2ab5f","size":30,"name":null,"type":"text/plain","charset":"us-ascii","disposition":null,"cid":null,"language":[],"location":null}],"attachedEmails":[],"attachedFiles":[],"hasAttachment":true,"preview":"This is a very simple message."}],"notFound":[],"accountId":"jt-70578ae4-6e66-11e8-b448-533ca35aa5ac"},"ax"]]}
=== END JMAP RESPONSE 3455.49 ===
@brong
Copy link
Member

brong commented Jun 25, 2018

I wonder if this is actually an annotator bug... looks like you're testing against a local instance.

@brong brong self-assigned this Jun 25, 2018
@wolfsage
Copy link
Contributor Author

Yeah, this is on my local vm. Let me know if you want configs or any info

@rsto rsto added the jmap affects the jmap implementation label Jul 3, 2018
@rsto
Copy link
Member

rsto commented Jul 3, 2018

Yes, looks like an annotator issue for top-level messages:

 7209     if (_wantprop(props, "hasAttachment")) {
 7210         int has_att = 0;
 7211         if (is_toplevel)
 7212             msgrecord_hasflag(msg->mr, JMAP_HAS_ATTACHMENT_FLAG, &has_att);
 7213         else
 7214             has_att = bodies.atts.count > 0;
 7215
 7216         json_object_set_new(email, "hasAttachment", json_boolean(has_att));
 7217     }

But for embedded emails it could be an issue with the JMAP bodypart parser.

@rsto
Copy link
Member

rsto commented Jul 3, 2018

Wait a second, this could be a bug in the JMAP Email/import code. I'm just rewriting the code use attachments rather than attachedFiles and attachedEmails, and this is issue is related.

@rsto
Copy link
Member

rsto commented Jul 3, 2018

So, this should be fixed in 7677ede (there's also a test at cyrusimap/cassandane@bcfb787).

But there's lots of ceremony in Email/import et al in the JMAP code to handle hasAttachment: I remember now that we decided to provide a Cyrus-native implementation of setting the hasAttachment flag on emails, rather than using the Perl annotator like we do at FastMail.

But currently, it looks as if we set the hasAttachment flag during email creation/import, which triggers the annotator, which again determines the hasAttachment flag. We might choose one of the following options:

  • set hasAttachment in both Cyrus and any outside annotator (= keep as is)
  • add a flag to Cyrus imapd.conf so the JMAP layer knows if to determine the hasAttachment flag internally (this will save us the cost to determine the flag twice)
  • remove the hasAttachment code from Cyrus and require all installations to use an outside annotator (we ship a prototype for this already in the perl subdirectory of the source distribution)

@elliefm
Copy link
Contributor

elliefm commented Jul 4, 2018

remove the hasAttachment code from Cyrus and require all installations to use an outside annotator (we ship a prototype for this already in the perl subdirectory of the source distribution)

Wearing release manager hat, I don't like this as an option. People struggle enough with getting fundamentals like smtp and sasl working, without mandating they scaffold up and maintain an annotator script too.

add a flag to Cyrus imapd.conf so the JMAP layer knows if to determine the hasAttachment flag internally (this will save us the cost to determine the flag twice)

This sounds good to me. Default it to on, and places like FastMail that already have that functionality externally can turn it off.

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

No branches or pull requests

4 participants