Re: [Gen-art] Genart last call review of draft-ietf-jmap-sharing-07

[Neil Jenkins <[e-mail]>]

Hi Susan,

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, details below.

> 1. Section 1.5.1, paragraph 1
> Why: Sentence structure makes the reference unclear
> 
> Old text:/
>    Represents support for the Principal and ShareNotification data types
>    and associated API methods. /
> 
> New text:/ The property "urn:ietf:params:jmap:principals" indicates
>   support for the Principal and ShareNotification data types
>   and associated API methods./

I have rewritten this to:

The `urn:ietf:params:jmap:principals` capability represents support… [etc]

> ------
> 2. Section 1.5.2, paragraph 1
> Why: Text is unclear as which URI and what is supported.
> Old text:/
>    This URI is solely used as a key in an account’s accountCapabilities
>    property; it does not appear in the JMAP Session capabilities.
>    Support is implied by the urn:ietf:params:jmap:principals session
>    capability.
> 
>    If present, the account (and data therein) is owned by a principal.
>    Some accounts may not be owned by a principal (e.g., the account that
>    contains the data for the principals themselves), in which case this
>    property is omitted. /
> 
> New text:/
>    The URI urn:ietf:params:jmap:principals:owner is solely used as a
>    key in an account’s accountCapabilities property, so it does not
>    appear in the JMAP Session Capabilities. A urn of
>    "urn:ietf:params:jmap:principals session capability" indicates support for
>    this capability.
> 
>    If this urn is present, the account (and data therein) is owned by a
>    principal. Some accounts may not be owned by a principal (e.g., the account
>    that contains the data for the principals themselves), in which case this
>    property is omitted./

I have rewritten this to:

The URI `urn:ietf:params:jmap:principals:owner` is solely used as a key in an account’s accountCapabilities property. It does not appear in the JMAP Session capabilities — support is indicated by the `urn:ietf:params:jmap:principals` URI being present in the session capabilities.

If `urn:ietf:params:jmap:principals:owner` is a key in an account’s accountCapabilities, that account (and data therein) is owned by a principal. Some accounts may not be owned by a principal (e.g., the account that contains the data for the principals themselves), in which case this property is omitted.

> ------
> 3. section 2.3, paragraph 3
> Why: Not clear how this fits in with the first 2 paragraphs.  Below
> I have given text based on the fact the changes are properties of
> Principal. However, the authors should validate.
> 
> old text:/
>    However, the server may reject this change, and probably will reject
>    any other change, with a forbidden SetError.  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./
> New text:/
>    However, the server may reject these changes to the properties of Principal
>    with a response of forbidden SetError. Such servers will probably reject
>    any other change with a forbidden SetError.  Managing principals is
>    likely tied to a directory service or some other vendor-specific
>    solution.  This management may occur out-of-band or via an additional
>    capability defined elsewhere./

This section has come up in all the reviews I think! I have rewritten this as follows:

Managing principals is likely tied to a directory service or some other vendor-specific solution. This management may occur out-of-band, or via an additional capability defined elsewhere. Allowing direct user modification of properties has security considerations, as noted in [Security considerations section]. Servers *MUST* reject any change it doesn’t allow with a `forbidden` SetError.

Where a server does support changes via this API, it *SHOULD* allow an update to the "name", "description" and "timeZone" properties of the Principal with the same id as the "currentUserPrincipalId" in the Account capabilities. This allows the user to update their own details.

> 4. section 4, paragraph 4
> Why issue: Unclarity of section may lead to
> issues in the security processing.
> 
> Old text:/
>    *  *shareWith*: Id[String[Boolean]]|null
>       A map of principal id to rights to give that principal (in the
>       same format as the myRights property), or null if not shared with
>       anyone.  The account id for the principal id can be found in the
>       capabilities of the Account this object is in (see Section 1.5.2).
>       Users with appropriate permission may set this property to modify
>       who the data is shared with.  The principal that owns the account
>       this data is in MUST NOT be in the set of sharees; their rights
>       are implicit./
> New text:/
>    *  *shareWith*: Id[String[Boolean]]|null
> 
>       The "sharewith" attribute provides a map of principal id to rights
>       to give each principal id listed (in the same format as the
>       myRights property), or null if not shared with anyone.  The
>       account id for the principal id can be found in the
>       capabilities of the Account this object is in (see Section 1.5.2).
>       Users with appropriate permission may set this property to modify
>       who the data is shared with. The principal id that owns the account
>       this data is in MUST NOT be in the set of shareWith" since the owner's
>       rights are implicit./

I have tried to clarify this as follows:

The value of this property is null if the data is not shared with anyone. Otherwise, it is a map where each key is the id of a principal with which this data is shared, and the value associated with that key is the rights to give that principal, in the same format as the `myRights` property. The account id for the principal id can be found in the capabilities of the Account this object is in (see Section 1.5.2 <https://author-tools.ietf.org/api/export/9040c978-97f1-4c79-b7a2-016ae7902356/sharing.html#urn-ietf-params-jmap-principals-owner>).

Users with appropriate permission may set this property to modify who the data is shared with. The principal that owns the account this data is in MUST NOT be in the set of sharees since the owner's rights are implicit.

> Nits/editorial comments:
> 1. Abstract
> Old text:/ Future documents can reference this one when defining
>    data types to support a consistent model of sharing./
> New text:/  Future documents can reference this document when defining
>    data types to support a consistent model of sharing./
> Why: Unclear what "this one" is in the text.

Adopted.

> -------
> 2. section 1.4, paragraph 1
> reason: English Grammar's definition of semi-colon
> 
> Old text:/ Clients will often want to differentiate
>    the two; for example, a company may share mailing list archives for
>    all departments with all employees, but a user may only generally be
>    interested in the few they belong to./
> 
> New text:/ Clients will often want to differentiate
>    the two. For example, a company may share mailing list archives for
>    all departments with all employees, but a user may only generally be
>    interested in the few they belong to./

Adopted.

> -----
> 3. Section 1.4, paragraph 3.
> Why:  The example is not part of the normative text.
> Old text:/The server MAY reject the user's attempt to subscribe to some
>    resources even if they have permission to access them, e.g., a
>    calendar representing a location./
> Next text:/   The server MAY reject the user's attempt to subscribe to some
>    resources even if they have permission to access them, (e.g., a
>    calendar representing a location). /

Adopted.

> -----
> 4. Section 1.5.1, paragraph 4
> 
> Why: formatting issues
> Old text:/ *  *currentUserPrincipalId*: Id|null
>       The id of the principal in this account that corresponds to the
>       user fetching this object, if any./
> 
> New text:/*currentUserPrincipalId*: Id|null
> 
>       The id of the principal in this account which corresponds to the
>       user fetching this object, if any./
> -------
> 5. section 1.5.2 - formatting
> Reason: Why are there two * * in ASCII Text and odd spacing in pdf?
> Old text:/
>   *  *accountIdForPrincipal*: Id
>       The id of an account with the urn:ietf:params:jmap:principals
>       capability that contains the corresponding Principal object.
>    *  *principalId*: Id
>       The id of the Principal that owns this account.
> /

This is an issue with the plain text version of the document — the first asterisk is because this is a bullet list (there's only one item in the list, but it's consistent with how properties are defined throughout the document), and then the bold text used for the property name causes it to also wrap that in asterisks. I will ask the RFC editors what they recommend for formatting here.

> 6. Section 2, paragraph 1.
> Why: Run on sentence without purpose.
> Old text:/ Sharing in JMAP is generally configured by assigning
>    rights to certain data within an account to other principals, for
>    example a user may assign permission to read their calendar to a
>    principal representing another user, or their team./
> 
> New text:/ Sharing in JMAP is generally configured by assigning
>    rights to certain data within an account to other principals. For
>    example, a user may assign permission to read their calendar to a
>    principal representing another user or their team./

Adopted.

> 7. Section 2, paragraph 3
> Why: Run-on Sentence and unclear context.
> Old text:/In most systems the user will have access to a single Account
>    containing Principal objects, but they may have access to multiple
>    if, for example, aggregating data from different places./
> 
> New text:/   In most systems, the user will have access to a single Account
>    containing Principal objects, but they may have access to multiple portions
>    of it. For example, clients aggregating data from different places./

I have rewritten as:

In most systems, the user will have access to a single Account containing Principal objects. In some situations, for example when aggregating data from different places, there may be multiple Accounts containing Principal objects.

> 7. Section 2, 2.4.1, 3.2, 3.6.1, 4, and 4.1 in descriptions of objects
> 
> In ASCII text, the text has multiple * per defined value.
> In PDF version, the text requires formatting.
> Please check your original text and fix so ASCII and PDF are correct.

(Same as 4/5 above.)

> 8. section 2.2, paragraph 1
> why: English errors relating to comma
> 
> Old text:/  Note, implementations backed by an external directory
>    may be unable to calculate changes, in which they will always return
>    a "cannotCalculateChanges" error, as described in the core JMAP
>    specification./
> New text:/ Note: implementations backed by an external directory
>    maybe unable to calculate changes, and in this case, they will always return
>    a "cannotCalculateChanges" error as described in the core JMAP
>    specification./

Adopted.

> 9. Section 2.5
> Why: Sentence clarity
> 
> Old text:/ Note, implementations backed by an external directory
>    may be unable to calculate changes, in which they will always return
>    a "cannotCalculateChanges" error, as described in the core JMAP
>    specification./
> 
> New text:/ Note: implementations backed by an external directory
>    may be unable to calculate changes.  In this case, they will always return
>    a "cannotCalculateChanges" error, as described in the core JMAP
>    specification./

Adopted.

> 10. Section 3.2, formatting of objections
> What is issue: In section 3.2 your objects use a format of:
> 
>    *  *id*: String (immutable; server-set)
> 
> It is normal to use the format:
>     * id: String (immutable, server-set)
> 
> Why are you using your format?

(Same as 4/5 above.)

> 11. section 3.2, last paragraph
> 
> Old text:/Determining the name will depend on the data type in question, for
>       example it might be the "title" property of a CalendarEvent or the
>       "name" of a Mailbox, and is implementation specific.  The name is
>       to show to users who have had their access rights to the object
>       removed, so that they know what it is they can no longer access./
> 
> New text:/ Determining the name will depend on the data type in question.
>       For example, it might be the "title" property of a CalendarEvent or the
>       "name" of a Mailbox. The name is to show to users who have had
>       their access rights to the object removed, so that these users
>       know what it is they can no longer access./

Adopted.

> ---
> 12. Section 4, IsSubscribed definition
> Why: Starting the definition off with a question creates a vague
> understanding for people not involved in the writing of the specification.
> 
> Suggested change:
> Old text:/
>   *  *isSubscribed*: Boolean
>       Has the user indicated they wish to see this data?  The initial
>       value for this when data is shared by another user is
>       implementation dependent, although data types may give advice on
>       appropriate defaults./
> 
> New text:/
>   *isSubscribed: Boolean
>       The value true indicates the user wishes to subscribe to see this data.
>       The value false indicates the user does not wish to subscribe to see
>       this data.  The initial value for this variable when data is
>       shared by another user is implementation dependent, although data types
>       may give advice on appropriate defaults.
>      /
> 
> If you accept this change, please also change the example in section 4.1.

Adopted.

> -----
> 13. Section 5.2
> Why: definition of e.g. (For example) and spelling error
> 
> Old text:/Sharing data with another user allows someone to turn a transitory
>    account compromise (e.g., brief access to an unlocked, logged in
>    client) into a persistant compromise (by setting up sharing with a
>    user controlled by the attacker)./
> 
> New text:/ Sharing data with another user allows someone to turn a transitory
>    account compromise (e.g., brief access to an unlocked or logged-in
>    client) into a persistent compromise (by setting up sharing with a
>    user-controlled by the attacker). /

Adopted.

> 14. Section 5.3, paragraph 1
> 
> Why: Normally, lists use a "," or a ";" to separate the clauses.
> In addition, the context usually suggests "and" or an "or" in the
> list.
> 
> Please consider whether you should follow this list format in this section.

I think the current format is fine, and don't believe this change would increase clarity here.

Cheers,
Neil.