Re: [Last-Call] Genart last call review of draft-ietf-jmap-sharing-07

Neil Jenkins <neilj=40fastmailteam.com@xxxxxxxxxxxxxx> · Fri, 12 Apr 2024 13:12:55 +1000

Hi Susan,

Thank you for your review. I have published an updated draft 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).

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.
-- 
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call