-
Notifications
You must be signed in to change notification settings - Fork 32
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
Added automatic creation of .netrc if none exists to address issue #93 #173
base: develop
Are you sure you want to change the base?
Conversation
…create a .netrc file with Earthdata login
…mod command on newly created .netrc file
…le to address issue podaac#152
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Anthony! Thanks for helping contribute to data-subscriber!
I had a few comments about what I think this PR needs before we can merge it in. Let me know if you have any questions/thoughts
subscriber/podaac_access.py
Outdated
with open(file_path, "w") as file: | ||
file.write(netrc_content) | ||
|
||
# running this on windows will cause the program to crash and is not necessary | ||
if platform.system() != 'Windows': | ||
# change .netrc permissions | ||
subprocess.run(["chmod", "og-rw", file_path]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using os.open
instead of running chmod
to set permissions at the same time the file is created, so we can avoid race condition vulnerabilities when the file is being written to/before the permissions are applied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to use os.open() like so:
# Create and open the .netrc file with the correct permissions
fd = os.open(netrc_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, netrc_permissions)
with os.fdopen(fd, 'w') as file:
file.write(netrc_content)
subscriber/podaac_access.py
Outdated
def create_netrc_file(response): | ||
""" | ||
Ask the user if they have created an Earthdata login | ||
if so: | ||
Prompt the user for their username and password | ||
Use the credentials to create a .netrc file in the users home directory | ||
if not: | ||
Prompt the user to create an Earthdata login at the appropriate url | ||
""" | ||
if response.lower() == 'y': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we need response
here. I think this should be handled in the podaac_data_downloader
and podaac_data_subscriber
modules which deal with the CLI directly. Functions in here should be strictly API-friendly
subscriber/podaac_access.py
Outdated
# FileNotFound = There's no .netrc file | ||
except FileNotFoundError: | ||
logging.warning("No .netrc file can be found. One will be attempted to be created.") | ||
create_netrc_file(input('Do you have an Earthdata login? (y/n): ')) | ||
username, _, password = netrc.netrc().authenticators(endpoint) | ||
# TypeError = The endpoint isn't in the netrc file, | ||
# causing the below to try unpacking None | ||
except TypeError: | ||
logging.warning("The endpoint isn't in the netrc file or is incorrect") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to avoid having this CLI code here because we want to keep these functions API friendly. Also, if the user doesn't create a netrc file on that call on line 115, wouldn't the retry of the netrc parse fail with a FileNotFound again but be uncatched?
I think we want to instead remove the try/except and allow it to boil up to the CLI layer. Maybe add a flag to be able to ignore a missing netrc as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved all the code from podaac_access to podaac_data_downloader and podaac_data_subscriber. I reverted the podaac_access.py file to what it was before I changed it.
The CLI modules will check if there is no .netrc file. If they do not have an Earthdata login a link to signup for an account will be logged to the console and the CLI tool will quit before a FileNotFound error occurs. Let me know if there is a better or different way you want me to handle this.
if not netrc_file_exists():
if input('Do you have an Earthdata login? (y/n): ').lower() == 'y':
create_netrc_file()
else:
logging.info('Go to https://urs.earthdata.nasa.gov/users/new to create an Earthdata login')
exit()
Since the CLI layer now checks for a .netrc file and handles it if there is none can we remove this try except from podaac_access as well? This is the original code for error handling in podaac_access.py before I modified it.
try:
username, _, password = netrc.netrc().authenticators(endpoint)
except (FileNotFoundError, TypeError):
# FileNotFound = There's no .netrc file
# TypeError = The endpoint isn't in the netrc file,
# causing the above to try unpacking None
logging.warning("There's no .netrc file or the The endpoint isn't in the netrc file")
I am happy to create a flag to ignore a missing .netrc, but want to make sure I get all of this right before doing so.
If no .netrc file is detected the program will ask if the user has an Earthdata login. If not it will give them the link to create one. If so It will prompt for credentials and automatically create a .netrc file in the home directory.
I changed the readme to say that if not .netrc file exists then one will be automatically created if the user has Earthdata login credentials.
I also added a note in the readme that the permissions for the .netrc file should be changed for mac and linux users to address issue #152