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

From: Matthew Horsfall <[email-masked]>
Date: Tue, 20 Nov 2018 09:14:25 -0500

Feedback on https://datatracker.ietf.org/doc/draft-ietf-jmap-core/?include_text=1

> 1.2.  The Int and PositiveInt data types
>   Where "Int" is given as a data type, it means an integer in the range
>   -2^53 <= value <= 2^53 (the maximum integer that may be reliably
>   stored in a floating-point double), represented as a JSON "Number".
>
>   Where "PositiveInt" is given as a data type, it means an "Int" where
>   the value MUST be in the range 0 <= value <= 2^53.

The ranges look wrong to me.

RFC 7159 states:

 Note that when such software is used, numbers that are integers and
   are in the range [-(2**53)+1, (2**53)-1] are interoperable in the
   sense that implementations will agree exactly on their numeric
   values.

> 1.4.  JSON as the data encoding format
>
>   JSON is a text-based data interchange format as specified in
>   [RFC7159].  The I-JSON format defined in [RFC7493] is a strict subset
>   of this, adding restrictions to avoid potentially confusing scenarios
>   (for example, it mandates that an object MUST NOT have two properties
>   with the same key).

Nit: "properties" and "keys" are not terms used by the JSON spec, instead "members" 
and "name/value" pairs.

> 1.5.2 Accounts:
>
>   In the event of a severe internal error, a server may have to
>   reallocate ids or do something else that violates standard JMAP data
>   constraints for an account.  In this situation, the data on the
>   server is no longer compatible with cached data the client may have
>   from before.  The server MUST treat this as though the account has
>   been deleted and then recreated with a new account id.  Clients will
>   then be forced to throw away any data with the old account id and
>   refetch all data from scratch.

This section is concerning to me.

If accountIds can change, how would a consumer know that happened?

Furthermore, how could an external service rely on a JMAP store if it can't use a reliable identifier to query/store that data?

> 1.6 Ids
>
> Ids are always "String"s.  An id MUST be at least 1 character in
>  length and maximum 255 octets in size, 

Nit: Mixing character and octet here seems strange

> 2. The JMAP Session Resource

   1.  The URL for the JMAP Session resource.  This may be requested
       directly from the user, or discovered automatically based on a
       username domain (see Service Autodiscovery section below).

Maybe link to a section identifier (2.2 vs 8.3) since there are two of these and it makes jumping around easier.

> 2.2 Service Autodiscovery
> 8.3 Service autodiscovery

Use consistent case

> 2. The JMAP Session Resource:
>
>      *  *maxObjectsInSet*: "PositiveInt" The maximum number of objects
>         the client may send to create, update or destroy in a single
>         "/set" type method call.

Maybe clarify that if the max is, say, 10, that means you can only have 5 creates, 
4 updates, and 1 destroy, or it means you can have 10 of each.

> 2.1. Example
> 
> In the following example JMAP Session object, the user has access to
>   his own mail and contacts via JMAP, as well as read-only access to
>   shared mail from another user. 

I'm not sure where RFCs stand on gender neutrality but I think 'their' is
a fine replacement for 'his'. This seems to be the only gendered bit in the whole document.

> 3.5
>
>  "invalidResultReference": The method used a back reference for one of
>   its arguments (see the next section), but this failed to resolve.

"see the next section" would be more useful as "see section 3.6" for quickly
searching/jumping/linking to the proper section.

> 3.6 References to previous result methods

The complicated example shows 4 api calls, 2 of which use '*', Thread/get and Email/get,
but we gloss over the Thread/get and explain the Email/get. Probably fine...

> 5.4 Copy
> 
>   o  *accountId*: "String" The id of the account to copy records to.
>      This MUST be different to the "fromAccountId".

Seems like this should be toAccountId since for every other call 'accountId' is
the account being operated on. Not sure. I guess you'd always be copying to *this* (the authenticated) account?

>  o  *ifInState*: "String|null" This is a state string as returned by
>     the _Foo/get_ method.  If supplied, the string must match the
>      current state of the account referenced by the accountId,
>      otherwise the method will be aborted and a "stateMismatch" error
>      returned.  If "null", any changes will be applied to the current
>      state.

Hrm, so for Foo/Copy, you can say "Only copy from X to Y if Y is currently in a specific
state". Seems weird?

>   o  *fromAccountId*: "String" The id of the account records were
>      copied from.
>
>   o  *accountId*: "String" The id of the account records were copied
>      to.
>
>   o  *oldState*: "String|null" The state string that would have been
>      returned by _Foo/get_ on this account before making the requested
>      changes, or "null" if the server doesn't know what the previous
>      state string was.
>
>   o  *newState*: "String" The state string that will now be returned by
>      _Foo/get_ on this account.

Which account is "this account"? The one being copied from, or to? I guess to.

> 5.8 Proxy Considerations
>
>   1.  It must pass a "createdIds" property with each subrequest.  If
>       this is not given by the client, an empty object should be used
>       for the first subrequest.  The "createIds" property of each
>       subresponse should be passed on in the next subrequest.

CreateIds -> CreatedIds

> 6. Binary Data
>
>   o  *type*: "String" The media type of the file (as specified in
>      [RFC6838], section 4.2) as set in the Content-Type header of the
>      upload HTTP request.

Why is 'type' returned for an Upload response when we say:

"The blobId solely represents
the raw bytes of data, not any associated metadata such as a file
name or content type. "

Also it's just returning what was set in the Content-Type header of
the request?

>   As the data for a particular blobId is immutable, and thus the
>   response in the generated download URL is too, implementors are
>   recommended to set long cache times for successful responses, for
>   example "Cache-Control: private, max-age=31536000".

What if someone wants to delete a message and ensure its blob is no
longer downloadable? With this recommendation, that data might continue
to be served up for a very long time.

> 7.1 The StateChange Object
>
> o  *changed*: "String[TypeState]" A map of _account id_ to an object
 >     encoding the state of data...

Should _account id_ be _accountId_ for consistency?

> 7.1.1 Example
>
> In this example, the server has almalgamated a few changes together
>  across two different accounts the user has access to, before pushing
>  the following StateChange object to the client:

almalgamated -> amalgamated (or use a simpler word)

> 7.2 Push Subscription
> 
>     *  *p256dh*: the P-256 ECDH Diffie-Hellman public key as described
>        in [RFC8291], encoded in URL-safe Base64 representation as
>        defined in [RFC4648].
>
>      *  *auth*: the authentication secret as described in [RFC8291],
>         encoded in URL-safe base64 representation as defined in
>         [RFC4648].

We should use consistent case for 'base64'

>   When these credentials are not time bounded (e.g.  [RFC7617] Basic
>   Authentication), the server SHOULD set an expiry time for the push
>   subscription if none given, and limit the expiry time if set too far
>   in the future.  This maximum expiry time MUST be at least 48 hours in
>   the future and SHOULD be at least 7 days in the future.

Why 48 hours / 7 days (explain?)

Cheers,

-- Matthew Horsfall (alh)

---

From: "Neil Jenkins" <[email-masked]>
Date: Thu, 22 Nov 2018 21:35:28 -0500

Thanks Matt, I've fixed the issues you raised. A few comments and answers to questions: 
 
>  sense that implementations will agree exactly on their numeric 
 
I've changed this to match RFC7159. I think there's a slight difference in opinion on what's considered "safe" but it's better to follow existing convention (and it makes no practical difference anyway). 2^53 can be stored exactly, however 2^53+1 cannot and so 2^53+1 == 2^53 in IEEE754 arithmetic, hence I guess why you might consider it "unsafe". 
 
> If accountIds can change, how would a consumer know that happened? 
 
The point of this is there's a catastrophic change, e.g. you lose all the ids you have assigned to objects and have to assign everything a new one. This would be so utterly confusing to clients, even if you return cannotCalculateChange errors for `/changes` calls etc., that it's better to pretend you have a whole new account and force the client to resync everything from scratch and throw away all assumptions. This should be a very, very rare occurrence, but it's a recommendation of what you must do to handle this scenario should you need. 
 
> Also it's just returning what was set in the Content-Type header of 
 
Because in browser contexts the Content-Type header may be set by the browser automatically and you want to know what it sent. 
 
> to be served up for a very long time. 
 
Hmm, yes this is a consideration. It depends where your caching layers are I guess whether this is likely to be a problem. `Cache-Control: private` means that the response is intended for a single user and MUST NOT be cached by a shared cache like a proxy server, but a local client might still keep this cached for some time after the last reference is deleted. Don't know if this needs a comment in the spec; anyone got an opinion? 
 
> Why 48 hours / 7 days (explain?) 
 
I've added this text to the end of the paragraph: *An app running on a mobile device may only be able to refresh the push subscription lifetime when it is in the foreground, and so this gives a reasonable timeframe to allow this to happen.* 
 
Neil.