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

Add signals when a token is generated #489

Closed
wants to merge 4 commits into from
Closed

Conversation

samupl
Copy link
Contributor

@samupl samupl commented Jun 16, 2017

This PR adds a very simple signal that fires up each time a token has been generated for an application.

Now I don't know if this is the right place to have such signal, but it looks like the right one (oauthlib isn't django-specific, so it didn't seem a good place to put it there in create_token_response).

Please let me know if it's a feature that you'd like to see in the upstream - if so, I will write additional unit tests.

@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.05%) to 93.44% when pulling cf165c6 on samupl:signals into bcb94b9 on evonove:master.

@samupl
Copy link
Contributor Author

samupl commented Jun 16, 2017

I wonder, however, if simply binding to oauth2_validators.OAuth2Validator._create_access_token or even oauth2_validators.OAuth2Validator.save_bearer_token and just trigger the signal when the token is created, what do you think?

if status == 200:
access_token = json.loads(body).get('access_token')
if access_token is not None:
token = AccessToken.objects.get(token=access_token)
Copy link

Choose a reason for hiding this comment

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

AccessToken is a swappable model, you need to instantiate is using get_access_token_model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied, thanks

@coveralls
Copy link

coveralls commented Jun 17, 2017

Coverage Status

Coverage increased (+0.05%) to 93.44% when pulling 73a6308 on samupl:signals into bcb94b9 on evonove:master.

@jleclanche
Copy link
Member

This looks great, could you add documentation?

@samupl
Copy link
Contributor Author

samupl commented Jun 18, 2017

@jleclanche Here you go ;)

@coveralls
Copy link

coveralls commented Jun 18, 2017

Coverage Status

Coverage increased (+0.05%) to 93.44% when pulling 7ab66a9 on samupl:signals into bcb94b9 on evonove:master.

@samupl
Copy link
Contributor Author

samupl commented Jun 20, 2017

I did a small change to the code - instead of passing the application down to the signal, I'm now passing the token, as it contains more useful information

@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage increased (+0.05%) to 93.44% when pulling 525ce48 on samupl:signals into bcb94b9 on evonove:master.

@samupl
Copy link
Contributor Author

samupl commented Jul 12, 2017

So, anything else is needed in this PR? Can it be safely merged?

@jleclanche
Copy link
Member

Landed in 18bbdc1 - thank you for your contribution!

@jleclanche jleclanche closed this Jul 12, 2017
@adam-tokarski
Copy link

When this change will be deployed to pip?

@jleclanche
Copy link
Member

I don't think any time soon seeing as it's the only change since 1.0. Tip: you can do pip install https://github.com/evonove/django-oauth-toolkit/archive/master.zip to install the latest master. Replace master with a commit hash if you want to bind to a commit:

pip install https://github.com/evonove/django-oauth-toolkit/archive/18bbdc1.zip

@adam-tokarski
Copy link

Yes, I was installing it from the latest master directly. Thanks for response!

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.

5 participants