Add a package for authenticating via OAuth2.

Add a package for authenticating via OAuth2.

Committed: https://code.google.com/p/dart/source/detail?r=15115

[Bob Nystrom, 2012-11-16 19:53:30]

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/oauth2.dart
File pkg/oauth2/lib/oauth2.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/oauth2.dart...
pkg/oauth2/lib/oauth2.dart:33: ///         new
Uri.fromString("http://example.com/oauth2/token");
Nit, but I wouldn't bother to type annotate these (and below).

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/oauth2.dart...
pkg/oauth2/lib/oauth2.dart:85: ///         // parameters the the
AuthorizationCodeGrant. It will validate them
"the the" -> "to the"

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/oauth2.dart...
pkg/oauth2/lib/oauth2.dart:102: ///     }).then(print);
Wonderful example. Nicely done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
File pkg/oauth2/lib/src/authorization_code_grant.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:9: import
'../../../http/lib/http.dart' as http;
Add TODO to use "package:" here. Mention bug 6745.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:29: class
AuthorizationCodeGrant {
The state enums and all of the StateErrors here are a bit of a code smell to me.
It feels like this is actually two classes masquerading as one. You could split
it into:

AuthorizationCodeGrant (_INITIAL_STATE)
- has getAuthorizationUrl() which returns a AuthorizationResponseHandler

AuthorizationResponseHandler (_AWAITING_RESPONSE_STATE)
- has handleResponse()

This might make things a little less error-prone. Of course, fewer types is good
too though. Thoughts?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:30: /// An enum value for
[_state] indicating that [getAuthorizationUrl] has
Nittiest of nits, but it looks like most of these comments could be word-wrapped
at 80 columns more precisely. Do that if you want, but it's fine if you don't
too.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:32: static final int
_INITIAL_STATE = 0;
"static final int" -> "static const". Here and elsewhere.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:77: final http.BaseClient
_httpClient;
Using BaseClient as a type annotation here and elsewhere makes sense, but it
makes me wonder if the type names could be better in http. It seems like you'll
mention clients in type annotations more often than in constructor calls, so I
wonder if it should be something like:

  BaseClient -> Client
  Client -> DefaultClient (or some other name here)

I worry that otherwise people will just annotate to take Client and not realize
they've made their type annotation incorrectly too specific. Thoughts?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:106: /// Returns the URL to
which the resource owner should be redirected to
"redirected" -> "directed"?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:108: /// `redirect`, which
should point to a server controlled by the client. This
backquotes -> square brackets. You can (and should) use [foo] for parameters
too. Here and elsewhere.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:122: /// It is a [StateError]
to call this more than once for a given instance of
Remove the "for a given..." part. That should be implied since this is an
instance method. :)

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:147: /// server. Note that this
"response" is not an HTTP response, but rather the
Maybe: "...rather the data passed to the redirect server as query parameters on
the redirect URL."

?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:163: throw new StateError('The
authorization URL has not yet been generated.');
Long line.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:165: throw new StateError('The
authorization code has already been received.');
Ditto.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:169: if (this._stateString !=
null) {
Is the "this." needed here?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:173: '"state".');
How about:

               '"$authorizationEndpoint": parameter "state" expected to be '
               '"$_stateString", was missing.');

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:174: } else if
(parameters['state'] != this._stateString) {
"this."?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:213: throw new StateError('The
authorization URL has not yet been generated.');
Long lines.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:217: _state = _FINISHED_STATE;
Move this into _handleAuthorizationCode so it's not duplicated in both
codepaths?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:248: /// continue using the
client.
Ugh, this makes me wonder if we should be ref-counting here, but that just makes
me feel dirty.

Maybe check the state and only close the client if you're not in
_FINISHED_STATE? And then document that?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
File pkg/oauth2/lib/src/authorization_exception.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_exception.dart:10: class AuthorizationException
{
extends Exception

Or implements?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_exception.dart:16: /// The description of the
error, provided by the server. This is optional.
Will it be `null` or `""` if not provided?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_exception.dart:34: return '$header.';
I think I'd prefer to rely on the description having it's own period. Leave this
off?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/client....
File pkg/oauth2/lib/src/client.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/client....
pkg/oauth2/lib/src/client.dart:23: /// the credentials are expired and can't be
refreshed.
This shouldn't be a StateError, I think. StateErrors should never be caught and
this seems like an error you may want to handle. Make an ExpirationException
type, or use AuthorizationException for this?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/client....
pkg/oauth2/lib/src/client.dart:93: /// This will throw a [StateError] if the
[Credentials] can't be refreshed, an
Throwing a StateError here is OK if it's something the programmer can check for
programmatically and prevent. Is the idea that I need to do if
(client.credentials.canRefresh) before calling this?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/client....
pkg/oauth2/lib/src/client.dart:100: Future<Client>
refreshCredentials([List<String> newScopes]) {
Any particular reason to complete with the client? Just convenience?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
File pkg/oauth2/lib/src/credentials.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:25: /// to save the refreshed credentials
for future use.
Given this, maybe client should raise some sort of event/notification/stream
when it has refreshed the credentials so you can save them?

I know the normal case is that you just save once right before exiting, but I
worry about the case where your app dies after refreshing but before you've had
a chance to save them.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:40: /// be specified via `scopes`. The scope
strings are specific to the
Just me, but I'd probably italicize scopes and not code font.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:74: if (tokenEndpoint != null) tokenEndpoint
= new Uri.fromString(tokenEndpoint);
Long line.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:81: parsed['accessToken'],
Throw a FormatException if this is missing?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:133: // preference to form parameters, but
Google doesn't support that. Should
Long line.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
File pkg/oauth2/lib/src/handle_access_token_response.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
pkg/oauth2/lib/src/handle_access_token_response.dart:19: final int
_EXPIRATION_GRACE = 10;
"final int" -> "const".

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
pkg/oauth2/lib/src/handle_access_token_response.dart:44: } catch (e) {
Restrict this:

} on FormatException catch (e) {

You really don't want to catch NoSuchMethod or OutOfMemory here. :)

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
pkg/oauth2/lib/src/handle_access_token_response.dart:46: '"$tokenEndpoint":
invalid JSON.\n\n${response.body}');
How about a helper function for all of these throws:

validate(bool condition, String message) {
  if (!condition) throw new FormatException('Invalid OAuth response for '
       '"$tokenEndpoint": $message.\n\n${response.body}');
}

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.dart
File pkg/oauth2/lib/src/utils.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.d...
pkg/oauth2/lib/src/utils.dart:27: var value = urlDecode(split.length > 1 ?
split[1] : "");
var value = split.length > 1 ? urlDecode(split[1]) : "";

Not much point in decoding "". :)

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.d...
pkg/oauth2/lib/src/utils.dart:44: }), "&");
Indent the preceding three lines - 2, like in the forEach call.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.d...
pkg/oauth2/lib/src/utils.dart:58: /// This will always return an array of two
elements or fewer.
"array" -> "list".

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.d...
pkg/oauth2/lib/src/utils.dart:65: toSplit.substring(index + pattern.length)];
Indent +2.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/authorizat...
File pkg/oauth2/test/authorization_code_grant_test.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/authorizat...
pkg/oauth2/test/authorization_code_grant_test.dart:17: final Uri redirectUrl =
new Uri.fromString('http://example.com/redirect');
Ditch the annotation.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/authorizat...
pkg/oauth2/test/authorization_code_grant_test.dart:81:
'&redirect_uri=http%3A%2F%2Fexample.com%2Fredirect'));
Since you're using a map at some point (if I recall), can we rely on the query
parameters always being a certain order?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/credential...
File pkg/oauth2/test/credentials_test.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/credential...
pkg/oauth2/test/credentials_test.dart:25: setUp(createHttpClient);
Why not just inline the function here?

[nweiz, 2012-11-17 01:06:27]

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/oauth2.dart
File pkg/oauth2/lib/oauth2.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/oauth2.dart...
pkg/oauth2/lib/oauth2.dart:33: ///         new
Uri.fromString("http://example.com/oauth2/token");
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Nit, but I wouldn't bother to type annotate these (and below).

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/oauth2.dart...
pkg/oauth2/lib/oauth2.dart:85: ///         // parameters the the
AuthorizationCodeGrant. It will validate them
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> "the the" -> "to the"

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/oauth2.dart...
pkg/oauth2/lib/oauth2.dart:102: ///     }).then(print);
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Wonderful example. Nicely done.

Thanks! It was kind of a pain, but I'm glad it's there.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
File pkg/oauth2/lib/src/authorization_code_grant.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:9: import
'../../../http/lib/http.dart' as http;
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Add TODO to use "package:" here. Mention bug 6745.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:29: class
AuthorizationCodeGrant {
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> The state enums and all of the StateErrors here are a bit of a code smell to
me.
> It feels like this is actually two classes masquerading as one. You could
split
> it into:
> 
> AuthorizationCodeGrant (_INITIAL_STATE)
> - has getAuthorizationUrl() which returns a AuthorizationResponseHandler
> 
> AuthorizationResponseHandler (_AWAITING_RESPONSE_STATE)
> - has handleResponse()
> 
> This might make things a little less error-prone. Of course, fewer types is
good
> too though. Thoughts?

When I was designing this, I tried to strike a balance between statically
guiding users along the right path and having classes that represented
reasonable things. I think splitting up this class goes too far in the direction
of the former; there's no longer one class you can point to that's in charge of
the entire flow.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:30: /// An enum value for
[_state] indicating that [getAuthorizationUrl] has
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Nittiest of nits, but it looks like most of these comments could be
word-wrapped
> at 80 columns more precisely. Do that if you want, but it's fine if you don't
> too.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:32: static final int
_INITIAL_STATE = 0;
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> "static final int" -> "static const". Here and elsewhere.

Done.

What's the difference between "static const" and "static final"?

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:77: final http.BaseClient
_httpClient;
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Using BaseClient as a type annotation here and elsewhere makes sense, but it
> makes me wonder if the type names could be better in http. It seems like
you'll
> mention clients in type annotations more often than in constructor calls, so I
> wonder if it should be something like:
> 
>   BaseClient -> Client
>   Client -> DefaultClient (or some other name here)
> 
> I worry that otherwise people will just annotate to take Client and not
realize
> they've made their type annotation incorrectly too specific. Thoughts?

I think this code gives a misleading impression about the degree to which people
will take clients as parameters versus constructing their own. I think in the
vast majority of cases, users will just want to construct a Client and make a
few HTTP requests with it. Creating wrappers like this is a much less common
case.

It's also worth noting that if you really need to pass a non-Client BaseClient
to a method annotated with "Client", you can have your client implement Client
as an interface.

One possibility would be to expose http.Client as an interface with a factory
constructor. BaseClient could then implement this interface. The downside to
that is that all of the BaseClient method signatures would have to be
duplicated, but maybe that's worthwhile.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:106: /// Returns the URL to
which the resource owner should be redirected to
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> "redirected" -> "directed"?

In most cases this will be run on a server, where the resource owner's browser
will actually be redirected.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:108: /// `redirect`, which
should point to a server controlled by the client. This
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> backquotes -> square brackets. You can (and should) use [foo] for parameters
> too. Here and elsewhere.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:122: /// It is a [StateError]
to call this more than once for a given instance of
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Remove the "for a given..." part. That should be implied since this is an
> instance method. :)

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:147: /// server. Note that this
"response" is not an HTTP response, but rather the
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Maybe: "...rather the data passed to the redirect server as query parameters
on
> the redirect URL."
> 
> ?

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:163: throw new StateError('The
authorization URL has not yet been generated.');
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:165: throw new StateError('The
authorization code has already been received.');
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Ditto.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:169: if (this._stateString !=
null) {
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Is the "this." needed here?

No. Removed.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:173: '"state".');
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> How about:
> 
>                '"$authorizationEndpoint": parameter "state" expected to be '
>                '"$_stateString", was missing.');

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:174: } else if
(parameters['state'] != this._stateString) {
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> "this."?

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:213: throw new StateError('The
authorization URL has not yet been generated.');
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Long lines.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:217: _state = _FINISHED_STATE;
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Move this into _handleAuthorizationCode so it's not duplicated in both
> codepaths?

_handleAuthorizationCode exists explicitly to avoid this code.
_handleAuthorizationResponse sets _state to _FINISHED_STATE, so if it runs this
code when it enters _handleAuthorizationCode, it'll always throw an error.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:248: /// continue using the
client.
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Ugh, this makes me wonder if we should be ref-counting here, but that just
makes
> me feel dirty.
> 
> Maybe check the state and only close the client if you're not in
> _FINISHED_STATE? And then document that?

I think it's fine as-is. This method isn't expected to be used except in
exceptional circumstances, when you want to close the AuthorizationCodeGrant
in-flight but you don't want to completely exit the program. If it's used
incorrectly, it'll already cause reasonably obvious bugs pretty quickly.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
File pkg/oauth2/lib/src/authorization_exception.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_exception.dart:10: class AuthorizationException
{
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> extends Exception
> 
> Or implements?

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_exception.dart:16: /// The description of the
error, provided by the server. This is optional.
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Will it be `null` or `""` if not provided?

null. Made this clear in the docs.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_exception.dart:34: return '$header.';
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> I think I'd prefer to rely on the description having it's own period. Leave
this
> off?

I haven't actually seen any OAuth2 providers that actually give descriptions
with their errors, but I suspect they're just short strings rather than full
punctuated sentences.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/client....
File pkg/oauth2/lib/src/client.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/client....
pkg/oauth2/lib/src/client.dart:23: /// the credentials are expired and can't be
refreshed.
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> This shouldn't be a StateError, I think. StateErrors should never be caught
and
> this seems like an error you may want to handle. Make an ExpirationException
> type, or use AuthorizationException for this?

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/client....
pkg/oauth2/lib/src/client.dart:93: /// This will throw a [StateError] if the
[Credentials] can't be refreshed, an
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Throwing a StateError here is OK if it's something the programmer can check
for
> programmatically and prevent. Is the idea that I need to do if
> (client.credentials.canRefresh) before calling this?

Yes.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/client....
pkg/oauth2/lib/src/client.dart:100: Future<Client>
refreshCredentials([List<String> newScopes]) {
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Any particular reason to complete with the client? Just convenience?

Yep.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
File pkg/oauth2/lib/src/credentials.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:25: /// to save the refreshed credentials
for future use.
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Given this, maybe client should raise some sort of event/notification/stream
> when it has refreshed the credentials so you can save them?
> 
> I know the normal case is that you just save once right before exiting, but I
> worry about the case where your app dies after refreshing but before you've
had
> a chance to save them.

That's a good idea. I've added a TODO to do so once we have event
infrastructure.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:40: /// be specified via `scopes`. The scope
strings are specific to the
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Just me, but I'd probably italicize scopes and not code font.

This shouldn't mention the field name at all. It was a holdover from copied
parameter documentation.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:74: if (tokenEndpoint != null) tokenEndpoint
= new Uri.fromString(tokenEndpoint);
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Long line.

My kingdom for ||=.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:81: parsed['accessToken'],
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Throw a FormatException if this is missing?

Done. I've added a bunch more format checking here.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/credent...
pkg/oauth2/lib/src/credentials.dart:133: // preference to form parameters, but
Google doesn't support that. Should
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
File pkg/oauth2/lib/src/handle_access_token_response.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
pkg/oauth2/lib/src/handle_access_token_response.dart:19: final int
_EXPIRATION_GRACE = 10;
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> "final int" -> "const".

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
pkg/oauth2/lib/src/handle_access_token_response.dart:44: } catch (e) {
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Restrict this:
> 
> } on FormatException catch (e) {
> 
> You really don't want to catch NoSuchMethod or OutOfMemory here. :)

JSON.parse doesn't throw a FormatException :-/.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
pkg/oauth2/lib/src/handle_access_token_response.dart:46: '"$tokenEndpoint":
invalid JSON.\n\n${response.body}');
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> How about a helper function for all of these throws:
> 
> validate(bool condition, String message) {
>   if (!condition) throw new FormatException('Invalid OAuth response for '
>        '"$tokenEndpoint": $message.\n\n${response.body}');
> }

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.dart
File pkg/oauth2/lib/src/utils.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.d...
pkg/oauth2/lib/src/utils.dart:27: var value = urlDecode(split.length > 1 ?
split[1] : "");
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> var value = split.length > 1 ? urlDecode(split[1]) : "";
> 
> Not much point in decoding "". :)

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.d...
pkg/oauth2/lib/src/utils.dart:44: }), "&");
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Indent the preceding three lines - 2, like in the forEach call.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.d...
pkg/oauth2/lib/src/utils.dart:58: /// This will always return an array of two
elements or fewer.
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> "array" -> "list".

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/utils.d...
pkg/oauth2/lib/src/utils.dart:65: toSplit.substring(index + pattern.length)];
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Indent +2.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/authorizat...
File pkg/oauth2/test/authorization_code_grant_test.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/authorizat...
pkg/oauth2/test/authorization_code_grant_test.dart:17: final Uri redirectUrl =
new Uri.fromString('http://example.com/redirect');
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Ditch the annotation.

Done.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/authorizat...
pkg/oauth2/test/authorization_code_grant_test.dart:81:
'&redirect_uri=http%3A%2F%2Fexample.com%2Fredirect'));
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Since you're using a map at some point (if I recall), can we rely on the query
> parameters always being a certain order?

All our maps are constructed as map literals, so they'll have deterministic
ordering.

I feel strongly that maps should have deterministic ordering, largely because it
makes testing so much easier, so I'm happy relying on it for now and complaining
loudly if it ever gets broken.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/credential...
File pkg/oauth2/test/credentials_test.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/credential...
pkg/oauth2/test/credentials_test.dart:25: setUp(createHttpClient);
On 2012/11/16 19:53:30, Bob Nystrom wrote:
> Why not just inline the function here?

Done.

[Adam, 2012-11-17 05:13:07]

lgtm

https://codereview.chromium.org/11420025/diff/11001/pkg/oauth2/lib/src/utils....
File pkg/oauth2/lib/src/utils.dart (right):

https://codereview.chromium.org/11420025/diff/11001/pkg/oauth2/lib/src/utils....
pkg/oauth2/lib/src/utils.dart:7: import 'dart:uri';
At some point could these utility functions be rolled into dart:uri? Multiple
projects do similar things to deal with query strings.

[Adam, 2012-11-17 07:29:21]

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
File pkg/oauth2/lib/src/authorization_code_grant.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:248: /// continue using the
client.
_httpClient is final so setting to null should be an error?

[nweiz, 2012-11-19 20:55:26]

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
File pkg/oauth2/lib/src/authorization_code_grant.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:248: /// continue using the
client.
On 2012/11/17 07:29:21, Adam wrote:
> _httpClient is final so setting to null should be an error?

Good catch. This was also an issue in oauth2.Client.

https://codereview.chromium.org/11420025/diff/11001/pkg/oauth2/lib/src/utils....
File pkg/oauth2/lib/src/utils.dart (right):

https://codereview.chromium.org/11420025/diff/11001/pkg/oauth2/lib/src/utils....
pkg/oauth2/lib/src/utils.dart:7: import 'dart:uri';
On 2012/11/17 05:13:07, Adam wrote:
> At some point could these utility functions be rolled into dart:uri? Multiple
> projects do similar things to deal with query strings. 

It's possible. See issue 2645. In the short term, I may also roll this into the
http package.

[Bob Nystrom, 2012-11-19 21:37:10]

- I'd like to see a tighter catch clause when parsing the JSON.
- Also, I think the BaseClient/Client type changes we discussed will be awesome.

Otherwise, LGTM.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
File pkg/oauth2/lib/src/authorization_code_grant.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:29: class
AuthorizationCodeGrant {
On 2012/11/17 01:06:27, nweiz wrote:
> On 2012/11/16 19:53:30, Bob Nystrom wrote:
> > The state enums and all of the StateErrors here are a bit of a code smell to
> me.
> > It feels like this is actually two classes masquerading as one. You could
> split
> > it into:
> > 
> > AuthorizationCodeGrant (_INITIAL_STATE)
> > - has getAuthorizationUrl() which returns a AuthorizationResponseHandler
> > 
> > AuthorizationResponseHandler (_AWAITING_RESPONSE_STATE)
> > - has handleResponse()
> > 
> > This might make things a little less error-prone. Of course, fewer types is
> good
> > too though. Thoughts?
> 
> When I was designing this, I tried to strike a balance between statically
> guiding users along the right path and having classes that represented
> reasonable things. I think splitting up this class goes too far in the
direction
> of the former; there's no longer one class you can point to that's in charge
of
> the entire flow.

SGTM.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:32: static final int
_INITIAL_STATE = 0;
On 2012/11/17 01:06:27, nweiz wrote:
> On 2012/11/16 19:53:30, Bob Nystrom wrote:
> > "static final int" -> "static const". Here and elsewhere.
> 
> Done.
> 
> What's the difference between "static const" and "static final"?

"const" declares an actual Dart constant: something you can use in a const
expression. "final" declares a lazy-initialized single-assignment variable.

So this is fine:

const FOO = 1;
bar([foo = FOO]) { ... } // can reference FOO in a const expression

but this isn't:

final FOO = 1;
bar([foo = FOO]) { ... }

since parameter default values have to be const expressions. There are a couple
of other places where const expressions are required, but they're fairly rare.
In general, it seems like we are using const less and less in the language. (For
example, the recent change to RegExp to remove its const constructor.)

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/authori...
pkg/oauth2/lib/src/authorization_code_grant.dart:77: final http.BaseClient
_httpClient;
On 2012/11/17 01:06:27, nweiz wrote:
> On 2012/11/16 19:53:30, Bob Nystrom wrote:
> > Using BaseClient as a type annotation here and elsewhere makes sense, but it
> > makes me wonder if the type names could be better in http. It seems like
> you'll
> > mention clients in type annotations more often than in constructor calls, so
I
> > wonder if it should be something like:
> > 
> >   BaseClient -> Client
> >   Client -> DefaultClient (or some other name here)
> > 
> > I worry that otherwise people will just annotate to take Client and not
> realize
> > they've made their type annotation incorrectly too specific. Thoughts?
> 
> I think this code gives a misleading impression about the degree to which
people
> will take clients as parameters versus constructing their own. I think in the
> vast majority of cases, users will just want to construct a Client and make a
> few HTTP requests with it. Creating wrappers like this is a much less common
> case.
> 
> It's also worth noting that if you really need to pass a non-Client BaseClient
> to a method annotated with "Client", you can have your client implement Client
> as an interface.
> 
> One possibility would be to expose http.Client as an interface with a factory
> constructor. BaseClient could then implement this interface. The downside to
> that is that all of the BaseClient method signatures would have to be
> duplicated, but maybe that's worthwhile.

Yeah, I like this.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
File pkg/oauth2/lib/src/handle_access_token_response.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
pkg/oauth2/lib/src/handle_access_token_response.dart:44: } catch (e) {
On 2012/11/17 01:06:27, nweiz wrote:
> On 2012/11/16 19:53:30, Bob Nystrom wrote:
> > Restrict this:
> > 
> > } on FormatException catch (e) {
> > 
> > You really don't want to catch NoSuchMethod or OutOfMemory here. :)
> 
> JSON.parse doesn't throw a FormatException :-/.

Restrict it to a JSONParseException? We definitely don't want a catch-all here.

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/authorizat...
File pkg/oauth2/test/authorization_code_grant_test.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/test/authorizat...
pkg/oauth2/test/authorization_code_grant_test.dart:81:
'&redirect_uri=http%3A%2F%2Fexample.com%2Fredirect'));
On 2012/11/17 01:06:27, nweiz wrote:
> On 2012/11/16 19:53:30, Bob Nystrom wrote:
> > Since you're using a map at some point (if I recall), can we rely on the
query
> > parameters always being a certain order?
> 
> All our maps are constructed as map literals, so they'll have deterministic
> ordering.
> 
> I feel strongly that maps should have deterministic ordering, largely because
it
> makes testing so much easier, so I'm happy relying on it for now and
complaining
> loudly if it ever gets broken.

Works for me.

[nweiz, 2012-11-19 22:20:11]

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
File pkg/oauth2/lib/src/handle_access_token_response.dart (right):

https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
pkg/oauth2/lib/src/handle_access_token_response.dart:44: } catch (e) {
On 2012/11/19 21:37:10, Bob Nystrom wrote:
> On 2012/11/17 01:06:27, nweiz wrote:
> > On 2012/11/16 19:53:30, Bob Nystrom wrote:
> > > Restrict this:
> > > 
> > > } on FormatException catch (e) {
> > > 
> > > You really don't want to catch NoSuchMethod or OutOfMemory here. :)
> > 
> > JSON.parse doesn't throw a FormatException :-/.
> 
> Restrict it to a JSONParseException? We definitely don't want a catch-all
here.

It doesn't throw a JSONParseException either :-/. It just throws a generic
string error.

[Bob Nystrom, 2012-11-19 22:52:12]

On 2012/11/19 22:20:11, nweiz wrote:
>
https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
> File pkg/oauth2/lib/src/handle_access_token_response.dart (right):
> 
>
https://codereview.chromium.org/11420025/diff/6001/pkg/oauth2/lib/src/handle_...
> pkg/oauth2/lib/src/handle_access_token_response.dart:44: } catch (e) {
> On 2012/11/19 21:37:10, Bob Nystrom wrote:
> > On 2012/11/17 01:06:27, nweiz wrote:
> > > On 2012/11/16 19:53:30, Bob Nystrom wrote:
> > > > Restrict this:
> > > > 
> > > > } on FormatException catch (e) {
> > > > 
> > > > You really don't want to catch NoSuchMethod or OutOfMemory here. :)
> > > 
> > > JSON.parse doesn't throw a FormatException :-/.
> > 
> > Restrict it to a JSONParseException? We definitely don't want a catch-all
> here.
> 
> It doesn't throw a JSONParseException either :-/. It just throws a generic
> string error.

Ugh. OK. Well leave a TODO and file a bug against the JSON library then?

[nweiz, 2012-11-19 22:59:47]

> Ugh. OK. Well leave a TODO and file a bug against the JSON library then?

Too late for this CL, but I'll send another with the TODOs.

[dgrove, 2012-11-20 05:18:16]

once pub moves to lib/_internal, this package should move there as well (and
from there, be published to pub).