Issue 351: Reimplement OAuth2 (Step 6): WinRT Support

Issue 351: Reimplement OAuth2 (Step 6): WinRT Support

The user code is going to look like this:

var credential = await GoogleWebAuthenticationBroker.AuthenticateAsync(new
Uri("ms-appx:///Assets/client_secrets.json"),
                new[] {
Uri.EscapeUriString(CalendarService.Scopes.Calendar.GetStringValue()) },
                "Eyal", CancellationToken.None);
var calendarService = new CalendarService(new BaseClientService.Initializer
{
  HttpClientInitializer = credential,
  ApplicationName = "APP NAME HERE",
});
var list = await calendarService.CalendarList.List().ExecuteAsync();

[peleyal, 2013-10-10 21:00:40]

The user code is going to look like this:

var credential = await GoogleWebAuthenticationBroker.AuthenticateAsync(new
Uri("ms-appx:///Assets/client_secrets.json"),
                new[] {
Uri.EscapeUriString(CalendarService.Scopes.Calendar.GetStringValue()) },
                "Eyal", CancellationToken.None);
 var calendarService = new CalendarService(new BaseClientService.Initializer
                {
                    HttpClientInitializer = credential,
                    ApplicationName = "APP NAME HERE",
                });
            var list = await calendarService.CalendarList.List().ExecuteAsync();

[peleyal, 2013-10-10 21:04:47]

Hi Noam,

You can take a look in the CLs which were done so far, important one are:

https://code.google.com/p/google-api-dotnet-client/source/detail?r=816d52c8fe...

https://code.google.com/p/google-api-dotnet-client/source/detail?r=8389857131...

and of course you can take a look in the current source code in the library.

[shovavnik, 2013-10-13 03:32:16]

The code looks good. I added some minor recommendations.

Note that I do have some feedback about the unit tests, and also about the
commenting policy, but they'll have to come later, and they weren't part of this
review.

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
File Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs (right):

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs:42: public
Task<AuthorizationCodeResponseUrl> ReceiveCodeAsync(AuthorizationCodeRequestUrl
url,
Why isn't this async? Is there a reason?

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs:57: =>
I think this would be more readable if it were refactored into separate methods.
Instead of CoreApplication.MainView...., try:

await InvokeFromUIThread(() => await AuthenticateAsync(tcs));

And then create two private methods:
1. InvokeFromUIThread receives a delegate.
2. AuthenticateAsync performs the actual authentication (the content of the
async delegate here.

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs:86: ErrorDescription =
"Error and code weren't received from WebAuthenticationBroker. " +
The description isn't quite correct. Should be something like: "The
WebAuthenticationBroker did not return a code or an error." You're not expecting
"Error AND code", and you're missing articles ("the", "a", "an").

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
File Src/GoogleApis.Auth.WinRT/OAuth2/AuthorizationCodeWinRTInstalledApp.cs
(right):

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthorizationCodeWinRTInstalledApp.cs:29: ///
This installed app class uses an internal <seealso
cref="AuthorizationCodeInstalledApp"/> with a
Are you sure this has to be called from the UI thread? The broker already
invokes the authentication on the UI thread, regardless of the initiating
thread.

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
File Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthenticationBroker.cs (right):

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthenticationBroker.cs:35: /// It
uses <seealso cref="Google.Apis.Util.Store.StroageDataStore"/> as the flow's
data store by default.
Reminder to consider the security needs of the client secrets. Local storage and
the assets folder may not be secure enough. On the other hand, they're probably
just as secure as a hard-coded string.

Maybe some form of crypto should be implemented?

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthenticationBroker.cs:55: 
Minor: I would consider extracting the code before this comment into a private
method (for SRP and clarity):

var clientSecrets = LoadGoogleClientSecrets(clientSecretsUri);

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/Prop...
File Src/GoogleApis.Auth.WinRT/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/Prop...
Src/GoogleApis.Auth.WinRT/Properties/AssemblyInfo.cs:25: [assembly:
AssemblyCompany("Google Inc")]
Consider splitting the AssemblyInfo.cs into a SolutionInfo.cs and
AssemblyInfo.cs. The SolutionInfo.cs would sit in a separate solution folder,
and physically in the root folder of the solution. You would then add the
SolutionInfo.cs as a linked file to each project.

This simplifies future updates that are common to all the assemblies.

The AssemblyTitle and possibly AssymblyProduct, AssemblyMetadata, as well as any
other assembly-specific attributes should stay in the AssemblyInfo.cs files.

[peleyal, 2013-10-15 21:43:50]

Thanks Noam,
Hopefully everything is good know so you will be able to LGTM (Looks Good To Me)
it

Eyal

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
File Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs (right):

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs:42: public
Task<AuthorizationCodeResponseUrl> ReceiveCodeAsync(AuthorizationCodeRequestUrl
url,
On 2013/10/13 03:32:17, shovavnik wrote:
> Why isn't this async? Is there a reason?

Done.

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs:57: =>
On 2013/10/13 03:32:17, shovavnik wrote:
> I think this would be more readable if it were refactored into separate
methods.
> Instead of CoreApplication.MainView...., try:
> 
> await InvokeFromUIThread(() => await AuthenticateAsync(tcs));
> 
> And then create two private methods:
> 1. InvokeFromUIThread receives a delegate.
> 2. AuthenticateAsync performs the actual authentication (the content of the
> async delegate here.

Done.

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs:86: ErrorDescription =
"Error and code weren't received from WebAuthenticationBroker. " +
On 2013/10/13 03:32:17, shovavnik wrote:
> The description isn't quite correct. Should be something like: "The
> WebAuthenticationBroker did not return a code or an error." You're not
expecting
> "Error AND code", and you're missing articles ("the", "a", "an").

Done.

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
File Src/GoogleApis.Auth.WinRT/OAuth2/AuthorizationCodeWinRTInstalledApp.cs
(right):

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthorizationCodeWinRTInstalledApp.cs:29: ///
This installed app class uses an internal <seealso
cref="AuthorizationCodeInstalledApp"/> with a
YOU ARE SO RIGHT!
THANKS
On 2013/10/13 03:32:17, shovavnik wrote:
> Are you sure this has to be called from the UI thread? The broker already
> invokes the authentication on the UI thread, regardless of the initiating
> thread.

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
File Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthenticationBroker.cs (right):

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthenticationBroker.cs:35: /// It
uses <seealso cref="Google.Apis.Util.Store.StroageDataStore"/> as the flow's
data store by default.
Unfortunately that's the way installed applications work.
It sucks. I know. It's not secured! Even if you somehow encrypt it then anyone
can get this key and just decrypt it.

See https://developers.google.com/accounts/docs/OAuth2#installed and to be more
specific:
The registration process will result in a client ID and, in some cases, a client
secret, which you embed in the source code of your application. (In this
context, the client secret is obviously not treated as a secret.)


On 2013/10/13 03:32:17, shovavnik wrote:
> Reminder to consider the security needs of the client secrets. Local storage
and
> the assets folder may not be secure enough. On the other hand, they're
probably
> just as secure as a hard-coded string.
> 
> Maybe some form of crypto should be implemented?

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/OAut...
Src/GoogleApis.Auth.WinRT/OAuth2/GoogleWebAuthenticationBroker.cs:55: 
On 2013/10/13 03:32:17, shovavnik wrote:
> Minor: I would consider extracting the code before this comment into a private
> method (for SRP and clarity):
> 
> var clientSecrets = LoadGoogleClientSecrets(clientSecretsUri);

Done.

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/Prop...
File Src/GoogleApis.Auth.WinRT/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/14433060/diff/1/Src/GoogleApis.Auth.WinRT/Prop...
Src/GoogleApis.Auth.WinRT/Properties/AssemblyInfo.cs:25: [assembly:
AssemblyCompany("Google Inc")]
I opened a new issue for that:
https://code.google.com/p/google-api-dotnet-client/issues/detail?id=402
On 2013/10/13 03:32:17, shovavnik wrote:
> Consider splitting the AssemblyInfo.cs into a SolutionInfo.cs and
> AssemblyInfo.cs. The SolutionInfo.cs would sit in a separate solution folder,
> and physically in the root folder of the solution. You would then add the
> SolutionInfo.cs as a linked file to each project.
> 
> This simplifies future updates that are common to all the assemblies.
> 
> The AssemblyTitle and possibly AssymblyProduct, AssemblyMetadata, as well as
any
> other assembly-specific attributes should stay in the AssemblyInfo.cs files.

[shovavnik, 2013-10-17 01:11:03]

Hey Eyal,

I added one comment to improve the grammar in a doc comment.

Other than that, LGTM :).

Noam

https://codereview.appspot.com/14433060/diff/25001/Src/GoogleApis.Auth.WinRT/...
File Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs (right):

https://codereview.appspot.com/14433060/diff/25001/Src/GoogleApis.Auth.WinRT/...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs:61: /// <param
name="tcs">Task completion source which its authorization code response will be
set</param>
"Task completion source whose result will be set to the authorization code
response"

[peleyal, 2013-10-17 13:42:45]

Thanks Noam,
I'll push it to the repository in the next minutes.

https://codereview.appspot.com/14433060/diff/25001/Src/GoogleApis.Auth.WinRT/...
File Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs (right):

https://codereview.appspot.com/14433060/diff/25001/Src/GoogleApis.Auth.WinRT/...
Src/GoogleApis.Auth.WinRT/OAuth2/AuthenticationBroker.cs:61: /// <param
name="tcs">Task completion source which its authorization code response will be
set</param>
On 2013/10/17 01:11:04, shovavnik wrote:
> "Task completion source whose result will be set to the authorization code
> response"

Done.