Issue 351: Reimplement OAuth2 (Step 3 - Tests, Flows and Credential)

Issue 351: Reimplement OAuth2 Step 3

[peleyal, 2013-09-26 20:51:00]

This CL is based on the two latest CLs which were already committed
https://codereview.appspot.com/13412046/
https://codereview.appspot.com/13632059/

Jon it will be awesome if you can take a look in my Credential and
AuthorizationCodeFlow classes.

Thanks in advance,
Eyal

[jonskeet, 2013-09-27 06:15:54]

I haven't got my head round the whole flow yet, so these are more stylistic
comments than anything else, but I hope they're useful.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:51: public class Initializer
Is "initializer class" a common term in this framework? I'd normally call it
"settings" or perhaps "parameters".

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:158: "At least one of the
client secret or client secret stream MUST be set");
Why at least one rather than exactly one?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:163: using
(initializer.ClientSecretsStream)
It's not clear to me that we should have a using statement here. In particular,
it's really odd that we dispose of ClientSecretsStream is ClientSecrets is null,
but not otherwise. I'd personally not dispose of it at all - let the caller do
that.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:168: catch (Exception ex)
Is this really useful? If you *do* want to translate it to an ArgumentException
(which I personally wouldn't) do you really need to catch Exception rather than
a subclass?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:174:
initializer.ClientSecrets.ThrowIfNull("Clock");
This should be initializer.Clock.ThrowIfNull.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:176: accessMethod =
initializer.AccessMethod;
Does ThrowIfNull not return the non-null value? If it doesn't, that's a pity -
it's nice to be able to write:

clock = initializer.Clock.ThrowIfNull("Clock");

Having said that, presumably this is throwing an ArgumentNullException, which
isn't *really* valid as there's no "Clock" parameter here.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:197: () => new
BackOffHandler(new ExponentialBackOff())));
Indent? (As this is a second argument to the ExponentialBackoffInitializer
constructor.)

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:199: httpClient =
(initializer.HttpClientFactory ?? new
HttpClientFactory()).CreateHttpClient(httpArgs);
Are these expensive to create? If you created a new one on each access and used
a using statement, you wouldn't need to implement IDisposable, which would be
nice.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:208: public async
Task<TokenResponse> LoadToken(string userId, CancellationToken
taskCancellationToken)
All these async methods are missing an Async suffix, contrary to the TAP
conventions: http://msdn.microsoft.com/en-us/library/hh873175.aspx

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:223: await
DataStore.Delete<TokenResponse>(userId).ConfigureAwait(false);
Do we not have a version which accepts a cancellation token?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:240:
taskCancellationToken.ThrowIfCancellationRequested();
See comment on RefreshToken

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:257:
taskCancellationToken.ThrowIfCancellationRequested();
I doubt that this is needed, as your other async calls should be doing this
anyway. (i.e. at some point, when we get to some code which really needs to do
some work rather than just awaiting something else, *that* should be dealing
with the token.)

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:307:
taskCancellationToken.ThrowIfCancellationRequested();
Again, won't this already have been handled by SendAsync?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:312: await
response.Content.ReadAsStringAsync().ConfigureAwait(false));
You may want to pass the cancellation token into ReadAsStringAsync here.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:321: await
response.Content.ReadAsStringAsync().ConfigureAwait(false));
And here.

Given that you fetch the content either way, I would extract this part into a
line before the "if":

    string content = await
response.Content.ReadAsStringAsync(taskCancellationToken)
       .ConfigureAwait(false));
    if (!response.IsSuccessStatusCode)
    {
        ...
    }

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:327: public void Dispose()
So IAuthorizationCodeFlow extends IDisposable?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Credential.cs (right):

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:41: 
Given that our immediate state is all read-only, it feels to me like it should
be TokenResponse which exposes atomic operations, and then we can ditch the
lock.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:64: 
TokenResponse would need to expose a method to work this out atomically, I
suspect.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:90: 
You're ignoring this return value in the calling code above. This feels like it
should be an exception.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:96: 
If this class isn't thread-safe, we shouldn't be locking in other places IMO.

It's not clear what race condition you're expecting though. If there are two
concurrent calls to RefreshToken, we end up with two requests, and the last one
will win - that feels okay to me.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:111: 
Is there a conflicting HttpStatusCode in another namespace, or could you just
have a using directive for System.Net?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:124: 
I haven't followed where this is called from, but it reinforces my impression
that ConfigurableHttpClient should really be considered a one-shot client;
otherwise changing credentials would end up with multiple interceptors. I may
well not be seeing the whole picture though.

[peleyal, 2013-09-27 14:32:33]

Thanks Jon!
Looking forward your second round.

Eyal

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:51: public class Initializer
It's indeed the common term in this library, although your suggested name is a
good option, I won't change it, since I prefer to keep the current standard
On 2013/09/27 06:15:54, jonskeet wrote:
> Is "initializer class" a common term in this framework? I'd normally call it
> "settings" or perhaps "parameters".

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:158: "At least one of the
client secret or client secret stream MUST be set");
On 2013/09/27 06:15:54, jonskeet wrote:
> Why at least one rather than exactly one?

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:163: using
(initializer.ClientSecretsStream)
You can set ClientSecret or ClientSecretStream.
If you set ClientSecretStream, I want to make your life easier by closing and
disposing the stream for you - so you can avoid another line of code. 
I documented that, so there is no surprise. 
In addition if the user choose to close it as well, it's OK.

On 2013/09/27 06:15:54, jonskeet wrote:
> It's not clear to me that we should have a using statement here. In
particular,
> it's really odd that we dispose of ClientSecretsStream is ClientSecrets is
null,
> but not otherwise. I'd personally not dispose of it at all - let the caller do
> that.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:168: catch (Exception ex)
I removed the try-catch, but I'm just curious - Why don't you recommend it?

On 2013/09/27 06:15:54, jonskeet wrote:
> Is this really useful? If you *do* want to translate it to an
ArgumentException
> (which I personally wouldn't) do you really need to catch Exception rather
than
> a subclass?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:174:
initializer.ClientSecrets.ThrowIfNull("Clock");
On 2013/09/27 06:15:54, jonskeet wrote:
> This should be initializer.Clock.ThrowIfNull.

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:176: accessMethod =
initializer.AccessMethod;
On 2013/09/27 06:15:54, jonskeet wrote:
> Does ThrowIfNull not return the non-null value? If it doesn't, that's a pity -
> it's nice to be able to write:
> 
> clock = initializer.Clock.ThrowIfNull("Clock");
> 
> Having said that, presumably this is throwing an ArgumentNullException, which
> isn't *really* valid as there's no "Clock" parameter here.

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:197: () => new
BackOffHandler(new ExponentialBackOff())));
On 2013/09/27 06:15:54, jonskeet wrote:
> Indent? (As this is a second argument to the ExponentialBackoffInitializer
> constructor.)

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:199: httpClient =
(initializer.HttpClientFactory ?? new
HttpClientFactory()).CreateHttpClient(httpArgs);
I prefer not do so, because every call will cost the handshake of opening an
HTTP connection.
There is also a similar code in BaseClientService, so I prefer to keep it as a
standard, unless you have s strong argue.

On 2013/09/27 06:15:54, jonskeet wrote:
> Are these expensive to create? If you created a new one on each access and
used
> a using statement, you wouldn't need to implement IDisposable, which would be
> nice.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:208: public async
Task<TokenResponse> LoadToken(string userId, CancellationToken
taskCancellationToken)
On 2013/09/27 06:15:54, jonskeet wrote:
> All these async methods are missing an Async suffix, contrary to the TAP
> conventions: http://msdn.microsoft.com/en-us/library/hh873175.aspx

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:223: await
DataStore.Delete<TokenResponse>(userId).ConfigureAwait(false);
No. I'm not sure I want to add it to my IDataStore interface. What do you think?
(https://code.google.com/p/google-api-dotnet-client/source/browse/Src/GoogleAp...)
On 2013/09/27 06:15:54, jonskeet wrote:
> Do we not have a version which accepts a cancellation token?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:240:
taskCancellationToken.ThrowIfCancellationRequested();
It's an inherited method. I hate to add documentation here and there, so I
prefer to leave the documentation only in the interface level, unless I'm doing
something special which I want to add documentation for that.
On 2013/09/27 06:15:54, jonskeet wrote:
> See comment on RefreshToken

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:257:
taskCancellationToken.ThrowIfCancellationRequested();
On 2013/09/27 06:15:54, jonskeet wrote:
> I doubt that this is needed, as your other async calls should be doing this
> anyway. (i.e. at some point, when we get to some code which really needs to do
> some work rather than just awaiting something else, *that* should be dealing
> with the token.)

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:307:
taskCancellationToken.ThrowIfCancellationRequested();
But what happen if someone cancel it after it was already sent? In that case I
prefer to cancel it now and not to continue parsing and stuff, right?
On 2013/09/27 06:15:54, jonskeet wrote:
> Again, won't this already have been handled by SendAsync?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:312: await
response.Content.ReadAsStringAsync().ConfigureAwait(false));
On 2013/09/27 06:15:54, jonskeet wrote:
> You may want to pass the cancellation token into ReadAsStringAsync here.

I WISH. I'm using the NuGet package of HttpClient (for .NET 4.0) and it doesn't
contain that option.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:321: await
response.Content.ReadAsStringAsync().ConfigureAwait(false));
On 2013/09/27 06:15:54, jonskeet wrote:
> And here.
> 
> Given that you fetch the content either way, I would extract this part into a
> line before the "if":
> 
>     string content = await
> response.Content.ReadAsStringAsync(taskCancellationToken)
>        .ConfigureAwait(false));
>     if (!response.IsSuccessStatusCode)
>     {
>         ...
>     }

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:327: public void Dispose()
YES SIR.
On 2013/09/27 06:15:54, jonskeet wrote:
> So IAuthorizationCodeFlow extends IDisposable?

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Credential.cs (right):

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:41: 
My only problem is with CopyFrom. It can't be atomic. because it copies couple
of fields.
See
https://code.google.com/p/google-api-dotnet-client/source/browse/Src/GoogleAp...

On 2013/09/27 06:15:54, jonskeet wrote:
> Given that our immediate state is all read-only, it feels to me like it should
> be TokenResponse which exposes atomic operations, and then we can ditch the
> lock.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:64: 
On 2013/09/27 06:15:54, jonskeet wrote:
> TokenResponse would need to expose a method to work this out atomically, I
> suspect.

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:90: 
On 2013/09/27 06:15:54, jonskeet wrote:
> You're ignoring this return value in the calling code above. This feels like
it
> should be an exception.

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:96: 
On 2013/09/27 06:15:54, jonskeet wrote:
> If this class isn't thread-safe, we shouldn't be locking in other places IMO.
> 
> It's not clear what race condition you're expecting though. If there are two
> concurrent calls to RefreshToken, we end up with two requests, and the last
one
> will win - that feels okay to me.

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:111: 
On 2013/09/27 06:15:54, jonskeet wrote:
> Is there a conflicting HttpStatusCode in another namespace, or could you just
> have a using directive for System.Net?

Done.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:124: 
It is one-shot client.
After the httpClient is created we calls all of its initializers (classes that
implement IConfigurableHttpClientInitializer).

In that case you configure the http client as you wish (add unsuccessful
response handlers, change num of tries for each request, etc.)

You can take a look in my customize ConfigurableHttpMessageHandler
(https://code.google.com/p/google-api-dotnet-client/source/browse/Src/GoogleAp...)
On 2013/09/27 06:15:54, jonskeet wrote:
> I haven't followed where this is called from, but it reinforces my impression
> that ConfigurableHttpClient should really be considered a one-shot client;
> otherwise changing credentials would end up with multiple interceptors. I may
> well not be seeing the whole picture though.

https://codereview.appspot.com/13972043/diff/43001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Credential.cs (right):

https://codereview.appspot.com/13972043/diff/43001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:92: token.CopyFrom(newToken);
What is your recommendation here?
Do you think that I should change token.copyFrom to use a lock?
Should I change all getters and setters in token to use a lock as well?
------
I think that it would be ok to leave it without lock. the only problem is that
we may create two-three requests to refresh the token although we need only one.
But I prefer to keep the code as simple as I can...
Any suggestions?

[jonskeet, 2013-09-27 15:15:25]

A few more comments.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:53: /// <summary>
Have you considered having a "CreateFlow" method which just calls the
constructor? That would allow for more fluent client code:

    var flow = new AuthorizationCodeFlow.Initializer { ... }.CreateFlow();

rather than:

    var flow = new AuthorizationCodeFlow(new AuthorizationCodeFlow.Initializer {
... });

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:163: using
(initializer.ClientSecretsStream)
On 2013/09/27 14:32:33, peleyal wrote:
> You can set ClientSecret or ClientSecretStream.
> If you set ClientSecretStream, I want to make your life easier by closing and
> disposing the stream for you - so you can avoid another line of code. 
> I documented that, so there is no surprise. 
> In addition if the user choose to close it as well, it's OK.

Hmm. It's unusual - though not unheard of - to close streams that you haven't
acquired yourself, unless you're taking ownership in general to wrap the stream.
In this case you're only reading data from it "temporarily" (rather than it
being part of the object thereafter) - it definitely feels unnatural to me. It's
no big deal, but it's a bit unusual.

I would probably avoid having the Stream as part of the Initializer class to
start with - I might have a method on it (e.g. WithSecretsFromStream) which
explicitly loads the secrets from a stream, but without closing it.

Aside from anything else, the fact that Initializer contains a disposable field
which you're not disposing, but the ownership is in some sense transferred in
the constructor is really weird.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:168: catch (Exception ex)
On 2013/09/27 14:32:33, peleyal wrote:
> I removed the try-catch, but I'm just curious - Why don't you recommend it?

Other than at very "top level" code, you should usually only catch *explicit*
exceptions - typically because you know how you want to handle them.

In this case it wasn't *too* bad as you were throwing another exception (instead
of just continuing) but it's usually better (IMO) to just let the original
exception bubble up. Fortunately no-one should be catching ArgumentException
anyway, of course.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:199: httpClient =
(initializer.HttpClientFactory ?? new
HttpClientFactory()).CreateHttpClient(httpArgs);
On 2013/09/27 14:32:33, peleyal wrote:
> I prefer not do so, because every call will cost the handshake of opening an
> HTTP connection.

So a client maintains the connection? That doesn't feel ideal to me.

> There is also a similar code in BaseClientService, so I prefer to keep it as a
> standard, unless you have s strong argue.

If this is the idiom for the rest of the framework, so be it. I would prefer to
have pooled connections behind the scenes, and make it cheap to construct a
client (just like SqlConnection etc). I'm not going to suggest the whole
framework should change at this point though.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:307:
taskCancellationToken.ThrowIfCancellationRequested();
On 2013/09/27 14:32:33, peleyal wrote:
> But what happen if someone cancel it after it was already sent? In that case I
> prefer to cancel it now and not to continue parsing and stuff, right?

If it's cancelled before the task returned by SendAsync has completed, that task
should end up cancelled - and the await expression will throw an exception, so
you're covered.

So basically you're accounting for the microscopic possibility of the
cancellation token being cancelled *after* the await has completed normally, but
*before* you get to this line of code. If you're trying to take account of that
possibility, you'd want to insert this between every pair of statements, which
just doesn't scale.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:312: await
response.Content.ReadAsStringAsync().ConfigureAwait(false));
On 2013/09/27 14:32:33, peleyal wrote:
> On 2013/09/27 06:15:54, jonskeet wrote:
> > You may want to pass the cancellation token into ReadAsStringAsync here.
> 
> I WISH. I'm using the NuGet package of HttpClient (for .NET 4.0) and it
doesn't
> contain that option.

Wow. That's really surprising :(

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:327: public void Dispose()
On 2013/09/27 14:32:33, peleyal wrote:
> YES SIR.

<sigh> Oh well. This flows from the earlier discussion about HTTP clients.

https://codereview.appspot.com/13972043/diff/43001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Credential.cs (right):

https://codereview.appspot.com/13972043/diff/43001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:92: token.CopyFrom(newToken);
On 2013/09/27 14:32:33, peleyal wrote:
> What is your recommendation here?
> Do you think that I should change token.copyFrom to use a lock?

Possibly. I think it makes more sense for the token to handle concurrency than
this class.

> Should I change all getters and setters in token to use a lock as well?
> ------
> I think that it would be ok to leave it without lock. the only problem is that
> we may create two-three requests to refresh the token although we need only
one.
> But I prefer to keep the code as simple as I can...
> Any suggestions?

An alternative would be to make TokenResponse immutable - that would be my
preference, to be honest. Heck, you might even want to consider making it a
value type. Then you'd need to either use locking or Interlocked within this
class, as the token can be refreshed.

It doesn't make much sense for a token to change, in my view. When you refresh a
token, you exchange a current one for a new one, surely - rather than changing
the *existing* one.

Again, I don't know how much of this is within scope for your work...

[peleyal, 2013-09-27 18:41:48]

Thanks again :)
Read for last (hopefully) review

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:53: /// <summary>
Actually I prefer not, because GoogleAuthorizationCodeFlow inherits from this
one. I can add CreateFlow in this initializer and in GoogleAuthorizationCodeFlow
as well, but I don't see the big benefit of it.
Again, if you have a strong argument please let me know and I'll do that :)

On 2013/09/27 15:15:25, jonskeet wrote:
> Have you considered having a "CreateFlow" method which just calls the
> constructor? That would allow for more fluent client code:
> 
>     var flow = new AuthorizationCodeFlow.Initializer { ... }.CreateFlow();
> 
> rather than:
> 
>     var flow = new AuthorizationCodeFlow(new AuthorizationCodeFlow.Initializer
{
> ... });

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:163: using
(initializer.ClientSecretsStream)
I thought about the user and I tried to make his life as easy as we can.
So with my code he will need to do the following:

var credential = await GoogleWebAuthenticationBroker.AuthenticateAsync(
 new FileStream("client_secrets.json", FileMode.Open, FileAccess.Read),
 new[] { BooksService.Scopes.Books.GetStringValue() },
 "user", CancellationToken.None).ConfigureAwait(false);

while the other options it to:

using (var stream = new FileStream("client_secrets.json", FileMode.Open,
FileAccess.Read))
 {
  var credential = await GoogleWebAuthenticationBroker.AuthenticateAsync(
  GoogleClientSecrets.Load(stream).Secrets,
  new[] { BooksService.Scopes.Books.GetStringValue() },
  "user", CancellationToken.None).ConfigureAwait(false);


cOn 2013/09/27 15:15:25, jonskeet wrote:
> On 2013/09/27 14:32:33, peleyal wrote:
> > You can set ClientSecret or ClientSecretStream.
> > If you set ClientSecretStream, I want to make your life easier by closing
and
> > disposing the stream for you - so you can avoid another line of code. 
> > I documented that, so there is no surprise. 
> > In addition if the user choose to close it as well, it's OK.
> 
> Hmm. It's unusual - though not unheard of - to close streams that you haven't
> acquired yourself, unless you're taking ownership in general to wrap the
stream.
> In this case you're only reading data from it "temporarily" (rather than it
> being part of the object thereafter) - it definitely feels unnatural to me.
It's
> no big deal, but it's a bit unusual.
> 
> I would probably avoid having the Stream as part of the Initializer class to
> start with - I might have a method on it (e.g. WithSecretsFromStream) which
> explicitly loads the secrets from a stream, but without closing it.
> 
> Aside from anything else, the fact that Initializer contains a disposable
field
> which you're not disposing, but the ownership is in some sense transferred in
> the constructor is really weird.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:199: httpClient =
(initializer.HttpClientFactory ?? new
HttpClientFactory()).CreateHttpClient(httpArgs);
ACK
On 2013/09/27 15:15:25, jonskeet wrote:
> On 2013/09/27 14:32:33, peleyal wrote:
> > I prefer not do so, because every call will cost the handshake of opening an
> > HTTP connection.
> 
> So a client maintains the connection? That doesn't feel ideal to me.
> 
> > There is also a similar code in BaseClientService, so I prefer to keep it as
a
> > standard, unless you have s strong argue.
> 
> If this is the idiom for the rest of the framework, so be it. I would prefer
to
> have pooled connections behind the scenes, and make it cheap to construct a
> client (just like SqlConnection etc). I'm not going to suggest the whole
> framework should change at this point though.

https://codereview.appspot.com/13972043/diff/23001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:307:
taskCancellationToken.ThrowIfCancellationRequested();
On 2013/09/27 15:15:25, jonskeet wrote:
> On 2013/09/27 14:32:33, peleyal wrote:
> > But what happen if someone cancel it after it was already sent? In that case
I
> > prefer to cancel it now and not to continue parsing and stuff, right?
> 
> If it's cancelled before the task returned by SendAsync has completed, that
task
> should end up cancelled - and the await expression will throw an exception, so
> you're covered.
> 
> So basically you're accounting for the microscopic possibility of the
> cancellation token being cancelled *after* the await has completed normally,
but
> *before* you get to this line of code. If you're trying to take account of
that
> possibility, you'd want to insert this between every pair of statements, which
> just doesn't scale.

Done.

https://codereview.appspot.com/13972043/diff/43001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/Credential.cs (right):

https://codereview.appspot.com/13972043/diff/43001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/Credential.cs:92: token.CopyFrom(newToken);
On 2013/09/27 15:15:25, jonskeet wrote:
> On 2013/09/27 14:32:33, peleyal wrote:
> > What is your recommendation here?
> > Do you think that I should change token.copyFrom to use a lock?
> 
> Possibly. I think it makes more sense for the token to handle concurrency than
> this class.
> 
> > Should I change all getters and setters in token to use a lock as well?
> > ------
> > I think that it would be ok to leave it without lock. the only problem is
that
> > we may create two-three requests to refresh the token although we need only
> one.
> > But I prefer to keep the code as simple as I can...
> > Any suggestions?
> 
> An alternative would be to make TokenResponse immutable - that would be my
> preference, to be honest. Heck, you might even want to consider making it a
> value type. Then you'd need to either use locking or Interlocked within this
> class, as the token can be refreshed.
> 
> It doesn't make much sense for a token to change, in my view. When you refresh
a
> token, you exchange a current one for a new one, surely - rather than changing
> the *existing* one.
> 
> Again, I don't know how much of this is within scope for your work...

Done.

[peleyal, 2013-10-01 18:42:00]

+shai.raiten@gmail.com

[class, 2013-10-02 17:37:17]

Minor comments, mostly optional.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.DotNet...
File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthenticationBroker.cs
(right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.DotNet...
Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthenticationBroker.cs:59: /// The
client secrets stream. The authorization code flow constructor is responsible
for disposing the stream
Just add a period here.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.DotNet...
Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthenticationBroker.cs:64: ///
<param name="dataStore">The data store. If not specified a file data store will
be used</param>
[Optional] Again, I feel that if you have full sentences in params, might as
well add periods, it could even make sense to just go ahead and use full
sentences as param descriptions (consistency)

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.DotNet...
Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthenticationBroker.cs:78: ///
<param name="scopes">The scopes</param>
The scopes for what? I assume this is "The scopes to authorize for the client."
but it is a little ambiguous.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
File
Src/GoogleApis.Auth.Tests/OAuth2/Requests/AuthorizationCodeRequestUrlTests.cs
(right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
Src/GoogleApis.Auth.Tests/OAuth2/Requests/AuthorizationCodeRequestUrlTests.cs:24:
/// Tests for <seealso
cref="Google.Apis.Google.Apis.Auth.OAuth2.Requests.AuthorizationCodeRequestUrl"/>.
Is this testing building URLs for authorization code requests? It might help to
be more specific.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
File
Src/GoogleApis.Auth.Tests/OAuth2/Requests/AuthorizationCodeTokenRequestTests.cs
(right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
Src/GoogleApis.Auth.Tests/OAuth2/Requests/AuthorizationCodeTokenRequestTests.cs:32:
[Test]
(TODO for later) You might want to add tests for post-constructor and cleanup.
You might even want to add a TODO (peyal) Test ...

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
File Src/GoogleApis.Auth.Tests/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
Src/GoogleApis.Auth.Tests/Properties/AssemblyInfo.cs:27: [assembly:
AssemblyCopyright("Copyright © Google Inc 2012")]
2013

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:227: public async
Task<TokenResponse> ExchangeCodeForTokenAsync(string userId, string code, string
redirectUri,
Missing <summary>

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:243: public async
Task<TokenResponse> RefreshTokenAsync(string userId, string refreshToken,
Missing <summary>

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeInstalledApp.cs (right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeInstalledApp.cs:65: // to retrieve a
new access token.
Should this be "Retrieve a refresh token and/or access token from the code
exchange flow."? Also, what happens if the client is revoked? E.g. we have a
refresh token but the access token is expired.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeWebApp.cs (right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeWebApp.cs:36: /// Gets or sets the
user's credentials or <c>null</c> in case the end user need to authorize.
"in case the user need to authorize" should be "in case the user needs to
authorize"

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeWebApp.cs:98: // TODO(peleyal):
Change the state to contain some random number.
Don't forget to verify the state again later as well to prevent CSRF

[peleyal, 2013-10-02 18:24:45]

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.DotNet...
File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthenticationBroker.cs
(right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.DotNet...
Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthenticationBroker.cs:59: /// The
client secrets stream. The authorization code flow constructor is responsible
for disposing the stream
On 2013/10/02 17:37:18, class wrote:
> Just add a period here.

Done.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.DotNet...
Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthenticationBroker.cs:64: ///
<param name="dataStore">The data store. If not specified a file data store will
be used</param>
On 2013/10/02 17:37:18, class wrote:
> [Optional] Again, I feel that if you have full sentences in params, might as
> well add periods, it could even make sense to just go ahead and use full
> sentences as param descriptions (consistency)

Done.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.DotNet...
Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthenticationBroker.cs:78: ///
<param name="scopes">The scopes</param>
On 2013/10/02 17:37:18, class wrote:
> The scopes for what? I assume this is "The scopes to authorize for the
client."
> but it is a little ambiguous.

Done.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
File
Src/GoogleApis.Auth.Tests/OAuth2/Requests/AuthorizationCodeRequestUrlTests.cs
(right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
Src/GoogleApis.Auth.Tests/OAuth2/Requests/AuthorizationCodeRequestUrlTests.cs:24:
/// Tests for <seealso
cref="Google.Apis.Google.Apis.Auth.OAuth2.Requests.AuthorizationCodeRequestUrl"/>.
On 2013/10/02 17:37:18, class wrote:
> Is this testing building URLs for authorization code requests? It might help
to
> be more specific.

Done.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
File
Src/GoogleApis.Auth.Tests/OAuth2/Requests/AuthorizationCodeTokenRequestTests.cs
(right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
Src/GoogleApis.Auth.Tests/OAuth2/Requests/AuthorizationCodeTokenRequestTests.cs:32:
[Test]
No need.
It just a model. I'm checking that all the default values are set as expected.
That's all.
On 2013/10/02 17:37:18, class wrote:
> (TODO for later) You might want to add tests for post-constructor and cleanup.
> You might even want to add a TODO (peyal) Test ...

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
File Src/GoogleApis.Auth.Tests/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth.Tests/...
Src/GoogleApis.Auth.Tests/Properties/AssemblyInfo.cs:27: [assembly:
AssemblyCopyright("Copyright © Google Inc 2012")]
On 2013/10/02 17:37:18, class wrote:
> 2013

Done.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs (right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:227: public async
Task<TokenResponse> ExchangeCodeForTokenAsync(string userId, string code, string
redirectUri,
on the interface :) On 2013/10/02 17:37:18, class wrote:
> Missing <summary>

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeFlow.cs:243: public async
Task<TokenResponse> RefreshTokenAsync(string userId, string refreshToken,
on the interface :) 
On 2013/10/02 17:37:18, class wrote:
> Missing <summary>

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeInstalledApp.cs (right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeInstalledApp.cs:65: // to retrieve a
new access token.
You mean what happen if the token itself is revoke (access and refresh).
In that case the first call will fail, and as a result the datastore will clean
the current token.
Next call we will start the authorization code flow again.
On 2013/10/02 17:37:18, class wrote:
> Should this be "Retrieve a refresh token and/or access token from the code
> exchange flow."? Also, what happens if the client is revoked? E.g. we have a
> refresh token but the access token is expired.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeWebApp.cs (right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeWebApp.cs:36: /// Gets or sets the
user's credentials or <c>null</c> in case the end user need to authorize.
On 2013/10/02 17:37:18, class wrote:
> "in case the user need to authorize" should be "in case the user needs to
> authorize"

Done.

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeWebApp.cs:98: // TODO(peleyal):
Change the state to contain some random number.
On 2013/10/02 17:37:18, class wrote:
> Don't forget to verify the state again later as well to prevent CSRF

Done.

[class, 2013-10-02 20:17:03]

LGTM

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
File Src/GoogleApis.Auth/OAuth2/AuthorizationCodeInstalledApp.cs (right):

https://codereview.appspot.com/13972043/diff/60001/Src/GoogleApis.Auth/OAuth2...
Src/GoogleApis.Auth/OAuth2/AuthorizationCodeInstalledApp.cs:65: // to retrieve a
new access token.
SGTM.
On 2013/10/02 18:24:45, peleyal wrote:
> You mean what happen if the token itself is revoke (access and refresh).
> In that case the first call will fail, and as a result the datastore will
clean
> the current token.
> Next call we will start the authorization code flow again.
> On 2013/10/02 17:37:18, class wrote:
> > Should this be "Retrieve a refresh token and/or access token from the code
> > exchange flow."? Also, what happens if the client is revoked? E.g. we have a
> > refresh token but the access token is expired.
>