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

Updated FolioClient to use dynamic properties for api token values #28

Conversation

bltravis
Copy link
Contributor

@bltravis bltravis commented Sep 29, 2023

Refactored login method to support login-with-expiry endpoint and additional RTR data. Implemented okapi_token and okapi_headers as dynamic properties that should always return a valid api token. Fixes Implement support for refresh tokens for FOLIO API keys (Poppy) #27

  • Update login method to support /authn/login-with-expiry in addition to /authn/login
  • Ensure that okapi_token and okapi_headers return a "fresh" api token when accessed
  • Confirm backwards-compatibility with Orchid and earlier

Copy link

@todolson todolson left a comment

Choose a reason for hiding this comment

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

I like the general approach. I've left three comments in the code.

folioclient/FolioClient.py Outdated Show resolved Hide resolved
folioclient/FolioClient.py Outdated Show resolved Hide resolved
folioclient/FolioClient.py Show resolved Hide resolved
…itional RTR data. Implemented okapi_token and okapi_headers as dynamic properties that should always return a valid api token. Fixes Implement support for refresh tokens for FOLIO API keys (Poppy) #27
@bltravis bltravis force-pushed the 27-implement-support-for-refresh-tokens-for-folio-api-keys-poppy branch from 3a37bf7 to 7495308 Compare October 18, 2023 21:52
@bltravis bltravis force-pushed the 27-implement-support-for-refresh-tokens-for-folio-api-keys-poppy branch from e714502 to 312c820 Compare November 1, 2023 02:03
@bltravis
Copy link
Contributor Author

bltravis commented Nov 1, 2023

@todolson Can you give this another quick glance. I've pulled out the refresh token approach in favor of just logging in again (per recommendation here: https://wiki.folio.org/x/Lyq-BQ#RefreshTokenRotation(RTR)-Aguidefornon-moduleclientssuchasscriptsorotherintegrations). I've also implemented the okapi_headers attribute as a dynamic property that allows modification of all headers except x-okapi-token. This was done to support using the client in an enhanced consortial support (ECS) context, where the same client (and token) may be used to address multiple data tenants.

@todolson
Copy link

todolson commented Nov 1, 2023

@bltravis , I've not used properties in earnest, and I'm a bit rusty in any case, so it took me a bit to get this. Have also confirmed in the REPL that this works. But it has the desired effect and I don't have any suggestions.
Also, I note that one can set the okapi_token property, which will update the x-okapi-token header. I believe is as intended.
No other comments.

Copy link

sonarcloud bot commented Nov 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bltravis
Copy link
Contributor Author

bltravis commented Nov 1, 2023

@bltravis … Also, I note that one can set the okapi_token property, which will update the x-okapi-token header. I believe is as intended. No other comments.

@todolson, Hmmm… 🤔 You're right. That really shouldn't be settable via the property. I've removed the setter and the deleter. self.okapi_token is now read-only, and the token is being set in self.login via self._okapi_token

@bltravis bltravis mentioned this pull request Nov 2, 2023
@bltravis bltravis merged commit afd686d into master Nov 7, 2023
5 checks passed
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.

Implement support for refresh tokens for FOLIO API keys (Poppy)
2 participants