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 us-west-2 check to API and improve auth repr #424

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Conversation

JessicaS11
Copy link
Collaborator

@JessicaS11 JessicaS11 commented Jan 8, 2024

Per #231, we've added API access to store._running_in_us_west_2() in the form of a printed statement that the user is (or is not) running in AWS region us-west-2. In the process of implementing this, it became clear that get_edl_token() had not been added to __init__.py, so that was added as well. It also adds a repr for the auth object via a auth.auth_info() property.

I'm seeking input on:

  • if this PR fully satisfies [proposed enhancement] AWS us-west-2 checking method #231 - was the spirit to check specifically for us-west-2, or to enable the user to see what region they are running in? Pending that answer, it might make sense to have a more generic earthaccess.region() function instead.
  • What should be in auth.__repr__? Currently it's not much better than the object ID:
Authentication Info
-------------------
authenticated?: True
tokens: [{'access_token': 'eyJ0eXAiOiJKV1QiLCJvcmlnaW4iOiJFYXJ0aGRhdGEgTG9naW4iLCJzaWciOiJlZGxqd3RwdWJrZXlfb3BzIiwiY[truncated]...', 'token_type': 'Bearer', 'expiration_date': '02/09/2024'}]
  • what tests should we add (if any) for new functions? As best as I can tell store._running_in_us_west_2() isn't tested. Should auth.auth_info() have a dictionary typing/keyword matching test?

@battistowx
Copy link
Collaborator

We can probably change the print statement to raise a ValueError with the same message. That way, it is a formal error message that will interrupt the library from opening the bucket out-of-region with a long, cryptic error message.

@JessicaS11
Copy link
Collaborator Author

Thanks to @battistowx during today's earthaccess cowork session for moving this forward with me!

@andypbarrett @betolink Any thoughts on what info we want to be in the auth repr? I'm happy to move that to a separate PR if we want to move forward with the s3check portion of this one.

@JessicaS11 JessicaS11 linked an issue Feb 6, 2024 that may be closed by this pull request
"login",
"search_datasets",
"search_data",
"get_requests_https_session",
"get_fsspec_https_session",
"get_s3fs_session",
"get_s3_credentials",
"granule_query",
"get_edl_token" "granule_query",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Warning: these strings will concatenate :)

Comment on lines +79 to +84
def __repr__(self) -> str:
print_str = "Authentication Info\n" + "-------------------\n"
for k, v in self.auth_info.items():
print_str += str("{}: {}\n".format(k, v))

return print_str
Copy link
Collaborator

Choose a reason for hiding this comment

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

I strongly discourage this implementation of __repr__ for at least 2 reasons:

  1. (minor) this is arguably an "abuse" of __repr__, which should generally be: "If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment)." (see https://docs.python.org/3/reference/datamodel.html#object.__repr__)
  2. (very important) this can lead to inadvertent leaking of the user's token(s) during logging since auth_info includes those tokens

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.

[proposed enhancement] AWS us-west-2 checking method
4 participants