Re: [Jmap] Orie Steele's Discuss on draft-ietf-jmap-sharing-07: (with DISCUSS and COMMENT)

[Neil Jenkins <[e-mail]>]

Hi Orie,

Thank you for your review. I have published an updated draft <https://www.ietf.org/archive/id/draft-ietf-jmap-sharing-08.html> addressing your comments, as detailed below.

> The repeated '*' make this section difficult to read, I initially assumed this
> syntax indicated the field is not optional.
> 
> After reviewing RFC8620, it seems to simply be a "bolding" / "emphasis" effect.

That's correct. In the HTML version of the document I think this makes it clearer, but in the plain text version I agree it's more confusing. I would probably defer to whatever the RFC Editors think is the best option here. (Ideally in my mind, it keeps the bold for the HTML version but doesn't wrap it in the asterisks for the plain text, if that's possible.)

> This section (and similar ones) contain many instances of "String" which is
> defined in https://datatracker.ietf.org/doc/html/rfc8620#section-1.1
> 
> As has been discussed on the list regarding a related document:
> https://mailarchive.ietf.org/arch/msg/jmap/nEumbk7DKcV6AOg6q-7yD_qetU0/
> 
> Unicode / i18n processing of these fields is possibly non trivial...
> The document would be greatly improved by a section commenting on what kinds of
> JSON Strings / UTF-8 strings must remain supported. Examples in an appendix
> demonstrating interesting edge cases / unicode characters might help.

I have added an Internationalisation Considerations section, with the following text:

Experience has shown that unrestricted use of Unicode can lead to problems such as inconsistent rendering, users reading text and interpreting it differently than intended, and unexpected results when copying text from one location to another. Servers MAY choose to mitigate this by restricting the set of characters allowed in otherwise unconstrained `String` fields. The FreeformClass, as documented in [RFC7564 <https://author-tools.ietf.org/api/export/75dc1bfd-a43e-4f48-a1e9-9622161dd7aa/sharing.html#RFC7564>], Section 4.3 <https://rfc-editor.org/rfc/rfc7564#section-4.3> may be a good starting point for this.

Attempts to set a value containing code points outside of the permissible set can be handled in a few ways by the server. The first option is to simply strip the forbidden characters and store the resulting string. This is likely to be appropriate for control characters for example, where they can end up in data accidentally due to copy-and-paste issues, and are probably invisible to the end user. JMAP allows the server to transform data on create/update, as long as any changed properties are returned to the client in the `/set` response, so it knows what has changed, as per [RFC8620 <https://author-tools.ietf.org/api/export/75dc1bfd-a43e-4f48-a1e9-9622161dd7aa/sharing.html#RFC8620>], Section 5.3 <https://rfc-editor.org/rfc/rfc8620#section-5.3>. Alternatively, the server MAY just reject the create/update with an `invalidProperties` SetError.

> ```
> 310        *  *accountIds*: String[]
> 311           A list of account ids.  The Principal matches if any of the ids in
> 312           this list are keys in the Principal's "accounts" property (i.e.,
> 313           if any of the account ids belong to the principal).
> ```
> 
> Should this not be Id[] ? Is full UTF-8 equality check expected here?

It should indeed be `Id[]`, thanks for spotting, I've fixed this.

> ```
> 354        The server MAY limit the maximum number of notifications it will
> 355        store for a user.  When the limit is reached, any new notification
> 356        will cause the previously oldest notification to be automatically
> 357        deleted.
> ```
> 
> Why not SHOULD or MUST? What happens if there is no limit?

You could end up with a lot of notifications. This is not necessarily a problem as long as your server and client are well architected. The point of this text was mainly to make it permissible for servers to choose not to store all the notifications if it thinks it's not required.

> ```
> 359        The server MAY coalesce notifications if appropriate, or remove
> 360        notifications that it deems are no longer relevant or after a certain
> 361        period of time.  The server SHOULD automatically destroy a
> 362        notification about an object if the user subscribes to that object.
> ```
> Why not MUST? Should 362 say "unsubscribes" ?

The original intention here was "subscribe" — if the notification was telling you that you now have access to an object and the user subscribes to it, they know it exists and so don't need the notification. Upon reflection though, I think this is more something a user presentation issue, and something a client should handle (when subscribing, it could choose to destroy the notification), so I have just removed this sentence.

> ```
> 439        *  *objectAccountId*: String
> 440           The objectAccountId value must be identical to the given value to
> 441           match the condition.
> ```
> 
> Should this be `*objectAccountId*: Id` ?

Yes, thanks. Fixed.

> ```
> 173        The server MAY reject the user's attempt to subscribe to some
> 174        resources even if they have permission to access them, e.g., a
> 175        calendar representing a location.
> ```
> 
> Can this be strengthened to a SHOULD with clear conditions for when the server
> MUST allow subscription?

Not really, it's dependent on the implementation. For example, with JMAP calendars, the client MUST be allowed to set their own sort order and a few other properties on subscribed calendars, so if the server can't support this for some calendars (e.g. a calendar representing a room that's actually auto-generated from another backend booking system) it must reject the subscription attempt. While the calendars spec also notes the server may reject the subscription, I think it's worth having up front as a general thing here too so they can't be seen as in conflict.

> ```
> 177        A user may query the set of Principals they have access to with
> 178        "Principal/query" (see Section 2.4).  The Principal object may then
> 179        provide Account objects if the user has permission to access data for
> 180        that principal, even if they are not yet subscribed.
> ```
> 
> Can this be strengthened with normative language?
> For example:
> The Principle object MUST contain / provide when...
> The Principle object MUST NOT contain / provide when...

I have rewritten this as:

*The Principal object will contain an Account object for all accounts where the user has permission to access data for that principal, even if they are not yet subscribed.*

I don't think the normative MUST is appropriate here as this is simply a statement of the structure of the object. It should avoid any hint that there is any kind of choice here!

> ```
> 295        However, the server may reject this change, and probably will reject
> 296        any other change, with a forbidden SetError.  Managing principals is
> 297        likely tied to a directory service or some other vendor-specific
> 298        solution, and may occur out-of-band, or via an additional capability
> 299        defined elsewhere.
> ```
> 
> Can this language be strengthened?
> 
> For example: When the server rejects updates it MUST use a SetError of type
> forbidden as described in Section 5.3 of RFC8620.

I have rewritten this as:

*Managing principals is likely tied to a directory service or some other vendor-specific solution and may occur out-of-band, or via an additional capability defined elsewhere. Allowing direct user modification of properties has security considerations, as noted in **Section 5* <https://author-tools.ietf.org/api/export/f6ba9860-9013-488b-8553-0f520eccc727/sharing.html#security-considerations>*. Servers MUST reject any change it doesn’t allow with a *`*forbidden*`* SetError.*

> ```
> 314        *  *email*: String
> 315           Looks for the text in the email property.
> ```
> 
> I suggest a stronger reference for IDNA compatible email identifiers.
> There are many combinations of unicode characters that will not result in a
> valid email address. Consider: https://datatracker.ietf.org/doc/html/rfc6531 If
> this field is not required to be a well formed email (possibly one build of an
> IDNA), perhaps note that directly.

I have added to the "email" property definition:

*If given, the value MUST be conform with the "addr-spec" syntax, as defined in **[**RFC5322* <https://author-tools.ietf.org/api/export/4a43b11b-ad50-4bc3-90cd-0c00e02ce767/sharing.html#RFC5322>*], **Section 3.4.1* <https://rfc-editor.org/rfc/rfc5322#section-3.4.1>*.*

> ```
> 318        *  *text* String
> 319           Looks for the text in the name, email, and description properties.
> ```
> 
> The use of the word text is slightly misleading here without inclusion of the
> charset, since strictly speaking this is filtering on octets (encoding some
> UTF-8 string), correct?
> 
> Thanks to Yaron Sheffer for his comments to this effect in the SEC DIR review.

I have updated this wording to address Yaron's comments.

> ```
> 347        The server SHOULD create a ShareNotification whenever the user's
> 348        permissions change on an object.  It SHOULD NOT create a notification
> 349        for permission changes to a group principal, even if the user is in
> 350        the group.
> ```
> 
> Why not MUST? The way this is written, an implementation could easily decide to
> reverse these recommendations.

The SHOULD is in recognition that this API is often going to be an interface onto an existing directory management system (unlike most other JMAP APIs, where we can be more proscriptive).

> ```
> 614     5.  Security Considerations
> ```
> 
> Please consider adding security considerations regarding deceptive use of
> unicode characters, perhaps drawing from previous work, for example from:
> https://datatracker.ietf.org/doc/html/rfc5894#section-1.4
> 
> """
>    The introduction of the larger repertoire of characters
>    potentially makes the set of misspellings larger, especially given
>    that in some cases the same appearance, for example on a business
>    card, might visually match several Unicode code points or several
>    sequences of code points.
> """

I have added into the security considerations:

*Note that simply forbidding the use of a name already in the system is insufficient protection, as a malicious user could still change their name to something easily confused with the existing name by using trivial misspellings or visually similar unicode characters.***

> ```
> 639        The set of principals within a shared environment SHOULD be strictly
> 640        controlled.  If adding a new principal is open to the public, risks
> 641        include:
> ```
> 
> Why not MUST?

This should be a MUST, I've changed it.

> Is consent from existing principles required or recommended when adding new principles?

Not normally, no. Think of a standard office environment, where the principals correspond to employees.

> ## Nits
> 
> ```
> 536        "u12345678": {
> 537            "name": "[e-mail]"
> 538            "isPersonal": true
> 539            "isReadOnly": false
> 540            "accountCapabilities": {
> 541                "urn:com.example:jmap:todo": {},
> 542                "urn:ietf:params:jmap:principals:owner": {
> 543                    accountIdForPrincipal: "u33084183",
> 544                    principalId: "P105aga511jaa"
> 545                }
> 546            }
> 547        }
> ```
> 
> This example is malformed JSON. accountIdForPrincipal and principalId are
> missing quotes, and the there is no indication of elision, consider starting
> with { ... "u12345678": ... }

Thanks, I've fixed this.

> Its also strange that "name" is an email in this example

I used this because the account name is most commonly the email address of the owner of the account.

Cheers,
Neil.