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

Allow for no authentication and NASA Earthdata (URS) authentication for OPeNDAP #57

Merged
merged 3 commits into from
Mar 11, 2020
Merged

Conversation

weiji14
Copy link
Collaborator

@weiji14 weiji14 commented Oct 16, 2019

Besides the default Earth System Grid Federation (ESGF) authentication method, this Pull Request also allows for NASA Earthdata Login (URS) authentication and no authentication.

TODO:

  • Initial setup and unit test for simple no authentication case
  • Add (mock?) tests for urs and esgf authentication methods
  • Add unit tests for invalid authentication

Links:

Besides the original "ESGF" authentication method, this commit adds a "URS" authentication and also no authentication. Also adding a no authentication test case to test_intake_xarray, which required adding 'pydap' to the conda environment.yml files and a new example dataset in catalog.yaml.
@martindurant
Copy link
Member

Personally I know nothing about these auth mechanisms, so please tag someone for a review when you are ready.

Mock testing the ESGF and URS authentication methods for connecting to an OPeNDAP server. Also updated inline docs to mention that the DAP_USER and DAP_PASSWORD environment variables need to be set for authentication to work. As pydap is not available in the default conda channel, I've removed it from the environment-py36-default.yml file.
Ensure ValueError is raised properly when we pass in an invalid OPeNDAP authentication method that is not None, 'esgf' or 'urs'.
@weiji14 weiji14 marked this pull request as ready for review October 17, 2019 03:47
@weiji14
Copy link
Collaborator Author

weiji14 commented Oct 17, 2019

Ok, this should be ready for review! @mmccarty, I see you've worked on the original ESGF authentication method, do you mind taking a look? This Pull Request basically adds a few more authentication methods, and includes some unit tests to cover the intake OPeNDAP extension.

@martindurant
Copy link
Member

Sorry that this dropped off my list.
@mmccarty , did you have anything to say here? If I don't hear in a few days, I am happy to merge.

password = os.getenv('DAP_PASSWORD', None)
session = setup_session(username, password, check_url=self.urlpath)

return session
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

session can be None? I would have thought you still need to make a non-authenticated session of some sort (not that I'm certain what this session thing is, perhaps pydap makes its own for the None case).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes session can be None, probably just an anonymous connection then? See https://github.com/pydap/pydap/blob/3f6aa190c59e3bbc6c834377a61579b20275ff69/src/pydap/client.py#L58

"""
name = 'opendap'

def __init__(self, urlpath, chunks, xarray_kwargs=None, metadata=None,
def __init__(self, urlpath, chunks, auth="esgf", xarray_kwargs=None, metadata=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you want the default auth to be esgf?

Copy link
Collaborator Author

@weiji14 weiji14 Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESGF was the 'default' authentication method in the original code. Personally I would prefer using URS as the default, but might be good to preserve backward compatibility here. Edit: Or do you mean we should set the default to None i.e. no authentication?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaults are hard. Perhaps, @danielballan or @jsignell care to weigh in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with @martindurant's comment in the main thread. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Same for me

@mmccarty
Copy link
Member

Thanks @weiji14 ! Just a few questions but looks good otherwise.

@weiji14
Copy link
Collaborator Author

weiji14 commented Mar 10, 2020

Cool, was just getting back to using intake recently so good timing! I think the issue comes down to whether we want to keep the default authentication as ESGF (falling back to None if no username/password is provided) or URS. Keeping it as ESGF would preserve backward compatibility, but it might make sense to move to URS if more people use it instead?

@martindurant
Copy link
Member

Alright, lets keep the same default as before for the time being, so long as it's documented that passing None or not supplying a username/password ends up with anonymous access.

I suppose there are other auths that would eventually be supported? Then, we may want to come up with something more generic.

@martindurant martindurant merged commit deecef7 into intake:master Mar 11, 2020
@martindurant
Copy link
Member

Thanks

@weiji14 weiji14 deleted the opendap/auth branch March 11, 2020 19:55
d70-t added a commit to d70-t/intake-xarray that referenced this pull request Aug 19, 2020
Due to intake#62, accessing OPeNDAP is not possible anymore using the
`netcdf`driver. But because of intake#57, this functionality is now included
in the `opendap` driver.
d70-t added a commit to d70-t/intake-xarray that referenced this pull request Sep 14, 2020
Apart from historic reasons, using ESGF as the default authentication
method does not seem to be a straight forward choice. Changing the
default to no authentication most likely is the more user friendly
alternative. Apart from that and following intake#57, the default if no
authentication method is given should be anonymous access, but currently
this fall back does not work (see also intake#74).
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.

5 participants