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

Add openneuro-py login command to enable authentication #74

Merged
merged 14 commits into from
Dec 13, 2022

Conversation

wmvanvliet
Copy link
Contributor

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. The openneuro-py download command has been modified to send the token along as a cookie.

fixes #60

@hoechenberger
Copy link
Owner

hoechenberger commented Oct 10, 2022

Hello, thanks for the work!!!

Injecting a cookie feels a little smelly too me, can we not use an Authorization: Bearer {token} header instead?

@wmvanvliet
Copy link
Contributor Author

I looked at how the official openneuro-cli does it, but that doesn't mean it's the best way. This Authorization: Bearer {token} header sounds cleaner. I'm not familiar with this header, is it as simple as just adding this line to the HTTP header?

@wmvanvliet
Copy link
Contributor Author

Hrm no, adding a base_headers={'Authorization': f'Bearer {token}'}) did not do the trick.

@hoechenberger
Copy link
Owner

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

@larsoner
Copy link
Collaborator

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 main not PRs and would only work locally for people with token-access to the dataset, which is less than ideal

@wmvanvliet
Copy link
Contributor Author

I've created a barebones dataset on open neuro using a dummy google account. Test added.

@hoechenberger
Copy link
Owner

hoechenberger commented Oct 11, 2022

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 Authorization bearer header for uploads -- why the difference?)

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.':
Copy link
Owner

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

Copy link
Contributor Author

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?

Comment on lines 205 to 208
try:
get_token()
raise RuntimeError('It seems you do not have access to '
'this dataset.')
Copy link
Owner

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?

Copy link
Contributor Author

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.

@hoechenberger
Copy link
Owner

@wmvanvliet Could you please pull in main or rebase? Then I can do a proper review :) Thanks!

@hoechenberger hoechenberger added this to the next-release milestone Dec 13, 2022
openneuro/config.py Outdated Show resolved Hide resolved
openneuro/config.py Outdated Show resolved Hide resolved
@hoechenberger hoechenberger enabled auto-merge (squash) December 13, 2022 13:22
@hoechenberger hoechenberger merged commit a82c677 into hoechenberger:main Dec 13, 2022
@hoechenberger
Copy link
Owner

Thanks, @wmvanvliet!

And sorry for the slow action on this one

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.

Can this access private datasets?
3 participants