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

Use new IIASA-manager API with token refresh #684

Merged

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Jul 29, 2022

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Name of contributors Added to AUTHORS.rst
  • Description in RELEASE_NOTES.md Added

Description of PR

This PR refactors the IIASA-manager-API authentication to the new (short-lived) token, including automated refresh. The PR also stricter behavior when reading a config file fails, and adds explicit test if arguments are missing from the credentials.

We decided to leave the current legacy-implementation for the anonymous login and application-list, because these features will change with the new ixmp package.

@meksor
Copy link

meksor commented Jul 29, 2022

The equivalent to the anonymous login would be logging in as the "guest" user.
Although I don't know if that user has a usable password set yet.

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #684 (2dc4f6f) into main (1fc2fee) will decrease coverage by 0.0%.
The diff coverage is 76.6%.

@@           Coverage Diff           @@
##            main    #684     +/-   ##
=======================================
- Coverage   94.7%   94.7%   -0.1%     
=======================================
  Files         59      59             
  Lines       5807    5836     +29     
=======================================
+ Hits        5504    5529     +25     
- Misses       303     307      +4     
Impacted Files Coverage Δ
pyam/iiasa.py 86.1% <72.3%> (-0.7%) ⬇️
tests/test_iiasa.py 97.1% <100.0%> (+0.2%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@danielhuppmann
Copy link
Member Author

The equivalent to the anonymous login would be logging in as the "guest" user.
Although I don't know if that user has a usable password set yet.

Ok, not sure if the "guest" user should have a password... Let's discuss next week.

@danielhuppmann danielhuppmann marked this pull request as ready for review August 2, 2022 13:19
Copy link

@meksor meksor left a comment

Choose a reason for hiding this comment

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

Lgtm from an integration standpoint, I made a few notes.

def obtain_jwt(self):
r = self.client.post("/v1/token/obtain/", json=self.creds)
if r.status_code == 401:
raise ValueError(
Copy link

Choose a reason for hiding this comment

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

Maybe use self.auth_url here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If a (non-expert) user reads the error message and tries to put the auth-url into his/her browser, this returns an unhelpful "Not Found" page - so I would rather use a url that users can "use".

pyam/iiasa.py Outdated
r = requests.get(url, headers=self._headers)
# TODO: application-list will be reimplemented in conjunction with ixmp-server
url = "/".join([self._auth_url, "legacy", "applications"])
r = requests.get(url, headers=self.auth())
Copy link

Choose a reason for hiding this comment

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

I would use httpx here as well. You can pass the auth object when instantiating the client and it will be called for every request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, implemented!

@danielhuppmann danielhuppmann merged commit d104630 into IAMconsortium:main Aug 3, 2022
@danielhuppmann danielhuppmann deleted the iiasa/manager-api-jwt branch August 3, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants