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

New auth strategy: token-only #484

Open
mfisher87 opened this issue Mar 5, 2024 · 14 comments · Fixed by #782
Open

New auth strategy: token-only #484

mfisher87 opened this issue Mar 5, 2024 · 14 comments · Fixed by #782
Labels
enhancement New feature or request
Milestone

Comments

@mfisher87
Copy link
Collaborator

mfisher87 commented Mar 5, 2024

Related: #415, #479

Should we provide a method of persisting tokens as well? Should bearer token become the default strategy?

From Hackday meeting 2024-10-15: Luis and Matt agree it should be the default strategy.

@mfisher87 mfisher87 added the enhancement New feature or request label Mar 5, 2024
@battistowx
Copy link
Collaborator

This would be awesome to have, instead of creating a requests query to the CMR API

@mfisher87
Copy link
Collaborator Author

@battistowx @asteiker I feel this feature should really be prioritized before 1.0. I think we need a good way to keep track of these things, so I created a milestone and added this issue to it. I'm not attached to this solution, just feel we need something and want to get the discussion going :) ETAFTP!

There was another issue I was also thinking should be completed before 1.0, and I forgot what it was. I'll find it at some point and add it to the milestone.

@mfisher87 mfisher87 added this to the Version 1.0 milestone Mar 5, 2024
@fwfichtner
Copy link
Contributor

First of all thanks for this library, it is very helpful for us! I have a question which is touching this issue. When we are doing earthaccess.search_data() we are login in via environment variables. So far so good, under https://urs.earthdata.nasa.gov/users/username/user_tokens we can see that two new tokens are created. I guess these are CMR Tokens? However, these tokens expire after 2 months and are not re-created and when we continue to use earthaccess we are getting RuntimeError('{"errors":["Token [Bearer eyJXXXI4SIQ] has expired. Note the token value has been partially redacted."]}'. This is solved by just deleting the Bearer Tokens, which triggers a re-creation.

@mfisher87
Copy link
Collaborator Author

Hm. When there are two tokens, the code is supposed to revoke and re-generate a token:

if len(self.tokens) == 2:
resp_revoked = self._revoke_user_token(self.token["access_token"])
if resp_revoked:
resp_tokens = self._generate_user_token(
username=self.username, password=self.password
)
if resp_tokens.ok:
self.token = resp_tokens.json()
self.tokens[0] = self.token
logger.debug(
f"earthaccess generated a token for CMR with expiration on: {self.token['expiration_date']}"
)
return True
else:
logger.info(resp_tokens)
return False

cc @betolink

@betolink
Copy link
Member

Token regeneration is not automatic, actually is not part of the top level API. It could be automatic if we encounter an expired token, as of now one can do this:

import earthaccess

auth = earthaccess.login()

auth.refresh_tokens()

@mfisher87
Copy link
Collaborator Author

Ah cool. Looks like we just need a bit of glue code to automate it :) Thanks, @betolink !

@fwfichtner
Copy link
Contributor

I had a look to support with a PR, but must admit that I struggle to understand what is going on.

If I delete the tokens manually they are re-generated, no problem, however and not changing anything I sometimes end up having three working tokens (even though earthaccess and the https://urs.earthdata.nasa.gov/users/username/user_tokens expect only two (I can see 3 tokens in the UI). Maybe that happens through some kind of race condition.

Screenshot

@battistowx
Copy link
Collaborator

Would it be better to use find_or_create_token? We've used that here with no issues: https://github.com/nasa/gesdisc-tutorials/blob/main/notebooks/How_To_Download_a_Spatial_and_Variable_Subset_of_Level_1B_Data_Using_OPeNDAP.ipynb

@chuckwondo
Copy link
Collaborator

Would it be better to use find_or_create_token? We've used that here with no issues: https://github.com/nasa/gesdisc-tutorials/blob/main/notebooks/How_To_Download_a_Spatial_and_Variable_Subset_of_Level_1B_Data_Using_OPeNDAP.ipynb

Yes, indeed, you beat me to the punch.

And in case anybody is wondering, that endpoint is implemented to automatically deal with expired tokens, such that it always returns an active token (automatically generating one and returning it, if you only have expired tokens at the time of the request).

@fwfichtner
Copy link
Contributor

Finally found time to check and that would work. Is it okay for you if I create a PR to change the handling of the token?

_get_user_tokens() could just use the endpoint you are suggesting. Is there any reason to work with a list of tokens, if not we can also remove the interaction with the other endpoints if find_or_creat_token only returns one valid token and also handles re-creation of expired tokens.

@chuckwondo
Copy link
Collaborator

There should be no need to deal with a list of tokens. Our current token logic should be greatly simplified by making use of find_or_create_token. If you want to take a stab at simplifying the logic, go for it. I'll be happy to review your PR.

@chuckwondo
Copy link
Collaborator

I don't believe that #782 directly addresses this particular issue, so I'm reopening this.

@chuckwondo chuckwondo reopened this Aug 7, 2024
@mfisher87
Copy link
Collaborator Author

Should we call this "token-only" strategy for clarity?

@chuckwondo
Copy link
Collaborator

Should we call this "token-only" strategy for clarity?

Works for me.

@mfisher87 mfisher87 changed the title New auth strategy: bearer token New auth strategy: token-only Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants