Skip to content

Conversation

@walgitrus
Copy link
Contributor

@boxcla
Copy link

boxcla commented Sep 16, 2015

Verified that @walgitrus has signed the CLA. Thanks for the pull request!

@walgitrus walgitrus force-pushed the enh-oauth2-retrieve-tokens branch from d1cd973 to f51c2a4 Compare September 16, 2015 05:24
@walgitrus
Copy link
Contributor Author

looks like requirements.txt should fix cryptography at 0.9.2 (or at least below 1.0) in order for pypy to work

@jmoldow
Copy link
Contributor

jmoldow commented Sep 16, 2015

Thanks for the PR. First look through looks good. We'll fix the cryptography issue (there's a fix in #70, though I might try something else), and then look to merge this.

@jmoldow
Copy link
Contributor

jmoldow commented Sep 16, 2015

I merged a fix for the PyPy issue. Going to close and re-open this PR to try to get Travis to generate the new test matrix.

@jmoldow jmoldow closed this Sep 16, 2015
@jmoldow jmoldow reopened this Sep 16, 2015
@walgitrus walgitrus force-pushed the enh-oauth2-retrieve-tokens branch from f51c2a4 to 18fc5d5 Compare September 16, 2015 18:05
@walgitrus
Copy link
Contributor Author

rebased to pull in the changes from master

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is similar to store_tokens (and they'll probably be used together), let's put this below store_tokens in the argument list. Same in the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes made and pushed

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 18fc5d5 on walgitrus:enh-oauth2-retrieve-tokens into 63b60a2 on box:master.

- add optional retrieve_tokens callback to OAuth2 to fetch possibly refreshed tokens from local store
@walgitrus walgitrus force-pushed the enh-oauth2-retrieve-tokens branch from 18fc5d5 to a880cc3 Compare September 18, 2015 17:59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's expand on this slightly.

We should specify that the callback takes no arguments, and that it should either return None (if no credentials are currently stored), or a (access token, refresh token) tuple.

And maybe we should add a brief description of what this feature would be used for. Something like "When using multiple processes or multiple separate OAuth2 objects, provide a retrieve_tokens function so that the same credentials can be reused across all your OAuth2 objects. The first object that needs to authenticate will do so, and store the tokens with the store_tokens callback (or the authentication and storage might happen entirely outside of the SDK). All subsequent objects, rather than re-authenticating, will use the tokens provided by retrieve_tokens. And the same behavior will occur when they all need to do token refresh." Feel free to reword or make that more concise.

@jmoldow
Copy link
Contributor

jmoldow commented Nov 3, 2015

Hi @walgitrus,

We're going to handle this in PR #81 (see the CooperativelyManagedOAuth2Mixin class). If you want, take a look over there and see if the proposed interface will work for you.

@jmoldow jmoldow closed this Nov 3, 2015
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

Successfully merging this pull request may close these issues.

4 participants