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-796 replace x-okapi-token credentials with RTR and cookies #1410

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

zburke
Copy link
Member

@zburke zburke commented 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).

Okapi and Keycloak do not handle the same situations in the same ways. Changes from the original implementation in PR #1376:

  • When a token is missing:
    • Okapi sends a 400 text/plain response
    • Keycloak sends a 401 application/json response
  • Keycloak authentication includes the extra step of exchanging the OTP
    for the AT/RT and that request needs the credentials and mode
    options
  • Some loginServices functions now retrieve the host the access from
    the stripes-config import instead of a function argument

Refs STCOR-796, STCOR-671

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)
Okapi and Keycloak do not handle the same situations in the same ways.
* When a token is missing:
  * Okapi sends a 400 text/plain response
  * Keycloak sends a 401 application/json response
* Keycloak authentication includes the extra step of exchanging the OTP
  for the AT/RT and that request needs the `credentials` and `mode`
  options
* Some `loginServices` functions now retrieve the host the access from
  the `stripes-config` import instead of a function argument

Refs STCOR-796
@zburke zburke marked this pull request as draft January 26, 2024 22:15
Copy link

github-actions bot commented Jan 26, 2024

Jest Unit Test Statistics

144 tests  +49   144 ✔️ +49   36s ⏱️ +5s
  27 suites +  3       0 💤 ±  0 
    1 files   ±  0       0 ±  0 

Results for commit 165adcf. ± Comparison against base commit c75f53b.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 26, 2024

BigTest Unit Test Statistics

    1 files  ±0      1 suites  ±0   10s ⏱️ ±0s
270 tests ±0  264 ✔️  - 1  6 💤 +1  0 ±0 
273 runs  ±0  267 ✔️  - 1  6 💤 +1  0 ±0 

Results for commit 165adcf. ± Comparison against base commit c75f53b.

♻️ This comment has been updated with latest results.

@zburke zburke marked this pull request as ready for review January 29, 2024 13:35
Copy link
Member

@ryandberger ryandberger left a comment

Choose a reason for hiding this comment

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

Lot to take in here, but approving knowing that most of this is already in master branch and the subtle changes have been tested against keycloak.

Like requests to `/bl-users/login-with-expiry`, requests to
`/authn/token` must be permitted without a valid AT since it is, itself,
part of the request pipeline to receive an AT.

`/authn/token` is the endpoint provided by `mod-login-keycloak` to
exchange the OTP provided by keycloak for the AT/RT cookies provided by
FOLIO.
Copy link

sonarcloud bot commented Jan 31, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

75.1% Coverage on New Code (required ≥ 80%)
14.5% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

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

@zburke zburke merged commit 2d21aa0 into keycloak Jan 31, 2024
5 of 6 checks passed
@zburke zburke deleted the STCOR-796 branch January 31, 2024 18:07
zburke added a commit that referenced this pull request Mar 28, 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).

Okapi and Keycloak do not handle the same situations in the same ways.
Changes from the original implementation in PR #1376:

* When a token is missing:
  * Okapi sends a 400 `text/plain` response
  * Keycloak sends a 401 `application/json` response
* Keycloak authentication includes the extra step of exchanging the OTP
  for the AT/RT and that request needs the `credentials` and `mode`
  options
* Some `loginServices` functions now retrieve the host the access from
  the `stripes-config` import instead of a function argument
* always permit `/authn/token` requests to go through

Refs STCOR-796, STCOR-671

(cherry picked from commit 0361353)
zburke added a commit that referenced this pull request Mar 28, 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).

Okapi and Keycloak do not handle the same situations in the same ways.
Changes from the original implementation in PR #1376:

* When a token is missing:
  * Okapi sends a 400 `text/plain` response
  * Keycloak sends a 401 `application/json` response
* Keycloak authentication includes the extra step of exchanging the OTP
  for the AT/RT and that request needs the `credentials` and `mode`
  options
* Some `loginServices` functions now retrieve the host the access from
  the `stripes-config` import instead of a function argument
* always permit `/authn/token` requests to go through

Refs STCOR-796, STCOR-671

(cherry picked from commit 0361353)
aidynoJ pushed a commit that referenced this pull request May 6, 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).

Okapi and Keycloak do not handle the same situations in the same ways.
Changes from the original implementation in PR #1376:

* When a token is missing:
  * Okapi sends a 400 `text/plain` response
  * Keycloak sends a 401 `application/json` response
* Keycloak authentication includes the extra step of exchanging the OTP
  for the AT/RT and that request needs the `credentials` and `mode`
  options
* Some `loginServices` functions now retrieve the host the access from
  the `stripes-config` import instead of a function argument
* always permit `/authn/token` requests to go through

Refs STCOR-796, STCOR-671

(cherry picked from commit 0361353)
ryandberger pushed a commit that referenced this pull request May 15, 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).

Okapi and Keycloak do not handle the same situations in the same ways.
Changes from the original implementation in PR #1376:

* When a token is missing:
  * Okapi sends a 400 `text/plain` response
  * Keycloak sends a 401 `application/json` response
* Keycloak authentication includes the extra step of exchanging the OTP
  for the AT/RT and that request needs the `credentials` and `mode`
  options
* Some `loginServices` functions now retrieve the host the access from
  the `stripes-config` import instead of a function argument
* always permit `/authn/token` requests to go through

Refs STCOR-796, STCOR-671

(cherry picked from commit 0361353)
zburke added a commit that referenced this pull request Jun 11, 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).

Okapi and Keycloak do not handle the same situations in the same ways.
Changes from the original implementation in PR #1376:

* When a token is missing:
  * Okapi sends a 400 `text/plain` response
  * Keycloak sends a 401 `application/json` response
* Keycloak authentication includes the extra step of exchanging the OTP
  for the AT/RT and that request needs the `credentials` and `mode`
  options
* Some `loginServices` functions now retrieve the host the access from
  the `stripes-config` import instead of a function argument
* always permit `/authn/token` requests to go through

Refs STCOR-796, STCOR-671

(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
3 participants