Add support for authentication with Earthdata login token only#1020
Add support for authentication with Earthdata login token only#1020chuckwondo merged 9 commits intonsidc:mainfrom
Conversation
|
I will automatically update this comment whenever this PR is modified
|
|
This is a great contribution @kgrimes2! we were jut working on something similar today, I think we should combine those but your PR probably needs to go first (FIFO 😄 ). Integrations tests will fail because we have no easy way of sharing the default username/password with PRs from forks. EDIT: I'll open a PR with the work we did today(draft) so you can take what's useful with the idea of eventually merging this one. #1021 |
|
@betolink That's awesome! @mfisher87 re-ran one of the integration tests because I ran into the permissions issue you mentioned. When he ran it it failed checking the number of files that got downloaded in one of the tests: https://github.com/nsidc/earthaccess/actions/runs/15286174529/job/42998438063?pr=1020#step:8:442 Looking at the test, I think it may be benign, and just a case where the number of files to download is genuinely 1. The test expects at least 2 files to be downloaded: but the method that gets a random sample even has a comment that the returned amount might be less than the sample size: earthaccess/tests/integration/sample.py Lines 37 to 38 in 310dc44 Retrying the integration test may resolve. I'll defer to whoever has permission to merge these PRs on what to do :) Happy to consolidate my work with yours, or vice versa. |
|
I think we should consolidate the changes from the other PR #1021 into this one, the main thing would be to actually use the token to create sessions, the token cannot be used to retrieve the user's profile or their credentials, this means when we use this auth method we'll assume the token works and the flag of |
|
Might there be another way to do a kind of "ping" and check that the token works before setting |
|
@danielfromearth Looking at the API docs for the EDL API, unfortunately they all seem to require a username and password. I tried using only the bearer token without luck. We could always make a test query to CMR or something to exercise the token? Like a search request for 0 documents |
Queries don't require credentials, so that won't help. A token is for obtaining S3 credentials via an S3 credentials endpoint, so we would need to choose an arbitrary endpoint and attempt a GET request with a bearer token auth header, without allowing redirect. If the response is a redirect, then the token is invalid. However, I don't think we should complicate things. As soon as a download is attempted, the user will find out whether or not their token is valid. If not, they should get a 401 response, which we could catch and perhaps rethrow with a more helpful error message for the user, such as, f"Your Earthdata Login user token for {host} is invalid. It may have expired or be for a different host.", where |
|
I think we should try to get this merged today and then merge #1021. I'm inclined to only update the docs saying that earthaccess assumes the token is valid. cc @chuckwondo @danielfromearth |
betolink
left a comment
There was a problem hiding this comment.
Good PR! it paves the way to using the token in the fsspec sessions! we just need to enhance the docs a little bit to let the users know that we are not validating the token beforehand.
There was a problem hiding this comment.
I think we just need to expand that we assume the token is valid and earthaccess will not try to validate it (since we know the token API doesn't accept tokens). Once we merge the next PR we should be good.
There was a problem hiding this comment.
I have token as a stand-alone auth mechanism, this makes more sense. Like you said we could enhance the "interactive" way yo input a token first and if not provided, will do the username/password as usual.
chuckwondo
left a comment
There was a problem hiding this comment.
@kgrimes2, thank you. Just a couple of suggestions for adjusting docs.
Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>
|
Looks like NSIDC on-prem datasets are no longer available since they were migrated to the cloud, (integration tests failing) I'll edit the PR to fix it. |
chuckwondo
left a comment
There was a problem hiding this comment.
Thank you @kgrimes2!
@betolink, I opened a new issue for the on-prem collections to be removed from our tests: #1036. I think the other 2 failures might be transient connections errors. |
@betolink, I reran the int. tests, and now there are only 2 failures, not 4. As I suspected, 2 were transient errors, and the 2 remaining are part of the list I created for #1036. I suggest we address #1036 as a separate PR rather than attempting to address the failing int. tests in this PR. |
|
Awesome! Thanks all! |
This change addresses the use case of authenticating with an existing Earthdata Login token (#484). Users may set the
EARTHDATA_TOKENenvironment variable before callingauth.login(). If a token is provided, only it is used; that is, any provided username and password are ignored.Note that this method does not support the "interactive" login strategy, yet (but that can be added if there's interest).
Pull Request (PR) draft checklist - click to expand
contributing documentation
before getting started.
title such as "Add testing details to the contributor section of the README".
Example PRs: #763
example
closes #1. SeeGitHub docs - Linking a pull request to an issue.
CHANGELOG.mdwith details about your change in a section titled## Unreleased. If such a section does not exist, please create one. FollowCommon Changelog for your additions.
Example PRs: #763
README.mdwith details of changes to theearthaccess interface, if any. Consider new environment variables, function names,
decorators, etc.
Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!
Pull Request (PR) merge checklist - click to expand
Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the
@nsidc/earthaccess-supportteam in a comment and wewill help you out!
Request containing "pre-commit.ci autofix" to automate this.
📚 Documentation preview 📚: https://earthaccess--1020.org.readthedocs.build/en/1020/