Subject: Re: [Jmap] Review of draft-ietf-jmap-core-06

From: Stephan Bosch <[email-masked]>
Date: Sat, 4 Aug 2018 10:20:50 +0200

Hi,

Got some more:

Op 30/07/2018 om 17:34 schreef Stephan Bosch:
> ## Section 4.4:
>
> -> How is/could the complexity of filters limited by the server? What 
> error should be
> returned if such limit is exceeded? Should the client be able to know 
> the limits from
> querying some session data?

-> The complex Foo/query filter syntax involving AND,OR,NOT operators 
could use some examples, both in core and mail. In fact, I generally 
think each defined method needs to have at least one example of moderate 
complexity in the same document section.

-> I was just looking at how the Foo/query filter structure would be 
parsed (assuming I understand it correctly). The definition 
"(FilterOperator|FilterCondition)" could be a bit annoying to parse it 
seems. The FilterOperator object has "operator" and "conditions" 
members, whereas FilterCondition has members that depend on the JMAP 
object involved. So, is FilterCondition allowed to have "operator" and 
"condition" members (for whatever reason those would make sense for that 
object)? And, conversely, is FilterOperator allowed to be extended in 
future specifications with any member that the parser could ignore? If 
either of these questions could be answered with "yes" there could be a 
parsing ambiguity, making parsing the filter hierarchy reliably pretty 
annoying.

Regards,

Stephan.

---

From: Neil Jenkins <[email-masked]>
Date: Mon, 06 Aug 2018 23:24:51 -0400

Hi Stephen,

Thanks for the review, much appreciated. I've made changes to account for your comments and suggestions. In answer to your questions and points that precipitated more substantial changes:

> -> Should we define a separate special "Size" Number type for size values? Otherwise, the >= 0 requirement for size values needs to be restated everywhere (which it currently isn't).

I've defined Int and PositiveInt types and updated the type signatures to use these where appropriate.

> -> How long is a client supposed to cache the session data? What if e.g. the API URLs need to change while clients are active?

Well, if you get a 404 back from a connection to the API endpoint, the client needs to see if it there is a new endpoint. I think thinks is probably sufficient (and the endpoints are unlikely to move frequently in real world usage), but if anyone has an alternate view I'm happy to hear it.

> -> Next section actually defines "resultReference", which was not listed 
> here.
> -> Shouldn't that be called e.g. "invalidResultReference" instead?

Yes, agreed, this is a better fit with the naming scheme. I have updated this.

> -> Would it be useful to add the ability for a client to make the server omit (specific) responses for a method? I.e. have certain response data available only for references from other methods but not return it to the client? This could save some effort on sending/parsing data that the client doesn't really need to see.

We've considered this before, but I've been trying to balance the number of knobs (and so complexity) with the power it gives you. This doesn't give any additional functionality, just saves a small amount of overhead. It also potentially makes proxying JMAP harder, since the proxy may have to rewrite the flag to include the method's output in the response so it can pass to another backend, then going back and removing the output before returning it to the client.

> -> Can result references also yield arrays of objects or only arrays of 
> strings (as in the example)?

It could yield an array of objects (e.g. referencing the list property of a /get response).

> If yes, this could maybe be used to make some really stupid/abusive requests that copy objects between contexts (accounts), without using the proper copy methods. What to do with that?

I don't think you could do this at the moment, because there's no way to transform the back reference to those objects into a map of id -> object, which you would need to pass it to the create function. But even if you could, I don't think this is likely to be a problem in practice as long as you enforce reasonable limits on the /set function (which JMAP already allows you to do).

If you have a specific idea of how this might be abused in mind, please post it so we can consider it in more detail.

> -> Can there be duplicate method responses? I.e., can there be 
> ambiguities in reference resolution?

There are no methods that produce multiple outputs with the same name at the moment, and I think it is unlikely anyone would define one, although it's not explicitly forbidden. The resolution algorithm specifies that you take the first matching instance, so there would be no ambiguity here, just the later result would not be addressable via backreferences.

> -> What about a generic temporary failure? "tryLater" ?

There's already a serverUnavailable method-level error. Do you think we need this at the SetError level too?

> -> What a bout a "tooBig" error?

I have added a generic tooLarge SetError.

> error should be returned if such limit is exceeded?

The spec already defines an unsupportedFilter error, which may be returned if ”the filter is syntactically valid, but the server cannot process it“.

> Should the client be able to know the limits from querying some session data?

I think this would be really hard to make definable, because the limit may be more to do with which parts of a message the server has indexed, rather than the number of operators (or whatever) in the filter.

> -> How would queryState be implemented; some hash of all results?

This is server dependent, but in Cyrus it depends a bit on the filter. If the filter is the common: { inMailbox: "foo" }  (i.e. the list of messages in the mailbox with id "foo"), then it serialises the HIGHESTMODSEQ and UIDNEXT for the mailbox. If these change, the results might have changed.

I've outlined an example algorithm (and data structures) for efficiently calculating /queryChanges here <https://jmap.io/server.html#email/querychanges>.

> parsed … So, is FilterCondition allowed to have "operator" and
> object)?

I've added to the spec that a FilterCondition object MUST NOT have an "operator" property, so they can always be disambiguated based on the presence/absence.

I will publish a revised draft shortly.

Cheers,
Neil.

---

From: Bron Gondwana <[email-masked]>
Date: Tue, 07 Aug 2018 22:49:58 +1000

On Tue, Aug 7, 2018, at 13:24, Neil Jenkins wrote:
> Hi Stephen,
> 
> Thanks for the review, much appreciated. I've made changes to account
> for your comments and suggestions. In answer to your questions and
> points that precipitated more substantial changes:> 
>> -> Should we define a separate special "Size" Number type for size
>> values? Otherwise, the >= 0 requirement for size values needs to be
>> restated everywhere (which it currently isn't).> 
I assume all the clarity around 0+, 1+ and -lots to +lots are covered...
>> -> How long is a client supposed to cache the session data? What if
>> e.g. the API URLs need to change while clients are active?> 
> Well, if you get a 404 back from a connection to the API endpoint, the
> client needs to see if it there is a new endpoint.. I think thinks is
> probably sufficient (and the endpoints are unlikely to move frequently
> in real world usage), but if anyone has an alternate view I'm happy to
> hear it.
The one interesting case here is "server/datacentre went offline" - and
either a 404 or timeout is the likely symptom that a client will see.
The other possibility is a controlled failover in which case I could see
an argument for the server replying with a 302 or even just a header
saying "contact this other address next time" to migrate existing
clients away.
It's tricky because... I dunno, send them a push and hope they make
another request?  It's hard to do a clean shutdown in this scenario.
>> -> Would it be useful to add the ability for a client to make the
>> server omit (specific) responses for a method? I.e. have certain
>> response data available only for references from other methods but
>> not return it to the client? This could save some effort on
>> sending/parsing data that the client doesn't really need to see.> 
> We've considered this before, but I've been trying to balance the
> number of knobs (and so complexity) with the power it gives you. This
> doesn't give any additional functionality, just saves a small amount
> of overhead. It also potentially makes proxying JMAP harder, since the
> proxy may have to rewrite the flag to include the method's output in
> the response so it can pass to another backend, then going back and
> removing the output before returning it to the client.
As much as I am in love with the general concept of "the server only
returns/processes what the client explicitly asks for" I too struggle
with this one :(  I'm so torn.
>> -> Can result references also yield arrays of objects or only
>> arrays of>> strings (as in the example)?
> 
> It could yield an array of objects (e.g. referencing the list property
> of a /get response).> 
>> If yes, this could maybe be used to make some really stupid/abusive
>> requests that copy objects between contexts (accounts), without using
>> the proper copy methods. What to do with that?> 
> I don't think you could do this at the moment, because there's no way
> to transform the back reference to those objects into a map of id ->
> object, which you would need to pass it to the create function. But
> even if you could, I don't think this is likely to be a problem in
> practice as long as you enforce reasonable limits on the /set function
> (which JMAP already allows you to do).> 
> If you have a specific idea of how this might be abused in mind,
> please post it so we can consider it in more detail.
It's definitely more likely to lead to servers returning a syntax error
than anything else - but it's definitely a good case to consider for the
test framework to check!
> There are no methods that produce multiple outputs with the same name
> at the moment, and I think it is unlikely anyone would define one,
> although it's not explicitly forbidden. The resolution algorithm
> specifies that you take the first matching instance, so there would be
> no ambiguity here, just the later result would not be addressable via
> backreferences.
Again worth testing if we ever get one.

> we need this at the SetError level too?
HTTP has a pretty rich set of  codes which are segmented into different
categories: temporary error, permanent error, success (but with notes),
etc.  The arguments here drop into the same conflict/tension between
being maximally expressive/informative and simplicity of implementation
(and even potential security risks with giving too much information
about why you're rejecting something).
>> -> How is/could the complexity of filters limited by the server? What>> error should be returned if such limit is exceeded?
> 
> The spec already defines an unsupportedFilter error, which may be
> returned if ”the filter is syntactically valid, but the server cannot
> process it“.
Given that the filter is generally a result of human input, that seems
reasonable.  The human can create something less complex.  The general
guiding principle of errors is that they should be informative and
actionable - the receiver of error should both understand what they did
wrong, and have a path to back to success.  The issue with just
"unsupportedFilter" could be that it's not easy for the creator to know
if it's because it's too complex, or includes an unknown condition.
> This is server dependent, but in Cyrus it depends a bit on the filter.
> If the filter is the common: { inMailbox: "foo" }  (i.e. the list of
> messages in the mailbox with id "foo"), then it serialises the
> HIGHESTMODSEQ and UIDNEXT for the mailbox. If these change, the
> results might have changed.> 
> I've outlined an example algorithm (and data structures) for
> efficiently calculating /queryChanges here[1].
Both the open source Cyrus IMAP server and the JMAP proxy have some
example code too.
>> -> I was just looking at how the Foo/query filter structure would be>> parsed … So, is FilterCondition allowed to have "operator" and
>> "condition" members (for whatever reason those would make sense
>> for that>> object)?
> 
> I've added to the spec that a FilterCondition object MUST NOT have an
> "operator" property, so they can always be disambiguated based on the
> presence/absence.
Yeah, that's a good call.  It's a pity there's not enough different
structures in JSON to do anything even clearer to separate between the
two, but that's true of any language really - mixing  leaves and
branches in a tree structure is always messy.
Bron.

--
  Bron Gondwana, CEO, FastMail Pty Ltd
  [email-masked]

Links:

  1. https://jmap.io/server.html#email/querychanges

---

From: Stephan Bosch <[email-masked]>
Date: Sun, 19 Aug 2018 01:21:05 +0200

Hi,

Op 07/08/2018 om 14:49 schreef Bron Gondwana:
>> points that precipitated more substantial changes:
>>
>>> restated everywhere (which it currently isn't)..
>>
>> I've defined Int and PositiveInt types and updated the type 
>> signatures to use these where appropriate.
>
>>> e.g. the API URLs need to change while clients are active?
>>
>> Well, if you get a 404 back from a connection to the API endpoint, 
>> the client needs to see if it there is a new endpoint.. I think 
>> thinks is probably sufficient (and the endpoints are unlikely to move 
>> frequently in real world usage), but if anyone has an alternate view 
>> I'm happy to hear it.
>
> The one interesting case here is "server/datacentre went offline" - 
> and either a 404 or timeout is the likely symptom that a client will 
> see.  The other possibility is a controlled failover in which case I 
> could see an argument for the server replying with a 302 or even just 
> a header saying "contact this other address next time" to migrate 
> existing clients away.
>

The thing is, for protocols like IMAP that keep the connection open, the 
solution is simple: kill all active connections from the server side, 
thereby making sure the clients reconnect and (likely) pick up on the 
new settings and capabilities. For JMAP, there's no such option: the 
loss of an HTTP connection from either side is usually insignificant. 
Yes, changed URLs cause visible problems at the client, but other 
session changes (adding accounts, changing capabilities) don't lead to 
problems quickly. If the client never re-downloads the session data, it 
will not pick up on the changes indefinitely and the server can do 
nothing about that. If there is some sort of session TTL, the situation 
will always resolve itself within a server-controlled reasonable time frame.

>>> sending/parsing data that the client doesn't really need to see.
>>
>> of overhead. It also potentially makes proxying JMAP harder, since 
>> the proxy may have to rewrite the flag to include the method's output 
>> in the response so it can pass to another backend, then going back 
>> and removing the output before returning it to the client.
>

Ah. I must say I don't have a strong preference either way. I was just 
wondering why this option was omitted.

And  a point I forgot to mention: this document could maybe use a 
(short) section with proxy considerations. It refers to proxying as a 
motivation for certain design decisions (e.g. sending createdIds in the 
request), but that is not explained much further. Why is proxying 
useful? What would a setup like that look like? What to be careful 
about? Etc..

>> It could yield an array of objects (e.g. referencing the 
>> |list| property of a |/get| response).
>>
>>> requests that copy objects between contexts (accounts), without 
>>> using the proper copy methods. What to do with that?
>>
>> practice as long as you enforce reasonable limits on the /set 
>> function (which JMAP already allows you to do).
>>
> It's definitely more likely to lead to servers returning a syntax 
> error than anything else - but it's definitely a good case to consider 
> for the test framework to check!

Yeah, I'm just making sure it is considered.

>> specifies that you take the first matching instance, so there would 
>> be no ambiguity here, just the later result would not be addressable 
>> via backreferences.
>

Again, I'm just making sure it is considered.

>> There's already a |serverUnavailable| method-level error. Do you 
>> think we need this at the SetError level too?
>
> categories: temporary error, permanent error, success (but with 
> notes), etc.  The arguments here drop into the same conflict/tension 
> between being maximally expressive/informative and simplicity of 
> implementation (and even potential security risks with giving too much 
> information about why you're rejecting something).

It all depends on whether a partial (per-object) temporary failure makes 
sense. I have no clear scenario in mind, but adding error codes in the 
future is usually quite a pain. Having something generic available can 
be useful.

>> The spec already defines an |unsupportedFilter| error, which may be 
> actionable - the receiver of error should both understand what they 
> did wrong, and have a path to back to success.  The issue with just 
> "unsupportedFilter" could be that it's not easy for the creator to 
> know if it's because it's too complex, or includes an unknown condition.

Bron is stating my point in his last sentence.

>> This is server dependent, but in Cyrus it depends a bit on the 
>> filter. If the filter is the common: |{ inMailbox: "foo" }|  (i.e. 
>> the list of messages in the mailbox with id |"foo"|), then it 
>> serialises the HIGHESTMODSEQ and UIDNEXT for the mailbox. If these 
>> change, the results might have changed.
>>
>> efficiently calculating /queryChanges here 
>> <https://jmap.io/server.html#email/querychanges>.
>

Ok.

> branches in a tree structure is always messy.

I've thought about this a little more. What about turning the 
FilterOperator into an array of conditions in which the first element is 
the operator? So, something like:

"filter": ["AND", {<FilterCondition>}, {<FilterCondition>}, ["OR", 
{<FilterCondition>}, {<FilterCondition>], ["NOT", ["OR", 
{<FilterCondition>}, {<FilterCondition>}, {<FilterCondition>}]]]

Summarizing, the first element is what used to be the "operator" member 
and the rest of the elements are the items of the "conditions" member. 
This makes it unambiguous and more concise.

This is similar to the way JMAP methods are arrays in which the position 
of elements has meaning.

Regards,

Stephan.

---

From: Stephan Bosch <[email-masked]>
Date: Sun, 19 Aug 2018 01:47:45 +0200

Hi,

Op 07/08/2018 om 05:24 schreef Neil Jenkins:
> Hi Stephan,
> I have added a generic |tooLarge| SetError.

Why is there now a separate emailTooLarge SetError in mail-07?

Regards,

Stephan.

---

From: Neil Jenkins <[email-masked]>
Date: Wed, 29 Aug 2018 01:22:01 -0400

Hi Stephan,

> The thing is, for protocols like IMAP that keep the connection open,
    the solution is simple: kill all active connections from the server
    side, thereby making sure the clients reconnect and (likely) pick up
    on the new settings and capabilities. For JMAP, there's no such
    option: the loss of an HTTP connection from either side is usually
    insignificant. Yes, changed URLs cause visible problems at the
    client, but other session changes (adding accounts, changing
    capabilities) don't lead to problems quickly. If the client never
    re-downloads the session data, it will not pick up on the changes
    indefinitely and the server can do nothing about that. If there is
    some sort of session TTL, the situation will always resolve itself
    within a server-controlled reasonable time frame.

The trouble with a TTL is always what to set the value to. Too long and the client can be out of date for a long time; too short and you have a large number of unnecessary requests. I think a better solution might be to include a state string on the session object itself, and have each API request include the current session state on the Response object. The client can compare them and refetch if different. This piggybacks onto existing requests so has very little overhead, but results in almost immediate updates. The only time this wouldn't work is if e.g. the API url has changed, and so the request fails. But a failing request probably should prompt the client to refetch the session object anyway, so you're still covered.

Sound reasonable?

> And  a point I forgot to mention: this document could maybe use a
    (short) section with proxy considerations. It refers to proxying as
    a motivation for certain design decisions (e.g. sending createdIds
    in the request), but that is not explained much further. Why is
    proxying useful? What would a setup like that look like? What to be
    careful about? Etc..

I'll add a section on this.

>>> The spec already defines an unsupportedFilter error,
          which may be returned if ”the filter is syntactically valid,
          but the server cannot process it“.
>> 
>> Given that the filter is generally
        a result of human input, that seems reasonable.  The human can
        create something less complex.  The general guiding principle of
        errors is that they should be informative and actionable - the
        receiver of error should both understand what they did wrong,
        and have a path to back to success.  The issue with just
        "unsupportedFilter" could be that it's not easy for the creator
        to know if it's because it's too complex, or includes an unknown
        condition.
> Bron is stating my point in his last sentence.

I think this error is the one you want here, but I'll update the description. If you specified conditions not in the spec, or gave the wrong type or something, that would be an invalidArguments error. The unsupportedFilter error is really "another server may be able to process this filter but, alas, this one cannot". Clients should then suggest users simplify their criteria to perhaps find something the server can support.

> I've thought about this a little more. What about turning the
    FilterOperator into an array of conditions in which the first
    element is the operator? So, something like:
> 
> "filter": ["AND", {<FilterCondition>},
    {<FilterCondition>}, ["OR", {<FilterCondition>},
    {<FilterCondition>], ["NOT", ["OR", {<FilterCondition>},
    {<FilterCondition>}, {<FilterCondition>}]]]

The trouble with this is that it is difficult to represent in a typed language. It's not an array of a single type. It's not even an array of (String|FilterCondition) because the different types are only allowed to appear in certain positions. But it's not a tuple either (like the JMAP method calls), because it's variable length.

I don't think this brings enough benefits to outweigh this pain.

> Why is there now a separate emailTooLarge SetError in mail-07?

My thinking was the tooLarge error is about the object being created/updated (i.e. the MessageSubmission object in this case), and it's the related email that's actually too big. But really that's part of the MessageSubmission, so I have changed this to be just a standard tooLarge error again. Thanks for pointing this out.

Neil.