App only auth requests #393
Conversation
…tribute in configurable
…present, lazy load bearer_token if needed
| # bearer_token = client.token | ||
| def token | ||
| # bearer_token = client.app_token | ||
| def app_token |
There was a problem hiding this comment.
why did you choose to rename this?
There was a problem hiding this comment.
The token endpoint will not necessarily remain specific to application only bearer tokens. Presumably twitter will upgrade their User OAuth flow to 2.0 and use the same endpoint for different grant types. Either way, the method is specific to an application token right now and there should be no ambiguity with a user authorized Access Token.
There was a problem hiding this comment.
I was not happy with the name token either since it sounded too generic. However, Twitter has named their method token after all, and the returned JSON data has a field that specifies the token_type.
As you say in your first sentence, it might be the case that this end point will not remain specific to application-only bearer tokens, so it seems wrong to name it app_token. Do you have any other suggestions?
|
Looks like this breaks some tests from the changes for app-only auth that were recently merged. Lazy-loading the bearer token is definitely something that we've wanted to support, can you update the tests as well? I also wonder if there's a way to simplify the logic inside |
|
@spagalloco Do you mind reviewing again please? The specs should be passing with my latest commits. Also, I've snuck in the the rate_limit_status endpoint ;-). |
| @@ -4,6 +4,7 @@ | |||
| require 'twitter/api/favorites' | |||
| require 'twitter/api/friends_and_followers' | |||
| require 'twitter/api/help' | |||
| require 'twitter/api/application' | |||
There was a problem hiding this comment.
these requires are all alphabetized, can you move this require to follow the convention?
|
Thanks for adding tests. I made some notes about ordering some the requires and module includes in We've historically not included the rate_limit_status call deliberately, see #268, #351 and #355 |
| @@ -19,4 +17,9 @@ group :test do | |||
| gem 'webmock' | |||
| end | |||
|
|
|||
| group :development, :test do | |||
There was a problem hiding this comment.
This change does not look relevant to the pull-request. Is this change left over from your local debugging?
|
Sorry to be late to the party. Here is my take on this. @paulwalker Your contribution looks good and is something we have been meaning to add to the library. In addition to @spagalloco's comments, I should note that the tests still do not pass. I think the fix is minor and I've noted what you need to change in line notes. Just to summarize, my points are:
As a final point, I would also like to see tests that check:
These should be simple enough to add. |
|
I'm wondering what the usefulness of the #token/#app_token method really is now with lazy loading of the bearer_token. I suppose there should be some way of reading the bearer token, but perhaps that's best exposed simply by making the attributes on configurable readable? Both "app_token" and "token" names convey a property, but in fact the token method does not actually persist it in the client. I'm really thinking now that the best name would simply be "get_bearer_token". That and make the credentials and bearer token readable properties on configurable. What do you think? |
…rer_token readable
|
I was incorrectly setting the bearer token on the module rather than the client on the module, which would cause any new instantiated clients to receive the bearer_token value. I've corrected that and made the other changes, including specs, with the changes you suggested. Also, the latest commit includes renaming #app_token to #get_bearer_token and making the credentials and bearer_token readable on configurable. Please take a look, I can still change that if desired. |
| # bearer_token = client.token | ||
| def token | ||
| # bearer_token = client.get_bearer_token | ||
| def get_bearer_token |
There was a problem hiding this comment.
I'm strongly against renaming this method to get_bearer_token, especially since it performs an HTTP POST.
|
Since @paulwalker hasn’t responded to the latest round of feedback, I’m just going to merge this whole pull requests and then manually remove the parts I don’t like. |
|
Honestly, the last hold up I believe was on the re-naming of the token method. I don't think token is better than get_bearer_token and I don't think the fact that obtaining a bearer token happens to be a POST request has any real bearing on the name of the method. The use of the verb "get" in method names does not need to be forever reserved for methods that include making an http get request...that's just an implementation detail and does not change the fact that the main "function" of the method is getting a bearer token. Again, with OAuth2, the /token endpoint is not exclusive to these type of tokens, but I guess that in the future token(:type => :bearer) would work as well...shrug. But all good, I didn't mean or want to hold it up on semantics...I was going to change the name back to token. Thanks for merging. |
As proposed by @paulwalker in #393.
Allows for parameter option to force app only auth request. Lazy loads and caches app token if needed.