Re: [OAUTH-WG] Draft 20 last call comments

[Justin Richer <[e-mail]>]

Comments inline.

On Wed, 2011-08-17 at 14:15 -0400, Eran Hammer-Lahav wrote:
> Thanks for the feedback.
> 
> > -----Original Message-----
> > From: [e-mail] [mailto:[e-mail]] On Behalf
> > Of Justin Richer
> > Sent: Thursday, August 11, 2011 2:07 PM
> 
> > 1.2/1.4: The term "authorization grant" remains confusing and the
> > introduction is riddled with jargon like "intermediate credential". With the
> > diagram in 1.2, it appears to be an explicit technological underpinning of the
> > protocol flow instead of a general conceptual construct that is used in several
> > different ways. Basically, what "authorization grant" *means* is not obvious
> > within this document.
> >
> > Section 4 makes much more sense than the introduction text does here.
> > Perhaps we should just replace most of 1.4 with just the introductory text to
> > 4 (perhaps slightly expanded), and then a reference to the sub-parts of
> > section 4 for the meat of the concept (and in the process, nix the subsections
> > of 1.4 entirely).
> > 
> > 1.2(B): "Provided" is wrong here (it implies a direct hand-over), and the last
> > sentence is awkward. Suggest reword to:
> >         The client receives an authorization grant which represents the
> >         authorization granted by the resource owner.  The type of
> > 	authorization grant is dependent on which methods are supported
> > 	by both the client and authorization server.
> 
> Change (B) in 1.2 to:
> 
>               The client receives an authorization grant which is a credential representing
>               the resource owner's authorization, expressed using one of four grant types defined
>               in this specification or using an extension grant type. The authorization grant type
>               depends on the method used by the client to request authorization and the types
>               supported by the authorization server.
>
> And changed 1.4 to:
> 
>           An authorization grant is a credential representing the resource owner's authorization
>           (to access its protected resources) used by the client to obtain an access token. This
>           specification defines four grant types: authorization code, implicit, resource owner
>           password credentials, and client credentials, as well as an extensibility mechanism for
>           defining additional types.

Much better for both overall though.


> > 1.3/1.4/1.5: Consider switching order to Authorization Grant, Access Token,
> > Refresh Token
> 
> Not sure. What do others think? I put access token first because it is a more important term to get out of the way.

I agree that access tokens are a more important topic to OAuth overall,
but the rest of the document presents things in protocol flow order: you
get the auth grant, then you get the tokens.

> > 1.4.1: We probably want to mention a user agent here in the exposure risk at
> > the end. Since that's really the problem -- the browser could steal the token,
> > not the end user.
> 
> Proposed text?

   The authorization code provides a few important security benefits
   such as the ability to authenticate the client and issuing the access
   token directly to the client without potentially exposing it to
   others, including the resource owner or other applications in the resource
   owner's user agent.

(Still not good wording... but the idea is to point out that you the
leak possibility here stems from using the front channel.)

> > 1.4.2: Still don't like the term "implicit". It's misleading. Even "direct
> > authorization" would be better, though still not ideal.
> 
> It's the best we've got. "Direct authorization" is not a grant type, but a flow description.

Fair enough. I shall remain a conscientious objector. :)

> > 1.4.5: Throw a simple reference to 8.3 here?
> 
> No forward references whenever possible.

Fair style point, but I think this one is worth it. This is why our
documents have hyperlinks.



For each of 1.4.x, I'd still rather see the 1.4.x sections just go away.



> > 2: Isn't "means" generally treated as singular in instances like this?
> > Thus "means ... is" instead of "means ... are".
> 
> Don't know.

I think it is, unless someone can put a stake in to say that's wrong.

> > 2.1/2.2: The requirements (2.2) should go first in section 2. The client types
> > are part of deciding the requirements, and the concepts flow better this way.
> 
> You need to first define client types before you can require it.

No you don't, you just need to reference them. You already don't define
the other requirements until after (such as the redirection urls). I
think it reads better to have the requirements up front, when the full
matter of what they're all about in the following sections. The client
As it is now, there are both forward and backward references in the
requirements paragraph and it's confusing to read like that.

> > 2.1: I like the calling out of the types of clients, it helps cement things.
> > 
> > 2.3: Suggest renaming to "Client Identification" to parallel "Client
> > Authentication" in 2.4
> 
> It's not about identification.

Then why is it called "Client Identifier"? Usually, one uses an
identifier for identification. :) Really, this comment was about
parallelizing the language in the two subsequent headers: Identification
and Authentication.

> > 2.4.2: Want to mention the MAC binding as an example here? This would
> > parallel the OAuth2 method of signing the fetch for a request token more
> > directly.
> 
> Nah.

Let me put it stronger: I would like to see an explicit reference to the
MAC binding here as an example of a stronger auth binding, along with an
example. OAuth1 allowed for binding against the token endpoint using a
client secret that was not passed across the wire, and the MAC spec
gives that capability to OAuth2. I would really like to see that.

I see no problem in the core document pointing out the MAC and Bearer
documents as prime examples where appropriate.



 -- Justin