Re: [secdir] [Jmap] secdir review of draft-ietf-jmap-quotas

Hello Wes,

Thank you for reviewing it again and addressing new feedback on this draft.

I will answer you inline as there is quite a bunch of comments.

Changes that I fixed on the new released version 11 of the draft:

- https://github.com/jmapio/jmap/pull/376/commits/7ebe04562ee591be4bcda77468bc45ab720631c7

- https://github.com/jmapio/jmap/pull/376/commits/4ff57bb602742c611b7d8e9cfab8ac5d16258ca5

Hope this will satisfy you, if not don't hesitate to give your feedback again!

Best regards,

Rene.

On Jan 12, 2023 5:06 AM, from Wes Hardaker 
I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the
IESG. These comments were written primarily for the benefit of the
security area directors. Document editors and WG chairs should treat
these comments just like any other last call comments.

Review summary: almost ready with issues

Apologies for my delay in getting to the new version of this document
(between vacation and a bad cold, I got behind in tasks).  Thank you for
the work you put into the new version; I find it much better than the
old and can see you took many suggestions (mine and others) into
account.

A few (mostly minor) thoughts on the new version:

- If it were me, I'd break the quota attributes section into it's own
  (becoming a new 4.1) starting with "The quota object MUST...".

+1. Fixed

- 4.1: document at section 5.1 -> document in section  5.1

+1. Fixed

- 4.3: any of which may be omitted -> any of which may be included or
  omitted

+1. Fixed

- 4.3: seems odd that the name attribute is the only one that isn't a
  list.

Not odd, there could be two filter conditions with a different name each under an operator (OR, AND) for example. However, scopes and resourceTypes should not be lists... More explanation below with your next concern

- 4.3: Larger issue wrt interoperability: My last note leads to the
  next: in the summary paragraph you state "A Quota object matches the
  FilterCondition if and only if all the given conditions match,
  including multiple array elements existing within a condition.", which
  I don't know how to interpret properly.  You say that all conditions
  match (which I'm sure means if both a scope and a resourceType are
  specified they both MUST match).  But the second part of the sentence
  leaves me confused about multiple array elements.  This would leave me
  to think that if you specified multiple resourceTypes in a list, then
  every type must match which should never be true so I doubt this is
  what you mean.  Maybe this is a good rewrite:

      A Quota object matches the FilterCondition if and only if all the
      given properties match (i.e. a logical and of all properties).
      For filter properties that are a list, at least one of the list
      elements must match for that property to be considered a match
      (i.e. a logical or of all the property's list element).

  But...  I am trying to figure out what you mean, and my interpretation
  may be wrong!

It wasn't logical indeed in that state. But rereading it again, and the jmap core spec, I actually think scopes and resourceTypes should not be lists, but single field. Scopes and resource types and well defined values by the server, and each quota can only have one. So if we need to fetch different accounts with different scopes and resource types, we can use different conditions and operators, they are here for that.

dataTypes is still a list though, as a quota can have multiple values. So it makes sense to want to search for quota that have all the values indicated in the filter condition. Thus the sentence makes sense as well now and shouldn't need change.

FYI I just realized when answering that I should drop the "s" from the scope(s) and resourceType(s) properties as they are not lists anymore. 

If you agree with that I will fix it and release it in a newer version (12). Just let me know if it would satisfy you first!

- For the example in section 5.2, I'd suggest actually using data that
  followed the previous example in a time-sequence.  Thus, if you
  changed the "sinceState" to "78540" to match the last value from the
  previous example, it would better show an example of commands over
  time.  (IMHO)

+1. Fixed

- The security section is improved (thank you), but there are some
  wording issues within it that need work:

    - "so he shouldn't know" -- I think you mean other users here
      shouldn't know.  So I'd change this to "so other users shouldn't
      know" or "no users should know".

+1. Fixed

    - The last sentence is hard to read as is.  I'd suggest the
      following replacement:

      In order to limit those attacks, quotas with "domain" or "global"
      scope SHOULD only be visible to server administrators and not to
      general users.

+1. Fixed

- I'm surprised you don't have an acknowledgment section, which
  customary to list all the people that help you put this specification
  together.  It's common but not required of course.

Actually I didn't know, as I didn't see any on other JMAP RFCs. But I looked again on other RFCs and I could see you were right... And I think it's a great idea. So I added one, thanks for the suggestion :)

-- 
Wes Hardaker
USC/ISI

_______________________________________________
Jmap mailing list
Jmap@ietf.org
https://www.ietf.org/mailman/listinfo/jmap