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

Issues performing .get() to CTA_FILE_URL from behind internal firewall #303

Open
3 tasks
aleks-sch opened this issue Aug 8, 2023 · 6 comments
Open
3 tasks

Comments

@aleks-sch
Copy link
Contributor

Our model currently uses v1.0.5 and we are struggling to update to v1.0.6+ because the requests.get() introduced in 2c9ca5d to get the CTA_FILE_URL from behind internal firewall fails. We can go through a semi-lengthy exemption process but below changes will make it simpler and generally safer.

this issue to discuss some possible changes

  • simpler/safer to return a JSON response from the API and not full Excel file (this eliminates risk of 'bad' macros in XLSX file)?
  • a fallback in cases where requests.get fails supported?
  • support ability to pass in verify= param to the request.get?
@mountainrambler
Copy link
Contributor

Hi and thanks for your input. I will look into the possbility of using JSON - if the SBTi has this possbility. A fallback will be introduced so that we check for a status code of 200, and if this fails we'll fetch the file included in the package instead. It might not be the very latest but I think it will be possible to manually load the file to that directory if the user can access it in some other way than through the tool.
As for the verify parameter, I believe it is set to True by default.

@aleks-sch
Copy link
Contributor Author

aleks-sch commented Aug 8, 2023

edited - hit send too soon!

thanks

JSON response would definitely be simpler for us to perform the request.get but i realise you would need to make a change to how the file is hosted and served!

shall I knock up a PR to add the targets.xlsx file to the GH repo and read from local if the request.get fails? I would just put the current file into https://github.com/ScienceBasedTargets/SBTi-finance-tool/tree/main/SBTi/data?

manually load the file to that directory if the user can access it in some other way than through the tool

suggestions for where could get user to load the file to?

as for verify param: our company decrypts our HTTPS traffic and would need verify to point to our trusted root cert. And I have realised that I may be able to just use the REQUESTS_CA_BUNDLE env var to point to this certificate >>
from https://requests.readthedocs.io/en/latest/user/advanced/#ssl-cert-verification
image

@mountainrambler
Copy link
Contributor

mountainrambler commented Aug 8, 2023

Well here's some good news. There will soon be a new api with the CTA delivered in JSON format. I don't have a date yet, but the code to update the tool is being prepared.
The change will include a test for status == 200 and it it fails we'll read the file target-dashbard.xlsx from the directory STBi/inputs.
And as for verify, is it enough for you if we add verify=True to the .get()? That's no problem from my side.

@aleks-sch
Copy link
Contributor Author

thats great thanks - shall we leave this issue open until the API and check if status == 200 PR has been opened so can link the PR and issue?

And as for verify, is it enough for you if we add verify=True to the .get()? That's no problem from my side.

We should be able to just use the REQUESTS_CA_BUNDLE env var in our model execution environment thanks, I should have found that earlier!

@mountainrambler
Copy link
Contributor

Yes, let's leave it open until the API is ready. But I will add the status == 200 and perhaps leave an option to read the CTA file from the directory "data" that is created when running the tool in Colab. (If you run it in your own Jupyter environment you should be able to see the CTA file anyway.)

@aleks-sch
Copy link
Contributor Author

thanks for changes in #306 and 8426dc8 to add local CTA file backup.

unfort we are still facing exceptions so wrapped the requests.get in try/except in #308.

your review plz?

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

No branches or pull requests

2 participants