Last Call Review of draft-ietf-jmap-sharing-07
review-ietf-jmap-sharing-07-genart-lc-hares-2024-04-09-00
Request | Review of | draft-ietf-jmap-sharing |
---|---|---|
Requested revision | No specific revision (document currently at 09) | |
Type | Last Call Review | |
Team | General Area Review Team (Gen-ART) (genart) | |
Deadline | 2024-04-01 | |
Requested | 2024-03-18 | |
Authors | Neil Jenkins | |
I-D last updated | 2024-04-09 | |
Completed reviews |
Secdir Last Call review of -07
by Yaron Sheffer
(diff)
Genart Last Call review of -07 by Susan Hares (diff) Opsdir Last Call review of -07 by Linda Dunbar (diff) |
|
Assignment | Reviewer | Susan Hares |
State | Completed | |
Request | Last Call review on draft-ietf-jmap-sharing by General Area Review Team (Gen-ART) Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/gen-art/nZz2fnG_Y5EFsQnoPvi1-q-_y7Y | |
Reviewed revision | 07 (document currently at 09) | |
Result | Ready w/issues | |
Completed | 2024-04-09 |
review-ietf-jmap-sharing-07-genart-lc-hares-2024-04-09-00
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.