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

API-Key #14

Closed
wush978 opened this issue Apr 20, 2015 · 12 comments
Closed

API-Key #14

wush978 opened this issue Apr 20, 2015 · 12 comments
Milestone

Comments

@wush978
Copy link
Contributor

wush978 commented Apr 20, 2015

Dear maintainers,

The ckan platform allow authentication and authorization with API-Key in http request.

I noticed that we could append the API-Key through add_headers with key X-CKAN-API-Key to authenticate with ckan server. The main problem to me is a secure way to let the user input their API-Key.

I propose two way to let the users input their API-Key.

The first way is let them set the option properly in .Rprofile or somewhere. That is to say, we could read the options from getOption. In such way, the user does not need to input their API-Key.

Another way is to receive the input through svDialogs::dlgInput where the input will not be log into the .history file.

How do you think?

Thanks,
Wush

@wush978
Copy link
Contributor Author

wush978 commented Apr 20, 2015

If you are willing to have this feature or want to check the code (sorry that my message might not be clear enough), I could create a PR for this.

@sckott
Copy link
Contributor

sckott commented Apr 20, 2015

@wush978 We do already have authentication in some of the functions that require it. using add_headers(Authorization = <key>), and have the key passed in as a parameter. E.g. see ds_create_dataset().

Where are docs for CKAN for using X-CKAN-API-Key?

I do not want to use svDialogs::dlgInput because it sounds like that is a GUI.

Right now we have key passed in as a parameter. One thing we could do is look for an option of the appropriate name in the users options, and read it in if there, or use the passed in key if that is provided. Sound okay? That way users have an option of how to do auth.

@wush978
Copy link
Contributor Author

wush978 commented Apr 20, 2015

Sorry that I did not notice it.

I agree with you that using options should be good enough. A possible improvement might be providing a helper function to let the user set the options more easily. And once they set the API-Key, maybe we could append the http option of api-key automatically to ckan_POST for not only ds_create_dataset but also group_list and all functions that requires authentications.

And I suggest that we use the X-CKAN-API-Key as the key in options for api-key. The related documentation is here: http://docs.ckan.org/en/ckan-2.2/api.html#authentication-and-api-keys. IMO, it is intuitive for developers and more specific than Authorization (less possible to collide with other options). With the helper function, the user does not need to reach the string X-CKAN-API-Key.

For the issue of svDialogs::dlgInput, I think this feature is less important and I understand that you might not want to introduce more dependencies on svDialogs. Moreover, it might be off topic. Maybe we could discuss it later in another issue after agreeing and finishing previous features.

@sckott
Copy link
Contributor

sckott commented Apr 20, 2015

good idea to allow setting via a helper function. Could change functions in https://github.com/ropensci/ckanr/blob/master/R/base_url.R to just be e.g., ckanr_options() or something like that, to set base url, api key, and anything else

Using X-CKAN-API-Key sounds good.

please do send a PR

wush978 added a commit to wush978/ckanr that referenced this issue Apr 25, 2015
@wush978
Copy link
Contributor Author

wush978 commented Apr 25, 2015

Dear @sckott ,

The 7eea607 changes the behavior of the ckan_POST, but I have not completely tested all functions in ckanr. In fact, I want to help you finish #1 so that I can conveniently test whether 7eea607 harms any existed feature.

Do you have any idea about this?

@sckott
Copy link
Contributor

sckott commented Apr 25, 2015

I agree to get the test suite filled out. And will make any more development easier as we will know when things break.

I think we can test against the demo server (at http://demo.ckan.org), right?

I'll start the tests, and a list in that issue, and we can tick them off as they are completed. You can put your name by ones you are working on.

@sckott sckott modified the milestone: v0.1 Apr 28, 2015
@florianm
Copy link
Contributor

@wush978 @sckott ckan_POST still needs to make use of its key parameter (currently it uses api_key()). FYI I've worked on using the API key in #35 and #31, but options didn't work for the

@sckott
Copy link
Contributor

sckott commented May 20, 2015

@florianm looks like you didn't finish a sentence?

@florianm
Copy link
Contributor

@wush978 @sckott ckan_POST still needs to make use of its key parameter (currently it uses api_key()). FYI I've worked on the API key in #35 and #31, but I had to use Sys.env to store API key (see set_test_env()), as testthat can't access options from its sandboxed test environment. Edit: bless you, Chrome-stable, for freezing my machine mid-comment.

@sckott
Copy link
Contributor

sckott commented May 20, 2015

@florianm note that I switched us to use all env variables now, so no more options

@florianm
Copy link
Contributor

I see, looks much cleaner now, ta!

@sckott
Copy link
Contributor

sckott commented May 27, 2015

I think this is handled now

@sckott sckott closed this as completed May 27, 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

No branches or pull requests

3 participants