Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incremental auth support #624

Merged
merged 11 commits into from Nov 30, 2015
Merged

Conversation

squid808
Copy link
Contributor

@squid808 squid808 commented Nov 4, 2015

This adds support of the library to authenticate with the include_granted_scopes parameter which does not currently exist.

Usage is intended to be exposed to a user of the library who is already using GoogleWebAuthorizationBroker.AuthorizeAsync() by adding a single new parameter - includeGrantedScopes - which is a nullable bool and set to null by default. For example:

userCredentials = await GoogleWebAuthorizationBroker.AuthorizeAsync(
                    MyClientSecrets, scopes, userName,
                    System.Threading.CancellationToken.None, new DataStore(),
                    true //this will set the include_granted_scopes=true in the url
                    );

If the user omits this optional parameter the null is trickled down and is ignored when building the authorization request URL (thereby defaulting to false in the request). When explicitly included, the query parameter is included in the URL with the value supplied, even if false.

I have tested it up to the point of verifying that it creates the url with the appropriate query parameters as expected, and ensured that the URL resolves and supplies an appropriate authentication page in the browser. I have not been able to test the resulting tokens however due to my applications being all installed applications at this time, which do not work with Incremental Authorization to my knowledge.

A proper test could likely be set up to ensure that the resulting url contains the parameter when expected, however I haven't fully wrapped my head around the mocking setup used in the project's tests. If someone would be willing to guide me through it I'd be more than happy to work on that as well, though my experience with tests is somewhat limited.

This also fixes two minor typos I came across for which I saw no need for a separate pull request. It's also my first pull request, so please let me know if I have done something incorrectly during the process.

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Nov 4, 2015
@squid808
Copy link
Contributor Author

squid808 commented Nov 4, 2015

I have signed the CLA.

@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@squid808
Copy link
Contributor Author

squid808 commented Nov 4, 2015

I have again signed the CLA with my other account I didn't realize I was using, apologies.

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Nov 4, 2015
@LindaLawton
Copy link
Collaborator

Wouldn't it be better to add it here then to just overload the method?
GoogleAuthorizationCodeRequestUrl.cs

@peleyal
Copy link
Collaborator

peleyal commented Nov 6, 2015

Thank you very much for this pull request! It's exciting to see contribution like you trying to make this library better 👍

  1. Before starting the review itself, I'm trying to understand what is the exact flow.
    So you create a service and call the authorization method to get your credential:

userCredentials = await GoogleWebAuthorizationBroker.AuthorizeAsync("scope 1", ... , "include_granted_scopes=true").

Then you need to actually add "scope 2", since your are trying call a service that requires it.
What do you do then? How do you add "scope2" with the current code?
Should you just call to

userCredentials = await GoogleWebAuthorizationBroker.AuthorizeAsync("scope 2", ... , "include_granted_scopes=true")

and since the user already added scope1, the output credential will result in token for both "scope 1" and "scope 2"?

  1. I think that Incremental Authorization should work for Installed App as well:
    https://developers.google.com/identity/protocols/OAuth2InstalledApp#incrementalAuth
    I will be happy to hear that it works for you, and I might try it myself after that. Thanks :)
  2. Regarding testing. I can guide you, but I prefer first to focus on 1&2. Later on we can add the tests as well.

Thanks again!
Hope that my general comments seem reasonable to you.
Eyal

@squid808
Copy link
Contributor Author

squid808 commented Nov 7, 2015

Linda: I'm not entirely understanding your question. I did add it to GoogleAuthorizationCodeRequestUrl.cs, but in addition I overloaded the methods in order to expose this setting easily to users of the library such as myself. Since the majority of the library tends to be abstracted away from those such as myself, after reviewing the code I determined the best way to grant a user access to control this setting was to overload the methods, starting with GoogleWebAuthorizationBroker.AuthorizeAsync(...) since that (to my understanding) is the highest level and easiest way to start with the code. Overloading the method it calls in turn also allows for catching the same functionality on multiple levels.

If I had just added it to the GoogleAuthorizationCodeRequestUrl.cs, I didn't see a good way to allow a general user to be able to append scopes, which I felt is a fairly necessary thing to be able to access and control (as opposed to ApprovalPrompt for instance, which may not be accessed or changed as often). I could, of course, have missed something in the code when I was walking through with the debugger. I tried to identify the best entry point for this code without breaking backward compatibility, so if I missed something I have no qualms against going a different direction!

Eyal, if I'm understanding your question correctly - the initial request (again, to my understanding of the api and the documentation) - wouldn't matter what you passed for the parameter since there is no previous authorization to work against. In my testing, setting it to true for the first authorization didn't error out, so it seems to just ignore it in that case (since there is nothing to append scopes to). In the subsequent authorizations when you don't want the default behavior of replacing scopes, you do have to explicitly set the parameter to true. Setting it to false or leaving it blank (null) will induce the default behavior of replacing scopes, rather than appending.

and since the user already added scope1, the output credential will result in token for both "scope 1" and "scope 2"?

Based on the description here, it will output a single token but instead of this token just representing the scopes you supplied with this auth request, it will append those scopes to the scopes you had assigned to this project from prior requests.

I think that Incremental Authorization should work for Installed App as well:
https://developers.google.com/identity/protocols/OAuth2InstalledApp#incrementalAuth
I will be happy to hear that it works for you, and I might try it myself after that. Thanks :)

Unfortunately, I and other users have had no such luck. At this time I don't have experience with non-installed apps to test this with but I wanted to get the ball rolling. I'd be happy to take this offline for another discussion if you feel it's not appropriate here, but until I can figure out how to do something other than an installed app without access to a webserver or something my hands are a bit tied (and no time, friggin babies man).

I'll be on the road for the next few days, so I apologize if I misread your questions in my haste or don't get back to any questions or comments soon enough! Thanks for your consideration.

@peleyal
Copy link
Collaborator

peleyal commented Nov 7, 2015

Thanks for all the content you added.

I'm still not convinced that your change will actually do something, and here is the reason:

Whenever you call AuthorizeAsync, you actually call this AuthorizeAsync of the InstalledApp class:
https://github.com/google/google-api-dotnet-client/blob/9618f6846b29d4b812ba61362215a2c9d25110a9/Src/GoogleApis.Auth/OAuth2/AuthorizationCodeInstalledApp.cs#L60

By taking a look in this implementation, I see that whenever we have a token, we just use it. So I think that in your flow although we will set include_granted_scopes=true, nothing will happen. We will load the token form the data store, and because with the current code we don't compare scopes (we actually don't have a way to do it right now), we will just use it.

We need to add some path in the code to check if include_granted_scopes is set to true, and then don't use the current token.

We definitely need to test it, and make sure it works. I'm currently swamped with other things on my plate (like everyone else, right?), but I'll try to get into it later on this week.

Thanks again, hope that my comment is clear and correct me if I'm wrong regarding anything I said...
Eyal

@LindaLawton
Copy link
Collaborator

Thinking out loud.

We need a way to know what scopes the initial datastore was created with. Then we can detect if the new scopes are being passed are the same as the old dataStore or not. If they aren't then we can request access for these new scopes. This would require a change to datastore to save the scopes also.

We could just set it always to true but i am not sure this is a good idea what if you wanted to remove them there will be no way to remove them.

Really i think it needs to be the client library that figures out what needs to be set developers shouldn't have to do this. However we could add an optionValues to AuthorizeAsync support this and other items like the ones below.

Here are a few other things we might want to consider merging in with this

approval_prompt #524
login_hint https://developers.google.com/identity/protocols/OpenIDConnect
state https://developers.google.com/identity/protocols/OpenIDConnect
TokenInfo validation
access_type
id_token A JWT that contains identity information about the user that is digitally signed by Google.

@peleyal
Copy link
Collaborator

peleyal commented Nov 9, 2015

I don't agree that we need to change the datastore structure, since in a
regular call to a google apis service we don't use scopes.

IMO, we just need to change the code I mentioned that if the incremental
authentication flag is true, we actually don't try to get a token from the
datastore since the token is missing scopes that we want. Then we send the
request to the authorization server using the broker we already have, get
the token, and override the existing one.

Eyal

On Mon, Nov 9, 2015, 6:49 AM Linda Lawton notifications@github.com wrote:

Thinking out loud.

We need a way to know what scopes the initial datastore was created with.
Then we can detect if the new scopes are being passed are the same as the
old dataStore or not. If they aren't then we can request access for these
new scopes. This would require a change to datastore to save the scopes
also.

We could add an optionValues to AuthorizeAsync support other items like the
ones below.

Here are a few other things we might want to consider merging in with this

approval_prompt #524
#524
login_hint https
https://developers.google.com/identity/protocols/OpenIDConnect://
https://developers.google.com/identity/protocols/OpenIDConnect
developers.google.com
https://developers.google.com/identity/protocols/OpenIDConnect
/identity/protocols/
https://developers.google.com/identity/protocols/OpenIDConnect
OpenIDConnect
https://developers.google.com/identity/protocols/OpenIDConnect
state https https://developers.google.com/identity/protocols/OpenIDConnect
:// https://developers.google.com/identity/protocols/OpenIDConnect
developers.google.com
https://developers.google.com/identity/protocols/OpenIDConnect
/identity/protocols/
https://developers.google.com/identity/protocols/OpenIDConnect
OpenIDConnect
https://developers.google.com/identity/protocols/OpenIDConnect
TokenInfo validation
access_type
id_token A JWT that contains identity information about the user that is
digitally signed by Google.


Reply to this email directly or view it on
#624 (comment)
GitHub
#624 (comment).

@squid808
Copy link
Contributor Author

squid808 commented Nov 9, 2015

Understood, thanks for clearing up what you meant. I had previously played with this method until realizing I could simplify things, but I had neglected to realize what you mention, thanks for catching that. I agree that there shouldn't be a need to keep track of what scopes we're using - we just in some way need to make sure the token coming back should or shouldn't replace what is there.

I think if we update AuthorizeAsync() method to also accept a new [defaulted] options parameter, we'd not mess up any backwards compatibility, not have to worry about changing object types or casting, and wouldn't add a lot of overhead. Like she said, we could potentially add in other parameters also if they needed to be specified and handled in much the same way. In that case, rather than supplying it via the Initializer and adding it to the flow, we could just declare it in the Broker and pass it along to the AuthorizeAsync method. It would also make things easier to update for future options.

Otherwise, if we wanted to explicitly check/use these parameters via my current implementation (via the initializer -> flow path) we'd have to at least cast the Flow to access these parameters that don't exist on the interface, which is ugly.

I get back later this week, I can throw something together and see if I can get something working. I'll also look in to the other parameters Linda mentioned - if most of them don't need to be considered in AuthorizeAsync() they'll be relatively simple to add in the same way.

@peleyal
Copy link
Collaborator

peleyal commented Nov 9, 2015

@LindaLawton
Copy link
Collaborator

I wonder if adding request_visible_actions could be added to this as well. Posting moments to g+ still doesn't work without that.

@squid808
Copy link
Contributor Author

Sorry for the delay.

An option is to add more parameters to the initializer at:
https://github.com/google/google-api-dotnet-client/blob/master/Src/GoogleApis.Auth/OAuth2/Flows/AuthorizationCodeFlow.cs

Why add it to AuthorizationCodeFlow.Initializer instead of the GoogleAuthorizationCodeFlow.Initializer that inherits it? With this and all the other parameters suggested here, if they're going to be specific to Google as opposed to generic OAuth 2.0 (for instance, request_visible_actions which is a G+ thing) shouldn't they be in the latter?

Either way, that's the direction I'm heading - adding the parameters to the initializer and making the AuthorizeAsyncCore() public so that rather than adding parameters to the methods, they can just call it directly.

As to what I'm thinking beyond that - since we need the AuthorizeAsync() method to know when it needs to force the data store to update, and since it uses Flow which has a contract with IAuthorizationCodeFlow as opposed to the derived specific types, if we added a simple method of ShouldForceRefresh() that returns a bool, we could have a simple return false; virtual method defined in the AuthorizationCodeFlow and then a more complex version overridden in the GoogleAuthorizationCodeFlow that checks for things like a value of true for the IncludeGrantedScopes property (and anything else in the future that might similarly affect it) and returns true in those cases.

A lot of word salad, I know. I'm working on the updates to the code now, just trying to properly add in the parameters suggested by Linda before updating the pull request.

@squid808
Copy link
Contributor Author

Just a thought in a different direction and please let me know if you'd rather not have the project act this way - but what if instead of explicitly adding all these query parameters, I were to instead add an array of KeyValuePair objects that could represent any other query parameters the user specified? The user would specify that array in much the same way as we discussed - via the auth initializer - but it would give them the freedom to add in any other parameters (and as many as) they wanted.

I've already targeted how to do it - roughly by adding a new type to the RequestParameterType enum (CustomQuery), updating the RequestBuilder.AddParameter() switch to handle that new type (and simply add any key value pairs to the QueryParameters list) and adding that property to GoogleAuthorizationCodeRequestUrl. I'm pretty sure it would work.

Assuming that most of the OpenID parameters mentioned above and other future parameters don't require an existing token to be replaced (as does the incremental auth), all it would need to do is show up in the URL, correct? The resulting code/token exchange would then proceed as normal. If so, I don't think that's in the scope of this pull request and I can open a separate one for that once I'm done with this. Please let me know your thoughts - otherwise, I'll look in to manually and explicitly adding the missing OpenID parameters as previously planned.

@squid808
Copy link
Contributor Author

Linda - I left out the other parameters for now pending your thoughts on my above comment.

Eyal - I've updated the interface to allow for the Flow to indicate if/when the token should be forced to be retrieved and stored, it should alleviate the issue you described. I elected to update the Interface to keep things clean, and the method I added could have applications down the line. If you'd rather me not alter the interface I can work around it though it might not be as clean.

@LindaLawton
Copy link
Collaborator

I know that request_visible_actions is proably just a G+ thing at this time but iwth out the ablity to set it we moments.insert wont work. I have had to hack my own version of the library to get this to work. Wall of text on this

My thoughts are you are awesome i have wanted to do this for months but could never figure out how to do it and make it look "nice" Keep up the good work. I will leave code review to Eyal :)

@@ -113,17 +113,22 @@ public class GoogleWebAuthorizationBroker
/// <param name="taskCancellationToken">Cancellation token to cancel an operation.</param>
/// <param name="dataStore">The data store, if not specified a file data store will be used.</param>
/// <returns>User credential.</returns>
private static async Task<UserCredential> AuthorizeAsyncCore(
public static async Task<UserCredential> AuthorizeAsyncCore(

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@peleyal
Copy link
Collaborator

peleyal commented Nov 18, 2015

  1. You are right, GoogleAuthorizationCodeFlow is the right place to do that. Thanks
  2. I think that your idea of ShouldForceRefresh is nice. I'm still trying to figure out if it's the right way.
  3. Your idea of KeyValuePair sounds like a good solution for future new parameters. I would focus for now for adding this new boolean you just created, and open a new issue \ PR for later on to add this capability as well. To be honest, I think it's a good idea for authentication and to service calls as well.
    But, again, lets focus on this PR for now.

I'm starting the review now...

GoogleAuthorizationCodeFlow.Initializer initializer, IEnumerable<string> scopes, string user,
CancellationToken taskCancellationToken, IDataStore dataStore = null)
CancellationToken taskCancellationToken, IDataStore dataStore = null,
bool? includeGrantedScopes = null)

This comment was marked as spam.

This comment was marked as spam.

@@ -88,7 +87,18 @@ public ICodeReceiver CodeReceiver
taskCancellationToken).ConfigureAwait(false);
}

return new UserCredential(flow, userId, token);
return new UserCredential(Flow, userId, token);

This comment was marked as spam.

This comment was marked as spam.

@squid808
Copy link
Contributor Author

Thanks again, you're too kind! My programming origins are isolated and humble indeed and I know I have a lot to pick up on, so I really do appreciate how cool and patient you've been with the whole process.

Unfortunately I've only ever dealt with the installed app scenarios. I'll look around for some sample code I might be able to quickly run without having to do a ton of binding redirects and modifications to in order to get it running. I'll check out the quick-start app mentioned here later when I can keep my eyes open a bit longer =)

@peleyal
Copy link
Collaborator

peleyal commented Nov 22, 2015

Hi thanks again :) You're pretty awesome as well.
I just ran an installed app with two different services, and, like you mentioned, the original scope was erased.
I'm going to contact some developers form the auth group to understand why it happens.

@squid808
Copy link
Contributor Author

Thanks so much! That means there might be a light at the end of the tunnel for my users and me!

@peleyal
Copy link
Collaborator

peleyal commented Nov 22, 2015

There is always light!
Hopefully it's something that we are doing wrong, so we will be able to fix it soon.

I'll keep you updated,
Eyal

@LindaLawton
Copy link
Collaborator

@squid808 and i went though this a little on stack all the documentation we found appears to refer only to web and mobile apps. It appear it wasn't implemented for installed apps.

I can put together a quick test without the client library to find out if it works or not. Either way look forward to hearing what the Oauth gurus say.

@peleyal
Copy link
Collaborator

peleyal commented Nov 23, 2015

Someone also mentioned to me that Incremental Auth doesn't work for installed app.

  1. We need to test an ASP.NET case, or WP...
    If it works,
  2. It will be a good idea to update our docs

Then we can commit 👍

I'll try to spend sometime with it later on this week. @squid808 if you have time, you should test it too.
Thanks,
Eyal

@peleyal
Copy link
Collaborator

peleyal commented Nov 28, 2015

OK. I tested your code and with a small fix it works for Web applications

  1. We need to change
    https://github.com/google/google-api-dotnet-client/blob/master/Src/GoogleApis.Auth/OAuth2/Web/AuthorizationCodeWebApp.cs#L105
    as well (as similar to the change you had for the Installed App

  2. We need to add some documentation about the fact that incremental auth wouldn't work for installed app, since it isn't support yet by the backend (as of November 2015).

It might be changed in the future but for now, it isn't supported. An alternative is to delete your code form the Installed Apps flow, but I think that we should leave it as it is, because maybe in the near future this support will be added.

So we need to add more documentation in
https://github.com/squid808/google-api-dotnet-client/blob/IncrementalAuthSupport/Src/GoogleApis.Auth/OAuth2/Requests/GoogleAuthorizationCodeRequestUrl.cs#L59
and to the installed app class documentation:
https://github.com/squid808/google-api-dotnet-client/blob/IncrementalAuthSupport/Src/GoogleApis.Auth/OAuth2/Flows/IAuthorizationCodeFlow.cs

  1. We should hurry and add those changes before Monday, because we are working on creating a new release (1.10) in the next few days.
    Otherwise, it will be part of 1.11

  2. Thanks!

@squid808
Copy link
Contributor Author

Sorry - I spent some time trying to learn how to do web dev enough to refactor the task api sample to work, but I hadn't gotten far enough in the limited time I had. You beat me to it =). I did however see that we needed to add it to the web auth, so I had done that. However, I duplicated the ShouldRequestAuthorizationCode for now to get things working similarly. I assume it should go in some utility class, but I wasn't sure if you had a particular class in mind.

That said, I added a sentence description for each class you mentioned (I assumed your second link was a typo and you actually meant the AuthorizationCodeInstalledApp.cs file?), let me know if it's good or not. I'll check in periodically tomorrow to try and get this done.

// If the stored token is null or it doesn't have a refresh token and the access token is expired we need
// to retrieve a new authorization code.
if (token == null || (token.RefreshToken == null && token.IsExpired(flow.Clock)))
//Check if a new authorization code is needed.

This comment was marked as spam.

@peleyal
Copy link
Collaborator

peleyal commented Nov 28, 2015

Thanks for the quick response. Just fix the small comments issues I raised, and we are ready to submit it.
Thanks so much for being patient and thorough!

@squid808
Copy link
Contributor Author

Ok, should be good per your comments. I noticed a spacing issue just after I hit push, and thus the second. Sorry for all the extra work, I feel like you've written more here in the comments than you would have just adding the code yourself. I'll check back throughout today in case you spot something else I missed. Thanks!

@@ -19,6 +19,7 @@
using System.Threading.Tasks;

using Google.Apis.Auth.OAuth2.Flows;
using Google.Apis.Auth.OAuth2.Responses;

This comment was marked as spam.

@peleyal
Copy link
Collaborator

peleyal commented Nov 29, 2015

I didn't. That's the process of the review. I added two tiny comments, address them and we are ready to push 👍 That's great because it's going to be part of the coming release! Exciting :)

@squid808
Copy link
Contributor Author

Done and done, thanks!

peleyal added a commit that referenced this pull request Nov 30, 2015
@peleyal peleyal merged commit 0bf8db8 into googleapis:master Nov 30, 2015
@peleyal
Copy link
Collaborator

peleyal commented Nov 30, 2015

I just thought to mention here the following SO thread:
http://stackoverflow.com/questions/33982392/specify-login-hint-using-net-google-apis-oauth2-v2
@squid808 you suggested in the past supporting a generic way to add any URL parameter. Do you want to create a new issue for it and tackle it in the next few weeks?

Thanks!
Eyal

@squid808
Copy link
Contributor Author

I actually have it mostly done and seemingly working - I've been keeping it in a separate branch until we merged this one. I can create a separate issue if you'd like, or just submit the pull request once it's ready. I figured this one was a little more simple to cut my teeth on for the unit tests, so I was attempting that - and I'm also trying to figure out how to do the web application to test it in. Now that I'm a little more familiar with the process, I can try to get as much of that done first as I can.

I also have to create the issue to request the unit tests for GoogleAuthorizationCodeFlow at the least, as we had discussed.

Either way, your call - I can create an issue for both if you'd prefer.

@peleyal
Copy link
Collaborator

peleyal commented Nov 30, 2015

  1. I think we should have a new Issue & PR for supporting any URL parameters, just to make things clearer.
  2. For testing your change in ASP.NET I hacked the following sample: https://github.com/google/google-api-dotnet-client-samples/tree/master/Tasks.SimpleOAuth2
  3. Yes, please add a new issue for adding unit tests for GoogleAuthorizationCodeFlow.

Thanks!
Eyal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants