-
Notifications
You must be signed in to change notification settings - Fork 26
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
STCOR-671 handle access-control via cookies #1364
Merged
Merged
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
Handle access-control via HTTP-only cookies instead of storing the JWT in local storage and providing it in the `X-Okapi-Token` header of fetch requests, and proxy all requests through a service worker that performs Refresh Token Rotation as needed to make sure the access-token remains fresh. Notable changes: * Fetch requests are proxied by a Service Worker that intercepts them, validates that the Access Token is still valid, and performs token rotation (if it is not) before completing the original fetch. * Sessions automatically end (i.e. the user is automatically logged out) when the Refresh Token expires. * Access control is managed by including an HTTP-only cookie with all requests. This means the Access Token formerly available in the response-header as `X-Okapi-Token` is never accessible to JS code. * Requires folio-org/stripes-connect/pull/223 * Requires folio-org/stripes-smart-components/pull/1397 * Requires folio-org/stripes-webpack/pull/125 Replaces #1340. It was gross and I really don't want to talk about it. Let us never mention it again. Refs STCOR-671, FOLIO-3627 (cherry picked from commit 27d2948)
Rotate tokens well before they expire. This solves a problem in ui-data-import where every-five-second polling has caused some requests to land in a gap of about three seconds between when the AT was actually minted and when we stored it on the client side, which could cause the UI to send an AT that mod-auth thinks is expired even though we thought it was still valid. i.e. it makes it much less likely that a token will expire in flight. Refs STCOR-574 (cherry picked from commit dd71819)
The most important work here is fixing the bug from #1361 that incorrectly evaluated whether a token was still valid. The original implementation shrank the total lifespan of the token rather than shrinking only the period of its lifespan in the future. Additional improvements here include: * evaluate `navigator.serviceWorker` more cautiously; some browsers (e.g. Firefox in Incognito) may deny access to service workers. See STCOR-757 for additional details. * use `{ source, type, value }` shaped messages consistently when exchanging messages with the service worker. * shrink the tokens' validity windows when receiving the `TOKEN_EXPIRATION` message instead of calculating the shorter window each time the token is evaluated. This is more efficient. * await the promise returned by `postTokenExpiration` from `navigator.serviceWorker.ready` in order to prevent requests from being sent when the service worker isn't ready for them. * use the `new Response()` constructor correctly, i.e. stringify the JSON value; otherwise clients will receive the string `[object Object]` instead of the empty JSON object, `{}`. Note: the login test is turned off here due to the fact that the login flow invokes `navigator.serviceWorker.ready`, which returns a Promise that only returns when the service worker enters a ready state, but the karma build does not configure the service worker, hence this test times out every time. This is not great, but resolving it is a non-trivial task. Refs STCOR-756 (cherry picked from commit 607dc5c)
SonarCloud Quality Gate failed. 0 Bugs 73.1% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
zburke
added a commit
that referenced
this pull request
Nov 9, 2023
This reverts commit 93b35bc. Revert RTR until it's fully baked.
zburke
added a commit
that referenced
this pull request
Nov 9, 2023
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.
Handle access-control via HTTP-only cookies instead of storing the JWT in local storage and providing it in the
X-Okapi-Token
header of fetch requests, and proxy all requests through a service worker that performs Refresh Token Rotation as needed to make sure the access-token remains fresh.Notable changes:
Fetch requests are proxied by a Service Worker that intercepts them, validates that the Access Token is still valid, and performs token rotation (if it is not) before completing the original fetch.
Sessions automatically end (i.e. the user is automatically logged out) when the Refresh Token expires.
Access control is managed by including an HTTP-only cookie with all requests. This means the Access Token formerly available in the response-header as
X-Okapi-Token
is never accessible to JS code.Requires STCON-138 handle access-control via cookies stripes-connect#223
Requires STSMACOM-714 handle access-control via cookies stripes-smart-components#1397
Requires STRWEB-99 bundle stripes-core's service-worker.js stripes-webpack#125
Note that this work DOES NOT address STCOR-758 (pages loaded with shift-reload have their ATs timeout after 10 minutes, causing one or more error alerts followed by auto-logout) or STCOR-759 (AT may timeout >= 10 minutes causing one or more error alerts followed by auto-logout).
Refs STCOR-671, STCOR-574, STCOR-756, FOLIO-3627