As before, Thanks S On 21 Jan 2012, at 02:53, Eran Hammer <[email protected]> wrote:
>> -----Original Message----- >> From: [email protected] [mailto:[email protected]] On Behalf >> Of Stephen Farrell >> Sent: Thursday, October 13, 2011 10:13 AM > >> Original list of nits: >> ---------------------- >> >> - Intro: Maybe add some ascii-art showing the roles of the user, browser, >> client etc. The term client as used here is still commonly confusing and >> especially worth going the extra mile to clarify, before it is first used. >> What I >> think needs to be said early is that what is here called a client is normally >> (running on) a web server. > > Happy to take suggestions, but can't come up with a useful diagram myself. > > Added to the client definition: > > The term client does not imply any particular implementation > characteristics (e.g. whether the application executes on a > server, a desktop, or > other devices). > >> - p4: Maybe s/a string/an octet string/ in the token descriptions, unless the >> access token encoding is constrained to what'd be understood as a string. > > Just a string. > >> - p8: Seems odd to speak of "issuing" an implicit grant - wouldn't that make >> something explicit? Maybe say "With an implicit grant..." >> instead in the 2nd para of 1.3.2? > > Changed to 'When issuing an access token during the implicit grant flow'. > >> - p8: I don't get what "its device operating system" means. Do you mean the >> client is an already-trusted part of the resource owner's device OS? > > Changed to 'the client is part of the device operating system'. > >> - p9 - "Issuing a refresh token is optional" - might be better to say that >> it's the >> authorization server's choice and there's no way for the client to signal a >> request for one. > > Changed to 'Issuing a refresh token is optional at the discretion of the > authorization server.' > >> - p10 - In figure 2, why is the Refresh token shown as optional in step (H) >> but >> not in step (B)? I think it'd be clearer for the figure to reflect the >> protocol >> options and say that the refresh token is optional in (B) as well. > > Because this is the refresh token flow. If it is optional, there is no flow... > >> - p12 - the confidential/public terminology isn't great, did the WG consider >> "authenticating" vs. "non-authenticating"? Trying again to find better terms >> would maybe be worthwhile. > > Didn't try this particular alternative, but it doesn't flow off my tongue. > >> Also, it may be worth explicitly saying that it >> doesn't matter how hard to guess a secret the client has nor how good a >> MAC algorithm you use with that secret - if anyone can get the secret then >> the client is still public. It nearly says that, but not quite and given >> that many >> developers just don't apparently still think that a hardcoded secret >> embedded into a binary is good enough, I'd argue its worth adding a bit more >> here. > > This seems to be cross the line as to what the server defines as > confidential, but I'm happy to take text proposals. > >> - p12/13 in the application profile descriptions - is "resource owner's >> device" >> correct? That seems to rule out kiosks, which may be correct, but which isn't >> obvious. Maybe you mean "device being used by the resource owner" with >> no implication as to ownership of the device? > > Changed to 'the device used by the resource owner' throughout. > >> - p13 - client application: typos: >> >> s/such access tokens/such as access tokens/ >> >> s/which...interact with/with which the application may interact/ > > Ok. > >> - p13, "establishing trust with the client or its developer" is badly >> phrased, I >> think you mean the AS SHOULD NOT just accept the client type unless it has >> some external reason to do so. Trusting the developer might be one such. >> Being paid is another, and maybe more likely;-) > > Changed to: > > The authorization server SHOULD NOT make assumptions about the > client type, nor accept the > type information provided by the client developer without first > establishing trust. > >> - p13, 2.3 has 2119 language both about the things established at >> registration >> time and things done in the protocol defined here - would it be better to >> identify those separately in different sections or maybe move the >> registration time stuff into 2.2 with a possible renaming of 2.2. > > Tweaked a bit, but overall it reads fine to me. > >> - 3.1.2.1 has a SHOULD for TLS which I think generated a lot of mail on the >> list. >> I think saying why this is not a MUST would be useful, since it's the kind of >> thing that might get revised later on if the situation changes. > > This specification does not mandate the use of TLS because at the > time of this writing, requiring clients to deploy TLS is a > significant hurdle for most > client developers. > >> - Figure 3, (p21) has multiple lines labeled A,B & C - saying why might >> reduce >> some confusion. Or maybe, say that the labels reflect rough event timing or >> something. It'd also be good if subsequent sections said which parts of >> figure >> 3 they're addressing, e.g. >> 4.1.1 maps to (A), right? It's hard to tell. > > Added clarification about the broken lines. > >> -p25, 4.1.3, what does "assigned" "authentication requirements" >> mean? Suggest deleting the parenthetical clause since "confidential client" >> should be sufficiently well-defined to cover that. > > Ok. > >> -p28, the description of step (D) isn't clear to me - who does what with the >> URI fragment? > > Seems pretty clear to me. > >> -p30, why refer to "HTTP client implementations" when you're previously >> said UA? If there's a substantive difference it'd be good to be clear about >> that. Same para: s/such client/such clients/ > > Changed to UA. > >> -p33 - Just checking - 4.3.1 says the client MUST discard the credentials >> once >> an access token has been obtained - why not before though, e.g. once an >> access token has been requested? Is there a re-tx thing that the client >> might >> do? > > Nope. > >> -p38, is "token_type":"example" a valid value? If not, better to use one that >> is. > > I refuse to use Bearer as an example and I expect others to take issue with > using MAC :-) > >> -p40, s/client it was issued/client to which it was issued/?' > > Ok. > >> -p40, s/require/REQUIRE/ in the 2nd last bullet on the page > > Require isn't an uppercase word (REQUIRED is). > >> -p43, s/native clients may require/native clients require/ I'd say the "may" >> is >> worth deleting both to avoid 2119 language and because we do know that >> these require special consideration. > > Ok. > >> -p46, s/such malicious clients/such potentially malicious clients/? >> Not all unauthenticated clients are bad, though all of them could be bad. > > Ok. > >> -p47, s/Access token (as well as.../Access tokens (as well as.../ > > Ok. > >> -p50, 10.8 seems to just repeat stuff from earlier in 10.3 & 10.4, is that >> deliberate? > > The requirements are but the context is different. > > This concludes this review and all other -22 feedback. > > EHL > <winmail.dat> _______________________________________________ OAuth mailing list [email protected] https://www.ietf.org/mailman/listinfo/oauth
