Get token from cookie when using /login-with-expiry in Poppy #105
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lwrubel
commented
Jan 9, 2024
| @@ -151,7 +154,14 @@ def connection | |||
| url: config.url, | |||
| headers: DEFAULT_HEADERS.merge(config.okapi_headers || {}), | |||
| request: { timeout: config.timeout } | |||
| ) | |||
| ) do |faraday| | |||
| faraday.use :cookie_jar, jar: cookie_jar | |||
There was a problem hiding this comment.
To access cookies directly, need to pass in a CookieJar.
lwrubel
commented
Jan 9, 2024
| @@ -276,8 +286,9 @@ def with_token_refresh_when_unauthorized | |||
| response = yield | |||
|
|
|||
| # if unauthorized, token has likely expired. try to get a new token and then retry the same request(s). | |||
| if response.status == 401 | |||
| config.token = Authenticator.token(config.login_params, connection) | |||
| if response.status == 401 || response.status == 403 | |||
There was a problem hiding this comment.
FOLIO docs say that with Poppy release onward, if you send an expired token, it will return a 403 rather than a 401.
8a2c14d to
3abe420
Compare
mjgiarlo
reviewed
Jan 9, 2024
Comment on lines
+28
to
+29
| access_cookies = cookie_jar.cookies.select { |cookie| cookie.name == 'folioAccessToken' } | ||
| access_cookies[0].value |
There was a problem hiding this comment.
Since we only care about the first value, we could consider Enumerable#find here too:
Suggested change
| access_cookies = cookie_jar.cookies.select { |cookie| cookie.name == 'folioAccessToken' } | |
| access_cookies[0].value | |
| cookie_jar.cookies.find { |cookie| cookie.name == 'folioAccessToken' }.value |
mjgiarlo
reviewed
Jan 9, 2024
| if legacy_auth | ||
| JSON.parse(response.body)['okapiToken'] | ||
| else | ||
| access_cookies = cookie_jar.cookies.select { |cookie| cookie.name == 'folioAccessToken' } |
There was a problem hiding this comment.
Do you think we should handle the case where this line returns an empty array (in which case the #value call immediately below will fail), or is that an unlikely/corner case?
mjgiarlo
approved these changes
Jan 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why was this change made? 🤔
Resolves #92 to implement new process for getting an access token.
Currently, the token is in the body of the response from the
/authn/loginendpoint. From Poppy onwards the token will be obtained from a/authn/login-with-expiryendpoint and will be in a cookie. See the heading "A guide for non-module clients such as scripts or other integrations" on the Folio Wiki.How the token is sent to FOLIO in a request isn't changing. If an expired access token is sent in a request, FOLIO will return a 403 rather than a 401 as it does in Nolana/Orchid.
If we try to use the
login-with-expiryendpoint on Nolana, FOLIO returns a 404. The EBSCO FOLIO client sends everything to that endpoint and if it gets a 404, it then tries theloginendpoint. I thought we might want more control over which endpoint we're sending requests to, especially since the docs say that both endpoints are supported in Poppy and Quesnalia (legacy tokens not removed until Ramsons). This has the downside that we are changing theFolioClient.configuremethod signature and would want to eventually update that to remove thelegacy_authparam with that is no longer relevant (once we've been on Poppy in production for a bit?).Will follow up with:
How was this change tested? 🤨
Unit. Tested that the changes do not break current usage by using
api_test.rblocally and running thesdr_deposit_spec.rbintegration test anditem_creation_with_folio_hrid_spec.rbwhen using this folio_client version on DSA and Argo QA.I haven't been able to test this on a live Poppy instance, so that would be good to do as soon as a Poppy dev/test instance is available (Libsys estimates in mid-January).