|
|
Created:
10 years, 10 months ago by peleyal Modified:
10 years, 9 months ago CC:
google-api-dotnet-client_googlegroup.com, shai.raiten_gmail.com Base URL:
https://google-api-dotnet-client.googlecode.com/hg/ Visibility:
Public. |
DescriptionIssue 351: Reimplement OAuth2 Step 3
Patch Set 1 #Patch Set 2 : minor #Patch Set 3 : minor #
Total comments: 55
Patch Set 4 : Jon Skeet review #Patch Set 5 : minor #
Total comments: 3
Patch Set 6 : 2nd round #
Total comments: 23
Patch Set 7 : Gus review #Patch Set 8 : minor #MessagesTotal messages: 9
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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...
Sign in to reply to this message.
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.
Sign in to reply to this message.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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. >
Sign in to reply to this message.
|