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

STCOR-671 handle access-control via cookies #1346

Merged
merged 36 commits into from
Oct 31, 2023
Merged

STCOR-671 handle access-control via cookies #1346

merged 36 commits into from
Oct 31, 2023

Conversation

zburke
Copy link
Member

@zburke zburke commented Oct 4, 2023

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.

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

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. The `login-with-expiry` endpoint returns an access-token and
refresh-token in HTTP-only cookies, along with information about when
those cookies expire in the response body. Stripes-core sets up a
service worker to track the AT's expiration timestamp and transparently
request a replacement by intercepting the fetch request, replacing (i.e.
rotating) both the AT and the RT before passing along the original
request.

Notable changes:

* Sessions now timeout after a period of inactivity, determined by the
  lifespan of the RT, instead of remaining valid indefinitely.
* Authentication requests are sent to `/bl-users/login-with-expiry`
  instead of `/bl-users/login`.
* "Activity" is tracked by a document-level event handler that listens
  for mouse-down and key-down events.

Refs STCOR-671, FOLIO-3627
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Jest Unit Test Statistics

125 tests  +48   125 ✔️ +48   17s ⏱️ +4s
  15 suites +  2       0 💤 ±  0 
    1 files   ±  0       0 ±  0 

Results for commit a17f72f. ± Comparison against base commit bbc722d.

This pull request removes 1 and adds 49 tests. Note that renamed tests count towards both.
createOkapiSession clears authentication errors ‑ createOkapiSession clears authentication errors
createOkapiSession clears authentication errors and sends a TOKEN_EXPIRATION message ‑ createOkapiSession clears authentication errors and sends a TOKEN_EXPIRATION message
handleServiceWorkerMessage handles same-origin events on RTR error, ends session ‑ handleServiceWorkerMessage handles same-origin events on RTR error, ends session
handleServiceWorkerMessage handles same-origin events on RTR, dispatches new token-expiration data ‑ handleServiceWorkerMessage handles same-origin events on RTR, dispatches new token-expiration data
handleServiceWorkerMessage handles same-origin events only handles events if data.source is "@folio/stripes-core" ‑ handleServiceWorkerMessage handles same-origin events only handles events if data.source is "@folio/stripes-core"
handleServiceWorkerMessage ignores cross-origin events mismatched event origin ‑ handleServiceWorkerMessage ignores cross-origin events mismatched event origin
handleServiceWorkerMessage ignores cross-origin events missing event origin ‑ handleServiceWorkerMessage ignores cross-origin events missing event origin
isLogoutRequest accepts logout endpoints /authn/logout ‑ isLogoutRequest accepts logout endpoints /authn/logout
isLogoutRequest rejects unknown endpoints ‑ isLogoutRequest rejects unknown endpoints
isOkapiRequest accepts requests whose origin matches okapi's ‑ isOkapiRequest accepts requests whose origin matches okapi's
isOkapiRequest rejects requests whose origin does not match okapi's ‑ isOkapiRequest rejects requests whose origin does not match okapi's
…

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

BigTest Unit Test Statistics

    1 files  ±0      1 suites  ±0   10s ⏱️ ±0s
271 tests ±0  265 ✔️  - 1  6 💤 +1  0 ±0 
274 runs  ±0  268 ✔️  - 1  6 💤 +1  0 ±0 

Results for commit a17f72f. ± Comparison against base commit bbc722d.

This pull request removes 5 and adds 3 tests. Note that renamed tests count towards both.
      equal to check email label in english translation
      equal to check email precautions label in english translation
      equal to sent email precautions label in english translation
Chrome_118_0_0_0_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the header with an appropriate text content
Chrome_118_0_0_0_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content
Chrome_118_0_0_0_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the header with an appropriate text content
      equal to check email label in english translation
Chrome_118_0_0_0_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content
      equal to check email precautions label in english translation
Chrome_118_0_0_0_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content
      equal to sent email precautions label in english translation
This pull request skips 1 test.
Chrome_118_0_0_0_(Linux_x86_64).Session timeout test ‑ Session timeout test clicking settings should redirect to login with session timeout message

♻️ This comment has been updated with latest results.

zburke added 13 commits October 4, 2023 15:06
don't bother setting up idle-timers; just intercept fetch requests and
pass messages from the service-worker when tokens are rotated or when
rotation fails.

move registration into App to gain access to the redux store, which may
be helpful/necessary when conducting auto-logout actions which need to
clear that store.
* when RTR is in-process, wait until it's finished before carrying on
  with a pending fetch request
* better logout handling since logout should always succeed
without an AT, there is no way to derive the tenant so it must be
pulled from the request and explicitly passed in the rtr request via the
`x-okapi-tenant` header.
* when rtr fails, post to clients and return an empty response. given how
  many unchecked fetch calls we have, this felt like a good options
  because a true error response, i.e. Promise.reject(), would trigger
  the default unchecked handler, which is a JS alert, which effectively
  takes precedence over whatever handling clients want to use to respond
  to the failure message.
* Also comments. comments are nice.
* Pure functions are nice
* Tested functions are even nicer
@zburke zburke marked this pull request as ready for review October 10, 2023 14:05
@NikitaSedyx
Copy link
Contributor

@zburke should we have so much logs here? - if there are removed in production bundle, it's fine to have them

@zburke zburke requested review from steveellis and a team October 10, 2023 14:49
@zburke
Copy link
Member Author

zburke commented Oct 10, 2023

@zburke should we have so much logs here? - if there are removed in production bundle, it's fine to have them

No, logs should be purged. I forgot to remove them when I moved this from "draft" to "ready". I wanted to use the stripes logger in service-worker.js, passed in from App.js through to registerServiceWorker and into the service-worker itself, but it didn't work. I could call logger.log() in the register function and it would work, but inside the service-worker none of the logger's functions were findable on the object. It was so weird.

export const rtr = async (event) => {
// console.log('-- (rtr-sw) ** RTR ...');

// if several fetches trigger rtr in a short window, all but the first will
Copy link

@steveellis steveellis Oct 10, 2023

Choose a reason for hiding this comment

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

I'm not completely understanding what problem this would solve. Why would "several fetches trigger rtr in a short window"? Is it that lots of requests can arrive in the filter at once since the browser of course processes 8 or 10 at once in separate threads normally. If this is firing every time that happens, things might not work very well...

The reason I ask is because of the single use feature of the RT. You can only POST a single RT once. If you post it twice (say if two requests send at roughly the same time or in close proximity), mod-at will consider this a "leaked token" and log the user out of all of their devices.

I think this means that the RT should only be sent when it needs to be sent or just lazily in the background, which is at some interval that is less than the AT TTL. Occasionally it will result in a 401 because the RT expired.

Choose a reason for hiding this comment

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

I think what we want is for the service worker to not be too ambitious. He's lazy after all. But his laziness is a virtue. If he does his work well, in his lazy way, he doesn't need to be ambitious. Ambitious would mean that he's trying to solve every problem that comes his way just as soon as these problems arrive. But he's not like that. His reliability is how he makes up for his lack of ambition. He takes care of everything eventually. And because of that he doesn't need to be ambitious.

But why does he care about every request that comes his way then?

Only because just in case, something unforeseen has caused a problem that is outside of his control, he can send the user to the login screen, which is after all, much better than the alternative which would never to be sent anywhere at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Simultaneous fetches are common, e.g. the ui-users landing page fires several requests to fill in the values on its search form: patron groups, departments, tags, custom fields. The browser handles these in parallel streams, but as you pointed out, the RT is single use, so we need to guard against the situation where an AT has expired and then several requests arrive all at once. We need to let the first one through and force the others to wait.

As to laziness, this code goes through great pains to be lazy, i.e. to implement just-in-time RTR, passing along requests as-is when it believes the AT is valid. But that's as lazy as we can be: the nature of a fetch event listener is that it hears about every fetch, so we need to handle all the surrounding edge cases: the expiration data in storage said the AT (or RT) was good but it turned out to be bad, here are five simultaneous fetches after a period of inactivity during which the AT expired, etc.

A timer-based approach is simpler, and doesn't need to involve service workers at all, but it limits sessions to the AT's expiration instead of the RT's. If we want sessions that last as long as the RT does, we have to implement JIT renewal as above, and it gets complicated pretty quickly.

src/service-worker.js Outdated Show resolved Hide resolved
src/service-worker.js Outdated Show resolved Hide resolved
Copy link
Contributor

@BogdanDenis BogdanDenis left a comment

Choose a reason for hiding this comment

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

Hey @zburke, looks good, don't have any issues with implementation.
One question though - do all UI modules need to remove X-Okapi-Token header from all requests, or will it just be ignored?

@zburke
Copy link
Member Author

zburke commented Oct 11, 2023

@BogdanDenis, that's tricky to answer. Basically, if you pass the X-Okapi-Token in a request header AND in an AT via a cookie, then I think they have to match. All the UI code I saw in tickets linked to FOLIO-3627 was implemented sorta like

if (stripes.okapi.token) {
  headers['x-okapi-token': stripes.okapi.token;
}

which should be fine, because stripes.okapi.token will be undefined after this work merges.

zburke added 16 commits October 20, 2023 10:04
An API may respond with a 403 because an AT is missing (which should
invoke RTR), because an RT is missing (which should generate an error
and end the session), or because a valid AT is inadequate to perform the
requested action. Previously, all three were handled the same way. Now,
only an actual RTR-failure will result in sending an `RTR_ERROR` event.

In other cases, no event is dispatched and the response is returned
as-is.
`self.addEventListener` should return void rather than returning a
Promise.
* correctly set scope to `/` in case the application entrypoint is a
  bookmarked URL
* tests are nice
@sonarcloud
Copy link

sonarcloud bot commented Oct 31, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

73.4% 73.4% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@zburke zburke merged commit 27d2948 into master Oct 31, 2023
4 of 5 checks passed
@zburke zburke deleted the FOLIO-3627-sw branch October 31, 2023 02:50
zburke added a commit that referenced this pull request Nov 6, 2023
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)
zburke added a commit that referenced this pull request Nov 10, 2023
zburke added a commit that referenced this pull request Nov 10, 2023
Revert all PRs related to STCOR-571, which implemented RTR
in a service worker. This includes the following:

* Revert "STCOR-759 return non-okapi request fetches as-is (#1369)"
  This reverts commit a323456.
* Revert "STCOR-759 read okapi config from micro-stripes-config (#1366)"
  This reverts commit 6c6a85e.
* Revert "STCOR-756 correctly evaluate token lifespan (#1363)"
  This reverts commit 607dc5c.
* Revert "STCOR-574 rotate tokens after 80% of their TTL (#1361)"
  This reverts commit dd71819.
* Revert "STCOR-671 handle access-control via cookies (#1346)"
  This reverts commit 27d2948.

Also, add a dummy service-worker.js file to appease stripes-webpack.
This removes the need to do extra clean-up work in that repository.
Dmitriy-Litvinenko pushed a commit that referenced this pull request Nov 23, 2023
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
Dmitriy-Litvinenko pushed a commit that referenced this pull request Nov 23, 2023
Revert all PRs related to STCOR-571, which implemented RTR
in a service worker. This includes the following:

* Revert "STCOR-759 return non-okapi request fetches as-is (#1369)"
  This reverts commit a323456.
* Revert "STCOR-759 read okapi config from micro-stripes-config (#1366)"
  This reverts commit 6c6a85e.
* Revert "STCOR-756 correctly evaluate token lifespan (#1363)"
  This reverts commit 607dc5c.
* Revert "STCOR-574 rotate tokens after 80% of their TTL (#1361)"
  This reverts commit dd71819.
* Revert "STCOR-671 handle access-control via cookies (#1346)"
  This reverts commit 27d2948.

Also, add a dummy service-worker.js file to appease stripes-webpack.
This removes the need to do extra clean-up work in that repository.
Dmitriy-Litvinenko pushed a commit that referenced this pull request Nov 23, 2023
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
Dmitriy-Litvinenko pushed a commit that referenced this pull request Nov 23, 2023
Revert all PRs related to STCOR-571, which implemented RTR
in a service worker. This includes the following:

* Revert "STCOR-759 return non-okapi request fetches as-is (#1369)"
  This reverts commit a323456.
* Revert "STCOR-759 read okapi config from micro-stripes-config (#1366)"
  This reverts commit 6c6a85e.
* Revert "STCOR-756 correctly evaluate token lifespan (#1363)"
  This reverts commit 607dc5c.
* Revert "STCOR-574 rotate tokens after 80% of their TTL (#1361)"
  This reverts commit dd71819.
* Revert "STCOR-671 handle access-control via cookies (#1346)"
  This reverts commit 27d2948.

Also, add a dummy service-worker.js file to appease stripes-webpack.
This removes the need to do extra clean-up work in that repository.
zburke added a commit that referenced this pull request Jan 10, 2024
* bump minor to 10.1 for new development (#1350)

* STCOR-747: Provide optional tenant argument to `useOkapiKy` hook (#1348)

* STCOR-749: Allow to import validateUser function from @folio/stripes/core (#1351)

* STCOR-747: Convert optional arg to object for useOkapiKy hook (#1352)

* CVE-2023-45133 require @babel/traverse >= 7.23.2 (#1353)

`@babel/traverse` < 7.23.2 is vulnerable to CVE-2023-45133.

* [STCOR-752]: Ensure <AppIcon> is not cut off (#1355)

* Revert "[STCOR-752]: Ensure <AppIcon> is not cut off (#1355)" (#1357)

This reverts commit a17be4c.

`<AppIcon>` may be used like `<AppIcon icon="foo">Title of an instance</AppIcon>` in SearchAndSort results lists, and this sets the width incorrectly.

* [STCOR-752]: Ensure <AppIcon> is not cut off (for real this time) (#1358)

* [STCOR-752]: Ensure <AppIcon> is not cut off

* Only apply min-width to appIcon classes

* Add changelog

* CVE-2023-46234 require browserify-sign >= 4.2.2 (#1359)

`browserify-sign` < 4.2.2 is vulnerable to CVE-2023-46234.

* Lokalise: updates

* STCOR-671 handle access-control via cookies (#1346)

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

* STCOR-574 rotate tokens after 80% of their TTL (#1361)

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

* STCOR-756 correctly evaluate token lifespan (#1363)

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

* STCOR-759 read okapi config from micro-stripes-config (#1366)

A service worker's global state is reset after each sleep/wake cycle,
meaning the `okapiUrl` and `okapiTenant` values so lovingly sent to the
service worker during registration are likely to be promptly forgotten
as soon as the browser is idle for a few minutes and decides it would be
good to clean up inactive processes.

Here, those values are directly imported from a virtual module created
at build-time by stripes-webpack, which forwards the values from the
stripes-config object (most likely, the `stripes.config.js` file) and
allows them to be compiled directly into the generated
`service-worker.js` asset.

An alternative approach would be to pass in those values as URL
parameters when the service worker is registered.

h/t @mkuklis and @JohnC-80 who did the heavy lifting here, both in
thinking through the potential solutions and actually figuring out how
to implement this in our highly customized build process.

* Requires folio-org/stripes-webpack/pull/132

Refs STCOR-759

---------

Co-authored-by: Michal Kuklis <[email protected]>

* STCOR-761 allow console to be preserved on logout (#1367)

Normally the console is cleared on logout. This is good for security but
bad for debugging in the case where a user may be automatically logged
out due to RTR.

Refs STCOR-761

* STCOR-759 return non-okapi request fetches as-is (#1369)

Instead of catching errors from non-Okapi requests and converting them
to rejected promises, just `return fetch()` straight up, whatever that
response contains.

h/t @MikeTaylor for pointing me in this direction, and reading minified
code to try to suss the problem.

Refs STCOR-759, UILDP-129

* Revert RTR (#1371)

Revert all PRs related to STCOR-571, which implemented RTR
in a service worker. This includes the following:

* Revert "STCOR-759 return non-okapi request fetches as-is (#1369)"
  This reverts commit a323456.
* Revert "STCOR-759 read okapi config from micro-stripes-config (#1366)"
  This reverts commit 6c6a85e.
* Revert "STCOR-756 correctly evaluate token lifespan (#1363)"
  This reverts commit 607dc5c.
* Revert "STCOR-574 rotate tokens after 80% of their TTL (#1361)"
  This reverts commit dd71819.
* Revert "STCOR-671 handle access-control via cookies (#1346)"
  This reverts commit 27d2948.

Also, add a dummy service-worker.js file to appease stripes-webpack.
This removes the need to do extra clean-up work in that repository.

* Revert RTR: auto-unregister zombie service workers (#1372)

* auto-unregister zombie service workers
* lint

* Lokalise: updates

* STCOR-671 handle access control via cookies

Refs STCOR-671

* add request handling to isOkapiRequest, replace localstorage usage with utility, rtr-specific error.

* clean up comments and resource handling

* comments are in sync with function arguments
* consistently handle non-string arguments passed to fetch

* implement fake XHR class

* add implementation of XHR override, move functions to token-util

* tests, comments

* rtr() tests

* tests and comments; rtr() refactor handles inner exceptions

This is mostly tests and comments. The lone exception is in
`Fetch::ffetch()` which was significantly refactored to handle RTR
exceptions at the innermost level where they occur, instead of wrapping
the whole block in a try/catch and handling them at the outer level,
which was not reliable: exceptions thrown from `rtr()` were escaping. It
seems like some aspect of the stack was synchronous, allowing that
exception to escape instead of being translated into a rejection.

* put RTR behind stripes-config.useSecureTokens

* add FFetch tests

* add FFetch tests

* FFetch tests at 100% coverage

* STCOR-762 disable login when cookies are disabled

* linty mclintface

* when config.useSecureTokens is absent, restore token access

* restore token-based endpoints since BTOG isn't configured with useSecureTokens=true

* replace polling; replace 403+inspection with 400 detection

* add test for FXHR

* restore tokenExpiration checking

Those 403s hitting the console were just too much to handle for me, so
we're restoring the `isValidAT` and `isValidRT` code to intercept
requests that we know will fail before they fail. Since a browser may
fire several fetches at once, we weren't just seeing one request failure
when the AT expired, we saw several, making a mess of the console.

This logic is a little more intricate, but the console is a lot more
clean.

* mock localforage to always return valid expiration data

* FFetch coverage

* lint

* backward compatibility: include x-okapi-token header when token is present

* apply same settings to withOkapiKy as useOkapiKy

* include mode and credentials flags in all okapi calls

* trivial commit

* handle cross-tab communication during rotation with localStorage

* log correctly

* don't worry about checking to see if context.rtrPromise exists

* STCOR-767 Fix duplicated FOLIO in document title in some cases (#1377)

* STCOR-767 Fix duplicated FOLIO in document title in some cases

* STCOR-767 Fix code smell

* CVE-2023-48631 require @adobe/css-tools >= 4.3.2 (#1379)

@adobe/css-tools < 4.3.2 is vulnerable to CVE-2023-48631.

* correctly handle single-window and multi-window rotation

The `storage` event is specifically designed for cross-window
communication, in fact, _exclusively_ designed for cross-window
communication. Thus, in order to handle both scenarios (rotation is
pending in the current window or in a separate window), we need to set
up listeners for both types of events.

* ignore stale rotation requests

Instead of a boolean, store a timestamp for the value of the
pending-rtr-request flag. This allows us to detect stale flags (i.e. a
flag indicating that a rotation request is active when in fact none is)
and clear them. Without this check, the `isRotating` value could get
stuck in the cache, causing all future rotation requests to wait for a
non-existent request to complete, which of course wouldn't end well.

* lint. it's always lint.

* STCOR-768 Refactor away from color() function. (#1380)

* refactor away from color() function in Toast.css

* log changes

* STCOR-770: Export getEventHandler to be able to create events in other modules. (#1383)

* backward compat with http-header token authz

---------

Co-authored-by: Mariia Aloshyna <[email protected]>
Co-authored-by: Oleksandr Hladchenko <[email protected]>
Co-authored-by: Noah Overcash <[email protected]>
Co-authored-by: FOLIO Translations Bot <[email protected]>
Co-authored-by: Michal Kuklis <[email protected]>
Co-authored-by: Peter Murray <[email protected]>
Co-authored-by: John Coburn <[email protected]>
Co-authored-by: John Malconian <[email protected]>
Co-authored-by: Denys Bohdan <[email protected]>
Co-authored-by: Dmytro-Melnyshyn <[email protected]>
zburke added a commit that referenced this pull request Jan 16, 2024
Move auth tokens into HTTP-only cookies and implement refresh token
rotation (STCOR-671) by overriding `global.fetch` and
`global.XMLHttpRequest`, disabling login when cookies are disabled
(STCOR-762). This functionality is implemented behind an opt-in
feature-flag (STCOR-763). The core RTR logic here is largely the same as
it was in PR #1346 😬 , though with several important differences:

1. No buggy service-worker
2. Handle `fetch` and `XMLHttpRequest`
3. Disable login if cookies are disabled
4. Everything is opt-in 😌

Not _everything_ in PR #1346 was awful, despite it being reverted in
#1371 😬 . The fundamental difference here is that the global `fetch`
and `XMLHttpRequest` functions have been replaced 🤢 by new
implementations that handle RTR instead of intercepting such requests
via the service-worker proxy. This is not lovely. It is not elegant. It
isn't pretty in any way, but it is extremely simple and effective.
Certainly, we want to migrate away from it, but given the options we
thought it was best choice in the short-term. The options:

1. Centralized fix within stripes-core by fixing the service worker.
   Let's be honest, I didn't get it right in #1346 and then couldn't get it
   right in #1361 or #1363 or #1366 or #1369. Why would anybody possibly
   believe that I could get it right now?
2. Decentralized fix: handle this in each UI-* repository by exporting a
   new function from stripes and refactoring each UI repo to leverage the
   new code. Probably not a big refactor, but not a small effort.
3. Centralized fix within stripes-core by overwriting `global.fetch`.
   Gross, but effective, and long term we can make this a decentralized
   approach by exporting our new `fetch` function, doing the refactor
   described in 2 (above), and removing the global-overwrite once all the
   refactoring is done.

In summary:

* Replaces #1340. It was gross and I really don't want to talk about it.
  Let us never mention it again.
* Replaces #1346. It was a terrible, horrible, no good, very bad PR.
  Alexander hated that PR more than lima beans.

Additional requirements:

* Requires folio-org/stripes-connect#223
* Requires folio-org/stripes-smart-components#1397
* Requires folio-org/stripes-webpack#125

Refs STCOR-671, FOLIO-3627
zburke added a commit that referenced this pull request Jan 26, 2024
Move auth tokens into HTTP-only cookies and implement refresh token
rotation (STCOR-671) by overriding `global.fetch` and
`global.XMLHttpRequest`, disabling login when cookies are disabled
(STCOR-762). This functionality is implemented behind an opt-in
feature-flag (STCOR-763). The core RTR logic here is largely the same as
it was in PR #1346 😬 , though with several important differences:

1. No buggy service-worker
2. Handle `fetch` and `XMLHttpRequest`
3. Disable login if cookies are disabled
4. Everything is opt-in 😌

Not _everything_ in PR #1346 was awful, despite it being reverted in
\#1371 😬 . The fundamental difference here is that the global `fetch`
and `XMLHttpRequest` functions have been replaced 🤢 by new
implementations that handle RTR instead of intercepting such requests
via the service-worker proxy. This is not lovely. It is not elegant. It
isn't pretty in any way, but it is extremely simple and effective.
Certainly, we want to migrate away from it, but given the options we
thought it was best choice in the short-term. The options:

1. Centralized fix within stripes-core by fixing the service worker.
   Let's be honest, I didn't get it right in #1346 and then couldn't get it
   right in #1361 or #1363 or #1366 or #1369. Why would anybody possibly
   believe that I could get it right now?
2. Decentralized fix: handle this in each UI-* repository by exporting a
   new function from stripes and refactoring each UI repo to leverage the
   new code. Probably not a big refactor, but not a small effort.
3. Centralized fix within stripes-core by overwriting `global.fetch`.
   Gross, but effective, and long term we can make this a decentralized
   approach by exporting our new `fetch` function, doing the refactor
   described in 2 (above), and removing the global-overwrite once all the
   refactoring is done.

In summary:

* Replaces #1340. It was gross and I really don't want to talk about it.
  Let us never mention it again.
* Replaces #1346. It was a terrible, horrible, no good, very bad PR.
  Alexander hated that PR more than lima beans.

Additional requirements:

* Requires folio-org/stripes-connect#223
* Requires folio-org/stripes-smart-components#1397
* Requires folio-org/stripes-webpack#125

Refs STCOR-671, FOLIO-3627

(cherry picked from commit 0361353)
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.

5 participants