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-918: *BREAKING* remove token-based authentication code #1592

Merged
merged 11 commits into from
Feb 14, 2025
Merged

Conversation

UladzislauKutarkin
Copy link
Contributor

@UladzislauKutarkin UladzislauKutarkin commented Feb 12, 2025

https://folio-org.atlassian.net/browse/STCOR-918
https://folio-org.atlassian.net/browse/STCOR-922

Here we removed token-based authentication endpoints. Also, removed useSecureTokens logic part.

Copy link

github-actions bot commented Feb 12, 2025

Bigtest Unit Test Results

189 tests  ±0   184 ✅ ±0   6s ⏱️ ±0s
  1 suites ±0     5 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit c4990bc. ± Comparison against base commit fa2ffcf.

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_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the header with an appropriate text content
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the header with an appropriate text content
      equal to check email label in english translation
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status 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_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status 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 comment has been updated with latest results.

Copy link

github-actions bot commented Feb 12, 2025

Jest Unit Test Results

  1 files  ±0   62 suites  ±0   1m 36s ⏱️ +2s
364 tests  - 1  364 ✅  - 1  0 💤 ±0  0 ❌ ±0 
367 runs   - 1  367 ✅  - 1  0 💤 ±0  0 ❌ ±0 

Results for commit c4990bc. ± Comparison against base commit fa2ffcf.

This pull request removes 1 test.
SessionEventContainer Renders nothing if useSecureTokens is false ‑ SessionEventContainer Renders nothing if useSecureTokens is false

♻️ This comment has been updated with latest results.

Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

For reasons I don't know remember, I filed STCOR-918 and STCOR-922 as separate tickets, but that seems silly now. I think it would make sense to handle them together since they all amount to the same change you made here in requestLogin. Your call if you want to tack that work onto this PR.

This ticket description also mentions purging the JWT-related code. It doesn't matter to me which PR that happens under, but if you don't do it here, please move that requirement to STCOR-922 and make sure it happens there :)

@UladzislauKutarkin
Copy link
Contributor Author

UladzislauKutarkin commented Feb 13, 2025

For reasons I don't know remember, I filed STCOR-918 and STCOR-922 as separate tickets, but that seems silly now. I think it would make sense to handle them together since they all amount to the same change you made here in requestLogin. Your call if you want to tack that work onto this PR.

This ticket description also mentions purging the JWT-related code. It doesn't matter to me which PR that happens under, but if you don't do it here, please move that requirement to STCOR-922 and make sure it happens there :)

@zburke you mean delete such conditions

  const tenant = config.useSecureTokens
    ? params?.tenantId
    : parseJWT(token)?.tenant;

  return tenant || store.getState()?.okapi?.tenant;
}; ?```

@zburke
Copy link
Member

zburke commented Feb 13, 2025

Yes, @UladzislauKutarkin . Conditionals like

const tenant = config.useSecureTokens ? params?.tenantId : parseJWT(token)?.tenant;
config.useSecureTokens && <SessionEventContainer />

should be converted to

const tenant = params?.tenantId;
<SessionEventContainer />

i.e. just leaving the side of the conditional that evaluates to true. Code that deals with tokens can also be removed because the UI no longer has access to the token. Responses from .../login return the token in the body so Stripes would keep it in local storage; reponses from .../login-with-expiry return the token in an HTTPOnly cookie which (of course) Stripes cannot access.

@zburke zburke merged commit f768417 into master Feb 14, 2025
16 checks passed
@zburke zburke deleted the STCOR-918 branch February 14, 2025 16:00
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.

3 participants