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

OAuth authentication #66

Merged
merged 8 commits into from
Dec 16, 2015
Merged

OAuth authentication #66

merged 8 commits into from
Dec 16, 2015

Conversation

jacegu
Copy link
Contributor

@jacegu jacegu commented Dec 10, 2015

Support for OAuth-based authentication in API v2

@jacegu jacegu self-assigned this Dec 10, 2015
@jacegu jacegu added this to the 2.0.0 milestone Dec 10, 2015
@jacegu
Copy link
Contributor Author

jacegu commented Dec 14, 2015

@weppos not sure what's the role of Dnsimple::Compatibility class. There are references to both: api_token and exchange_token.

The User struct also includes the api_token attribute which I'm not sure whether it will stick around.

@jacegu
Copy link
Contributor Author

jacegu commented Dec 14, 2015

I am having second thoughts against removing oauth_client_id and oauth_client_secret as configuration parameters for the client. I think it's more convenient to have them there via the environment variables than to having to pull them every time you want to get an access token.

I'll leave them there for now and we can go back to them before closing this PR.

@weppos
Copy link
Member

weppos commented Dec 14, 2015

@jacegu Dnsimple::Compatibility was introduced as a bridge between the API client v1.x and v2. It should be removed for the new version.

@jacegu
Copy link
Contributor Author

jacegu commented Dec 14, 2015

I'd like to have the knowledge of what endpoint to hit to make the OAuth login in the client itself. I think it's convenient as a client user to have it there... It would be something similar to (extracted from an spike):

def authorize_url
  "#{@url}:#{@port}/oauth/authorize?client_id=#{@client_id}&response_type=code&state=1234567"
end

@weppos how do you feel about it?

@weppos
Copy link
Member

weppos commented Dec 14, 2015

Yes, it may be useful.

But I have to ask you to hold off until the Oauth is properly documented and completed. The OAuth controller in fact is still incomplete, and it may result into an incomplete or inefficient implementation in the client.

@jacegu
Copy link
Contributor Author

jacegu commented Dec 14, 2015

With the changes in place you can see this working locally:

$: << 'lib'
require 'dnsimple'

client = Dnsimple::Client.new(oauth_access_token: "XXX")
puts client.get("/v2/1/domains")

@weppos weppos modified the milestones: API v2, 2.0.0 Dec 16, 2015
weppos added a commit that referenced this pull request Dec 16, 2015
OAuth authentication support
@weppos weppos merged commit c8ac607 into api-v2 Dec 16, 2015
@weppos weppos deleted the feature/oauth branch December 16, 2015 13:23
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.

2 participants