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

rebase-cleanup: restore logout AND ITS TESTS #1478

Closed
wants to merge 41 commits into from

Conversation

zburke
Copy link
Member

@zburke zburke commented May 23, 2024

The previous commit re-enabled logout by correctly passing the x-okapi-tenant header in the /authn/logout request. It turns out that if you want read the tenant from the store in a test, you have to mock the store in your test. WHO KNEW???

zburke and others added 30 commits May 6, 2024 16:50
…#1415)

To support Single Sign-On (SSO) authorization in consortium mode,
it's necessary to explicitly pass the tenant in the request header
to load data.

(cherry picked from commit d8db8b7)
There's a lot going on here, but fundamentally the changes are
split into two main categories:
* route authentication requests to/from keycloak
* handle discovery dynamically via an API request AFTER authentication
  instead of reading a static module list from `stripes.config.js`
* Update URL redirect to allow back button support

* Clean up unused and duplicate code

* lint

* Add URL param to indicate Consortium

---------

Co-authored-by: Zak Burke <zburke@ebsco.com>
* handle legacy discovery via Okapi APIs
* handle legacy logout via internal redirect to `/`
* handle legacy version display on `/settings/about`

There is not really as much work here as it appears. All the new
components were split out of `About` in order to allow sub-sections to
be reused with both application-based and module-based discovery
information. Likewise, `loginServices.js` and `discoveryServices.js`
were modestly refactored to handled both APIs.

And there are Jest/RTL tests to replace the BTOG test that could not be
easily updated to handle the new APIs since its `stripes-config` stub is
part of `@folio/stripes-cli` instead of being declared locally.

Refs STCOR-773
…1399)

When the `users-keycloak` interface is available, use the endpoints it
provides in place of the legacy endpoints.

Refs STCOR-795, UIU-3031
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)
* STCOR-803 Add config option for logout mode

* Lint fix
…component. (#1411) (#1422)

* move async localforage.clear to afterEach

* remove BTOG sso login tests, add sso login jest tests

* Update CHANGELOG.md

* move describe block comments to it blocks.. remove describe blocks

(cherry picked from commit 79c76c4)

Co-authored-by: John Coburn <jcoburn@ebsco.com>
If the response from `/auth/token?code=...` is OK, parse it to
immediately store the AT/RT expiration values (or use a near-future date
if values are not provided). Stripes must expect the RT to be valid for
any future API call to succeed; otherwise, it will assume the RT has
expired resulting in a race condition with the RTR handler dispatching
an RTR_ERROR_EVENT but discovery succeeding and re-rendering.

This would result in the API call to `.../_self` issued by
`requestUserWithPerms` being swallowed and `stripes.user` being
populated with an empty object, causing all kinds of problems down the
line for any code that leveraged it.

Refs STCOR-811
)

Include the `X-Okapi-Tenant` header in `/authn/logout` requests, and
clear `localStorage` settings as well. `X-Okapi-Tenant` is required for
requests to be properly routed; if this request failed, the browser
session would be destroyed by the keycloak session would remain active,
a security risk.

Clearing `localStorage.tenant` is necessary to prevent an incorrect
value from being cached and inadvertently reused on subsequent login
requests.

Refs STCOR-812
The shape of the permissions object differs between responses from calls
to `login` and calls to `_self`. This is not awesome. We didn't notice
this glitch prior to implementing keycloak because when resuming an
existing session (i.e. when calling `_self`), permissions are set as the
union of permissions in storage (i.e. stored by a call to `login`) and
those from the call to `_self`. We just never noticed that the latter
was always empty.

With keycloak handling authentication, however, the _only_ permissions
we ever receive are in the response from `_self`, so we noticed this
immediately.

Refs STCOR-813
…r modules. (#1383) (#1424)

(cherry picked from commit 190d87e)

Co-authored-by: Dmytro-Melnyshyn <77053927+Dmytro-Melnyshyn@users.noreply.github.com>
Remove references to the `stripes.config.js::config` values
`tenantManagerUrl` and `applicationManagerUrl`. These were present in
early drafts of this work but have since been deprecated and therefore
must be removed from code as well.

Refs STCOR-810
…page (#1426)

* STCOR-803 Add config option for logout mode

* Lint fix

* Revert "STCOR-803 Add config option for logout mode"

This reverts commit b9d2604.

* STCOR-803 Simplify logout workflow to bypass keycloak confirmation page.

* STCOR-803 PR comments

* Revert "STCOR-803 PR comments"

This reverts commit 037b6a2.

* STCOR-803 Restore console log
This reverts commit 13b9dc4.

The conditional here checked `okapi.tenantOptions`; it must check `config.tenantOptions`.
Remove references to the `stripes.config.js::config` values
`tenantManagerUrl` and `applicationManagerUrl`. These were present in
early drafts of the new discovery work but have since been deprecated
and therefore must be removed from code as well.

Replaces #1418, which did this work incorrectly (referring to
`okapi.[...]` instead of `config.[...]`).

Refs STCOR-810
* STCOR-816 only fetch /saml/check when login-saml is present

When restoring an existing session, after discovery, do not fetch from
`/saml/check` unless the `login-saml` interface (indicating SSO/SAML is
available). The 404 clutters the log.

Refs STCOR-816

* Missing semicolon

---------

Co-authored-by: Ryan Berger <rberger@ebsco.com>
* STCOR-776 show "Keep working?" prompt when session ages

The main feature here is to track the RT's TTL and use it to show a
"Your session is about to expire; keep working?" prompt so the user can
fire off RTR in order to keep the session alive. Knock-on effects
include tracking such events across multiple windows, so logging out in
one window immediately logs you out in others, and so a successful RTR
event in one window closes any open "Still working?" prompts in others.
It sounds big, and it looks big, but once you wrap your head around it
isn't so bad.

A couple things to note:

The `loginServices::eventManager()` function provides two event-related
function, `listen` and `emit` that handle single-window events (i.e.
`window.dispatchEvent()`/`window.addEventListener()`) and multi-window
events (i.e. BroadcastChannel.post() and
BroadcastChannel.addEventListener()`). This simplifies the API for
sending and receiving events. [Documentation for
BroadcastChannel](https://developer.mozilla.org/en-US/docs/Web/API/Broadcast_Channel_API)
is pretty good and worth a look. The thing to keep in mind with
single-window events is that they are sent and received in the same
window, whereas BroadcastChannel events are sent in one window but
received in all others.

`<SessionEventContainer>` is instantiated near the top of
`<RootWithIntl>`, just like `<OverlayContainer>`. It sets itself up as
the listener for all session-related events, including RTR-success
(which will close an open "Keep working?" prompt), RTR-failure (which
will cause logout), logout (i.e. a logout event from another window),
and idle session (i.e. the RT is about to expire, which will show the
"Keep working?" modal). Other session-related event handlers have been
removed in favor of consolidating them all in this component.

The `<KeepWorkingModal>` calls RTR if the user closes it, causing the
session to be extended. By default it displays 60 seconds before the
session expires but this can be changed by adjusting
`stripes.config.js::config.idleSessionWarningSeconds`. If the timer
counts down to 0, it emits a session-expired event, causing logout.

Refs STCOR-776

* test repair

* tests are nice

* test infrastructure cleanup

* import from @folio/jest-config-stripes/testing-library to get current
  versions
* rename `dismissible` to `_dismissible` on destructure to prevent
  complaints about it not being used. lint and jest are both happy!
* fix prop-types in test props, which is probably a losing battle

* tests for SessionEventContainer, KeepWorkingModal

Externalize SessionEventContainer event handlers and call them with
1,000 arguments, but at least DI makes them testable. I don't love this,
but jest really couldn't grok having an event-handler trigger a
state-change. If the render and event were both triggered within a
single `act()` the re-render got swallowed. If the original render was
outside an `act()` then jest complained state was changing outside the
`act()` function. Whaddayawant?

* codesmell cleanup recommended by sonar

* tyop

* Lint fixes

* Remove commented-out code

---------

Co-authored-by: Ryan Berger <rberger@ebsco.com>
…" (#1433)

Numerous bugs indicate this work was not ready for primetime:
* "Keep working?" prompt may not be dismissible
* missing translations in the "Keep working?" prompt
* console errors indicate attempts to post messages to closed BroadcastChannels

Reverts #1431

This reverts commit 6aabfc5.
…LFetch` (#1438)

* STCOR-821 Add `idName` and `limit` as passable parameters to `useChunkedCQLFetch`

* Update CHANGELOG
…in the path or query arguments (#1445)

* STCOR-820 Add support for optionaly passing token by URL param

* Remove console.log

* Update CHANGELOG.md
ryandberger and others added 11 commits May 6, 2024 16:56
…ssible API (#1452)

* STCOR-789: add /authn/token to always-permissible list. Refs STCOR-789

Include `/authn/token` on the list of always-permissible API in order to
allow OTP-for-cookie exchange on return from authentication. Without
this allowance in place, stripes will get stuck in a loop bouncing
between the authn-server (which believes, correctly, that the user has
authenticated) and stripes (which believes, wrongly, that the user has
not authenticated because its "valid AT?" check fails). The AT won't be
valid until after we get to exchange the OTP for an AT by visiting
`/authn/token`.

---------

Co-authored-by: Ryan Berger <rberger@ebsco.com>
Co-authored-by: Zak Burke <zburke@ebsco.com>
* switch to classname-only selectors for login screen.

* remove element selectors from reset, create password and forgot username UI's

* log changes

* correct nesting in CreateResetPassword.css
Provide user-tenant-permissions functionality, both centralizing this
functionality and insulating other applications from needing to depend
on the permissions interface.

* `useUserTenantPermissions` provides permissions for the currently
  authenticated user in a single tenant
* `getUserTenantsPermissions` provides permissions for the currently
  authenticated user across an array of tenants

Refs STCOR-830
…#1415)

To support Single Sign-On (SSO) authorization in consortium mode,
it's necessary to explicitly pass the tenant in the request header
to load data.

(cherry picked from commit d8db8b7)
* Navigate to base URL if using Eureka since backend doesn't recognize /login
…rg/stripes-core into keycloak-ramsons"

This reverts commit a2406bc, reversing
changes made to ff01ae9.
The previous commit re-enabled logout by correctly passing the
`x-okapi-tenant` header in the `/authn/logout` request. It turns out
that if you want read the tenant from the store in a test, you have to
mock the store in your test. WHO KNEW???
Copy link

Jest Unit Test Statistics

227 tests  ±0   223 ✔️ ±0   57s ⏱️ ±0s
  44 suites ±0       0 💤 ±0 
    1 files   ±0       4 ±0 

For more details on these failures, see this check.

Results for commit 0d745ad. ± Comparison against base commit 1faf77b.

Copy link

github-actions bot commented May 23, 2024

BigTest Unit Test Statistics

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ±0 

Results for commit 0d745ad. ± Comparison against base commit 1faf77b.

♻️ This comment has been updated with latest results.

@zburke
Copy link
Member Author

zburke commented May 23, 2024

branches got wacky due to an accidental merge; there should only be one commit on this branch :/ Gonna try re-opening it against current tip of keycloak-ramsons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants