-
Notifications
You must be signed in to change notification settings - Fork 12
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 openneuro-py login command to enable authentication #74
Conversation
Hello, thanks for the work!!! Injecting a cookie feels a little smelly too me, can we not use an |
I looked at how the official openneuro-cli does it, but that doesn't mean it's the best way. This |
Hrm no, adding a |
Ok never mind then, if the cookie works, let's use it! Do you think there is any way to add a test? I will only be able to do a full review during the next few days though |
Can we add a minimal dummy restricted dataset to openneuro, make a dummy user who is the only one with read access to it, make a token for that user, and use it in tests here (by hard-coding the token in the test file)? That way local and remote testing of this feature is possible. It sounds easy in principle for someone familiar with OpenNeuro, but maybe it's not Alternatively we could make a private GH-secrets token and access an actual dataset that way (presumably whatever dataset @wmvanvliet is working with), but this would only be used on |
I've created a barebones dataset on open neuro using a dummy google account. Test added. |
I think we should coordinate this with the OpenNeuro folks :) @nellh what we're trying to do is find (or create) a private dataset for testing, so we can check whether token-based auth works -- any recommendations welcome!! (btw apparently openneuro-cli sends the token as a cookie for download, but an |
raise RuntimeError(f'Query failed: ' | ||
f'"{response_json["errors"][0]["message"]}"') | ||
msg = response_json["errors"][0]["message"] | ||
if msg == 'You do not have access to read this dataset.': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to check the HTTP status code here instead of the error string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but do we have easy access to the status code at this point in the code?
openneuro/download.py
Outdated
try: | ||
get_token() | ||
raise RuntimeError('It seems you do not have access to ' | ||
'this dataset.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the logic here, shouldn't we have checked whether a token exists before even sending the request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We indeed check this when creating the session (the actual request is made by the graphql lib, so I figured that the easiest place to inject the cookie was at the session level). However, at that point, we don't want to raise any errors if there is no token. Usually, we are downloading public datasets and no token is necessary. So at that point, any error messages are lost.
When we get an "access denied" and want to guide the user to a solution, then we actually want the error message to guide the user to a solution. The easiest way to get it was to try and get the token again and catch the error. We could catch the error messages upon creating the session but then we would have to store them somewhere and that seems more complex than just doing get_token()
again.
That was the logic at the time and for the moment, I stand by it.
@wmvanvliet Could you please pull in |
Thanks, @wmvanvliet! And sorry for the slow action on this one |
To download restricted datasets, we need to authenticate ourselves using an API token. This PR adds a new command
openneuro login
to write the API token to the~/.openneuro
config file. Theopenneuro-py download
command has been modified to send the token along as a cookie.fixes #60