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

[Susan Hares via Datatracker <[e-mail]>]

Reviewer: Susan Hares
Review result: Ready with Issues

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at
<https://wiki.ietf.org/en/group/gen/GenArtFAQ>.

Document: draft-ietf-jmap-sharing-07
Reviewer: Susan Hares
Review Date: 2024-04-09
IETF LC End Date: 2024-04-01
IESG Telechat date: 2024-04-18

Summary: The technical content seems to be complete.
My issue is that the English text provides several points that are unclear
enough for two implementations to interpret it differently. Since the purpose
of IETF specifications is to allow two different implementations of a protocol
to communicate, I consider this an issue.  The text formatting is difficult to
follow in .txt and .pdf formats. These issues and grammatical issues are listed
in the NITS.

Major issues: 0, Minor issues: 4, NITS: 14

Major issues: None
Minor issues: 4 minor issues requiring edits to clarify text

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./
------
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./
------
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./
-----
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./

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.
-------
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./
-----
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). /
-----
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.
 /
---
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./

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./

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.

The problem is probably in your XML.

Old Text example:/
   *  *id*: Id (immutable; server-set)
      The id of the principal.
   *  *type*: String
      This MUST be one of the following values:
      -  individual: This represents a single person.
      -  group: This represents a group of people.
      -  resource: This represents some resource, e.g., a projector.
      -  location: This represents a location.
      -  other: This represents some other undefined principal./
-----
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./
-----
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./

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?

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./

---
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.
-----
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). /
-----
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.