Subject: [Jmap] Adam Roach's Discuss on draft-ietf-jmap-core-14: (with DISCUSS and COMMENT)

From: Adam Roach <[email-masked]>
Date: Mon, 25 Feb 2019 13:38:55 -0800

Adam Roach has entered the following ballot position for
draft-ietf-jmap-core-14: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-jmap-core/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

[After consultation with a security AD, I have removed one of my three
DISCUSS items, as I now believe it pertains more broadly to the use
of .well-known and is not specific to this document]

Thanks to the authors for a well-laid-out and easy-to-read document. Thanks also
to everyone who contributed to the completion of this work. I have three
DISCUSS-level items, and a handful of other suggestions that are largely
editorial

---------------------------------------------------------------------------

§7.3:

>  The client may add a query parameter called "types", with the value
...
>  The client may add a query parameter called "closeafter" with value
...
>  The client may add a query parameter called "ping", with a positive

This form of specification of URI parameters is not allowed -- it contravenes
the basic thesis of BCP 190; section 2.4 of which stipulates:

>  Applications MUST NOT directly specify the syntax of queries, as this
>  can cause operational difficulties for deployments that do not
>  support a particular form of a query.
...
>  Extensions MUST NOT constrain the format or semantics of queries.

Given that the base resource used to bootstrap the JMAP infrastructure already
uses URI templates, it seems that they would be a good candidate for specifying
a means of adding these parameters in a way that respects the basic principle
that URI design ownership belongs to the domain administrator.

---------------------------------------------------------------------------

The reference for Server-Sent events normatively points to the WHATWG HTML
spec, which changes on a continuing basis. I don't think we've really worked
out the mechanics of citing this kind of reference normatively; and JMAP takes
the especially dangerous step of citing a specific *section* of the document,
which may well change at arbitrary and potentially frequent intervals. There
is, in fact, no guarantee that the cited "text/event-stream" mechanism will
continue to exist in future versions of the WHATWG document (cf. the <keygen>
element).

In this particular case, I think we want to reference
https://www.w3.org/TR/eventsource/ instead.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

§1.1:

>  The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
>  "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
>  document are to be interpreted as described in [RFC2119].

Please use the boilerplate from RFC 8174.

---------------------------------------------------------------------------

§1.2:

>  Where "Id" is given as a datatype, it means a "String" of at least 1
>  and maximum 255 octets in size, and MUST only contain characters from
>  the "URL and Filename safe" Base 64 Alphabet, as defined in section 5
>  of [RFC4648].  This is the ASCII alphanumeric characters ("A-Za-
>  z0-9"), hyphen ("-"), and underscore ("_").

This is unclear as to whether "=" is allowed. It is included in the RFC 4648
"URL and Filename safe" alphabet, but not mentioned in this document's
reiteration of that alphabet's definition. If the intention is to exclude "=",
please say so explicitly.

---------------------------------------------------------------------------

§1.2 says:

>  All record ids are assigned by the server, and are immutable.

...in which no exceptions are allowed.

§1.6.3 says:

>  The id of a record is immutable, and normally assigned by the server.

...where "normally" indicates that there will be exceptions. These say different
things. Please change the inaccurate one to match the accurate one.

---------------------------------------------------------------------------

§2.1:

>       "accounts": {
>         "13824": {
>           "name": "[email-masked]",
>           "isPersonal": true,
>           "isReadOnly": false,
>           "hasDataFor": [
>             "urn:ietf:params:jmap:mail",
>             "urn:ietf:params:jmap:contacts"
>           ]
>         },

Section 1.2 offers the advice:

>  In particular, it is wise to avoid:
>
...
>
>  o  Ids starting with digits
>
>  o  Ids that contain only digits

It seems that the examples should conform to the document's advice --
implementors often pay more attention to examples than even normative text, much
less non-normative advice.

---------------------------------------------------------------------------

§3.2:

>     3.  A "String" *client id*: an arbitrary string from the client to
>         be echoed back with the responses emitted by that method call

I think the document wants to say a bit more about the scope of uniqueness for
this client ID. Also, the name is a bit confusing, as "Client ID" is most easily
interpreted as "an identifier that identifies a client." This appears to be more
of a "Method ID."

---------------------------------------------------------------------------

§5.7:

>  o  *keywords*: "String[Boolean]" (default: ) A set of keywords that

I think you're missing something after "default:" inside the parentheses.

As an aside -- and I know this is just an example, but people are likely to
follow it when designing things riding on top of JMAP -- I was initially a bit
confused about why this is "String[Boolean]" rather than "String[]",
especially since the only valid value appears to be "true". Is this to allow
for smaller patch operations? If so, text explaining this design decision
would probably be good.

---------------------------------------------------------------------------

§7.1.1:

>  It can wait until the call completes and
>  then compare if the new state string after the /set is the same as
>  was pushed in the StateChange object; if so, it does not need to
>  waste a request asking for changes it already knows.

I think this phrasing may oversimplify things a bit. For example:

 - my current cached notion of state is "A"
 - I send a /set operation
 - I get a "StateChange" that indicates that the new state is "C"
 - I get a response to the /set operation indicating that the state has changed
   from "B" to "C"

The new state after the /set is the same as was pushed in the StateChange
object; however, I'm still missing state that resulted in the change from "A" to
"B".

I think the language in this example needs to be updated to be more precise
about when such requests can be omitted.

---------------------------------------------------------------------------

§7.3:

>  When a new connection is made to the event-source endpoint, a
>  client following the server-sent events specification will send a
>  Last-Event-ID HTTP header

Nit: "...HTTP header field..."

---

From: "Neil Jenkins" <[email-masked]>
Date: Mon, 25 Feb 2019 20:47:04 -0500

Thanks again for the detailed feedback Adam.

On Tue, 26 Feb 2019, at 08:39, Adam Roach wrote:
> §7.3:
> 
> This form of specification of URI parameters is not allowed
> …

Yes, OK. I have changed this to be templated instead of specifying query parameters. It now reads:

The JMAP Session object has an eventSourceUrl property, which is in [@!RFC6570] URI Template (level 1) format. The URL MUST contain variables called `types`, `closeafter` and `ping`.

To connect to the resource, the client makes an authenticated GET request to the event-source URL with the appropriate variables substituted in:

 * `types`: This MUST be either:
   * A comma-separated list of type names, e.g. `Email,CalendarEvent`. The server MUST only push changes for the types in this list.
   * The single character: `*`. Changes to all types are pushed.
 * `closeafter`: This MUST be one of the following values:
   * `state`: The server MUST end the HTTP response after pushing a state event. This can be used by clients in environments where buffering proxies prevent the pushed data from arriving immediately, or indeed at all, when operating in the usual mode.
   * `no`: The connection is persisted by the server as a standard event-source resource.
 * `ping`: A positive integer value representing a length of time in seconds, e.g. `300`. If non-zero, the server MUST send an event called `ping` whenever this time elapses since the previous event was sent. This MUST NOT set a new event id. If the value is `0` the server MUST NOT send ping events.

The server MAY modify a requested ping interval to be subject to a minimum and/or maximum value. For interoperability, servers MUST NOT have a minimum allowed value higher than 30 or a maximum allowed value less than 300.

The data for the ping event MUST be a JSON object containing an *interval* property, the value (type `PositiveInt`) being the interval in seconds the server is using to send pings (this may be different to the requested value if the server clamped it to be within a min/max value).

Clients can monitor for the ping event to help determine when the closeafter mode may be required.

> https://www.w3.org/TR/eventsource/ instead.

OK, I have updated the reference to this instead.

> Please use the boilerplate from RFC 8174.

Done.

> please say so explicitly.

Thanks for pointing this out. "=" should not be allowed; I have updated the text to read:

Where `Id` is given as a datatype, it means a `String` of at least 1 and maximum 255 octets in size, and MUST only contain characters from the "URL and Filename safe" Base 64 Alphabet, as defined in section 5 of [@!RFC4648], excluding the pad character (`=`). This means the allowed characters are the ASCII alphanumeric characters (`A-Za-z0-9`), hyphen (`-`), and underscore (`_`).

> things. Please change the inaccurate one to match the accurate one.

§1.2 is correct; I have removed the word "normally" from §1.6.3.

> less non-normative advice.

Good point, I have updated the examples.

> interpreted as "an identifier that identifies a client." This appears to be more

I agree the name could be clearer; I have renamed this "method call id". As it is just echoed back by the server, there are no uniqueness requirements, although the client will probably want to make it unique within that request if it wants to use it.

> I think you're missing something after "default:" inside the parentheses.

Like in JMAP Mail this was a markdown error, now fixed. Should have read `{}`.

> for smaller patch operations? If so, text explaining this design decision

So the datatype is really a Set. Since JSON doesn't have this as a datatype, we can either map it to an Array or to an Object with dummy values (all `true` in this case). The latter is better because:
 1. You can use patch syntax to add remove items from it rather than having to replace the entire set at once.
 2. Shows that the order of items is unimportant, making it easier to check for equality.
I've added a short sentence to the example to note (1), since this is the most important point.

> object; however, I'm still missing state that resulted in the change from "A" to

Yes, regardless of the push, a client would have to fetch the changes since A if it received this response to the /set operation in order to bring its cache up to date. However, the common case is that the set operation happened on the state the client currently has cached, so it can do this optimisation.

> about when such requests can be omitted.

Hmm, I'm not convinced it was that unclear to begin with, but anyway I've added a sub-clause to clarify this:

*If the client is itself making changes, it may receive a StateChange object while the /set API call is in flight. It can wait until the call completes and then compare if the new state string after the /set is the same as was pushed in the StateChange object; if so, and the old state of the /set response matches the client's previous state, it does not need to waste a request asking for changes it already knows.*

> Nit: "...HTTP header field..."

Sure, fixed.

Neil.

---

From: Adam Roach <[email-masked]>
Date: Mon, 25 Feb 2019 21:24:12 -0600

Thanks. This all looks good, presuming section 2.1 is updated to match 
the new normative language in section 7.3. I'll clear my discusses when 
the next version of the documents are uploaded to the i-d repository.

/a

On 2/25/19 7:47 PM, Neil Jenkins wrote:
> The JMAP Session object has an eventSourceUrl property, which is in 
> [@!RFC6570] URI Template (level 1) format. The URL MUST contain 
> variables called |types|, |closeafter| and |ping|.
>
> To connect to the resource, the client makes an authenticated GET 
> request to the event-source URL with the appropriate variables 
> substituted in:
>
>   * |types|: This MUST be either:
>       o A comma-separated list of type names, e.g.
>         |Email,CalendarEvent|. The server MUST only push changes for
>         the types in this list.
>       o The single character: |*|. Changes to all types are pushed.
>   * |closeafter|: This MUST be one of the following values:
>       o |state|: The server MUST end the HTTP response after pushing a
>         state event. This can be used by clients in environments where
>         buffering proxies prevent the pushed data from arriving
>         immediately, or indeed at all, when operating in the usual mode.
>       o |no|: The connection is persisted by the server as a standard
>         event-source resource.
>   * |ping|: A positive integer value representing a length of time in
>     seconds, e.g. |300|. If non-zero, the server MUST send an event
>     called |ping| whenever this time elapses since the previous event
>     was sent. This MUST NOT set a new event id. If the value is
>     |0| the server MUST NOT send ping events.
>
>     The server MAY modify a requested ping interval to be subject to a
>     minimum and/or maximum value. For interoperability, servers MUST
>     NOT have a minimum allowed value higher than 30 or a maximum
>     allowed value less than 300.
>
>     The data for the ping event MUST be a JSON object containing an
>     /interval/ property, the value (type |PositiveInt|) being the
>     interval in seconds the server is using to send pings (this may be
>     different to the requested value if the server clamped it to be
>     within a min/max value).
>
> Where |Id| is given as a datatype, it means a |String| of at least 1 
> of [@!RFC4648], excluding the pad character (|=|). This means the 
> allowed characters are the ASCII alphanumeric characters 
> (|A-Za-z0-9|), hyphen (|-|), and underscore (|_|).
>
>> §1.2 says:
>>
> I agree the name could be clearer; I have renamed this "method call 
> id". As it is just echoed back by the server, there are no uniqueness 
> requirements, although the client will probably want to make it unique 
> within that request if it wants to use it.
>
>> §5.7:
>>
> Like in JMAP Mail this was a markdown error, now fixed. Should have 
> read |{}|.
>
> So the datatype is really a Set. Since JSON doesn't have this as a 
> datatype, we can either map it to an Array or to an Object with dummy 
> values (all |true| in this case). The latter is better because:
>
>  1. You can use patch syntax to add remove items from it rather than
>     having to replace the entire set at once.
> Yes, regardless of the push, a client would have to fetch the changes 
> since A if it received this response to the /set operation in order to 
> bring its cache up to date. However, the common case is that the set 
> operation happened on the state the client currently has cached, so it 
> can do this optimisation.
>
> /If the client is itself making changes, it may receive a StateChange 
> object while the /set API call is in flight. It can wait until the 
> call completes and then compare if the new state string after the /set 
> is the same as was pushed in the StateChange object; if so, and the 
> old state of the /set response matches the client's previous state, it 
> does not need to waste a request asking for changes it already knows./
>
>> §7.3:
>>