Issue 351: Reimplement OAuth2 (Step 5): WP support

Issue 351: Reimplement OAuth2 (Step 5): WP support

Sample code:
var credential = await GoogleWebAuthenticationBroker.AuthenticateAsync(
                new FileStream("client_secrets.json", FileMode.Open, FileAccess.Read),
                new[] { DriveService.Scope.Drive },
                "Eyal",
                CancellationToken.None);

            var i = new BaseClientService.Initializer()
                {
                    HttpClientInitializer = credential,
                    ApplicationName = "eyal application",
                };

[peleyal, 2013-10-02 22:26:09]

Hi Shai,
Need your review.
It's the implementation of WP. I also submitted several CLs (of Auth PCL data
types and flows).

I really want you to have this review, as you was the one that started that :)

Let me know!
Thanks,
Eyal

[Shai.Raiten, 2013-10-06 06:18:53]

Hi all,

I've completed my review, looks awesome!
You can see I added some questions and places that need to be change.

Feel free contacting me regarding those and everything else.

Thanks

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
File Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeBroker.cs (right):

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeBroker.cs:33: public class
AuthorizationCodeBroker : ICodeReceiver
Why creating AuthorizationCodeWPInstalledApp? 
What you gain from that separation?

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeBroker.cs:46: return await (await
Deployment.Current.Dispatcher.InvokeAsync(new
Func<Task<AuthorizationCodeResponseUrl>>(
Why double await?

return await Deployment.Current.Dispatcher.InvokeAsync(new
Func<Task<AuthorizationCodeResponseUrl>>(() => ReceivedCodeCoreAsync(url,
taskCancellationToken)));

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
File Src/GoogleApis.Auth.WP/OAuth2/GoogleWebAuthenticationBroker.cs (right):

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
Src/GoogleApis.Auth.WP/OAuth2/GoogleWebAuthenticationBroker.cs:64: public static
async Task<UserCredential> AuthenticateAsync(Stream clientSecretsStream,
IEnumerable<string> scopes,
You can leave the Stream but I think this is a security problem, as in IIS
servers you place your "Secrets" in web.config - but here the IIS prevent from
anyone to get the content of that file.
Now, you are enabling the user to save a local file - not encrypted in the app
folder that can be taken easily, or scanned by Phone search indexer etc..
Placing the keys as consts in the app is also something that can be hacked but
it require more.

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
File Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs (right):

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:41: {
You should check or catch null reference exception. for all methods here.

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:45: var content =
NewtonsoftJsonSerializer.Instance.Serialize(value);
I can't see NewtonsoftJsonSerializer.Instance.Serialize, why have you implement
it as singleton? 
Do you have checks inside it? like what happens if the value is null or empty?

Example of simple implementation (The one I wrote in the first version):

class JsonSerializerHelper
{
    public static T Deserializer<T>(string json)
    {
        if (string.IsNullOrEmpty(json))
            return default(T);

        try
        {
            return JsonConvert.DeserializeObject<T>(json);
        }
        catch (Exception ex)
        {
           
Debug.WriteLine(string.Format("JsonSerializerHelper->Deserializer->{0}",
ex.Message));
            return default(T);
        }
    }

    public static string Serializer(object obj)
    {
        if (obj == null)
            return string.Empty;

        try
        {
            return JsonConvert.SerializeObject(obj);
        }
        catch (Exception ex)
        {
           
Debug.WriteLine(string.Format("JsonSerializerHelper->Serializer->{0}",
ex.Message));
            return string.Empty;
        }
    }
}

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:76: await
LocalFolder.DeleteAsync();
Add -> StorageDeleteOption.PermanentDelete : you want to permanently deletes the
item without moving it to the recycle bin.

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:83: {
Again, due to phone development you need to consider file corruption
(Application crashed, user closed etc..) so add the proper checks.

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:97: {
It took me a sec to understand the propose of this method, I think you should
change the name to GenerateKeyName or something like that, after all you're not
getting StoredKey but the key name.

[peleyal, 2013-10-08 14:03:05]

Thanks for the review Shai!

I still didn't get which exact exceptions we should catch on the storage
mechanism in WP, so I look forward your explanations in the next review.

Eyal

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
File Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeBroker.cs (right):

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeBroker.cs:33: public class
AuthorizationCodeBroker : ICodeReceiver
It just separate the component which all its purpose is to retrieve the
authorization code (this class), 
from the component that actually authorizes the user.

That's the way I'm doing the authorization code flow in WinRT, MVC, etc.
I think that it's nice separation. If you think otherwise please explain what is
the benefit of your idea, and let's discuss it. We are still in a point that we
can change those kind of things, so let me know!

 
 On 2013/10/06 06:18:53, Shai.Raiten wrote:
> Why creating AuthorizationCodeWPInstalledApp? 
> What you gain from that separation?

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeBroker.cs:46: return await (await
Deployment.Current.Dispatcher.InvokeAsync(new
Func<Task<AuthorizationCodeResponseUrl>>(
that's the error I got when doing so:

Error	4	Since this is an async method, the return expression must be of type
'Google.Apis.Auth.OAuth2.Responses.AuthorizationCodeResponseUrl' rather than
'Task<Google.Apis.Auth.OAuth2.Responses.AuthorizationCodeResponseUrl>'	C:\code\google.com\google-api-dotnet-client\default_3\Src\GoogleApis.Auth.WP\OAuth2\AuthorizationCodeBroker.cs	46	20	GoogleApis.Auth.WP


On 2013/10/06 06:18:53, Shai.Raiten wrote:
> Why double await?
> 
> return await Deployment.Current.Dispatcher.InvokeAsync(new
> Func<Task<AuthorizationCodeResponseUrl>>(() => ReceivedCodeCoreAsync(url,
> taskCancellationToken)));

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
File Src/GoogleApis.Auth.WP/OAuth2/GoogleWebAuthenticationBroker.cs (right):

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.Auth.WP/OAuth2/...
Src/GoogleApis.Auth.WP/OAuth2/GoogleWebAuthenticationBroker.cs:64: public static
async Task<UserCredential> AuthenticateAsync(Stream clientSecretsStream,
IEnumerable<string> scopes,
If I'm adding the client_secrets as an embedded resource, is it still a security
issue?
On 2013/10/06 06:18:53, Shai.Raiten wrote:
> You can leave the Stream but I think this is a security problem, as in IIS
> servers you place your "Secrets" in web.config - but here the IIS prevent from
> anyone to get the content of that file.
> Now, you are enabling the user to save a local file - not encrypted in the app
> folder that can be taken easily, or scanned by Phone search indexer etc..
> Placing the keys as consts in the app is also something that can be hacked but
> it require more.

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
File Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs (right):

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:41: {
I didn't find any best practices on this case.
Can you please elaborate?

On 2013/10/06 06:18:53, Shai.Raiten wrote:
> You should check or catch null reference exception. for all methods here.

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:45: var content =
NewtonsoftJsonSerializer.Instance.Serialize(value);
First of all I didn't implement it :)
It's actually implements ISerializer, which we use in purpose to use a generic
serializer and not a specific one.
Here we don't need the generic case because I chose to serialize using the JSON
format.

On 2013/10/06 06:18:53, Shai.Raiten wrote:
> I can't see NewtonsoftJsonSerializer.Instance.Serialize, why have you
implement
> it as singleton? 
> Do you have checks inside it? like what happens if the value is null or empty?
> 
> Example of simple implementation (The one I wrote in the first version):
> 
> class JsonSerializerHelper
> {
>     public static T Deserializer<T>(string json)
>     {
>         if (string.IsNullOrEmpty(json))
>             return default(T);
> 
>         try
>         {
>             return JsonConvert.DeserializeObject<T>(json);
>         }
>         catch (Exception ex)
>         {
>            
> Debug.WriteLine(string.Format("JsonSerializerHelper->Deserializer->{0}",
> ex.Message));
>             return default(T);
>         }
>     }
> 
>     public static string Serializer(object obj)
>     {
>         if (obj == null)
>             return string.Empty;
> 
>         try
>         {
>             return JsonConvert.SerializeObject(obj);
>         }
>         catch (Exception ex)
>         {
>            
> Debug.WriteLine(string.Format("JsonSerializerHelper->Serializer->{0}",
> ex.Message));
>             return string.Empty;
>         }
>     }
> }

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:45: var content =
NewtonsoftJsonSerializer.Instance.Serialize(value);
I improved our implementation which is attach to this CL.
Take a look...
Thanks!

On 2013/10/06 06:18:53, Shai.Raiten wrote:
> I can't see NewtonsoftJsonSerializer.Instance.Serialize, why have you
implement
> it as singleton? 
> Do you have checks inside it? like what happens if the value is null or empty?
> 
> Example of simple implementation (The one I wrote in the first version):
> 
> class JsonSerializerHelper
> {
>     public static T Deserializer<T>(string json)
>     {
>         if (string.IsNullOrEmpty(json))
>             return default(T);
> 
>         try
>         {
>             return JsonConvert.DeserializeObject<T>(json);
>         }
>         catch (Exception ex)
>         {
>            
> Debug.WriteLine(string.Format("JsonSerializerHelper->Deserializer->{0}",
> ex.Message));
>             return default(T);
>         }
>     }
> 
>     public static string Serializer(object obj)
>     {
>         if (obj == null)
>             return string.Empty;
> 
>         try
>         {
>             return JsonConvert.SerializeObject(obj);
>         }
>         catch (Exception ex)
>         {
>            
> Debug.WriteLine(string.Format("JsonSerializerHelper->Serializer->{0}",
> ex.Message));
>             return string.Empty;
>         }
>     }
> }

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:76: await
LocalFolder.DeleteAsync();
On 2013/10/06 06:18:53, Shai.Raiten wrote:
> Add -> StorageDeleteOption.PermanentDelete : you want to permanently deletes
the
> item without moving it to the recycle bin.

Done.

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:83: {
ditto.
Please elaborate, which exception should I catch and what should I do in case
there is an exception?

Isn't it something that our developers should catch?

On 2013/10/06 06:18:53, Shai.Raiten wrote:
> Again, due to phone development you need to consider file corruption
> (Application crashed, user closed etc..) so add the proper checks.

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:97: {
On 2013/10/06 06:18:53, Shai.Raiten wrote:
> It took me a sec to understand the propose of this method, I think you should
> change the name to GenerateKeyName or something like that, after all you're
not
> getting StoredKey but the key name.

Done.

[peleyal, 2013-10-09 22:50:12]

ping :)

[Shai.Raiten, 2013-10-13 06:57:45]

Hi Eyal,

I've replay to your questions, this is all from my end! 
AmAzInG WoRk Eyal!!!

Thanks

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
File Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs (right):

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:41: {
Sometimes uses will Kill the app and this might cause file corruption, so it
good to make sure the following: 

IOException - The file could not be opened or retrieved as a stream.
And add Exception (generic), if the value is Null you'll receive an
ArgumentNullException, if the converter returns a negative buffer value you'll
receive ArgumentOutOfRangeException and couple more. so just to make sure.. :-)


On 2013/10/08 14:03:05, peleyal wrote:
> I didn't find any best practices on this case.
> Can you please elaborate?
> 
> On 2013/10/06 06:18:53, Shai.Raiten wrote:
> > You should check or catch null reference exception. for all methods here.
>

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:83: {
IsolatedStorageFileStream will throw exception if file not found so -
FileNotFoundException.

Or make sure the file exists.


On 2013/10/08 14:03:05, peleyal wrote:
> ditto.
> Please elaborate, which exception should I catch and what should I do in case
> there is an exception?
> 
> Isn't it something that our developers should catch?
> 
> On 2013/10/06 06:18:53, Shai.Raiten wrote:
> > Again, due to phone development you need to consider file corruption
> > (Application crashed, user closed etc..) so add the proper checks. 
>

https://codereview.appspot.com/14295045/diff/27001/Src/GoogleApis/Apis/Json/N...
File Src/GoogleApis/Apis/Json/NewtonsoftJsonSerializer.cs (right):

https://codereview.appspot.com/14295045/diff/27001/Src/GoogleApis/Apis/Json/N...
Src/GoogleApis/Apis/Json/NewtonsoftJsonSerializer.cs:53: {
Great, this will save tons of errors!

[peleyal, 2013-10-15 20:04:40]

Hi Shai.
Thanks!

I'm not sure about the exceptions you mentioned. 
I didn't find any documentation on which exceptions can be thrown from the store
API.
Meantime I prefer to leave it without catching any exception than
1) Catch general exception
2) Catch several exceptions that we are not sure in 100% that it's even possible
that they may be thrown.

Looking forward to your reply.

Thanks,
Eyal

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
File Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs (right):

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:41: {
I'm just trying to figure out where is the list of all possible exceptions.
I don't like to catch Exception (general exception), but I don't like to catch
the exceptions you mentioned because I didn't find any documentation that those
exceptions can be thrown.
Can you refer me to some doc like that?

Thanks!

On 2013/10/13 06:57:45, Shai.Raiten wrote:
> Sometimes uses will Kill the app and this might cause file corruption, so it
> good to make sure the following: 
> 
> IOException - The file could not be opened or retrieved as a stream.
> And add Exception (generic), if the value is Null you'll receive an
> ArgumentNullException, if the converter returns a negative buffer value you'll
> receive ArgumentOutOfRangeException and couple more. so just to make sure..
:-)
> 
> 
> On 2013/10/08 14:03:05, peleyal wrote:
> > I didn't find any best practices on this case.
> > Can you please elaborate?
> > 
> > On 2013/10/06 06:18:53, Shai.Raiten wrote:
> > > You should check or catch null reference exception. for all methods here.
> > 
>

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:83: {
We use FileMode.OpenOrCreate, so it won't throw an exception :)
On 2013/10/13 06:57:45, Shai.Raiten wrote:
> IsolatedStorageFileStream will throw exception if file not found so -
> FileNotFoundException.
> 
> Or make sure the file exists.
> 
> 
> On 2013/10/08 14:03:05, peleyal wrote:
> > ditto.
> > Please elaborate, which exception should I catch and what should I do in
case
> > there is an exception?
> > 
> > Isn't it something that our developers should catch?
> > 
> > On 2013/10/06 06:18:53, Shai.Raiten wrote:
> > > Again, due to phone development you need to consider file corruption
> > > (Application crashed, user closed etc..) so add the proper checks. 
> > 
>