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

Google offline #464

Closed
wants to merge 12 commits into from
Closed

Google offline #464

wants to merge 12 commits into from

Conversation

@nwmartin
Copy link
Contributor

@nwmartin nwmartin commented Nov 7, 2012

Adds support for specifying online or offline access types via google oauth2. We had a long running service that required it. The access type is configurable during UI configuration. The refresh token is stored in mongo for offline-style access - the thought was that actual offline calls will be well outside of meteor, so it's up to whomever is running that code to worry about replacing tokens when they expire.

Comments appreciated.

@heavyk
Copy link

@heavyk heavyk commented Nov 7, 2012

nice! look forward to using this

@scottburch
Copy link

@scottburch scottburch commented Nov 8, 2012

Thank you so much. I needed this today so I pulled it. Good timing.

@scottburch
Copy link

@scottburch scottburch commented Nov 8, 2012

I'm looking into it, but refreshToken is getting set to null. Might want to hold off pulling this one until a resolution. I'm looking into what I am seeing. Any thoughts nwmartin?

@scottburch
Copy link

@scottburch scottburch commented Nov 8, 2012

Ok, weird. I undid my change and retyped it and everything seems to be working again. Sorry about that. Must have been a slight typo.

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Nov 8, 2012

I've seen it happen in two ways. If the access type is set to online, there's no refresh token, so it's going to be null. Refresh tokens will only show up when offline access has been configured. The other way is a little bit different - you need to get a refresh token for a user, then blow away that user in mongo. Subsequent calls will be validated by google, but there won't be a refresh token in the response until the user revokes access.

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Nov 8, 2012

Whew, I'm glad it works for you! We've been using it for two days now, and with a ruby backend that actually refreshes access tokens using the refresh token, it's working pretty well.

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Nov 12, 2012

Caught a bug in this which I'm fixing now. Basically, refresh tokens will get swapped to null with a login/logout because google does not send them again.

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Nov 12, 2012

Should be all set.

@scottburch
Copy link

@scottburch scottburch commented Nov 12, 2012

Thanks. I'm going to pull again. I have noticed this happening, but was putting off looking into it.

@scottburch
Copy link

@scottburch scottburch commented Nov 14, 2012

Unfortunately, my refreshToken is back to 'null' again. I'm going to try to figure out why.

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Nov 15, 2012

Let me know if you figure it out. We haven't had it happen yet with relatively heavy usage of logging in and out.

@scottburch
Copy link

@scottburch scottburch commented Nov 16, 2012

It does not appear to be happening anymore. Question, how are you obtaining a new request token. Did I miss something in the code or are you doing it manually? Seems like it would be nice to run data requests through something that would ensure there is a new request token first.

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Nov 16, 2012

So I decided not to have meteor actually get new access tokens. In our design, offline tokens are only really used when the user is not online - which likely means no meteor. We just make a call against the google api after snagging a user's refreshToken to get new access tokens. This is in ruby.

@glasser
Copy link
Member

@glasser glasser commented Nov 16, 2012

@nwmartin Does that mean you don't want this pull request reviewed? Or that you're planning to remove part of it?

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Nov 16, 2012

Oh no, it was a conscious design decision when I wrote it to begin with. It doesn't seem to me like meteor is the right place for renewing access tokens with a refresh token.

To clarify: this should be reviewed =). If you guys disagree with that decision, let me know and I can add in the extra functionality.

@scottburch
Copy link

@scottburch scottburch commented Nov 16, 2012

Thanks for the response. I need to get new tokens only because I am using the app for a 'desktop' environment that is long lived and the tokens are expiring, so sync stops working. Right now I have to logout and log back in to get the new token.

I just wanted to ensure that I did not miss anything before I write something to refresh the token. It might be nice to have some mechanism to get a new token in the offline stuff. I can add this functionality if you like. I wanted to get an opinion on it before I did.

Anyone from the meteor team have an opinion on this? I am not an expert using these apis so it may be a bit Oauth specific. Not sure. Otherwise the patch is working well for me.

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Nov 16, 2012

@scottburch Right, that's pretty much the exact scenario we were facing.

While it'd be nice to have something in the offline code, the problem we were facing was how the heck would we call it? There's no guarantee any meteor code is running. Currently you'll have to write it yourself in the language of your choice.

If there's a clever way to have tokens autorefresh I'd be happy to add it in, but I didn't see a good way to do it on the server side in meteor.

@avital
Copy link
Contributor

@avital avital commented Nov 16, 2012

@scottburch @nwmartin Are you guys referring to #444?

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Nov 16, 2012

@avital Yes, but specifically for google's flavor.

@scottburch
Copy link

@scottburch scottburch commented Nov 20, 2012

@nwmartin - thanks. I guess it was called off for now.

@@ -33,9 +43,17 @@
throw result.error;
if (result.data.error) // if the http response was a json object with an error attribute
throw result.data;
return result.data.access_token;

This comment has been minimized.

@avital

avital Nov 20, 2012
Contributor

Don't worry about fixing this -- I'm taking this PR and making changes on top of it, but if you're going to make this function no longer return an access token it's name should no longer be getAccessToken.

This comment has been minimized.

@nwmartin

nwmartin Nov 21, 2012
Author Contributor

I agree with these sorts of things, but I honestly couldn't think of a better name at the time. Perhaps something more along the lines of getAccessTokenResponse? Starting to sound Java-y.

This comment has been minimized.

@avital

avital Nov 21, 2012
Contributor

I'm going for getTokens, but don't feel strongly either.

var identity = getIdentity(accessToken);

if (!refreshToken) {

This comment has been minimized.

@avital

avital Nov 20, 2012
Contributor

Can this if clause and the getRefreshToken function be made unnecessary if we change Accounts.updateOrCreateUserFromExternalService to not update fields that are already there? Specifically by making the following line dig into each property and construct specific $set directives? https://github.com/meteor/meteor/blob/master/packages/accounts-base/accounts_server.js#L222

(Not even totally sure this is a good change but want to understand this code better)

This comment has been minimized.

@nwmartin

nwmartin Nov 21, 2012
Author Contributor

That seems like a stab in the right direction, but what about online access tokens? Currently, they work just fine with nothing touching meteor, but I believe they do in fact update the access token with each log in?

This code currently exists to specifically handle the case of later logins through meteor - services will never return another refresh token, so we don't want to override them with a null on a login past the very first. I'm not sure any of the other fields behave similarly.

This comment has been minimized.

@nwmartin

nwmartin Nov 21, 2012
Author Contributor

Ah, from the comment above it looks like the set of email addresses associated with an account can change as well, FWIW.

This comment has been minimized.

@avital

avital Nov 21, 2012
Contributor

My point was not to not update existing fields but rather set each field separately so that we don't lost fields that had already been there and now have a null or undefined value.

But thinking about it some more, I understand that you introduced this change in response to this comment but I just want to better understand this -- is it the case that google consistently doesn't supply a refresh token at any point other than initial login? Their docs didn't seem to imply this?

This comment has been minimized.

@avital

avital Nov 21, 2012
Contributor

Interesting, found this explanation: http://stackoverflow.com/a/10857806/1352190

@ghost ghost assigned avital Nov 25, 2012
@joscha
Copy link

@joscha joscha commented Dec 3, 2012

@avital Is there any news on merging this in?

@avital
Copy link
Contributor

@avital avital commented Dec 3, 2012

Sorry for the delay. I will finish merging this in today, with the needed changes.

avital added a commit that referenced this pull request Dec 3, 2012
Don't lose fields that were added in a previous login, but that aren't
received on a subsequent one. An example of this is the refresh token
sent from Google only on first login, in the case that the service
is configured on "offline" mode.

This is a prerequisite for merging in #464
@avital
Copy link
Contributor

@avital avital commented Dec 3, 2012

I made some changes and merged this into the avital-google-offline branch. One of the changes is that now you configure to request an offline token in Accounts.ui.config, or as an argument to Meteor.loginWithGoogle, rather than as part of the set-up wizard.

Would any of you want to give this a try before I merge this into devel? I did some basic testing but since this wasn't my change originally just wanted to get some more feedback.

@scottburch
Copy link

@scottburch scottburch commented Dec 4, 2012

I'm happy to test it. I will do so tonight.

@scottburch
Copy link

@scottburch scottburch commented Dec 4, 2012

I do get a refresh token after the first login. However, if I log out and then back in the refreshToken is set to null. I remember this same thing happened with the original code until a patch was done to get around it.

@avital
Copy link
Contributor

@avital avital commented Dec 4, 2012

Thanks, this is interesting. That exact use-case was supposed to be fixed
by
27113a3.
I'll take a look.

On Mon, Dec 3, 2012 at 7:05 PM, Scott Burch notifications@github.comwrote:

I do get a refresh token after the first login. However, if I log out and
then back in the refreshToken is set to null. I remember this same thing
happened with the original code until a patch was done to get around it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/464#issuecomment-10982444.

@avital
Copy link
Contributor

@avital avital commented Dec 4, 2012

Found the bug.

On Mon, Dec 3, 2012 at 7:08 PM, Avital Oliver avital@meteor.com wrote:

Thanks, this is interesting. That exact use-case was supposed to be fixed
by
27113a3.
I'll take a look.

On Mon, Dec 3, 2012 at 7:05 PM, Scott Burch notifications@github.comwrote:

I do get a refresh token after the first login. However, if I log out and
then back in the refreshToken is set to null. I remember this same thing
happened with the original code until a patch was done to get around it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/464#issuecomment-10982444.

@joscha
Copy link

@joscha joscha commented Dec 4, 2012

yay! Is there anything else to test? I'd be happy to help out...

@avital
Copy link
Contributor

@avital avital commented Dec 4, 2012

I reproduced and fixed the bug on the latest version of the
avital-google-offline branch.

On Mon, Dec 3, 2012 at 7:15 PM, Avital Oliver avital@meteor.com wrote:

Found the bug.

On Mon, Dec 3, 2012 at 7:08 PM, Avital Oliver avital@meteor.com wrote:

Thanks, this is interesting. That exact use-case was supposed to be fixed
by
27113a3.
I'll take a look.

On Mon, Dec 3, 2012 at 7:05 PM, Scott Burch notifications@github.comwrote:

I do get a refresh token after the first login. However, if I log out
and then back in the refreshToken is set to null. I remember this same
thing happened with the original code until a patch was done to get around
it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/464#issuecomment-10982444.

@scottburch
Copy link

@scottburch scottburch commented Dec 4, 2012

Sorry, I think I just remembered why that happens. It is because google does not send a refresh token if you are already authorized so you have to check the return to ensure that it is there before writing it to the DB.

I'll pull and test again.

@avital
Copy link
Contributor

@avital avital commented Dec 4, 2012

Yes, you are exactly right.

On Mon, Dec 3, 2012 at 7:52 PM, Scott Burch notifications@github.comwrote:

Sorry, I think I just remembered why that happens. It is because google
does not send a refresh token if you are already authorized so you have to
check the return to ensure that it is there before writing it to the DB.

I'll pull and test again.


Reply to this email directly or view it on GitHubhttps://github.com//pull/464#issuecomment-10983295.

@nwmartin
Copy link
Contributor Author

@nwmartin nwmartin commented Dec 4, 2012

Yeah, this is the bane of oauth. The only way to get a new token is to force revoke access on that user and then make them sign up again =/

(Don't lose them =))

@avital
Copy link
Contributor

@avital avital commented Dec 4, 2012

Nope. We store the refresh token on first login and never forget it.
On Dec 3, 2012 7:56 PM, "Nicholas Martin" notifications@github.com wrote:

Yeah, this is the bane of oauth. The only way to get a new token is to
force revoke access on that user and then make them sign up again =/


Reply to this email directly or view it on GitHubhttps://github.com//pull/464#issuecomment-10983360.

@joscha
Copy link

@joscha joscha commented Dec 4, 2012

Tested and works like a charm.

For anyone else wanting to try this:

Accounts.ui.config({
  requestOfflineToken: {
    google: true
  }
});
@scottburch
Copy link

@scottburch scottburch commented Dec 4, 2012

The token is getting refreshed on login, but it is not getting refreshed by the wrapper. I'm going to find out why.

@scottburch
Copy link

@scottburch scottburch commented Dec 4, 2012

@avital I don't see the http wrapper stuff. Did it not get included? Is that coming next?

@avital
Copy link
Contributor

@avital avital commented Dec 4, 2012

@scottburch Let's continue that conversation on #495.

@avital
Copy link
Contributor

@avital avital commented Dec 4, 2012

This was merged to devel (and rebased, so github didn't automatically pick this up)

@avital avital closed this Dec 4, 2012
@avital avital mentioned this pull request Dec 4, 2012
@joscha
Copy link

@joscha joscha commented Dec 4, 2012

@avital When authenticating against Google, together with the accessToken, a number of seconds is given when the accessToken expires - is the resulting time (now + timeoutSeconds) saved somewhere? Because the refreshToken does only need to be used once the accesToken is not valid any more, hence when requests are made while it is still valid, numerous requests to get a new accessToken through the refreshToken can be saved.

@avital
Copy link
Contributor

@avital avital commented Dec 4, 2012

Good point. I don't think we currently store this. It seems like what you are proposing would be an optimization to #522, is that correct? Is so could you please write your comment there and we can all continue the conversation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants