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

Authentication should be in place for all API calls #33

Closed
albireox opened this issue Nov 2, 2016 · 4 comments
Closed

Authentication should be in place for all API calls #33

albireox opened this issue Nov 2, 2016 · 4 comments
Assignees
Labels
bug a general bug or other breaking feature enhancement indicates a new feature or functionality to be added
Milestone

Comments

@albireox
Copy link
Member

albireox commented Nov 2, 2016

Issue by havok2063
Friday Oct 07, 2016 at 18:54 GMT
Originally opened as https://github.com/marvin-manga/marvin/issues/70


Marvin API calls needs authentication. We can use the authentication in requests to do this. The short term basic solution to implement the netrc approach with the standard sdss SAS authentication. Long term is to implement a session key using requests-oauth, and OAuth1 library, which is common for most modern APIs. Also move the login to Trac authentication, to track who is using marvin.

@albireox albireox added this to the Marvin Beta milestone Nov 2, 2016
@albireox albireox added bug a general bug or other breaking feature enhancement indicates a new feature or functionality to be added labels Nov 2, 2016
@albireox
Copy link
Member Author

albireox commented Nov 2, 2016

Comment by havok2063
Saturday Oct 08, 2016 at 22:23 GMT


I have turned on the API authentication at Utah. And added some auth stuff into the Interaction class. By default, with no auth specified, requests will look for authentication credentials in a local .netrc file. So I didn't have to do much to implement this. I also added in the future option of changing the authentication to oauth.

I've also changed the request to use a Session instead. This will open and maintain a single request session so all requests use one tcp connection. I'm not sure if we want to do this since there's no way of dictating when to close the session. See
http://docs.python-requests.org/en/master/user/advanced/#request-and-response-objects
and
https://en.wikipedia.org/wiki/HTTP_persistent_connection

I've also added in optional session caching into the requests, using a new library called CacheControl. If Marvin can import this package it will use it, otherwise no. Go here
http://docs.python-requests.org/en/master/community/recommended/
and install / upgrade both CacheControl and Requests-Toolbelt

@albireox
Copy link
Member Author

albireox commented Nov 2, 2016

Comment by albireox
Sunday Oct 09, 2016 at 02:04 GMT


This looks good to me. I'm not sure about the single request session. I'll read the links, and let's talk about it next week on the call.

Does the interaction automatically prompts you for user and password if it cannot find them in the netrc file and, if introduced, saves them there?

@albireox
Copy link
Member Author

albireox commented Nov 2, 2016

Comment by havok2063
Tuesday Oct 11, 2016 at 15:58 GMT


Yeah, I'm not sure either about the request session. If we can figure out a nice way to close them after X time, then maybe it can help us. Otherwise, we can go back to ad hoc request calls, where each get and post is a separate tcp connection.

The Interaction raises a BrainAPIAuthError if the authentication fails and in the message tells you to create or check your credentials in your netrc file. With the way things are set up now, when a user tries to do something remotely without auth, it fails, a Warning gets generated when attempting to get the config.urlmap for the first time but keeps going, then fails inside whatever code with a MarvinError, cannot make remote calls, no urlmap.

@havok2063 havok2063 self-assigned this Nov 2, 2016
@havok2063 havok2063 modified the milestones: Post-Beta, Marvin Beta Nov 10, 2016
@havok2063
Copy link
Collaborator

This next step here is to do Oauth authentication, with Trac and API key. Closing this ticket and reopening a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug a general bug or other breaking feature enhancement indicates a new feature or functionality to be added
Projects
None yet
Development

No branches or pull requests

2 participants