Skip to content

Conversation

@malgosias
Copy link
Contributor

@malgosias malgosias commented May 11, 2022

Updated occweb authorization when fetching glimmmondb.sqlite3. The old version was using credentials of an account that has been discontinued.

In addition, corrected a bug in various paths used for copying and moving the output files.

#
cmd = 'curl -u tisobe:' + out
cmd = cmd + ' https://occweb.cfa.harvard.edu/occweb/FOT/engineering/thermal/'
cmd = f'curl -u {user}:{password} https://occweb.cfa.harvard.edu/occweb/FOT/engineering/thermal/'
Copy link
Member

Choose a reason for hiding this comment

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

This is probably not secure since it exposes the password in your processes while curl is running. Use kadi.occweb.get_occweb_page instead with binary=True.

Copy link

Choose a reason for hiding this comment

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

it seems like Tom had a low impact change. Do you want ot use that, or would you like to gow with this for now and change it later?

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 implemented the change suggested by Tom, see below.

@malgosias malgosias requested a review from swolk May 23, 2022 18:36
@malgosias
Copy link
Contributor Author

The change suggested by Tom has been implemented.

#
if len(data) < 1:
cmd = 'rm ' + temp_opfile
cmd = f'rm {temp_opfile}'
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge deal, but all these os.system commands can be replaced by Python built-in functions in https://docs.python.org/3.8/library/shutil.html and https://docs.python.org/3.8/library/os.html. There are move() and copy2() in shutil and remove in os.

The main problem here is that there is no real error handling in the current os.system(cmd) call. If that fails then a non-zero value is returned but that is being ignored in this code. At the least all the os.system(cmd) calls should be replaced by assert os.system(cmd) == 0 so an exception is raised for a problem.

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.

4 participants