Subject: [Jmap] Review: draft-murchison-jmap-sieve-01

From: "Bron Gondwana" <[email-masked]>
Date: Wed, 08 Jul 2020 22:22:50 +1000

Hi Ken,

Figured I'd do this here so it's all public :)
*
*
*Open issues:*

/set#create should fail if there's already a script with the same name. The JMAP request can include a "destroy" for the ID of the old script, or it can issue an "update" on the Id of the script with the same name to replace the content.

I like the fail with "another script is active" as the JMAP way - otherwise you have action at a distance on another object. Again, the client can issue an isActive: false for the other Id as part of the same request.

I don't think we need a SieveScript/copy method. There's nothing like it in MANAGESIEVE and the amount of data is so small that it seems pointless.

Likewise, I don't think we need quotas if we have size limits and count limits on the number of scripts. We can still return an error to the client if we hit quota limits, but I don't see a client presenting quota information to the user about their sieve storage space as being a realistic use case.

*Specific sections:
*

   o  *sieveExtensions*: "String[]" A list of Sieve extensions (as
      listed in Sieve "require" action [RFC5228], Section 3.2) supported
      by the Sieve engine.

It was unclear from me reading RFC5228 whether extension names are case significant, but for the avoidance of doubt we should clarify this here.

   o  *content*: "String" The Sieve code in the script.  Note that any
      double (") quote or backslash (\) characters appearing in the
      script content MUST be escaped by prefixing them with a backslash
      (\).

This needs clarity around what quoting is required on top of any quoting required by JSON. Is the backslash because of JSON, or is there an additional level of backslashery required as well, and if so why? It should be that passing the same octets to a MANAGESIEVE client for its quoting or a JSON encoded for JMAP encoding works the same.

2.1.  SieveScript/get

   This is a standard "/get" method as described in [RFC8620],
   Section 5.1.  The _ids_ argument may be "null" to fetch all at once.

There probably should be an example of using that with "properties": ["name", "isActive"] in order to work out which script is active without needing to pull all the contents.

2.2.  SieveScript/set

The discussion should cover whether updating the content is legal (per my earlier comments about replacing a script).

2.3. SieveScript/validate
 o *errorDescription*: "String" A description of the error to show to
 the user, or an empty string if the Sieve code is valid.

Is there a reason why this is empty string rather than NULL?

3.  Security Considerations

This needs to refer to the security considerations of RFC5228 as well!

*General overall comments:**
*

There's a couple of things I'd love. One is being able to reference sieve scripts as blobs with a blobId. This would allow them to be attached directly to emails, uploaded and downloaded without encoding troubles, etc. I have a draft that I should write up for Blob/set and Blob/info which allow for both setting blobs as part of a request to avoid a separate upload, and getting reverse index information about where blobs are referenced.

Another thing is a SieveScript/test endpoint which takes a sieveScriptId (or blobId of a sieve script to avoid uploading it every time without needing to name it!) and an emailId and returns a set of actions that would be taken if that email was processed with that sieve script. This is different enough that it might merit a separate capability called 'sievetest' or something, and will definitely want rate limiting!

Is it worth having a SieveScript/query? It probably on needs "name" and "isActive" filters. I mean, sorts as well if you wanted to get fancy and allow a client to manage thousands of these things.

Bron.

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

---

From: "Ricardo Signes" <[email-masked]>
Date: Mon, 13 Jul 2020 16:08:28 -0400

> /set#create should fail if there's already a script with the same name. The JMAP request can include a "destroy" for the ID of the old script, or it can issue an "update" on the Id of the script wi…

Well, you already know this doesn't get me what I want, but I have a suggestion. :-) First, my complaints:

This leads to more HTTP transactions than is otherwise necessary.

Say that from a position of ignorance of the remote state, I want to set my active Sieve script to the document S. This requires this set of interactions:

• HTTP request 1: SieveScript/get
• locally extract the id of the active script
• HTTP request 2: SieveScript/set { update: { activeId: { script: $NEW, isActive: true } } }

This is an extra round trip. (So the re-set of isActive at the end is to avoid a race. An ifInState would also do the trick.)

This would not be required in managesieve, where by convention the client could control the script name and know that a PUTSCRIPT for the conventional name would update the active script. The shift from client-controlled keys (script names) to server-controlled keys (JMAP ids) eliminates the possibility for this kind of convention. 

You suggest that the client can perform either a destroy-and-create or an update but I think that *only the update-based operation is safe*. Consider:

• client does a SieveScript/get and determines the active id script is "123"
• client issues SieveScript/set { destroy => [ "123" ], create => { newScript => { …something invalid… } } }

Because the create and destroy may be applied independently, the user could end up with no active script at all. This is too bad, because otherwise you could provide SieveScript/query (which would need to be invented), backreference the results for (filter:{isActive:true} OR name:NameWeWant) in a destroy, and create the replacement in the same breath, which would be safe. 

*I suggest instead*:

1. We drop isActive as a property from SieveScript.
2. We create a new type, SieveConfig
3. SieveConfig has exactly one instance per account, with the fixed id "singleton"
4. The SieveScriptConfig object has one property, activeScriptId, which is either a script id or null
5. If no "name" is provided, the server will provide a guaranteed unique name.

To replace the active Sieve script in the blind, you do this:

SieveScript/set { create: { newScript: { content: "..." } } }
SieveConfig/set { update: { "singleton": { activeScriptId: "#newScript" } } }

This is guaranteed safe. The worst case is that you end up with a uniquely-named script that is not, in fact, active.

We probably also need:

6. If the active script is deleted, the SieveScriptActive activeScriptId becomes null.

Alternatively, it could be illegal to destroy the active script.

*As for everything else…*

I agreed with all the rest of your points.

If we add SieveScript/query as you suggest, then this pair of methods would get tacked onto my couplet above:

SieveScript/query { filter: { isActive: false } }, A
SieveScript/set { "#destroy": { backreference-to-A-list } }

-- 
rjbs

---

From: Ken Murchison <[email-masked]>
Date: Wed, 29 Jul 2020 09:49:11 -0400

On 7/13/20 4:08 PM, Ricardo Signes wrote:
> This is guaranteed safe.  The worst case is that you end up with a 
> uniquely-named script that is not, in fact, active.
>
> We probably also need:
>

As an alternative to using a new data type for setting the active 
script, we could make isActive a server-set property (not directly set 
by the client within the SieveScript/set{create|update} method) and add 
a new onSuccessActivateScript argument to the SieveScript/set method 
which only gets processed if ALL create|update|delete are successful.

-- 
Kenneth Murchison
Senior Software Developer
Fastmail US LLC