Issue 220770043: Code Review: Adding ApplicationDefaultCredentials to .Net Client Library
|
Created:
9 years, 1 month ago by vsubramani Modified:
8 years, 11 months ago Base URL:
https://google-api-dotnet-client.googlecode.com/hg/ Visibility:
Public. |
DescriptionCode Review: Adding ApplicationDefaultCredentials to .Net Client Library
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressing Jan's comments #Patch Set 3 : Missed out adding the renamed file Credential.cs #
Total comments: 26
Patch Set 4 : Addressing Anthony's comments #
Total comments: 24
Patch Set 5 : Addressing Anthony's comments - Iteration 2 #
Total comments: 54
MessagesTotal messages: 19
https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth.DotNet4/O... File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs (right): https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth.DotNet4/O... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:49: public static GoogleCredential GetApplicationDefaultCredentials(IEnumerable<string> scopes) You are using Credentials in plural form for the method name, but all the classes are using singular form. I myself would prefer if all the classes were called "Credentials" rather than "Credential", but that's matter of taste. But whatever form we choose, we should stay consistent. https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Go... File Src/GoogleApis.Auth/OAuth2/GoogleCredential.cs (right): https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Go... Src/GoogleApis.Auth/OAuth2/GoogleCredential.cs:29: public abstract class GoogleCredential: IHttpExecuteInterceptor, IHttpUnsuccessfulResponseHandler, This looks ok, but for gRPC, I will not be using classes from System.Net.Http, because gRPC acts as a HTTP/2 client on its own. So for my use, it would be good if you could also provide a more general API I could use - as an alternative to the current one bound to System.Net.Http. https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Us... File Src/GoogleApis.Auth/OAuth2/UserCredential.cs (right): https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Us... Src/GoogleApis.Auth/OAuth2/UserCredential.cs:34: public class UserCredential : GoogleCredential Not all user credentials are necessarily Google credentials (AFAIK many other companies are also using OAuth2), so I am not sure about this inheritance pattern.
Sign in to reply to this message.
Jan - what tool do you use for the code review comments? The web interface doesn't provide me a way to respond to the comments. Agreed on the singular/plural consistency. I will use Singular everywhere, since that is what is already there on the public interface. Agreed on not naming the base class GoogleCredential. I will call it Credential instead. Regarding not using the System.Net.Http, I believe the Http concept is baked into the whole library from before. Lets sync up tomorrow/wednesday in person to see what is the ideal way. On Tue, Mar 17, 2015 at 8:38 AM, <jtattermusch@google.com> wrote: > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs > File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs > (right): > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode49 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:49: > public static GoogleCredential > GetApplicationDefaultCredentials(IEnumerable<string> scopes) > You are using Credentials in plural form for the method name, but all > the classes are using singular form. > I myself would prefer if all the classes were called "Credentials" > rather than "Credential", but that's matter of taste. > But whatever form we choose, we should stay consistent. > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth/OAuth2/GoogleCredential.cs > File Src/GoogleApis.Auth/OAuth2/GoogleCredential.cs (right): > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth/OAuth2/GoogleCredential.cs#newcode29 > Src/GoogleApis.Auth/OAuth2/GoogleCredential.cs:29: public abstract class > GoogleCredential: IHttpExecuteInterceptor, > IHttpUnsuccessfulResponseHandler, > This looks ok, but for gRPC, I will not be using classes from > System.Net.Http, because gRPC acts as a HTTP/2 client on its own. So for > my use, it would be good if you could also provide a more general API I > could use - as an alternative to the current one bound to > System.Net.Http. > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth/OAuth2/UserCredential.cs > File Src/GoogleApis.Auth/OAuth2/UserCredential.cs (right): > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth/OAuth2/UserCredential.cs#newcode34 > Src/GoogleApis.Auth/OAuth2/UserCredential.cs:34: public class > UserCredential : GoogleCredential > Not all user credentials are necessarily Google credentials (AFAIK many > other companies are also using OAuth2), so I am not sure about this > inheritance pattern. > > https://codereview.appspot.com/220770043/ >
Sign in to reply to this message.
Addressing Jan's feedback. https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth.DotNet4/O... File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs (right): https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth.DotNet4/O... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:49: public static GoogleCredential GetApplicationDefaultCredentials(IEnumerable<string> scopes) On 2015/03/17 15:38:31, jtattermusch wrote: > You are using Credentials in plural form for the method name, but all the > classes are using singular form. > I myself would prefer if all the classes were called "Credentials" rather than > "Credential", but that's matter of taste. > But whatever form we choose, we should stay consistent. Acknowledged. https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Go... File Src/GoogleApis.Auth/OAuth2/GoogleCredential.cs (right): https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Go... Src/GoogleApis.Auth/OAuth2/GoogleCredential.cs:29: public abstract class GoogleCredential: IHttpExecuteInterceptor, IHttpUnsuccessfulResponseHandler, On 2015/03/17 15:38:31, jtattermusch wrote: > This looks ok, but for gRPC, I will not be using classes from System.Net.Http, > because gRPC acts as a HTTP/2 client on its own. So for my use, it would be good > if you could also provide a more general API I could use - as an alternative to > the current one bound to System.Net.Http. Acknowledged. https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Us... File Src/GoogleApis.Auth/OAuth2/UserCredential.cs (right): https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Us... Src/GoogleApis.Auth/OAuth2/UserCredential.cs:34: public class UserCredential : GoogleCredential On 2015/03/17 15:38:31, jtattermusch wrote: > Not all user credentials are necessarily Google credentials (AFAIK many other > companies are also using OAuth2), so I am not sure about this inheritance > pattern. Lets sync up offline. The HTTP dependency is pre-existing and is core to this library. Lets sync up offline to see what is the ideal way forward.
Sign in to reply to this message.
I started to review, and I stopped in the middle. I think we need a design doc and some sample code that demonstrates how this is being used. Do we know by the way how many Compute .NET users we have? Feel free to set up a meeting in my cal Thanks! https://codereview.appspot.com/220770043/diff/1/GoogleApisClient.sln File GoogleApisClient.sln (right): https://codereview.appspot.com/220770043/diff/1/GoogleApisClient.sln#newcode51 GoogleApisClient.sln:51: Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" Bahhhhh... Please revert this file completely https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth.DotNet4/O... File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs (right): https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth.DotNet4/O... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:2: Copyright 2013 Google Inc 2015 https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth.DotNet4/O... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:17: using System; Rearrange as following: first system + microsoft then 3rd parties then google using System; using System.Collections; using System.Collections.Generic; using System.IO; using System.Net; using System.Net.Http; using System.Threading; using System.Threading.Tasks; using Newtonsoft.Json.Linq; using Newtonsoft.Json; using Google.Apis.Auth.OAuth2.Flows; using Google.Apis.Auth.OAuth2.Responses; using Google.Apis.Http; using Google.Apis.Logging; https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth.DotNet4/O... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: private GoogleCredential LoadCredentialFromStream(Stream stream, IEnumerable<string> scopes) I want to see a sample using this code. https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth.DotNet4/O... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:132: ClientSecrets clientSecrets = new ClientSecrets() Why don't you use GoogleClientSecrets.Load(stream) It's seems that this solution is not aligned with the one that we currently have: That's the user code for using UserCredential so far: UserCredential credential; using (var stream = new FileStream("client_secrets.json", FileMode.Open, FileAccess.Read)) { credential = await GoogleWebAuthorizationBroker.AuthorizeAsync( GoogleClientSecrets.Load(stream).Secrets, new[] { BooksService.Scope.Books }, "user", CancellationToken.None, new FileDataStore("Books.ListMyLibrary")); } I think you might want to set up a meeting and talk in person :) And maybe it's better to create a design doc first. https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Co... File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): https://codereview.appspot.com/220770043/diff/1/Src/GoogleApis.Auth/OAuth2/Co... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:24: using System.Collections.Generic; ditto
Sign in to reply to this message.
Eyal, I've shared the design details here <https://docs.google.com/a/google.com/document/d/1gKyVT5HFVilL2UMy2e6XsXQ-FL4h...>. I don't have the data on Compute .Net users. Do you know how to get this? If not, I am planning to ask the Compute Engine team for pointers. On Wed, Mar 18, 2015 at 7:01 AM, <peleyal@google.com> wrote: > I started to review, and I stopped in the middle. > I think we need a design doc and some sample code that demonstrates how > this is being used. > > Do we know by the way how many Compute .NET users we have? > > Feel free to set up a meeting in my cal > Thanks! > > > https://codereview.appspot.com/220770043/diff/1/GoogleApisClient.sln > File GoogleApisClient.sln (right): > > https://codereview.appspot.com/220770043/diff/1/ > GoogleApisClient.sln#newcode51 > GoogleApisClient.sln:51: > Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute > Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio > 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute > Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" > Bahhhhh... Please revert this file completely > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs > File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs > (right): > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode2 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:2: > Copyright 2013 Google Inc > 2015 > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode17 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:17: > using System; > Rearrange as following: > first system + microsoft > then 3rd parties > then google > > using System; > using System.Collections; > using System.Collections.Generic; > using System.IO; > using System.Net; > using System.Net.Http; > using System.Threading; > using System.Threading.Tasks; > > using Newtonsoft.Json.Linq; > using Newtonsoft.Json; > > using Google.Apis.Auth.OAuth2.Flows; > using Google.Apis.Auth.OAuth2.Responses; > using Google.Apis.Http; > using Google.Apis.Logging; > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode124 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: > private GoogleCredential LoadCredentialFromStream(Stream stream, > IEnumerable<string> scopes) > I want to see a sample using this code. > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode132 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:132: > ClientSecrets clientSecrets = new ClientSecrets() > Why don't you use GoogleClientSecrets.Load(stream) > > It's seems that this solution is not aligned with the one that we > currently have: > That's the user code for using UserCredential so far: > > UserCredential credential; > using (var stream = new FileStream("client_secrets.json", > FileMode.Open, FileAccess.Read)) > { > credential = await > GoogleWebAuthorizationBroker.AuthorizeAsync( > GoogleClientSecrets.Load(stream).Secrets, > new[] { BooksService.Scope.Books }, > "user", CancellationToken.None, new > FileDataStore("Books.ListMyLibrary")); > } > > > I think you might want to set up a meeting and talk in person :) > And maybe it's better to create a design doc first. > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth/OAuth2/ComputeCredential.cs > File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): > > https://codereview.appspot.com/220770043/diff/1/Src/ > GoogleApis.Auth/OAuth2/ComputeCredential.cs#newcode24 > Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:24: using > System.Collections.Generic; > ditto > > https://codereview.appspot.com/220770043/ >
Sign in to reply to this message.
Can you please give me comment permissions? On Wed, Mar 18, 2015 at 4:27 PM Vijay Subramani <vsubramani@google.com> wrote: > Eyal, > > I've shared the design details here > <https://docs.google.com/a/google.com/document/d/1gKyVT5HFVilL2UMy2e6XsXQ-FL4h...>. > I don't have the data on Compute .Net users. Do you know how to get this? > If not, I am planning to ask the Compute Engine team for pointers. > > > > > On Wed, Mar 18, 2015 at 7:01 AM, <peleyal@google.com> wrote: > >> I started to review, and I stopped in the middle. >> I think we need a design doc and some sample code that demonstrates how >> this is being used. >> >> Do we know by the way how many Compute .NET users we have? >> >> Feel free to set up a meeting in my cal >> Thanks! >> >> >> https://codereview.appspot.com/220770043/diff/1/GoogleApisClient.sln >> File GoogleApisClient.sln (right): >> >> https://codereview.appspot.com/220770043/diff/1/ >> GoogleApisClient.sln#newcode51 >> GoogleApisClient.sln:51: >> Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute >> Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio >> 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute >> Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" >> Bahhhhh... Please revert this file completely >> >> https://codereview.appspot.com/220770043/diff/1/Src/ >> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs >> File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs >> (right): >> >> https://codereview.appspot.com/220770043/diff/1/Src/ >> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode2 >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:2: >> Copyright 2013 Google Inc >> 2015 >> >> https://codereview.appspot.com/220770043/diff/1/Src/ >> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode17 >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:17: >> using System; >> Rearrange as following: >> first system + microsoft >> then 3rd parties >> then google >> >> using System; >> using System.Collections; >> using System.Collections.Generic; >> using System.IO; >> using System.Net; >> using System.Net.Http; >> using System.Threading; >> using System.Threading.Tasks; >> >> using Newtonsoft.Json.Linq; >> using Newtonsoft.Json; >> >> using Google.Apis.Auth.OAuth2.Flows; >> using Google.Apis.Auth.OAuth2.Responses; >> using Google.Apis.Http; >> using Google.Apis.Logging; >> >> https://codereview.appspot.com/220770043/diff/1/Src/ >> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode124 >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: >> private GoogleCredential LoadCredentialFromStream(Stream stream, >> IEnumerable<string> scopes) >> I want to see a sample using this code. >> >> https://codereview.appspot.com/220770043/diff/1/Src/ >> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode132 >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:132: >> ClientSecrets clientSecrets = new ClientSecrets() >> Why don't you use GoogleClientSecrets.Load(stream) >> >> It's seems that this solution is not aligned with the one that we >> currently have: >> That's the user code for using UserCredential so far: >> >> UserCredential credential; >> using (var stream = new FileStream("client_secrets.json", >> FileMode.Open, FileAccess.Read)) >> { >> credential = await >> GoogleWebAuthorizationBroker.AuthorizeAsync( >> GoogleClientSecrets.Load(stream).Secrets, >> new[] { BooksService.Scope.Books }, >> "user", CancellationToken.None, new >> FileDataStore("Books.ListMyLibrary")); >> } >> >> >> I think you might want to set up a meeting and talk in person :) >> And maybe it's better to create a design doc first. >> >> https://codereview.appspot.com/220770043/diff/1/Src/ >> GoogleApis.Auth/OAuth2/ComputeCredential.cs >> File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): >> >> https://codereview.appspot.com/220770043/diff/1/Src/ >> GoogleApis.Auth/OAuth2/ComputeCredential.cs#newcode24 >> Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:24: using >> System.Collections.Generic; >> ditto >> >> https://codereview.appspot.com/220770043/ >> > >
Sign in to reply to this message.
Done. On Wed, Mar 18, 2015 at 2:51 PM, Eyal Peled <peleyal@google.com> wrote: > Can you please give me comment permissions? > > > On Wed, Mar 18, 2015 at 4:27 PM Vijay Subramani <vsubramani@google.com> > wrote: > >> Eyal, >> >> I've shared the design details here >> <https://docs.google.com/a/google.com/document/d/1gKyVT5HFVilL2UMy2e6XsXQ-FL4h...>. >> I don't have the data on Compute .Net users. Do you know how to get this? >> If not, I am planning to ask the Compute Engine team for pointers. >> >> >> >> >> On Wed, Mar 18, 2015 at 7:01 AM, <peleyal@google.com> wrote: >> >>> I started to review, and I stopped in the middle. >>> I think we need a design doc and some sample code that demonstrates how >>> this is being used. >>> >>> Do we know by the way how many Compute .NET users we have? >>> >>> Feel free to set up a meeting in my cal >>> Thanks! >>> >>> >>> https://codereview.appspot.com/220770043/diff/1/GoogleApisClient.sln >>> File GoogleApisClient.sln (right): >>> >>> https://codereview.appspot.com/220770043/diff/1/ >>> GoogleApisClient.sln#newcode51 >>> GoogleApisClient.sln:51: >>> Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute >>> Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio >>> 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute >>> Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" >>> Bahhhhh... Please revert this file completely >>> >>> https://codereview.appspot.com/220770043/diff/1/Src/ >>> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs >>> File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs >>> (right): >>> >>> https://codereview.appspot.com/220770043/diff/1/Src/ >>> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode2 >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:2: >>> Copyright 2013 Google Inc >>> 2015 >>> >>> https://codereview.appspot.com/220770043/diff/1/Src/ >>> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode17 >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:17: >>> using System; >>> Rearrange as following: >>> first system + microsoft >>> then 3rd parties >>> then google >>> >>> using System; >>> using System.Collections; >>> using System.Collections.Generic; >>> using System.IO; >>> using System.Net; >>> using System.Net.Http; >>> using System.Threading; >>> using System.Threading.Tasks; >>> >>> using Newtonsoft.Json.Linq; >>> using Newtonsoft.Json; >>> >>> using Google.Apis.Auth.OAuth2.Flows; >>> using Google.Apis.Auth.OAuth2.Responses; >>> using Google.Apis.Http; >>> using Google.Apis.Logging; >>> >>> https://codereview.appspot.com/220770043/diff/1/Src/ >>> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode124 >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: >>> private GoogleCredential LoadCredentialFromStream(Stream stream, >>> IEnumerable<string> scopes) >>> I want to see a sample using this code. >>> >>> https://codereview.appspot.com/220770043/diff/1/Src/ >>> GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode132 >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:132: >>> ClientSecrets clientSecrets = new ClientSecrets() >>> Why don't you use GoogleClientSecrets.Load(stream) >>> >>> It's seems that this solution is not aligned with the one that we >>> currently have: >>> That's the user code for using UserCredential so far: >>> >>> UserCredential credential; >>> using (var stream = new FileStream("client_secrets.json", >>> FileMode.Open, FileAccess.Read)) >>> { >>> credential = await >>> GoogleWebAuthorizationBroker.AuthorizeAsync( >>> GoogleClientSecrets.Load(stream).Secrets, >>> new[] { BooksService.Scope.Books }, >>> "user", CancellationToken.None, new >>> FileDataStore("Books.ListMyLibrary")); >>> } >>> >>> >>> I think you might want to set up a meeting and talk in person :) >>> And maybe it's better to create a design doc first. >>> >>> https://codereview.appspot.com/220770043/diff/1/Src/ >>> GoogleApis.Auth/OAuth2/ComputeCredential.cs >>> File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): >>> >>> https://codereview.appspot.com/220770043/diff/1/Src/ >>> GoogleApis.Auth/OAuth2/ComputeCredential.cs#newcode24 >>> Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:24: using >>> System.Collections.Generic; >>> ditto >>> >>> https://codereview.appspot.com/220770043/ >>> >> >>
Sign in to reply to this message.
I was expecting the main entry points to be : GoogleCredential.getApplicationDefault and GoogleCredential.fromStream. Since the starting names in Java V1 were the same, without a very good reason we should probably go with those. https://codereview.appspot.com/220770043/diff/40001/GoogleApisClient.sln File GoogleApisClient.sln (right): https://codereview.appspot.com/220770043/diff/40001/GoogleApisClient.sln#newc... GoogleApisClient.sln:51: Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" Presumably this path can be changed before final submit. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs (right): https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:40: public class DefaultCredentialProvider The Java analog of this method was package-private. The factoring was purely to manage the implementation and test mocking. I would expect this to be "internal" in C# and to use "internalsVisbleTo" to enable the unit tests to see it. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:87: return LoadCredentialFromFile(credentialPath, scopes); A consequence of this simple pattern is that if the credentials fail to load, we can't include information about whether the environment variable or the well known file was what triggered this. It may be worth being slightly more verbose and having separate error messages for these 2 cases. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: private Credential LoadCredentialFromStream(Stream stream, IEnumerable<string> scopes) As per the spec for other languages, we have exposed the equivalent of this publicly as GoogleCredential.fromStream and have the default credential provider call into that. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:127: string fileType = jsonObject["type"].ToString(); What will happen if the field is not present. Will that cause a null reference? https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:143: var flow = new GoogleAuthorizationCodeFlow(initializer); This does not seem equivalent to the spec or the other implementations. This is ignoring the saved refresh token in the file and triggering a full 3LO. The behavior should be to just initialize the very end of a flow using the refresh token, id and secret. The intention of the pattern is that you should be able to use it for non-UI code and it won't trigger UI or block on user input. It should look more like finding the type of the credential that the 3LO flow produces and directly populating it. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:104: public async Task<bool> IsRunningOnComputeEngine(CancellationToken taskCancellationToken) I would expect an internal overload of this that takes a mockable transport so that this can be simulated for unit tests. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:106: var httpRequest = new HttpRequestMessage(HttpMethod.Get, GoogleAuthConsts.MetadataServerUrl); The Python implementation may be the best reference: https://github.com/google/oauth2client/blob/master/oauth2client/client.py#L927 We want to use the numeric IP and timeout after 1 second. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:29: public abstract class Credential: IHttpExecuteInterceptor, IHttpUnsuccessfulResponseHandler, If this is not strictly necessary I would leave it out. For these evolved client libraries, we probably want to put this feature in with minimum client surface. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs (right): https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs:48: public const string WELL_KNOWN_CREDENTIALS_FILE = "application_default_credentials.json"; The equivalent of all these new constants in Java was non-public. If possible I would play it safe by making them all internal. Probably the only ones that makes sense here for public use are the ENV_VAR and the HELP_PERMALINK, but I would either give them more complete names or make them internal.
Sign in to reply to this message.
https://codereview.appspot.com/220770043/diff/40001/GoogleApisClient.sln File GoogleApisClient.sln (right): https://codereview.appspot.com/220770043/diff/40001/GoogleApisClient.sln#newc... GoogleApisClient.sln:51: Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" On 2015/03/23 22:55:12, Anthony Moore wrote: > Presumably this path can be changed before final submit. Acknowledged. https://codereview.appspot.com/220770043/diff/40001/GoogleApisClient.sln#newc... GoogleApisClient.sln:51: Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" On 2015/03/23 22:55:12, Anthony Moore wrote: > Presumably this path can be changed before final submit. Yes. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs (right): https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:40: public class DefaultCredentialProvider On 2015/03/23 22:55:12, Anthony Moore wrote: > The Java analog of this method was package-private. The factoring was purely to > manage the implementation and test mocking. I would expect this to be "internal" > in C# and to use "internalsVisbleTo" to enable the unit tests to see it. This would create circular project references. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:87: return LoadCredentialFromFile(credentialPath, scopes); On 2015/03/23 22:55:12, Anthony Moore wrote: > A consequence of this simple pattern is that if the credentials fail to load, we > can't include information about whether the environment variable or the well > known file was what triggered this. It may be worth being slightly more verbose > and having separate error messages for these 2 cases. Acknowledged. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: private Credential LoadCredentialFromStream(Stream stream, IEnumerable<string> scopes) On 2015/03/23 22:55:12, Anthony Moore wrote: > As per the spec for other languages, we have exposed the equivalent of this > publicly as GoogleCredential.fromStream and have the default credential provider > call into that. Acknowledged. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:127: string fileType = jsonObject["type"].ToString(); On 2015/03/23 22:55:12, Anthony Moore wrote: > What will happen if the field is not present. Will that cause a null reference? This will failover to the default case which will throw the IOException. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:127: string fileType = jsonObject["type"].ToString(); On 2015/03/23 22:55:12, Anthony Moore wrote: > What will happen if the field is not present. Will that cause a null reference? Acknowledged. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:127: string fileType = jsonObject["type"].ToString(); On 2015/03/23 22:55:12, Anthony Moore wrote: > What will happen if the field is not present. Will that cause a null reference? Acknowledged. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:143: var flow = new GoogleAuthorizationCodeFlow(initializer); On 2015/03/23 22:55:12, Anthony Moore wrote: > This does not seem equivalent to the spec or the other implementations. This is > ignoring the saved refresh token in the file and triggering a full 3LO. The > behavior should be to just initialize the very end of a flow using the refresh > token, id and secret. The intention of the pattern is that you should be able to > use it for non-UI code and it won't trigger UI or block on user input. It should > look more like finding the type of the credential that the 3LO flow produces and > directly populating it. Agreed. Will make the change. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:143: var flow = new GoogleAuthorizationCodeFlow(initializer); On 2015/03/23 22:55:12, Anthony Moore wrote: > This does not seem equivalent to the spec or the other implementations. This is > ignoring the saved refresh token in the file and triggering a full 3LO. The > behavior should be to just initialize the very end of a flow using the refresh > token, id and secret. The intention of the pattern is that you should be able to > use it for non-UI code and it won't trigger UI or block on user input. It should > look more like finding the type of the credential that the 3LO flow produces and > directly populating it. Acknowledged. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:104: public async Task<bool> IsRunningOnComputeEngine(CancellationToken taskCancellationToken) On 2015/03/23 22:55:12, Anthony Moore wrote: > I would expect an internal overload of this that takes a mockable transport so > that this can be simulated for unit tests. Acknowledged. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:104: public async Task<bool> IsRunningOnComputeEngine(CancellationToken taskCancellationToken) On 2015/03/23 22:55:12, Anthony Moore wrote: > I would expect an internal overload of this that takes a mockable transport so > that this can be simulated for unit tests. I don't see an easy way to have pluggable transports in the current design. I will let Eyal chime in to see what his thoughts were when he introduced the ComputeCredential.ReadAccessTokenAsync method which also doesn't explicitly provide a pluggable transport. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:29: public abstract class Credential: IHttpExecuteInterceptor, IHttpUnsuccessfulResponseHandler, On 2015/03/23 22:55:13, Anthony Moore wrote: > If this is not strictly necessary I would leave it out. For these evolved client > libraries, we probably want to put this feature in with minimum client surface. Acknowledged. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:29: public abstract class Credential: IHttpExecuteInterceptor, IHttpUnsuccessfulResponseHandler, On 2015/03/23 22:55:13, Anthony Moore wrote: > If this is not strictly necessary I would leave it out. For these evolved client > libraries, we probably want to put this feature in with minimum client surface. As we discussed offline, this refactoring is essential if we want to shoot for a common base type. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs (right): https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs:48: public const string WELL_KNOWN_CREDENTIALS_FILE = "application_default_credentials.json"; On 2015/03/23 22:55:13, Anthony Moore wrote: > The equivalent of all these new constants in Java was non-public. If possible I > would play it safe by making them all internal. Probably the only ones that > makes sense here for public use are the ENV_VAR and the HELP_PERMALINK, but I > would either give them more complete names or make them internal. Fixed. https://codereview.appspot.com/220770043/diff/40001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs:48: public const string WELL_KNOWN_CREDENTIALS_FILE = "application_default_credentials.json"; On 2015/03/23 22:55:13, Anthony Moore wrote: > The equivalent of all these new constants in Java was non-public. If possible I > would play it safe by making them all internal. Probably the only ones that > makes sense here for public use are the ENV_VAR and the HELP_PERMALINK, but I > would either give them more complete names or make them internal. Acknowledged.
Sign in to reply to this message.
Updated patch sent for review. On Tue, Mar 24, 2015 at 5:57 PM, <vsubramani@google.com> wrote: > > https://codereview.appspot.com/220770043/diff/40001/GoogleApisClient.sln > File GoogleApisClient.sln (right): > > https://codereview.appspot.com/220770043/diff/40001/ > GoogleApisClient.sln#newcode51 > GoogleApisClient.sln:51: > Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute > Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio > 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute > Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> Presumably this path can be changed before final submit. >> > > Acknowledged. > > https://codereview.appspot.com/220770043/diff/40001/ > GoogleApisClient.sln#newcode51 > GoogleApisClient.sln:51: > Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute > Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio > 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute > Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> Presumably this path can be changed before final submit. >> > > Yes. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs > File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs > (right): > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode40 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:40: > public class DefaultCredentialProvider > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> The Java analog of this method was package-private. The factoring was >> > purely to > >> manage the implementation and test mocking. I would expect this to be >> > "internal" > >> in C# and to use "internalsVisbleTo" to enable the unit tests to see >> > it. > > This would create circular project references. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode87 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:87: > return LoadCredentialFromFile(credentialPath, scopes); > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> A consequence of this simple pattern is that if the credentials fail >> > to load, we > >> can't include information about whether the environment variable or >> > the well > >> known file was what triggered this. It may be worth being slightly >> > more verbose > >> and having separate error messages for these 2 cases. >> > > Acknowledged. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode124 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: > private Credential LoadCredentialFromStream(Stream stream, > IEnumerable<string> scopes) > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> As per the spec for other languages, we have exposed the equivalent of >> > this > >> publicly as GoogleCredential.fromStream and have the default >> > credential provider > >> call into that. >> > > Acknowledged. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode127 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:127: > string fileType = jsonObject["type"].ToString(); > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> What will happen if the field is not present. Will that cause a null >> > reference? > > This will failover to the default case which will throw the IOException. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode127 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:127: > string fileType = jsonObject["type"].ToString(); > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> What will happen if the field is not present. Will that cause a null >> > reference? > > Acknowledged. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode127 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:127: > string fileType = jsonObject["type"].ToString(); > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> What will happen if the field is not present. Will that cause a null >> > reference? > > Acknowledged. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode143 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:143: var > flow = new GoogleAuthorizationCodeFlow(initializer); > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> This does not seem equivalent to the spec or the other >> > implementations. This is > >> ignoring the saved refresh token in the file and triggering a full >> > 3LO. The > >> behavior should be to just initialize the very end of a flow using the >> > refresh > >> token, id and secret. The intention of the pattern is that you should >> > be able to > >> use it for non-UI code and it won't trigger UI or block on user input. >> > It should > >> look more like finding the type of the credential that the 3LO flow >> > produces and > >> directly populating it. >> > > Agreed. Will make the change. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs#newcode143 > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:143: var > flow = new GoogleAuthorizationCodeFlow(initializer); > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> This does not seem equivalent to the spec or the other >> > implementations. This is > >> ignoring the saved refresh token in the file and triggering a full >> > 3LO. The > >> behavior should be to just initialize the very end of a flow using the >> > refresh > >> token, id and secret. The intention of the pattern is that you should >> > be able to > >> use it for non-UI code and it won't trigger UI or block on user input. >> > It should > >> look more like finding the type of the credential that the 3LO flow >> > produces and > >> directly populating it. >> > > Acknowledged. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth/OAuth2/ComputeCredential.cs > File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth/OAuth2/ComputeCredential.cs#newcode104 > Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:104: public async > Task<bool> IsRunningOnComputeEngine(CancellationToken > taskCancellationToken) > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> I would expect an internal overload of this that takes a mockable >> > transport so > >> that this can be simulated for unit tests. >> > > Acknowledged. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth/OAuth2/ComputeCredential.cs#newcode104 > Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:104: public async > Task<bool> IsRunningOnComputeEngine(CancellationToken > taskCancellationToken) > On 2015/03/23 22:55:12, Anthony Moore wrote: > >> I would expect an internal overload of this that takes a mockable >> > transport so > >> that this can be simulated for unit tests. >> > > I don't see an easy way to have pluggable transports in the current > design. I will let Eyal chime in to see what his thoughts were when he > introduced the ComputeCredential.ReadAccessTokenAsync method which also > doesn't explicitly provide a pluggable transport. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth/OAuth2/Credential.cs > File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth/OAuth2/Credential.cs#newcode29 > Src/GoogleApis.Auth/OAuth2/Credential.cs:29: public abstract class > Credential: IHttpExecuteInterceptor, IHttpUnsuccessfulResponseHandler, > On 2015/03/23 22:55:13, Anthony Moore wrote: > >> If this is not strictly necessary I would leave it out. For these >> > evolved client > >> libraries, we probably want to put this feature in with minimum client >> > surface. > > Acknowledged. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth/OAuth2/Credential.cs#newcode29 > Src/GoogleApis.Auth/OAuth2/Credential.cs:29: public abstract class > Credential: IHttpExecuteInterceptor, IHttpUnsuccessfulResponseHandler, > On 2015/03/23 22:55:13, Anthony Moore wrote: > >> If this is not strictly necessary I would leave it out. For these >> > evolved client > >> libraries, we probably want to put this feature in with minimum client >> > surface. > > As we discussed offline, this refactoring is essential if we want to > shoot for a common base type. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth/OAuth2/GoogleConsts.cs > File Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs (right): > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth/OAuth2/GoogleConsts.cs#newcode48 > Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs:48: public const string > WELL_KNOWN_CREDENTIALS_FILE = "application_default_credentials.json"; > On 2015/03/23 22:55:13, Anthony Moore wrote: > >> The equivalent of all these new constants in Java was non-public. If >> > possible I > >> would play it safe by making them all internal. Probably the only ones >> > that > >> makes sense here for public use are the ENV_VAR and the >> > HELP_PERMALINK, but I > >> would either give them more complete names or make them internal. >> > > Fixed. > > https://codereview.appspot.com/220770043/diff/40001/Src/ > GoogleApis.Auth/OAuth2/GoogleConsts.cs#newcode48 > Src/GoogleApis.Auth/OAuth2/GoogleConsts.cs:48: public const string > WELL_KNOWN_CREDENTIALS_FILE = "application_default_credentials.json"; > On 2015/03/23 22:55:13, Anthony Moore wrote: > >> The equivalent of all these new constants in Java was non-public. If >> > possible I > >> would play it safe by making them all internal. Probably the only ones >> > that > >> makes sense here for public use are the ENV_VAR and the >> > HELP_PERMALINK, but I > >> would either give them more complete names or make them internal. >> > > Acknowledged. > > https://codereview.appspot.com/220770043/ >
Sign in to reply to this message.
Good progress. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs (right): https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class GoogleAuth I'd suggest a slightly different factoring to keep implementation details away from customers and help maintenance. In its current form this could be slightly confusing in that a customer might see this instance type and start searching for ways to new it up. GoogleAuth can be a public static class with just the GetApplicationDefault and FromStream public static methods. DefaultCredentialsProvider can be fully internal. Keep the caching, locking and state in DefaultCredentials. This would be closer to the separation advised for Java when it got the initial code review. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:45: const string WELL_KNOWN_CREDENTIALS_FILE = "application_default_credentials.json"; Can you add the visibilty (e.g, private or internal) to these constants? https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:107: Logger.Info(String.Format("Loading Credential from file {0}", credentialPath)); If you have this logging, should it go inside LoadCredentialfromFile instead of being duplicated? https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:117: } There was an acknowledged comment to be able to tread the environment variable and well known file with different exception messages. It looks like the error would still be identical in this case. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:120: bool isRunningOnComputeEngine; I'd suggest a different factoring here. The logic that the detection should be no more than 1 second seems like it should be inside IsRunningOnComputeEngine, and you would want that if you used the method from places other than fetching default credentials. Optimistically creating a ComputeEngine credential in case it turns out to work is a bit confusing to read and could be a maintenance issue, because it is depending on a complete lack of side-effects from that creation. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:167: public static async Task<Credential> LoadCredentialFromStream(Stream stream) This is not a long-running operation and the files have bounded size, so I'm wondering whether it would be simpler to either make this syncrhonous or provide a simple syncrhonous wrapper. I'd suggest naming this FromStream for brevity and consistency with other languages. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs (right): https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs:230: public override Task<Credential> CreateScoped(IEnumerable<string> scopes) This should definitely not be async, because at most it clones an object. The default implementation should "return this". It is normal to have code that calls this but only needs it for some credential types. We could have a default piece of logging to indicate that it was called on a credential type that does not need it and suggest checking IsCreateScopedRequired. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:155: throw new InvalidOperationException("CreateScoped() does not need to be invoked for ComputeCredential. AccessTokens minted for ComputeCredential automatically use the scopes from the Compute instance you are running on."); As above, suggest making this return this, but output a diagnostic message with this information. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:30: /// Base type for credentials for authorizing calls to Google APIs using OAuth2. Great abstraction. I think you could remove "to Google APIs" from the comment since scopes are a general Oauth2 concept. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:64: public abstract bool IsCreateScopedRequired { get; } I think we could perhaps provide default implementations of False for IsCreateScopedRequired. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:66: public abstract Task<Credential> CreateScoped(IEnumerable<string> scopes); As above, suggest that this be synchronous and return this by default https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/UserCredential.cs (right): https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/UserCredential.cs:191: public override bool IsCreateScopedRequired Expecting return false. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/UserCredential.cs:204: public override async Task<Credential> CreateScoped(IEnumerable<string> scopes) Not expecting this to trigger a 3LO flow. As per the other language implementations, the user credentials should just return this. You could have a diagnostic warning that indicating that IsCreateScopedRequired should be checked. The whole feature is meant to be usable from non-UI code. We can't have it popping up browsers and blocking on users.
Sign in to reply to this message.
https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs (right): https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:45: const string WELL_KNOWN_CREDENTIALS_FILE = "application_default_credentials.json"; On 2015/03/25 18:01:59, Anthony Moore wrote: > Can you add the visibilty (e.g, private or internal) to these constants? Acknowledged. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:45: const string WELL_KNOWN_CREDENTIALS_FILE = "application_default_credentials.json"; On 2015/03/25 18:01:59, Anthony Moore wrote: > Can you add the visibilty (e.g, private or internal) to these constants? The default in C# is internal. Hence I left it at that. I will add it explicity, if it makes it clearer. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:107: Logger.Info(String.Format("Loading Credential from file {0}", credentialPath)); On 2015/03/25 18:01:58, Anthony Moore wrote: > If you have this logging, should it go inside LoadCredentialfromFile instead of > being duplicated? Acknowledged. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:117: } On 2015/03/25 18:01:59, Anthony Moore wrote: > There was an acknowledged comment to be able to tread the environment variable > and well known file with different exception messages. It looks like the error > would still be identical in this case. The acknowledgement was for having a way to differentiate whether the failure was from the well known file or the file specified in the env var. There wasn't any mention of having different exception messages. I believe that logging the path to where we are loading the Credential file from should provide this information. Let me know if you don't feel it is sufficient. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:120: bool isRunningOnComputeEngine; On 2015/03/25 18:01:59, Anthony Moore wrote: > I'd suggest a different factoring here. The logic that the detection should be > no more than 1 second seems like it should be inside IsRunningOnComputeEngine, > and you would want that if you used the method from places other than fetching > default credentials. Optimistically creating a ComputeEngine credential in case > it turns out to work is a bit confusing to read and could be a maintenance > issue, because it is depending on a complete lack of side-effects from that > creation. Acknowledged. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:167: public static async Task<Credential> LoadCredentialFromStream(Stream stream) On 2015/03/25 18:01:59, Anthony Moore wrote: > This is not a long-running operation and the files have bounded size, so I'm > wondering whether it would be simpler to either make this syncrhonous or provide > a simple syncrhonous wrapper. > > I'd suggest naming this FromStream for brevity and consistency with other > languages. In other languages, this method was on the Credentials type, hence the FromStream() indicated what exactly you were loading from the stream. However in .Net, since this method is not on the Credential type, I want to make it explicitly clear that we are loading the Credential from the stream. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs (right): https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs:230: public override Task<Credential> CreateScoped(IEnumerable<string> scopes) On 2015/03/25 18:01:59, Anthony Moore wrote: > This should definitely not be async, because at most it clones an object. The > default implementation should "return this". It is normal to have code that > calls this but only needs it for some credential types. We could have a default > piece of logging to indicate that it was called on a credential type that does > not need it and suggest checking IsCreateScopedRequired. I will change it to "return this". The CreateScoped is an override and override is async since UserCredential.CreateScoped needs to be async. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:30: /// Base type for credentials for authorizing calls to Google APIs using OAuth2. On 2015/03/25 18:01:59, Anthony Moore wrote: > Great abstraction. I think you could remove "to Google APIs" from the comment > since scopes are a general Oauth2 concept. Acknowledged. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:64: public abstract bool IsCreateScopedRequired { get; } On 2015/03/25 18:01:59, Anthony Moore wrote: > I think we could perhaps provide default implementations of False for > IsCreateScopedRequired. Acknowledged. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:66: public abstract Task<Credential> CreateScoped(IEnumerable<string> scopes); On 2015/03/25 18:01:59, Anthony Moore wrote: > As above, suggest that this be synchronous and return this by default Since UserCredential makes network calls, I prefer to leave this async. Let me know if you strongly feel otherwise. https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/UserCredential.cs (right): https://codereview.appspot.com/220770043/diff/60001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/UserCredential.cs:204: public override async Task<Credential> CreateScoped(IEnumerable<string> scopes) On 2015/03/25 18:01:59, Anthony Moore wrote: > Not expecting this to trigger a 3LO flow. As per the other language > implementations, the user credentials should just return this. You could have a > diagnostic warning that indicating that IsCreateScopedRequired should be > checked. > > The whole feature is meant to be usable from non-UI code. We can't have it > popping up browsers and blocking on users. Acknowledged.
Sign in to reply to this message.
Looking good. May be a good time to add unit tests. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:107: } Regarding the issue that these two code-paths will return identical exceptions: I do think it is important to have different exception messages, even if we also have a way of detecting this if you opt-in to the logger. The failure paths here are fairly likely to get hit as people go through the getting started experience and we have explicit feedback that more specific exceptions for Auth is important during that experience. For example, if the cause was the environment varialble, the corrective action is to check the value of the variable. If the well-known file is the problem, you may want to delete the file or re-run a gcloud auth login command. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: ComputeCredential credential = new ComputeCredential(); This factoring is better, but there is still the issue that the compute credential is created optimistically and then discarded. Moving that into the if block would improve readability, and decrease fragility over time as this usage assumes there will never be side-effects in the compute credentials constructor. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class GoogleAuth Suggest "public static class GoogleAuth". It will clarify that this is just for static methods. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:44: public static Credential GetApplicationDefaultCredential() I'd suggest at least a minimal doc-comment for these 2 methods. It looks like this practice is inconsistent in this library so far, but these 2 are important entry points and at least the top-line pops up in intellisense in Visual Studio, and it would be good to make this a standard practise for new public methods as it is in the other langauges. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs:217: public bool IsCreateScopedRequired { get { return true; } } As long as this is not implemented, I would leave this false. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:121: if (!response.Headers.TryGetValues("Metadata-Flavor", out headerValues)) The logger warning below is great because this would be a genuinely unusual condition. It would be good to fall into it in both the case where the header value is missing and when it has the wrong value. Consider switching this to something like: if (response.Headers.TryGetValues("Metadata-Flavor", out headerValues)) { foreach (var value in headerValues) { if (value == "Google") return true; } } https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:135: Logger.Warning(String.Format("Could not reach the ComputeEngine Metadata service. Obtained HttpRequestException {0}", e.Message)); Of the logger warnings here, I think that this one should perhaps be removed. It's an non-error condition running off GCE. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:154: + " Hence returning the same object."); This is a really good log message. Some tweaks might be that I think we could remove the "Hence returning.." sentance and perhapcs suggest to check IsCreateScopedRequired to turn this off. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:66: public abstract Task<Credential> CreateScoped(IEnumerable<string> scopes); I's suggest a default implementation of "return this" also. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/UserCredential.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/UserCredential.cs:195: + " Hence returning the same object."); Similar suggestion to the Compute Case. We could perhaps tweak the second sentence to "UserCredential uses the scopes from the constent prompt when the Refresh Token was created."
Sign in to reply to this message.
First comment only on 2-3 files. Rest will come soon https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln File GoogleApisClient.sln (right): https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln#newc... GoogleApisClient.sln:51: Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" Please revert. CAn you also attach somewhere a sample of the final code of the end developer? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:2: Copyright 2013 Google Inc 2015 https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:20: using System.Threading; Please reorder, first all system ones then 3rd parties then Google, as following: using System; using System.Collections; using System.Collections.Generic; using System.IO; using System.Net; using System.Net.Http; using System.Threading; using System.Threading.Tasks; using Newtonsoft.Json; using Newtonsoft.Json.Linq; using Google.Apis.Auth.OAuth2.Flows; using Google.Apis.Auth.OAuth2.Responses; using Google.Apis.Http; using Google.Apis.Logging; https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:36: /// Provides the Application Default Credential from the environment. Can you add examples? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:40: class DefaultCredentialProvider Does it not public in purpose? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:65: // These variables should only be accessed inside a synchronized block This variable? Dot in the end https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:75: lock (this) It's better to add an object and don't lock this. http://stackoverflow.com/questions/251391/why-is-lockthis-bad https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:75: lock (this) Consider adding another if (cachedCredential == null) before the lock. you don't want to lock every time you enter to this function. http://en.wikipedia.org/wiki/Double-checked_locking https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:79: cachedCredential = GetDefaultCredentialUnsynchronized().Result; I don't recommend using Result at all. Why can't this method use async-await? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:83: if (cachedCredential != null) Please add {} https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:93: private async Task<Credential> GetDefaultCredentialUnsynchronized() Please add a comment to each method https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:93: private async Task<Credential> GetDefaultCredentialUnsynchronized() I don't understand the flow of this method. Please provide a comment that explain what we except the environment to look like (which files we expecting to have) in each case. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:95: // First try the environment variable Dot in the end of line. Here and there. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:95: // First try the environment variable Please explain - environment variable is used on what environment? It is part of the sdk installation? Is it available only on compute? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: ComputeCredential credential = new ComputeCredential(); Weird pattern here. You create a credential and then calling a function on it. Why not having a factory method or something similar? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: private string GetWellKnownCredentialFilePath() Please add a comment https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:150: JObject jsonObject = JsonConvert.DeserializeObject<JObject>(await (new StreamReader(stream)).ReadToEndAsync()); Can you please reuse the following method - https://github.com/google/google-api-dotnet-client/blob/master/Src/GoogleApis... If something is missing maybe we should add it and extend client secrets (we can talk about it...) I see that you use "type" later on. Is it part of the client secrets file? What spec are you following? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:163: { what is this type of file? How do we get it? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:186: catch(IOException e) space between catch and ( https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class GoogleAuth Why do we need this class? Why DefaultCredentialProvider isn't enough? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs:224: public override Task<Credential> CreateScoped(IEnumerable<string> scopes) bahhhh... Don't like this pattern. Wrong use of interface!
Sign in to reply to this message.
https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln File GoogleApisClient.sln (right): https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln#newc... GoogleApisClient.sln:51: Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" On 2015/04/15 19:57:06, peleyal wrote: > Please revert. > CAn you also attach somewhere a sample of the final code of the end developer? Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:2: Copyright 2013 Google Inc On 2015/04/15 19:57:06, peleyal wrote: > 2015 Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:20: using System.Threading; On 2015/04/15 19:57:07, peleyal wrote: > Please reorder, first all system ones > then 3rd parties then Google, as following: > > using System; > using System.Collections; > using System.Collections.Generic; > using System.IO; > using System.Net; > using System.Net.Http; > using System.Threading; > using System.Threading.Tasks; > > using Newtonsoft.Json; > using Newtonsoft.Json.Linq; > > using Google.Apis.Auth.OAuth2.Flows; > using Google.Apis.Auth.OAuth2.Responses; > using Google.Apis.Http; > using Google.Apis.Logging; Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:75: lock (this) On 2015/04/15 19:57:06, peleyal wrote: > It's better to add an object and don't lock this. > http://stackoverflow.com/questions/251391/why-is-lockthis-bad Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:79: cachedCredential = GetDefaultCredentialUnsynchronized().Result; On 2015/04/15 19:57:06, peleyal wrote: > I don't recommend using Result at all. Why can't this method use async-await? This was based on an earlier comment from Anthony to not expose the Async pattern on the public surface. I can move it one level up to the GoogleAuth class, but that wouldn't anything much since this line will be executed just once in the lifetime of the application domain. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:83: if (cachedCredential != null) On 2015/04/15 19:57:06, peleyal wrote: > Please add {} Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:93: private async Task<Credential> GetDefaultCredentialUnsynchronized() On 2015/04/15 19:57:06, peleyal wrote: > I don't understand the flow of this method. Please provide a comment that > explain what we except the environment to look like (which files we expecting to > have) in each case. I am not sure I understand what you are looking for? The method already has comments indicating that it first looks at the file referenced by the environment variable, then it looks for a well known file location, failing which it checks whether it is running in compute engine. Was there more explanation you were expecting? https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:95: // First try the environment variable On 2015/04/15 19:57:06, peleyal wrote: > Please explain - environment variable is used on what environment? It is part > of the sdk installation? Is it available only on compute? This is covered by the design doc and the feature documentation. This environment variable will be set by the developer, if they need to have explicit control over where the library looks for the credentials file - this potentially will be useful if the developer runs this code in AWS, Azure etc. The "How Application Default Credentials Work" section in the public documentation covers this is a bit more detail - https://developers.google.com/identity/protocols/application-default-credentials https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:107: } On 2015/03/26 17:40:02, Anthony Moore wrote: > Regarding the issue that these two code-paths will return identical exceptions: > I do think it is important to have different exception messages, even if we also > have a way of detecting this if you opt-in to the logger. The failure paths here > are fairly likely to get hit as people go through the getting started experience > and we have explicit feedback that more specific exceptions for Auth is > important during that experience. For example, if the cause was the environment > varialble, the corrective action is to check the value of the variable. If the > well-known file is the problem, you may want to delete the file or re-run a > gcloud auth login command. Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: ComputeCredential credential = new ComputeCredential(); On 2015/03/26 17:40:02, Anthony Moore wrote: > This factoring is better, but there is still the issue that the compute > credential is created optimistically and then discarded. Moving that into the if > block would improve readability, and decrease fragility over time as this usage > assumes there will never be side-effects in the compute credentials constructor. Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: ComputeCredential credential = new ComputeCredential(); On 2015/04/15 19:57:06, peleyal wrote: > Weird pattern here. You create a credential and then calling a function on it. > Why not having a factory method or something similar? Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: private string GetWellKnownCredentialFilePath() On 2015/04/15 19:57:06, peleyal wrote: > Please add a comment Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:150: JObject jsonObject = JsonConvert.DeserializeObject<JObject>(await (new StreamReader(stream)).ReadToEndAsync()); On 2015/04/15 19:57:07, peleyal wrote: > Can you please reuse the following method - > https://github.com/google/google-api-dotnet-client/blob/master/Src/GoogleApis... > If something is missing maybe we should add it and extend client secrets (we can > talk about it...) > > I see that you use "type" later on. Is it part of the client secrets file? What > spec are you following? This Credential file format is different from the Client Secrets file format. @Anthony - do you know whether the json file formats for the credentials are available in public documentation. I searched for it and couldn't find it. If it really doesn't exist, then lets see where would be the best place to document it. @Eyal - I had identified the file format by downloading it from the dev console and opening it :) https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:163: { On 2015/04/15 19:57:06, peleyal wrote: > what is this type of file? How do we get it? This would be auto generated by the gcloud sdk when the developer runs 'gcloud auth login' Added comment to the code to reflect this. Also done the same for the Service Account File case below. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:186: catch(IOException e) On 2015/04/15 19:57:06, peleyal wrote: > space between catch and ( Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class GoogleAuth On 2015/03/26 17:40:02, Anthony Moore wrote: > Suggest "public static class GoogleAuth". It will clarify that this is just for > static methods. Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class GoogleAuth On 2015/04/15 19:57:07, peleyal wrote: > Why do we need this class? Why DefaultCredentialProvider isn't enough? The DefaultCredentialProvider is not public. It is meant for internal testability (which is yet to be added though). https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:44: public static Credential GetApplicationDefaultCredential() On 2015/03/26 17:40:02, Anthony Moore wrote: > I'd suggest at least a minimal doc-comment for these 2 methods. It looks like > this practice is inconsistent in this library so far, but these 2 are important > entry points and at least the top-line pops up in intellisense in Visual Studio, > and it would be good to make this a standard practise for new public methods as > it is in the other langauges. Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... File Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs:217: public bool IsCreateScopedRequired { get { return true; } } On 2015/03/26 17:40:02, Anthony Moore wrote: > As long as this is not implemented, I would leave this false. Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:121: if (!response.Headers.TryGetValues("Metadata-Flavor", out headerValues)) On 2015/03/26 17:40:03, Anthony Moore wrote: > The logger warning below is great because this would be a genuinely unusual > condition. It would be good to fall into it in both the case where the header > value is missing and when it has the wrong value. Consider switching this to > something like: > > if (response.Headers.TryGetValues("Metadata-Flavor", out headerValues)) { > foreach (var value in headerValues) > { > if (value == "Google") > return true; > } > } Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:135: Logger.Warning(String.Format("Could not reach the ComputeEngine Metadata service. Obtained HttpRequestException {0}", e.Message)); On 2015/03/26 17:40:03, Anthony Moore wrote: > Of the logger warnings here, I think that this one should perhaps be removed. > It's an non-error condition running off GCE. Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:154: + " Hence returning the same object."); On 2015/03/26 17:40:03, Anthony Moore wrote: > This is a really good log message. Some tweaks might be that I think we could > remove the "Hence returning.." sentance and perhapcs suggest to check > IsCreateScopedRequired to turn this off. Acknowledged. https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... Src/GoogleApis.Auth/OAuth2/Credential.cs:66: public abstract Task<Credential> CreateScoped(IEnumerable<string> scopes); On 2015/03/26 17:40:03, Anthony Moore wrote: > I's suggest a default implementation of "return this" also. Acknowledged.
Sign in to reply to this message.
I am preparing these changes against the github repo. Currently figuring out the github process. Expect a github based CR on this in a couple of hours. On Mon, May 18, 2015 at 10:31 AM, <vsubramani@google.com> wrote: > > https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln > File GoogleApisClient.sln (right): > > > https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln#newc... > GoogleApisClient.sln:51: > Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute > Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio > 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute > Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" > On 2015/04/15 19:57:06, peleyal wrote: > >> Please revert. >> CAn you also attach somewhere a sample of the final code of the end >> > developer? > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs > (right): > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:2: > Copyright 2013 Google Inc > On 2015/04/15 19:57:06, peleyal wrote: > >> 2015 >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:20: > using System.Threading; > On 2015/04/15 19:57:07, peleyal wrote: > >> Please reorder, first all system ones >> then 3rd parties then Google, as following: >> > > using System; >> using System.Collections; >> using System.Collections.Generic; >> using System.IO; >> using System.Net; >> using System.Net.Http; >> using System.Threading; >> using System.Threading.Tasks; >> > > using Newtonsoft.Json; >> using Newtonsoft.Json.Linq; >> > > using Google.Apis.Auth.OAuth2.Flows; >> using Google.Apis.Auth.OAuth2.Responses; >> using Google.Apis.Http; >> using Google.Apis.Logging; >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:75: lock > (this) > On 2015/04/15 19:57:06, peleyal wrote: > >> It's better to add an object and don't lock this. >> http://stackoverflow.com/questions/251391/why-is-lockthis-bad >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:79: > cachedCredential = GetDefaultCredentialUnsynchronized().Result; > On 2015/04/15 19:57:06, peleyal wrote: > >> I don't recommend using Result at all. Why can't this method use >> > async-await? > > This was based on an earlier comment from Anthony to not expose the > Async pattern on the public surface. I can move it one level up to the > GoogleAuth class, but that wouldn't anything much since this line will > be executed just once in the lifetime of the application domain. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:83: if > (cachedCredential != null) > On 2015/04/15 19:57:06, peleyal wrote: > >> Please add {} >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:93: > private async Task<Credential> GetDefaultCredentialUnsynchronized() > On 2015/04/15 19:57:06, peleyal wrote: > >> I don't understand the flow of this method. Please provide a comment >> > that > >> explain what we except the environment to look like (which files we >> > expecting to > >> have) in each case. >> > > I am not sure I understand what you are looking for? The method already > has comments indicating that it first looks at the file referenced by > the environment variable, then it looks for a well known file location, > failing which it checks whether it is running in compute engine. Was > there more explanation you were expecting? > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:95: // > First try the environment variable > On 2015/04/15 19:57:06, peleyal wrote: > >> Please explain - environment variable is used on what environment? It >> > is part > >> of the sdk installation? Is it available only on compute? >> > > This is covered by the design doc and the feature documentation. This > environment variable will be set by the developer, if they need to have > explicit control over where the library looks for the credentials file - > this potentially will be useful if the developer runs this code in AWS, > Azure etc. The "How Application Default Credentials Work" section in the > public documentation covers this is a bit more detail - > > https://developers.google.com/identity/protocols/application-default-credentials > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:107: } > On 2015/03/26 17:40:02, Anthony Moore wrote: > >> Regarding the issue that these two code-paths will return identical >> > exceptions: > >> I do think it is important to have different exception messages, even >> > if we also > >> have a way of detecting this if you opt-in to the logger. The failure >> > paths here > >> are fairly likely to get hit as people go through the getting started >> > experience > >> and we have explicit feedback that more specific exceptions for Auth >> > is > >> important during that experience. For example, if the cause was the >> > environment > >> varialble, the corrective action is to check the value of the >> > variable. If the > >> well-known file is the problem, you may want to delete the file or >> > re-run a > >> gcloud auth login command. >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: > ComputeCredential credential = new ComputeCredential(); > On 2015/03/26 17:40:02, Anthony Moore wrote: > >> This factoring is better, but there is still the issue that the >> > compute > >> credential is created optimistically and then discarded. Moving that >> > into the if > >> block would improve readability, and decrease fragility over time as >> > this usage > >> assumes there will never be side-effects in the compute credentials >> > constructor. > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: > ComputeCredential credential = new ComputeCredential(); > On 2015/04/15 19:57:06, peleyal wrote: > >> Weird pattern here. You create a credential and then calling a >> > function on it. > >> Why not having a factory method or something similar? >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: > private string GetWellKnownCredentialFilePath() > On 2015/04/15 19:57:06, peleyal wrote: > >> Please add a comment >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:150: > JObject jsonObject = JsonConvert.DeserializeObject<JObject>(await (new > StreamReader(stream)).ReadToEndAsync()); > On 2015/04/15 19:57:07, peleyal wrote: > >> Can you please reuse the following method - >> > > > https://github.com/google/google-api-dotnet-client/blob/master/Src/GoogleApis... > ? > >> If something is missing maybe we should add it and extend client >> > secrets (we can > >> talk about it...) >> > > I see that you use "type" later on. Is it part of the client secrets >> > file? What > >> spec are you following? >> > > This Credential file format is different from the Client Secrets file > format. > > @Anthony - do you know whether the json file formats for the credentials > are available in public documentation. I searched for it and couldn't > find it. If it really doesn't exist, then lets see where would be the > best place to document it. > > @Eyal - I had identified the file format by downloading it from the dev > console and opening it :) > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:163: { > On 2015/04/15 19:57:06, peleyal wrote: > >> what is this type of file? How do we get it? >> > > This would be auto generated by the gcloud sdk when the developer runs > 'gcloud auth login' > > Added comment to the code to reflect this. > > Also done the same for the Service Account File case below. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:186: > catch(IOException e) > On 2015/04/15 19:57:06, peleyal wrote: > >> space between catch and ( >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs (right): > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class > GoogleAuth > On 2015/03/26 17:40:02, Anthony Moore wrote: > >> Suggest "public static class GoogleAuth". It will clarify that this is >> > just for > >> static methods. >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class > GoogleAuth > On 2015/04/15 19:57:07, peleyal wrote: > >> Why do we need this class? Why DefaultCredentialProvider isn't enough? >> > > The DefaultCredentialProvider is not public. It is meant for internal > testability (which is yet to be added though). > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:44: public static > Credential GetApplicationDefaultCredential() > On 2015/03/26 17:40:02, Anthony Moore wrote: > >> I'd suggest at least a minimal doc-comment for these 2 methods. It >> > looks like > >> this practice is inconsistent in this library so far, but these 2 are >> > important > >> entry points and at least the top-line pops up in intellisense in >> > Visual Studio, > >> and it would be good to make this a standard practise for new public >> > methods as > >> it is in the other langauges. >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > File Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs > (right): > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... > Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs:217: > public bool IsCreateScopedRequired { get { return true; } } > On 2015/03/26 17:40:02, Anthony Moore wrote: > >> As long as this is not implemented, I would leave this false. >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... > File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... > Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:121: if > (!response.Headers.TryGetValues("Metadata-Flavor", out headerValues)) > On 2015/03/26 17:40:03, Anthony Moore wrote: > >> The logger warning below is great because this would be a genuinely >> > unusual > >> condition. It would be good to fall into it in both the case where the >> > header > >> value is missing and when it has the wrong value. Consider switching >> > this to > >> something like: >> > > if (response.Headers.TryGetValues("Metadata-Flavor", out >> > headerValues)) { > >> foreach (var value in headerValues) >> { >> if (value == "Google") >> return true; >> } >> } >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... > Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:135: > Logger.Warning(String.Format("Could not reach the ComputeEngine Metadata > service. Obtained HttpRequestException {0}", e.Message)); > On 2015/03/26 17:40:03, Anthony Moore wrote: > >> Of the logger warnings here, I think that this one should perhaps be >> > removed. > >> It's an non-error condition running off GCE. >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... > Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:154: + " Hence returning > the same object."); > On 2015/03/26 17:40:03, Anthony Moore wrote: > >> This is a really good log message. Some tweaks might be that I think >> > we could > >> remove the "Hence returning.." sentance and perhapcs suggest to check >> IsCreateScopedRequired to turn this off. >> > > Acknowledged. > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... > File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): > > > https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... > Src/GoogleApis.Auth/OAuth2/Credential.cs:66: public abstract > Task<Credential> CreateScoped(IEnumerable<string> scopes); > On 2015/03/26 17:40:03, Anthony Moore wrote: > >> I's suggest a default implementation of "return this" also. >> > > Acknowledged. > > https://codereview.appspot.com/220770043/ >
Sign in to reply to this message.
Can you please keep uploading to the original thread? Instructions are in: https://developers.google.com/api-client-library/dotnet/contribute Use the upload command as following: upload.py --rev=HEAD -i 220770043 On Mon, May 18, 2015 at 2:03 PM Vijay Subramani <vsubramani@google.com> wrote: > I am preparing these changes against the github repo. Currently figuring > out the github process. Expect a github based CR on this in a couple of > hours. > > On Mon, May 18, 2015 at 10:31 AM, <vsubramani@google.com> wrote: > >> >> https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln >> File GoogleApisClient.sln (right): >> >> >> https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln#newc... >> GoogleApisClient.sln:51: >> Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute >> Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio >> 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute >> Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> Please revert. >>> CAn you also attach somewhere a sample of the final code of the end >>> >> developer? >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs >> (right): >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:2: >> Copyright 2013 Google Inc >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> 2015 >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:20: >> using System.Threading; >> On 2015/04/15 19:57:07, peleyal wrote: >> >>> Please reorder, first all system ones >>> then 3rd parties then Google, as following: >>> >> >> using System; >>> using System.Collections; >>> using System.Collections.Generic; >>> using System.IO; >>> using System.Net; >>> using System.Net.Http; >>> using System.Threading; >>> using System.Threading.Tasks; >>> >> >> using Newtonsoft.Json; >>> using Newtonsoft.Json.Linq; >>> >> >> using Google.Apis.Auth.OAuth2.Flows; >>> using Google.Apis.Auth.OAuth2.Responses; >>> using Google.Apis.Http; >>> using Google.Apis.Logging; >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:75: lock >> (this) >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> It's better to add an object and don't lock this. >>> http://stackoverflow.com/questions/251391/why-is-lockthis-bad >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:79: >> cachedCredential = GetDefaultCredentialUnsynchronized().Result; >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> I don't recommend using Result at all. Why can't this method use >>> >> async-await? >> >> This was based on an earlier comment from Anthony to not expose the >> Async pattern on the public surface. I can move it one level up to the >> GoogleAuth class, but that wouldn't anything much since this line will >> be executed just once in the lifetime of the application domain. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:83: if >> (cachedCredential != null) >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> Please add {} >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:93: >> private async Task<Credential> GetDefaultCredentialUnsynchronized() >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> I don't understand the flow of this method. Please provide a comment >>> >> that >> >>> explain what we except the environment to look like (which files we >>> >> expecting to >> >>> have) in each case. >>> >> >> I am not sure I understand what you are looking for? The method already >> has comments indicating that it first looks at the file referenced by >> the environment variable, then it looks for a well known file location, >> failing which it checks whether it is running in compute engine. Was >> there more explanation you were expecting? >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:95: // >> First try the environment variable >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> Please explain - environment variable is used on what environment? It >>> >> is part >> >>> of the sdk installation? Is it available only on compute? >>> >> >> This is covered by the design doc and the feature documentation. This >> environment variable will be set by the developer, if they need to have >> explicit control over where the library looks for the credentials file - >> this potentially will be useful if the developer runs this code in AWS, >> Azure etc. The "How Application Default Credentials Work" section in the >> public documentation covers this is a bit more detail - >> >> https://developers.google.com/identity/protocols/application-default-credentials >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:107: } >> On 2015/03/26 17:40:02, Anthony Moore wrote: >> >>> Regarding the issue that these two code-paths will return identical >>> >> exceptions: >> >>> I do think it is important to have different exception messages, even >>> >> if we also >> >>> have a way of detecting this if you opt-in to the logger. The failure >>> >> paths here >> >>> are fairly likely to get hit as people go through the getting started >>> >> experience >> >>> and we have explicit feedback that more specific exceptions for Auth >>> >> is >> >>> important during that experience. For example, if the cause was the >>> >> environment >> >>> varialble, the corrective action is to check the value of the >>> >> variable. If the >> >>> well-known file is the problem, you may want to delete the file or >>> >> re-run a >> >>> gcloud auth login command. >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: >> ComputeCredential credential = new ComputeCredential(); >> On 2015/03/26 17:40:02, Anthony Moore wrote: >> >>> This factoring is better, but there is still the issue that the >>> >> compute >> >>> credential is created optimistically and then discarded. Moving that >>> >> into the if >> >>> block would improve readability, and decrease fragility over time as >>> >> this usage >> >>> assumes there will never be side-effects in the compute credentials >>> >> constructor. >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: >> ComputeCredential credential = new ComputeCredential(); >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> Weird pattern here. You create a credential and then calling a >>> >> function on it. >> >>> Why not having a factory method or something similar? >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: >> private string GetWellKnownCredentialFilePath() >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> Please add a comment >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:150: >> JObject jsonObject = JsonConvert.DeserializeObject<JObject>(await (new >> StreamReader(stream)).ReadToEndAsync()); >> On 2015/04/15 19:57:07, peleyal wrote: >> >>> Can you please reuse the following method - >>> >> >> >> https://github.com/google/google-api-dotnet-client/blob/master/Src/GoogleApis... >> ? >> >>> If something is missing maybe we should add it and extend client >>> >> secrets (we can >> >>> talk about it...) >>> >> >> I see that you use "type" later on. Is it part of the client secrets >>> >> file? What >> >>> spec are you following? >>> >> >> This Credential file format is different from the Client Secrets file >> format. >> >> @Anthony - do you know whether the json file formats for the credentials >> are available in public documentation. I searched for it and couldn't >> find it. If it really doesn't exist, then lets see where would be the >> best place to document it. >> >> @Eyal - I had identified the file format by downloading it from the dev >> console and opening it :) >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:163: { >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> what is this type of file? How do we get it? >>> >> >> This would be auto generated by the gcloud sdk when the developer runs >> 'gcloud auth login' >> >> Added comment to the code to reflect this. >> >> Also done the same for the Service Account File case below. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:186: >> catch(IOException e) >> On 2015/04/15 19:57:06, peleyal wrote: >> >>> space between catch and ( >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs (right): >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class >> GoogleAuth >> On 2015/03/26 17:40:02, Anthony Moore wrote: >> >>> Suggest "public static class GoogleAuth". It will clarify that this is >>> >> just for >> >>> static methods. >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class >> GoogleAuth >> On 2015/04/15 19:57:07, peleyal wrote: >> >>> Why do we need this class? Why DefaultCredentialProvider isn't enough? >>> >> >> The DefaultCredentialProvider is not public. It is meant for internal >> testability (which is yet to be added though). >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:44: public static >> Credential GetApplicationDefaultCredential() >> On 2015/03/26 17:40:02, Anthony Moore wrote: >> >>> I'd suggest at least a minimal doc-comment for these 2 methods. It >>> >> looks like >> >>> this practice is inconsistent in this library so far, but these 2 are >>> >> important >> >>> entry points and at least the top-line pops up in intellisense in >>> >> Visual Studio, >> >>> and it would be good to make this a standard practise for new public >>> >> methods as >> >>> it is in the other langauges. >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> File Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs >> (right): >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >> Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs:217: >> public bool IsCreateScopedRequired { get { return true; } } >> On 2015/03/26 17:40:02, Anthony Moore wrote: >> >>> As long as this is not implemented, I would leave this false. >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >> File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >> Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:121: if >> (!response.Headers.TryGetValues("Metadata-Flavor", out headerValues)) >> On 2015/03/26 17:40:03, Anthony Moore wrote: >> >>> The logger warning below is great because this would be a genuinely >>> >> unusual >> >>> condition. It would be good to fall into it in both the case where the >>> >> header >> >>> value is missing and when it has the wrong value. Consider switching >>> >> this to >> >>> something like: >>> >> >> if (response.Headers.TryGetValues("Metadata-Flavor", out >>> >> headerValues)) { >> >>> foreach (var value in headerValues) >>> { >>> if (value == "Google") >>> return true; >>> } >>> } >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >> Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:135: >> Logger.Warning(String.Format("Could not reach the ComputeEngine Metadata >> service. Obtained HttpRequestException {0}", e.Message)); >> On 2015/03/26 17:40:03, Anthony Moore wrote: >> >>> Of the logger warnings here, I think that this one should perhaps be >>> >> removed. >> >>> It's an non-error condition running off GCE. >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >> Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:154: + " Hence returning >> the same object."); >> On 2015/03/26 17:40:03, Anthony Moore wrote: >> >>> This is a really good log message. Some tweaks might be that I think >>> >> we could >> >>> remove the "Hence returning.." sentance and perhapcs suggest to check >>> IsCreateScopedRequired to turn this off. >>> >> >> Acknowledged. >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >> File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): >> >> >> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >> Src/GoogleApis.Auth/OAuth2/Credential.cs:66: public abstract >> Task<Credential> CreateScoped(IEnumerable<string> scopes); >> On 2015/03/26 17:40:03, Anthony Moore wrote: >> >>> I's suggest a default implementation of "return this" also. >>> >> >> Acknowledged. >> >> https://codereview.appspot.com/220770043/ >> > >
Sign in to reply to this message.
Eyal, the original CR upload was against the code.google.com repo. Now, since this has moved to git, I had moved the changes over to git. However upload.py doesn't seem to like this. It errors out with 'Could not guess version control system. Are you in a working copy directory?'. While I try to figure this out and send out the CR through upload.py, is there anyway you can review the changes at the below git branch? https://github.com/vsubramani/google-api-dotnet-client/commit/09fa23956c0d64e... On Mon, May 18, 2015 at 2:32 PM, Eyal Peled <peleyal@google.com> wrote: > Can you please keep uploading to the original thread? Instructions are in: > https://developers.google.com/api-client-library/dotnet/contribute > Use the upload command as following: upload.py --rev=HEAD -i 220770043 > > On Mon, May 18, 2015 at 2:03 PM Vijay Subramani <vsubramani@google.com> > wrote: > >> I am preparing these changes against the github repo. Currently figuring >> out the github process. Expect a github based CR on this in a couple of >> hours. >> >> On Mon, May 18, 2015 at 10:31 AM, <vsubramani@google.com> wrote: >> >>> >>> https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln >>> File GoogleApisClient.sln (right): >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/GoogleApisClient.sln#newc... >>> GoogleApisClient.sln:51: >>> Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google Compute >>> Sample", "..\..\..\Users\vsubramani\Documents\Visual Studio >>> 2013\Projects\Google Compute Sample\Google Compute Sample\Google Compute >>> Sample.csproj", "{8A409FA2-6788-468B-A156-BA91173116DA}" >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> Please revert. >>>> CAn you also attach somewhere a sample of the final code of the end >>>> >>> developer? >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> File Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs >>> (right): >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:2: >>> Copyright 2013 Google Inc >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> 2015 >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:20: >>> using System.Threading; >>> On 2015/04/15 19:57:07, peleyal wrote: >>> >>>> Please reorder, first all system ones >>>> then 3rd parties then Google, as following: >>>> >>> >>> using System; >>>> using System.Collections; >>>> using System.Collections.Generic; >>>> using System.IO; >>>> using System.Net; >>>> using System.Net.Http; >>>> using System.Threading; >>>> using System.Threading.Tasks; >>>> >>> >>> using Newtonsoft.Json; >>>> using Newtonsoft.Json.Linq; >>>> >>> >>> using Google.Apis.Auth.OAuth2.Flows; >>>> using Google.Apis.Auth.OAuth2.Responses; >>>> using Google.Apis.Http; >>>> using Google.Apis.Logging; >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:75: lock >>> (this) >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> It's better to add an object and don't lock this. >>>> http://stackoverflow.com/questions/251391/why-is-lockthis-bad >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:79: >>> cachedCredential = GetDefaultCredentialUnsynchronized().Result; >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> I don't recommend using Result at all. Why can't this method use >>>> >>> async-await? >>> >>> This was based on an earlier comment from Anthony to not expose the >>> Async pattern on the public surface. I can move it one level up to the >>> GoogleAuth class, but that wouldn't anything much since this line will >>> be executed just once in the lifetime of the application domain. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:83: if >>> (cachedCredential != null) >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> Please add {} >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:93: >>> private async Task<Credential> GetDefaultCredentialUnsynchronized() >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> I don't understand the flow of this method. Please provide a comment >>>> >>> that >>> >>>> explain what we except the environment to look like (which files we >>>> >>> expecting to >>> >>>> have) in each case. >>>> >>> >>> I am not sure I understand what you are looking for? The method already >>> has comments indicating that it first looks at the file referenced by >>> the environment variable, then it looks for a well known file location, >>> failing which it checks whether it is running in compute engine. Was >>> there more explanation you were expecting? >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:95: // >>> First try the environment variable >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> Please explain - environment variable is used on what environment? It >>>> >>> is part >>> >>>> of the sdk installation? Is it available only on compute? >>>> >>> >>> This is covered by the design doc and the feature documentation. This >>> environment variable will be set by the developer, if they need to have >>> explicit control over where the library looks for the credentials file - >>> this potentially will be useful if the developer runs this code in AWS, >>> Azure etc. The "How Application Default Credentials Work" section in the >>> public documentation covers this is a bit more detail - >>> >>> https://developers.google.com/identity/protocols/application-default-credentials >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:107: } >>> On 2015/03/26 17:40:02, Anthony Moore wrote: >>> >>>> Regarding the issue that these two code-paths will return identical >>>> >>> exceptions: >>> >>>> I do think it is important to have different exception messages, even >>>> >>> if we also >>> >>>> have a way of detecting this if you opt-in to the logger. The failure >>>> >>> paths here >>> >>>> are fairly likely to get hit as people go through the getting started >>>> >>> experience >>> >>>> and we have explicit feedback that more specific exceptions for Auth >>>> >>> is >>> >>>> important during that experience. For example, if the cause was the >>>> >>> environment >>> >>>> varialble, the corrective action is to check the value of the >>>> >>> variable. If the >>> >>>> well-known file is the problem, you may want to delete the file or >>>> >>> re-run a >>> >>>> gcloud auth login command. >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: >>> ComputeCredential credential = new ComputeCredential(); >>> On 2015/03/26 17:40:02, Anthony Moore wrote: >>> >>>> This factoring is better, but there is still the issue that the >>>> >>> compute >>> >>>> credential is created optimistically and then discarded. Moving that >>>> >>> into the if >>> >>>> block would improve readability, and decrease fragility over time as >>>> >>> this usage >>> >>>> assumes there will never be side-effects in the compute credentials >>>> >>> constructor. >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:111: >>> ComputeCredential credential = new ComputeCredential(); >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> Weird pattern here. You create a credential and then calling a >>>> >>> function on it. >>> >>>> Why not having a factory method or something similar? >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:124: >>> private string GetWellKnownCredentialFilePath() >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> Please add a comment >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:150: >>> JObject jsonObject = JsonConvert.DeserializeObject<JObject>(await (new >>> StreamReader(stream)).ReadToEndAsync()); >>> On 2015/04/15 19:57:07, peleyal wrote: >>> >>>> Can you please reuse the following method - >>>> >>> >>> >>> https://github.com/google/google-api-dotnet-client/blob/master/Src/GoogleApis... >>> ? >>> >>>> If something is missing maybe we should add it and extend client >>>> >>> secrets (we can >>> >>>> talk about it...) >>>> >>> >>> I see that you use "type" later on. Is it part of the client secrets >>>> >>> file? What >>> >>>> spec are you following? >>>> >>> >>> This Credential file format is different from the Client Secrets file >>> format. >>> >>> @Anthony - do you know whether the json file formats for the credentials >>> are available in public documentation. I searched for it and couldn't >>> find it. If it really doesn't exist, then lets see where would be the >>> best place to document it. >>> >>> @Eyal - I had identified the file format by downloading it from the dev >>> console and opening it :) >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:163: { >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> what is this type of file? How do we get it? >>>> >>> >>> This would be auto generated by the gcloud sdk when the developer runs >>> 'gcloud auth login' >>> >>> Added comment to the code to reflect this. >>> >>> Also done the same for the Service Account File case below. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/DefaultCredentialProvider.cs:186: >>> catch(IOException e) >>> On 2015/04/15 19:57:06, peleyal wrote: >>> >>>> space between catch and ( >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> File Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs (right): >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class >>> GoogleAuth >>> On 2015/03/26 17:40:02, Anthony Moore wrote: >>> >>>> Suggest "public static class GoogleAuth". It will clarify that this is >>>> >>> just for >>> >>>> static methods. >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:40: public class >>> GoogleAuth >>> On 2015/04/15 19:57:07, peleyal wrote: >>> >>>> Why do we need this class? Why DefaultCredentialProvider isn't enough? >>>> >>> >>> The DefaultCredentialProvider is not public. It is meant for internal >>> testability (which is yet to be added though). >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleAuth.cs:44: public static >>> Credential GetApplicationDefaultCredential() >>> On 2015/03/26 17:40:02, Anthony Moore wrote: >>> >>>> I'd suggest at least a minimal doc-comment for these 2 methods. It >>>> >>> looks like >>> >>>> this practice is inconsistent in this library so far, but these 2 are >>>> >>> important >>> >>>> entry points and at least the top-line pops up in intellisense in >>>> >>> Visual Studio, >>> >>>> and it would be good to make this a standard practise for new public >>>> >>> methods as >>> >>>> it is in the other langauges. >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> File Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs >>> (right): >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth.DotNe... >>> Src/GoogleApis.Auth.DotNet4/OAuth2/ServiceAccountCredential.cs:217: >>> public bool IsCreateScopedRequired { get { return true; } } >>> On 2015/03/26 17:40:02, Anthony Moore wrote: >>> >>>> As long as this is not implemented, I would leave this false. >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >>> File Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs (right): >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >>> Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:121: if >>> (!response.Headers.TryGetValues("Metadata-Flavor", out headerValues)) >>> On 2015/03/26 17:40:03, Anthony Moore wrote: >>> >>>> The logger warning below is great because this would be a genuinely >>>> >>> unusual >>> >>>> condition. It would be good to fall into it in both the case where the >>>> >>> header >>> >>>> value is missing and when it has the wrong value. Consider switching >>>> >>> this to >>> >>>> something like: >>>> >>> >>> if (response.Headers.TryGetValues("Metadata-Flavor", out >>>> >>> headerValues)) { >>> >>>> foreach (var value in headerValues) >>>> { >>>> if (value == "Google") >>>> return true; >>>> } >>>> } >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >>> Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:135: >>> Logger.Warning(String.Format("Could not reach the ComputeEngine Metadata >>> service. Obtained HttpRequestException {0}", e.Message)); >>> On 2015/03/26 17:40:03, Anthony Moore wrote: >>> >>>> Of the logger warnings here, I think that this one should perhaps be >>>> >>> removed. >>> >>>> It's an non-error condition running off GCE. >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >>> Src/GoogleApis.Auth/OAuth2/ComputeCredential.cs:154: + " Hence returning >>> the same object."); >>> On 2015/03/26 17:40:03, Anthony Moore wrote: >>> >>>> This is a really good log message. Some tweaks might be that I think >>>> >>> we could >>> >>>> remove the "Hence returning.." sentance and perhapcs suggest to check >>>> IsCreateScopedRequired to turn this off. >>>> >>> >>> Acknowledged. >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >>> File Src/GoogleApis.Auth/OAuth2/Credential.cs (right): >>> >>> >>> https://codereview.appspot.com/220770043/diff/80001/Src/GoogleApis.Auth/OAuth... >>> Src/GoogleApis.Auth/OAuth2/Credential.cs:66: public abstract >>> Task<Credential> CreateScoped(IEnumerable<string> scopes); >>> On 2015/03/26 17:40:03, Anthony Moore wrote: >>> >>>> I's suggest a default implementation of "return this" also. >>>> >>> >>> Acknowledged. >>> >>> https://codereview.appspot.com/220770043/ >>> >> >>
Sign in to reply to this message.
|
