Subject: [Jmap] Review: draft-ietf-jmap-core-10

From: "Bron Gondwana" <[email-masked]>
Date: Thu, 15 Nov 2018 00:12:54 -0500

Finally... sorry this has taken so long. I actually sat down and did a real read through... 
 
*1.2: integer type ranges* 
 
I queried the inclusive vs exclusiveness of the ends of the ranges. Neil assured me that 53 bits can be inclusive at both ends while being exact in JSON. (https://github.com/stedolan/jq/issues/143) 
 
*1.3 Date types* 
 
Fractional seconds are probably going to arrive via Calendar datatypes. I recommend "MUST NOT include time-secfrac if the fractional seconds part is zero". Reword to be less clunky, but that concept. Since mail currently doesn't have fractional seconds, it won't change it, but it will be more forward compatible. 
 
*1.6 Ids* 
 
Please mention RFC8474 and duplicate or reference the advice on choosing IDs which are filesystem, commandline, spreadsheet, etc-safe. 
 
*2. Session resource* 
 
*maxCallsInRequest* has a "MUST" of 32. None of the others have limits defined, and that seems quite a large limit for a MUST. I'd be happy to have SHOULD of 32 there, but not force every server to support 32. 
 
None of the other fields provide guidelines at all. It would be good to document recommended minimum limits for each of them. 
 
*3.2 Request object* 
 
Not just here, but in a bunch of places there are arbitrary strings like "client id". These need a max length. Discussion with Neil suggested making them of "Id" type, which is defined as 255 bytes from the safe range of 64 allowed bytes back in 1.6. 
 
Likewise "accountId". Thought maybe we will hit issues if people want to use [email-masked] as an accountId and don't want to base64 it... 
 
*[ASIDE] I will probably write up a draft called something like draft-gondwana-jmap-errorinjection which adds a Core/error method and/or optional parameters to method calls which allow you to test a client framework by causing a supporting server to throw different errors to otherwise well-formed requests.* 
* *
*5.3 /set* 
 
returns: created/updated/destroyed which are either an object with 1+ items or null. Why not just require an empty object consistent with /changes responses? Likewise with the "not$X". Either that or allow omitting them entirely which is the same as the default value of empty/null. 
 
*5.4 /copy* 
 
For full state mangling consistency, this requires a ifFromInState or similar. So the logic would be: 
 
1. check fromAccountId account Foo state matches ifFromInState (if specified) 
2. generate a consistent snapshot of data out of fromAccount 
3. check accountId account Foo state matches ifInState (if specified) 
4. /import the snapshotted data into accountId. 
5. check fromAccountId account Foo state again, this time for matching destroyFromIfInState (I would rename this destroyIfFromInState actually) - (if specified) 
6. /set#destroy the source IDs on fromAccountId 
 
Obviously, if the state of fromAccount change during the copy, you could wind up with both the source and destination messages present at once, and would have to clean up. Or a server which could do the whole thing atomically could reject the whole request of course, knowing that it couldn't resolve all the states. Presumably ifFromInState and destroyFromIfInState would always be identical, but it still makes sense to allow specifying just one or just the other. 
 
*5.5 /query* 
 
What happens if you don't give a limit and don't ask for total to be calculated, but the server decides to limit the number of records returned? 
 
> If "null", no limit presumed.  The server MAY choose to
enforce a maximum "limit" argument.  In this case, if a greater
value is given (or if it is "null"), the limit is clamped to the
maximum; since the total number of results in the query is
returned, the client can determine if it has received all the
results. 
 
Since we now support calculateTotal, we need to double check how those two interact. 
 
*6. Binary Data* 
 
This is probably the most difficult part of the spec as someone used to implementing IMAP. Uploaded blobs are like temporary files. 
 
> Except where quota restrictions force early deletion, an
unreferenced blob MUST NOT be deleted for at least 1 hour from the
time of upload
 
What does that even mean? I'd say that if a blob has been uploaded an ACCEPTED by the server, then it has to be there for an hour (modulo server failures). Otherwise if you upload 10 blobs and then try to use them but 9 have been quotaed out, that would suck. Better to reject the last 9 uploads. 
 
Or if that's really going to be a concern, we need a Blob/query to list unreferences blobs and a Blob/set#destroy to remove them. 
 
Or at the very least, a Blob/cleanup method which wipes all unreferenced blobs, or maybe just unreferenced blobs, or at least unreferenced blobs created by this session, to clear up space when you get near quota. 
 
Also: some advice around access control and blobs would be useful here. For the Files/* methods at FastMail we create blobs in a special folder if you upload them to an account shared with somebody else, such that they have a folder ACL which contains the uploader and the account owner. Likewise we're planning to add similar ACLs to our Cyrus server for mail, such that it's uploader and account owner only that are allowed to add a reference to an referenced blob. I could also see an argument for "only uploader" and the owner only gets to see the blob once the reference gets created. See above for quota considerations though - if the owner's quota is being billed, then the owner needs a way to clear them. 
 
7.2 Push subscriptions 
 
> If the response code is "503" (Service Unavailable), the JMAP server
MAY try again later, but may also just drop the event.  If the
response code is "429" (Too Many Requests) the JMAP server SHOULD
attempt to reduce the frequency of pushes to that URL.  Any other
"4xx" or "5xx" response code MUST be considered a *permanent failure*
and the push subscription SHOULD be destroyed.
 
I'd like some advice from HTTP people about whether this is the right approach. It feels like it could lead to some pretty flaky feeling push subscription loss for clients in the face of legitimate network reality between different systems. 
 
.... 
 
And I think that's it! 
 
Bron. 
 
-- 
 Bron Gondwana, CEO, FastMail Pty Ltd 
 [email-masked]

---

From: "Neil Jenkins" <[email-masked]>
Date: Thu, 22 Nov 2018 23:02:53 -0500

Thanks for the comments, I've fixed these issues. A few comments and answers: 
 
> returns: created/updated/destroyed which are either an object with 1+ items or null. Why not just require an empty object consistent with /changes responses? Likewise with the "not$X". Either that…
 
Request and response arguments may already be omitted <https://jmap.io/spec-core.html#omitting-arguments> if the default value, and any nullable argument automatically has a default of `null`, so these arguments may be omitted. 
 
> Since we now support calculateTotal, we need to double check how those two interact. 
 
Good catch, I've now made it so the server returns the limit if it was set or modified by the server. 
 
>  
> What does that even mean? 
 
It means clients should have a reasonable expectation that the blob will still be there if they upload it and then try to reference it within a reasonable time frame. However, the server needs to also be able to stop a malicious client from trying to bypass quota restrictions by uploading a load of unreferenced content. 
 
In general it's meant to work like this: 
 
 * The server has some kind of unreferenced blob quota, let's say 250 MB. 
 * The maximum size message a client can send is 50 MB. 
 * The client uploads some blobs to be able to use them as attachments. These now count towards the unreferenced quota and will be deleted by the server after 24h (say) if still unreferenced. 
 * The client saves a draft referencing the blobs it has uploaded (this usually happens pretty much immediately after the upload finishes). The blobs are no longer unreferenced so they are now part of regular quota and no longer a concern. 
 * Rinse and repeat.
 
So the only time you should get anywhere close to the unreferenced blob quota limit under normal operation would be if you had lots of parallel uploads happening and something waiting for them all to finish uploading to use, or something like that I guess. This is really just a safety net. I think the current behaviour is reasonable, but I've added the following text to remind clients that an edge case exists that they should handle: 
 
*Clients should use the blobId returned in a timely manner. Under rare circumstances the server may have deleted the blob before the client uses it; the client should keep a reference to the local file so it can upload it again in such a situation.* 
 
> Also: some advice around access control and blobs would be useful here.  
 
I've added: 
 
*As access controls are often determined by the object holding the reference to a blob, unreferenced blobs MUST only be accessible to the uploader, even in shared accounts.* 
 
> I'd like some advice from HTTP people about whether this is the right approach. It feels like it could lead to some pretty flaky feeling push subscription loss for clients in the face of legitimate…
 
This was really about security concerns, so I've removed this paragraph and added a discussion in the security considerations section about the issues with connecting to unknown 3rd party servers. 
 
Neil.

---

From: "Bron Gondwana" <[email-masked]>
Date: Mon, 26 Nov 2018 17:20:10 -0500

Thanks - I've read the diffs and I'm happy with all these updates. 
 
> _______________________________________________ 
> Jmap mailing list 
> [email-masked] 
> https://www.ietf.org/mailman/listinfo/jmap 
>