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

OAuth2Session(..., auto_refresh_kwargs) and OAuth2Session.fetch_token(..., **kwargs) serve the same purpose #81

Closed
shazow opened this issue Sep 29, 2013 · 7 comments

Comments

@shazow
Copy link
Contributor

shazow commented Sep 29, 2013

Would be nice if they could be formally coupled. Such as maybe rename the former to something like fetch_token_kwargs, which can serve as the default if no fetch_token(..., **kwargs) are provided as an override?

@ib-lundgren
Copy link
Member

The auto_refresh_kwargs are meant for implicit/automatic token refresh*, docs, using refresh_token behind the scenes whenever a token expires during normal API interactions. In contrast, fetch_token(i.e. obtaining a new token) will almost always be explicitly called during the OAuth dance. Since the two differ in which parameters are needed I don't think re-using the refresh args is a good idea. I rather dislike the idea of passing/storing kwargs for a method in the constructor but its necessary for auto refresh. Adding a second set of kwargs for fetch_token I'd rather avoid.

However maybe your use-case can convince me otherwise, what did you have in mind where it would be better to pre-store the default args?

@shazow
Copy link
Contributor Author

shazow commented Oct 1, 2013

I agree with what you've said, though likely it is my understanding of the OAuth2 spec that is lacking, but when would an automatic token refresh be different from a manual token fetch?

I haven't worked with very many OAuth2 API's, so it's quite likely I simply haven't run into the scenario myself.

@ib-lundgren
Copy link
Member

An automatic refresh is not different from a manual refresh, but it is different from a token fetch in that you refresh your access token rather than obtaining an initial one.

Using Google as an example.

  1. You get an access token and a refresh token by through the webserver OAuth dance. Part of which includes redirecting the user to Google.
  2. You store the tokens in your DB
  3. You fetch some G+ posts using the access token.
  4. The user goes away for a while.
  5. The user returns and want to continue using your app.
  6. You load up the access token and refresh token from DB and continue.
  7. Fetch another G+ post, but it fails.
  8. Turns out the access token is expired so requests-oauthlib automatically goes and gets a new access token using the refresh token and a HTTPS POST request.
  9. Fetch the G+ post again, this time it worked.

As you see above the time point where a token is refreshed is very different from when it is first obtained. A refresh is invisible to the user. Obtaining a token (using fetch_token) is most often not.

To get an access and refresh token you could use most of different flavours of the OAuth dance, e.g. webserver and installed together with fetch_token. Once that is done you can refresh the access token when it expires (could be within an hour) using the refresh token (which maybe never expires). This refreshing differs from each of the OAuth dances above, it is actually its own OAuth dance (grant type) on its own, and a pretty simple one too. The token refresh can be done as long as you have a refresh token, it does not depend on how you got the refresh token in the first place.

The reason there is one fetch_token and one refresh_token method rather than just fetch_token is that the latter usually follows the former and was intended to be less confusing rather than re-using fetch_token for token refresh.

@shazow
Copy link
Contributor Author

shazow commented Oct 1, 2013

Yes, I get how the flow works. My point was: Are there scenarios when you would give fetch_token different extra kwargs than refresh_token?

In my code, I pass the client_id and client_secret to fetch_token(...), but I also pass the same extra kwargs to auto_refresh_kwargs=... in my constructor. Felt redundant, that's all. :)

@ib-lundgren
Copy link
Member

Ah, my apologies for the walls of text then :)

It is probably often the same for authorization code grant but less likely when using resource owner password credentials grant. It is a balancing act here, for many it might work to just re-use them but for some it won't and might end up causing very confusing problems since some arguments get passed unexpectedly. I agree it feels redundant but I tend to opt for least surprise.

There might be a nicer way to deal with it than appears to me now, feel free to point it out if you find it :)

@shazow
Copy link
Contributor Author

shazow commented Oct 1, 2013

Totally fair. Also, my fault for not being clear. Thank you for taking the time to explain. :)

Really enjoying the library so far, looking forward to contributing at some point!

@shazow shazow closed this as completed Oct 1, 2013
@ib-lundgren
Copy link
Member

Looking forward to it :) One invaluable thing you might do as you get familiar is to point out shortcomings in the documentation in #48.

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

No branches or pull requests

2 participants