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

Support --access-token parameter for CDSE #62

Conversation

LucHermitte
Copy link
Contributor

This new option is meant to simplify 2FA on CDSE.

The issue

2FA token is extremely short lived -- it's refreshed every minute or so. This means that if we want to call eof manually several times, or to call it from a batched process, we'll need to update the 2FA token every time before calling eof.

The proposed solution

If instead we manually acquire an access token, it's lifetime will be much longer (I've haven't exactly measured how much).

This PR proposes a new option --access-token where we could feed the access token instead of login/password[/2fa-token].

Or course the access token needs to be acquired manually thanks to the procedures described on https://documentation.dataspace.copernicus.eu/APIs/Token.html

@@ -224,6 +227,7 @@ def _construct_orbit_file_query(
query_template = (
"startswith(Name,'{mission_id}') and contains(Name,'{orbit_type}') "
"and ContentDate/Start lt '{start_time}' and ContentDate/End gt '{stop_time}'"
# " and productType eq {orbit_type}"
Copy link
Owner

Choose a reason for hiding this comment

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

Is this redundant because the current query template has contains(Name,'{orbit_type}')? or was this a mistake to comment it out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, this was something I wanted to test but I didn't find/take the time to do it. So far you were using contains(Name,'{orbit_type}') and while reading the doc, I wondered whether productType eq {orbit_type} would have been enough.

I shouldn't have commited it, but forgot about it.

Copy link
Owner

@scottstanie scottstanie left a comment

Choose a reason for hiding this comment

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

thanks for the improvements! only real question is about the query template change

except FileNotFoundError:
logger.warning("No netrc file found.")
return None
assert username and password, "Username and password values are expected!"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
assert username and password, "Username and password values are expected!"
if not (username and password):
raise ValueError("Username and password values are expected!")

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've considered username and password to be set as part of the function contract: if this precondition is not valid, then there is a programming error, and it's up to us to prevent this situation from happening.

Given username and password being set are a postcondition of get_netrc_credentials() (the function cannot return none values), which is called just before, everything is fine so far. Perfect DbC in action -- well... almost if we ignore I forgot to document the contracts in the docstring of each function, and that I should have tested for them to not be empty as well. ^^'

I usually dislike wide contracts that check every single things that are not meant to happen and throw logic errors if they do. That has a tendency to complexify source code: we add a lot of dead code that can/should never execute.

Here is the fixed code and docstring. If you really prefer wide contract instead of narrow ones. I'll throw then -- an AssertionError would still make more sense IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake. The 3 scenarios weren't correctly handled. This should be better now.

Luc Hermitte added 3 commits August 20, 2024 03:40
@scottstanie
Copy link
Owner

thanks for the patience @LucHermitte , looks good!

@scottstanie scottstanie merged commit b8622e8 into scottstanie:master Sep 4, 2024
7 checks passed
@LucHermitte
Copy link
Contributor Author

Thank you!

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.

2 participants